{"thread":{"id":"42056","subject":"[PATCH v6 3/6] verify-tag: change variable name for readability","startedAt":"2016-04-17T22:26:55Z","lastAt":"2016-04-19T05:16:12Z","messageCount":15,"participants":["santiago@nyu.edu","Eric Sunshine","Junio C Hamano","Jeff King"],"isPatch":true,"patchVersion":6,"patchTotal":6},"messages":[{"id":"283703","messageId":"1460932021-27633-1-git-send-email-santiago@nyu.edu","threadId":"42056","inReplyTo":null,"subject":"[PATCH v6 0/6] Move PGP verification out of verify-tag","fromName":"","fromEmail":"santiago@nyu.edu","sentAt":"2016-04-17T22:26:55Z","receivedAt":"2016-04-17T22:26:55Z","isPatch":true,"sender":{"key":"santiago@nyu.edu","avatar":"https://avatars.githubusercontent.com/u/3579933?v=4"},"body":"From: Santiago Torres \n\nThis is a follow up of [1], [2], [3], [4], and [5]. patches 1/6, 2/6 and 3/6\nare the same as the corresponding commits in pu.\n\nv6: \n * As Junio suggested, updated 4/6, to include the name argument and the\n ternary operator to provide more descriptive error messages. I propagated\n these changes to 5/6 and 6/6 as well. I'm unsure about the 80-column\n on 4/6, the ternary operator is rather long.\n * Updated and reviewed the commit messages based on Eric and Junio's\n feedback\n\nv5:\nAdded helpful feedback by Eric\n\n * Reordering of the patches, to avoid temporal inclusion of a regression\n * Fix typos here and there.\n * Review commit messages, as some weren't representative of what the patches\n were doing anymore.\n * Updated t7030 to include Peff's suggestion, and added a helped-by line here\n as it was mostly Peff's code.\n * Updated the error-handling/printing issues that were introduced when.\n libifying the verify_tag function.\n\nv4:\n\nThanks Eric, Jeff, and Hannes for the feedback.\n\n * I relocated the sigchain_push call so it comes after the error on\n gpg-interface (thanks Hannnes for catching this).\n * I updated the unit test to match the discussion on [3]. Now it generates\n the expected output of the tag on the fly for comparison. (This is just\n copy and paste from [3], but I verified that it works by breaking the\n while)\n * I split moving the code and renaming the variables into two patches so\n these are easier to review.\n * I used an adapter on builtin/tag.c instead of redefining all the fn*\n declarations everywhere. This introduces an issue with the way git tag -v\n resolves refnames though. I added a new commit to restore the previous\n behavior of git-tag. I'm not sure if I should've split this into two commits\n though.\n\nv3:\nThanks Eric, Jeff, for the feedback.\n\n * I separated the patch in multiple sub-patches.\n * I compared the behavior of previous git tag -v and git verify-tag \n invocations to make sure the behavior is the same\n * I dropped the multi-line comment, as suggested.\n * I fixed the issue with the missing brackets in the while (this is \n now detected by the test).\n\nv2:\n\n * I moved the pgp-verification code to tag.c \n * I added extra arguments so git tag -v and git verify-tag both work\n with the same function\n * Relocated the SIGPIPE handling code in verify-tag to gpg-interface\n\nv1:\n \nThe verify tag function is just a thin wrapper around the verify-tag\ncommand. We can avoid one fork call by doing the verification inside\nthe tag builtin instead.\n\n\nThis applies on v2.8.0. \nThanks!\n-Santiago\n\n[1] http://thread.gmane.org/gmane.comp.version-control.git/287649\n[2] http://thread.gmane.org/gmane.comp.version-control.git/289836\n[3] http://thread.gmane.org/gmane.comp.version-control.git/290608\n[4] http://thread.gmane.org/gmane.comp.version-control.git/290731\n[5] http://thread.gmane.org/gmane.comp.version-control.git/290790\n\nSantiago Torres (6):\n builtin/verify-tag.c: ignore SIGPIPE in gpg-interface\n t7030: test verifying multiple tags\n verify-tag: change variable name for readability\n verify-tag: add sha1 argument to verify_tag()\n verify-tag: move verification code to tag.c\n tag -v: verfy directly rather than exec-ing verify-tag\n\n builtin/tag.c | 8 +------\n builtin/verify-tag.c | 64 ++++++++-------------------------------------------\n gpg-interface.c | 2 ++\n t/t7030-verify-tag.sh | 13 +++++++++++\n tag.c | 49 +++++++++++++++++++++++++++++++++++++++\n tag.h | 3 ++-\n 6 files changed, 77 insertions(+), 62 deletions(-)\n\n-- \n2.8.0\n"},{"id":"283706","messageId":"1460932021-27633-2-git-send-email-santiago@nyu.edu","threadId":"42056","inReplyTo":"1460932021-27633-1-git-send-email-santiago@nyu.edu","subject":"[PATCH v6 1/6] builtin/verify-tag.c: ignore SIGPIPE in gpg-interface","fromName":"","fromEmail":"santiago@nyu.edu","sentAt":"2016-04-17T22:26:56Z","receivedAt":"2016-04-17T22:26:56Z","isPatch":true,"sender":{"key":"santiago@nyu.edu","avatar":"https://avatars.githubusercontent.com/u/3579933?v=4"},"body":"From: Santiago Torres \n\nThe verify_signed_buffer() function may trigger a SIGPIPE when the\nGPG child process terminates early (due to a bad keyid, for example)\nand Git tries to write to it afterwards. Previously, ignoring\nSIGPIPE was done in builtin/verify-tag.c to avoid this issue.\n\nHowever, any other caller who wants to call verify_signed_buffer()\nwould have to do the same.\n\nUse sigchain_push(SIGPIPE, SIG_IGN) in verify_signed_buffer(),\npretty much like in sign_buffer(), so that any caller is not\nrequired to perform this task.\n\nThis will avoid possible mistakes by further developers using\nverify_signed_buffer().\n\nSigned-off-by: Santiago Torres \nReviewed-by: Eric Sunshine \nSigned-off-by: Junio C Hamano \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":"283704","messageId":"1460932021-27633-3-git-send-email-santiago@nyu.edu","threadId":"42056","inReplyTo":"1460932021-27633-1-git-send-email-santiago@nyu.edu","subject":"[PATCH v6 2/6] t7030: test verifying multiple tags","fromName":"","fromEmail":"santiago@nyu.edu","sentAt":"2016-04-17T22:26:57Z","receivedAt":"2016-04-17T22:26:57Z","isPatch":true,"sender":{"key":"santiago@nyu.edu","avatar":"https://avatars.githubusercontent.com/u/3579933?v=4"},"body":"From: Santiago Torres \n\nThe verify-tag command supports multiple tag names to verify, but\nexisting tests only test for invocation with a single tag.\n\nAdd a test invoking it with multiple tags.\n\nHelped-by: Jeff King \nSigned-off-by: Santiago Torres \nReviewed-by: Eric Sunshine \nSigned-off-by: Junio C Hamano \n---\n t/t7030-verify-tag.sh | 13 +++++++++++++\n 1 file changed, 13 insertions(+)\n\ndiff --git a/t/t7030-verify-tag.sh b/t/t7030-verify-tag.sh\nindex 4608e71..07079a4 100755\n--- a/t/t7030-verify-tag.sh\n+++ b/t/t7030-verify-tag.sh\n@@ -112,4 +112,17 @@ 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\n+\tdo\n+\t\tgit verify-tag -v --raw $i || return 1\n+\tdone >expect.stdout 2>expect.stderr.1 &&\n+\tgrep \"^.GNUPG:.\" expect.stderr &&\n+\tgit verify-tag -v --raw $tags >actual.stdout 2>actual.stderr.1 &&\n+\tgrep \"^.GNUPG:.\" 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":"283700","messageId":"1460932021-27633-4-git-send-email-santiago@nyu.edu","threadId":"42056","inReplyTo":"1460932021-27633-1-git-send-email-santiago@nyu.edu","subject":"[PATCH v6 3/6] verify-tag: change variable name for readability","fromName":"","fromEmail":"santiago@nyu.edu","sentAt":"2016-04-17T22:26:58Z","receivedAt":"2016-04-17T22:26:58Z","isPatch":true,"sender":{"key":"santiago@nyu.edu","avatar":"https://avatars.githubusercontent.com/u/3579933?v=4"},"body":"From: Santiago Torres \n\nThe run_gpg_verify() function has two variables, size and len.\n\nThis may come off as confusing when reading the code. Clarify which\none pertains to the length of the tag headers by renaming len to\npayload_length.\n\nSigned-off-by: Santiago Torres \nReviewed-by: Eric Sunshine \nSigned-off-by: Junio C Hamano \n---\n builtin/verify-tag.c | 11 ++++++-----\n 1 file changed, 6 insertions(+), 5 deletions(-)\n\ndiff --git a/builtin/verify-tag.c b/builtin/verify-tag.c\nindex 77f070a..010353c 100644\n--- a/builtin/verify-tag.c\n+++ b/builtin/verify-tag.c\n@@ -21,20 +21,21 @@ static const char * const verify_tag_usage[] = {\n static int run_gpg_verify(const char *buf, unsigned long size, unsigned flags)\n {\n \tstruct signature_check sigc;\n-\tint len;\n+\tint payload_size;\n \tint ret;\n \n \tmemset(&sigc, 0, sizeof(sigc));\n \n-\tlen = parse_signature(buf, size);\n+\tpayload_size = parse_signature(buf, size);\n \n-\tif (size == len) {\n+\tif (size == payload_size) {\n \t\tif (flags & GPG_VERIFY_VERBOSE)\n-\t\t\twrite_in_full(1, buf, len);\n+\t\t\twrite_in_full(1, buf, payload_size);\n \t\treturn error(\"no signature found\");\n \t}\n \n-\tret = check_signature(buf, len, buf + len, size - len, &sigc);\n+\tret = check_signature(buf, payload_size, buf + payload_size,\n+\t\t\t\tsize - payload_size, &sigc);\n \tprint_signature_buffer(&sigc, flags);\n \n \tsignature_check_clear(&sigc);\n-- \n2.8.0\n"},{"id":"283705","messageId":"1460932021-27633-5-git-send-email-santiago@nyu.edu","threadId":"42056","inReplyTo":"1460932021-27633-1-git-send-email-santiago@nyu.edu","subject":"[PATCH v6 4/6] verify-tag: add sha1 argument to verify_tag()","fromName":"","fromEmail":"santiago@nyu.edu","sentAt":"2016-04-17T22:26:59Z","receivedAt":"2016-04-17T22:26:59Z","isPatch":true,"sender":{"key":"santiago@nyu.edu","avatar":"https://avatars.githubusercontent.com/u/3579933?v=4"},"body":"From: Santiago Torres \n\nThe current interface of verify_tag() resolves reference names to SHA1,\nwhich might be redundant as future callers may resolve the refname to\nSHA1 beforehand.\n\nAdd a SHA1 parameter to use instead of the name parameter. We also\nreplace the name argument to report_name and use it for error reporting\nonly.\n\nHelped-by: Junio C Hamano \nSigned-off-by: Santiago Torres \n---\n builtin/verify-tag.c | 25 ++++++++++++++++---------\n 1 file changed, 16 insertions(+), 9 deletions(-)\n\ndiff --git a/builtin/verify-tag.c b/builtin/verify-tag.c\nindex 010353c..1d1c5c2 100644\n--- a/builtin/verify-tag.c\n+++ b/builtin/verify-tag.c\n@@ -42,25 +42,24 @@ static int run_gpg_verify(const char *buf, unsigned long size, unsigned flags)\n \treturn ret;\n }\n \n-static int verify_tag(const char *name, unsigned flags)\n+static int verify_tag(const unsigned char *sha1, const char *report_name,\n+\t\t\tunsigned 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\t\t\treport_name ? report_name : find_unique_abbrev(sha1, DEFAULT_ABBREV),\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(\"%s: unable to read file.\",\n+\t\t\t\treport_name ? report_name : find_unique_abbrev(sha1, DEFAULT_ABBREV));\n \n \tret = run_gpg_verify(buf, size, flags);\n \n@@ -80,6 +79,8 @@ int cmd_verify_tag(int argc, const char **argv, const char *prefix)\n {\n \tint i = 1, verbose = 0, had_error = 0;\n \tunsigned flags = 0;\n+\tunsigned char sha1[20];\n+\tconst char *name;\n \tconst struct option verify_tag_options[] = {\n \t\tOPT__VERBOSE(&verbose, N_(\"print tag contents\")),\n \t\tOPT_BIT(0, \"raw\", &flags, N_(\"print raw gpg status output\"), GPG_VERIFY_RAW),\n@@ -96,8 +97,14 @@ int cmd_verify_tag(int argc, const char **argv, const char *prefix)\n \tif (verbose)\n \t\tflags |= GPG_VERIFY_VERBOSE;\n \n-\twhile (i < argc)\n-\t\tif (verify_tag(argv[i++], flags))\n+\twhile (i < argc) {\n+\t\tname = argv[i++];\n+\t\tif (get_sha1(name, sha1)) {\n+\t\t\terror(\"tag '%s' not found.\", name);\n \t\t\thad_error = 1;\n+\t\t}\n+\t\telse if (verify_tag(sha1, name, flags))\n+\t\t\thad_error = 1;\n+\t}\n \treturn had_error;\n }\n-- \n2.8.0\n"},{"id":"283701","messageId":"1460932021-27633-6-git-send-email-santiago@nyu.edu","threadId":"42056","inReplyTo":"1460932021-27633-1-git-send-email-santiago@nyu.edu","subject":"[PATCH v6 5/6] verify-tag: move verification code to tag.c","fromName":"","fromEmail":"santiago@nyu.edu","sentAt":"2016-04-17T22:27:00Z","receivedAt":"2016-04-17T22:27:00Z","isPatch":true,"sender":{"key":"santiago@nyu.edu","avatar":"https://avatars.githubusercontent.com/u/3579933?v=4"},"body":"From: Santiago Torres \n\nThe PGP verification routine for tags could be accessed by other modules\nthat require to do so.\n\nPublish the verify_tag function in tag.c and rename it to gpg_verify_tag\nso it does not conflict with builtin/mktag's static function.\n\nHelped-by: Junio C Hamano \nSigned-off-by: Santiago Torres \n---\n builtin/verify-tag.c | 51 +--------------------------------------------------\n tag.c | 49 +++++++++++++++++++++++++++++++++++++++++++++++++\n tag.h | 3 ++-\n 3 files changed, 52 insertions(+), 51 deletions(-)\n\ndiff --git a/builtin/verify-tag.c b/builtin/verify-tag.c\nindex 1d1c5c2..4e3b643 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 payload_size;\n-\tint ret;\n-\n-\tmemset(&sigc, 0, sizeof(sigc));\n-\n-\tpayload_size = parse_signature(buf, size);\n-\n-\tif (size == payload_size) {\n-\t\tif (flags & GPG_VERIFY_VERBOSE)\n-\t\t\twrite_in_full(1, buf, payload_size);\n-\t\treturn error(\"no signature found\");\n-\t}\n-\n-\tret = check_signature(buf, payload_size, buf + payload_size,\n-\t\t\t\tsize - payload_size, &sigc);\n-\tprint_signature_buffer(&sigc, flags);\n-\n-\tsignature_check_clear(&sigc);\n-\treturn ret;\n-}\n-\n-static int verify_tag(const unsigned char *sha1, const char *report_name,\n-\t\t\tunsigned flags)\n-{\n-\tenum object_type type;\n-\tchar *buf;\n-\tunsigned long size;\n-\tint ret;\n-\n-\ttype = sha1_object_info(sha1, NULL);\n-\tif (type != OBJ_TAG)\n-\t\treturn error(\"%s: cannot verify a non-tag object of type %s.\",\n-\t\t\t\treport_name ? report_name : find_unique_abbrev(sha1, DEFAULT_ABBREV),\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.\",\n-\t\t\t\treport_name ? report_name : find_unique_abbrev(sha1, DEFAULT_ABBREV));\n-\n-\tret = run_gpg_verify(buf, size, flags);\n-\n-\tfree(buf);\n-\treturn ret;\n-}\n-\n static int git_verify_tag_config(const char *var, const char *value, void *cb)\n {\n \tint status = git_gpg_config(var, value, cb);\n@@ -103,7 +54,7 @@ int cmd_verify_tag(int argc, const char **argv, const char *prefix)\n \t\t\terror(\"tag '%s' not found.\", name);\n \t\t\thad_error = 1;\n \t\t}\n-\t\telse if (verify_tag(sha1, name, flags))\n+\t\telse if (gpg_verify_tag(sha1, name, flags))\n \t\t\thad_error = 1;\n \t}\n \treturn had_error;\ndiff --git a/tag.c b/tag.c\nindex d72f742..e7f22c6 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 payload_size;\n+\tint ret;\n+\n+\tmemset(&sigc, 0, sizeof(sigc));\n+\n+\tpayload_size = parse_signature(buf, size);\n+\n+\tif (size == payload_size) {\n+\t\tif (flags & GPG_VERIFY_VERBOSE)\n+\t\t\twrite_in_full(1, buf, payload_size);\n+\t\treturn error(\"no signature found\");\n+\t}\n+\n+\tret = check_signature(buf, payload_size, buf + payload_size,\n+\t\t\t\tsize - payload_size, &sigc);\n+\tprint_signature_buffer(&sigc, flags);\n+\n+\tsignature_check_clear(&sigc);\n+\treturn ret;\n+}\n+\n+int gpg_verify_tag(const unsigned char *sha1, const char *report_name,\n+\t\t\tunsigned flags)\n+{\n+\tenum object_type type;\n+\tchar *buf;\n+\tunsigned long size;\n+\tint ret;\n+\n+\ttype = sha1_object_info(sha1, NULL);\n+\tif (type != OBJ_TAG)\n+\t\treturn error(\"%s: cannot verify a non-tag object of type %s.\",\n+\t\t\t\treport_name ? report_name : find_unique_abbrev(sha1, DEFAULT_ABBREV),\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.\",\n+\t\t\t\treport_name ? report_name : find_unique_abbrev(sha1, DEFAULT_ABBREV));\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..1a8d123 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-\n+extern int gpg_verify_tag(const unsigned char *sha1, const char *report_name,\n+\t\t\tunsigned flags);\n #endif /* TAG_H */\n-- \n2.8.0\n"},{"id":"283702","messageId":"1460932021-27633-7-git-send-email-santiago@nyu.edu","threadId":"42056","inReplyTo":"1460932021-27633-1-git-send-email-santiago@nyu.edu","subject":"[PATCH v6 6/6] tag -v: verfy directly rather than exec-ing verify-tag","fromName":"","fromEmail":"santiago@nyu.edu","sentAt":"2016-04-17T22:27:01Z","receivedAt":"2016-04-17T22:27:01Z","isPatch":true,"sender":{"key":"santiago@nyu.edu","avatar":"https://avatars.githubusercontent.com/u/3579933?v=4"},"body":"From: Santiago Torres \n\ntag -v forks into verify-tag, which only calls gpg_verify_tag().\n\nInstead of forking to verify-tag, call gpg_verify_tag directly().\n\nHelped-by: Eric Sunshine \nSigned-off-by: Santiago Torres \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..7b2918e 100644\n--- a/builtin/tag.c\n+++ b/builtin/tag.c\n@@ -104,13 +104,7 @@ static int delete_tag(const char *name, const char *ref,\n static int verify_tag(const char *name, const char *ref,\n \t\t\t\tconst unsigned char *sha1)\n {\n-\tconst char *argv_verify_tag[] = {\"verify-tag\",\n-\t\t\t\t\t\"-v\", \"SHA1_HEX\", NULL};\n-\targv_verify_tag[2] = sha1_to_hex(sha1);\n-\n-\tif (run_command_v_opt(argv_verify_tag, RUN_GIT_CMD))\n-\t\treturn error(_(\"could not verify the tag '%s'\"), name);\n-\treturn 0;\n+\treturn gpg_verify_tag(sha1, name, GPG_VERIFY_VERBOSE);\n }\n \n static int do_sign(struct strbuf *buffer)\n-- \n2.8.0\n"},{"id":"283735","messageId":"CAPig+cTV=RG-fprFwmY+f4Fy_rm77p0B0x652=f+p5vVOX4=Lw@mail.gmail.com","threadId":"42056","inReplyTo":"1460932021-27633-4-git-send-email-santiago@nyu.edu","subject":"Re: [PATCH v6 3/6] verify-tag: change variable name for readability","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2016-04-18T17:20:30Z","receivedAt":"2016-04-18T17:20:30Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sun, Apr 17, 2016 at 6:26 PM, wrote:\n> The run_gpg_verify() function has two variables, size and len.\n>\n> This may come off as confusing when reading the code. Clarify which\n> one pertains to the length of the tag headers by renaming len to\n> payload_length.\n\nThe commit message talks about 'payload_length', but the code names it\n'payload_size'.\n\n> Signed-off-by: Santiago Torres \n> ---\n> builtin/verify-tag.c | 11 ++++++-----\n> 1 file changed, 6 insertions(+), 5 deletions(-)\n>\n> diff --git a/builtin/verify-tag.c b/builtin/verify-tag.c\n> index 77f070a..010353c 100644\n> --- a/builtin/verify-tag.c\n> +++ b/builtin/verify-tag.c\n> @@ -21,20 +21,21 @@ static const char * const verify_tag_usage[] = {\n> static int run_gpg_verify(const char *buf, unsigned long size, unsigned flags)\n> {\n> struct signature_check sigc;\n> - int len;\n> + int payload_size;\n> int ret;\n>\n> memset(&sigc, 0, sizeof(sigc));\n>\n> - len = parse_signature(buf, size);\n> + payload_size = parse_signature(buf, size);\n>\n> - if (size == len) {\n> + if (size == payload_size) {\n> if (flags & GPG_VERIFY_VERBOSE)\n> - write_in_full(1, buf, len);\n> + write_in_full(1, buf, payload_size);\n> return error(\"no signature found\");\n> }\n>\n> - ret = check_signature(buf, len, buf + len, size - len, &sigc);\n> + ret = check_signature(buf, payload_size, buf + payload_size,\n> + size - payload_size, &sigc);\n> print_signature_buffer(&sigc, flags);\n>\n> signature_check_clear(&sigc);\n> --\n"},{"id":"283736","messageId":"CAPig+cSGR-AZPzPRenm8RkOa7XpUOqMAgUw_8p-OUL=qP0Hy6w@mail.gmail.com","threadId":"42056","inReplyTo":"1460932021-27633-5-git-send-email-santiago@nyu.edu","subject":"Re: [PATCH v6 4/6] verify-tag: add sha1 argument to verify_tag()","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2016-04-18T17:41:04Z","receivedAt":"2016-04-18T17:41:04Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sun, Apr 17, 2016 at 6:26 PM, wrote:\n> verify-tag: add sha1 argument to verify_tag()\n\nMentioned previously[1]: This subject is talking about low level\ndetails of the change rather than giving a high-level overview. A\nsuggested replacement[1] would be:\n\n verify-tag: prepare verify_tag() for libification\n\n> The current interface of verify_tag() resolves reference names to SHA1,\n> which might be redundant as future callers may resolve the refname to\n> SHA1 beforehand.\n\nThere is no mention here that the plan is to libify verify_tag() and\n\"might be redundant\" is a somewhat weak way to argue in favor of this\nchange. The commit messages proposed in the previous review[1] was\nmore explicit:\n\n verify_tag() accepts a tag name which it resolves to a SHA1\n before verification, however, the plan is to make this\n functionality public and it is possible that future callers will\n already have a SHA1 in hand. Since it would be wasteful for them\n to supply a tag name only to have it resolved again, change\n verify_tag() to accept a tag SHA1 rather than a name.\n\n> Add a SHA1 parameter to use instead of the name parameter. We also\n> replace the name argument to report_name and use it for error reporting\n> only.\n\nThe patch itself looks okay, though see a few nits below (which may\nnot be worth a re-roll).\n\n[1]: http://article.gmane.org/gmane.comp.version-control.git/290829\n\n> Signed-off-by: Santiago Torres \n> ---\n> diff --git a/builtin/verify-tag.c b/builtin/verify-tag.c\n> @@ -80,6 +79,8 @@ int cmd_verify_tag(int argc, const char **argv, const char *prefix)\n> {\n> int i = 1, verbose = 0, had_error = 0;\n> unsigned flags = 0;\n> + unsigned char sha1[20];\n> + const char *name;\n\nNit: These could have been declared in the scope of the while-loop\n(below) since you've added braces to it.\n\n> @@ -96,8 +97,14 @@ int cmd_verify_tag(int argc, const char **argv, const char *prefix)\n> - while (i < argc)\n> - if (verify_tag(argv[i++], flags))\n> + while (i < argc) {\n> + name = argv[i++];\n> + if (get_sha1(name, sha1)) {\n> + error(\"tag '%s' not found.\", name);\n> had_error = 1;\n\nThese lines could be combined:\n\n had_error = !!error(\"tag '%s' not found.\", name);\n\nwhich would allow you to drop the braces.\n\n> + }\n> + else if (verify_tag(sha1, name, flags))\n> + had_error = 1;\n\nStyle: cuddle '}' and else:\n\n } else\n\n> + }\n> return had_error;\n> }\n"},{"id":"283738","messageId":"CAPig+cTWit2NcVBV+r4o0ztOQx7=4XjNKbeo=HA7EAxasw6HsA@mail.gmail.com","threadId":"42056","inReplyTo":"1460932021-27633-7-git-send-email-santiago@nyu.edu","subject":"Re: [PATCH v6 6/6] tag -v: verfy directly rather than exec-ing verify-tag","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2016-04-18T18:00:37Z","receivedAt":"2016-04-18T18:00:37Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sun, Apr 17, 2016 at 6:27 PM, wrote:\n> tag -v forks into verify-tag, which only calls gpg_verify_tag().\n\n\"forks into\" sounds odd.\n\n> Instead of forking to verify-tag, call gpg_verify_tag directly().\n\ns/ directly()/() directly/\n\nI found the commit message of your previous version[1] more\ndescriptive and easier to understand (minus the grammo):\n\n Instead of running the verify-tag plumbing command, use the\n gpg_verify_tag() function to avoid doing an additional fork call.\n\nThe patch itself looks fine.\n\n[1]: http://article.gmane.org/gmane.comp.version-control.git/290831\n\n> Signed-off-by: Santiago Torres \n> ---\n> diff --git a/builtin/tag.c b/builtin/tag.c\n> index 1705c94..7b2918e 100644\n> --- a/builtin/tag.c\n> +++ b/builtin/tag.c\n> @@ -104,13 +104,7 @@ static int delete_tag(const char *name, const char *ref,\n> static int verify_tag(const char *name, const char *ref,\n> const unsigned char *sha1)\n> {\n> - const char *argv_verify_tag[] = {\"verify-tag\",\n> - \"-v\", \"SHA1_HEX\", NULL};\n> - argv_verify_tag[2] = sha1_to_hex(sha1);\n> -\n> - if (run_command_v_opt(argv_verify_tag, RUN_GIT_CMD))\n> - return error(_(\"could not verify the tag '%s'\"), name);\n> - return 0;\n> + return gpg_verify_tag(sha1, name, GPG_VERIFY_VERBOSE);\n> }\n>\n> static int do_sign(struct strbuf *buffer)\n> --\n"},{"id":"283739","messageId":"CAPig+cSmb9wFCV+9PS4LYfd3hAH5s6ifRk8orVv+e2Q=h7F3Ag@mail.gmail.com","threadId":"42056","inReplyTo":"1460932021-27633-1-git-send-email-santiago@nyu.edu","subject":"Re: [PATCH v6 0/6] Move PGP verification out of verify-tag","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2016-04-18T18:14:26Z","receivedAt":"2016-04-18T18:14:26Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sun, Apr 17, 2016 at 6:26 PM, wrote:\n> This is a follow up of [1], [2], [3], [4], and [5]. patches 1/6, 2/6 and 3/6\n> are the same as the corresponding commits in pu.\n>\n> v6:\n> * As Junio suggested, updated 4/6, to include the name argument and the\n> ternary operator to provide more descriptive error messages. I propagated\n> these changes to 5/6 and 6/6 as well. I'm unsure about the 80-column\n> on 4/6, the ternary operator is rather long.\n> * Updated and reviewed the commit messages based on Eric and Junio's\n> feedback\n\nThanks. See my responses to individual patches for a few very minor\nissues, not necessarily even deserving a re-roll. With or without the\ncode and commit message nits addressed, this version is:\n\n Reviewed-by: Eric Sunshine \n"},{"id":"283746","messageId":"xmqqshyi4qsj.fsf@gitster.mtv.corp.google.com","threadId":"42056","inReplyTo":"1460932021-27633-1-git-send-email-santiago@nyu.edu","subject":"Re: [PATCH v6 0/6] Move PGP verification out of verify-tag","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-04-18T20:15:24Z","receivedAt":"2016-04-18T20:15:24Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"santiago@nyu.edu writes:\n\n> I'm unsure about the 80-column\n> on 4/6, the ternary operator is rather long.\n\nYou can do something like this:\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 report_name ?\n report_name :\n find_unique_abbrev(sha1, DEFAULT_ABBREV),\n typename(type));\n\n buf = read_sha1_file(sha1, &type, &size);\n if (!buf)\n return error(\"%s: unable to read file.\",\n report_name ?\n report_name :\n find_unique_abbrev(sha1, DEFAULT_ABBREV));\n\nThanks.\n"},{"id":"283747","messageId":"xmqqoa964q6h.fsf@gitster.mtv.corp.google.com","threadId":"42056","inReplyTo":"CAPig+cSGR-AZPzPRenm8RkOa7XpUOqMAgUw_8p-OUL=qP0Hy6w@mail.gmail.com","subject":"Re: [PATCH v6 4/6] verify-tag: add sha1 argument to verify_tag()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-04-18T20:28:38Z","receivedAt":"2016-04-18T20:28:38Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Sunshine writes:\n\n> On Sun, Apr 17, 2016 at 6:26 PM, wrote:\n>> verify-tag: add sha1 argument to verify_tag()\n>\n> Mentioned previously[1]: This subject is talking about low level\n> details of the change rather than giving a high-level overview. A\n> suggested replacement[1] would be:\n>\n> verify-tag: prepare verify_tag() for libification\n>\n>> The current interface of verify_tag() resolves reference names to SHA1,\n>> which might be redundant as future callers may resolve the refname to\n>> SHA1 beforehand.\n>\n> There is no mention here that the plan is to libify verify_tag() and\n> \"might be redundant\" is a somewhat weak way to argue in favor of this\n> change. The commit messages proposed in the previous review[1] was\n> more explicit:\n>\n> verify_tag() accepts a tag name which it resolves to a SHA1\n> before verification, however, the plan is to make this\n> functionality public and it is possible that future callers will\n> already have a SHA1 in hand. Since it would be wasteful for them\n> to supply a tag name only to have it resolved again, change\n> verify_tag() to accept a tag SHA1 rather than a name.\n\nPhrased that way, it is not \"might be redundant\" that this change is\nfixing, and \"It is possible ... will already have\" is still weak. I\nthink the real reason for this change is that some future callers\nmay only have object name without the end-user supplied textual\ninput, and the current interface that takes strings is cumbersome\nfor them to use--they have to use sha1_to_hex() only so that the\ncallee can do get_sha1() on it.\n\nI guess with 's/already/only/', your version is good.\n\nBy the way, it can also be read in a negative way: \"Some current\ncallers may let this function resolve tagname, but they no longer\ncan rely on it, as we force them to resolve before we allow them to\ncall this function.\"\n\n>> Add a SHA1 parameter to use instead of the name parameter. We also\n>> replace the name argument to report_name and use it for error reporting\n>> only.\n\nI know I am to blame, but perhaps \"reported_name\" or\n\"name_to_report\"? \"report_name\" sounds as if it is a boolean that\ntells the function to report the name of the tag (as opposed to stay\nsilent).\n\nI agree all the clean-up points in the code part of your review.\n\nThanks.\n\n> The patch itself looks okay, though see a few nits below (which may\n> not be worth a re-roll).\n>\n> [1]: http://article.gmane.org/gmane.comp.version-control.git/290829\n>\n>> Signed-off-by: Santiago Torres \n>> ---\n>> diff --git a/builtin/verify-tag.c b/builtin/verify-tag.c\n>> @@ -80,6 +79,8 @@ int cmd_verify_tag(int argc, const char **argv, const char *prefix)\n>> {\n>> int i = 1, verbose = 0, had_error = 0;\n>> unsigned flags = 0;\n>> + unsigned char sha1[20];\n>> + const char *name;\n>\n> Nit: These could have been declared in the scope of the while-loop\n> (below) since you've added braces to it.\n>\n>> @@ -96,8 +97,14 @@ int cmd_verify_tag(int argc, const char **argv, const char *prefix)\n>> - while (i < argc)\n>> - if (verify_tag(argv[i++], flags))\n>> + while (i < argc) {\n>> + name = argv[i++];\n>> + if (get_sha1(name, sha1)) {\n>> + error(\"tag '%s' not found.\", name);\n>> had_error = 1;\n>\n> These lines could be combined:\n>\n> had_error = !!error(\"tag '%s' not found.\", name);\n>\n> which would allow you to drop the braces.\n>\n>> + }\n>> + else if (verify_tag(sha1, name, flags))\n>> + had_error = 1;\n>\n> Style: cuddle '}' and else:\n>\n> } else\n>\n>> + }\n>> return had_error;\n>> }\n"},{"id":"283766","messageId":"20160419051322.GB19439@sigill.intra.peff.net","threadId":"42056","inReplyTo":"1460932021-27633-4-git-send-email-santiago@nyu.edu","subject":"Re: [PATCH v6 3/6] verify-tag: change variable name for readability","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-04-19T05:13:22Z","receivedAt":"2016-04-19T05:13:22Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Apr 17, 2016 at 06:26:58PM -0400, santiago@nyu.edu wrote:\n\n> diff --git a/builtin/verify-tag.c b/builtin/verify-tag.c\n> index 77f070a..010353c 100644\n> --- a/builtin/verify-tag.c\n> +++ b/builtin/verify-tag.c\n> @@ -21,20 +21,21 @@ static const char * const verify_tag_usage[] = {\n> static int run_gpg_verify(const char *buf, unsigned long size, unsigned flags)\n> {\n> \tstruct signature_check sigc;\n> -\tint len;\n> +\tint payload_size;\n> \tint ret;\n> \n> \tmemset(&sigc, 0, sizeof(sigc));\n> \n> -\tlen = parse_signature(buf, size);\n> +\tpayload_size = parse_signature(buf, size);\n\nWhile we're here, can we make payload_size a \"size_t\"? That is the\nreturn type of parse_signature.\n\n> -\tif (size == len) {\n> +\tif (size == payload_size) {\n\nThat would make this a comparison between \"unsigned long\" and \"size_t\",\nbut I doubt it would be the first such place in git. And it works out in\npractice, because \"unsigned long\" is generally at least as large as\n\"size_t\", and if our buffer is larger than \"size_t\" can store, we'd have\nfailed long before, when trying to allocate the buffer.\n\nWe could make \"size\" also a \"size_t\", but I suspect then we'd just be\nbumping the mismatch out to its caller.\n\n-Peff\n"},{"id":"283767","messageId":"20160419051611.GC19439@sigill.intra.peff.net","threadId":"42056","inReplyTo":"1460932021-27633-1-git-send-email-santiago@nyu.edu","subject":"Re: [PATCH v6 0/6] Move PGP verification out of verify-tag","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-04-19T05:16:12Z","receivedAt":"2016-04-19T05:16:12Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Apr 17, 2016 at 06:26:55PM -0400, santiago@nyu.edu wrote:\n\n> From: Santiago Torres \n> \n> This is a follow up of [1], [2], [3], [4], and [5]. patches 1/6, 2/6 and 3/6\n> are the same as the corresponding commits in pu.\n\nAside from the minor point I mentioned, and the ones from Eric, this\nlooks good to me. None of them is that big a deal, but there are enough\nthat it's probably worth one more re-roll to clean them all up.\n\nThanks for sticking with it.\n\n-Peff\n"}]}