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

From: Jeff King <peff@peff.net>
Date: Aug 13, 2020, 09:01 UTC
Message-ID: <20200813090107.GB3092220@coredump.intra.peff.net>
In-Reply-To: <866055fd-7857-3eb3-6dae-befc8f891994@web.de>

On Thu, Aug 13, 2020 at 07:57:20AM +0200, René Scharfe wrote:

Show 13 quoted lines

> Am 13.08.20 um 07:17 schrieb Christian Couder:
> > On Wed, Aug 12, 2020 at 6:54 PM René Scharfe <l.s.r@web.de> wrote:
> >
> >> -       close(cmd->in);
> >> +       if (ferror(cmd_in) || fflush(cmd_in))
> >> +               goto error;
> >> +       fclose(cmd_in);
> >>         cmd->in = -1;
> >
> > I wonder if setting cmd->in to -1 is still useful...
> 
> The patch doesn't change its usefulness.  It probably was not necessary
> to begin with.

Right. We exit immediately after setting it, but it's not clearly a dead load: the "cmd" struct is passed in from the caller. Prior to 2997178ee6 (upload-pack: split check_unreachable() in two, prep for get_reachable_list(), 2016-06-12), it was part of a longer function which did have more error handling.

But after the split, has_unreachable() knows that it is always closed after do_reachable_revlist() returns, so it never looks at cmd.in again. It might be worth removing just to avoid confusion.

I thought this also implied that the conditional close() in the error block was not necessary, but it is. In the existing code we could "goto error" from start_command() failing, before we ever assign to cmd->in. The run-command API clears the struct in that case (so we'd see cmd->in == 0, and avoid closing).

Show 12 quoted lines

> >> -       if (cmd->in >= 0)
> >> -               close(cmd->in);
> >> +       if (cmd_in)
> >> +               fclose(cmd_in);
> >
> > ...as we don't check cmd->in anymore at the end of the function, but
> > we now check cmd_in instead. So should cmd_in have been set to -1
> > instead of cmd->in?
> 
> This error handler is not reached from the place that sets cmd->in back
> to -1.  It can be reached from a place where cmd_in is still NULL, so
> this check is necessary and setting cmd_in to NULL above is not needed.

Right, that makes sense. Your NULL cmd_in is replacing the zero'd cmd->in from start_command().

So I think your patch is correct. It might be worth removing the "-1" assignment on top.

-Peff