Skip to contentSkip to conversation

difftool improvements

PATCH In review John Keeping 2 participants 9 messages Jun 15, 2016

lore
Share & tools
lore

John Keeping

author
#0 lore raw

cover difftool improvements

This is a consolidated series replacing jk/t7800-modernize. The patches here have been rebased on master.

Patch 1 is new and moves the test added by commit 02c5631 (difftool --dir-diff: symlink all files matching the working tree) to the end of the test file. This means that once this is applied the second patch and the final three patches can become independent branches.

Patch 2 is a fix for a long standing deficiency, but the potential for this to happen has been increased by the commit mentioned above. It has already been through a couple of iterations [1].

The final three patches are the same as the current jk/t7800-modernize, with one tweaked commit message.

[1] http://article.gmane.org/gmane.comp.version-control.git/219100

John Keeping (5):
t7800: move '--symlinks' specific test to the end
difftool: don't overwrite modified files
t7800: don't hide grep output
t7800: fix tests when difftool uses --no-symlinks
t7800: run --dir-diff tests with and without symlinks

 git-difftool.perl   |  73 +++++++++++++++++++++++++++++-------
 t/t7800-difftool.sh | 105 ++++++++++++++++++++++++++++++++++------------------
 2 files changed, 127 insertions(+), 51 deletions(-)
Signature · 1 line
1.8.2.411.g65a544e
author
#1 lore raw

PATCH 1/5 t7800: move '--symlinks' specific test to the end

This will group the tests more logically when we introduce a helper to run most --dir-diff tests with both --symlinks and --no-symlinks.

Signed-off-by John Keeping <[email protected]>
---
 t/t7800-difftool.sh | 30 +++++++++++++++---------------
 1 file changed, 15 insertions(+), 15 deletions(-)
t/t7800-difftool.sh +15 -15
index c6d6b1c..e6a16cd 100755
@@ -340,6 +340,21 @@ test_expect_success PERL 'difftool --dir-diff' '
 	stdin_contains file <output
 '
 
+test_expect_success PERL 'difftool --dir-diff ignores --prompt' '
+	git difftool --dir-diff --prompt --extcmd ls branch >output &&
+	stdin_contains sub <output &&
+	stdin_contains file <output
+'
+
+test_expect_success PERL 'difftool --dir-diff from subdirectory' '
+	(
+		cd sub &&
+		git difftool --dir-diff --extcmd ls branch >output &&
+		stdin_contains sub <output &&
+		stdin_contains file <output
+	)
+'
+
 write_script .git/CHECK_SYMLINKS <<\EOF
 for f in file file2 sub/sub
 do

@@ -362,19 +377,4 @@ test_expect_success PERL,SYMLINKS 'difftool --dir-diff --symlink without unstage
 	test_cmp actual expect
 '
 
-test_expect_success PERL 'difftool --dir-diff ignores --prompt' '
-	git difftool --dir-diff --prompt --extcmd ls branch >output &&
-	stdin_contains sub <output &&
-	stdin_contains file <output
-'
-
-test_expect_success PERL 'difftool --dir-diff from subdirectory' '
-	(
-		cd sub &&
-		git difftool --dir-diff --extcmd ls branch >output &&
-		stdin_contains sub <output &&
-		stdin_contains file <output
-	)
-'
-
 test_done
Signature · 1 line
1.8.2.411.g65a544e
author
#2 lore raw

PATCH 2/5 difftool: don't overwrite modified files

In reply to John Keeping

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, create and index from the working tree files 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.

Note that we cannot use an existing index in git-difftool since those contain the modified files that need to be checked out but here we are looking at those files which are copied from the working tree and not checked out. These are precisely the files which are not in the existing indices.

Signed-off-by John Keeping <[email protected]>

---
Changes since v2:
- Set TMPDIR to $TRASH_DIRECTORY in the test where difftool fails

 git-difftool.perl   | 73 +++++++++++++++++++++++++++++++++++++++++++----------
 t/t7800-difftool.sh | 30 ++++++++++++++++++++++
 2 files changed, 89 insertions(+), 14 deletions(-)
git-difftool.perl +59 -14
index 663640d..844f619 100755
@@ -13,9 +13,9 @@
 use 5.008;
 use strict;
 use warnings;
+use Error qw(:try);
 use File::Basename qw(dirname);
 use File::Copy;
-use File::Compare;
 use File::Find;
 use File::stat;
 use File::Path qw(mkpath rmtree);

@@ -88,14 +88,45 @@ sub use_wt_file
 	my ($repo, $workdir, $file, $sha1, $symlinks) = @_;
 	my $null_sha1 = '0' x 40;
 
-	if ($sha1 eq $null_sha1) {
-		return 1;
-	} elsif (not $symlinks) {
+	if ($sha1 ne $null_sha1 and not $symlinks) {
 		return 0;
 	}
 
 	my $wt_sha1 = $repo->command_oneline('hash-object', "$workdir/$file");
-	return $sha1 eq $wt_sha1;
+	my $use = ($sha1 eq $null_sha1) || ($sha1 eq $wt_sha1);
+	return ($use, $wt_sha1);
+}
+
+sub changed_files
+{
+	my ($repo_path, $index, $worktree) = @_;
+	$ENV{GIT_INDEX_FILE} = $index;
+	$ENV{GIT_WORK_TREE} = $worktree;
+	my $must_unset_git_dir = 0;
+	if (not defined($ENV{GIT_DIR})) {
+		$must_unset_git_dir = 1;
+		$ENV{GIT_DIR} = $repo_path;
+	}
+
+	my @refreshargs = qw/update-index --really-refresh -q --unmerged/;
+	my @gitargs = qw/diff-files --name-only -z/;
+	try {
+		Git::command_oneline(@refreshargs);
+	} catch Git::Error::Command with {};
+
+	my $line = Git::command_oneline(@gitargs);
+	my @files;
+	if (defined $line) {
+		@files = split('\0', $line);
+	} else {
+		@files = ();
+	}
+
+	delete($ENV{GIT_INDEX_FILE});
+	delete($ENV{GIT_WORK_TREE});
+	delete($ENV{GIT_DIR}) if ($must_unset_git_dir);
+
+	return map { $_ => 1 } @files;
 }
 
 sub setup_dir_diff

@@ -121,6 +152,7 @@ sub setup_dir_diff
 	my $null_sha1 = '0' x 40;
 	my $lindex = '';
 	my $rindex = '';
+	my $wtindex = '';
 	my %submodule;
 	my %symlink;
 	my @working_tree = ();

@@ -174,8 +206,12 @@ EOF
 		}
 
 		if ($rmode ne $null_mode) {
-			if (use_wt_file($repo, $workdir, $dst_path, $rsha1, $symlinks)) {
-				push(@working_tree, $dst_path);
+			my ($use, $wt_sha1) = use_wt_file($repo, $workdir,
+							  $dst_path, $rsha1,
+							  $symlinks);
+			if ($use) {
+				push @working_tree, $dst_path;
+				$wtindex .= "$rmode $wt_sha1\t$dst_path\0";
 			} else {
 				$rindex .= "$rmode $rsha1\t$dst_path\0";
 			}

@@ -218,6 +254,12 @@ EOF
 	$rc = system('git', 'checkout-index', '--all', "--prefix=$rdir/");
 	exit_cleanup($tmpdir, $rc) if $rc != 0;
 
+	$ENV{GIT_INDEX_FILE} = "$tmpdir/wtindex";
+	($inpipe, $ctx) =
+		$repo->command_input_pipe(qw(update-index --info-only -z --index-info));
+	print($inpipe $wtindex);
+	$repo->command_close_pipe($inpipe, $ctx);
+
 	# If $GIT_DIR was explicitly set just for the update/checkout
 	# commands, then it should be unset before continuing.
 	delete($ENV{GIT_DIR}) if ($must_unset_git_dir);

@@ -390,19 +432,22 @@ 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.
+	my %wt_modified = changed_files($repo->repo_path(),
+		"$tmpdir/wtindex", "$workdir");
+	my %tmp_modified = changed_files($repo->repo_path(),
+		"$tmpdir/wtindex", "$b");
 	for my $file (@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";
+		if (exists $wt_modified{$file} and exists $tmp_modified{$file}) {
+			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{$file}) {
 			my $mode = stat("$b/$file")->mode;
 			copy("$b/$file", "$workdir/$file") or
 			exit_cleanup($tmpdir, 1);
t/t7800-difftool.sh +30 -0
index e6a16cd..017f55a 100755
@@ -377,4 +377,34 @@ test_expect_success PERL,SYMLINKS 'difftool --dir-diff --symlink without unstage
 	test_cmp actual expect
 '
 
+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 ' '
+	(
+		TMPDIR=$TRASH_DIRECTORY &&
+		export TMPDIR &&
+		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
Signature · 1 line
1.8.2.411.g65a544e
#3 lore raw

Re: [PATCH 2/5] difftool: don't overwrite modified files

Quoted 4 lines · John Keeping

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.

Thanks.

I should drop the t7800-modernize topic and queue this under a better name. Perhaps "difftool-no-overwrite-on-copyback", or something.

Quoted 125 lines

Instead of copying unconditionally when the files differ, create and index from the working tree files 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.

Note that we cannot use an existing index in git-difftool since those contain the modified files that need to be checked out but here we are looking at those files which are copied from the working tree and not checked out. These are precisely the files which are not in the existing indices.

Signed-off-by John Keeping <[email protected]>

---
Changes since v2:
- Set TMPDIR to $TRASH_DIRECTORY in the test where difftool fails

 git-difftool.perl   | 73 +++++++++++++++++++++++++++++++++++++++++++----------
 t/t7800-difftool.sh | 30 ++++++++++++++++++++++
 2 files changed, 89 insertions(+), 14 deletions(-)
git-difftool.perl +49 -7
index 663640d..844f619 100755
@@ -13,9 +13,9 @@
 use 5.008;
 use strict;
 use warnings;
+use Error qw(:try);
 use File::Basename qw(dirname);
 use File::Copy;
-use File::Compare;
 use File::Find;
 use File::stat;
 use File::Path qw(mkpath rmtree);

@@ -88,14 +88,45 @@ sub use_wt_file
 	my ($repo, $workdir, $file, $sha1, $symlinks) = @_;
 	my $null_sha1 = '0' x 40;
 
-	if ($sha1 eq $null_sha1) {
-		return 1;
-	} elsif (not $symlinks) {
+	if ($sha1 ne $null_sha1 and not $symlinks) {
 		return 0;
 	}
 
 	my $wt_sha1 = $repo->command_oneline('hash-object', "$workdir/$file");
-	return $sha1 eq $wt_sha1;
+	my $use = ($sha1 eq $null_sha1) || ($sha1 eq $wt_sha1);
+	return ($use, $wt_sha1);
+}
+
+sub changed_files
+{
+	my ($repo_path, $index, $worktree) = @_;
+	$ENV{GIT_INDEX_FILE} = $index;
+	$ENV{GIT_WORK_TREE} = $worktree;
+	my $must_unset_git_dir = 0;
+	if (not defined($ENV{GIT_DIR})) {
+		$must_unset_git_dir = 1;
+		$ENV{GIT_DIR} = $repo_path;
+	}
+
+	my @refreshargs = qw/update-index --really-refresh -q --unmerged/;
+	my @gitargs = qw/diff-files --name-only -z/;
+	try {
+		Git::command_oneline(@refreshargs);
+	} catch Git::Error::Command with {};
+
+	my $line = Git::command_oneline(@gitargs);
+	my @files;
+	if (defined $line) {
+		@files = split('\0', $line);
+	} else {
+		@files = ();
+	}
+
+	delete($ENV{GIT_INDEX_FILE});
+	delete($ENV{GIT_WORK_TREE});
+	delete($ENV{GIT_DIR}) if ($must_unset_git_dir);
+
+	return map { $_ => 1 } @files;
 }
 
 sub setup_dir_diff

@@ -121,6 +152,7 @@ sub setup_dir_diff
 	my $null_sha1 = '0' x 40;
 	my $lindex = '';
 	my $rindex = '';
+	my $wtindex = '';
 	my %submodule;
 	my %symlink;
 	my @working_tree = ();

@@ -174,8 +206,12 @@ EOF
 		}
 
 		if ($rmode ne $null_mode) {
-			if (use_wt_file($repo, $workdir, $dst_path, $rsha1, $symlinks)) {
-				push(@working_tree, $dst_path);
+			my ($use, $wt_sha1) = use_wt_file($repo, $workdir,
+							  $dst_path, $rsha1,
+							  $symlinks);
+			if ($use) {
+				push @working_tree, $dst_path;
+				$wtindex .= "$rmode $wt_sha1\t$dst_path\0";
 			} else {
 				$rindex .= "$rmode $rsha1\t$dst_path\0";
 			}

@@ -218,6 +254,12 @@ EOF
 	$rc = system('git', 'checkout-index', '--all', "--prefix=$rdir/");
 	exit_cleanup($tmpdir, $rc) if $rc != 0;
 
+	$ENV{GIT_INDEX_FILE} = "$tmpdir/wtindex";
+	($inpipe, $ctx) =
+		$repo->command_input_pipe(qw(update-index --info-only -z --index-info));
+	print($inpipe $wtindex);
+	$repo->command_close_pipe($inpipe, $ctx);
+
 	# If $GIT_DIR was explicitly set just for the update/checkout
 	# commands, then it should be unset before continuing.
 	delete($ENV{GIT_DIR}) if ($must_unset_git_dir);

git-difftool.perl sub dir_diff · line 438

Quoted 7 lines
git-difftool.perl +4 -0
@@ -390,19 +432,22 @@ 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.
+	my %wt_modified = changed_files($repo->repo_path(),
+		"$tmpdir/wtindex", "$workdir");
+	my %tmp_modified = changed_files($repo->repo_path(),
+		"$tmpdir/wtindex", "$b");

Do we need to run these two comparisons unconditionally?

It appears to me that in a sane and safe setting (i.e. $symlinks is set and the "diff viewer" touches the file through the symbolic link) does not ever look at wt_modified/tmp_modified at all because the first check in the loop will always trigger.

I wonder if it makes sense to populate these two hashes lazily so that the safe case does not have to pay the penalty at all.

git-difftool.perl sub dir_diff · line 450

Quoted 12 lines
git-difftool.perl +6 -0
 	for my $file (@worktree) {
 		next if $symlinks && -l "$b/$file";
 		next if ! -f "$b/$file";
 
+		if (exists $wt_modified{$file} and exists $tmp_modified{$file}) {
+			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 ($tmp_modified{$file}) {

"exists" if only for consistency?

author
#4 lore raw

Re: [PATCH 2/5] difftool: don't overwrite modified files

Quoted 148 lines · Junio C Hamano
Quoted 4 lines · John Keeping

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.

Thanks.

I should drop the t7800-modernize topic and queue this under a better name. Perhaps "difftool-no-overwrite-on-copyback", or something.

Quoted 125 lines

Instead of copying unconditionally when the files differ, create and index from the working tree files 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.

Note that we cannot use an existing index in git-difftool since those contain the modified files that need to be checked out but here we are looking at those files which are copied from the working tree and not checked out. These are precisely the files which are not in the existing indices.

Signed-off-by John Keeping <[email protected]>

---
Changes since v2:
- Set TMPDIR to $TRASH_DIRECTORY in the test where difftool fails

 git-difftool.perl   | 73 +++++++++++++++++++++++++++++++++++++++++++----------
 t/t7800-difftool.sh | 30 ++++++++++++++++++++++
 2 files changed, 89 insertions(+), 14 deletions(-)
git-difftool.perl +49 -7
index 663640d..844f619 100755
@@ -13,9 +13,9 @@
 use 5.008;
 use strict;
 use warnings;
+use Error qw(:try);
 use File::Basename qw(dirname);
 use File::Copy;
-use File::Compare;
 use File::Find;
 use File::stat;
 use File::Path qw(mkpath rmtree);

@@ -88,14 +88,45 @@ sub use_wt_file
 	my ($repo, $workdir, $file, $sha1, $symlinks) = @_;
 	my $null_sha1 = '0' x 40;
 
-	if ($sha1 eq $null_sha1) {
-		return 1;
-	} elsif (not $symlinks) {
+	if ($sha1 ne $null_sha1 and not $symlinks) {
 		return 0;
 	}
 
 	my $wt_sha1 = $repo->command_oneline('hash-object', "$workdir/$file");
-	return $sha1 eq $wt_sha1;
+	my $use = ($sha1 eq $null_sha1) || ($sha1 eq $wt_sha1);
+	return ($use, $wt_sha1);
+}
+
+sub changed_files
+{
+	my ($repo_path, $index, $worktree) = @_;
+	$ENV{GIT_INDEX_FILE} = $index;
+	$ENV{GIT_WORK_TREE} = $worktree;
+	my $must_unset_git_dir = 0;
+	if (not defined($ENV{GIT_DIR})) {
+		$must_unset_git_dir = 1;
+		$ENV{GIT_DIR} = $repo_path;
+	}
+
+	my @refreshargs = qw/update-index --really-refresh -q --unmerged/;
+	my @gitargs = qw/diff-files --name-only -z/;
+	try {
+		Git::command_oneline(@refreshargs);
+	} catch Git::Error::Command with {};
+
+	my $line = Git::command_oneline(@gitargs);
+	my @files;
+	if (defined $line) {
+		@files = split('\0', $line);
+	} else {
+		@files = ();
+	}
+
+	delete($ENV{GIT_INDEX_FILE});
+	delete($ENV{GIT_WORK_TREE});
+	delete($ENV{GIT_DIR}) if ($must_unset_git_dir);
+
+	return map { $_ => 1 } @files;
 }
 
 sub setup_dir_diff

@@ -121,6 +152,7 @@ sub setup_dir_diff
 	my $null_sha1 = '0' x 40;
 	my $lindex = '';
 	my $rindex = '';
+	my $wtindex = '';
 	my %submodule;
 	my %symlink;
 	my @working_tree = ();

@@ -174,8 +206,12 @@ EOF
 		}
 
 		if ($rmode ne $null_mode) {
-			if (use_wt_file($repo, $workdir, $dst_path, $rsha1, $symlinks)) {
-				push(@working_tree, $dst_path);
+			my ($use, $wt_sha1) = use_wt_file($repo, $workdir,
+							  $dst_path, $rsha1,
+							  $symlinks);
+			if ($use) {
+				push @working_tree, $dst_path;
+				$wtindex .= "$rmode $wt_sha1\t$dst_path\0";
 			} else {
 				$rindex .= "$rmode $rsha1\t$dst_path\0";
 			}

@@ -218,6 +254,12 @@ EOF
 	$rc = system('git', 'checkout-index', '--all', "--prefix=$rdir/");
 	exit_cleanup($tmpdir, $rc) if $rc != 0;
 
+	$ENV{GIT_INDEX_FILE} = "$tmpdir/wtindex";
+	($inpipe, $ctx) =
+		$repo->command_input_pipe(qw(update-index --info-only -z --index-info));
+	print($inpipe $wtindex);
+	$repo->command_close_pipe($inpipe, $ctx);
+
 	# If $GIT_DIR was explicitly set just for the update/checkout
 	# commands, then it should be unset before continuing.
 	delete($ENV{GIT_DIR}) if ($must_unset_git_dir);

git-difftool.perl sub dir_diff · line 438

Quoted 7 lines
git-difftool.perl +4 -0
@@ -390,19 +432,22 @@ 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.
+	my %wt_modified = changed_files($repo->repo_path(),
+		"$tmpdir/wtindex", "$workdir");
+	my %tmp_modified = changed_files($repo->repo_path(),
+		"$tmpdir/wtindex", "$b");

Do we need to run these two comparisons unconditionally?

Quoted 7 lines

It appears to me that in a sane and safe setting (i.e. $symlinks is set and the "diff viewer" touches the file through the symbolic link) does not ever look at wt_modified/tmp_modified at all because the first check in the loop will always trigger.

I wonder if it makes sense to populate these two hashes lazily so that the safe case does not have to pay the penalty at all.

Sounds sensible. I'll look into doing this.

Quoted 4 lines
Quoted 4 lines
git-difftool.perl +0 -0
 	for my $file (@worktree) {
 		next if $symlinks && -l "$b/$file";
 		next if ! -f "$b/$file";
Quoted 10 lines

git-difftool.perl sub dir_diff · line 450

Quoted 8 lines
git-difftool.perl +6 -0
+		if (exists $wt_modified{$file} and exists $tmp_modified{$file}) {
+			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 ($tmp_modified{$file}) {

"exists" if only for consistency?

Not just for consistency - this will generate an uninitialized value warning if the tmp version of the file hasn't been modified.

author
#5 lore raw

PATCH v2 2/5 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, create and index from the working tree files 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.

Note that we cannot use an existing index in git-difftool since those contain the modified files that need to be checked out but here we are looking at those files which are copied from the working tree and not checked out. These are precisely the files which are not in the existing indices.

Signed-off-by John Keeping <[email protected]>

---
Changes since v1:
- Lazily initialize "*_modified" hashes to avoid executing Git commands in the common case where symlinks are in use and the diff tool writes to the file through the symlink
- Use exists consistently when checking "*_modified" hashes

 git-difftool.perl   | 85 ++++++++++++++++++++++++++++++++++++++++++++---------
 t/t7800-difftool.sh | 30 +++++++++++++++++++
 2 files changed, 101 insertions(+), 14 deletions(-)
git-difftool.perl +71 -14
index 663640d..6780292 100755
@@ -13,9 +13,9 @@
 use 5.008;
 use strict;
 use warnings;
+use Error qw(:try);
 use File::Basename qw(dirname);
 use File::Copy;
-use File::Compare;
 use File::Find;
 use File::stat;
 use File::Path qw(mkpath rmtree);

@@ -88,14 +88,45 @@ sub use_wt_file
 	my ($repo, $workdir, $file, $sha1, $symlinks) = @_;
 	my $null_sha1 = '0' x 40;
 
-	if ($sha1 eq $null_sha1) {
-		return 1;
-	} elsif (not $symlinks) {
+	if ($sha1 ne $null_sha1 and not $symlinks) {
 		return 0;
 	}
 
 	my $wt_sha1 = $repo->command_oneline('hash-object', "$workdir/$file");
-	return $sha1 eq $wt_sha1;
+	my $use = ($sha1 eq $null_sha1) || ($sha1 eq $wt_sha1);
+	return ($use, $wt_sha1);
+}
+
+sub changed_files
+{
+	my ($repo_path, $index, $worktree) = @_;
+	$ENV{GIT_INDEX_FILE} = $index;
+	$ENV{GIT_WORK_TREE} = $worktree;
+	my $must_unset_git_dir = 0;
+	if (not defined($ENV{GIT_DIR})) {
+		$must_unset_git_dir = 1;
+		$ENV{GIT_DIR} = $repo_path;
+	}
+
+	my @refreshargs = qw/update-index --really-refresh -q --unmerged/;
+	my @gitargs = qw/diff-files --name-only -z/;
+	try {
+		Git::command_oneline(@refreshargs);
+	} catch Git::Error::Command with {};
+
+	my $line = Git::command_oneline(@gitargs);
+	my @files;
+	if (defined $line) {
+		@files = split('\0', $line);
+	} else {
+		@files = ();
+	}
+
+	delete($ENV{GIT_INDEX_FILE});
+	delete($ENV{GIT_WORK_TREE});
+	delete($ENV{GIT_DIR}) if ($must_unset_git_dir);
+
+	return map { $_ => 1 } @files;
 }
 
 sub setup_dir_diff

@@ -121,6 +152,7 @@ sub setup_dir_diff
 	my $null_sha1 = '0' x 40;
 	my $lindex = '';
 	my $rindex = '';
+	my $wtindex = '';
 	my %submodule;
 	my %symlink;
 	my @working_tree = ();

@@ -174,8 +206,12 @@ EOF
 		}
 
 		if ($rmode ne $null_mode) {
-			if (use_wt_file($repo, $workdir, $dst_path, $rsha1, $symlinks)) {
-				push(@working_tree, $dst_path);
+			my ($use, $wt_sha1) = use_wt_file($repo, $workdir,
+							  $dst_path, $rsha1,
+							  $symlinks);
+			if ($use) {
+				push @working_tree, $dst_path;
+				$wtindex .= "$rmode $wt_sha1\t$dst_path\0";
 			} else {
 				$rindex .= "$rmode $rsha1\t$dst_path\0";
 			}

@@ -218,6 +254,12 @@ EOF
 	$rc = system('git', 'checkout-index', '--all', "--prefix=$rdir/");
 	exit_cleanup($tmpdir, $rc) if $rc != 0;
 
+	$ENV{GIT_INDEX_FILE} = "$tmpdir/wtindex";
+	($inpipe, $ctx) =
+		$repo->command_input_pipe(qw(update-index --info-only -z --index-info));
+	print($inpipe $wtindex);
+	$repo->command_close_pipe($inpipe, $ctx);
+
 	# If $GIT_DIR was explicitly set just for the update/checkout
 	# commands, then it should be unset before continuing.
 	delete($ENV{GIT_DIR}) if ($must_unset_git_dir);

@@ -390,19 +432,34 @@ 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.
+	#
+	# These hashes are loaded lazily since they aren't needed
+	# in the common case of --symlinks and the difftool updating
+	# files through the symlink.
+	my %wt_modified;
+	my %tmp_modified;
+	my $indices_loaded = 0;
+
 	for my $file (@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";
+		if (!$indices_loaded) {
+			%wt_modified = changed_files($repo->repo_path(),
+				"$tmpdir/wtindex", "$workdir");
+			%tmp_modified = changed_files($repo->repo_path(),
+				"$tmpdir/wtindex", "$b");
+			$indices_loaded = 1;
+		}
+
+		if (exists $wt_modified{$file} and exists $tmp_modified{$file}) {
+			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 (exists $tmp_modified{$file}) {
 			my $mode = stat("$b/$file")->mode;
 			copy("$b/$file", "$workdir/$file") or
 			exit_cleanup($tmpdir, 1);
t/t7800-difftool.sh +30 -0
index e6a16cd..017f55a 100755
@@ -377,4 +377,34 @@ test_expect_success PERL,SYMLINKS 'difftool --dir-diff --symlink without unstage
 	test_cmp actual expect
 '
 
+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 ' '
+	(
+		TMPDIR=$TRASH_DIRECTORY &&
+		export TMPDIR &&
+		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
Signature · 1 line
1.8.2.411.g65a544e
author
#6 lore raw

PATCH 3/5 t7800: don't hide grep output

In reply to John Keeping

Remove the stdin_contains and stdin_doesnt_contain helper functions which add nothing but hide the output of grep, hurting debugging.

Suggested-by Johannes Sixt <[email protected]>
Signed-off-by John Keeping <[email protected]>
---
 t/t7800-difftool.sh | 44 +++++++++++++++++---------------------------
 1 file changed, 17 insertions(+), 27 deletions(-)
t/t7800-difftool.sh +17 -27
index 017f55a..9fd09db 100755
@@ -23,16 +23,6 @@ prompt_given ()
 	test "$prompt" = "Launch 'test-tool' [Y/n]: branch"
 }
 
-stdin_contains ()
-{
-	grep >/dev/null "$1"
-}
-
-stdin_doesnot_contain ()
-{
-	! stdin_contains "$1"
-}
-
 # Create a file on master and change it on branch
 test_expect_success PERL 'setup' '
 	echo master >file &&

@@ -296,24 +286,24 @@ test_expect_success PERL 'setup with 2 files different' '
 test_expect_success PERL 'say no to the first file' '
 	(echo n && echo) >input &&
 	git difftool -x cat branch <input >output &&
-	stdin_contains m2 <output &&
-	stdin_contains br2 <output &&
-	stdin_doesnot_contain master <output &&
-	stdin_doesnot_contain branch <output
+	grep m2 output &&
+	grep br2 output &&
+	! grep master output &&
+	! grep branch output
 '
 
 test_expect_success PERL 'say no to the second file' '
 	(echo && echo n) >input &&
 	git difftool -x cat branch <input >output &&
-	stdin_contains master <output &&
-	stdin_contains branch  <output &&
-	stdin_doesnot_contain m2 <output &&
-	stdin_doesnot_contain br2 <output
+	grep master output &&
+	grep branch output &&
+	! grep m2 output &&
+	! grep br2 output
 '
 
 test_expect_success PERL 'difftool --tool-help' '
 	git difftool --tool-help >output &&
-	stdin_contains tool <output
+	grep tool output
 '
 
 test_expect_success PERL 'setup change in subdirectory' '

@@ -330,28 +320,28 @@ test_expect_success PERL 'setup change in subdirectory' '
 
 test_expect_success PERL 'difftool -d' '
 	git difftool -d --extcmd ls branch >output &&
-	stdin_contains sub <output &&
-	stdin_contains file <output
+	grep sub output &&
+	grep file output
 '
 
 test_expect_success PERL 'difftool --dir-diff' '
 	git difftool --dir-diff --extcmd ls branch >output &&
-	stdin_contains sub <output &&
-	stdin_contains file <output
+	grep sub output &&
+	grep file output
 '
 
 test_expect_success PERL 'difftool --dir-diff ignores --prompt' '
 	git difftool --dir-diff --prompt --extcmd ls branch >output &&
-	stdin_contains sub <output &&
-	stdin_contains file <output
+	grep sub output &&
+	grep file output
 '
 
 test_expect_success PERL 'difftool --dir-diff from subdirectory' '
 	(
 		cd sub &&
 		git difftool --dir-diff --extcmd ls branch >output &&
-		stdin_contains sub <output &&
-		stdin_contains file <output
+		grep sub output &&
+		grep file output
 	)
 '
Signature · 1 line
1.8.2.411.g65a544e
author
#7 lore raw

PATCH 4/5 t7800: fix tests when difftool uses --no-symlinks

In reply to John Keeping

When 'git difftool --dir-diff' is using --no-symlinks (either explicitly or implicitly because it's running on Windows), any working tree files that have been copied to the temporary directory are copied back after the difftool completes.

Because an earlier test uses "git add .", the "output" file used by tests is tracked by Git and the following sequence occurs during some tests:

1) the shell opens "output" to redirect the difftool output
2) difftool copies the empty "output" to the temporary directory
3) difftool runs "ls" which writes to "output"
4) difftool copies the empty "output" file back over the output of the command
5) the output file doesn't contain the expected output, causing the test to fail

Instead of adding all changes, explicitly add only the files that the test is using, allowing later tests to write their result files into the working tree.

Signed-off-by John Keeping <[email protected]>

---
Changes since v2:
- Fix grammar issues in commit message
- Remove the final paragraph of the commit message since this is now addressed by patch 2/5 in this series

 t/t7800-difftool.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
t/t7800-difftool.sh +1 -1
index 9fd09db..df443a9 100755
@@ -314,7 +314,7 @@ test_expect_success PERL 'setup change in subdirectory' '
 	git commit -m "added sub/sub" &&
 	echo test >>file &&
 	echo test >>sub/sub &&
-	git add . &&
+	git add file sub/sub &&
 	git commit -m "modified both"
 '
Signature · 1 line
1.8.2.411.g65a544e
author
#8 lore raw

PATCH 5/5 t7800: run --dir-diff tests with and without symlinks

In reply to John Keeping

Currently the difftool --dir-diff tests may or may not use symlinks depending on the operating system on which they are run. In one case this has caused a test failure to be noticed only on Windows when the test also fails on Linux when difftool is invoked with --no-symlinks.

Rewrite these tests so that they do not depend on the environment but run explicitly with both --symlinks and --no-symlinks, protecting the --symlinks version with a SYMLINKS prerequisite.

Signed-off-by John Keeping <[email protected]>
---
 t/t7800-difftool.sh | 27 +++++++++++++++++++--------
 1 file changed, 19 insertions(+), 8 deletions(-)
t/t7800-difftool.sh +19 -8
index df443a9..a6bd99e 100755
@@ -318,28 +318,39 @@ test_expect_success PERL 'setup change in subdirectory' '
 	git commit -m "modified both"
 '
 
-test_expect_success PERL 'difftool -d' '
-	git difftool -d --extcmd ls branch >output &&
+run_dir_diff_test () {
+	test_expect_success PERL "$1 --no-symlinks" "
+		symlinks=--no-symlinks &&
+		$2
+	"
+	test_expect_success PERL,SYMLINKS "$1 --symlinks" "
+		symlinks=--symlinks &&
+		$2
+	"
+}
+
+run_dir_diff_test 'difftool -d' '
+	git difftool -d $symlinks --extcmd ls branch >output &&
 	grep sub output &&
 	grep file output
 '
 
-test_expect_success PERL 'difftool --dir-diff' '
-	git difftool --dir-diff --extcmd ls branch >output &&
+run_dir_diff_test 'difftool --dir-diff' '
+	git difftool --dir-diff $symlinks --extcmd ls branch >output &&
 	grep sub output &&
 	grep file output
 '
 
-test_expect_success PERL 'difftool --dir-diff ignores --prompt' '
-	git difftool --dir-diff --prompt --extcmd ls branch >output &&
+run_dir_diff_test 'difftool --dir-diff ignores --prompt' '
+	git difftool --dir-diff $symlinks --prompt --extcmd ls branch >output &&
 	grep sub output &&
 	grep file output
 '
 
-test_expect_success PERL 'difftool --dir-diff from subdirectory' '
+run_dir_diff_test 'difftool --dir-diff from subdirectory' '
 	(
 		cd sub &&
-		git difftool --dir-diff --extcmd ls branch >output &&
+		git difftool --dir-diff $symlinks --extcmd ls branch >output &&
 		grep sub output &&
 		grep file output
 	)
Signature · 1 line
1.8.2.411.g65a544e