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

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?

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

Agreed.

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

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?

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

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

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

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

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

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

> - _KOWN_MERGE_TOOLS really want to be a registry :)

I'll have a look at that.