{"thread":{"id":"48822","subject":"[PATCH 2/8] gpg-interface: make parse_gpg_output static and remove from interface header","startedAt":"2018-07-03T12:38:43Z","lastAt":"2018-07-10T17:15:08Z","messageCount":36,"participants":["Henning Schild","Martin Ågren","brian m. carlson","Junio C Hamano","Eric Sunshine","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":8},"messages":[{"id":"351609","messageId":"192cf9fc4e7a601d2639ec2d82c777d4c7b26e99.1530616446.git.henning.schild@siemens.com","threadId":"48822","inReplyTo":"cover.1530616446.git.henning.schild@siemens.com","subject":"[PATCH 2/8] gpg-interface: make parse_gpg_output static and remove from interface header","fromName":"Henning Schild","fromEmail":"henning.schild@siemens.com","sentAt":"2018-07-03T12:38:14Z","receivedAt":"2018-07-03T12:38:43Z","isPatch":true,"sender":{"key":"henning.schild@siemens.com","avatar":"https://avatars.githubusercontent.com/u/7237877?v=4"},"body":"This commit turns parse_gpg_output into an internal function, the only\noutside user was migrated in an earlier commit.\n\nSigned-off-by: Henning Schild \n---\n gpg-interface.c | 2 +-\n gpg-interface.h | 2 --\n 2 files changed, 1 insertion(+), 3 deletions(-)\n\ndiff --git a/gpg-interface.c b/gpg-interface.c\nindex 0647bd634..09ddfbc26 100644\n--- a/gpg-interface.c\n+++ b/gpg-interface.c\n@@ -35,7 +35,7 @@ static struct {\n \t{ 'R', \"\\n[GNUPG:] REVKEYSIG \"},\n };\n \n-void parse_gpg_output(struct signature_check *sigc)\n+static void parse_gpg_output(struct signature_check *sigc)\n {\n \tconst char *buf = sigc->gpg_status;\n \tint i;\ndiff --git a/gpg-interface.h b/gpg-interface.h\nindex a5e6517ae..5ecff4aa0 100644\n--- a/gpg-interface.h\n+++ b/gpg-interface.h\n@@ -33,8 +33,6 @@ void signature_check_clear(struct signature_check *sigc);\n */\n size_t parse_signature(const char *buf, size_t size);\n \n-void parse_gpg_output(struct signature_check *);\n-\n /*\n * Create a detached signature for the contents of \"buffer\" and append\n * it after \"signature\"; \"buffer\" and \"signature\" can be the same\n-- \n2.16.4\n\n"},{"id":"351610","messageId":"f9e371c8dd2a17ddb5fd5989a7fdad1c0d1bb6e7.1530616446.git.henning.schild@siemens.com","threadId":"48822","inReplyTo":"cover.1530616446.git.henning.schild@siemens.com","subject":"[PATCH 1/8] builtin/receive-pack: use check_signature from gpg-interface","fromName":"Henning Schild","fromEmail":"henning.schild@siemens.com","sentAt":"2018-07-03T12:38:13Z","receivedAt":"2018-07-03T12:38:46Z","isPatch":true,"sender":{"key":"henning.schild@siemens.com","avatar":"https://avatars.githubusercontent.com/u/7237877?v=4"},"body":"The combination of verify_signed_buffer followed by parse_gpg_output is\navailable as check_signature. Use that instead of implementing it again.\n\nSigned-off-by: Henning Schild \n---\n builtin/receive-pack.c | 17 ++---------------\n 1 file changed, 2 insertions(+), 15 deletions(-)\n\ndiff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\nindex 68d36e0a5..9f0583deb 100644\n--- a/builtin/receive-pack.c\n+++ b/builtin/receive-pack.c\n@@ -629,8 +629,6 @@ static void prepare_push_cert_sha1(struct child_process *proc)\n \t\treturn;\n \n \tif (!already_done) {\n-\t\tstruct strbuf gpg_output = STRBUF_INIT;\n-\t\tstruct strbuf gpg_status = STRBUF_INIT;\n \t\tint bogs /* beginning_of_gpg_sig */;\n \n \t\talready_done = 1;\n@@ -639,22 +637,11 @@ static void prepare_push_cert_sha1(struct child_process *proc)\n \t\t\toidclr(&push_cert_oid);\n \n \t\tmemset(&sigcheck, '\\0', sizeof(sigcheck));\n-\t\tsigcheck.result = 'N';\n \n \t\tbogs = parse_signature(push_cert.buf, push_cert.len);\n-\t\tif (verify_signed_buffer(push_cert.buf, bogs,\n-\t\t\t\t\t push_cert.buf + bogs, push_cert.len - bogs,\n-\t\t\t\t\t &gpg_output, &gpg_status) < 0) {\n-\t\t\t; /* error running gpg */\n-\t\t} else {\n-\t\t\tsigcheck.payload = push_cert.buf;\n-\t\t\tsigcheck.gpg_output = gpg_output.buf;\n-\t\t\tsigcheck.gpg_status = gpg_status.buf;\n-\t\t\tparse_gpg_output(&sigcheck);\n-\t\t}\n+\t\tcheck_signature(push_cert.buf, bogs, push_cert.buf + bogs,\n+\t\t\t\tpush_cert.len - bogs, &sigcheck);\n \n-\t\tstrbuf_release(&gpg_output);\n-\t\tstrbuf_release(&gpg_status);\n \t\tnonce_status = check_nonce(push_cert.buf, bogs);\n \t}\n \tif (!is_null_oid(&push_cert_oid)) {\n-- \n2.16.4\n\n"},{"id":"351611","messageId":"cover.1530616446.git.henning.schild@siemens.com","threadId":"48822","inReplyTo":null,"subject":"[PATCH 0/8] X509 (gpgsm) commit signing support","fromName":"Henning Schild","fromEmail":"henning.schild@siemens.com","sentAt":"2018-07-03T12:38:12Z","receivedAt":"2018-07-03T12:38:52Z","isPatch":true,"sender":{"key":"henning.schild@siemens.com","avatar":"https://avatars.githubusercontent.com/u/7237877?v=4"},"body":"This series adds support for signing commits with gpgsm.\n\nThe first two patches are cleanups of gpg-interface, while the next\nfour prepare for the introduction of the actual feature in patch 7.\nFinally patch 8 extends the testsuite to cover the new feature.\n\nThis series can be seen as a follow up of a series that appeared under\nthe name \"gpg-interface: Multiple signing tools\" in april 2018 [1]. After\nthat series was not merged i decided to get my patches ready. The\noriginal series aimed at being generic for any sort of signing tool, while\nthis series just introduced the X509 variant of gpg. (gpgsm)\nI collected authors and reviewers of that first series and already put them\non cc.\n\n[1] https://public-inbox.org/git/20180409204129.43537-1-mastahyeti@gmail.com/\n\nHenning Schild (8):\n builtin/receive-pack: use check_signature from gpg-interface\n gpg-interface: make parse_gpg_output static and remove from interface\n header\n gpg-interface: add new config to select how to sign a commit\n gpg-interface: introduce an abstraction for multiple gpg formats\n t/t7510: check the validation of the new config gpg.format\n gpg-interface: do not hardcode the key string len anymore\n gpg-interface: introduce new signature format \"X509\" using gpgsm\n gpg-interface t: extend the existing GPG tests with GPGSM\n\n Documentation/config.txt | 7 ++++\n builtin/receive-pack.c | 17 +--------\n gpg-interface.c | 94 ++++++++++++++++++++++++++++++++++++++--------\n gpg-interface.h | 2 -\n t/lib-gpg.sh | 9 ++++-\n t/lib-gpg/gpgsm-gen-key.in | 6 +++\n t/t4202-log.sh | 66 ++++++++++++++++++++++++++++++++\n t/t5534-push-signed.sh | 52 +++++++++++++++++++++++++\n t/t7003-filter-branch.sh | 15 ++++++++\n t/t7030-verify-tag.sh | 47 ++++++++++++++++++++++-\n t/t7510-signed-commit.sh | 10 +++++\n t/t7600-merge.sh | 31 +++++++++++++++\n 12 files changed, 321 insertions(+), 35 deletions(-)\n create mode 100644 t/lib-gpg/gpgsm-gen-key.in\n\n-- \n2.16.4\n\n"},{"id":"351612","messageId":"9fd9d0d2ee3ae549baf8e9a710dfffbc118e66f7.1530616446.git.henning.schild@siemens.com","threadId":"48822","inReplyTo":"cover.1530616446.git.henning.schild@siemens.com","subject":"[PATCH 5/8] t/t7510: check the validation of the new config gpg.format","fromName":"Henning Schild","fromEmail":"henning.schild@siemens.com","sentAt":"2018-07-03T12:38:17Z","receivedAt":"2018-07-03T12:38:54Z","isPatch":true,"sender":{"key":"henning.schild@siemens.com","avatar":"https://avatars.githubusercontent.com/u/7237877?v=4"},"body":"Valid values are already covered by all tests that use GPG, now also\ntest what happens if we go for an invalid one.\n\nSigned-off-by: Henning Schild \n---\n t/t7510-signed-commit.sh | 10 ++++++++++\n 1 file changed, 10 insertions(+)\n\ndiff --git a/t/t7510-signed-commit.sh b/t/t7510-signed-commit.sh\nindex 6e2015ed9..cb523513f 100755\n--- a/t/t7510-signed-commit.sh\n+++ b/t/t7510-signed-commit.sh\n@@ -227,4 +227,14 @@ test_expect_success GPG 'log.showsignature behaves like --show-signature' '\n \tgrep \"gpg: Good signature\" actual\n '\n \n+test_expect_success GPG 'check gpg config for malformed values' '\n+\tmv .git/config .git/config.old &&\n+\ttest_when_finished \"mv .git/config.old .git/config\" &&\n+\tgit config gpg.format malformed &&\n+\ttest_expect_code 128 git commit -S --amend -m \"fail\" 2>result &&\n+\ttest_i18ngrep \"malformed value for gpg.format: malformed\" result &&\n+\ttest_i18ngrep \"fatal: .*\\.git/config\" result &&\n+\ttest_i18ngrep \"fatal: .*line 2\" result\n+'\n+\n test_done\n-- \n2.16.4\n\n"},{"id":"351614","messageId":"0d83d4d5c1c90011db90947d767ea0191a7ecbd7.1530616446.git.henning.schild@siemens.com","threadId":"48822","inReplyTo":"cover.1530616446.git.henning.schild@siemens.com","subject":"[PATCH 6/8] gpg-interface: do not hardcode the key string len anymore","fromName":"Henning Schild","fromEmail":"henning.schild@siemens.com","sentAt":"2018-07-03T12:38:18Z","receivedAt":"2018-07-03T12:53:37Z","isPatch":true,"sender":{"key":"henning.schild@siemens.com","avatar":"https://avatars.githubusercontent.com/u/7237877?v=4"},"body":"gnupg does print the keyid followed by a space and the signer comes\nnext. The same pattern is also used in gpgsm, but there the key length\nwould be 40 instead of 16. Instead of hardcoding the expected length,\nfind the first space and calculate it.\n\nSigned-off-by: Henning Schild \n---\n gpg-interface.c | 5 +++--\n 1 file changed, 3 insertions(+), 2 deletions(-)\n\ndiff --git a/gpg-interface.c b/gpg-interface.c\nindex cd3b1b568..aa747278e 100644\n--- a/gpg-interface.c\n+++ b/gpg-interface.c\n@@ -88,10 +88,11 @@ static void parse_gpg_output(struct signature_check *sigc)\n \t\tsigc->result = sigcheck_gpg_status[i].result;\n \t\t/* The trust messages are not followed by key/signer information */\n \t\tif (sigc->result != 'U') {\n-\t\t\tsigc->key = xmemdupz(found, 16);\n+\t\t\tnext = strchrnul(found, ' ');\n+\t\t\tsigc->key = xmemdupz(found, next - found);\n \t\t\t/* The ERRSIG message is not followed by signer information */\n \t\t\tif (sigc-> result != 'E') {\n-\t\t\t\tfound += 17;\n+\t\t\t\tfound = next + 1;\n \t\t\t\tnext = strchrnul(found, '\\n');\n \t\t\t\tsigc->signer = xmemdupz(found, next - found);\n \t\t\t}\n-- \n2.16.4\n\n"},{"id":"351615","messageId":"9bb55c019c3bc025d77d9d491818e442233bf8d3.1530616446.git.henning.schild@siemens.com","threadId":"48822","inReplyTo":"cover.1530616446.git.henning.schild@siemens.com","subject":"[PATCH 3/8] gpg-interface: add new config to select how to sign a commit","fromName":"Henning Schild","fromEmail":"henning.schild@siemens.com","sentAt":"2018-07-03T12:38:15Z","receivedAt":"2018-07-03T12:53:41Z","isPatch":true,"sender":{"key":"henning.schild@siemens.com","avatar":"https://avatars.githubusercontent.com/u/7237877?v=4"},"body":"Add \"gpg.format\" where the user can specify which type of signature to\nuse for commits. At the moment only \"PGP\" is supported and the value is\nnot even used. This commit prepares for a new types of signatures.\n\nSigned-off-by: Henning Schild \n---\n Documentation/config.txt | 4 ++++\n gpg-interface.c | 7 +++++++\n 2 files changed, 11 insertions(+)\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex 1cc18a828..c88903399 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -1828,6 +1828,10 @@ gpg.program::\n \tsigned, and the program is expected to send the result to its\n \tstandard output.\n \n+gpg.format::\n+\tSpecifies which key format to use when signing with `--gpg-sign`.\n+\tDefault is \"PGP\", that is also the only supported value.\n+\n gui.commitMsgWidth::\n \tDefines how wide the commit message window is in the\n \tlinkgit:git-gui[1]. \"75\" is the default.\ndiff --git a/gpg-interface.c b/gpg-interface.c\nindex 09ddfbc26..1def1f131 100644\n--- a/gpg-interface.c\n+++ b/gpg-interface.c\n@@ -7,6 +7,7 @@\n #include \"tempfile.h\"\n \n static char *configured_signing_key;\n+static const char *gpg_format = \"PGP\";\n static const char *gpg_program = \"gpg\";\n \n #define PGP_SIGNATURE \"-----BEGIN PGP SIGNATURE-----\"\n@@ -138,6 +139,12 @@ int git_gpg_config(const char *var, const char *value, void *cb)\n \t\treturn 0;\n \t}\n \n+\tif (!strcmp(var, \"gpg.format\")) {\n+\t\tif (!strcmp(value, \"PGP\"))\n+\t\t\treturn error(\"malformed value for %s: %s\", var, value);\n+\t\treturn git_config_string(&gpg_format, var, value);\n+\t}\n+\n \tif (!strcmp(var, \"gpg.program\")) {\n \t\tif (!value)\n \t\t\treturn config_error_nonbool(var);\n-- \n2.16.4\n\n"},{"id":"351616","messageId":"1f99ef66bc5e115ec06a3c456658ad54ce405ce3.1530616446.git.henning.schild@siemens.com","threadId":"48822","inReplyTo":"cover.1530616446.git.henning.schild@siemens.com","subject":"[PATCH 7/8] gpg-interface: introduce new signature format \"X509\" using gpgsm","fromName":"Henning Schild","fromEmail":"henning.schild@siemens.com","sentAt":"2018-07-03T12:38:19Z","receivedAt":"2018-07-03T12:53:44Z","isPatch":true,"sender":{"key":"henning.schild@siemens.com","avatar":"https://avatars.githubusercontent.com/u/7237877?v=4"},"body":"This commit allows git to create and check X509 type signatures using\ngpgsm.\n\nSigned-off-by: Henning Schild \n---\n Documentation/config.txt | 5 ++++-\n gpg-interface.c | 10 +++++++++-\n 2 files changed, 13 insertions(+), 2 deletions(-)\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex c88903399..337df6e48 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -1828,9 +1828,12 @@ gpg.program::\n \tsigned, and the program is expected to send the result to its\n \tstandard output.\n \n+gpg.programX509::\n+\tJust like gpg.program, here the default you override is \"`gpgsm`\".\n+\n gpg.format::\n \tSpecifies which key format to use when signing with `--gpg-sign`.\n-\tDefault is \"PGP\", that is also the only supported value.\n+\tDefault is \"PGP\" and another possible value is \"X509\".\n \n gui.commitMsgWidth::\n \tDefines how wide the commit message window is in the\ndiff --git a/gpg-interface.c b/gpg-interface.c\nindex aa747278e..85d721007 100644\n--- a/gpg-interface.c\n+++ b/gpg-interface.c\n@@ -16,13 +16,18 @@ struct gpg_format_data {\n \n #define PGP_SIGNATURE \"-----BEGIN PGP SIGNATURE-----\"\n #define PGP_MESSAGE \"-----BEGIN PGP MESSAGE-----\"\n+#define X509_SIGNATURE \"-----BEGIN SIGNED MESSAGE-----\"\n \n-enum gpgformats { PGP_FMT };\n+enum gpgformats { PGP_FMT, X509_FMT };\n struct gpg_format_data gpg_formats[] = {\n \t{ .format = \"PGP\", .program = \"gpg\",\n \t .extra_args_verify = { \"--keyid-format=long\", },\n \t .sigs = { PGP_SIGNATURE, PGP_MESSAGE, },\n \t},\n+\t{ .format = \"X509\", .program = \"gpgsm\",\n+\t .extra_args_verify = { NULL },\n+\t .sigs = {X509_SIGNATURE, NULL, }\n+\t},\n };\n static const char *gpg_format = \"PGP\";\n \n@@ -190,6 +195,9 @@ int git_gpg_config(const char *var, const char *value, void *cb)\n \tif (!strcmp(var, \"gpg.program\"))\n \t\treturn git_config_string(&gpg_formats[PGP_FMT].program, var,\n \t\t\t\t\t value);\n+\tif (!strcmp(var, \"gpg.programX509\"))\n+\t\treturn git_config_string(&gpg_formats[X509_FMT].program, var,\n+\t\t\t\t\t value);\n \treturn 0;\n }\n \n-- \n2.16.4\n\n"},{"id":"351617","messageId":"db46732d1cd09a71200e00c2d09605d5beaff364.1530616446.git.henning.schild@siemens.com","threadId":"48822","inReplyTo":"cover.1530616446.git.henning.schild@siemens.com","subject":"[PATCH 4/8] gpg-interface: introduce an abstraction for multiple gpg formats","fromName":"Henning Schild","fromEmail":"henning.schild@siemens.com","sentAt":"2018-07-03T12:38:16Z","receivedAt":"2018-07-03T12:57:36Z","isPatch":true,"sender":{"key":"henning.schild@siemens.com","avatar":"https://avatars.githubusercontent.com/u/7237877?v=4"},"body":"Create a struct that holds the format details for the supported formats.\nAt the moment that is still just \"PGP\". This commit prepares for the\nintroduction of more formats, that might use other programs and match\nother signatures.\n\nSigned-off-by: Henning Schild \n---\n gpg-interface.c | 80 +++++++++++++++++++++++++++++++++++++++++++++------------\n 1 file changed, 64 insertions(+), 16 deletions(-)\n\ndiff --git a/gpg-interface.c b/gpg-interface.c\nindex 1def1f131..cd3b1b568 100644\n--- a/gpg-interface.c\n+++ b/gpg-interface.c\n@@ -7,12 +7,46 @@\n #include \"tempfile.h\"\n \n static char *configured_signing_key;\n-static const char *gpg_format = \"PGP\";\n-static const char *gpg_program = \"gpg\";\n+struct gpg_format_data {\n+\tconst char *format;\n+\tconst char *program;\n+\tconst char *extra_args_verify[1];\n+\tconst char *sigs[2];\n+};\n \n #define PGP_SIGNATURE \"-----BEGIN PGP SIGNATURE-----\"\n #define PGP_MESSAGE \"-----BEGIN PGP MESSAGE-----\"\n \n+enum gpgformats { PGP_FMT };\n+struct gpg_format_data gpg_formats[] = {\n+\t{ .format = \"PGP\", .program = \"gpg\",\n+\t .extra_args_verify = { \"--keyid-format=long\", },\n+\t .sigs = { PGP_SIGNATURE, PGP_MESSAGE, },\n+\t},\n+};\n+static const char *gpg_format = \"PGP\";\n+\n+static struct gpg_format_data *get_format_data(void)\n+{\n+\tint i;\n+\tfor (i = 0; i < ARRAY_SIZE(gpg_formats); i++)\n+\t\tif (!strcmp(gpg_formats[i].format, gpg_format))\n+\t\t\treturn gpg_formats + i;\n+\tassert(0);\n+}\n+\n+static struct gpg_format_data *get_format_data_by_sig(const char *sig)\n+{\n+\tint i, j;\n+\tfor (i = 0; i < ARRAY_SIZE(gpg_formats); i++)\n+\t\tfor (j = 0; j < ARRAY_SIZE(gpg_formats[i].sigs); j++)\n+\t\t\tif (gpg_formats[i].sigs[j] && \n+\t\t\t !strncmp(gpg_formats[i].sigs[j], sig,\n+\t\t\t\t strlen(gpg_formats[i].sigs[j])))\n+\t\t\t\treturn gpg_formats + i;\n+\treturn NULL;\n+}\n+\n void signature_check_clear(struct signature_check *sigc)\n {\n \tFREE_AND_NULL(sigc->payload);\n@@ -104,8 +138,7 @@ void print_signature_buffer(const struct signature_check *sigc, unsigned flags)\n \n static int is_gpg_start(const char *line)\n {\n-\treturn starts_with(line, PGP_SIGNATURE) ||\n-\t\tstarts_with(line, PGP_MESSAGE);\n+\treturn (get_format_data_by_sig(line) != NULL);\n }\n \n size_t parse_signature(const char *buf, size_t size)\n@@ -132,6 +165,8 @@ void set_signing_key(const char *key)\n \n int git_gpg_config(const char *var, const char *value, void *cb)\n {\n+\tint i, j;\n+\n \tif (!strcmp(var, \"user.signingkey\")) {\n \t\tif (!value)\n \t\t\treturn config_error_nonbool(var);\n@@ -140,18 +175,20 @@ int git_gpg_config(const char *var, const char *value, void *cb)\n \t}\n \n \tif (!strcmp(var, \"gpg.format\")) {\n-\t\tif (!strcmp(value, \"PGP\"))\n+\t\tj = 0;\n+\t\tfor (i = 0; i < ARRAY_SIZE(gpg_formats); i++)\n+\t\t\tif (!strcmp(value, gpg_formats[i].format)) {\n+\t\t\t\tj++;\n+\t\t\t\tbreak;\n+\t\t\t}\n+\t\tif (!j)\n \t\t\treturn error(\"malformed value for %s: %s\", var, value);\n \t\treturn git_config_string(&gpg_format, var, value);\n \t}\n \n-\tif (!strcmp(var, \"gpg.program\")) {\n-\t\tif (!value)\n-\t\t\treturn config_error_nonbool(var);\n-\t\tgpg_program = xstrdup(value);\n-\t\treturn 0;\n-\t}\n-\n+\tif (!strcmp(var, \"gpg.program\"))\n+\t\treturn git_config_string(&gpg_formats[PGP_FMT].program, var,\n+\t\t\t\t\t value);\n \treturn 0;\n }\n \n@@ -165,12 +202,14 @@ const char *get_signing_key(void)\n int sign_buffer(struct strbuf *buffer, struct strbuf *signature, const char *signing_key)\n {\n \tstruct child_process gpg = CHILD_PROCESS_INIT;\n+\tstruct gpg_format_data *fmt;\n \tint ret;\n \tsize_t i, j, bottom;\n \tstruct strbuf gpg_status = STRBUF_INIT;\n \n+\tfmt = get_format_data();\n \targv_array_pushl(&gpg.args,\n-\t\t\t gpg_program,\n+\t\t\t fmt->program,\n \t\t\t \"--status-fd=2\",\n \t\t\t \"-bsau\", signing_key,\n \t\t\t NULL);\n@@ -208,8 +247,9 @@ int verify_signed_buffer(const char *payload, size_t payload_size,\n \t\t\t struct strbuf *gpg_output, struct strbuf *gpg_status)\n {\n \tstruct child_process gpg = CHILD_PROCESS_INIT;\n+\tstruct gpg_format_data *fmt;\n \tstruct tempfile *temp;\n-\tint ret;\n+\tint ret, i;\n \tstruct strbuf buf = STRBUF_INIT;\n \n \ttemp = mks_tempfile_t(\".git_vtag_tmpXXXXXX\");\n@@ -223,10 +263,18 @@ int verify_signed_buffer(const char *payload, size_t payload_size,\n \t\treturn -1;\n \t}\n \n+\tfmt = get_format_data_by_sig(signature);\n+\tassert(fmt);\n+\n+\targv_array_pushl(&gpg.args,\n+\t\t\t fmt->program, NULL);\n+\tfor (i = 0; i < ARRAY_SIZE(fmt->extra_args_verify); i++)\n+\t\tif (fmt->extra_args_verify[i])\n+\t\t\targv_array_pushl(&gpg.args,\n+\t\t\t\t\t fmt->extra_args_verify[i], NULL);\n+\n \targv_array_pushl(&gpg.args,\n-\t\t\t gpg_program,\n \t\t\t \"--status-fd=1\",\n-\t\t\t \"--keyid-format=long\",\n \t\t\t \"--verify\", temp->filename.buf, \"-\",\n \t\t\t NULL);\n \n-- \n2.16.4\n\n"},{"id":"351618","messageId":"9ad3e4b6adb38440d5766e34e3a41b7f64281dca.1530616446.git.henning.schild@siemens.com","threadId":"48822","inReplyTo":"cover.1530616446.git.henning.schild@siemens.com","subject":"[PATCH 8/8] gpg-interface t: extend the existing GPG tests with GPGSM","fromName":"Henning Schild","fromEmail":"henning.schild@siemens.com","sentAt":"2018-07-03T12:38:20Z","receivedAt":"2018-07-03T12:59:01Z","isPatch":true,"sender":{"key":"henning.schild@siemens.com","avatar":"https://avatars.githubusercontent.com/u/7237877?v=4"},"body":"Add test cases to cover the new X509/gpgsm support. Most of them\nresemble existing ones. They just switch the format to X509 and set the\nsigningkey when creating signatures. Validation of signatures does not\nneed any configuration of git, it does need gpgsm to be configured to\ntrust the key(-chain).\nWe generate a self-signed key for committer@example.com and configure\ngpgsm to trust it.\n\nSigned-off-by: Henning Schild \n---\n t/lib-gpg.sh | 9 ++++++-\n t/lib-gpg/gpgsm-gen-key.in | 6 +++++\n t/t4202-log.sh | 66 ++++++++++++++++++++++++++++++++++++++++++++++\n t/t5534-push-signed.sh | 52 ++++++++++++++++++++++++++++++++++++\n t/t7003-filter-branch.sh | 15 +++++++++++\n t/t7030-verify-tag.sh | 47 +++++++++++++++++++++++++++++++--\n t/t7600-merge.sh | 31 ++++++++++++++++++++++\n 7 files changed, 223 insertions(+), 3 deletions(-)\n create mode 100644 t/lib-gpg/gpgsm-gen-key.in\n\ndiff --git a/t/lib-gpg.sh b/t/lib-gpg.sh\nindex a5d3b2cba..9dcb4e990 100755\n--- a/t/lib-gpg.sh\n+++ b/t/lib-gpg.sh\n@@ -38,7 +38,14 @@ then\n \t\t\t\"$TEST_DIRECTORY\"/lib-gpg/ownertrust &&\n \t\tgpg --homedir \"${GNUPGHOME}\" /dev/null 2>&1 \\\n \t\t\t--sign -u committer@example.com &&\n-\t\ttest_set_prereq GPG\n+\t\ttest_set_prereq GPG &&\n+\t\techo | gpgsm --homedir \"${GNUPGHOME}\" -o \"$TEST_DIRECTORY\"/lib-gpg/gpgsm.crt.user --passphrase-fd 0 --pinentry-mode loopback --generate-key --batch \"$TEST_DIRECTORY\"/lib-gpg/gpgsm-gen-key.in &&\n+\t\tgpgsm --homedir \"${GNUPGHOME}\" --import \"$TEST_DIRECTORY\"/lib-gpg/gpgsm.crt.user &&\n+\t\tgpgsm --homedir \"${GNUPGHOME}\" -K | grep fingerprint: | cut -d\" \" -f4 | tr -d '\\n' > ${GNUPGHOME}/trustlist.txt &&\n+\t\techo \" S relax\" >> ${GNUPGHOME}/trustlist.txt &&\n+\t\t(gpgconf --kill gpg-agent >/dev/null 2>&1 || : ) &&\n+\t\techo hello | gpgsm --homedir \"${GNUPGHOME}\" -u committer@example.com -o /dev/null --sign - 2>&1 &&\n+\t\ttest_set_prereq GPGSM\n \t\t;;\n \tesac\n fi\ndiff --git a/t/lib-gpg/gpgsm-gen-key.in b/t/lib-gpg/gpgsm-gen-key.in\nnew file mode 100644\nindex 000000000..3470b9dc7\n--- /dev/null\n+++ b/t/lib-gpg/gpgsm-gen-key.in\n@@ -0,0 +1,6 @@\n+Key-Type: RSA\n+Key-Length: 2048\n+Key-Usage: sign\n+Serial: random\n+Name-DN: CN=C O Mitter, O=Example, SN=C O, GN=Mitter\n+Name-Email: committer@example.com\ndiff --git a/t/t4202-log.sh b/t/t4202-log.sh\nindex 25b1f8cc7..a2f234053 100755\n--- a/t/t4202-log.sh\n+++ b/t/t4202-log.sh\n@@ -1556,12 +1556,30 @@ test_expect_success GPG 'setup signed branch' '\n \tgit commit -S -m signed_commit\n '\n \n+test_expect_success GPGSM 'setup signed branch x509' '\n+\ttest_when_finished \"git reset --hard && git checkout master\" &&\n+\tgit checkout -b signed-x509 master &&\n+\techo foo >foo &&\n+\tgit add foo &&\n+\tgit config gpg.format X509 &&\n+\tgit config user.signingkey $GIT_COMMITTER_EMAIL &&\n+\tgit commit -S -m signed_commit &&\n+\tgit config --unset gpg.format &&\n+\tgit config --unset user.signingkey\n+'\n+\n test_expect_success GPG 'log --graph --show-signature' '\n \tgit log --graph --show-signature -n1 signed >actual &&\n \tgrep \"^| gpg: Signature made\" actual &&\n \tgrep \"^| gpg: Good signature\" actual\n '\n \n+test_expect_success GPGSM 'log --graph --show-signature x509' '\n+\tgit log --graph --show-signature -n1 signed-x509 >actual &&\n+\tgrep \"^| gpgsm: Signature made\" actual &&\n+\tgrep \"^| gpgsm: Good signature\" actual\n+'\n+\n test_expect_success GPG 'log --graph --show-signature for merged tag' '\n \ttest_when_finished \"git reset --hard && git checkout master\" &&\n \tgit checkout -b plain master &&\n@@ -1581,11 +1599,39 @@ test_expect_success GPG 'log --graph --show-signature for merged tag' '\n \tgrep \"^| | gpg: Good signature\" actual\n '\n \n+test_expect_success GPGSM 'log --graph --show-signature for merged tag x509' '\n+\ttest_when_finished \"git reset --hard && git checkout master\" &&\n+\tgit config gpg.format X509 &&\n+\tgit config user.signingkey $GIT_COMMITTER_EMAIL &&\n+\tgit checkout -b plain-x509 master &&\n+\techo aaa >bar &&\n+\tgit add bar &&\n+\tgit commit -m bar_commit &&\n+\tgit checkout -b tagged-x509 master &&\n+\techo bbb >baz &&\n+\tgit add baz &&\n+\tgit commit -m baz_commit &&\n+\tgit tag -s -m signed_tag_msg signed_tag_x509 &&\n+\tgit checkout plain-x509 &&\n+\tgit merge --no-ff -m msg signed_tag_x509 &&\n+\tgit log --graph --show-signature -n1 plain-x509 >actual &&\n+\tgrep \"^|\\\\\\ merged tag\" actual &&\n+\tgrep \"^| | gpgsm: Signature made\" actual &&\n+\tgrep \"^| | gpgsm: Good signature\" actual &&\n+\tgit config --unset gpg.format &&\n+\tgit config --unset user.signingkey\n+'\n+\n test_expect_success GPG '--no-show-signature overrides --show-signature' '\n \tgit log -1 --show-signature --no-show-signature signed >actual &&\n \t! grep \"^gpg:\" actual\n '\n \n+test_expect_success GPGSM '--no-show-signature overrides --show-signature x509' '\n+\tgit log -1 --show-signature --no-show-signature signed-x509 >actual &&\n+\t! grep \"^gpgsm:\" actual\n+'\n+\n test_expect_success GPG 'log.showsignature=true behaves like --show-signature' '\n \ttest_config log.showsignature true &&\n \tgit log -1 signed >actual &&\n@@ -1593,12 +1639,25 @@ test_expect_success GPG 'log.showsignature=true behaves like --show-signature' '\n \tgrep \"gpg: Good signature\" actual\n '\n \n+test_expect_success GPGSM 'log.showsignature=true behaves like --show-signature x509' '\n+\ttest_config log.showsignature true &&\n+\tgit log -1 signed-x509 >actual &&\n+\tgrep \"gpgsm: Signature made\" actual &&\n+\tgrep \"gpgsm: Good signature\" actual\n+'\n+\n test_expect_success GPG '--no-show-signature overrides log.showsignature=true' '\n \ttest_config log.showsignature true &&\n \tgit log -1 --no-show-signature signed >actual &&\n \t! grep \"^gpg:\" actual\n '\n \n+test_expect_success GPGSM '--no-show-signature overrides log.showsignature=true x509' '\n+\ttest_config log.showsignature true &&\n+\tgit log -1 --no-show-signature signed-x509 >actual &&\n+\t! grep \"^gpgsm:\" actual\n+'\n+\n test_expect_success GPG '--show-signature overrides log.showsignature=false' '\n \ttest_config log.showsignature false &&\n \tgit log -1 --show-signature signed >actual &&\n@@ -1606,6 +1665,13 @@ test_expect_success GPG '--show-signature overrides log.showsignature=false' '\n \tgrep \"gpg: Good signature\" actual\n '\n \n+test_expect_success GPGSM '--show-signature overrides log.showsignature=false x509' '\n+\ttest_config log.showsignature false &&\n+\tgit log -1 --show-signature signed-x509 >actual &&\n+\tgrep \"gpgsm: Signature made\" actual &&\n+\tgrep \"gpgsm: Good signature\" actual\n+'\n+\n test_expect_success 'log --graph --no-walk is forbidden' '\n \ttest_must_fail git log --graph --no-walk\n '\ndiff --git a/t/t5534-push-signed.sh b/t/t5534-push-signed.sh\nindex 1cea758f7..9d5029fcf 100755\n--- a/t/t5534-push-signed.sh\n+++ b/t/t5534-push-signed.sh\n@@ -218,4 +218,56 @@ test_expect_success GPG 'fail without key and heed user.signingkey' '\n \ttest_cmp expect dst/push-cert-status\n '\n \n+test_expect_success GPGSM 'fail without key and heed user.signingkey x509' '\n+\tgit config gpg.format X509 &&\n+\tenv | grep GIT > envfile &&\n+\tprepare_dst &&\n+\tmkdir -p dst/.git/hooks &&\n+\tgit -C dst config receive.certnonceseed sekrit &&\n+\twrite_script dst/.git/hooks/post-receive <<-\\EOF &&\n+\t# discard the update list\n+\tcat >/dev/null\n+\t# record the push certificate\n+\tif test -n \"${GIT_PUSH_CERT-}\"\n+\tthen\n+\t\tgit cat-file blob $GIT_PUSH_CERT >../push-cert\n+\tfi &&\n+\n+\tcat >../push-cert-status <expect.in &&\n+\tkey=$(cat \"${GNUPGHOME}/trustlist.txt\" | cut -d\" \" -f1 | tr -d \":\") &&\n+\tsed -e \"s/^KEY=/KEY=${key}/\" expect.in > expect &&\n+\n+\tnoop=$(git rev-parse noop) &&\n+\tff=$(git rev-parse ff) &&\n+\tnoff=$(git rev-parse noff) &&\n+\tgrep \"$noop $ff refs/heads/ff\" dst/push-cert &&\n+\tgrep \"$noop $noff refs/heads/noff\" dst/push-cert &&\n+\ttest_cmp expect dst/push-cert-status\n+'\n+\n+\n test_done\ndiff --git a/t/t7003-filter-branch.sh b/t/t7003-filter-branch.sh\nindex ec4b160dd..69b9e05d6 100755\n--- a/t/t7003-filter-branch.sh\n+++ b/t/t7003-filter-branch.sh\n@@ -309,6 +309,21 @@ test_expect_success GPG 'Filtering retains message of gpg signed commit' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success GPGSM 'Filtering retains message of gpgsm signed commit' '\n+\tmkdir gpgsm &&\n+\ttouch gpgsm/foo &&\n+\tgit add gpgsm &&\n+\tgit config gpg.format X509 &&\n+\tgit config user.signingkey $GIT_COMMITTER_EMAIL &&\n+\ttest_tick &&\n+\tgit commit -S -m \"Adding gpgsm\" &&\n+\n+\tgit log -1 --format=\"%s\" > expect &&\n+\tgit filter-branch -f --msg-filter \"cat\" &&\n+\tgit log -1 --format=\"%s\" > actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_expect_success 'Tag name filtering allows slashes in tag names' '\n \tgit tag -m tag-with-slash X/1 &&\n \tgit cat-file tag X/1 | sed -e s,X/1,X/2, > expect &&\ndiff --git a/t/t7030-verify-tag.sh b/t/t7030-verify-tag.sh\nindex 291a1e2b0..a76bcf333 100755\n--- a/t/t7030-verify-tag.sh\n+++ b/t/t7030-verify-tag.sh\n@@ -41,6 +41,13 @@ test_expect_success GPG 'create signed tags' '\n \tgit tag -uB7227189 -m eighth eighth-signed-alt\n '\n \n+test_expect_success GPGSM 'create signed tags x509 ' '\n+\tgit config gpg.format X509 &&\n+\tgit config user.signingkey $GIT_COMMITTER_EMAIL &&\n+\techo 9 >file && test_tick && git commit -a -m \"nineth gpgsm-signed\" &&\n+\tgit tag -s -m nineth nineth-signed-x509\n+'\n+\n test_expect_success GPG 'verify and show signatures' '\n \t(\n \t\tfor tag in initial second merge fourth-signed sixth-signed seventh-signed\n@@ -72,6 +79,18 @@ test_expect_success GPG 'verify and show signatures' '\n \t)\n '\n \n+test_expect_success GPGSM 'verify and show signatures x509' '\n+\t(\n+\t\tfor tag in nineth-signed-x509\n+\t\tdo\n+\t\t\tgit verify-tag $tag 2>actual &&\n+\t\t\tgrep \"Good signature from\" actual &&\n+\t\t\t! grep \"BAD signature from\" actual &&\n+\t\t\techo $tag OK || exit 1\n+\t\tdone\n+\t)\n+'\n+\n test_expect_success GPG 'detect fudged signature' '\n \tgit cat-file tag seventh-signed >raw &&\n \tsed -e \"/^tag / s/seventh/7th forged/\" raw >forged1 &&\n@@ -112,8 +131,32 @@ test_expect_success GPG 'verify signatures with --raw' '\n \t)\n '\n \n-test_expect_success GPG 'verify multiple tags' '\n-\ttags=\"fourth-signed sixth-signed seventh-signed\" &&\n+test_expect_success GPGSM 'verify signatures with --raw x509' '\n+\t(\n+\t\tfor tag in nineth-signed-x509\n+\t\tdo\n+\t\t\tgit verify-tag --raw $tag 2>actual &&\n+\t\t\tgrep \"GOODSIG\" actual &&\n+\t\t\t! grep \"BADSIG\" actual &&\n+\t\t\techo $tag OK || exit 1\n+\t\tdone\n+\t)\n+'\n+test_expect_success GPGSM 'verify multiple tags' '\n+\ttags=\"fourth-signed sixth-signed seventh-signed nineth-signed-x509\" &&\n+\tfor i in $tags\n+\tdo\n+\t\tgit verify-tag -v --raw $i || return 1\n+\tdone >expect.stdout 2>expect.stderr.1 &&\n+\tgrep \"^.GNUPG:.\" expect.stderr &&\n+\tgit verify-tag -v --raw $tags >actual.stdout 2>actual.stderr.1 &&\n+\tgrep \"^.GNUPG:.\" actual.stderr &&\n+\ttest_cmp expect.stdout actual.stdout &&\n+\ttest_cmp expect.stderr actual.stderr\n+'\n+\n+test_expect_success GPGSM 'verify multiple tags x509' '\n+\ttags=\"fourth-signed sixth-signed seventh-signed nineth-signed-x509\" &&\n \tfor i in $tags\n \tdo\n \t\tgit verify-tag -v --raw $i || return 1\ndiff --git a/t/t7600-merge.sh b/t/t7600-merge.sh\nindex 6736d8d13..ef742aa16 100755\n--- a/t/t7600-merge.sh\n+++ b/t/t7600-merge.sh\n@@ -747,6 +747,21 @@ test_expect_success GPG 'merge --ff-only tag' '\n \tgit rev-parse HEAD >actual &&\n \ttest_cmp expect actual\n '\n+test_expect_success GPGSM 'merge --ff-only tag x509' '\n+\tgit reset --hard c0 &&\n+\tgit commit --allow-empty -m \"A newer commit\" &&\n+\tgit config gpg.format X509 &&\n+\tgit config user.signingkey $GIT_COMMITTER_EMAIL &&\n+\tgit tag -s -m \"A newer commit\" signed-x509 &&\n+\tgit reset --hard c0 &&\n+\n+\tgit merge --ff-only signed &&\n+\tgit rev-parse signed^0 >expect &&\n+\tgit rev-parse HEAD >actual &&\n+\ttest_cmp expect actual &&\n+\tgit config --unset gpg.format &&\n+\tgit config --unset user.signingkey\n+'\n \n test_expect_success GPG 'merge --no-edit tag should skip editor' '\n \tgit reset --hard c0 &&\n@@ -760,6 +775,22 @@ test_expect_success GPG 'merge --no-edit tag should skip editor' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success GPGSM 'merge --no-edit tag should skip editor x509' '\n+\tgit reset --hard c0 &&\n+\tgit commit --allow-empty -m \"A newer commit\" &&\n+\tgit config gpg.format X509 &&\n+\tgit config user.signingkey $GIT_COMMITTER_EMAIL &&\n+\tgit tag -f -s -m \"A newer commit\" signed &&\n+\tgit reset --hard c0 &&\n+\n+\tEDITOR=false git merge --no-edit --no-ff signed-x509 &&\n+\tgit rev-parse signed^0 >expect &&\n+\tgit rev-parse HEAD^2 >actual &&\n+\ttest_cmp expect actual &&\n+\tgit config --unset gpg.format &&\n+\tgit config --unset user.signingkey\n+'\n+\n test_expect_success 'set up mod-256 conflict scenario' '\n \t# 256 near-identical stanzas...\n \tfor i in $(test_seq 1 256); do\n-- \n2.16.4\n\n"},{"id":"351680","messageId":"CAN0heSrXpLCDRjnZC80QXBG27gd6m5reBn1hfNd_KXxnPVkA2g@mail.gmail.com","threadId":"48822","inReplyTo":"db46732d1cd09a71200e00c2d09605d5beaff364.1530616446.git.henning.schild@siemens.com","subject":"Re: [PATCH 4/8] gpg-interface: introduce an abstraction for multiple gpg formats","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2018-07-04T07:10:17Z","receivedAt":"2018-07-04T07:11:36Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"Hi Henning,\n\nOn 3 July 2018 at 14:38, Henning Schild wrote:\n> Create a struct that holds the format details for the supported formats.\n> At the moment that is still just \"PGP\". This commit prepares for the\n> introduction of more formats, that might use other programs and match\n> other signatures.\n>\n> Signed-off-by: Henning Schild \n\nWelcome to the mailing list! :-)\n\nI'll just comment on a few thoughts I had while skimming this.\n\n> static char *configured_signing_key;\n> -static const char *gpg_format = \"PGP\";\n> -static const char *gpg_program = \"gpg\";\n> +struct gpg_format_data {\n> + const char *format;\n> + const char *program;\n> + const char *extra_args_verify[1];\n> + const char *sigs[2];\n> +};\n>\n> #define PGP_SIGNATURE \"-----BEGIN PGP SIGNATURE-----\"\n> #define PGP_MESSAGE \"-----BEGIN PGP MESSAGE-----\"\n>\n> +enum gpgformats { PGP_FMT };\n> +struct gpg_format_data gpg_formats[] = {\n> + { .format = \"PGP\", .program = \"gpg\",\n> + .extra_args_verify = { \"--keyid-format=long\", },\n> + .sigs = { PGP_SIGNATURE, PGP_MESSAGE, },\n> + },\n> +};\n\nI think those trailing commas are ok now, but I'm not sure...\n\nI had the same thought about designated initializers. Those should be ok\nnow, c.f. cbc0f81d96 (strbuf: use designated initializers in STRBUF_INIT,\n2017-07-10) and a73b3680c4 (Add and use generic name->id mapping code\nfor color slot parsing, 2018-05-26).\n\n> +static const char *gpg_format = \"PGP\";\n> +\n> +static struct gpg_format_data *get_format_data(void)\n> +{\n> + int i;\n> + for (i = 0; i < ARRAY_SIZE(gpg_formats); i++)\n> + if (!strcmp(gpg_formats[i].format, gpg_format))\n> + return gpg_formats + i;\n> + assert(0);\n\nThis might be better written as `BUG(\"bad gpg_format '%s'\",\ngpg_format);` or something like that.\n\n(It's not supposed to be triggered, not even by invalid data from the\nuser, right?)\n\n> if (!strcmp(var, \"gpg.format\")) {\n> - if (!strcmp(value, \"PGP\"))\n\nThis line was added in patch 3. It errors out precisely when gpg.format\nis \"PGP\", no? That this doesn't break the whole series is explained by\n1) it being removed in this patch 4, and 2) there being no tests. It\nmakes me wonder if something like patch 5 (test gpg.format) could be\npart of patch 3, both with negative (\"= malformed\") and positive (\"=\nPGP\") tests?\n\n> + j = 0;\n> + for (i = 0; i < ARRAY_SIZE(gpg_formats); i++)\n> + if (!strcmp(value, gpg_formats[i].format)) {\n> + j++;\n> + break;\n> + }\n> + if (!j)\n> return error(\"malformed value for %s: %s\", var, value);\n\n`if (i == ARRAY_SIZE(gpg_formats))` and drop `j`?\n\nOr check whether `get_format_data()` returns NULL? Hmm, well you can't,\nsince it takes its \"input\" from a global variable...\n\nIf you want to keep that global nature, the duplication of search-logic\ncould perhaps be avoided by having a helper function for returning the\nindex of a gpg_format (or -1).\n\n> return git_config_string(&gpg_format, var, value);\n> }\n\nMartin\n"},{"id":"351703","messageId":"20180705152158.6f661c85@md1pvb1c.ad001.siemens.net","threadId":"48822","inReplyTo":"CAN0heSrXpLCDRjnZC80QXBG27gd6m5reBn1hfNd_KXxnPVkA2g@mail.gmail.com","subject":"Re: [PATCH 4/8] gpg-interface: introduce an abstraction for multiple gpg formats","fromName":"Henning Schild","fromEmail":"henning.schild@siemens.com","sentAt":"2018-07-05T13:21:58Z","receivedAt":"2018-07-05T13:22:32Z","isPatch":true,"sender":{"key":"henning.schild@siemens.com","avatar":"https://avatars.githubusercontent.com/u/7237877?v=4"},"body":"Am Wed, 4 Jul 2018 09:10:17 +0200\nschrieb Martin Ågren :\n\n> Hi Henning,\n> \n> On 3 July 2018 at 14:38, Henning Schild \n> wrote:\n> > Create a struct that holds the format details for the supported\n> > formats. At the moment that is still just \"PGP\". This commit\n> > prepares for the introduction of more formats, that might use other\n> > programs and match other signatures.\n> >\n> > Signed-off-by: Henning Schild \n> \n> Welcome to the mailing list! :-)\n\nThanks!\n\n> I'll just comment on a few thoughts I had while skimming this.\n> \n> > static char *configured_signing_key;\n> > -static const char *gpg_format = \"PGP\";\n> > -static const char *gpg_program = \"gpg\";\n> > +struct gpg_format_data {\n> > + const char *format;\n> > + const char *program;\n> > + const char *extra_args_verify[1];\n> > + const char *sigs[2];\n> > +};\n> >\n> > #define PGP_SIGNATURE \"-----BEGIN PGP SIGNATURE-----\"\n> > #define PGP_MESSAGE \"-----BEGIN PGP MESSAGE-----\"\n> >\n> > +enum gpgformats { PGP_FMT };\n> > +struct gpg_format_data gpg_formats[] = {\n> > + { .format = \"PGP\", .program = \"gpg\",\n> > + .extra_args_verify = { \"--keyid-format=long\", },\n> > + .sigs = { PGP_SIGNATURE, PGP_MESSAGE, },\n> > + },\n> > +}; \n> \n> I think those trailing commas are ok now, but I'm not sure...\n> \n> I had the same thought about designated initializers. Those should be\n> ok now, c.f. cbc0f81d96 (strbuf: use designated initializers in\n> STRBUF_INIT, 2017-07-10) and a73b3680c4 (Add and use generic name->id\n> mapping code for color slot parsing, 2018-05-26).\n\nOk, i did not actually check coding style yet. I could run it through a\ntool, given there is a suggestion. Or i could address issues someone\npoints out in the review.\nWhat i get from your comment is that it might be ok to leave the code\nas is, others have introduces similar constructs before me.\n\n> > +static const char *gpg_format = \"PGP\";\n> > +\n> > +static struct gpg_format_data *get_format_data(void)\n> > +{\n> > + int i;\n> > + for (i = 0; i < ARRAY_SIZE(gpg_formats); i++)\n> > + if (!strcmp(gpg_formats[i].format, gpg_format))\n> > + return gpg_formats + i;\n> > + assert(0); \n> \n> This might be better written as `BUG(\"bad gpg_format '%s'\",\n> gpg_format);` or something like that.\n> \n> (It's not supposed to be triggered, not even by invalid data from the\n> user, right?)\n\nYes that is code that can not (should not) be reached. I agree that an\nassert(0) is not very expressive and will fix that in v2.\n\n> \n> > if (!strcmp(var, \"gpg.format\")) {\n> > - if (!strcmp(value, \"PGP\")) \n> \n> This line was added in patch 3. It errors out precisely when\n> gpg.format is \"PGP\", no? That this doesn't break the whole series is\n> explained by 1) it being removed in this patch 4, and 2) there being\n> no tests. It makes me wonder if something like patch 5 (test\n> gpg.format) could be part of patch 3, both with negative (\"=\n> malformed\") and positive (\"= PGP\") tests?\n\nI will pull the tests from patch 5 before touching that code and fix up\nissues inbetween. The whole series saw a \"make test\" inbetween all\ncommits.\n\n> > + j = 0;\n> > + for (i = 0; i < ARRAY_SIZE(gpg_formats); i++)\n> > + if (!strcmp(value, gpg_formats[i].format)) {\n> > + j++;\n> > + break;\n> > + }\n> > + if (!j)\n> > return error(\"malformed value for %s: %s\",\n> > var, value); \n> \n> `if (i == ARRAY_SIZE(gpg_formats))` and drop `j`?\n> \n> Or check whether `get_format_data()` returns NULL? Hmm, well you\n> can't, since it takes its \"input\" from a global variable...\n> \n> If you want to keep that global nature, the duplication of\n> search-logic could perhaps be avoided by having a helper function for\n> returning the index of a gpg_format (or -1).\n\nTrue, the two are almost the same and should be merged. Will do in v2.\n\nHenning\n\n> > return git_config_string(&gpg_format, var, value);\n> > } \n> \n> Martin\n\n"},{"id":"351742","messageId":"20180706010148.GA7697@genre.crustytoothpaste.net","threadId":"48822","inReplyTo":"9bb55c019c3bc025d77d9d491818e442233bf8d3.1530616446.git.henning.schild@siemens.com","subject":"Re: [PATCH 3/8] gpg-interface: add new config to select how to sign a commit","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2018-07-06T01:01:48Z","receivedAt":"2018-07-06T01:02:00Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On Tue, Jul 03, 2018 at 02:38:15PM +0200, Henning Schild wrote:\n> Add \"gpg.format\" where the user can specify which type of signature to\n> use for commits. At the moment only \"PGP\" is supported and the value is\n> not even used. This commit prepares for a new types of signatures.\n\nWe typically prefer to have option values specified in lower case. I\nalso think \"openpgp\" might be better than \"PGP\", since that's the name\nof the specification and it would avoid any potential unhappiness about\ncompatibility with PGP or trademarks.\n-- \nbrian m. carlson: Houston, Texas, US\nOpenPGP: https://keybase.io/bk2204\n"},{"id":"351743","messageId":"20180706011013.GB7697@genre.crustytoothpaste.net","threadId":"48822","inReplyTo":"1f99ef66bc5e115ec06a3c456658ad54ce405ce3.1530616446.git.henning.schild@siemens.com","subject":"Re: [PATCH 7/8] gpg-interface: introduce new signature format \"X509\" using gpgsm","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2018-07-06T01:10:13Z","receivedAt":"2018-07-06T01:10:23Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On Tue, Jul 03, 2018 at 02:38:19PM +0200, Henning Schild wrote:\n> This commit allows git to create and check X509 type signatures using\n> gpgsm.\n> \n> Signed-off-by: Henning Schild \n> ---\n> Documentation/config.txt | 5 ++++-\n> gpg-interface.c | 10 +++++++++-\n> 2 files changed, 13 insertions(+), 2 deletions(-)\n> \n> diff --git a/Documentation/config.txt b/Documentation/config.txt\n> index c88903399..337df6e48 100644\n> --- a/Documentation/config.txt\n> +++ b/Documentation/config.txt\n> @@ -1828,9 +1828,12 @@ gpg.program::\n> \tsigned, and the program is expected to send the result to its\n> \tstandard output.\n> \n> +gpg.programX509::\n\nI'm not super excited about this name. It seems to indicate we want a\nlevel of hierarchy involved.\n\nA hierarchy like sign.openpgp.program (falling back to gpg.program) and\nsign.x509.program might be more logical.\n\n> diff --git a/gpg-interface.c b/gpg-interface.c\n> index aa747278e..85d721007 100644\n> --- a/gpg-interface.c\n> +++ b/gpg-interface.c\n> @@ -16,13 +16,18 @@ struct gpg_format_data {\n> \n> #define PGP_SIGNATURE \"-----BEGIN PGP SIGNATURE-----\"\n> #define PGP_MESSAGE \"-----BEGIN PGP MESSAGE-----\"\n> +#define X509_SIGNATURE \"-----BEGIN SIGNED MESSAGE-----\"\n> \n> -enum gpgformats { PGP_FMT };\n> +enum gpgformats { PGP_FMT, X509_FMT };\n> struct gpg_format_data gpg_formats[] = {\n> \t{ .format = \"PGP\", .program = \"gpg\",\n> \t .extra_args_verify = { \"--keyid-format=long\", },\n> \t .sigs = { PGP_SIGNATURE, PGP_MESSAGE, },\n> \t},\n> +\t{ .format = \"X509\", .program = \"gpgsm\",\n\nSimilarly to my comment about \"PGP\", I think this would do well as\n\"x509\".\n-- \nbrian m. carlson: Houston, Texas, US\nOpenPGP: https://keybase.io/bk2204\n"},{"id":"351744","messageId":"20180706011447.GC7697@genre.crustytoothpaste.net","threadId":"48822","inReplyTo":"9ad3e4b6adb38440d5766e34e3a41b7f64281dca.1530616446.git.henning.schild@siemens.com","subject":"Re: [PATCH 8/8] gpg-interface t: extend the existing GPG tests with GPGSM","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2018-07-06T01:14:47Z","receivedAt":"2018-07-06T01:15:03Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On Tue, Jul 03, 2018 at 02:38:20PM +0200, Henning Schild wrote:\n> Add test cases to cover the new X509/gpgsm support. Most of them\n> resemble existing ones. They just switch the format to X509 and set the\n> signingkey when creating signatures. Validation of signatures does not\n> need any configuration of git, it does need gpgsm to be configured to\n> trust the key(-chain).\n> We generate a self-signed key for committer@example.com and configure\n> gpgsm to trust it.\n> \n> Signed-off-by: Henning Schild \n> ---\n> t/lib-gpg.sh | 9 ++++++-\n> t/lib-gpg/gpgsm-gen-key.in | 6 +++++\n> t/t4202-log.sh | 66 ++++++++++++++++++++++++++++++++++++++++++++++\n> t/t5534-push-signed.sh | 52 ++++++++++++++++++++++++++++++++++++\n> t/t7003-filter-branch.sh | 15 +++++++++++\n> t/t7030-verify-tag.sh | 47 +++++++++++++++++++++++++++++++--\n> t/t7600-merge.sh | 31 ++++++++++++++++++++++\n> 7 files changed, 223 insertions(+), 3 deletions(-)\n> create mode 100644 t/lib-gpg/gpgsm-gen-key.in\n> \n> diff --git a/t/lib-gpg.sh b/t/lib-gpg.sh\n> index a5d3b2cba..9dcb4e990 100755\n> --- a/t/lib-gpg.sh\n> +++ b/t/lib-gpg.sh\n> @@ -38,7 +38,14 @@ then\n> \t\t\t\"$TEST_DIRECTORY\"/lib-gpg/ownertrust &&\n> \t\tgpg --homedir \"${GNUPGHOME}\" /dev/null 2>&1 \\\n> \t\t\t--sign -u committer@example.com &&\n> -\t\ttest_set_prereq GPG\n> +\t\ttest_set_prereq GPG &&\n> +\t\techo | gpgsm --homedir \"${GNUPGHOME}\" -o \"$TEST_DIRECTORY\"/lib-gpg/gpgsm.crt.user --passphrase-fd 0 --pinentry-mode loopback --generate-key --batch \"$TEST_DIRECTORY\"/lib-gpg/gpgsm-gen-key.in &&\n> +\t\tgpgsm --homedir \"${GNUPGHOME}\" --import \"$TEST_DIRECTORY\"/lib-gpg/gpgsm.crt.user &&\n> +\t\tgpgsm --homedir \"${GNUPGHOME}\" -K | grep fingerprint: | cut -d\" \" -f4 | tr -d '\\n' > ${GNUPGHOME}/trustlist.txt &&\n> +\t\techo \" S relax\" >> ${GNUPGHOME}/trustlist.txt &&\n> +\t\t(gpgconf --kill gpg-agent >/dev/null 2>&1 || : ) &&\n> +\t\techo hello | gpgsm --homedir \"${GNUPGHOME}\" -u committer@example.com -o /dev/null --sign - 2>&1 &&\n> +\t\ttest_set_prereq GPGSM\n\nIt looks like the GPGSM prerequisite will only be set if the GPG\nprerequisite is set as well. Do we want to consider the case when the\nuser might have gpgsm but not gpg?\n-- \nbrian m. carlson: Houston, Texas, US\nOpenPGP: https://keybase.io/bk2204\n"},{"id":"351745","messageId":"20180706011834.GD7697@genre.crustytoothpaste.net","threadId":"48822","inReplyTo":"cover.1530616446.git.henning.schild@siemens.com","subject":"Re: [PATCH 0/8] X509 (gpgsm) commit signing support","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2018-07-06T01:18:35Z","receivedAt":"2018-07-06T01:18:44Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On Tue, Jul 03, 2018 at 02:38:12PM +0200, Henning Schild wrote:\n> This series adds support for signing commits with gpgsm.\n> \n> The first two patches are cleanups of gpg-interface, while the next\n> four prepare for the introduction of the actual feature in patch 7.\n> Finally patch 8 extends the testsuite to cover the new feature.\n> \n> This series can be seen as a follow up of a series that appeared under\n> the name \"gpg-interface: Multiple signing tools\" in april 2018 [1]. After\n> that series was not merged i decided to get my patches ready. The\n> original series aimed at being generic for any sort of signing tool, while\n> this series just introduced the X509 variant of gpg. (gpgsm)\n> I collected authors and reviewers of that first series and already put them\n> on cc.\n\nOverall, I think this is heading in a good direction. I left a few\ncomments, but it seemed pretty sane.\n-- \nbrian m. carlson: Houston, Texas, US\nOpenPGP: https://keybase.io/bk2204\n"},{"id":"351764","messageId":"20180706100148.7b878808@md1pvb1c.ad001.siemens.net","threadId":"48822","inReplyTo":"20180706011834.GD7697@genre.crustytoothpaste.net","subject":"Re: [PATCH 0/8] X509 (gpgsm) commit signing support","fromName":"Henning Schild","fromEmail":"henning.schild@siemens.com","sentAt":"2018-07-06T08:01:48Z","receivedAt":"2018-07-06T08:02:25Z","isPatch":true,"sender":{"key":"henning.schild@siemens.com","avatar":"https://avatars.githubusercontent.com/u/7237877?v=4"},"body":"Am Fri, 6 Jul 2018 01:18:35 +0000\nschrieb \"brian m. carlson\" :\n\n> On Tue, Jul 03, 2018 at 02:38:12PM +0200, Henning Schild wrote:\n> > This series adds support for signing commits with gpgsm.\n> > \n> > The first two patches are cleanups of gpg-interface, while the next\n> > four prepare for the introduction of the actual feature in patch 7.\n> > Finally patch 8 extends the testsuite to cover the new feature.\n> > \n> > This series can be seen as a follow up of a series that appeared\n> > under the name \"gpg-interface: Multiple signing tools\" in april\n> > 2018 [1]. After that series was not merged i decided to get my\n> > patches ready. The original series aimed at being generic for any\n> > sort of signing tool, while this series just introduced the X509\n> > variant of gpg. (gpgsm) I collected authors and reviewers of that\n> > first series and already put them on cc. \n> \n> Overall, I think this is heading in a good direction. I left a few\n> comments, but it seemed pretty sane.\n\nThanks, i hope others think so too and that will eventually get merged.\n\nHenning\n"},{"id":"351765","messageId":"20180706100159.730054ce@md1pvb1c.ad001.siemens.net","threadId":"48822","inReplyTo":"20180706011013.GB7697@genre.crustytoothpaste.net","subject":"Re: [PATCH 7/8] gpg-interface: introduce new signature format \"X509\" using gpgsm","fromName":"Henning Schild","fromEmail":"henning.schild@siemens.com","sentAt":"2018-07-06T08:01:59Z","receivedAt":"2018-07-06T08:02:31Z","isPatch":true,"sender":{"key":"henning.schild@siemens.com","avatar":"https://avatars.githubusercontent.com/u/7237877?v=4"},"body":"Am Fri, 6 Jul 2018 01:10:13 +0000\nschrieb \"brian m. carlson\" :\n\n> On Tue, Jul 03, 2018 at 02:38:19PM +0200, Henning Schild wrote:\n> > This commit allows git to create and check X509 type signatures\n> > using gpgsm.\n> > \n> > Signed-off-by: Henning Schild \n> > ---\n> > Documentation/config.txt | 5 ++++-\n> > gpg-interface.c | 10 +++++++++-\n> > 2 files changed, 13 insertions(+), 2 deletions(-)\n> > \n> > diff --git a/Documentation/config.txt b/Documentation/config.txt\n> > index c88903399..337df6e48 100644\n> > --- a/Documentation/config.txt\n> > +++ b/Documentation/config.txt\n> > @@ -1828,9 +1828,12 @@ gpg.program::\n> > \tsigned, and the program is expected to send the result to\n> > its standard output.\n> > \n> > +gpg.programX509:: \n> \n> I'm not super excited about this name. It seems to indicate we want a\n> level of hierarchy involved.\n> \n> A hierarchy like sign.openpgp.program (falling back to gpg.program)\n> and sign.x509.program might be more logical.\n> \n> > diff --git a/gpg-interface.c b/gpg-interface.c\n> > index aa747278e..85d721007 100644\n> > --- a/gpg-interface.c\n> > +++ b/gpg-interface.c\n> > @@ -16,13 +16,18 @@ struct gpg_format_data {\n> > \n> > #define PGP_SIGNATURE \"-----BEGIN PGP SIGNATURE-----\"\n> > #define PGP_MESSAGE \"-----BEGIN PGP MESSAGE-----\"\n> > +#define X509_SIGNATURE \"-----BEGIN SIGNED MESSAGE-----\"\n> > \n> > -enum gpgformats { PGP_FMT };\n> > +enum gpgformats { PGP_FMT, X509_FMT };\n> > struct gpg_format_data gpg_formats[] = {\n> > \t{ .format = \"PGP\", .program = \"gpg\",\n> > \t .extra_args_verify = { \"--keyid-format=long\", },\n> > \t .sigs = { PGP_SIGNATURE, PGP_MESSAGE, },\n> > \t},\n> > +\t{ .format = \"X509\", .program = \"gpgsm\", \n> \n> Similarly to my comment about \"PGP\", I think this would do well as\n> \"x509\".\n\nAnother naming discussion, lets keep discussing and i will implement it\nonce settled.\n\nHenning\n"},{"id":"351766","messageId":"20180706100152.317da7f7@md1pvb1c.ad001.siemens.net","threadId":"48822","inReplyTo":"20180706011447.GC7697@genre.crustytoothpaste.net","subject":"Re: [PATCH 8/8] gpg-interface t: extend the existing GPG tests with GPGSM","fromName":"Henning Schild","fromEmail":"henning.schild@siemens.com","sentAt":"2018-07-06T08:01:52Z","receivedAt":"2018-07-06T08:02:33Z","isPatch":true,"sender":{"key":"henning.schild@siemens.com","avatar":"https://avatars.githubusercontent.com/u/7237877?v=4"},"body":"Am Fri, 6 Jul 2018 01:14:47 +0000\nschrieb \"brian m. carlson\" :\n\n> On Tue, Jul 03, 2018 at 02:38:20PM +0200, Henning Schild wrote:\n> > Add test cases to cover the new X509/gpgsm support. Most of them\n> > resemble existing ones. They just switch the format to X509 and set\n> > the signingkey when creating signatures. Validation of signatures\n> > does not need any configuration of git, it does need gpgsm to be\n> > configured to trust the key(-chain).\n> > We generate a self-signed key for committer@example.com and\n> > configure gpgsm to trust it.\n> > \n> > Signed-off-by: Henning Schild \n> > ---\n> > t/lib-gpg.sh | 9 ++++++-\n> > t/lib-gpg/gpgsm-gen-key.in | 6 +++++\n> > t/t4202-log.sh | 66\n> > ++++++++++++++++++++++++++++++++++++++++++++++\n> > t/t5534-push-signed.sh | 52\n> > ++++++++++++++++++++++++++++++++++++ t/t7003-filter-branch.sh |\n> > 15 +++++++++++ t/t7030-verify-tag.sh | 47\n> > +++++++++++++++++++++++++++++++-- t/t7600-merge.sh | 31\n> > ++++++++++++++++++++++ 7 files changed, 223 insertions(+), 3\n> > deletions(-) create mode 100644 t/lib-gpg/gpgsm-gen-key.in\n> > \n> > diff --git a/t/lib-gpg.sh b/t/lib-gpg.sh\n> > index a5d3b2cba..9dcb4e990 100755\n> > --- a/t/lib-gpg.sh\n> > +++ b/t/lib-gpg.sh\n> > @@ -38,7 +38,14 @@ then\n> > \t\t\t\"$TEST_DIRECTORY\"/lib-gpg/ownertrust &&\n> > \t\tgpg --homedir \"${GNUPGHOME}\" /dev/null\n> > 2>&1 \\ --sign -u committer@example.com &&\n> > -\t\ttest_set_prereq GPG\n> > +\t\ttest_set_prereq GPG &&\n> > +\t\techo | gpgsm --homedir \"${GNUPGHOME}\" -o\n> > \"$TEST_DIRECTORY\"/lib-gpg/gpgsm.crt.user --passphrase-fd 0\n> > --pinentry-mode loopback --generate-key --batch\n> > \"$TEST_DIRECTORY\"/lib-gpg/gpgsm-gen-key.in &&\n> > +\t\tgpgsm --homedir \"${GNUPGHOME}\" --import\n> > \"$TEST_DIRECTORY\"/lib-gpg/gpgsm.crt.user &&\n> > +\t\tgpgsm --homedir \"${GNUPGHOME}\" -K | grep\n> > fingerprint: | cut -d\" \" -f4 | tr -d '\\n' >\n> > ${GNUPGHOME}/trustlist.txt &&\n> > +\t\techo \" S relax\" >> ${GNUPGHOME}/trustlist.txt &&\n> > +\t\t(gpgconf --kill gpg-agent >/dev/null 2>&1 || : ) &&\n> > +\t\techo hello | gpgsm --homedir \"${GNUPGHOME}\" -u\n> > committer@example.com -o /dev/null --sign - 2>&1 &&\n> > +\t\ttest_set_prereq GPGSM \n> \n> It looks like the GPGSM prerequisite will only be set if the GPG\n> prerequisite is set as well. Do we want to consider the case when the\n> user might have gpgsm but not gpg?\n\nNice finding, i should have tried to hide that better ;).\n\nI thought about it when writing the code. There might be distributions\nwhere you can install one without the other. I also introduces a few\ntests that rely on the implication, where GPGSM tests on top of GPG.\n(i.e. t7030 \"create signed tags x509\")\nThe implication is really just there for the tests, not for end-users.\nDropping it would create more variations in testing (make it more\nexpensive).\n\nI would say it is not worth it at the moment.\n\nImplementing the gpg.format detection by actually calling the \"other\"\nprogram to find which one knows the key, would shine another light on\nthat one. But i kind of doubt that idea is a good one.\n\nHenning\n"},{"id":"351767","messageId":"20180706100202.6e3efd45@md1pvb1c.ad001.siemens.net","threadId":"48822","inReplyTo":"20180706010148.GA7697@genre.crustytoothpaste.net","subject":"Re: [PATCH 3/8] gpg-interface: add new config to select how to sign a commit","fromName":"Henning Schild","fromEmail":"henning.schild@siemens.com","sentAt":"2018-07-06T08:02:02Z","receivedAt":"2018-07-06T08:17:51Z","isPatch":true,"sender":{"key":"henning.schild@siemens.com","avatar":"https://avatars.githubusercontent.com/u/7237877?v=4"},"body":"Am Fri, 6 Jul 2018 01:01:48 +0000\nschrieb \"brian m. carlson\" :\n\n> On Tue, Jul 03, 2018 at 02:38:15PM +0200, Henning Schild wrote:\n> > Add \"gpg.format\" where the user can specify which type of signature\n> > to use for commits. At the moment only \"PGP\" is supported and the\n> > value is not even used. This commit prepares for a new types of\n> > signatures. \n> \n> We typically prefer to have option values specified in lower case. I\n> also think \"openpgp\" might be better than \"PGP\", since that's the name\n> of the specification and it would avoid any potential unhappiness\n> about compatibility with PGP or trademarks.\n\nThanks for your input. I was assuming the names to start a discussion\nand i do not have a preference here.\nLet us wait for further comments on naming, i will then implement\nwhatever the consensus is or what the maintainer requests.\n\nIn fact \"gpg.format\" could be dropped and we could try both \"gpg\" and\n\"gpgsm\" and see where the signing-key is available. I decided to not\nimplement that because gpgsm (unlike) gpg does not return an error when\n\"--list-secret-keys\" does not find anything. And we could have the odd\ncase where both would find a matching key. In which case we would need\n\"gpg.format\" again to specify which one we prefer.\n\nHenning\n"},{"id":"351784","messageId":"xmqqa7r4mg45.fsf@gitster-ct.c.googlers.com","threadId":"48822","inReplyTo":"CAN0heSrXpLCDRjnZC80QXBG27gd6m5reBn1hfNd_KXxnPVkA2g@mail.gmail.com","subject":"Re: [PATCH 4/8] gpg-interface: introduce an abstraction for multiple gpg formats","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-07-06T17:24:58Z","receivedAt":"2018-07-06T17:25:04Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Martin Ågren writes:\n\n>> +enum gpgformats { PGP_FMT };\n>> +struct gpg_format_data gpg_formats[] = {\n>> + { .format = \"PGP\", .program = \"gpg\",\n>> + .extra_args_verify = { \"--keyid-format=long\", },\n>> + .sigs = { PGP_SIGNATURE, PGP_MESSAGE, },\n>> + },\n>> +};\n>\n> I think those trailing commas are ok now, but I'm not sure...\n>\n> I had the same thought about designated initializers. Those should be ok\n> now, c.f. cbc0f81d96 (strbuf: use designated initializers in STRBUF_INIT,\n> 2017-07-10) and a73b3680c4 (Add and use generic name->id mapping code\n> for color slot parsing, 2018-05-26).\n\nAs you said, we dipped our toes in designated initializers in both\nstruct and array, i.e. { .field = init }, { [offset] = init } last\nsummer and we haven't got complaints from minor platforms so far.\n\nThe \"comma\" thing you are wondering is something else. The comma we\nsee above is after the last element in an array's initializer (and\nthe last element in a struct's initializer), which we have been\nhappily using from very early days (and they are kosher ANSI C).\n\nWhat we've been avoiding was the comma after the last element in the\nenum (in other words, if PGP_FMT had ',' after it in the above\nquoted addition, that would have been violation of that rule), as\nhaving such a trailing comma used to be ANSI C violation as well. I\ndo not recall offhand if we loosened that deliberately.\n\n4b05548f (\"enums: omit trailing comma for portability\", 2010-05-14),\nc9b6782a (\"enums: omit trailing comma for portability\", 2011-03-16)\n"},{"id":"351801","messageId":"xmqqsh4wjg7b.fsf@gitster-ct.c.googlers.com","threadId":"48822","inReplyTo":"f9e371c8dd2a17ddb5fd5989a7fdad1c0d1bb6e7.1530616446.git.henning.schild@siemens.com","subject":"Re: [PATCH 1/8] builtin/receive-pack: use check_signature from gpg-interface","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-07-06T19:51:20Z","receivedAt":"2018-07-06T19:51:25Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Henning Schild writes:\n\n> The combination of verify_signed_buffer followed by parse_gpg_output is\n> available as check_signature. Use that instead of implementing it again.\n>\n> Signed-off-by: Henning Schild \n> ---\n\nMakes sense. \n\nWhen d05b9618 (\"receive-pack: GPG-validate push certificates\",\n2014-08-14) implemented the check, there wasn't check_signature()\navailable. The commit probably should have done what a4cc18f2\n(\"verify-tag: share code with verify-commit\", 2015-06-21) later did\nto introduce the check_signature() function by factoring it out of\ncommit.c::check_commit_signature() as a preparatory step.\n\nWill queue. Thanks.\n\n> builtin/receive-pack.c | 17 ++---------------\n> 1 file changed, 2 insertions(+), 15 deletions(-)\n>\n> diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\n> index 68d36e0a5..9f0583deb 100644\n> --- a/builtin/receive-pack.c\n> +++ b/builtin/receive-pack.c\n> @@ -629,8 +629,6 @@ static void prepare_push_cert_sha1(struct child_process *proc)\n> \t\treturn;\n> \n> \tif (!already_done) {\n> -\t\tstruct strbuf gpg_output = STRBUF_INIT;\n> -\t\tstruct strbuf gpg_status = STRBUF_INIT;\n> \t\tint bogs /* beginning_of_gpg_sig */;\n> \n> \t\talready_done = 1;\n> @@ -639,22 +637,11 @@ static void prepare_push_cert_sha1(struct child_process *proc)\n> \t\t\toidclr(&push_cert_oid);\n> \n> \t\tmemset(&sigcheck, '\\0', sizeof(sigcheck));\n> -\t\tsigcheck.result = 'N';\n> \n> \t\tbogs = parse_signature(push_cert.buf, push_cert.len);\n> -\t\tif (verify_signed_buffer(push_cert.buf, bogs,\n> -\t\t\t\t\t push_cert.buf + bogs, push_cert.len - bogs,\n> -\t\t\t\t\t &gpg_output, &gpg_status) < 0) {\n> -\t\t\t; /* error running gpg */\n> -\t\t} else {\n> -\t\t\tsigcheck.payload = push_cert.buf;\n> -\t\t\tsigcheck.gpg_output = gpg_output.buf;\n> -\t\t\tsigcheck.gpg_status = gpg_status.buf;\n> -\t\t\tparse_gpg_output(&sigcheck);\n> -\t\t}\n> +\t\tcheck_signature(push_cert.buf, bogs, push_cert.buf + bogs,\n> +\t\t\t\tpush_cert.len - bogs, &sigcheck);\n> \n> -\t\tstrbuf_release(&gpg_output);\n> -\t\tstrbuf_release(&gpg_status);\n> \t\tnonce_status = check_nonce(push_cert.buf, bogs);\n> \t}\n> \tif (!is_null_oid(&push_cert_oid)) {\n"},{"id":"351802","messageId":"xmqqo9fkjfvt.fsf@gitster-ct.c.googlers.com","threadId":"48822","inReplyTo":"20180706010148.GA7697@genre.crustytoothpaste.net","subject":"Re: [PATCH 3/8] gpg-interface: add new config to select how to sign a commit","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-07-06T19:58:14Z","receivedAt":"2018-07-06T19:58:20Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"brian m. carlson\" writes:\n\n> On Tue, Jul 03, 2018 at 02:38:15PM +0200, Henning Schild wrote:\n>> Add \"gpg.format\" where the user can specify which type of signature to\n>> use for commits. At the moment only \"PGP\" is supported and the value is\n>> not even used. This commit prepares for a new types of signatures.\n>\n> We typically prefer to have option values specified in lower case. I\n> also think \"openpgp\" might be better than \"PGP\", since that's the name\n> of the specification and it would avoid any potential unhappiness about\n> compatibility with PGP or trademarks.\n\nI do not know about trademarks, but the suggested \"'openpgp' in\nlowercase\" sounds sensible. Some people might even favor doing\nstrcasecmp() on the value; I do not have a strong opinion on that\nyet.\n"},{"id":"351807","messageId":"xmqqfu0wjetl.fsf@gitster-ct.c.googlers.com","threadId":"48822","inReplyTo":"9fd9d0d2ee3ae549baf8e9a710dfffbc118e66f7.1530616446.git.henning.schild@siemens.com","subject":"Re: [PATCH 5/8] t/t7510: check the validation of the new config gpg.format","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-07-06T20:21:10Z","receivedAt":"2018-07-06T20:21:16Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Henning Schild writes:\n\n> Valid values are already covered by all tests that use GPG, now also\n> test what happens if we go for an invalid one.\n>\n> Signed-off-by: Henning Schild \n> ---\n> t/t7510-signed-commit.sh | 10 ++++++++++\n> 1 file changed, 10 insertions(+)\n>\n> diff --git a/t/t7510-signed-commit.sh b/t/t7510-signed-commit.sh\n> index 6e2015ed9..cb523513f 100755\n> --- a/t/t7510-signed-commit.sh\n> +++ b/t/t7510-signed-commit.sh\n> @@ -227,4 +227,14 @@ test_expect_success GPG 'log.showsignature behaves like --show-signature' '\n> \tgrep \"gpg: Good signature\" actual\n> '\n> \n> +test_expect_success GPG 'check gpg config for malformed values' '\n> +\tmv .git/config .git/config.old &&\n> +\ttest_when_finished \"mv .git/config.old .git/config\" &&\n\nHmmmmm. \n\nIs the damage caused by throwing a bad value at gpg.format designed\nto be so severe that \"test_when_finished test_unconfig ...\" cannot\nrecover from? This test script is not about how \"git config\" is\nimplemented and works, so it would be a good idea for it to be even\noblivious to the fact that .git/config is the file being mucked with\nwhen we do \"git config\".\n\nI have a suspicion that you can just use test_config (which would\narrange \"test_when_finished test_unconfig ...\" for free).\n\n> +\tgit config gpg.format malformed &&\n> +\ttest_expect_code 128 git commit -S --amend -m \"fail\" 2>result &&\n\nIs this 128 something we document and have users rely on? Or should\nwe rather say\n\n\ttest_must_fail git commit ...\n\nhere instead?\n\n> +\ttest_i18ngrep \"malformed value for gpg.format: malformed\" result &&\n> +\ttest_i18ngrep \"fatal: .*\\.git/config\" result &&\n> +\ttest_i18ngrep \"fatal: .*line 2\" result\n> +'\n> +\n> test_done\n"},{"id":"351808","messageId":"xmqqbmbkjer1.fsf@gitster-ct.c.googlers.com","threadId":"48822","inReplyTo":"0d83d4d5c1c90011db90947d767ea0191a7ecbd7.1530616446.git.henning.schild@siemens.com","subject":"Re: [PATCH 6/8] gpg-interface: do not hardcode the key string len anymore","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-07-06T20:22:42Z","receivedAt":"2018-07-06T20:22:48Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Henning Schild writes:\n\n> gnupg does print the keyid followed by a space and the signer comes\n> next. The same pattern is also used in gpgsm, but there the key length\n> would be 40 instead of 16. Instead of hardcoding the expected length,\n> find the first space and calculate it.\n>\n> Signed-off-by: Henning Schild \n> ---\n> gpg-interface.c | 5 +++--\n> 1 file changed, 3 insertions(+), 2 deletions(-)\n>\n\nNicely explained and nicely implemented.\n\n> diff --git a/gpg-interface.c b/gpg-interface.c\n> index cd3b1b568..aa747278e 100644\n> --- a/gpg-interface.c\n> +++ b/gpg-interface.c\n> @@ -88,10 +88,11 @@ static void parse_gpg_output(struct signature_check *sigc)\n> \t\tsigc->result = sigcheck_gpg_status[i].result;\n> \t\t/* The trust messages are not followed by key/signer information */\n> \t\tif (sigc->result != 'U') {\n> -\t\t\tsigc->key = xmemdupz(found, 16);\n> +\t\t\tnext = strchrnul(found, ' ');\n> +\t\t\tsigc->key = xmemdupz(found, next - found);\n> \t\t\t/* The ERRSIG message is not followed by signer information */\n> \t\t\tif (sigc-> result != 'E') {\n> -\t\t\t\tfound += 17;\n> +\t\t\t\tfound = next + 1;\n> \t\t\t\tnext = strchrnul(found, '\\n');\n> \t\t\t\tsigc->signer = xmemdupz(found, next - found);\n> \t\t\t}\n"},{"id":"351811","messageId":"xmqq7em8je6q.fsf@gitster-ct.c.googlers.com","threadId":"48822","inReplyTo":"1f99ef66bc5e115ec06a3c456658ad54ce405ce3.1530616446.git.henning.schild@siemens.com","subject":"Re: [PATCH 7/8] gpg-interface: introduce new signature format \"X509\" using gpgsm","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-07-06T20:34:53Z","receivedAt":"2018-07-06T20:34:59Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Henning Schild writes:\n\n> -enum gpgformats { PGP_FMT };\n> +enum gpgformats { PGP_FMT, X509_FMT };\n> struct gpg_format_data gpg_formats[] = {\n> \t{ .format = \"PGP\", .program = \"gpg\",\n> \t .extra_args_verify = { \"--keyid-format=long\", },\n> \t .sigs = { PGP_SIGNATURE, PGP_MESSAGE, },\n> \t},\n> +\t{ .format = \"X509\", .program = \"gpgsm\",\n> +\t .extra_args_verify = { NULL },\n> +\t .sigs = {X509_SIGNATURE, NULL, }\n\nMissing SP between \"{X\" is a bit irritating.\n\nAlso the trailing comma (the issue is shared with the PGP side) when\nthe initializer is smashed on a single line feels pretty much\npointless. If it were multi-line, then such a trailing comma would \nhelp future developers to add a new entry, i.e.\n\n\t .sigs = { \n\t \tPGP_SIGNATURE,\n\t \tPGP_MESSAGE,\n\t+\tPGP_SOMETHING_NEW,\n\t }\n\nwithout touching the last existing entry. But on a single line?\n\n\t-.sigs = { PGP_SIGNATURE, PGP_MESSAGE }\n\t+.sigs = { PGP_SIGNATURE, PGP_MESSAGE, PGP_SOMETHING_NEW }\n\nis probably prettier without such a trailing comma.\n\n> @@ -190,6 +195,9 @@ int git_gpg_config(const char *var, const char *value, void *cb)\n> \tif (!strcmp(var, \"gpg.program\"))\n> \t\treturn git_config_string(&gpg_formats[PGP_FMT].program, var,\n> \t\t\t\t\t value);\n> +\tif (!strcmp(var, \"gpg.programX509\"))\n> +\t\treturn git_config_string(&gpg_formats[X509_FMT].program, var,\n> +\t\t\t\t\t value);\n\nThis is a git_config() callback, isn't it? A two-level variable\nname is given to a callback after downcasing, so nothing will match\n\"gpg.programX509\", I suspect. I see Brian already commented on the\nname and the better organization being\n\n - gpg.format defines 'openpgp' or whatever other values;\n - gpg..program defines the actual program\n\nwhere is the value gpg.format would take\n(e.g. \"gpg.openpgp.program = gnupg\"). And I agree with these\nsuggestions.\n\n\n\n"},{"id":"351822","messageId":"xmqqk1q8hwta.fsf@gitster-ct.c.googlers.com","threadId":"48822","inReplyTo":"xmqqsh4wjg7b.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 1/8] builtin/receive-pack: use check_signature from gpg-interface","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-07-06T21:35:29Z","receivedAt":"2018-07-06T21:35:34Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Henning Schild writes:\n>\n>> The combination of verify_signed_buffer followed by parse_gpg_output is\n>> available as check_signature. Use that instead of implementing it again.\n>>\n>> Signed-off-by: Henning Schild \n>> ---\n>\n> Makes sense. \n>\n> When d05b9618 (\"receive-pack: GPG-validate push certificates\",\n> 2014-08-14) implemented the check, there wasn't check_signature()\n> available. The commit probably should have done what a4cc18f2\n> (\"verify-tag: share code with verify-commit\", 2015-06-21) later did\n> to introduce the check_signature() function by factoring it out of\n> commit.c::check_commit_signature() as a preparatory step.\n>\n> Will queue. Thanks.\n\nWell, I guess I won't queue this version that would waste others'\ntime, as you'd be rerolling to update variable names and such, so\nI'd wait for that (and you in turn would wait for the names and\nother discussions to settle).\n\nThanks anyway.\n\n"},{"id":"351932","messageId":"20180709101854.2777aae2@md1pvb1c.ad001.siemens.net","threadId":"48822","inReplyTo":"xmqqk1q8hwta.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 1/8] builtin/receive-pack: use check_signature from gpg-interface","fromName":"Henning Schild","fromEmail":"henning.schild@siemens.com","sentAt":"2018-07-09T08:18:54Z","receivedAt":"2018-07-09T08:19:22Z","isPatch":true,"sender":{"key":"henning.schild@siemens.com","avatar":"https://avatars.githubusercontent.com/u/7237877?v=4"},"body":"Am Fri, 6 Jul 2018 14:35:29 -0700\nschrieb Junio C Hamano :\n\n> Junio C Hamano writes:\n> \n> > Henning Schild writes:\n> > \n> >> The combination of verify_signed_buffer followed by\n> >> parse_gpg_output is available as check_signature. Use that instead\n> >> of implementing it again.\n> >>\n> >> Signed-off-by: Henning Schild \n> >> --- \n> >\n> > Makes sense. \n> >\n> > When d05b9618 (\"receive-pack: GPG-validate push certificates\",\n> > 2014-08-14) implemented the check, there wasn't check_signature()\n> > available. The commit probably should have done what a4cc18f2\n> > (\"verify-tag: share code with verify-commit\", 2015-06-21) later did\n> > to introduce the check_signature() function by factoring it out of\n> > commit.c::check_commit_signature() as a preparatory step.\n> >\n> > Will queue. Thanks. \n> \n> Well, I guess I won't queue this version that would waste others'\n> time, as you'd be rerolling to update variable names and such, so\n> I'd wait for that (and you in turn would wait for the names and\n> other discussions to settle).\n> \n> Thanks anyway.\n\nI think 1 and 2 can be seen as somewhat unrelated to the gpgsm feature,\nthey are more general refactoring. So i think picking them is a good\nidea. It will make the series shorter and ease review in the next round.\n\nHenning\n"},{"id":"351933","messageId":"20180709102139.274f0560@md1pvb1c.ad001.siemens.net","threadId":"48822","inReplyTo":"xmqqa7r4mg45.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 4/8] gpg-interface: introduce an abstraction for multiple gpg formats","fromName":"Henning Schild","fromEmail":"henning.schild@siemens.com","sentAt":"2018-07-09T08:21:39Z","receivedAt":"2018-07-09T08:22:04Z","isPatch":true,"sender":{"key":"henning.schild@siemens.com","avatar":"https://avatars.githubusercontent.com/u/7237877?v=4"},"body":"Am Fri, 6 Jul 2018 10:24:58 -0700\nschrieb Junio C Hamano :\n\n> Martin Ågren writes:\n> \n> >> +enum gpgformats { PGP_FMT };\n> >> +struct gpg_format_data gpg_formats[] = {\n> >> + { .format = \"PGP\", .program = \"gpg\",\n> >> + .extra_args_verify = { \"--keyid-format=long\", },\n> >> + .sigs = { PGP_SIGNATURE, PGP_MESSAGE, },\n> >> + },\n> >> +}; \n> >\n> > I think those trailing commas are ok now, but I'm not sure...\n> >\n> > I had the same thought about designated initializers. Those should\n> > be ok now, c.f. cbc0f81d96 (strbuf: use designated initializers in\n> > STRBUF_INIT, 2017-07-10) and a73b3680c4 (Add and use generic\n> > name->id mapping code for color slot parsing, 2018-05-26). \n> \n> As you said, we dipped our toes in designated initializers in both\n> struct and array, i.e. { .field = init }, { [offset] = init } last\n> summer and we haven't got complaints from minor platforms so far.\n> \n> The \"comma\" thing you are wondering is something else. The comma we\n> see above is after the last element in an array's initializer (and\n> the last element in a struct's initializer), which we have been\n> happily using from very early days (and they are kosher ANSI C).\n> \n> What we've been avoiding was the comma after the last element in the\n> enum (in other words, if PGP_FMT had ',' after it in the above\n> quoted addition, that would have been violation of that rule), as\n> having such a trailing comma used to be ANSI C violation as well. I\n> do not recall offhand if we loosened that deliberately.\n> \n> 4b05548f (\"enums: omit trailing comma for portability\", 2010-05-14),\n> c9b6782a (\"enums: omit trailing comma for portability\", 2011-03-16)\n\nI guess that means the style is acceptable and does not require\nchanges, please correct me if i am wrong.\n\nHenning\n"},{"id":"351935","messageId":"20180709102726.754c37cf@md1pvb1c.ad001.siemens.net","threadId":"48822","inReplyTo":"xmqqfu0wjetl.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 5/8] t/t7510: check the validation of the new config gpg.format","fromName":"Henning Schild","fromEmail":"henning.schild@siemens.com","sentAt":"2018-07-09T08:27:26Z","receivedAt":"2018-07-09T08:27:47Z","isPatch":true,"sender":{"key":"henning.schild@siemens.com","avatar":"https://avatars.githubusercontent.com/u/7237877?v=4"},"body":"Am Fri, 6 Jul 2018 13:21:10 -0700\nschrieb Junio C Hamano :\n\n> Henning Schild writes:\n> \n> > Valid values are already covered by all tests that use GPG, now also\n> > test what happens if we go for an invalid one.\n> >\n> > Signed-off-by: Henning Schild \n> > ---\n> > t/t7510-signed-commit.sh | 10 ++++++++++\n> > 1 file changed, 10 insertions(+)\n> >\n> > diff --git a/t/t7510-signed-commit.sh b/t/t7510-signed-commit.sh\n> > index 6e2015ed9..cb523513f 100755\n> > --- a/t/t7510-signed-commit.sh\n> > +++ b/t/t7510-signed-commit.sh\n> > @@ -227,4 +227,14 @@ test_expect_success GPG 'log.showsignature\n> > behaves like --show-signature' ' grep \"gpg: Good signature\" actual\n> > '\n> > \n> > +test_expect_success GPG 'check gpg config for malformed values' '\n> > +\tmv .git/config .git/config.old &&\n> > +\ttest_when_finished \"mv .git/config.old .git/config\" && \n> \n> Hmmmmm. \n> \n> Is the damage caused by throwing a bad value at gpg.format designed\n> to be so severe that \"test_when_finished test_unconfig ...\" cannot\n> recover from? This test script is not about how \"git config\" is\n> implemented and works, so it would be a good idea for it to be even\n> oblivious to the fact that .git/config is the file being mucked with\n> when we do \"git config\".\n> \n> I have a suspicion that you can just use test_config (which would\n> arrange \"test_when_finished test_unconfig ...\" for free).\n> \n> > +\tgit config gpg.format malformed &&\n> > +\ttest_expect_code 128 git commit -S --amend -m \"fail\"\n> > 2>result && \n> \n> Is this 128 something we document and have users rely on? Or should\n> we rather say\n> \n> \ttest_must_fail git commit ...\n> \n> here instead?\n\nThis is basically an adopted copy of t1308 'check line errors for\nmalformed values'.\n\nI will have a look at test_config.\n\nHenning\n\n> > +\ttest_i18ngrep \"malformed value for gpg.format: malformed\"\n> > result &&\n> > +\ttest_i18ngrep \"fatal: .*\\.git/config\" result &&\n> > +\ttest_i18ngrep \"fatal: .*line 2\" result\n> > +'\n> > +\n> > test_done \n\n"},{"id":"351937","messageId":"CAPig+cQyXC2o4fUARL_kqPn5+6adBpr+WUfAWWgu2QLjDn1eoQ@mail.gmail.com","threadId":"48822","inReplyTo":"20180709102139.274f0560@md1pvb1c.ad001.siemens.net","subject":"Re: [PATCH 4/8] gpg-interface: introduce an abstraction for multiple gpg formats","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2018-07-09T08:44:01Z","receivedAt":"2018-07-09T08:44:16Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Mon, Jul 9, 2018 at 4:22 AM Henning Schild\n wrote:\n> Am Fri, 6 Jul 2018 10:24:58 -0700\n> schrieb Junio C Hamano :\n> > Martin Ågren writes:\n> > >> +struct gpg_format_data gpg_formats[] = {\n> > >> + { .format = \"PGP\", .program = \"gpg\",\n> > >> + .extra_args_verify = { \"--keyid-format=long\", },\n> > >> + .sigs = { PGP_SIGNATURE, PGP_MESSAGE, },\n> > >> + },\n> > >> +};\n> > >\n> > > I think those trailing commas are ok now, but I'm not sure...\n> >\n> > What we've been avoiding was the comma after the last element in the\n> > enum (in other words, if PGP_FMT had ',' after it in the above\n> > quoted addition, that would have been violation of that rule), as\n> > having such a trailing comma used to be ANSI C violation as well.\n>\n> I guess that means the style is acceptable and does not require\n> changes, please correct me if i am wrong.\n\nThe trailing comma in the 'sigs' initializer is bothersome because\n'sigs' is declared as a 2-element array, and this initializer already\nhas two elements. Therefore, the comma is misleading to anyone reading\nthe code, making it appears as if additional items can be added. For\nthat reason, alone, it would be nice to see the unnecessary comma\nremoved.\n\nDitto with regard to the trailing comma in the 'extra_args_verify'\ninitializer since 'extra_args_verify' is declared as a 1-element\narray.\n"},{"id":"351949","messageId":"xmqqy3ekfm2r.fsf@gitster-ct.c.googlers.com","threadId":"48822","inReplyTo":"CAPig+cQyXC2o4fUARL_kqPn5+6adBpr+WUfAWWgu2QLjDn1eoQ@mail.gmail.com","subject":"Re: [PATCH 4/8] gpg-interface: introduce an abstraction for multiple gpg formats","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-07-09T15:47:08Z","receivedAt":"2018-07-09T15:47:14Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Sunshine writes:\n\n> On Mon, Jul 9, 2018 at 4:22 AM Henning Schild\n> wrote:\n>> Am Fri, 6 Jul 2018 10:24:58 -0700\n>> schrieb Junio C Hamano :\n>> > Martin Ågren writes:\n>> > >> +struct gpg_format_data gpg_formats[] = {\n>> > >> + { .format = \"PGP\", .program = \"gpg\",\n>> > >> + .extra_args_verify = { \"--keyid-format=long\", },\n>> > >> + .sigs = { PGP_SIGNATURE, PGP_MESSAGE, },\n>> > >> + },\n>> > >> +};\n>> > >\n>> > > I think those trailing commas are ok now, but I'm not sure...\n>> >\n>> > What we've been avoiding was the comma after the last element in the\n>> > enum (in other words, if PGP_FMT had ',' after it in the above\n>> > quoted addition, that would have been violation of that rule), as\n>> > having such a trailing comma used to be ANSI C violation as well.\n>>\n>> I guess that means the style is acceptable and does not require\n>> changes, please correct me if i am wrong.\n>\n> The trailing comma in the 'sigs' initializer is bothersome because\n> 'sigs' is declared as a 2-element array, and this initializer already\n> has two elements. Therefore, the comma is misleading to anyone reading\n> the code, making it appears as if additional items can be added. For\n> that reason, alone, it would be nice to see the unnecessary comma\n> removed.\n>\n> Ditto with regard to the trailing comma in the 'extra_args_verify'\n> initializer since 'extra_args_verify' is declared as a 1-element\n> array.\n\nI am not sure I agree with that reasoning. The primary benefit we\ngain from the convention to allow trailing comma in struct/array\ninitializers like these is that we can add a new field with a patch\nlike this:\n\n\t struct {\n\t \tchar *foo;\n\t \tchar *bar;\n\t+\tchar *baz;\n\t } xyzzy = {\n\t \t\"foo\",\n\t \t\"bar\",\n\t+\t\"baz\",\n\t };\n\nwithout having to touch the initialization of the field that used to\nbe at the end, i.e. .bar=\"bar\", and just can add the new stuff at\nthe end. But that is only possible as long as we allow the trailing\ncomma after the last element. The same argument applies to an array\ninitialization (i.e. when we need to allow more elements to the\n.extra_args_to_verify[] array).\n\nAll of the above is somewhat moot for the other reason I already\nstated in the thread, though. And that may be a good reason why we\nwould want to lose these trailing commas from the last elements\n(i.e. if it does not prepare the current code to avoid having to\ntouch the last line when we add a new element at the end, then\ntrailing comma is a visual hiccup that does not serve any useful\npurpose).\n\n\n\n"},{"id":"351950","messageId":"xmqqtvp8flok.fsf@gitster-ct.c.googlers.com","threadId":"48822","inReplyTo":"20180709101854.2777aae2@md1pvb1c.ad001.siemens.net","subject":"Re: [PATCH 1/8] builtin/receive-pack: use check_signature from gpg-interface","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-07-09T15:55:39Z","receivedAt":"2018-07-09T15:55:47Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Henning Schild writes:\n\n> I think 1 and 2 can be seen as somewhat unrelated to the gpgsm feature,\n> they are more general refactoring. So i think picking them is a good\n> idea. It will make the series shorter and ease review in the next round.\n\nSurely, resending from patch 3 and upwards labelled as \"add support\nfor gpgsm\", saying that the topic depends on a different topic\nbranch named $X in my tree (after $X actually gets pushed out,\npreferrably as part of 'next'---which is a promise that the changes\nwon't see any more drastic rewrites), is a good approach.\n\nThanks.\n\n"},{"id":"352115","messageId":"20180710153724.GA23438@sigill.intra.peff.net","threadId":"48822","inReplyTo":"xmqqa7r4mg45.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 4/8] gpg-interface: introduce an abstraction for multiple gpg formats","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-07-10T15:37:25Z","receivedAt":"2018-07-10T15:37:31Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jul 06, 2018 at 10:24:58AM -0700, Junio C Hamano wrote:\n\n> What we've been avoiding was the comma after the last element in the\n> enum (in other words, if PGP_FMT had ',' after it in the above\n> quoted addition, that would have been violation of that rule), as\n> having such a trailing comma used to be ANSI C violation as well. I\n> do not recall offhand if we loosened that deliberately.\n> \n> 4b05548f (\"enums: omit trailing comma for portability\", 2010-05-14),\n> c9b6782a (\"enums: omit trailing comma for portability\", 2011-03-16)\n\nI think we accidentally did a weather-balloon in e1327023ea (grep:\nrefactor the concept of \"grep source\" into an object, 2012-02-02).\nIt's still there and nobody has complained about it yet.\n\nSo I think we can consider that requirement loosened at this point.\n\n-Peff\n"},{"id":"352120","messageId":"xmqqmuuz9jih.fsf@gitster-ct.c.googlers.com","threadId":"48822","inReplyTo":"20180710153724.GA23438@sigill.intra.peff.net","subject":"Re: [PATCH 4/8] gpg-interface: introduce an abstraction for multiple gpg formats","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-07-10T15:51:18Z","receivedAt":"2018-07-10T15:51:24Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Fri, Jul 06, 2018 at 10:24:58AM -0700, Junio C Hamano wrote:\n>\n>> What we've been avoiding was the comma after the last element in the\n>> enum (in other words, if PGP_FMT had ',' after it in the above\n>> quoted addition, that would have been violation of that rule), as\n>> having such a trailing comma used to be ANSI C violation as well. I\n>> do not recall offhand if we loosened that deliberately.\n>> \n>> 4b05548f (\"enums: omit trailing comma for portability\", 2010-05-14),\n>> c9b6782a (\"enums: omit trailing comma for portability\", 2011-03-16)\n>\n> I think we accidentally did a weather-balloon in e1327023ea (grep:\n> refactor the concept of \"grep source\" into an object, 2012-02-02).\n> It's still there and nobody has complained about it yet.\n>\n> So I think we can consider that requirement loosened at this point.\n\n#leftoverbits: update CodingGuidelines and refer to these three\ncommits.\n\nTakers?\n\n \n"},{"id":"352124","messageId":"xmqqin5n9j6f.fsf@gitster-ct.c.googlers.com","threadId":"48822","inReplyTo":"20180710153724.GA23438@sigill.intra.peff.net","subject":"Re: [PATCH 4/8] gpg-interface: introduce an abstraction for multiple gpg formats","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-07-10T15:58:32Z","receivedAt":"2018-07-10T15:58:38Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Fri, Jul 06, 2018 at 10:24:58AM -0700, Junio C Hamano wrote:\n>\n>> What we've been avoiding was the comma after the last element in the\n>> enum (in other words, if PGP_FMT had ',' after it in the above\n>> quoted addition, that would have been violation of that rule), as\n>> having such a trailing comma used to be ANSI C violation as well. I\n>> do not recall offhand if we loosened that deliberately.\n>> \n>> 4b05548f (\"enums: omit trailing comma for portability\", 2010-05-14),\n>> c9b6782a (\"enums: omit trailing comma for portability\", 2011-03-16)\n>\n> I think we accidentally did a weather-balloon in e1327023ea (grep:\n> refactor the concept of \"grep source\" into an object, 2012-02-02).\n> It's still there and nobody has complained about it yet.\n>\n> So I think we can consider that requirement loosened at this point.\n>\n> -Peff\n\nYup, thanks for digging it out. It seems that I did the same\ndigging some time ago but forgot about it.\n"},{"id":"352139","messageId":"20180710171502.GB23438@sigill.intra.peff.net","threadId":"48822","inReplyTo":"xmqqin5n9j6f.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 4/8] gpg-interface: introduce an abstraction for multiple gpg formats","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-07-10T17:15:02Z","receivedAt":"2018-07-10T17:15:08Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jul 10, 2018 at 08:58:32AM -0700, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > On Fri, Jul 06, 2018 at 10:24:58AM -0700, Junio C Hamano wrote:\n> >\n> >> What we've been avoiding was the comma after the last element in the\n> >> enum (in other words, if PGP_FMT had ',' after it in the above\n> >> quoted addition, that would have been violation of that rule), as\n> >> having such a trailing comma used to be ANSI C violation as well. I\n> >> do not recall offhand if we loosened that deliberately.\n> >> \n> >> 4b05548f (\"enums: omit trailing comma for portability\", 2010-05-14),\n> >> c9b6782a (\"enums: omit trailing comma for portability\", 2011-03-16)\n> >\n> > I think we accidentally did a weather-balloon in e1327023ea (grep:\n> > refactor the concept of \"grep source\" into an object, 2012-02-02).\n> > It's still there and nobody has complained about it yet.\n> >\n> > So I think we can consider that requirement loosened at this point.\n> >\n> > -Peff\n> \n> Yup, thanks for digging it out. It seems that I did the same\n> digging some time ago but forgot about it.\n\nI actually cheated and just searched for \"enum, comma\" in my list\narchive and found your message. ;)\n\n-Peff\n"}]}