From: Jeff King <peff@peff.net>
Date: Sat, 28 Jun 2014 00:44:59 GMT
Subject: Re: [PATCH] log: correctly identify mergetag signature verification status
Message-ID: <20140628004458.GB13228@sigill.intra.peff.net>
In-Reply-To: <41f5b516d13fd02408f66ee38704a6c1bb09cbb3.1403875032.git.git@drmicha.warpmail.net>

On Fri, Jun 27, 2014 at 03:18:36PM +0200, Michael J Gruber wrote:

> A wrong '}' made our code record the results of mergetag signature
> verification incorrectly.
> 
> Fix it.

I think this is the right thing to do, but we went from:

  if (...)
	if (...) {
		if (...)
			...
		else
			...
	}

to:

  if (...)
	if (...) {
		if (...)
			...
	}
	else
		...

I think part of the point of the original {} was to make it more clear
which else went with which if. Perhaps we should use more here.

I also find the logic a bit hard to follow in that the outer conditions are:

  if (needed_for_goodsig)
	if (sig_is_not_good)
		...

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.

-Peff