On Wed, Apr 4, 2012 at 12:21 PM, Tim Henigan <tim.henigan@xxxxxxxxx> 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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html