On Wed, Mar 22, 2017 at 06:15:57PM -0400, Santiago Torres wrote:
> > > Like 2/3, this one also produces test failures for me. It looks like
> > > "verify-tag" does not show a tag which has been forged. I'm not sure if
> > > that's intentional (and the test is wrong) or a bug. +cc Santiago
> >
> > It appears that the test expected a broken one to be shown, and my
> > reading of its log message is that the change expected --format= to
> > be used with %G? so that scripts can tell between pass and fail?
> >
> > So if I have to judge, the code becoming silent for a tag that does
> > not pass verification is not doing what the commit wanted it to do.
> >
>
> Yes, considering the test name is:
>
> "verifying a forged tag with --format fail and format accordingly"
>
> It feels as if the behavior of verify-tag/tag -v is not the one
> intended. I could add two patches on top of those two commits.
I worked up the patch to do that (see below), but I got stumped trying
to write the commit message. Perhaps that is what the test intended, but
I don't think tag's --format understands "%G" codes at all. So you
cannot tell from the output if a tag was valid or not; you have to check
the exit code separately.
And if you do something like:
git tag -v --format='%(tag)' foo bar |
while read tag
do
...
done
you cannot tell at all which ones are bogus. Whereas with the current
behavior, the bogus ones are quietly omitted. Which can also be
confusing, but I'd think would generally err on the side of caution.
-Peff
---
diff --git a/builtin/tag.c b/builtin/tag.c
index fbb85ba3d..37e768db4 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -107,19 +107,19 @@ static int verify_tag(const char *name, const char *ref,
const unsigned char *sha1, const void *cb_data)
{
int flags;
+ int ret;
const char *fmt_pretty = cb_data;
flags = GPG_VERIFY_VERBOSE;
if (fmt_pretty)
flags = GPG_VERIFY_OMIT_STATUS;
- if (gpg_verify_tag(sha1, name, flags))
- return -1;
+ ret = gpg_verify_tag(sha1, name, flags);
if (fmt_pretty)
pretty_print_ref(name, sha1, fmt_pretty);
- return 0;
+ return ret;
}
static int do_sign(struct strbuf *buffer)
diff --git a/builtin/verify-tag.c b/builtin/verify-tag.c
index 5199553d9..cb1024ef4 100644
--- a/builtin/verify-tag.c
+++ b/builtin/verify-tag.c
@@ -62,10 +62,8 @@ int cmd_verify_tag(int argc, const char **argv, const char
*prefix)
continue;
}
- if (gpg_verify_tag(sha1, name, flags)) {
+ if (gpg_verify_tag(sha1, name, flags))
had_error = 1;
- continue;
- }
if (fmt_pretty)
pretty_print_ref(name, sha1, fmt_pretty);
diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh
index b53a2e5e4..633b08956 100755
--- a/t/t7004-tag.sh
+++ b/t/t7004-tag.sh
@@ -848,17 +848,17 @@ test_expect_success GPG 'verifying a forged tag should
fail' '
'
test_expect_success 'verifying a proper tag with --format pass and format
accordingly' '
- cat >expect <<-\EOF
+ cat >expect <<-\EOF &&
tagname : signed-tag
- EOF &&
+ EOF
git tag -v --format="tagname : %(tag)" "signed-tag" >actual &&
test_cmp expect actual
'
test_expect_success 'verifying a forged tag with --format fail and format
accordingly' '
- cat >expect <<-\EOF
+ cat >expect <<-\EOF &&
tagname : forged-tag
- EOF &&
+ EOF
test_must_fail git tag -v --format="tagname : %(tag)" "forged-tag"
>actual &&
test_cmp expect actual
'
diff --git a/t/t7030-verify-tag.sh b/t/t7030-verify-tag.sh
index d62ccbb98..ce37fd986 100755
--- a/t/t7030-verify-tag.sh
+++ b/t/t7030-verify-tag.sh
@@ -126,17 +126,17 @@ test_expect_success GPG 'verify multiple tags' '
'
test_expect_success 'verifying tag with --format' '
- cat >expect <<-\EOF
+ cat >expect <<-\EOF &&
tagname : fourth-signed
- EOF &&
+ EOF
git verify-tag --format="tagname : %(tag)" "fourth-signed" >actual &&
test_cmp expect actual
'
test_expect_success 'verifying a forged tag with --format fail and format
accordingly' '
- cat >expect <<-\EOF
+ cat >expect <<-\EOF &&
tagname : 7th forged-signed
- EOF &&
+ EOF
test_must_fail git verify-tag --format="tagname : %(tag)" $(cat
forged1.tag) >actual-forged &&
test_cmp expect actual-forged
'
