{"thread":{"id":"50267","subject":"[PATCH v4 1/2] t7510: invoke git as part of &&-chain","startedAt":"2019-01-19T23:24:17Z","lastAt":"2019-01-22T21:43:55Z","messageCount":5,"participants":["Brandon Richardson","Martin Ågren","Junio C Hamano"],"isPatch":true,"patchVersion":4,"patchTotal":2},"messages":[{"id":"367194","messageId":"20190119232334.31646-1-brandon1024.br@gmail.com","threadId":"50267","inReplyTo":null,"subject":"[PATCH v4 1/2] t7510: invoke git as part of &&-chain","fromName":"Brandon Richardson","fromEmail":"brandon1024.br@gmail.com","sentAt":"2019-01-19T23:23:33Z","receivedAt":"2019-01-19T23:24:17Z","isPatch":true,"sender":{"key":"brandon1024.br@gmail.com","avatar":"https://avatars.githubusercontent.com/u/22732449?v=4"},"body":"From: Martin Ågren \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 \nSigned-off-by: Brandon Richardson \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 86d3f93fa..58f528b98 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\n\n"},{"id":"367195","messageId":"20190119232334.31646-2-brandon1024.br@gmail.com","threadId":"50267","inReplyTo":"20190119232334.31646-1-brandon1024.br@gmail.com","subject":"[PATCH v4 2/2] commit-tree: add missing --gpg-sign flag","fromName":"Brandon Richardson","fromEmail":"brandon1024.br@gmail.com","sentAt":"2019-01-19T23:23:34Z","receivedAt":"2019-01-19T23:24:29Z","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 builtin/commit-tree.c | 8 +++++++-\n t/t7510-signed-commit.sh | 15 ++++++++++++---\n 2 files changed, 19 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin/commit-tree.c b/builtin/commit-tree.c\nindex 9ec36a82b..12cc403bd 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 58f528b98..682b23a06 100755\n--- a/t/t7510-signed-commit.sh\n+++ b/t/t7510-signed-commit.sh\n@@ -55,13 +55,22 @@ test_expect_success GPG 'create signed commits' '\n \t# explicit -S of course must sign.\n \techo 10 | git commit-tree -S HEAD^{tree} >oid &&\n \ttest_line_count = 1 oid &&\n-\tgit tag tenth-signed $(cat oid)\n+\tgit tag tenth-signed $(cat oid) &&\n+\n+\t# --gpg-sign[=] must sign.\n+\techo 11 | git commit-tree --gpg-sign HEAD^{tree} >oid &&\n+\ttest_line_count = 1 oid &&\n+\tgit tag eleventh-signed $(cat oid) &&\n+\techo 12 | git commit-tree --gpg-sign=B7227189 HEAD^{tree} >oid &&\n+\ttest_line_count = 1 oid &&\n+\tgit tag twelfth-signed-alt $(cat oid)\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\n \t\tdo\n \t\t\tgit verify-commit $commit &&\n \t\t\tgit show --pretty=short --show-signature $commit >actual &&\n@@ -82,7 +91,7 @@ test_expect_success GPG 'verify and show signatures' '\n \t\tdone\n \t) &&\n \t(\n-\t\tfor commit in eighth-signed-alt\n+\t\tfor commit in eighth-signed-alt twelfth-signed-alt\n \t\tdo\n \t\t\tgit show --pretty=short --show-signature $commit >actual &&\n \t\t\tgrep \"Good signature from\" actual &&\n-- \n2.20.1\n\n"},{"id":"367198","messageId":"CAN0heSr3a9H46j3wiTwwbw7HFh4+4aFs5-qe=gtxYB3vC73KAA@mail.gmail.com","threadId":"50267","inReplyTo":"20190119232334.31646-2-brandon1024.br@gmail.com","subject":"Re: [PATCH v4 2/2] commit-tree: add missing --gpg-sign flag","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2019-01-20T09:02:03Z","receivedAt":"2019-01-20T09:02:20Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"Hi Brandon,\n\nOn Sun, 20 Jan 2019 at 00:24, Brandon Richardson\n wrote:\n> # explicit -S of course must sign.\n> echo 10 | git commit-tree -S HEAD^{tree} >oid &&\n> test_line_count = 1 oid &&\n> - git tag tenth-signed $(cat oid)\n> + git tag tenth-signed $(cat oid) &&\n> +\n> + # --gpg-sign[=] must sign.\n> + echo 11 | git commit-tree --gpg-sign HEAD^{tree} >oid &&\n> + test_line_count = 1 oid &&\n> + git tag eleventh-signed $(cat oid) &&\n> + echo 12 | git commit-tree --gpg-sign=B7227189 HEAD^{tree} >oid &&\n> + test_line_count = 1 oid &&\n> + git tag twelfth-signed-alt $(cat oid)\n> '\n\nThank you for following through.\n\nLet's see if there any opinions from others about this more verbose\nconstruction, vs placing the oid in a variable and quoting it. We\nobviously went several years without realizing that using $(...) as an\nobject id risked falling back to HEAD and that a completely broken `git\ncommit-tree -S` would pass the test. So being over-careful and extra\nobvious might very well be the right thing.\n\n> test_expect_success GPG 'verify and show signatures' '\n> (\n> for commit in initial second merge fourth-signed \\\n> - fifth-signed sixth-signed seventh-signed tenth-signed\n> + fifth-signed sixth-signed seventh-signed tenth-signed \\\n> + eleventh-signed\n> do\n> git verify-commit $commit &&\n> git show --pretty=short --show-signature $commit >actual &&\n> @@ -82,7 +91,7 @@ test_expect_success GPG 'verify and show signatures' '\n> done\n> ) &&\n> (\n> - for commit in eighth-signed-alt\n> + for commit in eighth-signed-alt twelfth-signed-alt\n> do\n> git show --pretty=short --show-signature $commit >actual &&\n> grep \"Good signature from\" actual &&\n\nAh, good catch. I didn't notice that we had a separate for-loop for this\nkey. This comes from 4baf839fe0 (\"t7510: test a commit signed by an\nunknown key\", 2014-06-16). What we want to test here is something\ndifferent, namely that we're using a specific, named key. But FWIW, I\nthink we're fine, and that we're not abusing the existing difference\nbetween these two loops too much.\n\nMartin\n"},{"id":"367342","messageId":"xmqqzhrsfr4c.fsf@gitster-ct.c.googlers.com","threadId":"50267","inReplyTo":"CAN0heSr3a9H46j3wiTwwbw7HFh4+4aFs5-qe=gtxYB3vC73KAA@mail.gmail.com","subject":"Re: [PATCH v4 2/2] commit-tree: add missing --gpg-sign flag","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-01-22T19:07:31Z","receivedAt":"2019-01-22T19:07:36Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Martin Ågren writes:\n\n>> + echo 11 | git commit-tree --gpg-sign HEAD^{tree} >oid &&\n>> + test_line_count = 1 oid &&\n>> + git tag eleventh-signed $(cat oid) &&\n>> +...\n> Let's see if there any opinions from others about this more verbose\n> construction, vs placing the oid in a variable and quoting it. We\n> obviously went several years without realizing that using $(...) as an\n> object id risked falling back to HEAD and that a completely broken `git\n> commit-tree -S` would pass the test. So being over-careful and extra\n> obvious might very well be the right thing.\n\nSorry, but I am not sure what issue you are worried about. If the\n\"commit-tree\" command failed in this construct:\n\n\toid=$(echo 11 | git commit-tree ...) &&\n\tgit tag eleventh-signed \"$oid\"\n\nwouldn't the &&-chain break after the assignment of an empty string\nto oid, skip \"git tag\" and make the whole test fail, with or without\n'$oid\" fed to \"git tag\" quoted? It is wrong not to quote \"$oid\" for\nthe \"git tag\" command (the test should not rely on the fact that the\nobject names given by \"git commit-tree\" have no $IFS in them), but\nthat is a separate issue.\n\n"},{"id":"367379","messageId":"CAN0heSrSAupNkRHCsgsvMoOE7VE=yxv0MkaOi4WmZQm_2_Foaw@mail.gmail.com","threadId":"50267","inReplyTo":"xmqqzhrsfr4c.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v4 2/2] commit-tree: add missing --gpg-sign flag","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2019-01-22T21:43:40Z","receivedAt":"2019-01-22T21:43:55Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"On Tue, 22 Jan 2019 at 20:07, Junio C Hamano wrote:\n>\n> Martin Ågren writes:\n>\n> >> + echo 11 | git commit-tree --gpg-sign HEAD^{tree} >oid &&\n> >> + test_line_count = 1 oid &&\n> >> + git tag eleventh-signed $(cat oid) &&\n> >> +...\n> > Let's see if there any opinions from others about this more verbose\n> > construction, vs placing the oid in a variable and quoting it. We\n> > obviously went several years without realizing that using $(...) as an\n> > object id risked falling back to HEAD and that a completely broken `git\n> > commit-tree -S` would pass the test. So being over-careful and extra\n> > obvious might very well be the right thing.\n>\n> Sorry, but I am not sure what issue you are worried about. If the\n> \"commit-tree\" command failed in this construct:\n>\n> oid=$(echo 11 | git commit-tree ...) &&\n> git tag eleventh-signed \"$oid\"\n>\n> wouldn't the &&-chain break after the assignment of an empty string\n> to oid, skip \"git tag\" and make the whole test fail, with or without\n> '$oid\" fed to \"git tag\" quoted?\n\nYes.\n\n> It is wrong not to quote \"$oid\" for\n> the \"git tag\" command (the test should not rely on the fact that the\n> object names given by \"git commit-tree\" have no $IFS in them), but\n> that is a separate issue.\n\nIt'd also protect against a failure mode where we get no output and a\nzero exit code. (Maybe that's ridiculous, but we're testing `git\ncommit-tree -S` here -- plus, I'm lazy, so I'd rather double-quote than\nthink. ;-) )\n\nBut you asked me what issue I worried about... To recap, master has a\ntest with a one-liner that doesn't bark if you completely drop the\nimplementation of `git commit-tree -S`. I don't think that's the\nworrying that you're puzzled about.\n\nI posted a three-line replacement that verified the exit code and quotes\nthe output, but which also has a pretty paranoid middle step to verify\nthat there was precisely one line of output. I then followed up with a\nless paranoid two-liner, which avoids some round-tripping, and which\ndoesn't verify the line count, but which rather assumes that `git tag`\nwill bark on a bad oid.\n\nI think that last thing is a fair assumption, and that's why I referred\nto the three-line test as being over-careful and extra obvious. I'm not\nworrying about the quoting as such.\n\nMartin\n"}]}