{"thread":{"id":"41905","subject":"[PATCH v3 0/4] tag: move PGP verification code to tag.c","startedAt":"2016-04-02T23:16:11Z","lastAt":"2016-04-04T20:19:37Z","messageCount":24,"participants":["santiago@nyu.edu","Jeff King","Johannes Sixt","Eric Sunshine","Santiago Torres"],"isPatch":true,"patchVersion":3,"patchTotal":4},"messages":[{"id":"282531","messageId":"1459638975-17705-1-git-send-email-santiago@nyu.edu","threadId":"41905","inReplyTo":null,"subject":"[PATCH v3 0/4] tag: move PGP verification code to tag.c","fromName":"","fromEmail":"santiago@nyu.edu","sentAt":"2016-04-02T23:16:11Z","receivedAt":"2016-04-02T23:16:11Z","isPatch":true,"sender":{"key":"santiago@nyu.edu","avatar":"https://avatars.githubusercontent.com/u/3579933?v=4"},"body":"This is a follow up of [1] and [2]:\n\nv3 (this):\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[1]\nhttp://git.661346.n2.nabble.com/PATCH-RFC-builtin-tag-c-move-PGP-verification-inside-builtin-td7651529.html#a7651547\n[2] http://git.661346.n2.nabble.com/PATCH-tag-c-move-PGP-verification-code-from-plumbing-td7651562.html \n"},{"id":"282532","messageId":"1459638975-17705-2-git-send-email-santiago@nyu.edu","threadId":"41905","inReplyTo":"1459638975-17705-1-git-send-email-santiago@nyu.edu","subject":"[PATCH v3 1/4] builtin/verify-tag.c: Ignore SIGPIPE on gpg-interface","fromName":"","fromEmail":"santiago@nyu.edu","sentAt":"2016-04-02T23:16:12Z","receivedAt":"2016-04-02T23:16:12Z","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/gpg-verify.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---\nNotes:\n I dropped the multiline comment altogheter.\n\n builtin/verify-tag.c | 3 ---\n gpg-interface.c      | 3 +++\n 2 files changed, 3 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..c1f6b2d 100644\n--- a/gpg-interface.c\n+++ b/gpg-interface.c\n@@ -232,6 +232,8 @@ 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+\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 +252,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":"282534","messageId":"1459638975-17705-3-git-send-email-santiago@nyu.edu","threadId":"41905","inReplyTo":"1459638975-17705-1-git-send-email-santiago@nyu.edu","subject":"[PATCH v3 2/4] t/t7030-verify-tag.sh: Adds validation for multiple tags","fromName":"","fromEmail":"santiago@nyu.edu","sentAt":"2016-04-02T23:16:13Z","receivedAt":"2016-04-02T23:16:13Z","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 mutliple tag names as an argument.\nHowever, no previous tests try to verify multiple tags at once. This\ntest runs the verify-tag command against three trusted tags (created\npreviously), and ensures that:\n\n\t1) Three tags are verified appropriately (grep GOODSIG) and\n\t2) The three tags verified are indeed differently (uniq\n\t\t-u)\n\nSigned-off-by: Santiago Torres <santiago@nyu.edu>\n---\n t/t7030-verify-tag.sh | 8 ++++++++\n 1 file changed, 8 insertions(+)\n\ndiff --git a/t/t7030-verify-tag.sh b/t/t7030-verify-tag.sh\nindex 4608e71..5918f86 100755\n--- a/t/t7030-verify-tag.sh\n+++ b/t/t7030-verify-tag.sh\n@@ -112,4 +112,12 @@ test_expect_success GPG 'verify signatures with --raw' '\n \t)\n '\n \n+test_expect_success GPG 'verify multiple tags' '\n+\tgit verify-tag -v --raw fourth-signed sixth-signed seventh-signed 2>actual 1> tagnames &&\n+\t\tgrep -c \"GOODSIG\" actual > count &&\n+\t\t! grep \"BADSIG\" actual &&\n+\t\tgrep -E \"tag\\ .*\" tagnames | uniq -u | wc - l | grep \"3\"\n+'\n+\n+\n test_done\n-- \n2.8.0\n"},{"id":"282533","messageId":"1459638975-17705-4-git-send-email-santiago@nyu.edu","threadId":"41905","inReplyTo":"1459638975-17705-1-git-send-email-santiago@nyu.edu","subject":"[PATCH v3 3/4] builtin/verify-tag: move verification code to tag.c","fromName":"","fromEmail":"santiago@nyu.edu","sentAt":"2016-04-02T23:16:14Z","receivedAt":"2016-04-02T23:16:14Z","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\ncode. We rename the verify_tag() function to pgp_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 | 51 +--------------------------------------------------\n tag.c                | 50 ++++++++++++++++++++++++++++++++++++++++++++++++++\n tag.h                |  1 +\n 3 files changed, 52 insertions(+), 50 deletions(-)\n\ndiff --git a/builtin/verify-tag.c b/builtin/verify-tag.c\nindex 77f070a..f776778 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@@ -96,7 +47,7 @@ int cmd_verify_tag(int argc, const char **argv, const char *prefix)\n \t\tflags |= GPG_VERIFY_VERBOSE;\n \n \twhile (i < argc)\n-\t\tif (verify_tag(argv[i++], flags))\n+\t\tif (pgp_verify_tag(argv[i++], flags))\n \t\t\thad_error = 1;\n \treturn had_error;\n }\ndiff --git a/tag.c b/tag.c\nindex d72f742..918ae39 100644\n--- a/tag.c\n+++ b/tag.c\n@@ -6,6 +6,56 @@\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\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, unsigned flags)\n+{\n+\n+\tenum object_type type;\n+\tunsigned long size;\n+\tunsigned char sha1[20];\n+\tchar* buf;\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 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..09e71f9 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 pgp_verify_tag(const char *name, unsigned flags);\n \n #endif /* TAG_H */\n-- \n2.8.0\n"},{"id":"282535","messageId":"1459638975-17705-5-git-send-email-santiago@nyu.edu","threadId":"41905","inReplyTo":"1459638975-17705-1-git-send-email-santiago@nyu.edu","subject":"[PATCH v3 4/4] tag: use pgp_verify_function in tag -v call","fromName":"","fromEmail":"santiago@nyu.edu","sentAt":"2016-04-02T23:16:15Z","receivedAt":"2016-04-02T23:16:15Z","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\npgp_verify_tag(). This avoids the usage of an extra fork call. To do\nthis, we extend the number of parameters that tag.c takes, and\nverify-tag passes. Redundant calls done in the pgp_verify_tag function\nare removed.\n\nSigned-off-by: Santiago Torres <santiago@nyu.edu>\n---\n Notes:\n - In this version I fixed the issues with the brackets (the old patch\n   doesn't work in this case due to the new test.\n\n builtin/tag.c        | 28 +++++++++-------------------\n builtin/verify-tag.c | 14 ++++++++++++--\n tag.c                |  7 ++-----\n tag.h                |  3 ++-\n 4 files changed, 25 insertions(+), 27 deletions(-)\n\ndiff --git a/builtin/tag.c b/builtin/tag.c\nindex 1705c94..3dffdff 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,33 +87,21 @@ 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 \treturn sign_buffer(buffer, buffer, get_signing_key());\n@@ -424,9 +413,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 f776778..8abc357 100644\n--- a/builtin/verify-tag.c\n+++ b/builtin/verify-tag.c\n@@ -30,6 +30,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@@ -46,8 +48,16 @@ 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 (pgp_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}\n+\n+\t\tif (pgp_verify_tag(name, NULL, sha1, flags))\n+\t\t\thad_error = 1;\n+\n+\t}\n \treturn had_error;\n }\ndiff --git a/tag.c b/tag.c\nindex 918ae39..2a0b24c 100644\n--- a/tag.c\n+++ b/tag.c\n@@ -29,18 +29,15 @@ static int run_gpg_verify(const char *buf, unsigned long size, unsigned flags)\n \treturn ret;\n }\n \n-int pgp_verify_tag(const char *name, unsigned flags)\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-\tunsigned char sha1[20];\n \tchar* buf;\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.\",\ndiff --git a/tag.h b/tag.h\nindex 09e71f9..22289a5 100644\n--- a/tag.h\n+++ b/tag.h\n@@ -17,6 +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, unsigned flags);\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.8.0\n"},{"id":"282548","messageId":"20160403043041.GA1519@sigill.intra.peff.net","threadId":"41905","inReplyTo":"1459638975-17705-2-git-send-email-santiago@nyu.edu","subject":"Re: [PATCH v3 1/4] builtin/verify-tag.c: Ignore SIGPIPE on gpg-interface","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-04-03T04:30:41Z","receivedAt":"2016-04-03T04:30:41Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Apr 02, 2016 at 07:16:12PM -0400, santiago@nyu.edu wrote:\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/gpg-verify.c command to avoid this issue. However,\n\ns/gpg-verify/verify-tag/ here, I think.\n\nOther than that, nicely explained.\n\n-Peff\n"},{"id":"282549","messageId":"20160403044054.GB1519@sigill.intra.peff.net","threadId":"41905","inReplyTo":"1459638975-17705-3-git-send-email-santiago@nyu.edu","subject":"Re: [PATCH v3 2/4] t/t7030-verify-tag.sh: Adds validation for multiple tags","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-04-03T04:40:55Z","receivedAt":"2016-04-03T04:40:55Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Apr 02, 2016 at 07:16:13PM -0400, santiago@nyu.edu wrote:\n\n> The verify-tag command supports mutliple tag names as an argument.\n\ns/mutliple/multiple/\n\n> +test_expect_success GPG 'verify multiple tags' '\n> +\tgit verify-tag -v --raw fourth-signed sixth-signed seventh-signed 2>actual 1> tagnames &&\n\nStyle: we don't put a space between \">\" and the filename. Also, we\nusually omit \"1\" when redirecting stdout.\n\n> +\t\tgrep -c \"GOODSIG\" actual > count &&\n\nFunny indentation here.\n\nI wondered if we could use test_cmp instead of a counting grep here, but\nthis is looking at gpg spew, and we probably don't want to count on that\nnever changing.\n\nI don't see us actually verifying that \"count\" is 3, though.\n\n> +\t\t! grep \"BADSIG\" actual &&\n\nMakes sense...\n\n> +\t\tgrep -E \"tag\\ .*\" tagnames | uniq -u | wc - l | grep \"3\"\n\nDo we need \"grep -E\" here? I don't see any extended regex in use. Is\nthere a reason to backslash-escape the space?\n\nYour \"wc -l\" has an extra space, which means \"read stdin, and then the\nfile 'l'\". Which sort-of happens to work, except as you noticed, you\nhave to grep for \"3\" instead of matching it.\n\nI think, though, that rather than counting we could just write what we\nexpect into a file and compare that. It makes it easier for somebody\nreading the test to see what it is we're trying to do.\n\nIn fact, I suspect you could replace the \"GOODSIG\" check as well by\ndoing something like:\n\n  # verifying 3 tags in one invocation should be exactly like\n  # verifying the 3 separately\n  tags=\"fourth-signed sixth-signed seventh-signed\"\n  for i in $tags; do\n          git verify-tag -v --raw $i || return 1\n  done >expect.stdout 2>expect.stderr &&\n  git verify-tag -v --raw $tags >actual.stdout 2>actual.stderr &&\n  test_cmp expect.stdout actual.stdout &&\n  test_cmp expect.stderr actual.stderr\n\nbut I didn't test it.\n\n-Peff\n"},{"id":"282550","messageId":"20160403044502.GC1519@sigill.intra.peff.net","threadId":"41905","inReplyTo":"1459638975-17705-4-git-send-email-santiago@nyu.edu","subject":"Re: [PATCH v3 3/4] builtin/verify-tag: move verification code to tag.c","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-04-03T04:45:03Z","receivedAt":"2016-04-03T04:45:03Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Apr 02, 2016 at 07:16:14PM -0400, santiago@nyu.edu wrote:\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> code. We rename the verify_tag() function to pgp_verify_tag() to avoid\n> conflicts with the mktag.c function.\n\nOne nit: even though GPG is just an implementation of PGP, and\ntechnically the standard and formats are called PGP, we tend to name\neverything GPG in the code. So this probably should be gpg_verify_tag().\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> +\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\nI'm happy to see the more readable variable name here. I wonder if we\nshould leave the error message as-is, though, as this is just supposed\nto be about code movement (and if we are changing it, it should adhere\nto our usual style of not starting with a capital letter, and not ending\nin punctuation).\n\n-Peff\n"},{"id":"282551","messageId":"20160403045600.GD1519@sigill.intra.peff.net","threadId":"41905","inReplyTo":"1459638975-17705-5-git-send-email-santiago@nyu.edu","subject":"Re: [PATCH v3 4/4] tag: use pgp_verify_function in tag -v call","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-04-03T04:56:00Z","receivedAt":"2016-04-03T04:56:00Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Apr 02, 2016 at 07:16:15PM -0400, santiago@nyu.edu wrote:\n\n> diff --git a/builtin/tag.c b/builtin/tag.c\n> index 1705c94..3dffdff 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\nI'm not sure it's a good idea to add a flags field here; most of the\ncallbacks don't use it, and as you probably noticed, it makes the patch\na lot noisier. It does let you directly use pgp_verify_tag like this:\n\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\nbut I think that is coupling too closely. What happens later when the\npublic, multi-file pgp_verify_tag function changes its interface? Or we\nwant to change our interface here, and it no longer matches\npgp_verify_tag? The results ripple a lot further than they should.\n\nI think you probably want to keep a simple adapter callback in this\nfile, like:\n\n  int verify_tag(const char *name, const char *ref, const unsigned char *sha1)\n  {\n\treturn pgp_verify_tag(name, GPG_VERIFY_VERBOSE));\n  }\n\n> diff --git a/builtin/verify-tag.c b/builtin/verify-tag.c\n> index f776778..8abc357 100644\n> --- a/builtin/verify-tag.c\n> +++ b/builtin/verify-tag.c\n> @@ -30,6 +30,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> @@ -46,8 +48,16 @@ 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 (pgp_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}\n> +\n> +\t\tif (pgp_verify_tag(name, NULL, sha1, flags))\n> +\t\t\thad_error = 1;\n> +\n> +\t}\n\nSo this is a good example of the rippling I mentioned earlier.\n\nAs a side note, it might actually be an improvement for pgp_verify_tag\nto take a sha1 (so that git-tag is sure that it is verifying the same\nobject that it is printing), but that refactoring should probably come\nseparately, I think.\n\n-Peff\n"},{"id":"282554","messageId":"5700BD33.6090602@kdbg.org","threadId":"41905","inReplyTo":"1459638975-17705-2-git-send-email-santiago@nyu.edu","subject":"Re: [PATCH v3 1/4] builtin/verify-tag.c: Ignore SIGPIPE on gpg-interface","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2016-04-03T06:50:27Z","receivedAt":"2016-04-03T06:50:27Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 03.04.2016 um 01:16 schrieb santiago@nyu.edu:\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/gpg-verify.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>\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>\n> Signed-off-by: Santiago Torres <santiago@nyu.edu>\n> ---\n> Notes:\n>   I dropped the multiline comment altogheter.\n>\n>   builtin/verify-tag.c | 3 ---\n>   gpg-interface.c      | 3 +++\n>   2 files changed, 3 insertions(+), 3 deletions(-)\n>\n> diff --git a/builtin/verify-tag.c b/builtin/verify-tag.c\n> index 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;\n> diff --git a/gpg-interface.c b/gpg-interface.c\n> index 3dc2fe3..c1f6b2d 100644\n> --- a/gpg-interface.c\n> +++ b/gpg-interface.c\n> @@ -232,6 +232,8 @@ 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> +\tsigchain_push(SIGPIPE, SIG_IGN);\n>   \tif (start_command(&gpg)) {\n>   \t\tunlink(path);\n>   \t\treturn error(_(\"could not run gpg.\"));\n\nBut no sigchain_pop() in the error path that we see here?\n\nPerhaps you can even defer the sigchain_push() until after start_command()?\n\n> @@ -250,6 +252,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>\n\n-- Hannes\n"},{"id":"282556","messageId":"CAPig+cQp2t0y7imR5tTHdLdMO6w13ia-GxZXxq4c2NGaZTjErw@mail.gmail.com","threadId":"41905","inReplyTo":"20160403044054.GB1519@sigill.intra.peff.net","subject":"Re: [PATCH v3 2/4] t/t7030-verify-tag.sh: Adds validation for multiple tags","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2016-04-03T07:59:46Z","receivedAt":"2016-04-03T07:59:46Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sun, Apr 3, 2016 at 12:40 AM, Jeff King <peff@peff.net> wrote:\n> In fact, I suspect you could replace the \"GOODSIG\" check as well by\n> doing something like:\n>\n>   # verifying 3 tags in one invocation should be exactly like\n>   # verifying the 3 separately\n>   tags=\"fourth-signed sixth-signed seventh-signed\"\n>   for i in $tags; do\n>           git verify-tag -v --raw $i || return 1\n>   done >expect.stdout 2>expect.stderr &&\n>   git verify-tag -v --raw $tags >actual.stdout 2>actual.stderr &&\n>   test_cmp expect.stdout actual.stdout &&\n>   test_cmp expect.stderr actual.stderr\n\nHmm, does [1] suggest that using test_cmp on stderr here would be\ncontraindicated?\n\n[1]: http://article.gmane.org/gmane.comp.version-control.git/289077\n"},{"id":"282557","messageId":"CAPig+cQzeRk66eptatSy8=1xbnBNrO5ATrfqBX8730t3gOmNQw@mail.gmail.com","threadId":"41905","inReplyTo":"20160403044502.GC1519@sigill.intra.peff.net","subject":"Re: [PATCH v3 3/4] builtin/verify-tag: move verification code to tag.c","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2016-04-03T08:11:11Z","receivedAt":"2016-04-03T08:11:11Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sun, Apr 3, 2016 at 12:45 AM, Jeff King <peff@peff.net> wrote:\n> On Sat, Apr 02, 2016 at 07:16:14PM -0400, santiago@nyu.edu wrote:\n>> -     len = parse_signature(buf, size);\n>> -\n>> -     if (size == len) {\n>> -             if (flags & GPG_VERIFY_VERBOSE)\n>> -                     write_in_full(1, buf, len);\n>> -             return error(\"no signature found\");\n>> -     }\n>> [...]\n>> +     payload_size = parse_signature(buf, size);\n>> +\n>> +     if (size == payload_size) {\n>> +             write_in_full(1, buf, payload_size);\n>> +             return error(\"No PGP signature found in this tag!\");\n>> +     }\n>\n> I'm happy to see the more readable variable name here. I wonder if we\n> should leave the error message as-is, though, as this is just supposed\n> to be about code movement (and if we are changing it, it should adhere\n> to our usual style of not starting with a capital letter, and not ending\n> in punctuation).\n\nAgreed it would be nice for this patch to be just code movement since\nit's difficult for a reviewer to spot actual changes. A pure code\nmovement patch was suggested by [1], but perhaps it should also have\nexplained the reason (\"code changes are difficult to spot in\nmovement\").\n\nSuch changes could be done as preparatory or follow-on patches.\nAlternately, since these are such minor changes, it might also be okay\njust to mention them in the commit message (as the function rename is\nalready mentioned).\n\n[1]: http://article.gmane.org/gmane.comp.version-control.git/289847\n"},{"id":"282558","messageId":"CAPig+cS47LSmwtLWPJZG8eFByEJ+t_hegM93nqsfTZEoH-+f4Q@mail.gmail.com","threadId":"41905","inReplyTo":"1459638975-17705-4-git-send-email-santiago@nyu.edu","subject":"Re: [PATCH v3 3/4] builtin/verify-tag: move verification code to tag.c","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2016-04-03T08:19:26Z","receivedAt":"2016-04-03T08:19:26Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sat, Apr 2, 2016 at 7:16 PM,  <santiago@nyu.edu> wrote:\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> code. We rename the verify_tag() function to pgp_verify_tag() to avoid\n> conflicts with the mktag.c function.\n>\n> Signed-off-by: Santiago Torres <santiago@nyu.edu>\n> ---\n> -       len = parse_signature(buf, size);\n> -\n> -       if (size == len) {\n> -               if (flags & GPG_VERIFY_VERBOSE)\n> -                       write_in_full(1, buf, len);\n> -               return error(\"no signature found\");\n> -       }\n> [...]\n> +       payload_size = parse_signature(buf, size);\n> +\n> +       if (size == payload_size) {\n> +               write_in_full(1, buf, payload_size);\n> +               return error(\"No PGP signature found in this tag!\");\n> +       }\n\nAlso, [1] asked why the moved code no longer respects\nGPG_VERIFY_VERBOSE, and that question doesn't seem to be answered\neither in the previous review thread or by this patch's commit\nmessage. It's not clear at a casual glance why this change is\ndesirable.\n\n[1]: http://article.gmane.org/gmane.comp.version-control.git/289977\n"},{"id":"282562","messageId":"20160403130724.GA16206@sigill.intra.peff.net","threadId":"41905","inReplyTo":"CAPig+cQp2t0y7imR5tTHdLdMO6w13ia-GxZXxq4c2NGaZTjErw@mail.gmail.com","subject":"Re: [PATCH v3 2/4] t/t7030-verify-tag.sh: Adds validation for multiple tags","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-04-03T13:07:25Z","receivedAt":"2016-04-03T13:07:25Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Apr 03, 2016 at 03:59:46AM -0400, Eric Sunshine wrote:\n\n> On Sun, Apr 3, 2016 at 12:40 AM, Jeff King <peff@peff.net> wrote:\n> > In fact, I suspect you could replace the \"GOODSIG\" check as well by\n> > doing something like:\n> >\n> >   # verifying 3 tags in one invocation should be exactly like\n> >   # verifying the 3 separately\n> >   tags=\"fourth-signed sixth-signed seventh-signed\"\n> >   for i in $tags; do\n> >           git verify-tag -v --raw $i || return 1\n> >   done >expect.stdout 2>expect.stderr &&\n> >   git verify-tag -v --raw $tags >actual.stdout 2>actual.stderr &&\n> >   test_cmp expect.stdout actual.stdout &&\n> >   test_cmp expect.stderr actual.stderr\n> \n> Hmm, does [1] suggest that using test_cmp on stderr here would be\n> contraindicated?\n> \n> [1]: http://article.gmane.org/gmane.comp.version-control.git/289077\n\nIt does, but I am not sure I agree with the advice in that email in the\ngeneral case (I like making \"-x\" work, too, but not at the cost of\nmaking the tests harder to read and write). In this case, I suppose you\ncould grep for gpg raw-output on stderr, though, and compare only that.\n\n-Peff\n"},{"id":"282579","messageId":"20160403214354.GA28933@LykOS","threadId":"41905","inReplyTo":"20160403045600.GD1519@sigill.intra.peff.net","subject":"Re: [PATCH v3 4/4] tag: use pgp_verify_function in tag -v call","fromName":"Santiago Torres","fromEmail":"santiago@nyu.edu","sentAt":"2016-04-03T21:43:55Z","receivedAt":"2016-04-03T21:43:55Z","isPatch":true,"sender":{"key":"santiago@nyu.edu","avatar":"https://avatars.githubusercontent.com/u/3579933?v=4"},"body":"> >  \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> but I think that is coupling too closely. What happens later when the\n> public, multi-file pgp_verify_tag function changes its interface? Or we\n> want to change our interface here, and it no longer matches\n> pgp_verify_tag? The results ripple a lot further than they should.\n> \n> I think you probably want to keep a simple adapter callback in this\n> file, like:\n> \n>   int verify_tag(const char *name, const char *ref, const unsigned char *sha1)\n>   {\n> \treturn pgp_verify_tag(name, GPG_VERIFY_VERBOSE));\n>   }\n\nYes, agreed. I'll give this a go\n\nThanks!\n-Santiago\n"},{"id":"282580","messageId":"20160403214606.GB28933@LykOS","threadId":"41905","inReplyTo":"5700BD33.6090602@kdbg.org","subject":"Re: [PATCH v3 1/4] builtin/verify-tag.c: Ignore SIGPIPE on gpg-interface","fromName":"Santiago Torres","fromEmail":"santiago@nyu.edu","sentAt":"2016-04-03T21:46:07Z","receivedAt":"2016-04-03T21:46:07Z","isPatch":true,"sender":{"key":"santiago@nyu.edu","avatar":"https://avatars.githubusercontent.com/u/3579933?v=4"},"body":"On Sun, Apr 03, 2016 at 08:50:27AM +0200, Johannes Sixt wrote:\n> Am 03.04.2016 um 01:16 schrieb santiago@nyu.edu:\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/gpg-verify.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> > \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> > \n> > Signed-off-by: Santiago Torres <santiago@nyu.edu>\n> > ---\n> > Notes:\n> >   I dropped the multiline comment altogheter.\n> > \n> >   builtin/verify-tag.c | 3 ---\n> >   gpg-interface.c      | 3 +++\n> >   2 files changed, 3 insertions(+), 3 deletions(-)\n> > \n> > diff --git a/builtin/verify-tag.c b/builtin/verify-tag.c\n> > index 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;\n> > diff --git a/gpg-interface.c b/gpg-interface.c\n> > index 3dc2fe3..c1f6b2d 100644\n> > --- a/gpg-interface.c\n> > +++ b/gpg-interface.c\n> > @@ -232,6 +232,8 @@ 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> > +\tsigchain_push(SIGPIPE, SIG_IGN);\n> >   \tif (start_command(&gpg)) {\n> >   \t\tunlink(path);\n> >   \t\treturn error(_(\"could not run gpg.\"));\n> \n> But no sigchain_pop() in the error path that we see here?\n> \n> Perhaps you can even defer the sigchain_push() until after start_command()?\n\nYes, after reviewing where the sigchain_push() call is made on\nsign_buffer(), I should put the push call after starting the command in\nthe same way sign_buffer() does. \n\nThanks!\n-Santiago.\n"},{"id":"282581","messageId":"20160403215310.GC28933@LykOS","threadId":"41905","inReplyTo":"CAPig+cS47LSmwtLWPJZG8eFByEJ+t_hegM93nqsfTZEoH-+f4Q@mail.gmail.com","subject":"Re: [PATCH v3 3/4] builtin/verify-tag: move verification code to tag.c","fromName":"Santiago Torres","fromEmail":"santiago@nyu.edu","sentAt":"2016-04-03T21:53:11Z","receivedAt":"2016-04-03T21:53:11Z","isPatch":true,"sender":{"key":"santiago@nyu.edu","avatar":"https://avatars.githubusercontent.com/u/3579933?v=4"},"body":"On Sun, Apr 03, 2016 at 04:19:26AM -0400, Eric Sunshine wrote:\n> On Sat, Apr 2, 2016 at 7:16 PM,  <santiago@nyu.edu> wrote:\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> > code. We rename the verify_tag() function to pgp_verify_tag() to avoid\n> > conflicts with the mktag.c function.\n> >\n> > Signed-off-by: Santiago Torres <santiago@nyu.edu>\n> > ---\n> > -       len = parse_signature(buf, size);\n> > -\n> > -       if (size == len) {\n> > -               if (flags & GPG_VERIFY_VERBOSE)\n> > -                       write_in_full(1, buf, len);\n> > -               return error(\"no signature found\");\n> > -       }\n> > [...]\n> > +       payload_size = parse_signature(buf, size);\n> > +\n> > +       if (size == payload_size) {\n> > +               write_in_full(1, buf, payload_size);\n> > +               return error(\"No PGP signature found in this tag!\");\n> > +       }\n> \n> Also, [1] asked why the moved code no longer respects\n> GPG_VERIFY_VERBOSE, and that question doesn't seem to be answered\n> either in the previous review thread or by this patch's commit\n> message. It's not clear at a casual glance why this change is\n> desirable.\n> \n\nI must've missed this when moving code around. I don't think that this\nif should change in any way. I'll put it back as it is (other than the\nvariable naming that is)\n"},{"id":"282582","messageId":"20160403215847.GD28933@LykOS","threadId":"41905","inReplyTo":"20160403130724.GA16206@sigill.intra.peff.net","subject":"Re: [PATCH v3 2/4] t/t7030-verify-tag.sh: Adds validation for multiple tags","fromName":"Santiago Torres","fromEmail":"santiago@nyu.edu","sentAt":"2016-04-03T21:58:47Z","receivedAt":"2016-04-03T21:58:47Z","isPatch":true,"sender":{"key":"santiago@nyu.edu","avatar":"https://avatars.githubusercontent.com/u/3579933?v=4"},"body":"On Sun, Apr 03, 2016 at 09:07:25AM -0400, Jeff King wrote:\n> On Sun, Apr 03, 2016 at 03:59:46AM -0400, Eric Sunshine wrote:\n> \n> > On Sun, Apr 3, 2016 at 12:40 AM, Jeff King <peff@peff.net> wrote:\n> > > In fact, I suspect you could replace the \"GOODSIG\" check as well by\n> > > doing something like:\n> > >\n> > >   # verifying 3 tags in one invocation should be exactly like\n> > >   # verifying the 3 separately\n> > >   tags=\"fourth-signed sixth-signed seventh-signed\"\n> > >   for i in $tags; do\n> > >           git verify-tag -v --raw $i || return 1\n> > >   done >expect.stdout 2>expect.stderr &&\n> > >   git verify-tag -v --raw $tags >actual.stdout 2>actual.stderr &&\n> > >   test_cmp expect.stdout actual.stdout &&\n> > >   test_cmp expect.stderr actual.stderr\n> > \n> > Hmm, does [1] suggest that using test_cmp on stderr here would be\n> > contraindicated?\n> > \n> > [1]: http://article.gmane.org/gmane.comp.version-control.git/289077\n> \n> It does, but I am not sure I agree with the advice in that email in the\n> general case (I like making \"-x\" work, too, but not at the cost of\n> making the tests harder to read and write). In this case, I suppose you\n> could grep for gpg raw-output on stderr, though, and compare only that.\n> \n> -Peff\n\nI just read [1], I'll take the later advice and use test_i18ngrep\ninstead.\n\nThanks!\n-Santiago.\n"},{"id":"282590","messageId":"CAPig+cTccb8nr7sV1PKkEf30nRz123TTgeZXkac01RaRwqvTZw@mail.gmail.com","threadId":"41905","inReplyTo":"20160403215847.GD28933@LykOS","subject":"Re: [PATCH v3 2/4] t/t7030-verify-tag.sh: Adds validation for multiple tags","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2016-04-04T01:38:34Z","receivedAt":"2016-04-04T01:38:34Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sun, Apr 3, 2016 at 5:58 PM, Santiago Torres <santiago@nyu.edu> wrote:\n> On Sun, Apr 03, 2016 at 09:07:25AM -0400, Jeff King wrote:\n>> On Sun, Apr 03, 2016 at 03:59:46AM -0400, Eric Sunshine wrote:\n>> > On Sun, Apr 3, 2016 at 12:40 AM, Jeff King <peff@peff.net> wrote:\n>> > > In fact, I suspect you could replace the \"GOODSIG\" check as well by\n>> > > doing something like:\n>> > >\n>> > >   tags=\"fourth-signed sixth-signed seventh-signed\"\n>> > >   for i in $tags; do\n>> > >           git verify-tag -v --raw $i || return 1\n>> > >   done >expect.stdout 2>expect.stderr &&\n>> > >   git verify-tag -v --raw $tags >actual.stdout 2>actual.stderr &&\n>> > >   test_cmp expect.stdout actual.stdout &&\n>> > >   test_cmp expect.stderr actual.stderr\n>> >\n>> > Hmm, does [1] suggest that using test_cmp on stderr here would be\n>> > contraindicated?\n>> >\n>> > [1]: http://article.gmane.org/gmane.comp.version-control.git/289077\n>>\n>> It does, but I am not sure I agree with the advice in that email in the\n>> general case (I like making \"-x\" work, too, but not at the cost of\n>> making the tests harder to read and write). In this case, I suppose you\n>> could grep for gpg raw-output on stderr, though, and compare only that.\n>\n> I just read [1], I'll take the later advice and use test_i18ngrep\n> instead.\n\nI think Peff meant that a simple grep would suffice; no need for\ntest_i18ngrep. In other words (reproducing Peff's example), something\nlike this:\n\n    tags=\"fourth-signed sixth-signed seventh-signed\" &&\n    for i in $tags; do\n        git verify-tag -v --raw $i || return 1\n    done >expect.stdout 2>expect.stderr.1 &&\n    grep GOODSIG <expect.stderr.1 >expect.stderr &&\n    git verify-tag -v --raw $tags >actual.stdout 2>actual.stderr.1 &&\n    grep GOODSIG <actual.stderr.1 >actual.stderr &&\n    test_cmp expect.stdout actual.stdout &&\n    test_cmp expect.stderr actual.stderr\n"},{"id":"282594","messageId":"20160404041203.GE28933@LykOS","threadId":"41905","inReplyTo":"20160403045600.GD1519@sigill.intra.peff.net","subject":"Re: [PATCH v3 4/4] tag: use pgp_verify_function in tag -v call","fromName":"Santiago Torres","fromEmail":"santiago@nyu.edu","sentAt":"2016-04-04T04:12:04Z","receivedAt":"2016-04-04T04:12:04Z","isPatch":true,"sender":{"key":"santiago@nyu.edu","avatar":"https://avatars.githubusercontent.com/u/3579933?v=4"},"body":"> > diff --git a/builtin/verify-tag.c b/builtin/verify-tag.c\n> > index f776778..8abc357 100644\n> > --- a/builtin/verify-tag.c\n> > +++ b/builtin/verify-tag.c\n> > @@ -30,6 +30,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> > @@ -46,8 +48,16 @@ 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 (pgp_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}\n> > +\n> > +\t\tif (pgp_verify_tag(name, NULL, sha1, flags))\n> > +\t\t\thad_error = 1;\n> > +\n> > +\t}\n> \n> So this is a good example of the rippling I mentioned earlier.\n> \n> As a side note, it might actually be an improvement for pgp_verify_tag\n> to take a sha1 (so that git-tag is sure that it is verifying the same\n> object that it is printing), but that refactoring should probably come\n> separately, I think.\n> \n> -Peff\n\nJust to be sure, this refactoring is something we should still include\nin this set of patches, right? I think that otherwise we'd lose the\ndesambigutaion that git tag -v does in this patch.\n\nI also think that most of the rippling is gone if we use and adaptor as\nyou suggested. Should I add a patch on top of this to support a sha1 as\npart for gpg_verify_tag()?\n\nThanks!\n-Santiago.\n"},{"id":"282613","messageId":"20160404133853.GB25404@sigill.intra.peff.net","threadId":"41905","inReplyTo":"20160404041203.GE28933@LykOS","subject":"Re: [PATCH v3 4/4] tag: use pgp_verify_function in tag -v call","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-04-04T13:38:54Z","receivedAt":"2016-04-04T13:38:54Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Apr 04, 2016 at 12:12:04AM -0400, Santiago Torres wrote:\n\n> > As a side note, it might actually be an improvement for pgp_verify_tag\n> > to take a sha1 (so that git-tag is sure that it is verifying the same\n> > object that it is printing), but that refactoring should probably come\n> > separately, I think.\n> \n> Just to be sure, this refactoring is something we should still include\n> in this set of patches, right? I think that otherwise we'd lose the\n> desambigutaion that git tag -v does in this patch.\n\nI think it can be part of this series, but doesn't have to be. As I\nunderstand it, the current code is just handing the name to the `git\nverify-tag` process, so if we continue to do so, that would be OK.\n\n> I also think that most of the rippling is gone if we use and adaptor as\n> you suggested. Should I add a patch on top of this to support a sha1 as\n> part for gpg_verify_tag()?\n\nYes, though I'd generally advise against a function taking either a name or\na sha1, and ignoring the other option. That often leads to confusing\ninterfaces for the callers. Instead, perhaps just take the sha1, and let\nthe caller do the get_sha1() themselves. Or possibly provide two\nfunctions, one of which is a convenience to translate the name to sha1\nand then call the other.\n\n-Peff\n"},{"id":"282614","messageId":"20160404134143.GC25404@sigill.intra.peff.net","threadId":"41905","inReplyTo":"CAPig+cTccb8nr7sV1PKkEf30nRz123TTgeZXkac01RaRwqvTZw@mail.gmail.com","subject":"Re: [PATCH v3 2/4] t/t7030-verify-tag.sh: Adds validation for multiple tags","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-04-04T13:41:43Z","receivedAt":"2016-04-04T13:41:43Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Apr 03, 2016 at 09:38:34PM -0400, Eric Sunshine wrote:\n\n> I think Peff meant that a simple grep would suffice; no need for\n> test_i18ngrep. In other words (reproducing Peff's example), something\n> like this:\n> \n>     tags=\"fourth-signed sixth-signed seventh-signed\" &&\n>     for i in $tags; do\n>         git verify-tag -v --raw $i || return 1\n>     done >expect.stdout 2>expect.stderr.1 &&\n>     grep GOODSIG <expect.stderr.1 >expect.stderr &&\n>     git verify-tag -v --raw $tags >actual.stdout 2>actual.stderr.1 &&\n>     grep GOODSIG <actual.stderr.1 >actual.stderr &&\n>     test_cmp expect.stdout actual.stdout &&\n>     test_cmp expect.stderr actual.stderr\n\nYep, though I think I would actually have done:\n\n   grep '^.GNUPG:.' ...\n\nor something to just catch all of the gnupg output.\n\n-Peff\n"},{"id":"282638","messageId":"20160404182447.GA6773@LykOS","threadId":"41905","inReplyTo":"20160404133853.GB25404@sigill.intra.peff.net","subject":"Re: [PATCH v3 4/4] tag: use pgp_verify_function in tag -v call","fromName":"Santiago Torres","fromEmail":"santiago@nyu.edu","sentAt":"2016-04-04T18:24:48Z","receivedAt":"2016-04-04T18:24:48Z","isPatch":true,"sender":{"key":"santiago@nyu.edu","avatar":"https://avatars.githubusercontent.com/u/3579933?v=4"},"body":"On Mon, Apr 04, 2016 at 09:38:54AM -0400, Jeff King wrote:\n> On Mon, Apr 04, 2016 at 12:12:04AM -0400, Santiago Torres wrote:\n> \n> > > As a side note, it might actually be an improvement for pgp_verify_tag\n> > > to take a sha1 (so that git-tag is sure that it is verifying the same\n> > > object that it is printing), but that refactoring should probably come\n> > > separately, I think.\n> > \n> > Just to be sure, this refactoring is something we should still include\n> > in this set of patches, right? I think that otherwise we'd lose the\n> > desambigutaion that git tag -v does in this patch.\n> \n> I think it can be part of this series, but doesn't have to be. As I\n> understand it, the current code is just handing the name to the `git\n> verify-tag` process, so if we continue to do so, that would be OK.\n\nIIRC, the current code for git tag -v hands the hex-representation[1] of\nthe sha1 to git verify-tag --- I believe that's related to the\ndesamgibuation issue I've seen people discuss.  I think this behavior is\nlost unless we add this on top of the patch.\n\n> \n> > I also think that most of the rippling is gone if we use and adaptor as\n> > you suggested. Should I add a patch on top of this to support a sha1 as\n> > part for gpg_verify_tag()?\n> \n> Yes, though I'd generally advise against a function taking either a name or\n> a sha1, and ignoring the other option. That often leads to confusing\n> interfaces for the callers. Instead, perhaps just take the sha1, and let\n> the caller do the get_sha1() themselves. Or possibly provide two\n> functions, one of which is a convenience to translate the name to sha1\n> and then call the other.\n\nI think the former sounds easier. I can replace the name argument and\nmove the sha1-resolution code to in verify-tag. git tag -v already\nresolves the tagname to a sha1, so it is easier there.\n\nDoes this sound reasonable? \n\nThanks!\n-Santiago\n\n[1] https://git.kernel.org/cgit/git/git.git/tree/builtin/tag.c#n109\n"},{"id":"282643","messageId":"20160404201937.GB4570@sigill.intra.peff.net","threadId":"41905","inReplyTo":"20160404182447.GA6773@LykOS","subject":"Re: [PATCH v3 4/4] tag: use pgp_verify_function in tag -v call","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-04-04T20:19:37Z","receivedAt":"2016-04-04T20:19:37Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Apr 04, 2016 at 02:24:48PM -0400, Santiago Torres wrote:\n\n> > I think it can be part of this series, but doesn't have to be. As I\n> > understand it, the current code is just handing the name to the `git\n> > verify-tag` process, so if we continue to do so, that would be OK.\n> \n> IIRC, the current code for git tag -v hands the hex-representation[1] of\n> the sha1 to git verify-tag --- I believe that's related to the\n> desamgibuation issue I've seen people discuss.  I think this behavior is\n> lost unless we add this on top of the patch.\n\nOh, you're right. I didn't notice that. So yeah, we should make sure in\nthis series to hand the sha1 over to gpg_verify_tag().\n\n> > Yes, though I'd generally advise against a function taking either a name or\n> > a sha1, and ignoring the other option. That often leads to confusing\n> > interfaces for the callers. Instead, perhaps just take the sha1, and let\n> > the caller do the get_sha1() themselves. Or possibly provide two\n> > functions, one of which is a convenience to translate the name to sha1\n> > and then call the other.\n> \n> I think the former sounds easier. I can replace the name argument and\n> move the sha1-resolution code to in verify-tag. git tag -v already\n> resolves the tagname to a sha1, so it is easier there.\n> \n> Does this sound reasonable?\n\nYes, I think that is a good solution.\n\nThanks.\n\n-Peff\n"}]}