{"thread":{"id":"36856","subject":"[PATCH 0/3] verify-commit: verify commit signatures","startedAt":"2014-06-06T14:15:25Z","lastAt":"2014-07-10T22:27:14Z","messageCount":75,"participants":["Michael J Gruber","Jeff King","Johannes Sixt","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"243470","messageId":"cover.1402063795.git.git@drmicha.warpmail.net","threadId":"36856","inReplyTo":null,"subject":"[PATCH 0/3] verify-commit: verify commit signatures","fromName":"Michael J Gruber","fromEmail":"git@drmicha.warpmail.net","sentAt":"2014-06-06T14:15:25Z","receivedAt":"2014-06-06T14:15:25Z","isPatch":true,"sender":{"key":"git@grubix.eu","avatar":"https://avatars.githubusercontent.com/u/233215?v=4"},"body":"Hi there,\n\nSome of you may remember me from my more active times...\n\nAnyways, a recent blog post about signed commits in git triggered me to\nlook at our tools for that again. It seems that we only have the\nlog/pretty family on the user facing side, but everything we need under\nthe hood.\n\nSo here's a suggestion to implement verify-commit in a way which is\ncompletely analogous to verify-tag. In fact, it could be coded more\nelegantly, but I kept it this way so that we could merge the two more\neasily in case we wish to do so.\n\nI will follow up with tests if the design principle is something we agree\nupon.\n\nMichael J Gruber (3):\n pretty: free the gpg status buf\n gpg-interface: provide access to the payload\n verify-commit: scriptable commit signature verification\n\n Documentation/git-verify-commit.txt | 28 +++++++++++\n Makefile | 1 +\n builtin.h | 1 +\n builtin/merge.c | 1 +\n builtin/verify-commit.c | 98 +++++++++++++++++++++++++++++++++++++\n command-list.txt | 1 +\n commit.c | 1 +\n git.c | 1 +\n gpg-interface.h | 1 +\n pretty.c | 2 +\n 10 files changed, 135 insertions(+)\n create mode 100644 Documentation/git-verify-commit.txt\n create mode 100644 builtin/verify-commit.c\n\n-- \n2.0.0.533.gae2e602\n"},{"id":"243471","messageId":"a4509bc715843834b9943640f83926a157327c2a.1402063796.git.git@drmicha.warpmail.net","threadId":"36856","inReplyTo":"cover.1402063795.git.git@drmicha.warpmail.net","subject":"[PATCH 1/3] pretty: free the gpg status buf","fromName":"Michael J Gruber","fromEmail":"git@drmicha.warpmail.net","sentAt":"2014-06-06T14:15:26Z","receivedAt":"2014-06-06T14:15:26Z","isPatch":true,"sender":{"key":"git@grubix.eu","avatar":"https://avatars.githubusercontent.com/u/233215?v=4"},"body":"4a868fd (pretty: parse the gpg status lines rather than the output, 2013-02-14)\nmade the gpg status lines available to callers and made sure they freed\nthe used space, but missed one spot.\n\nFree the status line buffer also in the remaining spot.\n\nSigned-off-by: Michael J Gruber \n---\n pretty.c | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/pretty.c b/pretty.c\nindex 4f51287..f1e8a70 100644\n--- a/pretty.c\n+++ b/pretty.c\n@@ -1538,6 +1538,7 @@ void format_commit_message(const struct commit *commit,\n \tfree(context.commit_encoding);\n \tlogmsg_free(context.message, commit);\n \tfree(context.signature_check.gpg_output);\n+\tfree(context.signature_check.gpg_status);\n \tfree(context.signature_check.signer);\n }\n \n-- \n2.0.0.533.gae2e602\n"},{"id":"243473","messageId":"558044a975c940d19370394900bb44bf4fb2387d.1402063796.git.git@drmicha.warpmail.net","threadId":"36856","inReplyTo":"cover.1402063795.git.git@drmicha.warpmail.net","subject":"[PATCH 2/3] gpg-interface: provide access to the payload","fromName":"Michael J Gruber","fromEmail":"git@drmicha.warpmail.net","sentAt":"2014-06-06T14:15:27Z","receivedAt":"2014-06-06T14:15:27Z","isPatch":true,"sender":{"key":"git@grubix.eu","avatar":"https://avatars.githubusercontent.com/u/233215?v=4"},"body":"In contrast to tag signatures, commit signatures are put into the\nheader, that is between the other header parts and commit messages.\n\nProvide access to the commit content sans the signature, which is the\npayload that is actually signed. Commit signature verification does the\nparsing anyways, and callers may wish to act on or display the commit\nobject sans the signature.\n\nSigned-off-by: Michael J Gruber \n---\n builtin/merge.c | 1 +\n commit.c | 1 +\n gpg-interface.h | 1 +\n pretty.c | 1 +\n 4 files changed, 4 insertions(+)\n\ndiff --git a/builtin/merge.c b/builtin/merge.c\nindex 428ca24..6a9812a 100644\n--- a/builtin/merge.c\n+++ b/builtin/merge.c\n@@ -1282,6 +1282,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix)\n \t\t\t\tprintf(_(\"Commit %s has a good GPG signature by %s\\n\"),\n \t\t\t\t hex, signature_check.signer);\n \n+\t\t\tfree(signature_check.payload);\n \t\t\tfree(signature_check.gpg_output);\n \t\t\tfree(signature_check.gpg_status);\n \t\t\tfree(signature_check.signer);\ndiff --git a/commit.c b/commit.c\nindex f479331..e9686b2 100644\n--- a/commit.c\n+++ b/commit.c\n@@ -1219,6 +1219,7 @@ void check_commit_signature(const struct commit* commit, struct signature_check\n \t\t\t\t &gpg_output, &gpg_status);\n \tif (status && !gpg_output.len)\n \t\tgoto out;\n+\tsigc->payload = strbuf_detach(&payload, NULL);\n \tsigc->gpg_output = strbuf_detach(&gpg_output, NULL);\n \tsigc->gpg_status = strbuf_detach(&gpg_status, NULL);\n \tparse_gpg_output(sigc);\ndiff --git a/gpg-interface.h b/gpg-interface.h\nindex a85cb5b..d727c25 100644\n--- a/gpg-interface.h\n+++ b/gpg-interface.h\n@@ -2,6 +2,7 @@\n #define GPG_INTERFACE_H\n \n struct signature_check {\n+\tchar *payload;\n \tchar *gpg_output;\n \tchar *gpg_status;\n \tchar result; /* 0 (not checked),\ndiff --git a/pretty.c b/pretty.c\nindex f1e8a70..24fb877 100644\n--- a/pretty.c\n+++ b/pretty.c\n@@ -1537,6 +1537,7 @@ void format_commit_message(const struct commit *commit,\n \n \tfree(context.commit_encoding);\n \tlogmsg_free(context.message, commit);\n+\tfree(context.signature_check.payload);\n \tfree(context.signature_check.gpg_output);\n \tfree(context.signature_check.gpg_status);\n \tfree(context.signature_check.signer);\n-- \n2.0.0.533.gae2e602\n"},{"id":"243472","messageId":"cc5fd1d554e0357dfb514e3f9ad100d98c16d4d5.1402063796.git.git@drmicha.warpmail.net","threadId":"36856","inReplyTo":"cover.1402063795.git.git@drmicha.warpmail.net","subject":"[PATCH 3/3] verify-commit: scriptable commit signature verification","fromName":"Michael J Gruber","fromEmail":"git@drmicha.warpmail.net","sentAt":"2014-06-06T14:15:28Z","receivedAt":"2014-06-06T14:15:28Z","isPatch":true,"sender":{"key":"git@grubix.eu","avatar":"https://avatars.githubusercontent.com/u/233215?v=4"},"body":"Commit signatures can be verified using \"git show -s --show-signature\"\nor the \"%G?\" pretty format and parsing the output, which is well suited\nfor user inspection, but not for scripting.\n\nProvide a command \"verify-commit\" which is analogous to \"verify-tag\": It\nreturns 0 for good signatures and non-zero otherwise, has the gpg output\non stderr and (optionally) the commit object on stdout, sans the\nsignature, just like \"verify-tag\" does.\n\nSigned-off-by: Michael J Gruber \n---\n Documentation/git-verify-commit.txt | 28 +++++++++++\n Makefile | 1 +\n builtin.h | 1 +\n builtin/verify-commit.c | 98 +++++++++++++++++++++++++++++++++++++\n command-list.txt | 1 +\n git.c | 1 +\n 6 files changed, 130 insertions(+)\n create mode 100644 Documentation/git-verify-commit.txt\n create mode 100644 builtin/verify-commit.c\n\ndiff --git a/Documentation/git-verify-commit.txt b/Documentation/git-verify-commit.txt\nnew file mode 100644\nindex 0000000..dcd7803\n--- /dev/null\n+++ b/Documentation/git-verify-commit.txt\n@@ -0,0 +1,28 @@\n+git-verify-commit(1)\n+=================\n+\n+NAME\n+----\n+git-verify-commit - Check the GPG signature of commits\n+\n+SYNOPSIS\n+--------\n+[verse]\n+'git verify-commit' ...\n+\n+DESCRIPTION\n+-----------\n+Validates the gpg signature created by 'git commit -S'.\n+\n+OPTIONS\n+-------\n+-v::\n+--verbose::\n+\tPrint the contents of the commit object before validating it.\n+\n+...::\n+\tSHA-1 identifiers of Git commit objects.\n+\n+GIT\n+---\n+Part of the linkgit:git[1] suite\ndiff --git a/Makefile b/Makefile\nindex 07ea105..b92418d 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -999,6 +999,7 @@ BUILTIN_OBJS += builtin/update-ref.o\n BUILTIN_OBJS += builtin/update-server-info.o\n BUILTIN_OBJS += builtin/upload-archive.o\n BUILTIN_OBJS += builtin/var.o\n+BUILTIN_OBJS += builtin/verify-commit.o\n BUILTIN_OBJS += builtin/verify-pack.o\n BUILTIN_OBJS += builtin/verify-tag.o\n BUILTIN_OBJS += builtin/write-tree.o\ndiff --git a/builtin.h b/builtin.h\nindex c47c110..5d91f31 100644\n--- a/builtin.h\n+++ b/builtin.h\n@@ -128,6 +128,7 @@ extern int cmd_update_server_info(int argc, const char **argv, const char *prefi\n extern int cmd_upload_archive(int argc, const char **argv, const char *prefix);\n extern int cmd_upload_archive_writer(int argc, const char **argv, const char *prefix);\n extern int cmd_var(int argc, const char **argv, const char *prefix);\n+extern int cmd_verify_commit(int argc, const char **argv, const char *prefix);\n extern int cmd_verify_tag(int argc, const char **argv, const char *prefix);\n extern int cmd_version(int argc, const char **argv, const char *prefix);\n extern int cmd_whatchanged(int argc, const char **argv, const char *prefix);\ndiff --git a/builtin/verify-commit.c b/builtin/verify-commit.c\nnew file mode 100644\nindex 0000000..69b7c6d\n--- /dev/null\n+++ b/builtin/verify-commit.c\n@@ -0,0 +1,98 @@\n+/*\n+ * Builtin \"git commit-commit\"\n+ *\n+ * Copyright (c) 2014 Michael J Gruber \n+ *\n+ * Based on git-verify-tag\n+ */\n+#include \"cache.h\"\n+#include \"builtin.h\"\n+#include \"commit.h\"\n+#include \"run-command.h\"\n+#include \n+#include \"parse-options.h\"\n+#include \"gpg-interface.h\"\n+\n+static const char * const verify_commit_usage[] = {\n+\t\tN_(\"git verify-commit [-v|--verbose] ...\"),\n+\t\tNULL\n+};\n+\n+static int run_gpg_verify(const unsigned char *sha1, const char *buf, unsigned long size, int verbose)\n+{\n+\tstruct signature_check signature_check;\n+\n+\tmemset(&signature_check, 0, sizeof(signature_check));\n+\n+\tcheck_commit_signature(lookup_commit(sha1), &signature_check);\n+\n+\tif (verbose && signature_check.payload)\n+\t\tfputs(signature_check.payload, stdout);\n+\n+\tif (signature_check.gpg_output)\n+\t\tfputs(signature_check.gpg_output, stderr);\n+\n+\tfree(signature_check.gpg_output);\n+\tfree(signature_check.gpg_status);\n+\tfree(signature_check.signer);\n+\tfree(signature_check.key);\n+\treturn signature_check.result != 'G';\n+}\n+\n+static int verify_commit(const char *name, int verbose)\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(\"commit '%s' not found.\", name);\n+\n+\ttype = sha1_object_info(sha1, NULL);\n+\tif (type != OBJ_COMMIT)\n+\t\treturn error(\"%s: cannot verify a non-commit 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(sha1, buf, size, verbose);\n+\n+\tfree(buf);\n+\treturn ret;\n+}\n+\n+static int git_verify_commit_config(const char *var, const char *value, void *cb)\n+{\n+\tint status = git_gpg_config(var, value, cb);\n+\tif (status)\n+\t\treturn status;\n+\treturn git_default_config(var, value, cb);\n+}\n+\n+int cmd_verify_commit(int argc, const char **argv, const char *prefix)\n+{\n+\tint i = 1, verbose = 0, had_error = 0;\n+\tconst struct option verify_commit_options[] = {\n+\t\tOPT__VERBOSE(&verbose, N_(\"print commit contents\")),\n+\t\tOPT_END()\n+\t};\n+\n+\tgit_config(git_verify_commit_config, NULL);\n+\n+\targc = parse_options(argc, argv, prefix, verify_commit_options,\n+\t\t\t verify_commit_usage, PARSE_OPT_KEEP_ARGV0);\n+\tif (argc <= i)\n+\t\tusage_with_options(verify_commit_usage, verify_commit_options);\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_commit(argv[i++], verbose))\n+\t\t\thad_error = 1;\n+\treturn had_error;\n+}\ndiff --git a/command-list.txt b/command-list.txt\nindex cf36c3d..a3ff0c9 100644\n--- a/command-list.txt\n+++ b/command-list.txt\n@@ -132,6 +132,7 @@ git-update-server-info synchingrepositories\n git-upload-archive synchelpers\n git-upload-pack synchelpers\n git-var plumbinginterrogators\n+git-verify-commit ancillaryinterrogators\n git-verify-pack plumbinginterrogators\n git-verify-tag ancillaryinterrogators\n gitweb ancillaryinterrogators\ndiff --git a/git.c b/git.c\nindex 7780572..bbc3293 100644\n--- a/git.c\n+++ b/git.c\n@@ -441,6 +441,7 @@ static struct cmd_struct commands[] = {\n \t{ \"upload-archive\", cmd_upload_archive },\n \t{ \"upload-archive--writer\", cmd_upload_archive_writer },\n \t{ \"var\", cmd_var, RUN_SETUP_GENTLY },\n+\t{ \"verify-commit\", cmd_verify_commit, RUN_SETUP },\n \t{ \"verify-pack\", cmd_verify_pack },\n \t{ \"verify-tag\", cmd_verify_tag, RUN_SETUP },\n \t{ \"version\", cmd_version },\n-- \n2.0.0.533.gae2e602\n"},{"id":"243923","messageId":"42816455-e873-4c81-a630-0be1869e9233@email.android.com","threadId":"36856","inReplyTo":"cc5fd1d554e0357dfb514e3f9ad100d98c16d4d5.1402063796.git.git@drmicha.warpmail.net","subject":"Re: [PATCH 3/3] verify-commit: scriptable commit signature verification","fromName":"Michael J Gruber","fromEmail":"git@drmicha.warpmail.net","sentAt":"2014-06-11T19:48:48Z","receivedAt":"2014-06-11T19:48:48Z","isPatch":true,"sender":{"key":"git@grubix.eu","avatar":"https://avatars.githubusercontent.com/u/233215?v=4"},"body":"\n\nOn 6. Juni 2014 16:15:28 MESZ, Michael J Gruber wrote:\n>Commit signatures can be verified using \"git show -s --show-signature\"\n>or the \"%G?\" pretty format and parsing the output, which is well suited\n>for user inspection, but not for scripting.\n>\n>Provide a command \"verify-commit\" which is analogous to \"verify-tag\":\n>It\n>returns 0 for good signatures and non-zero otherwise, has the gpg\n>output\n>on stderr and (optionally) the commit object on stdout, sans the\n>signature, just like \"verify-tag\" does.\n>\n>Signed-off-by: Michael J Gruber \n>---\n> Documentation/git-verify-commit.txt | 28 +++++++++++\n> Makefile | 1 +\n> builtin.h | 1 +\n>builtin/verify-commit.c | 98\n>+++++++++++++++++++++++++++++++++++++\n> command-list.txt | 1 +\n> git.c | 1 +\n> 6 files changed, 130 insertions(+)\n> create mode 100644 Documentation/git-verify-commit.txt\n> create mode 100644 builtin/verify-commit.c\n>\n>diff --git a/Documentation/git-verify-commit.txt\n>b/Documentation/git-verify-commit.txt\n>new file mode 100644\n>index 0000000..dcd7803\n>--- /dev/null\n>+++ b/Documentation/git-verify-commit.txt\n>@@ -0,0 +1,28 @@\n>+git-verify-commit(1)\n>+=================\n\nThat line will need 3 more \"=\" thanks to asciidoc stubbornness.\n\nv2 plus the tests coming tomorrow when my dev box is online again.\n\n>+\n>+NAME\n>+----\n>+git-verify-commit - Check the GPG signature of commits\n>+\n>+SYNOPSIS\n>+--------\n>+[verse]\n>+'git verify-commit' ...\n>+\n>+DESCRIPTION\n>+-----------\n>+Validates the gpg signature created by 'git commit -S'.\n>+\n>+OPTIONS\n>+-------\n>+-v::\n>+--verbose::\n>+\tPrint the contents of the commit object before validating it.\n>+\n>+...::\n>+\tSHA-1 identifiers of Git commit objects.\n>+\n>+GIT\n>+---\n>+Part of the linkgit:git[1] suite\n>diff --git a/Makefile b/Makefile\n>index 07ea105..b92418d 100644\n>--- a/Makefile\n>+++ b/Makefile\n>@@ -999,6 +999,7 @@ BUILTIN_OBJS += builtin/update-ref.o\n> BUILTIN_OBJS += builtin/update-server-info.o\n> BUILTIN_OBJS += builtin/upload-archive.o\n> BUILTIN_OBJS += builtin/var.o\n>+BUILTIN_OBJS += builtin/verify-commit.o\n> BUILTIN_OBJS += builtin/verify-pack.o\n> BUILTIN_OBJS += builtin/verify-tag.o\n> BUILTIN_OBJS += builtin/write-tree.o\n>diff --git a/builtin.h b/builtin.h\n>index c47c110..5d91f31 100644\n>--- a/builtin.h\n>+++ b/builtin.h\n>@@ -128,6 +128,7 @@ extern int cmd_update_server_info(int argc, const\n>char **argv, const char *prefi\n>extern int cmd_upload_archive(int argc, const char **argv, const char\n>*prefix);\n>extern int cmd_upload_archive_writer(int argc, const char **argv, const\n>char *prefix);\n> extern int cmd_var(int argc, const char **argv, const char *prefix);\n>+extern int cmd_verify_commit(int argc, const char **argv, const char\n>*prefix);\n>extern int cmd_verify_tag(int argc, const char **argv, const char\n>*prefix);\n>extern int cmd_version(int argc, const char **argv, const char\n>*prefix);\n>extern int cmd_whatchanged(int argc, const char **argv, const char\n>*prefix);\n>diff --git a/builtin/verify-commit.c b/builtin/verify-commit.c\n>new file mode 100644\n>index 0000000..69b7c6d\n>--- /dev/null\n>+++ b/builtin/verify-commit.c\n>@@ -0,0 +1,98 @@\n>+/*\n>+ * Builtin \"git commit-commit\"\n>+ *\n>+ * Copyright (c) 2014 Michael J Gruber \n>+ *\n>+ * Based on git-verify-tag\n>+ */\n>+#include \"cache.h\"\n>+#include \"builtin.h\"\n>+#include \"commit.h\"\n>+#include \"run-command.h\"\n>+#include \n>+#include \"parse-options.h\"\n>+#include \"gpg-interface.h\"\n>+\n>+static const char * const verify_commit_usage[] = {\n>+\t\tN_(\"git verify-commit [-v|--verbose] ...\"),\n>+\t\tNULL\n>+};\n>+\n>+static int run_gpg_verify(const unsigned char *sha1, const char *buf,\n>unsigned long size, int verbose)\n>+{\n>+\tstruct signature_check signature_check;\n>+\n>+\tmemset(&signature_check, 0, sizeof(signature_check));\n>+\n>+\tcheck_commit_signature(lookup_commit(sha1), &signature_check);\n>+\n>+\tif (verbose && signature_check.payload)\n>+\t\tfputs(signature_check.payload, stdout);\n>+\n>+\tif (signature_check.gpg_output)\n>+\t\tfputs(signature_check.gpg_output, stderr);\n>+\n>+\tfree(signature_check.gpg_output);\n>+\tfree(signature_check.gpg_status);\n>+\tfree(signature_check.signer);\n>+\tfree(signature_check.key);\n>+\treturn signature_check.result != 'G';\n>+}\n>+\n>+static int verify_commit(const char *name, int verbose)\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(\"commit '%s' not found.\", name);\n>+\n>+\ttype = sha1_object_info(sha1, NULL);\n>+\tif (type != OBJ_COMMIT)\n>+\t\treturn error(\"%s: cannot verify a non-commit 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(sha1, buf, size, verbose);\n>+\n>+\tfree(buf);\n>+\treturn ret;\n>+}\n>+\n>+static int git_verify_commit_config(const char *var, const char\n>*value, void *cb)\n>+{\n>+\tint status = git_gpg_config(var, value, cb);\n>+\tif (status)\n>+\t\treturn status;\n>+\treturn git_default_config(var, value, cb);\n>+}\n>+\n>+int cmd_verify_commit(int argc, const char **argv, const char *prefix)\n>+{\n>+\tint i = 1, verbose = 0, had_error = 0;\n>+\tconst struct option verify_commit_options[] = {\n>+\t\tOPT__VERBOSE(&verbose, N_(\"print commit contents\")),\n>+\t\tOPT_END()\n>+\t};\n>+\n>+\tgit_config(git_verify_commit_config, NULL);\n>+\n>+\targc = parse_options(argc, argv, prefix, verify_commit_options,\n>+\t\t\t verify_commit_usage, PARSE_OPT_KEEP_ARGV0);\n>+\tif (argc <= i)\n>+\t\tusage_with_options(verify_commit_usage, verify_commit_options);\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_commit(argv[i++], verbose))\n>+\t\t\thad_error = 1;\n>+\treturn had_error;\n>+}\n>diff --git a/command-list.txt b/command-list.txt\n>index cf36c3d..a3ff0c9 100644\n>--- a/command-list.txt\n>+++ b/command-list.txt\n>@@ -132,6 +132,7 @@ git-update-server-info \n>synchingrepositories\n> git-upload-archive synchelpers\n> git-upload-pack synchelpers\n> git-var plumbinginterrogators\n>+git-verify-commit ancillaryinterrogators\n> git-verify-pack plumbinginterrogators\n> git-verify-tag ancillaryinterrogators\n> gitweb ancillaryinterrogators\n>diff --git a/git.c b/git.c\n>index 7780572..bbc3293 100644\n>--- a/git.c\n>+++ b/git.c\n>@@ -441,6 +441,7 @@ static struct cmd_struct commands[] = {\n> \t{ \"upload-archive\", cmd_upload_archive },\n> \t{ \"upload-archive--writer\", cmd_upload_archive_writer },\n> \t{ \"var\", cmd_var, RUN_SETUP_GENTLY },\n>+\t{ \"verify-commit\", cmd_verify_commit, RUN_SETUP },\n> \t{ \"verify-pack\", cmd_verify_pack },\n> \t{ \"verify-tag\", cmd_verify_tag, RUN_SETUP },\n> \t{ \"version\", cmd_version },\n"},{"id":"244068","messageId":"20140613075550.GI7908@sigill.intra.peff.net","threadId":"36856","inReplyTo":"558044a975c940d19370394900bb44bf4fb2387d.1402063796.git.git@drmicha.warpmail.net","subject":"Re: [PATCH 2/3] gpg-interface: provide access to the payload","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-06-13T07:55:51Z","receivedAt":"2014-06-13T07:55:51Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jun 06, 2014 at 04:15:27PM +0200, Michael J Gruber wrote:\n\n> diff --git a/builtin/merge.c b/builtin/merge.c\n> [...]\n> +\t\t\tfree(signature_check.payload);\n> \t\t\tfree(signature_check.gpg_output);\n> \t\t\tfree(signature_check.gpg_status);\n> \t\t\tfree(signature_check.signer);\n> [...]\n> diff --git a/pretty.c b/pretty.c\n> [...]\n> +\tfree(context.signature_check.payload);\n> \tfree(context.signature_check.gpg_output);\n> \tfree(context.signature_check.gpg_status);\n> \tfree(context.signature_check.signer);\n\nPerhaps this is a sign that we need a \"signature_check_clear()\" helper?\n\n-Peff\n"},{"id":"244071","messageId":"20140613080229.GJ7908@sigill.intra.peff.net","threadId":"36856","inReplyTo":"cc5fd1d554e0357dfb514e3f9ad100d98c16d4d5.1402063796.git.git@drmicha.warpmail.net","subject":"Re: [PATCH 3/3] verify-commit: scriptable commit signature verification","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-06-13T08:02:29Z","receivedAt":"2014-06-13T08:02:29Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jun 06, 2014 at 04:15:28PM +0200, Michael J Gruber wrote:\n\n> Commit signatures can be verified using \"git show -s --show-signature\"\n> or the \"%G?\" pretty format and parsing the output, which is well suited\n> for user inspection, but not for scripting.\n> \n> Provide a command \"verify-commit\" which is analogous to \"verify-tag\": It\n> returns 0 for good signatures and non-zero otherwise, has the gpg output\n> on stderr and (optionally) the commit object on stdout, sans the\n> signature, just like \"verify-tag\" does.\n> \n> Signed-off-by: Michael J Gruber \n\nI think the general direction of this series is reasonable.\n\nDid you give any thought to just having a \"git verify\" command, instead\nof separate tag/verify commands?\n\nAnother thought, that may be orthogonal to your series: what does it\nmean to verify a commit? We check for _some_ signature from a key that\nis in your keyring. But we do not check whether the signature matches\nthe committer field (or for tags, the tagger field). You have to parse\nthe gpg output, run \"git cat-file\", and then correlate the two. Should\nthere be an option to have git check that one of the signed uids from\ngpg matches the commit's committer?\n\n-Peff\n"},{"id":"244073","messageId":"539AC7FC.8040707@drmicha.warpmail.net","threadId":"36856","inReplyTo":"20140613075550.GI7908@sigill.intra.peff.net","subject":"Re: [PATCH 2/3] gpg-interface: provide access to the payload","fromName":"Michael J Gruber","fromEmail":"git@drmicha.warpmail.net","sentAt":"2014-06-13T09:44:28Z","receivedAt":"2014-06-13T09:44:28Z","isPatch":true,"sender":{"key":"git@grubix.eu","avatar":"https://avatars.githubusercontent.com/u/233215?v=4"},"body":"Jeff King venit, vidit, dixit 13.06.2014 09:55:\n> On Fri, Jun 06, 2014 at 04:15:27PM +0200, Michael J Gruber wrote:\n> \n>> diff --git a/builtin/merge.c b/builtin/merge.c\n>> [...]\n>> +\t\t\tfree(signature_check.payload);\n>> \t\t\tfree(signature_check.gpg_output);\n>> \t\t\tfree(signature_check.gpg_status);\n>> \t\t\tfree(signature_check.signer);\n>> [...]\n>> diff --git a/pretty.c b/pretty.c\n>> [...]\n>> +\tfree(context.signature_check.payload);\n>> \tfree(context.signature_check.gpg_output);\n>> \tfree(context.signature_check.gpg_status);\n>> \tfree(context.signature_check.signer);\n> \n> Perhaps this is a sign that we need a \"signature_check_clear()\" helper?\n\n... or simply switch to language which has (or can overload) free for an\nobject :)\n\nDo we have prior art for such helpers so that the new one would be\nanalogous?\n\nMichael\n"},{"id":"244074","messageId":"539ACA8A.90108@drmicha.warpmail.net","threadId":"36856","inReplyTo":"20140613080229.GJ7908@sigill.intra.peff.net","subject":"Re: [PATCH 3/3] verify-commit: scriptable commit signature verification","fromName":"Michael J Gruber","fromEmail":"git@drmicha.warpmail.net","sentAt":"2014-06-13T09:55:22Z","receivedAt":"2014-06-13T09:55:22Z","isPatch":true,"sender":{"key":"git@grubix.eu","avatar":"https://avatars.githubusercontent.com/u/233215?v=4"},"body":"Jeff King venit, vidit, dixit 13.06.2014 10:02:\n> On Fri, Jun 06, 2014 at 04:15:28PM +0200, Michael J Gruber wrote:\n> \n>> Commit signatures can be verified using \"git show -s --show-signature\"\n>> or the \"%G?\" pretty format and parsing the output, which is well suited\n>> for user inspection, but not for scripting.\n>>\n>> Provide a command \"verify-commit\" which is analogous to \"verify-tag\": It\n>> returns 0 for good signatures and non-zero otherwise, has the gpg output\n>> on stderr and (optionally) the commit object on stdout, sans the\n>> signature, just like \"verify-tag\" does.\n>>\n>> Signed-off-by: Michael J Gruber \n> \n> I think the general direction of this series is reasonable.\n> \n> Did you give any thought to just having a \"git verify\" command, instead\n> of separate tag/verify commands?\n\nYes. (mathematician's answer)\n\nYou know not only the outcome but also why I refrained from doing so:\ncompatibility. We would need to deprecate verify-tag.\n\nBut there is also a more subtle reason: If you want to verify a signed\ncommit, you want to be sure that it actually is a commit. \"verify\" could\neasily branch code paths based on the object type, but I'm not sure that\nis desirable, at least not by default.\n\n> Another thought, that may be orthogonal to your series: what does it\n> mean to verify a commit? We check for _some_ signature from a key that\n> is in your keyring. But we do not check whether the signature matches\n> the committer field (or for tags, the tagger field). You have to parse\n> the gpg output, run \"git cat-file\", and then correlate the two. Should\n> there be an option to have git check that one of the signed uids from\n> gpg matches the commit's committer?\n> \n> -Peff\n> \n\nThat is a general issue with verifying signatures: it can be automated\nonly if you employ a strict trust model and a very limited keyring.\n\"valid signature\" means only as much as the signatures that your gpg\naccepts can be really trusted.\n\nComparing uid's really buys you nothing in the sense that everyone can\nhave a key with uid \"Jeff King signed by some other\nkeys. On the other hand, it's perfectly OK to use different uids for git\ncommits and signatures. The e-mail address I use for the git list and\ncommits, for example, is clearly a \"plus address\", which helps me\norganize things; my personal key has the primary address as uid.\n\nI really think all this is up to local policies for individual use cases.\n\nMichael\n"},{"id":"244076","messageId":"20140613103419.GA14066@sigill.intra.peff.net","threadId":"36856","inReplyTo":"539AC7FC.8040707@drmicha.warpmail.net","subject":"Re: [PATCH 2/3] gpg-interface: provide access to the payload","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-06-13T10:34:19Z","receivedAt":"2014-06-13T10:34:19Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jun 13, 2014 at 11:44:28AM +0200, Michael J Gruber wrote:\n\n> > Perhaps this is a sign that we need a \"signature_check_clear()\" helper?\n> \n> ... or simply switch to language which has (or can overload) free for an\n> object :)\n\nI hear somebody has reimplemented git in pure javascript. ;P\n\n> Do we have prior art for such helpers so that the new one would be\n> analogous?\n\nI was thinking of credential_clear, string_list_clear, etc. Literally\njust:\n\n void signature_check_clear(struct signature_check *s)\n {\n\tfree(s->gpg_output);\n\tfree(s->gpg_status);\n\tfree(s->signer);\n\tfree(s->key);\n }\n\nYour first commit fixed a leak on gpg_status. Did it also need to handle\nthe \"key\" field there?\n\nFor some structs, we'd also do:\n\n\tmemset(s, 0, sizeof(*s));\n\nto get us back to a usable, initialized state so the struct can be\nreused. However, check_commit_signature doesn't care if the struct is\ninitialized or not (i.e., there is no initialized state). Doing so does\nhelp detect use-after-free conditions, though.\n\n-Peff\n"},{"id":"244077","messageId":"cover.1402655838.git.git@drmicha.warpmail.net","threadId":"36856","inReplyTo":"cover.1402063795.git.git@drmicha.warpmail.net","subject":"[PATCHv2 0/6] verify-commit: verify commit signatures","fromName":"Michael J Gruber","fromEmail":"git@drmicha.warpmail.net","sentAt":"2014-06-13T10:42:42Z","receivedAt":"2014-06-13T10:42:42Z","isPatch":false,"sender":{"key":"git@grubix.eu","avatar":"https://avatars.githubusercontent.com/u/233215?v=4"},"body":"Patch1: unchanged\nPatch2: unchanged\nPatch3: as fixed in Junio's tree already (===)\nPatch4: minor test fix for t7510\nPatch5: tests for the verify-commit\nPatch6: make all struct signature_check users employ the same clear helper\n\nMichael J Gruber (6):\n pretty: free the gpg status buf\n gpg-interface: provide access to the payload\n verify-commit: scriptable commit signature verification\n t7510: exit for loop with test result\n t7510: test verify-commit\n gpg-interface: provide clear helper for struct signature_check\n\n Documentation/git-verify-commit.txt | 28 +++++++++++\n Makefile | 1 +\n builtin.h | 1 +\n builtin/merge.c | 5 +-\n builtin/verify-commit.c | 95 +++++++++++++++++++++++++++++++++++++\n command-list.txt | 1 +\n commit.c | 1 +\n git.c | 1 +\n gpg-interface.c | 14 ++++++\n gpg-interface.h | 2 +\n pretty.c | 3 +-\n t/t7510-signed-commit.sh | 24 ++++++++--\n 12 files changed, 167 insertions(+), 9 deletions(-)\n create mode 100644 Documentation/git-verify-commit.txt\n create mode 100644 builtin/verify-commit.c\n\n-- \n2.0.0.426.g37dbf84\n"},{"id":"244079","messageId":"3211097b9e6eb07b98d3cefdaf4eabd0d2eb55b0.1402655839.git.git@drmicha.warpmail.net","threadId":"36856","inReplyTo":"cover.1402063795.git.git@drmicha.warpmail.net","subject":"[PATCHv2 1/6] pretty: free the gpg status buf","fromName":"Michael J Gruber","fromEmail":"git@drmicha.warpmail.net","sentAt":"2014-06-13T10:42:43Z","receivedAt":"2014-06-13T10:42:43Z","isPatch":false,"sender":{"key":"git@grubix.eu","avatar":"https://avatars.githubusercontent.com/u/233215?v=4"},"body":"4a868fd (pretty: parse the gpg status lines rather than the output, 2013-02-14)\nmade the gpg status lines available to callers and made sure they freed\nthe used space, but missed one spot.\n\nFree the status line buffer also in the remaining spot.\n\nSigned-off-by: Michael J Gruber \n---\n pretty.c | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/pretty.c b/pretty.c\nindex 4f51287..f1e8a70 100644\n--- a/pretty.c\n+++ b/pretty.c\n@@ -1538,6 +1538,7 @@ void format_commit_message(const struct commit *commit,\n \tfree(context.commit_encoding);\n \tlogmsg_free(context.message, commit);\n \tfree(context.signature_check.gpg_output);\n+\tfree(context.signature_check.gpg_status);\n \tfree(context.signature_check.signer);\n }\n \n-- \n2.0.0.426.g37dbf84\n"},{"id":"244080","messageId":"c774a7be8ccef5c459a6d593cdf2d706a6283f91.1402655839.git.git@drmicha.warpmail.net","threadId":"36856","inReplyTo":"cover.1402063795.git.git@drmicha.warpmail.net","subject":"[PATCHv2 2/6] gpg-interface: provide access to the payload","fromName":"Michael J Gruber","fromEmail":"git@drmicha.warpmail.net","sentAt":"2014-06-13T10:42:44Z","receivedAt":"2014-06-13T10:42:44Z","isPatch":false,"sender":{"key":"git@grubix.eu","avatar":"https://avatars.githubusercontent.com/u/233215?v=4"},"body":"In contrast to tag signatures, commit signatures are put into the\nheader, that is between the other header parts and commit messages.\n\nProvide access to the commit content sans the signature, which is the\npayload that is actually signed. Commit signature verification does the\nparsing anyways, and callers may wish to act on or display the commit\nobject sans the signature.\n\nSigned-off-by: Michael J Gruber \n---\n builtin/merge.c | 1 +\n commit.c | 1 +\n gpg-interface.h | 1 +\n pretty.c | 1 +\n 4 files changed, 4 insertions(+)\n\ndiff --git a/builtin/merge.c b/builtin/merge.c\nindex 428ca24..6a9812a 100644\n--- a/builtin/merge.c\n+++ b/builtin/merge.c\n@@ -1282,6 +1282,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix)\n \t\t\t\tprintf(_(\"Commit %s has a good GPG signature by %s\\n\"),\n \t\t\t\t hex, signature_check.signer);\n \n+\t\t\tfree(signature_check.payload);\n \t\t\tfree(signature_check.gpg_output);\n \t\t\tfree(signature_check.gpg_status);\n \t\t\tfree(signature_check.signer);\ndiff --git a/commit.c b/commit.c\nindex 881be3b..d0ad7f0 100644\n--- a/commit.c\n+++ b/commit.c\n@@ -1219,6 +1219,7 @@ void check_commit_signature(const struct commit* commit, struct signature_check\n \t\t\t\t &gpg_output, &gpg_status);\n \tif (status && !gpg_output.len)\n \t\tgoto out;\n+\tsigc->payload = strbuf_detach(&payload, NULL);\n \tsigc->gpg_output = strbuf_detach(&gpg_output, NULL);\n \tsigc->gpg_status = strbuf_detach(&gpg_status, NULL);\n \tparse_gpg_output(sigc);\ndiff --git a/gpg-interface.h b/gpg-interface.h\nindex a85cb5b..d727c25 100644\n--- a/gpg-interface.h\n+++ b/gpg-interface.h\n@@ -2,6 +2,7 @@\n #define GPG_INTERFACE_H\n \n struct signature_check {\n+\tchar *payload;\n \tchar *gpg_output;\n \tchar *gpg_status;\n \tchar result; /* 0 (not checked),\ndiff --git a/pretty.c b/pretty.c\nindex f1e8a70..24fb877 100644\n--- a/pretty.c\n+++ b/pretty.c\n@@ -1537,6 +1537,7 @@ void format_commit_message(const struct commit *commit,\n \n \tfree(context.commit_encoding);\n \tlogmsg_free(context.message, commit);\n+\tfree(context.signature_check.payload);\n \tfree(context.signature_check.gpg_output);\n \tfree(context.signature_check.gpg_status);\n \tfree(context.signature_check.signer);\n-- \n2.0.0.426.g37dbf84\n"},{"id":"244083","messageId":"48984553eb1d9ac53dd4b9604af568e22c735109.1402655839.git.git@drmicha.warpmail.net","threadId":"36856","inReplyTo":"cover.1402063795.git.git@drmicha.warpmail.net","subject":"[PATCHv2 3/6] verify-commit: scriptable commit signature verification","fromName":"Michael J Gruber","fromEmail":"git@drmicha.warpmail.net","sentAt":"2014-06-13T10:42:45Z","receivedAt":"2014-06-13T10:42:45Z","isPatch":false,"sender":{"key":"git@grubix.eu","avatar":"https://avatars.githubusercontent.com/u/233215?v=4"},"body":"Commit signatures can be verified using \"git show -s --show-signature\"\nor the \"%G?\" pretty format and parsing the output, which is well suited\nfor user inspection, but not for scripting.\n\nProvide a command \"verify-commit\" which is analogous to \"verify-tag\": It\nreturns 0 for good signatures and non-zero otherwise, has the gpg output\non stderr and (optionally) the commit object on stdout, sans the\nsignature, just like \"verify-tag\" does.\n\nSigned-off-by: Michael J Gruber \n---\n Documentation/git-verify-commit.txt | 28 +++++++++++\n Makefile | 1 +\n builtin.h | 1 +\n builtin/verify-commit.c | 98 +++++++++++++++++++++++++++++++++++++\n command-list.txt | 1 +\n git.c | 1 +\n 6 files changed, 130 insertions(+)\n create mode 100644 Documentation/git-verify-commit.txt\n create mode 100644 builtin/verify-commit.c\n\ndiff --git a/Documentation/git-verify-commit.txt b/Documentation/git-verify-commit.txt\nnew file mode 100644\nindex 0000000..9413e28\n--- /dev/null\n+++ b/Documentation/git-verify-commit.txt\n@@ -0,0 +1,28 @@\n+git-verify-commit(1)\n+====================\n+\n+NAME\n+----\n+git-verify-commit - Check the GPG signature of commits\n+\n+SYNOPSIS\n+--------\n+[verse]\n+'git verify-commit' ...\n+\n+DESCRIPTION\n+-----------\n+Validates the gpg signature created by 'git commit -S'.\n+\n+OPTIONS\n+-------\n+-v::\n+--verbose::\n+\tPrint the contents of the commit object before validating it.\n+\n+...::\n+\tSHA-1 identifiers of Git commit objects.\n+\n+GIT\n+---\n+Part of the linkgit:git[1] suite\ndiff --git a/Makefile b/Makefile\nindex 07ea105..b92418d 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -999,6 +999,7 @@ BUILTIN_OBJS += builtin/update-ref.o\n BUILTIN_OBJS += builtin/update-server-info.o\n BUILTIN_OBJS += builtin/upload-archive.o\n BUILTIN_OBJS += builtin/var.o\n+BUILTIN_OBJS += builtin/verify-commit.o\n BUILTIN_OBJS += builtin/verify-pack.o\n BUILTIN_OBJS += builtin/verify-tag.o\n BUILTIN_OBJS += builtin/write-tree.o\ndiff --git a/builtin.h b/builtin.h\nindex c47c110..5d91f31 100644\n--- a/builtin.h\n+++ b/builtin.h\n@@ -128,6 +128,7 @@ extern int cmd_update_server_info(int argc, const char **argv, const char *prefi\n extern int cmd_upload_archive(int argc, const char **argv, const char *prefix);\n extern int cmd_upload_archive_writer(int argc, const char **argv, const char *prefix);\n extern int cmd_var(int argc, const char **argv, const char *prefix);\n+extern int cmd_verify_commit(int argc, const char **argv, const char *prefix);\n extern int cmd_verify_tag(int argc, const char **argv, const char *prefix);\n extern int cmd_version(int argc, const char **argv, const char *prefix);\n extern int cmd_whatchanged(int argc, const char **argv, const char *prefix);\ndiff --git a/builtin/verify-commit.c b/builtin/verify-commit.c\nnew file mode 100644\nindex 0000000..69b7c6d\n--- /dev/null\n+++ b/builtin/verify-commit.c\n@@ -0,0 +1,98 @@\n+/*\n+ * Builtin \"git commit-commit\"\n+ *\n+ * Copyright (c) 2014 Michael J Gruber \n+ *\n+ * Based on git-verify-tag\n+ */\n+#include \"cache.h\"\n+#include \"builtin.h\"\n+#include \"commit.h\"\n+#include \"run-command.h\"\n+#include \n+#include \"parse-options.h\"\n+#include \"gpg-interface.h\"\n+\n+static const char * const verify_commit_usage[] = {\n+\t\tN_(\"git verify-commit [-v|--verbose] ...\"),\n+\t\tNULL\n+};\n+\n+static int run_gpg_verify(const unsigned char *sha1, const char *buf, unsigned long size, int verbose)\n+{\n+\tstruct signature_check signature_check;\n+\n+\tmemset(&signature_check, 0, sizeof(signature_check));\n+\n+\tcheck_commit_signature(lookup_commit(sha1), &signature_check);\n+\n+\tif (verbose && signature_check.payload)\n+\t\tfputs(signature_check.payload, stdout);\n+\n+\tif (signature_check.gpg_output)\n+\t\tfputs(signature_check.gpg_output, stderr);\n+\n+\tfree(signature_check.gpg_output);\n+\tfree(signature_check.gpg_status);\n+\tfree(signature_check.signer);\n+\tfree(signature_check.key);\n+\treturn signature_check.result != 'G';\n+}\n+\n+static int verify_commit(const char *name, int verbose)\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(\"commit '%s' not found.\", name);\n+\n+\ttype = sha1_object_info(sha1, NULL);\n+\tif (type != OBJ_COMMIT)\n+\t\treturn error(\"%s: cannot verify a non-commit 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(sha1, buf, size, verbose);\n+\n+\tfree(buf);\n+\treturn ret;\n+}\n+\n+static int git_verify_commit_config(const char *var, const char *value, void *cb)\n+{\n+\tint status = git_gpg_config(var, value, cb);\n+\tif (status)\n+\t\treturn status;\n+\treturn git_default_config(var, value, cb);\n+}\n+\n+int cmd_verify_commit(int argc, const char **argv, const char *prefix)\n+{\n+\tint i = 1, verbose = 0, had_error = 0;\n+\tconst struct option verify_commit_options[] = {\n+\t\tOPT__VERBOSE(&verbose, N_(\"print commit contents\")),\n+\t\tOPT_END()\n+\t};\n+\n+\tgit_config(git_verify_commit_config, NULL);\n+\n+\targc = parse_options(argc, argv, prefix, verify_commit_options,\n+\t\t\t verify_commit_usage, PARSE_OPT_KEEP_ARGV0);\n+\tif (argc <= i)\n+\t\tusage_with_options(verify_commit_usage, verify_commit_options);\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_commit(argv[i++], verbose))\n+\t\t\thad_error = 1;\n+\treturn had_error;\n+}\ndiff --git a/command-list.txt b/command-list.txt\nindex cf36c3d..a3ff0c9 100644\n--- a/command-list.txt\n+++ b/command-list.txt\n@@ -132,6 +132,7 @@ git-update-server-info synchingrepositories\n git-upload-archive synchelpers\n git-upload-pack synchelpers\n git-var plumbinginterrogators\n+git-verify-commit ancillaryinterrogators\n git-verify-pack plumbinginterrogators\n git-verify-tag ancillaryinterrogators\n gitweb ancillaryinterrogators\ndiff --git a/git.c b/git.c\nindex 7780572..bbc3293 100644\n--- a/git.c\n+++ b/git.c\n@@ -441,6 +441,7 @@ static struct cmd_struct commands[] = {\n \t{ \"upload-archive\", cmd_upload_archive },\n \t{ \"upload-archive--writer\", cmd_upload_archive_writer },\n \t{ \"var\", cmd_var, RUN_SETUP_GENTLY },\n+\t{ \"verify-commit\", cmd_verify_commit, RUN_SETUP },\n \t{ \"verify-pack\", cmd_verify_pack },\n \t{ \"verify-tag\", cmd_verify_tag, RUN_SETUP },\n \t{ \"version\", cmd_version },\n-- \n2.0.0.426.g37dbf84\n"},{"id":"244081","messageId":"99893263f1819646a3a324b2fff0bcd0d56f3818.1402655839.git.git@drmicha.warpmail.net","threadId":"36856","inReplyTo":"cover.1402063795.git.git@drmicha.warpmail.net","subject":"[PATCHv2 4/6] t7510: exit for loop with test result","fromName":"Michael J Gruber","fromEmail":"git@drmicha.warpmail.net","sentAt":"2014-06-13T10:42:46Z","receivedAt":"2014-06-13T10:42:46Z","isPatch":false,"sender":{"key":"git@grubix.eu","avatar":"https://avatars.githubusercontent.com/u/233215?v=4"},"body":"When t7510 was introduced, the author made sure that a for loop in\na subshell would return with the appropriate error code.\n\nMake sure this is true also the for the first line in each loop, which\nwas missed.\n\nSigned-off-by: Michael J Gruber \n---\n t/t7510-signed-commit.sh | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/t/t7510-signed-commit.sh b/t/t7510-signed-commit.sh\nindex 5ddac1a..a5ba48e 100755\n--- a/t/t7510-signed-commit.sh\n+++ b/t/t7510-signed-commit.sh\n@@ -49,7 +49,7 @@ test_expect_success GPG 'show signatures' '\n \t(\n \t\tfor commit in initial second merge fourth-signed fifth-signed sixth-signed master\n \t\tdo\n-\t\t\tgit show --pretty=short --show-signature $commit >actual &&\n+\t\t\tgit show --pretty=short --show-signature $commit >actual || exit 1\n \t\t\tgrep \"Good signature from\" actual || exit 1\n \t\t\t! grep \"BAD signature from\" actual || exit 1\n \t\t\techo $commit OK\n@@ -58,7 +58,7 @@ test_expect_success GPG 'show signatures' '\n \t(\n \t\tfor commit in merge^2 fourth-unsigned sixth-unsigned seventh-unsigned\n \t\tdo\n-\t\t\tgit show --pretty=short --show-signature $commit >actual &&\n+\t\t\tgit show --pretty=short --show-signature $commit >actual || exit 1\n \t\t\tgrep \"Good signature from\" actual && exit 1\n \t\t\t! grep \"BAD signature from\" actual || exit 1\n \t\t\techo $commit OK\n-- \n2.0.0.426.g37dbf84\n"},{"id":"244078","messageId":"3aaf65288733117909150771cc62508f3236d107.1402655839.git.git@drmicha.warpmail.net","threadId":"36856","inReplyTo":"cover.1402063795.git.git@drmicha.warpmail.net","subject":"[PATCHv2 5/6] t7510: test verify-commit","fromName":"Michael J Gruber","fromEmail":"git@drmicha.warpmail.net","sentAt":"2014-06-13T10:42:47Z","receivedAt":"2014-06-13T10:42:47Z","isPatch":false,"sender":{"key":"git@grubix.eu","avatar":"https://avatars.githubusercontent.com/u/233215?v=4"},"body":"This mixes the \"git verify-commit\" tests in with the \"git show\n--show-signature\" tests, to keep the tests more readable.\n\nThe tests already mix in the \"call show\" tests with the \"verify\" tests.\nSo in case of a test beakage, a '-v' run would be needed to reveal the\nexact point of breakage anyway.\n\nAdditionally, test the actual output of \"git verify-commit\" and \"git\nshow --show-signature\" and compare to \"git cat-file\".\n\nSigned-off-by: Michael J Gruber \n---\n t/t7510-signed-commit.sh | 20 +++++++++++++++++++-\n 1 file changed, 19 insertions(+), 1 deletion(-)\n\ndiff --git a/t/t7510-signed-commit.sh b/t/t7510-signed-commit.sh\nindex a5ba48e..89941a7 100755\n--- a/t/t7510-signed-commit.sh\n+++ b/t/t7510-signed-commit.sh\n@@ -45,10 +45,11 @@ test_expect_success GPG 'create signed commits' '\n \tgit tag seventh-signed\n '\n \n-test_expect_success GPG 'show signatures' '\n+test_expect_success GPG 'verify and show signatures' '\n \t(\n \t\tfor commit in initial second merge fourth-signed fifth-signed sixth-signed master\n \t\tdo\n+\t\t\tgit verify-commit $commit || exit 1\n \t\t\tgit show --pretty=short --show-signature $commit >actual || exit 1\n \t\t\tgrep \"Good signature from\" actual || exit 1\n \t\t\t! grep \"BAD signature from\" actual || exit 1\n@@ -58,6 +59,7 @@ test_expect_success GPG 'show signatures' '\n \t(\n \t\tfor commit in merge^2 fourth-unsigned sixth-unsigned seventh-unsigned\n \t\tdo\n+\t\t\t! git verify-commit $commit || exit 1\n \t\t\tgit show --pretty=short --show-signature $commit >actual || exit 1\n \t\t\tgrep \"Good signature from\" actual && exit 1\n \t\t\t! grep \"BAD signature from\" actual || exit 1\n@@ -66,11 +68,25 @@ test_expect_success GPG 'show signatures' '\n \t)\n '\n \n+test_expect_success GPG 'show signed commit with signature' '\n+\tgit show -s initial >commit &&\n+\tgit show -s --show-signature initial >show &&\n+\tgit verify-commit -v initial >verify.1 2>verify.2 &&\n+\tgit cat-file commit initial >cat &&\n+\tgrep -v \"gpg: \" show >show.commit &&\n+\tgrep \"gpg: \" show >show.gpg &&\n+\tgrep -v \"^ \" cat | grep -v \"^gpgsig \" >cat.commit &&\n+\ttest_cmp show.commit commit &&\n+\ttest_cmp show.gpg verify.2 &&\n+\ttest_cmp cat.commit verify.1\n+'\n+\n test_expect_success GPG 'detect fudged signature' '\n \tgit cat-file commit master >raw &&\n \n \tsed -e \"s/seventh/7th forged/\" raw >forged1 &&\n \tgit hash-object -w -t commit forged1 >forged1.commit &&\n+\t! git verify-commit $(cat forged1.commit) &&\n \tgit show --pretty=short --show-signature $(cat forged1.commit) >actual1 &&\n \tgrep \"BAD signature from\" actual1 &&\n \t! grep \"Good signature from\" actual1\n@@ -81,6 +97,7 @@ test_expect_success GPG 'detect fudged signature with NUL' '\n \tcat raw >forged2 &&\n \techo Qwik | tr \"Q\" \"\\000\" >>forged2 &&\n \tgit hash-object -w -t commit forged2 >forged2.commit &&\n+\t! git verify-commit $(cat forged2.commit) &&\n \tgit show --pretty=short --show-signature $(cat forged2.commit) >actual2 &&\n \tgrep \"BAD signature from\" actual2 &&\n \t! grep \"Good signature from\" actual2\n@@ -89,6 +106,7 @@ test_expect_success GPG 'detect fudged signature with NUL' '\n test_expect_success GPG 'amending already signed commit' '\n \tgit checkout fourth-signed^0 &&\n \tgit commit --amend -S --no-edit &&\n+\tgit verify-commit HEAD &&\n \tgit show -s --show-signature HEAD >actual &&\n \tgrep \"Good signature from\" actual &&\n \t! grep \"BAD signature from\" actual\n-- \n2.0.0.426.g37dbf84\n"},{"id":"244082","messageId":"8227d9b69bd6f8e8c318908f49bd52b9379f01ba.1402655839.git.git@drmicha.warpmail.net","threadId":"36856","inReplyTo":"cover.1402063795.git.git@drmicha.warpmail.net","subject":"[PATCHv2 6/6] gpg-interface: provide clear helper for struct signature_check","fromName":"Michael J Gruber","fromEmail":"git@drmicha.warpmail.net","sentAt":"2014-06-13T10:42:48Z","receivedAt":"2014-06-13T10:42:48Z","isPatch":false,"sender":{"key":"git@grubix.eu","avatar":"https://avatars.githubusercontent.com/u/233215?v=4"},"body":"The struct has been growing members whose malloced memory needs to be\nfreed. Do this with one helper function so that no malloced memory shall\nbe left unfreed.\n\nSigned-off-by: Michael J Gruber \n---\n builtin/merge.c | 6 +-----\n builtin/verify-commit.c | 5 +----\n gpg-interface.c | 14 ++++++++++++++\n gpg-interface.h | 1 +\n pretty.c | 5 +----\n 5 files changed, 18 insertions(+), 13 deletions(-)\n\ndiff --git a/builtin/merge.c b/builtin/merge.c\nindex 6a9812a..e50323d 100644\n--- a/builtin/merge.c\n+++ b/builtin/merge.c\n@@ -1282,11 +1282,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix)\n \t\t\t\tprintf(_(\"Commit %s has a good GPG signature by %s\\n\"),\n \t\t\t\t hex, signature_check.signer);\n \n-\t\t\tfree(signature_check.payload);\n-\t\t\tfree(signature_check.gpg_output);\n-\t\t\tfree(signature_check.gpg_status);\n-\t\t\tfree(signature_check.signer);\n-\t\t\tfree(signature_check.key);\n+\t\t\tsignature_check_clear(&signature_check);\n \t\t}\n \t}\n \ndiff --git a/builtin/verify-commit.c b/builtin/verify-commit.c\nindex 69b7c6d..d254fcf 100644\n--- a/builtin/verify-commit.c\n+++ b/builtin/verify-commit.c\n@@ -32,10 +32,7 @@ static int run_gpg_verify(const unsigned char *sha1, const char *buf, unsigned l\n \tif (signature_check.gpg_output)\n \t\tfputs(signature_check.gpg_output, stderr);\n \n-\tfree(signature_check.gpg_output);\n-\tfree(signature_check.gpg_status);\n-\tfree(signature_check.signer);\n-\tfree(signature_check.key);\n+\tsignature_check_clear(&signature_check);\n \treturn signature_check.result != 'G';\n }\n \ndiff --git a/gpg-interface.c b/gpg-interface.c\nindex 8b0e874..ff07012 100644\n--- a/gpg-interface.c\n+++ b/gpg-interface.c\n@@ -7,6 +7,20 @@\n static char *configured_signing_key;\n static const char *gpg_program = \"gpg\";\n \n+void signature_check_clear(struct signature_check *sigc)\n+{\n+\tfree(sigc->payload);\n+\tfree(sigc->gpg_output);\n+\tfree(sigc->gpg_status);\n+\tfree(sigc->signer);\n+\tfree(sigc->key);\n+\tsigc->payload = NULL;\n+\tsigc->gpg_output = NULL;\n+\tsigc->gpg_status = NULL;\n+\tsigc->signer = NULL;\n+\tsigc->key = NULL;\n+}\n+\n void set_signing_key(const char *key)\n {\n \tfree(configured_signing_key);\ndiff --git a/gpg-interface.h b/gpg-interface.h\nindex d727c25..37c23da 100644\n--- a/gpg-interface.h\n+++ b/gpg-interface.h\n@@ -14,6 +14,7 @@ struct signature_check {\n \tchar *key;\n };\n \n+extern void signature_check_clear(struct signature_check *sigc);\n extern int sign_buffer(struct strbuf *buffer, struct strbuf *signature, const char *signing_key);\n extern int verify_signed_buffer(const char *payload, size_t payload_size, const char *signature, size_t signature_size, struct strbuf *gpg_output, struct strbuf *gpg_status);\n extern int git_gpg_config(const char *, const char *, void *);\ndiff --git a/pretty.c b/pretty.c\nindex 24fb877..ac901b8 100644\n--- a/pretty.c\n+++ b/pretty.c\n@@ -1537,10 +1537,7 @@ void format_commit_message(const struct commit *commit,\n \n \tfree(context.commit_encoding);\n \tlogmsg_free(context.message, commit);\n-\tfree(context.signature_check.payload);\n-\tfree(context.signature_check.gpg_output);\n-\tfree(context.signature_check.gpg_status);\n-\tfree(context.signature_check.signer);\n+\tsignature_check_clear(&context.signature_check);\n }\n \n static void pp_header(struct pretty_print_context *pp,\n-- \n2.0.0.426.g37dbf84\n"},{"id":"244084","messageId":"20140613110901.GB14066@sigill.intra.peff.net","threadId":"36856","inReplyTo":"539ACA8A.90108@drmicha.warpmail.net","subject":"Re: [PATCH 3/3] verify-commit: scriptable commit signature verification","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-06-13T11:09:01Z","receivedAt":"2014-06-13T11:09:01Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jun 13, 2014 at 11:55:22AM +0200, Michael J Gruber wrote:\n\n> > Did you give any thought to just having a \"git verify\" command, instead\n> > of separate tag/verify commands?\n> \n> Yes. (mathematician's answer)\n\nCute.\n\n> You know not only the outcome but also why I refrained from doing so:\n> compatibility. We would need to deprecate verify-tag.\n\nYes, we'd certainly leave verify-tag in place for compatibility. I don't\nthink that makes \"git verify\" a bad idea necessarily, if it is a better\ninterface. But...\n\n> But there is also a more subtle reason: If you want to verify a signed\n> commit, you want to be sure that it actually is a commit. \"verify\" could\n> easily branch code paths based on the object type, but I'm not sure that\n> is desirable, at least not by default.\n\nYes, I wasn't sure about that part. I think it really depends on what\npeople want to use it for. I was thinking more of a porcelain, anyway,\nto just check whatever signatures are available. But I don't really sign\nmy commits right now anyway, so I'm somewhat guessing.\n\nEven if we decide to do something like that, I suppose having\nverify-commit isn't the end of the world. It could just remain the\nplumbing interface, as verify-tag would. So I don't think my half-formed\nthoughts are any reason to block your series.\n\n> That is a general issue with verifying signatures: it can be automated\n> only if you employ a strict trust model and a very limited keyring.\n> \"valid signature\" means only as much as the signatures that your gpg\n> accepts can be really trusted.\n> \n> Comparing uid's really buys you nothing in the sense that everyone can\n> have a key with uid \"Jeff King signed by some other\n> keys.\n\nSort of. The crypto proves that the commit was signed by a particular\nkey, and then there are two mappings:\n\n 1. What is the identity associated with that key?\n\n 2. Is that identity somebody who \"should\" have signed the commit?\n\nWe assume that GPG takes care of the first one with the web of trust.\nEven if you have the key and can check the signature, it will still\ncomplain about an untrusted uid (and we reflect that with 'U' in the gpg\nstatus). There is no point in looking at step 2 if step 1 did not check\nout.\n\nFor the second one, I think it really depends on the project workflow,\nand you may even want multiple policies within a project (e.g., perhaps\nonly some uids can merge to master). But one obvious check we can make\nis \"does the identity in the commit data match one of the uids?\". True,\nyou don't _need_ that; you can always just use \"%GS\", and throw away the\ncommitter and author headers. But we show those names in lots of output.\nYou could, for example, \"verify\" that each commit's author id matches\nits gpg signature, and then use the regular tools to do further work\n(e.g., running \"git blame\"), and be confident that the author you see\nmatches the signature.\n\nThis should definitely be optional. Even something as simple as \"author\nid matches the gpg key\" would not always work (for example, in git.git\nwe pick patches from the list, which means only Junio can sign the\nobjects, but he is not the author). But just because it is optional does\nnot mean it would not be a useful tool for some workflows.\n\n> On the other hand, it's perfectly OK to use different uids for git\n> commits and signatures. The e-mail address I use for the git list and\n> commits, for example, is clearly a \"plus address\", which helps me\n> organize things; my personal key has the primary address as uid.\n> \n> I really think all this is up to local policies for individual use cases.\n\nVery much agreed. My suggestion was more about providing tools for\npeople to build those policies.\n\nI realize this isn't really your itch to scratch. It's just that when I\nsee a description like \"verify a commit\", I wonder what exactly \"verify\"\nmeans.\n\n-Peff\n"},{"id":"244085","messageId":"20140613111945.GC14066@sigill.intra.peff.net","threadId":"36856","inReplyTo":"48984553eb1d9ac53dd4b9604af568e22c735109.1402655839.git.git@drmicha.warpmail.net","subject":"Re: [PATCHv2 3/6] verify-commit: scriptable commit signature verification","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-06-13T11:19:45Z","receivedAt":"2014-06-13T11:19:45Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jun 13, 2014 at 12:42:45PM +0200, Michael J Gruber wrote:\n\n> +\n> +\tfree(signature_check.gpg_output);\n> +\tfree(signature_check.gpg_status);\n> +\tfree(signature_check.signer);\n> +\tfree(signature_check.key);\n> +\treturn signature_check.result != 'G';\n> +}\n\nHow about .payload here?\n\n> +\ttype = sha1_object_info(sha1, NULL);\n> +\tif (type != OBJ_COMMIT)\n> +\t\treturn error(\"%s: cannot verify a non-commit 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\nI think you can drop the sha1_object_info call and just check \"type\"\nfrom read_sha1_file (they _should_ agree, but if they do not, I'd rather\npay attention to the one that came along with the buffer). And this is\nthe uncommon error path, so expanding the object into memory before we\ndie is not a big deal.\n\nShould this peel to a commit if given a tag? I'd say probably. I know\nyou raised the issue elsewhere of keeping things simple, but I think if\nyou are calling verify-commit, you know you want a commit, and we should\ntreat the argument as a commit-ish. Anyway, if you go that route, then\nlookup_commit_or_die is probably what you want.\n\nAlso, minor nit, but we typically do not end the error messages with a\nfull stop (we've been rather inconsistent in the past, but these days\nseem to mostly settle on no punctuation).\n\n-Peff\n"},{"id":"244086","messageId":"20140613113957.GD14066@sigill.intra.peff.net","threadId":"36856","inReplyTo":"3211097b9e6eb07b98d3cefdaf4eabd0d2eb55b0.1402655839.git.git@drmicha.warpmail.net","subject":"Re: [PATCHv2 1/6] pretty: free the gpg status buf","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-06-13T11:39:57Z","receivedAt":"2014-06-13T11:39:57Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jun 13, 2014 at 12:42:43PM +0200, Michael J Gruber wrote:\n\n> 4a868fd (pretty: parse the gpg status lines rather than the output, 2013-02-14)\n> made the gpg status lines available to callers and made sure they freed\n> the used space, but missed one spot.\n> \n> Free the status line buffer also in the remaining spot.\n> \n> Signed-off-by: Michael J Gruber \n> ---\n> pretty.c | 1 +\n> 1 file changed, 1 insertion(+)\n> \n> diff --git a/pretty.c b/pretty.c\n> index 4f51287..f1e8a70 100644\n> --- a/pretty.c\n> +++ b/pretty.c\n> @@ -1538,6 +1538,7 @@ void format_commit_message(const struct commit *commit,\n> \tfree(context.commit_encoding);\n> \tlogmsg_free(context.message, commit);\n> \tfree(context.signature_check.gpg_output);\n> +\tfree(context.signature_check.gpg_status);\n> \tfree(context.signature_check.signer);\n\nWhat about .key?\n\nI would have expected your patch 6 to come first, which would fix this,\nand then save you from making similar mistakes in patch 3. :)\n\n-Peff\n"},{"id":"244087","messageId":"539AE476.4030205@drmicha.warpmail.net","threadId":"36856","inReplyTo":"20140613111945.GC14066@sigill.intra.peff.net","subject":"Re: [PATCHv2 3/6] verify-commit: scriptable commit signature verification","fromName":"Michael J Gruber","fromEmail":"git@drmicha.warpmail.net","sentAt":"2014-06-13T11:45:58Z","receivedAt":"2014-06-13T11:45:58Z","isPatch":false,"sender":{"key":"git@grubix.eu","avatar":"https://avatars.githubusercontent.com/u/233215?v=4"},"body":"Jeff King venit, vidit, dixit 13.06.2014 13:19:\n> On Fri, Jun 13, 2014 at 12:42:45PM +0200, Michael J Gruber wrote:\n> \n>> +\n>> +\tfree(signature_check.gpg_output);\n>> +\tfree(signature_check.gpg_status);\n>> +\tfree(signature_check.signer);\n>> +\tfree(signature_check.key);\n>> +\treturn signature_check.result != 'G';\n>> +}\n> \n> How about .payload here?\n\nI sneekily fix this in 6/6... I thought 3/6 is on next already, too late\nfor a real v2. Otherwise I would put 6/6 before everything else.\n\n>> +\ttype = sha1_object_info(sha1, NULL);\n>> +\tif (type != OBJ_COMMIT)\n>> +\t\treturn error(\"%s: cannot verify a non-commit 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> I think you can drop the sha1_object_info call and just check \"type\"\n> from read_sha1_file (they _should_ agree, but if they do not, I'd rather\n> pay attention to the one that came along with the buffer). And this is\n> the uncommon error path, so expanding the object into memory before we\n> die is not a big deal.\n> \n> Should this peel to a commit if given a tag? I'd say probably. I know\n> you raised the issue elsewhere of keeping things simple, but I think if\n> you are calling verify-commit, you know you want a commit, and we should\n> treat the argument as a commit-ish. Anyway, if you go that route, then\n> lookup_commit_or_die is probably what you want.\n> \n> Also, minor nit, but we typically do not end the error messages with a\n> full stop (we've been rather inconsistent in the past, but these days\n> seem to mostly settle on no punctuation).\n> \n> -Peff\n\nBoth of these issues actually come for following verify-tag as closely\nas possible. If 3 is not applied already, I should do away with\nsha1_object_info.\n\nAbout the peeling I'm not so sure, since there's a difference between a\nsigned tag pointing to a commit and a signed commit. Since\nverify-{tag,commit} are bare metal plumbing, I would expect callers to\nuse ^{commit} explicitly if they don't care how peels to a\ncommit.\n\nMichael\n"},{"id":"244088","messageId":"20140613114615.GE14066@sigill.intra.peff.net","threadId":"36856","inReplyTo":"99893263f1819646a3a324b2fff0bcd0d56f3818.1402655839.git.git@drmicha.warpmail.net","subject":"Re: [PATCHv2 4/6] t7510: exit for loop with test result","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-06-13T11:46:15Z","receivedAt":"2014-06-13T11:46:15Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jun 13, 2014 at 12:42:46PM +0200, Michael J Gruber wrote:\n\n> When t7510 was introduced, the author made sure that a for loop in\n> a subshell would return with the appropriate error code.\n> \n> Make sure this is true also the for the first line in each loop, which\n> was missed.\n> \n> Signed-off-by: Michael J Gruber \n> ---\n> t/t7510-signed-commit.sh | 4 ++--\n> 1 file changed, 2 insertions(+), 2 deletions(-)\n> \n> diff --git a/t/t7510-signed-commit.sh b/t/t7510-signed-commit.sh\n> index 5ddac1a..a5ba48e 100755\n> --- a/t/t7510-signed-commit.sh\n> +++ b/t/t7510-signed-commit.sh\n> @@ -49,7 +49,7 @@ test_expect_success GPG 'show signatures' '\n> \t(\n> \t\tfor commit in initial second merge fourth-signed fifth-signed sixth-signed master\n> \t\tdo\n> -\t\t\tgit show --pretty=short --show-signature $commit >actual &&\n> +\t\t\tgit show --pretty=short --show-signature $commit >actual || exit 1\n> \t\t\tgrep \"Good signature from\" actual || exit 1\n\nHrm. The original is:\n\n X &&\n Y || exit 1\n\nWon't that still exit (i.e., it is already correct)? Doing:\n\n for X in true false; do\n for Y in true false; do\n ($X && $Y || exit 1)\n echo \"$X/$Y: $?\"\n done\n done\n\nyields:\n\n true/true: 0\n true/false: 1\n false/true: 1\n false/false: 1\n\n(and should still short-circuit Y, because we go from left-to-right).\n\nI do not mind changing it to keep the style of each line consistent,\nthough. I would have written it as a series of \"&&\"-chains, with a\nsingle exit at the end, but I think that is just a matter of preference.\n\n-Peff\n"},{"id":"244089","messageId":"20140613115004.GF14066@sigill.intra.peff.net","threadId":"36856","inReplyTo":"539AE476.4030205@drmicha.warpmail.net","subject":"Re: [PATCHv2 3/6] verify-commit: scriptable commit signature verification","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-06-13T11:50:04Z","receivedAt":"2014-06-13T11:50:04Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jun 13, 2014 at 01:45:58PM +0200, Michael J Gruber wrote:\n\n> I sneekily fix this in 6/6... I thought 3/6 is on next already, too late\n> for a real v2. Otherwise I would put 6/6 before everything else.\n\nAh, yeah, I assumed we were still re-rolling (and it looks like you're\njust on pu so far).\n\n> About the peeling I'm not so sure, since there's a difference between a\n> signed tag pointing to a commit and a signed commit.\n\nThere is, but \"verify-commit\" is always going to verify the commit, no?\nNot peeling will always result in an error, and never do anything\nuseful.\n\nI admit it's probably not going to come up too often, though. And I\ndon't have any argument beyond \"it makes sense to me\", so I won't push\nfor it further.\n\n-Peff\n"},{"id":"244090","messageId":"20140613115125.GG14066@sigill.intra.peff.net","threadId":"36856","inReplyTo":"3aaf65288733117909150771cc62508f3236d107.1402655839.git.git@drmicha.warpmail.net","subject":"Re: [PATCHv2 5/6] t7510: test verify-commit","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-06-13T11:51:25Z","receivedAt":"2014-06-13T11:51:25Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jun 13, 2014 at 12:42:47PM +0200, Michael J Gruber wrote:\n\n> test_expect_success GPG 'detect fudged signature' '\n> \tgit cat-file commit master >raw &&\n> \n> \tsed -e \"s/seventh/7th forged/\" raw >forged1 &&\n> \tgit hash-object -w -t commit forged1 >forged1.commit &&\n> +\t! git verify-commit $(cat forged1.commit) &&\n\nPlease use test_must_fail here (and further down), which will catch\nthings like signal death.\n\n-Peff\n"},{"id":"244092","messageId":"539AE8CA.50009@drmicha.warpmail.net","threadId":"36856","inReplyTo":"20140613114615.GE14066@sigill.intra.peff.net","subject":"Re: [PATCHv2 4/6] t7510: exit for loop with test result","fromName":"Michael J Gruber","fromEmail":"git@drmicha.warpmail.net","sentAt":"2014-06-13T12:04:26Z","receivedAt":"2014-06-13T12:04:26Z","isPatch":false,"sender":{"key":"git@grubix.eu","avatar":"https://avatars.githubusercontent.com/u/233215?v=4"},"body":"Jeff King venit, vidit, dixit 13.06.2014 13:46:\n> On Fri, Jun 13, 2014 at 12:42:46PM +0200, Michael J Gruber wrote:\n> \n>> When t7510 was introduced, the author made sure that a for loop in\n>> a subshell would return with the appropriate error code.\n>>\n>> Make sure this is true also the for the first line in each loop, which\n>> was missed.\n>>\n>> Signed-off-by: Michael J Gruber \n>> ---\n>> t/t7510-signed-commit.sh | 4 ++--\n>> 1 file changed, 2 insertions(+), 2 deletions(-)\n>>\n>> diff --git a/t/t7510-signed-commit.sh b/t/t7510-signed-commit.sh\n>> index 5ddac1a..a5ba48e 100755\n>> --- a/t/t7510-signed-commit.sh\n>> +++ b/t/t7510-signed-commit.sh\n>> @@ -49,7 +49,7 @@ test_expect_success GPG 'show signatures' '\n>> \t(\n>> \t\tfor commit in initial second merge fourth-signed fifth-signed sixth-signed master\n>> \t\tdo\n>> -\t\t\tgit show --pretty=short --show-signature $commit >actual &&\n>> +\t\t\tgit show --pretty=short --show-signature $commit >actual || exit 1\n>> \t\t\tgrep \"Good signature from\" actual || exit 1\n> \n> Hrm. The original is:\n> \n> X &&\n> Y || exit 1\n> \n> Won't that still exit (i.e., it is already correct)? Doing:\n> \n> for X in true false; do\n> for Y in true false; do\n> ($X && $Y || exit 1)\n> echo \"$X/$Y: $?\"\n> done\n> done\n> \n> yields:\n> \n> true/true: 0\n> true/false: 1\n> false/true: 1\n> false/false: 1\n> \n> (and should still short-circuit Y, because we go from left-to-right).\n> \n> I do not mind changing it to keep the style of each line consistent,\n> though. I would have written it as a series of \"&&\"-chains, with a\n> single exit at the end, but I think that is just a matter of preference.\n\nIf I remember correctly, I put something failing on the first line of\nthe original version, and the test succeeded. I think the point is that\nwe have a for loop in a subshell, and we need to make sure that the\nfalse of one iteration is not overwritten by the true of the next one -\n\"exit 1\" makes sure to \"break\" the for loop and exit the subshell.\n(The chain should do that as well, I'll recheck.)\n\nMichael\n"},{"id":"244093","messageId":"539AEABD.3070906@drmicha.warpmail.net","threadId":"36856","inReplyTo":"20140613115004.GF14066@sigill.intra.peff.net","subject":"Re: [PATCHv2 3/6] verify-commit: scriptable commit signature verification","fromName":"Michael J Gruber","fromEmail":"git@drmicha.warpmail.net","sentAt":"2014-06-13T12:12:45Z","receivedAt":"2014-06-13T12:12:45Z","isPatch":false,"sender":{"key":"git@grubix.eu","avatar":"https://avatars.githubusercontent.com/u/233215?v=4"},"body":"Jeff King venit, vidit, dixit 13.06.2014 13:50:\n> On Fri, Jun 13, 2014 at 01:45:58PM +0200, Michael J Gruber wrote:\n> \n>> I sneekily fix this in 6/6... I thought 3/6 is on next already, too late\n>> for a real v2. Otherwise I would put 6/6 before everything else.\n> \n> Ah, yeah, I assumed we were still re-rolling (and it looks like you're\n> just on pu so far).\n\nWe are, I had misread a \"What's cooking\". So a reroll it is.\n\n>> About the peeling I'm not so sure, since there's a difference between a\n>> signed tag pointing to a commit and a signed commit.\n> \n> There is, but \"verify-commit\" is always going to verify the commit, no?\n> Not peeling will always result in an error, and never do anything\n> useful.\n> \n> I admit it's probably not going to come up too often, though. And I\n> don't have any argument beyond \"it makes sense to me\", so I won't push\n> for it further.\n> \n> -Peff\n\nI guess it boils down to the fact how plumberish it's supposed to be.\nSince it's about verification, for some definition of \"verify\", I'd\nrather apply as few automatisms as possible. If the caller wants to know\nwhether commit deadbeef carries a valid commit signature I'd rather\ncheck that very object.\n\nI also picture doing the \"git-verify\" thing in the future (with\n\"--commit\" and \"--tag\" options which make the command insist on the\nobject type), and then we would not want to peel tags under the hood.\n\nMichael\n"},{"id":"244091","messageId":"539AEB41.6090306@drmicha.warpmail.net","threadId":"36856","inReplyTo":"20140613115125.GG14066@sigill.intra.peff.net","subject":"Re: [PATCHv2 5/6] t7510: test verify-commit","fromName":"Michael J Gruber","fromEmail":"git@drmicha.warpmail.net","sentAt":"2014-06-13T12:14:57Z","receivedAt":"2014-06-13T12:14:57Z","isPatch":false,"sender":{"key":"git@grubix.eu","avatar":"https://avatars.githubusercontent.com/u/233215?v=4"},"body":"Jeff King venit, vidit, dixit 13.06.2014 13:51:\n> On Fri, Jun 13, 2014 at 12:42:47PM +0200, Michael J Gruber wrote:\n> \n>> test_expect_success GPG 'detect fudged signature' '\n>> \tgit cat-file commit master >raw &&\n>> \n>> \tsed -e \"s/seventh/7th forged/\" raw >forged1 &&\n>> \tgit hash-object -w -t commit forged1 >forged1.commit &&\n>> +\t! git verify-commit $(cat forged1.commit) &&\n> \n> Please use test_must_fail here (and further down), which will catch\n> things like signal death.\n\nAgain, that is an issue of keeping the style of the surrounding code\n(which is relatively new) vs. doing it differently. I don't mind\nchanging t7510 to a different style, though.\n\nMichael\n"},{"id":"244120","messageId":"539AED0C.8050107@drmicha.warpmail.net","threadId":"36856","inReplyTo":"539AE8CA.50009@drmicha.warpmail.net","subject":"Re: [PATCHv2 4/6] t7510: exit for loop with test result","fromName":"Michael J Gruber","fromEmail":"git@drmicha.warpmail.net","sentAt":"2014-06-13T12:22:36Z","receivedAt":"2014-06-13T12:22:36Z","isPatch":false,"sender":{"key":"git@grubix.eu","avatar":"https://avatars.githubusercontent.com/u/233215?v=4"},"body":"Michael J Gruber venit, vidit, dixit 13.06.2014 14:04:\n> Jeff King venit, vidit, dixit 13.06.2014 13:46:\n>> On Fri, Jun 13, 2014 at 12:42:46PM +0200, Michael J Gruber wrote:\n>>\n>>> When t7510 was introduced, the author made sure that a for loop in\n>>> a subshell would return with the appropriate error code.\n>>>\n>>> Make sure this is true also the for the first line in each loop, which\n>>> was missed.\n>>>\n>>> Signed-off-by: Michael J Gruber \n>>> ---\n>>> t/t7510-signed-commit.sh | 4 ++--\n>>> 1 file changed, 2 insertions(+), 2 deletions(-)\n>>>\n>>> diff --git a/t/t7510-signed-commit.sh b/t/t7510-signed-commit.sh\n>>> index 5ddac1a..a5ba48e 100755\n>>> --- a/t/t7510-signed-commit.sh\n>>> +++ b/t/t7510-signed-commit.sh\n>>> @@ -49,7 +49,7 @@ test_expect_success GPG 'show signatures' '\n>>> \t(\n>>> \t\tfor commit in initial second merge fourth-signed fifth-signed sixth-signed master\n>>> \t\tdo\n>>> -\t\t\tgit show --pretty=short --show-signature $commit >actual &&\n>>> +\t\t\tgit show --pretty=short --show-signature $commit >actual || exit 1\n>>> \t\t\tgrep \"Good signature from\" actual || exit 1\n>>\n>> Hrm. The original is:\n>>\n>> X &&\n>> Y || exit 1\n>>\n>> Won't that still exit (i.e., it is already correct)? Doing:\n>>\n>> for X in true false; do\n>> for Y in true false; do\n>> ($X && $Y || exit 1)\n>> echo \"$X/$Y: $?\"\n>> done\n>> done\n>>\n>> yields:\n>>\n>> true/true: 0\n>> true/false: 1\n>> false/true: 1\n>> false/false: 1\n>>\n>> (and should still short-circuit Y, because we go from left-to-right).\n>>\n>> I do not mind changing it to keep the style of each line consistent,\n>> though. I would have written it as a series of \"&&\"-chains, with a\n>> single exit at the end, but I think that is just a matter of preference.\n> \n> If I remember correctly, I put something failing on the first line of\n> the original version, and the test succeeded. I think the point is that\n> we have a for loop in a subshell, and we need to make sure that the\n> false of one iteration is not overwritten by the true of the next one -\n> \"exit 1\" makes sure to \"break\" the for loop and exit the subshell.\n> (The chain should do that as well, I'll recheck.)\n\n... the chain does not, which is the point :)\n\nWith X && Y || exit 1 inside the loop, the loop statement will return\nfalse, but the loop will continue (if X returns false), which is exactly\nthe problem that the exit avoids.\n\nMake your example iterate over false true instead in the inner loop and\nyou'll see ;)\n\nMichael\n"},{"id":"244128","messageId":"539AEF7E.1050402@drmicha.warpmail.net","threadId":"36856","inReplyTo":"539AED0C.8050107@drmicha.warpmail.net","subject":"Re: [PATCHv2 4/6] t7510: exit for loop with test result","fromName":"Michael J Gruber","fromEmail":"git@drmicha.warpmail.net","sentAt":"2014-06-13T12:33:02Z","receivedAt":"2014-06-13T12:33:02Z","isPatch":false,"sender":{"key":"git@grubix.eu","avatar":"https://avatars.githubusercontent.com/u/233215?v=4"},"body":"Michael J Gruber venit, vidit, dixit 13.06.2014 14:22:\n> Michael J Gruber venit, vidit, dixit 13.06.2014 14:04:\n>> Jeff King venit, vidit, dixit 13.06.2014 13:46:\n>>> On Fri, Jun 13, 2014 at 12:42:46PM +0200, Michael J Gruber wrote:\n>>>\n>>>> When t7510 was introduced, the author made sure that a for loop in\n>>>> a subshell would return with the appropriate error code.\n>>>>\n>>>> Make sure this is true also the for the first line in each loop, which\n>>>> was missed.\n>>>>\n>>>> Signed-off-by: Michael J Gruber \n>>>> ---\n>>>> t/t7510-signed-commit.sh | 4 ++--\n>>>> 1 file changed, 2 insertions(+), 2 deletions(-)\n>>>>\n>>>> diff --git a/t/t7510-signed-commit.sh b/t/t7510-signed-commit.sh\n>>>> index 5ddac1a..a5ba48e 100755\n>>>> --- a/t/t7510-signed-commit.sh\n>>>> +++ b/t/t7510-signed-commit.sh\n>>>> @@ -49,7 +49,7 @@ test_expect_success GPG 'show signatures' '\n>>>> \t(\n>>>> \t\tfor commit in initial second merge fourth-signed fifth-signed sixth-signed master\n>>>> \t\tdo\n>>>> -\t\t\tgit show --pretty=short --show-signature $commit >actual &&\n>>>> +\t\t\tgit show --pretty=short --show-signature $commit >actual || exit 1\n>>>> \t\t\tgrep \"Good signature from\" actual || exit 1\n>>>\n>>> Hrm. The original is:\n>>>\n>>> X &&\n>>> Y || exit 1\n>>>\n>>> Won't that still exit (i.e., it is already correct)? Doing:\n>>>\n>>> for X in true false; do\n>>> for Y in true false; do\n>>> ($X && $Y || exit 1)\n>>> echo \"$X/$Y: $?\"\n>>> done\n>>> done\n>>>\n>>> yields:\n>>>\n>>> true/true: 0\n>>> true/false: 1\n>>> false/true: 1\n>>> false/false: 1\n>>>\n>>> (and should still short-circuit Y, because we go from left-to-right).\n>>>\n>>> I do not mind changing it to keep the style of each line consistent,\n>>> though. I would have written it as a series of \"&&\"-chains, with a\n>>> single exit at the end, but I think that is just a matter of preference.\n>>\n>> If I remember correctly, I put something failing on the first line of\n>> the original version, and the test succeeded. I think the point is that\n>> we have a for loop in a subshell, and we need to make sure that the\n>> false of one iteration is not overwritten by the true of the next one -\n>> \"exit 1\" makes sure to \"break\" the for loop and exit the subshell.\n>> (The chain should do that as well, I'll recheck.)\n> \n> ... the chain does not, which is the point :)\n> \n> With X && Y || exit 1 inside the loop, the loop statement will return\n> false, but the loop will continue (if X returns false), which is exactly\n> the problem that the exit avoids.\n> \n> Make your example iterate over false true instead in the inner loop and\n> you'll see ;)\n> \n> Michael\n\n... with this loop, sorry:\n\nfor X in true false; do\n for Y in false true; do\n ($X && $Y || exit 1)\n done\n echo \"$X/last inner $Y: $?\"\ndone\n\ngives\n\ntrue/last inner true: 0\nfalse/last inner true: 1\n\neven though on both cases we have at least one failure of Y. (failure of\none subtest = failure of the test)\n\nLooping in the other order:\n\nfor X in true false; do\n for Y in true false; do\n ($X && $Y || exit 1)\n done\n echo \"$X/last inner $Y: $?\"\ndone\n\ngives\n\ntrue/last inner false: 1\nfalse/last inner false: 1\n\nas it should be.\n"},{"id":"244129","messageId":"20140613124544.GA18621@sigill.intra.peff.net","threadId":"36856","inReplyTo":"539AEF7E.1050402@drmicha.warpmail.net","subject":"Re: [PATCHv2 4/6] t7510: exit for loop with test result","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-06-13T12:45:44Z","receivedAt":"2014-06-13T12:45:44Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jun 13, 2014 at 02:33:02PM +0200, Michael J Gruber wrote:\n\n> > With X && Y || exit 1 inside the loop, the loop statement will return\n> > false, but the loop will continue (if X returns false), which is exactly\n> > the problem that the exit avoids.\n> > \n> > Make your example iterate over false true instead in the inner loop and\n> > you'll see ;)\n> > \n> > Michael\n> \n> ... with this loop, sorry:\n> \n> for X in true false; do\n> for Y in false true; do\n> ($X && $Y || exit 1)\n> done\n> echo \"$X/last inner $Y: $?\"\n> done\n\nI'm somewhat confused, as my loops were meant only to expand the truth\ntable, not to simulate a real loop in the code. That is why I have a\nsubshell in the loop around my exit, to make sure we keep looping. In\nthe real code, the subshell surrounds the whole loop (so that an \"exit\"\nleaves the entire loop without leaving the whole script).\n\nThe actual code is more like:\n\n (\n for i in a b c; do\n echo $i: got to first step &&\n test $i != b && \n echo $i: got to second step || exit 1\n done\n )\n echo overall status: $?\n\nwhich should fail on the second loop iteration. And it does:\n\n a: got to first step\n a: got to second step\n b: got to first step\n overall status: 1\n\nThat is, we short-circuit to the \"exit 1\" as soon as \"test $i != b\"\nfails. You can replace the use of \"$?\" above with more \"&&\"-chaining, of\ncourse.\n\n-Peff\n"},{"id":"244130","messageId":"539AF49F.3010309@viscovery.net","threadId":"36856","inReplyTo":"539AEF7E.1050402@drmicha.warpmail.net","subject":"Re: [PATCHv2 4/6] t7510: exit for loop with test result","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2014-06-13T12:54:55Z","receivedAt":"2014-06-13T12:54:55Z","isPatch":false,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 6/13/2014 14:33, schrieb Michael J Gruber:\n> .... with this loop, sorry:\n> \n> for X in true false; do\n> for Y in false true; do\n> ($X && $Y || exit 1)\n> done\n> echo \"$X/last inner $Y: $?\"\n> done\n> \n> gives\n> \n> true/last inner true: 0\n> false/last inner true: 1\n> \n> even though on both cases we have at least one failure of Y. (failure of\n> one subtest = failure of the test)\n\nPlace the loop(s) inside the subshell, and you observe termination on the\nfirst failure, and a failure exit code of the subshell.\n\nThe change proposed in this patch should not be necessary. Something else\nmust be wrong with your tests.\n\nAh, here it is:\n\n@@ -58,7 +58,7 @@ test_expect_success GPG 'show signatures' '\n \t(\n \t\tfor commit in merge^2 fourth-unsigned sixth-unsigned seventh-unsigned\n \t\tdo\n-\t\t\tgit show --pretty=short --show-signature $commit >actual &&\n+\t\t\tgit show --pretty=short --show-signature $commit >actual || exit 1\n \t\t\tgrep \"Good signature from\" actual && exit 1\n \t\t\t! grep \"BAD signature from\" actual || exit 1\n \t\t\techo $commit OK\n\nNotice the '&& exit 1'! It should be '|| exit 1', right?\n\n-- Hannes\n"},{"id":"244131","messageId":"539AF748.3000901@drmicha.warpmail.net","threadId":"36856","inReplyTo":"539AF49F.3010309@viscovery.net","subject":"Re: [PATCHv2 4/6] t7510: exit for loop with test result","fromName":"Michael J Gruber","fromEmail":"git@drmicha.warpmail.net","sentAt":"2014-06-13T13:06:16Z","receivedAt":"2014-06-13T13:06:16Z","isPatch":false,"sender":{"key":"git@grubix.eu","avatar":"https://avatars.githubusercontent.com/u/233215?v=4"},"body":"Johannes Sixt venit, vidit, dixit 13.06.2014 14:54:\n> Am 6/13/2014 14:33, schrieb Michael J Gruber:\n>> .... with this loop, sorry:\n>>\n>> for X in true false; do\n>> for Y in false true; do\n>> ($X && $Y || exit 1)\n>> done\n>> echo \"$X/last inner $Y: $?\"\n>> done\n>>\n>> gives\n>>\n>> true/last inner true: 0\n>> false/last inner true: 1\n>>\n>> even though on both cases we have at least one failure of Y. (failure of\n>> one subtest = failure of the test)\n> \n> Place the loop(s) inside the subshell, and you observe termination on the\n> first failure, and a failure exit code of the subshell.\n> \n> The change proposed in this patch should not be necessary. Something else\n> must be wrong with your tests.\n\nI know I started this (or Jeff did, who knows ;) ), but we keep\nconfusing each other more and more:\n\n> Ah, here it is:\n> \n> @@ -58,7 +58,7 @@ test_expect_success GPG 'show signatures' '\n> \t(\n> \t\tfor commit in merge^2 fourth-unsigned sixth-unsigned seventh-unsigned\n> \t\tdo\n> -\t\t\tgit show --pretty=short --show-signature $commit >actual &&\n> +\t\t\tgit show --pretty=short --show-signature $commit >actual || exit 1\n> \t\t\tgrep \"Good signature from\" actual && exit 1\n\nThis is as in the original, it tests invalid signatures, so \"Good\nsignature\" should not be in the response.\n\n> \t\t\t! grep \"BAD signature from\" actual || exit 1\n> \t\t\techo $commit OK\n> \n> Notice the '&& exit 1'! It should be '|| exit 1', right?\n> \n> -- Hannes\n\nIn other words, the original tests already had\n\ngrep foo && exit 1\n! grep bar || exit 1\n\nto test that we have neither foo nor bar. The reason is (supposedly) to\nhave this portion of the test mostly analogous to the previous one,\nwhere we want foo and do want bar.\n\nSo this is completely unrelated.\n\nOtoh, it seems the original test could have had\n\na &&\nb &&\nc || exit 1\n\nor\n\na || exit 1\nb || exit 1\nc || exit 1\n\nrather than\n\na &&\nb || exit 1\nc || exit 1\n\nwhich I thought was incorrect (but I can't recreate the proof right\nnow). I'd say both of the former versions are preferable to the last one\nunless there is a difference that neither Jeff nor I see.\n\nI need a break before looking at this again ;)\n\nMichael\n"},{"id":"244132","messageId":"539AFAF3.9040707@viscovery.net","threadId":"36856","inReplyTo":"539AF748.3000901@drmicha.warpmail.net","subject":"Re: [PATCHv2 4/6] t7510: exit for loop with test result","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2014-06-13T13:21:55Z","receivedAt":"2014-06-13T13:21:55Z","isPatch":false,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 6/13/2014 15:06, schrieb Michael J Gruber:\n> Johannes Sixt venit, vidit, dixit 13.06.2014 14:54:\n>> Am 6/13/2014 14:33, schrieb Michael J Gruber:\n>>> .... with this loop, sorry:\n>>>\n>>> for X in true false; do\n>>> for Y in false true; do\n>>> ($X && $Y || exit 1)\n>>> done\n>>> echo \"$X/last inner $Y: $?\"\n>>> done\n>>>\n>>> gives\n>>>\n>>> true/last inner true: 0\n>>> false/last inner true: 1\n>>>\n>>> even though on both cases we have at least one failure of Y. (failure of\n>>> one subtest = failure of the test)\n>>\n>> Place the loop(s) inside the subshell, and you observe termination on the\n>> first failure, and a failure exit code of the subshell.\n>>\n>> The change proposed in this patch should not be necessary. Something else\n>> must be wrong with your tests.\n> \n> I know I started this (or Jeff did, who knows ;) ), but we keep\n> confusing each other more and more:\n> \n>> Ah, here it is:\n>>\n>> @@ -58,7 +58,7 @@ test_expect_success GPG 'show signatures' '\n>> \t(\n>> \t\tfor commit in merge^2 fourth-unsigned sixth-unsigned seventh-unsigned\n>> \t\tdo\n>> -\t\t\tgit show --pretty=short --show-signature $commit >actual &&\n>> +\t\t\tgit show --pretty=short --show-signature $commit >actual || exit 1\n>> \t\t\tgrep \"Good signature from\" actual && exit 1\n> \n> This is as in the original, it tests invalid signatures, so \"Good\n> signature\" should not be in the response.\n> \n>> \t\t\t! grep \"BAD signature from\" actual || exit 1\n>> \t\t\techo $commit OK\n>>\n>> Notice the '&& exit 1'! It should be '|| exit 1', right?\n>>\n>> -- Hannes\n> \n> In other words, the original tests already had\n> \n> grep foo && exit 1\n> ! grep bar || exit 1\n> \n> to test that we have neither foo nor bar. The reason is (supposedly) to\n> have this portion of the test mostly analogous to the previous one,\n> where we want foo and do want bar.\n> \n> So this is completely unrelated.\n\nI don't think so. What is the outcome of\n\n\tfalse && # simulate a regression\n\tgrep foo && exit 1\n\t! grep bar || exit 1\n\nassuming that the '! grep bar' happens to be true? Answer: The regression\nis not diagnosed because the &&-chain is broken.\n\n*That* is what I think you described earlier in this thread as \"I put\nsomething failing on the first line of the original version, and the test\nsucceeded.\"\n\n-- Hannes\n"},{"id":"244133","messageId":"20140613133028.GB18715@sigill.intra.peff.net","threadId":"36856","inReplyTo":"539AFAF3.9040707@viscovery.net","subject":"Re: [PATCHv2 4/6] t7510: exit for loop with test result","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-06-13T13:30:29Z","receivedAt":"2014-06-13T13:30:29Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jun 13, 2014 at 03:21:55PM +0200, Johannes Sixt wrote:\n\n> I don't think so. What is the outcome of\n> \n> \tfalse && # simulate a regression\n> \tgrep foo && exit 1\n> \t! grep bar || exit 1\n> \n> assuming that the '! grep bar' happens to be true? Answer: The regression\n> is not diagnosed because the &&-chain is broken.\n> \n> *That* is what I think you described earlier in this thread as \"I put\n> something failing on the first line of the original version, and the test\n> succeeded.\"\n\nYeah, I think that is the bit that I was missing from my original\nconfusion.\n\n false &&\n anything || exit 1\n\n_does_ work. But that is not what is written. ;)\n\nThanks for pointing it out.\n\n-Peff\n"},{"id":"244134","messageId":"539AFD3D.8090407@drmicha.warpmail.net","threadId":"36856","inReplyTo":"539AFAF3.9040707@viscovery.net","subject":"Re: [PATCHv2 4/6] t7510: exit for loop with test result","fromName":"Michael J Gruber","fromEmail":"git@drmicha.warpmail.net","sentAt":"2014-06-13T13:31:41Z","receivedAt":"2014-06-13T13:31:41Z","isPatch":false,"sender":{"key":"git@grubix.eu","avatar":"https://avatars.githubusercontent.com/u/233215?v=4"},"body":"Johannes Sixt venit, vidit, dixit 13.06.2014 15:21:\n> Am 6/13/2014 15:06, schrieb Michael J Gruber:\n>> Johannes Sixt venit, vidit, dixit 13.06.2014 14:54:\n>>> Am 6/13/2014 14:33, schrieb Michael J Gruber:\n>>>> .... with this loop, sorry:\n>>>>\n>>>> for X in true false; do\n>>>> for Y in false true; do\n>>>> ($X && $Y || exit 1)\n>>>> done\n>>>> echo \"$X/last inner $Y: $?\"\n>>>> done\n>>>>\n>>>> gives\n>>>>\n>>>> true/last inner true: 0\n>>>> false/last inner true: 1\n>>>>\n>>>> even though on both cases we have at least one failure of Y. (failure of\n>>>> one subtest = failure of the test)\n>>>\n>>> Place the loop(s) inside the subshell, and you observe termination on the\n>>> first failure, and a failure exit code of the subshell.\n>>>\n>>> The change proposed in this patch should not be necessary. Something else\n>>> must be wrong with your tests.\n>>\n>> I know I started this (or Jeff did, who knows ;) ), but we keep\n>> confusing each other more and more:\n>>\n>>> Ah, here it is:\n>>>\n>>> @@ -58,7 +58,7 @@ test_expect_success GPG 'show signatures' '\n>>> \t(\n>>> \t\tfor commit in merge^2 fourth-unsigned sixth-unsigned seventh-unsigned\n>>> \t\tdo\n>>> -\t\t\tgit show --pretty=short --show-signature $commit >actual &&\n>>> +\t\t\tgit show --pretty=short --show-signature $commit >actual || exit 1\n>>> \t\t\tgrep \"Good signature from\" actual && exit 1\n>>\n>> This is as in the original, it tests invalid signatures, so \"Good\n>> signature\" should not be in the response.\n>>\n>>> \t\t\t! grep \"BAD signature from\" actual || exit 1\n>>> \t\t\techo $commit OK\n>>>\n>>> Notice the '&& exit 1'! It should be '|| exit 1', right?\n>>>\n>>> -- Hannes\n>>\n>> In other words, the original tests already had\n>>\n>> grep foo && exit 1\n>> ! grep bar || exit 1\n>>\n>> to test that we have neither foo nor bar. The reason is (supposedly) to\n>> have this portion of the test mostly analogous to the previous one,\n>> where we want foo and do want bar.\n>>\n>> So this is completely unrelated.\n> \n> I don't think so. What is the outcome of\n> \n> \tfalse && # simulate a regression\n> \tgrep foo && exit 1\n> \t! grep bar || exit 1\n> \n> assuming that the '! grep bar' happens to be true? Answer: The regression\n> is not diagnosed because the &&-chain is broken.\n> \n> *That* is what I think you described earlier in this thread as \"I put\n> something failing on the first line of the original version, and the test\n> succeeded.\"\n> \n> -- Hannes\n\nIf you say that something I have said makes sense I'm happy, because I\ncan't confirm that myself right now. I'll take a break and look into a\nrewrite of the form\n\na &&\nb &&\ntest_must_fail c &&\nd || exit 1\n\nhoping that will make things both readable (by avoiding !) and concise\n(by avoiding repeated exits).\n\nMichael\n"},{"id":"244136","messageId":"539AFFD7.6010908@viscovery.net","threadId":"36856","inReplyTo":"539AFD3D.8090407@drmicha.warpmail.net","subject":"Re: [PATCHv2 4/6] t7510: exit for loop with test result","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2014-06-13T13:42:47Z","receivedAt":"2014-06-13T13:42:47Z","isPatch":false,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 6/13/2014 15:31, schrieb Michael J Gruber:\n> rewrite of the form\n> \n> a &&\n> b &&\n> test_must_fail c &&\n> d || exit 1\n> \n> hoping that will make things both readable (by avoiding !) and concise\n> (by avoiding repeated exits).\n\nThanks!\n\nPlease note that we use 'test_must_fail' only for git invocations, but we\ndo write '!' in front of system commands that we expect to fail, e.g.,\n'! grep'.\n\n-- Hannes\n"},{"id":"244150","messageId":"xmqqbntw4u4d.fsf@gitster.dls.corp.google.com","threadId":"36856","inReplyTo":"20140613110901.GB14066@sigill.intra.peff.net","subject":"Re: [PATCH 3/3] verify-commit: scriptable commit signature verification","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-06-13T17:06:10Z","receivedAt":"2014-06-13T17:06:10Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> I realize this isn't really your itch to scratch. It's just that when I\n> see a description like \"verify a commit\", I wonder what exactly \"verify\"\n> means.\n\nI think that is an important point. If a tool only verifies the\nsignature of the commit when conceivably other aspect of it could\nalso be verified but we cannot decide how or we decide we should not\ndictate one-way-fits-all, using a generic name \"verify-commit\" or\n\"verify\" without marking that it is currently only on the signature\nclearly somewhere might close the door to the future.\n\n git verify ::\n Verify whatever we currently deem is appropriate for the\n given type of object.\n\n git verify --gpg-signature::\n\tVerify the GPG signature for a signed tag, a signed commit,\n or a merge with signed tags.\n\n git verify --commit-author ::\n\tVerify the GPG signer matches the \"author \" header of the\n\tcommit.\n\nand more, perhaps?\n"},{"id":"244161","messageId":"xmqqha3o3cau.fsf@gitster.dls.corp.google.com","threadId":"36856","inReplyTo":"539AEB41.6090306@drmicha.warpmail.net","subject":"Re: [PATCHv2 5/6] t7510: test verify-commit","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-06-13T18:16:25Z","receivedAt":"2014-06-13T18:16:25Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael J Gruber writes:\n\n> Jeff King venit, vidit, dixit 13.06.2014 13:51:\n>> On Fri, Jun 13, 2014 at 12:42:47PM +0200, Michael J Gruber wrote:\n>> \n>>> test_expect_success GPG 'detect fudged signature' '\n>>> \tgit cat-file commit master >raw &&\n>>> \n>>> \tsed -e \"s/seventh/7th forged/\" raw >forged1 &&\n>>> \tgit hash-object -w -t commit forged1 >forged1.commit &&\n>>> +\t! git verify-commit $(cat forged1.commit) &&\n>> \n>> Please use test_must_fail here (and further down), which will catch\n>> things like signal death.\n>\n> Again, that is an issue of keeping the style of the surrounding code\n> (which is relatively new) vs. doing it differently. I don't mind\n> changing t7510 to a different style, though.\n\nWe do not have any \"! git anything\" in that script, don't we? We do\nnot expect \"grep\" supplied by the platform to lie, and that is why\nwe should never use test_must_fail on them, but we do want to notice\nwhen git starts segfaulting, and that is what test_must_fail is for.\n\nWe tell novices and cluelesses who do not know better to try to\nfollow and match the surrounding style. This is because it would at\nleast not make things worse than if we let them run loose on our\ncodebase. It is not about \"if we have a bad style, it is better to\nmatch that existing bad style to spread the badness than making it\npartly better\". But you have been here long enough and know what is\npreferred and and what is to be avoided---more importantly, Peff has\nbeen here long enough to know and has given such an advice, so...\n\nIn any case, I do not offhand see anything wrong style-wise in 7510,\nso please do not change anything in it for the sake of styles.\n"},{"id":"244163","messageId":"xmqqd2ec3byy.fsf@gitster.dls.corp.google.com","threadId":"36856","inReplyTo":"20140613114615.GE14066@sigill.intra.peff.net","subject":"Re: [PATCHv2 4/6] t7510: exit for loop with test result","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-06-13T18:23:33Z","receivedAt":"2014-06-13T18:23:33Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n>> \t(\n>> \t\tfor commit in initial second merge fourth-signed fifth-signed sixth-signed master\n>> \t\tdo\n>> -\t\t\tgit show --pretty=short --show-signature $commit >actual &&\n>> +\t\t\tgit show --pretty=short --show-signature $commit >actual || exit 1\n>> \t\t\tgrep \"Good signature from\" actual || exit 1\n>\n> Hrm. The original is:\n>\n> X &&\n> Y || exit 1\n>\n> Won't that still exit (i.e., it is already correct)? Doing:\n>\n> for X in true false; do\n> for Y in true false; do\n> ($X && $Y || exit 1)\n> echo \"$X/$Y: $?\"\n> done\n> done\n>\n> yields:\n>\n> true/true: 0\n> true/false: 1\n> false/true: 1\n> false/false: 1\n>\n> (and should still short-circuit Y, because we go from left-to-right).\n>\n> I do not mind changing it to keep the style of each line consistent,\n> though. I would have written it as a series of \"&&\"-chains, with a\n> single exit at the end, but I think that is just a matter of preference.\n\nYeah, series of && chain with a single exit at the end is good, and\nthe subshell is there only to allow us to do that \"exit at the end\".\n"},{"id":"244243","messageId":"539EB722.8@drmicha.warpmail.net","threadId":"36856","inReplyTo":"xmqqbntw4u4d.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 3/3] verify-commit: scriptable commit signature verification","fromName":"Michael J Gruber","fromEmail":"git@drmicha.warpmail.net","sentAt":"2014-06-16T09:21:38Z","receivedAt":"2014-06-16T09:21:38Z","isPatch":true,"sender":{"key":"git@grubix.eu","avatar":"https://avatars.githubusercontent.com/u/233215?v=4"},"body":"Junio C Hamano venit, vidit, dixit 13.06.2014 19:06:\n> Jeff King writes:\n> \n>> I realize this isn't really your itch to scratch. It's just that when I\n>> see a description like \"verify a commit\", I wonder what exactly \"verify\"\n>> means.\n> \n> I think that is an important point. If a tool only verifies the\n> signature of the commit when conceivably other aspect of it could\n> also be verified but we cannot decide how or we decide we should not\n> dictate one-way-fits-all, using a generic name \"verify-commit\" or\n> \"verify\" without marking that it is currently only on the signature\n> clearly somewhere might close the door to the future.\n> \n> git verify ::\n> Verify whatever we currently deem is appropriate for the\n> given type of object.\n> \n> git verify --gpg-signature::\n> \tVerify the GPG signature for a signed tag, a signed commit,\n> or a merge with signed tags.\n> \n> git verify --commit-author ::\n> \tVerify the GPG signer matches the \"author \" header of the\n> \tcommit.\n> \n> and more, perhaps?\n> \n\nSo what does that mean? And what does it mean for verify-tag, which does\nnothing but checking the return code from gpg, just like the proposed\nverify-commit?\n\nAs pointed out, strict verification is a matter of policy, very much\nlike accepting certain ref updates etc. is. Do we want a signature\nverification hook?\n\nWe currently don't have a scriptable commit signature verification in\nthe same way we have one for tag signatures. That's the gap that I\nwanted to fill in in response to a blog post about commit signatures in\ngit. But it's not my itch, I don't use signatures.\n\nMichael\n"},{"id":"244343","messageId":"20140616195428.GB24376@sigill.intra.peff.net","threadId":"36856","inReplyTo":"xmqqbntw4u4d.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 3/3] verify-commit: scriptable commit signature verification","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-06-16T19:54:29Z","receivedAt":"2014-06-16T19:54:29Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jun 13, 2014 at 10:06:10AM -0700, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > I realize this isn't really your itch to scratch. It's just that when I\n> > see a description like \"verify a commit\", I wonder what exactly \"verify\"\n> > means.\n> \n> I think that is an important point. If a tool only verifies the\n> signature of the commit when conceivably other aspect of it could\n> also be verified but we cannot decide how or we decide we should not\n> dictate one-way-fits-all, using a generic name \"verify-commit\" or\n> \"verify\" without marking that it is currently only on the signature\n> clearly somewhere might close the door to the future.\n> \n> git verify ::\n> Verify whatever we currently deem is appropriate for the\n> given type of object.\n> \n> git verify --gpg-signature::\n> \tVerify the GPG signature for a signed tag, a signed commit,\n> or a merge with signed tags.\n> \n> git verify --commit-author ::\n> \tVerify the GPG signer matches the \"author \" header of the\n> \tcommit.\n> \n> and more, perhaps?\n\nThat is certainly the direction I was thinking of when I suggested \"git\nverify\".\n\nHowever, I do not think it is too bad a thing to add a verify-commit\nthat matches verify-tag, as long as they do the exact same thing\n(namely, check the gpg signature). We may find it is later obsoleted by\n\"git verify --gpg-signature\", but given that verify-tag is already there\nand will remain for compatibility, I don't think we are increasing the\ncognitive load too much.\n\nYour middle example above did make me think of one other thing, though.\nAs you noted, we actually have _three_ signature types:\n\n 1. signed tags\n\n 2. signed commits\n\n 3. merges with embedded mergetag headers\n\nWe already have a tool for (1). Michael is adding a tool for (2). How\nwould one check (3) in a similar way?\n\n-Peff\n"},{"id":"244355","messageId":"xmqqk38gpp9v.fsf@gitster.dls.corp.google.com","threadId":"36856","inReplyTo":"20140616195428.GB24376@sigill.intra.peff.net","subject":"Re: [PATCH 3/3] verify-commit: scriptable commit signature verification","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-06-16T20:34:20Z","receivedAt":"2014-06-16T20:34:20Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Fri, Jun 13, 2014 at 10:06:10AM -0700, Junio C Hamano wrote:\n> ...\n>> and more, perhaps?\n>\n> That is certainly the direction I was thinking of when I suggested \"git\n> verify\".\n>\n> However, I do not think it is too bad a thing to add a verify-commit\n> that matches verify-tag, as long as they do the exact same thing\n> (namely, check the gpg signature). We may find it is later obsoleted by\n> \"git verify --gpg-signature\", but given that verify-tag is already there\n> and will remain for compatibility, I don't think we are increasing the\n> cognitive load too much.\n\nYup, I think we are exactly on the same page.\n\nThanks for sanity-checking.\n\n> Your middle example above did make me think of one other thing, though.\n> As you noted, we actually have _three_ signature types:\n>\n> 1. signed tags\n>\n> 2. signed commits\n>\n> 3. merges with embedded mergetag headers\n>\n> We already have a tool for (1). Michael is adding a tool for (2). How\n> would one check (3) in a similar way?\n\nHmph, somehow I misread the patch that it was for both 2 & 3 X-<.\n"},{"id":"244356","messageId":"20140616203956.GA3546@sigill.intra.peff.net","threadId":"36856","inReplyTo":"xmqqk38gpp9v.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 3/3] verify-commit: scriptable commit signature verification","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-06-16T20:39:56Z","receivedAt":"2014-06-16T20:39:56Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Jun 16, 2014 at 01:34:20PM -0700, Junio C Hamano wrote:\n\n> > Your middle example above did make me think of one other thing, though.\n> > As you noted, we actually have _three_ signature types:\n> >\n> > 1. signed tags\n> >\n> > 2. signed commits\n> >\n> > 3. merges with embedded mergetag headers\n> >\n> > We already have a tool for (1). Michael is adding a tool for (2). How\n> > would one check (3) in a similar way?\n> \n> Hmph, somehow I misread the patch that it was for both 2 & 3 X-<.\n\nI was just assuming it handles only (2) without checking further, so I\nmay be wrong. But I do not think it makes sense to conflate (2) and (3).\nA merge commit may have both, and they are separate signatures.\n\nFor that matter, is there a way to expose (3) currently, besides via\n--show-signature? It does not trigger \"%GG\" and friends (nor should it).\nIt may make sense to add extra format specifiers for mergetag\nsignatures. Though I do not use them myself, so I am not clear on what\nthe use case is besides a manual, human verification of a particular\nmerge.\n\n-Peff\n"},{"id":"244844","messageId":"cover.1403506792.git.git@drmicha.warpmail.net","threadId":"36856","inReplyTo":"cover.1402655838.git.git@drmicha.warpmail.net","subject":"[PATCHv3 0/5] verify-commit: verify commit signatures","fromName":"Michael J Gruber","fromEmail":"git@drmicha.warpmail.net","sentAt":"2014-06-23T07:05:46Z","receivedAt":"2014-06-23T07:05:46Z","isPatch":false,"sender":{"key":"git@grubix.eu","avatar":"https://avatars.githubusercontent.com/u/233215?v=4"},"body":"This incorporates all remarks about the test coding guidelines and\nrearranging the series.\n\nOpen questions:\n- There was some debate about (optionally) verifying more than what\ngit-verify-{commit,tag} currently do, or going for a generic git-verify command.\nThe former would require both to be changed (in order to treat similar cases similarly),\nthe latter would need a deprecation for git-verify-tag.\n\n- I haven't looked yet at what happened over the weekend.\n\nMichael J Gruber (5):\n gpg-interface: provide clear helper for struct signature_check\n gpg-interface: provide access to the payload\n verify-commit: scriptable commit signature verification\n t7510: exit for loop with test result\n t7510: test verify-commit\n\n Documentation/git-verify-commit.txt | 28 +++++++++++\n Makefile | 1 +\n builtin.h | 1 +\n builtin/merge.c | 5 +-\n builtin/verify-commit.c | 93 +++++++++++++++++++++++++++++++++++++\n command-list.txt | 1 +\n commit.c | 1 +\n git.c | 1 +\n gpg-interface.c | 14 ++++++\n gpg-interface.h | 2 +\n pretty.c | 3 +-\n t/t7510-signed-commit.sh | 24 ++++++++--\n 12 files changed, 165 insertions(+), 9 deletions(-)\n create mode 100644 Documentation/git-verify-commit.txt\n create mode 100644 builtin/verify-commit.c\n\n-- \n2.0.0.426.g37dbf84\n"},{"id":"244845","messageId":"872a2ba85cc1d9e6c3be1cd4a882dbf08b7c656d.1403506792.git.git@drmicha.warpmail.net","threadId":"36856","inReplyTo":"cover.1402655838.git.git@drmicha.warpmail.net","subject":"[PATCHv3 1/5] gpg-interface: provide clear helper for struct signature_check","fromName":"Michael J Gruber","fromEmail":"git@drmicha.warpmail.net","sentAt":"2014-06-23T07:05:47Z","receivedAt":"2014-06-23T07:05:47Z","isPatch":false,"sender":{"key":"git@grubix.eu","avatar":"https://avatars.githubusercontent.com/u/233215?v=4"},"body":"The struct has been growing members whose malloced memory needs to be\nfreed. Do this with one helper function so that no malloced memory shall\nbe left unfreed.\n\nSigned-off-by: Michael J Gruber \n---\n builtin/merge.c | 5 +----\n gpg-interface.c | 12 ++++++++++++\n gpg-interface.h | 1 +\n pretty.c | 3 +--\n 4 files changed, 15 insertions(+), 6 deletions(-)\n\ndiff --git a/builtin/merge.c b/builtin/merge.c\nindex 428ca24..e50323d 100644\n--- a/builtin/merge.c\n+++ b/builtin/merge.c\n@@ -1282,10 +1282,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix)\n \t\t\t\tprintf(_(\"Commit %s has a good GPG signature by %s\\n\"),\n \t\t\t\t hex, signature_check.signer);\n \n-\t\t\tfree(signature_check.gpg_output);\n-\t\t\tfree(signature_check.gpg_status);\n-\t\t\tfree(signature_check.signer);\n-\t\t\tfree(signature_check.key);\n+\t\t\tsignature_check_clear(&signature_check);\n \t\t}\n \t}\n \ndiff --git a/gpg-interface.c b/gpg-interface.c\nindex 8b0e874..e71b59d 100644\n--- a/gpg-interface.c\n+++ b/gpg-interface.c\n@@ -7,6 +7,18 @@\n static char *configured_signing_key;\n static const char *gpg_program = \"gpg\";\n \n+void signature_check_clear(struct signature_check *sigc)\n+{\n+\tfree(sigc->gpg_output);\n+\tfree(sigc->gpg_status);\n+\tfree(sigc->signer);\n+\tfree(sigc->key);\n+\tsigc->gpg_output = NULL;\n+\tsigc->gpg_status = NULL;\n+\tsigc->signer = NULL;\n+\tsigc->key = NULL;\n+}\n+\n void set_signing_key(const char *key)\n {\n \tfree(configured_signing_key);\ndiff --git a/gpg-interface.h b/gpg-interface.h\nindex a85cb5b..9f0784a 100644\n--- a/gpg-interface.h\n+++ b/gpg-interface.h\n@@ -13,6 +13,7 @@ struct signature_check {\n \tchar *key;\n };\n \n+extern void signature_check_clear(struct signature_check *sigc);\n extern int sign_buffer(struct strbuf *buffer, struct strbuf *signature, const char *signing_key);\n extern int verify_signed_buffer(const char *payload, size_t payload_size, const char *signature, size_t signature_size, struct strbuf *gpg_output, struct strbuf *gpg_status);\n extern int git_gpg_config(const char *, const char *, void *);\ndiff --git a/pretty.c b/pretty.c\nindex 4f51287..ac901b8 100644\n--- a/pretty.c\n+++ b/pretty.c\n@@ -1537,8 +1537,7 @@ void format_commit_message(const struct commit *commit,\n \n \tfree(context.commit_encoding);\n \tlogmsg_free(context.message, commit);\n-\tfree(context.signature_check.gpg_output);\n-\tfree(context.signature_check.signer);\n+\tsignature_check_clear(&context.signature_check);\n }\n \n static void pp_header(struct pretty_print_context *pp,\n-- \n2.0.0.426.g37dbf84\n"},{"id":"244848","messageId":"e021b30801f8e917c171643334feb66a2baf61b7.1403506792.git.git@drmicha.warpmail.net","threadId":"36856","inReplyTo":"cover.1402655838.git.git@drmicha.warpmail.net","subject":"[PATCHv3 2/5] gpg-interface: provide access to the payload","fromName":"Michael J Gruber","fromEmail":"git@drmicha.warpmail.net","sentAt":"2014-06-23T07:05:48Z","receivedAt":"2014-06-23T07:05:48Z","isPatch":false,"sender":{"key":"git@grubix.eu","avatar":"https://avatars.githubusercontent.com/u/233215?v=4"},"body":"In contrast to tag signatures, commit signatures are put into the\nheader, that is between the other header parts and commit messages.\n\nProvide access to the commit content sans the signature, which is the\npayload that is actually signed. Commit signature verification does the\nparsing anyways, and callers may wish to act on or display the commit\nobject sans the signature.\n\nSigned-off-by: Michael J Gruber \n---\n commit.c | 1 +\n gpg-interface.c | 2 ++\n gpg-interface.h | 1 +\n 3 files changed, 4 insertions(+)\n\ndiff --git a/commit.c b/commit.c\nindex 881be3b..d0ad7f0 100644\n--- a/commit.c\n+++ b/commit.c\n@@ -1219,6 +1219,7 @@ void check_commit_signature(const struct commit* commit, struct signature_check\n \t\t\t\t &gpg_output, &gpg_status);\n \tif (status && !gpg_output.len)\n \t\tgoto out;\n+\tsigc->payload = strbuf_detach(&payload, NULL);\n \tsigc->gpg_output = strbuf_detach(&gpg_output, NULL);\n \tsigc->gpg_status = strbuf_detach(&gpg_status, NULL);\n \tparse_gpg_output(sigc);\ndiff --git a/gpg-interface.c b/gpg-interface.c\nindex e71b59d..ff07012 100644\n--- a/gpg-interface.c\n+++ b/gpg-interface.c\n@@ -9,10 +9,12 @@ static const char *gpg_program = \"gpg\";\n \n void signature_check_clear(struct signature_check *sigc)\n {\n+\tfree(sigc->payload);\n \tfree(sigc->gpg_output);\n \tfree(sigc->gpg_status);\n \tfree(sigc->signer);\n \tfree(sigc->key);\n+\tsigc->payload = NULL;\n \tsigc->gpg_output = NULL;\n \tsigc->gpg_status = NULL;\n \tsigc->signer = NULL;\ndiff --git a/gpg-interface.h b/gpg-interface.h\nindex 9f0784a..37c23da 100644\n--- a/gpg-interface.h\n+++ b/gpg-interface.h\n@@ -2,6 +2,7 @@\n #define GPG_INTERFACE_H\n \n struct signature_check {\n+\tchar *payload;\n \tchar *gpg_output;\n \tchar *gpg_status;\n \tchar result; /* 0 (not checked),\n-- \n2.0.0.426.g37dbf84\n"},{"id":"244849","messageId":"f344d5a5b1d336e1d7a3c4c0f225a3f3fc62beac.1403506792.git.git@drmicha.warpmail.net","threadId":"36856","inReplyTo":"cover.1402655838.git.git@drmicha.warpmail.net","subject":"[PATCHv3 3/5] verify-commit: scriptable commit signature verification","fromName":"Michael J Gruber","fromEmail":"git@drmicha.warpmail.net","sentAt":"2014-06-23T07:05:49Z","receivedAt":"2014-06-23T07:05:49Z","isPatch":false,"sender":{"key":"git@grubix.eu","avatar":"https://avatars.githubusercontent.com/u/233215?v=4"},"body":"Commit signatures can be verified using \"git show -s --show-signature\"\nor the \"%G?\" pretty format and parsing the output, which is well suited\nfor user inspection, but not for scripting.\n\nProvide a command \"verify-commit\" which is analogous to \"verify-tag\": It\nreturns 0 for good signatures and non-zero otherwise, has the gpg output\non stderr and (optionally) the commit object on stdout, sans the\nsignature, just like \"verify-tag\" does.\n\nSigned-off-by: Michael J Gruber \n---\n Documentation/git-verify-commit.txt | 28 +++++++++++\n Makefile | 1 +\n builtin.h | 1 +\n builtin/verify-commit.c | 93 +++++++++++++++++++++++++++++++++++++\n command-list.txt | 1 +\n git.c | 1 +\n 6 files changed, 125 insertions(+)\n create mode 100644 Documentation/git-verify-commit.txt\n create mode 100644 builtin/verify-commit.c\n\ndiff --git a/Documentation/git-verify-commit.txt b/Documentation/git-verify-commit.txt\nnew file mode 100644\nindex 0000000..9413e28\n--- /dev/null\n+++ b/Documentation/git-verify-commit.txt\n@@ -0,0 +1,28 @@\n+git-verify-commit(1)\n+====================\n+\n+NAME\n+----\n+git-verify-commit - Check the GPG signature of commits\n+\n+SYNOPSIS\n+--------\n+[verse]\n+'git verify-commit' ...\n+\n+DESCRIPTION\n+-----------\n+Validates the gpg signature created by 'git commit -S'.\n+\n+OPTIONS\n+-------\n+-v::\n+--verbose::\n+\tPrint the contents of the commit object before validating it.\n+\n+...::\n+\tSHA-1 identifiers of Git commit objects.\n+\n+GIT\n+---\n+Part of the linkgit:git[1] suite\ndiff --git a/Makefile b/Makefile\nindex 07ea105..b92418d 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -999,6 +999,7 @@ BUILTIN_OBJS += builtin/update-ref.o\n BUILTIN_OBJS += builtin/update-server-info.o\n BUILTIN_OBJS += builtin/upload-archive.o\n BUILTIN_OBJS += builtin/var.o\n+BUILTIN_OBJS += builtin/verify-commit.o\n BUILTIN_OBJS += builtin/verify-pack.o\n BUILTIN_OBJS += builtin/verify-tag.o\n BUILTIN_OBJS += builtin/write-tree.o\ndiff --git a/builtin.h b/builtin.h\nindex c47c110..5d91f31 100644\n--- a/builtin.h\n+++ b/builtin.h\n@@ -128,6 +128,7 @@ extern int cmd_update_server_info(int argc, const char **argv, const char *prefi\n extern int cmd_upload_archive(int argc, const char **argv, const char *prefix);\n extern int cmd_upload_archive_writer(int argc, const char **argv, const char *prefix);\n extern int cmd_var(int argc, const char **argv, const char *prefix);\n+extern int cmd_verify_commit(int argc, const char **argv, const char *prefix);\n extern int cmd_verify_tag(int argc, const char **argv, const char *prefix);\n extern int cmd_version(int argc, const char **argv, const char *prefix);\n extern int cmd_whatchanged(int argc, const char **argv, const char *prefix);\ndiff --git a/builtin/verify-commit.c b/builtin/verify-commit.c\nnew file mode 100644\nindex 0000000..b0f8504\n--- /dev/null\n+++ b/builtin/verify-commit.c\n@@ -0,0 +1,93 @@\n+/*\n+ * Builtin \"git commit-commit\"\n+ *\n+ * Copyright (c) 2014 Michael J Gruber \n+ *\n+ * Based on git-verify-tag\n+ */\n+#include \"cache.h\"\n+#include \"builtin.h\"\n+#include \"commit.h\"\n+#include \"run-command.h\"\n+#include \n+#include \"parse-options.h\"\n+#include \"gpg-interface.h\"\n+\n+static const char * const verify_commit_usage[] = {\n+\t\tN_(\"git verify-commit [-v|--verbose] ...\"),\n+\t\tNULL\n+};\n+\n+static int run_gpg_verify(const unsigned char *sha1, const char *buf, unsigned long size, int verbose)\n+{\n+\tstruct signature_check signature_check;\n+\n+\tmemset(&signature_check, 0, sizeof(signature_check));\n+\n+\tcheck_commit_signature(lookup_commit(sha1), &signature_check);\n+\n+\tif (verbose && signature_check.payload)\n+\t\tfputs(signature_check.payload, stdout);\n+\n+\tif (signature_check.gpg_output)\n+\t\tfputs(signature_check.gpg_output, stderr);\n+\n+\tsignature_check_clear(&signature_check);\n+\treturn signature_check.result != 'G';\n+}\n+\n+static int verify_commit(const char *name, int verbose)\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(\"commit '%s' not found.\", name);\n+\n+\tbuf = read_sha1_file(sha1, &type, &size);\n+\tif (!buf)\n+\t\treturn error(\"%s: unable to read file.\", name);\n+\tif (type != OBJ_COMMIT)\n+\t\treturn error(\"%s: cannot verify a non-commit object of type %s.\",\n+\t\t\t\tname, typename(type));\n+\n+\tret = run_gpg_verify(sha1, buf, size, verbose);\n+\n+\tfree(buf);\n+\treturn ret;\n+}\n+\n+static int git_verify_commit_config(const char *var, const char *value, void *cb)\n+{\n+\tint status = git_gpg_config(var, value, cb);\n+\tif (status)\n+\t\treturn status;\n+\treturn git_default_config(var, value, cb);\n+}\n+\n+int cmd_verify_commit(int argc, const char **argv, const char *prefix)\n+{\n+\tint i = 1, verbose = 0, had_error = 0;\n+\tconst struct option verify_commit_options[] = {\n+\t\tOPT__VERBOSE(&verbose, N_(\"print commit contents\")),\n+\t\tOPT_END()\n+\t};\n+\n+\tgit_config(git_verify_commit_config, NULL);\n+\n+\targc = parse_options(argc, argv, prefix, verify_commit_options,\n+\t\t\t verify_commit_usage, PARSE_OPT_KEEP_ARGV0);\n+\tif (argc <= i)\n+\t\tusage_with_options(verify_commit_usage, verify_commit_options);\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_commit(argv[i++], verbose))\n+\t\t\thad_error = 1;\n+\treturn had_error;\n+}\ndiff --git a/command-list.txt b/command-list.txt\nindex cf36c3d..a3ff0c9 100644\n--- a/command-list.txt\n+++ b/command-list.txt\n@@ -132,6 +132,7 @@ git-update-server-info synchingrepositories\n git-upload-archive synchelpers\n git-upload-pack synchelpers\n git-var plumbinginterrogators\n+git-verify-commit ancillaryinterrogators\n git-verify-pack plumbinginterrogators\n git-verify-tag ancillaryinterrogators\n gitweb ancillaryinterrogators\ndiff --git a/git.c b/git.c\nindex 7780572..bbc3293 100644\n--- a/git.c\n+++ b/git.c\n@@ -441,6 +441,7 @@ static struct cmd_struct commands[] = {\n \t{ \"upload-archive\", cmd_upload_archive },\n \t{ \"upload-archive--writer\", cmd_upload_archive_writer },\n \t{ \"var\", cmd_var, RUN_SETUP_GENTLY },\n+\t{ \"verify-commit\", cmd_verify_commit, RUN_SETUP },\n \t{ \"verify-pack\", cmd_verify_pack },\n \t{ \"verify-tag\", cmd_verify_tag, RUN_SETUP },\n \t{ \"version\", cmd_version },\n-- \n2.0.0.426.g37dbf84\n"},{"id":"244846","messageId":"9013cca44cba0235e5fbb2f99f88cc6cbb0e0032.1403506792.git.git@drmicha.warpmail.net","threadId":"36856","inReplyTo":"cover.1402655838.git.git@drmicha.warpmail.net","subject":"[PATCHv3 4/5] t7510: exit for loop with test result","fromName":"Michael J Gruber","fromEmail":"git@drmicha.warpmail.net","sentAt":"2014-06-23T07:05:50Z","receivedAt":"2014-06-23T07:05:50Z","isPatch":false,"sender":{"key":"git@grubix.eu","avatar":"https://avatars.githubusercontent.com/u/233215?v=4"},"body":"t7510 uses for loops in a subshell, which need to make sure that the test\nreturns with the appropriate error code from within the loop.\n\nRestructure the loops as the usual && chains with a single point of\n\"exit 1\" at the end of the loop to make this clearer.\n\nSigned-off-by: Michael J Gruber \n---\n t/t7510-signed-commit.sh | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/t/t7510-signed-commit.sh b/t/t7510-signed-commit.sh\nindex 5ddac1a..96cfddf 100755\n--- a/t/t7510-signed-commit.sh\n+++ b/t/t7510-signed-commit.sh\n@@ -50,7 +50,7 @@ test_expect_success GPG 'show signatures' '\n \t\tfor commit in initial second merge fourth-signed fifth-signed sixth-signed master\n \t\tdo\n \t\t\tgit show --pretty=short --show-signature $commit >actual &&\n-\t\t\tgrep \"Good signature from\" actual || exit 1\n+\t\t\tgrep \"Good signature from\" actual &&\n \t\t\t! grep \"BAD signature from\" actual || exit 1\n \t\t\techo $commit OK\n \t\tdone\n@@ -59,7 +59,7 @@ test_expect_success GPG 'show signatures' '\n \t\tfor commit in merge^2 fourth-unsigned sixth-unsigned seventh-unsigned\n \t\tdo\n \t\t\tgit show --pretty=short --show-signature $commit >actual &&\n-\t\t\tgrep \"Good signature from\" actual && exit 1\n+\t\t\t! grep \"Good signature from\" actual &&\n \t\t\t! grep \"BAD signature from\" actual || exit 1\n \t\t\techo $commit OK\n \t\tdone\n-- \n2.0.0.426.g37dbf84\n"},{"id":"244847","messageId":"340919e5e17a582acaa57faefb362141e64d32a1.1403506792.git.git@drmicha.warpmail.net","threadId":"36856","inReplyTo":"cover.1402655838.git.git@drmicha.warpmail.net","subject":"[PATCHv3 5/5] t7510: test verify-commit","fromName":"Michael J Gruber","fromEmail":"git@drmicha.warpmail.net","sentAt":"2014-06-23T07:05:51Z","receivedAt":"2014-06-23T07:05:51Z","isPatch":false,"sender":{"key":"git@grubix.eu","avatar":"https://avatars.githubusercontent.com/u/233215?v=4"},"body":"This mixes the \"git verify-commit\" tests in with the \"git show\n--show-signature\" tests, to keep the tests more readable.\n\nThe tests already mix in the \"call show\" tests with the \"verify\" tests.\nSo in case of a test beakage, a '-v' run would be needed to reveal the\nexact point of breakage anyway.\n\nAdditionally, test the actual output of \"git verify-commit\" and \"git\nshow --show-signature\" and compare to \"git cat-file\".\n\nSigned-off-by: Michael J Gruber \n---\n t/t7510-signed-commit.sh | 20 +++++++++++++++++++-\n 1 file changed, 19 insertions(+), 1 deletion(-)\n\ndiff --git a/t/t7510-signed-commit.sh b/t/t7510-signed-commit.sh\nindex 96cfddf..dd4b948 100755\n--- a/t/t7510-signed-commit.sh\n+++ b/t/t7510-signed-commit.sh\n@@ -45,10 +45,11 @@ test_expect_success GPG 'create signed commits' '\n \tgit tag seventh-signed\n '\n \n-test_expect_success GPG 'show signatures' '\n+test_expect_success GPG 'verify and show signatures' '\n \t(\n \t\tfor commit in initial second merge fourth-signed fifth-signed sixth-signed master\n \t\tdo\n+\t\t\tgit verify-commit $commit &&\n \t\t\tgit show --pretty=short --show-signature $commit >actual &&\n \t\t\tgrep \"Good signature from\" actual &&\n \t\t\t! grep \"BAD signature from\" actual || exit 1\n@@ -58,6 +59,7 @@ test_expect_success GPG 'show signatures' '\n \t(\n \t\tfor commit in merge^2 fourth-unsigned sixth-unsigned seventh-unsigned\n \t\tdo\n+\t\t\ttest_must_fail git verify-commit $commit &&\n \t\t\tgit show --pretty=short --show-signature $commit >actual &&\n \t\t\t! grep \"Good signature from\" actual &&\n \t\t\t! grep \"BAD signature from\" actual || exit 1\n@@ -66,11 +68,25 @@ test_expect_success GPG 'show signatures' '\n \t)\n '\n \n+test_expect_success GPG 'show signed commit with signature' '\n+\tgit show -s initial >commit &&\n+\tgit show -s --show-signature initial >show &&\n+\tgit verify-commit -v initial >verify.1 2>verify.2 &&\n+\tgit cat-file commit initial >cat &&\n+\tgrep -v \"gpg: \" show >show.commit &&\n+\tgrep \"gpg: \" show >show.gpg &&\n+\tgrep -v \"^ \" cat | grep -v \"^gpgsig \" >cat.commit &&\n+\ttest_cmp show.commit commit &&\n+\ttest_cmp show.gpg verify.2 &&\n+\ttest_cmp cat.commit verify.1\n+'\n+\n test_expect_success GPG 'detect fudged signature' '\n \tgit cat-file commit master >raw &&\n \n \tsed -e \"s/seventh/7th forged/\" raw >forged1 &&\n \tgit hash-object -w -t commit forged1 >forged1.commit &&\n+\t! git verify-commit $(cat forged1.commit) &&\n \tgit show --pretty=short --show-signature $(cat forged1.commit) >actual1 &&\n \tgrep \"BAD signature from\" actual1 &&\n \t! grep \"Good signature from\" actual1\n@@ -81,6 +97,7 @@ test_expect_success GPG 'detect fudged signature with NUL' '\n \tcat raw >forged2 &&\n \techo Qwik | tr \"Q\" \"\\000\" >>forged2 &&\n \tgit hash-object -w -t commit forged2 >forged2.commit &&\n+\t! git verify-commit $(cat forged2.commit) &&\n \tgit show --pretty=short --show-signature $(cat forged2.commit) >actual2 &&\n \tgrep \"BAD signature from\" actual2 &&\n \t! grep \"Good signature from\" actual2\n@@ -89,6 +106,7 @@ test_expect_success GPG 'detect fudged signature with NUL' '\n test_expect_success GPG 'amending already signed commit' '\n \tgit checkout fourth-signed^0 &&\n \tgit commit --amend -S --no-edit &&\n+\tgit verify-commit HEAD &&\n \tgit show -s --show-signature HEAD >actual &&\n \tgrep \"Good signature from\" actual &&\n \t! grep \"BAD signature from\" actual\n-- \n2.0.0.426.g37dbf84\n"},{"id":"244881","messageId":"20140623172805.GD4838@sigill.intra.peff.net","threadId":"36856","inReplyTo":"cover.1403506792.git.git@drmicha.warpmail.net","subject":"Re: [PATCHv3 0/5] verify-commit: verify commit signatures","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-06-23T17:28:05Z","receivedAt":"2014-06-23T17:28:05Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Jun 23, 2014 at 09:05:46AM +0200, Michael J Gruber wrote:\n\n> This incorporates all remarks about the test coding guidelines and\n> rearranging the series.\n> \n> Open questions:\n> - There was some debate about (optionally) verifying more than what\n> git-verify-{commit,tag} currently do, or going for a generic git-verify command.\n> The former would require both to be changed (in order to treat similar cases similarly),\n> the latter would need a deprecation for git-verify-tag.\n\nI think that a potential \"git verify\" doesn't need to block this series,\nper the logic I gave elsewhere.\n\nThe one thing that does give me pause is that we do not seem to have any\nway of accessing mergetag signatures. We should perhaps stop and think\nfor a second about how we might expose those (and whether it would fit\ninto the \"git-verify-{commit,tag}\" paradigm). I am tempted to say that\n\"git verify-tag\" on a commit should verify the mergetag (right now it\nwould simply be an error). But I haven't though that hard on it.\n\nI don't think implementation of that needs to be a blocker for this\nseries, but I'd rather see at least a vague plan so that we do not paint\nourselves into a corner.\n\n> Michael J Gruber (5):\n> gpg-interface: provide clear helper for struct signature_check\n> gpg-interface: provide access to the payload\n> verify-commit: scriptable commit signature verification\n> t7510: exit for loop with test result\n> t7510: test verify-commit\n\nI didn't see anything objectionable here. I think you may want to rebase\non top of jk/pretty-G-format-fixes. That makes your patch 4 redundant,\nand your patch 5 will probably need a few minor updates to match\ncleanups in the surrounding code.\n\n-Peff\n"},{"id":"244887","messageId":"xmqq61jrcy3d.fsf@gitster.dls.corp.google.com","threadId":"36856","inReplyTo":"20140623172805.GD4838@sigill.intra.peff.net","subject":"Re: [PATCHv3 0/5] verify-commit: verify commit signatures","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-06-23T17:52:38Z","receivedAt":"2014-06-23T17:52:38Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Mon, Jun 23, 2014 at 09:05:46AM +0200, Michael J Gruber wrote:\n>\n>> This incorporates all remarks about the test coding guidelines and\n>> rearranging the series.\n>> \n>> Open questions:\n>> - There was some debate about (optionally) verifying more than what\n>> git-verify-{commit,tag} currently do, or going for a generic git-verify command.\n>> The former would require both to be changed (in order to treat similar cases similarly),\n>> the latter would need a deprecation for git-verify-tag.\n>\n> I think that a potential \"git verify\" doesn't need to block this series,\n> per the logic I gave elsewhere.\n>\n> The one thing that does give me pause is that we do not seem to have any\n> way of accessing mergetag signatures. We should perhaps stop and think\n> for a second about how we might expose those (and whether it would fit\n> into the \"git-verify-{commit,tag}\" paradigm). I am tempted to say that\n> \"git verify-tag\" on a commit should verify the mergetag (right now it\n> would simply be an error). But I haven't though that hard on it.\n\nI agree that \"verify-commit\" that lives next to \"verify-tag\" is fine\nand does not have to wait for a unified \"verify\" that may not even\nbe a good idea, but if we were to verify the mergetags in one of\nthese \"verify-$OBJECTTYPE\" commands, I would think \"verify-commit\"\nshould be the one to check them, for the simple reason that they\nappear in \"commit\" objects, not in \"tag\" objects.\n\nI would imagine that I would not mind too much if \"verify-tag\"\ndelegated the verification to \"verify-commit\" automatically when it\nis given a commit object, but for a command fairly low-level to be\nuseful for scripting, such a DWIMmage may be too unexpected and make\nthem unnecessarily unreliable. Using scripts that want strictness\n(and who in the right mind that wants to verify things do not want\nstrictness?) would need to \"cat-file -t\" upfront to switch on the\nobject type.\n"},{"id":"244894","messageId":"20140623210930.GB15766@sigill.intra.peff.net","threadId":"36856","inReplyTo":"xmqq61jrcy3d.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCHv3 0/5] verify-commit: verify commit signatures","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-06-23T21:09:31Z","receivedAt":"2014-06-23T21:09:31Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Jun 23, 2014 at 10:52:38AM -0700, Junio C Hamano wrote:\n\n> > The one thing that does give me pause is that we do not seem to have any\n> > way of accessing mergetag signatures. We should perhaps stop and think\n> > for a second about how we might expose those (and whether it would fit\n> > into the \"git-verify-{commit,tag}\" paradigm). I am tempted to say that\n> > \"git verify-tag\" on a commit should verify the mergetag (right now it\n> > would simply be an error). But I haven't though that hard on it.\n> \n> I agree that \"verify-commit\" that lives next to \"verify-tag\" is fine\n> and does not have to wait for a unified \"verify\" that may not even\n> be a good idea, but if we were to verify the mergetags in one of\n> these \"verify-$OBJECTTYPE\" commands, I would think \"verify-commit\"\n> should be the one to check them, for the simple reason that they\n> appear in \"commit\" objects, not in \"tag\" objects.\n\nMy thinking was the opposite: it is a signature on a tag, but that\nsignature happens to be stuffed into a commit object. But I do not have\na strong feeling either way.\n\n-Peff\n"},{"id":"244895","messageId":"xmqqoaxjb9rr.fsf@gitster.dls.corp.google.com","threadId":"36856","inReplyTo":"20140623210930.GB15766@sigill.intra.peff.net","subject":"Re: [PATCHv3 0/5] verify-commit: verify commit signatures","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-06-23T21:23:20Z","receivedAt":"2014-06-23T21:23:20Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Mon, Jun 23, 2014 at 10:52:38AM -0700, Junio C Hamano wrote:\n>\n>> > The one thing that does give me pause is that we do not seem to have any\n>> > way of accessing mergetag signatures. We should perhaps stop and think\n>> > for a second about how we might expose those (and whether it would fit\n>> > into the \"git-verify-{commit,tag}\" paradigm). I am tempted to say that\n>> > \"git verify-tag\" on a commit should verify the mergetag (right now it\n>> > would simply be an error). But I haven't though that hard on it.\n>> \n>> I agree that \"verify-commit\" that lives next to \"verify-tag\" is fine\n>> and does not have to wait for a unified \"verify\" that may not even\n>> be a good idea, but if we were to verify the mergetags in one of\n>> these \"verify-$OBJECTTYPE\" commands, I would think \"verify-commit\"\n>> should be the one to check them, for the simple reason that they\n>> appear in \"commit\" objects, not in \"tag\" objects.\n>\n> My thinking was the opposite: it is a signature on a tag, but that\n> signature happens to be stuffed into a commit object. But I do not have\n> a strong feeling either way.\n\nWell, the whole point of storing mergetag inside commit objects is\nso that these transient \"please pull, here is a tag to prove you\nthat it is from me\" tags do not have to be kept in the history;\nhence people who are following along only see commits and not tags.\nThe signature being sent via a signed tag from the requestor to the\nintegrator is merely an implementation detail after the mergetag is\nrecorded and when people would want to verify them.\n\nSo...\n"},{"id":"244900","messageId":"xmqq1tufb56h.fsf@gitster.dls.corp.google.com","threadId":"36856","inReplyTo":"340919e5e17a582acaa57faefb362141e64d32a1.1403506792.git.git@drmicha.warpmail.net","subject":"Re: [PATCHv3 5/5] t7510: test verify-commit","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-06-23T23:02:30Z","receivedAt":"2014-06-23T23:02:30Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael J Gruber writes:\n\n> test_expect_success GPG 'detect fudged signature' '\n> \tgit cat-file commit master >raw &&\n> \n> \tsed -e \"s/seventh/7th forged/\" raw >forged1 &&\n> \tgit hash-object -w -t commit forged1 >forged1.commit &&\n> +\t! git verify-commit $(cat forged1.commit) &&\n\nThis should be \"test_must_fail git verify-commit ...\", I think.\nOtherwise you would end up declaring a segfaulting implementation a\ngood one.\n\n> +\t! git verify-commit $(cat forged2.commit) &&\n\nLikewise.\n"},{"id":"245065","messageId":"53AD640A.9060006@drmicha.warpmail.net","threadId":"36856","inReplyTo":"20140616203956.GA3546@sigill.intra.peff.net","subject":"Re: [PATCH 3/3] verify-commit: scriptable commit signature verification","fromName":"Michael J Gruber","fromEmail":"git@drmicha.warpmail.net","sentAt":"2014-06-27T12:31:06Z","receivedAt":"2014-06-27T12:31:06Z","isPatch":true,"sender":{"key":"git@grubix.eu","avatar":"https://avatars.githubusercontent.com/u/233215?v=4"},"body":"Jeff King venit, vidit, dixit 16.06.2014 22:39:\n> On Mon, Jun 16, 2014 at 01:34:20PM -0700, Junio C Hamano wrote:\n> \n>>> Your middle example above did make me think of one other thing, though.\n>>> As you noted, we actually have _three_ signature types:\n>>>\n>>> 1. signed tags\n>>>\n>>> 2. signed commits\n>>>\n>>> 3. merges with embedded mergetag headers\n>>>\n>>> We already have a tool for (1). Michael is adding a tool for (2). How\n>>> would one check (3) in a similar way?\n>>\n>> Hmph, somehow I misread the patch that it was for both 2 & 3 X-<.\n> \n> I was just assuming it handles only (2) without checking further, so I\n> may be wrong. But I do not think it makes sense to conflate (2) and (3).\n> A merge commit may have both, and they are separate signatures.\n> \n> For that matter, is there a way to expose (3) currently, besides via\n> --show-signature? It does not trigger \"%GG\" and friends (nor should it).\n> It may make sense to add extra format specifiers for mergetag\n> signatures. Though I do not use them myself, so I am not clear on what\n> the use case is besides a manual, human verification of a particular\n> merge.\n\nI'm afraid I'm on a weekly git schedule at best, sorry. Just trying to\ncatch up on this:\n\nAdmittedly, I simply don't know about \"3.\". I know only 1. and 2. (and\ndon't remember why they are implemented differently).\n\nAre they documented/decribed somewhere?\n\nMeanwhile, I'm rebasing on top of the %G related patches by Junio and\nJeff and hope to send out a v4 later today.\n\nMichael\n"},{"id":"245066","messageId":"53AD685A.1030401@drmicha.warpmail.net","threadId":"36856","inReplyTo":"53AD640A.9060006@drmicha.warpmail.net","subject":"Re: [PATCH 3/3] verify-commit: scriptable commit signature verification","fromName":"Michael J Gruber","fromEmail":"git@drmicha.warpmail.net","sentAt":"2014-06-27T12:49:30Z","receivedAt":"2014-06-27T12:49:30Z","isPatch":true,"sender":{"key":"git@grubix.eu","avatar":"https://avatars.githubusercontent.com/u/233215?v=4"},"body":"Michael J Gruber venit, vidit, dixit 27.06.2014 14:31:\n> Jeff King venit, vidit, dixit 16.06.2014 22:39:\n>> On Mon, Jun 16, 2014 at 01:34:20PM -0700, Junio C Hamano wrote:\n>>\n>>>> Your middle example above did make me think of one other thing, though.\n>>>> As you noted, we actually have _three_ signature types:\n>>>>\n>>>> 1. signed tags\n>>>>\n>>>> 2. signed commits\n>>>>\n>>>> 3. merges with embedded mergetag headers\n>>>>\n>>>> We already have a tool for (1). Michael is adding a tool for (2). How\n>>>> would one check (3) in a similar way?\n>>>\n>>> Hmph, somehow I misread the patch that it was for both 2 & 3 X-<.\n>>\n>> I was just assuming it handles only (2) without checking further, so I\n>> may be wrong. But I do not think it makes sense to conflate (2) and (3).\n>> A merge commit may have both, and they are separate signatures.\n>>\n>> For that matter, is there a way to expose (3) currently, besides via\n>> --show-signature? It does not trigger \"%GG\" and friends (nor should it).\n>> It may make sense to add extra format specifiers for mergetag\n>> signatures. Though I do not use them myself, so I am not clear on what\n>> the use case is besides a manual, human verification of a particular\n>> merge.\n> \n> I'm afraid I'm on a weekly git schedule at best, sorry. Just trying to\n> catch up on this:\n> \n> Admittedly, I simply don't know about \"3.\". I know only 1. and 2. (and\n> don't remember why they are implemented differently).\n> \n> Are they documented/decribed somewhere?\n> \n> Meanwhile, I'm rebasing on top of the %G related patches by Junio and\n> Jeff and hope to send out a v4 later today.\n> \n> Michael\n\nOK, found the two commits which \"git log -Smergetag\" outputs, but no tests.\n\nA merge commit with embedded signed tag it is, then.\n\nThe commit could carry it's own commit signature, couldn't it?\nThat would suggest that we use \"git verify-tag\" to verify the embedded\nsigned tag of a merge commit and \"git verify-commit\" to verify the\ncommit signature.\n\nOTOH I would like these basic commands to be as strict as possible,\nincluding type-checks. Does that mean having \"git verify-mergetag\" which\nverifies that it is being used on a merge commit with embedded mergetag?\n\n(BTW: Is there anything keeping a non-merge commit from having an\nembedded (merge) tag?)\n\nMichael\n"},{"id":"245067","messageId":"53AD6C6E.1080208@drmicha.warpmail.net","threadId":"36856","inReplyTo":"53AD685A.1030401@drmicha.warpmail.net","subject":"Re: [PATCH 3/3] verify-commit: scriptable commit signature verification","fromName":"Michael J Gruber","fromEmail":"git@drmicha.warpmail.net","sentAt":"2014-06-27T13:06:54Z","receivedAt":"2014-06-27T13:06:54Z","isPatch":true,"sender":{"key":"git@grubix.eu","avatar":"https://avatars.githubusercontent.com/u/233215?v=4"},"body":"Michael J Gruber venit, vidit, dixit 27.06.2014 14:49:\n> Michael J Gruber venit, vidit, dixit 27.06.2014 14:31:\n>> Jeff King venit, vidit, dixit 16.06.2014 22:39:\n>>> On Mon, Jun 16, 2014 at 01:34:20PM -0700, Junio C Hamano wrote:\n>>>\n>>>>> Your middle example above did make me think of one other thing, though.\n>>>>> As you noted, we actually have _three_ signature types:\n>>>>>\n>>>>> 1. signed tags\n>>>>>\n>>>>> 2. signed commits\n>>>>>\n>>>>> 3. merges with embedded mergetag headers\n>>>>>\n>>>>> We already have a tool for (1). Michael is adding a tool for (2). How\n>>>>> would one check (3) in a similar way?\n>>>>\n>>>> Hmph, somehow I misread the patch that it was for both 2 & 3 X-<.\n>>>\n>>> I was just assuming it handles only (2) without checking further, so I\n>>> may be wrong. But I do not think it makes sense to conflate (2) and (3).\n>>> A merge commit may have both, and they are separate signatures.\n>>>\n>>> For that matter, is there a way to expose (3) currently, besides via\n>>> --show-signature? It does not trigger \"%GG\" and friends (nor should it).\n>>> It may make sense to add extra format specifiers for mergetag\n>>> signatures. Though I do not use them myself, so I am not clear on what\n>>> the use case is besides a manual, human verification of a particular\n>>> merge.\n>>\n>> I'm afraid I'm on a weekly git schedule at best, sorry. Just trying to\n>> catch up on this:\n>>\n>> Admittedly, I simply don't know about \"3.\". I know only 1. and 2. (and\n>> don't remember why they are implemented differently).\n>>\n>> Are they documented/decribed somewhere?\n>>\n>> Meanwhile, I'm rebasing on top of the %G related patches by Junio and\n>> Jeff and hope to send out a v4 later today.\n>>\n>> Michael\n> \n> OK, found the two commits which \"git log -Smergetag\" outputs, but no tests.\n> \n> A merge commit with embedded signed tag it is, then.\n> \n> The commit could carry it's own commit signature, couldn't it?\n> That would suggest that we use \"git verify-tag\" to verify the embedded\n> signed tag of a merge commit and \"git verify-commit\" to verify the\n> commit signature.\n> \n> OTOH I would like these basic commands to be as strict as possible,\n> including type-checks. Does that mean having \"git verify-mergetag\" which\n> verifies that it is being used on a merge commit with embedded mergetag?\n> \n> (BTW: Is there anything keeping a non-merge commit from having an\n> embedded (merge) tag?)\n> \n> Michael\n\nAnother observation:\n\ngit merge -S branch\n#fix conflict\ngit commit # \"Merge --continue\"\n\nforgets that the merge was supposed to be signed. One needs to \"commit\n-S\" to sign the merge.\n\nAnother one:\nThe color for merge tags in \"log --color\" is terrible (colored\nbackground)...\n\nMichael\n"},{"id":"245068","messageId":"41f5b516d13fd02408f66ee38704a6c1bb09cbb3.1403875032.git.git@drmicha.warpmail.net","threadId":"36856","inReplyTo":"53AD6C6E.1080208@drmicha.warpmail.net","subject":"[PATCH] log: correctly identify mergetag signature verification status","fromName":"Michael J Gruber","fromEmail":"git@drmicha.warpmail.net","sentAt":"2014-06-27T13:18:36Z","receivedAt":"2014-06-27T13:18:36Z","isPatch":true,"sender":{"key":"git@grubix.eu","avatar":"https://avatars.githubusercontent.com/u/233215?v=4"},"body":"A wrong '}' made our code record the results of mergetag signature\nverification incorrectly.\n\nFix it.\n\nSigned-off-by: Michael J Gruber \n---\nThis is the \"color fix\"...\n\n log-tree.c | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/log-tree.c b/log-tree.c\nindex 10e6844..fdea358 100644\n--- a/log-tree.c\n+++ b/log-tree.c\n@@ -453,9 +453,9 @@ static void show_one_mergetag(struct rev_info *opt,\n \t\t\t\t\t &verify_message, NULL)) {\n \t\t\tif (verify_message.len <= gpg_message_offset)\n \t\t\t\tstrbuf_addstr(&verify_message, \"No signature\\n\");\n-\t\t\telse\n-\t\t\t\tstatus = 0;\n \t\t}\n+\t\telse\n+\t\t\tstatus = 0;\n \n \tshow_sig_lines(opt, status, verify_message.buf);\n \tstrbuf_release(&verify_message);\n-- \n2.0.1.563.g162087b.dirty\n"},{"id":"245069","messageId":"53AD76C3.3060702@drmicha.warpmail.net","threadId":"36856","inReplyTo":"53AD685A.1030401@drmicha.warpmail.net","subject":"Re: [PATCH 3/3] verify-commit: scriptable commit signature verification","fromName":"Michael J Gruber","fromEmail":"git@drmicha.warpmail.net","sentAt":"2014-06-27T13:50:59Z","receivedAt":"2014-06-27T13:50:59Z","isPatch":true,"sender":{"key":"git@grubix.eu","avatar":"https://avatars.githubusercontent.com/u/233215?v=4"},"body":"Michael J Gruber venit, vidit, dixit 27.06.2014 14:49:\n> Michael J Gruber venit, vidit, dixit 27.06.2014 14:31:\n>> Jeff King venit, vidit, dixit 16.06.2014 22:39:\n>>> On Mon, Jun 16, 2014 at 01:34:20PM -0700, Junio C Hamano wrote:\n>>>\n>>>>> Your middle example above did make me think of one other thing, though.\n>>>>> As you noted, we actually have _three_ signature types:\n>>>>>\n>>>>> 1. signed tags\n>>>>>\n>>>>> 2. signed commits\n>>>>>\n>>>>> 3. merges with embedded mergetag headers\n>>>>>\n>>>>> We already have a tool for (1). Michael is adding a tool for (2). How\n>>>>> would one check (3) in a similar way?\n>>>>\n>>>> Hmph, somehow I misread the patch that it was for both 2 & 3 X-<.\n>>>\n>>> I was just assuming it handles only (2) without checking further, so I\n>>> may be wrong. But I do not think it makes sense to conflate (2) and (3).\n>>> A merge commit may have both, and they are separate signatures.\n>>>\n>>> For that matter, is there a way to expose (3) currently, besides via\n>>> --show-signature? It does not trigger \"%GG\" and friends (nor should it).\n>>> It may make sense to add extra format specifiers for mergetag\n>>> signatures. Though I do not use them myself, so I am not clear on what\n>>> the use case is besides a manual, human verification of a particular\n>>> merge.\n>>\n>> I'm afraid I'm on a weekly git schedule at best, sorry. Just trying to\n>> catch up on this:\n>>\n>> Admittedly, I simply don't know about \"3.\". I know only 1. and 2. (and\n>> don't remember why they are implemented differently).\n>>\n>> Are they documented/decribed somewhere?\n>>\n>> Meanwhile, I'm rebasing on top of the %G related patches by Junio and\n>> Jeff and hope to send out a v4 later today.\n>>\n>> Michael\n> \n> OK, found the two commits which \"git log -Smergetag\" outputs, but no tests.\n> \n> A merge commit with embedded signed tag it is, then.\n> \n> The commit could carry it's own commit signature, couldn't it?\n> That would suggest that we use \"git verify-tag\" to verify the embedded\n> signed tag of a merge commit and \"git verify-commit\" to verify the\n> commit signature.\n> \n> OTOH I would like these basic commands to be as strict as possible,\n> including type-checks. Does that mean having \"git verify-mergetag\" which\n> verifies that it is being used on a merge commit with embedded mergetag?\n\n... or an extension ^{mergetag} to our machinery, defaulting to the\ntag object containing the mergetag for the 2nd parent, with an optional\nversion ^{mergetag}?\n\nOTOH, verifying a mergetag involves both looking at the signed tag and\nthe parent list of the commit. We probably should require signed (merge)\ntags on all but the first parent. Oh Can of Worms, oh Can of Worms (\"Oh\nCan'o'Worms\" to the tune of \"Oh Canada\").\n\nMichael\n"},{"id":"245070","messageId":"cover.1403877430.git.git@drmicha.warpmail.net","threadId":"36856","inReplyTo":"xmqqoaxjb9rr.fsf@gitster.dls.corp.google.com","subject":"[PATCHv4 0/4] verify-commit: verify commit signatures","fromName":"Michael J Gruber","fromEmail":"git@drmicha.warpmail.net","sentAt":"2014-06-27T14:13:22Z","receivedAt":"2014-06-27T14:13:22Z","isPatch":false,"sender":{"key":"git@grubix.eu","avatar":"https://avatars.githubusercontent.com/u/233215?v=4"},"body":"This is v3 rebased on current next (the %G works by Jeff & Junio).\n\nOpen questions:\n\n- Should one of git verify-{commit,tag} learn how to verify mergetags?\n(Probably no, it differs from both other cases.)\n\n- Should we do this now or go for generic \"git verify\" right away?\nThat depends on whether signed commits need to be verified by scripts now,\nor whether mergetags are more important.\n\nFor a general command which allows different verification policies,\nI'm still wondering whether we may need hooks which receive all\nthe relevant information in the environment. Otherwise we'll have a ton of\noptions such as --match-committer-uid, --verify--AllParentsHaveMergeTags,\n--verify--All-ParentsAreSignedCommits, --peel-to-commit, --merge-commit-only, ...\n\nI imagine that a generic \"git verify\" would provide \"git verify-{commit,tag}\"\naliases which call \"git verify\" with options that reproduce the current (suggested)\nbehavior.\n\nMichael J Gruber (4):\n gpg-interface: provide clear helper for struct signature_check\n gpg-interface: provide access to the payload\n verify-commit: scriptable commit signature verification\n t7510: test verify-commit\n\n Documentation/git-verify-commit.txt | 28 +++++++++++\n Makefile | 1 +\n builtin.h | 1 +\n builtin/merge.c | 5 +-\n builtin/verify-commit.c | 93 +++++++++++++++++++++++++++++++++++++\n command-list.txt | 1 +\n commit.c | 1 +\n git.c | 1 +\n gpg-interface.c | 14 ++++++\n gpg-interface.h | 2 +\n pretty.c | 3 +-\n t/t7510-signed-commit.sh | 20 +++++++-\n 12 files changed, 163 insertions(+), 7 deletions(-)\n create mode 100644 Documentation/git-verify-commit.txt\n create mode 100644 builtin/verify-commit.c\n\n-- \n2.0.1.563.g162087b.dirty\n"},{"id":"245074","messageId":"0a49d22936ffa4ffea1fa25b09b829b7b64d2e51.1403877430.git.git@drmicha.warpmail.net","threadId":"36856","inReplyTo":"xmqqoaxjb9rr.fsf@gitster.dls.corp.google.com","subject":"[PATCHv4 1/4] gpg-interface: provide clear helper for struct signature_check","fromName":"Michael J Gruber","fromEmail":"git@drmicha.warpmail.net","sentAt":"2014-06-27T14:13:23Z","receivedAt":"2014-06-27T14:13:23Z","isPatch":false,"sender":{"key":"git@grubix.eu","avatar":"https://avatars.githubusercontent.com/u/233215?v=4"},"body":"The struct has been growing members whose malloced memory needs to be\nfreed. Do this with one helper function so that no malloced memory shall\nbe left unfreed.\n\nSigned-off-by: Michael J Gruber \n---\n builtin/merge.c | 5 +----\n gpg-interface.c | 12 ++++++++++++\n gpg-interface.h | 1 +\n pretty.c | 3 +--\n 4 files changed, 15 insertions(+), 6 deletions(-)\n\ndiff --git a/builtin/merge.c b/builtin/merge.c\nindex b49c310..86e9c61 100644\n--- a/builtin/merge.c\n+++ b/builtin/merge.c\n@@ -1282,10 +1282,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix)\n \t\t\t\tprintf(_(\"Commit %s has a good GPG signature by %s\\n\"),\n \t\t\t\t hex, signature_check.signer);\n \n-\t\t\tfree(signature_check.gpg_output);\n-\t\t\tfree(signature_check.gpg_status);\n-\t\t\tfree(signature_check.signer);\n-\t\t\tfree(signature_check.key);\n+\t\t\tsignature_check_clear(&signature_check);\n \t\t}\n \t}\n \ndiff --git a/gpg-interface.c b/gpg-interface.c\nindex 8b0e874..e71b59d 100644\n--- a/gpg-interface.c\n+++ b/gpg-interface.c\n@@ -7,6 +7,18 @@\n static char *configured_signing_key;\n static const char *gpg_program = \"gpg\";\n \n+void signature_check_clear(struct signature_check *sigc)\n+{\n+\tfree(sigc->gpg_output);\n+\tfree(sigc->gpg_status);\n+\tfree(sigc->signer);\n+\tfree(sigc->key);\n+\tsigc->gpg_output = NULL;\n+\tsigc->gpg_status = NULL;\n+\tsigc->signer = NULL;\n+\tsigc->key = NULL;\n+}\n+\n void set_signing_key(const char *key)\n {\n \tfree(configured_signing_key);\ndiff --git a/gpg-interface.h b/gpg-interface.h\nindex a85cb5b..9f0784a 100644\n--- a/gpg-interface.h\n+++ b/gpg-interface.h\n@@ -13,6 +13,7 @@ struct signature_check {\n \tchar *key;\n };\n \n+extern void signature_check_clear(struct signature_check *sigc);\n extern int sign_buffer(struct strbuf *buffer, struct strbuf *signature, const char *signing_key);\n extern int verify_signed_buffer(const char *payload, size_t payload_size, const char *signature, size_t signature_size, struct strbuf *gpg_output, struct strbuf *gpg_status);\n extern int git_gpg_config(const char *, const char *, void *);\ndiff --git a/pretty.c b/pretty.c\nindex ce3bbaa..353af81 100644\n--- a/pretty.c\n+++ b/pretty.c\n@@ -1520,8 +1520,7 @@ void format_commit_message(const struct commit *commit,\n \n \tfree(context.commit_encoding);\n \tunuse_commit_buffer(commit, context.message);\n-\tfree(context.signature_check.gpg_output);\n-\tfree(context.signature_check.signer);\n+\tsignature_check_clear(&context.signature_check);\n }\n \n static void pp_header(struct pretty_print_context *pp,\n-- \n2.0.1.563.g162087b.dirty\n"},{"id":"245071","messageId":"c22945363b2ffdd69a66463c5bce851aba765b71.1403877431.git.git@drmicha.warpmail.net","threadId":"36856","inReplyTo":"xmqqoaxjb9rr.fsf@gitster.dls.corp.google.com","subject":"[PATCHv4 2/4] gpg-interface: provide access to the payload","fromName":"Michael J Gruber","fromEmail":"git@drmicha.warpmail.net","sentAt":"2014-06-27T14:13:24Z","receivedAt":"2014-06-27T14:13:24Z","isPatch":false,"sender":{"key":"git@grubix.eu","avatar":"https://avatars.githubusercontent.com/u/233215?v=4"},"body":"In contrast to tag signatures, commit signatures are put into the\nheader, that is between the other header parts and commit messages.\n\nProvide access to the commit content sans the signature, which is the\npayload that is actually signed. Commit signature verification does the\nparsing anyways, and callers may wish to act on or display the commit\nobject sans the signature.\n\nSigned-off-by: Michael J Gruber \n---\n commit.c | 1 +\n gpg-interface.c | 2 ++\n gpg-interface.h | 1 +\n 3 files changed, 4 insertions(+)\n\ndiff --git a/commit.c b/commit.c\nindex fb7897c..acb74b5 100644\n--- a/commit.c\n+++ b/commit.c\n@@ -1270,6 +1270,7 @@ void check_commit_signature(const struct commit* commit, struct signature_check\n \t\t\t\t &gpg_output, &gpg_status);\n \tif (status && !gpg_output.len)\n \t\tgoto out;\n+\tsigc->payload = strbuf_detach(&payload, NULL);\n \tsigc->gpg_output = strbuf_detach(&gpg_output, NULL);\n \tsigc->gpg_status = strbuf_detach(&gpg_status, NULL);\n \tparse_gpg_output(sigc);\ndiff --git a/gpg-interface.c b/gpg-interface.c\nindex e71b59d..ff07012 100644\n--- a/gpg-interface.c\n+++ b/gpg-interface.c\n@@ -9,10 +9,12 @@ static const char *gpg_program = \"gpg\";\n \n void signature_check_clear(struct signature_check *sigc)\n {\n+\tfree(sigc->payload);\n \tfree(sigc->gpg_output);\n \tfree(sigc->gpg_status);\n \tfree(sigc->signer);\n \tfree(sigc->key);\n+\tsigc->payload = NULL;\n \tsigc->gpg_output = NULL;\n \tsigc->gpg_status = NULL;\n \tsigc->signer = NULL;\ndiff --git a/gpg-interface.h b/gpg-interface.h\nindex 9f0784a..37c23da 100644\n--- a/gpg-interface.h\n+++ b/gpg-interface.h\n@@ -2,6 +2,7 @@\n #define GPG_INTERFACE_H\n \n struct signature_check {\n+\tchar *payload;\n \tchar *gpg_output;\n \tchar *gpg_status;\n \tchar result; /* 0 (not checked),\n-- \n2.0.1.563.g162087b.dirty\n"},{"id":"245072","messageId":"72341c940cb1d5f4045b64d50722265962a694da.1403877431.git.git@drmicha.warpmail.net","threadId":"36856","inReplyTo":"xmqqoaxjb9rr.fsf@gitster.dls.corp.google.com","subject":"[PATCHv4 3/4] verify-commit: scriptable commit signature verification","fromName":"Michael J Gruber","fromEmail":"git@drmicha.warpmail.net","sentAt":"2014-06-27T14:13:25Z","receivedAt":"2014-06-27T14:13:25Z","isPatch":false,"sender":{"key":"git@grubix.eu","avatar":"https://avatars.githubusercontent.com/u/233215?v=4"},"body":"Commit signatures can be verified using \"git show -s --show-signature\"\nor the \"%G?\" pretty format and parsing the output, which is well suited\nfor user inspection, but not for scripting.\n\nProvide a command \"verify-commit\" which is analogous to \"verify-tag\": It\nreturns 0 for good signatures and non-zero otherwise, has the gpg output\non stderr and (optionally) the commit object on stdout, sans the\nsignature, just like \"verify-tag\" does.\n\nSigned-off-by: Michael J Gruber \n---\n Documentation/git-verify-commit.txt | 28 +++++++++++\n Makefile | 1 +\n builtin.h | 1 +\n builtin/verify-commit.c | 93 +++++++++++++++++++++++++++++++++++++\n command-list.txt | 1 +\n git.c | 1 +\n 6 files changed, 125 insertions(+)\n create mode 100644 Documentation/git-verify-commit.txt\n create mode 100644 builtin/verify-commit.c\n\ndiff --git a/Documentation/git-verify-commit.txt b/Documentation/git-verify-commit.txt\nnew file mode 100644\nindex 0000000..9413e28\n--- /dev/null\n+++ b/Documentation/git-verify-commit.txt\n@@ -0,0 +1,28 @@\n+git-verify-commit(1)\n+====================\n+\n+NAME\n+----\n+git-verify-commit - Check the GPG signature of commits\n+\n+SYNOPSIS\n+--------\n+[verse]\n+'git verify-commit' ...\n+\n+DESCRIPTION\n+-----------\n+Validates the gpg signature created by 'git commit -S'.\n+\n+OPTIONS\n+-------\n+-v::\n+--verbose::\n+\tPrint the contents of the commit object before validating it.\n+\n+...::\n+\tSHA-1 identifiers of Git commit objects.\n+\n+GIT\n+---\n+Part of the linkgit:git[1] suite\ndiff --git a/Makefile b/Makefile\nindex 07ea105..b92418d 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -999,6 +999,7 @@ BUILTIN_OBJS += builtin/update-ref.o\n BUILTIN_OBJS += builtin/update-server-info.o\n BUILTIN_OBJS += builtin/upload-archive.o\n BUILTIN_OBJS += builtin/var.o\n+BUILTIN_OBJS += builtin/verify-commit.o\n BUILTIN_OBJS += builtin/verify-pack.o\n BUILTIN_OBJS += builtin/verify-tag.o\n BUILTIN_OBJS += builtin/write-tree.o\ndiff --git a/builtin.h b/builtin.h\nindex c47c110..5d91f31 100644\n--- a/builtin.h\n+++ b/builtin.h\n@@ -128,6 +128,7 @@ extern int cmd_update_server_info(int argc, const char **argv, const char *prefi\n extern int cmd_upload_archive(int argc, const char **argv, const char *prefix);\n extern int cmd_upload_archive_writer(int argc, const char **argv, const char *prefix);\n extern int cmd_var(int argc, const char **argv, const char *prefix);\n+extern int cmd_verify_commit(int argc, const char **argv, const char *prefix);\n extern int cmd_verify_tag(int argc, const char **argv, const char *prefix);\n extern int cmd_version(int argc, const char **argv, const char *prefix);\n extern int cmd_whatchanged(int argc, const char **argv, const char *prefix);\ndiff --git a/builtin/verify-commit.c b/builtin/verify-commit.c\nnew file mode 100644\nindex 0000000..b0f8504\n--- /dev/null\n+++ b/builtin/verify-commit.c\n@@ -0,0 +1,93 @@\n+/*\n+ * Builtin \"git commit-commit\"\n+ *\n+ * Copyright (c) 2014 Michael J Gruber \n+ *\n+ * Based on git-verify-tag\n+ */\n+#include \"cache.h\"\n+#include \"builtin.h\"\n+#include \"commit.h\"\n+#include \"run-command.h\"\n+#include \n+#include \"parse-options.h\"\n+#include \"gpg-interface.h\"\n+\n+static const char * const verify_commit_usage[] = {\n+\t\tN_(\"git verify-commit [-v|--verbose] ...\"),\n+\t\tNULL\n+};\n+\n+static int run_gpg_verify(const unsigned char *sha1, const char *buf, unsigned long size, int verbose)\n+{\n+\tstruct signature_check signature_check;\n+\n+\tmemset(&signature_check, 0, sizeof(signature_check));\n+\n+\tcheck_commit_signature(lookup_commit(sha1), &signature_check);\n+\n+\tif (verbose && signature_check.payload)\n+\t\tfputs(signature_check.payload, stdout);\n+\n+\tif (signature_check.gpg_output)\n+\t\tfputs(signature_check.gpg_output, stderr);\n+\n+\tsignature_check_clear(&signature_check);\n+\treturn signature_check.result != 'G';\n+}\n+\n+static int verify_commit(const char *name, int verbose)\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(\"commit '%s' not found.\", name);\n+\n+\tbuf = read_sha1_file(sha1, &type, &size);\n+\tif (!buf)\n+\t\treturn error(\"%s: unable to read file.\", name);\n+\tif (type != OBJ_COMMIT)\n+\t\treturn error(\"%s: cannot verify a non-commit object of type %s.\",\n+\t\t\t\tname, typename(type));\n+\n+\tret = run_gpg_verify(sha1, buf, size, verbose);\n+\n+\tfree(buf);\n+\treturn ret;\n+}\n+\n+static int git_verify_commit_config(const char *var, const char *value, void *cb)\n+{\n+\tint status = git_gpg_config(var, value, cb);\n+\tif (status)\n+\t\treturn status;\n+\treturn git_default_config(var, value, cb);\n+}\n+\n+int cmd_verify_commit(int argc, const char **argv, const char *prefix)\n+{\n+\tint i = 1, verbose = 0, had_error = 0;\n+\tconst struct option verify_commit_options[] = {\n+\t\tOPT__VERBOSE(&verbose, N_(\"print commit contents\")),\n+\t\tOPT_END()\n+\t};\n+\n+\tgit_config(git_verify_commit_config, NULL);\n+\n+\targc = parse_options(argc, argv, prefix, verify_commit_options,\n+\t\t\t verify_commit_usage, PARSE_OPT_KEEP_ARGV0);\n+\tif (argc <= i)\n+\t\tusage_with_options(verify_commit_usage, verify_commit_options);\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_commit(argv[i++], verbose))\n+\t\t\thad_error = 1;\n+\treturn had_error;\n+}\ndiff --git a/command-list.txt b/command-list.txt\nindex cf36c3d..a3ff0c9 100644\n--- a/command-list.txt\n+++ b/command-list.txt\n@@ -132,6 +132,7 @@ git-update-server-info synchingrepositories\n git-upload-archive synchelpers\n git-upload-pack synchelpers\n git-var plumbinginterrogators\n+git-verify-commit ancillaryinterrogators\n git-verify-pack plumbinginterrogators\n git-verify-tag ancillaryinterrogators\n gitweb ancillaryinterrogators\ndiff --git a/git.c b/git.c\nindex dd54f57..5b6c761 100644\n--- a/git.c\n+++ b/git.c\n@@ -478,6 +478,7 @@ static struct cmd_struct commands[] = {\n \t{ \"upload-archive\", cmd_upload_archive },\n \t{ \"upload-archive--writer\", cmd_upload_archive_writer },\n \t{ \"var\", cmd_var, RUN_SETUP_GENTLY },\n+\t{ \"verify-commit\", cmd_verify_commit, RUN_SETUP },\n \t{ \"verify-pack\", cmd_verify_pack },\n \t{ \"verify-tag\", cmd_verify_tag, RUN_SETUP },\n \t{ \"version\", cmd_version },\n-- \n2.0.1.563.g162087b.dirty\n"},{"id":"245073","messageId":"02b475e94770ed809e18e6258d593242abd23569.1403877431.git.git@drmicha.warpmail.net","threadId":"36856","inReplyTo":"xmqqoaxjb9rr.fsf@gitster.dls.corp.google.com","subject":"[PATCHv4 4/4] t7510: test verify-commit","fromName":"Michael J Gruber","fromEmail":"git@drmicha.warpmail.net","sentAt":"2014-06-27T14:13:26Z","receivedAt":"2014-06-27T14:13:26Z","isPatch":false,"sender":{"key":"git@grubix.eu","avatar":"https://avatars.githubusercontent.com/u/233215?v=4"},"body":"This mixes the \"git verify-commit\" tests in with the \"git show\n--show-signature\" tests, to keep the tests more readable.\n\nThe tests already mix in the \"call show\" tests with the \"verify\" tests.\nSo in case of a test beakage, a '-v' run would be needed to reveal the\nexact point of breakage anyway.\n\nAdditionally, test the actual output of \"git verify-commit\" and \"git\nshow --show-signature\" and compare to \"git cat-file\".\n\nSigned-off-by: Michael J Gruber \n---\n t/t7510-signed-commit.sh | 20 +++++++++++++++++++-\n 1 file changed, 19 insertions(+), 1 deletion(-)\n\ndiff --git a/t/t7510-signed-commit.sh b/t/t7510-signed-commit.sh\nindex e97477a..474dab3 100755\n--- a/t/t7510-signed-commit.sh\n+++ b/t/t7510-signed-commit.sh\n@@ -48,10 +48,11 @@ test_expect_success GPG 'create signed commits' '\n \tgit tag eighth-signed-alt\n '\n \n-test_expect_success GPG 'show signatures' '\n+test_expect_success GPG 'verify and show signatures' '\n \t(\n \t\tfor commit in initial second merge fourth-signed fifth-signed sixth-signed seventh-signed\n \t\tdo\n+\t\t\tgit verify-commit $commit &&\n \t\t\tgit show --pretty=short --show-signature $commit >actual &&\n \t\t\tgrep \"Good signature from\" actual &&\n \t\t\t! grep \"BAD signature from\" actual &&\n@@ -61,6 +62,7 @@ test_expect_success GPG 'show signatures' '\n \t(\n \t\tfor commit in merge^2 fourth-unsigned sixth-unsigned seventh-unsigned\n \t\tdo\n+\t\t\ttest_must_fail git verify-commit $commit &&\n \t\t\tgit show --pretty=short --show-signature $commit >actual &&\n \t\t\t! grep \"Good signature from\" actual &&\n \t\t\t! grep \"BAD signature from\" actual &&\n@@ -79,11 +81,25 @@ test_expect_success GPG 'show signatures' '\n \t)\n '\n \n+test_expect_success GPG 'show signed commit with signature' '\n+\tgit show -s initial >commit &&\n+\tgit show -s --show-signature initial >show &&\n+\tgit verify-commit -v initial >verify.1 2>verify.2 &&\n+\tgit cat-file commit initial >cat &&\n+\tgrep -v \"gpg: \" show >show.commit &&\n+\tgrep \"gpg: \" show >show.gpg &&\n+\tgrep -v \"^ \" cat | grep -v \"^gpgsig \" >cat.commit &&\n+\ttest_cmp show.commit commit &&\n+\ttest_cmp show.gpg verify.2 &&\n+\ttest_cmp cat.commit verify.1\n+'\n+\n test_expect_success GPG 'detect fudged signature' '\n \tgit cat-file commit seventh-signed >raw &&\n \n \tsed -e \"s/seventh/7th forged/\" raw >forged1 &&\n \tgit hash-object -w -t commit forged1 >forged1.commit &&\n+\t! git verify-commit $(cat forged1.commit) &&\n \tgit show --pretty=short --show-signature $(cat forged1.commit) >actual1 &&\n \tgrep \"BAD signature from\" actual1 &&\n \t! grep \"Good signature from\" actual1\n@@ -94,6 +110,7 @@ test_expect_success GPG 'detect fudged signature with NUL' '\n \tcat raw >forged2 &&\n \techo Qwik | tr \"Q\" \"\\000\" >>forged2 &&\n \tgit hash-object -w -t commit forged2 >forged2.commit &&\n+\t! git verify-commit $(cat forged2.commit) &&\n \tgit show --pretty=short --show-signature $(cat forged2.commit) >actual2 &&\n \tgrep \"BAD signature from\" actual2 &&\n \t! grep \"Good signature from\" actual2\n@@ -102,6 +119,7 @@ test_expect_success GPG 'detect fudged signature with NUL' '\n test_expect_success GPG 'amending already signed commit' '\n \tgit checkout fourth-signed^0 &&\n \tgit commit --amend -S --no-edit &&\n+\tgit verify-commit HEAD &&\n \tgit show -s --show-signature HEAD >actual &&\n \tgrep \"Good signature from\" actual &&\n \t! grep \"BAD signature from\" actual\n-- \n2.0.1.563.g162087b.dirty\n"},{"id":"245087","messageId":"xmqqwqc219og.fsf@gitster.dls.corp.google.com","threadId":"36856","inReplyTo":"53AD685A.1030401@drmicha.warpmail.net","subject":"Re: [PATCH 3/3] verify-commit: scriptable commit signature verification","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-06-27T18:36:47Z","receivedAt":"2014-06-27T18:36:47Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael J Gruber writes:\n\n> A merge commit with embedded signed tag it is, then.\n>\n> The commit could carry it's own commit signature, couldn't it?\n\nYes, an integrator can choose to sign a merge he creates, merging\nthe work by a contributor who gave him a pull-request for a tag\nsigned by the contributor. The resulting commit will embed the\ncontributor's signature to let historians verify the second parent,\nas well as the integrator's signature to allow verification of the\nmerge result. The integrator does not need to keep the signed tag\nused as an implementation detail of transferring the signature of\nthe contributor, and in general such a signed tag used only to\nrequest pulls is not available to the general public and historians\nafter such a merge is created.\n\nAs these signatures are part of a commit object, \"git verify-commit\"\nwould be the logical place to validate them, if we were to do so.\n"},{"id":"245088","messageId":"xmqqsimq18t0.fsf@gitster.dls.corp.google.com","threadId":"36856","inReplyTo":"53AD76C3.3060702@drmicha.warpmail.net","subject":"Re: [PATCH 3/3] verify-commit: scriptable commit signature verification","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-06-27T18:55:39Z","receivedAt":"2014-06-27T18:55:39Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael J Gruber writes:\n\n> ... or an extension ^{mergetag} to our machinery, defaulting to the\n> tag object containing the mergetag for the 2nd parent, with an optional\n> version ^{mergetag}?\n\nOne thing you should not forget is that with mergetag, the original\ntag object is not even necessary to exist in the repository, and\noften the original tag will not be propagated to the general public.\n\nYou _could_ extract the necessary information from the commit that\nmerges a signed tag to recreate the tag, but for what purpose? Your\n\"$commit^{mergetag}\" needs to recreate the named tag object but\nit is unclear to me how you envision it to be used.\n"},{"id":"245090","messageId":"xmqqoaxe1894.fsf@gitster.dls.corp.google.com","threadId":"36856","inReplyTo":"cover.1403877430.git.git@drmicha.warpmail.net","subject":"Re: [PATCHv4 0/4] verify-commit: verify commit signatures","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-06-27T19:07:35Z","receivedAt":"2014-06-27T19:07:35Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael J Gruber writes:\n\n> This is v3 rebased on current next (the %G works by Jeff & Junio).\n>\n> Open questions:\n>\n> - Should one of git verify-{commit,tag} learn how to verify mergetags?\n> (Probably no, it differs from both other cases.)\n\nIf we were to teach one of them, \"verify-commit\" as part of\n\"verifying what is recorded in the commit object\", would be the\nlogical place to do so.\n\nIt is OK to implement only verification of signatures on commit\nobjects themselves, but we would need a plan for handling other\nkinds of verifications later, so that we can give a stable output to\nscripts.\n\nIf we decide to signal successful verification of the signature on\nthe commit itself one way in this implementation, that should\nreliably be the way to do so even if we later add verification of\nother aspects on the commit object (e.g. mergetags it carries). If\nrunning \"verify-commit $commit\" and checking the zero-ness of its\nexit status is the way, that should not change if later versions of\nGit learns to verify mergetags as well; even if the given $commit\ncarries a mergetag that does not verify, as long as the signature in\nthe commit itself is valid, the script should continue to receive\n\"success\" from the command.\n\n> - Should we do this now or go for generic \"git verify\" right away?\n\nI do not think we are ready to do \"git verify\" yet. We first need\nto design how \"verify-commit\" should communicate failure/success\ncombinations of verification of a commit that has a signature on\nitself and a mergetag on one of its parents, and of a commit that\nhas zero or one signature on itself and two or more mergetags on its\nparents. Do we fail unless all of them are found to be valid? Do\nwe use bits in exit status? Output to the standard output, one line\nper signature verified?\n"},{"id":"245092","messageId":"xmqqk382173l.fsf@gitster.dls.corp.google.com","threadId":"36856","inReplyTo":"02b475e94770ed809e18e6258d593242abd23569.1403877431.git.git@drmicha.warpmail.net","subject":"Re: [PATCHv4 4/4] t7510: test verify-commit","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-06-27T19:32:30Z","receivedAt":"2014-06-27T19:32:30Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael J Gruber writes:\n\n> \tsed -e \"s/seventh/7th forged/\" raw >forged1 &&\n> \tgit hash-object -w -t commit forged1 >forged1.commit &&\n> +\t! git verify-commit $(cat forged1.commit) &&\n\ntest_must_fail git verify-commit ... &&\n\n> \tgit show --pretty=short --show-signature $(cat forged1.commit) >actual1 &&\n> \tgrep \"BAD signature from\" actual1 &&\n> \t! grep \"Good signature from\" actual1\n> @@ -94,6 +110,7 @@ test_expect_success GPG 'detect fudged signature with NUL' '\n> \tcat raw >forged2 &&\n> \techo Qwik | tr \"Q\" \"\\000\" >>forged2 &&\n> \tgit hash-object -w -t commit forged2 >forged2.commit &&\n> +\t! git verify-commit $(cat forged2.commit) &&\n\ntest_must_fail git verify-commit ... &&\n\n> \tgit show --pretty=short --show-signature $(cat forged2.commit) >actual2 &&\n> \tgrep \"BAD signature from\" actual2 &&\n> \t! grep \"Good signature from\" actual2\n> @@ -102,6 +119,7 @@ test_expect_success GPG 'detect fudged signature with NUL' '\n> test_expect_success GPG 'amending already signed commit' '\n> \tgit checkout fourth-signed^0 &&\n> \tgit commit --amend -S --no-edit &&\n> +\tgit verify-commit HEAD &&\n> \tgit show -s --show-signature HEAD >actual &&\n> \tgrep \"Good signature from\" actual &&\n> \t! grep \"BAD signature from\" actual\n\nMost of the tests, unlike \"git show --show-signature\" tests, do not\nseem to check the output from the command. Is it because its\nprimary interface to scripts is its exit status [*1*]?\n\n[Footnote]\n\n*1* \"Yes\" is totally an acceptable answer and a justification for\nnot checking the output in many of these tests.\n"},{"id":"245097","messageId":"cf4cc867-7626-4981-898b-7de29d8c701f@email.android.com","threadId":"36856","inReplyTo":"xmqqk382173l.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCHv4 4/4] t7510: test verify-commit","fromName":"Michael J Gruber","fromEmail":"git@drmicha.warpmail.net","sentAt":"2014-06-27T20:26:55Z","receivedAt":"2014-06-27T20:26:55Z","isPatch":false,"sender":{"key":"git@grubix.eu","avatar":"https://avatars.githubusercontent.com/u/233215?v=4"},"body":"\n\nOn 27. Juni 2014 21:32:30 MESZ, Junio C Hamano wrote:\n>Michael J Gruber writes:\n>\n>> \tsed -e \"s/seventh/7th forged/\" raw >forged1 &&\n>> \tgit hash-object -w -t commit forged1 >forged1.commit &&\n>> +\t! git verify-commit $(cat forged1.commit) &&\n>\n>test_must_fail git verify-commit ... &&\n>\n>> \tgit show --pretty=short --show-signature $(cat forged1.commit)\n>>actual1 &&\n>> \tgrep \"BAD signature from\" actual1 &&\n>> \t! grep \"Good signature from\" actual1\n>> @@ -94,6 +110,7 @@ test_expect_success GPG 'detect fudged signature\n>with NUL' '\n>> \tcat raw >forged2 &&\n>> \techo Qwik | tr \"Q\" \"\\000\" >>forged2 &&\n>> \tgit hash-object -w -t commit forged2 >forged2.commit &&\n>> +\t! git verify-commit $(cat forged2.commit) &&\n>\n>test_must_fail git verify-commit ... &&\n\nSorry for missing or mis-rebasing these. I meant to cover them all.\n\n>\n>> \tgit show --pretty=short --show-signature $(cat forged2.commit)\n>>actual2 &&\n>> \tgrep \"BAD signature from\" actual2 &&\n>> \t! grep \"Good signature from\" actual2\n>> @@ -102,6 +119,7 @@ test_expect_success GPG 'detect fudged signature\n>with NUL' '\n>> test_expect_success GPG 'amending already signed commit' '\n>> \tgit checkout fourth-signed^0 &&\n>> \tgit commit --amend -S --no-edit &&\n>> +\tgit verify-commit HEAD &&\n>> \tgit show -s --show-signature HEAD >actual &&\n>> \tgrep \"Good signature from\" actual &&\n>> \t! grep \"BAD signature from\" actual\n>\n>Most of the tests, unlike \"git show --show-signature\" tests, do not\n>seem to check the output from the command. Is it because its\n>primary interface to scripts is its exit status [*1*]?\n>\n>[Footnote]\n>\n>*1* \"Yes\" is totally an acceptable answer and a justification for\n>not checking the output in many of these tests.\n\nYes, the idea was to check the exit status in all cases (the loopy subtests) and the textual output in a few exemplary ones. I didn't want to bloat the test unnecessarily.\n\nMichael\n"},{"id":"245107","messageId":"20140628003246.GA13228@sigill.intra.peff.net","threadId":"36856","inReplyTo":"xmqqwqc219og.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 3/3] verify-commit: scriptable commit signature verification","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-06-28T00:32:46Z","receivedAt":"2014-06-28T00:32:46Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jun 27, 2014 at 11:36:47AM -0700, Junio C Hamano wrote:\n\n> Michael J Gruber writes:\n> \n> > A merge commit with embedded signed tag it is, then.\n> >\n> > The commit could carry it's own commit signature, couldn't it?\n> \n> Yes, an integrator can choose to sign a merge he creates, merging\n> the work by a contributor who gave him a pull-request for a tag\n> signed by the contributor. The resulting commit will embed the\n> contributor's signature to let historians verify the second parent,\n> as well as the integrator's signature to allow verification of the\n> merge result. The integrator does not need to keep the signed tag\n> used as an implementation detail of transferring the signature of\n> the contributor, and in general such a signed tag used only to\n> request pulls is not available to the general public and historians\n> after such a merge is created.\n> \n> As these signatures are part of a commit object, \"git verify-commit\"\n> would be the logical place to validate them, if we were to do so.\n\nWe already disagreed on this earlier in the discussion, but I have given\nit some more thought, and somehow using \"verify-commit\" for mergetags\nstill feels wrong to me. Let me see if I can put it into words.\n\nFirst off, I agree that \"verify-tag\" is probably not the right place.\nThere _is_ no tag object to verify anymore (the only reason it is a tag\nat all is that the signature came out of what once was a tag).\n\nLet us imagine that we have a \"verify-commit\" that verifies commit\nsignatures made by \"commit -S\". If I run \"verify-commit foo\", that\nimplies to me two things:\n\n 1. I am verifying the signature over the contents of \"foo\".\n\n But the mergetag is _not_ a statement about \"foo\"; the person who\n signed it never saw \"foo\". It is a statement about foo^2 at best,\n or possibly about the whole history of foo^1..foo^2.\n\n 2. I am verifying _only_ the contents of foo. That is, I expect people\n to use \"commit -S\" to cryptographically claim authorship of a\n commit. Whereas with tags, I think we have typically taken them to\n mean \"I have signed the tip of a segment of history, and I am\n taking responsibility for the final state\" (e.g., signing release\n tags).\n\n I realize that this claim is somewhat tenuous. It's not something\n inherent in the crypto, but rather in the social convention of what\n it means to sign a commit. One could easily say \"signing a commit\n is about signing the whole tree state\". But I would ask, then: what\n is the point of signing a commit versus signing a tag? I think\n that people who wanted commit signatures wanted them to give a\n stronger guarantee of authorship of individual commits.\n\n Git has largely stayed agnostic about what such signatures mean.\n But if we accept that some projects _are_ going to make that\n distinction, I think conflating verification of the two within the\n same command leads to a potential for confusion.\n\nSo for that reason, I think I'd be in favor of simply treating mergetag\nsignatures as a separate, third entity. Give them their own\n%G-specifiers, and give them a separate plumbing command. Let projects\nwork out how they want to use them, but do not create any particular\naffiliation between mergetags and commit signatures (nor between tag\nsignatures and mergetag signatures).\n\n-Peff\n"},{"id":"245110","messageId":"20140628004458.GB13228@sigill.intra.peff.net","threadId":"36856","inReplyTo":"41f5b516d13fd02408f66ee38704a6c1bb09cbb3.1403875032.git.git@drmicha.warpmail.net","subject":"Re: [PATCH] log: correctly identify mergetag signature verification status","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-06-28T00:44:59Z","receivedAt":"2014-06-28T00:44:59Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jun 27, 2014 at 03:18:36PM +0200, Michael J Gruber wrote:\n\n> A wrong '}' made our code record the results of mergetag signature\n> verification incorrectly.\n> \n> Fix it.\n\nI think this is the right thing to do, but we went from:\n\n if (...)\n\tif (...) {\n\t\tif (...)\n\t\t\t...\n\t\telse\n\t\t\t...\n\t}\n\nto:\n\n if (...)\n\tif (...) {\n\t\tif (...)\n\t\t\t...\n\t}\n\telse\n\t\t...\n\nI think part of the point of the original {} was to make it more clear\nwhich else went with which if. Perhaps we should use more here.\n\nI also find the logic a bit hard to follow in that the outer conditions are:\n\n if (needed_for_goodsig)\n\tif (sig_is_not_good)\n\t\t...\n\nPerhaps it would be easier to read (and would have made the logic error\nyou are fixing more obvious) as:\n\n if (extra->len > payload_size) {\n\tif (!verify_signed_buffer(...))\n\t\tstatus = 0; /* good; all other code paths leave it as -1 */\n\telse if (verify_message.len <= gpg_message_offset)\n\t\tstrbuf_addstr(&verify_message, \"No signature\\n\");\n }\n\nThat is, for each conditional to check one more thing needed for a good\nsignature, and then know that all other code paths leave status as -1.\n\n-Peff\n"},{"id":"245111","messageId":"20140628004814.GC13228@sigill.intra.peff.net","threadId":"36856","inReplyTo":"xmqqoaxe1894.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCHv4 0/4] verify-commit: verify commit signatures","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-06-28T00:48:14Z","receivedAt":"2014-06-28T00:48:14Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jun 27, 2014 at 12:07:35PM -0700, Junio C Hamano wrote:\n\n> > - Should we do this now or go for generic \"git verify\" right away?\n> \n> I do not think we are ready to do \"git verify\" yet.\n\nIf there is one thing that this discussion have convinced me of, it is\nthis. :)\n\n-Peff\n"},{"id":"245112","messageId":"20140628004946.GD13228@sigill.intra.peff.net","threadId":"36856","inReplyTo":"cover.1403877430.git.git@drmicha.warpmail.net","subject":"Re: [PATCHv4 0/4] verify-commit: verify commit signatures","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-06-28T00:49:47Z","receivedAt":"2014-06-28T00:49:47Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jun 27, 2014 at 04:13:22PM +0200, Michael J Gruber wrote:\n\n> This is v3 rebased on current next (the %G works by Jeff & Junio).\n\nAside from the minor test issues Junio pointed out, I think this version\nlooks OK.\n\n> For a general command which allows different verification policies,\n> I'm still wondering whether we may need hooks which receive all\n> the relevant information in the environment. Otherwise we'll have a ton of\n> options such as --match-committer-uid, --verify--AllParentsHaveMergeTags,\n> --verify--All-ParentsAreSignedCommits, --peel-to-commit, --merge-commit-only, ...\n\nI'd guess these would be project policies that could go in config. But I\ndo not think your series needs to be concerned with that yet.\n\n-Peff\n"},{"id":"245168","messageId":"xmqqzjguzzxo.fsf@gitster.dls.corp.google.com","threadId":"36856","inReplyTo":"20140628003246.GA13228@sigill.intra.peff.net","subject":"Re: [PATCH 3/3] verify-commit: scriptable commit signature verification","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-06-30T06:14:43Z","receivedAt":"2014-06-30T06:14:43Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> First off, I agree that \"verify-tag\" is probably not the right place.\n> There _is_ no tag object to verify anymore (the only reason it is a tag\n> at all is that the signature came out of what once was a tag).\n\nYes, if we imagine that the header were called \"mergesig\", it may be\nmore apparent what it is. Similar to the commit signature itself\n(which gives GPG protection over the commit itself), a \"mergesig\"\ncovers one of the parent commit object name recorded in the commit.\n\nYou could look at it as one aspect of the child commit object being\nverified, and that is why I would suggest it is the responsibility\nof verify-commit that is run on the child commit (I saw suggestions\nto verify other aspects such as \"author ident matches the GPG signer\nof the signature on the commit itself\", etc.)\n\n> Let us imagine that we have a \"verify-commit\" that verifies commit\n> signatures made by \"commit -S\". If I run \"verify-commit foo\", that\n> implies to me two things:\n>\n> 1. I am verifying the signature over the contents of \"foo\".\n>\n> But the mergetag is _not_ a statement about \"foo\"; the person who\n> signed it never saw \"foo\". It is a statement about foo^2 at best,\n> or possibly about the whole history of foo^1..foo^2.\n>\n> 2. I am verifying _only_ the contents of foo. That is, I expect people\n> to use \"commit -S\" to cryptographically claim authorship of a\n> commit. Whereas with tags, I think we have typically taken them to\n> mean \"I have signed the tip of a segment of history, and I am\n> taking responsibility for the final state\" (e.g., signing release\n> tags).\n\nI think you are making it too hard ;-). Fundamentally, a signature\non a commit object itself _can_ be used to attest _some_ property\nabout the whole history behind it by the signer, but it does not\nprevent a project from adopting a lot looser convention. If a\nproject's stance is \"commit signature in this project is about the\ncontents of the tree\" (i.e. the signer does not have much confidence\nin his memory on how he got there), the result from verify-commit on\nthe signature of the commit itself should be interpreted in the\ncontext of such a project as such---the signer is confident that the\ntree matches what he signed and the signature wouldn't mean any more\nthan that.\n\n> I realize that this claim is somewhat tenuous. It's not something\n> inherent in the crypto, but rather in the social convention of what\n> it means to sign a commit.\n\nYup, I think we are on the same page.\n\n> But I would ask, then: what\n> is the point of signing a commit versus signing a tag? I think\n> that people who wanted commit signatures wanted them to give a\n> stronger guarantee of authorship of individual commits.\n\nThere are practical concerns.\n\n - tags are easier to manage because you can make a commit, give it\n out for trials by guinea pigs and then after it proves good, you\n can add a signed tag after the fact without rewriting the commit\n itself. Incidentally, this is the primary reason behind the\n design of mergetags that were introduced after the k.org break-in\n fiasco.\n\n Theoretically you could have argued that lieutenants can sign the\n commits they want Linus to pull and we wouldn't have had to add\n anything to the system to support the new \"not only we can use\n third-party publishing sites that we do not know how much we can\n trust, such as GitHub, we do not even have to trust k.org blindly\"\n workflow. But they really wanted to be able to commit first,\n let time pass and then sign to request pulling.\n\n - tags are a lot more cumbersome if you want to sign each and every\n commit (which some of us view as pointless, given the chain of\n SHA-1 hashes our history storage is based on), because you end up\n having to name, keep and transfer O(n) refs to represent them for\n the history of project of size n. Embedding signature in each\n commit would be the only workable way if somebody wants to sign\n each and every commit.\n\n> Git has largely stayed agnostic about what such signatures mean.\n> But if we accept that some projects _are_ going to make that\n> distinction, I think conflating verification of the two within the\n> same command leads to a potential for confusion.\n\nI agree that the users and scripts must be able to inspect the\nvalidity of \"gpgsig\" and \"mergetag\"(s) separately. When you inspect\na merge commit, if gpgsig verifies but mergetag does not, we know\nthat the integrator created and signed the merge but we do not know\nif the lieutenant whose name appears on the mergetag really asked\nthe side branch to be pulled, or if the merge was made in response\nto a request by an imposter. For that matter, when verifying an\noctopus merge, we must be able to see the validity of \"mergetag\" for\neach parent separately. We can do that by having two separate\ncommands (one to verify \"gpgsig\", the other to verify \"mergetag\"),\nor we can do that with a separate command line options to a single\ncommand (\"verify-commit --gpgsig\" and \"verify-commit --mergsig \")\nor a single \"verify-commit\" may report multi-values to its output\nfor scripts to parse. I do not have a strong opinion.\n"},{"id":"245728","messageId":"xmqq7g3k7su5.fsf@gitster.dls.corp.google.com","threadId":"36856","inReplyTo":"20140628004458.GB13228@sigill.intra.peff.net","subject":"Re: [PATCH] log: correctly identify mergetag signature verification status","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-07-10T22:27:14Z","receivedAt":"2014-07-10T22:27:14Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> Perhaps it would be easier to read (and would have made the logic error\n> you are fixing more obvious) as:\n>\n> if (extra->len > payload_size) {\n> \tif (!verify_signed_buffer(...))\n> \t\tstatus = 0; /* good; all other code paths leave it as -1 */\n> \telse if (verify_message.len <= gpg_message_offset)\n> \t\tstrbuf_addstr(&verify_message, \"No signature\\n\");\n> }\n>\n> That is, for each conditional to check one more thing needed for a good\n> signature, and then know that all other code paths leave status as -1.\n\nThanks. Let's do it this way, then.\n\n log-tree.c | 21 +++++++++++----------\n 1 file changed, 11 insertions(+), 10 deletions(-)\n\ndiff --git a/log-tree.c b/log-tree.c\nindex 1982631..b4bbfe1 100644\n--- a/log-tree.c\n+++ b/log-tree.c\n@@ -446,16 +446,17 @@ static void show_one_mergetag(struct rev_info *opt,\n \n \tpayload_size = parse_signature(extra->value, extra->len);\n \tstatus = -1;\n-\tif (extra->len > payload_size)\n-\t\tif (verify_signed_buffer(extra->value, payload_size,\n-\t\t\t\t\t extra->value + payload_size,\n-\t\t\t\t\t extra->len - payload_size,\n-\t\t\t\t\t &verify_message, NULL)) {\n-\t\t\tif (verify_message.len <= gpg_message_offset)\n-\t\t\t\tstrbuf_addstr(&verify_message, \"No signature\\n\");\n-\t\t\telse\n-\t\t\t\tstatus = 0;\n-\t\t}\n+\tif (extra->len > payload_size) {\n+\t\t/* could have a good signature */\n+\t\tif (!verify_signed_buffer(extra->value, payload_size,\n+\t\t\t\t\t extra->value + payload_size,\n+\t\t\t\t\t extra->len - payload_size,\n+\t\t\t\t\t &verify_message, NULL))\n+\t\t\tstatus = 0; /* good */\n+\t\telse if (verify_message.len <= gpg_message_offset)\n+\t\t\tstrbuf_addstr(&verify_message, \"No signature\\n\");\n+\t\t/* otherwise we couldn't verify, which is shown as bad */\n+\t}\n \n \tshow_sig_lines(opt, status, verify_message.buf);\n \tstrbuf_release(&verify_message);\n"}]}