* [PATCH] difftool: Do not remove temporary files on error
@ 2012-07-25 6:14 David Aguilar
2012-07-25 16:49 ` Junio C Hamano
0 siblings, 1 reply; 3+ messages in thread
From: David Aguilar @ 2012-07-25 6:14 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Tim Henigan, git
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@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));
+ }
# 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
--
1.7.11.9.g2b1cfc7.dirty
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] difftool: Do not remove temporary files on error
2012-07-25 6:14 [PATCH] difftool: Do not remove temporary files on error David Aguilar
@ 2012-07-25 16:49 ` Junio C Hamano
2012-07-25 18:28 ` David Aguilar
0 siblings, 1 reply; 3+ messages in thread
From: Junio C Hamano @ 2012-07-25 16:49 UTC (permalink / raw)
To: David Aguilar; +Cc: Tim Henigan, git
David Aguilar <davvid@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 <davvid@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.
> # 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
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] difftool: Do not remove temporary files on error
2012-07-25 16:49 ` Junio C Hamano
@ 2012-07-25 18:28 ` David Aguilar
0 siblings, 0 replies; 3+ messages in thread
From: David Aguilar @ 2012-07-25 18:28 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Tim Henigan, git
On Wed, Jul 25, 2012 at 9:49 AM, Junio C Hamano <gitster@pobox.com> wrote:
> David Aguilar <davvid@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 <davvid@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
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2012-07-25 18:29 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2012-07-25 6:14 [PATCH] difftool: Do not remove temporary files on error David Aguilar
2012-07-25 16:49 ` Junio C Hamano
2012-07-25 18:28 ` David Aguilar
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).