[PATCH 0/4] Fix "git difftool --tool-help"

[PATCH 0/4] Fix "git difftool --tool-help"

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

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

-- 
1.8.1

[PATCH 1/4] git-mergetool: move show_tool_help to mergetool--lib

[John Keeping]

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

diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh
index f013a03..89a857f 100644
--- a/git-mergetool--lib.sh
+++ b/git-mergetool--lib.sh
@@ -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"
diff --git a/git-mergetool.sh b/git-mergetool.sh
index c50e18a..c0ee9aa 100755
--- a/git-mergetool.sh
+++ b/git-mergetool.sh
@@ -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
-- 
1.8.1

[PATCH 2/4] git-mergetool: remove redundant assignment

[John Keeping]

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

diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh
index 89a857f..1748315 100644
--- a/git-mergetool--lib.sh
+++ b/git-mergetool--lib.sh
@@ -175,7 +175,6 @@ list_merge_tool_candidates () {
 }
 
 show_tool_help () {
-	TOOL_MODE=merge
 	list_merge_tool_candidates
 	unavailable= available= LF='
 '
-- 
1.8.1

[PATCH 3/4] git-mergetool: don't hardcode 'mergetool' in show_tool_help

[John Keeping]

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

diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh
index 1748315..4c1e129 100644
--- a/git-mergetool--lib.sh
+++ b/git-mergetool--lib.sh
@@ -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
-- 
1.8.1

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

[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_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(-)

diff --git a/git-difftool.perl b/git-difftool.perl
index edd0493..0a90de4 100755
--- a/git-difftool.perl
+++ b/git-difftool.perl
@@ -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
-- 
1.8.1

Re: [PATCH 0/4] Fix "git difftool --tool-help"

[Junio C Hamano]

Nice code reduction ;-)

Thanks, will queue.

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

[David Aguilar]

On Thu, Jan 24, 2013 at 11:55 AM, John Keeping <john@keeping.me.uk> wrote:
> 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.


> diff --git a/git-difftool.perl b/git-difftool.perl
> index edd0493..0a90de4 100755
> --- a/git-difftool.perl
> +++ b/git-difftool.perl
> @@ -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);
> -               }
> -       }

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?
-- 
David

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

[John Keeping]

On Thu, Jan 24, 2013 at 09:29:58PM -0800, David Aguilar wrote:
> On Thu, Jan 24, 2013 at 11:55 AM, John Keeping <john@keeping.me.uk> wrote:
> > 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.


John

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

[David Aguilar]

On Fri, Jan 25, 2013 at 1:19 AM, John Keeping <john@keeping.me.uk> wrote:
> On Thu, Jan 24, 2013 at 09:29:58PM -0800, David Aguilar wrote:
>> On Thu, Jan 24, 2013 at 11:55 AM, John Keeping <john@keeping.me.uk> wrote:
>> > 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

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

[John Keeping]

On Fri, Jan 25, 2013 at 01:55:03AM -0800, David Aguilar wrote:
> 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.

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