[PATCH 0/5] difftool improvements

[PATCH 0/5] difftool improvements

[John Keeping]

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(-)

-- 
1.8.2.411.g65a544e

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

[John Keeping]

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 <john@keeping.me.uk>
---
 t/t7800-difftool.sh | 30 +++++++++++++++---------------
 1 file changed, 15 insertions(+), 15 deletions(-)

diff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh
index c6d6b1c..e6a16cd 100755
--- a/t/t7800-difftool.sh
+++ b/t/t7800-difftool.sh
@@ -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
-- 
1.8.2.411.g65a544e

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

[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 <john@keeping.me.uk>

---
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(-)

diff --git a/git-difftool.perl b/git-difftool.perl
index 663640d..844f619 100755
--- a/git-difftool.perl
+++ b/git-difftool.perl
@@ -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);
diff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh
index e6a16cd..017f55a 100755
--- a/t/t7800-difftool.sh
+++ b/t/t7800-difftool.sh
@@ -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
-- 
1.8.2.411.g65a544e

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

[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 <j.sixt@viscovery.net>
Signed-off-by: John Keeping <john@keeping.me.uk>
---
 t/t7800-difftool.sh | 44 +++++++++++++++++---------------------------
 1 file changed, 17 insertions(+), 27 deletions(-)

diff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh
index 017f55a..9fd09db 100755
--- a/t/t7800-difftool.sh
+++ b/t/t7800-difftool.sh
@@ -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
 	)
 '
 
-- 
1.8.2.411.g65a544e

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

[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 <john@keeping.me.uk>

---
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(-)

diff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh
index 9fd09db..df443a9 100755
--- a/t/t7800-difftool.sh
+++ b/t/t7800-difftool.sh
@@ -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"
 '
 
-- 
1.8.2.411.g65a544e

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

[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 <john@keeping.me.uk>
---
 t/t7800-difftool.sh | 27 +++++++++++++++++++--------
 1 file changed, 19 insertions(+), 8 deletions(-)

diff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh
index df443a9..a6bd99e 100755
--- a/t/t7800-difftool.sh
+++ b/t/t7800-difftool.sh
@@ -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
 	)
-- 
1.8.2.411.g65a544e

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

[Junio C Hamano]

John Keeping <john@keeping.me.uk> writes:

> 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.

> 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 <john@keeping.me.uk>
>
> ---
> 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(-)
>
> diff --git a/git-difftool.perl b/git-difftool.perl
> index 663640d..844f619 100755
> --- a/git-difftool.perl
> +++ b/git-difftool.perl
> @@ -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");

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.

>  	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?

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

[John Keeping]

On Fri, Mar 29, 2013 at 01:20:50PM -0700, Junio C Hamano wrote:
> John Keeping <john@keeping.me.uk> writes:
> 
> > 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.
> 
> > 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 <john@keeping.me.uk>
> >
> > ---
> > 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(-)
> >
> > diff --git a/git-difftool.perl b/git-difftool.perl
> > index 663640d..844f619 100755
> > --- a/git-difftool.perl
> > +++ b/git-difftool.perl
> > @@ -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");
> 
> 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.

Sounds sensible.  I'll look into doing this.

> >  	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?

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

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

[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 <john@keeping.me.uk>

---
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(-)

diff --git a/git-difftool.perl b/git-difftool.perl
index 663640d..6780292 100755
--- a/git-difftool.perl
+++ b/git-difftool.perl
@@ -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);
diff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh
index e6a16cd..017f55a 100755
--- a/t/t7800-difftool.sh
+++ b/t/t7800-difftool.sh
@@ -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
-- 
1.8.2.411.g65a544e