On Wed, Jul 25, 2012 at 9:49 AM, Junio C Hamano <gits...@pobox.com> wrote:
> David Aguilar <dav...@gmail.com> writes:
>
>> Keep the temporary directory around when either compare() or
>> the difftool returns a non-zero exit status.
>>
>> Tell the user about the location of the temporary files so that
>> they can recover from the failure.
>>
>> Signed-off-by: David Aguilar <dav...@gmail.com>
>> ---
>>  git-difftool.perl | 36 ++++++++++++++++++++++++++----------
>>  1 file changed, 26 insertions(+), 10 deletions(-)
>>
>> diff --git a/git-difftool.perl b/git-difftool.perl
>> index 10d3d97..f4f7d4a 100755
>> --- a/git-difftool.perl
>> +++ b/git-difftool.perl
>> @@ -18,7 +18,7 @@ use File::Copy;
>>  use File::Compare;
>>  use File::Find;
>>  use File::stat;
>> -use File::Path qw(mkpath);
>> +use File::Path qw(mkpath rmtree);
>>  use File::Temp qw(tempdir);
>>  use Getopt::Long qw(:config pass_through);
>>  use Git;
>> @@ -119,7 +119,7 @@ sub setup_dir_diff
>>       exit(0) if (length($diffrtn) == 0);
>>
>>       # Setup temp directories
>> -     my $tmpdir = tempdir('git-difftool.XXXXX', CLEANUP => 1, TMPDIR => 1);
>> +     my $tmpdir = tempdir('git-difftool.XXXXX', CLEANUP => 0, TMPDIR => 1);
>>       my $ldir = "$tmpdir/left";
>>       my $rdir = "$tmpdir/right";
>>       mkpath($ldir) or die $!;
>> @@ -257,7 +257,7 @@ sub setup_dir_diff
>>               }
>>       }
>>
>> -     return ($ldir, $rdir, @working_tree);
>> +     return ($ldir, $rdir, $tmpdir, @working_tree);
>>  }
>>
>>  sub write_to_file
>> @@ -349,20 +349,23 @@ sub main
>>  sub dir_diff
>>  {
>>       my ($extcmd, $symlinks) = @_;
>> -
>>       my $rc;
>> +     my $error = 0;
>>       my $repo = Git->repository();
>> -
>>       my $workdir = find_worktree($repo);
>> -     my ($a, $b, @worktree) = setup_dir_diff($repo, $workdir, $symlinks);
>> +     my ($a, $b, $tmpdir, @worktree) =
>> +             setup_dir_diff($repo, $workdir, $symlinks);
>> +
>>       if (defined($extcmd)) {
>>               $rc = system($extcmd, $a, $b);
>>       } else {
>>               $ENV{GIT_DIFFTOOL_DIRDIFF} = 'true';
>>               $rc = system('git', 'difftool--helper', $a, $b);
>>       }
>> -
>> -     exit($rc | ($rc >> 8)) if ($rc != 0);
>> +     if ($rc != 0) {
>> +             dir_diff_tmpdir_warning($tmpdir);
>> +             exit($rc | ($rc >> 8));
>> +     }
>
> Hrm.
>
> What does a non-zero exit code from these "compare two trees" diff
> tools (e.g. "diff -r a/ b/") mean?  Isn't "there are difference
> between the two trees" the usual meaning?  And we treat that as a
> failure and make the user clean up after us?
>
> The patch is not making things any worse with respect to that point,
> so I'd queue it as-is, but it smells like a fishy design decision to
> me.

This is true.  We do have mergetool.<tool>.trustExitCode, but I don't
think that's really what we want here.

I'm slightly on the fence about this.  The "do not cleanup" mode was
really added to deal with compare() returning -1 (which should be
extremely rare, if not non-existent in practice), and we'd be making
things worse by leaving junk around for the common case.

I personally think this should be redone so that we leave files around
for the compare() == -1 failure case only.


>
>>       # If the diff including working copy files and those
>>       # files were modified during the diff, then the changes
>> @@ -377,16 +380,29 @@ sub dir_diff
>>               if ($diff == 0) {
>>                       next;
>>               } elsif ($diff == -1 ) {
>> -                     my $errmsg = "warning: could not compare ";
>> +                     my $errmsg = "warning: Could not compare ";
>>                       $errmsg += "'$b/$file' with '$workdir/$file'\n";
>>                       warn $errmsg;
>> +                     $error = 1;
>>               } elsif ($diff == 1) {
>>                       copy("$b/$file", "$workdir/$file") or die $!;
>>                       my $mode = stat("$b/$file")->mode;
>>                       chmod($mode, "$workdir/$file") or die $!;
>>               }
>>       }
>> -     exit(0);
>> +     if ($error) {
>> +             dir_diff_tmpdir_warning($tmpdir);
>> +     } else {
>> +             rmtree($tmpdir);
>> +     }
>> +     exit($error);
>> +}
>> +
>> +sub dir_diff_tmpdir_warning
>> +{
>> +     my ($tmpdir) = @_;
>> +     warn "warning: Temporary files exist in '$tmpdir'.\n";
>> +     warn "warning: You may want to cleanup or recover these.\n";
>>  }
>>
>>  sub file_diff



-- 
David
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Reply via email to