patch, 4 partsFix "git difftool --tool-help"

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.

This series fixes this by changing it to simply delegate to a function in git-mergetool--lib.

The first three patches are just refactorings to move the show_tool_help function from git-mergetool to git-mergetool--lib and make it usable by git-difftool. The final patch switches git-difftool to use this function.

John Keeping (4):
  git-mergetool: move show_tool_help to mergetool--lib
  git-mergetool: remove redundant assignment
  git-mergetool: don't hardcode 'mergetool' in show_tool_help
  git-difftool: use git-mergetool--lib for "--tool-help"

 git-difftool.perl     | 57 ++++++++-------------------------------------------
 git-mergetool--lib.sh | 38 ++++++++++++++++++++++++++++++++++
 git-mergetool.sh      | 37 ---------------------------------
 3 files changed, 46 insertions(+), 86 deletions(-)

This is the first step in unifying "git difftool --tool-help" and "git mergetool --tool-help".

Signed-off-by: John Keeping <john@keeping.me.uk>
---
 git-mergetool--lib.sh | 37 +++++++++++++++++++++++++++++++++++++
 git-mergetool.sh      | 37 -------------------------------------
 2 files changed, 37 insertions(+), 37 deletions(-)

TOOL_MODE is set at the top of git-mergetool.sh so there is no need to set it again in show_tool_help. Removing this lets us re-use show_tool_help in git-difftool.

Signed-off-by: John Keeping <john@keeping.me.uk>
---
 git-mergetool--lib.sh | 1 -
 1 file changed, 1 deletion(-)

When using show_tool_help from git-difftool we will want it to print "git difftool" not "git mergetool" so use "git ${TOOL_MODE}tool".

Signed-off-by: John Keeping <john@keeping.me.uk>
---
 git-mergetool--lib.sh | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

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.

Signed-off-by: John Keeping <john@keeping.me.uk>
---
 git-difftool.perl | 57 ++++++++-----------------------------------------------
 1 file changed, 8 insertions(+), 49 deletions(-)

On Thu, Jan 24, 2013 at 11:55 AM, John Keeping <john@keeping.me.uk> wrote:

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.

This is where we conflated can_diff with "is the tool available?". The nice thing is that the old code did actually check can_diff, but since it conflated these things it ended up putting them in the @notfound list. It should ignore those completely, IMO.

That could be a nice follow-up patch.  What do you think?

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

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.

John

On Fri, Jan 25, 2013 at 1:19 AM, John Keeping <john@keeping.me.uk> wrote:

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,

On Fri, Jan 25, 2013 at 01:55:03AM -0800, David Aguilar wrote:

Actually, I'm not sure it does. I asked one of my colleagues whether he used git-mergetool the other day and he said no because he couldn't understand the OS X FileMerge tool and was happier to edit things manually in vim. I don't think he'd realised that he could configure a different mergetool.

Perhaps we're trying to be too clever by guessing what the user wants and should instead exit with a message saying:

   You have not configured a merge tool to use.  Please select one of
   the following tools and configure it using:

        git config merge.tool <tool>

    These tools are availalble on your system:
        ...

    These tools are supported but unavailable:
        ...

This may be too much of a regression for current users though.

John

Nice code reduction ;-)

Thanks, will queue.