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

From: Chris Torek <chris.torek@gmail.com>
To: "René Scharfe" <l.s.r@web.de>
Cc: Git Mailing List <git@vger.kernel.org>,
	Junio C Hamano <gitster@pobox.com>
Subject: Re: [PATCH] connected: use buffered I/O to talk to rev-list
Date: Sun, 2 Aug 2020 09:08:08 -0700	[thread overview]
Message-ID: <CAPx1Gvesmwf_CBj6D=YriFQgtEsDyUe7PHa6p9AvxG=fwAvHpA@mail.gmail.com> (raw)
In-Reply-To: <2e2907ac-3be9-c0ed-830a-f8aa28b471aa@web.de>

On Sun, Aug 2, 2020 at 7:39 AM René Scharfe <l.s.r@web.de> wrote:
> @@ -135,16 +135,10 @@ int check_connected(oid_iterate_fn fn, void *cb_data,
>                 if (new_pack && find_pack_entry_one(oid.hash, new_pack))
>                         continue;
>
> -               memcpy(commit, oid_to_hex(&oid), hexsz);
> -               if (write_in_full(rev_list.in, commit, hexsz + 1) < 0) {
> -                       if (errno != EPIPE && errno != EINVAL)
> -                               error_errno(_("failed write to rev-list"));
> -                       err = -1;
> -                       break;
> -               }
> +               fprintf(rev_list_in, "%s\n", oid_to_hex(&oid));
>         } while (!fn(cb_data, &oid));
>
> -       if (close(rev_list.in))
> +       if (fclose(rev_list_in))
>                 err = error_errno(_("failed to close rev-list's stdin"));
>
>         sigchain_pop(SIGPIPE);
> --
> 2.28.0

The same ferror()-before-fclose() remarks apply here too,
but this time the explicit errno checking (EPIPE) cannot
be done -- it's too late, errno is probably overwritten.  I'm
not sure how valuable the explicit errno tests are in the first
place so I will leave that to others, but if we want to keep
the explicit tests, use:

    if (fprintf(...) < 0)

to check each fprintf(), and add a final fflush() call (with
another check) before the fclose().

Chris