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

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

[Tim Henigan]

Since bc7a96a, it is possible to add new diff tools by creating a
simple script file in the '$(git --exec-path)/mergetools' directory.
However, updating the difftool help text is still a manual process.

This commit reads the list of valid diff tools from the 'mergetools'
directory and prints them for the user when the '--tool-help' option
is given.

Signed-off-by: Tim Henigan <tim.henigan@gmail.com>
---
 Documentation/git-difftool.txt |    7 ++++---
 git-difftool.perl              |   14 ++++++++++++--
 2 files changed, 16 insertions(+), 5 deletions(-)

diff --git a/Documentation/git-difftool.txt b/Documentation/git-difftool.txt
index 19d473c..2b4b6ee 100644
--- a/Documentation/git-difftool.txt
+++ b/Documentation/git-difftool.txt
@@ -31,9 +31,10 @@ OPTIONS
 -t <tool>::
 --tool=<tool>::
 	Use the diff tool specified by <tool>.
-	Valid diff tools are:
-	araxis, bc3, diffuse, emerge, ecmerge, gvimdiff, kdiff3,
-	kompare, meld, opendiff, p4merge, tkdiff, vimdiff and xxdiff.
+	See `--tool-help` for the list of valid <tool> settings.
+
+--tool-help::
+	Print a list of diff tools that may be used with `--tool`.
 +
 If a diff tool is not specified, 'git difftool'
 will use the configuration variable `diff.tool`.  If the
diff --git a/git-difftool.perl b/git-difftool.perl
index 09b65f1..4cfb1ae 100755
--- a/git-difftool.perl
+++ b/git-difftool.perl
@@ -14,7 +14,7 @@ use 5.008;
 use strict;
 use warnings;
 use Cwd qw(abs_path);
-use File::Basename qw(dirname);
+use File::Basename qw(dirname basename);
 
 require Git;
 
@@ -24,7 +24,8 @@ my $DIR = abs_path(dirname($0));
 sub usage
 {
 	print << 'USAGE';
-usage: git difftool [-t|--tool=<tool>] [-x|--extcmd=<cmd>]
+usage: git difftool [-t|--tool=<tool>]   [--tool-help]
+                    [-x|--extcmd=<cmd>]
                     [-y|--no-prompt]   [-g|--gui]
                     ['git diff' options]
 USAGE
@@ -100,6 +101,15 @@ sub generate_command
 		if ($arg eq '-h') {
 			usage();
 		}
+		if ($arg eq '--tool-help') {
+			my $gitpath = Git::exec_path();
+			print "'git difftool --tool=<tool>' may be set to one of the following:\n";
+			for (glob "$gitpath/mergetools/*") {
+				next if /defaults$/;
+				print "\t" . basename($_) . "\n";
+			}
+			exit(1);
+		}
 		push @command, $arg;
 	}
 	if ($prompt eq 'yes') {
-- 
1.7.9.GIT

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

[Junio C Hamano]

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

> @@ -100,6 +101,15 @@ sub generate_command
>  		if ($arg eq '-h') {
>  			usage();
>  		}
> +		if ($arg eq '--tool-help') {
> +			my $gitpath = Git::exec_path();
> +			print "'git difftool --tool=<tool>' may be set to one of the following:\n";
> +			for (glob "$gitpath/mergetools/*") {
> +				next if /defaults$/;
> +				print "\t" . basename($_) . "\n";
> +			}
> +			exit(1);
> +		}

I know the call to usage() against "-h" has the same issue, but I think
people find it offensive when they ask for help and the command reports a
failure with a non-zero exit code.

Other than that, looks good from a cursory look.  Davidd, any comments?

>  		push @command, $arg;
>  	}
>  	if ($prompt eq 'yes') {

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

[David Aguilar]

On Thu, Mar 15, 2012 at 3:32 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Tim Henigan <tim.henigan@gmail.com> writes:
>
>> @@ -100,6 +101,15 @@ sub generate_command
>>               if ($arg eq '-h') {
>>                       usage();
>>               }
>> +             if ($arg eq '--tool-help') {
>> +                     my $gitpath = Git::exec_path();
>> +                     print "'git difftool --tool=<tool>' may be set to one of the following:\n";
>> +                     for (glob "$gitpath/mergetools/*") {
>> +                             next if /defaults$/;
>> +                             print "\t" . basename($_) . "\n";
>> +                     }
>> +                     exit(1);
>> +             }
>
> I know the call to usage() against "-h" has the same issue, but I think
> people find it offensive when they ask for help and the command reports a
> failure with a non-zero exit code.
>
> Other than that, looks good from a cursory look.  Davidd, any comments?


I agree that the rework to make -h return 0 is sensible.  I also think
the `cando()` suggestion later in this thread is the way to go.

Thanks guys,
-- 
David

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

[Junio C Hamano]

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

> @@ -100,6 +101,15 @@ sub generate_command
>  		if ($arg eq '-h') {
>  			usage();
>  		}
> +		if ($arg eq '--tool-help') {
> +			my $gitpath = Git::exec_path();
> +			print "'git difftool --tool=<tool>' may be set to one of the following:\n";
> +			for (glob "$gitpath/mergetools/*") {
> +				next if /defaults$/;
> +				print "\t" . basename($_) . "\n";
> +			}

As this topic to show list of tools dynamically has plenty of time to be
in the mainline (it will be post 1.7.10), I would suggest a follow-up
series to this patch to do things like the following (just thinking
aloud):

 - define a new entry point to these mergetools/ scriptlets, let's call
   it "cando".  An entry for mergetools/kompare might look like this:

        cando () {
                type kompare >/dev/null && test -n "$DISPLAY"
        }

   that would yield true only when kompare is available and $DISPLAY is
   set.

 - instead of dumping everything in $gitpath/mergetools/*, check if each
   tool says it can be used in the user's environment.

        for (glob "$gitpath/mergetools/*") {
                next unless can_run($_);
                print ...
        }
        ...

   and "can_run" may look like this:

        sub can_run {
                my ($script) = @_;
             my $cmd = ". '$script' && cando";
             system('sh', '-c', $cmd) == 0;
        }

 - perhaps show the result in columnar layout.

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

[Tim Henigan]

On Thu, Mar 15, 2012 at 7:18 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Tim Henigan <tim.henigan@gmail.com> writes:
>
> As this topic to show list of tools dynamically has plenty of time to be
> in the mainline (it will be post 1.7.10), I would suggest a follow-up
> series to this patch to do things like the following (just thinking
> aloud):
>
>  - define a new entry point to these mergetools/ scriptlets, let's call
>   it "cando".  An entry for mergetools/kompare might look like this:
>
>        cando () {
>                type kompare >/dev/null && test -n "$DISPLAY"
>        }
>
>   that would yield true only when kompare is available and $DISPLAY is
>   set.
>
>  - instead of dumping everything in $gitpath/mergetools/*, check if each
>   tool says it can be used in the user's environment.

I experimented with this a bit and came up with the following idea
(not protected for GMail whitespace mangling):

# The following is to be added to 'git-difftool.perl'
sub print_tool_help
{
	my ($cmd, @found, @notfound);
	my $gitpath = Git::exec_path();

	for (glob "$gitpath/mergetools/*") {
		my $tool = basename($_);
		next if ($tool eq "defaults");

		$cmd  = '. "$(git --exec-path)/git-mergetool--lib"';
		$cmd .= " && get_merge_tool_path $tool >/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 (@found);

	print "\nThe following are valid tools, but not currently installed:\n";
	print "\t$_\n" for (@notfound);

	exit(0);
}

Rather than create a new entry point, I used the existing
'get_merge_tool_path' that resides in 'git-mergetool--lib' to
determine if a given tool is actually installed on the system.

The '$DISPLAY' variable is lost in this implementation, but honestly I
don't understand how it was intended to be used.

Does this look useful?

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

[Junio C Hamano]

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

> Rather than create a new entry point, I used the existing
> 'get_merge_tool_path' that resides in 'git-mergetool--lib' to
> determine if a given tool is actually installed on the system.
>
> The '$DISPLAY' variable is lost in this implementation, but honestly I
> don't understand how it was intended to be used.
>
> Does this look useful?

Sounds like a better way to do it.

The point of checking DISPLAY in "cando" is to allow for a tool that
changes its behaviour when launched in terminal-only vs windowed
environment. For example, if somebody tries to spawn a tool that *only*
works in windowed environment (e.g. "kompare") in a terminal-only session,
it would fail, and the "cando" mechanism was designed to allow a tool
detect such a situation and excuse itself from the available tools list.

I think get_merge_tool_path could do the same thing (i.e. report "no
executable available to run this tool" when appropriate), so I do not
think you lost anything.

[PATCH 9/9 v7] 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.

Teach the command to read the list of valid tools from the 'mergetools'
directory, determine which of them are actually installed and then print
them for the user when the '--tool-help' option is given.

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

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

Changes in v7:

  - The list of possible tools is now tested to find out which are
    actually installed and available to run.
  - The user is informed that tools that require a windowed session will
    fail if they are running a terminal-only session.

This change is based on a suggestion given by Junio Hamano on the Git
developer list [1].

[1]: http://permalink.gmane.org/gmane.comp.version-control.git/193237


 Documentation/git-difftool.txt |   11 ++++++-----
 git-difftool.perl              |   40 +++++++++++++++++++++++++++++++++++++---
 t/t7800-difftool.sh            |    5 +++++
 3 files changed, 48 insertions(+), 8 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..35370b8 100755
--- a/git-difftool.perl
+++ b/git-difftool.perl
@@ -14,7 +14,7 @@
 use 5.008;
 use strict;
 use warnings;
-use File::Basename qw(dirname);
+use File::Basename qw(dirname basename);
 use File::Copy;
 use File::stat;
 use File::Path qw(mkpath);
@@ -28,7 +28,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 +38,36 @@ USAGE
 	exit($exitcode);
 }
 
+sub print_tool_help
+{
+	my ($cmd, @found, @notfound);
+	my $gitpath = Git::exec_path();
+
+	for (glob "$gitpath/mergetools/*") {
+		my $tool = basename($_);
+		next if ($tool eq "defaults");
+
+		$cmd  = '. "$(git --exec-path)/git-mergetool--lib"';
+		$cmd .= " && get_merge_tool_path $tool >/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 (@found);
+
+	print "\nThe following tools are valid, but not currently available:\n";
+	print "\t$_\n" for (@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 +162,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.rc1.36.gdd92a