{"thread":{"id":"31107","subject":"[PATCH v2 2/2] difftool: Handle compare() returning -1","startedAt":"2012-07-26T19:36:18Z","lastAt":"2012-07-26T19:36:19Z","messageCount":2,"participants":["David Aguilar"],"isPatch":true,"patchVersion":2,"patchTotal":2},"messages":[{"id":"195888","messageId":"1343331379-99746-1-git-send-email-davvid@gmail.com","threadId":"31107","inReplyTo":null,"subject":"[PATCH v2 1/2] difftool: Wrap long lines for readability","fromName":"David Aguilar","fromEmail":"davvid@gmail.com","sentAt":"2012-07-26T19:36:18Z","receivedAt":"2012-07-26T19:36:18Z","isPatch":true,"sender":{"key":"davvid@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13196?v=4"},"body":"Keep everything within 80 columns.  Wrap the user-facing messages too.\n\nSigned-off-by: David Aguilar <davvid@gmail.com>\n---\nUnchanged since last time -- resending for patch 2/2\n\n git-difftool.perl | 46 ++++++++++++++++++++++++++++++++--------------\n 1 file changed, 32 insertions(+), 14 deletions(-)\n\ndiff --git a/git-difftool.perl b/git-difftool.perl\nindex 10d3d97..8e51238 100755\n--- a/git-difftool.perl\n+++ b/git-difftool.perl\n@@ -93,15 +93,22 @@ sub print_tool_help\n \t\t}\n \t}\n \n-\tprint \"'git difftool --tool=<tool>' may be set to one of the following:\\n\";\n+\tprint << 'EOF';\n+'git difftool --tool=<tool>' may be set to one of the following:\n+EOF\n \tprint \"\\t$_\\n\" for (sort(@found));\n \n-\tprint \"\\nThe following tools are valid, but not currently available:\\n\";\n+\tprint << 'EOF';\n+\n+The following tools are valid, but not currently available:\n+EOF\n \tprint \"\\t$_\\n\" for (sort(@notfound));\n \n-\tprint \"\\nNOTE: Some of the tools listed above only work in a windowed\\n\";\n-\tprint \"environment. If run in a terminal-only session, they will fail.\\n\";\n+\tprint << 'EOF';\n \n+NOTE: Some of the tools listed above only work in a windowed\n+environment. If run in a terminal-only session, they will fail.\n+EOF\n \texit(0);\n }\n \n@@ -114,8 +121,11 @@ sub setup_dir_diff\n \t# if $GIT_DIR and $GIT_WORK_TREE are set in ENV, they are actually used\n \t# by Git->repository->command*.\n \tmy $repo_path = $repo->repo_path();\n-\tmy $diffrepo = Git->repository(Repository => $repo_path, WorkingCopy => $workdir);\n-\tmy $diffrtn = $diffrepo->command_oneline('diff', '--raw', '--no-abbrev', '-z', @ARGV);\n+\tmy %repo_args = (Repository => $repo_path, WorkingCopy => $workdir);\n+\tmy $diffrepo = Git->repository(%repo_args);\n+\n+\tmy @gitargs = ('diff', '--raw', '--no-abbrev', '-z', @ARGV);\n+\tmy $diffrtn = $diffrepo->command_oneline(@gitargs);\n \texit(0) if (length($diffrtn) == 0);\n \n \t# Setup temp directories\n@@ -140,11 +150,15 @@ sub setup_dir_diff\n \tmy $i = 0;\n \twhile ($i < $#rawdiff) {\n \t\tif ($rawdiff[$i] =~ /^::/) {\n-\t\t\tprint \"Combined diff formats ('-c' and '--cc') are not supported in directory diff mode.\\n\";\n+\t\t\twarn << 'EOF';\n+Combined diff formats ('-c' and '--cc') are not supported in\n+directory diff mode ('-d' and '--dir-diff').\n+EOF\n \t\t\texit(1);\n \t\t}\n \n-\t\tmy ($lmode, $rmode, $lsha1, $rsha1, $status) = split(' ', substr($rawdiff[$i], 1));\n+\t\tmy ($lmode, $rmode, $lsha1, $rsha1, $status) =\n+\t\t\tsplit(' ', substr($rawdiff[$i], 1));\n \t\tmy $src_path = $rawdiff[$i + 1];\n \t\tmy $dst_path;\n \n@@ -156,7 +170,7 @@ sub setup_dir_diff\n \t\t\t$i += 2;\n \t\t}\n \n-\t\tif (($lmode eq $submodule_mode) or ($rmode eq $submodule_mode)) {\n+\t\tif ($lmode eq $submodule_mode or $rmode eq $submodule_mode) {\n \t\t\t$submodule{$src_path}{left} = $lsha1;\n \t\t\tif ($lsha1 ne $rsha1) {\n \t\t\t\t$submodule{$dst_path}{right} = $rsha1;\n@@ -167,14 +181,16 @@ sub setup_dir_diff\n \t\t}\n \n \t\tif ($lmode eq $symlink_mode) {\n-\t\t\t$symlink{$src_path}{left} = $diffrepo->command_oneline('show', \"$lsha1\");\n+\t\t\t$symlink{$src_path}{left} =\n+\t\t\t\t$diffrepo->command_oneline('show', \"$lsha1\");\n \t\t}\n \n \t\tif ($rmode eq $symlink_mode) {\n-\t\t\t$symlink{$dst_path}{right} = $diffrepo->command_oneline('show', \"$rsha1\");\n+\t\t\t$symlink{$dst_path}{right} =\n+\t\t\t\t$diffrepo->command_oneline('show', \"$rsha1\");\n \t\t}\n \n-\t\tif (($lmode ne $null_mode) and ($status !~ /^C/)) {\n+\t\tif ($lmode ne $null_mode and $status !~ /^C/) {\n \t\t\t$lindex .= \"$lmode $lsha1\\t$src_path\\0\";\n \t\t}\n \n@@ -199,14 +215,16 @@ sub setup_dir_diff\n \t# Populate the left and right directories based on each index file\n \tmy ($inpipe, $ctx);\n \t$ENV{GIT_INDEX_FILE} = \"$tmpdir/lindex\";\n-\t($inpipe, $ctx) = $repo->command_input_pipe(qw/update-index -z --index-info/);\n+\t($inpipe, $ctx) =\n+\t\t$repo->command_input_pipe(qw(update-index -z --index-info));\n \tprint($inpipe $lindex);\n \t$repo->command_close_pipe($inpipe, $ctx);\n \tmy $rc = system('git', 'checkout-index', '--all', \"--prefix=$ldir/\");\n \texit($rc | ($rc >> 8)) if ($rc != 0);\n \n \t$ENV{GIT_INDEX_FILE} = \"$tmpdir/rindex\";\n-\t($inpipe, $ctx) = $repo->command_input_pipe(qw/update-index -z --index-info/);\n+\t($inpipe, $ctx) =\n+\t\t$repo->command_input_pipe(qw(update-index -z --index-info));\n \tprint($inpipe $rindex);\n \t$repo->command_close_pipe($inpipe, $ctx);\n \t$rc = system('git', 'checkout-index', '--all', \"--prefix=$rdir/\");\n-- \n1.7.11.11.g8d9a67a\n"},{"id":"195887","messageId":"1343331379-99746-2-git-send-email-davvid@gmail.com","threadId":"31107","inReplyTo":"1343331379-99746-1-git-send-email-davvid@gmail.com","subject":"[PATCH v2 2/2] difftool: Handle compare() returning -1","fromName":"David Aguilar","fromEmail":"davvid@gmail.com","sentAt":"2012-07-26T19:36:19Z","receivedAt":"2012-07-26T19:36:19Z","isPatch":true,"sender":{"key":"davvid@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13196?v=4"},"body":"Keep the temporary directory around when compare()\ncannot read its input files, which is indicated by -1.\n\nDefer tempdir creation to allow an early exit in setup_dir_diff().\nWrap the rest of the entry points in an exit_cleanup() function\nto handle removing temporary files and error reporting.\n\nPrint the temporary files' location so that the user can\nrecover them.\n\nSigned-off-by: David Aguilar <davvid@gmail.com>\n---\nDifferences since v1:\n\nStore errno before calling rmtree() so that we are not affected\nwhen rmtree() clobbers errno.\n\n git-difftool.perl | 101 +++++++++++++++++++++++++++++++++++++-----------------\n 1 file changed, 69 insertions(+), 32 deletions(-)\n\ndiff --git a/git-difftool.perl b/git-difftool.perl\nindex 8e51238..e4277f5 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@@ -112,6 +112,18 @@ EOF\n \texit(0);\n }\n \n+sub exit_cleanup\n+{\n+\tmy ($tmpdir, $status) = @_;\n+\tmy $errno = $!;\n+\trmtree($tmpdir);\n+\tif ($status and $errno) {\n+\t\tmy ($package, $file, $line) = caller();\n+\t\twarn \"$file line $line: $errno\\n\";\n+\t}\n+\texit($status | ($status >> 8));\n+}\n+\n sub setup_dir_diff\n {\n \tmy ($repo, $workdir, $symlinks) = @_;\n@@ -128,13 +140,6 @@ sub setup_dir_diff\n \tmy $diffrtn = $diffrepo->command_oneline(@gitargs);\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 $ldir = \"$tmpdir/left\";\n-\tmy $rdir = \"$tmpdir/right\";\n-\tmkpath($ldir) or die $!;\n-\tmkpath($rdir) or die $!;\n-\n \t# Build index info for left and right sides of the diff\n \tmy $submodule_mode = '160000';\n \tmy $symlink_mode = '120000';\n@@ -203,6 +208,13 @@ EOF\n \t\t}\n \t}\n \n+\t# Setup temp directories\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 exit_cleanup($tmpdir, 1);\n+\tmkpath($rdir) or exit_cleanup($tmpdir, 1);\n+\n \t# If $GIT_DIR is not set prior to calling 'git update-index' and\n \t# 'git checkout-index', then those commands will fail if difftool\n \t# is called from a directory other than the repo root.\n@@ -219,16 +231,18 @@ EOF\n \t\t$repo->command_input_pipe(qw(update-index -z --index-info));\n \tprint($inpipe $lindex);\n \t$repo->command_close_pipe($inpipe, $ctx);\n+\n \tmy $rc = system('git', 'checkout-index', '--all', \"--prefix=$ldir/\");\n-\texit($rc | ($rc >> 8)) if ($rc != 0);\n+\texit_cleanup($tmpdir, $rc) if $rc != 0;\n \n \t$ENV{GIT_INDEX_FILE} = \"$tmpdir/rindex\";\n \t($inpipe, $ctx) =\n \t\t$repo->command_input_pipe(qw(update-index -z --index-info));\n \tprint($inpipe $rindex);\n \t$repo->command_close_pipe($inpipe, $ctx);\n+\n \t$rc = system('git', 'checkout-index', '--all', \"--prefix=$rdir/\");\n-\texit($rc | ($rc >> 8)) if ($rc != 0);\n+\texit_cleanup($tmpdir, $rc) if $rc != 0;\n \n \t# If $GIT_DIR was explicitly set just for the update/checkout\n \t# commands, then it should be unset before continuing.\n@@ -240,14 +254,19 @@ EOF\n \tfor my $file (@working_tree) {\n \t\tmy $dir = dirname($file);\n \t\tunless (-d \"$rdir/$dir\") {\n-\t\t\tmkpath(\"$rdir/$dir\") or die $!;\n+\t\t\tmkpath(\"$rdir/$dir\") or\n+\t\t\texit_cleanup($tmpdir, 1);\n \t\t}\n \t\tif ($symlinks) {\n-\t\t\tsymlink(\"$workdir/$file\", \"$rdir/$file\") or die $!;\n+\t\t\tsymlink(\"$workdir/$file\", \"$rdir/$file\") or\n+\t\t\texit_cleanup($tmpdir, 1);\n \t\t} else {\n-\t\t\tcopy(\"$workdir/$file\", \"$rdir/$file\") or die $!;\n+\t\t\tcopy(\"$workdir/$file\", \"$rdir/$file\") or\n+\t\t\texit_cleanup($tmpdir, 1);\n+\n \t\t\tmy $mode = stat(\"$workdir/$file\")->mode;\n-\t\t\tchmod($mode, \"$rdir/$file\") or die $!;\n+\t\t\tchmod($mode, \"$rdir/$file\") or\n+\t\t\texit_cleanup($tmpdir, 1);\n \t\t}\n \t}\n \n@@ -255,27 +274,35 @@ EOF\n \t# temporary file to both the left and right directories to show the\n \t# change in the recorded SHA1 for the submodule.\n \tfor my $path (keys %submodule) {\n+\t\tmy $ok;\n \t\tif (defined($submodule{$path}{left})) {\n-\t\t\twrite_to_file(\"$ldir/$path\", \"Subproject commit $submodule{$path}{left}\");\n+\t\t\t$ok = write_to_file(\"$ldir/$path\",\n+\t\t\t\t\"Subproject commit $submodule{$path}{left}\");\n \t\t}\n \t\tif (defined($submodule{$path}{right})) {\n-\t\t\twrite_to_file(\"$rdir/$path\", \"Subproject commit $submodule{$path}{right}\");\n+\t\t\t$ok = write_to_file(\"$rdir/$path\",\n+\t\t\t\t\"Subproject commit $submodule{$path}{right}\");\n \t\t}\n+\t\texit_cleanup($tmpdir, 1) if not $ok;\n \t}\n \n \t# Symbolic links require special treatment. The standard \"git diff\"\n \t# shows only the link itself, not the contents of the link target.\n \t# This loop replicates that behavior.\n \tfor my $path (keys %symlink) {\n+\t\tmy $ok;\n \t\tif (defined($symlink{$path}{left})) {\n-\t\t\twrite_to_file(\"$ldir/$path\", $symlink{$path}{left});\n+\t\t\t$ok = write_to_file(\"$ldir/$path\",\n+\t\t\t\t\t$symlink{$path}{left});\n \t\t}\n \t\tif (defined($symlink{$path}{right})) {\n-\t\t\twrite_to_file(\"$rdir/$path\", $symlink{$path}{right});\n+\t\t\t$ok = write_to_file(\"$rdir/$path\",\n+\t\t\t\t\t$symlink{$path}{right});\n \t\t}\n+\t\texit_cleanup($tmpdir, 1) if not $ok;\n \t}\n \n-\treturn ($ldir, $rdir, @working_tree);\n+\treturn ($ldir, $rdir, $tmpdir, @working_tree);\n }\n \n sub write_to_file\n@@ -286,16 +313,18 @@ sub write_to_file\n \t# Make sure the path to the file exists\n \tmy $dir = dirname($path);\n \tunless (-d \"$dir\") {\n-\t\tmkpath(\"$dir\") or die $!;\n+\t\tmkpath(\"$dir\") or return 0;\n \t}\n \n \t# If the file already exists in that location, delete it.  This\n \t# is required in the case of symbolic links.\n-\tunlink(\"$path\");\n+\tunlink($path);\n \n-\topen(my $fh, '>', \"$path\") or die $!;\n+\topen(my $fh, '>', $path) or return 0;\n \tprint($fh $value);\n \tclose($fh);\n+\n+\treturn 1;\n }\n \n sub main\n@@ -367,21 +396,19 @@ 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-\n \t# If the diff including working copy files and those\n \t# files were modified during the diff, then the changes\n \t# should be copied back to the working tree.\n@@ -395,16 +422,26 @@ 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\tcopy(\"$b/$file\", \"$workdir/$file\") or\n+\t\t\texit_cleanup($tmpdir, 1);\n+\n+\t\t\tchmod($mode, \"$workdir/$file\") or\n+\t\t\texit_cleanup($tmpdir, 1);\n \t\t}\n \t}\n-\texit(0);\n+\tif ($error) {\n+\t\twarn \"warning: Temporary files exist in '$tmpdir'.\\n\";\n+\t\twarn \"warning: You may want to cleanup or recover these.\\n\";\n+\t\texit(1);\n+\t} else {\n+\t\texit_cleanup($tmpdir, $rc);\n+\t}\n }\n \n sub file_diff\n-- \n1.7.11.11.g8d9a67a\n"}]}