Bugfix: GIT_EXTERNAL_DIFF with more than one changed files

Regression introduced in 479b0ae81c9291a8bb8d7b2347cc58eeaa701304.

When there is more than one file that are changed, running git diff with GIT_EXTERNAL_DIFF works only for the first file.

This patch fixes this problem and added a test case for it.

---
 diff.c                   |    8 ++++----
 t/t4020-diff-external.sh |    8 ++++++++
 2 files changed, 12 insertions(+), 4 deletions(-)

index a5a540f..be3859e 100644
@@ -184,11 +184,11 @@ static int remove_tempfile_installed;
 static void remove_tempfile(void)
 {
 	int i;
-	for (i = 0; i < ARRAY_SIZE(diff_temp); i++)
-		if (diff_temp[i].name == diff_temp[i].tmp_path) {
+	for (i = 0; i < ARRAY_SIZE(diff_temp); i++) {
+		if (diff_temp[i].name == diff_temp[i].tmp_path)
 			unlink(diff_temp[i].name);
-			diff_temp[i].name = NULL;
-		}
+		diff_temp[i].name = NULL;
+	}
 }
 
 static void remove_tempfile_on_signal(int signo)

index caea292..281680d 100755
@@ -128,4 +128,12 @@ test_expect_success 'force diff with "diff"' '
 	test_cmp "$TEST_DIRECTORY"/t4020/diff.NUL actual
 '
 
+test_expect_success 'GIT_EXTERNAL_DIFF with more than one changed files' '
+	echo anotherfile > file2 &&
+	git add file2 &&
+	git commit -m "added 2nd file" &&
+	echo modified >file2 &&
+	GIT_EXTERNAL_DIFF=echo git diff
+'
+
 test_done

1.6.2.rc0.55.g30aa4f

Yikes. Thanks for finding this.

Actually, the situation is a little more complex than what you describe.

We used to just re-use the diff_temp array unconditionally; the commit you mention introduced a safety check to make sure we are not overwriting an existing tempfile that should be cleaned up. That safety check looks for the "name" field being NULL to signal an unused slot.

But the "remove_tempfile" function uses a different test to see if a slot needs to be deleted: if the name and the tmp_path are the same. And they would not be if we were able to reuse a working tree file as part of the diff, in which case there would be nothing to clean up. And if we did clean something up, we set the name field to NULL.

So this bug should trigger only in the face of reusing worktree files. I checked your test; it constructs a diff between the worktree and the index, so it correctly finds the problem.

Thanks, both.

Jeff, according to your analysis, this shouldn't trigger when core.autocrlf is set, should it?

Depending on the diff you are doing. If one of the sides is the worktree, then we should always be using the worktree file. But for "diff --cached" it depends on the file contents matching the index (in theory, it would work for arbitrary tree diffs when one side matches the worktree, but see the comment in reuse_worktree_file -- if nobody has looked at the cache already, we don't load it just for this).

I tried to construct a simple test that shows this behavior, but I couldn't. I did:

mkdir repo && cd repo && git init

git config core.autocrlf true

printf 'one\r\n' >file1
printf 'one\r\n' >file2
git add .
git commit -m one

printf 'two\r\n' >file1
printf 'two\r\n' >file2
git add -u

  PAGER=cat GIT_EXTERNAL_DIFF=echo git diff --cached

which should fail without the core.autocrlf setting, but work otherwise. But it doesn't, and the reason is that the content in the index actually has the CRLF:

  $ xxd < file1
  0000000: 7477 6f0d 0a                             two..
  $ git cat-file blob :file1 | xxd
  0000000: 7477 6f0d 0a                             two..

which has me confused. Am I using autocrlf wrong? I have been fortunate enough in the past never to work on filesystems that needed such a thing.

-Peff