{"thread":{"id":"30084","subject":"[PATCH 9/9 v8] difftool: print list of valid tools with '--tool-help'","startedAt":"2012-03-28T18:34:44Z","lastAt":"2012-03-28T20:16:46Z","messageCount":5,"participants":["Tim Henigan","Junio C Hamano"],"isPatch":true,"patchVersion":8,"patchTotal":9},"messages":[{"id":"187965","messageId":"1332959684-2231-1-git-send-email-tim.henigan@gmail.com","threadId":"30084","inReplyTo":null,"subject":"[PATCH 9/9 v8] difftool: print list of valid tools with '--tool-help'","fromName":"Tim Henigan","fromEmail":"tim.henigan@gmail.com","sentAt":"2012-03-28T18:34:44Z","receivedAt":"2012-03-28T18:34:44Z","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\nThis commit teaches difftool the '--tool-help' option, which:\n - Reads the list of valid tools from 'mergetools/*'\n - Determines which of them are actually installed\n - Determines which are capable of diffing (i.e. not just a merge tool)\n - Prints the resulting list for the user\n\nSigned-off-by: Tim Henigan \n---\n\nThis replaces 9/9 in the previous version of the series.\n\nChanges in v8:\n - Replaced 'glob' with 'File::Find'. Glob will fail if file paths include\n spaces. Using File::Find overcomes that limitation.\n - Added 'TOOL_MODE=DIFF' prior to calling 'git-mergetool--lib.sh'. This\n insures that the shell script executes as designed.\n - difftool now calls 'can_diff' from 'git-mergetool--lib.sh' to insure that\n only tools that are capable of diffing are shown as valid options. For\n example, 'tortoisemerge' cannot be used as a diff viewer.\n - The list of tools shown to the user is now sorted.\n\n\n Documentation/git-difftool.txt | 11 +++++-----\n git-difftool.perl | 43 ++++++++++++++++++++++++++++++++++++++--\n t/t7800-difftool.sh | 5 +++++\n 3 files changed, 52 insertions(+), 7 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..15fd572 100755\n--- a/git-difftool.perl\n+++ b/git-difftool.perl\n@@ -16,6 +16,7 @@ use strict;\n use warnings;\n use File::Basename qw(dirname);\n use File::Copy;\n+use File::Find;\n use File::stat;\n use File::Path qw(mkpath);\n use File::Temp qw(tempdir);\n@@ -28,7 +29,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 +39,40 @@ USAGE\n \texit($exitcode);\n }\n \n+sub print_tool_help\n+{\n+\tmy ($cmd, @found, @notfound, @tools);\n+\tmy $gitpath = Git::exec_path();\n+\n+\tfind(sub { push(@tools, $_) unless (-d $_) }, \"$gitpath/mergetools\");\n+\n+\tfor (@tools) {\n+\t\tmy $tool = $_;\n+\t\tnext if ($tool eq \"defaults\");\n+\n+\t\t$cmd = \"TOOL_MODE=diff\";\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\t$cmd .= \" && can_diff >/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 (sort(@found));\n+\n+\tprint \"\\nThe following tools are valid, but not currently available:\\n\";\n+\tprint \"\\t$_\\n\" for (sort(@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 +167,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.rc2.21.g8cb1a\n"},{"id":"187970","messageId":"7vvclov9hi.fsf@alter.siamese.dyndns.org","threadId":"30084","inReplyTo":"1332959684-2231-1-git-send-email-tim.henigan@gmail.com","subject":"Re: [PATCH 9/9 v8] difftool: print list of valid tools with '--tool-help'","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-03-28T18:58:01Z","receivedAt":"2012-03-28T18:58:01Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Tim Henigan writes:\n\n> Changes in v8:\n> - Replaced 'glob' with 'File::Find'. Glob will fail if file paths include\n> spaces. Using File::Find overcomes that limitation.\n\nOK, but doesn't File::Find recurse into its subdirectories? If you create\na 'foo' directory there and drop a 'bar' script in it, is the rest of the\ncode prepared to give you \"git difftool -t foo/bar\"?\n\n> - Added 'TOOL_MODE=DIFF' prior to calling 'git-mergetool--lib.sh'. This\n> insures that the shell script executes as designed.\n> - difftool now calls 'can_diff' from 'git-mergetool--lib.sh' to insure that\n> only tools that are capable of diffing are shown as valid options. For\n> example, 'tortoisemerge' cannot be used as a diff viewer.\n\nGood that you caught these brown-paper-bag bugs ;-) It feels a bit awkward\nthat nobody pointed these out, even though the series has been queued in\n'pu' since its early iterations.\n\n> diff --git a/git-difftool.perl b/git-difftool.perl\n> index 0fa131c..15fd572 100755\n> --- a/git-difftool.perl\n> +++ b/git-difftool.perl\n> @@ -38,6 +39,40 @@ USAGE\n> \texit($exitcode);\n> }\n> \n> +sub print_tool_help\n> +{\n> +\tmy ($cmd, @found, @notfound, @tools);\n> +\tmy $gitpath = Git::exec_path();\n> +\n> +\tfind(sub { push(@tools, $_) unless (-d $_) }, \"$gitpath/mergetools\");\n> +\n> +\tfor (@tools) {\n> +\t\tmy $tool = $_;\n> +\t\tnext if ($tool eq \"defaults\");\n\nNow you use File::Find::find(), you probably should do this kind of\ntrivial filtering inside the callback, no?\n"},{"id":"187983","messageId":"CAFouetjbq+A04HECXN39KeLBgkTd+HJyxeM0wSDhS2Xo=_gQVQ@mail.gmail.com","threadId":"30084","inReplyTo":"7vvclov9hi.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 9/9 v8] difftool: print list of valid tools with '--tool-help'","fromName":"Tim Henigan","fromEmail":"tim.henigan@gmail.com","sentAt":"2012-03-28T19:48:05Z","receivedAt":"2012-03-28T19:48:05Z","isPatch":true,"sender":{"key":"tim.henigan@gmail.com","avatar":"https://avatars.githubusercontent.com/u/42022?v=4"},"body":"On Wed, Mar 28, 2012 at 2:58 PM, Junio C Hamano wrote:\n> Tim Henigan writes:\n>\n> OK, but doesn't File::Find recurse into its subdirectories?  If you create\n> a 'foo' directory there and drop a 'bar' script in it, is the rest of the\n> code prepared to give you \"git difftool -t foo/bar\"?\n\nIt does recurse, but in this context '$_' only contains the current\nfile name within the directory...not the directory itself [1]. So if\nwe call 'find' on a directory that contains:\n\n foo\n bar/\n baz\n\nthen @tools = ('foo', 'baz')\n\n[1]: http://perldoc.perl.org/File/Find.html#The-wanted-function\n\n\n>> +     for (@tools) {\n>> +             my $tool = $_;\n>> +             next if ($tool eq \"defaults\");\n>\n> Now you use File::Find::find(), you probably should do this kind of\n> trivial filtering inside the callback, no?\n\nI thought about that, but the filter is so simple that it seemed like\noverkill to add another function. If there is another revision of\nthis patch, I will reconsider.\n\nAs always, thanks for the review :)\n"},{"id":"187988","messageId":"7v4nt8v6ip.fsf@alter.siamese.dyndns.org","threadId":"30084","inReplyTo":"CAFouetjbq+A04HECXN39KeLBgkTd+HJyxeM0wSDhS2Xo=_gQVQ@mail.gmail.com","subject":"Re: [PATCH 9/9 v8] difftool: print list of valid tools with '--tool-help'","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-03-28T20:02:06Z","receivedAt":"2012-03-28T20:02:06Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Tim Henigan writes:\n\n> On Wed, Mar 28, 2012 at 2:58 PM, Junio C Hamano wrote:\n>> Tim Henigan writes:\n>>\n>> OK, but doesn't File::Find recurse into its subdirectories?  If you create\n>> a 'foo' directory there and drop a 'bar' script in it, is the rest of the\n>> code prepared to give you \"git difftool -t foo/bar\"?\n>\n> It does recurse, but in this context '$_' only contains the current\n> file name within the directory...not the directory itself [1]. So if\n> we call 'find' on a directory that contains:\n>\n> foo\n> bar/\n> baz\n>\n> then @tools = ('foo', 'baz')\n\nThat is even worse, no? Is the rest of the code prepared to give you \"git\ndifftool -t baz\" in such a layout? What if you have another baz next to\nfoo and bar?\n\nWhat I was hinting at was that you may want to $File::Find::prune=1 when\nyou find a subdirectory. While at it, you may also want to replace\nthe \"unless -d $_\" with \"if -f $_ && -x _\" or something.\n"},{"id":"187990","messageId":"CAFouetiCmmEZDN0ATYE8fQ=CFzA-F+DEWoe-uSNL6Hu_8__rJQ@mail.gmail.com","threadId":"30084","inReplyTo":"7v4nt8v6ip.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 9/9 v8] difftool: print list of valid tools with '--tool-help'","fromName":"Tim Henigan","fromEmail":"tim.henigan@gmail.com","sentAt":"2012-03-28T20:16:46Z","receivedAt":"2012-03-28T20:16:46Z","isPatch":true,"sender":{"key":"tim.henigan@gmail.com","avatar":"https://avatars.githubusercontent.com/u/42022?v=4"},"body":"On Wed, Mar 28, 2012 at 4:02 PM, Junio C Hamano wrote:\n>\n> That is even worse, no?  Is the rest of the code prepared to give you \"git\n> difftool -t baz\" in such a layout?  What if you have another baz next to\n> foo and bar?\n>\n> What I was hinting at was that you may want to $File::Find::prune=1 when\n> you find a subdirectory.  While at it, you may also want to replace\n> the \"unless -d $_\" with \"if -f $_ && -x _\" or something.\n\nI see now...sorry I missed your obvious point. I will look into this further.\n\nThe script makes some assumptions right now:\n 1) All tool config scripts must be located in \"$(git --exec-path)/mergetools\".\n 2) There is only one tool config in each script\n - See my other other recent patch that splits vim/gvim [1]\n 3) The name of the tool script matches the name of the tool itself.\n\nDo these assumptions seem reasonable? If so, perhaps I should add a\nREADME to the mergetools directory that outlines these assumptions?\n\n[1]: http://thread.gmane.org/gmane.comp.version-control.git/194179\n"}]}