Michał Górny <mgorny@xxxxxxxxxx> 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.
