{"thread":{"id":"31102","subject":"[PATCH 1/2] difftool: Wrap long lines for readability","startedAt":"2012-07-26T06:07:57Z","lastAt":"2012-07-26T19:25:12Z","messageCount":4,"participants":["David Aguilar","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"195815","messageId":"1343282878-86431-1-git-send-email-davvid@gmail.com","threadId":"31102","inReplyTo":null,"subject":"[PATCH 1/2] difftool: Wrap long lines for readability","fromName":"David Aguilar","fromEmail":"davvid@gmail.com","sentAt":"2012-07-26T06:07:57Z","receivedAt":"2012-07-26T06:07:57Z","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---\nThe only remaining long lines were touched in 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.g72d9886.dirty\n"},{"id":"195816","messageId":"1343282878-86431-2-git-send-email-davvid@gmail.com","threadId":"31102","inReplyTo":"1343282878-86431-1-git-send-email-davvid@gmail.com","subject":"[PATCH 2/2] difftool: Handle compare() returning -1","fromName":"David Aguilar","fromEmail":"davvid@gmail.com","sentAt":"2012-07-26T06:07:58Z","receivedAt":"2012-07-26T06:07:58Z","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---\nReplaces \"difftool: Check all return codes from compare()\",\nwhich was the original permutation of this patch.\n\nThis ended up touching much more than the original patch\nsince it now handles every exit point, but it's more thorough\nand complete.\n\n git-difftool.perl | 100 +++++++++++++++++++++++++++++++++++++-----------------\n 1 file changed, 68 insertions(+), 32 deletions(-)\n\ndiff --git a/git-difftool.perl b/git-difftool.perl\nindex 8e51238..5ed0b3a 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,17 @@ EOF\n \texit(0);\n }\n \n+sub exit_cleanup\n+{\n+\tmy ($tmpdir, $status) = @_;\n+\trmtree($tmpdir);\n+\tif ($status and $!) {\n+\t\tmy ($package, $file, $line) = caller();\n+\t\twarn \"$file line $line: $!\\n\";\n+\t}\n+\texit($status | ($status >> 8));\n+}\n+\n sub setup_dir_diff\n {\n \tmy ($repo, $workdir, $symlinks) = @_;\n@@ -128,13 +139,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 +207,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 +230,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 +253,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 +273,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 +312,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 +395,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 +421,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.g72d9886.dirty\n"},{"id":"195883","messageId":"7vd33i733s.fsf@alter.siamese.dyndns.org","threadId":"31102","inReplyTo":"1343282878-86431-2-git-send-email-davvid@gmail.com","subject":"Re: [PATCH 2/2] difftool: Handle compare() returning -1","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-07-26T19:07:51Z","receivedAt":"2012-07-26T19:07:51Z","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> +sub exit_cleanup\n> +{\n> +\tmy ($tmpdir, $status) = @_;\n> +\trmtree($tmpdir);\n> +\tif ($status and $!) {\n> +\t\tmy ($package, $file, $line) = caller();\n> +\t\twarn \"$file line $line: $!\\n\";\n\nAre you sure rmtree() would not clobber $! before the code checks it\nhere?\n"},{"id":"195884","messageId":"CAJDDKr7xRL_bXOs5Ka2g9v=_AVpr7vzt3fd-5DYtiF_4gLE7QQ@mail.gmail.com","threadId":"31102","inReplyTo":"7vd33i733s.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 2/2] difftool: Handle compare() returning -1","fromName":"David Aguilar","fromEmail":"davvid@gmail.com","sentAt":"2012-07-26T19:25:12Z","receivedAt":"2012-07-26T19:25:12Z","isPatch":true,"sender":{"key":"davvid@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13196?v=4"},"body":"On Thu, Jul 26, 2012 at 12:07 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> David Aguilar <davvid@gmail.com> writes:\n>\n>> +sub exit_cleanup\n>> +{\n>> +     my ($tmpdir, $status) = @_;\n>> +     rmtree($tmpdir);\n>> +     if ($status and $!) {\n>> +             my ($package, $file, $line) = caller();\n>> +             warn \"$file line $line: $!\\n\";\n>\n> Are you sure rmtree() would not clobber $! before the code checks it\n> here?\n\nrmtree() calls croak() on fatal error, but it would certainly be\nclearer and safer to store $! before calling rmtree.\n-- \nDavid\n"}]}