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

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

I'm not sure what conventions are used to decide when new copyright
authors are added.  Given the amount of change to this script, it
seems appropriate to add me to the list. However, if that is change
is outside norms, I have no problem taking it out.


 Documentation/git-difftool.txt |    6 ++
 git-difftool--helper.sh        |   20 ++++--
 git-difftool.perl              |  142 ++++++++++++++++++++++++++++++++++++----
 3 files changed, 150 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..0262c7a 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('/tmp/git-diffall.XXXXX', CLEANUP => 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(SUBMOD, ">$ldir/$path") or die $!;
+			print(SUBMOD "Subproject commit $submodule{$path}{left}");
+			close(SUBMOD);
+		}
+		if (defined $submodule{$path}{right}) {
+			open(SUBMOD, ">$rdir/$path") or die $!;
+			print(SUBMOD "Subproject commit $submodule{$path}{right}");
+			close(SUBMOD);
+		}
+	}
+
+	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,31 @@ 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';
+		system("git difftool--helper $a $b");
+	}
+	# 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

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

[David Aguilar]

On Fri, Mar 16, 2012 at 6:59 PM, Tim Henigan <tim.henigan@gmail.com> wrote:
> 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>
> ---
>
>  Documentation/git-difftool.txt |    6 ++
>  git-difftool--helper.sh        |   20 ++++--
>  git-difftool.perl              |  142 ++++++++++++++++++++++++++++++++++++----
>  3 files changed, 150 insertions(+), 18 deletions(-)
>
> diff --git a/git-difftool.perl b/git-difftool.perl
> index 8498089..0262c7a 100755
> --- a/git-difftool.perl
> +++ b/git-difftool.perl
> @@ -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('/tmp/git-diffall.XXXXX', CLEANUP => 1);

Is it okay to hardcode /tmp here, or should we instead do something like this?:

    my $tmp = $ENV{TMPDIR} || '/tmp';
-- 
David

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

[Tim Henigan]

On Fri, Mar 16, 2012 at 11:08 PM, David Aguilar <davvid@gmail.com> wrote:
> On Fri, Mar 16, 2012 at 6:59 PM, Tim Henigan <tim.henigan@gmail.com> wrote:

>> +       # Setup temp directories
>> +       my $tmpdir = tempdir('/tmp/git-diffall.XXXXX', CLEANUP => 1);
>
> Is it okay to hardcode /tmp here, or should we instead do something like this?:
>
>    my $tmp = $ENV{TMPDIR} || '/tmp';

This could be done better.  Based on the documentation [1], it appears
that the proper way to do this is:

    my $tmpdir = tempdir('git-diffall.XXXXX', CLEANUP => 1, TMPDIR => 1);

Adding 'TMPDIR => 1' is the equivalent of commanding the tmp dir to be
created in 'File::Spec->tmpdir' [2].

I will try this out and send a v2 patch.

[1]: http://perldoc.perl.org/File/Temp.html
[2]: http://perldoc.perl.org/File/Spec.html

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

[Jakub Narebski]

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

> +	foreach my $path (keys %submodule) {
> +		if (defined $submodule{$path}{left}) {
> +			open(SUBMOD, ">$ldir/$path") or die $!;
> +			print(SUBMOD "Subproject commit $submodule{$path}{left}");
> +			close(SUBMOD);
> +		}
> +		if (defined $submodule{$path}{right}) {
> +			open(SUBMOD, ">$rdir/$path") or die $!;
> +			print(SUBMOD "Subproject commit $submodule{$path}{right}");
> +			close(SUBMOD);

Could you please use modern Perl, and use lexical filehandles instead
of globs, and 3-arg version of 'open', i.e.

  +			open my $submod_fh, ">", "$ldir/$path" or die $!;
  +			print $submod_fh "Subproject commit $submodule{$path}{left}";
  +			close $submod_fh;


> +if (defined($dirdiff)) {
> +	my ($a, $b) = setup_dir_diff();
> +	if (defined($extcmd)) {
> +		system("$extcmd $a $b");
> +	} else {
> +		$ENV{GIT_DIFFTOOL_DIRDIFF} = 'true';
> +		system("git difftool--helper $a $b");
> +	}
> +	# 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';
> +}

Why you use 'system' (and not in list form which does not require
escaping shell characters) instead of git_cmd_try for first 
"git difftool--helper" invocation?

Is $extcmd and resultof setup_dir_diff() to be treated as shell
snippet, and used in string form of 'system' without escaping shell
metacharacters (like ' ' in filename)?

-- 
Jakub Narebski

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

[Tim Henigan]

On Sat, Mar 17, 2012 at 10:32 AM, Jakub Narebski <jnareb@gmail.com> wrote:
> Tim Henigan <tim.henigan@gmail.com> writes:
>
>> +     foreach my $path (keys %submodule) {
>> +             if (defined $submodule{$path}{left}) {
>> +                     open(SUBMOD, ">$ldir/$path") or die $!;
>> +                     print(SUBMOD "Subproject commit $submodule{$path}{left}");
>> +                     close(SUBMOD);
>> +             }
>> +             if (defined $submodule{$path}{right}) {
>> +                     open(SUBMOD, ">$rdir/$path") or die $!;
>> +                     print(SUBMOD "Subproject commit $submodule{$path}{right}");
>> +                     close(SUBMOD);
>
> Could you please use modern Perl, and use lexical filehandles instead
> of globs, and 3-arg version of 'open', i.e.
>
>  +                     open my $submod_fh, ">", "$ldir/$path" or die $!;
>  +                     print $submod_fh "Subproject commit $submodule{$path}{left}";
>  +                     close $submod_fh;

I will make this change in v2.  Thank you for the review and the reminder.


>> +if (defined($dirdiff)) {
>> +     my ($a, $b) = setup_dir_diff();
>> +     if (defined($extcmd)) {
>> +             system("$extcmd $a $b");
>> +     } else {
>> +             $ENV{GIT_DIFFTOOL_DIRDIFF} = 'true';
>> +             system("git difftool--helper $a $b");
>> +     }
>
> Why you use 'system' (and not in list form which does not require
> escaping shell characters) instead of git_cmd_try for first
> "git difftool--helper" invocation?

There is no good reason to use 'system' here.  I will change this to
'Git::command_noisy' in v2.


> Is $extcmd and resultof setup_dir_diff() to be treated as shell
> snippet, and used in string form of 'system' without escaping shell
> metacharacters (like ' ' in filename)?

In v2, I will change to the list form of 'system' for the call to
$extcmd. I think I made some bad assumptions about the contents of
$extcmd, $a and $b because of my local test setup.  However, changing
to list form should correct the oversight.

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

[Tim Henigan]

I have run into a problem with the way that the tmp directories are
populated using 'git update-index' and 'git checkout-index'.  I would
appreciate any feedback that may help me understand what I am doing
wrong.

This is in regard to the following section of code:

On Fri, Mar 16, 2012 at 9:59 PM, Tim Henigan <tim.henigan@gmail.com> wrote:
>
> +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);

<snip>

> +       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/"]);

When run from the root directory of a Git repository, the 'difftool
--dir-diff' command operates as expected:
  - Temporary index files for each side of the diff are created using
'git update-index'.
  - 'git checkout-index --all --prefix=$tmp' is used to populate the
tmp directory based on the index file.

However, when 'difftool --dir-diff' is run from a subdirectory of the
Git repository, there are problems.
  - The temporary index file generated by 'git update-index' appears
to be correct.
  - 'git checkout-index --all --prefix=$tmp' command does not work
(output tmp dir is empty).

I have tried some combinations of setting $ENV{GIT_DIR} and
$ENV{GIT_WORK_TREE}, but it has not fixed the problem.  Is there
anything obvious that I missed?

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

[Junio C Hamano]

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

> However, when 'difftool --dir-diff' is run from a subdirectory of the
> Git repository, there are problems.
>   - The temporary index file generated by 'git update-index' appears
> to be correct.
>   - 'git checkout-index --all --prefix=$tmp' command does not work
> (output tmp dir is empty).

Hrmph.

$ rm -fr /tmp/xyz
$ cd Documentation
$ git checkout-index --all --prefix=/tmp/xyz/
$ ls -F /tmp/xyz
Documentation/
$ rm -fr /tmp/xyz
$ GIT_DIR=$(pwd)/../.git GIT_WORK_TREE=$(pwd) \
  git checkout-index --all --prefix=/tmp/xyz/
$ ls -FC /tmp/xyz | head -2 
abspath.c                   git-merge-octopus.sh*       reflog-walk.c
aclocal.m4                  git-merge-one-file.sh*      reflog-walk.h

Be sure to have the trailing slash in $tmp.

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

[Tim Henigan]

On Tue, Mar 20, 2012 at 2:35 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Tim Henigan <tim.henigan@gmail.com> writes:
>
>> However, when 'difftool --dir-diff' is run from a subdirectory of the
>> Git repository, there are problems.
>>   - The temporary index file generated by 'git update-index' appears
>> to be correct.
>>   - 'git checkout-index --all --prefix=$tmp' command does not work
>> (output tmp dir is empty).
>
> Hrmph.
>
> $ rm -fr /tmp/xyz
> $ cd Documentation
> $ git checkout-index --all --prefix=/tmp/xyz/
> $ ls -F /tmp/xyz
> Documentation/
> $ rm -fr /tmp/xyz
> $ GIT_DIR=$(pwd)/../.git GIT_WORK_TREE=$(pwd) \
>  git checkout-index --all --prefix=/tmp/xyz/
> $ ls -FC /tmp/xyz | head -2
> abspath.c                   git-merge-octopus.sh*       reflog-walk.c
> aclocal.m4                  git-merge-one-file.sh*      reflog-walk.h
>
> Be sure to have the trailing slash in $tmp.

It turns out that 'Git::command_oneline()' does not honor environment
variables set prior to calling the function.

This change fixed the problem:

- $repo->command_oneline(["checkout-index", "-a", "--prefix=$ldir/"]);
+ $ENV{GIT_DIR} = $repo->repo_path();
+ system(('git', 'checkout-index', '-a', "--prefix=$ldir/"));

I'm still testing, but will submit an updated patch soon.