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

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

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?