Re: [PATCH] tag.c: move PGP verification code from plumbing
- From
Jeff King <peff@peff.net>
- Date
- Mar 25, 2016, 05:31 UTC
- Message-ID
- <20160325053134.GA27614@sigill.intra.peff.net>
- In-Reply-To
- <CAPig+cQe5bwHXq4_qegBCM8Kqoqiz7K2ZtVk0FGMSEUPWQHyYA@mail.gmail.com>
On Fri, Mar 25, 2016 at 01:23:57AM -0400, Eric Sunshine wrote:
Show 9 quoted lines
> On Thu, Mar 24, 2016 at 8:33 PM, <santiago@nyu.edu> wrote: > > The verify tag function is just a thin wrapper around the verify-tag > > command. We can avoid one fork call by doing the verification inside > > the tag builtin instead. > > Hopefully, the below review comments are meaningful, however, aside > from having just read Peff's review of the previous version of this > patch, I haven't been following this discussion, so it's possible some > comments may be off the mark. Caveat emptor.
Thanks for reviewing. I agree with all of the comments you made, but I'd add a little more to the last one:
Show 17 quoted lines
> > + > > + /* sometimes the program was terminated because this signal > > + * was received in the process of writing the gpg input. > > + * We ignore it for this call and restore it afterwards */ > > I realize that the bulk of this comment block was merely relocated > from builtin/verify-tag.c, however, now would be a good time to fix > its style violation and format it like this: > > /* > * This is a multi-line > * comment. > */ > > Also, the last line of the comment, which you added when relocating > it, merely repeats what the code itself already says clearly, thus is > not particularly useful and should be dropped.
I actually think we can drop this comment entirely. Pushing and popping SIGPIPE when piping to a sub-program like this is not that exotic in our code base. And if the SIGPIPE handling here is done in its own patch, then if somebody wants to see more discussion or reasoning, they can go to its commit message (which can then go into more detail about why we might see SIGPIPE, and not just a vague "sometimes...").
-Peff