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

[David Aguilar <davvid@gmail.com>]

On Wed, Apr 4, 2012 at 12:21 PM, Tim Henigan <tim.henigan@gmail.com> wrote:
> diff --git a/git-difftool.perl b/git-difftool.perl
> index d4fe998..5bb01e1 100755
> --- a/git-difftool.perl
> +++ b/git-difftool.perl
> @@ -1,21 +1,29 @@
>  #!/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.

What if we punt on enumerating each variable and reword this to:

# This script exports GIT_EXTERNAL_DIFF and GIT_PAGER for use by git.
# The GIT_DIFF* variables are exported for use by git-difftool--helper.

I also think we should change the shebang line to #!/usr/bin/perl.


> @@ -24,15 +32,121 @@ usage: git difftool [-t|--tool=<tool>]
>                     [-x|--extcmd=<cmd>]
>                     [-g|--gui] [--no-gui]
>                     [--prompt] [-y|--no-prompt]
> +                    [-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;

I know Perl allows it, but my eyes get confused.

How about writing "0" x 6 and "0" x 40?
That way we can visually see that the result is a string.


> +       my $lindex = "";
> +       my $rindex = "";
> +       my %submodule;
> +       my @rawdiff = split('\0', $diffrtn);
> +
> +       for (my $i=0; $i<@rawdiff; $i+=2) {

We use $i + 1 to grab list elements, so how about $#rawdiff instead of @rawdiff?
It doesn't matter in practice, though...


> +               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;
> +                       if ($lsha1 ne $rsha1) {
> +                               $submodule{$path}{right} = $rsha1;
> +                       } else {
> +                               $submodule{$path}{right} = "$rsha1-dirty";
> +                       }
> +                       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_DIR} = $repo->repo_path();
> +       $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);
> +       system(('git', 'checkout-index', '--all', "--prefix=$ldir/"));

Please drop the extra parens.  Perl ignores them.
We should also check the return value from system() here.


> +
> +       $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);
> +       system(('git', 'checkout-index', '--all', "--prefix=$rdir/"));
> +
> +       # Changes in the working tree need special treatment since they are
> +       # not part of the index
> +       my $workdir = $repo->repo_path() . "/..";
> +       for (@working_tree) {

Please use the "for my $foo (@list)" form so we can say $foo instead of $_.


> +               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) {

I think it's better to use "for" instead of "foreach" since we do not
modify $path.


> +               if (defined $submodule{$path}{left}) {

In some places we write "defined(...)" but here it's "defined ...".
We should be consistent...


> @@ -65,22 +179,40 @@ if ($gui) {
>                $ENV{GIT_DIFF_TOOL} = $guitool;
>        }
>  }
> -if (defined($prompt)) {
> -       if ($prompt) {
> -               $ENV{GIT_DIFFTOOL_PROMPT} = 'true';
> +
> +# 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_NO_PROMPT} = 'true';
> +               $ENV{GIT_DIFFTOOL_DIRDIFF} = 'true';
> +               system(('git', 'difftool--helper', $a, $b));
> +       }
> +
> +       # 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->repo_path() . "/..";

Does this work when $GIT_WORK_TREE / core.worktree are defined?


> +       for (@working_tree) {
> +               copy("$b/$_", "$workdir/$_") or die $!;
> +               chmod(stat("$b/$_")->mode, "$workdir/$_") or die $!;
> +       }

"for my ..."


> +} else {
> +       if (defined($prompt)) {
> +               if ($prompt) {
> +                       $ENV{GIT_DIFFTOOL_PROMPT} = 'true';
> +               } else {
> +                       $ENV{GIT_DIFFTOOL_NO_PROMPT} = 'true';
> +               }
>        }
> -}
>
> -$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));
> +       $ENV{GIT_PAGER} = '';
> +       $ENV{GIT_EXTERNAL_DIFF} = 'git-difftool--helper';
> +       my $rc = system(('git', 'diff', @ARGV));
> +       exit($rc | ($rc >> 8));
> +}


We went back and forth a few times on this section,
eventually landing back on using system().

Should we retain this comment to help future readers from
having to re-learn it the hard way again?

We could also link to the ML threads, if you think that's helpful.
-- 
David