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

[Phillip Wood <phillip.wood123@gmail.com>]

Hi Curtis

On 13/10/2026 07:58, Curtis Allen Smith wrote:
> "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.

The commit message talks about "git status" but presumably this change 
affects the output of "git diff-files" and "git diff-index" as well. 
Does this change improve the performance of "git diff" because it 
no-longer tries to diff two blobs that are identical after conversion or 
is the existing implementation smart enough to avoid that already?

> A follow-up that skips the conversion and the hash when the length
> already rules out a match is ready if wanted.  It takes the
> "genuinely modified" rows to 57 ms and 25 ms.

What does that entail? A file that consists of blank lines can double or 
halve its length going from '\n' to '\r\n' and vice-versa. Is that range 
narrow enough to avoid converting a significant number of "real" changes?

Looking at t0020-crlf.sh none of the existing tests use a separate 
repository, so maybe we could avoid that overhead here as well.

Thanks

Phillip

>   read-cache.c    |  4 +++-
>   t/t0020-crlf.sh | 45 +++++++++++++++++++++++++++++++++++++++++++++
>   2 files changed, 48 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..b76c678ce 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 sees through a CRLF rewrite of a file with a lone CR' '
> +	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 length 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