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

> I think I've addressed all of the comments so far. Please have another look
> and let me know if I've missed anything.

Well, for a start you missed the ones I proposed earlier in
lp:~vila/bzr/mergetools :-D

The missing vertical spaces, the config options renaming and the
use of overrideAttr and they can't be re-merged now.

I think poolie mentioned early in these reviews that we try to
avoid the "smart" operators (__eq__, __ne__), see
doc/developers/code-style.txt 'Imitating standard objects'. The
rationale is it's often more work to maintain these methods than
having explicit comparison operators (they make the code clearer
and easier to maintain).

The arg splitting functions are not necessary, the MergeTool
object receives the commandline when built. It should just
preserve it as-is, no need to rebuild it (splitting and
un-spliting just add possible failure points). This would also
get rid of the three commandline accessors.

There is also a failing test:
ERROR: bzrlib.tests.test_osutils.TestFindExecutableInPath.test_other
----------------------------------------------------------------------
_StringException: Text attachment: log
------------

------------
Text attachment: traceback
------------
Traceback (most recent call last):
  File "/home/vila/lib/python/testtools/runtest.py", line 144, in _run_user
    return fn(*args)
  File "/home/vila/lib/python/testtools/testcase.py", line 487, in _run_test_method
    testMethod()
  File "/home/vila/src/bzr/reviews/mergetools/bzrlib/tests/test_osutils.py", line 2133, in test_other
    self.assertTrue(osutils.is_executable_on_path('sh') is not None)
AttributeError: 'module' object has no attribute 'is_executable_on_path'
------------

But this goes in the right direction nevertheless :)

So, I went ahead fixing the style issues, renaming the options,
making the command line splitting internal and deleting the
associated accessors (and tests). This means making the merge
tool 'name' mandatory which also meant making _KNOWN_MERGE_TOOLS
a dict, until it becomes a proper registry (you may want to fix
the module docstring until it really is a registry).

There are still some weird things around find_executable_on_path,
PATHEXT is windows specific but seems to be applied
unconditionnally. Also, I think windows search in the current
directory if PATH is empty (which we may not want to mimick but
is worth documenting too).

In is_available, you check only for the existence of the file and
not whether the x bit is is set. There is a grey area there that
deserves at least a FIXME comment or better, a fix with proper
multi-platform tests (I won't block on that as long as it's
documented (i.e. a FIXME will do)).

I still don't agree with set_merge_tools, it will hardwire
_KNOWN_MERGE_TOOLS in whatever config file it is used against,
defeating the purpose of having default values configured (and
kept up to date) by bzr itself.

I think the core feature here is to check the availability of a
merge tool, but this needs to be done when bzr is about to need
it, doing it ahead of time is wrong as things could change. A
tool that you registered may have disapeared and a tool that you
ignored may have appeared. In both cases, you still want the
feature, but registering the results in the config file is wrong.

So I think seeing these feature used in your other mp will make
things clearer, you may want to move the relevant parts
(refactored as needed) there instead, for a final review of
*this* proposal.

set_default_merge_tool doesn't respect the way we handle all
other config options:
- it uses the None value to imply removing the option,
- it accepts either a string or a MergeTool object (we use only strings)
I fixed this one too.