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

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

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

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