[PATCH v2 1/2] utf8.c: Prepare workaround for iconv under macOS 14/15

[PATCH v2 1/2] utf8.c: Prepare workaround for iconv under macOS 14/15

[tboegi]

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

MacOS14 (Sonoma) has started to ship an iconv library with bugs.
The same bugs exists even in MacOS 15 (Sequoia)

A bug report running the Git test suite says:

three tests of t3900 fail on macOS 26.1 for me:

  not ok 17 - ISO-2022-JP should be shown in UTF-8 now
  not ok 25 - ISO-2022-JP should be shown in UTF-8 now
  not ok 38 - commit --fixup into ISO-2022-JP from UTF-8

Here's the verbose output of the first one:

----- snip! -----
expecting success of 3900.17 'ISO-2022-JP should be shown in UTF-8 now':
                compare_with ISO-2022-JP "$TEST_DIRECTORY"/t3900/2-UTF-8.txt

--- /Users/x/src/git/t/t3900/2-UTF-8.txt 2024-10-01 19:43:24.605230684 +0000
+++ current     2025-12-08 21:52:45.786161909 +0000
@@ -1,4 +1,4 @@
 はれひほふ

 しているのが、いるので。
-濱浜ほれぷりぽれまびぐりろへ。
+濱浜ほれぷりぽれまび$0$j$m$X!#
not ok 17 - ISO-2022-JP should be shown in UTF-8 now
1..17
----- snap! -----

compare_with runs git show to display a commit message, which in this
case here was encoded using ISO-2022-JP and is supposed to be reencoded
to UTF-8, but git show only does that half-way -- the "$0$j$m$X!#" part
is from the original ISO-2022-JP representation.

That botched conversion is done by utf8.c::reencode_string_iconv().  It
calls iconv(3) to do the actual work, initially with an output buffer of
the same size as the input.  If the output needs more space the function
enlarges the buffer and calls iconv(3) again.

iconv(3) won't tell us how much space it needs, but it will report what
part it already managed to convert, so we can increase the buffer and
continue from there.  ISO-2022-JP has escape codes for switching between
character sets, so it's a stateful encoding.  I guess the iconv(3) on my
machine forgets the state at the end of part one and then messes up part
two.

[end of citation]

Working around the buggy iconv shipped with the OS can be done in
two  ways:
a) Link Git against a different version of iconv
b) Improve the handling when iconv needs a larger output buffer

a) is already done by default when either Fink [1] or MacPorts [2]
   or Homebrew [3] is installed.
b) is implemented here, in case that no fixed iconv is available:
   When the output buffer is too short, increase it (as before)
   and start from scratch (this is new).

This workound needs to be enabled with
'#define ICONV_RESTART_RESET'
and a makefile knob will be added in the next commit

Suggested-by: René Scharfe <l.s.r@web.de>
Signed-off-by: Torsten Bögershausen <tboegi@web.de>

[1] https://www.finkproject.org/
[2] https://www.macports.org/
[3] https://brew.sh/

Signed-off-by: Torsten Bögershausen <tboegi@web.de>
---
 utf8.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/utf8.c b/utf8.c
index 35a0251939..96460cc414 100644
--- a/utf8.c
+++ b/utf8.c
@@ -515,6 +515,19 @@ char *reencode_string_iconv(const char *in, size_t insz, iconv_t conv,
 			out = xrealloc(out, outalloc);
 			outpos = out + sofar;
 			outsz = outalloc - sofar - 1;
+#ifdef ICONV_RESTART_RESET
+			/*
+			 * If iconv(3) messes up piecemeal conversions
+			 * then restore the original pointers, sizes,
+			 * and converter state, then retry converting
+			 * the full string using the reallocated buffer.
+			 */
+			insz += cp - (iconv_ibp)in; /* Restore insz */
+			cp = (iconv_ibp)in;         /* original start value */
+			outpos = out + bom_len;     /* original start value */
+			outsz = outalloc - bom_len - 1; /* new len */
+			iconv(conv, NULL, NULL, NULL, NULL); /* reset iconv machinery */
+#endif
 		}
 		else {
 			*outpos = '\0';
-- 
2.50.0.rc0.46.g7014b55638.dirty

Re: [PATCH v2 1/2] utf8.c: Prepare workaround for iconv under macOS 14/15

[Junio C Hamano]

tboegi@web.de writes:

> From: Torsten Bögershausen <tboegi@web.de>
>
> MacOS14 (Sonoma) has started to ship an iconv library with bugs.
> The same bugs exists even in MacOS 15 (Sequoia)
>
> A bug report running the Git test suite says:
>
> three tests of t3900 fail on macOS 26.1 for me:
>
>   not ok 17 - ISO-2022-JP should be shown in UTF-8 now
>   not ok 25 - ISO-2022-JP should be shown in UTF-8 now
>   not ok 38 - commit --fixup into ISO-2022-JP from UTF-8
>
> Here's the verbose output of the first one:
>
> ----- snip! -----

Doesn't this tell "git am" that your log message ends here, and ...

> expecting success of 3900.17 'ISO-2022-JP should be shown in UTF-8 now':
>                 compare_with ISO-2022-JP "$TEST_DIRECTORY"/t3900/2-UTF-8.txt
>
> --- /Users/x/src/git/t/t3900/2-UTF-8.txt 2024-10-01 19:43:24.605230684 +0000
> +++ current     2025-12-08 21:52:45.786161909 +0000

... makes the tool to apply the patch to file "current"?

> @@ -1,4 +1,4 @@
>  はれひほふ
>
>  しているのが、いるので。
> -濱浜ほれぷりぽれまびぐりろへ。
> +濱浜ほれぷりぽれまび$0$j$m$X!#
> not ok 17 - ISO-2022-JP should be shown in UTF-8 now
> 1..17
> ----- snap! -----

IOW, indent the displayed material used as an example in the
proposed log message.

> compare_with runs git show to display a commit message, which in this
> case here was encoded using ISO-2022-JP and is supposed to be reencoded
> to UTF-8, but git show only does that half-way -- the "$0$j$m$X!#" part
> is from the original ISO-2022-JP representation.
>
> That botched conversion is done by utf8.c::reencode_string_iconv().  It
> calls iconv(3) to do the actual work, initially with an output buffer of
> the same size as the input.  If the output needs more space the function
> enlarges the buffer and calls iconv(3) again.
>
> iconv(3) won't tell us how much space it needs, but it will report what
> part it already managed to convert, so we can increase the buffer and
> continue from there.  ISO-2022-JP has escape codes for switching between
> character sets, so it's a stateful encoding.  I guess the iconv(3) on my
> machine forgets the state at the end of part one and then messes up part
> two.
>
> [end of citation]
>
> Working around the buggy iconv shipped with the OS can be done in
> two  ways:
> a) Link Git against a different version of iconv
> b) Improve the handling when iconv needs a larger output buffer
>
> a) is already done by default when either Fink [1] or MacPorts [2]
>    or Homebrew [3] is installed.
> b) is implemented here, in case that no fixed iconv is available:
>    When the output buffer is too short, increase it (as before)
>    and start from scratch (this is new).
>
> This workound needs to be enabled with
> '#define ICONV_RESTART_RESET'
> and a makefile knob will be added in the next commit
>
> Suggested-by: René Scharfe <l.s.r@web.de>
> Signed-off-by: Torsten Bögershausen <tboegi@web.de>
>
> [1] https://www.finkproject.org/
> [2] https://www.macports.org/
> [3] https://brew.sh/
>
> Signed-off-by: Torsten Bögershausen <tboegi@web.de>
> ---
>  utf8.c | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
>
> diff --git a/utf8.c b/utf8.c
> index 35a0251939..96460cc414 100644
> --- a/utf8.c
> +++ b/utf8.c
> @@ -515,6 +515,19 @@ char *reencode_string_iconv(const char *in, size_t insz, iconv_t conv,
>  			out = xrealloc(out, outalloc);
>  			outpos = out + sofar;
>  			outsz = outalloc - sofar - 1;
> +#ifdef ICONV_RESTART_RESET
> +			/*
> +			 * If iconv(3) messes up piecemeal conversions
> +			 * then restore the original pointers, sizes,
> +			 * and converter state, then retry converting
> +			 * the full string using the reallocated buffer.
> +			 */
> +			insz += cp - (iconv_ibp)in; /* Restore insz */
> +			cp = (iconv_ibp)in;         /* original start value */
> +			outpos = out + bom_len;     /* original start value */
> +			outsz = outalloc - bom_len - 1; /* new len */
> +			iconv(conv, NULL, NULL, NULL, NULL); /* reset iconv machinery */
> +#endif
>  		}
>  		else {
>  			*outpos = '\0';

[PATCH v3 0/2] Workaround for iconv under macOS 14/15

[tboegi]

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

Changes since v1:
  Re-fined commit message (thanks René)
  Installed a fresh Fink. It lives under /opt/sw now (/sw in older versions)
  However, people can choose other places, I did use the default.
  Make more clear that libiconv from Homebrew, Macports or Fink can be used.
Changes since v2:
  Even more details in commit message, and now
  git am works (Thanks Junio for the patience)


Torsten Bögershausen (2):
  utf8.c: Prepare workaround for iconv under macOS 14/15
  utf8.c: Enable workaround for iconv under macOS 14/15

 Makefile         | 16 ++++++++++++++++
 config.mak.uname |  1 +
 utf8.c           | 13 +++++++++++++
 3 files changed, 30 insertions(+)

-- 
2.50.0.rc0.46.g7014b55638.dirty

[PATCH v3 1/2] utf8.c: Prepare workaround for iconv under macOS 14/15

[tboegi]

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

MacOS14 (Sonoma) has started to ship an iconv library with bugs.
The same bugs exists even in MacOS 15 (Sequoia)

A bug report running the Git test suite says:

three tests of t3900 fail on macOS 26.1 for me:

  not ok 17 - ISO-2022-JP should be shown in UTF-8 now
  not ok 25 - ISO-2022-JP should be shown in UTF-8 now
  not ok 38 - commit --fixup into ISO-2022-JP from UTF-8

Here's the verbose output of the first one:

=================
expecting success of 3900.17 'ISO-2022-JP should be shown in UTF-8 now':
                compare_with ISO-2022-JP "$TEST_DIRECTORY"/t3900/2-UTF-8.txt

 --- /Users/x/src/git/t/t3900/2-UTF-8.txt 2024-10-01 19:43:24.605230684 +0000
 +++ current     2025-12-08 21:52:45.786161909 +0000
@@ -1,5 +1,5 @@
 はれひほふ

 しているのが、いるので。
 -濱浜ほれぷりぽれまびぐりろへ。
 +濱浜ほれぷりぽれまび$0$j$m$X!#
not ok 17 - ISO-2022-JP should be shown in UTF-8 now
1..17
=================

compare_with runs git show to display a commit message, which in this
case here was encoded using ISO-2022-JP and is supposed to be reencoded
to UTF-8, but git show only does that half-way -- the "$0$j$m$X!#" part
is from the original ISO-2022-JP representation.

That botched conversion is done by utf8.c::reencode_string_iconv().  It
calls iconv(3) to do the actual work, initially with an output buffer of
the same size as the input.  If the output needs more space the function
enlarges the buffer and calls iconv(3) again.

iconv(3) won't tell us how much space it needs, but it will report what
part it already managed to convert, so we can increase the buffer and
continue from there.  ISO-2022-JP has escape codes for switching between
character sets, so it's a stateful encoding.  I guess the iconv(3) on my
machine forgets the state at the end of part one and then messes up part
two.

[end of citation]

Working around the buggy iconv shipped with the OS can be done in
two  ways:
a) Link Git against a different version of iconv
b) Improve the handling when iconv needs a larger output buffer

a) is already done by default when either Fink [1] or MacPorts [2]
   or Homebrew [3] is installed.
b) is implemented here, in case that no fixed iconv is available:
   When the output buffer is too short, increase it (as before)
   and start from scratch (this is new).

This workound needs to be enabled with
'#define ICONV_RESTART_RESET'
and a makefile knob will be added in the next commit

Suggested-by: René Scharfe <l.s.r@web.de>
Signed-off-by: Torsten Bögershausen <tboegi@web.de>

[1] https://www.finkproject.org/
[2] https://www.macports.org/
[3] https://brew.sh/

Signed-off-by: Torsten Bögershausen <tboegi@web.de>
---
 utf8.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/utf8.c b/utf8.c
index 35a0251939..96460cc414 100644
--- a/utf8.c
+++ b/utf8.c
@@ -515,6 +515,19 @@ char *reencode_string_iconv(const char *in, size_t insz, iconv_t conv,
 			out = xrealloc(out, outalloc);
 			outpos = out + sofar;
 			outsz = outalloc - sofar - 1;
+#ifdef ICONV_RESTART_RESET
+			/*
+			 * If iconv(3) messes up piecemeal conversions
+			 * then restore the original pointers, sizes,
+			 * and converter state, then retry converting
+			 * the full string using the reallocated buffer.
+			 */
+			insz += cp - (iconv_ibp)in; /* Restore insz */
+			cp = (iconv_ibp)in;         /* original start value */
+			outpos = out + bom_len;     /* original start value */
+			outsz = outalloc - bom_len - 1; /* new len */
+			iconv(conv, NULL, NULL, NULL, NULL); /* reset iconv machinery */
+#endif
 		}
 		else {
 			*outpos = '\0';
-- 
2.50.0.rc0.46.g7014b55638.dirty

[PATCH v3 2/2] utf8.c: Enable workaround for iconv under macOS 14/15

[tboegi]

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

The previous commit introduced a workaround in utf8.c to deal
with broken iconv implementations.

It is enabled when a MacOS version is used that has a buggy
iconv library and there is no external library provided
(and linked against) from neither MacPorts nor Homebrew nor Fink.
For Homebrew, MacPorts and Fink we check if libiconv exist.
Introduce 2 new macros: HAS_GOOD_LIBICONV and NEEDS_GOOD_LIBICONV.

For Homebrew HAS_GOOD_LIBICONV is set when the libiconv directory
exist.
MacPorts can be installed with or without libiconv, so check if
libiconv.dylib exists (which is a softlink)

Fink compiles and installs libiconv by default.
Note that a fresh installation of Fink now defaults to /opt/sw.
Older versions used /sw as default, so leave the check and setting
of BASIC_CFLAGS and BASIC_LDFLAGS as is.
For the new default check for the existance of /opt/sw as well.
Add a check for /opt/sw/lib/libiconv.dylib which sets HAS_GOOD_LIBICONV

Signed-off-by: Torsten Bögershausen <tboegi@web.de>
---
 Makefile         | 16 ++++++++++++++++
 config.mak.uname |  1 +
 2 files changed, 17 insertions(+)

diff --git a/Makefile b/Makefile
index b7eba509c6..8aa489f3b6 100644
--- a/Makefile
+++ b/Makefile
@@ -1687,11 +1687,21 @@ ifeq ($(uname_S),Darwin)
 			BASIC_CFLAGS += -I/sw/include
 			BASIC_LDFLAGS += -L/sw/lib
                 endif
+                ifeq ($(shell test -d /opt/sw/lib && echo y),y)
+			BASIC_CFLAGS += -I/opt/sw/include
+			BASIC_LDFLAGS += -L/opt/sw/lib
+			ifeq ($(shell test -e /opt/sw/lib/libiconv.dylib && echo y),y)
+				HAS_GOOD_LIBICONV = Yes
+			endif
+                endif
         endif
         ifndef NO_DARWIN_PORTS
                 ifeq ($(shell test -d /opt/local/lib && echo y),y)
 			BASIC_CFLAGS += -I/opt/local/include
 			BASIC_LDFLAGS += -L/opt/local/lib
+			ifeq ($(shell test -e /opt/local/lib/libiconv.dylib && echo y),y)
+				HAS_GOOD_LIBICONV = Yes
+			endif
                 endif
         endif
         ifndef NO_APPLE_COMMON_CRYPTO
@@ -1714,6 +1724,7 @@ endif
 ifdef USE_HOMEBREW_LIBICONV
 ifeq ($(shell test -d $(HOMEBREW_PREFIX)/opt/libiconv && echo y),y)
 	ICONVDIR ?= $(HOMEBREW_PREFIX)/opt/libiconv
+	HAS_GOOD_LIBICONV = Yes
 endif
 endif
 endif
@@ -1859,6 +1870,11 @@ ifndef NO_ICONV
                 endif
 		EXTLIBS += $(ICONV_LINK) -liconv
         endif
+        ifdef NEEDS_GOOD_LIBICONV
+        ifndef HAS_GOOD_LIBICONV
+                BASIC_CFLAGS += -DICONV_RESTART_RESET
+        endif
+        endif
 endif
 ifdef ICONV_OMITS_BOM
 	BASIC_CFLAGS += -DICONV_OMITS_BOM
diff --git a/config.mak.uname b/config.mak.uname
index 38b35af366..3c35ae33a3 100644
--- a/config.mak.uname
+++ b/config.mak.uname
@@ -157,6 +157,7 @@ ifeq ($(uname_S),Darwin)
         endif
         ifeq ($(shell test "$(DARWIN_MAJOR_VERSION)" -ge 24 && echo 1),1)
 		USE_HOMEBREW_LIBICONV = UnfortunatelyYes
+		NEEDS_GOOD_LIBICONV = UnfortunatelyYes
         endif
 
 	# The builtin FSMonitor on MacOS builds upon Simple-IPC.  Both require
-- 
2.50.0.rc0.46.g7014b55638.dirty

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

[tboegi]

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

- Pick up the patch from Curtis Allen Smith
- Reduce the commit message and especially the code changes
- All tests pass here.
- The commit message feels still somewhat bumpy - better suggestions welcome.
- The original series add a configuration switch - that is removed
- I took ownwership of this patch and add a Signed-Off Curtis Allen Smith,
  is this OK ?
- All in all this is not the final version, trying to move things forward

Torsten Bögershausen (1):
  read-cache: do not trust a size change when conversion is active

 read-cache.c    | 25 ++++++++++++++++++++++++-
 t/t0020-crlf.sh | 45 +++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 69 insertions(+), 1 deletion(-)

-- 
2.50.0.rc0.46.g7014b55638

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

[tboegi]

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

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

[Junio C Hamano]

tboegi@web.de writes:

> 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:

"in real life"?

>   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.

That is not just size but contents are changed, no?

>   Editors that insist to replace 4 space with a TAB,
>     even if the file is changed manually forth and back -

"back and forth"?

>     Using a clean filter that changes the TAB back into space when
>     committing will cause the same confusion.

This also changes size as a result of changing contents.

> `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.

"when size differs, without looking at the contents"?

> Signed-off-by: Curtis Allen Smith <curtis.allen.smith@gmail.com>
> Signed-off-by: Torsten Bögershausen <tboegi@web.de>

How is the collaboration going here, Have you taken over the series,
or are you two working together on the topic (or I take that your
intention to CC Curtis is to work together starting with this
version)?

I am wondering if we want co-authored-by or similar when we lose the
RFC status.

> ---
>  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

"compatable" -> "identical"?

> + * 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);
> +}

Wooo, that is surprisingly simple.  Even attribute look-up is not
free.  How much overhead are we adding to the case where no clean
filter is involved, I wonder?

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

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> tboegi@web.de writes:
>
>> From: Torsten Bögershausen <tboegi@web.de>
>> ...
>> Signed-off-by: Curtis Allen Smith <curtis.allen.smith@gmail.com>
>> Signed-off-by: Torsten Bögershausen <tboegi@web.de>
>
> How is the collaboration going here, Have you taken over the series,
> or are you two working together on the topic (or I take that your
> intention to CC Curtis is to work together starting with this
> version)?
>
> I am wondering if we want co-authored-by or similar when we lose the
> RFC status.

If you two are collaborating outside the mailing list, that would be
fine and I would have no reason to complain.  But such collaboration
is invisible to others, of course, so I would like to double-check
that you are not unilaterally taking over the topic.  Can you
confirm that you are working well together, or say something
similar, before we proceed to other technical points?

This is a tangent, but it is the reason why my mailer settings
drop into the trash all incoming Git-related messages that are
addressed only to me without CC'ing the list.  This forces me to
resist the temptation to collude with selected others behind
closed doors.

Thanks.

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

[Curtis Smith]

Junio C Hamano <gitster@pobox.com> writes:

> If you two are collaborating outside the mailing list, that would be
> fine and I would have no reason to complain.  But such collaboration
> is invisible to others, of course, so I would like to double-check
> that you are not unilaterally taking over the topic.

We are not collaborating off the list, and Torsten did not ask me
before sending v3.  v3 is my v1 patch 1 with the optimization taken
out, but it keeps the size_change_is_conclusive() helper.  In my
reply to you on Saturday I proposed the fix inlined into the
condition in ie_modified() instead, with no new function.

I would like to keep the topic.  v4 follows in this thread: that
inlined fix, my tests, a short log message, and Torsten credited with
Helped-by.  Thanks, Torsten, for the review.

From your earlier message:

> Wooo, that is surprisingly simple.  Even attribute look-up is not
> free.  How much overhead are we adding to the case where no clean
> filter is involved, I wonder?

The lookup is the last test in the "changed & DATA_CHANGED"
condition, so it runs only for a path whose size changed.  A clean
tree never reaches it.  With no conversion configured, "git status"
on 10000 files with a size change takes 26ms on stock git and 24ms
with the fix.  The full numbers are below the "---" in v4.

On Mon, Oct 12, 2026 at 7:29 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> Junio C Hamano <gitster@pobox.com> writes:
>
> > tboegi@web.de writes:
> >
> >> From: Torsten Bögershausen <tboegi@web.de>
> >> ...
> >> Signed-off-by: Curtis Allen Smith <curtis.allen.smith@gmail.com>
> >> Signed-off-by: Torsten Bögershausen <tboegi@web.de>
> >
> > How is the collaboration going here, Have you taken over the series,
> > or are you two working together on the topic (or I take that your
> > intention to CC Curtis is to work together starting with this
> > version)?
> >
> > I am wondering if we want co-authored-by or similar when we lose the
> > RFC status.
>
> If you two are collaborating outside the mailing list, that would be
> fine and I would have no reason to complain.  But such collaboration
> is invisible to others, of course, so I would like to double-check
> that you are not unilaterally taking over the topic.  Can you
> confirm that you are working well together, or say something
> similar, before we proceed to other technical points?
>
> This is a tangent, but it is the reason why my mailer settings
> drop into the trash all incoming Git-related messages that are
> addressed only to me without CC'ing the list.  This forces me to
> resist the temptation to collude with selected others behind
> closed doors.
>
> Thanks.

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

[Johannes Sixt]

Am 12.10.26 um 19:56 schrieb tboegi@web.de:
> @@ -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:

I cannot agree with this reasoning. The index is a cache of the stat
information. The size recorded in the index is only used as a clue
whether the file was changed or not. It does *not* record the size of
the data that the object database holds for the blob.

If your analysis found that, after the content was `git add`-ed, the
size in the index is *not* identical to the size taken from the
filesystem, then the problem is that an incorrect size was put into the
index, and not that the size between the blob data in the database and
the filesystem's file are different (and unusable).

> + * 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.
> + */

-- Hannes

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

[Curtis Smith]

Johannes Sixt <j6t@kdbg.org> writes:

> I cannot agree with this reasoning. The index is a cache of the stat
> information. The size recorded in the index is only used as a clue
> whether the file was changed or not. It does *not* record the size of
> the data that the object database holds for the blob.

Agreed, and that comment is worded badly.  The size in the index is
correct here: it is the size of the LF file as it was checked out or
added.  The tool then rewrites the file with CRLF endings, so the
size on disk really does change.

The problem is what ie_modified() concludes from that.  A size change
proves that the bytes in the working tree changed.  Without
conversion, that means the content Git would record changed too.
With conversion it does not: the CRLF file converts to the same
blob, which is why "git diff" shows nothing.

v4 has no such comment, and its log message says it this way.

On Mon, Oct 12, 2026 at 11:34 PM Johannes Sixt <j6t@kdbg.org> wrote:
>
> Am 12.10.26 um 19:56 schrieb tboegi@web.de:
> > @@ -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:
>
> I cannot agree with this reasoning. The index is a cache of the stat
> information. The size recorded in the index is only used as a clue
> whether the file was changed or not. It does *not* record the size of
> the data that the object database holds for the blob.
>
> If your analysis found that, after the content was `git add`-ed, the
> size in the index is *not* identical to the size taken from the
> filesystem, then the problem is that an incorrect size was put into the
> index, and not that the size between the blob data in the database and
> the filesystem's file are different (and unusable).
>
> > + * 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.
> > + */
>
> -- Hannes
>

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

[Curtis Allen Smith]

"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.

Helped-by: Torsten Bögershausen <tboegi@web.de>
Signed-off-by: Curtis Allen Smith <curtis.allen.smith@gmail.com>
---
Changes since v3:
 - The fix is inlined into the condition in ie_modified(), with no
   helper function.
 - Shorter log message, using Junio's wording; the "real life"
   examples are gone.

Cost, i9-14900K under WSL2, warm cache, fastest of 5 runs of
"git status -uno", stock 6de20f6 against this patch:

	10000 files x 2.6 KB, no conversion       stock      v4
	  clean tree                               5 ms     5 ms
	  10000 files with a size change          26 ms    24 ms

	10000 files x 2.6 KB, "* text=auto eol=lf"
	  clean tree                               5 ms     5 ms
	  10000 genuinely modified                25 ms   137 ms
	  10000 CRLF-rewritten, 1st run           21 ms   143 ms
	  the same, every run after               21 ms     5 ms

	200 files x 1 MB, "* text=auto eol=lf"
	  clean tree                               3 ms     3 ms
	  200 genuinely modified                   3 ms   498 ms
	  200 CRLF-rewritten, 1st run              3 ms   584 ms
	  the same, every run after                3 ms     3 ms

The attribute lookup happens only for a path whose size changed, so a
clean tree pays nothing, and a tree without conversion pays one
lookup per size-changed path (the first block).  A converted path
whose size changed is now read, converted and hashed, which is what
already happens when only its mtime changes.  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 ready if wanted.  It takes the
"genuinely modified" rows to 57 ms and 25 ms.

 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
-- 
2.53.0

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

[Phillip Wood]

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

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

[Curtis Smith]

Hi Phillip,

Thanks for looking at this.

On 13/10/2026, Phillip Wood wrote:
> The commit message talks about "git status" but presumably this change
> affects the output of "git diff-files" and "git diff-index" as well.

You are right that the message is too narrow, though the mechanism is
not quite that.  ie_modified() is reached from refresh_cache_ent(),
and from ls-files, mv and submodule.c; that is all four callers.
run_diff_files() and run_diff_index() go through
match_stat_with_submodule(), which calls ie_match_stat() and never
ie_modified().  So diff-files and diff-index are not changed directly.

What changes is the index refresh.  Stock git can never clear the
stat-dirty mark on such a path: refresh_cache_ent() calls
ie_modified(), ie_modified() returns early on the size change, and the
entry is left alone, so the next refresh repeats the whole thing.
With the patch the refresh resolves the path and writes the new stat
data, and everything downstream of a refreshed index settles.  On the
example in the commit message, stock 6de20f6 against the patch:

                                      stock              patched
  git update-index --refresh          exit 1,            exit 0,
                                      "file.txt:         no output
                                       needs update"
  git ls-files -m (after a refresh)   file.txt           no output
  git status --porcelain -uno          M file.txt        no output
  git diff-files --name-only          file.txt           file.txt
  git diff-files --name-only,
    after anything that refreshes
    and writes the index              file.txt           no output
  git diff-index --name-only HEAD,
    likewise                          file.txt           no output

"git diff" is one of the commands that refreshes, so in practice a
second "git diff-files" in the same tree does change.

So the plumbing answer is: diff-files run against an unrefreshed index
is unchanged, and that is correct, since its contract is to report
what differs by stat.  But stock git leaves these paths permanently
stat-dirty, so the report never settles no matter how many times you
refresh.  The patch is what lets it settle.  "git update-index
--refresh", which is the usual way a script asks "is the working tree
clean", is the plainest case: today it fails on a file that git itself
agrees is unmodified.

The visible cost of that is in the commands that refresh the index
before they will run.  require_clean_work_tree() calls
refresh_index(), so with one CRLF-rewritten file in the tree, stock
git refuses:

    $ git rebase HEAD~1
    error: cannot rebase: You have unstaged changes.
    error: Please commit or stash them.

and then will not let you take its advice:

    $ git add file.txt && git commit -m x
    On branch master
    nothing to commit, working tree clean

The "git add" repairs the stat data as a side effect, so a second
"git rebase" does work; there is just no way for the user to know
that.  With the patch the first one runs.  "git pull --rebase"
without --autostash is the same, since it calls
require_clean_work_tree() as well.  A merging "git pull" does not,
and is unaffected.

> 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?

Not for a single "git diff": diff-files flags the path on the stat
difference, the diff machinery reads the file, converts it, finds the
result identical to the blob and emits nothing.  That read and
conversion is the only way to know, and it happens in stock git today.
The patch neither adds nor removes it.

The saving is on the runs after the first.  Once a refresh has
recorded the stat data, the path matches on stat and is never opened
again.  Stock git reads and converts it on every single run, forever.
That is the "steady state" row in the numbers below the --- in v4:
21 ms against 5 ms for 10000 files, and it is the same effect.

I will say this in the commit message.  Replacing the last paragraph:

  -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.
  +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.

Happy to make that longer or shorter.

> > A follow-up that skips the conversion and the hash when the length
> > already rules out a match is ready if wanted.
>
> 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?

It is not a range test, so the blank-line file is fine.  When the
end-of-line conversion is the only one that applies (no clean filter,
no "ident", no working-tree-encoding), "git add" either leaves the
file alone or turns every CRLF into LF.  The converted length is
therefore exactly one of two values: the raw length, or the raw length
minus the number of CRLF pairs.  A memchr scan for CR gets that count.
If neither value is the size of the blob, the file is modified, and it
is neither converted nor hashed.  "text=auto" strips every CR rather
than CRLF pairs only, but it refuses to convert a file containing a
lone CR, so those two values still cover it.

Your example comes out right.  N blank lines is N bytes with LF and 2N
with CRLF.  Rewritten to CRLF the raw length is 2N and the CRLF count
is N, so the two candidates are 2N and N.  The blob is N, that
matches, and the file goes on to be converted and hashed as before.
The length test never reports such a file as modified; it only
declines to short-circuit.

Your underlying question still stands, though, so let me answer it
with the losing cases in the table and not just the winning one.
Measured on an i9-14900K under WSL2, warm cache, fastest of 5 runs of
"git status -uno":

  10000 files x 2.6 KB, "* text=auto eol=lf" stock     v4  v4+opt
    length-changing edit                     20 ms 136 ms   57 ms
    same-length edit                        133 ms 135 ms  146 ms
    CRLF-rewritten, 1st run                  20 ms 139 ms  160 ms

  200 files x 1 MB, same attributes
    length-changing edit                      2 ms 506 ms   24 ms
    same-length edit                        509 ms 511 ms  516 ms
    CRLF-rewritten, 1st run                   3 ms 605 ms  623 ms

The length-changing rows are the ones the scan decides: an edit that
adds or removes a character cannot produce the blob's length, so the
file is neither converted nor hashed.

Two cases it does not help, and I would rather say so than let one
row speak for the patch.  The CRLF rewrite this series is about has,
by definition, a converted length equal to the blob's size, so the
scan there is pure overhead: 139 ms against 160 ms.  And a
same-length edit does not change the size at all, so it goes down the
mtime path that stock git already walks; the scan adds about 8
percent to a path that exists today, 133 ms against 146 ms.  Those
rows are also the clearest evidence that the fix by itself adds
nothing to that path: 133 ms against 135 ms.

Which brings me to whether the follow-up is wanted at all.  Those
corpora are deliberately extreme; they exist to make the effect
measurable.  On a tree with a realistic number of modified files the
fix does not show up:

  10000 files x 2.6 KB, "* text=auto eol=lf" stock     v4  v4+opt
    clean tree                                5 ms    4 ms     5 ms
    1 file edited                             5 ms    5 ms     5 ms
    10 files edited                           4 ms    4 ms     4 ms
    100 files edited                          5 ms    8 ms     5 ms
    1000 files edited                         6 ms   20 ms    10 ms
    10000 files edited                       20 ms  136 ms    57 ms

  the same tree, n files CRLF-rewritten, 1st run
    1 file                                    6 ms    8 ms     7 ms
    10 files                                  8 ms    8 ms     8 ms
    100 files                                 8 ms    9 ms     9 ms
    1000 files                                9 ms   18 ms    13 ms

The cost is proportional to the bytes in the files you actually
changed: roughly 12 microseconds per modified 2.6 KB file, and about
2.5 ms per modified 1 MB file.  It takes on the order of a thousand
modified converted paths before it is worth measuring at all.

So I would rather leave the follow-up out for now.  Nothing in the
fix depends on it; it is correct on its own, and this only changes
what it costs.  The fix is one condition and an #include, and it is
free in every case anyone is likely to hit; 124 more lines to speed
up trees far larger than the one in the bug report is a poor trade
unless someone is actually hitting it.  If the fix's cost does bother
people, say so and I will send it.

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

Agreed, and both tests now run in t0020's own repository.  One wrinkle
worth noting: "* text eol=lf" makes the existing allcrlf and mixed
files modified, so the attribute is scoped to the file under test.
Both tests pass with the patch and fail on stock 6de20f6, as before.

For what it is worth, the overhead was not measurable: t0020 takes
0.41s either way.  The change is for consistency with the rest of the
file.

That will be in v5.

Thanks,
Curtis

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

[Torsten Bögershausen]

On Tue, Oct 13, 2026 at 12:58:28AM -0600, Curtis Allen Smith wrote:

Thanks for the fast and good answers everybody.
As a comment: no, I didn't intend to tak over the series -
just want to move things forward.
And the Helped-By is of course ok.

I don't know if we need more explanations why this change
is needed (after so long time), but here is my undestanding:

When we run


git init t && cd t
printf 'one\ntwo\nthree\n' >file.txt
git add . && git commit -m init

followed by

printf 'two\ntwo\nthree\n' >file.txt

then Git will notice that the actual  mtime as a result of lstat()
is different from what is recorded in the index.
As a result, git needs to re-hash the content to conclude if the
content has changed.
However, in order to avoid the expensive hashing,
Git concludes that when the size of a file has changed
then the content -must- be different - files having a different size
are never identical.
This did work well until the .gitattributes feature was introduced.
Now, in the example below, a file having a different size -can-
result in the same blob once it is commited.
Therefore this patch does need to check if attributes
(for input conversions) are defined.
If not, take the shortcut and conclude that the file is modified.
But if, take the long way, which is what happens when only mtime
(and possibly other elements from struct st are changed.

And now the new size, mtime (and the rest)
will be recorded in the index. So it is a one-time cost.

Does this make sense ?


> "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.
> 
> Helped-by: Torsten Bögershausen <tboegi@web.de>
> Signed-off-by: Curtis Allen Smith <curtis.allen.smith@gmail.com>
> ---
> Changes since v3:
>  - The fix is inlined into the condition in ie_modified(), with no
>    helper function.
>  - Shorter log message, using Junio's wording; the "real life"
>    examples are gone.
> 
> Cost, i9-14900K under WSL2, warm cache, fastest of 5 runs of
> "git status -uno", stock 6de20f6 against this patch:
> 
> 	10000 files x 2.6 KB, no conversion       stock      v4
> 	  clean tree                               5 ms     5 ms
> 	  10000 files with a size change          26 ms    24 ms
> 
> 	10000 files x 2.6 KB, "* text=auto eol=lf"
> 	  clean tree                               5 ms     5 ms
> 	  10000 genuinely modified                25 ms   137 ms
> 	  10000 CRLF-rewritten, 1st run           21 ms   143 ms
> 	  the same, every run after               21 ms     5 ms
> 
> 	200 files x 1 MB, "* text=auto eol=lf"
> 	  clean tree                               3 ms     3 ms
> 	  200 genuinely modified                   3 ms   498 ms
> 	  200 CRLF-rewritten, 1st run              3 ms   584 ms
> 	  the same, every run after                3 ms     3 ms
> 
> The attribute lookup happens only for a path whose size changed, so a
> clean tree pays nothing, and a tree without conversion pays one
> lookup per size-changed path (the first block).  A converted path
> whose size changed is now read, converted and hashed, which is what
> already happens when only its mtime changes.  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 ready if wanted.  It takes the
> "genuinely modified" rows to 57 ms and 25 ms.

[snip]

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

[Curtis Allen Smith]

"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