Re: Update _virsh completion

[Daniel Shahaf <d.s@xxxxxxxxxxxxxxxxxx>]

Marko Myllynen wrote on Mon, Aug 29, 2016 at 11:03:13 +0300:
> Hi,
> 
> The patch below (almost) completes _virsh completions:
> 
> * fix/improve help command completion
> * fix/improve enabling file completion
> * selective completion support for
>   + domains
>   + interfaces
>   + networks
>   + devices
>   + nwfilters
>   + pools
>   + secrets
>   + snapshots
>   + volumes
> * pass -c/--connect parameter as needed
>   + thanks to Daniel for the suggestion
> * sanitize parameters passed to commands run with sudo(8)
> * cosmetics
> 

Nice!

> The list of commands for which selective completion support has been
> defined may not be complete (I didn't go through all the ~220 virsh
> commands) but if there is no definition for a command then, e.g., all
> domains instead of inactive domains are being offered.
> 

Makes sense.

> Few remarks:
> 
> 1) I wonder is there more elegant way to do this:
> 
> [[ ${(k)words[(I)--pool]} -gt 0 ]] && pool=${words[1+${(k)words[(I)--pool]}]}
> 

Normally this is handled by $opt_args which is populated by _arguments.
Have you considered calling _arguments from the 'virsh subcommand
--<TAB>' completion?

This would also handle the --pool=foo (as a single argument word) syntax.

Secondarily, this code isn't idiomatic: the 'k' flag applies to
associative arrays, and (( )) is more idiomatic than [[ ]] for dealing
with integer arithmetics (compare users/21820).

> 2) For some reason virsh command options are offered more than once
> even with this (-w):
> 
> +    [[ -n ${=_cache_virsh_cmd_opts[$cmd]} ]] && \
> +      _values -w options ${=_cache_virsh_cmd_opts[$cmd]} && ret=0
> 
> $ virsh start --domain foo --autodestroy <TAB>
> --autodestroy   --console       --pass-fds                                  

That's because the expansion contains "--autodestroy" twice.  You can
easily uniquify the expansion with ${(u)foo} (or 'typeset -aU').

Maybe _values should do that automatically?

> 3) The proper handling of calling sudo from completion function
> was left open last time, the last email was:
> 
> http://www.zsh.org/mla/workers/2016/msg01406.html
> 
> It's unclear at least to me what would be the preferred
> approach so I think a follow-up patch is needed if a consensus
> emerges.

*nod*

workers@: Could people chime in please about the sudo question?

> -    if (( ! $+_cache_virsh_cmdopts )); then
> -      typeset -gA _cache_virsh_cmdopts
> +    if (( ! $+_cache_virsh_cmd_opts )); then
> +      typeset -gA _cache_virsh_cmd_opts

There's a cache mechanism in compsys, see the 'use-cache' style.
However, I don't know whether it would make sense to use it here.

(Not a blocker; can be done in a followup if needed)

>  esac
>  
> +local -a conn_opt
> +if [[ -n ${(v)opt_args[(I)-c|--connect]} ]]; then
> +  local uri=${(v)opt_args[(I)-c|--connect]}
> +  [[ -z ${(Q)uri//([[:alnum:]]|+|:|\/|@|-|\.|\?|=)} ]] && \
> +    conn_opt=( -c $uri )

A comment explaining the condition, please?

If you're trying to check whether $uri is shell-safe, see below about
using ${(q)conn_opt}.

> +  virsh_cmd_opts)
> +    ⋮
> +    case $words[-2] in

I think you want $words[CURRENT-1]; this matters when $SUFFIX is not
empty, i.e., when you complete at —

     % virsh subcommand --<CURSOR> --foo

> +      --vol)
> +        local pool ; [[ ${(k)words[(I)--pool]} -gt 0 ]] && pool=${words[1+${(k)words[(I)--pool]}]}
> +        [[ -z $pool ]] && return 1
> +        values=( ${${${${(f):-"$(_call_program volumes "virsh ${(Q)conn_opt} vol-list --pool $pool 2>/dev/null")"}/ Name*/}:#---*}/  */} )

That needs to be ${(q)pool}.  (I'm not sure whether pool names can
contain shell-unsafe characters, but I tend to always use ${(q)} since
it's never wrong.)  There are a few other instances of this, notably one
in a _call_program invocation that uses sudo.

Same question about the (Q): is it correct?  It would be correct if the
value of $conn_opt is twice shell-escaped — as though by ${(q)${(q)foo}}) —
which isn't the case here.

> +        [[ -n $values ]] && _values volume ${=values} && return 0
> +        return 1
> +      ;;
> +    esac
> +    if [[ $cmd == help ]]; then
> +      [[ $words[-1] == -* ]] && _values -w -- --command && return 0

«_values -w -- --command» doesn't do anything useful:

$ zsh -f
% autoload compinit && compinit
% _f() _values -w -- --foo
% compdef _f f
% f 
(eval):1: bad substitution
(eval):1: not an identifier: [_-]=* r:|=*[@]

Perhaps you meant «_values -w description -- --foo» or «_values -w
description --foo».  (The former actually adds the «--» as one of the
completions.) 

>

Cheers,

Daniel