| Message ID | 20190119232334.31646-2-brandon1024.br@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v4,1/2] t7510: invoke git as part of &&-chain | expand |
Hi Brandon, On Sun, 20 Jan 2019 at 00:24, Brandon Richardson <brandon1024.br@gmail.com> wrote: > # explicit -S of course must sign. > echo 10 | git commit-tree -S HEAD^{tree} >oid && > test_line_count = 1 oid && > - git tag tenth-signed $(cat oid) > + git tag tenth-signed $(cat oid) && > + > + # --gpg-sign[=<key-id>] must sign. > + echo 11 | git commit-tree --gpg-sign HEAD^{tree} >oid && > + test_line_count = 1 oid && > + git tag eleventh-signed $(cat oid) && > + echo 12 | git commit-tree --gpg-sign=B7227189 HEAD^{tree} >oid && > + test_line_count = 1 oid && > + git tag twelfth-signed-alt $(cat oid) > ' Thank you for following through. Let's see if there any opinions from others about this more verbose construction, vs placing the oid in a variable and quoting it. We obviously went several years without realizing that using $(...) as an object id risked falling back to HEAD and that a completely broken `git commit-tree -S` would pass the test. So being over-careful and extra obvious might very well be the right thing. > test_expect_success GPG 'verify and show signatures' ' > ( > for commit in initial second merge fourth-signed \ > - fifth-signed sixth-signed seventh-signed tenth-signed > + fifth-signed sixth-signed seventh-signed tenth-signed \ > + eleventh-signed > do > git verify-commit $commit && > git show --pretty=short --show-signature $commit >actual && > @@ -82,7 +91,7 @@ test_expect_success GPG 'verify and show signatures' ' > done > ) && > ( > - for commit in eighth-signed-alt > + for commit in eighth-signed-alt twelfth-signed-alt > do > git show --pretty=short --show-signature $commit >actual && > grep "Good signature from" actual && Ah, good catch. I didn't notice that we had a separate for-loop for this key. This comes from 4baf839fe0 ("t7510: test a commit signed by an unknown key", 2014-06-16). What we want to test here is something different, namely that we're using a specific, named key. But FWIW, I think we're fine, and that we're not abusing the existing difference between these two loops too much. Martin
Martin Ågren <martin.agren@gmail.com> writes: >> + echo 11 | git commit-tree --gpg-sign HEAD^{tree} >oid && >> + test_line_count = 1 oid && >> + git tag eleventh-signed $(cat oid) && >> +... > Let's see if there any opinions from others about this more verbose > construction, vs placing the oid in a variable and quoting it. We > obviously went several years without realizing that using $(...) as an > object id risked falling back to HEAD and that a completely broken `git > commit-tree -S` would pass the test. So being over-careful and extra > obvious might very well be the right thing. Sorry, but I am not sure what issue you are worried about. If the "commit-tree" command failed in this construct: oid=$(echo 11 | git commit-tree ...) && git tag eleventh-signed "$oid" wouldn't the &&-chain break after the assignment of an empty string to oid, skip "git tag" and make the whole test fail, with or without '$oid" fed to "git tag" quoted? It is wrong not to quote "$oid" for the "git tag" command (the test should not rely on the fact that the object names given by "git commit-tree" have no $IFS in them), but that is a separate issue.
On Tue, 22 Jan 2019 at 20:07, Junio C Hamano <gitster@pobox.com> wrote: > > Martin Ågren <martin.agren@gmail.com> writes: > > >> + echo 11 | git commit-tree --gpg-sign HEAD^{tree} >oid && > >> + test_line_count = 1 oid && > >> + git tag eleventh-signed $(cat oid) && > >> +... > > Let's see if there any opinions from others about this more verbose > > construction, vs placing the oid in a variable and quoting it. We > > obviously went several years without realizing that using $(...) as an > > object id risked falling back to HEAD and that a completely broken `git > > commit-tree -S` would pass the test. So being over-careful and extra > > obvious might very well be the right thing. > > Sorry, but I am not sure what issue you are worried about. If the > "commit-tree" command failed in this construct: > > oid=$(echo 11 | git commit-tree ...) && > git tag eleventh-signed "$oid" > > wouldn't the &&-chain break after the assignment of an empty string > to oid, skip "git tag" and make the whole test fail, with or without > '$oid" fed to "git tag" quoted? Yes. > It is wrong not to quote "$oid" for > the "git tag" command (the test should not rely on the fact that the > object names given by "git commit-tree" have no $IFS in them), but > that is a separate issue. It'd also protect against a failure mode where we get no output and a zero exit code. (Maybe that's ridiculous, but we're testing `git commit-tree -S` here -- plus, I'm lazy, so I'd rather double-quote than think. ;-) ) But you asked me what issue I worried about... To recap, master has a test with a one-liner that doesn't bark if you completely drop the implementation of `git commit-tree -S`. I don't think that's the worrying that you're puzzled about. I posted a three-line replacement that verified the exit code and quotes the output, but which also has a pretty paranoid middle step to verify that there was precisely one line of output. I then followed up with a less paranoid two-liner, which avoids some round-tripping, and which doesn't verify the line count, but which rather assumes that `git tag` will bark on a bad oid. I think that last thing is a fair assumption, and that's why I referred to the three-line test as being over-careful and extra obvious. I'm not worrying about the quoting as such. Martin
diff --git a/builtin/commit-tree.c b/builtin/commit-tree.c index 9ec36a82b..12cc403bd 100644 --- a/builtin/commit-tree.c +++ b/builtin/commit-tree.c @@ -66,7 +66,13 @@ int cmd_commit_tree(int argc, const char **argv, const char *prefix) continue; } - if (skip_prefix(arg, "-S", &sign_commit)) + if (!strcmp(arg, "--gpg-sign")) { + sign_commit = ""; + continue; + } + + if (skip_prefix(arg, "-S", &sign_commit) || + skip_prefix(arg, "--gpg-sign=", &sign_commit)) continue; if (!strcmp(arg, "--no-gpg-sign")) { diff --git a/t/t7510-signed-commit.sh b/t/t7510-signed-commit.sh index 58f528b98..682b23a06 100755 --- a/t/t7510-signed-commit.sh +++ b/t/t7510-signed-commit.sh @@ -55,13 +55,22 @@ test_expect_success GPG 'create signed commits' ' # explicit -S of course must sign. echo 10 | git commit-tree -S HEAD^{tree} >oid && test_line_count = 1 oid && - git tag tenth-signed $(cat oid) + git tag tenth-signed $(cat oid) && + + # --gpg-sign[=<key-id>] must sign. + echo 11 | git commit-tree --gpg-sign HEAD^{tree} >oid && + test_line_count = 1 oid && + git tag eleventh-signed $(cat oid) && + echo 12 | git commit-tree --gpg-sign=B7227189 HEAD^{tree} >oid && + test_line_count = 1 oid && + git tag twelfth-signed-alt $(cat oid) ' test_expect_success GPG 'verify and show signatures' ' ( for commit in initial second merge fourth-signed \ - fifth-signed sixth-signed seventh-signed tenth-signed + fifth-signed sixth-signed seventh-signed tenth-signed \ + eleventh-signed do git verify-commit $commit && git show --pretty=short --show-signature $commit >actual && @@ -82,7 +91,7 @@ test_expect_success GPG 'verify and show signatures' ' done ) && ( - for commit in eighth-signed-alt + for commit in eighth-signed-alt twelfth-signed-alt do git show --pretty=short --show-signature $commit >actual && grep "Good signature from" actual &&
Add --gpg-sign option in commit-tree, which was documented, but not implemented, in 55ca3f99ae. Add tests for the --gpg-sign option. Signed-off-by: Brandon Richardson <brandon1024.br@gmail.com> --- builtin/commit-tree.c | 8 +++++++- t/t7510-signed-commit.sh | 15 ++++++++++++--- 2 files changed, 19 insertions(+), 4 deletions(-)