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

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

Jeff King <peff@peff.net> writes:

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

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

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

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