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

On Wed, Apr 4, 2012 at 12:21 PM, Tim Henigan [off-list ref] wrote:

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.

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...

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