Re: [PATCH] send-pack: use buffered I/O to talk to pack-objects

From: Junio C Hamano <gitster@pobox.com>
Date: Jun 9, 2016, 16:40 UTC
Message-ID: <xmqqr3c6iad1.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <vpqwplypnpr.fsf@anie.imag.fr>

Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:

Show 28 quoted lines

> Jeff King <peff@peff.net> writes:
>
>> --- a/send-pack.c
>> +++ b/send-pack.c
>> @@ -36,18 +36,15 @@ int option_parse_push_signed(const struct option *opt,
>>  	die("bad %s argument: %s", opt->long_name, arg);
>>  }
>>  
>> -static int feed_object(const unsigned char *sha1, int fd, int negative)
>> +static void feed_object(const unsigned char *sha1, FILE *fh, int negative)
>>  {
>> -	char buf[42];
>> -
>>  	if (negative && !has_sha1_file(sha1))
>> -		return 1;
>> +		return;
> [...]
>> @@ -97,21 +95,22 @@ static int pack_objects(int fd, struct ref *refs, struct sha1_array *extra, stru
> [...]
>>  	for (i = 0; i < extra->nr; i++)
>> -		if (!feed_object(extra->sha1[i], po.in, 1))
>> -			break;
>> +		feed_object(extra->sha1[i], po_in, 1);
>
> I may have missed the obvious, but doesn't this change the behavior when
> "negative && !has_sha1_file(sha1)" happens? I understand that you don't
> need write_or_whine anymore, but don't understand how you get rid of the
> "return 1" here.

The original feed_object() has somewhat strange interface in that a non-zero return from it is "Everything went alright!", and zero means "Oops, something went wrong". When the function actually writes things out, it calls write_or_whine(), whose return value also uses that (unusual) convention, which is the reason why it behaves that way.

The "return 1" you noticed happens when the function is told not to send history behind one object, but that object does not exist in our repository. This is a perfectly normal condition and the function just ignores it and returns without feeding it to pack-objects. It reports to the caller "Everything went alright!".

The original caller checks for errors to break out the feeding of the process early, with things like:

	if (!feed_object(...))
        	break;

IOW, the caller would have continued when hitting that "return 1" codepath.

And the code with the patch, the caller continues unconditionally, so there is no behaviour change, if I am reading the code correctly.

Thanks for carefully reading the change and pointing out hard-to-read parts, as always.