[PATCH v4 1/2] t7510: invoke git as part of &&-chain

[PATCH v4 1/2] t7510: invoke git as part of &&-chain

[Brandon Richardson]

From: Martin Ågren <martin.agren@gmail.com>

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>
Signed-off-by: Brandon Richardson <brandon1024.br@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 86d3f93fa..58f528b98 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' '
-- 
2.20.1

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

[Brandon Richardson]

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

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

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

[Martin Ågren]

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

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

[Junio C Hamano]

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.

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

[Martin Ågren]

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