From: David Aguilar Date: Mon, 09 Apr 2012 12:14:49 GMT Subject: Re: [PATCH 8/9 v11] difftool: teach difftool to handle directory diffs Message-ID: In-Reply-To: <1333567265-23986-1-git-send-email-tim.henigan@gmail.com> On Wed, Apr 4, 2012 at 12:21 PM, Tim Henigan 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=] >                     [-x|--extcmd=] >                     [-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