On Thu, Aug 13, 2020 at 07:57:20AM +0200, René Scharfe wrote: > Am 13.08.20 um 07:17 schrieb Christian Couder: > > On Wed, Aug 12, 2020 at 6:54 PM René Scharfe <l.s.r@xxxxxx> 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). > >> - 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
