From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 12742 invoked by alias); 31 Aug 2016 17:49:49 -0000 Mailing-List: contact zsh-workers-help@zsh.org; run by ezmlm Precedence: bulk X-No-Archive: yes List-Id: Zsh Workers List List-Post: List-Help: X-Seq: 39143 Received: (qmail 4076 invoked from network); 31 Aug 2016 17:49:48 -0000 X-Qmail-Scanner-Diagnostics: from out4-smtp.messagingengine.com by f.primenet.com.au (envelope-from , uid 7791) with qmail-scanner-2.11 (clamdscan: 0.99.2/21882. spamassassin: 3.4.1. Clear:RC:0(66.111.4.28):SA:0(0.0/5.0):. Processed in 0.748172 secs); 31 Aug 2016 17:49:48 -0000 X-Spam-Checker-Version: SpamAssassin 3.4.1 (2015-04-28) on f.primenet.com.au X-Spam-Level: X-Spam-Status: No, score=0.0 required=5.0 tests=T_DKIM_INVALID autolearn=unavailable autolearn_force=no version=3.4.1 X-Envelope-From: d.s@daniel.shahaf.name X-Qmail-Scanner-Mime-Attachments: | X-Qmail-Scanner-Zip-Files: | Received-SPF: none (ns1.primenet.com.au: domain at daniel.shahaf.name does not designate permitted sender hosts) DKIM-Signature: v=1; a=rsa-sha1; c=relaxed/relaxed; d= daniel.shahaf.name; h=cc:content-transfer-encoding:content-type :date:from:in-reply-to:message-id:mime-version:references :subject:to:x-sasl-enc:x-sasl-enc; s=mesmtp; bh=pPKcf4aPoO/ghMu/ XzRlMrW1l/Q=; b=xSsYulmICPSzBg1P9yJJGG5upU5OR+WAMFTe4iaPNd3Bd8Jw f4OVrHS4kfO2d2mxRlB7ncud8kC/iGbRVOo/WK/KdapPxHCOuW3IAYp0hfF/hq7G ++QMskIqGyDkRMrLVZ01Adk5ZfLaQ0dkLwrQnbC8Yk5vJyErEJdeX3v8dRA= DKIM-Signature: v=1; a=rsa-sha1; c=relaxed/relaxed; d= messagingengine.com; h=cc:content-transfer-encoding:content-type :date:from:in-reply-to:message-id:mime-version:references :subject:to:x-sasl-enc:x-sasl-enc; s=smtpout; bh=pPKcf4aPoO/ghMu /XzRlMrW1l/Q=; b=U27Wunj4JEM1h+iQioGaj8AU5sKAIlqk/xWzlA9AFdS9DoU pia2zO9gFgC6WGzD9RmCM25XpYItOf2WeL9gP1JJRK0JNxRgOVZC8HB8BXH9+Sss SS15vBMxWfifyHeaIcWUtxLTwuT9OXc1JkOj0WA5Cz7DqrbY17fOq82MJlQE= X-Sasl-enc: dLOSsgcJYEq4obY9+i7fQJzQ48GVee2AMTteABzx7Dh5 1472665780 Date: Wed, 31 Aug 2016 17:49:15 +0000 From: Daniel Shahaf To: Marko Myllynen Cc: zsh workers Subject: Re: Update _virsh completion Message-ID: <20160831174915.GA11043@fujitsu.shahaf.local2> References: MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: User-Agent: Mutt/1.5.23 (2014-03-12) 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 --' 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 > --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 -- --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