{"thread":{"id":"50265","subject":"[PATCH v3] commit-tree: add missing --gpg-sign flag","startedAt":"2019-01-19T03:36:28Z","lastAt":"2019-01-19T23:19:01Z","messageCount":6,"participants":["Brandon Richardson","Martin Ågren"],"isPatch":true,"patchVersion":3,"patchTotal":null},"messages":[{"id":"367169","messageId":"20190119033530.4241-1-brandon1024.br@gmail.com","threadId":"50265","inReplyTo":null,"subject":"[PATCH v3] commit-tree: add missing --gpg-sign flag","fromName":"Brandon Richardson","fromEmail":"brandon1024.br@gmail.com","sentAt":"2019-01-19T03:35:30Z","receivedAt":"2019-01-19T03:36:28Z","isPatch":true,"sender":{"key":"brandon1024.br@gmail.com","avatar":"https://avatars.githubusercontent.com/u/22732449?v=4"},"body":"Add --gpg-sign option in commit-tree, which was documented, but not\nimplemented, in 55ca3f99ae. Add tests for the --gpg-sign option.\n\nSigned-off-by: Brandon Richardson \n---\n\nHi all,\n\nThird and (hopefully) final version. Thanks again Martin for the helpful\ncomments.\n\n---\n\n builtin/commit-tree.c | 8 +++++++-\n t/t7510-signed-commit.sh | 13 +++++++++++--\n 2 files changed, 18 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/commit-tree.c b/builtin/commit-tree.c\nindex 9ec36a82b..298e499ac 100644\n--- a/builtin/commit-tree.c\n+++ b/builtin/commit-tree.c\n@@ -66,7 +66,13 @@ int cmd_commit_tree(int argc, const char **argv, const char *prefix)\n \t\t\tcontinue;\n \t\t}\n \n-\t\tif (skip_prefix(arg, \"-S\", &sign_commit))\n+\t\tif(!strcmp(arg, \"--gpg-sign\")) {\n+\t\t sign_commit = \"\";\n+\t\t continue;\n+\t\t}\n+\n+\t\tif (skip_prefix(arg, \"-S\", &sign_commit) ||\n+\t\t\tskip_prefix(arg, \"--gpg-sign=\", &sign_commit))\n \t\t\tcontinue;\n \n \t\tif (!strcmp(arg, \"--no-gpg-sign\")) {\ndiff --git a/t/t7510-signed-commit.sh b/t/t7510-signed-commit.sh\nindex 86d3f93fa..095d4b254 100755\n--- a/t/t7510-signed-commit.sh\n+++ b/t/t7510-signed-commit.sh\n@@ -51,13 +51,22 @@ test_expect_success GPG 'create signed commits' '\n \t# commit.gpgsign is still on but this must not be signed\n \tgit tag ninth-unsigned $(echo 9 | git commit-tree HEAD^{tree}) &&\n \t# explicit -S of course must sign.\n-\tgit tag tenth-signed $(echo 9 | git commit-tree -S HEAD^{tree})\n+\tgit tag tenth-signed $(echo 10 | git commit-tree -S HEAD^{tree}) &&\n+\n+\t# --gpg-sign[=] must sign.\n+\techo 11 >file && test_tick && git commit -S -a -m \"eleventh signed\" &&\n+\tgit tag eleventh-signed &&\n+\tgit commit-tree --gpg-sign -m \"twelfth signed\" HEAD^{tree} &&\n+\tgit tag twelfth-signed &&\n+ git commit-tree --gpg-sign=B7227189 -m \"thirteenth signed\" HEAD^{tree} &&\n+ git tag thirteenth-signed\n '\n \n test_expect_success GPG 'verify and show signatures' '\n \t(\n \t\tfor commit in initial second merge fourth-signed \\\n-\t\t\tfifth-signed sixth-signed seventh-signed tenth-signed\n+\t\t\tfifth-signed sixth-signed seventh-signed tenth-signed \\\n+\t\t\televenth-signed twelfth-signed thirteenth-signed\n \t\tdo\n \t\t\tgit verify-commit $commit &&\n \t\t\tgit show --pretty=short --show-signature $commit >actual &&\n-- \n2.20.1\n\n"},{"id":"367174","messageId":"20190119154552.12189-1-martin.agren@gmail.com","threadId":"50265","inReplyTo":"20190119033530.4241-1-brandon1024.br@gmail.com","subject":"Re: [PATCH v3] commit-tree: add missing --gpg-sign flag","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2019-01-19T15:45:51Z","receivedAt":"2019-01-19T15:46:26Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"Hi Brandon,\n\nThanks for a v3.\n\nOn Sat, 19 Jan 2019 at 04:36, Brandon Richardson wrote:\n> -\t\tif (skip_prefix(arg, \"-S\", &sign_commit))\n> +\t\tif(!strcmp(arg, \"--gpg-sign\")) {\n\n(Same nit as Junio about the missing space after \"if\".)\n\n> +\t\t\tsign_commit = \"\";\n\nNice. ;-)\n\n> +\t\t\tcontinue;\n> +\t\t}\n\n>\t# commit.gpgsign is still on but this must not be signed\n>\tgit tag ninth-unsigned $(echo 9 | git commit-tree HEAD^{tree}) &&\n>\t# explicit -S of course must sign.\n> -\tgit tag tenth-signed $(echo 9 | git commit-tree -S HEAD^{tree})\n> +\tgit tag tenth-signed $(echo 10 | git commit-tree -S HEAD^{tree}) &&\n> +\n> +\t# --gpg-sign[=] must sign.\n> +\techo 11 >file && test_tick && git commit -S -a -m \"eleventh signed\" &&\n> +\tgit tag eleventh-signed &&\n> +\tgit commit-tree --gpg-sign -m \"twelfth signed\" HEAD^{tree} &&\n> +\tgit tag twelfth-signed &&\n> + git commit-tree --gpg-sign=B7227189 -m \"thirteenth signed\" HEAD^{tree} &&\n> + git tag thirteenth-signed\n> '\n\n(These last two lines are not tab-indented, but indented by four spaces.\nThey were perhaps mangled by some copy-pasting.)\n\nRunning this test, we end up with three tags on one commit:\neleventh-signed, twelfth-signed and thirteenth-signed. So as long as\n`git commit -S` works when we use the number 11, everything will pass,\nand we won't really test what we wanted to test. We will verify that\n`git commit-tree` doesn't choke on \"--gpg-sign[=foo]\", but we won't\nverify that it handles it correctly.\n\n(Just recently, it was pointed out on this list that `git log --count`\nwon't complain about \"--count\", but won't act on it, either. So such\nerrors are not unheard of.)\n\nI looked into this test in a bit more detail, and it seems to be quite\nhard to get right. Part of the reason is that `git commit-tree` requires\na bit more careful use than `git commit`, but part of it is that the\ntests that we already have for `git commit-tree [-S]` right before the\nones you're adding are a bit too loose, IMHO. So they're not ideal for\ncopy-pasting... I've come up with the patch below, which you might want\nto use as a basis for your work.\n\nThat is, you could `git am --scissors` this patch on a fresh branch and\n`git commit --amend --signoff --no-edit` it (see\nDocumentation/SubmittingPatches, \"forwarding somebody else's patch\"),\nthen base your work on it, e.g., by cherry-picking your v3 commit.\n\nI think you would want to add 2x3 lines of tests (3 for `--gpg-sign`, 3\nfor `--gpg-sign=...`). That would give you eleventh-signed and\ntwelfth-signed and you wouldn't need any invocation of `git commit` (so\nno thirteenth-signed).\n\nIf you're not up for that, just let me know and I could instead rebase\nyour patch on top of mine and submit both as a v4. I think this has come\nalong nicely, and now it's really just about having a robust test.\n\nMartin\n\n-- >8 --\nSubject: [PATCH] t7510: invoke git as part of &&-chain\n\nIf `git commit-tree HEAD^{tree}` fails on us and produces no output on\nstdout, we will substitute that empty string and execute `git tag\nninth-unsigned`, i.e., we will tag HEAD rather than a newly created\nobject. But we are lucky: we have a signature on HEAD, so we should\neventually fail the next test, where we verify that \"ninth-unsigned\" is\nindeed unsigned.\n\nWe have a similar problem a few lines later. If `git commit-tree -S`\nfails with no output, we will happily tag HEAD as \"tenth-signed\". Here,\nwe are not so lucky. The tag ends up on the same commit as\n\"eighth-signed-alt\", and that's a signed commit, so t7510-signed-commit\nwill pass, despite `git commit-tree -S` failing.\n\nMake these `git commit-tree` invocations a direct part of the &&-chain,\nso that we can rely less on luck and set a better example for future\ntests modeled after this one. Fix a 9/10 copy/paste error while at it.\n\nSigned-off-by: Martin Ågren \n---\n t/t7510-signed-commit.sh | 8 ++++++--\n 1 file changed, 6 insertions(+), 2 deletions(-)\n\ndiff --git a/t/t7510-signed-commit.sh b/t/t7510-signed-commit.sh\nindex 86d3f93fa2..58f528b98f 100755\n--- a/t/t7510-signed-commit.sh\n+++ b/t/t7510-signed-commit.sh\n@@ -49,9 +49,13 @@ test_expect_success GPG 'create signed commits' '\n \tgit tag eighth-signed-alt &&\n \n \t# commit.gpgsign is still on but this must not be signed\n-\tgit tag ninth-unsigned $(echo 9 | git commit-tree HEAD^{tree}) &&\n+\techo 9 | git commit-tree HEAD^{tree} >oid &&\n+\ttest_line_count = 1 oid &&\n+\tgit tag ninth-unsigned $(cat oid) &&\n \t# explicit -S of course must sign.\n-\tgit tag tenth-signed $(echo 9 | git commit-tree -S HEAD^{tree})\n+\techo 10 | git commit-tree -S HEAD^{tree} >oid &&\n+\ttest_line_count = 1 oid &&\n+\tgit tag tenth-signed $(cat oid)\n '\n \n test_expect_success GPG 'verify and show signatures' '\n-- \n2.20.1.98.gecbdaf0899\n\n"},{"id":"367175","messageId":"CAN0heSocQu+w9V3HUJDQVtFZ-NT++dubyt49QOMfvHpJSfNgZA@mail.gmail.com","threadId":"50265","inReplyTo":"20190119154552.12189-1-martin.agren@gmail.com","subject":"Re: [PATCH v3] commit-tree: add missing --gpg-sign flag","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2019-01-19T16:48:11Z","receivedAt":"2019-01-19T16:48:29Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"On Sat, 19 Jan 2019 at 16:46, Martin Ågren wrote:\n> # commit.gpgsign is still on but this must not be signed\n> - git tag ninth-unsigned $(echo 9 | git commit-tree HEAD^{tree}) &&\n> + echo 9 | git commit-tree HEAD^{tree} >oid &&\n> + test_line_count = 1 oid &&\n> + git tag ninth-unsigned $(cat oid) &&\n> # explicit -S of course must sign.\n> - git tag tenth-signed $(echo 9 | git commit-tree -S HEAD^{tree})\n> + echo 10 | git commit-tree -S HEAD^{tree} >oid &&\n> + test_line_count = 1 oid &&\n> + git tag tenth-signed $(cat oid)\n> '\n\nOr, a bit simpler:\n\n oid=$(echo 10 | git commit-tree -S HEAD^{tree}) &&\n git tag tenth-signed \"$oid\"\n\nMartin\n"},{"id":"367179","messageId":"CAETBDP5Ve=85Jtkb55=htPO1eiZQmqG7deUX_BF6ih259gY-XQ@mail.gmail.com","threadId":"50265","inReplyTo":"20190119154552.12189-1-martin.agren@gmail.com","subject":"Re: [PATCH v3] commit-tree: add missing --gpg-sign flag","fromName":"Brandon Richardson","fromEmail":"brandon1024.br@gmail.com","sentAt":"2019-01-19T18:05:09Z","receivedAt":"2019-01-19T18:05:23Z","isPatch":true,"sender":{"key":"brandon1024.br@gmail.com","avatar":"https://avatars.githubusercontent.com/u/22732449?v=4"},"body":"Hi Martin,\n\n> I looked into this test in a bit more detail, and it seems to be quite\n> hard to get right. Part of the reason is that `git commit-tree` requires\n> a bit more careful use than `git commit`, but part of it is that the\n> tests that we already have for `git commit-tree [-S]` right before the\n> ones you're adding are a bit too loose, IMHO. So they're not ideal for\n> copy-pasting... I've come up with the patch below, which you might want\n> to use as a basis for your work.\n>\n> That is, you could `git am --scissors` this patch on a fresh branch and\n> `git commit --amend --signoff --no-edit` it (see\n> Documentation/SubmittingPatches, \"forwarding somebody else's patch\"),\n> then base your work on it, e.g., by cherry-picking your v3 commit.\n>\n> I think you would want to add 2x3 lines of tests (3 for `--gpg-sign`, 3\n> for `--gpg-sign=...`). That would give you eleventh-signed and\n> twelfth-signed and you wouldn't need any invocation of `git commit` (so\n> no thirteenth-signed).\n\nJust finished adding in the changes you suggested, and everything looks\ngood on my end. I based my changes on the patch you provided.\n\n> Or, a bit simpler:\n>\n> oid=$(echo 10 | git commit-tree -S HEAD^{tree}) &&\n> git tag tenth-signed \"$oid\"\n\nJust noticed your latest email. Do you prefer it this way? If so, I can amend\nwhat I have before I submit v4.\n\nWhen I submit v4, should I submit the patch you created as well, given\nthat my changes are based off of it?\n\nBrandon\n"},{"id":"367192","messageId":"CAN0heSo7CmuAYJGK5RjRkT9TX+RUyNDk-Rp_n-OCN8q1O6xNzA@mail.gmail.com","threadId":"50265","inReplyTo":"CAETBDP5Ve=85Jtkb55=htPO1eiZQmqG7deUX_BF6ih259gY-XQ@mail.gmail.com","subject":"Re: [PATCH v3] commit-tree: add missing --gpg-sign flag","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2019-01-19T21:18:46Z","receivedAt":"2019-01-19T21:19:04Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"Hi Brandon,\n\nOn Sat, 19 Jan 2019 at 19:05, Brandon Richardson\n wrote:\n> > I looked into this test in a bit more detail, and it seems to be quite\n> > hard to get right. Part of the reason is that `git commit-tree` requires\n> > a bit more careful use than `git commit`, but part of it is that the\n> > tests that we already have for `git commit-tree [-S]` right before the\n> > ones you're adding are a bit too loose, IMHO. So they're not ideal for\n> > copy-pasting... I've come up with the patch below, which you might want\n> > to use as a basis for your work.\n\n> Just finished adding in the changes you suggested, and everything looks\n> good on my end. I based my changes on the patch you provided.\n>\n> > Or, a bit simpler:\n> >\n> > oid=$(echo 10 | git commit-tree -S HEAD^{tree}) &&\n> > git tag tenth-signed \"$oid\"\n>\n> Just noticed your latest email. Do you prefer it this way?\n\nI think so, yeah. (But who knows what others might prefer? ;-) )\n\nThe use of \"\" around $oid is perhaps a bit subtle, but not too much so,\nI think. The \"test_line_count\" version was probably a bit too paranoid\nand verbose, for no real gain.\n\n> If so, I can amend\n> what I have before I submit v4.\n>\n> When I submit v4, should I submit the patch you created as well, given\n> that my changes are based off of it?\n\nI think the cleanest would be to submit a two-patch series, v4.\n\nAlternatively, you could submit only a patch of your own, but it should\nthen be based directly off of origin/master. So the test in it could\nbe inspired by my patch, but yours would not have mine as a parent and\nthe context lines of your patch would look like what is currently in\nmaster. My patch could then go on top of yours, as a \"the new tests are\nmore robust than these old ones; let's rewrite them to the new style\".\n\nThanks\nMartin\n"},{"id":"367193","messageId":"CAETBDP5xgPwu9ejg3j0oZGpwN0T1Me0n0CiaaCtrK37pZrJgZw@mail.gmail.com","threadId":"50265","inReplyTo":"CAN0heSo7CmuAYJGK5RjRkT9TX+RUyNDk-Rp_n-OCN8q1O6xNzA@mail.gmail.com","subject":"Re: [PATCH v3] commit-tree: add missing --gpg-sign flag","fromName":"Brandon Richardson","fromEmail":"brandon1024.br@gmail.com","sentAt":"2019-01-19T23:18:46Z","receivedAt":"2019-01-19T23:19:01Z","isPatch":true,"sender":{"key":"brandon1024.br@gmail.com","avatar":"https://avatars.githubusercontent.com/u/22732449?v=4"},"body":"Hi Martin,\n\nOn Sat, 19 Jan 2019 at 17:19, Martin Ågren wrote:\n> > > Or, a bit simpler:\n> > >\n> > > oid=$(echo 10 | git commit-tree -S HEAD^{tree}) &&\n> > > git tag tenth-signed \"$oid\"\n> >\n> > Just noticed your latest email. Do you prefer it this way?\n>\n> I think so, yeah. (But who knows what others might prefer? ;-) )\n>\n\nI'm personally a fan of your initial patch, I found it to be quite elegant.\nI think I'll submit your first version, and if people prefer another way\nwe will go in that direction.\n\n> I think the cleanest would be to submit a two-patch series, v4.\n\nFor simplicity, I'll do that :-)\n\nBrandon\n"}]}