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

John Keeping [off-list ref] writes:

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

Good.

In the longer term, difftool probably needs to learn to warn the user
instead of overwrite any changes that have been made to the working tree
file.

Questionable.

Admittedly I do not use difftool myself, and I have long assumed
that difftool users are using the tools to _view_ the changes, but
apparently some of the tools let the user muck with what is shown,
and also apparently people seem to like the fact that they can make
changes.  So I've led to believe the "update in difftool, take the
change back to working tree, either by making symbolic links or
copying them back" behaviour was a _feature_.

It is possible that this is not universally considerd as a feature,
but if that is the case, I think the right way to do this is to tell
the tools _not_ to let the user to modify contents they show in the
first place, not letting the user modify and then warning after the
fact.

On Sat, Mar 23, 2013 at 10:19:36PM -0700, Junio C Hamano wrote:

quoted

In the longer term, difftool probably needs to learn to warn the user
instead of overwrite any changes that have been made to the working tree
file.

Questionable.

Admittedly I do not use difftool myself, and I have long assumed
that difftool users are using the tools to _view_ the changes, but
apparently some of the tools let the user muck with what is shown,
and also apparently people seem to like the fact that they can make
changes.  So I've led to believe the "update in difftool, take the
change back to working tree, either by making symbolic links or
copying them back" behaviour was a _feature_.

Yes it is.  I think my explanation wasn't clear enough here.

What currently happens is that after the user's tool has finished
running the working tree file and temporary file are compared and if
they are different then the temporary file is copied over the working
tree file.

This is good if the user has edited the temporary file, but what if they
edit they working tree file while using the tool to examine the
differences?  I think we need to at the very least look at the mtime of
the files and refuse to copy over the temporary file if that of the
working tree file is newer.

Obviously none of this matters if we can use symlinks, but 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.

It is possible that this is not universally considerd as a feature,
but if that is the case, I think the right way to do this is to tell
the tools _not_ to let the user to modify contents they show in the
first place, not letting the user modify and then warning after the
fact.

On Sun, Mar 24, 2013 at 1:19 AM, Junio C Hamano [off-list ref] wrote:

Admittedly I do not use difftool myself, and I have long assumed
that difftool users are using the tools to _view_ the changes, but
apparently some of the tools let the user muck with what is shown,
and also apparently people seem to like the fact that they can make
changes.  So I've led to believe the "update in difftool, take the
change back to working tree, either by making symbolic links or
copying them back" behaviour was a _feature_.

Definitely. Here's how I use it:
http://matthewlmcclure.com/s/2013/03/14/use-emacs-dircmp-mode-as-a-git-difftool.html

-- 
Matt McClure
http://www.matthewlmcclure.com
http://www.mapmyfitness.com/profile/matthewlmcclure

On Sun, Mar 24, 2013 at 8:36 AM, John Keeping [off-list ref] 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

-- 
Matt McClure
http://www.matthewlmcclure.com
http://www.mapmyfitness.com/profile/matthewlmcclure

On Sun, Mar 24, 2013 at 09:31:45AM -0400, Matt McClure wrote:

On Sun, Mar 24, 2013 at 8:36 AM, John Keeping [off-list ref] wrote:

quoted

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

On Sun, Mar 24, 2013 at 5:36 AM, John Keeping [off-list ref] wrote:

On Sat, Mar 23, 2013 at 10:19:36PM -0700, Junio C Hamano wrote:

quoted

quoted

In the longer term, difftool probably needs to learn to warn the user
instead of overwrite any changes that have been made to the working tree
file.

Questionable.

Admittedly I do not use difftool myself, and I have long assumed
that difftool users are using the tools to _view_ the changes, but
apparently some of the tools let the user muck with what is shown,
and also apparently people seem to like the fact that they can make
changes.  So I've led to believe the "update in difftool, take the
change back to working tree, either by making symbolic links or
copying them back" behaviour was a _feature_.

Yes it is.  I think my explanation wasn't clear enough here.

What currently happens is that after the user's tool has finished
running the working tree file and temporary file are compared and if
they are different then the temporary file is copied over the working
tree file.

This is good if the user has edited the temporary file, but what if they
edit they working tree file while using the tool to examine the
differences?  I think we need to at the very least look at the mtime of
the files and refuse to copy over the temporary file if that of the
working tree file is newer.

Obviously none of this matters if we can use symlinks, but 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.

Thanks, this adds a little more safety to the operation, which is good.
The downside is that it's a performance hit since we end up running
an additional hash-object on every worktree file.
I would definitely choose safety/correctness in this situation.

This makes me wonder whether the modifiable mode should be made
more explicit, either in the documentation or via a flag.

Imagine if --dir-diff also honored --edit and --no-edit flags.

Right now --edit is the default.  If we had foreseen these various
edge cases and unintended copy-backs then we may have initially
chosen --no-edit as the default, but that's not really my point.

What I'm thinking is that it might be good for the tool to
learn --edit/--no-edit so that the symlink/copy-back heuristic
can be documented alongside that option.  Users can then know
what to expect when using this mode.  --no-edit would also be
faster since it can avoid all these extra steps.

It could also learn "difftool.dirDiffEditable" to control the
default, which would eliminate the pain in needing to supply
the flag on every invocation.

What do you think about officially supporting a read-only mode?
-- 
David

Am 3/24/2013 16:15, schrieb John Keeping:

This is gross. Can't we do much better here? Difftool already keeps a
GIT_INDEX of the files in the temporary tree ($tmpdir/rindex). Running
git-diff-files should be sufficient to tell which ones where edited via
the users's diff-tool. Then you can restrict calling hash-object to only
those worktree files where an "edit collision" needs to be checked for.

You could also keep a parallel index that keeps the state of the same set
of files in the worktree. Then another git-diff-files call could replace
the other half of hash-object calls.

+		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);

-- Hannes

On Mon, Mar 25, 2013 at 08:41:59AM +0100, Johannes Sixt wrote:

Am 3/24/2013 16:15, schrieb John Keeping:

quoted

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 <redacted>
---
 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");

This is gross. Can't we do much better here? Difftool already keeps a
GIT_INDEX of the files in the temporary tree ($tmpdir/rindex). Running
git-diff-files should be sufficient to tell which ones where edited via
the users's diff-tool. Then you can restrict calling hash-object to only
those worktree files where an "edit collision" needs to be checked for.

That's only the case for files that are not copied from the working
tree, so the temporary index doesn't contain the files that are of
interest here.

You could also keep a parallel index that keeps the state of the same set
of files in the worktree. Then another git-diff-files call could replace
the other half of hash-object calls.

I like the idea of creating an index from the working tree files and
using it here.  If we create a "starting state" index for these files,
we should be able to run git-diff-files against both the working tree
and the temporary tree at this point and compare the output.  I'll try
this approach this evening.

quoted

+		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);

On Sun, Mar 24, 2013 at 02:29:40PM -0700, David Aguilar wrote:

This makes me wonder whether the modifiable mode should be made
more explicit, either in the documentation or via a flag.

Imagine if --dir-diff also honored --edit and --no-edit flags.

Right now --edit is the default.  If we had foreseen these various
edge cases and unintended copy-backs then we may have initially
chosen --no-edit as the default, but that's not really my point.

I view --symlinks as the default, which avoids most of this pain ;-)
I guess we're talking about three different "working tree files" modes
here: symlink, copy-copyback and copy-readonly.

I wonder if anyone uses --no-symlinks when they are not forced to by
their operating system?  What is the use case if they do?

What I'm thinking is that it might be good for the tool to
learn --edit/--no-edit so that the symlink/copy-back heuristic
can be documented alongside that option.  Users can then know
what to expect when using this mode.  --no-edit would also be
faster since it can avoid all these extra steps.

It could also learn "difftool.dirDiffEditable" to control the
default, which would eliminate the pain in needing to supply
the flag on every invocation.

What do you think about officially supporting a read-only mode?

How would that interoperate with symlink mode?  Should --no-edit imply
--no-symlinks or does the --[no-]edit option only have an effect if
--no-symlinks is in effect?

I don't think this is the first time this idea has been suggested, so
that's some indicator that it's a good idea.  I'm not sure about
--edit/--no-edit for this though.  The behaviour isn't really similar to
the way that option works with git-commit, git-merge, etc.  I don't have
a better suggestion at the moment though.


John

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 <redacted>
---
On Mon, Mar 25, 2013 at 10:42:19AM +0000, John Keeping wrote:

On Mon, Mar 25, 2013 at 08:41:59AM +0100, Johannes Sixt wrote:

quoted

This is gross. Can't we do much better here? Difftool already keeps a
GIT_INDEX of the files in the temporary tree ($tmpdir/rindex). Running
git-diff-files should be sufficient to tell which ones where edited via
the users's diff-tool. Then you can restrict calling hash-object to only
those worktree files where an "edit collision" needs to be checked for.

That's only the case for files that are not copied from the working
tree, so the temporary index doesn't contain the files that are of
interest here.

quoted

You could also keep a parallel index that keeps the state of the same set
of files in the worktree. Then another git-diff-files call could replace
the other half of hash-object calls.

I like the idea of creating an index from the working tree files and
using it here.  If we create a "starting state" index for these files,
we should be able to run git-diff-files against both the working tree
and the temporary tree at this point and compare the output.

Here's an attempt at taking this approach, built on
jk/difftool-dir-diff-edit-fix.

 git-difftool.perl   | 73 +++++++++++++++++++++++++++++++++++++++++++----------
 t/t7800-difftool.sh | 26 +++++++++++++++++++
 2 files changed, 85 insertions(+), 14 deletions(-)

diff --git a/git-difftool.perl b/git-difftool.perl
index c433e86..d10f7d2 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 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

Am 3/25/2013 22:44, schrieb John Keeping:

I don't have a lot to say about the patch text, except that there is
nothing obvious out of the ordinary, but please take this with a large
grain of salt, as I'm lacking context. (It's the first time these days
that I'm looking at difftool.)

BTW, did you know that perl is mostly a write-only language? ;-)

The new tests look good.

One question though: Do I understand correctly that the temporary
directories are leaked in the case of an "edit conflict"? If so, is it
worth a warning for the user to clean up the garbage?

-- Hannes

Forgot to mention: The patch passes t7800 on Windows.

Thanks,
-- Hannes

On Tue, Mar 26, 2013 at 09:38:42AM +0100, Johannes Sixt wrote:

Am 3/25/2013 22:44, schrieb John Keeping:

quoted

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 <redacted>
---
On Mon, Mar 25, 2013 at 10:42:19AM +0000, John Keeping wrote:

quoted

On Mon, Mar 25, 2013 at 08:41:59AM +0100, Johannes Sixt wrote:

quoted

This is gross. Can't we do much better here? Difftool already keeps a
GIT_INDEX of the files in the temporary tree ($tmpdir/rindex). Running
git-diff-files should be sufficient to tell which ones where edited via
the users's diff-tool. Then you can restrict calling hash-object to only
those worktree files where an "edit collision" needs to be checked for.

That's only the case for files that are not copied from the working
tree, so the temporary index doesn't contain the files that are of
interest here.

quoted

You could also keep a parallel index that keeps the state of the same set
of files in the worktree. Then another git-diff-files call could replace
the other half of hash-object calls.

I like the idea of creating an index from the working tree files and
using it here.  If we create a "starting state" index for these files,
we should be able to run git-diff-files against both the working tree
and the temporary tree at this point and compare the output.

Here's an attempt at taking this approach, built on
jk/difftool-dir-diff-edit-fix.

 git-difftool.perl   | 73 +++++++++++++++++++++++++++++++++++++++++++----------
 t/t7800-difftool.sh | 26 +++++++++++++++++++
 2 files changed, 85 insertions(+), 14 deletions(-)

diff --git a/git-difftool.perl b/git-difftool.perl
index c433e86..d10f7d2 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);

I don't have a lot to say about the patch text, except that there is
nothing obvious out of the ordinary, but please take this with a large
grain of salt, as I'm lacking context. (It's the first time these days
that I'm looking at difftool.)

BTW, did you know that perl is mostly a write-only language? ;-)

quoted

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"
+'

The new tests look good.

One question though: Do I understand correctly that the temporary
directories are leaked in the case of an "edit conflict"? If so, is it
worth a warning for the user to clean up the garbage?

Do you mean for normal users or for those running the tests?  In normal
usage we do print a warning - it's in the existing code, triggered by
setting "$error = 1" - you can see that if you run the tests with "-v".

The last test does result in /tmp filling up with temporary directories
though, it would be good if the test could clean up after itself.  The
best I can come up with is adding something like this immediately after
running difftool but I'm not entirely happy with the ".." in the
argument to rm:

	test_when_finished rm -rf "$(cat tmpdir)/.."

Am 3/26/2013 10:31, schrieb John Keeping:

On Tue, Mar 26, 2013 at 09:38:42AM +0100, Johannes Sixt wrote:

quoted

One question though: Do I understand correctly that the temporary
directories are leaked in the case of an "edit conflict"? If so, is it
worth a warning for the user to clean up the garbage?

Do you mean for normal users or for those running the tests?  In normal
usage we do print a warning - it's in the existing code, triggered by
setting "$error = 1" - you can see that if you run the tests with "-v".

I meant for normal users. I see the error now. Thanks.

The last test does result in /tmp filling up with temporary directories
though, it would be good if the test could clean up after itself.  The
best I can come up with is adding something like this immediately after
running difftool but I'm not entirely happy with the ".." in the
argument to rm:

	test_when_finished rm -rf "$(cat tmpdir)/.."

Wrap the test in

	(
		TMPDIR=$TRASH_DIRECTORY &&
		export TMPDIR &&
	...
	)

It works for me.

-- Hannes

On Tue, Mar 26, 2013 at 10:53:48AM +0100, Johannes Sixt wrote:

Am 3/26/2013 10:31, schrieb John Keeping:

quoted

On Tue, Mar 26, 2013 at 09:38:42AM +0100, Johannes Sixt wrote:
The last test does result in /tmp filling up with temporary directories
though, it would be good if the test could clean up after itself.  The
best I can come up with is adding something like this immediately after
running difftool but I'm not entirely happy with the ".." in the
argument to rm:

	test_when_finished rm -rf "$(cat tmpdir)/.."

Wrap the test in

	(
		TMPDIR=$TRASH_DIRECTORY &&
		export TMPDIR &&
	...
	)

It works for me.

Nice.  I've reviewed File::Spec and it looks like that TMPDIR takes
priority on every operating system except VMS, and I don't think we care
about that.

Unless Junio says otherwise, I'll hold off sending this until difftool
calms down a bit to avoid too many conflicted merges.

On Mon, Mar 25, 2013 at 5:44 PM, John Keeping [off-list ref] wrote:

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.

When there's a conflict, does difftool save both conflicting files? Or
only the working tree copy? I think it should preserve both copies on
disk.

-- 
Matt McClure
http://www.matthewlmcclure.com
http://www.mapmyfitness.com/profile/matthewlmcclure

On Tue, Mar 26, 2013 at 04:52:02PM -0400, Matt McClure wrote:

On Mon, Mar 25, 2013 at 5:44 PM, John Keeping [off-list ref] wrote:

quoted

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.

When there's a conflict, does difftool save both conflicting files? Or
only the working tree copy? I think it should preserve both copies on
disk.

It preserves both copies - the "clean the temporary directory" step is
just skipped.

This isn't ideal since the temporary copy will be under a temporary
directory somewhere but is better than the current behaviour.  It might
be nice to move the temporary file back with an extension so that the
files are at least near each other but I don't think that's needed in
the first version of this change.