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

Thanks John and Martin for the feedback!

> 1) I would like to see 'bzr diff' handled in the same way, so there is a
> consistent approach for external tools.

I have been giving this some thought but I wasn't sure if it would be trying to do too much in one patch.

> 2) I think that the 'add' operation should allow for optionally specifying a
> name. This covers the ability to add the same tool twice with different flags,
> the ability to work with two executables that happen to have the same name,
> and the ability to choose an alias that conveys additional information (such
> as 'cygwin-sdiff' on a Windows platform).

I have thought about this as well. It complicates the storage in the config, although not to the point of being infeasible. And as you say, it's almost certainly going to be necessary in some situations. I'll see what I can do.

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

I am passing a list to subprocess.call. The commandline is given by the user and stored as a single string and I use cmdline.split to convert it to a list before calling subprocess.call. Therefore, when performing filename substitution, I have to quote the filenames so that cmdline.split will split it correctly. However, I probably could do the cmdline.split first and then do filename substitution in each argument without worrying about quoting. I'll try that.

I don't recall why I was using shell=True. It must have seemed like a good idea at the time. I'll remove 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.

It's the same syntax as the old external merge tool support as well as the extmerge plugin. Python string interpolation syntax may be too arcane/verbose for users. Although, I suppose they are probably going to be developers. Still, "kdiff3 %(base)s %(this)s %(other)s -o %(result)s" is kinda icky.

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

Good point, I should be using unicode literals.

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

Before this, I had a separate command for invoking a merge tool on conflicts but a quick survey on the bzr list indicated this wasn't what people were expecting, and they suggested something like the above.

> + 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"`?

Because the quoting, if necessary, could get extremely funky. I figured if they could just write exactly like they normally would, then it would be less confusing.

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

Well, .com doesn't exist anymore. ;) But, you're right, it should handle other extensions. I didn't know about splitext, thanks for pointing that out.

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

This is used in the GUI to indicate that the selected tool is not available and to disable the button used to launch the tool.

> +# Authors: Robert Collins <email address hidden>
>
> You conned Robert into writing your tests for you? :)

Woops! Copy & paste for the lose. :P

Thanks for the comments! They're greatly appreciated.