[PATCH] _gpg: Use explicit UIDs for public / secret keys.

[PATCH] _gpg: Use explicit UIDs for public / secret keys.

[doron.behar]

From: Doron Behar <doron.behar@gmail.com>

Use the `--with-colons` option and parse the output.
---
 Completion/Unix/Command/_gpg | 69 ++++++++++++++++++++++++++++++++----
 1 file changed, 63 insertions(+), 6 deletions(-)

diff --git a/Completion/Unix/Command/_gpg b/Completion/Unix/Command/_gpg
index 48a36eff2..7e707c5f6 100644
--- a/Completion/Unix/Command/_gpg
+++ b/Completion/Unix/Command/_gpg
@@ -206,20 +206,77 @@ fi
 
 case "$state" in
   public-keys)
-    _wanted public-keys expl 'public key' \
-	compadd ${${(Mo)$(_call_program public-keys $words[1] $needed --list-public-keys --list-options no-show-photos):%<*>}//(<|>)/} && return
+    local public_keys=(${(@s.:.)$(_call_program public-keys eval IFS=$'\n' $words[1] $needed --list-public-keys --list-options no-show-photos --with-colons)})
+    local -a uids_and_emails
+    local i
+    for i in {1..${#public_keys[@]}}; do
+      if [[ ${public_keys[$i]} == "fpr" ]]; then
+        i=$((i + 1))
+        local j=$i
+        while [[ ${public_keys[$j]} != "fpr" ]] && [ $j -lt ${#public_keys[@]} ]; do
+          if [[ ${public_keys[$j]} =~ "@" ]]; then
+            local email="${public_keys[$j]}"
+            local uid=${public_keys[$i]}
+            uids_and_emails+=("${uid}":"${email}")
+            i=$j
+            break
+          fi
+          j=$((j + 1))
+        done
+        i=$j
+      fi
+    done
+    _describe -t public-keys 'public key' uids_and_emails
   ;;
   secret-keys)
-    _wanted secret-keys expl 'secret key' compadd \
-	${${(Mo)$(_call_program secret-keys $words[1] $needed --list-secret-keys --list-options no-show-photos):%<*>}//(<|>)/} && return
+    local secret_keys=(${(@s.:.)$(_call_program secret-keys eval IFS=$'\n' $words[1] $needed --list-secret-keys --list-options no-show-photos --with-colons)})
+    local -a uids_and_emails
+    local i
+    for i in {1..${#secret_keys[@]}}; do
+      if [[ ${secret_keys[$i]} == "fpr" ]]; then
+        i=$((i + 1))
+        local j=$i
+        while [[ ${secret_keys[$j]} != "fpr" ]] && [ $j -lt ${#secret_keys[@]} ]; do
+          if [[ ${secret_keys[$j]} =~ "@" ]]; then
+            local email="${secret_keys[$j]}"
+            local uid=${secret_keys[$i]}
+            uids_and_emails+=("${uid}":"${email}")
+            i=$j
+            break
+          fi
+          j=$((j + 1))
+        done
+        i=$j
+      fi
+    done
+    _describe -t secret-keys 'secret key' uids_and_emails
   ;;
   ciphers)
     _wanted ciphers expl cipher compadd \
         ${${(s.,.)${(M)${(f)${"$(_call_program ciphers $words[1] $needed --version)"}//,$'\n' #/, }:#Cipher*}#*:}# } && return
   ;;
   (public-keyids)
-    _wanted public-keys expl 'public keyid' \
-      compadd ${(M)${${(f)"$(_call_program public-keyids $words[1] $needed --list-public-keys --list-options no-show-photos)"}## #}:#[0-9A-F](#c40)} && return
+    local public_keys=(${(@s.:.)$(_call_program public-keyids eval IFS=$'\n' $words[1] $needed --list-public-keys --list-options no-show-photos --with-colons)})
+    local -a uids_and_emails
+    local i
+    for i in {1..${#public_keys[@]}}; do
+      if [[ ${public_keys[$i]} == "fpr" ]]; then
+        i=$((i + 1))
+        local j=$i
+        while [[ ${public_keys[$j]} != "fpr" ]] && [ $j -lt ${#public_keys[@]} ]; do
+          if [[ ${public_keys[$j]} =~ "@" ]]; then
+            local email="${public_keys[$j]}"
+            local uid=${public_keys[$i]}
+            uids_and_emails+=("${uid}":"${email}")
+            i=$j
+            break
+          fi
+          j=$((j + 1))
+        done
+        i=$j
+      fi
+    done
+    _describe -t public-keyids 'public keyids' uids_and_emails
   ;;
   (option-list)
     _sequence _wanted options expl option \
-- 
2.17.1

Re: [PATCH] _gpg: Use explicit UIDs for public / secret keys.

[Daniel Shahaf]

doron.behar@gmail.com wrote on Sat, Jun 02, 2018 at 18:26:51 +0300:
> From: Doron Behar <doron.behar@gmail.com>
> 
> Use the `--with-colons` option and parse the output.
> ---
>  Completion/Unix/Command/_gpg | 69 ++++++++++++++++++++++++++++++++----
>  1 file changed, 63 insertions(+), 6 deletions(-)
> 
> diff --git a/Completion/Unix/Command/_gpg b/Completion/Unix/Command/_gpg
> index 48a36eff2..7e707c5f6 100644
> --- a/Completion/Unix/Command/_gpg
> +++ b/Completion/Unix/Command/_gpg
> @@ -206,20 +206,77 @@ fi
>  
>  case "$state" in
>    public-keys)
> -    _wanted public-keys expl 'public key' \
> -	compadd ${${(Mo)$(_call_program public-keys $words[1] $needed --list-public-keys --list-options no-show-photos):%<*>}//(<|>)/} && return
> +    local public_keys=(${(@s.:.)$(_call_program public-keys eval IFS=$'\n' $words[1] $needed --list-public-keys --list-options no-show-photos --with-colons)})

This isn't quite right.

The first argument to «eval» here is the five bytes «IFS=\n» (where \n stands
for an 0x0A byte), so the eval'd code sets IFS to the empty string, not to the
one-byte string $'\n', and the assignment isn't specific to the command either.

I'm not sure what value you _meant_ to set IFS to.  If you meant to set it to a
newline, you could do something like this:

    …$(_call_program public-keys eval IFS=${(q):-$'\n'} ${(q)words[1]} ${(q)needed} --list-public-keys --list-options no-show-photos --with-colons)…

I also added (q) to the other variable expansions.

Re: [PATCH] _gpg: Use explicit UIDs for public / secret keys.

[Doron Behar]

On Sun, Jun 03, 2018 at 09:43:50PM +0000, Daniel Shahaf wrote:
> doron.behar@gmail.com wrote on Sat, Jun 02, 2018 at 18:26:51 +0300:
> > From: Doron Behar <doron.behar@gmail.com>
> > 
> > Use the `--with-colons` option and parse the output.
> > ---
> >  Completion/Unix/Command/_gpg | 69 ++++++++++++++++++++++++++++++++----
> >  1 file changed, 63 insertions(+), 6 deletions(-)
> > 
> > diff --git a/Completion/Unix/Command/_gpg b/Completion/Unix/Command/_gpg
> > index 48a36eff2..7e707c5f6 100644
> > --- a/Completion/Unix/Command/_gpg
> > +++ b/Completion/Unix/Command/_gpg
> > @@ -206,20 +206,77 @@ fi
> >  
> >  case "$state" in
> >    public-keys)
> > -    _wanted public-keys expl 'public key' \
> > -	compadd ${${(Mo)$(_call_program public-keys $words[1] $needed --list-public-keys --list-options no-show-photos):%<*>}//(<|>)/} && return
> > +    local public_keys=(${(@s.:.)$(_call_program public-keys eval IFS=$'\n' $words[1] $needed --list-public-keys --list-options no-show-photos --with-colons)})
> 
> This isn't quite right.
> 
> The first argument to «eval» here is the five bytes «IFS=\n» (where \n stands
> for an 0x0A byte), so the eval'd code sets IFS to the empty string, not to the
> one-byte string $'\n', and the assignment isn't specific to the command either.
> 
> I'm not sure what value you _meant_ to set IFS to.  If you meant to set it to a
> newline, you could do something like this:
> 
>     …$(_call_program public-keys eval IFS=${(q):-$'\n'} ${(q)words[1]} ${(q)needed} --list-public-keys --list-options no-show-photos --with-colons)…
> 
> I also added (q) to the other variable expansions.

Actually, I've mistakenly sent the patch with eval, I got confused with
`env` which was my original intention. Both using my original version
with `env` instead of `eval` and your version with the quoted IFS work,
what shall it be?

Re: [PATCH] _gpg: Use explicit UIDs for public / secret keys.

[Daniel Shahaf]

Doron Behar wrote on Tue, 05 Jun 2018 18:47 +0300:
> On Sun, Jun 03, 2018 at 09:43:50PM +0000, Daniel Shahaf wrote:
> > doron.behar@gmail.com wrote on Sat, Jun 02, 2018 at 18:26:51 +0300:
> > > From: Doron Behar <doron.behar@gmail.com>
> > > 
> > > Use the `--with-colons` option and parse the output.
> > > ---
> > >  Completion/Unix/Command/_gpg | 69 ++++++++++++++++++++++++++++++++----
> > >  1 file changed, 63 insertions(+), 6 deletions(-)
> > > 
> > > diff --git a/Completion/Unix/Command/_gpg b/Completion/Unix/Command/_gpg
> > > index 48a36eff2..7e707c5f6 100644
> > > --- a/Completion/Unix/Command/_gpg
> > > +++ b/Completion/Unix/Command/_gpg
> > > @@ -206,20 +206,77 @@ fi
> > >  
> > >  case "$state" in
> > >    public-keys)
> > > -    _wanted public-keys expl 'public key' \
> > > -	compadd ${${(Mo)$(_call_program public-keys $words[1] $needed --list-public-keys --list-options no-show-photos):%<*>}//(<|>)/} && return
> > > +    local public_keys=(${(@s.:.)$(_call_program public-keys eval IFS=$'\n' $words[1] $needed --list-public-keys --list-options no-show-photos --with-colons)})
> > 
> > This isn't quite right.
> > 
> > The first argument to «eval» here is the five bytes «IFS=\n» (where \n stands
> > for an 0x0A byte), so the eval'd code sets IFS to the empty string, not to the
> > one-byte string $'\n', and the assignment isn't specific to the command either.
> > 
> > I'm not sure what value you _meant_ to set IFS to.  If you meant to set it to a
> > newline, you could do something like this:
> > 
> >     …$(_call_program public-keys eval IFS=${(q):-$'\n'} ${(q)words[1]} ${(q)needed} --list-public-keys --list-options no-show-photos --with-colons)…
> > 
> > I also added (q) to the other variable expansions.
> 
> Actually, I've mistakenly sent the patch with eval, I got confused with
> `env` which was my original intention. Both using my original version
> with `env` instead of `eval` and your version with the quoted IFS work,
> what shall it be?

IFS should never be exported, so use eval please.

Please use double escaping, though (that is, ${(q)${(q)foo}}: that's needed since
_call_program does an eval anyway, in addition to the one you spell out
in _call_program's arguments.

Re: [PATCH] _gpg: Use explicit UIDs for public / secret keys.

[Doron Behar]

Very weirdly, from some reason, when I tried to test both with `env` and
with `eval`, sometimes it worked and sometimes not, no idea why it
wasn't even consistent.

Therefor, after reading the documentation, I've created a better and
probably truly deterministic to do it. Here it is including the quoted
`words[1]` and `needed`:

    local public_keys=(${(@s.:.)${(f)"$(_call_program public-keys ${(q)words[1]} ${(q)needed} --list-public-keys --list-options no-show-photos --with-colons)"}})

I hope you'll find it better. I'm sending a final patch after this
message, tell me if there is anything more to improve.

On Thu, Jun 07, 2018 at 06:40:14AM +0000, Daniel Shahaf wrote:
> Doron Behar wrote on Tue, 05 Jun 2018 18:47 +0300:
> > On Sun, Jun 03, 2018 at 09:43:50PM +0000, Daniel Shahaf wrote:
> > > doron.behar@gmail.com wrote on Sat, Jun 02, 2018 at 18:26:51 +0300:
> > > > From: Doron Behar <doron.behar@gmail.com>
> > > > 
> > > > Use the `--with-colons` option and parse the output.
> > > > ---
> > > >  Completion/Unix/Command/_gpg | 69 ++++++++++++++++++++++++++++++++----
> > > >  1 file changed, 63 insertions(+), 6 deletions(-)
> > > > 
> > > > diff --git a/Completion/Unix/Command/_gpg b/Completion/Unix/Command/_gpg
> > > > index 48a36eff2..7e707c5f6 100644
> > > > --- a/Completion/Unix/Command/_gpg
> > > > +++ b/Completion/Unix/Command/_gpg
> > > > @@ -206,20 +206,77 @@ fi
> > > >  
> > > >  case "$state" in
> > > >    public-keys)
> > > > -    _wanted public-keys expl 'public key' \
> > > > -	compadd ${${(Mo)$(_call_program public-keys $words[1] $needed --list-public-keys --list-options no-show-photos):%<*>}//(<|>)/} && return
> > > > +    local public_keys=(${(@s.:.)$(_call_program public-keys eval IFS=$'\n' $words[1] $needed --list-public-keys --list-options no-show-photos --with-colons)})
> > > 
> > > This isn't quite right.
> > > 
> > > The first argument to «eval» here is the five bytes «IFS=\n» (where \n stands
> > > for an 0x0A byte), so the eval'd code sets IFS to the empty string, not to the
> > > one-byte string $'\n', and the assignment isn't specific to the command either.
> > > 
> > > I'm not sure what value you _meant_ to set IFS to.  If you meant to set it to a
> > > newline, you could do something like this:
> > > 
> > >     …$(_call_program public-keys eval IFS=${(q):-$'\n'} ${(q)words[1]} ${(q)needed} --list-public-keys --list-options no-show-photos --with-colons)…
> > > 
> > > I also added (q) to the other variable expansions.
> > 
> > Actually, I've mistakenly sent the patch with eval, I got confused with
> > `env` which was my original intention. Both using my original version
> > with `env` instead of `eval` and your version with the quoted IFS work,
> > what shall it be?
> 
> IFS should never be exported, so use eval please.
> 
> Please use double escaping, though (that is, ${(q)${(q)foo}}: that's needed since
> _call_program does an eval anyway, in addition to the one you spell out
> in _call_program's arguments.

Re: [PATCH] _gpg: Use explicit UIDs for public / secret keys.

[Daniel Shahaf]

Doron Behar wrote on Wed, Jun 13, 2018 at 18:17:48 +0300:
> On Tue, Jun 12, 2018 at 06:14:11PM -0400, Phil Pennock wrote:
> > Unless you need trust information or some of the specific parts of the
> > userid, using `--fast-list-mode` can have significant wins too.
> 
> I have ran `diff` on the output of `gpg --list-public-keys
> --with-colons` and `gpg --list-public-keys --fast-list-mode
> --with-colons` and there was no significant reduce in the amount of
> output with my version of gpg, as noted in the commit message you
> quoted.
> 
> > 
> > Matthew's link to
> > <https://git.gnupg.org/cgi-bin/gitweb.cgi?p=gnupg.git;a=blob_plain;f=doc/DETAILS>
> > is accurate and good guidance.  As is his pointer to check the correct
> > column numbers.
> > 
> 
> Since columns may be added or removed in the format, as explained in
> this documentation, I think it'll be better not to hard-code the
> columns' numbers while parsing.

Where exactly does the gpg documentation say that columns may be *removed* in
future gpg 2.x releases?  I don't see any such statement in doc/DETAILS.

My understanding is that columns may be added *at the end* only, and never
removed until 3.x; that way, forward-compatible output parsing is possible.

That implies that our parsing should (a) hardcode column numbers per column
type, e.g., if the value of the first column is "fpr" then only check the Nth
and Mths columns, etc; and that any fields after the last *must not* be parsed
in any way.

> Therefor, I'm inclined to go through all
> of the fields after `fpr` and after `uid` in every line. With the field
> in the line with `fpr`, it's easy to be sure of the `uid` we get from
> this line since we can check if it's matched against `[A-F0-9]{40}`.
> 
> As for the line with description (or usually an email address), I'm not
> sure what regex match to test on it before putting it in the right
> array.. As already pointed out by Daniel, testing it with `=~ "@"` is
> not good enough. Perhaps here we'll use a hard-coded number as
> documented in gpg's repository? I need some opinions here..
> 

The value to match against should be obtained from a hard-coded field number
for a given value of the first line, yes.  As to matching against it by regex,
ideally we'd leave matching to compsys so matchers like _approximate could kick
in.  

> > Beware that recent versions of GnuPG always show fingerprints, for keys
> > and subkeys, because (per commit message) "The fingerprint should always
> > be used thus we should always print it."; so you'll get multiple `fpr:`
> > records per top-level key, although between the `sec` or `pub` top-level
> > introducer and the `uid:` lines for _that_ key there should just be the
> > top-level fingerprint.
> > 
> 
> 
> > 
> > Welcome to the world of GnuPG integration.  You have my sympathy.  But
> > also my encouragement.  :)
> > 
> > -Phil
> 
> Anyway, I've created a draft that should be a much better and much more
> understood but it's not ready yet as I'm not sure about what I explained
> above, this is it:
> 
> 
>     local public_keys_lines=(${(f)"$(_call_program public-keys ${(q)words[1]} ${(q)needed} --list-public-keys --list-options no-show-photos --with-colons)"})
>     local -a uids emails
>     local i j parts
>     for (( i = 1; i < ${#public_keys_lines[@]}; ++i )); do
>       parts=(${(@s.:.)public_keys_lines[$i]})
>       if [[ ${parts[1]} == "fpr" ]]; then
>         # This loop ensures that no matter if fields are added, the last field
>         # that is built from 40 upper case A-Z letters is used as the uid.

As explained above: the parsing should completely ignore any fields in an "fpr"
line beyond the 11th (since it so happens that currently 'fpr' lines have 11
fields in them).

>         # We named the variable current_uid becuase it may have many email
>         # addresses.
>         for (( j = 2; j <${#parts[@]}; ++j )); do
>           if [[ "${parts[$j]}" =~ "[A-F0-9]{40}" ]]; then
>             current_uid="${parts[$j]}"
>           fi
>         done
>         i=$((i + 1))
>         parts=(${(@s.:.)public_keys_lines[$i]})
>         while [[ ${parts[1]} == "uid" ]]; do
>           for (( j = 2; j <${#parts[@]}; ++j )); do
>             # FIXME?
>             if [[ "${parts[$j]}" =~ "@" ]]; then

Can't you just change this condition to «if true; then», so compsys would do
the matching itself>

>               uids+=("${current_uid}")
>               emails+=("${parts[$j]}")
>             fi
>           done
>           i=$((i + 1))
>           parts=(${(@s.:.)public_keys_lines[$i]})
>         done
>       fi
>     done
>     _describe -t public-keys 'public key' emails uids
> 
> For me it works great and it is much faster then before, yet I'm not
> sure about the if statement right below the `FIXME` comment.

Cheers,

Daniel

Re: [PATCH] _gpg: Use explicit UIDs for public / secret keys.

[Doron Behar]

On Tue, Jun 12, 2018 at 06:14:11PM -0400, Phil Pennock wrote:
> Unless you need trust information or some of the specific parts of the
> userid, using `--fast-list-mode` can have significant wins too.

I have ran `diff` on the output of `gpg --list-public-keys
--with-colons` and `gpg --list-public-keys --fast-list-mode
--with-colons` and there was no significant reduce in the amount of
output with my version of gpg, as noted in the commit message you
quoted.

> 
> Matthew's link to
> <https://git.gnupg.org/cgi-bin/gitweb.cgi?p=gnupg.git;a=blob_plain;f=doc/DETAILS>
> is accurate and good guidance.  As is his pointer to check the correct
> column numbers.
> 

Since columns may be added or removed in the format, as explained in
this documentation, I think it'll be better not to hard-code the
columns' numbers while parsing. Therefor, I'm inclined to go through all
of the fields after `fpr` and after `uid` in every line. With the field
in the line with `fpr`, it's easy to be sure of the `uid` we get from
this line since we can check if it's matched against `[A-F0-9]{40}`.

As for the line with description (or usually an email address), I'm not
sure what regex match to test on it before putting it in the right
array.. As already pointed out by Daniel, testing it with `=~ "@"` is
not good enough. Perhaps here we'll use a hard-coded number as
documented in gpg's repository? I need some opinions here..

> Beware that recent versions of GnuPG always show fingerprints, for keys
> and subkeys, because (per commit message) "The fingerprint should always
> be used thus we should always print it."; so you'll get multiple `fpr:`
> records per top-level key, although between the `sec` or `pub` top-level
> introducer and the `uid:` lines for _that_ key there should just be the
> top-level fingerprint.
> 


> 
> Welcome to the world of GnuPG integration.  You have my sympathy.  But
> also my encouragement.  :)
> 
> -Phil

Anyway, I've created a draft that should be a much better and much more
understood but it's not ready yet as I'm not sure about what I explained
above, this is it:


    local public_keys_lines=(${(f)"$(_call_program public-keys ${(q)words[1]} ${(q)needed} --list-public-keys --list-options no-show-photos --with-colons)"})
    local -a uids emails
    local i j parts
    for (( i = 1; i < ${#public_keys_lines[@]}; ++i )); do
      parts=(${(@s.:.)public_keys_lines[$i]})
      if [[ ${parts[1]} == "fpr" ]]; then
        # This loop ensures that no matter if fields are added, the last field
        # that is built from 40 upper case A-Z letters is used as the uid.
        # We named the variable current_uid becuase it may have many email
        # addresses.
        for (( j = 2; j <${#parts[@]}; ++j )); do
          if [[ "${parts[$j]}" =~ "[A-F0-9]{40}" ]]; then
            current_uid="${parts[$j]}"
          fi
        done
        i=$((i + 1))
        parts=(${(@s.:.)public_keys_lines[$i]})
        while [[ ${parts[1]} == "uid" ]]; do
          for (( j = 2; j <${#parts[@]}; ++j )); do
            # FIXME?
            if [[ "${parts[$j]}" =~ "@" ]]; then
              uids+=("${current_uid}")
              emails+=("${parts[$j]}")
            fi
          done
          i=$((i + 1))
          parts=(${(@s.:.)public_keys_lines[$i]})
        done
      fi
    done
    _describe -t public-keys 'public key' emails uids

For me it works great and it is much faster then before, yet I'm not
sure about the if statement right below the `FIXME` comment.

Re: [PATCH] _gpg: Use explicit UIDs for public / secret keys.

[Phil Pennock]

On 2018-06-12 at 13:54 +0300, Doron Behar wrote:
> To tell you the truth, I have no idea what `fpr` means. I just know, by

Fingerprint.  It's the fullest form of the keyid and probably the best
choice for identifying keys today; within the GnuPG tooling community,
using any of the shorter keyid formats is moving into "frowned upon"
territory.

Unless you need trust information or some of the specific parts of the
userid, using `--fast-list-mode` can have significant wins too.

Doing any form of parsing without `--with-colons` is prone to breaking
depending upon tuning options in the gpg.conf file, so switching is a
good thing.

Matthew's link to
<https://git.gnupg.org/cgi-bin/gitweb.cgi?p=gnupg.git;a=blob_plain;f=doc/DETAILS>
is accurate and good guidance.  As is his pointer to check the correct
column numbers.

Beware that recent versions of GnuPG always show fingerprints, for keys
and subkeys, because (per commit message) "The fingerprint should always
be used thus we should always print it."; so you'll get multiple `fpr:`
records per top-level key, although between the `sec` or `pub` top-level
introducer and the `uid:` lines for _that_ key there should just be the
top-level fingerprint.

Note that people can want to explicitly specify a subkey fingerprint,
although if they do, they'll want to follow it with an exclamation mark
to indicate "no really, use this subkey, I'm not just giving you a
pointer to find the top key".

Welcome to the world of GnuPG integration.  You have my sympathy.  But
also my encouragement.  :)

-Phil

Re: [PATCH] _gpg: Use explicit UIDs for public / secret keys.

[Matthew Martin]

On Tue, Jun 12, 2018 at 01:54:57PM +0300, Doron Behar wrote:
> Daniel,
> 
> I would like you to tell me how can I solve better the `fpr` thing.
> 
> To tell you the truth, I have no idea what `fpr` means. I just know, by
> comparing the output of `gpg --list-public-keys` with and without
> `--with-colons`, that the hexadecimal number afterwards is the actual
> key that needs to be used as an argument for these options. The way I
> understand it, only if there is an email address / description coming
> somewhere after the array element `fpr` (without another `fpr` in
> between), then this field is the description.
> 
> Please tell me, how can this algorithm get better? I've had one idea in
> mind: Perhaps we can perform a more strict pattern matchings test before
> adding a uid for example?

Not a gpg user, but it seems like the documentation for the format is at
https://git.gnupg.org/cgi-bin/gitweb.cgi?p=gnupg.git;a=blob_plain;f=doc/DETAILS

With regards to better parsing, you might consider something like

local line
for line in "${(@f)secret_keys}"; do
  local parts=("${(@s,:,)line}")
  if [[ $parts[1] == fpr ]]; then
    ...

This way only the first column in a line is checked for fpr and the
correct column can be used for the other values.

- Matthew Martin

Re: [PATCH] _gpg: Use explicit UIDs for public / secret keys.

[Doron Behar]

Daniel,

I would like you to tell me how can I solve better the `fpr` thing.

To tell you the truth, I have no idea what `fpr` means. I just know, by
comparing the output of `gpg --list-public-keys` with and without
`--with-colons`, that the hexadecimal number afterwards is the actual
key that needs to be used as an argument for these options. The way I
understand it, only if there is an email address / description coming
somewhere after the array element `fpr` (without another `fpr` in
between), then this field is the description.

Please tell me, how can this algorithm get better? I've had one idea in
mind: Perhaps we can perform a more strict pattern matchings test before
adding a uid for example?

On Sat, Jun 09, 2018 at 08:39:32PM +0000, Daniel Shahaf wrote:
> Good morning Doron,
> 
> Thanks for revising the patch, however, I'm afraid I do have a few more
> comments:
> 
> doron.behar@gmail.com wrote on Sat, Jun 09, 2018 at 23:09:40 +0300:
> >    secret-keys)
> > -    _wanted secret-keys expl 'secret key' compadd \
> > -	${${(Mo)$(_call_program secret-keys $words[1] $needed --list-secret-keys --list-options no-show-photos):%<*>}//(<|>)/} && return
> > +    local secret_keys=(${(@s.:.)${(f)"$(_call_program secret-keys ${(q)words[1]} ${(q)needed} --list-secret-keys --list-options no-show-photos --with-colons)"}})
> > +    local -a uids emails
> > +    local i
> > +    for i in {1..${#secret_keys[@]}}; do
> > +      if [[ ${secret_keys[$i]} == "fpr" ]]; then
> 
> I'd like to see the 'fpr' thing (which I have pointed out twice by now) fixed
> before merging.  _gpg isn't a function I'm comfortable adding
> relaxed/inaccurate parsing to, and doing the parsing correctly isn't onerous.
> 
> > +        i=$((i + 1))
> > +        local j=$i
> > +        while [[ ${secret_keys[$j]} != "fpr" ]] && [ $j -lt ${#secret_keys[@]} ]; do
> > +          if [[ ${secret_keys[$j]} =~ "@" ]]; then
> 
> This condition false negatives for me, probably because I used a test secret
> key that didn't have an email address attached.
> 
> > +            emails+="${secret_keys[$j]}"
> > +            uids+="${secret_keys[$i]}"
> > +            break
> > +          fi
> > +          j=$((j + 1))
> > +        done
> > +        i=$j
> > +      fi
> > +    done
> > +    _describe -t secret-keys 'secret key' uids_and_emails
> 
> s/uids_and_emails/emails uids/
> 
> >    ;;
> >    ciphers)
> >      _wanted ciphers expl cipher compadd \
> > -        ${${(s.,.)${(M)${(f)${"$(_call_program ciphers $words[1] $needed --version)"}//,$'\n' #/, }:#Cipher*}#*:}# } && return
> > +        ${${(s.,.)${(M)${(f)${"$(_call_program ciphers ${(q)words[1]} ${(q)needed} --version)"}//,$'\n' #/, }:#Cipher*}#*:}# } && return
> >    ;;
> >    (public-keyids)
> > +    _describe -t public-keyids 'public keyids' uids_and_emails
> 
> Ditto.
> 
> Thanks for hanging in there.
> 
> Daniel

Re: [PATCH] _gpg: Use explicit UIDs for public / secret keys.

[Daniel Shahaf]

Good morning Doron,

Thanks for revising the patch, however, I'm afraid I do have a few more
comments:

doron.behar@gmail.com wrote on Sat, Jun 09, 2018 at 23:09:40 +0300:
>    secret-keys)
> -    _wanted secret-keys expl 'secret key' compadd \
> -	${${(Mo)$(_call_program secret-keys $words[1] $needed --list-secret-keys --list-options no-show-photos):%<*>}//(<|>)/} && return
> +    local secret_keys=(${(@s.:.)${(f)"$(_call_program secret-keys ${(q)words[1]} ${(q)needed} --list-secret-keys --list-options no-show-photos --with-colons)"}})
> +    local -a uids emails
> +    local i
> +    for i in {1..${#secret_keys[@]}}; do
> +      if [[ ${secret_keys[$i]} == "fpr" ]]; then

I'd like to see the 'fpr' thing (which I have pointed out twice by now) fixed
before merging.  _gpg isn't a function I'm comfortable adding
relaxed/inaccurate parsing to, and doing the parsing correctly isn't onerous.

> +        i=$((i + 1))
> +        local j=$i
> +        while [[ ${secret_keys[$j]} != "fpr" ]] && [ $j -lt ${#secret_keys[@]} ]; do
> +          if [[ ${secret_keys[$j]} =~ "@" ]]; then

This condition false negatives for me, probably because I used a test secret
key that didn't have an email address attached.

> +            emails+="${secret_keys[$j]}"
> +            uids+="${secret_keys[$i]}"
> +            break
> +          fi
> +          j=$((j + 1))
> +        done
> +        i=$j
> +      fi
> +    done
> +    _describe -t secret-keys 'secret key' uids_and_emails

s/uids_and_emails/emails uids/

>    ;;
>    ciphers)
>      _wanted ciphers expl cipher compadd \
> -        ${${(s.,.)${(M)${(f)${"$(_call_program ciphers $words[1] $needed --version)"}//,$'\n' #/, }:#Cipher*}#*:}# } && return
> +        ${${(s.,.)${(M)${(f)${"$(_call_program ciphers ${(q)words[1]} ${(q)needed} --version)"}//,$'\n' #/, }:#Cipher*}#*:}# } && return
>    ;;
>    (public-keyids)
> +    _describe -t public-keyids 'public keyids' uids_and_emails

Ditto.

Thanks for hanging in there.

Daniel

[PATCH] _gpg: Use explicit UIDs for public / secret keys.

[doron.behar]

From: Doron Behar <doron.behar@gmail.com>

Use the `--with-colons` option and parse the output according to the
format.
---
 Completion/Unix/Command/_gpg | 65 ++++++++++++++++++++++++++++++++----
 1 file changed, 58 insertions(+), 7 deletions(-)

diff --git a/Completion/Unix/Command/_gpg b/Completion/Unix/Command/_gpg
index 48a36eff2..c7830bd34 100644
--- a/Completion/Unix/Command/_gpg
+++ b/Completion/Unix/Command/_gpg
@@ -206,20 +206,71 @@ fi
 
 case "$state" in
   public-keys)
-    _wanted public-keys expl 'public key' \
-	compadd ${${(Mo)$(_call_program public-keys $words[1] $needed --list-public-keys --list-options no-show-photos):%<*>}//(<|>)/} && return
+    local public_keys=(${(@s.:.)${(f)"$(_call_program public-keys ${(q)words[1]} ${(q)needed} --list-public-keys --list-options no-show-photos --with-colons)"}})
+    local -a uids emails
+    local i
+    for i in {1..${#public_keys[@]}}; do
+      if [[ ${public_keys[$i]} == "fpr" ]]; then
+        i=$((i + 1))
+        local j=$i
+        while [[ ${public_keys[$j]} != "fpr" ]] && [ $j -lt ${#public_keys[@]} ]; do
+          if [[ ${public_keys[$j]} =~ "@" ]]; then
+            emails+="${public_keys[$j]}"
+            uids+="${public_keys[$i]}"
+            break
+          fi
+          j=$((j + 1))
+        done
+        i=$j
+      fi
+    done
+    _describe -t public-keys 'public key' emails uids
   ;;
   secret-keys)
-    _wanted secret-keys expl 'secret key' compadd \
-	${${(Mo)$(_call_program secret-keys $words[1] $needed --list-secret-keys --list-options no-show-photos):%<*>}//(<|>)/} && return
+    local secret_keys=(${(@s.:.)${(f)"$(_call_program secret-keys ${(q)words[1]} ${(q)needed} --list-secret-keys --list-options no-show-photos --with-colons)"}})
+    local -a uids emails
+    local i
+    for i in {1..${#secret_keys[@]}}; do
+      if [[ ${secret_keys[$i]} == "fpr" ]]; then
+        i=$((i + 1))
+        local j=$i
+        while [[ ${secret_keys[$j]} != "fpr" ]] && [ $j -lt ${#secret_keys[@]} ]; do
+          if [[ ${secret_keys[$j]} =~ "@" ]]; then
+            emails+="${secret_keys[$j]}"
+            uids+="${secret_keys[$i]}"
+            break
+          fi
+          j=$((j + 1))
+        done
+        i=$j
+      fi
+    done
+    _describe -t secret-keys 'secret key' uids_and_emails
   ;;
   ciphers)
     _wanted ciphers expl cipher compadd \
-        ${${(s.,.)${(M)${(f)${"$(_call_program ciphers $words[1] $needed --version)"}//,$'\n' #/, }:#Cipher*}#*:}# } && return
+        ${${(s.,.)${(M)${(f)${"$(_call_program ciphers ${(q)words[1]} ${(q)needed} --version)"}//,$'\n' #/, }:#Cipher*}#*:}# } && return
   ;;
   (public-keyids)
-    _wanted public-keys expl 'public keyid' \
-      compadd ${(M)${${(f)"$(_call_program public-keyids $words[1] $needed --list-public-keys --list-options no-show-photos)"}## #}:#[0-9A-F](#c40)} && return
+    local public_keys=(${(@s.:.)${(f)"$(_call_program public-keys ${(q)words[1]} ${(q)needed} --list-public-keys --list-options no-show-photos --with-colons)"}})
+    local -a uids emails
+    local i
+    for i in {1..${#public_keys[@]}}; do
+      if [[ ${public_keys[$i]} == "fpr" ]]; then
+        i=$((i + 1))
+        local j=$i
+        while [[ ${public_keys[$j]} != "fpr" ]] && [ $j -lt ${#public_keys[@]} ]; do
+          if [[ ${public_keys[$j]} =~ "@" ]]; then
+            emails+="${public_keys[$j]}"
+            uids+="${public_keys[$i]}"
+            break
+          fi
+          j=$((j + 1))
+        done
+        i=$j
+      fi
+    done
+    _describe -t public-keyids 'public keyids' uids_and_emails
   ;;
   (option-list)
     _sequence _wanted options expl option \
-- 
2.17.1