Code review comment for lp:~doxxx/bzr/mergetools

On Thu, November 11, 2010 1:19 pm, Vincent Ladeuil wrote:
> > I'm not sure how I would actually go about splitting it. Branch from
> > lp:~doxxx/bzr/mergetools and then remove the extra bits from
> > lp:~doxxx/bzr/mergetools?
>
> Whatever works :) The above is ok. Then you'll have to decide whether
> you want to regularly merge from one branch to the other, use a pipeline
> or a loom. And don't forget to set the pre-requisite branch when
> submitting.

Won't the removal of extra bits in the original get merged into the second
when I try and merge new changes in the original into the second?

> All things being equal, I think implementing it directly into config may
> be require less work overall, but let's not introduce more dependencies
> there, postpone this precise point and we'll see what is available when
> you come to it (configobj provides the necessary hooks and most of the
> infrastructure, including the common errors (definition loops, undefined
> option) so no need to re-implement from scratch). I still need to submit
> my braindump on the new config scheme I envision where I've already
> describe some related stuff. I should clean it up and submit it
> tomorrow.

Okay, leaving it as is then.

> How about using option names like 'bzr.mergetool.bcompare'
> 'bzr.mergetool.kdiff3' and so on ? Then we could query the config object
> for option whose names start with 'bzr.mergetool'. Would this address
> some of your concerns ?

I already create option names like that but I didn't see a way to query
for options whose names start with a prefix, thus the separate list
option. I'll take another look at the config stuff to see how to do this.

But yeah, if I can get this working then it does become more
user-editable, in which case I would be okay with removing the --add,
--update and --remove options from the mergetools command.

Should these options go into a different section in the config file? Or
would that prevent putting them in locations.conf and branch.conf?

What about tool names which have "funny" characters in them like spaces or
other stuff? What's legal in an option name?

> isinstance(conflict, conflicts.TextConflict) to filter only text
> conflicts, bt.test_conflicts, bb.test_conflicts should contain almost
> all the tests related to conflicts otherwise.

Thanks.

> Right, my editor does to, but if we can keep a coherent line ending in
> the code base, that would be better.

Will do.

> Well, two places for the same doc mean they should be both maintained,
> that's error-prone. All other config options are described
> bzrlib/help_topics/en/configuration.txt, it makes our life easier when
> we want to point users to this place.

Okay.

> >> - _KOWN_MERGE_TOOLS really want to be a registry :)
>
> > I'll have a look at that.
>
> May be not :) If you rely on a naming rule for that...

It's just a place to list the names and command-lines of merge tools that
we already know about, so that we can try detect them on the path. Is a
registry suitable for that?