Re: [PATCH 4/4] git-difftool: use git-mergetool--lib for "--tool-help"

On Fri, Jan 25, 2013 at 1:19 AM, John Keeping [off-list ref] wrote:

On Thu, Jan 24, 2013 at 09:29:58PM -0800, David Aguilar wrote:

quoted

On Thu, Jan 24, 2013 at 11:55 AM, John Keeping [off-list ref] wrote:

quoted

The "--tool-help" option to git-difftool currently displays incorrect
output since it uses the names of the files in
"$GIT_EXEC_PATH/mergetools/" rather than the list of command names in
git-mergetool--lib.

Fix this by simply delegating the "--tool-help" argument to the
show_tool_help function in git-mergetool--lib.

Very nice.

One thought I had was that the unified show_tool_help should
probably check TOOL_MODE=diff and skip over the
!can_diff entries.

The current output of "git difftool --tool-help" before your
patches has the problem that it will list tools such as
"tortoisemerge" as "valid but not available" because it
does not differentiate between missing and !can_diff.

list_merge_tool_candidates does this for us, so it should Just Work
since we use that to generate the list of tools that we loop over.

Yup, kind of.  I looked more closely and found a lot of
special-cases around vim which I've eliminated in the series
I just sent (which includes your patches as its base).

list_merge_tool_candidates() has a bunch of other special cases
for $EDITOR, $DISPLAY, $GNOME-something and such so I think
we should keep using it only for the guess_merge_tool() path.

I honestly want to remove list_merge_tool_candidates every
time I read it, but I recognize that it does serve a purpose
for users who have not configured anything.

In order to be useful for the documentation I think the
code could be refactored a tiny bit more beyond what I've
sent so far.  The gathering of available tools can be split
off from the reporting, and then show_tool_help() can use
the gatherer.  With what I sent, though, there's at least
a 1:1 correspondance between the name of the scriptlets
and the names accepted by --tool=<tool>, which is the first
step towards doing that.

I have to check out for now but I'll keep poking at this
when the weekend rolls around.

cheers,
-- 
David