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

I don't know about any of the high level design stuff, but noticed a few things reading through the diff that I was getting round to writing up.

I like that you're using cmdline here, but why are you then getting in the business of quoting the arguments again? If there's a reason not to pass subprocess.call a list, that should be documented. Also, you're using shell=True always, as that's creating an intermediate process you should say why you need it.

Using percent-followed-by-letter is slightly misleading for the templating, as this is a little more magical than straight interpolation. Also odd that I could use %TMP% in my line and be suprised by just %T expanding. I'm guessing there are good historical reasons though.

What happens with non-ascii filenames? I see no handling, so will you just be throwing UnicodeError somewhere?

+def _resolve_using_merge_tool(tool_name, conflicts):

I'm not mad about this function or the --resolve-using interface, but don't have any great alternatives.

+ If you need to include options in your external merge tool's
+ command-line, insert '--' before the command-line to prevent bzr from
+ processing them as options to the ``bzr mergetools`` command:

This seems silly. Why is:

+ bzr mergetools --add -- kdiff3 %b %t %o -o %r

Preferable to `bzr mergetools --add "kdiff3 %b %t %o -o %r"`?

+ def get_name(self):
+ name = os.path.basename(self.get_executable())
+ if sys.platform == 'win32' and name.lower().endswith('.exe'):
+ name = name[:-4]

What's special about .exe over .bat or .com? Should this be using os.path.splitext and the PATHEXT envvar?

+ def is_available(self):
+ executable = self.get_executable()
+ return os.path.exists(executable) or _find_executable(executable)

Don't like this look-before-you-leap style. Would prefer you catch the right OSError from subprocess.call to detect and warn about missing executables.

+# Authors: Robert Collins <email address hidden>

You conned Robert into writing your tests for you? :)