Re: Signed commit regression?

[Hans Jerry Illikainen <hji@dyntopia.com>]

On Fri, Feb 28 2020, Junio C Hamano wrote:
> I'd expect that there may be another round of attempt to update the
> GPG interface.  Let's make sure we won't lose info given to the
> end-users while doing so.

The regression was introduced by this botched chunk in 72b006f4bf:

@@ -521,18 +522,19 @@ static int show_one_mergetag(struct commit *commit,
 	gpg_message_offset = verify_message.len;

 	payload_size = parse_signature(extra->value, extra->len);
 	status = -1;
 	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))
+		if (!check_signature(extra->value, payload_size,
+				     extra->value + payload_size,
+				     extra->len - payload_size, &sigc)) {
+			strbuf_addstr(&verify_message, sigc.gpg_output);
+			signature_check_clear(&sigc);
 			status = 0; /* good */
-		else if (verify_message.len <= gpg_message_offset)
+		} 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 */
 	}

There are two ridiculous bugs in my original patch:

1. The output from GPG is only added to `verify_message' if a signature
   verifies successfully.

2. On verification failure, the "No signature" message is always added
   to `verify_message'.  This is because, again, no output from GPG is
   added to `verify_message' on failure, so its length will always equal
   `gpg_message_offset' (see the initial assignment) when
   `check_signature()' returns non-zero, sigh...

Not sure of the proper way of fixing a reverted commit, but I'll send v1
based on pu that includes regression tests.

I'm sorry for my fuckup and the headache it caused!

-- 
hji