Re: [PATCH v1 1/1] git diff --quiet exits with 1 on clean tree with CRLF conversions

[Mike Crowe <mac@mcrowe.com>]

On Wednesday 01 March 2017 at 13:54:26 -0800, Junio C Hamano wrote:
> Now I thought about it through a bit more thoroughly, I think this
> is the right approach, so here is my (tenative) final version.
> 
> I seem to be getty really rusty---after all the codepaths involved
> are practically all my code and I should have noticed the real
> culprit during my first attempt X-<.
> 
> Thanks for helping.
> 
> -- >8 --
> Subject: [PATCH] diff: do not short-cut CHECK_SIZE_ONLY check in diff_populate_filespec()
> 
> Callers of diff_populate_filespec() can choose to ask only for the
> size of the blob without grabbing the blob data, and the function,
> after running lstat() when the filespec points at a working tree
> file, returns by copying the value in size field of the stat
> structure into the size field of the filespec when this is the case.
> 
> However, this short-cut cannot be taken if the contents from the
> path needs to go through convert_to_git(), whose resulting real blob
> data may be different from what is in the working tree file.
> 
> As "git diff --quiet" compares the .size fields of filespec
> structures to skip content comparison, this bug manifests as a
> false "there are differences" for a file that needs eol conversion,
> for example.
> 
> Reported-by: Mike Crowe <mac@mcrowe.com>
> Helped-by: Torsten Bögershausen <tboegi@web.de>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
>  diff.c                    | 19 ++++++++++++++++++-
>  t/t0028-diff-converted.sh | 27 +++++++++++++++++++++++++++
>  2 files changed, 45 insertions(+), 1 deletion(-)
>  create mode 100755 t/t0028-diff-converted.sh
> 
> diff --git a/diff.c b/diff.c
> index 8c78fce49d..dc51dceb44 100644
> --- a/diff.c
> +++ b/diff.c
> @@ -2792,8 +2792,25 @@ int diff_populate_filespec(struct diff_filespec *s, unsigned int flags)
>  			s->should_free = 1;
>  			return 0;
>  		}
> -		if (size_only)
> +
> +		/*
> +		 * Even if the caller would be happy with getting
> +		 * only the size, we cannot return early at this
> +		 * point if the path requires us to run the content
> +		 * conversion.
> +		 */
> +		if (!would_convert_to_git(s->path) && size_only)
>  			return 0;
> +
> +		/*
> +		 * Note: this check uses xsize_t(st.st_size) that may
> +		 * not be the true size of the blob after it goes
> +		 * through convert_to_git().  This may not strictly be
> +		 * correct, but the whole point of big_file_threashold
> +		 * and is_binary check being that we want to avoid
> +		 * opening the file and inspecting the contents, this
> +		 * is probably fine.
> +		 */
>  		if ((flags & CHECK_BINARY) &&
>  		    s->size > big_file_threshold && s->is_binary == -1) {
>  			s->is_binary = 1;

This patch solves the problem for me. Including my tests where the file
size doesn't change but the file has been touched. It also doesn't have the
side effect of failing to report the extra trailing newline that the
original fix suffered from.

All the solutions presented so far do cause a small change in behaviour
when using git diff --quiet: they may now cause warning messages like:

 warning: CRLF will be replaced by LF in crlf.txt.
 The file will have its original line endings in your working directory.

to be emitted (unless of course core.safecrlf=false.) I think this is an
unavoidable side-effect of doing the job properly but it might be worth
mentioning.

> diff --git a/t/t0028-diff-converted.sh b/t/t0028-diff-converted.sh
> new file mode 100755
> index 0000000000..3d5ab9565b
> --- /dev/null
> +++ b/t/t0028-diff-converted.sh
> @@ -0,0 +1,27 @@
> +#!/bin/sh
> +#
> +# Copyright (c) 2017 Mike Crowe
> +#
> +# These tests ensure that files changing line endings in the presence
> +# of .gitattributes to indicate that line endings should be ignored
> +# don't cause 'git diff' or 'git diff --quiet' to think that they have
> +# been changed.
> +
> +test_description='git diff with files that require CRLF conversion'
> +
> +. ./test-lib.sh
> +
> +test_expect_success setup '
> +	echo "* text=auto" >.gitattributes &&
> +	printf "Hello\r\nWorld\r\n" >crlf.txt &&
> +	git add .gitattributes crlf.txt &&
> +	git commit -m "initial"
> +'
> +
> +test_expect_success 'quiet diff works on file with line-ending change that has no effect on repository' '
> +	printf "Hello\r\nWorld\n" >crlf.txt &&
> +	git status &&
> +	git diff --quiet
> +'
> +
> +test_done

As I said before, this doesn't actually test the case when the file sizes
match. However, given the way that the code has changed the actual file
sizes are not compared, so perhaps this doesn't matter.

Thanks for all your help investigating this.

Mike.