Hi Brandon,
On Fri, 18 Jan 2019 at 02:12, Brandon Richardson
<brandon1024.br@xxxxxxxxx> wrote:
>
> Add --gpg-sign option in commit-tree, which was documented, but not
> implemented, in 55ca3f99ae.
>
> Signed-off-by: Brandon Richardson <brandon1024.br@xxxxxxxxx>
> ---
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
