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

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