Re: [PATCH v2 2/3] t7800: fix tests when difftool uses --no-symlinks

[John Keeping <john@keeping.me.uk>]

On Sun, Mar 24, 2013 at 09:31:45AM -0400, Matt McClure wrote:
> On Sun, Mar 24, 2013 at 8:36 AM, John Keeping <john@keeping.me.uk> wrote:
> > In the
> > non-symlink case I think a user might find it surprising if the
> > (unmodified) file used by their diff tool were suddenly copied over the
> > working tree wiping out the changes they have just made.
> 
> That's exactly what I was describing here:
> http://thread.gmane.org/gmane.comp.version-control.git/217979/focus=218006

Ahh, I guess I didn't fully register the impact of that at the time and
had to rediscover the problem for myself ;-)

How about doing this (on top of jk/difftool-dir-diff-edit-fix)?

-- >8 --
Subject: [PATCH] difftool: don't overwrite modified files

After running the user's diff tool, git-difftool will copy any files
that differ between the working tree and the temporary tree.  This is
useful when the user edits the file in their diff tool but is wrong if
they edit the working tree file while examining the diff.

Instead of copying unconditionally when the files differ, store the
initial hash of the working tree file and only copy the temporary file
back if it was modified and the working tree file was not.  If both
files have been modified, print a warning and exit with an error.

Signed-off-by: John Keeping <john@keeping.me.uk>
---
 git-difftool.perl   | 35 +++++++++++++++++++++--------------
 t/t7800-difftool.sh | 26 ++++++++++++++++++++++++++
 2 files changed, 47 insertions(+), 14 deletions(-)

diff --git a/git-difftool.perl b/git-difftool.perl
index c433e86..be82b5a 100755
--- a/git-difftool.perl
+++ b/git-difftool.perl
@@ -15,7 +15,6 @@ use strict;
 use warnings;
 use File::Basename qw(dirname);
 use File::Copy;
-use File::Compare;
 use File::Find;
 use File::stat;
 use File::Path qw(mkpath rmtree);
@@ -123,7 +122,7 @@ sub setup_dir_diff
 	my $rindex = '';
 	my %submodule;
 	my %symlink;
-	my @working_tree = ();
+	my %working_tree;
 	my @rawdiff = split('\0', $diffrtn);
 
 	my $i = 0;
@@ -175,7 +174,9 @@ EOF
 
 		if ($rmode ne $null_mode) {
 			if (use_wt_file($repo, $workdir, $dst_path, $rsha1, $symlinks)) {
-				push(@working_tree, $dst_path);
+				$working_tree{$dst_path} =
+					$repo->command_oneline('hash-object',
+						"$workdir/$dst_path");
 			} else {
 				$rindex .= "$rmode $rsha1\t$dst_path\0";
 			}
@@ -227,7 +228,7 @@ EOF
 	# not part of the index. Remove any trailing slash from $workdir
 	# before starting to avoid double slashes in symlink targets.
 	$workdir =~ s|/$||;
-	for my $file (@working_tree) {
+	for my $file (keys %working_tree) {
 		my $dir = dirname($file);
 		unless (-d "$rdir/$dir") {
 			mkpath("$rdir/$dir") or
@@ -278,7 +279,7 @@ EOF
 		exit_cleanup($tmpdir, 1) if not $ok;
 	}
 
-	return ($ldir, $rdir, $tmpdir, @working_tree);
+	return ($ldir, $rdir, $tmpdir, %working_tree);
 }
 
 sub write_to_file
@@ -376,7 +377,7 @@ sub dir_diff
 	my $error = 0;
 	my $repo = Git->repository();
 	my $workdir = find_worktree($repo);
-	my ($a, $b, $tmpdir, @worktree) =
+	my ($a, $b, $tmpdir, %worktree) =
 		setup_dir_diff($repo, $workdir, $symlinks);
 
 	if (defined($extcmd)) {
@@ -390,19 +391,25 @@ sub dir_diff
 	# should be copied back to the working tree.
 	# Do not copy back files when symlinks are used and the
 	# external tool did not replace the original link with a file.
-	for my $file (@worktree) {
+	for my $file (keys %worktree) {
 		next if $symlinks && -l "$b/$file";
 		next if ! -f "$b/$file";
 
-		my $diff = compare("$b/$file", "$workdir/$file");
-		if ($diff == 0) {
-			next;
-		} elsif ($diff == -1) {
-			my $errmsg = "warning: Could not compare ";
-			$errmsg += "'$b/$file' with '$workdir/$file'\n";
+		my $wt_hash = $repo->command_oneline('hash-object',
+			"$workdir/$file");
+		my $tmp_hash = $repo->command_oneline('hash-object',
+			"$b/$file");
+		my $wt_modified = $wt_hash ne $worktree{$file};
+		my $tmp_modified = $tmp_hash ne $worktree{$file};
+
+		if ($wt_modified and $tmp_modified) {
+			my $errmsg = "warning: Both files modified: ";
+			$errmsg .= "'$workdir/$file' and '$b/$file'.\n";
+			$errmsg .= "warning: Working tree file has been left.\n";
+			$errmsg .= "warning:\n";
 			warn $errmsg;
 			$error = 1;
-		} elsif ($diff == 1) {
+		} elsif ($tmp_modified) {
 			my $mode = stat("$b/$file")->mode;
 			copy("$b/$file", "$workdir/$file") or
 			exit_cleanup($tmpdir, 1);
diff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh
index db3d3d6..be2042d 100755
--- a/t/t7800-difftool.sh
+++ b/t/t7800-difftool.sh
@@ -407,4 +407,30 @@ test_expect_success PERL 'difftool --dir-diff from subdirectory' '
 	)
 '
 
+write_script modify-file <<\EOF
+echo "new content" >file
+EOF
+
+test_expect_success PERL 'difftool --no-symlinks does not overwrite working tree file ' '
+	echo "orig content" >file &&
+	git difftool --dir-diff --no-symlinks --extcmd "$(pwd)/modify-file" branch &&
+	echo "new content" >expect &&
+	test_cmp expect file
+'
+
+write_script modify-both-files <<\EOF
+echo "wt content" >file &&
+echo "tmp content" >"$2/file" &&
+echo "$2" >tmpdir
+EOF
+
+test_expect_success PERL 'difftool --no-symlinks detects conflict ' '
+	echo "orig content" >file &&
+	test_must_fail git difftool --dir-diff --no-symlinks --extcmd "$(pwd)/modify-both-files" branch &&
+	echo "wt content" >expect &&
+	test_cmp expect file &&
+	echo "tmp content" >expect &&
+	test_cmp expect "$(cat tmpdir)/file"
+'
+
 test_done
-- 
1.8.2.411.g65a544e