* [PATCH v4 1/2] t7510: invoke git as part of &&-chain
@ 2019-01-19 23:23 Brandon Richardson
2019-01-19 23:23 ` [PATCH v4 2/2] commit-tree: add missing --gpg-sign flag Brandon Richardson
0 siblings, 1 reply; 5+ messages in thread
From: Brandon Richardson @ 2019-01-19 23:23 UTC (permalink / raw)
To: git, martin.agren, gitster; +Cc: 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
^ permalink raw reply related [flat|nested] 5+ messages in thread
* [PATCH v4 2/2] commit-tree: add missing --gpg-sign flag
2019-01-19 23:23 [PATCH v4 1/2] t7510: invoke git as part of &&-chain Brandon Richardson
@ 2019-01-19 23:23 ` Brandon Richardson
2019-01-20 9:02 ` Martin Ågren
0 siblings, 1 reply; 5+ messages in thread
From: Brandon Richardson @ 2019-01-19 23:23 UTC (permalink / raw)
To: git, martin.agren, gitster; +Cc: 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
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH v4 2/2] commit-tree: add missing --gpg-sign flag
2019-01-19 23:23 ` [PATCH v4 2/2] commit-tree: add missing --gpg-sign flag Brandon Richardson
@ 2019-01-20 9:02 ` Martin Ågren
2019-01-22 19:07 ` Junio C Hamano
0 siblings, 1 reply; 5+ messages in thread
From: Martin Ågren @ 2019-01-20 9:02 UTC (permalink / raw)
To: Brandon Richardson; +Cc: Git Mailing List, Junio C Hamano
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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v4 2/2] commit-tree: add missing --gpg-sign flag
2019-01-20 9:02 ` Martin Ågren
@ 2019-01-22 19:07 ` Junio C Hamano
2019-01-22 21:43 ` Martin Ågren
0 siblings, 1 reply; 5+ messages in thread
From: Junio C Hamano @ 2019-01-22 19:07 UTC (permalink / raw)
To: Martin Ågren; +Cc: Brandon Richardson, Git Mailing List
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.
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v4 2/2] commit-tree: add missing --gpg-sign flag
2019-01-22 19:07 ` Junio C Hamano
@ 2019-01-22 21:43 ` Martin Ågren
0 siblings, 0 replies; 5+ messages in thread
From: Martin Ågren @ 2019-01-22 21:43 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Brandon Richardson, Git Mailing List
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
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2019-01-22 21:43 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-01-19 23:23 [PATCH v4 1/2] t7510: invoke git as part of &&-chain Brandon Richardson
2019-01-19 23:23 ` [PATCH v4 2/2] commit-tree: add missing --gpg-sign flag Brandon Richardson
2019-01-20 9:02 ` Martin Ågren
2019-01-22 19:07 ` Junio C Hamano
2019-01-22 21:43 ` Martin Ågren
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).