[PATCH 1/2] difftool: Wrap long lines for readability

[PATCH 1/2] difftool: Wrap long lines for readability

[David Aguilar]

Keep everything within 80 columns.  Wrap the user-facing messages too.

Signed-off-by: David Aguilar <davvid@gmail.com>
---
The only remaining long lines were touched in 2/2.

 git-difftool.perl | 46 ++++++++++++++++++++++++++++++++--------------
 1 file changed, 32 insertions(+), 14 deletions(-)

diff --git a/git-difftool.perl b/git-difftool.perl
index 10d3d97..8e51238 100755
--- a/git-difftool.perl
+++ b/git-difftool.perl
@@ -93,15 +93,22 @@ sub print_tool_help
 		}
 	}
 
-	print "'git difftool --tool=<tool>' may be set to one of the following:\n";
+	print << 'EOF';
+'git difftool --tool=<tool>' may be set to one of the following:
+EOF
 	print "\t$_\n" for (sort(@found));
 
-	print "\nThe following tools are valid, but not currently available:\n";
+	print << 'EOF';
+
+The following tools are valid, but not currently available:
+EOF
 	print "\t$_\n" for (sort(@notfound));
 
-	print "\nNOTE: Some of the tools listed above only work in a windowed\n";
-	print "environment. If run in a terminal-only session, they will fail.\n";
+	print << 'EOF';
 
+NOTE: Some of the tools listed above only work in a windowed
+environment. If run in a terminal-only session, they will fail.
+EOF
 	exit(0);
 }
 
@@ -114,8 +121,11 @@ sub setup_dir_diff
 	# if $GIT_DIR and $GIT_WORK_TREE are set in ENV, they are actually used
 	# by Git->repository->command*.
 	my $repo_path = $repo->repo_path();
-	my $diffrepo = Git->repository(Repository => $repo_path, WorkingCopy => $workdir);
-	my $diffrtn = $diffrepo->command_oneline('diff', '--raw', '--no-abbrev', '-z', @ARGV);
+	my %repo_args = (Repository => $repo_path, WorkingCopy => $workdir);
+	my $diffrepo = Git->repository(%repo_args);
+
+	my @gitargs = ('diff', '--raw', '--no-abbrev', '-z', @ARGV);
+	my $diffrtn = $diffrepo->command_oneline(@gitargs);
 	exit(0) if (length($diffrtn) == 0);
 
 	# Setup temp directories
@@ -140,11 +150,15 @@ sub setup_dir_diff
 	my $i = 0;
 	while ($i < $#rawdiff) {
 		if ($rawdiff[$i] =~ /^::/) {
-			print "Combined diff formats ('-c' and '--cc') are not supported in directory diff mode.\n";
+			warn << 'EOF';
+Combined diff formats ('-c' and '--cc') are not supported in
+directory diff mode ('-d' and '--dir-diff').
+EOF
 			exit(1);
 		}
 
-		my ($lmode, $rmode, $lsha1, $rsha1, $status) = split(' ', substr($rawdiff[$i], 1));
+		my ($lmode, $rmode, $lsha1, $rsha1, $status) =
+			split(' ', substr($rawdiff[$i], 1));
 		my $src_path = $rawdiff[$i + 1];
 		my $dst_path;
 
@@ -156,7 +170,7 @@ sub setup_dir_diff
 			$i += 2;
 		}
 
-		if (($lmode eq $submodule_mode) or ($rmode eq $submodule_mode)) {
+		if ($lmode eq $submodule_mode or $rmode eq $submodule_mode) {
 			$submodule{$src_path}{left} = $lsha1;
 			if ($lsha1 ne $rsha1) {
 				$submodule{$dst_path}{right} = $rsha1;
@@ -167,14 +181,16 @@ sub setup_dir_diff
 		}
 
 		if ($lmode eq $symlink_mode) {
-			$symlink{$src_path}{left} = $diffrepo->command_oneline('show', "$lsha1");
+			$symlink{$src_path}{left} =
+				$diffrepo->command_oneline('show', "$lsha1");
 		}
 
 		if ($rmode eq $symlink_mode) {
-			$symlink{$dst_path}{right} = $diffrepo->command_oneline('show', "$rsha1");
+			$symlink{$dst_path}{right} =
+				$diffrepo->command_oneline('show', "$rsha1");
 		}
 
-		if (($lmode ne $null_mode) and ($status !~ /^C/)) {
+		if ($lmode ne $null_mode and $status !~ /^C/) {
 			$lindex .= "$lmode $lsha1\t$src_path\0";
 		}
 
@@ -199,14 +215,16 @@ sub setup_dir_diff
 	# Populate the left and right directories based on each index file
 	my ($inpipe, $ctx);
 	$ENV{GIT_INDEX_FILE} = "$tmpdir/lindex";
-	($inpipe, $ctx) = $repo->command_input_pipe(qw/update-index -z --index-info/);
+	($inpipe, $ctx) =
+		$repo->command_input_pipe(qw(update-index -z --index-info));
 	print($inpipe $lindex);
 	$repo->command_close_pipe($inpipe, $ctx);
 	my $rc = system('git', 'checkout-index', '--all', "--prefix=$ldir/");
 	exit($rc | ($rc >> 8)) if ($rc != 0);
 
 	$ENV{GIT_INDEX_FILE} = "$tmpdir/rindex";
-	($inpipe, $ctx) = $repo->command_input_pipe(qw/update-index -z --index-info/);
+	($inpipe, $ctx) =
+		$repo->command_input_pipe(qw(update-index -z --index-info));
 	print($inpipe $rindex);
 	$repo->command_close_pipe($inpipe, $ctx);
 	$rc = system('git', 'checkout-index', '--all', "--prefix=$rdir/");
-- 
1.7.11.11.g72d9886.dirty

[PATCH 2/2] difftool: Handle compare() returning -1

[David Aguilar]

Keep the temporary directory around when compare()
cannot read its input files, which is indicated by -1.

Defer tempdir creation to allow an early exit in setup_dir_diff().
Wrap the rest of the entry points in an exit_cleanup() function
to handle removing temporary files and error reporting.

Print the temporary files' location so that the user can
recover them.

Signed-off-by: David Aguilar <davvid@gmail.com>
---
Replaces "difftool: Check all return codes from compare()",
which was the original permutation of this patch.

This ended up touching much more than the original patch
since it now handles every exit point, but it's more thorough
and complete.

 git-difftool.perl | 100 +++++++++++++++++++++++++++++++++++++-----------------
 1 file changed, 68 insertions(+), 32 deletions(-)

diff --git a/git-difftool.perl b/git-difftool.perl
index 8e51238..5ed0b3a 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;
@@ -112,6 +112,17 @@ EOF
 	exit(0);
 }
 
+sub exit_cleanup
+{
+	my ($tmpdir, $status) = @_;
+	rmtree($tmpdir);
+	if ($status and $!) {
+		my ($package, $file, $line) = caller();
+		warn "$file line $line: $!\n";
+	}
+	exit($status | ($status >> 8));
+}
+
 sub setup_dir_diff
 {
 	my ($repo, $workdir, $symlinks) = @_;
@@ -128,13 +139,6 @@ sub setup_dir_diff
 	my $diffrtn = $diffrepo->command_oneline(@gitargs);
 	exit(0) if (length($diffrtn) == 0);
 
-	# Setup temp directories
-	my $tmpdir = tempdir('git-difftool.XXXXX', CLEANUP => 1, TMPDIR => 1);
-	my $ldir = "$tmpdir/left";
-	my $rdir = "$tmpdir/right";
-	mkpath($ldir) or die $!;
-	mkpath($rdir) or die $!;
-
 	# Build index info for left and right sides of the diff
 	my $submodule_mode = '160000';
 	my $symlink_mode = '120000';
@@ -203,6 +207,13 @@ EOF
 		}
 	}
 
+	# Setup temp directories
+	my $tmpdir = tempdir('git-difftool.XXXXX', CLEANUP => 0, TMPDIR => 1);
+	my $ldir = "$tmpdir/left";
+	my $rdir = "$tmpdir/right";
+	mkpath($ldir) or exit_cleanup($tmpdir, 1);
+	mkpath($rdir) or exit_cleanup($tmpdir, 1);
+
 	# If $GIT_DIR is not set prior to calling 'git update-index' and
 	# 'git checkout-index', then those commands will fail if difftool
 	# is called from a directory other than the repo root.
@@ -219,16 +230,18 @@ EOF
 		$repo->command_input_pipe(qw(update-index -z --index-info));
 	print($inpipe $lindex);
 	$repo->command_close_pipe($inpipe, $ctx);
+
 	my $rc = system('git', 'checkout-index', '--all', "--prefix=$ldir/");
-	exit($rc | ($rc >> 8)) if ($rc != 0);
+	exit_cleanup($tmpdir, $rc) if $rc != 0;
 
 	$ENV{GIT_INDEX_FILE} = "$tmpdir/rindex";
 	($inpipe, $ctx) =
 		$repo->command_input_pipe(qw(update-index -z --index-info));
 	print($inpipe $rindex);
 	$repo->command_close_pipe($inpipe, $ctx);
+
 	$rc = system('git', 'checkout-index', '--all', "--prefix=$rdir/");
-	exit($rc | ($rc >> 8)) if ($rc != 0);
+	exit_cleanup($tmpdir, $rc) if $rc != 0;
 
 	# If $GIT_DIR was explicitly set just for the update/checkout
 	# commands, then it should be unset before continuing.
@@ -240,14 +253,19 @@ EOF
 	for my $file (@working_tree) {
 		my $dir = dirname($file);
 		unless (-d "$rdir/$dir") {
-			mkpath("$rdir/$dir") or die $!;
+			mkpath("$rdir/$dir") or
+			exit_cleanup($tmpdir, 1);
 		}
 		if ($symlinks) {
-			symlink("$workdir/$file", "$rdir/$file") or die $!;
+			symlink("$workdir/$file", "$rdir/$file") or
+			exit_cleanup($tmpdir, 1);
 		} else {
-			copy("$workdir/$file", "$rdir/$file") or die $!;
+			copy("$workdir/$file", "$rdir/$file") or
+			exit_cleanup($tmpdir, 1);
+
 			my $mode = stat("$workdir/$file")->mode;
-			chmod($mode, "$rdir/$file") or die $!;
+			chmod($mode, "$rdir/$file") or
+			exit_cleanup($tmpdir, 1);
 		}
 	}
 
@@ -255,27 +273,35 @@ EOF
 	# temporary file to both the left and right directories to show the
 	# change in the recorded SHA1 for the submodule.
 	for my $path (keys %submodule) {
+		my $ok;
 		if (defined($submodule{$path}{left})) {
-			write_to_file("$ldir/$path", "Subproject commit $submodule{$path}{left}");
+			$ok = write_to_file("$ldir/$path",
+				"Subproject commit $submodule{$path}{left}");
 		}
 		if (defined($submodule{$path}{right})) {
-			write_to_file("$rdir/$path", "Subproject commit $submodule{$path}{right}");
+			$ok = write_to_file("$rdir/$path",
+				"Subproject commit $submodule{$path}{right}");
 		}
+		exit_cleanup($tmpdir, 1) if not $ok;
 	}
 
 	# Symbolic links require special treatment. The standard "git diff"
 	# shows only the link itself, not the contents of the link target.
 	# This loop replicates that behavior.
 	for my $path (keys %symlink) {
+		my $ok;
 		if (defined($symlink{$path}{left})) {
-			write_to_file("$ldir/$path", $symlink{$path}{left});
+			$ok = write_to_file("$ldir/$path",
+					$symlink{$path}{left});
 		}
 		if (defined($symlink{$path}{right})) {
-			write_to_file("$rdir/$path", $symlink{$path}{right});
+			$ok = write_to_file("$rdir/$path",
+					$symlink{$path}{right});
 		}
+		exit_cleanup($tmpdir, 1) if not $ok;
 	}
 
-	return ($ldir, $rdir, @working_tree);
+	return ($ldir, $rdir, $tmpdir, @working_tree);
 }
 
 sub write_to_file
@@ -286,16 +312,18 @@ sub write_to_file
 	# Make sure the path to the file exists
 	my $dir = dirname($path);
 	unless (-d "$dir") {
-		mkpath("$dir") or die $!;
+		mkpath("$dir") or return 0;
 	}
 
 	# If the file already exists in that location, delete it.  This
 	# is required in the case of symbolic links.
-	unlink("$path");
+	unlink($path);
 
-	open(my $fh, '>', "$path") or die $!;
+	open(my $fh, '>', $path) or return 0;
 	print($fh $value);
 	close($fh);
+
+	return 1;
 }
 
 sub main
@@ -367,21 +395,19 @@ 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 the diff including working copy files and those
 	# files were modified during the diff, then the changes
 	# should be copied back to the working tree.
@@ -395,16 +421,26 @@ 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 $!;
+			copy("$b/$file", "$workdir/$file") or
+			exit_cleanup($tmpdir, 1);
+
+			chmod($mode, "$workdir/$file") or
+			exit_cleanup($tmpdir, 1);
 		}
 	}
-	exit(0);
+	if ($error) {
+		warn "warning: Temporary files exist in '$tmpdir'.\n";
+		warn "warning: You may want to cleanup or recover these.\n";
+		exit(1);
+	} else {
+		exit_cleanup($tmpdir, $rc);
+	}
 }
 
 sub file_diff
-- 
1.7.11.11.g72d9886.dirty

Re: [PATCH 2/2] difftool: Handle compare() returning -1

[Junio C Hamano]

David Aguilar <davvid@gmail.com> writes:

> +sub exit_cleanup
> +{
> +	my ($tmpdir, $status) = @_;
> +	rmtree($tmpdir);
> +	if ($status and $!) {
> +		my ($package, $file, $line) = caller();
> +		warn "$file line $line: $!\n";

Are you sure rmtree() would not clobber $! before the code checks it
here?

Re: [PATCH 2/2] difftool: Handle compare() returning -1

[David Aguilar]

On Thu, Jul 26, 2012 at 12:07 PM, Junio C Hamano <gitster@pobox.com> wrote:
> David Aguilar <davvid@gmail.com> writes:
>
>> +sub exit_cleanup
>> +{
>> +     my ($tmpdir, $status) = @_;
>> +     rmtree($tmpdir);
>> +     if ($status and $!) {
>> +             my ($package, $file, $line) = caller();
>> +             warn "$file line $line: $!\n";
>
> Are you sure rmtree() would not clobber $! before the code checks it
> here?

rmtree() calls croak() on fatal error, but it would certainly be
clearer and safer to store $! before calling rmtree.
-- 
David