| Message ID | 20190119033530.4241-1-brandon1024.br@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v3] commit-tree: add missing --gpg-sign flag | expand |
Hi Brandon, Thanks for a v3. On Sat, 19 Jan 2019 at 04:36, Brandon Richardson <brandon1024.br@gmail.com> wrote: > - if (skip_prefix(arg, "-S", &sign_commit)) > + if(!strcmp(arg, "--gpg-sign")) { (Same nit as Junio about the missing space after "if".) > + sign_commit = ""; Nice. ;-) > + continue; > + } > # commit.gpgsign is still on but this must not be signed > git tag ninth-unsigned $(echo 9 | git commit-tree HEAD^{tree}) && > # explicit -S of course must sign. > - git tag tenth-signed $(echo 9 | git commit-tree -S HEAD^{tree}) > + git tag tenth-signed $(echo 10 | git commit-tree -S HEAD^{tree}) && > + > + # --gpg-sign[=<key-id>] must sign. > + echo 11 >file && test_tick && git commit -S -a -m "eleventh signed" && > + git tag eleventh-signed && > + git commit-tree --gpg-sign -m "twelfth signed" HEAD^{tree} && > + git tag twelfth-signed && > + git commit-tree --gpg-sign=B7227189 -m "thirteenth signed" HEAD^{tree} && > + git tag thirteenth-signed > ' (These last two lines are not tab-indented, but indented by four spaces. They were perhaps mangled by some copy-pasting.) Running this test, we end up with three tags on one commit: eleventh-signed, twelfth-signed and thirteenth-signed. So as long as `git commit -S` works when we use the number 11, everything will pass, and we won't really test what we wanted to test. We will verify that `git commit-tree` doesn't choke on "--gpg-sign[=foo]", but we won't verify that it handles it correctly. (Just recently, it was pointed out on this list that `git log --count` won't complain about "--count", but won't act on it, either. So such errors are not unheard of.) I looked into this test in a bit more detail, and it seems to be quite hard to get right. Part of the reason is that `git commit-tree` requires a bit more careful use than `git commit`, but part of it is that the tests that we already have for `git commit-tree [-S]` right before the ones you're adding are a bit too loose, IMHO. So they're not ideal for copy-pasting... I've come up with the patch below, which you might want to use as a basis for your work. That is, you could `git am --scissors` this patch on a fresh branch and `git commit --amend --signoff --no-edit` it (see Documentation/SubmittingPatches, "forwarding somebody else's patch"), then base your work on it, e.g., by cherry-picking your v3 commit. I think you would want to add 2x3 lines of tests (3 for `--gpg-sign`, 3 for `--gpg-sign=...`). That would give you eleventh-signed and twelfth-signed and you wouldn't need any invocation of `git commit` (so no thirteenth-signed). If you're not up for that, just let me know and I could instead rebase your patch on top of mine and submit both as a v4. I think this has come along nicely, and now it's really just about having a robust test. Martin -- >8 -- Subject: [PATCH] t7510: invoke git as part of &&-chain If `git commit-tree HEAD^{tree}` fails on us and produces no output on stdout, we will substitute that empty string and execute `git tag ninth-unsigned`, i.e., we will tag HEAD rather than a newly created object. But we are lucky: we have a signature on HEAD, so we should eventually fail the next test, where we verify that "ninth-unsigned" is indeed unsigned. We have a similar problem a few lines later. If `git commit-tree -S` fails with no output, we will happily tag HEAD as "tenth-signed". Here, we are not so lucky. The tag ends up on the same commit as "eighth-signed-alt", and that's a signed commit, so t7510-signed-commit will pass, despite `git commit-tree -S` failing. Make these `git commit-tree` invocations a direct part of the &&-chain, so that we can rely less on luck and set a better example for future tests modeled after this one. Fix a 9/10 copy/paste error while at it. Signed-off-by: Martin Ågren <martin.agren@gmail.com> --- t/t7510-signed-commit.sh | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/t/t7510-signed-commit.sh b/t/t7510-signed-commit.sh index 86d3f93fa2..58f528b98f 100755 --- a/t/t7510-signed-commit.sh +++ b/t/t7510-signed-commit.sh @@ -49,9 +49,13 @@ test_expect_success GPG 'create signed commits' ' git tag eighth-signed-alt && # commit.gpgsign is still on but this must not be signed - git tag ninth-unsigned $(echo 9 | git commit-tree HEAD^{tree}) && + echo 9 | git commit-tree HEAD^{tree} >oid && + test_line_count = 1 oid && + git tag ninth-unsigned $(cat oid) && # explicit -S of course must sign. - git tag tenth-signed $(echo 9 | git commit-tree -S HEAD^{tree}) + echo 10 | git commit-tree -S HEAD^{tree} >oid && + test_line_count = 1 oid && + git tag tenth-signed $(cat oid) ' test_expect_success GPG 'verify and show signatures' '
On Sat, 19 Jan 2019 at 16:46, Martin Ågren <martin.agren@gmail.com> wrote: > # commit.gpgsign is still on but this must not be signed > - git tag ninth-unsigned $(echo 9 | git commit-tree HEAD^{tree}) && > + echo 9 | git commit-tree HEAD^{tree} >oid && > + test_line_count = 1 oid && > + git tag ninth-unsigned $(cat oid) && > # explicit -S of course must sign. > - git tag tenth-signed $(echo 9 | git commit-tree -S HEAD^{tree}) > + echo 10 | git commit-tree -S HEAD^{tree} >oid && > + test_line_count = 1 oid && > + git tag tenth-signed $(cat oid) > ' Or, a bit simpler: oid=$(echo 10 | git commit-tree -S HEAD^{tree}) && git tag tenth-signed "$oid" Martin
Hi Martin, > I looked into this test in a bit more detail, and it seems to be quite > hard to get right. Part of the reason is that `git commit-tree` requires > a bit more careful use than `git commit`, but part of it is that the > tests that we already have for `git commit-tree [-S]` right before the > ones you're adding are a bit too loose, IMHO. So they're not ideal for > copy-pasting... I've come up with the patch below, which you might want > to use as a basis for your work. > > That is, you could `git am --scissors` this patch on a fresh branch and > `git commit --amend --signoff --no-edit` it (see > Documentation/SubmittingPatches, "forwarding somebody else's patch"), > then base your work on it, e.g., by cherry-picking your v3 commit. > > I think you would want to add 2x3 lines of tests (3 for `--gpg-sign`, 3 > for `--gpg-sign=...`). That would give you eleventh-signed and > twelfth-signed and you wouldn't need any invocation of `git commit` (so > no thirteenth-signed). Just finished adding in the changes you suggested, and everything looks good on my end. I based my changes on the patch you provided. > Or, a bit simpler: > > oid=$(echo 10 | git commit-tree -S HEAD^{tree}) && > git tag tenth-signed "$oid" Just noticed your latest email. Do you prefer it this way? If so, I can amend what I have before I submit v4. When I submit v4, should I submit the patch you created as well, given that my changes are based off of it? Brandon
Hi Brandon, On Sat, 19 Jan 2019 at 19:05, Brandon Richardson <brandon1024.br@gmail.com> wrote: > > I looked into this test in a bit more detail, and it seems to be quite > > hard to get right. Part of the reason is that `git commit-tree` requires > > a bit more careful use than `git commit`, but part of it is that the > > tests that we already have for `git commit-tree [-S]` right before the > > ones you're adding are a bit too loose, IMHO. So they're not ideal for > > copy-pasting... I've come up with the patch below, which you might want > > to use as a basis for your work. > Just finished adding in the changes you suggested, and everything looks > good on my end. I based my changes on the patch you provided. > > > Or, a bit simpler: > > > > oid=$(echo 10 | git commit-tree -S HEAD^{tree}) && > > git tag tenth-signed "$oid" > > Just noticed your latest email. Do you prefer it this way? I think so, yeah. (But who knows what others might prefer? ;-) ) The use of "" around $oid is perhaps a bit subtle, but not too much so, I think. The "test_line_count" version was probably a bit too paranoid and verbose, for no real gain. > If so, I can amend > what I have before I submit v4. > > When I submit v4, should I submit the patch you created as well, given > that my changes are based off of it? I think the cleanest would be to submit a two-patch series, v4. Alternatively, you could submit only a patch of your own, but it should then be based directly off of origin/master. So the test in it could be inspired by my patch, but yours would not have mine as a parent and the context lines of your patch would look like what is currently in master. My patch could then go on top of yours, as a "the new tests are more robust than these old ones; let's rewrite them to the new style". Thanks Martin
Hi Martin, On Sat, 19 Jan 2019 at 17:19, Martin Ågren <martin.agren@gmail.com> wrote: > > > Or, a bit simpler: > > > > > > oid=$(echo 10 | git commit-tree -S HEAD^{tree}) && > > > git tag tenth-signed "$oid" > > > > Just noticed your latest email. Do you prefer it this way? > > I think so, yeah. (But who knows what others might prefer? ;-) ) > I'm personally a fan of your initial patch, I found it to be quite elegant. I think I'll submit your first version, and if people prefer another way we will go in that direction. > I think the cleanest would be to submit a two-patch series, v4. For simplicity, I'll do that :-) Brandon
diff --git a/builtin/commit-tree.c b/builtin/commit-tree.c index 9ec36a82b..298e499ac 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 86d3f93fa..095d4b254 100755 --- a/t/t7510-signed-commit.sh +++ b/t/t7510-signed-commit.sh @@ -51,13 +51,22 @@ test_expect_success GPG 'create signed commits' ' # commit.gpgsign is still on but this must not be signed git tag ninth-unsigned $(echo 9 | git commit-tree HEAD^{tree}) && # explicit -S of course must sign. - git tag tenth-signed $(echo 9 | git commit-tree -S HEAD^{tree}) + git tag tenth-signed $(echo 10 | git commit-tree -S HEAD^{tree}) && + + # --gpg-sign[=<key-id>] must sign. + echo 11 >file && test_tick && git commit -S -a -m "eleventh signed" && + git tag eleventh-signed && + git commit-tree --gpg-sign -m "twelfth signed" HEAD^{tree} && + git tag twelfth-signed && + git commit-tree --gpg-sign=B7227189 -m "thirteenth signed" HEAD^{tree} && + git tag thirteenth-signed ' 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 twelfth-signed thirteenth-signed do git verify-commit $commit && git show --pretty=short --show-signature $commit >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> --- Hi all, Third and (hopefully) final version. Thanks again Martin for the helpful comments. --- builtin/commit-tree.c | 8 +++++++- t/t7510-signed-commit.sh | 13 +++++++++++-- 2 files changed, 18 insertions(+), 3 deletions(-)