From: Jeff King <email@example.com> To: "René Scharfe" <firstname.lastname@example.org> Cc: Christian Couder <email@example.com>, Git Mailing List <firstname.lastname@example.org>, Junio C Hamano <email@example.com>, Chris Torek <firstname.lastname@example.org>, Johannes Sixt <email@example.com> Subject: Re: [PATCH v2] upload-pack: use buffered I/O to talk to rev-list Date: Thu, 13 Aug 2020 05:01:07 -0400 [thread overview] Message-ID: <20200813090107.GB3092220@coredump.intra.peff.net> (raw) In-Reply-To: <firstname.lastname@example.org> 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 <email@example.com> 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
prev parent reply other threads:[~2020-08-13 9:01 UTC|newest] Thread overview: 9+ messages / expand[flat|nested] mbox.gz Atom feed top 2020-08-02 14:38 [PATCH] " René Scharfe 2020-08-02 16:03 ` Chris Torek 2020-08-03 14:00 ` René Scharfe 2020-08-03 15:54 ` Chris Torek 2020-08-03 18:15 ` Johannes Sixt 2020-08-12 16:52 ` [PATCH v2] " René Scharfe 2020-08-13 5:17 ` Christian Couder 2020-08-13 5:57 ` René Scharfe 2020-08-13 9:01 ` Jeff King [this message]
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style List information: http://vger.kernel.org/majordomo-info.html * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20200813090107.GB3092220@coredump.intra.peff.net \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --subject='Re: [PATCH v2] upload-pack: use buffered I/O to talk to rev-list' \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: link
Code repositories for project(s) associated with this inbox: https://80x24.org/mirrors/git.git This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).