Eike Rathke wrote: > > For the pgp_getkeybystr() patch, I'd really like to have the fingerprint > > stored in the pgp_key_t structure, rather than in one of the address > > records. > > But then the fingerprint wouldn't be displayed in the list, or would it?
No, but right now, we also allow the user to enter a long id, even when
pgp_long_ids is unset, and it still only shows the short id in the
results.
> The nice side effect currently is, that the fingerprints of all matching
> keys are displayed, regardless whether a fingerprint was queried or not.
> I suggest to leave that to a follow-up change once displaying
> fingerprints is implemented properly.
The interface issues can be discussed and implemented later. The
purpose of this patchset is simply to allow the user to enter a
fingerprint when searching for a key.
Putting it in a pgp_uid_t is the easiest approach, but it's not the
correct approach. I'd really rather have it done right the first time.
> > In both of the "getkeybystr" modifications, I'd like to see the comparison
> > kept more simple:
> > Since pfcopy, pl, and ps are all mutually exclusive, it would be clearer
> > just to have:
> > if (!*p ||
> > (pfcopy && mutt_strcasecmp (pfcopy, k->fingerprint) == 0) ||
> > (pl && mutt_strcasecmp (pl, pgp_long_keyid (k)) == 0) ||
> > (ps && mutt_strcasecmp (ps, pgp_short_keyid (k)) == 0))
>
> But that would mean to rely on an implementation detail of
> crypt_get_fingerprint_or_id(). As is, implementation could be changed to
> assign pointers to all possible valid IDs found and the comparison here
> would still work correctly. If we simplify it, adding a contract to
> crypt_get_fingerprint_or_id(), at least in its description, would be
> necessary/helpful for the next dev.
Mutt isn't that big, and the function isn't sitting in an external
library. I'd say the current "pl is a long keyid only if != ps"
behavior is an implementation detail to be concerned about. However,
the new function's behavior is quite clear, so there's no reason to
make the loop less clear because "maybe" the crypt_get_fingerprint_or_id
function might change.
Furthermore, even if the behavior were changed so that pl and ps were
set to a substring of/in pfcopy, there wouldn't be a big or confusing
problem: the get_candidates() search is against the largest value.
> > You can ignore this comment, as it's your patch and mutt already has
> > plenty of code more complicated than this ;-). But wouldn't it be
> > clearer just to rename "s1" to something like "hex_digit_count" and
> > start it at 0? Having variables like s1, s2, and pf made me have to
> > stare at this code longer than I should to make sure I understood it.
>
> It's simple ;) s1 is the first, left pointer to copy to; s2 is the
> second, right pointer to copy from. But I can change it to whatever you
> wish if you want ;-)
(Again, this isn't important and you don't need to change anything, but
just to clarify my point).
I wasn't referring to
do
{
*(s1++) = *(s2 = mutt_skip_whitespace (s2));
} while (*(s2++));
I was talking about the first loop:
isid = 2; /* unknown */
s1 = s2 = pf;
do
{
c = *(s2++);
if (('0' <= c && c <= '9') || ('A' <= c && c <= 'F') || ('a' <= c && c <=
'f'))
{
++s1; /* hex digit */
if (isid == 2)
isid = 1; /* it is an ID so far */
}
else if (c)
{
isid = 0; /* not an ID */
if (c == ' ' && (((s1 - pf) % 4) == 0))
; /* skip blank before or after 4 hex digits */
else
break; /* any other character or position */
}
} while (c);
Here, s1 is a char*, and appears to be keeping track of a position in
the string, but in actuality it's only being used as a counter for the
number of hex digits. Once you understand the code, it's clear enough,
but on first reading using a char* when you really just need an integer
counter makes the code harder to read.
>
> > I'm a little uncomfortable with the code just assuming anything with
> > modulo-4 spaces and a correct length is a fingerprint.
>
> It doesn't, but you already found that out in the mean time ;-)
>
> > It's an edge case, but if we want to remove the spaces, I would be more
> > comfortable if we made sure all the characters were hex-digits, and
> > return a LIST with both the original and de-spaced versions as hints.
>
> Hmm.. given that only grouped hex digits of the correct length are
> stripped of spaces, I think this is not necessary (anymore)?
>
> > I did notice gpg will return the correct result if I search using
> > the fingerprint with spaces in it. Perhaps we could return the
> > "with-spaces" version as the hint, or just remove hint as a parameter
> > and just use "p" in the getkeybystr functions?
>
> With spaces works with gpg command line, I'm not sure how gpgme would
> handle that, I didn't try. Passing the stripped fingerprint works in all
> cases.
Sorry, please just ignore the above quoted feedback. I was tired and
got confused last night. :-)
-Kevin
signature.asc
Description: PGP signature
