Hi Kevin, On Wednesday, 2015-02-11 19:57:18 -0800, Kevin J. McCarthy wrote:
> First, just a general note. If you add (see #3695) to the top of
> a patch, the commit will be associated with the ticket automatically.
Mentioning the number like in (part of #3695) does not? Ok, I'll use
'see' then.
> 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?
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.
> However, if the eventual goal is to
> switch the interface to show fingerprints, it would be better to have it
> in the pgp_key_t struct.
Makes sense in the long term.
> (As an aside, another problem with the current implementation is
> that trust values aren't present in the pgp_uid_t record for the
> fingerprint.)
Maybe that could be inherited from the primary uid.
> > For both the pgp_list_pubring_command and pgp_list_secring_command
> > need to contain the --with-fingerprint option, or have
> > with-fingerprint in ~/.gnupg/gpg.conf
>
> It would be good to include modifications to contrib/gpg.rc inside the
> patchset.
Yeah, I'll do.
> I would also suggest adding the '--with-fingerprint' option
> twice, so that subkeys also get a fingerprint record. (Which would of
> course only be used if OPTPGPIGNORESUB was unset)
Can do as well.
> In both of the "getkeybystr" modifications, I'd like to see the comparison
> kept more simple:
>
> > diff --git a/pgpkey.c b/pgpkey.c
>
> > if (!*p ||
> > - (ps != pl && mutt_strcasecmp (pl, pgp_long_keyid (k)) == 0) ||
> > - (ps == pl && mutt_strcasecmp (ps, pgp_short_keyid (k)) == 0))
> > + (!pfcopy &&
> > + ((pl && mutt_strcasecmp (pl, pgp_long_keyid (k)) == 0) ||
> > + (ps && !pl && mutt_strcasecmp (ps, pgp_short_keyid (k)) == 0))))
>
> 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.
> > diff --git a/crypt-gpgme.c b/crypt-gpgme.c
Same.
> > diff --git a/crypt.c b/crypt.c
> > + /* Check if a fingerprint is given, must be hex digits only, blanks
> > + * separating groups of 4 hex digits are allowed. Also pre-check for ID.
> > */
> > + 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 */
> > + }
>
> 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 ;-)
> 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.
I'll come up with amended patches later.
Eike
--
OpenPGP/GnuPG encrypted mail preferred in all private communication.
Key "ID" 0x65632D3A - 2265 D7F3 A7B0 95CC 3918 630B 6A6C D5B7 6563 2D3A
Better use 64-bit 0x6A6CD5B765632D3A here is why: https://evil32.com/
Care about Free Software, support the FSFE https://fsfe.org/support/?erack
Use LibreOffice! https://www.libreoffice.org/
signature.asc
Description: Digital signature
