{"thread":{"id":"29960","subject":"[PATCH] difftool: print list of valid tools with '--tool-help'","startedAt":"2012-03-15T22:25:11Z","lastAt":"2012-03-26T16:23:35Z","messageCount":7,"participants":["Tim Henigan","Junio C Hamano","David Aguilar"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"187064","messageId":"1331850311-4620-1-git-send-email-tim.henigan@gmail.com","threadId":"29960","inReplyTo":null,"subject":"[PATCH] difftool: print list of valid tools with '--tool-help'","fromName":"Tim Henigan","fromEmail":"tim.henigan@gmail.com","sentAt":"2012-03-15T22:25:11Z","receivedAt":"2012-03-15T22:25:11Z","isPatch":true,"sender":{"key":"tim.henigan@gmail.com","avatar":"https://avatars.githubusercontent.com/u/42022?v=4"},"body":"Since bc7a96a, it is possible to add new diff tools by creating a\nsimple script file in the '$(git --exec-path)/mergetools' directory.\nHowever, updating the difftool help text is still a manual process.\n\nThis commit reads the list of valid diff tools from the 'mergetools'\ndirectory and prints them for the user when the '--tool-help' option\nis given.\n\nSigned-off-by: Tim Henigan \n---\n Documentation/git-difftool.txt | 7 ++++---\n git-difftool.perl | 14 ++++++++++++--\n 2 files changed, 16 insertions(+), 5 deletions(-)\n\ndiff --git a/Documentation/git-difftool.txt b/Documentation/git-difftool.txt\nindex 19d473c..2b4b6ee 100644\n--- a/Documentation/git-difftool.txt\n+++ b/Documentation/git-difftool.txt\n@@ -31,9 +31,10 @@ OPTIONS\n -t ::\n --tool=::\n \tUse the diff tool specified by .\n-\tValid diff tools are:\n-\taraxis, bc3, diffuse, emerge, ecmerge, gvimdiff, kdiff3,\n-\tkompare, meld, opendiff, p4merge, tkdiff, vimdiff and xxdiff.\n+\tSee `--tool-help` for the list of valid settings.\n+\n+--tool-help::\n+\tPrint a list of diff tools that may be used with `--tool`.\n +\n If a diff tool is not specified, 'git difftool'\n will use the configuration variable `diff.tool`. If the\ndiff --git a/git-difftool.perl b/git-difftool.perl\nindex 09b65f1..4cfb1ae 100755\n--- a/git-difftool.perl\n+++ b/git-difftool.perl\n@@ -14,7 +14,7 @@ use 5.008;\n use strict;\n use warnings;\n use Cwd qw(abs_path);\n-use File::Basename qw(dirname);\n+use File::Basename qw(dirname basename);\n \n require Git;\n \n@@ -24,7 +24,8 @@ my $DIR = abs_path(dirname($0));\n sub usage\n {\n \tprint << 'USAGE';\n-usage: git difftool [-t|--tool=] [-x|--extcmd=]\n+usage: git difftool [-t|--tool=] [--tool-help]\n+ [-x|--extcmd=]\n [-y|--no-prompt] [-g|--gui]\n ['git diff' options]\n USAGE\n@@ -100,6 +101,15 @@ sub generate_command\n \t\tif ($arg eq '-h') {\n \t\t\tusage();\n \t\t}\n+\t\tif ($arg eq '--tool-help') {\n+\t\t\tmy $gitpath = Git::exec_path();\n+\t\t\tprint \"'git difftool --tool=' may be set to one of the following:\\n\";\n+\t\t\tfor (glob \"$gitpath/mergetools/*\") {\n+\t\t\t\tnext if /defaults$/;\n+\t\t\t\tprint \"\\t\" . basename($_) . \"\\n\";\n+\t\t\t}\n+\t\t\texit(1);\n+\t\t}\n \t\tpush @command, $arg;\n \t}\n \tif ($prompt eq 'yes') {\n-- \n1.7.9.GIT\n"},{"id":"187066","messageId":"7vk42lijfu.fsf@alter.siamese.dyndns.org","threadId":"29960","inReplyTo":"1331850311-4620-1-git-send-email-tim.henigan@gmail.com","subject":"Re: [PATCH] difftool: print list of valid tools with '--tool-help'","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-03-15T22:32:53Z","receivedAt":"2012-03-15T22:32:53Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Tim Henigan writes:\n\n> @@ -100,6 +101,15 @@ sub generate_command\n> \t\tif ($arg eq '-h') {\n> \t\t\tusage();\n> \t\t}\n> +\t\tif ($arg eq '--tool-help') {\n> +\t\t\tmy $gitpath = Git::exec_path();\n> +\t\t\tprint \"'git difftool --tool=' may be set to one of the following:\\n\";\n> +\t\t\tfor (glob \"$gitpath/mergetools/*\") {\n> +\t\t\t\tnext if /defaults$/;\n> +\t\t\t\tprint \"\\t\" . basename($_) . \"\\n\";\n> +\t\t\t}\n> +\t\t\texit(1);\n> +\t\t}\n\nI know the call to usage() against \"-h\" has the same issue, but I think\npeople find it offensive when they ask for help and the command reports a\nfailure with a non-zero exit code.\n\nOther than that, looks good from a cursory look. Davidd, any comments?\n\n> \t\tpush @command, $arg;\n> \t}\n> \tif ($prompt eq 'yes') {\n"},{"id":"187068","messageId":"7v8vj1ihcn.fsf@alter.siamese.dyndns.org","threadId":"29960","inReplyTo":"1331850311-4620-1-git-send-email-tim.henigan@gmail.com","subject":"Re: [PATCH] difftool: print list of valid tools with '--tool-help'","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-03-15T23:18:00Z","receivedAt":"2012-03-15T23:18:00Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Tim Henigan writes:\n\n> @@ -100,6 +101,15 @@ sub generate_command\n> \t\tif ($arg eq '-h') {\n> \t\t\tusage();\n> \t\t}\n> +\t\tif ($arg eq '--tool-help') {\n> +\t\t\tmy $gitpath = Git::exec_path();\n> +\t\t\tprint \"'git difftool --tool=' may be set to one of the following:\\n\";\n> +\t\t\tfor (glob \"$gitpath/mergetools/*\") {\n> +\t\t\t\tnext if /defaults$/;\n> +\t\t\t\tprint \"\\t\" . basename($_) . \"\\n\";\n> +\t\t\t}\n\nAs this topic to show list of tools dynamically has plenty of time to be\nin the mainline (it will be post 1.7.10), I would suggest a follow-up\nseries to this patch to do things like the following (just thinking\naloud):\n\n - define a new entry point to these mergetools/ scriptlets, let's call\n it \"cando\". An entry for mergetools/kompare might look like this:\n\n cando () {\n type kompare >/dev/null && test -n \"$DISPLAY\"\n }\n\n that would yield true only when kompare is available and $DISPLAY is\n set.\n\n - instead of dumping everything in $gitpath/mergetools/*, check if each\n tool says it can be used in the user's environment.\n\n for (glob \"$gitpath/mergetools/*\") {\n next unless can_run($_);\n print ...\n }\n ...\n\n and \"can_run\" may look like this:\n\n sub can_run {\n my ($script) = @_;\n my $cmd = \". '$script' && cando\";\n system('sh', '-c', $cmd) == 0;\n }\n\n - perhaps show the result in columnar layout.\n"},{"id":"187131","messageId":"CAJDDKr4bevWPjvQVLiWYm_6LxiHObms+i2QfeKKJB2g4eF6RnA@mail.gmail.com","threadId":"29960","inReplyTo":"7vk42lijfu.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] difftool: print list of valid tools with '--tool-help'","fromName":"David Aguilar","fromEmail":"davvid@gmail.com","sentAt":"2012-03-17T03:31:07Z","receivedAt":"2012-03-17T03:31:07Z","isPatch":true,"sender":{"key":"davvid@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13196?v=4"},"body":"On Thu, Mar 15, 2012 at 3:32 PM, Junio C Hamano wrote:\n> Tim Henigan writes:\n>\n>> @@ -100,6 +101,15 @@ sub generate_command\n>>               if ($arg eq '-h') {\n>>                       usage();\n>>               }\n>> +             if ($arg eq '--tool-help') {\n>> +                     my $gitpath = Git::exec_path();\n>> +                     print \"'git difftool --tool=' may be set to one of the following:\\n\";\n>> +                     for (glob \"$gitpath/mergetools/*\") {\n>> +                             next if /defaults$/;\n>> +                             print \"\\t\" . basename($_) . \"\\n\";\n>> +                     }\n>> +                     exit(1);\n>> +             }\n>\n> I know the call to usage() against \"-h\" has the same issue, but I think\n> people find it offensive when they ask for help and the command reports a\n> failure with a non-zero exit code.\n>\n> Other than that, looks good from a cursory look.  Davidd, any comments?\n\n\nI agree that the rework to make -h return 0 is sensible. I also think\nthe `cando()` suggestion later in this thread is the way to go.\n\nThanks guys,\n-- \nDavid\n"},{"id":"187596","messageId":"CAFouethYEmY7g3Uw0HfFMOKFNcBv=TyAsSyEkBU-cQrhaf6v7g@mail.gmail.com","threadId":"29960","inReplyTo":"7v8vj1ihcn.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] difftool: print list of valid tools with '--tool-help'","fromName":"Tim Henigan","fromEmail":"tim.henigan@gmail.com","sentAt":"2012-03-23T18:26:32Z","receivedAt":"2012-03-23T18:26:32Z","isPatch":true,"sender":{"key":"tim.henigan@gmail.com","avatar":"https://avatars.githubusercontent.com/u/42022?v=4"},"body":"On Thu, Mar 15, 2012 at 7:18 PM, Junio C Hamano wrote:\n> Tim Henigan writes:\n>\n> As this topic to show list of tools dynamically has plenty of time to be\n> in the mainline (it will be post 1.7.10), I would suggest a follow-up\n> series to this patch to do things like the following (just thinking\n> aloud):\n>\n>  - define a new entry point to these mergetools/ scriptlets, let's call\n>   it \"cando\".  An entry for mergetools/kompare might look like this:\n>\n>        cando () {\n>                type kompare >/dev/null && test -n \"$DISPLAY\"\n>        }\n>\n>   that would yield true only when kompare is available and $DISPLAY is\n>   set.\n>\n>  - instead of dumping everything in $gitpath/mergetools/*, check if each\n>   tool says it can be used in the user's environment.\n\nI experimented with this a bit and came up with the following idea\n(not protected for GMail whitespace mangling):\n\n# The following is to be added to 'git-difftool.perl'\nsub print_tool_help\n{\n\tmy ($cmd, @found, @notfound);\n\tmy $gitpath = Git::exec_path();\n\n\tfor (glob \"$gitpath/mergetools/*\") {\n\t\tmy $tool = basename($_);\n\t\tnext if ($tool eq \"defaults\");\n\n\t\t$cmd = '. \"$(git --exec-path)/git-mergetool--lib\"';\n\t\t$cmd .= \" && get_merge_tool_path $tool >/dev/null 2>&1\";\n\t\tif (system('sh', '-c', $cmd) == 0) {\n\t\t\tpush(@found, $tool);\n\t\t} else {\n\t\t\tpush(@notfound, $tool);\n\t\t}\n\t}\n\n\tprint \"'git difftool --tool=' may be set to one of the following:\\n\";\n\tprint \"\\t$_\\n\" for (@found);\n\n\tprint \"\\nThe following are valid tools, but not currently installed:\\n\";\n\tprint \"\\t$_\\n\" for (@notfound);\n\n\texit(0);\n}\n\nRather than create a new entry point, I used the existing\n'get_merge_tool_path' that resides in 'git-mergetool--lib' to\ndetermine if a given tool is actually installed on the system.\n\nThe '$DISPLAY' variable is lost in this implementation, but honestly I\ndon't understand how it was intended to be used.\n\nDoes this look useful?\n"},{"id":"187607","messageId":"7vzkb7rtd0.fsf@alter.siamese.dyndns.org","threadId":"29960","inReplyTo":"CAFouethYEmY7g3Uw0HfFMOKFNcBv=TyAsSyEkBU-cQrhaf6v7g@mail.gmail.com","subject":"Re: [PATCH] difftool: print list of valid tools with '--tool-help'","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-03-23T19:48:43Z","receivedAt":"2012-03-23T19:48:43Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Tim Henigan writes:\n\n> Rather than create a new entry point, I used the existing\n> 'get_merge_tool_path' that resides in 'git-mergetool--lib' to\n> determine if a given tool is actually installed on the system.\n>\n> The '$DISPLAY' variable is lost in this implementation, but honestly I\n> don't understand how it was intended to be used.\n>\n> Does this look useful?\n\nSounds like a better way to do it.\n\nThe point of checking DISPLAY in \"cando\" is to allow for a tool that\nchanges its behaviour when launched in terminal-only vs windowed\nenvironment. For example, if somebody tries to spawn a tool that *only*\nworks in windowed environment (e.g. \"kompare\") in a terminal-only session,\nit would fail, and the \"cando\" mechanism was designed to allow a tool\ndetect such a situation and excuse itself from the available tools list.\n\nI think get_merge_tool_path could do the same thing (i.e. report \"no\nexecutable available to run this tool\" when appropriate), so I do not\nthink you lost anything.\n"},{"id":"187734","messageId":"1332779015-22882-1-git-send-email-tim.henigan@gmail.com","threadId":"29960","inReplyTo":"7vzkb7rtd0.fsf@alter.siamese.dyndns.org","subject":"[PATCH 9/9 v7] difftool: print list of valid tools with '--tool-help'","fromName":"Tim Henigan","fromEmail":"tim.henigan@gmail.com","sentAt":"2012-03-26T16:23:35Z","receivedAt":"2012-03-26T16:23:35Z","isPatch":true,"sender":{"key":"tim.henigan@gmail.com","avatar":"https://avatars.githubusercontent.com/u/42022?v=4"},"body":"Since bc7a96a (mergetool--lib: Refactor tools into separate files,\n2011-08-18), it is possible to add a new diff tool by creating a simple\nscript in the '$(git --exec-path)/mergetools' directory. Updating the\ndifftool help text is still a manual process, and the documentation can\neasily go out of sync.\n\nTeach the command to read the list of valid tools from the 'mergetools'\ndirectory, determine which of them are actually installed and then print\nthem for the user when the '--tool-help' option is given.\n\nSigned-off-by: Tim Henigan \n---\n\nThis replaces 9/9 in the previous version the series.\n\nChanges in v7:\n\n - The list of possible tools is now tested to find out which are\n actually installed and available to run.\n - The user is informed that tools that require a windowed session will\n fail if they are running a terminal-only session.\n\nThis change is based on a suggestion given by Junio Hamano on the Git\ndeveloper list [1].\n\n[1]: http://permalink.gmane.org/gmane.comp.version-control.git/193237\n\n\n Documentation/git-difftool.txt | 11 ++++++-----\n git-difftool.perl | 40 +++++++++++++++++++++++++++++++++++++---\n t/t7800-difftool.sh | 5 +++++\n 3 files changed, 48 insertions(+), 8 deletions(-)\n\ndiff --git a/Documentation/git-difftool.txt b/Documentation/git-difftool.txt\nindex aba5e76..31fc2e3 100644\n--- a/Documentation/git-difftool.txt\n+++ b/Documentation/git-difftool.txt\n@@ -36,11 +36,9 @@ OPTIONS\n \n -t ::\n --tool=::\n-\tUse the diff tool specified by .\n-\tValid diff tools are:\n-\taraxis, bc3, deltawalker, diffuse, emerge, ecmerge, gvimdiff,\n-\tkdiff3,\tkompare, meld, opendiff, p4merge, tkdiff, vimdiff and\n-\txxdiff.\n+\tUse the diff tool specified by . Valid values include\n+\temerge, kompare, meld, and vimdiff. Run `git difftool --tool-help`\n+\tfor the list of valid settings.\n +\n If a diff tool is not specified, 'git difftool'\n will use the configuration variable `diff.tool`. If the\n@@ -68,6 +66,9 @@ of the diff post-image. `$MERGED` is the name of the file which is\n being compared. `$BASE` is provided for compatibility\n with custom merge tool commands and has the same value as `$MERGED`.\n \n+--tool-help::\n+\tPrint a list of diff tools that may be used with `--tool`.\n+\n -x ::\n --extcmd=::\n \tSpecify a custom command for viewing diffs.\ndiff --git a/git-difftool.perl b/git-difftool.perl\nindex 0fa131c..35370b8 100755\n--- a/git-difftool.perl\n+++ b/git-difftool.perl\n@@ -14,7 +14,7 @@\n use 5.008;\n use strict;\n use warnings;\n-use File::Basename qw(dirname);\n+use File::Basename qw(dirname basename);\n use File::Copy;\n use File::stat;\n use File::Path qw(mkpath);\n@@ -28,7 +28,7 @@ sub usage\n {\n \tmy $exitcode = shift;\n \tprint << 'USAGE';\n-usage: git difftool [-t|--tool=]\n+usage: git difftool [-t|--tool=] [--tool-help]\n [-x|--extcmd=]\n [-g|--gui] [--no-gui]\n [--prompt] [-y|--no-prompt]\n@@ -38,6 +38,36 @@ USAGE\n \texit($exitcode);\n }\n \n+sub print_tool_help\n+{\n+\tmy ($cmd, @found, @notfound);\n+\tmy $gitpath = Git::exec_path();\n+\n+\tfor (glob \"$gitpath/mergetools/*\") {\n+\t\tmy $tool = basename($_);\n+\t\tnext if ($tool eq \"defaults\");\n+\n+\t\t$cmd = '. \"$(git --exec-path)/git-mergetool--lib\"';\n+\t\t$cmd .= \" && get_merge_tool_path $tool >/dev/null 2>&1\";\n+\t\tif (system('sh', '-c', $cmd) == 0) {\n+\t\t\tpush(@found, $tool);\n+\t\t} else {\n+\t\t\tpush(@notfound, $tool);\n+\t\t}\n+\t}\n+\n+\tprint \"'git difftool --tool=' may be set to one of the following:\\n\";\n+\tprint \"\\t$_\\n\" for (@found);\n+\n+\tprint \"\\nThe following tools are valid, but not currently available:\\n\";\n+\tprint \"\\t$_\\n\" for (@notfound);\n+\n+\tprint \"\\nNOTE: Some of the tools listed above only work in a windowed\\n\";\n+\tprint \"environment. If run in a terminal-only session, they will fail.\\n\";\n+\n+\texit(0);\n+}\n+\n sub setup_dir_diff\n {\n \t# Run the diff; exit immediately if no diff found\n@@ -132,18 +162,22 @@ sub setup_dir_diff\n \n # parse command-line options. all unrecognized options and arguments\n # are passed through to the 'git diff' command.\n-my ($difftool_cmd, $dirdiff, $extcmd, $gui, $help, $prompt);\n+my ($difftool_cmd, $dirdiff, $extcmd, $gui, $help, $prompt, $tool_help);\n GetOptions('g|gui!' => \\$gui,\n \t'd|dir-diff' => \\$dirdiff,\n \t'h' => \\$help,\n \t'prompt!' => \\$prompt,\n \t'y' => sub { $prompt = 0; },\n \t't|tool:s' => \\$difftool_cmd,\n+\t'tool-help' => \\$tool_help,\n \t'x|extcmd:s' => \\$extcmd);\n \n if (defined($help)) {\n \tusage(0);\n } \n+if (defined($tool_help)) {\n+\tprint_tool_help();\n+}\n if (defined($difftool_cmd)) {\n \tif (length($difftool_cmd) > 0) {\n \t\t$ENV{GIT_DIFF_TOOL} = $difftool_cmd;\ndiff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh\nindex 478c1be..bbe71e5 100755\n--- a/t/t7800-difftool.sh\n+++ b/t/t7800-difftool.sh\n@@ -319,6 +319,11 @@ test_expect_success PERL 'say no to the second file' '\n \techo \"$diff\" | stdin_doesnot_contain br2\n '\n \n+test_expect_success PERL 'difftool --tool-help' '\n+\ttool_help=$(git difftool --tool-help) &&\n+\techo \"$tool_help\" | stdin_contains tool\n+'\n+\n test_expect_success PERL 'setup change in subdirectory' '\n \tgit checkout master &&\n \tmkdir sub &&\n-- \n1.7.10.rc1.36.gdd92a\n"}]}