[PATCH v5] read-cache: do not trust a size change when conversion is active

[Curtis Allen Smith <curtis.allen.smith@gmail.com>]

"git status" can report a file as modified while "git diff" and
"git add" agree that 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        # -> nothing to commit

ie_modified() takes a change in the file's size since the index
entry was last refreshed as proof that the file is modified, without
looking at the contents.  A size change proves only that the bytes in
the working tree changed.  When the path is subject to conversion
(the "text" and "eol" attributes, core.autocrlf, a clean filter),
different bytes can convert to the same blob, as the CRLF rewrite
above does.

For such a path, fall through to ce_modified_check_fs(), as is
already done when only the mtime changed.  Paths without conversion
return early as before.

ie_modified() runs when the index is refreshed, so this reaches
every command that refreshes it: "git update-index --refresh" now
succeeds instead of reporting "needs update", "git ls-files -m" and
"git status" stop reporting the path, and "git rebase" and
"git pull --rebase" no longer refuse to run.  "git diff" itself is
unchanged; it already reads and converts the file and prints
nothing.

Helped-by: Torsten Bögershausen <tboegi@web.de>
Signed-off-by: Curtis Allen Smith <curtis.allen.smith@gmail.com>
---
Changes since v4:
 - The two t0020 tests run in the test script's own repository rather
   than creating one; the attribute is scoped to the file under test
   so the existing allcrlf and mixed files are untouched.
 - The log message says which commands the index refresh reaches.

Cost, i9-14900K under WSL2, warm cache, fastest of 5 runs of
"git status -uno" (the first-run rows, of 3), stock 6de20f6 against
this patch:

	10000 files x 2.6 KB, "* text=auto eol=lf" stock      v5
	  clean tree                                5 ms     5 ms
	  10 files edited                           5 ms     5 ms
	  100 files edited                          5 ms     7 ms
	  1000 files edited                         6 ms    20 ms
	  10000 files edited                       23 ms   138 ms
	  10000 same-length edits                 138 ms   133 ms
	  10000 CRLF-rewritten, 1st run            22 ms   114 ms
	    the same, every run after              21 ms     5 ms

	200 files x 1 MB, same attributes
	  clean tree                                3 ms     2 ms
	  200 files edited                          3 ms   514 ms
	  200 same-length edits                   523 ms   513 ms
	  200 CRLF-rewritten, 1st run               3 ms   436 ms
	    the same, every run after               3 ms     2 ms

	10000 files x 2.6 KB, no conversion
	  clean tree                                5 ms     5 ms
	  10000 files with a size change           21 ms    22 ms

The attribute lookup is the last test in the condition, so it happens
only for a path whose size changed: a clean tree pays nothing, and a
tree without conversion pays one lookup per size-changed path (the
last block).  What the patch adds is proportional to the bytes in the
files actually changed, roughly 12 microseconds per modified 2.6 KB
file, so it takes on the order of a thousand modified converted paths
before it is measurable at all.  The same-length rows go down the
pre-existing mtime path and are unaffected by this patch.  Stock git
keeps reporting the CRLF-rewritten files as modified on every run.

A follow-up that skips the conversion and the hash when the length
already rules out a match is written and tested, but on these numbers
I do not think it earns its size.  It is there if anyone wants it.
 read-cache.c    |  4 +++-
 t/t0020-crlf.sh | 39 +++++++++++++++++++++++++++++++++++++++
 2 files changed, 42 insertions(+), 1 deletion(-)

diff --git a/read-cache.c b/read-cache.c
index c4cf08a3a..70f4787de 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"
@@ -480,7 +481,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) &&
+	    !(S_ISREG(st->st_mode) && would_convert_to_git(istate, ce->name)))
 		return changed;
 
 	changed_fs = ce_modified_check_fs(istate, ce, st);
diff --git a/t/t0020-crlf.sh b/t/t0020-crlf.sh
index fd1cae09e..9c8d07cfc 100755
--- a/t/t0020-crlf.sh
+++ b/t/t0020-crlf.sh
@@ -397,4 +397,43 @@ 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' '
+	test_when_finished "git reset --hard autocrlf-checkpoint" &&
+	echo "eol.txt text eol=lf" >.gitattributes &&
+	printf "one\ntwo\nthree\n" >eol.txt &&
+	git add .gitattributes eol.txt &&
+	git commit -m "eol.txt" &&
+
+	# a generator rewrites the file with CRLF, same content
+	printf "one\r\ntwo\r\nthree\r\n" >eol.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" >eol.txt &&
+	git status --porcelain -uno >actual &&
+	echo " M eol.txt" >expect &&
+	test_cmp expect actual
+'
+
+test_expect_success 'status sees through a CRLF rewrite of a file with a lone CR' '
+	test_when_finished "git reset --hard autocrlf-checkpoint" &&
+	echo "cr.txt text eol=lf" >.gitattributes &&
+	printf "one\rtwo\nthree\n" >cr.txt &&
+	git add .gitattributes cr.txt &&
+	git commit -m "cr.txt" &&
+
+	# "git add" keeps the lone CR and drops the others
+	printf "one\rtwo\r\nthree\r\n" >cr.txt &&
+	git status --porcelain -uno >actual &&
+	test_must_be_empty actual &&
+
+	# the converted length matches the blob, the content does not
+	printf "one\rtwo\nthrEE\r\n" >cr.txt &&
+	git status --porcelain -uno >actual &&
+	echo " M cr.txt" >expect &&
+	test_cmp expect actual
+'
+
 test_done
-- 
2.53.0