[PATCH v3/RFC 1/1] read-cache: do not trust a size change when conversion is active

[tboegi@web.de]

From: Torsten Bögershausen <tboegi@web.de>

"git status" can report a file as modified while "git diff" and
"git add", which both run the clean filter, agree its contents are
unchanged:

	git init t && cd t
	printf '* text eol=lf\n' >.gitattributes
	printf 'one\ntwo\nthree\n' >file.txt
	git add . && git commit -m init

	printf 'one\r\ntwo\r\nthree\r\n' >file.txt

	git status --short      # ->  M file.txt
	git diff                # -> empty
	git add file.txt        # -> updates the index

Note that "clean filter" here means the "clean filter" and CRLF conversion,
and all converting of a file in the worktree into the index.

Problems that have been found in real live:
  Tools that rewrites unchanged files in native line endings e.g.
    javadoc, code generators, formatters and some editors change
    the size of every file it touches.
  Editors that insist to replace 4 space with a TAB,
    even if the file is changed manually forth and back -
    Using a clean filter that changes the TAB back into space when
    committing will cause the same confusion.

`git status` and `git diff` disagree because ie_modified()
in read-cache.c concludes that a file is modified
as soon as the size has changed.
This had been true when Git was young, before the clean filter
and CRLF conversion had been introduced.

Introduce size_change_is_conclusive() in read-cache.c, which calls
would_convert_to_git().
The content of the file in the worktree is now run through all filters
and CRLF conversion whenever the size is different.
After that, the new size (and mtime) is stored in the index
together with the modified status.

Technical correctness vs performance considerations:
Instead of running the chain of
`git status` `git diff` (hm, nothing changed ?) `git add`
we now handle everything in `git status`, which may give a slower
execution time for this command.
Different measurements came up with 20ms per file which are now
spend in `git status` instead of `git add`.

Signed-off-by: Curtis Allen Smith <curtis.allen.smith@gmail.com>
Signed-off-by: Torsten Bögershausen <tboegi@web.de>
---
 read-cache.c    | 25 ++++++++++++++++++++++++-
 t/t0020-crlf.sh | 45 +++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 69 insertions(+), 1 deletion(-)

diff --git a/read-cache.c b/read-cache.c
index c4cf08a3a3..9aebe4c394 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -9,6 +9,7 @@
 
 #include "git-compat-util.h"
 #include "config.h"
+#include "convert.h"
 #include "date.h"
 #include "diff.h"
 #include "diffcore.h"
@@ -438,6 +439,27 @@ int ie_match_stat(struct index_state *istate,
 	return changed;
 }
 
+/*
+ * A difference between the size of the file in the working tree and the
+ * size recorded for it in the index proves that the contents changed
+ * only as long as the two are byte-for-byte comparable.  That stops
+ * being true as soon as the path is run through a clean filter:
+ * rewriting a file with CRLF endings under "text eol=lf", for example,
+ * changes its size in the working tree without changing the blob Git
+ * would record for it.  For such a path the only way to tell is to read
+ * the contents and convert them, which is what we already do when only
+ * the mtime changed.
+ */
+static int size_change_is_conclusive(struct index_state *istate,
+				     const struct cache_entry *ce,
+				     struct stat *st)
+{
+	if (!S_ISREG(st->st_mode))
+		return 1;
+
+	return !would_convert_to_git(istate, ce->name);
+}
+
 int ie_modified(struct index_state *istate,
 		const struct cache_entry *ce,
 		struct stat *st, unsigned int options)
@@ -480,7 +502,8 @@ int ie_modified(struct index_state *istate,
 	     */
 	    (!S_ISLNK(st->st_mode) || ce->ce_stat_data.sd_size != MAX_PATH) &&
 #endif
-	    (S_ISGITLINK(ce->ce_mode) || ce->ce_stat_data.sd_size != 0))
+	    (S_ISGITLINK(ce->ce_mode) || ce->ce_stat_data.sd_size != 0) &&
+	    size_change_is_conclusive(istate, ce, st))
 		return changed;
 
 	changed_fs = ce_modified_check_fs(istate, ce, st);
diff --git a/t/t0020-crlf.sh b/t/t0020-crlf.sh
index fd1cae09ed..88b728d509 100755
--- a/t/t0020-crlf.sh
+++ b/t/t0020-crlf.sh
@@ -397,4 +397,49 @@ test_expect_success 'New CRLF file gets LF in repo' '
 	test_cmp alllf alllf2
 '
 
+test_expect_success 'status does not report a CRLF-only rewrite as modified' '
+	git init eol-status &&
+	(
+		cd eol-status &&
+		echo "* text eol=lf" >.gitattributes &&
+		printf "one\ntwo\nthree\n" >file.txt &&
+		git add .gitattributes file.txt &&
+		git commit -m initial &&
+
+		# a generator rewrites the file with CRLF, same content
+		printf "one\r\ntwo\r\nthree\r\n" >file.txt &&
+		git status --porcelain -uno >actual &&
+		test_must_be_empty actual &&
+		git diff --exit-code &&
+
+		# a real change is still reported
+		printf "one\r\ntwo\r\nfour\r\n" >file.txt &&
+		git status --porcelain -uno >actual &&
+		echo " M file.txt" >expect &&
+		test_cmp expect actual
+	)
+'
+
+test_expect_success 'status sizes a text file by its CRLF pairs, not its CRs' '
+	git init eol-status-lone-cr &&
+	(
+		cd eol-status-lone-cr &&
+		echo "* text eol=lf" >.gitattributes &&
+		printf "one\rtwo\nthree\n" >file.txt &&
+		git add .gitattributes file.txt &&
+		git commit -m initial &&
+
+		# "git add" keeps the lone CR and drops the others
+		printf "one\rtwo\r\nthree\r\n" >file.txt &&
+		git status --porcelain -uno >actual &&
+		test_must_be_empty actual &&
+
+		# the converted size matches the blob, the content does not
+		printf "one\rtwo\nthrEE\r\n" >file.txt &&
+		git status --porcelain -uno >actual &&
+		echo " M file.txt" >expect &&
+		test_cmp expect actual
+	)
+'
+
 test_done
-- 
2.50.0.rc0.46.g7014b55638