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

On 12/6/2010 10:31 AM, Vincent Ladeuil wrote:
> The missing vertical spaces, the config options renaming and the
> use of overrideAttr and they can't be re-merged now.

Woops, I thought the 'bzr.' prefix was some weird notation that you were
just using for the purposes of discussion. :)

And somehow I missed what you said about overrideAttr. That's cool...

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

I must have missed that. I jsut had a look through the code and I don't
think I'm using them anymore. I needed them for sorting MergeTools in
lexical order for comparing lists during tests, I think. But the tests
don't do that anymore.

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

Since I'm using string.format to do filename substitution, you're right
that this can be done with a single string commandline form.

> There is also a failing test:
> ERROR: bzrlib.tests.test_osutils.TestFindExecutableInPath.test_other
> 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'

My bad, missed a reference when I renamed the function.

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

Ok.

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

PATHEXT is applied if it exists. Since it won't exist on non-win32, that
shouldn't be a problem, but for safety sake I suppose we could add a
platform check.

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

Right. I should use the same os.access call that find_executable_on_path
uses.

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

I think I see what you mean. You think _KNOWN_MERGE_TOOLS should be
checked when a merge tool is requested and availability of that tool
should only be checked when the tool is actually invoked.

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

Ok.

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

Ok.

Just had a quick look at your changes. Config._find_merge_tool needs to
be a public function since it will be used by (G)UI code to get the
merge tool as specified by the user.