[PATCH] commit-tree: add missing --gpg-sign flag

[PATCH] commit-tree: add missing --gpg-sign flag

[Brandon Richardson]

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(-)

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' '
-- 
2.20.1

Re: [PATCH] commit-tree: add missing --gpg-sign flag

[Martin Ågren]

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

Re: [PATCH] commit-tree: add missing --gpg-sign flag

[Brandon Richardson]

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

Re: [PATCH] commit-tree: add missing --gpg-sign flag

[Junio C Hamano]

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' '