[PATCH 9/9 v8] difftool: print list of valid tools with '--tool-help'

[PATCH 9/9 v8] difftool: print list of valid tools with '--tool-help'

[Tim Henigan]

Since bc7a96a (mergetool--lib: Refactor tools into separate files,
2011-08-18), it is possible to add a new diff tool by creating a simple
script in the '$(git --exec-path)/mergetools' directory.  Updating the
difftool help text is still a manual process, and the documentation can
easily go out of sync.

This commit teaches difftool the '--tool-help' option, which:
  - Reads the list of valid tools from 'mergetools/*'
  - Determines which of them are actually installed
  - Determines which are capable of diffing (i.e. not just a merge tool)
  - Prints the resulting list for the user

Signed-off-by: Tim Henigan <tim.henigan@gmail.com>
---

This replaces 9/9 in the previous version of the series.

Changes in v8:
  - Replaced 'glob' with 'File::Find'. Glob will fail if file paths include
    spaces.  Using File::Find overcomes that limitation.
  - Added 'TOOL_MODE=DIFF' prior to calling 'git-mergetool--lib.sh'. This
    insures that the shell script executes as designed.
  - difftool now calls 'can_diff' from 'git-mergetool--lib.sh' to insure that
    only tools that are capable of diffing are shown as valid options. For
    example, 'tortoisemerge' cannot be used as a diff viewer.
  - The list of tools shown to the user is now sorted.


 Documentation/git-difftool.txt |   11 +++++-----
 git-difftool.perl              |   43 ++++++++++++++++++++++++++++++++++++++--
 t/t7800-difftool.sh            |    5 +++++
 3 files changed, 52 insertions(+), 7 deletions(-)

diff --git a/Documentation/git-difftool.txt b/Documentation/git-difftool.txt
index aba5e76..31fc2e3 100644
--- a/Documentation/git-difftool.txt
+++ b/Documentation/git-difftool.txt
@@ -36,11 +36,9 @@ OPTIONS
 
 -t <tool>::
 --tool=<tool>::
-	Use the diff tool specified by <tool>.
-	Valid diff tools are:
-	araxis, bc3, deltawalker, diffuse, emerge, ecmerge, gvimdiff,
-	kdiff3,	kompare, meld, opendiff, p4merge, tkdiff, vimdiff and
-	xxdiff.
+	Use the diff tool specified by <tool>.  Valid values include
+	emerge, kompare, meld, and vimdiff. Run `git difftool --tool-help`
+	for the list of valid <tool> settings.
 +
 If a diff tool is not specified, 'git difftool'
 will use the configuration variable `diff.tool`.  If the
@@ -68,6 +66,9 @@ of the diff post-image.  `$MERGED` is the name of the file which is
 being compared. `$BASE` is provided for compatibility
 with custom merge tool commands and has the same value as `$MERGED`.
 
+--tool-help::
+	Print a list of diff tools that may be used with `--tool`.
+
 -x <command>::
 --extcmd=<command>::
 	Specify a custom command for viewing diffs.
diff --git a/git-difftool.perl b/git-difftool.perl
index 0fa131c..15fd572 100755
--- a/git-difftool.perl
+++ b/git-difftool.perl
@@ -16,6 +16,7 @@ use strict;
 use warnings;
 use File::Basename qw(dirname);
 use File::Copy;
+use File::Find;
 use File::stat;
 use File::Path qw(mkpath);
 use File::Temp qw(tempdir);
@@ -28,7 +29,7 @@ sub usage
 {
 	my $exitcode = shift;
 	print << 'USAGE';
-usage: git difftool [-t|--tool=<tool>]
+usage: git difftool [-t|--tool=<tool>] [--tool-help]
                     [-x|--extcmd=<cmd>]
                     [-g|--gui] [--no-gui]
                     [--prompt] [-y|--no-prompt]
@@ -38,6 +39,40 @@ USAGE
 	exit($exitcode);
 }
 
+sub print_tool_help
+{
+	my ($cmd, @found, @notfound, @tools);
+	my $gitpath = Git::exec_path();
+
+	find(sub { push(@tools, $_) unless (-d $_) }, "$gitpath/mergetools");
+
+	for (@tools) {
+		my $tool = $_;
+		next if ($tool eq "defaults");
+
+		$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 "'git difftool --tool=<tool>' may be set to one of the following:\n";
+	print "\t$_\n" for (sort(@found));
+
+	print "\nThe following tools are valid, but not currently available:\n";
+	print "\t$_\n" for (sort(@notfound));
+
+	print "\nNOTE: Some of the tools listed above only work in a windowed\n";
+	print "environment. If run in a terminal-only session, they will fail.\n";
+
+	exit(0);
+}
+
 sub setup_dir_diff
 {
 	# Run the diff; exit immediately if no diff found
@@ -132,18 +167,22 @@ sub setup_dir_diff
 
 # parse command-line options. all unrecognized options and arguments
 # are passed through to the 'git diff' command.
-my ($difftool_cmd, $dirdiff, $extcmd, $gui, $help, $prompt);
+my ($difftool_cmd, $dirdiff, $extcmd, $gui, $help, $prompt, $tool_help);
 GetOptions('g|gui!' => \$gui,
 	'd|dir-diff' => \$dirdiff,
 	'h' => \$help,
 	'prompt!' => \$prompt,
 	'y' => sub { $prompt = 0; },
 	't|tool:s' => \$difftool_cmd,
+	'tool-help' => \$tool_help,
 	'x|extcmd:s' => \$extcmd);
 
 if (defined($help)) {
 	usage(0);
 } 
+if (defined($tool_help)) {
+	print_tool_help();
+}
 if (defined($difftool_cmd)) {
 	if (length($difftool_cmd) > 0) {
 		$ENV{GIT_DIFF_TOOL} = $difftool_cmd;
diff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh
index 478c1be..bbe71e5 100755
--- a/t/t7800-difftool.sh
+++ b/t/t7800-difftool.sh
@@ -319,6 +319,11 @@ test_expect_success PERL 'say no to the second file' '
 	echo "$diff" | stdin_doesnot_contain br2
 '
 
+test_expect_success PERL 'difftool --tool-help' '
+	tool_help=$(git difftool --tool-help) &&
+	echo "$tool_help" | stdin_contains tool
+'
+
 test_expect_success PERL 'setup change in subdirectory' '
 	git checkout master &&
 	mkdir sub &&
-- 
1.7.10.rc2.21.g8cb1a

Re: [PATCH 9/9 v8] difftool: print list of valid tools with '--tool-help'

[Junio C Hamano]

Tim Henigan <tim.henigan@gmail.com> writes:

> Changes in v8:
>   - Replaced 'glob' with 'File::Find'. Glob will fail if file paths include
>     spaces.  Using File::Find overcomes that limitation.

OK, but doesn't File::Find recurse into its subdirectories?  If you create
a 'foo' directory there and drop a 'bar' script in it, is the rest of the
code prepared to give you "git difftool -t foo/bar"?

>   - Added 'TOOL_MODE=DIFF' prior to calling 'git-mergetool--lib.sh'. This
>     insures that the shell script executes as designed.
>   - difftool now calls 'can_diff' from 'git-mergetool--lib.sh' to insure that
>     only tools that are capable of diffing are shown as valid options. For
>     example, 'tortoisemerge' cannot be used as a diff viewer.

Good that you caught these brown-paper-bag bugs ;-) It feels a bit awkward
that nobody pointed these out, even though the series has been queued in
'pu' since its early iterations.

> diff --git a/git-difftool.perl b/git-difftool.perl
> index 0fa131c..15fd572 100755
> --- a/git-difftool.perl
> +++ b/git-difftool.perl
> @@ -38,6 +39,40 @@ USAGE
>  	exit($exitcode);
>  }
>  
> +sub print_tool_help
> +{
> +	my ($cmd, @found, @notfound, @tools);
> +	my $gitpath = Git::exec_path();
> +
> +	find(sub { push(@tools, $_) unless (-d $_) }, "$gitpath/mergetools");
> +
> +	for (@tools) {
> +		my $tool = $_;
> +		next if ($tool eq "defaults");

Now you use File::Find::find(), you probably should do this kind of
trivial filtering inside the callback, no?

Re: [PATCH 9/9 v8] difftool: print list of valid tools with '--tool-help'

[Tim Henigan]

On Wed, Mar 28, 2012 at 2:58 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Tim Henigan <tim.henigan@gmail.com> writes:
>
> OK, but doesn't File::Find recurse into its subdirectories?  If you create
> a 'foo' directory there and drop a 'bar' script in it, is the rest of the
> code prepared to give you "git difftool -t foo/bar"?

It does recurse, but in this context '$_' only contains the current
file name within the directory...not the directory itself [1].  So if
we call 'find' on a directory that contains:

  foo
  bar/
      baz

then @tools = ('foo', 'baz')

[1]: http://perldoc.perl.org/File/Find.html#The-wanted-function


>> +     for (@tools) {
>> +             my $tool = $_;
>> +             next if ($tool eq "defaults");
>
> Now you use File::Find::find(), you probably should do this kind of
> trivial filtering inside the callback, no?

I thought about that, but the filter is so simple that it seemed like
overkill to add another function.  If there is another revision of
this patch, I will reconsider.

As always, thanks for the review :)

Re: [PATCH 9/9 v8] difftool: print list of valid tools with '--tool-help'

[Junio C Hamano]

Tim Henigan <tim.henigan@gmail.com> writes:

> On Wed, Mar 28, 2012 at 2:58 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> Tim Henigan <tim.henigan@gmail.com> writes:
>>
>> OK, but doesn't File::Find recurse into its subdirectories?  If you create
>> a 'foo' directory there and drop a 'bar' script in it, is the rest of the
>> code prepared to give you "git difftool -t foo/bar"?
>
> It does recurse, but in this context '$_' only contains the current
> file name within the directory...not the directory itself [1].  So if
> we call 'find' on a directory that contains:
>
>   foo
>   bar/
>       baz
>
> then @tools = ('foo', 'baz')

That is even worse, no?  Is the rest of the code prepared to give you "git
difftool -t baz" in such a layout?  What if you have another baz next to
foo and bar?

What I was hinting at was that you may want to $File::Find::prune=1 when
you find a subdirectory.  While at it, you may also want to replace
the "unless -d $_" with "if -f $_ && -x _" or something.

Re: [PATCH 9/9 v8] difftool: print list of valid tools with '--tool-help'

[Tim Henigan]

On Wed, Mar 28, 2012 at 4:02 PM, Junio C Hamano <gitster@pobox.com> wrote:
>
> That is even worse, no?  Is the rest of the code prepared to give you "git
> difftool -t baz" in such a layout?  What if you have another baz next to
> foo and bar?
>
> What I was hinting at was that you may want to $File::Find::prune=1 when
> you find a subdirectory.  While at it, you may also want to replace
> the "unless -d $_" with "if -f $_ && -x _" or something.

I see now...sorry I missed your obvious point.  I will look into this further.

The script makes some assumptions right now:
  1) All tool config scripts must be located in "$(git --exec-path)/mergetools".
  2) There is only one tool config in each script
        - See my other other recent patch that splits vim/gvim [1]
  3) The name of the tool script matches the name of the tool itself.

Do these assumptions seem reasonable? If so, perhaps I should add a
README to the mergetools directory that outlines these assumptions?

[1]: http://thread.gmane.org/gmane.comp.version-control.git/194179