{"thread":{"id":"41934","subject":"[PATCH v5 0/6] tag: move PGP verification code to tag.c","startedAt":"2016-04-05T16:07:23Z","lastAt":"2016-04-17T18:53:28Z","messageCount":28,"participants":["santiago@nyu.edu","Santiago Torres","Eric Sunshine","Junio C Hamano"],"isPatch":true,"patchVersion":5,"patchTotal":6},"messages":[{"id":"282713","messageId":"1459872449-7537-1-git-send-email-santiago@nyu.edu","threadId":"41934","inReplyTo":null,"subject":"[PATCH v5 0/6] tag: move PGP verification code to tag.c","fromName":"","fromEmail":"santiago@nyu.edu","sentAt":"2016-04-05T16:07:23Z","receivedAt":"2016-04-05T16:07:23Z","isPatch":true,"sender":{"key":"santiago@nyu.edu","avatar":"https://avatars.githubusercontent.com/u/3579933?v=4"},"body":"From: Santiago Torres <santiago@nyu.edu>\n\nv5 (this): \nAdded helpful feedback by Eric\n\n * Reordering of the patches, to avoid temporal inclusion of a regression\n * Fix typos here and there.\n * Review commit messages, as some weren't representative of what the patches\n   were doing anymore.\n * Updated t7030 to include Peff's suggestion, and added a helped-by line here\n   as it was mostly Peff's code.\n * Updated the error-handling/printing issues that were introduced when.\n   libifying the verify_tag function.\n   \nv4:\n\nThanks Eric, Jeff, and Hannes for the feedback.\n\n * I relocated the sigchain_push call so it comes after the error on\n   gpg-interface (thanks Hannnes for catching this).\n * I updated the unit test to match the discussion on [3]. Now it generates\n   the expected output of the tag on the fly for comparison. (This is just\n   copy and paste from [3], but I verified that it works by breaking the\n   while)\n * I split moving the code and renaming the variables into two patches so\n   these are easier to review.\n * I used an adapter on builtin/tag.c instead of redefining all the fn*\n   declarations everywhere. This introduces an issue with the way git tag -v\n   resolves refnames though. I added a new commit to restore the previous\n   behavior of git-tag. I'm not sure if I should've split this into two commits\n   though.\n\nv3:\nThanks Eric, Jeff, for the feedback.\n\n * I separated the patch in multiple sub-patches.\n * I compared the behavior of previous git tag -v and git verify-tag \n   invocations to make sure the behavior is the same\n * I dropped the multi-line comment, as suggested.\n * I fixed the issue with the missing brackets in the while (this is \n   now detected by the test).\n\nv2:\n\n * I moved the pgp-verification code to tag.c \n * I added extra arguments so git tag -v and git verify-tag both work\n   with the same function\n * Relocated the SIGPIPE handling code in verify-tag to gpg-interface\n\nv1:\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\n\nThis applies on v2.8.0.\n\nThanks!\n-Santiago\n\n\nSantiago Torres (6):\n  builtin/verify-tag.c: Ignore SIGPIPE on gpg-interface\n  t7030-verify-tag: Adds validation for multiple tags\n  builtin/verify-tag: change variable name for readability\n  builtin/verify-tag: replace name argument with sha1\n  builtin/verify-tag: move verification code to tag.c\n  tag: use gpg_verify_function in tag -v call\n\n builtin/tag.c         |  8 +------\n builtin/verify-tag.c  | 65 +++++++++------------------------------------------\n gpg-interface.c       |  2 ++\n t/t7030-verify-tag.sh | 12 ++++++++++\n tag.c                 | 48 +++++++++++++++++++++++++++++++++++++\n tag.h                 |  1 +\n 6 files changed, 75 insertions(+), 61 deletions(-)\n\n-- \n2.8.0\n"},{"id":"282716","messageId":"1459872449-7537-2-git-send-email-santiago@nyu.edu","threadId":"41934","inReplyTo":"1459872449-7537-1-git-send-email-santiago@nyu.edu","subject":"[PATCH v5 1/6] builtin/verify-tag.c: Ignore SIGPIPE on gpg-interface","fromName":"","fromEmail":"santiago@nyu.edu","sentAt":"2016-04-05T16:07:24Z","receivedAt":"2016-04-05T16:07:24Z","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_signed_buffer comand might cause a SIGPIPE signal when the\ngpg child process terminates early (due to a bad keyid, for example) and\ngit tries to write to it afterwards. Previously, ignoring SIGPIPE was\ndone on the builtin/verify-tag.c command to avoid this issue. However,\nany other caller who wanted to use the verify_signed_buffer command\nwould have to include this signal call.\n\nInstead, we use sigchain_push(SIGPIPE, SIG_IGN) on the\nverify_signed_buffer call (pretty much like in sign_buffer()) so\nthat any caller is not required to perform this task. This will avoid\npossible mistakes by further developers using verify_signed_buffer.\n\nSigned-off-by: Santiago Torres <santiago@nyu.edu>\n---\n builtin/verify-tag.c | 3 ---\n gpg-interface.c      | 2 ++\n 2 files changed, 2 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/verify-tag.c b/builtin/verify-tag.c\nindex 00663f6..77f070a 100644\n--- a/builtin/verify-tag.c\n+++ b/builtin/verify-tag.c\n@@ -95,9 +95,6 @@ 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\t\thad_error = 1;\ndiff --git a/gpg-interface.c b/gpg-interface.c\nindex 3dc2fe3..2259938 100644\n--- a/gpg-interface.c\n+++ b/gpg-interface.c\n@@ -237,6 +237,7 @@ int verify_signed_buffer(const char *payload, size_t payload_size,\n \t\treturn error(_(\"could not run gpg.\"));\n \t}\n \n+\tsigchain_push(SIGPIPE, SIG_IGN);\n \twrite_in_full(gpg.in, payload, payload_size);\n \tclose(gpg.in);\n \n@@ -250,6 +251,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 \n-- \n2.8.0\n"},{"id":"282717","messageId":"1459872449-7537-3-git-send-email-santiago@nyu.edu","threadId":"41934","inReplyTo":"1459872449-7537-1-git-send-email-santiago@nyu.edu","subject":"[PATCH v5 2/6] t7030-verify-tag: Adds validation for multiple tags","fromName":"","fromEmail":"santiago@nyu.edu","sentAt":"2016-04-05T16:07:25Z","receivedAt":"2016-04-05T16:07:25Z","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 command supports multiple tag names as an argument.\nHowever, existing tests only test for invocation with a single tag, so\nwe add a test invoking with multiple tags.\n\nHelped-by: Jeff King <peff@peff.net>\nSigned-off-by: Santiago Torres <santiago@nyu.edu>\n---\n t/t7030-verify-tag.sh | 12 ++++++++++++\n 1 file changed, 12 insertions(+)\n\ndiff --git a/t/t7030-verify-tag.sh b/t/t7030-verify-tag.sh\nindex 4608e71..c01621a 100755\n--- a/t/t7030-verify-tag.sh\n+++ b/t/t7030-verify-tag.sh\n@@ -112,4 +112,16 @@ test_expect_success GPG 'verify signatures with --raw' '\n \t)\n '\n \n+test_expect_success GPG 'verify multiple tags' '\n+\ttags=\"fourth-signed sixth-signed seventh-signed\" &&\n+\tfor i in $tags; do\n+\t\tgit verify-tag -v --raw $i || return 1\n+\tdone >expect.stdout 2>expect.stderr.1 &&\n+\tgrep \"^.GNUPG\" <expect.stderr.1 >expect.stderr &&\n+\tgit verify-tag -v --raw $tags >actual.stdout 2>actual.stderr.1 &&\n+\tgrep \"^.GNUPG\" <actual.stderr.1 >actual.stderr &&\n+\ttest_cmp expect.stdout actual.stdout &&\n+\ttest_cmp expect.stderr actual.stderr\n+'\n+\n test_done\n-- \n2.8.0\n"},{"id":"282718","messageId":"1459872449-7537-4-git-send-email-santiago@nyu.edu","threadId":"41934","inReplyTo":"1459872449-7537-1-git-send-email-santiago@nyu.edu","subject":"[PATCH v5 3/6] builtin/verify-tag: change variable name for readability","fromName":"","fromEmail":"santiago@nyu.edu","sentAt":"2016-04-05T16:07:26Z","receivedAt":"2016-04-05T16:07:26Z","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 run_gpg_verify function has two variables size, and len. This may\ncome off as confusing when reading the code. We clarify which one\npertains to the length of the tag headers by renaming len to\npayload_length.\n\nSigned-off-by: Santiago Torres <santiago@nyu.edu>\n---\n builtin/verify-tag.c | 11 ++++++-----\n 1 file changed, 6 insertions(+), 5 deletions(-)\n\ndiff --git a/builtin/verify-tag.c b/builtin/verify-tag.c\nindex 77f070a..1ca9a05 100644\n--- a/builtin/verify-tag.c\n+++ b/builtin/verify-tag.c\n@@ -21,20 +21,21 @@ static const char * const verify_tag_usage[] = {\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 payload_size;\n \tint ret;\n \n \tmemset(&sigc, 0, sizeof(sigc));\n \n-\tlen = parse_signature(buf, size);\n+\tpayload_size = parse_signature(buf, size);\n \n-\tif (size == len) {\n+\tif (size == payload_size) {\n \t\tif (flags & GPG_VERIFY_VERBOSE)\n-\t\t\twrite_in_full(1, buf, len);\n+\t\t\twrite_in_full(1, buf, payload_size);\n \t\treturn error(\"no signature found\");\n \t}\n \n-\tret = check_signature(buf, len, buf + len, size - len, &sigc);\n+\tret = check_signature(buf, payload_size, buf + payload_size,\n+\t\t\t\tsize - payload_size, &sigc);\n \tprint_signature_buffer(&sigc, flags);\n \n \tsignature_check_clear(&sigc);\n-- \n2.8.0\n"},{"id":"282715","messageId":"1459872449-7537-5-git-send-email-santiago@nyu.edu","threadId":"41934","inReplyTo":"1459872449-7537-1-git-send-email-santiago@nyu.edu","subject":"[PATCH v5 4/6] builtin/verify-tag: replace name argument with sha1","fromName":"","fromEmail":"santiago@nyu.edu","sentAt":"2016-04-05T16:07:27Z","receivedAt":"2016-04-05T16:07:27Z","isPatch":true,"sender":{"key":"santiago@nyu.edu","avatar":"https://avatars.githubusercontent.com/u/3579933?v=4"},"body":"From: Santiago Torres <santiago@nyu.edu>\n\nThis change is meant to prepare verify_tag for libification. Many\nexisting modules/commands already do the refname to sha1 resolution, so\nshould avoid resolving the refname twice. To avoid breaking\nbuiltin/verify-tag, we move the refname resolution outside of the\nverify_tag() call.\n\nSigned-off-by: Santiago Torres <santiago@nyu.edu>\n---\n builtin/verify-tag.c | 25 ++++++++++++++++---------\n 1 file changed, 16 insertions(+), 9 deletions(-)\n\ndiff --git a/builtin/verify-tag.c b/builtin/verify-tag.c\nindex 1ca9a05..7a7c376 100644\n--- a/builtin/verify-tag.c\n+++ b/builtin/verify-tag.c\n@@ -42,25 +42,23 @@ static int run_gpg_verify(const char *buf, unsigned long size, unsigned flags)\n \treturn ret;\n }\n \n-static int verify_tag(const char *name, unsigned flags)\n+static int verify_tag(const unsigned char *sha1, unsigned flags)\n {\n \tenum object_type type;\n-\tunsigned char sha1[20];\n \tchar *buf;\n+\tchar *hex_sha1;\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+\thex_sha1 = sha1_to_hex(sha1);\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+\t\t\t\thex_sha1, 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+\t\treturn error(\"%s: unable to read file.\", hex_sha1);\n \n \tret = run_gpg_verify(buf, size, flags);\n \n@@ -80,6 +78,8 @@ int cmd_verify_tag(int argc, const char **argv, const char *prefix)\n {\n \tint i = 1, verbose = 0, had_error = 0;\n \tunsigned flags = 0;\n+\tunsigned char sha1[20];\n+\tconst char *name;\n \tconst struct option verify_tag_options[] = {\n \t\tOPT__VERBOSE(&verbose, N_(\"print tag contents\")),\n \t\tOPT_BIT(0, \"raw\", &flags, N_(\"print raw gpg status output\"), GPG_VERIFY_RAW),\n@@ -96,8 +96,15 @@ int cmd_verify_tag(int argc, const char **argv, const char *prefix)\n \tif (verbose)\n \t\tflags |= GPG_VERIFY_VERBOSE;\n \n-\twhile (i < argc)\n-\t\tif (verify_tag(argv[i++], flags))\n+\twhile (i < argc) {\n+\t\tname = argv[i++];\n+\t\tif (get_sha1(name, sha1)) {\n+\t\t\terror(\"tag '%s' not found.\", name);\n \t\t\thad_error = 1;\n+\t\t\tcontinue;\n+\t\t}\n+\t\tif (verify_tag(sha1, flags))\n+\t\t\thad_error = 1;\n+\t}\n \treturn had_error;\n }\n-- \n2.8.0\n"},{"id":"282719","messageId":"1459872449-7537-6-git-send-email-santiago@nyu.edu","threadId":"41934","inReplyTo":"1459872449-7537-1-git-send-email-santiago@nyu.edu","subject":"[PATCH v5 5/6] builtin/verify-tag: move verification code to tag.c","fromName":"","fromEmail":"santiago@nyu.edu","sentAt":"2016-04-05T16:07:28Z","receivedAt":"2016-04-05T16:07:28Z","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 PGP verification routine for tags could be accessed by other\ncommands that require it. We do this by moving it to the common tag.c\nmodule. We rename the verify_tag() function to gpg_verify_tag() to avoid\nconflicts with the mktag.c function.\n\nSigned-off-by: Santiago Torres <santiago@nyu.edu>\n---\n builtin/verify-tag.c | 50 +-------------------------------------------------\n tag.c                | 48 ++++++++++++++++++++++++++++++++++++++++++++++++\n tag.h                |  1 +\n 3 files changed, 50 insertions(+), 49 deletions(-)\n\ndiff --git a/builtin/verify-tag.c b/builtin/verify-tag.c\nindex 7a7c376..e9a2005 100644\n--- a/builtin/verify-tag.c\n+++ b/builtin/verify-tag.c\n@@ -18,54 +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 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\tif (flags & GPG_VERIFY_VERBOSE)\n-\t\t\twrite_in_full(1, buf, payload_size);\n-\t\treturn error(\"no signature found\");\n-\t}\n-\n-\tret = check_signature(buf, payload_size, buf + payload_size,\n-\t\t\t\tsize - payload_size, &sigc);\n-\tprint_signature_buffer(&sigc, flags);\n-\n-\tsignature_check_clear(&sigc);\n-\treturn ret;\n-}\n-\n-static int verify_tag(const unsigned char *sha1, unsigned flags)\n-{\n-\tenum object_type type;\n-\tchar *buf;\n-\tchar *hex_sha1;\n-\tunsigned long size;\n-\tint ret;\n-\n-\thex_sha1 = sha1_to_hex(sha1);\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\thex_sha1, typename(type));\n-\n-\tbuf = read_sha1_file(sha1, &type, &size);\n-\tif (!buf)\n-\t\treturn error(\"%s: unable to read file.\", hex_sha1);\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@@ -103,7 +55,7 @@ int cmd_verify_tag(int argc, const char **argv, const char *prefix)\n \t\t\thad_error = 1;\n \t\t\tcontinue;\n \t\t}\n-\t\tif (verify_tag(sha1, flags))\n+\t\tif (gpg_verify_tag(sha1, flags))\n \t\t\thad_error = 1;\n \t}\n \treturn had_error;\ndiff --git a/tag.c b/tag.c\nindex d72f742..3f7669f 100644\n--- a/tag.c\n+++ b/tag.c\n@@ -6,6 +6,54 @@\n \n const char *tag_type = \"tag\";\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\tif (flags & GPG_VERIFY_VERBOSE)\n+\t\t\twrite_in_full(1, buf, payload_size);\n+\t\treturn error(\"no signature found\");\n+\t}\n+\n+\tret = check_signature(buf, payload_size, buf + payload_size,\n+\t\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 gpg_verify_tag(const unsigned char *sha1, unsigned flags)\n+{\n+\tenum object_type type;\n+\tchar *buf;\n+\tchar *hex_sha1;\n+\tunsigned long size;\n+\tint ret;\n+\n+\thex_sha1 = sha1_to_hex(sha1);\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\thex_sha1, typename(type));\n+\n+\tbuf = read_sha1_file(sha1, &type, &size);\n+\tif (!buf)\n+\t\treturn error(\"%s: unable to read file.\", hex_sha1);\n+\n+\tret = run_gpg_verify(buf, size, flags);\n+\n+\tfree(buf);\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..cb643b9 100644\n--- a/tag.h\n+++ b/tag.h\n@@ -17,5 +17,6 @@ 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 gpg_verify_tag(const unsigned char *sha1, unsigned flags);\n \n #endif /* TAG_H */\n-- \n2.8.0\n"},{"id":"282714","messageId":"1459872449-7537-7-git-send-email-santiago@nyu.edu","threadId":"41934","inReplyTo":"1459872449-7537-1-git-send-email-santiago@nyu.edu","subject":"[PATCH v5 6/6] tag: use gpg_verify_function in tag -v call","fromName":"","fromEmail":"santiago@nyu.edu","sentAt":"2016-04-05T16:07:29Z","receivedAt":"2016-04-05T16:07:29Z","isPatch":true,"sender":{"key":"santiago@nyu.edu","avatar":"https://avatars.githubusercontent.com/u/3579933?v=4"},"body":"From: Santiago Torres <santiago@nyu.edu>\n\nInstead of running the verify-tag plumbing command, we use the\ngpg_verify_tag() function within the verify_tag function to avoid doing\nan additional fork call.\n\nSigned-off-by: Santiago Torres <santiago@nyu.edu>\n---\n builtin/tag.c | 8 +-------\n 1 file changed, 1 insertion(+), 7 deletions(-)\n\ndiff --git a/builtin/tag.c b/builtin/tag.c\nindex 1705c94..398c892 100644\n--- a/builtin/tag.c\n+++ b/builtin/tag.c\n@@ -104,13 +104,7 @@ static int delete_tag(const char *name, const char *ref,\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+\treturn gpg_verify_tag(sha1, GPG_VERIFY_VERBOSE);\n }\n \n static int do_sign(struct strbuf *buffer)\n-- \n2.8.0\n"},{"id":"282721","messageId":"20160405164402.GB9507@LykOS","threadId":"41934","inReplyTo":"1459872449-7537-1-git-send-email-santiago@nyu.edu","subject":"Re: [PATCH v5 0/6] tag: move PGP verification code to tag.c","fromName":"Santiago Torres","fromEmail":"santiago@nyu.edu","sentAt":"2016-04-05T16:44:03Z","receivedAt":"2016-04-05T16:44:03Z","isPatch":true,"sender":{"key":"santiago@nyu.edu","avatar":"https://avatars.githubusercontent.com/u/3579933?v=4"},"body":"Sorry, forgot to add, this is a follow on to [1].\n\nThanks,\n-Santiago.\n\n[1]:\nhttp://git.661346.n2.nabble.com/PATCH-v4-0-6-tag-move-PGP-verification-code-to-tag-c-td7652451.html\nOn Tue, Apr 05, 2016 at 12:07:23PM -0400, santiago@nyu.edu wrote:\n> From: Santiago Torres <santiago@nyu.edu>\n> \n> v5 (this): \n> Added helpful feedback by Eric\n> \n>  * Reordering of the patches, to avoid temporal inclusion of a regression\n>  * Fix typos here and there.\n>  * Review commit messages, as some weren't representative of what the patches\n>    were doing anymore.\n>  * Updated t7030 to include Peff's suggestion, and added a helped-by line here\n>    as it was mostly Peff's code.\n>  * Updated the error-handling/printing issues that were introduced when.\n>    libifying the verify_tag function.\n>    \n> v4:\n> \n> Thanks Eric, Jeff, and Hannes for the feedback.\n> \n>  * I relocated the sigchain_push call so it comes after the error on\n>    gpg-interface (thanks Hannnes for catching this).\n>  * I updated the unit test to match the discussion on [3]. Now it generates\n>    the expected output of the tag on the fly for comparison. (This is just\n>    copy and paste from [3], but I verified that it works by breaking the\n>    while)\n>  * I split moving the code and renaming the variables into two patches so\n>    these are easier to review.\n>  * I used an adapter on builtin/tag.c instead of redefining all the fn*\n>    declarations everywhere. This introduces an issue with the way git tag -v\n>    resolves refnames though. I added a new commit to restore the previous\n>    behavior of git-tag. I'm not sure if I should've split this into two commits\n>    though.\n> \n> v3:\n> Thanks Eric, Jeff, for the feedback.\n> \n>  * I separated the patch in multiple sub-patches.\n>  * I compared the behavior of previous git tag -v and git verify-tag \n>    invocations to make sure the behavior is the same\n>  * I dropped the multi-line comment, as suggested.\n>  * I fixed the issue with the missing brackets in the while (this is \n>    now detected by the test).\n> \n> v2:\n> \n>  * I moved the pgp-verification code to tag.c \n>  * I added extra arguments so git tag -v and git verify-tag both work\n>    with the same function\n>  * Relocated the SIGPIPE handling code in verify-tag to gpg-interface\n> \n> v1:\n>  \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> \n> This applies on v2.8.0.\n> \n> Thanks!\n> -Santiago\n> \n> \n> Santiago Torres (6):\n>   builtin/verify-tag.c: Ignore SIGPIPE on gpg-interface\n>   t7030-verify-tag: Adds validation for multiple tags\n>   builtin/verify-tag: change variable name for readability\n>   builtin/verify-tag: replace name argument with sha1\n>   builtin/verify-tag: move verification code to tag.c\n>   tag: use gpg_verify_function in tag -v call\n> \n>  builtin/tag.c         |  8 +------\n>  builtin/verify-tag.c  | 65 +++++++++------------------------------------------\n>  gpg-interface.c       |  2 ++\n>  t/t7030-verify-tag.sh | 12 ++++++++++\n>  tag.c                 | 48 +++++++++++++++++++++++++++++++++++++\n>  tag.h                 |  1 +\n>  6 files changed, 75 insertions(+), 61 deletions(-)\n> \n> -- \n> 2.8.0\n> \n"},{"id":"282749","messageId":"CAPig+cSOLnCoX77O7khEX1cTh0Hu29d6MRdcpfN8ytTkUSVU=A@mail.gmail.com","threadId":"41934","inReplyTo":"1459872449-7537-3-git-send-email-santiago@nyu.edu","subject":"Re: [PATCH v5 2/6] t7030-verify-tag: Adds validation for multiple tags","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2016-04-06T06:21:38Z","receivedAt":"2016-04-06T06:21:38Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, Apr 5, 2016 at 12:07 PM,  <santiago@nyu.edu> wrote:\n> t7030-verify-tag: Adds validation for multiple tags\n\ns/t7030-verify-tag/t7030/\ns/Adds/add\n\n> The verify-tag command supports multiple tag names as an argument.\n> However, existing tests only test for invocation with a single tag, so\n> we add a test invoking with multiple tags.\n>\n> Helped-by: Jeff King <peff@peff.net>\n> Signed-off-by: Santiago Torres <santiago@nyu.edu>\n> ---\n> diff --git a/t/t7030-verify-tag.sh b/t/t7030-verify-tag.sh\n> @@ -112,4 +112,16 @@ test_expect_success GPG 'verify signatures with --raw' '\n> +test_expect_success GPG 'verify multiple tags' '\n> +       tags=\"fourth-signed sixth-signed seventh-signed\" &&\n> +       for i in $tags; do\n\nStyle: 'do' on its own line; drop semicolon\n\n    for i in ...\n    do\n        ...\n    done\n\n> +               git verify-tag -v --raw $i || return 1\n> +       done >expect.stdout 2>expect.stderr.1 &&\n> +       grep \"^.GNUPG\" <expect.stderr.1 >expect.stderr &&\n\nHmm, is there a reason you didn't stick with the more strict regex\nPeff suggested?\n\n    ^.GNUPG:.\n\n(Genuine question: I'm not saying your choice is wrong, I'm just\ninterested in the reasoning behind the decision.)\n\n> +       git verify-tag -v --raw $tags >actual.stdout 2>actual.stderr.1 &&\n> +       grep \"^.GNUPG\" <actual.stderr.1 >actual.stderr &&\n> +       test_cmp expect.stdout actual.stdout &&\n> +       test_cmp expect.stderr actual.stderr\n> +'\n> +\n>  test_done\n"},{"id":"282752","messageId":"CAPig+cQ_R+YJVAsDDDPBHRZ=Un=34kum096YeFrGOo3_pU-g3A@mail.gmail.com","threadId":"41934","inReplyTo":"1459872449-7537-5-git-send-email-santiago@nyu.edu","subject":"Re: [PATCH v5 4/6] builtin/verify-tag: replace name argument with sha1","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2016-04-06T06:56:36Z","receivedAt":"2016-04-06T06:56:36Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, Apr 5, 2016 at 12:07 PM,  <santiago@nyu.edu> wrote:\n> builtin/verify-tag: replace name argument with sha1\n\n    builtin/verify-tag: prepare verify_tag() for libification\n\n> This change is meant to prepare verify_tag for libification. Many\n> existing modules/commands already do the refname to sha1 resolution, so\n> should avoid resolving the refname twice.\n\nIf I hadn't already understood the purpose of the patch, I think I'd\nstill be somewhat clueless after reading this because it doesn't do a\nthorough job of explaining what the actual problem is that this is\nsolving. Perhaps something like this might be better:\n\n    verify_tag() accepts a tag name which it resolves to a SHA1\n    before verification, however, the plan is to make this\n    functionality public and it is possible that future callers will\n    already have a SHA1 in hand. Since it would be wasteful for them\n    to supply a tag name only to have it resolved again, change\n    verify_tag() to accept a tag SHA1 rather than a name.\n\n> To avoid breaking\n> builtin/verify-tag, we move the refname resolution outside of the\n> verify_tag() call.\n\nThe reasonably intelligent reader should understand implicitly that\nthis is a natural consequence of changing the signature of\nverify_tag(), thus it's not really necessary to state it explicitly.\n(It makes the commit message noisier without adding value.)\n\nMore below...\n\n> Signed-off-by: Santiago Torres <santiago@nyu.edu>\n> ---\n> diff --git a/builtin/verify-tag.c b/builtin/verify-tag.c\n> @@ -42,25 +42,23 @@ static int run_gpg_verify(const char *buf, unsigned long size, unsigned flags)\n> -static int verify_tag(const char *name, unsigned flags)\n> +static int verify_tag(const unsigned char *sha1, unsigned flags)\n>  {\n>         enum object_type type;\n> -       unsigned char sha1[20];\n>         char *buf;\n> +       char *hex_sha1;\n>         unsigned long size;\n>         int ret;\n>\n> -       if (get_sha1(name, sha1))\n> -               return error(\"tag '%s' not found.\", name);\n> -\n> +       hex_sha1 = sha1_to_hex(sha1);\n>         type = sha1_object_info(sha1, NULL);\n>         if (type != OBJ_TAG)\n>                 return error(\"%s: cannot verify a non-tag object of type %s.\",\n> -                               name, typename(type));\n> +                               hex_sha1, typename(type));\n>\n>         buf = read_sha1_file(sha1, &type, &size);\n>         if (!buf)\n> -               return error(\"%s: unable to read file.\", name);\n> +               return error(\"%s: unable to read file.\", hex_sha1);\n\nNit: sha1_to_hex() gets invoked *always*, even when there is no error.\nAn alternative would be to call it within each error() invocation,\nwhen it's actually needed.\n\n    return error(\"%s: unable to read file.\", sha1_to_hex(sha1));\n\n> @@ -96,8 +96,15 @@ int cmd_verify_tag(int argc, const char **argv, const char *prefix)\n> -       while (i < argc)\n> -               if (verify_tag(argv[i++], flags))\n> +       while (i < argc) {\n> +               name = argv[i++];\n> +               if (get_sha1(name, sha1)) {\n> +                       error(\"tag '%s' not found.\", name);\n>                         had_error = 1;\n> +                       continue;\n> +               }\n> +               if (verify_tag(sha1, flags))\n> +                       had_error = 1;\n\nAn alternative without 'continue':\n\n    if (get_sha1(...)) {\n        error(\"tag ...\");\n        had_error = 1;\n    } else if (verify_tag(...))\n        had_error = 1;\n\nI don't feel strongly about it, and it's certainly not worth a re-roll.\n\n> +       }\n>         return had_error;\n>  }\n"},{"id":"282754","messageId":"CAPig+cS8hc7G7gyivVFNDkpWpGu_sr1YyJUoMYUTWvigxH+U2g@mail.gmail.com","threadId":"41934","inReplyTo":"1459872449-7537-7-git-send-email-santiago@nyu.edu","subject":"Re: [PATCH v5 6/6] tag: use gpg_verify_function in tag -v call","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2016-04-06T07:09:13Z","receivedAt":"2016-04-06T07:09:13Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, Apr 5, 2016 at 12:07 PM,  <santiago@nyu.edu> wrote:\n> tag: use gpg_verify_function in tag -v call\n\nThis is a low-level description of what the patch is doing, but you\nnormally want the subject to present a high-level overview. Perhaps\nsomething like:\n\n    tag -v: verify directly rather than exec'ing git-verify-tag\n\n> Instead of running the verify-tag plumbing command, we use the\n> gpg_verify_tag() function within the verify_tag function to avoid doing\n> an additional fork call.\n\nI'm a bit confused about \"gpg_verify_tag() function within the\nverify_tag function\". I *think* you can drop the \"within the\nverify_tag function\" bit.\n\n> Signed-off-by: Santiago Torres <santiago@nyu.edu>\n> ---\n>  builtin/tag.c | 8 +-------\n>  1 file changed, 1 insertion(+), 7 deletions(-)\n>\n> diff --git a/builtin/tag.c b/builtin/tag.c\n> index 1705c94..398c892 100644\n> --- a/builtin/tag.c\n> +++ b/builtin/tag.c\n> @@ -104,13 +104,7 @@ static int delete_tag(const char *name, const char *ref,\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> +       return gpg_verify_tag(sha1, GPG_VERIFY_VERBOSE);\n>  }\n>\n>  static int do_sign(struct strbuf *buffer)\n> --\n> 2.8.0\n"},{"id":"282755","messageId":"CAPig+cQ4n5j4Q-WF-0cd=2+5eSAaimh3A7La+8Fe9Ox4anjtBQ@mail.gmail.com","threadId":"41934","inReplyTo":"1459872449-7537-1-git-send-email-santiago@nyu.edu","subject":"Re: [PATCH v5 0/6] tag: move PGP verification code to tag.c","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2016-04-06T07:18:38Z","receivedAt":"2016-04-06T07:18:38Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, Apr 5, 2016 at 12:07 PM,  <santiago@nyu.edu> wrote:\n> v5 (this):\n> Added helpful feedback by Eric\n>\n>  * Reordering of the patches, to avoid temporal inclusion of a regression\n>  * Fix typos here and there.\n>  * Review commit messages, as some weren't representative of what the patches\n>    were doing anymore.\n>  * Updated t7030 to include Peff's suggestion, and added a helped-by line here\n>    as it was mostly Peff's code.\n>  * Updated the error-handling/printing issues that were introduced when.\n>    libifying the verify_tag function.\n\nThis version is a more pleasant read, easier to digest and understand.\nAll of my review comments were minor; nothing demanding a re-roll. As\nsuch, this version is:\n\n    Reviewed-by: Eric Sunshine <sunshine@sunshineco.com>\n\nIf you do happen to re-roll based upon the review comments, feel free\nto add my Reviewed-by: (but not if you make larger changes).\n\nThanks.\n"},{"id":"282769","messageId":"xmqqa8l6zoqc.fsf@gitster.mtv.corp.google.com","threadId":"41934","inReplyTo":"1459872449-7537-5-git-send-email-santiago@nyu.edu","subject":"Re: [PATCH v5 4/6] builtin/verify-tag: replace name argument with sha1","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-04-06T16:27:55Z","receivedAt":"2016-04-06T16:27:55Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"santiago@nyu.edu writes:\n\n> From: Santiago Torres <santiago@nyu.edu>\n>\n> This change is meant to prepare verify_tag for libification. Many\n> existing modules/commands already do the refname to sha1 resolution, so\n> should avoid resolving the refname twice. To avoid breaking\n> builtin/verify-tag, we move the refname resolution outside of the\n> verify_tag() call.\n>\n> Signed-off-by: Santiago Torres <santiago@nyu.edu>\n> ---\n>  builtin/verify-tag.c | 25 ++++++++++++++++---------\n>  1 file changed, 16 insertions(+), 9 deletions(-)\n>\n> diff --git a/builtin/verify-tag.c b/builtin/verify-tag.c\n> index 1ca9a05..7a7c376 100644\n> --- a/builtin/verify-tag.c\n> +++ b/builtin/verify-tag.c\n> @@ -42,25 +42,23 @@ static int run_gpg_verify(const char *buf, unsigned long size, unsigned flags)\n>  \treturn ret;\n>  }\n>  \n> -static int verify_tag(const char *name, unsigned flags)\n> +static int verify_tag(const unsigned char *sha1, unsigned flags)\n>  {\n>  \tenum object_type type;\n> -\tunsigned char sha1[20];\n>  \tchar *buf;\n> +\tchar *hex_sha1;\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> +\thex_sha1 = sha1_to_hex(sha1);\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> +\t\t\t\thex_sha1, typename(type));\n\nSo, if I said\n\n    $ git verify-tag master\n\nthe code used to take \"master\" from argv[], fed it to verify_tag()\nas parameter 'name', turned it to the object name of the commit,\nnoticed that it is not a tag, and complained that \"master: cannot verify\".\n\nWith this rewrite, the same invocation would cause \"master\" to be\nturned into the object name, which is passed to verify_tag() and the\ncomplaint is an overlong\n\n    76bece327f490cb344b95ae8f869cbeb89a4d20b: cannot verify a non-tag object of type commit\n\nThat does not sound like a good change at all.\n\nIf you want to support a future caller of a libified version of\nverify_tag() that has a raw object name but not the original name,\nI'd suggest to make this function keep parameter 'name' while adding\nthe new parameter 'sha1'.  Then, the error reporting may become:\n\n\treturn error(\"%s: cannot verify a non-tag object of type '%s'\",\n\t\t     name ? name : sha1_to_hex(sha1), typename(type));\n\nand the output would still be useful.  Further improvements may be\n\n - rename 'name' to 'report_name' to clarify that the parameter is\n   only used for reporting, and that the tag object to verify is\n   always identified by the new 'sha1' parameter.\n\n - use find_unique_abbrev() to shorten the fallback name displayed\n   in the error message.\n"},{"id":"282770","messageId":"xmqq60vuzohe.fsf@gitster.mtv.corp.google.com","threadId":"41934","inReplyTo":"1459872449-7537-6-git-send-email-santiago@nyu.edu","subject":"Re: [PATCH v5 5/6] builtin/verify-tag: move verification code to tag.c","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-04-06T16:33:17Z","receivedAt":"2016-04-06T16:33:17Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"santiago@nyu.edu writes:\n\n> From: Santiago Torres <santiago@nyu.edu>\n>\n> The PGP verification routine for tags could be accessed by other\n> commands that require it. We do this by moving it to the common tag.c\n> module. We rename the verify_tag() function to gpg_verify_tag() to avoid\n> conflicts with the mktag.c function.\n\nAfter outlining the observation and opinion, we describe the changes\nas if we are ordering somebody to \"do this, do that\" to the codebase,\nlike so:\n\n\t... require it.\n\n        Move the function to tag.c and make it public, and rename it\n\tgpg_verify_tag() to make its name more descriptive and also\n\tavoid conflicts with a static function in builtin/mktag.c.\n"},{"id":"282772","messageId":"xmqqy48qy9mc.fsf@gitster.mtv.corp.google.com","threadId":"41934","inReplyTo":"1459872449-7537-1-git-send-email-santiago@nyu.edu","subject":"Re: [PATCH v5 0/6] tag: move PGP verification code to tag.c","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-04-06T16:39:39Z","receivedAt":"2016-04-06T16:39:39Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"The first three looked almost good.  I do not fully agree with what\n4/6 does, and update to its function signature is likely to require\nupdates to 5/6 and 6/6.\n\nThanks.\n"},{"id":"282773","messageId":"xmqqpou2y9fy.fsf@gitster.mtv.corp.google.com","threadId":"41934","inReplyTo":"1459872449-7537-2-git-send-email-santiago@nyu.edu","subject":"Re: [PATCH v5 1/6] builtin/verify-tag.c: Ignore SIGPIPE on gpg-interface","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-04-06T16:43:29Z","receivedAt":"2016-04-06T16:43:29Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"santiago@nyu.edu writes:\n\n> Subject: [PATCH v5 1/6] builtin/verify-tag.c: Ignore SIGPIPE on gpg-interface\n\ns/Ignore/ignore/\n\n> From: Santiago Torres <santiago@nyu.edu>\n>\n> The verify_signed_buffer comand might cause a SIGPIPE signal when the\n> gpg child process terminates early (due to a bad keyid, for example) and\n> git tries to write to it afterwards. Previously, ignoring SIGPIPE was\n> done on the builtin/verify-tag.c command to avoid this issue. However,\n> any other caller who wanted to use the verify_signed_buffer command\n> would have to include this signal call.\n\ns/comand/command/ but more importantly, it is not a command ;-)\n\n> Instead, we use sigchain_push(SIGPIPE, SIG_IGN) on the\n> verify_signed_buffer call (pretty much like in sign_buffer()) so\n> that any caller is not required to perform this task. This will avoid\n> possible mistakes by further developers using verify_signed_buffer.\n\nI'd rephrase like this if I were doing this patch:\n\n    verify-tag: ignore SIGPIPE in verify_signed_buffer()\n\n    The verify_signed_buffer() function may trigger a SIGPIPE when\n    the GPG child process terminates early (due to a bad keyid, for\n    example) and we try to write to it afterwards.  Ignoring SIGPIPE\n    is done in builtin/verify-tag.c to avoid dying from it, but any\n    other caller who wants to call verify_signed_buffer() would have\n    to do the same.\n\n    Use sigchain_push(SIGPIPE, SIG_IGN) in verify_signed_buffer(),\n    pretty much like in sign_buffer(), so that any caller is not\n    required to perform this task.\n\n    This will avoid possible mistakes by further developers using\n    verify_signed_buffer().\n\nThe patch text (as I already said in the response to the cover\nletter) looked fine.\n"},{"id":"282774","messageId":"xmqqk2kay9cq.fsf@gitster.mtv.corp.google.com","threadId":"41934","inReplyTo":"1459872449-7537-3-git-send-email-santiago@nyu.edu","subject":"Re: [PATCH v5 2/6] t7030-verify-tag: Adds validation for multiple tags","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-04-06T16:45:25Z","receivedAt":"2016-04-06T16:45:25Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"santiago@nyu.edu writes:\n\n> From: Santiago Torres <santiago@nyu.edu>\n>\n> The verify-tag command supports multiple tag names as an argument.\n\nThat makes it sound as if this is a valid input\n\n\t$ git verify-tag \"one two three four\"\n\nbut that is not what you meant.\n\n> However, existing tests only test for invocation with a single tag, so\n> we add a test invoking with multiple tags.\n\nI'd rephrase the above like so:\n\n    t7030: test verifying multiple tags\n\n    The verify-tag command accepts multiple tag names to verify, but\n    existing tests only test for invocation with a single tag.\n\n    Add a test invoking it with multiple tags.\n\nI had the same (minor) problem with Eric on the patch text itself.\n\n>\n> Helped-by: Jeff King <peff@peff.net>\n> Signed-off-by: Santiago Torres <santiago@nyu.edu>\n> ---\n>  t/t7030-verify-tag.sh | 12 ++++++++++++\n>  1 file changed, 12 insertions(+)\n>\n> diff --git a/t/t7030-verify-tag.sh b/t/t7030-verify-tag.sh\n> index 4608e71..c01621a 100755\n> --- a/t/t7030-verify-tag.sh\n> +++ b/t/t7030-verify-tag.sh\n> @@ -112,4 +112,16 @@ test_expect_success GPG 'verify signatures with --raw' '\n>  \t)\n>  '\n>  \n> +test_expect_success GPG 'verify multiple tags' '\n> +\ttags=\"fourth-signed sixth-signed seventh-signed\" &&\n> +\tfor i in $tags; do\n> +\t\tgit verify-tag -v --raw $i || return 1\n> +\tdone >expect.stdout 2>expect.stderr.1 &&\n> +\tgrep \"^.GNUPG\" <expect.stderr.1 >expect.stderr &&\n> +\tgit verify-tag -v --raw $tags >actual.stdout 2>actual.stderr.1 &&\n> +\tgrep \"^.GNUPG\" <actual.stderr.1 >actual.stderr &&\n> +\ttest_cmp expect.stdout actual.stdout &&\n> +\ttest_cmp expect.stderr actual.stderr\n> +'\n> +\n>  test_done\n"},{"id":"282775","messageId":"xmqqfuuyy9a5.fsf@gitster.mtv.corp.google.com","threadId":"41934","inReplyTo":"1459872449-7537-4-git-send-email-santiago@nyu.edu","subject":"Re: [PATCH v5 3/6] builtin/verify-tag: change variable name for readability","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-04-06T16:46:58Z","receivedAt":"2016-04-06T16:46:58Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"santiago@nyu.edu writes:\n\n> From: Santiago Torres <santiago@nyu.edu>\n>\n> The run_gpg_verify function has two variables size, and len. \n\nI somehow found this harder to parse than necessary.\n\n> This may come off as confusing when reading the code. We clarify which one\n> pertains to the length of the tag headers by renaming len to\n> payload_length.\n\nI'd rephrase it like so:\n\n    verify-tag: change variable name for readability\n\n    The run_gpg_verify() function has two variables, size and len.\n    This may come off as confusing when reading the code.\n\n    Clarify which one pertains to the length of the tag headers by\n    renaming len to payload_length.\n\nThe patch text looked good.\n"},{"id":"282825","messageId":"20160407033313.GA17848@LykOS","threadId":"41934","inReplyTo":"xmqqy48qy9mc.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v5 0/6] tag: move PGP verification code to tag.c","fromName":"Santiago Torres","fromEmail":"santiago@nyu.edu","sentAt":"2016-04-07T03:33:14Z","receivedAt":"2016-04-07T03:33:14Z","isPatch":true,"sender":{"key":"santiago@nyu.edu","avatar":"https://avatars.githubusercontent.com/u/3579933?v=4"},"body":"Thanks for the reviews!\n\nI'll update the commit messages. It appears that I there's a lot to\nlearn on my side on the art of writing good commit messages.\n\n-Santiago.\n\nOn Wed, Apr 06, 2016 at 09:39:39AM -0700, Junio C Hamano wrote:\n> The first three looked almost good.  I do not fully agree with what\n> 4/6 does, and update to its function signature is likely to require\n> updates to 5/6 and 6/6.\n> \n> Thanks.\n"},{"id":"282826","messageId":"20160407033844.GB17848@LykOS","threadId":"41934","inReplyTo":"xmqqa8l6zoqc.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v5 4/6] builtin/verify-tag: replace name argument with sha1","fromName":"Santiago Torres","fromEmail":"santiago@nyu.edu","sentAt":"2016-04-07T03:38:45Z","receivedAt":"2016-04-07T03:38:45Z","isPatch":true,"sender":{"key":"santiago@nyu.edu","avatar":"https://avatars.githubusercontent.com/u/3579933?v=4"},"body":"> >  \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> > +\t\t\t\thex_sha1, typename(type));\n> \n> So, if I said\n> \n>     $ git verify-tag master\n> \n> the code used to take \"master\" from argv[], fed it to verify_tag()\n> as parameter 'name', turned it to the object name of the commit,\n> noticed that it is not a tag, and complained that \"master: cannot verify\".\n> \n> With this rewrite, the same invocation would cause \"master\" to be\n> turned into the object name, which is passed to verify_tag() and the\n> complaint is an overlong\n> \n>     76bece327f490cb344b95ae8f869cbeb89a4d20b: cannot verify a non-tag object of type commit\n> \n> That does not sound like a good change at all.\n\nYep, I agree. At least I believe that we can do better in terms of user\nfeedback.\n\n> \n> If you want to support a future caller of a libified version of\n> verify_tag() that has a raw object name but not the original name,\n> I'd suggest to make this function keep parameter 'name' while adding\n> the new parameter 'sha1'.  Then, the error reporting may become:\n> \n> \treturn error(\"%s: cannot verify a non-tag object of type '%s'\",\n> \t\t     name ? name : sha1_to_hex(sha1), typename(type));\n> \n\nYep, I'll do some experimenting with this.\n\n> and the output would still be useful.  Further improvements may be\n> \n>  - rename 'name' to 'report_name' to clarify that the parameter is\n>    only used for reporting, and that the tag object to verify is\n>    always identified by the new 'sha1' parameter.\n\nThis seems to be fundamental. As it was suggested before, making it\nclear that name is only for feedback purposes is really necessary. \n\n> \n>  - use find_unique_abbrev() to shorten the fallback name displayed in\n>  the error message.\n\nI think we can do both. Let me try this out. \n\nWould you suggest having these changes in a separate patch?\n\n\nThanks!\n-Santiago.\n"},{"id":"282827","messageId":"20160407034007.GC17848@LykOS","threadId":"41934","inReplyTo":"CAPig+cQ4n5j4Q-WF-0cd=2+5eSAaimh3A7La+8Fe9Ox4anjtBQ@mail.gmail.com","subject":"Re: [PATCH v5 0/6] tag: move PGP verification code to tag.c","fromName":"Santiago Torres","fromEmail":"santiago@nyu.edu","sentAt":"2016-04-07T03:40:07Z","receivedAt":"2016-04-07T03:40:07Z","isPatch":true,"sender":{"key":"santiago@nyu.edu","avatar":"https://avatars.githubusercontent.com/u/3579933?v=4"},"body":"> > v5 (this):\n> > Added helpful feedback by Eric\n> >\n> >  * Reordering of the patches, to avoid temporal inclusion of a regression\n> >  * Fix typos here and there.\n> >  * Review commit messages, as some weren't representative of what the patches\n> >    were doing anymore.\n> >  * Updated t7030 to include Peff's suggestion, and added a helped-by line here\n> >    as it was mostly Peff's code.\n> >  * Updated the error-handling/printing issues that were introduced when.\n> >    libifying the verify_tag function.\n> \n> This version is a more pleasant read, easier to digest and understand.\n> All of my review comments were minor; nothing demanding a re-roll. As\n> such, this version is:\n> \n>     Reviewed-by: Eric Sunshine <sunshine@sunshineco.com>\n> \n> If you do happen to re-roll based upon the review comments, feel free\n> to add my Reviewed-by: (but not if you make larger changes).\n\nThanks! I'll add your and Junio's in another re-roll.\n-Santiago.\n"},{"id":"282846","messageId":"CAPig+cQxmovGckrbwuEoeA=hW1idukj3qKTfjmr8B+AM7E2J0A@mail.gmail.com","threadId":"41934","inReplyTo":"20160407034007.GC17848@LykOS","subject":"Re: [PATCH v5 0/6] tag: move PGP verification code to tag.c","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2016-04-07T16:19:37Z","receivedAt":"2016-04-07T16:19:37Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Wed, Apr 6, 2016 at 11:40 PM, Santiago Torres <santiago@nyu.edu> wrote:\n>> > v5 (this):\n>> > Added helpful feedback by Eric\n>> >\n>> >  * Reordering of the patches, to avoid temporal inclusion of a regression\n>> >  * Fix typos here and there.\n>> >  * Review commit messages, as some weren't representative of what the patches\n>> >    were doing anymore.\n>> >  * Updated t7030 to include Peff's suggestion, and added a helped-by line here\n>> >    as it was mostly Peff's code.\n>> >  * Updated the error-handling/printing issues that were introduced when.\n>> >    libifying the verify_tag function.\n>>\n>> This version is a more pleasant read, easier to digest and understand.\n>> All of my review comments were minor; nothing demanding a re-roll. As\n>> such, this version is:\n>>\n>>     Reviewed-by: Eric Sunshine <sunshine@sunshineco.com>\n>>\n>> If you do happen to re-roll based upon the review comments, feel free\n>> to add my Reviewed-by: (but not if you make larger changes).\n>\n> Thanks! I'll add your and Junio's in another re-roll.\n\nI don't think Junio explicitly gave his Reviewed-by: (indicating his\napproval of the patches as-is), so you wouldn't want to include his\nReviewed-by:.\n\nIf you make any changes beyond the minor ones mentioned in my reviews\nor beyond plagiarizing commit message enhancements offered by my or\nJunio's reviews, then you'd also probably want to hold off adding my\nReviewed-by: since I wouldn't yet have reviewed whatever new changes\nyou're making. (And, if you do make changes beyond ones I mentioned,\nand if I review them and consider them issue-free, I can always\nre-extend my Reviewed-by:.)\n"},{"id":"283689","messageId":"20160417173155.GA1014@LykOS","threadId":"41934","inReplyTo":"CAPig+cSOLnCoX77O7khEX1cTh0Hu29d6MRdcpfN8ytTkUSVU=A@mail.gmail.com","subject":"Re: [PATCH v5 2/6] t7030-verify-tag: Adds validation for multiple tags","fromName":"Santiago Torres","fromEmail":"santiago@nyu.edu","sentAt":"2016-04-17T17:31:56Z","receivedAt":"2016-04-17T17:31:56Z","isPatch":true,"sender":{"key":"santiago@nyu.edu","avatar":"https://avatars.githubusercontent.com/u/3579933?v=4"},"body":"Sorry for the delay! I just realized I had missed the second comment.\n\n> +       grep \"^.GNUPG\" <expect.stderr.1 >expect.stderr &&\n> \n> Hmm, is there a reason you didn't stick with the more strict regex\n> Peff suggested?\n> \n>     ^.GNUPG:.\n> \n> (Genuine question: I'm not saying your choice is wrong, I'm just\n> interested in the reasoning behind the decision.)\n> \nI actually had missed the \":\". I read the email and tried to internalize\nwhat the new test was actually doing, then I rewrote the test. \n\nI think I could add it for completeness though.\n\nThanks again for the reviews!\n-Santiago.\n"},{"id":"283690","messageId":"20160417173406.GB1014@LykOS","threadId":"41934","inReplyTo":"CAPig+cQxmovGckrbwuEoeA=hW1idukj3qKTfjmr8B+AM7E2J0A@mail.gmail.com","subject":"Re: [PATCH v5 0/6] tag: move PGP verification code to tag.c","fromName":"Santiago Torres","fromEmail":"santiago@nyu.edu","sentAt":"2016-04-17T17:34:06Z","receivedAt":"2016-04-17T17:34:06Z","isPatch":true,"sender":{"key":"santiago@nyu.edu","avatar":"https://avatars.githubusercontent.com/u/3579933?v=4"},"body":"On Thu, Apr 07, 2016 at 12:19:37PM -0400, Eric Sunshine wrote:\n> On Wed, Apr 6, 2016 at 11:40 PM, Santiago Torres <santiago@nyu.edu> wrote:\n> >> > v5 (this):\n> >> > Added helpful feedback by Eric\n> >> >\n> >> >  * Reordering of the patches, to avoid temporal inclusion of a regression\n> >> >  * Fix typos here and there.\n> >> >  * Review commit messages, as some weren't representative of what the patches\n> >> >    were doing anymore.\n> >> >  * Updated t7030 to include Peff's suggestion, and added a helped-by line here\n> >> >    as it was mostly Peff's code.\n> >> >  * Updated the error-handling/printing issues that were introduced when.\n> >> >    libifying the verify_tag function.\n> >>\n> >> This version is a more pleasant read, easier to digest and understand.\n> >> All of my review comments were minor; nothing demanding a re-roll. As\n> >> such, this version is:\n> >>\n> >>     Reviewed-by: Eric Sunshine <sunshine@sunshineco.com>\n> >>\n> >> If you do happen to re-roll based upon the review comments, feel free\n> >> to add my Reviewed-by: (but not if you make larger changes).\n> >\n> > Thanks! I'll add your and Junio's in another re-roll.\n> \n> I don't think Junio explicitly gave his Reviewed-by: (indicating his\n> approval of the patches as-is), so you wouldn't want to include his\n> Reviewed-by:.\n\nYeah, I didn't mean to imply that. I'm rewriting the commit messages and\ntesting out patches 3+/6, so I'm not going to assume there's any\nreviewed-by.\n\n> \n> If you make any changes beyond the minor ones mentioned in my reviews\n> or beyond plagiarizing commit message enhancements offered by my or\n> Junio's reviews, then you'd also probably want to hold off adding my\n> Reviewed-by: since I wouldn't yet have reviewed whatever new changes\n> you're making.\n\nSpeaking of which, would it make sense to add \"helped-by\" to the patches\nin which I'm plagiarizing the commit messages?\n\n> (And, if you do make changes beyond ones I mentioned, and if I review\n> them and consider them issue-free, I can always re-extend my\n> Reviewed-by:.)\n\nThanks!\n-Santiago.\n"},{"id":"283691","messageId":"CAPig+cR+6-sDK7kFP0y7OeYCPUQWZ7wWeaJtzF_ee3DVoymvNQ@mail.gmail.com","threadId":"41934","inReplyTo":"20160417173406.GB1014@LykOS","subject":"Re: [PATCH v5 0/6] tag: move PGP verification code to tag.c","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2016-04-17T18:14:33Z","receivedAt":"2016-04-17T18:14:33Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sun, Apr 17, 2016 at 1:34 PM, Santiago Torres <santiago@nyu.edu> wrote:\n> On Thu, Apr 07, 2016 at 12:19:37PM -0400, Eric Sunshine wrote:\n>> If you make any changes beyond the minor ones mentioned in my reviews\n>> or beyond plagiarizing commit message enhancements offered by my or\n>> Junio's reviews, then you'd also probably want to hold off adding my\n>> Reviewed-by: since I wouldn't yet have reviewed whatever new changes\n>> you're making.\n>\n> Speaking of which, would it make sense to add \"helped-by\" to the patches\n> in which I'm plagiarizing the commit messages?\n\nYes, that would be quite sensible.\n"},{"id":"283692","messageId":"CAPig+cQYAAG7hVe6Li3-TZb99etkE22QzcWbKBCzr8L0SP5LXg@mail.gmail.com","threadId":"41934","inReplyTo":"20160417173155.GA1014@LykOS","subject":"Re: [PATCH v5 2/6] t7030-verify-tag: Adds validation for multiple tags","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2016-04-17T18:19:00Z","receivedAt":"2016-04-17T18:19:00Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sun, Apr 17, 2016 at 1:31 PM, Santiago Torres <santiago@nyu.edu> wrote:\n>> +       grep \"^.GNUPG\" <expect.stderr.1 >expect.stderr &&\n>>\n>> Hmm, is there a reason you didn't stick with the more strict regex\n>> Peff suggested?\n>>\n>>     ^.GNUPG:.\n>>\n>> (Genuine question: I'm not saying your choice is wrong, I'm just\n>> interested in the reasoning behind the decision.)\n>\n> I actually had missed the \":\". I read the email and tried to internalize\n> what the new test was actually doing, then I rewrote the test.\n>\n> I think I could add it for completeness though.\n\nJunio already made this correction and others in the three patches he\nqueued on his 'pu' branch. It's possible that he also made other\ntweaks not mentioned in the reviews, so it's a good idea to compare\nwhat he queued against what you plan to send for the re-roll to ensure\nthat nothing is missed. Thanks.\n"},{"id":"283693","messageId":"20160417183815.GC1014@LykOS","threadId":"41934","inReplyTo":"CAPig+cQYAAG7hVe6Li3-TZb99etkE22QzcWbKBCzr8L0SP5LXg@mail.gmail.com","subject":"Re: [PATCH v5 2/6] t7030-verify-tag: Adds validation for multiple tags","fromName":"Santiago Torres","fromEmail":"santiago@nyu.edu","sentAt":"2016-04-17T18:38:15Z","receivedAt":"2016-04-17T18:38:15Z","isPatch":true,"sender":{"key":"santiago@nyu.edu","avatar":"https://avatars.githubusercontent.com/u/3579933?v=4"},"body":"On Sun, Apr 17, 2016 at 02:19:00PM -0400, Eric Sunshine wrote:\n> On Sun, Apr 17, 2016 at 1:31 PM, Santiago Torres <santiago@nyu.edu> wrote:\n> >> +       grep \"^.GNUPG\" <expect.stderr.1 >expect.stderr &&\n> >>\n> >> Hmm, is there a reason you didn't stick with the more strict regex\n> >> Peff suggested?\n> >>\n> >>     ^.GNUPG:.\n> >>\n> >> (Genuine question: I'm not saying your choice is wrong, I'm just\n> >> interested in the reasoning behind the decision.)\n> >\n> > I actually had missed the \":\". I read the email and tried to internalize\n> > what the new test was actually doing, then I rewrote the test.\n> >\n> > I think I could add it for completeness though.\n> \n> Junio already made this correction and others in the three patches he\n> queued on his 'pu' branch. It's possible that he also made other\n> tweaks not mentioned in the reviews, so it's a good idea to compare\n> what he queued against what you plan to send for the re-roll to ensure\n> that nothing is missed. Thanks.\n\nOh, I'm looking at the patches in pu, I didn't know they were there yet.\nThanks for the heads up. \n\nAlso, would it make sense to copy the commit messages as they are on the pu\nbranch? for consistency? Or should ommit those three patches and work\non 4+ for the re-roll instead?\n"},{"id":"283694","messageId":"CAPig+cTS8KBtAg9q9ToXO4c2kZMgm3WGC-_vrPyHDdRHk0T9Yg@mail.gmail.com","threadId":"41934","inReplyTo":"20160417183815.GC1014@LykOS","subject":"Re: [PATCH v5 2/6] t7030-verify-tag: Adds validation for multiple tags","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2016-04-17T18:53:28Z","receivedAt":"2016-04-17T18:53:28Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sun, Apr 17, 2016 at 2:38 PM, Santiago Torres <santiago@nyu.edu> wrote:\n> On Sun, Apr 17, 2016 at 02:19:00PM -0400, Eric Sunshine wrote:\n>> Junio already made this correction and others in the three patches he\n>> queued on his 'pu' branch. It's possible that he also made other\n>> tweaks not mentioned in the reviews, so it's a good idea to compare\n>> what he queued against what you plan to send for the re-roll to ensure\n>> that nothing is missed. Thanks.\n>\n> Oh, I'm looking at the patches in pu, I didn't know they were there yet.\n> Thanks for the heads up.\n>\n> Also, would it make sense to copy the commit messages as they are on the pu\n> branch? for consistency? Or should ommit those three patches and work\n> on 4+ for the re-roll instead?\n\nI just re-read the commit messages as Junio queued them on 'pu', and\nthey are all good, so yes it would be plenty sensible to copy the\ncommit messages from the three queued patches (along with whatever\nother tweaks he made).\n\nSince the patches are only in 'pu' (but not in 'next'), when you\nre-roll, resubmit the entire series, not just the patches he didn't\nqueue.\n"}]}