Re: [PATCH] upload-pack: use buffered I/O to talk to rev-list

[Johannes Sixt <j6t@kdbg.org>]

Am 02.08.20 um 18:03 schrieb Chris Torek:
> One suggestion here:
> 
> On Sun, Aug 2, 2020 at 7:41 AM René Scharfe <l.s.r@web.de> wrote:
>> Like f0bca72dc77 (send-pack: use buffered I/O to talk to pack-objects,
>> 2016-06-08), significantly reduce the number of system calls and
>> simplify the code for sending object IDs to rev-list by using stdio's
>> buffering and handling errors after the loops.
>>
>> Signed-off-by: René Scharfe <l.s.r@web.de>
>> ---
>>  upload-pack.c | 21 +++++++--------------
>>  1 file changed, 7 insertions(+), 14 deletions(-)
>>
>> diff --git a/upload-pack.c b/upload-pack.c
>> index 86737410709..9f616c2c6a6 100644
>> --- a/upload-pack.c
>> +++ b/upload-pack.c
> 
> [snip]
> 
>> @@ -640,12 +636,11 @@ static int do_reachable_revlist(struct child_process *cmd,
>>                 }
>>                 if (reachable && o->type == OBJ_COMMIT)
>>                         o->flags |= TMP_MARK;
>> -               memcpy(namebuf, oid_to_hex(&o->oid), hexsz);
>> -               if (write_in_full(cmd->in, namebuf, hexsz + 1) < 0)
>> -                       goto error;
>> +               fprintf(cmd_in, "%s\n", oid_to_hex(&o->oid));
> 
> The fprintf() call here *can* return an error, e.g., if the
> connection has died.  If it does, it should set things up so that
> a later ferror(cmd_in) returns true.

True. We need an explicit test after each fprintf anyway because SIGPIPE
may be ignored, and then writing fails with EPIPE. On Windows, this is
doubly important because we do not have SIGPIPE at all (and always see
EPIPE), but we see EPIPE only on the first failed write; subsequent
writes produce EINVAL.

-- Hannes