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

[Junio C Hamano <gitster@pobox.com>]

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

>> 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.