On Fri, Mar 25, 2016 at 01:23:57AM -0400, Eric Sunshine wrote: > On Thu, Mar 24, 2016 at 8:33 PM, <santiago@xxxxxxx> 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: > > + > > + /* 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 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html