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

>>>>> Gordon Tyler <email address hidden> writes:

    > Thanks for the review, Vincent. :)
    > On Thu, November 11, 2010 11:32 am, Vincent Ladeuil wrote:
    >> This is certainly related to the size of the patch so splitting it into
    >> several parts will help, but since your have already invested quite a
    >> bunch of time on it, I suspect you won't be rejoiced by such a proposal
    >> :-/

    > 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.

    >> - if you want to split it anyway (who knows ;) the configuration part and
    >> the command behaviour modifications sounds like good candidates,

    > Agreed.

Great.

    >> - you use a FakeConfig for some tests which is a bit risky, especially
    >> considering that you didn't take the {option} syntax into account as
    >> suggested by poolie (and I agree this would be better than using the
    >> obscure %B %t %o options). I think that configuring such tools is rarely
    >> done and will most of the time be done by copying an existing one. In this
    >> context having verbose options means you don't have to search the
    >> documentation for the meaning of the short ones. I won't block on that
    >> though as I'd like this syntax to be supported by bzrlib.config and that's
    >> outside the scope of your proposal.

    > I think I wrote those tests before I discovered that TestCaseInTempDir
    > would isolate tests from changing the user's real config. I'll try
    > changing the tests to use a real config object.

Ok.

    > Regarding verbose placeholders, I'm beginning to agree with you
    > and other reviewers. So, I can do the substitution myself for now
    > and then once the config object is capable of doing it, we can
    > change mergetools to use that?

Hehe, chiken-and-egg problem :)

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.

    >> - like John, I think there is some overlapping with the ``bzr config``
    >> command and that will be a maintenance burden down the road, mergetools

    > The problem I have with using ``bzr config`` is that it requires that the
    > user understand the layout of the config file, that they must add the name
    > to one setting and then create another setting using that name which
    > contains the commandline. They need to get the quoting right and the
    > commas right, etc. This is not just some simple "name=value" setting like
    > email. Why can't we have both?

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 ?

    >> can keep the list and detect subcommands, also note that checking the
    >> external merge tool availability at this point won't fly, you are
    >> forbidding users to configure them under special setups and you still need
    >> to check their availability when you *use* them. Also, by using special

    > Checking of availability only logs a warning if it's not available. I
    > think its useful because it warns them immediately so that they're not
    > surprised later when they're trying do resolve conflicts and they have to
    > stop what they're doing to go fix the configuration.

You're right, this goes both ways...

    >> commands to add/modify/delete there you also forbid setting such tools in
    >> more specific ways in locations.conf and branch.conf (which I want to
    >> allow for *any* option in the new config scheme),

    > That's a good point. I don't really have a good solution for that.

See above :)

    >> - you said that the merge tool is called for text conflicts but I see no
    >> tests that you don't try to call it for other types of conflicts,

    > Not sure how to test that. Could you give me some pointers?

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.

    >> - bzrlib.mergetools.py use DOS line endings, please use Unix line endings
    >> (and watch for line with only spaces :-/),

    > Sorry, I didn't know that it mattered. My editor is capable of
    > reading DOS and Unix line endings, so I don't really notice
    > whether its one or the other. I'll fix it and check if it has a
    > trim spaces option (I think it does but I disabled it because
    > somebody else complained a while back about spurious changes due
    > to whitespace stripping).

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

    >> - regarding docs, most of the cmd_mergetools docstring should be part of
    >> configuration doc, that's where people are directed for questions about
    >> config options,

    > I can add it to the config docs but I don't see why it can't be in both
    > places.

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.

    >> - _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...