Re: [PATCH v3] gpg-interface.c: detect and reject multiple signatures on commits

From: Junio C Hamano <gitster@pobox.com>
Date: Oct 16, 2018, 02:13 UTC
Message-ID: <xmqqva623agh.fsf@gitster-ct.c.googlers.com>
In-Reply-To: <1539636266.1014.6.camel@gentoo.org>

Michał Górny <mgorny@gentoo.org> writes:

Show 34 quoted lines

>> OK, so the whole thing makes sense to me.
>> 
>> Having said that, if we wanted to short-circuit, I think
>> 
>>                 for (each line) {
>>                         for (each sigcheck_gpg_status[]) {
>>                                 if (not the one on line)
>>                                         continue;
>>                                 if (sigc->result != 'U') {
>>                                         if (sigc->key)
>>                                                 goto found_dup;
>>                                         sigc->key = make a copy;
>>                                         if (*next && sigc->result != 'E') {
>>                                                 if (sigc->signer)
>>                                                         goto found_dup;
>>                                                 sigc->signer = make a copy;
>>                                         }
>>                                 }
>>                                 break;
>>                         }
>>                 }
>>                 return;
>> 
>>         found_dup:
>>                 sigc->result = 'E';
>>                 FREE_AND_NULL(sigc->signer);
>>                 FREE_AND_NULL(sigc->key);
>>                 return;
>> 		
>> would also be fine.
>
> Do I understand correctly that you mean to take advantage that 'seen
> exclusive status' cases match 'seen key' cases?  I think this would be
> a little less readable.

Yes, the above is taking advantage of: exclusive ones do give us key and/or signer, so it is a sign that we've found collision between two exclusive status line if we need to free and replace.

But that was "whole thing makes sense, but if we wanted to...". I do not know if we want to short-circuit upon finding a single problem, or parse the whole thing to the end. I guess we could short-circuit while still using the "seen-exclusive" variable (we can just do so at the place seen-exclusive is incremented---if it is already one, then we know we have seen one already and we are looking at another one).

> That said, I was planning on next patch that replaced the "!= 'U'" test
> with explicit flags for whether a particular status includes key
> and UID.  If you'd agree with this direction, I think having this one
> separate as well would make sense.

Yup, it might be a bit over-engineered for this code, but we are adding the "exclusive" bit to the status[] array already, and I think it makes sense to also have "does this give us key?" and "does this tell us signer?" bit there.

Thanks.