{"thread":{"id":"31094","subject":"[PATCH] difftool: Do not remove temporary files on error","startedAt":"2012-07-25T06:14:36Z","lastAt":"2012-07-25T18:28:58Z","messageCount":3,"participants":["David Aguilar","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"195718","messageId":"1343196876-59195-1-git-send-email-davvid@gmail.com","threadId":"31094","inReplyTo":null,"subject":"[PATCH] difftool: Do not remove temporary files on error","fromName":"David Aguilar","fromEmail":"davvid@gmail.com","sentAt":"2012-07-25T06:14:36Z","receivedAt":"2012-07-25T06:14:36Z","isPatch":true,"sender":{"key":"davvid@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13196?v=4"},"body":"Keep the temporary directory around when either compare() or\nthe difftool returns a non-zero exit status.\n\nTell the user about the location of the temporary files so that\nthey can recover from the failure.\n\nSigned-off-by: David Aguilar <davvid@gmail.com>\n---\n git-difftool.perl | 36 ++++++++++++++++++++++++++----------\n 1 file changed, 26 insertions(+), 10 deletions(-)\n\ndiff --git a/git-difftool.perl b/git-difftool.perl\nindex 10d3d97..f4f7d4a 100755\n--- a/git-difftool.perl\n+++ b/git-difftool.perl\n@@ -18,7 +18,7 @@ use File::Copy;\n use File::Compare;\n use File::Find;\n use File::stat;\n-use File::Path qw(mkpath);\n+use File::Path qw(mkpath rmtree);\n use File::Temp qw(tempdir);\n use Getopt::Long qw(:config pass_through);\n use Git;\n@@ -119,7 +119,7 @@ sub setup_dir_diff\n \texit(0) if (length($diffrtn) == 0);\n \n \t# Setup temp directories\n-\tmy $tmpdir = tempdir('git-difftool.XXXXX', CLEANUP => 1, TMPDIR => 1);\n+\tmy $tmpdir = tempdir('git-difftool.XXXXX', CLEANUP => 0, TMPDIR => 1);\n \tmy $ldir = \"$tmpdir/left\";\n \tmy $rdir = \"$tmpdir/right\";\n \tmkpath($ldir) or die $!;\n@@ -257,7 +257,7 @@ sub setup_dir_diff\n \t\t}\n \t}\n \n-\treturn ($ldir, $rdir, @working_tree);\n+\treturn ($ldir, $rdir, $tmpdir, @working_tree);\n }\n \n sub write_to_file\n@@ -349,20 +349,23 @@ sub main\n sub dir_diff\n {\n \tmy ($extcmd, $symlinks) = @_;\n-\n \tmy $rc;\n+\tmy $error = 0;\n \tmy $repo = Git->repository();\n-\n \tmy $workdir = find_worktree($repo);\n-\tmy ($a, $b, @worktree) = setup_dir_diff($repo, $workdir, $symlinks);\n+\tmy ($a, $b, $tmpdir, @worktree) =\n+\t\tsetup_dir_diff($repo, $workdir, $symlinks);\n+\n \tif (defined($extcmd)) {\n \t\t$rc = system($extcmd, $a, $b);\n \t} else {\n \t\t$ENV{GIT_DIFFTOOL_DIRDIFF} = 'true';\n \t\t$rc = system('git', 'difftool--helper', $a, $b);\n \t}\n-\n-\texit($rc | ($rc >> 8)) if ($rc != 0);\n+\tif ($rc != 0) {\n+\t\tdir_diff_tmpdir_warning($tmpdir);\n+\t\texit($rc | ($rc >> 8));\n+\t}\n \n \t# If the diff including working copy files and those\n \t# files were modified during the diff, then the changes\n@@ -377,16 +380,29 @@ sub dir_diff\n \t\tif ($diff == 0) {\n \t\t\tnext;\n \t\t} elsif ($diff == -1 ) {\n-\t\t\tmy $errmsg = \"warning: could not compare \";\n+\t\t\tmy $errmsg = \"warning: Could not compare \";\n \t\t\t$errmsg += \"'$b/$file' with '$workdir/$file'\\n\";\n \t\t\twarn $errmsg;\n+\t\t\t$error = 1;\n \t\t} elsif ($diff == 1) {\n \t\t\tcopy(\"$b/$file\", \"$workdir/$file\") or die $!;\n \t\t\tmy $mode = stat(\"$b/$file\")->mode;\n \t\t\tchmod($mode, \"$workdir/$file\") or die $!;\n \t\t}\n \t}\n-\texit(0);\n+\tif ($error) {\n+\t\tdir_diff_tmpdir_warning($tmpdir);\n+\t} else {\n+\t\trmtree($tmpdir);\n+\t}\n+\texit($error);\n+}\n+\n+sub dir_diff_tmpdir_warning\n+{\n+\tmy ($tmpdir) = @_;\n+\twarn \"warning: Temporary files exist in '$tmpdir'.\\n\";\n+\twarn \"warning: You may want to cleanup or recover these.\\n\";\n }\n \n sub file_diff\n-- \n1.7.11.9.g2b1cfc7.dirty\n"},{"id":"195765","messageId":"7vlii7dbuy.fsf@alter.siamese.dyndns.org","threadId":"31094","inReplyTo":"1343196876-59195-1-git-send-email-davvid@gmail.com","subject":"Re: [PATCH] difftool: Do not remove temporary files on error","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-07-25T16:49:57Z","receivedAt":"2012-07-25T16:49:57Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"David Aguilar <davvid@gmail.com> writes:\n\n> Keep the temporary directory around when either compare() or\n> the difftool returns a non-zero exit status.\n>\n> Tell the user about the location of the temporary files so that\n> they can recover from the failure.\n>\n> Signed-off-by: David Aguilar <davvid@gmail.com>\n> ---\n>  git-difftool.perl | 36 ++++++++++++++++++++++++++----------\n>  1 file changed, 26 insertions(+), 10 deletions(-)\n>\n> diff --git a/git-difftool.perl b/git-difftool.perl\n> index 10d3d97..f4f7d4a 100755\n> --- a/git-difftool.perl\n> +++ b/git-difftool.perl\n> @@ -18,7 +18,7 @@ use File::Copy;\n>  use File::Compare;\n>  use File::Find;\n>  use File::stat;\n> -use File::Path qw(mkpath);\n> +use File::Path qw(mkpath rmtree);\n>  use File::Temp qw(tempdir);\n>  use Getopt::Long qw(:config pass_through);\n>  use Git;\n> @@ -119,7 +119,7 @@ sub setup_dir_diff\n>  \texit(0) if (length($diffrtn) == 0);\n>  \n>  \t# Setup temp directories\n> -\tmy $tmpdir = tempdir('git-difftool.XXXXX', CLEANUP => 1, TMPDIR => 1);\n> +\tmy $tmpdir = tempdir('git-difftool.XXXXX', CLEANUP => 0, TMPDIR => 1);\n>  \tmy $ldir = \"$tmpdir/left\";\n>  \tmy $rdir = \"$tmpdir/right\";\n>  \tmkpath($ldir) or die $!;\n> @@ -257,7 +257,7 @@ sub setup_dir_diff\n>  \t\t}\n>  \t}\n>  \n> -\treturn ($ldir, $rdir, @working_tree);\n> +\treturn ($ldir, $rdir, $tmpdir, @working_tree);\n>  }\n>  \n>  sub write_to_file\n> @@ -349,20 +349,23 @@ sub main\n>  sub dir_diff\n>  {\n>  \tmy ($extcmd, $symlinks) = @_;\n> -\n>  \tmy $rc;\n> +\tmy $error = 0;\n>  \tmy $repo = Git->repository();\n> -\n>  \tmy $workdir = find_worktree($repo);\n> -\tmy ($a, $b, @worktree) = setup_dir_diff($repo, $workdir, $symlinks);\n> +\tmy ($a, $b, $tmpdir, @worktree) =\n> +\t\tsetup_dir_diff($repo, $workdir, $symlinks);\n> +\n>  \tif (defined($extcmd)) {\n>  \t\t$rc = system($extcmd, $a, $b);\n>  \t} else {\n>  \t\t$ENV{GIT_DIFFTOOL_DIRDIFF} = 'true';\n>  \t\t$rc = system('git', 'difftool--helper', $a, $b);\n>  \t}\n> -\n> -\texit($rc | ($rc >> 8)) if ($rc != 0);\n> +\tif ($rc != 0) {\n> +\t\tdir_diff_tmpdir_warning($tmpdir);\n> +\t\texit($rc | ($rc >> 8));\n> +\t}\n\nHrm.\n\nWhat does a non-zero exit code from these \"compare two trees\" diff\ntools (e.g. \"diff -r a/ b/\") mean?  Isn't \"there are difference\nbetween the two trees\" the usual meaning?  And we treat that as a\nfailure and make the user clean up after us?\n\nThe patch is not making things any worse with respect to that point,\nso I'd queue it as-is, but it smells like a fishy design decision to\nme.\n\n>  \t# If the diff including working copy files and those\n>  \t# files were modified during the diff, then the changes\n> @@ -377,16 +380,29 @@ sub dir_diff\n>  \t\tif ($diff == 0) {\n>  \t\t\tnext;\n>  \t\t} elsif ($diff == -1 ) {\n> -\t\t\tmy $errmsg = \"warning: could not compare \";\n> +\t\t\tmy $errmsg = \"warning: Could not compare \";\n>  \t\t\t$errmsg += \"'$b/$file' with '$workdir/$file'\\n\";\n>  \t\t\twarn $errmsg;\n> +\t\t\t$error = 1;\n>  \t\t} elsif ($diff == 1) {\n>  \t\t\tcopy(\"$b/$file\", \"$workdir/$file\") or die $!;\n>  \t\t\tmy $mode = stat(\"$b/$file\")->mode;\n>  \t\t\tchmod($mode, \"$workdir/$file\") or die $!;\n>  \t\t}\n>  \t}\n> -\texit(0);\n> +\tif ($error) {\n> +\t\tdir_diff_tmpdir_warning($tmpdir);\n> +\t} else {\n> +\t\trmtree($tmpdir);\n> +\t}\n> +\texit($error);\n> +}\n> +\n> +sub dir_diff_tmpdir_warning\n> +{\n> +\tmy ($tmpdir) = @_;\n> +\twarn \"warning: Temporary files exist in '$tmpdir'.\\n\";\n> +\twarn \"warning: You may want to cleanup or recover these.\\n\";\n>  }\n>  \n>  sub file_diff\n"},{"id":"195774","messageId":"CAJDDKr5CcOo9Oewim9V1Kr8NoCoaSy9BE=esMvXxdtDZAJ_VUg@mail.gmail.com","threadId":"31094","inReplyTo":"7vlii7dbuy.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] difftool: Do not remove temporary files on error","fromName":"David Aguilar","fromEmail":"davvid@gmail.com","sentAt":"2012-07-25T18:28:58Z","receivedAt":"2012-07-25T18:28:58Z","isPatch":true,"sender":{"key":"davvid@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13196?v=4"},"body":"On Wed, Jul 25, 2012 at 9:49 AM, Junio C Hamano <gitster@pobox.com> wrote:\n> David Aguilar <davvid@gmail.com> writes:\n>\n>> Keep the temporary directory around when either compare() or\n>> the difftool returns a non-zero exit status.\n>>\n>> Tell the user about the location of the temporary files so that\n>> they can recover from the failure.\n>>\n>> Signed-off-by: David Aguilar <davvid@gmail.com>\n>> ---\n>>  git-difftool.perl | 36 ++++++++++++++++++++++++++----------\n>>  1 file changed, 26 insertions(+), 10 deletions(-)\n>>\n>> diff --git a/git-difftool.perl b/git-difftool.perl\n>> index 10d3d97..f4f7d4a 100755\n>> --- a/git-difftool.perl\n>> +++ b/git-difftool.perl\n>> @@ -18,7 +18,7 @@ use File::Copy;\n>>  use File::Compare;\n>>  use File::Find;\n>>  use File::stat;\n>> -use File::Path qw(mkpath);\n>> +use File::Path qw(mkpath rmtree);\n>>  use File::Temp qw(tempdir);\n>>  use Getopt::Long qw(:config pass_through);\n>>  use Git;\n>> @@ -119,7 +119,7 @@ sub setup_dir_diff\n>>       exit(0) if (length($diffrtn) == 0);\n>>\n>>       # Setup temp directories\n>> -     my $tmpdir = tempdir('git-difftool.XXXXX', CLEANUP => 1, TMPDIR => 1);\n>> +     my $tmpdir = tempdir('git-difftool.XXXXX', CLEANUP => 0, TMPDIR => 1);\n>>       my $ldir = \"$tmpdir/left\";\n>>       my $rdir = \"$tmpdir/right\";\n>>       mkpath($ldir) or die $!;\n>> @@ -257,7 +257,7 @@ sub setup_dir_diff\n>>               }\n>>       }\n>>\n>> -     return ($ldir, $rdir, @working_tree);\n>> +     return ($ldir, $rdir, $tmpdir, @working_tree);\n>>  }\n>>\n>>  sub write_to_file\n>> @@ -349,20 +349,23 @@ sub main\n>>  sub dir_diff\n>>  {\n>>       my ($extcmd, $symlinks) = @_;\n>> -\n>>       my $rc;\n>> +     my $error = 0;\n>>       my $repo = Git->repository();\n>> -\n>>       my $workdir = find_worktree($repo);\n>> -     my ($a, $b, @worktree) = setup_dir_diff($repo, $workdir, $symlinks);\n>> +     my ($a, $b, $tmpdir, @worktree) =\n>> +             setup_dir_diff($repo, $workdir, $symlinks);\n>> +\n>>       if (defined($extcmd)) {\n>>               $rc = system($extcmd, $a, $b);\n>>       } else {\n>>               $ENV{GIT_DIFFTOOL_DIRDIFF} = 'true';\n>>               $rc = system('git', 'difftool--helper', $a, $b);\n>>       }\n>> -\n>> -     exit($rc | ($rc >> 8)) if ($rc != 0);\n>> +     if ($rc != 0) {\n>> +             dir_diff_tmpdir_warning($tmpdir);\n>> +             exit($rc | ($rc >> 8));\n>> +     }\n>\n> Hrm.\n>\n> What does a non-zero exit code from these \"compare two trees\" diff\n> tools (e.g. \"diff -r a/ b/\") mean?  Isn't \"there are difference\n> between the two trees\" the usual meaning?  And we treat that as a\n> failure and make the user clean up after us?\n>\n> The patch is not making things any worse with respect to that point,\n> so I'd queue it as-is, but it smells like a fishy design decision to\n> me.\n\nThis is true.  We do have mergetool.<tool>.trustExitCode, but I don't\nthink that's really what we want here.\n\nI'm slightly on the fence about this.  The \"do not cleanup\" mode was\nreally added to deal with compare() returning -1 (which should be\nextremely rare, if not non-existent in practice), and we'd be making\nthings worse by leaving junk around for the common case.\n\nI personally think this should be redone so that we leave files around\nfor the compare() == -1 failure case only.\n\n\n>\n>>       # If the diff including working copy files and those\n>>       # files were modified during the diff, then the changes\n>> @@ -377,16 +380,29 @@ sub dir_diff\n>>               if ($diff == 0) {\n>>                       next;\n>>               } elsif ($diff == -1 ) {\n>> -                     my $errmsg = \"warning: could not compare \";\n>> +                     my $errmsg = \"warning: Could not compare \";\n>>                       $errmsg += \"'$b/$file' with '$workdir/$file'\\n\";\n>>                       warn $errmsg;\n>> +                     $error = 1;\n>>               } elsif ($diff == 1) {\n>>                       copy(\"$b/$file\", \"$workdir/$file\") or die $!;\n>>                       my $mode = stat(\"$b/$file\")->mode;\n>>                       chmod($mode, \"$workdir/$file\") or die $!;\n>>               }\n>>       }\n>> -     exit(0);\n>> +     if ($error) {\n>> +             dir_diff_tmpdir_warning($tmpdir);\n>> +     } else {\n>> +             rmtree($tmpdir);\n>> +     }\n>> +     exit($error);\n>> +}\n>> +\n>> +sub dir_diff_tmpdir_warning\n>> +{\n>> +     my ($tmpdir) = @_;\n>> +     warn \"warning: Temporary files exist in '$tmpdir'.\\n\";\n>> +     warn \"warning: You may want to cleanup or recover these.\\n\";\n>>  }\n>>\n>>  sub file_diff\n\n\n\n-- \nDavid\n"}]}