[PATCH 0/9] difftool: teach command to perform directory diffs

[PATCH 0/9] difftool: teach command to perform directory diffs

[Tim Henigan]

'git difftool' is a very useful command that allows git diffs to be opened
in an external tool. Currently, difftool opens a separate instance of the
external tool for each file that changed. This can be tedious when many
files have changed.

This series teaches difftool to perform directory diffs, so that all file
changes can be opened/reviewed in a single instance of the external tool.

This is the second phase of development for this feature. The first phase
was added as a separate command (git diffall) in 1252bbe (contrib: add
git-diffall script). During review of that script on the Git developers
list, an informal development roadmap was suggested [1]. The next phase
of that plan is to integrate the 'git-diffall' feature into 'difftool'.
This series gets that done.

[1]: http://thread.gmane.org/gmane.comp.version-control.git/191297/focus=191383


Patches 1-6 are clean-up and rewrite to prepare for later changes. They
are not intended to change the behavior of difftool. Instead, they teach
the script to use standard modules for option parsing and interacting with
the Git repo. I understand that these changes may be controversial due to
the amount of rewrite, but the end result is that the script is half the
previous size and I believe is easier to understand and maintain.

Patches 7 and 8 are the core changes in this series. They teach difftool the
new "--dir-diff" option. When this option is used, difftool will copy files
that were changed to a temporary location and run a single directory diff on
them rather than run a separate instance of the diff tool for each file.

Patch 9 is a new implementation of a patch that I submitted recently.
061e672 (difftool: print list of valid tools with '--tool-help' was written
prior to these other changes. It needed to be rewritten to fit into the
Getopt::Long option parsing framework included with this series.

Tim Henigan (9):
  difftool: parse options using Getopt::Long
  difftool: exit(0) when usage is printed
  difftool: remove explicit change of PATH
  difftool: stop appending '.exe' to git
  difftool: eliminate setup_environment function
  difftool: replace system call with Git::command_noisy
  difftool: teach difftool to handle directory diffs
  difftool: teach dir-diff to copy modified files back to working tree
  difftool: print list of valid tools with '--tool-help'

 Documentation/git-difftool.txt |   17 ++-
 git-difftool--helper.sh        |   20 +++-
 git-difftool.perl              |  257 +++++++++++++++++++++++++++-------------
 3 files changed, 200 insertions(+), 94 deletions(-)

-- 
1.7.9.1.290.gbd444

[PATCH 1/9] difftool: parse options using Getopt::Long

[Tim Henigan]

Replace custom option/argument parser with standard Getopt::Long
module.  This shortens the code and makes it easier to understand.

Signed-off-by: Tim Henigan <tim.henigan@gmail.com>
---
 git-difftool.perl |  109 +++++++++++++++++++++--------------------------------
 1 file changed, 44 insertions(+), 65 deletions(-)

diff --git a/git-difftool.perl b/git-difftool.perl
index 09b65f1..e365727 100755
--- a/git-difftool.perl
+++ b/git-difftool.perl
@@ -15,11 +15,8 @@ use strict;
 use warnings;
 use Cwd qw(abs_path);
 use File::Basename qw(dirname);
-
-require Git;
-
-my $DIR = abs_path(dirname($0));
-
+use Getopt::Long qw(:config pass_through);
+use Git;
 
 sub usage
 {
@@ -33,6 +30,7 @@ USAGE
 
 sub setup_environment
 {
+	my $DIR = abs_path(dirname($0));
 	$ENV{PATH} = "$DIR:$ENV{PATH}";
 	$ENV{GIT_PAGER} = '';
 	$ENV{GIT_EXTERNAL_DIFF} = 'git-difftool--helper';
@@ -47,75 +45,56 @@ sub exe
 	return $exe;
 }
 
-sub generate_command
-{
-	my @command = (exe('git'), 'diff');
-	my $skip_next = 0;
-	my $idx = -1;
-	my $prompt = '';
-	for my $arg (@ARGV) {
-		$idx++;
-		if ($skip_next) {
-			$skip_next = 0;
-			next;
-		}
-		if ($arg eq '-t' || $arg eq '--tool') {
-			usage() if $#ARGV <= $idx;
-			$ENV{GIT_DIFF_TOOL} = $ARGV[$idx + 1];
-			$skip_next = 1;
-			next;
-		}
-		if ($arg =~ /^--tool=/) {
-			$ENV{GIT_DIFF_TOOL} = substr($arg, 7);
-			next;
-		}
-		if ($arg eq '-x' || $arg eq '--extcmd') {
-			usage() if $#ARGV <= $idx;
-			$ENV{GIT_DIFFTOOL_EXTCMD} = $ARGV[$idx + 1];
-			$skip_next = 1;
-			next;
-		}
-		if ($arg =~ /^--extcmd=/) {
-			$ENV{GIT_DIFFTOOL_EXTCMD} = substr($arg, 9);
-			next;
-		}
-		if ($arg eq '-g' || $arg eq '--gui') {
-			eval {
-				my $tool = Git::command_oneline('config',
-				                                'diff.guitool');
-				if (length($tool)) {
-					$ENV{GIT_DIFF_TOOL} = $tool;
-				}
-			};
-			next;
-		}
-		if ($arg eq '-y' || $arg eq '--no-prompt') {
-			$prompt = 'no';
-			next;
-		}
-		if ($arg eq '--prompt') {
-			$prompt = 'yes';
-			next;
-		}
-		if ($arg eq '-h') {
-			usage();
-		}
-		push @command, $arg;
+# parse command-line options. all unrecognized options and arguments
+# are passed through to the 'git diff' command.
+my ($difftool_cmd, $extcmd, $gui, $help, $no_prompt, $prompt);
+GetOptions('g|gui' => \$gui,
+	'h' => \$help,
+	'prompt' => \$prompt,
+	't|tool:s' => \$difftool_cmd,
+	'x|extcmd:s' => \$extcmd,
+	'y|no-prompt' => \$no_prompt);
+
+if (defined($help)) {
+	usage();
+} 
+if (defined($difftool_cmd)) {
+	if (length($difftool_cmd) > 0) {
+		$ENV{GIT_DIFF_TOOL} = $difftool_cmd;
+	} else {
+		print "No <tool> given for --tool=<tool>\n";
+		usage();
 	}
-	if ($prompt eq 'yes') {
-		$ENV{GIT_DIFFTOOL_PROMPT} = 'true';
-	} elsif ($prompt eq 'no') {
-		$ENV{GIT_DIFFTOOL_NO_PROMPT} = 'true';
+}
+if (defined($extcmd)) {
+	if (length($extcmd) > 0) {
+		$ENV{GIT_DIFFTOOL_EXTCMD} = $extcmd;
+	} else {
+		print "No <cmd> given for --extcmd=<cmd>\n";
+		usage();
+	}
+}
+if (defined($gui)) {
+	my $guitool = "";
+	$guitool = Git::config('diff.guitool');
+	if (length($guitool) > 0) {
+		$ENV{GIT_DIFF_TOOL} = $guitool;
 	}
-	return @command
+}
+if (defined($prompt)) {
+	$ENV{GIT_DIFFTOOL_PROMPT} = 'true';
+}
+elsif (defined($no_prompt)) {
+	$ENV{GIT_DIFFTOOL_NO_PROMPT} = 'true';
 }
 
 setup_environment();
+my @command = (exe('git'), 'diff', @ARGV);
 
 # ActiveState Perl for Win32 does not implement POSIX semantics of
 # exec* system call. It just spawns the given executable and finishes
 # the starting program, exiting with code 0.
 # system will at least catch the errors returned by git diff,
 # allowing the caller of git difftool better handling of failures.
-my $rc = system(generate_command());
+my $rc = system(@command);
 exit($rc | ($rc >> 8));
-- 
1.7.9.1.290.gbd444

[PATCH 2/9] difftool: exit(0) when usage is printed

[Tim Henigan]

Prior to this commit, the script exited with an error whenever the
usage string was printed, regardless of the reason it was done. In
cases where usage was printed due to a user request (e.g. '-h'
option), the script should exit without error (exit 0).

This commit adds an argument to the usage function that allows the
exit code to be specified when the function is called.

Signed-off-by: Tim Henigan <tim.henigan@gmail.com>
---
 git-difftool.perl |    9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/git-difftool.perl b/git-difftool.perl
index e365727..81ecf34 100755
--- a/git-difftool.perl
+++ b/git-difftool.perl
@@ -20,12 +20,13 @@ use Git;
 
 sub usage
 {
+	my $exitcode = shift;
 	print << 'USAGE';
 usage: git difftool [-t|--tool=<tool>] [-x|--extcmd=<cmd>]
                     [-y|--no-prompt]   [-g|--gui]
                     ['git diff' options]
 USAGE
-	exit 1;
+	exit($exitcode);
 }
 
 sub setup_environment
@@ -56,14 +57,14 @@ GetOptions('g|gui' => \$gui,
 	'y|no-prompt' => \$no_prompt);
 
 if (defined($help)) {
-	usage();
+	usage(0);
 } 
 if (defined($difftool_cmd)) {
 	if (length($difftool_cmd) > 0) {
 		$ENV{GIT_DIFF_TOOL} = $difftool_cmd;
 	} else {
 		print "No <tool> given for --tool=<tool>\n";
-		usage();
+		usage(1);
 	}
 }
 if (defined($extcmd)) {
@@ -71,7 +72,7 @@ if (defined($extcmd)) {
 		$ENV{GIT_DIFFTOOL_EXTCMD} = $extcmd;
 	} else {
 		print "No <cmd> given for --extcmd=<cmd>\n";
-		usage();
+		usage(1);
 	}
 }
 if (defined($gui)) {
-- 
1.7.9.1.290.gbd444

[PATCH 3/9] difftool: remove explicit change of PATH

[Tim Henigan]

Adding the script directory to PATH is not needed. The script is
located at '$(git --exec-path)', which is already on the PATH.

Signed-off-by: Tim Henigan <tim.henigan@gmail.com>
---
 git-difftool.perl |    4 ----
 1 file changed, 4 deletions(-)

diff --git a/git-difftool.perl b/git-difftool.perl
index 81ecf34..53fcd7e 100755
--- a/git-difftool.perl
+++ b/git-difftool.perl
@@ -13,8 +13,6 @@
 use 5.008;
 use strict;
 use warnings;
-use Cwd qw(abs_path);
-use File::Basename qw(dirname);
 use Getopt::Long qw(:config pass_through);
 use Git;
 
@@ -31,8 +29,6 @@ USAGE
 
 sub setup_environment
 {
-	my $DIR = abs_path(dirname($0));
-	$ENV{PATH} = "$DIR:$ENV{PATH}";
 	$ENV{GIT_PAGER} = '';
 	$ENV{GIT_EXTERNAL_DIFF} = 'git-difftool--helper';
 }
-- 
1.7.9.1.290.gbd444

[PATCH 4/9] difftool: stop appending '.exe' to git

[Tim Henigan]

The system call to Git works the same whether or not ".exe" is
appended to "git". The extra code is not necessary.

Signed-off-by: Tim Henigan <tim.henigan@gmail.com>
---
 git-difftool.perl |   11 +----------
 1 file changed, 1 insertion(+), 10 deletions(-)

diff --git a/git-difftool.perl b/git-difftool.perl
index 53fcd7e..99ff587 100755
--- a/git-difftool.perl
+++ b/git-difftool.perl
@@ -33,15 +33,6 @@ sub setup_environment
 	$ENV{GIT_EXTERNAL_DIFF} = 'git-difftool--helper';
 }
 
-sub exe
-{
-	my $exe = shift;
-	if ($^O eq 'MSWin32' || $^O eq 'msys') {
-		return "$exe.exe";
-	}
-	return $exe;
-}
-
 # parse command-line options. all unrecognized options and arguments
 # are passed through to the 'git diff' command.
 my ($difftool_cmd, $extcmd, $gui, $help, $no_prompt, $prompt);
@@ -86,7 +77,7 @@ elsif (defined($no_prompt)) {
 }
 
 setup_environment();
-my @command = (exe('git'), 'diff', @ARGV);
+my @command = ('git', 'diff', @ARGV);
 
 # ActiveState Perl for Win32 does not implement POSIX semantics of
 # exec* system call. It just spawns the given executable and finishes
-- 
1.7.9.1.290.gbd444

Re: [PATCH 0/9] difftool: teach command to perform directory diffs

[Junio C Hamano]

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

> 'git difftool' is a very useful command that allows git diffs to be opened
> in an external tool. Currently, difftool opens a separate instance of the
> external tool for each file that changed. This can be tedious when many
> files have changed.
>
> This series teaches difftool to perform directory diffs, so that all file
> changes can be opened/reviewed in a single instance of the external tool.
>
> This is the second phase of development for this feature. The first phase
> was added as a separate command (git diffall) in 1252bbe (contrib: add
> git-diffall script).

Ummm.  I do not think the first step is not even done yet; fixing the
whitespace-in-pathspec and whitespace-or-lf-in-paths issues now the script
is in contrib/, while having people play with it.

Re: [PATCH 0/9] difftool: teach command to perform directory diffs

[Tim Henigan]

On Fri, Mar 16, 2012 at 10:06 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Tim Henigan <tim.henigan@gmail.com> writes:
>
>> This is the second phase of development for this feature. The first phase
>> was added as a separate command (git diffall) in 1252bbe (contrib: add
>> git-diffall script).
>
> Ummm.  I do not think the first step is not even done yet; fixing the
> whitespace-in-pathspec and whitespace-or-lf-in-paths issues now the script
> is in contrib/, while having people play with it.

I began fixing those issues in 'git-diffall' by rewriting the loop
that populates the tmp dirs in Perl.  By the time I changed the loop
to use 'git diff --raw -z' and handle working copy changes and
submodules, it seemed I was very near to finishing the things I wanted
to accomplish in "phase 2".  Finishing the work by updating 'difftool'
seemed to actually be easier than trying to update 'diffall'.

I realize the feature hasn't had any time to cook in
contrib/...perhaps I am over-enthusiastic.  However, the 'diffall'
concept has been in steady use by a small audience for nearly two
years (by people who downloaded/forked the GitHub project).  This past
week I was lucky enough to have some time to study the problem and
propose a solution.  If it needs to wait while 'diffall' cooks in
contrib, I will try to be more patient ;)

The changes in patches 1-6 may be useful regardless.  They cut the
length of the script in half without changing how it behaves.  The
only functional change in those patches is that `git difftool -h` now
finishes with exit(0) instead of exit(1).  I understand that may not
be enough reason to accept such a large change in working code...but
like I said, I was lucky enough to have time to study the problem in
detail this week.

Re: [PATCH 1/9] difftool: parse options using Getopt::Long

[David Aguilar]

On Fri, Mar 16, 2012 at 6:57 PM, Tim Henigan <tim.henigan@gmail.com> wrote:
> Replace custom option/argument parser with standard Getopt::Long
> module.  This shortens the code and makes it easier to understand.

I've also wanted to do the same in the past.  The one thing holding me
back was this note from the perldocs:

"If pass_through is also enabled, options processing will terminate at
the first unrecognized option, or non-option, whichever comes first."

http://search.cpan.org/~jv/Getopt-Long-2.38/lib/Getopt/Long.pm

That leads me to believe that using Getopt::Long would break this use case:

    $ git difftool --cached --tool xxdiff ...

According to the docs, it will stop at --cached and leave it (and the
rest) in @ARGV.  When git-diff runs it will see the --tool argument
and bail out.

Is this indeed the case?  I am a little ashamed that the difftool
tests do not cover this area.  That would be a valuable first step
towards exploring this approach, IMO.


BTW, I hate @ARGV parsing loops just as much as anyone!  I was not
ignorant of Getopt::Long, and no, I was not re-inventing the wheel for
no reason.  The reason it was done that way was so that we can forward
everything we don't know about to git-diff.

I haven't tried this patch, but my reading of the documentation leads
me to believe that this is a regression.



> Signed-off-by: Tim Henigan <tim.henigan@gmail.com>
> ---
>  git-difftool.perl |  109 +++++++++++++++++++++--------------------------------
>  1 file changed, 44 insertions(+), 65 deletions(-)
>
> diff --git a/git-difftool.perl b/git-difftool.perl
> index 09b65f1..e365727 100755
> --- a/git-difftool.perl
> +++ b/git-difftool.perl
> @@ -15,11 +15,8 @@ use strict;
>  use warnings;
>  use Cwd qw(abs_path);
>  use File::Basename qw(dirname);
> -
> -require Git;
> -
> -my $DIR = abs_path(dirname($0));
> -
> +use Getopt::Long qw(:config pass_through);
> +use Git;
>
>  sub usage
>  {
> @@ -33,6 +30,7 @@ USAGE
>
>  sub setup_environment
>  {
> +       my $DIR = abs_path(dirname($0));
>        $ENV{PATH} = "$DIR:$ENV{PATH}";
>        $ENV{GIT_PAGER} = '';
>        $ENV{GIT_EXTERNAL_DIFF} = 'git-difftool--helper';
> @@ -47,75 +45,56 @@ sub exe
>        return $exe;
>  }
>
> -sub generate_command
> -{
> -       my @command = (exe('git'), 'diff');
> -       my $skip_next = 0;
> -       my $idx = -1;
> -       my $prompt = '';
> -       for my $arg (@ARGV) {
> -               $idx++;
> -               if ($skip_next) {
> -                       $skip_next = 0;
> -                       next;
> -               }
> -               if ($arg eq '-t' || $arg eq '--tool') {
> -                       usage() if $#ARGV <= $idx;
> -                       $ENV{GIT_DIFF_TOOL} = $ARGV[$idx + 1];
> -                       $skip_next = 1;
> -                       next;
> -               }
> -               if ($arg =~ /^--tool=/) {
> -                       $ENV{GIT_DIFF_TOOL} = substr($arg, 7);
> -                       next;
> -               }
> -               if ($arg eq '-x' || $arg eq '--extcmd') {
> -                       usage() if $#ARGV <= $idx;
> -                       $ENV{GIT_DIFFTOOL_EXTCMD} = $ARGV[$idx + 1];
> -                       $skip_next = 1;
> -                       next;
> -               }
> -               if ($arg =~ /^--extcmd=/) {
> -                       $ENV{GIT_DIFFTOOL_EXTCMD} = substr($arg, 9);
> -                       next;
> -               }
> -               if ($arg eq '-g' || $arg eq '--gui') {
> -                       eval {
> -                               my $tool = Git::command_oneline('config',
> -                                                               'diff.guitool');
> -                               if (length($tool)) {
> -                                       $ENV{GIT_DIFF_TOOL} = $tool;
> -                               }
> -                       };
> -                       next;
> -               }
> -               if ($arg eq '-y' || $arg eq '--no-prompt') {
> -                       $prompt = 'no';
> -                       next;
> -               }
> -               if ($arg eq '--prompt') {
> -                       $prompt = 'yes';
> -                       next;
> -               }
> -               if ($arg eq '-h') {
> -                       usage();
> -               }
> -               push @command, $arg;
> +# parse command-line options. all unrecognized options and arguments
> +# are passed through to the 'git diff' command.
> +my ($difftool_cmd, $extcmd, $gui, $help, $no_prompt, $prompt);
> +GetOptions('g|gui' => \$gui,
> +       'h' => \$help,
> +       'prompt' => \$prompt,
> +       't|tool:s' => \$difftool_cmd,
> +       'x|extcmd:s' => \$extcmd,
> +       'y|no-prompt' => \$no_prompt);
> +
> +if (defined($help)) {
> +       usage();
> +}
> +if (defined($difftool_cmd)) {
> +       if (length($difftool_cmd) > 0) {
> +               $ENV{GIT_DIFF_TOOL} = $difftool_cmd;
> +       } else {
> +               print "No <tool> given for --tool=<tool>\n";
> +               usage();
>        }
> -       if ($prompt eq 'yes') {
> -               $ENV{GIT_DIFFTOOL_PROMPT} = 'true';
> -       } elsif ($prompt eq 'no') {
> -               $ENV{GIT_DIFFTOOL_NO_PROMPT} = 'true';
> +}
> +if (defined($extcmd)) {
> +       if (length($extcmd) > 0) {
> +               $ENV{GIT_DIFFTOOL_EXTCMD} = $extcmd;
> +       } else {
> +               print "No <cmd> given for --extcmd=<cmd>\n";
> +               usage();
> +       }
> +}
> +if (defined($gui)) {
> +       my $guitool = "";
> +       $guitool = Git::config('diff.guitool');
> +       if (length($guitool) > 0) {
> +               $ENV{GIT_DIFF_TOOL} = $guitool;
>        }
> -       return @command
> +}
> +if (defined($prompt)) {
> +       $ENV{GIT_DIFFTOOL_PROMPT} = 'true';
> +}
> +elsif (defined($no_prompt)) {
> +       $ENV{GIT_DIFFTOOL_NO_PROMPT} = 'true';
>  }
>
>  setup_environment();
> +my @command = (exe('git'), 'diff', @ARGV);
>
>  # ActiveState Perl for Win32 does not implement POSIX semantics of
>  # exec* system call. It just spawns the given executable and finishes
>  # the starting program, exiting with code 0.
>  # system will at least catch the errors returned by git diff,
>  # allowing the caller of git difftool better handling of failures.
> -my $rc = system(generate_command());
> +my $rc = system(@command);
>  exit($rc | ($rc >> 8));
> --
> 1.7.9.1.290.gbd444
>



-- 
David

Re: [PATCH 0/9] difftool: teach command to perform directory diffs

[Junio C Hamano]

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

> I began fixing those issues in 'git-diffall' by rewriting the loop
> that populates the tmp dirs in Perl.  By the time I changed the loop
> to use 'git diff --raw -z' and handle working copy changes and
> submodules, it seemed I was very near to finishing the things I wanted
> to accomplish in "phase 2".  Finishing the work by updating 'difftool'
> seemed to actually be easier than trying to update 'diffall'.

;-)  Good.

Re: [PATCH 1/9] difftool: parse options using Getopt::Long

[Tim Henigan]

On Fri, Mar 16, 2012 at 11:21 PM, David Aguilar <davvid@gmail.com> wrote:
> On Fri, Mar 16, 2012 at 6:57 PM, Tim Henigan <tim.henigan@gmail.com> wrote:
>
> I've also wanted to do the same in the past.  The one thing holding me
> back was this note from the perldocs:
>
> "If pass_through is also enabled, options processing will terminate at
> the first unrecognized option, or non-option, whichever comes first."
>
> http://search.cpan.org/~jv/Getopt-Long-2.38/lib/Getopt/Long.pm

That sentence is listed under the documentation of the 'permute'
option.  The documentation of 'pass_through' states that:

"Options that are unknown, ambiguous or supplied with an invalid
option value are passed through in @ARGV instead of being flagged as
errors. This makes it possible to write wrapper scripts that process
only part of the user supplied command line arguments, and pass the
remaining options to some other program.

If require_order is enabled, options processing will terminate at the
first unrecognized option, or non-option, whichever comes first.
However, if permute is enabled instead, results can become confusing."

So early termination would only be a problem if 'pass_through' was
enabled at the same time as 'require_order' or 'permute'.  To verify,
I confirmed that 'git difftool --cached --diff-dir' works as expected.


> Is this indeed the case?  I am a little ashamed that the difftool
> tests do not cover this area.  That would be a valuable first step
> towards exploring this approach, IMO.

I will review the the test cases.  If this goes forward, I still need
to add test cases to confirm the new '--dir-diff' option.


> BTW, I hate @ARGV parsing loops just as much as anyone!  I was not
> ignorant of Getopt::Long, and no, I was not re-inventing the wheel for
> no reason.  The reason it was done that way was so that we can forward
> everything we don't know about to git-diff.

I understand and was not implying anything different.  I simply
thought this would be a positive change.

[PATCH 0/9 v2] difftool: teach command to perform directory diffs

[Tim Henigan]

Changes in v2:
  [PATCH 7/9] difftool: teach difftool to handle directory diffs
    - Location of /tmp is now determined via the File::Temp::tempdir. In
      v1, the location was hard-coded to '/tmp'.
    - Lexical filehandles are now used for open().
    - Modern, 3 argument, form of open() is now used.
    - Arguments to the 'system' call for $extcmd are now given in list form.
    - Replaced 'system' call to 'git difftool--helper' with Git::command_noisy

  [PATCH 9/9] difftool: print list of valid tools with '--tool-help'
    - The glob statement used to find the mergetool options now uses the
      fully qualified path.  This insures that 'git difftool --tool-help'
      can be called from any directory.

Series overview:

'git difftool' is a very useful command that allows git diffs to be opened
in an external tool. Currently, difftool opens a separate instance of the
external tool for each file that changed. This can be tedious when many
files have changed.

This series teaches difftool to perform directory diffs, so that all file
changes can be opened/reviewed in a single instance of the external tool.

This is the second phase of development for this feature. The first phase
was added as a separate command (git diffall) in 1252bbe (contrib: add
git-diffall script). During review of that script on the Git developers
list, an informal development roadmap was suggested [1]. The next phase
of that plan is to integrate the 'git-diffall' feature into 'difftool'.
This series gets that done.

[1]: http://thread.gmane.org/gmane.comp.version-control.git/191297/focus=191383


Patches 1-6 are clean-up and rewrite to prepare for later changes. They
are not intended to change the behavior of difftool. Instead, they teach
the script to use standard modules for option parsing and interacting with
the Git repo. I understand that these changes may be controversial due to
the amount of rewrite, but the end result is that the script is half the
previous size and I believe is easier to understand and maintain.

Patches 7 and 8 are the core changes in this series. They teach difftool the
new "--dir-diff" option. When this option is used, difftool will copy files
that were changed to a temporary location and run a single directory diff on
them rather than run a separate instance of the diff tool for each file.

Patch 9 is a new implementation of a patch that I submitted recently.
061e672 (difftool: print list of valid tools with '--tool-help' was written
prior to these other changes. It needed to be rewritten to fit into the
Getopt::Long option parsing framework included with this series.

Tim Henigan (9):
  difftool: parse options using Getopt::Long
  difftool: exit(0) when usage is printed
  difftool: remove explicit change of PATH
  difftool: stop appending '.exe' to git
  difftool: eliminate setup_environment function
  difftool: replace system call with Git::command_noisy
  difftool: teach difftool to handle directory diffs
  difftool: teach dir-diff to copy modified files back to working tree
  difftool: print list of valid tools with '--tool-help'

 Documentation/git-difftool.txt |   17 ++-
 git-difftool--helper.sh        |   20 +++-
 git-difftool.perl              |  259 +++++++++++++++++++++++++++-------------
 3 files changed, 202 insertions(+), 94 deletions(-)

-- 
1.7.9.1.290.gbd444

[PATCH 1/9 v2] difftool: parse options using Getopt::Long

[Tim Henigan]

Replace custom option/argument parser with standard Getopt::Long
module.  This shortens the code and makes it easier to understand.

Signed-off-by: Tim Henigan <tim.henigan@gmail.com>
---
 git-difftool.perl |  109 +++++++++++++++++++++--------------------------------
 1 file changed, 44 insertions(+), 65 deletions(-)

diff --git a/git-difftool.perl b/git-difftool.perl
index 09b65f1..e365727 100755
--- a/git-difftool.perl
+++ b/git-difftool.perl
@@ -15,11 +15,8 @@ use strict;
 use warnings;
 use Cwd qw(abs_path);
 use File::Basename qw(dirname);
-
-require Git;
-
-my $DIR = abs_path(dirname($0));
-
+use Getopt::Long qw(:config pass_through);
+use Git;
 
 sub usage
 {
@@ -33,6 +30,7 @@ USAGE
 
 sub setup_environment
 {
+	my $DIR = abs_path(dirname($0));
 	$ENV{PATH} = "$DIR:$ENV{PATH}";
 	$ENV{GIT_PAGER} = '';
 	$ENV{GIT_EXTERNAL_DIFF} = 'git-difftool--helper';
@@ -47,75 +45,56 @@ sub exe
 	return $exe;
 }
 
-sub generate_command
-{
-	my @command = (exe('git'), 'diff');
-	my $skip_next = 0;
-	my $idx = -1;
-	my $prompt = '';
-	for my $arg (@ARGV) {
-		$idx++;
-		if ($skip_next) {
-			$skip_next = 0;
-			next;
-		}
-		if ($arg eq '-t' || $arg eq '--tool') {
-			usage() if $#ARGV <= $idx;
-			$ENV{GIT_DIFF_TOOL} = $ARGV[$idx + 1];
-			$skip_next = 1;
-			next;
-		}
-		if ($arg =~ /^--tool=/) {
-			$ENV{GIT_DIFF_TOOL} = substr($arg, 7);
-			next;
-		}
-		if ($arg eq '-x' || $arg eq '--extcmd') {
-			usage() if $#ARGV <= $idx;
-			$ENV{GIT_DIFFTOOL_EXTCMD} = $ARGV[$idx + 1];
-			$skip_next = 1;
-			next;
-		}
-		if ($arg =~ /^--extcmd=/) {
-			$ENV{GIT_DIFFTOOL_EXTCMD} = substr($arg, 9);
-			next;
-		}
-		if ($arg eq '-g' || $arg eq '--gui') {
-			eval {
-				my $tool = Git::command_oneline('config',
-				                                'diff.guitool');
-				if (length($tool)) {
-					$ENV{GIT_DIFF_TOOL} = $tool;
-				}
-			};
-			next;
-		}
-		if ($arg eq '-y' || $arg eq '--no-prompt') {
-			$prompt = 'no';
-			next;
-		}
-		if ($arg eq '--prompt') {
-			$prompt = 'yes';
-			next;
-		}
-		if ($arg eq '-h') {
-			usage();
-		}
-		push @command, $arg;
+# parse command-line options. all unrecognized options and arguments
+# are passed through to the 'git diff' command.
+my ($difftool_cmd, $extcmd, $gui, $help, $no_prompt, $prompt);
+GetOptions('g|gui' => \$gui,
+	'h' => \$help,
+	'prompt' => \$prompt,
+	't|tool:s' => \$difftool_cmd,
+	'x|extcmd:s' => \$extcmd,
+	'y|no-prompt' => \$no_prompt);
+
+if (defined($help)) {
+	usage();
+} 
+if (defined($difftool_cmd)) {
+	if (length($difftool_cmd) > 0) {
+		$ENV{GIT_DIFF_TOOL} = $difftool_cmd;
+	} else {
+		print "No <tool> given for --tool=<tool>\n";
+		usage();
 	}
-	if ($prompt eq 'yes') {
-		$ENV{GIT_DIFFTOOL_PROMPT} = 'true';
-	} elsif ($prompt eq 'no') {
-		$ENV{GIT_DIFFTOOL_NO_PROMPT} = 'true';
+}
+if (defined($extcmd)) {
+	if (length($extcmd) > 0) {
+		$ENV{GIT_DIFFTOOL_EXTCMD} = $extcmd;
+	} else {
+		print "No <cmd> given for --extcmd=<cmd>\n";
+		usage();
+	}
+}
+if (defined($gui)) {
+	my $guitool = "";
+	$guitool = Git::config('diff.guitool');
+	if (length($guitool) > 0) {
+		$ENV{GIT_DIFF_TOOL} = $guitool;
 	}
-	return @command
+}
+if (defined($prompt)) {
+	$ENV{GIT_DIFFTOOL_PROMPT} = 'true';
+}
+elsif (defined($no_prompt)) {
+	$ENV{GIT_DIFFTOOL_NO_PROMPT} = 'true';
 }
 
 setup_environment();
+my @command = (exe('git'), 'diff', @ARGV);
 
 # ActiveState Perl for Win32 does not implement POSIX semantics of
 # exec* system call. It just spawns the given executable and finishes
 # the starting program, exiting with code 0.
 # system will at least catch the errors returned by git diff,
 # allowing the caller of git difftool better handling of failures.
-my $rc = system(generate_command());
+my $rc = system(@command);
 exit($rc | ($rc >> 8));
-- 
1.7.9.1.290.gbd444

[PATCH 2/9 v2] difftool: exit(0) when usage is printed

[Tim Henigan]

Prior to this commit, the script exited with an error whenever the
usage string was printed, regardless of the reason it was done. In
cases where usage was printed due to a user request (e.g. '-h'
option), the script should exit without error (exit 0).

This commit adds an argument to the usage function that allows the
exit code to be specified when the function is called.

Signed-off-by: Tim Henigan <tim.henigan@gmail.com>
---
 git-difftool.perl |    9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/git-difftool.perl b/git-difftool.perl
index e365727..81ecf34 100755
--- a/git-difftool.perl
+++ b/git-difftool.perl
@@ -20,12 +20,13 @@ use Git;
 
 sub usage
 {
+	my $exitcode = shift;
 	print << 'USAGE';
 usage: git difftool [-t|--tool=<tool>] [-x|--extcmd=<cmd>]
                     [-y|--no-prompt]   [-g|--gui]
                     ['git diff' options]
 USAGE
-	exit 1;
+	exit($exitcode);
 }
 
 sub setup_environment
@@ -56,14 +57,14 @@ GetOptions('g|gui' => \$gui,
 	'y|no-prompt' => \$no_prompt);
 
 if (defined($help)) {
-	usage();
+	usage(0);
 } 
 if (defined($difftool_cmd)) {
 	if (length($difftool_cmd) > 0) {
 		$ENV{GIT_DIFF_TOOL} = $difftool_cmd;
 	} else {
 		print "No <tool> given for --tool=<tool>\n";
-		usage();
+		usage(1);
 	}
 }
 if (defined($extcmd)) {
@@ -71,7 +72,7 @@ if (defined($extcmd)) {
 		$ENV{GIT_DIFFTOOL_EXTCMD} = $extcmd;
 	} else {
 		print "No <cmd> given for --extcmd=<cmd>\n";
-		usage();
+		usage(1);
 	}
 }
 if (defined($gui)) {
-- 
1.7.9.1.290.gbd444

[PATCH 3/9 v2] difftool: remove explicit change of PATH

[Tim Henigan]

Adding the script directory to PATH is not needed. The script is
located at '$(git --exec-path)', which is already on the PATH.

Signed-off-by: Tim Henigan <tim.henigan@gmail.com>
---
 git-difftool.perl |    4 ----
 1 file changed, 4 deletions(-)

diff --git a/git-difftool.perl b/git-difftool.perl
index 81ecf34..53fcd7e 100755
--- a/git-difftool.perl
+++ b/git-difftool.perl
@@ -13,8 +13,6 @@
 use 5.008;
 use strict;
 use warnings;
-use Cwd qw(abs_path);
-use File::Basename qw(dirname);
 use Getopt::Long qw(:config pass_through);
 use Git;
 
@@ -31,8 +29,6 @@ USAGE
 
 sub setup_environment
 {
-	my $DIR = abs_path(dirname($0));
-	$ENV{PATH} = "$DIR:$ENV{PATH}";
 	$ENV{GIT_PAGER} = '';
 	$ENV{GIT_EXTERNAL_DIFF} = 'git-difftool--helper';
 }
-- 
1.7.9.1.290.gbd444

[PATCH 4/9 v2] difftool: stop appending '.exe' to git

[Tim Henigan]

The system call to Git works the same whether or not ".exe" is
appended to "git". The extra code is not necessary.

Signed-off-by: Tim Henigan <tim.henigan@gmail.com>
---
 git-difftool.perl |   11 +----------
 1 file changed, 1 insertion(+), 10 deletions(-)

diff --git a/git-difftool.perl b/git-difftool.perl
index 53fcd7e..99ff587 100755
--- a/git-difftool.perl
+++ b/git-difftool.perl
@@ -33,15 +33,6 @@ sub setup_environment
 	$ENV{GIT_EXTERNAL_DIFF} = 'git-difftool--helper';
 }
 
-sub exe
-{
-	my $exe = shift;
-	if ($^O eq 'MSWin32' || $^O eq 'msys') {
-		return "$exe.exe";
-	}
-	return $exe;
-}
-
 # parse command-line options. all unrecognized options and arguments
 # are passed through to the 'git diff' command.
 my ($difftool_cmd, $extcmd, $gui, $help, $no_prompt, $prompt);
@@ -86,7 +77,7 @@ elsif (defined($no_prompt)) {
 }
 
 setup_environment();
-my @command = (exe('git'), 'diff', @ARGV);
+my @command = ('git', 'diff', @ARGV);
 
 # ActiveState Perl for Win32 does not implement POSIX semantics of
 # exec* system call. It just spawns the given executable and finishes
-- 
1.7.9.1.290.gbd444

[PATCH 5/9 v2] difftool: eliminate setup_environment function

[Tim Henigan]

Removing this function shortens the code and makes it easier to read.
Now all environment variables are set as part of procedural operation.

Signed-off-by: Tim Henigan <tim.henigan@gmail.com>
---
 git-difftool.perl |    9 ++-------
 1 file changed, 2 insertions(+), 7 deletions(-)

diff --git a/git-difftool.perl b/git-difftool.perl
index 99ff587..9495f14 100755
--- a/git-difftool.perl
+++ b/git-difftool.perl
@@ -27,12 +27,6 @@ USAGE
 	exit($exitcode);
 }
 
-sub setup_environment
-{
-	$ENV{GIT_PAGER} = '';
-	$ENV{GIT_EXTERNAL_DIFF} = 'git-difftool--helper';
-}
-
 # parse command-line options. all unrecognized options and arguments
 # are passed through to the 'git diff' command.
 my ($difftool_cmd, $extcmd, $gui, $help, $no_prompt, $prompt);
@@ -76,7 +70,8 @@ elsif (defined($no_prompt)) {
 	$ENV{GIT_DIFFTOOL_NO_PROMPT} = 'true';
 }
 
-setup_environment();
+$ENV{GIT_PAGER} = '';
+$ENV{GIT_EXTERNAL_DIFF} = 'git-difftool--helper';
 my @command = ('git', 'diff', @ARGV);
 
 # ActiveState Perl for Win32 does not implement POSIX semantics of
-- 
1.7.9.1.290.gbd444

[PATCH 6/9 v2] difftool: replace system call with Git::command_noisy

[Tim Henigan]

The Git.pm module includes functions intended to standardize working
with Git repositories in Perl scripts. This commit teaches difftool
to use Git::command_noisy rather than a system call to run the diff
command.

Signed-off-by: Tim Henigan <tim.henigan@gmail.com>
---
 git-difftool.perl |   10 +---------
 1 file changed, 1 insertion(+), 9 deletions(-)

diff --git a/git-difftool.perl b/git-difftool.perl
index 9495f14..8498089 100755
--- a/git-difftool.perl
+++ b/git-difftool.perl
@@ -72,12 +72,4 @@ elsif (defined($no_prompt)) {
 
 $ENV{GIT_PAGER} = '';
 $ENV{GIT_EXTERNAL_DIFF} = 'git-difftool--helper';
-my @command = ('git', 'diff', @ARGV);
-
-# ActiveState Perl for Win32 does not implement POSIX semantics of
-# exec* system call. It just spawns the given executable and finishes
-# the starting program, exiting with code 0.
-# system will at least catch the errors returned by git diff,
-# allowing the caller of git difftool better handling of failures.
-my $rc = system(@command);
-exit($rc | ($rc >> 8));
+git_cmd_try { Git::command_noisy(('diff', @ARGV)) } 'exit code %d';
-- 
1.7.9.1.290.gbd444

[PATCH 7/9 v2] difftool: teach difftool to handle directory diffs

[Tim Henigan]

Prior to this commit, the difftool utility could only open files one
at a time.  The new '--dir-diff' option copies all the modified files
to a temporary location and runs a directory diff on them.

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

Changes in v2:
    - Location of /tmp is now determined via the File::Temp::tempdir. In
      v1, the location was hard-coded to '/tmp'.
    - Lexical filehandles are now used for open().
    - Modern, 3 argument, form of open() is now used.
    - Arguments to the 'system' call for $extcmd are now given in list form.
    - Replaced 'system' call to 'git difftool--helper' with Git::command_noisy


 Documentation/git-difftool.txt |    6 ++
 git-difftool--helper.sh        |   20 ++++--
 git-difftool.perl              |  144 ++++++++++++++++++++++++++++++++++++----
 3 files changed, 152 insertions(+), 18 deletions(-)

diff --git a/Documentation/git-difftool.txt b/Documentation/git-difftool.txt
index fe38f66..aba5e76 100644
--- a/Documentation/git-difftool.txt
+++ b/Documentation/git-difftool.txt
@@ -19,6 +19,12 @@ linkgit:git-diff[1].
 
 OPTIONS
 -------
+-d::
+--dir-diff::
+	Copy the modified files to a temporary location and perform
+	a directory diff on them. This mode never prompts before
+	launching the diff tool.
+
 -y::
 --no-prompt::
 	Do not prompt before launching a diff tool.
diff --git a/git-difftool--helper.sh b/git-difftool--helper.sh
index e6558d1..bd0556f 100755
--- a/git-difftool--helper.sh
+++ b/git-difftool--helper.sh
@@ -6,6 +6,7 @@
 # Copyright (c) 2009, 2010 David Aguilar
 
 TOOL_MODE=diff
+SUBDIRECTORY_OK=1
 . git-mergetool--lib
 
 # difftool.prompt controls the default prompt/no-prompt behavior
@@ -73,9 +74,16 @@ then
 	fi
 fi
 
-# Launch the merge tool on each path provided by 'git diff'
-while test $# -gt 6
-do
-	launch_merge_tool "$1" "$2" "$5"
-	shift 7
-done
+if test -n "$GIT_DIFFTOOL_DIRDIFF"
+then
+	LOCAL="$1"
+	REMOTE="$2"
+	run_merge_tool "$merge_tool" false
+else
+	# Launch the merge tool on each path provided by 'git diff'
+	while test $# -gt 6
+	do
+		launch_merge_tool "$1" "$2" "$5"
+		shift 7
+	done
+fi
diff --git a/git-difftool.perl b/git-difftool.perl
index 8498089..1f7b8f2 100755
--- a/git-difftool.perl
+++ b/git-difftool.perl
@@ -1,18 +1,24 @@
 #!/usr/bin/env perl
 # Copyright (c) 2009, 2010 David Aguilar
+# Copyright (c) 2012 Tim Henigan
 #
 # This is a wrapper around the GIT_EXTERNAL_DIFF-compatible
 # git-difftool--helper script.
 #
 # This script exports GIT_EXTERNAL_DIFF and GIT_PAGER for use by git.
-# GIT_DIFFTOOL_NO_PROMPT, GIT_DIFFTOOL_PROMPT, and GIT_DIFF_TOOL
-# are exported for use by git-difftool--helper.
+# GIT_DIFFTOOL_NO_PROMPT, GIT_DIFFTOOL_PROMPT, GIT_DIFFTOOL_DIRDIFF,
+# and GIT_DIFF_TOOL are exported for use by git-difftool--helper.
 #
 # Any arguments that are unknown to this script are forwarded to 'git diff'.
 
 use 5.008;
 use strict;
 use warnings;
+use File::Basename qw(dirname);
+use File::Copy;
+use File::stat;
+use File::Path qw(mkpath);
+use File::Temp qw(tempdir);
 use Getopt::Long qw(:config pass_through);
 use Git;
 
@@ -22,15 +28,109 @@ sub usage
 	print << 'USAGE';
 usage: git difftool [-t|--tool=<tool>] [-x|--extcmd=<cmd>]
                     [-y|--no-prompt]   [-g|--gui]
+                    [-d|--dir-diff]
                     ['git diff' options]
 USAGE
 	exit($exitcode);
 }
 
+sub setup_dir_diff
+{
+	# Run the diff; exit immediately if no diff found
+	my $repo = Git->repository();
+	my $diffrtn = $repo->command_oneline(['diff', '--raw', '--no-abbrev', '-z', @ARGV]);
+	exit(0) if (length($diffrtn) == 0);
+
+	# Setup temp directories
+	my $tmpdir = tempdir('git-diffall.XXXXX', CLEANUP => 1, TMPDIR => 1);
+	my $ldir = "$tmpdir/left";
+	my $rdir = "$tmpdir/right";
+	mkpath($ldir) or die $!;
+	mkpath($rdir) or die $!;
+
+	# Build index info for left and right sides of the diff
+	my $submodule_mode = "160000";
+	my $null_mode = 0 x 6;
+	my $null_sha1 = 0 x 40;
+	my $lindex = "";
+	my $rindex = "";
+	my @working_tree;
+	my %submodule;
+	my @rawdiff = split('\0', $diffrtn);
+
+	for (my $i=0; $i<@rawdiff; $i+=2) {
+		my ($lmode, $rmode, $lsha1, $rsha1, $status) = split(' ', substr($rawdiff[$i], 1));
+		my $path = $rawdiff[$i + 1];
+
+		if (($lmode eq $submodule_mode) or ($rmode eq $submodule_mode)) {
+			$submodule{$path}{left} = $lsha1;
+			$submodule{$path}{right} = $rsha1;
+			next;
+		}
+
+		if ($lmode ne $null_mode) {
+			$lindex .= "$lmode $lsha1\t$path\0";
+		}
+
+		if ($rmode ne $null_mode) {
+			if ($rsha1 ne $null_sha1) {
+				$rindex .= "$rmode $rsha1\t$path\0";
+			} else {
+				push(@working_tree, $path);
+			}
+		}
+	}
+
+	# Populate the left and right directories based on each index file
+	my ($inpipe, $ctx);
+	$ENV{GIT_INDEX_FILE} = "$tmpdir/lindex";
+	($inpipe, $ctx) = $repo->command_input_pipe(qw/update-index -z --index-info/);
+	print($inpipe $lindex);
+	$repo->command_close_pipe($inpipe, $ctx);
+	$repo->command_oneline(["checkout-index", "-a", "--prefix=$ldir/"]);
+
+	$ENV{GIT_INDEX_FILE} = "$tmpdir/rindex";
+	($inpipe, $ctx) = $repo->command_input_pipe(qw/update-index -z --index-info/);
+	print($inpipe $rindex);
+	$repo->command_close_pipe($inpipe, $ctx);
+	$repo->command_oneline(["checkout-index", "-a", "--prefix=$rdir/"]);
+
+	# Changes in the working tree need special treatment since they are
+	# not part of the index
+	my $workdir = $repo->wc_path();
+	for (@working_tree) {
+		my $dir = dirname($_);
+		unless (-d "$rdir/$dir") {
+			mkpath("$rdir/$dir") or die $!;
+		}
+		copy("$workdir/$_", "$rdir/$_") or die $!;
+		chmod(stat("$workdir/$_")->mode, "$rdir/$_") or die $!;
+	}
+
+	# Changes to submodules require special treatment. This loop writes a
+	# temporary file to both the left and right directories to show the
+	# change in the recorded SHA1 for the submodule.
+	foreach my $path (keys %submodule) {
+		if (defined $submodule{$path}{left}) {
+			open(my $fh, ">", "$ldir/$path") or die $!;
+			print($fh "Subproject commit $submodule{$path}{left}");
+			close($fh);
+		}
+		if (defined $submodule{$path}{right}) {
+			open(my $fh, ">", "$rdir/$path") or die $!;
+			print($fh "Subproject commit $submodule{$path}{right}");
+			close($fh);
+		}
+	}
+
+	return ($ldir, $rdir);
+}
+
 # parse command-line options. all unrecognized options and arguments
 # are passed through to the 'git diff' command.
-my ($difftool_cmd, $extcmd, $gui, $help, $no_prompt, $prompt);
+my ($difftool_cmd, $dirdiff, $extcmd, $gui, $help, $no_prompt, $prompt);
 GetOptions('g|gui' => \$gui,
+	'd|dir-diff' => \$dirdiff,
 	'h' => \$help,
 	'prompt' => \$prompt,
 	't|tool:s' => \$difftool_cmd,
@@ -63,13 +163,33 @@ if (defined($gui)) {
 		$ENV{GIT_DIFF_TOOL} = $guitool;
 	}
 }
-if (defined($prompt)) {
-	$ENV{GIT_DIFFTOOL_PROMPT} = 'true';
-}
-elsif (defined($no_prompt)) {
-	$ENV{GIT_DIFFTOOL_NO_PROMPT} = 'true';
-}
 
-$ENV{GIT_PAGER} = '';
-$ENV{GIT_EXTERNAL_DIFF} = 'git-difftool--helper';
-git_cmd_try { Git::command_noisy(('diff', @ARGV)) } 'exit code %d';
+# In directory diff mode, 'git-difftool--helper' is called once
+# to compare the a/b directories.  In file diff mode, 'git diff'
+# will invoke a separate instance of 'git-difftool--helper' for
+# each file that changed.
+if (defined($dirdiff)) {
+	my ($a, $b) = setup_dir_diff();
+	if (defined($extcmd)) {
+		system(($extcmd, $a, $b));
+	} else {
+		$ENV{GIT_DIFFTOOL_DIRDIFF} = 'true';
+		git_cmd_try {
+			Git::command_noisy(('difftool--helper', $a, $b))
+		} 'exit code %d';
+	}
+	# TODO: if the diff including working copy files and those
+	# files were modified during the diff, then the changes
+	# should be copied back to the working tree
+} else {
+	if (defined($prompt)) {
+		$ENV{GIT_DIFFTOOL_PROMPT} = 'true';
+	}
+	elsif (defined($no_prompt)) {
+		$ENV{GIT_DIFFTOOL_NO_PROMPT} = 'true';
+	}
+
+	$ENV{GIT_PAGER} = '';
+	$ENV{GIT_EXTERNAL_DIFF} = 'git-difftool--helper';
+	git_cmd_try { Git::command_noisy(('diff', @ARGV)) } 'exit code %d';
+}
-- 
1.7.9.1.290.gbd444

[PATCH 8/9 v2] difftool: teach dir-diff to copy modified files back to working tree

[Tim Henigan]

If the user runs a diff when there are modified files in the working copy,
they expect to be able to be edit the files in the diff viewer and save
the changes. When using difftool in file mode (i.e. diffing one file at
a time), this works as expected.  However, when using difftool in directory
diff mode, the changes are not saved.

This commit teaches the directory diff mode to copy changes from the diff
viewer back to the working copy.

Signed-off-by: Tim Henigan <tim.henigan@gmail.com>
---
 git-difftool.perl |   12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/git-difftool.perl b/git-difftool.perl
index 1f7b8f2..e306977 100755
--- a/git-difftool.perl
+++ b/git-difftool.perl
@@ -22,6 +22,8 @@ use File::Temp qw(tempdir);
 use Getopt::Long qw(:config pass_through);
 use Git;
 
+my @working_tree;
+
 sub usage
 {
 	my $exitcode = shift;
@@ -54,7 +56,6 @@ sub setup_dir_diff
 	my $null_sha1 = 0 x 40;
 	my $lindex = "";
 	my $rindex = "";
-	my @working_tree;
 	my %submodule;
 	my @rawdiff = split('\0', $diffrtn);
 
@@ -178,9 +179,16 @@ if (defined($dirdiff)) {
 			Git::command_noisy(('difftool--helper', $a, $b))
 		} 'exit code %d';
 	}
-	# TODO: if the diff including working copy files and those
+
+	# If the diff including working copy files and those
 	# files were modified during the diff, then the changes
 	# should be copied back to the working tree
+	my $repo = Git->repository();
+	my $workdir = $repo->wc_path();
+	for (@working_tree) {
+		copy("$b/$_", "$workdir/$_") or die $!;
+		chmod(stat("$b/$_")->mode, "$workdir/$_") or die $!;
+	}
 } else {
 	if (defined($prompt)) {
 		$ENV{GIT_DIFFTOOL_PROMPT} = 'true';
-- 
1.7.9.1.290.gbd444

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

[Tim Henigan]

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

Teach the command to read the list of valid tools from the 'mergetools'
directory and print them for the user when the '--tool-help' option is
given.

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

Changes in v2:
  The glob statement used to find the mergetool options now uses the
  fully qualified path.  This insures that 'git difftool --tool-help'
  can be called from any diretory.

 Documentation/git-difftool.txt |   11 ++++++-----
 git-difftool.perl              |   17 ++++++++++++++---
 2 files changed, 20 insertions(+), 8 deletions(-)


diff --git a/Documentation/git-difftool.txt b/Documentation/git-difftool.txt
index aba5e76..31fc2e3 100644
--- a/Documentation/git-difftool.txt
+++ b/Documentation/git-difftool.txt
@@ -36,11 +36,9 @@ OPTIONS
 
 -t <tool>::
 --tool=<tool>::
-	Use the diff tool specified by <tool>.
-	Valid diff tools are:
-	araxis, bc3, deltawalker, diffuse, emerge, ecmerge, gvimdiff,
-	kdiff3,	kompare, meld, opendiff, p4merge, tkdiff, vimdiff and
-	xxdiff.
+	Use the diff tool specified by <tool>.  Valid values include
+	emerge, kompare, meld, and vimdiff. Run `git difftool --tool-help`
+	for the list of valid <tool> settings.
 +
 If a diff tool is not specified, 'git difftool'
 will use the configuration variable `diff.tool`.  If the
@@ -68,6 +66,9 @@ of the diff post-image.  `$MERGED` is the name of the file which is
 being compared. `$BASE` is provided for compatibility
 with custom merge tool commands and has the same value as `$MERGED`.
 
+--tool-help::
+	Print a list of diff tools that may be used with `--tool`.
+
 -x <command>::
 --extcmd=<command>::
 	Specify a custom command for viewing diffs.
diff --git a/git-difftool.perl b/git-difftool.perl
index e306977..fb4e3e6 100755
--- a/git-difftool.perl
+++ b/git-difftool.perl
@@ -14,7 +14,7 @@
 use 5.008;
 use strict;
 use warnings;
-use File::Basename qw(dirname);
+use File::Basename qw(dirname basename);
 use File::Copy;
 use File::stat;
 use File::Path qw(mkpath);
@@ -28,7 +28,8 @@ sub usage
 {
 	my $exitcode = shift;
 	print << 'USAGE';
-usage: git difftool [-t|--tool=<tool>] [-x|--extcmd=<cmd>]
+usage: git difftool [-t|--tool=<tool>] [--tool-help]
+                    [-x|--extcmd=<cmd>]
                     [-y|--no-prompt]   [-g|--gui]
                     [-d|--dir-diff]
                     ['git diff' options]
@@ -129,18 +130,28 @@ sub setup_dir_diff
 
 # parse command-line options. all unrecognized options and arguments
 # are passed through to the 'git diff' command.
-my ($difftool_cmd, $dirdiff, $extcmd, $gui, $help, $no_prompt, $prompt);
+my ($difftool_cmd, $dirdiff, $extcmd, $gui, $help, $no_prompt, $prompt, $tool_help);
 GetOptions('g|gui' => \$gui,
 	'd|dir-diff' => \$dirdiff,
 	'h' => \$help,
 	'prompt' => \$prompt,
 	't|tool:s' => \$difftool_cmd,
+	'tool-help' => \$tool_help,
 	'x|extcmd:s' => \$extcmd,
 	'y|no-prompt' => \$no_prompt);
 
 if (defined($help)) {
 	usage(0);
 } 
+if (defined($tool_help)) {
+	my $gitpath = Git::exec_path();
+	print "'git difftool --tool=<tool>' may be set to one of the following:\n";
+	for (glob "$gitpath/mergetools/*") {
+		next if /defaults$/;
+		print "\t" . basename($_) . "\n";
+	}
+	exit(0);
+}
 if (defined($difftool_cmd)) {
 	if (length($difftool_cmd) > 0) {
 		$ENV{GIT_DIFF_TOOL} = $difftool_cmd;
-- 
1.7.9.1.290.gbd444

Re: [PATCH 1/9] difftool: parse options using Getopt::Long

[David Aguilar]

On Sat, Mar 17, 2012 at 6:54 AM, Tim Henigan <tim.henigan@gmail.com> wrote:
> On Fri, Mar 16, 2012 at 11:21 PM, David Aguilar <davvid@gmail.com> wrote:
>> On Fri, Mar 16, 2012 at 6:57 PM, Tim Henigan <tim.henigan@gmail.com> wrote:
>>
>> I've also wanted to do the same in the past.  The one thing holding me
>> back was this note from the perldocs:
>>
>> "If pass_through is also enabled, options processing will terminate at
>> the first unrecognized option, or non-option, whichever comes first."
>>
>> http://search.cpan.org/~jv/Getopt-Long-2.38/lib/Getopt/Long.pm
>
> That sentence is listed under the documentation of the 'permute'
> option.  The documentation of 'pass_through' states that:
>
> "Options that are unknown, ambiguous or supplied with an invalid
> option value are passed through in @ARGV instead of being flagged as
> errors. This makes it possible to write wrapper scripts that process
> only part of the user supplied command line arguments, and pass the
> remaining options to some other program.
>
> If require_order is enabled, options processing will terminate at the
> first unrecognized option, or non-option, whichever comes first.
> However, if permute is enabled instead, results can become confusing."
>
> So early termination would only be a problem if 'pass_through' was
> enabled at the same time as 'require_order' or 'permute'.  To verify,
> I confirmed that 'git difftool --cached --diff-dir' works as expected.
>
>
>> Is this indeed the case?  I am a little ashamed that the difftool
>> tests do not cover this area.  That would be a valuable first step
>> towards exploring this approach, IMO.
>
> I will review the the test cases.  If this goes forward, I still need
> to add test cases to confirm the new '--dir-diff' option.
>
>
>> BTW, I hate @ARGV parsing loops just as much as anyone!  I was not
>> ignorant of Getopt::Long, and no, I was not re-inventing the wheel for
>> no reason.  The reason it was done that way was so that we can forward
>> everything we don't know about to git-diff.
>
> I understand and was not implying anything different.  I simply
> thought this would be a positive change.

Hehehe, well apparently I was ignorant about the permute_order and
pass_through interaction afterall ;-)  Thanks for the careful reading
of the docs.  I should have done that sooner.

I highly appreciate your rework of this stuff -- this cleanup is quite
nice.  ++Getopt::Long.

I sent a patch testing the "forward options to diff" behavior sometime
yesterday(?) so we should be good on that side.  Testing the diff-diff
stuff will be good.

Thanks a lot for this, it's a sweet feature and I know the users I
help will get a lot of mileage out of it.
-- 
David

Re: [PATCH 0/9 v2] difftool: teach command to perform directory diffs

[Junio C Hamano]

Thanks.

I do not know a difftool user, and I wasn't paying close attention to the
discussion but I recall these points raised and I do not recall the
resolutions:

 * In [1/9], use of pass_through would mean 'git difftool' must be fed the
   options for difftool and then options meant for underlying diff
   machinery.  Is this limitation still there?  If so, isn't this a
   regression?  Shouldn't it at least be advertised to the users a lot
   stronger in documentation?

 * In [4/9], you remove the .exe extension when running git, which was
   there since the beginning of difftool 5c38ea3 (contrib: add 'git
   difftool' for launching common merge tools, 2009-01-16).  I think it is
   safe but are Windows folks OK with this?

 * In [6/9], the exit code in the failure case seem to be modified from
   that of the underlying "git diff" to whatever croak gives us.  Is this
   a regression, or nobody cares error status from difftool?  I personally
   suspect it is the latter, but just double-checking if you have
   considered it.

 * In [7/9], difftool--helper declares SUBDIRECTORY_OK, but there doesn't
   seem to be any inclusion of git-sh-setup in this script, and the patch
   does not have any effort to prepend $prefix to paths relative to $cwd.
   What good does the variable do here?

Re: [PATCH 0/9 v2] difftool: teach command to perform directory diffs

[David Aguilar]

On Mon, Mar 19, 2012 at 2:00 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Thanks.
>
> I do not know a difftool user, and I wasn't paying close attention to the
> discussion but I recall these points raised and I do not recall the
> resolutions:
>
>  * In [1/9], use of pass_through would mean 'git difftool' must be fed the
>   options for difftool and then options meant for underlying diff
>   machinery.  Is this limitation still there?  If so, isn't this a
>   regression?  Shouldn't it at least be advertised to the users a lot
>   stronger in documentation?

Tim asserted that this is not the case.  The options should pass
through.  Hopefully there aren't any behavior changes between perl
versions and this option.

I sent a patch adding a test case to cover this scenario.  I would
prefer that we avoid a regression.  If there's a regression then we
can do without Getopt::Long, IMO.

I believe users should be oblivious as to which options are for
difftool and which are for diff.  E.g. `git difftool --cached -t
xxdiff` -- the user does not care that --cached is for diff and -t is
for difftool.

>  * In [4/9], you remove the .exe extension when running git, which was
>   there since the beginning of difftool 5c38ea3 (contrib: add 'git
>   difftool' for launching common merge tools, 2009-01-16).  I think it is
>   safe but are Windows folks OK with this?

I do not have Windows to test with, but this supposedly works since
Git.pm does not use git.exe either.  The git.exe stuff was originally
there because difftool originally did exec('git.exe', ...).  It was
later changed to use system() and it's possible that we could have
dropped the .exe extension at that time.

But, like I said, I don't have any Windows machines with which to
verify this behavior, and highly appreciate feedback from Windows
folks.


>  * In [6/9], the exit code in the failure case seem to be modified from
>   that of the underlying "git diff" to whatever croak gives us.  Is this
>   a regression, or nobody cares error status from difftool?  I personally
>   suspect it is the latter, but just double-checking if you have
>   considered it.

This doesn't seem like too big of an issue.  Had we documented the old
exit codes then it would be a bigger concern.  I would personally
prefer to preserve the exit code from diff itself, but if that
complicates it too much then the new behavior is ok.


>  * In [7/9], difftool--helper declares SUBDIRECTORY_OK, but there doesn't
>   seem to be any inclusion of git-sh-setup in this script, and the patch
>   does not have any effort to prepend $prefix to paths relative to $cwd.
>   What good does the variable do here?

I'll defer to Tim on this one.  This seems like an oversight.  It
seems like something should be done to handle it.

I recall that we made $GIT_PREFIX available to aliases and builtins
not too long ago.  That may help here.  See
1f5d271f5e8f7b1e2a5b296ff43ca4087eb08244.

Also.. I think we need some tests to cover the new behavior.  A test
to cover the subdirectory behavior would be especially helpful given
the note about [7/9].
-- 
David

Re: [PATCH 0/9 v2] difftool: teach command to perform directory diffs

[Junio C Hamano]

David Aguilar <davvid@gmail.com> writes:

> On Mon, Mar 19, 2012 at 2:00 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> Thanks.
>>
>> I do not know a difftool user, and I wasn't paying close attention to the
>> discussion but I recall these points raised and I do not recall the
>> resolutions:
>>
>>  * In [1/9], use of pass_through would mean 'git difftool' must be fed the
>>   options for difftool and then options meant for underlying diff
>>   machinery.  Is this limitation still there?  If so, isn't this a
>>   regression?  Shouldn't it at least be advertised to the users a lot
>>   stronger in documentation?
>
> Tim asserted that this is not the case.  The options should pass
> through.  Hopefully there aren't any behavior changes between perl
> versions and this option.
>
> I sent a patch adding a test case to cover this scenario.  I would
> prefer that we avoid a regression.  If there's a regression then we
> can do without Getopt::Long, IMO.

Yeah.  Does tonight's 'pu' pass for you?

> I do not have Windows to test with, but this supposedly works since
> Git.pm does not use git.exe either.

Yeah, that matches my guess.

>>  * In [7/9], difftool--helper declares SUBDIRECTORY_OK, but there doesn't
>>   seem to be any inclusion of git-sh-setup in this script, and the patch
>>   does not have any effort to prepend $prefix to paths relative to $cwd.
>>   What good does the variable do here?
>
> I'll defer to Tim on this one.  This seems like an oversight.  It
> seems like something should be done to handle it.

Well, it does not seem to dot-source git-sh-setup so it probably stays
where it was launched, so in that case there is nothing that needs to be
done, including SUBDIRECTORY_OK which nobody would look at IIRC.

> Also.. I think we need some tests to cover the new behavior.  A test
> to cover the subdirectory behavior would be especially helpful given
> the note about [7/9].

Yeah, that makes sense.

Re: [PATCH 0/9 v2] difftool: teach command to perform directory diffs

[Tim Henigan]

I have successfully tested on:
  - Linux with Git v1.7.10-rc0 and 1
  - Windows 7 with msysgit v1.7.9

I installed cygwin Git on Windows 7, but found that 'git difftool'
would not work, even before my patches were applied.  It appears that
I need to learn more about cygwin before I can use it as a test
platform.


On Mon, Mar 19, 2012 at 5:00 PM, Junio C Hamano <gitster@pobox.com> wrote:
>
>  * In [1/9], use of pass_through would mean 'git difftool' must be fed the
>   options for difftool and then options meant for underlying diff
>   machinery.  Is this limitation still there?  If so, isn't this a
>   regression?  Shouldn't it at least be advertised to the users a lot
>   stronger in documentation?

Both the documentation and testing has shown that 'pass_through' works
as I expected.  That is, options that difftool cares about are
extracted from @ARGV while all others are passed on to the underlying
'git diff' command.  The order of the arguments/options does not
matter.  We "use 5.008;" at the top of the script, so I believe this
change is correct and safe.  That being said, I have a limited number
of computers/configurations to test on.


>  * In [4/9], you remove the .exe extension when running git, which was
>   there since the beginning of difftool 5c38ea3 (contrib: add 'git
>   difftool' for launching common merge tools, 2009-01-16).  I think it is
>   safe but are Windows folks OK with this?

My testing on Windows with msysgit has shown that this change is safe.
 Also, even though I had other problems when testing on cygwin (noted
at the top of the email), the 'difftool' script was able to
successfully execute 'git diff --raw -z' without appending '.exe'.


>  * In [6/9], the exit code in the failure case seem to be modified from
>   that of the underlying "git diff" to whatever croak gives us.  Is this
>   a regression, or nobody cares error status from difftool?  I personally
>   suspect it is the latter, but just double-checking if you have
>   considered it.

In my testing, if "Git::command_noisy(('diff', @ARGV))" fails, the
exit code from 'git diff' is passed back to the terminal.  So not only
is the 'git diff' exit code echoed to the terminal, but '$?' also
contains the correct exit code.


>  * In [7/9], difftool--helper declares SUBDIRECTORY_OK, but there doesn't
>   seem to be any inclusion of git-sh-setup in this script, and the patch
>   does not have any effort to prepend $prefix to paths relative to $cwd.
>   What good does the variable do here?

This was an oversight.  It was copied over from a previous
implementation that still used ". git-sh-setup".

So the only outstanding changes that I am planning are:
  1) Remove SUBDIRECTORY_OK
  2) Add tests for the new '--dir-diff' option

Would it be better to resend the complete patches series with these
changes or just add new patches to the end?

Please let me know if I missed anything else.

Re: [PATCH 0/9 v2] difftool: teach command to perform directory diffs

[Junio C Hamano]

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

> So the only outstanding changes that I am planning are:
>   1) Remove SUBDIRECTORY_OK
>   2) Add tests for the new '--dir-diff' option
>
> Would it be better to resend the complete patches series with these
> changes or just add new patches to the end?

For the former, a replacement patch ("[PATCH v3 7/9]" with "this replaces
patch 5/9 from the v2" after "---" line) for the ones that need replacing
is preferred, as the topic is still in 'pu'.  For the latter, we can go
either with a separate patch "[PATCH v3 10/9]" or an update to the one
that finishes introduction of the new option.

> Please let me know if I missed anything else.

I think I saw t7800 failed with this series somewhwere around "last flag
wins" test.