{"thread":{"id":"29966","subject":"[PATCH 0/9] difftool: teach command to perform directory diffs","startedAt":"2012-03-17T01:57:13Z","lastAt":"2012-03-20T16:36:15Z","messageCount":26,"participants":["Tim Henigan","Junio C Hamano","David Aguilar"],"isPatch":true,"patchVersion":1,"patchTotal":9},"messages":[{"id":"187116","messageId":"1331949442-15039-1-git-send-email-tim.henigan@gmail.com","threadId":"29966","inReplyTo":null,"subject":"[PATCH 0/9] difftool: teach command to perform directory diffs","fromName":"Tim Henigan","fromEmail":"tim.henigan@gmail.com","sentAt":"2012-03-17T01:57:13Z","receivedAt":"2012-03-17T01:57:13Z","isPatch":true,"sender":{"key":"tim.henigan@gmail.com","avatar":"https://avatars.githubusercontent.com/u/42022?v=4"},"body":"'git difftool' is a very useful command that allows git diffs to be opened\nin an external tool. Currently, difftool opens a separate instance of the\nexternal tool for each file that changed. This can be tedious when many\nfiles have changed.\n\nThis series teaches difftool to perform directory diffs, so that all file\nchanges can be opened/reviewed in a single instance of the external tool.\n\nThis is the second phase of development for this feature. The first phase\nwas added as a separate command (git diffall) in 1252bbe (contrib: add\ngit-diffall script). During review of that script on the Git developers\nlist, an informal development roadmap was suggested [1]. The next phase\nof that plan is to integrate the 'git-diffall' feature into 'difftool'.\nThis series gets that done.\n\n[1]: http://thread.gmane.org/gmane.comp.version-control.git/191297/focus=191383\n\n\nPatches 1-6 are clean-up and rewrite to prepare for later changes. They\nare not intended to change the behavior of difftool. Instead, they teach\nthe script to use standard modules for option parsing and interacting with\nthe Git repo. I understand that these changes may be controversial due to\nthe amount of rewrite, but the end result is that the script is half the\nprevious size and I believe is easier to understand and maintain.\n\nPatches 7 and 8 are the core changes in this series. They teach difftool the\nnew \"--dir-diff\" option. When this option is used, difftool will copy files\nthat were changed to a temporary location and run a single directory diff on\nthem rather than run a separate instance of the diff tool for each file.\n\nPatch 9 is a new implementation of a patch that I submitted recently.\n061e672 (difftool: print list of valid tools with '--tool-help' was written\nprior to these other changes. It needed to be rewritten to fit into the\nGetopt::Long option parsing framework included with this series.\n\nTim Henigan (9):\n  difftool: parse options using Getopt::Long\n  difftool: exit(0) when usage is printed\n  difftool: remove explicit change of PATH\n  difftool: stop appending '.exe' to git\n  difftool: eliminate setup_environment function\n  difftool: replace system call with Git::command_noisy\n  difftool: teach difftool to handle directory diffs\n  difftool: teach dir-diff to copy modified files back to working tree\n  difftool: print list of valid tools with '--tool-help'\n\n Documentation/git-difftool.txt |   17 ++-\n git-difftool--helper.sh        |   20 +++-\n git-difftool.perl              |  257 +++++++++++++++++++++++++++-------------\n 3 files changed, 200 insertions(+), 94 deletions(-)\n\n-- \n1.7.9.1.290.gbd444\n"},{"id":"187119","messageId":"1331949442-15039-2-git-send-email-tim.henigan@gmail.com","threadId":"29966","inReplyTo":"1331949442-15039-1-git-send-email-tim.henigan@gmail.com","subject":"[PATCH 1/9] difftool: parse options using Getopt::Long","fromName":"Tim Henigan","fromEmail":"tim.henigan@gmail.com","sentAt":"2012-03-17T01:57:14Z","receivedAt":"2012-03-17T01:57:14Z","isPatch":true,"sender":{"key":"tim.henigan@gmail.com","avatar":"https://avatars.githubusercontent.com/u/42022?v=4"},"body":"Replace custom option/argument parser with standard Getopt::Long\nmodule.  This shortens the code and makes it easier to understand.\n\nSigned-off-by: Tim Henigan <tim.henigan@gmail.com>\n---\n git-difftool.perl |  109 +++++++++++++++++++++--------------------------------\n 1 file changed, 44 insertions(+), 65 deletions(-)\n\ndiff --git a/git-difftool.perl b/git-difftool.perl\nindex 09b65f1..e365727 100755\n--- a/git-difftool.perl\n+++ b/git-difftool.perl\n@@ -15,11 +15,8 @@ use strict;\n use warnings;\n use Cwd qw(abs_path);\n use File::Basename qw(dirname);\n-\n-require Git;\n-\n-my $DIR = abs_path(dirname($0));\n-\n+use Getopt::Long qw(:config pass_through);\n+use Git;\n \n sub usage\n {\n@@ -33,6 +30,7 @@ USAGE\n \n sub setup_environment\n {\n+\tmy $DIR = abs_path(dirname($0));\n \t$ENV{PATH} = \"$DIR:$ENV{PATH}\";\n \t$ENV{GIT_PAGER} = '';\n \t$ENV{GIT_EXTERNAL_DIFF} = 'git-difftool--helper';\n@@ -47,75 +45,56 @@ sub exe\n \treturn $exe;\n }\n \n-sub generate_command\n-{\n-\tmy @command = (exe('git'), 'diff');\n-\tmy $skip_next = 0;\n-\tmy $idx = -1;\n-\tmy $prompt = '';\n-\tfor my $arg (@ARGV) {\n-\t\t$idx++;\n-\t\tif ($skip_next) {\n-\t\t\t$skip_next = 0;\n-\t\t\tnext;\n-\t\t}\n-\t\tif ($arg eq '-t' || $arg eq '--tool') {\n-\t\t\tusage() if $#ARGV <= $idx;\n-\t\t\t$ENV{GIT_DIFF_TOOL} = $ARGV[$idx + 1];\n-\t\t\t$skip_next = 1;\n-\t\t\tnext;\n-\t\t}\n-\t\tif ($arg =~ /^--tool=/) {\n-\t\t\t$ENV{GIT_DIFF_TOOL} = substr($arg, 7);\n-\t\t\tnext;\n-\t\t}\n-\t\tif ($arg eq '-x' || $arg eq '--extcmd') {\n-\t\t\tusage() if $#ARGV <= $idx;\n-\t\t\t$ENV{GIT_DIFFTOOL_EXTCMD} = $ARGV[$idx + 1];\n-\t\t\t$skip_next = 1;\n-\t\t\tnext;\n-\t\t}\n-\t\tif ($arg =~ /^--extcmd=/) {\n-\t\t\t$ENV{GIT_DIFFTOOL_EXTCMD} = substr($arg, 9);\n-\t\t\tnext;\n-\t\t}\n-\t\tif ($arg eq '-g' || $arg eq '--gui') {\n-\t\t\teval {\n-\t\t\t\tmy $tool = Git::command_oneline('config',\n-\t\t\t\t                                'diff.guitool');\n-\t\t\t\tif (length($tool)) {\n-\t\t\t\t\t$ENV{GIT_DIFF_TOOL} = $tool;\n-\t\t\t\t}\n-\t\t\t};\n-\t\t\tnext;\n-\t\t}\n-\t\tif ($arg eq '-y' || $arg eq '--no-prompt') {\n-\t\t\t$prompt = 'no';\n-\t\t\tnext;\n-\t\t}\n-\t\tif ($arg eq '--prompt') {\n-\t\t\t$prompt = 'yes';\n-\t\t\tnext;\n-\t\t}\n-\t\tif ($arg eq '-h') {\n-\t\t\tusage();\n-\t\t}\n-\t\tpush @command, $arg;\n+# parse command-line options. all unrecognized options and arguments\n+# are passed through to the 'git diff' command.\n+my ($difftool_cmd, $extcmd, $gui, $help, $no_prompt, $prompt);\n+GetOptions('g|gui' => \\$gui,\n+\t'h' => \\$help,\n+\t'prompt' => \\$prompt,\n+\t't|tool:s' => \\$difftool_cmd,\n+\t'x|extcmd:s' => \\$extcmd,\n+\t'y|no-prompt' => \\$no_prompt);\n+\n+if (defined($help)) {\n+\tusage();\n+} \n+if (defined($difftool_cmd)) {\n+\tif (length($difftool_cmd) > 0) {\n+\t\t$ENV{GIT_DIFF_TOOL} = $difftool_cmd;\n+\t} else {\n+\t\tprint \"No <tool> given for --tool=<tool>\\n\";\n+\t\tusage();\n \t}\n-\tif ($prompt eq 'yes') {\n-\t\t$ENV{GIT_DIFFTOOL_PROMPT} = 'true';\n-\t} elsif ($prompt eq 'no') {\n-\t\t$ENV{GIT_DIFFTOOL_NO_PROMPT} = 'true';\n+}\n+if (defined($extcmd)) {\n+\tif (length($extcmd) > 0) {\n+\t\t$ENV{GIT_DIFFTOOL_EXTCMD} = $extcmd;\n+\t} else {\n+\t\tprint \"No <cmd> given for --extcmd=<cmd>\\n\";\n+\t\tusage();\n+\t}\n+}\n+if (defined($gui)) {\n+\tmy $guitool = \"\";\n+\t$guitool = Git::config('diff.guitool');\n+\tif (length($guitool) > 0) {\n+\t\t$ENV{GIT_DIFF_TOOL} = $guitool;\n \t}\n-\treturn @command\n+}\n+if (defined($prompt)) {\n+\t$ENV{GIT_DIFFTOOL_PROMPT} = 'true';\n+}\n+elsif (defined($no_prompt)) {\n+\t$ENV{GIT_DIFFTOOL_NO_PROMPT} = 'true';\n }\n \n setup_environment();\n+my @command = (exe('git'), 'diff', @ARGV);\n \n # ActiveState Perl for Win32 does not implement POSIX semantics of\n # exec* system call. It just spawns the given executable and finishes\n # the starting program, exiting with code 0.\n # system will at least catch the errors returned by git diff,\n # allowing the caller of git difftool better handling of failures.\n-my $rc = system(generate_command());\n+my $rc = system(@command);\n exit($rc | ($rc >> 8));\n-- \n1.7.9.1.290.gbd444\n"},{"id":"187117","messageId":"1331949442-15039-3-git-send-email-tim.henigan@gmail.com","threadId":"29966","inReplyTo":"1331949442-15039-1-git-send-email-tim.henigan@gmail.com","subject":"[PATCH 2/9] difftool: exit(0) when usage is printed","fromName":"Tim Henigan","fromEmail":"tim.henigan@gmail.com","sentAt":"2012-03-17T01:57:15Z","receivedAt":"2012-03-17T01:57:15Z","isPatch":true,"sender":{"key":"tim.henigan@gmail.com","avatar":"https://avatars.githubusercontent.com/u/42022?v=4"},"body":"Prior to this commit, the script exited with an error whenever the\nusage string was printed, regardless of the reason it was done. In\ncases where usage was printed due to a user request (e.g. '-h'\noption), the script should exit without error (exit 0).\n\nThis commit adds an argument to the usage function that allows the\nexit code to be specified when the function is called.\n\nSigned-off-by: Tim Henigan <tim.henigan@gmail.com>\n---\n git-difftool.perl |    9 +++++----\n 1 file changed, 5 insertions(+), 4 deletions(-)\n\ndiff --git a/git-difftool.perl b/git-difftool.perl\nindex e365727..81ecf34 100755\n--- a/git-difftool.perl\n+++ b/git-difftool.perl\n@@ -20,12 +20,13 @@ use Git;\n \n sub usage\n {\n+\tmy $exitcode = shift;\n \tprint << 'USAGE';\n usage: git difftool [-t|--tool=<tool>] [-x|--extcmd=<cmd>]\n                     [-y|--no-prompt]   [-g|--gui]\n                     ['git diff' options]\n USAGE\n-\texit 1;\n+\texit($exitcode);\n }\n \n sub setup_environment\n@@ -56,14 +57,14 @@ GetOptions('g|gui' => \\$gui,\n \t'y|no-prompt' => \\$no_prompt);\n \n if (defined($help)) {\n-\tusage();\n+\tusage(0);\n } \n if (defined($difftool_cmd)) {\n \tif (length($difftool_cmd) > 0) {\n \t\t$ENV{GIT_DIFF_TOOL} = $difftool_cmd;\n \t} else {\n \t\tprint \"No <tool> given for --tool=<tool>\\n\";\n-\t\tusage();\n+\t\tusage(1);\n \t}\n }\n if (defined($extcmd)) {\n@@ -71,7 +72,7 @@ if (defined($extcmd)) {\n \t\t$ENV{GIT_DIFFTOOL_EXTCMD} = $extcmd;\n \t} else {\n \t\tprint \"No <cmd> given for --extcmd=<cmd>\\n\";\n-\t\tusage();\n+\t\tusage(1);\n \t}\n }\n if (defined($gui)) {\n-- \n1.7.9.1.290.gbd444\n"},{"id":"187118","messageId":"1331949442-15039-4-git-send-email-tim.henigan@gmail.com","threadId":"29966","inReplyTo":"1331949442-15039-1-git-send-email-tim.henigan@gmail.com","subject":"[PATCH 3/9] difftool: remove explicit change of PATH","fromName":"Tim Henigan","fromEmail":"tim.henigan@gmail.com","sentAt":"2012-03-17T01:57:16Z","receivedAt":"2012-03-17T01:57:16Z","isPatch":true,"sender":{"key":"tim.henigan@gmail.com","avatar":"https://avatars.githubusercontent.com/u/42022?v=4"},"body":"Adding the script directory to PATH is not needed. The script is\nlocated at '$(git --exec-path)', which is already on the PATH.\n\nSigned-off-by: Tim Henigan <tim.henigan@gmail.com>\n---\n git-difftool.perl |    4 ----\n 1 file changed, 4 deletions(-)\n\ndiff --git a/git-difftool.perl b/git-difftool.perl\nindex 81ecf34..53fcd7e 100755\n--- a/git-difftool.perl\n+++ b/git-difftool.perl\n@@ -13,8 +13,6 @@\n use 5.008;\n use strict;\n use warnings;\n-use Cwd qw(abs_path);\n-use File::Basename qw(dirname);\n use Getopt::Long qw(:config pass_through);\n use Git;\n \n@@ -31,8 +29,6 @@ USAGE\n \n sub setup_environment\n {\n-\tmy $DIR = abs_path(dirname($0));\n-\t$ENV{PATH} = \"$DIR:$ENV{PATH}\";\n \t$ENV{GIT_PAGER} = '';\n \t$ENV{GIT_EXTERNAL_DIFF} = 'git-difftool--helper';\n }\n-- \n1.7.9.1.290.gbd444\n"},{"id":"187120","messageId":"1331949442-15039-5-git-send-email-tim.henigan@gmail.com","threadId":"29966","inReplyTo":"1331949442-15039-1-git-send-email-tim.henigan@gmail.com","subject":"[PATCH 4/9] difftool: stop appending '.exe' to git","fromName":"Tim Henigan","fromEmail":"tim.henigan@gmail.com","sentAt":"2012-03-17T01:57:17Z","receivedAt":"2012-03-17T01:57:17Z","isPatch":true,"sender":{"key":"tim.henigan@gmail.com","avatar":"https://avatars.githubusercontent.com/u/42022?v=4"},"body":"The system call to Git works the same whether or not \".exe\" is\nappended to \"git\". The extra code is not necessary.\n\nSigned-off-by: Tim Henigan <tim.henigan@gmail.com>\n---\n git-difftool.perl |   11 +----------\n 1 file changed, 1 insertion(+), 10 deletions(-)\n\ndiff --git a/git-difftool.perl b/git-difftool.perl\nindex 53fcd7e..99ff587 100755\n--- a/git-difftool.perl\n+++ b/git-difftool.perl\n@@ -33,15 +33,6 @@ sub setup_environment\n \t$ENV{GIT_EXTERNAL_DIFF} = 'git-difftool--helper';\n }\n \n-sub exe\n-{\n-\tmy $exe = shift;\n-\tif ($^O eq 'MSWin32' || $^O eq 'msys') {\n-\t\treturn \"$exe.exe\";\n-\t}\n-\treturn $exe;\n-}\n-\n # parse command-line options. all unrecognized options and arguments\n # are passed through to the 'git diff' command.\n my ($difftool_cmd, $extcmd, $gui, $help, $no_prompt, $prompt);\n@@ -86,7 +77,7 @@ elsif (defined($no_prompt)) {\n }\n \n setup_environment();\n-my @command = (exe('git'), 'diff', @ARGV);\n+my @command = ('git', 'diff', @ARGV);\n \n # ActiveState Perl for Win32 does not implement POSIX semantics of\n # exec* system call. It just spawns the given executable and finishes\n-- \n1.7.9.1.290.gbd444\n"},{"id":"187126","messageId":"7vlin0dlrm.fsf@alter.siamese.dyndns.org","threadId":"29966","inReplyTo":"1331949442-15039-1-git-send-email-tim.henigan@gmail.com","subject":"Re: [PATCH 0/9] difftool: teach command to perform directory diffs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-03-17T02:06:05Z","receivedAt":"2012-03-17T02:06:05Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Tim Henigan <tim.henigan@gmail.com> writes:\n\n> 'git difftool' is a very useful command that allows git diffs to be opened\n> in an external tool. Currently, difftool opens a separate instance of the\n> external tool for each file that changed. This can be tedious when many\n> files have changed.\n>\n> This series teaches difftool to perform directory diffs, so that all file\n> changes can be opened/reviewed in a single instance of the external tool.\n>\n> This is the second phase of development for this feature. The first phase\n> was added as a separate command (git diffall) in 1252bbe (contrib: add\n> git-diffall script).\n\nUmmm.  I do not think the first step is not even done yet; fixing the\nwhitespace-in-pathspec and whitespace-or-lf-in-paths issues now the script\nis in contrib/, while having people play with it.\n"},{"id":"187127","messageId":"CAFouetgqsLv9HUYiUXe8aw+NQ-i=k+OGosPRNKSrqePf5wUnQA@mail.gmail.com","threadId":"29966","inReplyTo":"7vlin0dlrm.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 0/9] difftool: teach command to perform directory diffs","fromName":"Tim Henigan","fromEmail":"tim.henigan@gmail.com","sentAt":"2012-03-17T02:26:11Z","receivedAt":"2012-03-17T02:26:11Z","isPatch":true,"sender":{"key":"tim.henigan@gmail.com","avatar":"https://avatars.githubusercontent.com/u/42022?v=4"},"body":"On Fri, Mar 16, 2012 at 10:06 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> Tim Henigan <tim.henigan@gmail.com> writes:\n>\n>> This is the second phase of development for this feature. The first phase\n>> was added as a separate command (git diffall) in 1252bbe (contrib: add\n>> git-diffall script).\n>\n> Ummm.  I do not think the first step is not even done yet; fixing the\n> whitespace-in-pathspec and whitespace-or-lf-in-paths issues now the script\n> is in contrib/, while having people play with it.\n\nI began fixing those issues in 'git-diffall' by rewriting the loop\nthat populates the tmp dirs in Perl.  By the time I changed the loop\nto use 'git diff --raw -z' and handle working copy changes and\nsubmodules, it seemed I was very near to finishing the things I wanted\nto accomplish in \"phase 2\".  Finishing the work by updating 'difftool'\nseemed to actually be easier than trying to update 'diffall'.\n\nI realize the feature hasn't had any time to cook in\ncontrib/...perhaps I am over-enthusiastic.  However, the 'diffall'\nconcept has been in steady use by a small audience for nearly two\nyears (by people who downloaded/forked the GitHub project).  This past\nweek I was lucky enough to have some time to study the problem and\npropose a solution.  If it needs to wait while 'diffall' cooks in\ncontrib, I will try to be more patient ;)\n\nThe changes in patches 1-6 may be useful regardless.  They cut the\nlength of the script in half without changing how it behaves.  The\nonly functional change in those patches is that `git difftool -h` now\nfinishes with exit(0) instead of exit(1).  I understand that may not\nbe enough reason to accept such a large change in working code...but\nlike I said, I was lucky enough to have time to study the problem in\ndetail this week.\n"},{"id":"187130","messageId":"CAJDDKr7BTz-2THw1JaJEDcK1G4Uxwc88=gFsHCSONHAtqtypRw@mail.gmail.com","threadId":"29966","inReplyTo":"1331949442-15039-2-git-send-email-tim.henigan@gmail.com","subject":"Re: [PATCH 1/9] difftool: parse options using Getopt::Long","fromName":"David Aguilar","fromEmail":"davvid@gmail.com","sentAt":"2012-03-17T03:21:31Z","receivedAt":"2012-03-17T03:21:31Z","isPatch":true,"sender":{"key":"davvid@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13196?v=4"},"body":"On Fri, Mar 16, 2012 at 6:57 PM, Tim Henigan <tim.henigan@gmail.com> wrote:\n> Replace custom option/argument parser with standard Getopt::Long\n> module.  This shortens the code and makes it easier to understand.\n\nI've also wanted to do the same in the past.  The one thing holding me\nback was this note from the perldocs:\n\n\"If pass_through is also enabled, options processing will terminate at\nthe first unrecognized option, or non-option, whichever comes first.\"\n\nhttp://search.cpan.org/~jv/Getopt-Long-2.38/lib/Getopt/Long.pm\n\nThat leads me to believe that using Getopt::Long would break this use case:\n\n    $ git difftool --cached --tool xxdiff ...\n\nAccording to the docs, it will stop at --cached and leave it (and the\nrest) in @ARGV.  When git-diff runs it will see the --tool argument\nand bail out.\n\nIs this indeed the case?  I am a little ashamed that the difftool\ntests do not cover this area.  That would be a valuable first step\ntowards exploring this approach, IMO.\n\n\nBTW, I hate @ARGV parsing loops just as much as anyone!  I was not\nignorant of Getopt::Long, and no, I was not re-inventing the wheel for\nno reason.  The reason it was done that way was so that we can forward\neverything we don't know about to git-diff.\n\nI haven't tried this patch, but my reading of the documentation leads\nme to believe that this is a regression.\n\n\n\n> Signed-off-by: Tim Henigan <tim.henigan@gmail.com>\n> ---\n>  git-difftool.perl |  109 +++++++++++++++++++++--------------------------------\n>  1 file changed, 44 insertions(+), 65 deletions(-)\n>\n> diff --git a/git-difftool.perl b/git-difftool.perl\n> index 09b65f1..e365727 100755\n> --- a/git-difftool.perl\n> +++ b/git-difftool.perl\n> @@ -15,11 +15,8 @@ use strict;\n>  use warnings;\n>  use Cwd qw(abs_path);\n>  use File::Basename qw(dirname);\n> -\n> -require Git;\n> -\n> -my $DIR = abs_path(dirname($0));\n> -\n> +use Getopt::Long qw(:config pass_through);\n> +use Git;\n>\n>  sub usage\n>  {\n> @@ -33,6 +30,7 @@ USAGE\n>\n>  sub setup_environment\n>  {\n> +       my $DIR = abs_path(dirname($0));\n>        $ENV{PATH} = \"$DIR:$ENV{PATH}\";\n>        $ENV{GIT_PAGER} = '';\n>        $ENV{GIT_EXTERNAL_DIFF} = 'git-difftool--helper';\n> @@ -47,75 +45,56 @@ sub exe\n>        return $exe;\n>  }\n>\n> -sub generate_command\n> -{\n> -       my @command = (exe('git'), 'diff');\n> -       my $skip_next = 0;\n> -       my $idx = -1;\n> -       my $prompt = '';\n> -       for my $arg (@ARGV) {\n> -               $idx++;\n> -               if ($skip_next) {\n> -                       $skip_next = 0;\n> -                       next;\n> -               }\n> -               if ($arg eq '-t' || $arg eq '--tool') {\n> -                       usage() if $#ARGV <= $idx;\n> -                       $ENV{GIT_DIFF_TOOL} = $ARGV[$idx + 1];\n> -                       $skip_next = 1;\n> -                       next;\n> -               }\n> -               if ($arg =~ /^--tool=/) {\n> -                       $ENV{GIT_DIFF_TOOL} = substr($arg, 7);\n> -                       next;\n> -               }\n> -               if ($arg eq '-x' || $arg eq '--extcmd') {\n> -                       usage() if $#ARGV <= $idx;\n> -                       $ENV{GIT_DIFFTOOL_EXTCMD} = $ARGV[$idx + 1];\n> -                       $skip_next = 1;\n> -                       next;\n> -               }\n> -               if ($arg =~ /^--extcmd=/) {\n> -                       $ENV{GIT_DIFFTOOL_EXTCMD} = substr($arg, 9);\n> -                       next;\n> -               }\n> -               if ($arg eq '-g' || $arg eq '--gui') {\n> -                       eval {\n> -                               my $tool = Git::command_oneline('config',\n> -                                                               'diff.guitool');\n> -                               if (length($tool)) {\n> -                                       $ENV{GIT_DIFF_TOOL} = $tool;\n> -                               }\n> -                       };\n> -                       next;\n> -               }\n> -               if ($arg eq '-y' || $arg eq '--no-prompt') {\n> -                       $prompt = 'no';\n> -                       next;\n> -               }\n> -               if ($arg eq '--prompt') {\n> -                       $prompt = 'yes';\n> -                       next;\n> -               }\n> -               if ($arg eq '-h') {\n> -                       usage();\n> -               }\n> -               push @command, $arg;\n> +# parse command-line options. all unrecognized options and arguments\n> +# are passed through to the 'git diff' command.\n> +my ($difftool_cmd, $extcmd, $gui, $help, $no_prompt, $prompt);\n> +GetOptions('g|gui' => \\$gui,\n> +       'h' => \\$help,\n> +       'prompt' => \\$prompt,\n> +       't|tool:s' => \\$difftool_cmd,\n> +       'x|extcmd:s' => \\$extcmd,\n> +       'y|no-prompt' => \\$no_prompt);\n> +\n> +if (defined($help)) {\n> +       usage();\n> +}\n> +if (defined($difftool_cmd)) {\n> +       if (length($difftool_cmd) > 0) {\n> +               $ENV{GIT_DIFF_TOOL} = $difftool_cmd;\n> +       } else {\n> +               print \"No <tool> given for --tool=<tool>\\n\";\n> +               usage();\n>        }\n> -       if ($prompt eq 'yes') {\n> -               $ENV{GIT_DIFFTOOL_PROMPT} = 'true';\n> -       } elsif ($prompt eq 'no') {\n> -               $ENV{GIT_DIFFTOOL_NO_PROMPT} = 'true';\n> +}\n> +if (defined($extcmd)) {\n> +       if (length($extcmd) > 0) {\n> +               $ENV{GIT_DIFFTOOL_EXTCMD} = $extcmd;\n> +       } else {\n> +               print \"No <cmd> given for --extcmd=<cmd>\\n\";\n> +               usage();\n> +       }\n> +}\n> +if (defined($gui)) {\n> +       my $guitool = \"\";\n> +       $guitool = Git::config('diff.guitool');\n> +       if (length($guitool) > 0) {\n> +               $ENV{GIT_DIFF_TOOL} = $guitool;\n>        }\n> -       return @command\n> +}\n> +if (defined($prompt)) {\n> +       $ENV{GIT_DIFFTOOL_PROMPT} = 'true';\n> +}\n> +elsif (defined($no_prompt)) {\n> +       $ENV{GIT_DIFFTOOL_NO_PROMPT} = 'true';\n>  }\n>\n>  setup_environment();\n> +my @command = (exe('git'), 'diff', @ARGV);\n>\n>  # ActiveState Perl for Win32 does not implement POSIX semantics of\n>  # exec* system call. It just spawns the given executable and finishes\n>  # the starting program, exiting with code 0.\n>  # system will at least catch the errors returned by git diff,\n>  # allowing the caller of git difftool better handling of failures.\n> -my $rc = system(generate_command());\n> +my $rc = system(@command);\n>  exit($rc | ($rc >> 8));\n> --\n> 1.7.9.1.290.gbd444\n>\n\n\n\n-- \nDavid\n"},{"id":"187133","messageId":"7vehsretww.fsf@alter.siamese.dyndns.org","threadId":"29966","inReplyTo":"CAFouetgqsLv9HUYiUXe8aw+NQ-i=k+OGosPRNKSrqePf5wUnQA@mail.gmail.com","subject":"Re: [PATCH 0/9] difftool: teach command to perform directory diffs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-03-17T04:24:47Z","receivedAt":"2012-03-17T04:24:47Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Tim Henigan <tim.henigan@gmail.com> writes:\n\n> I began fixing those issues in 'git-diffall' by rewriting the loop\n> that populates the tmp dirs in Perl.  By the time I changed the loop\n> to use 'git diff --raw -z' and handle working copy changes and\n> submodules, it seemed I was very near to finishing the things I wanted\n> to accomplish in \"phase 2\".  Finishing the work by updating 'difftool'\n> seemed to actually be easier than trying to update 'diffall'.\n\n;-)  Good.\n"},{"id":"187150","messageId":"CAFouetgpz__1q7F_a3LQoeypCVBXOyTPWmL0e9=rw7vV65d7_g@mail.gmail.com","threadId":"29966","inReplyTo":"CAJDDKr7BTz-2THw1JaJEDcK1G4Uxwc88=gFsHCSONHAtqtypRw@mail.gmail.com","subject":"Re: [PATCH 1/9] difftool: parse options using Getopt::Long","fromName":"Tim Henigan","fromEmail":"tim.henigan@gmail.com","sentAt":"2012-03-17T13:54:09Z","receivedAt":"2012-03-17T13:54:09Z","isPatch":true,"sender":{"key":"tim.henigan@gmail.com","avatar":"https://avatars.githubusercontent.com/u/42022?v=4"},"body":"On Fri, Mar 16, 2012 at 11:21 PM, David Aguilar <davvid@gmail.com> wrote:\n> On Fri, Mar 16, 2012 at 6:57 PM, Tim Henigan <tim.henigan@gmail.com> wrote:\n>\n> I've also wanted to do the same in the past.  The one thing holding me\n> back was this note from the perldocs:\n>\n> \"If pass_through is also enabled, options processing will terminate at\n> the first unrecognized option, or non-option, whichever comes first.\"\n>\n> http://search.cpan.org/~jv/Getopt-Long-2.38/lib/Getopt/Long.pm\n\nThat sentence is listed under the documentation of the 'permute'\noption.  The documentation of 'pass_through' states that:\n\n\"Options that are unknown, ambiguous or supplied with an invalid\noption value are passed through in @ARGV instead of being flagged as\nerrors. This makes it possible to write wrapper scripts that process\nonly part of the user supplied command line arguments, and pass the\nremaining options to some other program.\n\nIf require_order is enabled, options processing will terminate at the\nfirst unrecognized option, or non-option, whichever comes first.\nHowever, if permute is enabled instead, results can become confusing.\"\n\nSo early termination would only be a problem if 'pass_through' was\nenabled at the same time as 'require_order' or 'permute'.  To verify,\nI confirmed that 'git difftool --cached --diff-dir' works as expected.\n\n\n> Is this indeed the case?  I am a little ashamed that the difftool\n> tests do not cover this area.  That would be a valuable first step\n> towards exploring this approach, IMO.\n\nI will review the the test cases.  If this goes forward, I still need\nto add test cases to confirm the new '--dir-diff' option.\n\n\n> BTW, I hate @ARGV parsing loops just as much as anyone!  I was not\n> ignorant of Getopt::Long, and no, I was not re-inventing the wheel for\n> no reason.  The reason it was done that way was so that we can forward\n> everything we don't know about to git-diff.\n\nI understand and was not implying anything different.  I simply\nthought this would be a positive change.\n"},{"id":"187181","messageId":"1332035734-5443-1-git-send-email-tim.henigan@gmail.com","threadId":"29966","inReplyTo":"1331949442-15039-1-git-send-email-tim.henigan@gmail.com","subject":"[PATCH 0/9 v2] difftool: teach command to perform directory diffs","fromName":"Tim Henigan","fromEmail":"tim.henigan@gmail.com","sentAt":"2012-03-18T01:55:25Z","receivedAt":"2012-03-18T01:55:25Z","isPatch":true,"sender":{"key":"tim.henigan@gmail.com","avatar":"https://avatars.githubusercontent.com/u/42022?v=4"},"body":"Changes in v2:\n  [PATCH 7/9] difftool: teach difftool to handle directory diffs\n    - Location of /tmp is now determined via the File::Temp::tempdir. In\n      v1, the location was hard-coded to '/tmp'.\n    - Lexical filehandles are now used for open().\n    - Modern, 3 argument, form of open() is now used.\n    - Arguments to the 'system' call for $extcmd are now given in list form.\n    - Replaced 'system' call to 'git difftool--helper' with Git::command_noisy\n\n  [PATCH 9/9] difftool: print list of valid tools with '--tool-help'\n    - The glob statement used to find the mergetool options now uses the\n      fully qualified path.  This insures that 'git difftool --tool-help'\n      can be called from any directory.\n\nSeries overview:\n\n'git difftool' is a very useful command that allows git diffs to be opened\nin an external tool. Currently, difftool opens a separate instance of the\nexternal tool for each file that changed. This can be tedious when many\nfiles have changed.\n\nThis series teaches difftool to perform directory diffs, so that all file\nchanges can be opened/reviewed in a single instance of the external tool.\n\nThis is the second phase of development for this feature. The first phase\nwas added as a separate command (git diffall) in 1252bbe (contrib: add\ngit-diffall script). During review of that script on the Git developers\nlist, an informal development roadmap was suggested [1]. The next phase\nof that plan is to integrate the 'git-diffall' feature into 'difftool'.\nThis series gets that done.\n\n[1]: http://thread.gmane.org/gmane.comp.version-control.git/191297/focus=191383\n\n\nPatches 1-6 are clean-up and rewrite to prepare for later changes. They\nare not intended to change the behavior of difftool. Instead, they teach\nthe script to use standard modules for option parsing and interacting with\nthe Git repo. I understand that these changes may be controversial due to\nthe amount of rewrite, but the end result is that the script is half the\nprevious size and I believe is easier to understand and maintain.\n\nPatches 7 and 8 are the core changes in this series. They teach difftool the\nnew \"--dir-diff\" option. When this option is used, difftool will copy files\nthat were changed to a temporary location and run a single directory diff on\nthem rather than run a separate instance of the diff tool for each file.\n\nPatch 9 is a new implementation of a patch that I submitted recently.\n061e672 (difftool: print list of valid tools with '--tool-help' was written\nprior to these other changes. It needed to be rewritten to fit into the\nGetopt::Long option parsing framework included with this series.\n\nTim Henigan (9):\n  difftool: parse options using Getopt::Long\n  difftool: exit(0) when usage is printed\n  difftool: remove explicit change of PATH\n  difftool: stop appending '.exe' to git\n  difftool: eliminate setup_environment function\n  difftool: replace system call with Git::command_noisy\n  difftool: teach difftool to handle directory diffs\n  difftool: teach dir-diff to copy modified files back to working tree\n  difftool: print list of valid tools with '--tool-help'\n\n Documentation/git-difftool.txt |   17 ++-\n git-difftool--helper.sh        |   20 +++-\n git-difftool.perl              |  259 +++++++++++++++++++++++++++-------------\n 3 files changed, 202 insertions(+), 94 deletions(-)\n\n-- \n1.7.9.1.290.gbd444\n"},{"id":"187190","messageId":"1332035734-5443-2-git-send-email-tim.henigan@gmail.com","threadId":"29966","inReplyTo":"1332035734-5443-1-git-send-email-tim.henigan@gmail.com","subject":"[PATCH 1/9 v2] difftool: parse options using Getopt::Long","fromName":"Tim Henigan","fromEmail":"tim.henigan@gmail.com","sentAt":"2012-03-18T01:55:26Z","receivedAt":"2012-03-18T01:55:26Z","isPatch":true,"sender":{"key":"tim.henigan@gmail.com","avatar":"https://avatars.githubusercontent.com/u/42022?v=4"},"body":"Replace custom option/argument parser with standard Getopt::Long\nmodule.  This shortens the code and makes it easier to understand.\n\nSigned-off-by: Tim Henigan <tim.henigan@gmail.com>\n---\n git-difftool.perl |  109 +++++++++++++++++++++--------------------------------\n 1 file changed, 44 insertions(+), 65 deletions(-)\n\ndiff --git a/git-difftool.perl b/git-difftool.perl\nindex 09b65f1..e365727 100755\n--- a/git-difftool.perl\n+++ b/git-difftool.perl\n@@ -15,11 +15,8 @@ use strict;\n use warnings;\n use Cwd qw(abs_path);\n use File::Basename qw(dirname);\n-\n-require Git;\n-\n-my $DIR = abs_path(dirname($0));\n-\n+use Getopt::Long qw(:config pass_through);\n+use Git;\n \n sub usage\n {\n@@ -33,6 +30,7 @@ USAGE\n \n sub setup_environment\n {\n+\tmy $DIR = abs_path(dirname($0));\n \t$ENV{PATH} = \"$DIR:$ENV{PATH}\";\n \t$ENV{GIT_PAGER} = '';\n \t$ENV{GIT_EXTERNAL_DIFF} = 'git-difftool--helper';\n@@ -47,75 +45,56 @@ sub exe\n \treturn $exe;\n }\n \n-sub generate_command\n-{\n-\tmy @command = (exe('git'), 'diff');\n-\tmy $skip_next = 0;\n-\tmy $idx = -1;\n-\tmy $prompt = '';\n-\tfor my $arg (@ARGV) {\n-\t\t$idx++;\n-\t\tif ($skip_next) {\n-\t\t\t$skip_next = 0;\n-\t\t\tnext;\n-\t\t}\n-\t\tif ($arg eq '-t' || $arg eq '--tool') {\n-\t\t\tusage() if $#ARGV <= $idx;\n-\t\t\t$ENV{GIT_DIFF_TOOL} = $ARGV[$idx + 1];\n-\t\t\t$skip_next = 1;\n-\t\t\tnext;\n-\t\t}\n-\t\tif ($arg =~ /^--tool=/) {\n-\t\t\t$ENV{GIT_DIFF_TOOL} = substr($arg, 7);\n-\t\t\tnext;\n-\t\t}\n-\t\tif ($arg eq '-x' || $arg eq '--extcmd') {\n-\t\t\tusage() if $#ARGV <= $idx;\n-\t\t\t$ENV{GIT_DIFFTOOL_EXTCMD} = $ARGV[$idx + 1];\n-\t\t\t$skip_next = 1;\n-\t\t\tnext;\n-\t\t}\n-\t\tif ($arg =~ /^--extcmd=/) {\n-\t\t\t$ENV{GIT_DIFFTOOL_EXTCMD} = substr($arg, 9);\n-\t\t\tnext;\n-\t\t}\n-\t\tif ($arg eq '-g' || $arg eq '--gui') {\n-\t\t\teval {\n-\t\t\t\tmy $tool = Git::command_oneline('config',\n-\t\t\t\t                                'diff.guitool');\n-\t\t\t\tif (length($tool)) {\n-\t\t\t\t\t$ENV{GIT_DIFF_TOOL} = $tool;\n-\t\t\t\t}\n-\t\t\t};\n-\t\t\tnext;\n-\t\t}\n-\t\tif ($arg eq '-y' || $arg eq '--no-prompt') {\n-\t\t\t$prompt = 'no';\n-\t\t\tnext;\n-\t\t}\n-\t\tif ($arg eq '--prompt') {\n-\t\t\t$prompt = 'yes';\n-\t\t\tnext;\n-\t\t}\n-\t\tif ($arg eq '-h') {\n-\t\t\tusage();\n-\t\t}\n-\t\tpush @command, $arg;\n+# parse command-line options. all unrecognized options and arguments\n+# are passed through to the 'git diff' command.\n+my ($difftool_cmd, $extcmd, $gui, $help, $no_prompt, $prompt);\n+GetOptions('g|gui' => \\$gui,\n+\t'h' => \\$help,\n+\t'prompt' => \\$prompt,\n+\t't|tool:s' => \\$difftool_cmd,\n+\t'x|extcmd:s' => \\$extcmd,\n+\t'y|no-prompt' => \\$no_prompt);\n+\n+if (defined($help)) {\n+\tusage();\n+} \n+if (defined($difftool_cmd)) {\n+\tif (length($difftool_cmd) > 0) {\n+\t\t$ENV{GIT_DIFF_TOOL} = $difftool_cmd;\n+\t} else {\n+\t\tprint \"No <tool> given for --tool=<tool>\\n\";\n+\t\tusage();\n \t}\n-\tif ($prompt eq 'yes') {\n-\t\t$ENV{GIT_DIFFTOOL_PROMPT} = 'true';\n-\t} elsif ($prompt eq 'no') {\n-\t\t$ENV{GIT_DIFFTOOL_NO_PROMPT} = 'true';\n+}\n+if (defined($extcmd)) {\n+\tif (length($extcmd) > 0) {\n+\t\t$ENV{GIT_DIFFTOOL_EXTCMD} = $extcmd;\n+\t} else {\n+\t\tprint \"No <cmd> given for --extcmd=<cmd>\\n\";\n+\t\tusage();\n+\t}\n+}\n+if (defined($gui)) {\n+\tmy $guitool = \"\";\n+\t$guitool = Git::config('diff.guitool');\n+\tif (length($guitool) > 0) {\n+\t\t$ENV{GIT_DIFF_TOOL} = $guitool;\n \t}\n-\treturn @command\n+}\n+if (defined($prompt)) {\n+\t$ENV{GIT_DIFFTOOL_PROMPT} = 'true';\n+}\n+elsif (defined($no_prompt)) {\n+\t$ENV{GIT_DIFFTOOL_NO_PROMPT} = 'true';\n }\n \n setup_environment();\n+my @command = (exe('git'), 'diff', @ARGV);\n \n # ActiveState Perl for Win32 does not implement POSIX semantics of\n # exec* system call. It just spawns the given executable and finishes\n # the starting program, exiting with code 0.\n # system will at least catch the errors returned by git diff,\n # allowing the caller of git difftool better handling of failures.\n-my $rc = system(generate_command());\n+my $rc = system(@command);\n exit($rc | ($rc >> 8));\n-- \n1.7.9.1.290.gbd444\n"},{"id":"187182","messageId":"1332035734-5443-3-git-send-email-tim.henigan@gmail.com","threadId":"29966","inReplyTo":"1332035734-5443-1-git-send-email-tim.henigan@gmail.com","subject":"[PATCH 2/9 v2] difftool: exit(0) when usage is printed","fromName":"Tim Henigan","fromEmail":"tim.henigan@gmail.com","sentAt":"2012-03-18T01:55:27Z","receivedAt":"2012-03-18T01:55:27Z","isPatch":true,"sender":{"key":"tim.henigan@gmail.com","avatar":"https://avatars.githubusercontent.com/u/42022?v=4"},"body":"Prior to this commit, the script exited with an error whenever the\nusage string was printed, regardless of the reason it was done. In\ncases where usage was printed due to a user request (e.g. '-h'\noption), the script should exit without error (exit 0).\n\nThis commit adds an argument to the usage function that allows the\nexit code to be specified when the function is called.\n\nSigned-off-by: Tim Henigan <tim.henigan@gmail.com>\n---\n git-difftool.perl |    9 +++++----\n 1 file changed, 5 insertions(+), 4 deletions(-)\n\ndiff --git a/git-difftool.perl b/git-difftool.perl\nindex e365727..81ecf34 100755\n--- a/git-difftool.perl\n+++ b/git-difftool.perl\n@@ -20,12 +20,13 @@ use Git;\n \n sub usage\n {\n+\tmy $exitcode = shift;\n \tprint << 'USAGE';\n usage: git difftool [-t|--tool=<tool>] [-x|--extcmd=<cmd>]\n                     [-y|--no-prompt]   [-g|--gui]\n                     ['git diff' options]\n USAGE\n-\texit 1;\n+\texit($exitcode);\n }\n \n sub setup_environment\n@@ -56,14 +57,14 @@ GetOptions('g|gui' => \\$gui,\n \t'y|no-prompt' => \\$no_prompt);\n \n if (defined($help)) {\n-\tusage();\n+\tusage(0);\n } \n if (defined($difftool_cmd)) {\n \tif (length($difftool_cmd) > 0) {\n \t\t$ENV{GIT_DIFF_TOOL} = $difftool_cmd;\n \t} else {\n \t\tprint \"No <tool> given for --tool=<tool>\\n\";\n-\t\tusage();\n+\t\tusage(1);\n \t}\n }\n if (defined($extcmd)) {\n@@ -71,7 +72,7 @@ if (defined($extcmd)) {\n \t\t$ENV{GIT_DIFFTOOL_EXTCMD} = $extcmd;\n \t} else {\n \t\tprint \"No <cmd> given for --extcmd=<cmd>\\n\";\n-\t\tusage();\n+\t\tusage(1);\n \t}\n }\n if (defined($gui)) {\n-- \n1.7.9.1.290.gbd444\n"},{"id":"187184","messageId":"1332035734-5443-4-git-send-email-tim.henigan@gmail.com","threadId":"29966","inReplyTo":"1332035734-5443-1-git-send-email-tim.henigan@gmail.com","subject":"[PATCH 3/9 v2] difftool: remove explicit change of PATH","fromName":"Tim Henigan","fromEmail":"tim.henigan@gmail.com","sentAt":"2012-03-18T01:55:28Z","receivedAt":"2012-03-18T01:55:28Z","isPatch":true,"sender":{"key":"tim.henigan@gmail.com","avatar":"https://avatars.githubusercontent.com/u/42022?v=4"},"body":"Adding the script directory to PATH is not needed. The script is\nlocated at '$(git --exec-path)', which is already on the PATH.\n\nSigned-off-by: Tim Henigan <tim.henigan@gmail.com>\n---\n git-difftool.perl |    4 ----\n 1 file changed, 4 deletions(-)\n\ndiff --git a/git-difftool.perl b/git-difftool.perl\nindex 81ecf34..53fcd7e 100755\n--- a/git-difftool.perl\n+++ b/git-difftool.perl\n@@ -13,8 +13,6 @@\n use 5.008;\n use strict;\n use warnings;\n-use Cwd qw(abs_path);\n-use File::Basename qw(dirname);\n use Getopt::Long qw(:config pass_through);\n use Git;\n \n@@ -31,8 +29,6 @@ USAGE\n \n sub setup_environment\n {\n-\tmy $DIR = abs_path(dirname($0));\n-\t$ENV{PATH} = \"$DIR:$ENV{PATH}\";\n \t$ENV{GIT_PAGER} = '';\n \t$ENV{GIT_EXTERNAL_DIFF} = 'git-difftool--helper';\n }\n-- \n1.7.9.1.290.gbd444\n"},{"id":"187183","messageId":"1332035734-5443-5-git-send-email-tim.henigan@gmail.com","threadId":"29966","inReplyTo":"1332035734-5443-1-git-send-email-tim.henigan@gmail.com","subject":"[PATCH 4/9 v2] difftool: stop appending '.exe' to git","fromName":"Tim Henigan","fromEmail":"tim.henigan@gmail.com","sentAt":"2012-03-18T01:55:29Z","receivedAt":"2012-03-18T01:55:29Z","isPatch":true,"sender":{"key":"tim.henigan@gmail.com","avatar":"https://avatars.githubusercontent.com/u/42022?v=4"},"body":"The system call to Git works the same whether or not \".exe\" is\nappended to \"git\". The extra code is not necessary.\n\nSigned-off-by: Tim Henigan <tim.henigan@gmail.com>\n---\n git-difftool.perl |   11 +----------\n 1 file changed, 1 insertion(+), 10 deletions(-)\n\ndiff --git a/git-difftool.perl b/git-difftool.perl\nindex 53fcd7e..99ff587 100755\n--- a/git-difftool.perl\n+++ b/git-difftool.perl\n@@ -33,15 +33,6 @@ sub setup_environment\n \t$ENV{GIT_EXTERNAL_DIFF} = 'git-difftool--helper';\n }\n \n-sub exe\n-{\n-\tmy $exe = shift;\n-\tif ($^O eq 'MSWin32' || $^O eq 'msys') {\n-\t\treturn \"$exe.exe\";\n-\t}\n-\treturn $exe;\n-}\n-\n # parse command-line options. all unrecognized options and arguments\n # are passed through to the 'git diff' command.\n my ($difftool_cmd, $extcmd, $gui, $help, $no_prompt, $prompt);\n@@ -86,7 +77,7 @@ elsif (defined($no_prompt)) {\n }\n \n setup_environment();\n-my @command = (exe('git'), 'diff', @ARGV);\n+my @command = ('git', 'diff', @ARGV);\n \n # ActiveState Perl for Win32 does not implement POSIX semantics of\n # exec* system call. It just spawns the given executable and finishes\n-- \n1.7.9.1.290.gbd444\n"},{"id":"187185","messageId":"1332035734-5443-6-git-send-email-tim.henigan@gmail.com","threadId":"29966","inReplyTo":"1332035734-5443-1-git-send-email-tim.henigan@gmail.com","subject":"[PATCH 5/9 v2] difftool: eliminate setup_environment function","fromName":"Tim Henigan","fromEmail":"tim.henigan@gmail.com","sentAt":"2012-03-18T01:55:30Z","receivedAt":"2012-03-18T01:55:30Z","isPatch":true,"sender":{"key":"tim.henigan@gmail.com","avatar":"https://avatars.githubusercontent.com/u/42022?v=4"},"body":"Removing this function shortens the code and makes it easier to read.\nNow all environment variables are set as part of procedural operation.\n\nSigned-off-by: Tim Henigan <tim.henigan@gmail.com>\n---\n git-difftool.perl |    9 ++-------\n 1 file changed, 2 insertions(+), 7 deletions(-)\n\ndiff --git a/git-difftool.perl b/git-difftool.perl\nindex 99ff587..9495f14 100755\n--- a/git-difftool.perl\n+++ b/git-difftool.perl\n@@ -27,12 +27,6 @@ USAGE\n \texit($exitcode);\n }\n \n-sub setup_environment\n-{\n-\t$ENV{GIT_PAGER} = '';\n-\t$ENV{GIT_EXTERNAL_DIFF} = 'git-difftool--helper';\n-}\n-\n # parse command-line options. all unrecognized options and arguments\n # are passed through to the 'git diff' command.\n my ($difftool_cmd, $extcmd, $gui, $help, $no_prompt, $prompt);\n@@ -76,7 +70,8 @@ elsif (defined($no_prompt)) {\n \t$ENV{GIT_DIFFTOOL_NO_PROMPT} = 'true';\n }\n \n-setup_environment();\n+$ENV{GIT_PAGER} = '';\n+$ENV{GIT_EXTERNAL_DIFF} = 'git-difftool--helper';\n my @command = ('git', 'diff', @ARGV);\n \n # ActiveState Perl for Win32 does not implement POSIX semantics of\n-- \n1.7.9.1.290.gbd444\n"},{"id":"187188","messageId":"1332035734-5443-7-git-send-email-tim.henigan@gmail.com","threadId":"29966","inReplyTo":"1332035734-5443-1-git-send-email-tim.henigan@gmail.com","subject":"[PATCH 6/9 v2] difftool: replace system call with Git::command_noisy","fromName":"Tim Henigan","fromEmail":"tim.henigan@gmail.com","sentAt":"2012-03-18T01:55:31Z","receivedAt":"2012-03-18T01:55:31Z","isPatch":true,"sender":{"key":"tim.henigan@gmail.com","avatar":"https://avatars.githubusercontent.com/u/42022?v=4"},"body":"The Git.pm module includes functions intended to standardize working\nwith Git repositories in Perl scripts. This commit teaches difftool\nto use Git::command_noisy rather than a system call to run the diff\ncommand.\n\nSigned-off-by: Tim Henigan <tim.henigan@gmail.com>\n---\n git-difftool.perl |   10 +---------\n 1 file changed, 1 insertion(+), 9 deletions(-)\n\ndiff --git a/git-difftool.perl b/git-difftool.perl\nindex 9495f14..8498089 100755\n--- a/git-difftool.perl\n+++ b/git-difftool.perl\n@@ -72,12 +72,4 @@ elsif (defined($no_prompt)) {\n \n $ENV{GIT_PAGER} = '';\n $ENV{GIT_EXTERNAL_DIFF} = 'git-difftool--helper';\n-my @command = ('git', 'diff', @ARGV);\n-\n-# ActiveState Perl for Win32 does not implement POSIX semantics of\n-# exec* system call. It just spawns the given executable and finishes\n-# the starting program, exiting with code 0.\n-# system will at least catch the errors returned by git diff,\n-# allowing the caller of git difftool better handling of failures.\n-my $rc = system(@command);\n-exit($rc | ($rc >> 8));\n+git_cmd_try { Git::command_noisy(('diff', @ARGV)) } 'exit code %d';\n-- \n1.7.9.1.290.gbd444\n"},{"id":"187186","messageId":"1332035734-5443-8-git-send-email-tim.henigan@gmail.com","threadId":"29966","inReplyTo":"1332035734-5443-1-git-send-email-tim.henigan@gmail.com","subject":"[PATCH 7/9 v2] difftool: teach difftool to handle directory diffs","fromName":"Tim Henigan","fromEmail":"tim.henigan@gmail.com","sentAt":"2012-03-18T01:55:32Z","receivedAt":"2012-03-18T01:55:32Z","isPatch":true,"sender":{"key":"tim.henigan@gmail.com","avatar":"https://avatars.githubusercontent.com/u/42022?v=4"},"body":"Prior to this commit, the difftool utility could only open files one\nat a time.  The new '--dir-diff' option copies all the modified files\nto a temporary location and runs a directory diff on them.\n\nSigned-off-by: Tim Henigan <tim.henigan@gmail.com>\n---\n\nChanges in v2:\n    - Location of /tmp is now determined via the File::Temp::tempdir. In\n      v1, the location was hard-coded to '/tmp'.\n    - Lexical filehandles are now used for open().\n    - Modern, 3 argument, form of open() is now used.\n    - Arguments to the 'system' call for $extcmd are now given in list form.\n    - Replaced 'system' call to 'git difftool--helper' with Git::command_noisy\n\n\n Documentation/git-difftool.txt |    6 ++\n git-difftool--helper.sh        |   20 ++++--\n git-difftool.perl              |  144 ++++++++++++++++++++++++++++++++++++----\n 3 files changed, 152 insertions(+), 18 deletions(-)\n\ndiff --git a/Documentation/git-difftool.txt b/Documentation/git-difftool.txt\nindex fe38f66..aba5e76 100644\n--- a/Documentation/git-difftool.txt\n+++ b/Documentation/git-difftool.txt\n@@ -19,6 +19,12 @@ linkgit:git-diff[1].\n \n OPTIONS\n -------\n+-d::\n+--dir-diff::\n+\tCopy the modified files to a temporary location and perform\n+\ta directory diff on them. This mode never prompts before\n+\tlaunching the diff tool.\n+\n -y::\n --no-prompt::\n \tDo not prompt before launching a diff tool.\ndiff --git a/git-difftool--helper.sh b/git-difftool--helper.sh\nindex e6558d1..bd0556f 100755\n--- a/git-difftool--helper.sh\n+++ b/git-difftool--helper.sh\n@@ -6,6 +6,7 @@\n # Copyright (c) 2009, 2010 David Aguilar\n \n TOOL_MODE=diff\n+SUBDIRECTORY_OK=1\n . git-mergetool--lib\n \n # difftool.prompt controls the default prompt/no-prompt behavior\n@@ -73,9 +74,16 @@ then\n \tfi\n fi\n \n-# Launch the merge tool on each path provided by 'git diff'\n-while test $# -gt 6\n-do\n-\tlaunch_merge_tool \"$1\" \"$2\" \"$5\"\n-\tshift 7\n-done\n+if test -n \"$GIT_DIFFTOOL_DIRDIFF\"\n+then\n+\tLOCAL=\"$1\"\n+\tREMOTE=\"$2\"\n+\trun_merge_tool \"$merge_tool\" false\n+else\n+\t# Launch the merge tool on each path provided by 'git diff'\n+\twhile test $# -gt 6\n+\tdo\n+\t\tlaunch_merge_tool \"$1\" \"$2\" \"$5\"\n+\t\tshift 7\n+\tdone\n+fi\ndiff --git a/git-difftool.perl b/git-difftool.perl\nindex 8498089..1f7b8f2 100755\n--- a/git-difftool.perl\n+++ b/git-difftool.perl\n@@ -1,18 +1,24 @@\n #!/usr/bin/env perl\n # Copyright (c) 2009, 2010 David Aguilar\n+# Copyright (c) 2012 Tim Henigan\n #\n # This is a wrapper around the GIT_EXTERNAL_DIFF-compatible\n # git-difftool--helper script.\n #\n # This script exports GIT_EXTERNAL_DIFF and GIT_PAGER for use by git.\n-# GIT_DIFFTOOL_NO_PROMPT, GIT_DIFFTOOL_PROMPT, and GIT_DIFF_TOOL\n-# are exported for use by git-difftool--helper.\n+# GIT_DIFFTOOL_NO_PROMPT, GIT_DIFFTOOL_PROMPT, GIT_DIFFTOOL_DIRDIFF,\n+# and GIT_DIFF_TOOL are exported for use by git-difftool--helper.\n #\n # Any arguments that are unknown to this script are forwarded to 'git diff'.\n \n use 5.008;\n use strict;\n use warnings;\n+use File::Basename qw(dirname);\n+use File::Copy;\n+use File::stat;\n+use File::Path qw(mkpath);\n+use File::Temp qw(tempdir);\n use Getopt::Long qw(:config pass_through);\n use Git;\n \n@@ -22,15 +28,109 @@ sub usage\n \tprint << 'USAGE';\n usage: git difftool [-t|--tool=<tool>] [-x|--extcmd=<cmd>]\n                     [-y|--no-prompt]   [-g|--gui]\n+                    [-d|--dir-diff]\n                     ['git diff' options]\n USAGE\n \texit($exitcode);\n }\n \n+sub setup_dir_diff\n+{\n+\t# Run the diff; exit immediately if no diff found\n+\tmy $repo = Git->repository();\n+\tmy $diffrtn = $repo->command_oneline(['diff', '--raw', '--no-abbrev', '-z', @ARGV]);\n+\texit(0) if (length($diffrtn) == 0);\n+\n+\t# Setup temp directories\n+\tmy $tmpdir = tempdir('git-diffall.XXXXX', CLEANUP => 1, TMPDIR => 1);\n+\tmy $ldir = \"$tmpdir/left\";\n+\tmy $rdir = \"$tmpdir/right\";\n+\tmkpath($ldir) or die $!;\n+\tmkpath($rdir) or die $!;\n+\n+\t# Build index info for left and right sides of the diff\n+\tmy $submodule_mode = \"160000\";\n+\tmy $null_mode = 0 x 6;\n+\tmy $null_sha1 = 0 x 40;\n+\tmy $lindex = \"\";\n+\tmy $rindex = \"\";\n+\tmy @working_tree;\n+\tmy %submodule;\n+\tmy @rawdiff = split('\\0', $diffrtn);\n+\n+\tfor (my $i=0; $i<@rawdiff; $i+=2) {\n+\t\tmy ($lmode, $rmode, $lsha1, $rsha1, $status) = split(' ', substr($rawdiff[$i], 1));\n+\t\tmy $path = $rawdiff[$i + 1];\n+\n+\t\tif (($lmode eq $submodule_mode) or ($rmode eq $submodule_mode)) {\n+\t\t\t$submodule{$path}{left} = $lsha1;\n+\t\t\t$submodule{$path}{right} = $rsha1;\n+\t\t\tnext;\n+\t\t}\n+\n+\t\tif ($lmode ne $null_mode) {\n+\t\t\t$lindex .= \"$lmode $lsha1\\t$path\\0\";\n+\t\t}\n+\n+\t\tif ($rmode ne $null_mode) {\n+\t\t\tif ($rsha1 ne $null_sha1) {\n+\t\t\t\t$rindex .= \"$rmode $rsha1\\t$path\\0\";\n+\t\t\t} else {\n+\t\t\t\tpush(@working_tree, $path);\n+\t\t\t}\n+\t\t}\n+\t}\n+\n+\t# Populate the left and right directories based on each index file\n+\tmy ($inpipe, $ctx);\n+\t$ENV{GIT_INDEX_FILE} = \"$tmpdir/lindex\";\n+\t($inpipe, $ctx) = $repo->command_input_pipe(qw/update-index -z --index-info/);\n+\tprint($inpipe $lindex);\n+\t$repo->command_close_pipe($inpipe, $ctx);\n+\t$repo->command_oneline([\"checkout-index\", \"-a\", \"--prefix=$ldir/\"]);\n+\n+\t$ENV{GIT_INDEX_FILE} = \"$tmpdir/rindex\";\n+\t($inpipe, $ctx) = $repo->command_input_pipe(qw/update-index -z --index-info/);\n+\tprint($inpipe $rindex);\n+\t$repo->command_close_pipe($inpipe, $ctx);\n+\t$repo->command_oneline([\"checkout-index\", \"-a\", \"--prefix=$rdir/\"]);\n+\n+\t# Changes in the working tree need special treatment since they are\n+\t# not part of the index\n+\tmy $workdir = $repo->wc_path();\n+\tfor (@working_tree) {\n+\t\tmy $dir = dirname($_);\n+\t\tunless (-d \"$rdir/$dir\") {\n+\t\t\tmkpath(\"$rdir/$dir\") or die $!;\n+\t\t}\n+\t\tcopy(\"$workdir/$_\", \"$rdir/$_\") or die $!;\n+\t\tchmod(stat(\"$workdir/$_\")->mode, \"$rdir/$_\") or die $!;\n+\t}\n+\n+\t# Changes to submodules require special treatment. This loop writes a\n+\t# temporary file to both the left and right directories to show the\n+\t# change in the recorded SHA1 for the submodule.\n+\tforeach my $path (keys %submodule) {\n+\t\tif (defined $submodule{$path}{left}) {\n+\t\t\topen(my $fh, \">\", \"$ldir/$path\") or die $!;\n+\t\t\tprint($fh \"Subproject commit $submodule{$path}{left}\");\n+\t\t\tclose($fh);\n+\t\t}\n+\t\tif (defined $submodule{$path}{right}) {\n+\t\t\topen(my $fh, \">\", \"$rdir/$path\") or die $!;\n+\t\t\tprint($fh \"Subproject commit $submodule{$path}{right}\");\n+\t\t\tclose($fh);\n+\t\t}\n+\t}\n+\n+\treturn ($ldir, $rdir);\n+}\n+\n # parse command-line options. all unrecognized options and arguments\n # are passed through to the 'git diff' command.\n-my ($difftool_cmd, $extcmd, $gui, $help, $no_prompt, $prompt);\n+my ($difftool_cmd, $dirdiff, $extcmd, $gui, $help, $no_prompt, $prompt);\n GetOptions('g|gui' => \\$gui,\n+\t'd|dir-diff' => \\$dirdiff,\n \t'h' => \\$help,\n \t'prompt' => \\$prompt,\n \t't|tool:s' => \\$difftool_cmd,\n@@ -63,13 +163,33 @@ if (defined($gui)) {\n \t\t$ENV{GIT_DIFF_TOOL} = $guitool;\n \t}\n }\n-if (defined($prompt)) {\n-\t$ENV{GIT_DIFFTOOL_PROMPT} = 'true';\n-}\n-elsif (defined($no_prompt)) {\n-\t$ENV{GIT_DIFFTOOL_NO_PROMPT} = 'true';\n-}\n \n-$ENV{GIT_PAGER} = '';\n-$ENV{GIT_EXTERNAL_DIFF} = 'git-difftool--helper';\n-git_cmd_try { Git::command_noisy(('diff', @ARGV)) } 'exit code %d';\n+# In directory diff mode, 'git-difftool--helper' is called once\n+# to compare the a/b directories.  In file diff mode, 'git diff'\n+# will invoke a separate instance of 'git-difftool--helper' for\n+# each file that changed.\n+if (defined($dirdiff)) {\n+\tmy ($a, $b) = setup_dir_diff();\n+\tif (defined($extcmd)) {\n+\t\tsystem(($extcmd, $a, $b));\n+\t} else {\n+\t\t$ENV{GIT_DIFFTOOL_DIRDIFF} = 'true';\n+\t\tgit_cmd_try {\n+\t\t\tGit::command_noisy(('difftool--helper', $a, $b))\n+\t\t} 'exit code %d';\n+\t}\n+\t# TODO: if the diff including working copy files and those\n+\t# files were modified during the diff, then the changes\n+\t# should be copied back to the working tree\n+} else {\n+\tif (defined($prompt)) {\n+\t\t$ENV{GIT_DIFFTOOL_PROMPT} = 'true';\n+\t}\n+\telsif (defined($no_prompt)) {\n+\t\t$ENV{GIT_DIFFTOOL_NO_PROMPT} = 'true';\n+\t}\n+\n+\t$ENV{GIT_PAGER} = '';\n+\t$ENV{GIT_EXTERNAL_DIFF} = 'git-difftool--helper';\n+\tgit_cmd_try { Git::command_noisy(('diff', @ARGV)) } 'exit code %d';\n+}\n-- \n1.7.9.1.290.gbd444\n"},{"id":"187189","messageId":"1332035734-5443-9-git-send-email-tim.henigan@gmail.com","threadId":"29966","inReplyTo":"1332035734-5443-1-git-send-email-tim.henigan@gmail.com","subject":"[PATCH 8/9 v2] difftool: teach dir-diff to copy modified files back to working tree","fromName":"Tim Henigan","fromEmail":"tim.henigan@gmail.com","sentAt":"2012-03-18T01:55:33Z","receivedAt":"2012-03-18T01:55:33Z","isPatch":true,"sender":{"key":"tim.henigan@gmail.com","avatar":"https://avatars.githubusercontent.com/u/42022?v=4"},"body":"If the user runs a diff when there are modified files in the working copy,\nthey expect to be able to be edit the files in the diff viewer and save\nthe changes. When using difftool in file mode (i.e. diffing one file at\na time), this works as expected.  However, when using difftool in directory\ndiff mode, the changes are not saved.\n\nThis commit teaches the directory diff mode to copy changes from the diff\nviewer back to the working copy.\n\nSigned-off-by: Tim Henigan <tim.henigan@gmail.com>\n---\n git-difftool.perl |   12 ++++++++++--\n 1 file changed, 10 insertions(+), 2 deletions(-)\n\ndiff --git a/git-difftool.perl b/git-difftool.perl\nindex 1f7b8f2..e306977 100755\n--- a/git-difftool.perl\n+++ b/git-difftool.perl\n@@ -22,6 +22,8 @@ use File::Temp qw(tempdir);\n use Getopt::Long qw(:config pass_through);\n use Git;\n \n+my @working_tree;\n+\n sub usage\n {\n \tmy $exitcode = shift;\n@@ -54,7 +56,6 @@ sub setup_dir_diff\n \tmy $null_sha1 = 0 x 40;\n \tmy $lindex = \"\";\n \tmy $rindex = \"\";\n-\tmy @working_tree;\n \tmy %submodule;\n \tmy @rawdiff = split('\\0', $diffrtn);\n \n@@ -178,9 +179,16 @@ if (defined($dirdiff)) {\n \t\t\tGit::command_noisy(('difftool--helper', $a, $b))\n \t\t} 'exit code %d';\n \t}\n-\t# TODO: if the diff including working copy files and those\n+\n+\t# If the diff including working copy files and those\n \t# files were modified during the diff, then the changes\n \t# should be copied back to the working tree\n+\tmy $repo = Git->repository();\n+\tmy $workdir = $repo->wc_path();\n+\tfor (@working_tree) {\n+\t\tcopy(\"$b/$_\", \"$workdir/$_\") or die $!;\n+\t\tchmod(stat(\"$b/$_\")->mode, \"$workdir/$_\") or die $!;\n+\t}\n } else {\n \tif (defined($prompt)) {\n \t\t$ENV{GIT_DIFFTOOL_PROMPT} = 'true';\n-- \n1.7.9.1.290.gbd444\n"},{"id":"187187","messageId":"1332035734-5443-10-git-send-email-tim.henigan@gmail.com","threadId":"29966","inReplyTo":"1332035734-5443-1-git-send-email-tim.henigan@gmail.com","subject":"[PATCH 9/9 v2] difftool: print list of valid tools with '--tool-help'","fromName":"Tim Henigan","fromEmail":"tim.henigan@gmail.com","sentAt":"2012-03-18T01:55:34Z","receivedAt":"2012-03-18T01:55:34Z","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 and print them for the user when the '--tool-help' option is\ngiven.\n\nSigned-off-by: Tim Henigan <tim.henigan@gmail.com>\n---\n\nChanges in v2:\n  The glob statement used to find the mergetool options now uses the\n  fully qualified path.  This insures that 'git difftool --tool-help'\n  can be called from any diretory.\n\n Documentation/git-difftool.txt |   11 ++++++-----\n git-difftool.perl              |   17 ++++++++++++++---\n 2 files changed, 20 insertions(+), 8 deletions(-)\n\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 <tool>::\n --tool=<tool>::\n-\tUse the diff tool specified by <tool>.\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 <tool>.  Valid values include\n+\temerge, kompare, meld, and vimdiff. Run `git difftool --tool-help`\n+\tfor the list of valid <tool> 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 <command>::\n --extcmd=<command>::\n \tSpecify a custom command for viewing diffs.\ndiff --git a/git-difftool.perl b/git-difftool.perl\nindex e306977..fb4e3e6 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,8 @@ sub usage\n {\n \tmy $exitcode = shift;\n \tprint << 'USAGE';\n-usage: git difftool [-t|--tool=<tool>] [-x|--extcmd=<cmd>]\n+usage: git difftool [-t|--tool=<tool>] [--tool-help]\n+                    [-x|--extcmd=<cmd>]\n                     [-y|--no-prompt]   [-g|--gui]\n                     [-d|--dir-diff]\n                     ['git diff' options]\n@@ -129,18 +130,28 @@ 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, $no_prompt, $prompt);\n+my ($difftool_cmd, $dirdiff, $extcmd, $gui, $help, $no_prompt, $prompt, $tool_help);\n GetOptions('g|gui' => \\$gui,\n \t'd|dir-diff' => \\$dirdiff,\n \t'h' => \\$help,\n \t'prompt' => \\$prompt,\n \t't|tool:s' => \\$difftool_cmd,\n+\t'tool-help' => \\$tool_help,\n \t'x|extcmd:s' => \\$extcmd,\n \t'y|no-prompt' => \\$no_prompt);\n \n if (defined($help)) {\n \tusage(0);\n } \n+if (defined($tool_help)) {\n+\tmy $gitpath = Git::exec_path();\n+\tprint \"'git difftool --tool=<tool>' may be set to one of the following:\\n\";\n+\tfor (glob \"$gitpath/mergetools/*\") {\n+\t\tnext if /defaults$/;\n+\t\tprint \"\\t\" . basename($_) . \"\\n\";\n+\t}\n+\texit(0);\n+}\n if (defined($difftool_cmd)) {\n \tif (length($difftool_cmd) > 0) {\n \t\t$ENV{GIT_DIFF_TOOL} = $difftool_cmd;\n-- \n1.7.9.1.290.gbd444\n"},{"id":"187193","messageId":"CAJDDKr5bvNK2Pd+-wbTNgZdui91-zs_qST3PWKnHVmp-vGYVqg@mail.gmail.com","threadId":"29966","inReplyTo":"CAFouetgpz__1q7F_a3LQoeypCVBXOyTPWmL0e9=rw7vV65d7_g@mail.gmail.com","subject":"Re: [PATCH 1/9] difftool: parse options using Getopt::Long","fromName":"David Aguilar","fromEmail":"davvid@gmail.com","sentAt":"2012-03-18T03:29:45Z","receivedAt":"2012-03-18T03:29:45Z","isPatch":true,"sender":{"key":"davvid@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13196?v=4"},"body":"On Sat, Mar 17, 2012 at 6:54 AM, Tim Henigan <tim.henigan@gmail.com> wrote:\n> On Fri, Mar 16, 2012 at 11:21 PM, David Aguilar <davvid@gmail.com> wrote:\n>> On Fri, Mar 16, 2012 at 6:57 PM, Tim Henigan <tim.henigan@gmail.com> wrote:\n>>\n>> I've also wanted to do the same in the past.  The one thing holding me\n>> back was this note from the perldocs:\n>>\n>> \"If pass_through is also enabled, options processing will terminate at\n>> the first unrecognized option, or non-option, whichever comes first.\"\n>>\n>> http://search.cpan.org/~jv/Getopt-Long-2.38/lib/Getopt/Long.pm\n>\n> That sentence is listed under the documentation of the 'permute'\n> option.  The documentation of 'pass_through' states that:\n>\n> \"Options that are unknown, ambiguous or supplied with an invalid\n> option value are passed through in @ARGV instead of being flagged as\n> errors. This makes it possible to write wrapper scripts that process\n> only part of the user supplied command line arguments, and pass the\n> remaining options to some other program.\n>\n> If require_order is enabled, options processing will terminate at the\n> first unrecognized option, or non-option, whichever comes first.\n> However, if permute is enabled instead, results can become confusing.\"\n>\n> So early termination would only be a problem if 'pass_through' was\n> enabled at the same time as 'require_order' or 'permute'.  To verify,\n> I confirmed that 'git difftool --cached --diff-dir' works as expected.\n>\n>\n>> Is this indeed the case?  I am a little ashamed that the difftool\n>> tests do not cover this area.  That would be a valuable first step\n>> towards exploring this approach, IMO.\n>\n> I will review the the test cases.  If this goes forward, I still need\n> to add test cases to confirm the new '--dir-diff' option.\n>\n>\n>> BTW, I hate @ARGV parsing loops just as much as anyone!  I was not\n>> ignorant of Getopt::Long, and no, I was not re-inventing the wheel for\n>> no reason.  The reason it was done that way was so that we can forward\n>> everything we don't know about to git-diff.\n>\n> I understand and was not implying anything different.  I simply\n> thought this would be a positive change.\n\nHehehe, well apparently I was ignorant about the permute_order and\npass_through interaction afterall ;-)  Thanks for the careful reading\nof the docs.  I should have done that sooner.\n\nI highly appreciate your rework of this stuff -- this cleanup is quite\nnice.  ++Getopt::Long.\n\nI sent a patch testing the \"forward options to diff\" behavior sometime\nyesterday(?) so we should be good on that side.  Testing the diff-diff\nstuff will be good.\n\nThanks a lot for this, it's a sweet feature and I know the users I\nhelp will get a lot of mileage out of it.\n-- \nDavid\n"},{"id":"187279","messageId":"7vhaxk9ui6.fsf@alter.siamese.dyndns.org","threadId":"29966","inReplyTo":"1332035734-5443-1-git-send-email-tim.henigan@gmail.com","subject":"Re: [PATCH 0/9 v2] difftool: teach command to perform directory diffs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-03-19T21:00:01Z","receivedAt":"2012-03-19T21:00:01Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Thanks.\n\nI do not know a difftool user, and I wasn't paying close attention to the\ndiscussion but I recall these points raised and I do not recall the\nresolutions:\n\n * In [1/9], use of pass_through would mean 'git difftool' must be fed the\n   options for difftool and then options meant for underlying diff\n   machinery.  Is this limitation still there?  If so, isn't this a\n   regression?  Shouldn't it at least be advertised to the users a lot\n   stronger in documentation?\n\n * In [4/9], you remove the .exe extension when running git, which was\n   there since the beginning of difftool 5c38ea3 (contrib: add 'git\n   difftool' for launching common merge tools, 2009-01-16).  I think it is\n   safe but are Windows folks OK with this?\n\n * In [6/9], the exit code in the failure case seem to be modified from\n   that of the underlying \"git diff\" to whatever croak gives us.  Is this\n   a regression, or nobody cares error status from difftool?  I personally\n   suspect it is the latter, but just double-checking if you have\n   considered it.\n\n * In [7/9], difftool--helper declares SUBDIRECTORY_OK, but there doesn't\n   seem to be any inclusion of git-sh-setup in this script, and the patch\n   does not have any effort to prepend $prefix to paths relative to $cwd.\n   What good does the variable do here?\n"},{"id":"187301","messageId":"CAJDDKr74cLSfj9JCeQQcm8U_26Re0u0nueoQ3nvj0O4FWBsqxg@mail.gmail.com","threadId":"29966","inReplyTo":"7vhaxk9ui6.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 0/9 v2] difftool: teach command to perform directory diffs","fromName":"David Aguilar","fromEmail":"davvid@gmail.com","sentAt":"2012-03-20T02:52:30Z","receivedAt":"2012-03-20T02:52:30Z","isPatch":true,"sender":{"key":"davvid@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13196?v=4"},"body":"On Mon, Mar 19, 2012 at 2:00 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> Thanks.\n>\n> I do not know a difftool user, and I wasn't paying close attention to the\n> discussion but I recall these points raised and I do not recall the\n> resolutions:\n>\n>  * In [1/9], use of pass_through would mean 'git difftool' must be fed the\n>   options for difftool and then options meant for underlying diff\n>   machinery.  Is this limitation still there?  If so, isn't this a\n>   regression?  Shouldn't it at least be advertised to the users a lot\n>   stronger in documentation?\n\nTim asserted that this is not the case.  The options should pass\nthrough.  Hopefully there aren't any behavior changes between perl\nversions and this option.\n\nI sent a patch adding a test case to cover this scenario.  I would\nprefer that we avoid a regression.  If there's a regression then we\ncan do without Getopt::Long, IMO.\n\nI believe users should be oblivious as to which options are for\ndifftool and which are for diff.  E.g. `git difftool --cached -t\nxxdiff` -- the user does not care that --cached is for diff and -t is\nfor difftool.\n\n>  * In [4/9], you remove the .exe extension when running git, which was\n>   there since the beginning of difftool 5c38ea3 (contrib: add 'git\n>   difftool' for launching common merge tools, 2009-01-16).  I think it is\n>   safe but are Windows folks OK with this?\n\nI do not have Windows to test with, but this supposedly works since\nGit.pm does not use git.exe either.  The git.exe stuff was originally\nthere because difftool originally did exec('git.exe', ...).  It was\nlater changed to use system() and it's possible that we could have\ndropped the .exe extension at that time.\n\nBut, like I said, I don't have any Windows machines with which to\nverify this behavior, and highly appreciate feedback from Windows\nfolks.\n\n\n>  * In [6/9], the exit code in the failure case seem to be modified from\n>   that of the underlying \"git diff\" to whatever croak gives us.  Is this\n>   a regression, or nobody cares error status from difftool?  I personally\n>   suspect it is the latter, but just double-checking if you have\n>   considered it.\n\nThis doesn't seem like too big of an issue.  Had we documented the old\nexit codes then it would be a bigger concern.  I would personally\nprefer to preserve the exit code from diff itself, but if that\ncomplicates it too much then the new behavior is ok.\n\n\n>  * In [7/9], difftool--helper declares SUBDIRECTORY_OK, but there doesn't\n>   seem to be any inclusion of git-sh-setup in this script, and the patch\n>   does not have any effort to prepend $prefix to paths relative to $cwd.\n>   What good does the variable do here?\n\nI'll defer to Tim on this one.  This seems like an oversight.  It\nseems like something should be done to handle it.\n\nI recall that we made $GIT_PREFIX available to aliases and builtins\nnot too long ago.  That may help here.  See\n1f5d271f5e8f7b1e2a5b296ff43ca4087eb08244.\n\nAlso.. I think we need some tests to cover the new behavior.  A test\nto cover the subdirectory behavior would be especially helpful given\nthe note about [7/9].\n-- \nDavid\n"},{"id":"187304","messageId":"7v4ntj95ub.fsf@alter.siamese.dyndns.org","threadId":"29966","inReplyTo":"CAJDDKr74cLSfj9JCeQQcm8U_26Re0u0nueoQ3nvj0O4FWBsqxg@mail.gmail.com","subject":"Re: [PATCH 0/9 v2] difftool: teach command to perform directory diffs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-03-20T05:52:44Z","receivedAt":"2012-03-20T05:52:44Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"David Aguilar <davvid@gmail.com> writes:\n\n> On Mon, Mar 19, 2012 at 2:00 PM, Junio C Hamano <gitster@pobox.com> wrote:\n>> Thanks.\n>>\n>> I do not know a difftool user, and I wasn't paying close attention to the\n>> discussion but I recall these points raised and I do not recall the\n>> resolutions:\n>>\n>>  * In [1/9], use of pass_through would mean 'git difftool' must be fed the\n>>   options for difftool and then options meant for underlying diff\n>>   machinery.  Is this limitation still there?  If so, isn't this a\n>>   regression?  Shouldn't it at least be advertised to the users a lot\n>>   stronger in documentation?\n>\n> Tim asserted that this is not the case.  The options should pass\n> through.  Hopefully there aren't any behavior changes between perl\n> versions and this option.\n>\n> I sent a patch adding a test case to cover this scenario.  I would\n> prefer that we avoid a regression.  If there's a regression then we\n> can do without Getopt::Long, IMO.\n\nYeah.  Does tonight's 'pu' pass for you?\n\n> I do not have Windows to test with, but this supposedly works since\n> Git.pm does not use git.exe either.\n\nYeah, that matches my guess.\n\n>>  * In [7/9], difftool--helper declares SUBDIRECTORY_OK, but there doesn't\n>>   seem to be any inclusion of git-sh-setup in this script, and the patch\n>>   does not have any effort to prepend $prefix to paths relative to $cwd.\n>>   What good does the variable do here?\n>\n> I'll defer to Tim on this one.  This seems like an oversight.  It\n> seems like something should be done to handle it.\n\nWell, it does not seem to dot-source git-sh-setup so it probably stays\nwhere it was launched, so in that case there is nothing that needs to be\ndone, including SUBDIRECTORY_OK which nobody would look at IIRC.\n\n> Also.. I think we need some tests to cover the new behavior.  A test\n> to cover the subdirectory behavior would be especially helpful given\n> the note about [7/9].\n\nYeah, that makes sense.\n"},{"id":"187327","messageId":"CAFouetj2gME+X47EUAQTUKX_GJ8PW=cSWRj7VUdBsi91Aa9Wyw@mail.gmail.com","threadId":"29966","inReplyTo":"7vhaxk9ui6.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 0/9 v2] difftool: teach command to perform directory diffs","fromName":"Tim Henigan","fromEmail":"tim.henigan@gmail.com","sentAt":"2012-03-20T13:07:41Z","receivedAt":"2012-03-20T13:07:41Z","isPatch":true,"sender":{"key":"tim.henigan@gmail.com","avatar":"https://avatars.githubusercontent.com/u/42022?v=4"},"body":"I have successfully tested on:\n  - Linux with Git v1.7.10-rc0 and 1\n  - Windows 7 with msysgit v1.7.9\n\nI installed cygwin Git on Windows 7, but found that 'git difftool'\nwould not work, even before my patches were applied.  It appears that\nI need to learn more about cygwin before I can use it as a test\nplatform.\n\n\nOn Mon, Mar 19, 2012 at 5:00 PM, Junio C Hamano <gitster@pobox.com> wrote:\n>\n>  * In [1/9], use of pass_through would mean 'git difftool' must be fed the\n>   options for difftool and then options meant for underlying diff\n>   machinery.  Is this limitation still there?  If so, isn't this a\n>   regression?  Shouldn't it at least be advertised to the users a lot\n>   stronger in documentation?\n\nBoth the documentation and testing has shown that 'pass_through' works\nas I expected.  That is, options that difftool cares about are\nextracted from @ARGV while all others are passed on to the underlying\n'git diff' command.  The order of the arguments/options does not\nmatter.  We \"use 5.008;\" at the top of the script, so I believe this\nchange is correct and safe.  That being said, I have a limited number\nof computers/configurations to test on.\n\n\n>  * In [4/9], you remove the .exe extension when running git, which was\n>   there since the beginning of difftool 5c38ea3 (contrib: add 'git\n>   difftool' for launching common merge tools, 2009-01-16).  I think it is\n>   safe but are Windows folks OK with this?\n\nMy testing on Windows with msysgit has shown that this change is safe.\n Also, even though I had other problems when testing on cygwin (noted\nat the top of the email), the 'difftool' script was able to\nsuccessfully execute 'git diff --raw -z' without appending '.exe'.\n\n\n>  * In [6/9], the exit code in the failure case seem to be modified from\n>   that of the underlying \"git diff\" to whatever croak gives us.  Is this\n>   a regression, or nobody cares error status from difftool?  I personally\n>   suspect it is the latter, but just double-checking if you have\n>   considered it.\n\nIn my testing, if \"Git::command_noisy(('diff', @ARGV))\" fails, the\nexit code from 'git diff' is passed back to the terminal.  So not only\nis the 'git diff' exit code echoed to the terminal, but '$?' also\ncontains the correct exit code.\n\n\n>  * In [7/9], difftool--helper declares SUBDIRECTORY_OK, but there doesn't\n>   seem to be any inclusion of git-sh-setup in this script, and the patch\n>   does not have any effort to prepend $prefix to paths relative to $cwd.\n>   What good does the variable do here?\n\nThis was an oversight.  It was copied over from a previous\nimplementation that still used \". git-sh-setup\".\n\nSo the only outstanding changes that I am planning are:\n  1) Remove SUBDIRECTORY_OK\n  2) Add tests for the new '--dir-diff' option\n\nWould it be better to resend the complete patches series with these\nchanges or just add new patches to the end?\n\nPlease let me know if I missed anything else.\n"},{"id":"187335","messageId":"7vmx7b6xhc.fsf@alter.siamese.dyndns.org","threadId":"29966","inReplyTo":"CAFouetj2gME+X47EUAQTUKX_GJ8PW=cSWRj7VUdBsi91Aa9Wyw@mail.gmail.com","subject":"Re: [PATCH 0/9 v2] difftool: teach command to perform directory diffs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-03-20T16:36:15Z","receivedAt":"2012-03-20T16:36:15Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Tim Henigan <tim.henigan@gmail.com> writes:\n\n> So the only outstanding changes that I am planning are:\n>   1) Remove SUBDIRECTORY_OK\n>   2) Add tests for the new '--dir-diff' option\n>\n> Would it be better to resend the complete patches series with these\n> changes or just add new patches to the end?\n\nFor the former, a replacement patch (\"[PATCH v3 7/9]\" with \"this replaces\npatch 5/9 from the v2\" after \"---\" line) for the ones that need replacing\nis preferred, as the topic is still in 'pu'.  For the latter, we can go\neither with a separate patch \"[PATCH v3 10/9]\" or an update to the one\nthat finishes introduction of the new option.\n\n> Please let me know if I missed anything else.\n\nI think I saw t7800 failed with this series somewhwere around \"last flag\nwins\" test.\n"}]}