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. > To do this, the run_pgp_verify() and verify_tag() functions are moved to > tag.c. The definition of verify_tag was changed to support extra > arguments that match the builtin/tag and builtin/verify-tag modules. The > SIGPIPE ignore call in tag-verify was also moved to a more sensible > place, now that both modules need it. This patch is doing too much, thus making it difficult to review and reason about. Based upon this paragraph alone, you may want to split the patch into three or more patches. At the very least, have a patch which does the code movement (and nothing else); one which adjusts the argument lists; and one which adjusts SIGPIPE handling. More generally, figure out the distinct conceptual changes being made here, and give each such change its own patch. > The function name was also changed to pgp_verify_tag to avoid conflicts with > mktag.c's. > > Signed-off-by: Santiago Torres <santiago@xxxxxxx> > --- Right here below the "---" line is where, for the sake of reviewers, you would explain what changed since the previous version. Also, as a reviewer aid, provide a link to the previous discussion, like this[1]. Finally, indicate the version number of the patch in the subject, like this [PATCH v3] (see git-format-patch's -v option). More below... [1]: http://thread.gmane.org/gmane.comp.version-control.git/289803 > diff --git a/builtin/tag.c b/builtin/tag.c > @@ -86,32 +87,22 @@ static int for_each_tag_name(const char **argv, each_tag_name_fn fn) > return 0; > } > > -static int verify_tag(const char *name, const char *ref, > - const unsigned char *sha1) > -{ > - const char *argv_verify_tag[] = {"verify-tag", > - "-v", "SHA1_HEX", NULL}; > - argv_verify_tag[2] = sha1_to_hex(sha1); > > - if (run_command_v_opt(argv_verify_tag, RUN_GIT_CMD)) > - return error(_("could not verify the tag '%s'"), name); > - return 0; > -} > This deletion inconsistently leaves an extra blank line between functions; thus you'd want to delete one more (blank) line. > static int do_sign(struct strbuf *buffer) > { > diff --git a/builtin/verify-tag.c b/builtin/verify-tag.c > @@ -95,11 +48,12 @@ int cmd_verify_tag(int argc, const char **argv, const char *prefix) > if (verbose) > flags |= GPG_VERIFY_VERBOSE; > > - /* sometimes the program was terminated because this signal > - * was received in the process of writing the gpg input: */ > - signal(SIGPIPE, SIG_IGN); > while (i < argc) > - if (verify_tag(argv[i++], flags)) > + name = argv[i++]; > + if (get_sha1(name, sha1)) > + return error("tag '%s' not found.", name); > + > + if (pgp_verify_tag(NULL, NULL, sha1, flags)) > had_error = 1; Meh, this isn't Python. Due to the missing braces, the only thing inside the while() loop is the assignment to 'name'; all the other indented code is outside the while(). Did you run the test suite following this change? Did it all pass? If so, perhaps an additional test or two to catch this sort of error would be warranted. > return had_error; > } > diff --git a/gpg-interface.c b/gpg-interface.c > @@ -232,6 +232,11 @@ int verify_signed_buffer(const char *payload, size_t payload_size, > if (gpg_output) > gpg.err = -1; > args_gpg[3] = path; > + > + /* 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. > + sigchain_push(SIGPIPE, SIG_IGN); > if (start_command(&gpg)) { > unlink(path); > return error(_("could not run gpg.")); > @@ -250,6 +255,7 @@ int verify_signed_buffer(const char *payload, size_t payload_size, > close(gpg.out); > > ret = finish_command(&gpg); > + sigchain_pop(SIGPIPE); > > unlink_or_warn(path); -- 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