| Message ID | 20190118010918.43705-1-brandon1024.br@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | commit-tree: add missing --gpg-sign flag | expand |
Hi Brandon, On Fri, 18 Jan 2019 at 02:12, Brandon Richardson <brandon1024.br@gmail.com> wrote: > > Add --gpg-sign option in commit-tree, which was documented, but not > implemented, in 55ca3f99ae. > > Signed-off-by: Brandon Richardson <brandon1024.br@gmail.com> > --- Thank you for an updated version! > builtin/commit-tree.c | 8 +++++++- > t/t7510-signed-commit.sh | 4 +++- Ah, a test, nice. :-) > - if (skip_prefix(arg, "-S", &sign_commit)) > + if(!strcmp(arg, "--gpg-sign")) { > + skip_prefix(arg, "--gpg-sign", &sign_commit); I personally find this a bit convoluted, compared to just assigning "". Maybe there are reasons for doing it this way that I don't see? > + continue; > + } > + > + if (skip_prefix(arg, "-S", &sign_commit) || > + skip_prefix(arg, "--gpg-sign=", &sign_commit)) > continue; Looks good. > --- a/t/t7510-signed-commit.sh > +++ b/t/t7510-signed-commit.sh > @@ -51,7 +51,9 @@ 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 must sign. > + git tag eleventh-signed $(echo 11 | git commit-tree --gpg-sign HEAD^{tree}) > ' Thanks for providing a test, and thanks for fixing the "9"/"10" copy-paste error. (You might want to remark "Fix a 9/10 typo while we're here." in the commit message, especially since that line requires editing anyway, see next paragraph.) All of the commands above are suffixed with "&&" which means that the shell only keeps executing as long as the commands succeed. If any of those 20-30 commands fails, the shell will jump out of the &&-chain and land ... at this line you're adding. If that one succeeds, the test will be reported as succeeding. So please add a "&&" to the "10" line. All of that said ... if I add the missing "&&" and run your test on the *old* implementation, it still succeeds. The reason is that I grepped for the "best" place to put this, and directed you to a part of the test suite where it might be a bit too easy to copy existing code and end up with something non-ideal. Sorry about that. :-( What happens is that git commit-tree reports "fatal: Not a valid object name --gpg-sign", then we go on to happily execute `git tag eleventh-signed` where we've just substituted the empty string produced by git commit-tree. The verifications will succeed, because there's already a signature there... (BTW, the verifications happen further down, so you'll want to add "eleventh-signed" to the list of tags there.) One way of making the test more robust might be to add a brand new commit, similar to how it's done earlier in the test. It's not your fault that you fell into this trap. If you want to look into making this more robust -- and try running the test before and after your fix -- great! If you feel it's out of your comfort zone or interest range, just let me know and I could try to take it from here. > test_expect_success GPG 'verify and show signatures' ' BTW, here's that test where the signatures are verified. Martin
Hi Martin, Thanks again for your comments and patience, I appreciate it :-) > > - if (skip_prefix(arg, "-S", &sign_commit)) > > + if(!strcmp(arg, "--gpg-sign")) { > > + skip_prefix(arg, "--gpg-sign", &sign_commit); > > I personally find this a bit convoluted, compared to just assigning "". > Maybe there are reasons for doing it this way that I don't see? You're right, it is a bit convoluted. The reason I opted to do it this way is because I wanted to reuse part of "arg" by simply advancing the pointer, but this is silly and wastes cycles, and after thinking about it a bit more I realized how silly it was. > All of the commands above are suffixed with "&&" which means that the > shell only keeps executing as long as the commands succeed. If any of > those 20-30 commands fails, the shell will jump out of the &&-chain and > land ... at this line you're adding. If that one succeeds, the test will > be reported as succeeding. So please add a "&&" to the "10" line. Woops, I can't believe I missed that. > All of that said ... if I add the missing "&&" and run your test on the > *old* implementation, it still succeeds. The reason is that I grepped > for the "best" place to put this, and directed you to a part of the test > suite where it might be a bit too easy to copy existing code and end > up with something non-ideal. Sorry about that. :-( > > What happens is that git commit-tree reports "fatal: Not a valid object > name --gpg-sign", then we go on to happily execute `git tag > eleventh-signed` where we've just substituted the empty string produced > by git commit-tree. The verifications will succeed, because there's > already a signature there... (BTW, the verifications happen further > down, so you'll want to add "eleventh-signed" to the list of tags > there.) No need to apologize, I jumped the gun. I'm going to look into putting together a more robust test for this change. Brandon
Brandon Richardson <brandon1024.br@gmail.com> writes: > - if (skip_prefix(arg, "-S", &sign_commit)) > + if(!strcmp(arg, "--gpg-sign")) { Style. "if (!strcmp(arg, "--gpg-sign")) {" > + skip_prefix(arg, "--gpg-sign", &sign_commit); > + continue; Technically, skipping the prefix S of string S will make us point at an empty substring at the end. So from that point of view, skip_prefix(arg, "--gpg-sign", &sign_commit) is not incorrect per-se, but it is highly misleading. We have already determined that the user gave us "--gpg-sign" option without anything after it, so we want to summon the "use the default key" behaviour by giving an empty string to sign_commit. An explicit assignment sign_commit = ""; would be a lot more readable and make the intent a lot more clear. > + } > + > + if (skip_prefix(arg, "-S", &sign_commit) || > + skip_prefix(arg, "--gpg-sign=", &sign_commit)) This side is OK. "-S" gives us an empty string, but "-Skeyid" gives us "keyid" in 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..efc136eaf 100755 > --- a/t/t7510-signed-commit.sh > +++ b/t/t7510-signed-commit.sh > @@ -51,7 +51,9 @@ 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 must sign. > + git tag eleventh-signed $(echo 11 | git commit-tree --gpg-sign HEAD^{tree}) > ' > > test_expect_success GPG 'verify and show signatures' '
diff --git a/builtin/commit-tree.c b/builtin/commit-tree.c index 9ec36a82b..a51b2c8d7 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")) { + skip_prefix(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..efc136eaf 100755 --- a/t/t7510-signed-commit.sh +++ b/t/t7510-signed-commit.sh @@ -51,7 +51,9 @@ 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 must sign. + git tag eleventh-signed $(echo 11 | git commit-tree --gpg-sign HEAD^{tree}) ' test_expect_success GPG 'verify and show signatures' '
Add --gpg-sign option in commit-tree, which was documented, but not implemented, in 55ca3f99ae. Signed-off-by: Brandon Richardson <brandon1024.br@gmail.com> --- Thanks Martin for the tips and suggestions! builtin/commit-tree.c | 8 +++++++- t/t7510-signed-commit.sh | 4 +++- 2 files changed, 10 insertions(+), 2 deletions(-)