On Wed, Jul 25, 2012 at 9:49 AM, Junio C Hamano <gitster@xxxxxxxxx> wrote:
> David Aguilar <davvid@xxxxxxxxx> 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 <davvid@xxxxxxxxx>
>> ---
>> 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 majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html