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

> I'm not sure what handling I'm meant to do other than use unicode literals
> (e.g. u'blah') when working with the command-line in a merge tool, like so:

I'd suggest step one should be testing, so you know what is breaking where. Just using unicode literals won't save you because of python limitations:

<http://bugs.python.org/issue1759845>

> 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 see _optional_quote_arg is still being used. I think you should be storing a list not a string in self._commandline so you can delete that and the related functions.

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

We don't want the user to be doing any quoting. Needing to wrap every %var in quotes just in case it contains a space is shell-level stupidity we're avoiding by handling that for them.

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

I really don't like grovelling through the filesystem with _find_executable as an idea. (Does it even have a clear licence?) The 'detect' command for merge tools does seem reasonable though so maybe it's unavoidable.