{"thread":{"id":"41925","subject":"[PATCH v4 4/6] tag.c: Replace varialbe name for readability","startedAt":"2016-04-04T22:22:09Z","lastAt":"2016-04-05T03:51:38Z","messageCount":16,"participants":["santiago@nyu.edu","Eric Sunshine","Santiago Torres"],"isPatch":true,"patchVersion":4,"patchTotal":6},"messages":[{"id":"282654","messageId":"1459808535-8895-1-git-send-email-santiago@nyu.edu","threadId":"41925","inReplyTo":null,"subject":"[PATCH v4 0/6] tag: move PGP verification code to tag.c","fromName":"","fromEmail":"santiago@nyu.edu","sentAt":"2016-04-04T22:22:09Z","receivedAt":"2016-04-04T22:22:09Z","isPatch":true,"sender":{"key":"santiago@nyu.edu","avatar":"https://avatars.githubusercontent.com/u/3579933?v=4"},"body":"This is a follow up of [1], [2], and [3]:\n\nv4 (this):\n\nThanks Eric, Peff, 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[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[3] http://git.661346.n2.nabble.com/PATCH-v3-0-4-tag-move-PGP-verification-code-to-tag-c-td7652334.html\n"},{"id":"282656","messageId":"1459808535-8895-2-git-send-email-santiago@nyu.edu","threadId":"41925","inReplyTo":"1459808535-8895-1-git-send-email-santiago@nyu.edu","subject":"[PATCH v4 1/6] builtin/verify-tag.c: Ignore SIGPIPE on gpg-interface","fromName":"","fromEmail":"santiago@nyu.edu","sentAt":"2016-04-04T22:22:10Z","receivedAt":"2016-04-04T22:22:10Z","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\tNote: On this version i moved the sigchain_push below the return error\nin the same way that sign_buffer does(). Thanks to Johanes Sixt for catching\nthis. \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":"282653","messageId":"1459808535-8895-3-git-send-email-santiago@nyu.edu","threadId":"41925","inReplyTo":"1459808535-8895-1-git-send-email-santiago@nyu.edu","subject":"[PATCH v4 2/6] t/t7030-verify-tag.sh: Adds validation for multiple tags","fromName":"","fromEmail":"santiago@nyu.edu","sentAt":"2016-04-04T22:22:11Z","receivedAt":"2016-04-04T22:22:11Z","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 tags separately and then\ncompares the result against the invocation with the same three tags as\nan argument. The results shouldn't differ.\n\nSigned-off-by: Santiago Torres <santiago@nyu.edu>\n---\n\tNote: In this version we don't check or uniq for the verify output, but\ninstead compare against an invocation for each of the three tags indivdually.\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..f9161332 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 GOODSIG <expect.stderr.1 >expect.stderr &&\n+\tgit verify-tag -v --raw $tags >actual.stdout 2>actual.stderr.1 &&\n+\tgrep GOODSIG <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":"282655","messageId":"1459808535-8895-4-git-send-email-santiago@nyu.edu","threadId":"41925","inReplyTo":"1459808535-8895-1-git-send-email-santiago@nyu.edu","subject":"[PATCH v4 3/6] builtin/verify-tag: move verification code to tag.c","fromName":"","fromEmail":"santiago@nyu.edu","sentAt":"2016-04-04T22:22:12Z","receivedAt":"2016-04-04T22:22: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 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 gpg_verify_tag() to avoid\nconflicts with the mktag.c function.\n\nSigned-off-by: Santiago Torres <santiago@nyu.edu>\n---\n\tNote: In this version, I just blndly moved code around and renamed\nthe function's name for ease of review. The following patch only renames\nvariables.\n\n builtin/verify-tag.c | 51 +--------------------------------------------------\n tag.c                | 49 +++++++++++++++++++++++++++++++++++++++++++++++++\n tag.h                |  1 +\n 3 files changed, 51 insertions(+), 50 deletions(-)\n\ndiff --git a/builtin/verify-tag.c b/builtin/verify-tag.c\nindex 77f070a..7e36d53 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 (gpg_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..81e86e6 100644\n--- a/tag.c\n+++ b/tag.c\n@@ -6,6 +6,55 @@\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 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+       }\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+int gpg_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 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..877f180 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 char *name, unsigned flags);\n \n #endif /* TAG_H */\n-- \n2.8.0\n"},{"id":"282652","messageId":"1459808535-8895-5-git-send-email-santiago@nyu.edu","threadId":"41925","inReplyTo":"1459808535-8895-1-git-send-email-santiago@nyu.edu","subject":"[PATCH v4 4/6] tag.c: Replace varialbe name for readability","fromName":"","fromEmail":"santiago@nyu.edu","sentAt":"2016-04-04T22:22:13Z","receivedAt":"2016-04-04T22:22: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 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\tNote: this used to be part of the previous patch on v3. I moved the\nrenaming part to a single patch to simplify the review. \n\n tag.c | 11 ++++++-----\n 1 file changed, 6 insertions(+), 5 deletions(-)\n\ndiff --git a/tag.c b/tag.c\nindex 81e86e6..f6443db 100644\n--- a/tag.c\n+++ b/tag.c\n@@ -9,20 +9,21 @@ const char *tag_type = \"tag\";\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_length;\n \tint ret;\n \n \tmemset(&sigc, 0, sizeof(sigc));\n \n-\tlen = parse_signature(buf, size);\n+\tpayload_length = parse_signature(buf, size);\n \n-\tif (size == len) {\n+\tif (size == payload_length) {\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_length);\n \t\treturn error(\"no signature found\");\n        }\n \n-\tret = check_signature(buf, len, buf + len, size - len, &sigc);\n+\tret = check_signature(buf, payload_length, buf + payload_length,\n+\t\t\t\tsize - payload_length, &sigc);\n \tprint_signature_buffer(&sigc, flags);\n \n \tsignature_check_clear(&sigc);\n-- \n2.8.0\n"},{"id":"282658","messageId":"1459808535-8895-6-git-send-email-santiago@nyu.edu","threadId":"41925","inReplyTo":"1459808535-8895-1-git-send-email-santiago@nyu.edu","subject":"[PATCH v4 5/6] tag: use pgp_verify_function in tag -v call","fromName":"","fromEmail":"santiago@nyu.edu","sentAt":"2016-04-04T22:22:14Z","receivedAt":"2016-04-04T22:22: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\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\tNote: This follows Peff's suggestion to use an adapter, instead of \nchanging the the fn* pointer structure to match gpg_verify_tag.\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..f4450f8 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(name, GPG_VERIFY_VERBOSE);\n }\n \n static int do_sign(struct strbuf *buffer)\n-- \n2.8.0\n"},{"id":"282657","messageId":"1459808535-8895-7-git-send-email-santiago@nyu.edu","threadId":"41925","inReplyTo":"1459808535-8895-1-git-send-email-santiago@nyu.edu","subject":"[PATCH v4 6/6] tag.c: Change gpg_verify_tag argument to sha1","fromName":"","fromEmail":"santiago@nyu.edu","sentAt":"2016-04-04T22:22:15Z","receivedAt":"2016-04-04T22:22: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\nThe gpg_verify_tag function resolves the ref for any existing object.\nHowever, git tag -v resolves to only tag-refs. We can provide support\nfor sha1 by moving the refname resolution code out of gpg_verify_tag and\nallow for the object's sha1 as an argument.\n\nSigned-off-by: Santiago Torres <santiago@nyu.edu>\n---\n builtin/tag.c        |  2 +-\n builtin/verify-tag.c |  9 +++++++--\n tag.c                | 12 ++++--------\n tag.h                |  2 +-\n 4 files changed, 13 insertions(+), 12 deletions(-)\n\ndiff --git a/builtin/tag.c b/builtin/tag.c\nindex f4450f8..398c892 100644\n--- a/builtin/tag.c\n+++ b/builtin/tag.c\n@@ -104,7 +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-\treturn gpg_verify_tag(name, GPG_VERIFY_VERBOSE);\n+\treturn gpg_verify_tag(sha1, GPG_VERIFY_VERBOSE);\n }\n \n static int do_sign(struct strbuf *buffer)\ndiff --git a/builtin/verify-tag.c b/builtin/verify-tag.c\nindex 7e36d53..2ff01d8 100644\n--- a/builtin/verify-tag.c\n+++ b/builtin/verify-tag.c\n@@ -30,6 +30,7 @@ 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 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 +47,12 @@ 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 (gpg_verify_tag(argv[i++], flags))\n+\twhile (i < argc) {\n+\t\tif (get_sha1(argv[i++], sha1))\n+\t\t\treturn error(\"tag '%s' not found.\", argv[i]);\n+\n+\t\tif (gpg_verify_tag(sha1, flags))\n \t\t\thad_error = 1;\n+\t}\n \treturn had_error;\n }\ndiff --git a/tag.c b/tag.c\nindex f6443db..8ac9de5 100644\n--- a/tag.c\n+++ b/tag.c\n@@ -30,25 +30,21 @@ static int run_gpg_verify(const char *buf, unsigned long size, unsigned flags)\n \treturn ret;\n }\n \n-int gpg_verify_tag(const char *name, unsigned flags)\n+int gpg_verify_tag(const unsigned char *sha1, 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+\t\treturn error(\"cannot verify a non-tag object of type %s.\",\n+\t\t\t\ttypename(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(\"unable to read file.\");\n \n \tret = run_gpg_verify(buf, size, flags);\n \ndiff --git a/tag.h b/tag.h\nindex 877f180..cb643b9 100644\n--- a/tag.h\n+++ b/tag.h\n@@ -17,6 +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 char *name, unsigned flags);\n+extern int gpg_verify_tag(const unsigned char *sha1, unsigned flags);\n \n #endif /* TAG_H */\n-- \n2.8.0\n"},{"id":"282669","messageId":"CAPig+cQJ6EzJ0976NJBHXRZyuTf1dr+0ZOvzrCfiTVZOp6fXBA@mail.gmail.com","threadId":"41925","inReplyTo":"1459808535-8895-3-git-send-email-santiago@nyu.edu","subject":"Re: [PATCH v4 2/6] t/t7030-verify-tag.sh: Adds validation for multiple tags","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2016-04-05T01:27:09Z","receivedAt":"2016-04-05T01:27:09Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Mon, Apr 4, 2016 at 6:22 PM,  <santiago@nyu.edu> wrote:\n> t/t7030-verify-tag.sh: Adds validation for multiple tags\n\nRewrite:\n\n    t7030: test verify-tag with multiple tags\n\nThe leading \"t/\" and the trailing \"-*.sh\" were dropped since they add no value.\n\n> The verify-tag command supports mutliple tag names as an argument.\n\ns/mutliple/multiple/\n\n> However, no previous tests try to verify multiple tags at once. This\n> test runs the verify-tag command against three tags separately and then\n> compares the result against the invocation with the same three tags as\n> an argument. The results shouldn't differ.\n\nThe bulk of this description is merely repeating what the patch itself\nalready says, thus is redundant. The entire commit message could\nprobably be collapsed to:\n\n    t7030: test verify-tag with multiple tags\n\n    git-verify-tag accepts multiple tags as arguments, however,\n    existing tests only ever invoke it with a single tag, so add a\n    test invoking it with multiple tags.\n\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> index 4608e71..f9161332 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> +test_expect_success GPG 'verify multiple tags' '\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\nHmm, I had expected you to adopt Peff's suggestion[1] for the greps:\n\n    grep '^.GNUPG:.' ...\n\n[1]: http://article.gmane.org/gmane.comp.version-control.git/290691\n\n> +       test_cmp expect.stdout actual.stdout &&\n> +       test_cmp expect.stderr actual.stderr\n> +'\n> +\n>  test_done\n> --\n> 2.8.0\n"},{"id":"282670","messageId":"CAPig+cSs_gYgQ6dO0SpSwEgUd0Pm3uwFcSYeXqYMqxRC+NMVQg@mail.gmail.com","threadId":"41925","inReplyTo":"1459808535-8895-5-git-send-email-santiago@nyu.edu","subject":"Re: [PATCH v4 4/6] tag.c: Replace varialbe name for readability","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2016-04-05T01:30:47Z","receivedAt":"2016-04-05T01:30:47Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Mon, Apr 4, 2016 at 6:22 PM,  <santiago@nyu.edu> wrote:\n> tag.c: Replace varialbe name for readability\n\ns/Replace/replace/\ns/varialbe/variable/\n\n> The run_gpg_verify function has two variables size, and len. This may\n> 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>\n> Signed-off-by: Santiago Torres <santiago@nyu.edu>\n> ---\n"},{"id":"282671","messageId":"CAPig+cQkqaYd2Vpgpd5jB8ZUD41VvjryB8tYc9JNEydNaKrr=Q@mail.gmail.com","threadId":"41925","inReplyTo":"1459808535-8895-6-git-send-email-santiago@nyu.edu","subject":"Re: [PATCH v4 5/6] tag: use pgp_verify_function in tag -v call","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2016-04-05T01:39:29Z","receivedAt":"2016-04-05T01:39:29Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Mon, Apr 4, 2016 at 6:22 PM,  <santiago@nyu.edu> wrote:\n> Instead of running the verify-tag plumbing command, we use the\n> pgp_verify_tag(). This avoids the usage of an extra fork call. To do\n> this, we extend the number of parameters that tag.c takes, and\n> verify-tag passes. Redundant calls done in the pgp_verify_tag function\n> are removed.\n\nI'm confused about everything following \"an extra fork call\" since\nthose subsequent sentences don't seem to pertain to this patch. Is\nthat leftover gunk from the previous version of this series?\n\n> Signed-off-by: Santiago Torres <santiago@nyu.edu>\n> ---\n> diff --git a/builtin/tag.c b/builtin/tag.c\n> index 1705c94..f4450f8 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(name, GPG_VERIFY_VERBOSE);\n>  }\n>\n>  static int do_sign(struct strbuf *buffer)\n> --\n"},{"id":"282672","messageId":"20160405014354.GA12006@LykOS","threadId":"41925","inReplyTo":"CAPig+cQkqaYd2Vpgpd5jB8ZUD41VvjryB8tYc9JNEydNaKrr=Q@mail.gmail.com","subject":"Re: [PATCH v4 5/6] tag: use pgp_verify_function in tag -v call","fromName":"Santiago Torres","fromEmail":"santiago@nyu.edu","sentAt":"2016-04-05T01:43:55Z","receivedAt":"2016-04-05T01:43:55Z","isPatch":true,"sender":{"key":"santiago@nyu.edu","avatar":"https://avatars.githubusercontent.com/u/3579933?v=4"},"body":"On Mon, Apr 04, 2016 at 09:39:29PM -0400, Eric Sunshine wrote:\n> On Mon, Apr 4, 2016 at 6:22 PM,  <santiago@nyu.edu> wrote:\n> > Instead of running the verify-tag plumbing command, we use the\n> > pgp_verify_tag(). This avoids the usage of an extra fork call. To do\n> > this, we extend the number of parameters that tag.c takes, and\n> > verify-tag passes. Redundant calls done in the pgp_verify_tag function\n> > are removed.\n> \n> I'm confused about everything following \"an extra fork call\" since\n> those subsequent sentences don't seem to pertain to this patch. Is\n> that leftover gunk from the previous version of this series?\n\nYes, I thought I had taken this part of the commit message away. \nI'll take it out right away. Apologies for this.\n\n-Santiago.\n"},{"id":"282673","messageId":"20160405014609.GB12006@LykOS","threadId":"41925","inReplyTo":"CAPig+cQJ6EzJ0976NJBHXRZyuTf1dr+0ZOvzrCfiTVZOp6fXBA@mail.gmail.com","subject":"Re: [PATCH v4 2/6] t/t7030-verify-tag.sh: Adds validation for multiple tags","fromName":"Santiago Torres","fromEmail":"santiago@nyu.edu","sentAt":"2016-04-05T01:46:10Z","receivedAt":"2016-04-05T01:46:10Z","isPatch":true,"sender":{"key":"santiago@nyu.edu","avatar":"https://avatars.githubusercontent.com/u/3579933?v=4"},"body":"> \n> The bulk of this description is merely repeating what the patch itself\n> already says, thus is redundant. The entire commit message could\n> probably be collapsed to:\n> \n>     t7030: test verify-tag with multiple tags\n> \n>     git-verify-tag accepts multiple tags as arguments, however,\n>     existing tests only ever invoke it with a single tag, so add a\n>     test invoking it with multiple tags.\n\nYeah, this is actually clearer. Sorry for the academ-ose commit message.\n\n> > +test_expect_success GPG 'verify multiple tags' '\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> \n> Hmm, I had expected you to adopt Peff's suggestion[1] for the greps:\n> \n>     grep '^.GNUPG:.' ...\n> \n> [1]: http://article.gmane.org/gmane.comp.version-control.git/290691\n\nI thought this was an stylistic thing. I can of course adopt this\nsuggestion.\n\nThanks,\n-Santiago\n"},{"id":"282677","messageId":"CAPig+cQ4i86JpLFe0tqA-tjFj6Y3DvxDz3nFL8XAMVDBLwLUPg@mail.gmail.com","threadId":"41925","inReplyTo":"1459808535-8895-7-git-send-email-santiago@nyu.edu","subject":"Re: [PATCH v4 6/6] tag.c: Change gpg_verify_tag argument to sha1","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2016-04-05T02:00:17Z","receivedAt":"2016-04-05T02:00:17Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Mon, Apr 4, 2016 at 6:22 PM,  <santiago@nyu.edu> wrote:\n> tag.c: Change gpg_verify_tag argument to sha1\n\ns/Change/change/\n\n> The gpg_verify_tag function resolves the ref for any existing object.\n> However, git tag -v resolves to only tag-refs. We can provide support\n> for sha1 by moving the refname resolution code out of gpg_verify_tag and\n> allow for the object's sha1 as an argument.\n\nThis description leaves me fairly clueless about why this change is\nbeing made since justification seems to be lacking. More about this\nbelow...\n\n> Signed-off-by: Santiago Torres <santiago@nyu.edu>\n> ---\n> diff --git a/builtin/tag.c b/builtin/tag.c\n> @@ -104,7 +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> -       return gpg_verify_tag(name, GPG_VERIFY_VERBOSE);\n> +       return gpg_verify_tag(sha1, GPG_VERIFY_VERBOSE);\n>  }\n\nSo, by this point, 'name' has already been resolved to 'sha1', thus\nthis change avoids a second resolution of 'name' inside\ngpg_verify_tag(). Therefore, this is really an optimization, right?\nPerhaps the intent of the patch would be clearer if the commit message\nsold it as such. For instance, the commit message might start off:\n\n    tag: avoid resolving tag name twice\n\nand then go on to say that by hefting tag name resolution out of\ngpg_verify_tag(), the extra resolution can be avoided.\n\n>  static int do_sign(struct strbuf *buffer)\n> diff --git a/builtin/verify-tag.c b/builtin/verify-tag.c\n> @@ -46,8 +47,12 @@ int cmd_verify_tag(int argc, const char **argv, const char *prefix)\n>         if (verbose)\n>                 flags |= GPG_VERIFY_VERBOSE;\n>\n> -       while (i < argc)\n> -               if (gpg_verify_tag(argv[i++], flags))\n> +       while (i < argc) {\n> +               if (get_sha1(argv[i++], sha1))\n> +                       return error(\"tag '%s' not found.\", argv[i]);\n\nWhy does this 'return' after the first error, but the gpg_verify_tag()\ncall below merely sets a 'had_error' flag and continues? I would\nexpect this one to set the flag and continue, as well.\n\n> +\n\nStyle: unnecessary blank line\n\n> +               if (gpg_verify_tag(sha1, flags))\n>                         had_error = 1;\n> +       }\n>         return had_error;\n>  }\n> diff --git a/tag.c b/tag.c\n> @@ -30,25 +30,21 @@ static int run_gpg_verify(const char *buf, unsigned long size, unsigned flags)\n> -int gpg_verify_tag(const char *name, unsigned flags)\n> +int gpg_verify_tag(const unsigned char *sha1, unsigned flags)\n>  {\n>         enum object_type type;\n> -       unsigned char sha1[20];\n>         char *buf;\n>         unsigned long size;\n>         int ret;\n>\n> -       if (get_sha1(name, sha1))\n> -               return error(\"tag '%s' not found.\", name);\n> -\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> +               return error(\"cannot verify a non-tag object of type %s.\",\n> +                               typename(type));\n\nThis error message becomes much less useful since it now only says\nthat there is a problem with *some* tag but doesn't give any\nidentifying information. How about including the sha1 in the error\nmessage?\n\n>\n>         buf = read_sha1_file(sha1, &type, &size);\n>         if (!buf)\n> -               return error(\"%s: unable to read file.\", name);\n> +               return error(\"unable to read file.\");\n\nDitto regarding making this more useful by including the sha1.\n\n>         ret = run_gpg_verify(buf, size, flags);\n"},{"id":"282678","messageId":"20160405021028.GE12006@LykOS","threadId":"41925","inReplyTo":"CAPig+cQ4i86JpLFe0tqA-tjFj6Y3DvxDz3nFL8XAMVDBLwLUPg@mail.gmail.com","subject":"Re: [PATCH v4 6/6] tag.c: Change gpg_verify_tag argument to sha1","fromName":"Santiago Torres","fromEmail":"santiago@nyu.edu","sentAt":"2016-04-05T02:10:29Z","receivedAt":"2016-04-05T02:10:29Z","isPatch":true,"sender":{"key":"santiago@nyu.edu","avatar":"https://avatars.githubusercontent.com/u/3579933?v=4"},"body":"On Mon, Apr 04, 2016 at 10:00:17PM -0400, Eric Sunshine wrote:\n> On Mon, Apr 4, 2016 at 6:22 PM,  <santiago@nyu.edu> wrote:\n> > tag.c: Change gpg_verify_tag argument to sha1\n> \n> s/Change/change/\n\nSorry I've been consistently missing these... \n> \n> > The gpg_verify_tag function resolves the ref for any existing object.\n> > However, git tag -v resolves to only tag-refs. We can provide support\n> > for sha1 by moving the refname resolution code out of gpg_verify_tag and\n> > allow for the object's sha1 as an argument.\n> \n> This description leaves me fairly clueless about why this change is\n> being made since justification seems to be lacking. More about this\n> below...\n> \n> > Signed-off-by: Santiago Torres <santiago@nyu.edu>\n> > ---\n> > diff --git a/builtin/tag.c b/builtin/tag.c\n> > @@ -104,7 +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> > -       return gpg_verify_tag(name, GPG_VERIFY_VERBOSE);\n> > +       return gpg_verify_tag(sha1, GPG_VERIFY_VERBOSE);\n> >  }\n> \n> So, by this point, 'name' has already been resolved to 'sha1', thus\n> this change avoids a second resolution of 'name' inside\n> gpg_verify_tag(). Therefore, this is really an optimization, right?\n> Perhaps the intent of the patch would be clearer if the commit message\n> sold it as such. For instance, the commit message might start off:\n> \n>     tag: avoid resolving tag name twice\n> \n> and then go on to say that by hefting tag name resolution out of\n> gpg_verify_tag(), the extra resolution can be avoided.\n\nYep, this is actually true, but something I didn't consider. I think\nthat, from what I could draw on [1] and [2], git tag -v is reserved to\ntags only (refs/tags iirc). This patch makes it so that this behavior is\nnot lost. I'm not sure if it should be separate from 5/6 though. \n> \n> >  static int do_sign(struct strbuf *buffer)\n> > diff --git a/builtin/verify-tag.c b/builtin/verify-tag.c\n> > @@ -46,8 +47,12 @@ int cmd_verify_tag(int argc, const char **argv, const char *prefix)\n> >         if (verbose)\n> >                 flags |= GPG_VERIFY_VERBOSE;\n> >\n> > -       while (i < argc)\n> > -               if (gpg_verify_tag(argv[i++], flags))\n> > +       while (i < argc) {\n> > +               if (get_sha1(argv[i++], sha1))\n> > +                       return error(\"tag '%s' not found.\", argv[i]);\n> \n> Why does this 'return' after the first error, but the gpg_verify_tag()\n> call below merely sets a 'had_error' flag and continues? I would\n> expect this one to set the flag and continue, as well.\n\nThis sounds better than what I had thought. I'll set had error and do\ncontinue instead.\n> \n >  {\n> >         enum object_type type;\n> > -       unsigned char sha1[20];\n> >         char *buf;\n> >         unsigned long size;\n> >         int ret;\n> >\n> > -       if (get_sha1(name, sha1))\n> > -               return error(\"tag '%s' not found.\", name);\n> > -\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> > +               return error(\"cannot verify a non-tag object of type %s.\",\n> > +                               typename(type));\n> \n> This error message becomes much less useful since it now only says\n> that there is a problem with *some* tag but doesn't give any\n> identifying information. How about including the sha1 in the error\n> message?\n> \n> >\n> >         buf = read_sha1_file(sha1, &type, &size);\n> >         if (!buf)\n> > -               return error(\"%s: unable to read file.\", name);\n> > +               return error(\"unable to read file.\");\n> \n> Ditto regarding making this more useful by including the sha1.\n\nyes, I wasn't sure about how to move forward here. I'll replace the name\nwith the sha1 instead of just removing it.\n\nThanks!\n-Santiago.\n\n\n[1] https://git.kernel.org/cgit/git/git.git/tree/builtin/tag.c\n[2]\nhttp://git.661346.n2.nabble.com/PATCH-v3-0-4-tag-move-PGP-verification-code-to-tag-c-tp7652334p7652437.html\n"},{"id":"282680","messageId":"CAPig+cSkVtXezozamFohzRyvH46oO3ZFDBXzpV4fHtUUm3epVw@mail.gmail.com","threadId":"41925","inReplyTo":"20160405021028.GE12006@LykOS","subject":"Re: [PATCH v4 6/6] tag.c: Change gpg_verify_tag argument to sha1","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2016-04-05T03:46:27Z","receivedAt":"2016-04-05T03:46:27Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Mon, Apr 4, 2016 at 10:10 PM, Santiago Torres <santiago@nyu.edu> wrote:\n> On Mon, Apr 04, 2016 at 10:00:17PM -0400, Eric Sunshine wrote:\n>> On Mon, Apr 4, 2016 at 6:22 PM,  <santiago@nyu.edu> wrote:\n>> > -       return gpg_verify_tag(name, GPG_VERIFY_VERBOSE);\n>> > +       return gpg_verify_tag(sha1, GPG_VERIFY_VERBOSE);\n>>\n>> So, by this point, 'name' has already been resolved to 'sha1', thus\n>> this change avoids a second resolution of 'name' inside\n>> gpg_verify_tag(). Therefore, this is really an optimization, right?\n>> Perhaps the intent of the patch would be clearer if the commit message\n>> sold it as such. For instance, the commit message might start off:\n>>\n>>     tag: avoid resolving tag name twice\n>>\n>> and then go on to say that by hefting tag name resolution out of\n>> gpg_verify_tag(), the extra resolution can be avoided.\n>\n> Yep, this is actually true, but something I didn't consider. I think\n> that, from what I could draw on [1] and [2], git tag -v is reserved to\n> tags only (refs/tags iirc). This patch makes it so that this behavior is\n> not lost. I'm not sure if it should be separate from 5/6 though.\n\nOkay, so this is a fix for a regression introduced by the previous\npatch. I agree that this is suboptimal. You can avoid this regression\nissue altogether by merely re-ordering the patch series. For instance,\nthe series could be ordered like this:\n\n1. SIGPIPE dance\n2. add new t7030 test\n3. improve variable name in verify_tag()\n4. heft get_sha1() lookup out of verify_tag()\n5. move code and publish gpg_verify_tag()\n6. re-implement \"git-tag -v\" in terms of gpg_verify_tag()\n\nSell patch #4 as a libification preparation step, explaining as\njustification that some future clients may already have the sha1 in\nhand and would want to avoid duplicate resolution of the tag name.\n"},{"id":"282681","messageId":"CAPig+cTkQn45ax5pEGghj6_c=FKwP=ApxRo85Jyyu1UD28FrJA@mail.gmail.com","threadId":"41925","inReplyTo":"20160405014609.GB12006@LykOS","subject":"Re: [PATCH v4 2/6] t/t7030-verify-tag.sh: Adds validation for multiple tags","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2016-04-05T03:51:38Z","receivedAt":"2016-04-05T03:51:38Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Mon, Apr 4, 2016 at 9:46 PM, Santiago Torres <santiago@nyu.edu> wrote:\n> Eric Sunshine wrote:\n>> > +test_expect_success GPG 'verify multiple tags' '\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>>\n>> Hmm, I had expected you to adopt Peff's suggestion[1] for the greps:\n>>\n>>     grep '^.GNUPG:.' ...\n>>\n>> [1]: http://article.gmane.org/gmane.comp.version-control.git/290691\n>\n> I thought this was an stylistic thing. I can of course adopt this\n> suggestion.\n\nIt's probably not a big deal, but Peff's suggestion (at least feels\nlike it) makes the test a bit more comprehensive.\n\nBy the way, as the test is heavily inspired by Peff's example, it\nmight be worth giving him a nod via a Helped-by: just above your\nSigned-off-by:.\n"}]}