Re: [PATCH 1/1] _gpg: Use explicit UIDs for state = public keys.

[Doron Behar <doron.behar@xxxxxxxxx>]

I've successfully managed to solve address all the comments you've had
for my patch. Yet, I'm having trouble with `_call_program`. The line I'm
trying to put is this:

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

I debugged this a little bit and the variable `$public_keys` is empty
but if I remove the words: `_call_program public-keys` it works good.

I've had a glimpse at `htop` when this function was called when I tested
the completion function and I saw these commands running there:

	pkgfile -b -v -- --list-public-keys
	pkgfile -b -v -- --list-options
	pkgfile -b -v -- no-show-photos
	pkgfile -b -v -- --with-colons

What is _call_program supposed to be doing?

Besides that, the revised patch is ready. I've also included similar
improvements for the other states - `secret-keys` and `public-keyids`.

On Sat, May 26, 2018 at 04:25:41PM +0000, Daniel Shahaf wrote:
> On Sat, May 26, 2018 at 06:16:28PM +0300, doron.behar@xxxxxxxxx wrote:
> > From: Doron Behar <doron.behar@xxxxxxxxx>
> > 
> > Use the `--with-colons` option and parse the output while IFS=":"
> > according to the output format.
> > ---
> >  Completion/Unix/Command/_gpg | 15 +++++++++++++--
> >  1 file changed, 13 insertions(+), 2 deletions(-)
> > 
> > diff --git a/Completion/Unix/Command/_gpg b/Completion/Unix/Command/_gpg
> > index 48a36eff2..71fa7667d 100644
> > --- a/Completion/Unix/Command/_gpg
> > +++ b/Completion/Unix/Command/_gpg
> > @@ -206,8 +206,19 @@ 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
> > +    OLDIFS="${IFS}"
> 
> OLDIFS isn't made local so it leaks.
> 
> You could alternatively have set IFS in a precommand assignment (e.g., «IFS=:
> /bin/echo foo:bar»), or made it local to a function.  Another approach is use
> parameter expansion flags such as «${(s.:.)foo}».
> 
> > +    IFS=":"
> > +    public_keys=($($words[1] $needed --list-public-keys --list-options no-show-photos --with-colons))
> 
> Another parameter leak.  (See WARN_CREATE_GLOBAL.)
> 
> Why did you remove the use of _call_program?
> 
> > +    for i in {1..${#public_keys[@]}}; do
> > +      if [[ ${public_keys[$i]} =~ "fpr" ]] && [[ ${public_keys[$((i + 19))]} =~ "@" ]] ; then
> 
> The parameter 'i' leaks.
> 
> "fpr" should be looked for as a complete string, not as a substring.  Also, it
> should be looked for only in the first column, not in every single output
> field.
> 
> A subscript is always parsed as a math context so you can just do «$foo[i+9]»
> without an additional $((…)) inside.
> 
> > +        # +9 is the uid
> > +        # +19 is the description
> > +        uids_and_emails+=(${public_keys[$((i + 9))]}":"${public_keys[$((i + 19))]})
> 
> I'm sorry, but that's not forward compatible.  The output format spec
> (doc/DETAILS, which is referred to from the manpage) states that fields may be
> added in the future, so that "19" may not be hardcoded here.
> 
> > +        i=$((i+20))
> 
> This line doesn't have any effect, does it?  This isn't an arithmetic for, it's
> a list-of-words for, and the next word is equal to $(( pre_assignment_value_of_i + 1 )).
> 
> > +      fi
> > +    done
> > +    _describe uids uids_and_emails
> 
> The use of describe doesn't set the 'public-keys' tag that _wanted set.  (You
> need to pass -t.)
> 
> > +    IFS="${OLDIFS}"
> 
> Thanks for the patch.  I agree that it would be better to use --with-colons.
> We look forward to a revised patch. :-)
> 
> Cheers,
> 
> Daniel