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

[Daniel Shahaf <d.s@xxxxxxxxxxxxxxxxxx>]

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