{"thread":{"id":"41821","subject":"[PATCH] tag.c: move PGP verification code from plumbing","startedAt":"2016-03-25T00:33:37Z","lastAt":"2016-03-26T06:33:28Z","messageCount":5,"participants":["santiago@nyu.edu","Eric Sunshine","Jeff King","Santiago Torres"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"281760","messageId":"1458866017-15490-1-git-send-email-santiago@nyu.edu","threadId":"41821","inReplyTo":null,"subject":"[PATCH] tag.c: move PGP verification code from plumbing","fromName":"","fromEmail":"santiago@nyu.edu","sentAt":"2016-03-25T00:33:37Z","receivedAt":"2016-03-25T00:33:37Z","isPatch":true,"sender":{"key":"santiago@nyu.edu","avatar":"https://avatars.githubusercontent.com/u/3579933?v=4"},"body":"From: Santiago Torres <santiago@nyu.edu>\n\nThe verify tag function is just a thin wrapper around the verify-tag\ncommand. We can avoid one fork call by doing the verification inside\nthe tag builtin instead.\n\nTo do this, the run_pgp_verify() and verify_tag() functions are moved to\ntag.c. The definition of verify_tag was changed to support extra\narguments that match the builtin/tag and builtin/verify-tag modules. The\nSIGPIPE ignore call in tag-verify was also moved to a more sensible\nplace, now that both modules need it.\n\nThe function name was also changed to pgp_verify_tag to avoid conflicts with\nmktag.c's.\n\nSigned-off-by: Santiago Torres <santiago@nyu.edu>\n---\n builtin/tag.c        | 26 ++++++++---------------\n builtin/verify-tag.c | 60 ++++++----------------------------------------------\n gpg-interface.c      |  6 ++++++\n tag.c                | 48 +++++++++++++++++++++++++++++++++++++++++\n tag.h                |  2 ++\n 5 files changed, 72 insertions(+), 70 deletions(-)\n\ndiff --git a/builtin/tag.c b/builtin/tag.c\nindex 1705c94..af8f3ba 100644\n--- a/builtin/tag.c\n+++ b/builtin/tag.c\n@@ -65,9 +65,10 @@ static int list_tags(struct ref_filter *filter, struct ref_sorting *sorting, con\n }\n \n typedef int (*each_tag_name_fn)(const char *name, const char *ref,\n-\t\t\t\tconst unsigned char *sha1);\n+\t\t\t\tconst unsigned char *sha1, unsigned flags);\n \n-static int for_each_tag_name(const char **argv, each_tag_name_fn fn)\n+static int for_each_tag_name(const char **argv, each_tag_name_fn fn,\n+\t\tunsigned flags)\n {\n \tconst char **p;\n \tchar ref[PATH_MAX];\n@@ -86,32 +87,22 @@ static int for_each_tag_name(const char **argv, each_tag_name_fn fn)\n \t\t\thad_error = 1;\n \t\t\tcontinue;\n \t\t}\n-\t\tif (fn(*p, ref, sha1))\n+\t\tif (fn(*p, ref, sha1, flags))\n \t\t\thad_error = 1;\n \t}\n \treturn had_error;\n }\n \n static int delete_tag(const char *name, const char *ref,\n-\t\t\t\tconst unsigned char *sha1)\n+\t\t\t\tconst unsigned char *sha1, unsigned flags)\n {\n-\tif (delete_ref(ref, sha1, 0))\n+\tif (delete_ref(ref, sha1, flags))\n \t\treturn 1;\n \tprintf(_(\"Deleted tag '%s' (was %s)\\n\"), name, find_unique_abbrev(sha1, DEFAULT_ABBREV));\n \treturn 0;\n }\n \n-static int verify_tag(const char *name, const char *ref,\n-\t\t\t\tconst unsigned char *sha1)\n-{\n-\tconst char *argv_verify_tag[] = {\"verify-tag\",\n-\t\t\t\t\t\"-v\", \"SHA1_HEX\", NULL};\n-\targv_verify_tag[2] = sha1_to_hex(sha1);\n \n-\tif (run_command_v_opt(argv_verify_tag, RUN_GIT_CMD))\n-\t\treturn error(_(\"could not verify the tag '%s'\"), name);\n-\treturn 0;\n-}\n \n static int do_sign(struct strbuf *buffer)\n {\n@@ -424,9 +415,10 @@ int cmd_tag(int argc, const char **argv, const char *prefix)\n \tif (filter.merge_commit)\n \t\tdie(_(\"--merged and --no-merged option are only allowed with -l\"));\n \tif (cmdmode == 'd')\n-\t\treturn for_each_tag_name(argv, delete_tag);\n+\t\treturn for_each_tag_name(argv, delete_tag, 0);\n \tif (cmdmode == 'v')\n-\t\treturn for_each_tag_name(argv, verify_tag);\n+\t\treturn for_each_tag_name(argv, pgp_verify_tag,\n+\t\t\t\tGPG_VERIFY_VERBOSE);\n \n \tif (msg.given || msgfile) {\n \t\tif (msg.given && msgfile)\ndiff --git a/builtin/verify-tag.c b/builtin/verify-tag.c\nindex 00663f6..2e6a175 100644\n--- a/builtin/verify-tag.c\n+++ b/builtin/verify-tag.c\n@@ -18,55 +18,6 @@ static const char * const verify_tag_usage[] = {\n \t\tNULL\n };\n \n-static int run_gpg_verify(const char *buf, unsigned long size, unsigned flags)\n-{\n-\tstruct signature_check sigc;\n-\tint len;\n-\tint ret;\n-\n-\tmemset(&sigc, 0, sizeof(sigc));\n-\n-\tlen = parse_signature(buf, size);\n-\n-\tif (size == len) {\n-\t\tif (flags & GPG_VERIFY_VERBOSE)\n-\t\t\twrite_in_full(1, buf, len);\n-\t\treturn error(\"no signature found\");\n-\t}\n-\n-\tret = check_signature(buf, len, buf + len, size - len, &sigc);\n-\tprint_signature_buffer(&sigc, flags);\n-\n-\tsignature_check_clear(&sigc);\n-\treturn ret;\n-}\n-\n-static int verify_tag(const char *name, unsigned flags)\n-{\n-\tenum object_type type;\n-\tunsigned char sha1[20];\n-\tchar *buf;\n-\tunsigned long size;\n-\tint ret;\n-\n-\tif (get_sha1(name, sha1))\n-\t\treturn error(\"tag '%s' not found.\", name);\n-\n-\ttype = sha1_object_info(sha1, NULL);\n-\tif (type != OBJ_TAG)\n-\t\treturn error(\"%s: cannot verify a non-tag object of type %s.\",\n-\t\t\t\tname, typename(type));\n-\n-\tbuf = read_sha1_file(sha1, &type, &size);\n-\tif (!buf)\n-\t\treturn error(\"%s: unable to read file.\", name);\n-\n-\tret = run_gpg_verify(buf, size, flags);\n-\n-\tfree(buf);\n-\treturn ret;\n-}\n-\n static int git_verify_tag_config(const char *var, const char *value, void *cb)\n {\n \tint status = git_gpg_config(var, value, cb);\n@@ -78,6 +29,8 @@ static int git_verify_tag_config(const char *var, const char *value, void *cb)\n int cmd_verify_tag(int argc, const char **argv, const char *prefix)\n {\n \tint i = 1, verbose = 0, had_error = 0;\n+\tunsigned char sha1[20];\n+\tconst char *name;\n \tunsigned flags = 0;\n \tconst struct option verify_tag_options[] = {\n \t\tOPT__VERBOSE(&verbose, N_(\"print tag contents\")),\n@@ -95,11 +48,12 @@ int cmd_verify_tag(int argc, const char **argv, const char *prefix)\n \tif (verbose)\n \t\tflags |= GPG_VERIFY_VERBOSE;\n \n-\t/* sometimes the program was terminated because this signal\n-\t * was received in the process of writing the gpg input: */\n-\tsignal(SIGPIPE, SIG_IGN);\n \twhile (i < argc)\n-\t\tif (verify_tag(argv[i++], flags))\n+\t\tname = argv[i++];\n+\t\tif (get_sha1(name, sha1))\n+\t\t\treturn error(\"tag '%s' not found.\", name);\n+\n+\t\tif (pgp_verify_tag(NULL, NULL, sha1, flags))\n \t\t\thad_error = 1;\n \treturn had_error;\n }\ndiff --git a/gpg-interface.c b/gpg-interface.c\nindex 3dc2fe3..1b421b7 100644\n--- a/gpg-interface.c\n+++ b/gpg-interface.c\n@@ -232,6 +232,11 @@ int verify_signed_buffer(const char *payload, size_t payload_size,\n \tif (gpg_output)\n \t\tgpg.err = -1;\n \targs_gpg[3] = path;\n+\n+\t/* sometimes the program was terminated because this signal\n+\t * was received in the process of writing the gpg input.\n+\t * We ignore it for this call and restore it afterwards */\n+\tsigchain_push(SIGPIPE, SIG_IGN);\n \tif (start_command(&gpg)) {\n \t\tunlink(path);\n \t\treturn error(_(\"could not run gpg.\"));\n@@ -250,6 +255,7 @@ int verify_signed_buffer(const char *payload, size_t payload_size,\n \tclose(gpg.out);\n \n \tret = finish_command(&gpg);\n+\tsigchain_pop(SIGPIPE);\n \n \tunlink_or_warn(path);\n \ndiff --git a/tag.c b/tag.c\nindex d72f742..7097a84 100644\n--- a/tag.c\n+++ b/tag.c\n@@ -3,9 +3,57 @@\n #include \"commit.h\"\n #include \"tree.h\"\n #include \"blob.h\"\n+#include \"sigchain.h\"\n \n const char *tag_type = \"tag\";\n \n+\n+static int run_gpg_verify(const char *buf, unsigned long size, unsigned flags)\n+{\n+\tstruct signature_check sigc;\n+\tint payload_size;\n+\tint ret;\n+\n+\tmemset(&sigc, 0, sizeof(sigc));\n+\n+\tpayload_size = parse_signature(buf, size);\n+\n+\tif (size == payload_size) {\n+\t\twrite_in_full(1, buf, payload_size);\n+\t\treturn error(\"No PGP signature found in this tag!\");\n+\t}\n+\n+\tret = check_signature(buf, payload_size, buf + payload_size,\n+\t\t\tsize - payload_size, &sigc);\n+\tprint_signature_buffer(&sigc, flags);\n+\n+\tsignature_check_clear(&sigc);\n+\treturn ret;\n+}\n+\n+int pgp_verify_tag(const char *name, const char *ref,\n+\t\tconst unsigned char *sha1, unsigned flags)\n+{\n+\n+\tenum object_type type;\n+\tunsigned long size;\n+\tconst char* buf;\n+\tint ret;\n+\n+\ttype = sha1_object_info(sha1, NULL);\n+\tif (type != OBJ_TAG)\n+\t\treturn error(\"%s: cannot verify a non-tag object of type %s.\",\n+\t\t\t\tname, typename(type));\n+\n+\tbuf = read_sha1_file(sha1, &type, &size);\n+\tif (!buf)\n+\t\treturn error(\"%s: unable to read file.\", name);\n+\n+\tret = run_gpg_verify(buf, size, flags);\n+\n+\treturn ret;\n+}\n+\n struct object *deref_tag(struct object *o, const char *warn, int warnlen)\n {\n \twhile (o && o->type == OBJ_TAG)\ndiff --git a/tag.h b/tag.h\nindex f4580ae..22289a5 100644\n--- a/tag.h\n+++ b/tag.h\n@@ -17,5 +17,7 @@ extern int parse_tag_buffer(struct tag *item, const void *data, unsigned long si\n extern int parse_tag(struct tag *item);\n extern struct object *deref_tag(struct object *, const char *, int);\n extern struct object *deref_tag_noverify(struct object *);\n+extern int pgp_verify_tag(const char *name, const char *ref,\n+\t\tconst unsigned char *sha1, unsigned flags);\n \n #endif /* TAG_H */\n-- \n2.7.3\n"},{"id":"281771","messageId":"CAPig+cQe5bwHXq4_qegBCM8Kqoqiz7K2ZtVk0FGMSEUPWQHyYA@mail.gmail.com","threadId":"41821","inReplyTo":"1458866017-15490-1-git-send-email-santiago@nyu.edu","subject":"Re: [PATCH] tag.c: move PGP verification code from plumbing","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2016-03-25T05:23:57Z","receivedAt":"2016-03-25T05:23:57Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Thu, Mar 24, 2016 at 8:33 PM,  <santiago@nyu.edu> wrote:\n> The verify tag function is just a thin wrapper around the verify-tag\n> command. We can avoid one fork call by doing the verification inside\n> the tag builtin instead.\n\nHopefully, the below review comments are meaningful, however, aside\nfrom having just read Peff's review of the previous version of this\npatch, I haven't been following this discussion, so it's possible some\ncomments may be off the mark. Caveat emptor.\n\n> To do this, the run_pgp_verify() and verify_tag() functions are moved to\n> tag.c. The definition of verify_tag was changed to support extra\n> arguments that match the builtin/tag and builtin/verify-tag modules. The\n> SIGPIPE ignore call in tag-verify was also moved to a more sensible\n> place, now that both modules need it.\n\nThis patch is doing too much, thus making it difficult to review and\nreason about. Based upon this paragraph alone, you may want to split\nthe patch into three or more patches. At the very least, have a patch\nwhich does the code movement (and nothing else); one which adjusts the\nargument lists; and one which adjusts SIGPIPE handling. More\ngenerally, figure out the distinct conceptual changes being made here,\nand give each such change its own patch.\n\n> The function name was also changed to pgp_verify_tag to avoid conflicts with\n> mktag.c's.\n>\n> Signed-off-by: Santiago Torres <santiago@nyu.edu>\n> ---\n\nRight here below the \"---\" line is where, for the sake of reviewers,\nyou would explain what changed since the previous version. Also, as a\nreviewer aid, provide a link to the previous discussion, like this[1].\nFinally, indicate the version number of the patch in the subject, like\nthis [PATCH v3] (see git-format-patch's -v option).\n\nMore below...\n\n[1]: http://thread.gmane.org/gmane.comp.version-control.git/289803\n\n> diff --git a/builtin/tag.c b/builtin/tag.c\n> @@ -86,32 +87,22 @@ static int for_each_tag_name(const char **argv, each_tag_name_fn fn)\n>         return 0;\n>  }\n>\n> -static int verify_tag(const char *name, const char *ref,\n> -                               const unsigned char *sha1)\n> -{\n> -       const char *argv_verify_tag[] = {\"verify-tag\",\n> -                                       \"-v\", \"SHA1_HEX\", NULL};\n> -       argv_verify_tag[2] = sha1_to_hex(sha1);\n>\n> -       if (run_command_v_opt(argv_verify_tag, RUN_GIT_CMD))\n> -               return error(_(\"could not verify the tag '%s'\"), name);\n> -       return 0;\n> -}\n>\n\nThis deletion inconsistently leaves an extra blank line between\nfunctions; thus you'd want to delete one more (blank) line.\n\n>  static int do_sign(struct strbuf *buffer)\n>  {\n> diff --git a/builtin/verify-tag.c b/builtin/verify-tag.c\n> @@ -95,11 +48,12 @@ int cmd_verify_tag(int argc, const char **argv, const char *prefix)\n>         if (verbose)\n>                 flags |= GPG_VERIFY_VERBOSE;\n>\n> -       /* sometimes the program was terminated because this signal\n> -        * was received in the process of writing the gpg input: */\n> -       signal(SIGPIPE, SIG_IGN);\n>         while (i < argc)\n> -               if (verify_tag(argv[i++], flags))\n> +               name = argv[i++];\n> +               if (get_sha1(name, sha1))\n> +                       return error(\"tag '%s' not found.\", name);\n> +\n> +               if (pgp_verify_tag(NULL, NULL, sha1, flags))\n>                         had_error = 1;\n\nMeh, this isn't Python. Due to the missing braces, the only thing\ninside the while() loop is the assignment to 'name'; all the other\nindented code is outside the while().\n\nDid you run the test suite following this change? Did it all pass? If\nso, perhaps an additional test or two to catch this sort of error\nwould be warranted.\n\n>         return had_error;\n>  }\n> diff --git a/gpg-interface.c b/gpg-interface.c\n> @@ -232,6 +232,11 @@ int verify_signed_buffer(const char *payload, size_t payload_size,\n>         if (gpg_output)\n>                 gpg.err = -1;\n>         args_gpg[3] = path;\n> +\n> +       /* sometimes the program was terminated because this signal\n> +        * was received in the process of writing the gpg input.\n> +        * We ignore it for this call and restore it afterwards */\n\nI realize that the bulk of this comment block was merely relocated\nfrom builtin/verify-tag.c, however, now would be a good time to fix\nits style violation and format it like this:\n\n    /*\n     * This is a multi-line\n     * comment.\n     */\n\nAlso, the last line of the comment, which you added when relocating\nit, merely repeats what the code itself already says clearly, thus is\nnot particularly useful and should be dropped.\n\n> +       sigchain_push(SIGPIPE, SIG_IGN);\n>         if (start_command(&gpg)) {\n>                 unlink(path);\n>                 return error(_(\"could not run gpg.\"));\n> @@ -250,6 +255,7 @@ int verify_signed_buffer(const char *payload, size_t payload_size,\n>         close(gpg.out);\n>\n>         ret = finish_command(&gpg);\n> +       sigchain_pop(SIGPIPE);\n>\n>         unlink_or_warn(path);\n"},{"id":"281772","messageId":"20160325053134.GA27614@sigill.intra.peff.net","threadId":"41821","inReplyTo":"CAPig+cQe5bwHXq4_qegBCM8Kqoqiz7K2ZtVk0FGMSEUPWQHyYA@mail.gmail.com","subject":"Re: [PATCH] tag.c: move PGP verification code from plumbing","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-03-25T05:31:34Z","receivedAt":"2016-03-25T05:31:34Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Mar 25, 2016 at 01:23:57AM -0400, Eric Sunshine wrote:\n\n> On Thu, Mar 24, 2016 at 8:33 PM,  <santiago@nyu.edu> wrote:\n> > The verify tag function is just a thin wrapper around the verify-tag\n> > command. We can avoid one fork call by doing the verification inside\n> > the tag builtin instead.\n> \n> Hopefully, the below review comments are meaningful, however, aside\n> from having just read Peff's review of the previous version of this\n> patch, I haven't been following this discussion, so it's possible some\n> comments may be off the mark. Caveat emptor.\n\nThanks for reviewing.  I agree with all of the comments you made, but\nI'd add a little more to the last one:\n\n> > +\n> > +       /* sometimes the program was terminated because this signal\n> > +        * was received in the process of writing the gpg input.\n> > +        * We ignore it for this call and restore it afterwards */\n> \n> I realize that the bulk of this comment block was merely relocated\n> from builtin/verify-tag.c, however, now would be a good time to fix\n> its style violation and format it like this:\n> \n>     /*\n>      * This is a multi-line\n>      * comment.\n>      */\n> \n> Also, the last line of the comment, which you added when relocating\n> it, merely repeats what the code itself already says clearly, thus is\n> not particularly useful and should be dropped.\n\nI actually think we can drop this comment entirely. Pushing and popping\nSIGPIPE when piping to a sub-program like this is not that exotic in our\ncode base. And if the SIGPIPE handling here is done in its own patch,\nthen if somebody wants to see more discussion or reasoning, they can go\nto its commit message (which can then go into more detail about why we\nmight see SIGPIPE, and not just a vague \"sometimes...\").\n\n-Peff\n"},{"id":"281816","messageId":"20160325144509.GA20375@LykOS","threadId":"41821","inReplyTo":"CAPig+cQe5bwHXq4_qegBCM8Kqoqiz7K2ZtVk0FGMSEUPWQHyYA@mail.gmail.com","subject":"Re: [PATCH] tag.c: move PGP verification code from plumbing","fromName":"Santiago Torres","fromEmail":"santiago@nyu.edu","sentAt":"2016-03-25T14:45:10Z","receivedAt":"2016-03-25T14:45:10Z","isPatch":true,"sender":{"key":"santiago@nyu.edu","avatar":"https://avatars.githubusercontent.com/u/3579933?v=4"},"body":"> > -       signal(SIGPIPE, SIG_IGN);\n> >         while (i < argc)\n> > -               if (verify_tag(argv[i++], flags))\n> > +               name = argv[i++];\n> > +               if (get_sha1(name, sha1))\n> > +                       return error(\"tag '%s' not found.\", name);\n> > +\n> > +               if (pgp_verify_tag(NULL, NULL, sha1, flags))\n> >                         had_error = 1;\n> \n> Meh, this isn't Python. Due to the missing braces, the only thing\n> inside the while() loop is the assignment to 'name'; all the other\n> indented code is outside the while().\n> \n> Did you run the test suite following this change? Did it all pass? If\n> so, perhaps an additional test or two to catch this sort of error\n> would be warranted.\n\nWow, you're right! I just re-ran the tests again to make sure I didn't\nmiss anything. All the tests pass for me, so I'll write an extra case to\navoid this. Just to be sure, I should include it in t7030-verify-tag.sh\nright?\n\nAll your other comments seem straightforward and on point. I'll apply\nthem right away :)\n\nThanks!\n-Santiago.\n"},{"id":"281901","messageId":"CAPig+cSQ2+yc6UCM08zMUDXKFgRBj5FUUEe+wFLxGkykE2yKxA@mail.gmail.com","threadId":"41821","inReplyTo":"20160325144509.GA20375@LykOS","subject":"Re: [PATCH] tag.c: move PGP verification code from plumbing","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2016-03-26T06:33:28Z","receivedAt":"2016-03-26T06:33:28Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Fri, Mar 25, 2016 at 10:45 AM, Santiago Torres <santiago@nyu.edu> wrote:\n>> >         while (i < argc)\n>> > -               if (verify_tag(argv[i++], flags))\n>> > +               name = argv[i++];\n>> > +               if (get_sha1(name, sha1))\n>> > +                       return error(\"tag '%s' not found.\", name);\n>> > +\n>> > +               if (pgp_verify_tag(NULL, NULL, sha1, flags))\n>> >                         had_error = 1;\n>>\n>> Meh, this isn't Python. Due to the missing braces, the only thing\n>> inside the while() loop is the assignment to 'name'; all the other\n>> indented code is outside the while().\n>>\n>> Did you run the test suite following this change? Did it all pass? If\n>> so, perhaps an additional test or two to catch this sort of error\n>> would be warranted.\n>\n> Wow, you're right! I just re-ran the tests again to make sure I didn't\n> miss anything. All the tests pass for me, so I'll write an extra case to\n> avoid this. Just to be sure, I should include it in t7030-verify-tag.sh\n> right?\n\nGenerally speaking, it is desirable to have test coverage for all\nfunctionality you're refactoring to ensure that the refactoring\ndoesn't break that functionality.\n\nt7030-verify-tag.sh indeed seems like a good place to add a new test\nfor catching this sort of regression.\n\nt7004-tag.sh is also of interest since, in an earlier version of this\npatch, if I recall correctly, Peff caught a regression where \"git tag\n-v\" failed to pass the GPG_VERIFY_VERBOSE flag, and I don't think\nt7004-tag.sh would have caught that problem either, so a new test for\nthat would be good, as well.\n\nSpeaking of GPG_VERIFY_VERBOSE, now that I'm examining the changes\nmore closely, it appears that this version of the patch no longer\nrespects GPG_VERIFY_VERBOSE at all but is instead hard-coded to be\nverbose. Is that desirable or intentional?\n"}]}