Re: [PATCH 3/3] t7004, t7030: fix here-doc syntax errors

From: Jeff King <peff@peff.net>
Date: Mar 22, 2017, 22:22 UTC
Message-ID: <20170322222230.yqqv6x4gokvb4jbz@sigill.intra.peff.net>
In-Reply-To: <20170322221556.j7uj4vvgbcubcr3b@LykOS.localdomain>

On Wed, Mar 22, 2017 at 06:15:57PM -0400, Santiago Torres wrote:

Show 18 quoted lines

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