Re: [PATCH 8/9 v11] difftool: teach difftool to handle directory diffs
- From
David Aguilar <davvid@gmail.com>
- Date
- Apr 9, 2012, 12:14 UTC
- Message-ID
- <CAJDDKr76eMiA4rOHQhar3aToVThDfbc8Ki5tr2PGU_UpDMAVeA@mail.gmail.com>
- In-Reply-To
- <1333567265-23986-1-git-send-email-tim.henigan@gmail.com>
On Wed, Apr 4, 2012 at 12:21 PM, Tim Henigan <tim.henigan@gmail.com> wrote:
Show 17 quoted lines
> 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.
Show 28 quoted lines
> @@ -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.
Show 6 quoted lines
> + 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...
Show 34 quoted lines
> + 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.
Show 11 quoted lines
> +
> + $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 $_.
Show 12 quoted lines
> + 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...
Show 27 quoted lines
> @@ -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 ..."
Show 26 quoted lines
> +} 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