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

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 2, 2017, 17:52 UTC
Message-ID: <xmqqmvd3d0ru.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <20170302085313.r6dox4wa2kqnp7ao@sigill.intra.peff.net>

Jeff King <peff@peff.net> writes:

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

Yes.  I think it makes sense.

Show 8 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/

Thanks. I felt there was something wrong and looked at the line three times but somehow failed to spot exactly what was wrong ;-)

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

Note that the comment is just documenting what I learned and thought while working on an unrelated thing that happened to be sitting next to it.

Nobody asks "I am OK without the contents i.e. size-only" and "Can you see if this is binary?" at the same time (and if a caller did, it would never have got is_binary with the original code). s->size is still a copy of st.size at this point of the code (we have not actually updated it to the size of the real blob, which happens a bit later in the flow of this codepath where we actually slurp the thing in and run the conversion). So with or without this patch, there shouldn't be any change in the behaviour wrt CHECK_BINARY.

Show 6 quoted lines

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

To be quite honest, I do not think this code should cater to LFS or any other conversion hack. They all install their own diff driver and they can tell diff_filespec_is_binary() if the thing is binary or not without falling back to this heuristics codepath.

What is done here *is* inconsistent with what is done on the other side of if/else, that compares big_file_threshold with in-git size, not in-working-tree size. That may want to be addressed, but I am not sure if it is worth it.