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

[Junio C Hamano <gitster@pobox.com>]

Curtis Allen Smith <curtis.allen.smith@gmail.com> writes:

> 	git status --short      # ->  M file.txt
> 	git diff                # -> empty
> 	git add file.txt        # -> nothing to commit

I do not think "git add" would ever say nothing to commit.

	git commit file.txt

may say "nothing added to commit" when there is no modification made
to file.txt to indicate that there is no change worth comitting.

There are code paths that make performance trade-offs (see the
introductory part of Documentation/technical/racy-git.adoc for
examples) and are designed to tolerate false positives from
ie_modified(), filtering them out themselves.  I am somewhat
hesitant to butcher code paths at this low level to make
unconditional changes that reduce false positives without thinking
things through.  For example, we may want to disable this new code
when the 'CE_MATCH_RACY_IS_DIRTY' bit is set.  But the goal
nevertheless should be to avoid surfacing these false positives,
which are deliberately allowed as performance trade-offs, to code
paths like 'git status' that do not further filter them out (as you
demonstrated above).

There may be other cases where this patch unknowingly breaks
carefully made performance trade-offs.

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

Tests like these may run fast enough to trigger "oops, we may be in
the racy-git situation so make the comparison differently from the
normal code paths (usually spends extra cycles to compare the
contents)", which may hide existing problems, so these tests need to
be extra careful.

> +
>  test_done

Thanks.