Re: [PATCH v1 1/1] git diff --quiet exits with 1 on clean tree with CRLF conversions

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 1, 2017, 21:54 UTC
Message-ID: <xmqqa894fyst.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <xmqqr32gg0o6.fsf@gitster.mtv.corp.google.com>

Now I thought about it through a bit more thoroughly, I think this is the right approach, so here is my (tenative) final version.

I seem to be getty really rusty---after all the codepaths involved are practically all my code and I should have noticed the real culprit during my first attempt X-<.

Thanks for helping.

-- >8 --
Subject: [PATCH] diff: do not short-cut CHECK_SIZE_ONLY check in diff_populate_filespec()

Callers of diff_populate_filespec() can choose to ask only for the size of the blob without grabbing the blob data, and the function, after running lstat() when the filespec points at a working tree file, returns by copying the value in size field of the stat structure into the size field of the filespec when this is the case.

However, this short-cut cannot be taken if the contents from the path needs to go through convert_to_git(), whose resulting real blob data may be different from what is in the working tree file.

As "git diff --quiet" compares the .size fields of filespec structures to skip content comparison, this bug manifests as a false "there are differences" for a file that needs eol conversion, for example.

Reported-by: Mike Crowe <mac@mcrowe.com>
Helped-by: Torsten Bögershausen <tboegi@web.de>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 diff.c                    | 19 ++++++++++++++++++-
 t/t0028-diff-converted.sh | 27 +++++++++++++++++++++++++++
 2 files changed, 45 insertions(+), 1 deletion(-)
 create mode 100755 t/t0028-diff-converted.sh

diff --git a/diff.c b/diff.c
index 8c78fce49d..dc51dceb44 100644
--- a/diff.c
+++ b/diff.c
@@ -2792,8 +2792,25 @@ int diff_populate_filespec(struct diff_filespec *s, unsigned int flags)
 			s->should_free = 1;
 			return 0;
 		}
-		if (size_only)
+
+		/*
+		 * Even if the caller would be happy with getting
+		 * only the size, we cannot return early at this
+		 * point if the path requires us to run the content
+		 * conversion.
+		 */
+		if (!would_convert_to_git(s->path) && size_only)
 			return 0;
+
+		/*
+		 * Note: this check uses xsize_t(st.st_size) that may
+		 * not be the true size of the blob after it goes
+		 * through convert_to_git().  This may not strictly be
+		 * correct, but the whole point of big_file_threashold
+		 * and is_binary check being that we want to avoid
+		 * opening the file and inspecting the contents, this
+		 * is probably fine.
+		 */
 		if ((flags & CHECK_BINARY) &&
 		    s->size > big_file_threshold && s->is_binary == -1) {
 			s->is_binary = 1;
diff --git a/t/t0028-diff-converted.sh b/t/t0028-diff-converted.sh
new file mode 100755
index 0000000000..3d5ab9565b
--- /dev/null
+++ b/t/t0028-diff-converted.sh
@@ -0,0 +1,27 @@
+#!/bin/sh
+#
+# Copyright (c) 2017 Mike Crowe
+#
+# These tests ensure that files changing line endings in the presence
+# of .gitattributes to indicate that line endings should be ignored
+# don't cause 'git diff' or 'git diff --quiet' to think that they have
+# been changed.
+
+test_description='git diff with files that require CRLF conversion'
+
+. ./test-lib.sh
+
+test_expect_success setup '
+	echo "* text=auto" >.gitattributes &&
+	printf "Hello\r\nWorld\r\n" >crlf.txt &&
+	git add .gitattributes crlf.txt &&
+	git commit -m "initial"
+'
+
+test_expect_success 'quiet diff works on file with line-ending change that has no effect on repository' '
+	printf "Hello\r\nWorld\n" >crlf.txt &&
+	git status &&
+	git diff --quiet
+'
+
+test_done

-- 
2.12.0-319-gc5f21175ee