Re: [PATCH] log: correctly identify mergetag signature verification status

From

Junio C Hamano <gitster@pobox.com>

Date

Jul 10, 2014, 22:27 UTC

Message-ID

<xmqq7g3k7su5.fsf@gitster.dls.corp.google.com>

In-Reply-To

<20140628004458.GB13228@sigill.intra.peff.net>

Jeff King <peff@peff.net> writes:

> Perhaps it would be easier to read (and would have made the logic error
> you are fixing more obvious) as:
>
>   if (extra->len > payload_size) {
> 	if (!verify_signed_buffer(...))
> 		status = 0; /* good; all other code paths leave it as -1 */
> 	else if (verify_message.len <= gpg_message_offset)
> 		strbuf_addstr(&verify_message, "No signature\n");
>   }
>
> That is, for each conditional to check one more thing needed for a good
> signature, and then know that all other code paths leave status as -1.

Thanks.  Let's do it this way, then.

 log-tree.c | 21 +++++++++++----------
 1 file changed, 11 insertions(+), 10 deletions(-)

diff --git a/log-tree.c b/log-tree.c
index 1982631..b4bbfe1 100644
--- a/log-tree.c
+++ b/log-tree.c
@@ -446,16 +446,17 @@ static void show_one_mergetag(struct rev_info *opt,
 
 	payload_size = parse_signature(extra->value, extra->len);
 	status = -1;
-	if (extra->len > payload_size)
-		if (verify_signed_buffer(extra->value, payload_size,
-					 extra->value + payload_size,
-					 extra->len - payload_size,
-					 &verify_message, NULL)) {
-			if (verify_message.len <= gpg_message_offset)
-				strbuf_addstr(&verify_message, "No signature\n");
-			else
-				status = 0;
-		}
+	if (extra->len > payload_size) {
+		/* could have a good signature */
+		if (!verify_signed_buffer(extra->value, payload_size,
+					  extra->value + payload_size,
+					  extra->len - payload_size,
+					  &verify_message, NULL))
+			status = 0; /* good */
+		else if (verify_message.len <= gpg_message_offset)
+			strbuf_addstr(&verify_message, "No signature\n");
+		/* otherwise we couldn't verify, which is shown as bad */
+	}
 
 	show_sig_lines(opt, status, verify_message.buf);
 	strbuf_release(&verify_message);