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

From: Jeff King <peff@peff.net>
Date: Mar 2, 2017, 08:53 UTC
Message-ID: <20170302085313.r6dox4wa2kqnp7ao@sigill.intra.peff.net>
In-Reply-To: <xmqqa894fyst.fsf@gitster.mtv.corp.google.com>

On Wed, Mar 01, 2017 at 01:54:26PM -0800, Junio C Hamano wrote:

> -- >8 --
> Subject: [PATCH] diff: do not short-cut CHECK_SIZE_ONLY check in diff_populate_filespec()

Thanks, this is well-explained, and the new comments in the code really help.

I wondered if we should be checking would_convert_to_git() in reuse_worktree_file(), but we already do. It's just that we may still end up in this code-path when we're _actually_ diffing the working tree file, not just trying to optimize.

Show 18 quoted lines

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

The would_convert_to_git() function is a little expensive (it may have to do an attribute lookup). It may be worth swapping the two halves of the conditional here to get the short-circuit.

It may not matter much in practice, though, because in the !size_only case we'd make the same query lower a few lines later (and in theory expensive bits of the attr lookup are cached).

Show 6 quoted lines

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

s/threashold/threshold/

Show 7 quoted lines

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

I'm trying to think how this "not strictly correct" could bite us. For line-ending conversion, I'd say that the before/after are going to be approximately the same size. But what about something like LFS? If I have a 600MB file that convert_to_git() filters into a short LFS pointer, I think this changes the behavior. Before, we would diff the pointer file, but now we'll get "binary file changed".

I wonder if we should take the opposite approach, and ignore big_file_threshold for converted files. One assumes that such gigantic files are binary, and therefore do not have line endings to convert. And any filtering has a reasonable chance of condensing them to something much smaller.

I dunno. I'm sure somebody has some horrific 500MB-filtering example that can prove me wrong.

-Peff