Fix "git difftool --tool-help"
cover Fix "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(-)
Signature · 1 line
1.8.1
PATCH 1/4 git-mergetool: move show_tool_help to mergetool--lib
This is the first step in unifying "git difftool --tool-help" and "git mergetool --tool-help".
--- git-mergetool--lib.sh | 37 +++++++++++++++++++++++++++++++++++++ git-mergetool.sh | 37 ------------------------------------- 2 files changed, 37 insertions(+), 37 deletions(-)
git-mergetool--lib.sh +37 -0
@@ -174,6 +174,43 @@ list_merge_tool_candidates () { esac } +show_tool_help () { + TOOL_MODE=merge + list_merge_tool_candidates + unavailable= available= LF=' +' + for i in $tools + do + merge_tool_path=$(translate_merge_tool_path "$i") + if type "$merge_tool_path" >/dev/null 2>&1 + then + available="$available$i$LF" + else + unavailable="$unavailable$i$LF" + fi + done + if test -n "$available" + then + echo "'git mergetool --tool=<tool>' may be set to one of the following:" + echo "$available" | sort | sed -e 's/^/ /' + else + echo "No suitable tool for 'git mergetool --tool=<tool>' found." + fi + if test -n "$unavailable" + then + echo + echo 'The following tools are valid, but not currently available:' + echo "$unavailable" | sort | sed -e 's/^/ /' + fi + if test -n "$unavailable$available" + then + echo + echo "Some of the tools listed above only work in a windowed" + echo "environment. If run in a terminal-only session, they will fail." + fi + exit 0 +} + guess_merge_tool () { list_merge_tool_candidates echo >&2 "merge tool candidates: $tools"
git-mergetool.sh +0 -37
@@ -315,43 +315,6 @@ merge_file () { return 0 } -show_tool_help () { - TOOL_MODE=merge - list_merge_tool_candidates - unavailable= available= LF=' -' - for i in $tools - do - merge_tool_path=$(translate_merge_tool_path "$i") - if type "$merge_tool_path" >/dev/null 2>&1 - then - available="$available$i$LF" - else - unavailable="$unavailable$i$LF" - fi - done - if test -n "$available" - then - echo "'git mergetool --tool=<tool>' may be set to one of the following:" - echo "$available" | sort | sed -e 's/^/ /' - else - echo "No suitable tool for 'git mergetool --tool=<tool>' found." - fi - if test -n "$unavailable" - then - echo - echo 'The following tools are valid, but not currently available:' - echo "$unavailable" | sort | sed -e 's/^/ /' - fi - if test -n "$unavailable$available" - then - echo - echo "Some of the tools listed above only work in a windowed" - echo "environment. If run in a terminal-only session, they will fail." - fi - exit 0 -} - prompt=$(git config --bool mergetool.prompt || echo true) while test $# != 0
Signature · 1 line
1.8.1
PATCH 2/4 git-mergetool: remove redundant assignment
In reply to John KeepingTOOL_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.
--- git-mergetool--lib.sh | 1 - 1 file changed, 1 deletion(-)
git-mergetool--lib.sh +0 -1
@@ -175,7 +175,6 @@ list_merge_tool_candidates () { } show_tool_help () { - TOOL_MODE=merge list_merge_tool_candidates unavailable= available= LF=' '
Signature · 1 line
1.8.1
Nice code reduction ;-)
Thanks, will queue.
PATCH 3/4 git-mergetool: don't hardcode 'mergetool' in show_tool_help
In reply to John KeepingWhen using show_tool_help from git-difftool we will want it to print "git difftool" not "git mergetool" so use "git ${TOOL_MODE}tool".
--- git-mergetool--lib.sh | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)
git-mergetool--lib.sh +4 -2
@@ -188,12 +188,14 @@ show_tool_help () { unavailable="$unavailable$i$LF" fi done + + cmd_name=${TOOL_MODE}tool if test -n "$available" then - echo "'git mergetool --tool=<tool>' may be set to one of the following:" + echo "'git $cmd_name --tool=<tool>' may be set to one of the following:" echo "$available" | sort | sed -e 's/^/ /' else - echo "No suitable tool for 'git mergetool --tool=<tool>' found." + echo "No suitable tool for 'git $cmd_name --tool=<tool>' found." fi if test -n "$unavailable" then
Signature · 1 line
1.8.1
PATCH 4/4 git-difftool: use git-mergetool--lib for "--tool-help"
In reply to John KeepingThe "--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.
--- git-difftool.perl | 57 ++++++++----------------------------------------------- 1 file changed, 8 insertions(+), 49 deletions(-)
git-difftool.perl +8 -49
@@ -59,57 +59,16 @@ sub find_worktree return $worktree; } -sub filter_tool_scripts -{ - my ($tools) = @_; - if (-d $_) { - if ($_ ne ".") { - # Ignore files in subdirectories - $File::Find::prune = 1; - } - } else { - if ((-f $_) && ($_ ne "defaults")) { - push(@$tools, $_); - } - } -} - sub print_tool_help { - my ($cmd, @found, @notfound, @tools); - my $gitpath = Git::exec_path(); - - find(sub { filter_tool_scripts(\@tools) }, "$gitpath/mergetools"); - - foreach my $tool (@tools) { - $cmd = "TOOL_MODE=diff"; - $cmd .= ' && . "$(git --exec-path)/git-mergetool--lib"'; - $cmd .= " && get_merge_tool_path $tool >/dev/null 2>&1"; - $cmd .= " && can_diff >/dev/null 2>&1"; - if (system('sh', '-c', $cmd) == 0) { - push(@found, $tool); - } else { - push(@notfound, $tool); - } - } - - print << 'EOF'; -'git difftool --tool=<tool>' may be set to one of the following: -EOF - print "\t$_\n" for (sort(@found)); - - print << 'EOF'; - -The following tools are valid, but not currently available: -EOF - print "\t$_\n" for (sort(@notfound)); - - print << 'EOF'; - -NOTE: Some of the tools listed above only work in a windowed -environment. If run in a terminal-only session, they will fail. -EOF - exit(0); + my $cmd = 'TOOL_MODE=diff'; + $cmd .= ' && . "$(git --exec-path)/git-mergetool--lib"'; + $cmd .= ' && show_tool_help'; + + # See the comment at the bottom of file_diff() for the reason behind + # using system() followed by exit() instead of exec(). + my $rc = system('sh', '-c', $cmd); + exit($rc | ($rc >> 8)); } sub exit_cleanup
Signature · 1 line
1.8.1
Re: [PATCH 4/4] git-difftool: use git-mergetool--lib for "--tool-help"
Quoted 7 lines · John Keeping
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_helpfunction 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.
Quoted 31 lines
git-difftool.perl +0
-21@@ -59,57 +59,16 @@ sub find_worktree return $worktree; } -sub filter_tool_scripts -{ - my ($tools) = @_; - if (-d $_) { - if ($_ ne ".") { - # Ignore files in subdirectories - $File::Find::prune = 1; - } - } else { - if ((-f $_) && ($_ ne "defaults")) { - push(@$tools, $_); - } - } -} - sub print_tool_help { - my ($cmd, @found, @notfound, @tools); - my $gitpath = Git::exec_path(); - - find(sub { filter_tool_scripts(\@tools) }, "$gitpath/mergetools"); - - foreach my $tool (@tools) {
git-difftool.perl sub find_worktree · line 94
Quoted 10 lines
git-difftool.perl +0
-10- $cmd = "TOOL_MODE=diff"; - $cmd .= ' && . "$(git --exec-path)/git-mergetool--lib"'; - $cmd .= " && get_merge_tool_path $tool >/dev/null 2>&1"; - $cmd .= " && can_diff >/dev/null 2>&1"; - if (system('sh', '-c', $cmd) == 0) { - push(@found, $tool); - } else { - push(@notfound, $tool); - } - }
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?
Signature · 1 line
David
Re: [PATCH 4/4] git-difftool: use git-mergetool--lib for "--tool-help"
Quoted 11 lines · David Aguilar
Quoted 7 lines · John Keeping
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_helpfunction in git-mergetool--lib.Very nice.
Quoted 8 lines
One thought I had was that the unified
show_tool_helpshould 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.
John
Re: [PATCH 4/4] git-difftool: use git-mergetool--lib for "--tool-help"
Quoted 16 lines · John Keeping
Quoted 15 lines · David Aguilar
Quoted 7 lines · John Keeping
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_helpfunction in git-mergetool--lib.Very nice.
One thought I had was that the unified
show_tool_helpshould probably check TOOL_MODE=diff and skip over the !can_diff entries.
Quoted 7 lines
Quoted 4 lines
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_candidatesdoes 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,
Signature · 1 line
David
Re: [PATCH 4/4] git-difftool: use git-mergetool--lib for "--tool-help"
Quoted 7 lines · David Aguilar
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 theguess_merge_tool()path.I honestly want to remove
list_merge_tool_candidatesevery time I read it, but I recognize that it does serve a purpose for users who have not configured anything.
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