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

Wow, nice work and numerous reviews already, but still, this has not landed...

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 :-/

Anyway, I have some high level remarks:

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

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

- like John, I think there is some overlapping with the ``bzr config`` command and that will be a maintenance burden down the road, mergetools 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 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),

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

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

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

- _KOWN_MERGE_TOOLS really want to be a registry :)

I hope I'm sounding too negative here, I really think this proposal is valuable and I will do my best to help you land it.