Eike Rathke wrote: > These patches probably don't qualify to close #3695, but at least > a fingerprint is accepted as user input and fingerprints of keys are > displayed. 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
Hi Eike, Thank you for submitting another very nice patchset! I do have some issues and comments about the patches that I'd like to work through before committing though. 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. You can instead use (closes #1234) to close a ticket automatically when it's committed. Take a look at the bottom of http://trac.edgewall.org/wiki/CommitTicketUpdater to see more examples. Now, on to the code... 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. There is already a field we could use: fingerprint. (Currently only pgppubring uses that field, and rather than NULL terminate it, it uses the fp_len field.) If we use the same field, I'd suggest changing it to unsigned char fingerprint[41]; and adding a comment next to fp_len saying that fp_len is only used by pgppubring. I realize this change makes modifying parse_pub_line() a bit more involved, and the code will have to make sure a second fpr record for a subkey doesn't overwrite the primary record fingerprint when option(OPTPGPIGNORESUB) is set. 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. (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.) > 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. 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) 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)) > diff --git a/crypt-gpgme.c b/crypt-gpgme.c > if (!*p > - || (ps != pl && mutt_strcasecmp (pl, crypt_long_keyid (k)) == 0) > - || (ps == pl && mutt_strcasecmp (ps, crypt_short_keyid (k)) == 0) > + || (pfcopy && mutt_strcasecmp (pfcopy, crypt_fpr (k)) == 0) > + || (!pfcopy > + && ((pl && mutt_strcasecmp (pl, crypt_long_keyid (k)) == 0) > + || (ps && !pl && mutt_strcasecmp (ps, crypt_short_keyid (k)) > == 0))) > || mutt_stristr (k->uid, p)) And this could also be much clearer with just: if (!*p || (pfcopy && mutt_strcasecmp (pfcopy, crypt_fpr (k)) == 0) || (pl && mutt_strcasecmp (pl, crypt_long_keyid (k)) == 0) || (ps && mutt_strcasecmp (ps, crypt_short_keyid (k)) == 0) || mutt_stristr (k->uid, p)) > 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. > + 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); > + > + /* If at end of input, check for correct fingerprint length and copy if. */ > + pfcopy = (!c && (((s1 - pf) == 40) || ((s1 - pf) == 32)) ? safe_strdup > (pf) : NULL); I'm a little uncomfortable with the code just assuming anything with modulo-4 spaces and a correct length is a fingerprint. If someone happens to be named 'JohnPaulStan Tomifuluslow Blahblue' and I searched for that, the code would treat this as a fingerprint, and *more importantly*, would remove the spaces and return that as "phint" (and so nothing would be returned by the search). 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. 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? -Kevin
signature.asc
Description: PGP signature
