difftool: silence uninitialized variable warning

t/t7800-difftool.sh +0 -0

index fb00273..21fbba9 100755

Quoted 5 lines · David Aguilar

t/t7800-difftool.sh +0 -0

index fb00273..21fbba9 100755

t/t7800-difftool.sh test_expect_success PERL 'custom commands' ' · line 67

Quoted 10 lines

t/t7800-difftool.sh +2 -2

@@ -60,9 +60,9 @@ test_expect_success PERL 'custom commands' '
 '
 
 test_expect_success PERL 'custom tool commands override built-ins' '
-     test_config difftool.defaults.cmd "cat \"\$REMOTE\"" &&
+     test_config difftool.vimdiff "cat \"\$REMOTE\"" &&
      echo master >expect &&
-     git difftool --tool defaults --no-prompt branch >actual &&
+     git difftool --tool vimdiff --no-prompt branch >actual &&
      test_cmp expect actual
 '

Eek.

$ sh t7800-difftool.sh -i
ok 1 - setup
ok 2 - custom commands
not ok 3 - custom tool commands override built-ins
#
#               test_config difftool.vimdiff "cat \"\$REMOTE\"" &&
#               echo master >expect &&
#               git difftool --tool vimdiff --no-prompt branch >actual &&
#               test_cmp expect actual
#

Running the same test with "-v" seems to get stuck with this forever:

expecting success:

        test_config difftool.vimdiff "cat \"\$REMOTE\"" &&
        echo master >expect &&
        git difftool --tool vimdiff --no-prompt branch >actual &&
        test_cmp expect actual

Vim: Warning: Output is not to a terminal
Vim: Warning: Input is not from a terminal

Am 2/21/2013 5:03, schrieb David Aguilar:

t/t7800-difftool.sh test_expect_success PERL 'setup change in subdirectory' ' · line 335

Quoted 8 lines

test_expect_success PERL 'difftool -d' '

t/t7800-difftool.sh +3 -3

-     diff=$(git difftool -d --extcmd ls branch) &&
-     echo "$diff" | stdin_contains sub &&
-     echo "$diff" | stdin_contains file
+     git difftool -d --extcmd ls branch >output &&
+     stdin_contains sub <output &&
+     stdin_contains file <output
 '

This test is broken on Windows. There is this code in git-difftool.perl

        for my $file (@worktree) {
...
                        copy("$b/$file", "$workdir/$file") or
                        exit_cleanup($tmpdir, 1);
...
        }

@worktree is populated with all files in the worktree. At this point, "output" is among them. Then follows an attempt to copy a file over "$workdir/$file". I guess that is some link+remove magic going on behind the scenes. At any rate, this fails on Windows with "D:/Src/mingw-git/t/trash directory.t7800-difftool/../../git-difftool line 408: Bad file number", because files that are open cannot be written from outside (the file is open due to the redirection in the test snippet).

What is going on here? Why can this ever succeed even on Unix?

Same for some later tests.

BTW, while debugging this, I found the use of the helper function stdin_contains() highly unhelpful; it just resolves to a 'grep' that on top of all hides stdout. Please don't do that. Just use unadorned grep like we do everywhere else.

I started digging in and the @worktree_files (aka @worktree above) is populated from the output of "git diff --raw ...".

Seeing the "output" filename in "diff --raw" implies that one of the tests added "output" to the index somehow. I do not see that happening anywhere, though, so I do not know how it would end up in the @worktree array if it is not reported by "diff --raw".

My current understanding of how it could possibly be open twice:

1. via the >output redirect
2. via the copy() perl code which is fed by @worktree

So I'm confused. Why would we get different results on Windows?

Am 3/20/2013 23:59, schrieb David Aguilar:

Quoted 9 lines

I started digging in and the @worktree_files (aka @worktree above) is populated from the output of "git diff --raw ...".

Seeing the "output" filename in "diff --raw" implies that one of the tests added "output" to the index somehow. I do not see that happening anywhere, though, so I do not know how it would end up in the @worktree array if it is not reported by "diff --raw".

Quoted 6 lines

My current understanding of how it could possibly be open twice:

1. via the >output redirect
2. via the copy() perl code which is fed by @worktree

So I'm confused. Why would we get different results on Windows?

I tracked down the difference between Windows and Linux, and it is...

	for my $file (@worktree) {
		next if $symlinks && -l "$b/$file";

... this line in sub dir_diff. On Linux, we take the short-cut, but on Windows we proceed through the rest of the loop,

Am 3/21/2013 8:41, schrieb Johannes Sixt:

Quoted 17 lines

Am 3/20/2013 23:59, schrieb David Aguilar:

Quoted 9 lines

I started digging in and the @worktree_files (aka @worktree above) is populated from the output of "git diff --raw ...".

Seeing the "output" filename in "diff --raw" implies that one of the tests added "output" to the index somehow. I do not see that happening anywhere, though, so I do not know how it would end up in the @worktree array if it is not reported by "diff --raw".

Quoted 6 lines

My current understanding of how it could possibly be open twice:

1. via the >output redirect
2. via the copy() perl code which is fed by @worktree

So I'm confused. Why would we get different results on Windows?

Quoted 7 lines

I tracked down the difference between Windows and Linux, and it is...

	for my $file (@worktree) {
		next if $symlinks && -l "$b/$file";

... this line in sub dir_diff. On Linux, we take the short-cut, but on Windows we proceed through the rest of the loop,

And that is likely by design. From the docs:

--symlinks
--no-symlinks

git difftool's default behavior is create symlinks to the working tree when run in --dir-diff mode.

Specifying --no-symlinks instructs 'git difftool' to create copies instead. --no-symlinks is the default on Windows.

And indeed, we have this initialization:

	my %opts = (
		...
		symlinks => $^O ne 'cygwin' &&
				$^O ne 'MSWin32' && $^O ne 'msys',
		...
	);

Can the --dir-diff tests case pass on Cygwin when neither --symlinks nor --no-symlinks is passed?

Perhaps the right solution is this:

t/t7800-difftool.sh +10 -8

index c6d6b1c..19238f6 100755
@@ -328,14 +328,16 @@ 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 &&
+# passing --symlinks helps Cygwin, which defaults to --no-symlinks
+
+test_expect_success PERL,SYMLINKS 'difftool -d' '
+	git difftool -d --symlinks --extcmd ls branch >output &&
 	stdin_contains sub <output &&
 	stdin_contains file <output
 '
 
-test_expect_success PERL 'difftool --dir-diff' '
-	git difftool --dir-diff --extcmd ls branch >output &&
+test_expect_success PERL,SYMLINKS 'difftool --dir-diff' '
+	git difftool --dir-diff --symlinks --extcmd ls branch >output &&
 	stdin_contains sub <output &&
 	stdin_contains file <output
 '

@@ -362,16 +364,16 @@ 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 &&
+test_expect_success PERL,SYMLINKS 'difftool --dir-diff ignores --prompt' '
+	git difftool --dir-diff --symlinks --prompt --extcmd ls branch >output &&
 	stdin_contains sub <output &&
 	stdin_contains file <output
 '
 
-test_expect_success PERL 'difftool --dir-diff from subdirectory' '
+test_expect_success PERL,SYMLINKS 'difftool --dir-diff from subdirectory' '
 	(
 		cd sub &&
-		git difftool --dir-diff --extcmd ls branch >output &&
+		git difftool --dir-diff --symlinks --extcmd ls branch >output &&
 		stdin_contains sub <output &&
 		stdin_contains file <output
 	)

(Only tested on MinGW, which skips the tests.) I leave it to you to write --no-symlinks tests.

-- Hannes

Quoted 46 lines

Am 3/21/2013 8:41, schrieb Johannes Sixt:

Quoted 17 lines

Am 3/20/2013 23:59, schrieb David Aguilar:

Quoted 9 lines

I started digging in and the @worktree_files (aka @worktree above) is populated from the output of "git diff --raw ...".

Seeing the "output" filename in "diff --raw" implies that one of the tests added "output" to the index somehow. I do not see that happening anywhere, though, so I do not know how it would end up in the @worktree array if it is not reported by "diff --raw".

Quoted 6 lines

My current understanding of how it could possibly be open twice:

1. via the >output redirect
2. via the copy() perl code which is fed by @worktree

So I'm confused. Why would we get different results on Windows?

Quoted 7 lines

I tracked down the difference between Windows and Linux, and it is...

	for my $file (@worktree) {
		next if $symlinks && -l "$b/$file";

... this line in sub dir_diff. On Linux, we take the short-cut, but on Windows we proceed through the rest of the loop,

And that is likely by design. From the docs:

--symlinks
--no-symlinks

git difftool's default behavior is create symlinks to the working tree when run in --dir-diff mode.

Specifying --no-symlinks instructs 'git difftool' to create copies instead. --no-symlinks is the default on Windows.

And indeed, we have this initialization:

	my %opts = (
		...
		symlinks => $^O ne 'cygwin' &&
				$^O ne 'MSWin32' && $^O ne 'msys',
		...
	);

Quoted 4 lines

Can the --dir-diff tests case pass on Cygwin when neither --symlinks nor --no-symlinks is passed?

Perhaps the right solution is this:

We already have tests that explicitly pass '--symlinks'. I wonder if it would be better to change "output" to ".git/output", which should avoid the problem by moving the output file out of the working tree.

Am 3/22/2013 11:00, schrieb John Keeping:

Quoted 52 lines · Johannes Sixt

Quoted 50 lines

Am 3/21/2013 8:41, schrieb Johannes Sixt:

Quoted 17 lines

Am 3/20/2013 23:59, schrieb David Aguilar:

Quoted 9 lines

I started digging in and the @worktree_files (aka @worktree above) is populated from the output of "git diff --raw ...".

Seeing the "output" filename in "diff --raw" implies that one of the tests added "output" to the index somehow. I do not see that happening anywhere, though, so I do not know how it would end up in the @worktree array if it is not reported by "diff --raw".

Quoted 6 lines

My current understanding of how it could possibly be open twice:

1. via the >output redirect
2. via the copy() perl code which is fed by @worktree

So I'm confused. Why would we get different results on Windows?

Quoted 7 lines

I tracked down the difference between Windows and Linux, and it is...

	for my $file (@worktree) {
		next if $symlinks && -l "$b/$file";

... this line in sub dir_diff. On Linux, we take the short-cut, but on Windows we proceed through the rest of the loop,

And that is likely by design. From the docs:

--symlinks
--no-symlinks

git difftool's default behavior is create symlinks to the working tree when run in --dir-diff mode.

Specifying --no-symlinks instructs 'git difftool' to create copies instead. --no-symlinks is the default on Windows.

And indeed, we have this initialization:

	my %opts = (
		...
		symlinks => $^O ne 'cygwin' &&
				$^O ne 'MSWin32' && $^O ne 'msys',
		...
	);

Can the --dir-diff tests case pass on Cygwin when neither --symlinks nor --no-symlinks is passed?

Perhaps the right solution is this:

Quoted 3 lines

We already have tests that explicitly pass '--symlinks'. I wonder if it would be better to change "output" to ".git/output", which should avoid the problem by moving the output file out of the working tree.

The point of using --symlinks is not to test the effect of the option, but to test the same thing on Unix and on Cygwin, because the latter uses --no-symlinks by default. Therefore, I think that this sketch is the right thing to do.

But the real problem seems to be that "output" should not be among the files treated in the cited pieces of code (unless I'm wrong, of course; I know next to nothing about git-difftool). It should not matter where the file lives. Just add --no-symlinks to the difftool invocation of test "difftool -d" and watch it fail on Linux, too.

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

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. This includes untracked files in the working tree.

During the tests, this means that the following sequence occurs:

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

Avoid this by writing the output into .git/ which will not be copied or overwritten.

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.

t/t7800-difftool.sh +3 -3

index e694972..1eed439 100755
@@ -319,29 +319,29 @@ test_expect_success PERL 'setup change in subdirectory' '
 '
 
 test_expect_success PERL 'difftool -d' '
-	git difftool -d --extcmd ls branch >output &&
-	grep sub output &&
-	grep file output
+	git difftool -d --extcmd ls branch >.git/output &&
+	grep sub .git/output &&
+	grep file .git/output
 '
...

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. This includes untracked files in the working tree.

During the tests, this means that the following sequence occurs:

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

Avoid this by writing the output into .git/ which will not be copied or overwritten.

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.

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

---
 t/t7800-difftool.sh | 24 ++++++++++++------------
 1 file changed, 12 insertions(+), 12 deletions(-)

t/t7800-difftool.sh +12 -12

index e694972..1eed439 100755
@@ -319,29 +319,29 @@ test_expect_success PERL 'setup change in subdirectory' '
 '
 
 test_expect_success PERL 'difftool -d' '
-	git difftool -d --extcmd ls branch >output &&
-	grep sub output &&
-	grep file output
+	git difftool -d --extcmd ls branch >.git/output &&
+	grep sub .git/output &&
+	grep file .git/output
 '
 
 test_expect_success PERL 'difftool --dir-diff' '
-	git difftool --dir-diff --extcmd ls branch >output &&
-	grep sub output &&
-	grep file output
+	git difftool --dir-diff --extcmd ls branch >.git/output &&
+	grep sub .git/output &&
+	grep file .git/output
 '
 
 test_expect_success PERL 'difftool --dir-diff ignores --prompt' '
-	git difftool --dir-diff --prompt --extcmd ls branch >output &&
-	grep sub output &&
-	grep file output
+	git difftool --dir-diff --prompt --extcmd ls branch >.git/output &&
+	grep sub .git/output &&
+	grep file .git/output
 '
 
 test_expect_success PERL 'difftool --dir-diff from subdirectory' '
 	(
 		cd sub &&
-		git difftool --dir-diff --extcmd ls branch >output &&
-		grep sub output &&
-		grep file output
+		git difftool --dir-diff --extcmd ls branch >../.git/output &&
+		grep sub ../.git/output &&
+		grep file ../.git/output
 	)
 '

Quoted 5 lines · 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. This includes untracked files in the working tree.

Hmph. Why do we populate the temporary directory with a copy of an untracked path in the first place? I thought the point of dir-diff was to materialize only the relevant paths to two temporaries and compare these temporaries with a tool that knows how to compare two directories?

Even if you had path F in HEAD that you are no longer tracking in the working tree, a normal

	$ git diff HEAD

would report the path F to have been deleted, so I would imagine that the preimage side of the temporary directory should get a copy of HEAD:F at path F, while the postimage side of the temporary directory should not even have anything at path F, when dir-diff runs, no?

Isn't that the real reason why the test fails? The path 'output' is not being tracked at any revision or in the index that is involved in the test, is it?

Quoted 10 lines

During the tests, this means that the following sequence occurs:

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

Quoted 2 lines

Avoid this by writing the output into .git/ which will not be copied or overwritten.

It is a good idea to move these test output and expect test vectore files to a different place to make it easier to distinguish them from test input (e.g. "sub", "file", etc.) in general, but the description of the original problem sounds like it is just working around a bug to me. What am I missing?

Quoted 44 lines

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.

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

---
 t/t7800-difftool.sh | 24 ++++++++++++------------
 1 file changed, 12 insertions(+), 12 deletions(-)

t/t7800-difftool.sh +9 -9

index e694972..1eed439 100755
@@ -319,29 +319,29 @@ test_expect_success PERL 'setup change in subdirectory' '
 '
 
 test_expect_success PERL 'difftool -d' '
-	git difftool -d --extcmd ls branch >output &&
-	grep sub output &&
-	grep file output
+	git difftool -d --extcmd ls branch >.git/output &&
+	grep sub .git/output &&
+	grep file .git/output
 '
 
 test_expect_success PERL 'difftool --dir-diff' '
-	git difftool --dir-diff --extcmd ls branch >output &&
-	grep sub output &&
-	grep file output
+	git difftool --dir-diff --extcmd ls branch >.git/output &&
+	grep sub .git/output &&
+	grep file .git/output
 '
 
 test_expect_success PERL 'difftool --dir-diff ignores --prompt' '
-	git difftool --dir-diff --prompt --extcmd ls branch >output &&
-	grep sub output &&
-	grep file output
+	git difftool --dir-diff --prompt --extcmd ls branch >.git/output &&
+	grep sub .git/output &&
+	grep file .git/output
 '
 
 test_expect_success PERL 'difftool --dir-diff from subdirectory' '

t/t7800-difftool.sh test_expect_success PERL 'setup change in subdirectory' ' · line 346

Quoted 10 lines

t/t7800-difftool.sh +3 -3

 	(
 		cd sub &&
-		git difftool --dir-diff --extcmd ls branch >output &&
-		grep sub output &&
-		grep file output
+		git difftool --dir-diff --extcmd ls branch >../.git/output &&
+		grep sub ../.git/output &&
+		grep file ../.git/output
 	)
 '

--
To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Quoted 25 lines · Junio C Hamano

Quoted 5 lines · 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. This includes untracked files in the working tree.

Hmph. Why do we populate the temporary directory with a copy of an untracked path in the first place? I thought the point of dir-diff was to materialize only the relevant paths to two temporaries and compare these temporaries with a tool that knows how to compare two directories?

Even if you had path F in HEAD that you are no longer tracking in the working tree, a normal

      $ git diff HEAD

would report the path F to have been deleted, so I would imagine that the preimage side of the temporary directory should get a copy of HEAD:F at path F, while the postimage side of the temporary directory should not even have anything at path F, when dir-diff runs, no?

Quoted 3 lines

Isn't that the real reason why the test fails? The path 'output' is not being tracked at any revision or in the index that is involved in the test, is it?

Actually it is, which is what I missed earlier.

A couple of tests before this 'setup change in subdirectory' does 'git add .' which is far more general than it needs. Perhaps this is a better change:

t/t7800-difftool.sh +1 -1

index bba8a9d..561c993 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"
 '
 

Quoted 11 lines

Quoted 11 lines

During the tests, this means that the following sequence occurs:

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

Quoted 8 lines

Quoted 2 lines

Avoid this by writing the output into .git/ which will not be copied or overwritten.

It is a good idea to move these test output and expect test vectore files to a different place to make it easier to distinguish them from test input (e.g. "sub", "file", etc.) in general, but the description of the original problem sounds like it is just working around a bug to me. What am I missing?

I think there is a bug, as described in the paragraph below, and this test should be made independent of that. In light of the above I think we can drop this patch and do this with that change instead.

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.

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.

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

---
Changes since v1:
- Fix the actual cause of the issue in the test instead of masking it by moving the output file under .git/

 t/t7800-difftool.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

t/t7800-difftool.sh +1 -1

index e694972..a0b8042 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"
 '

Quoted 3 lines

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.

Quoted 6 lines · Junio C Hamano

Quoted 3 lines

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.

Quoted 7 lines

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.

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?

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?

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.

Quoted 4 lines · John Keeping

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

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 <[email protected]>

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

git-difftool.perl +8 -7

index c433e86..be82b5a 100755
@@ -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";
 

git-difftool.perl +5 -0

+		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: ";

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

Quoted 86 lines

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 <[email protected]>

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

git-difftool.perl +8 -7

index c433e86..be82b5a 100755
@@ -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";
 

git-difftool.perl sub dir_diff · line 401

Quoted 10 lines

git-difftool.perl +4 -6

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

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.

Quoted 15 lines

git-difftool.perl +9 -1

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

Quoted 5 lines · Johannes Sixt

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 3 lines

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.

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

---

Quoted 11 lines · John Keeping

Quoted 5 lines · Johannes Sixt

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 8 lines

Quoted 3 lines

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

git-difftool.perl +55 -13

index c433e86..d10f7d2 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: ";

t/t7800-difftool.sh +15 -0

index db3d3d6..be2042d 100755
@@ -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

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

Quoted 167 lines

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

---

Quoted 11 lines · John Keeping

Quoted 5 lines · Johannes Sixt

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 8 lines

Quoted 3 lines

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

git-difftool.perl +55 -13

index c433e86..d10f7d2 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: ";

git-difftool.perl sub dir_diff · line 453

Quoted 10 lines

git-difftool.perl +4 -1

+			$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 23 lines

t/t7800-difftool.sh +15 -0

index db3d3d6..be2042d 100755
@@ -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

t/t7800-difftool.sh test_expect_success PERL 'difftool --dir-diff from subdirectory' ' · line 434

Quoted 10 lines

t/t7800-difftool.sh +10 -0

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

Quoted 3 lines

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:

Quoted 8 lines · Johannes Sixt

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.

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.

Quoted 4 lines · John Keeping

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.

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

Quoted 4 lines

git-difftool.perl +1 -0

+	for my $file (keys %worktree) {
 		next if $symlinks && -l "$b/$file";
 		next if ! -f "$b/$file";

git-difftool.perl sub dir_diff · line 401

Quoted 10 lines

git-difftool.perl +4 -6

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

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

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.

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.

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

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.

The series looks good, but I can't test it because it does not apply anywhere here.

Am 3/23/2013 14:31, schrieb John Keeping:

Quoted 8 lines

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.

At first, I wondered what the point of having --symlinks and --no-symlinks was when there is no discernable difference. But 1f229345 (difftool: Use symlinks when diffing against the worktree) makes it pretty clear: It's an optimization, and --no-symlinks is only intended as an escape hatch.

Quoted 2 lines

The series looks good, but I can't test it because it does not apply anywhere here.

It's built on top of da/difftool-fixes, is there some problem that stops it applying cleanly on top of that?

Am 3/25/2013 11:35, schrieb John Keeping:

Quoted 6 lines · Johannes Sixt

Quoted 2 lines

The series looks good, but I can't test it because it does not apply anywhere here.

It's built on top of da/difftool-fixes, is there some problem that stops it applying cleanly on top of that?

Thanks. I had only tried trees that were "contaminated" by jk/difftool-dir-diff-edit-fix, which is in conflict with da/difftool-fixes.

t7800 passes on Windows with these patches.

Am 3/25/2013 11:35, schrieb John Keeping:

Quoted 6 lines · Johannes Sixt

Quoted 2 lines

The series looks good, but I can't test it because it does not apply anywhere here.

It's built on top of da/difftool-fixes, is there some problem that stops it applying cleanly on top of that?

Thanks. I had only tried trees that were "contaminated" by jk/difftool-dir-diff-edit-fix, which is in conflict with da/difftool-fixes.

Quoted 8 lines · Johannes Sixt

Am 3/25/2013 11:35, schrieb John Keeping:

Quoted 6 lines · Johannes Sixt

Quoted 2 lines

The series looks good, but I can't test it because it does not apply anywhere here.

It's built on top of da/difftool-fixes, is there some problem that stops it applying cleanly on top of that?

Quoted 2 lines

Thanks. I had only tried trees that were "contaminated" by jk/difftool-dir-diff-edit-fix, which is in conflict with da/difftool-fixes.

Yes, the conflict was unpleasant to deal with. John, I think what is queued on 'pu' is OK, but please double check after I push it out in a few hours.