Re: [PATCH] _pick_variant: Update builtin check

[Matthew Martin <phy1729@xxxxxxxxx>]

On Wed, Mar 20, 2019 at 01:05:23PM +0000, Daniel Shahaf wrote:
> Matthew Martin wrote on Wed, Mar 20, 2019 at 07:52:39 -0500:
> > There are four booleans in play:
> > - If command (or a non-builtin-preserving precommand) is specified
> >   (${#precommands:|builtin_precommands})
> > - If builtin is specified ($+precommands[(r)builtin])
> > - If -b is passed to _pick_variant ($+opts[-b])
> > - If the command is a builtin ($+builtins[$opts[-c]])
> ⋮
> > +++ b/Completion/Base/Utility/_pick_variant
> > @@ -1,9 +1,11 @@
> >  #autoload
> >  
> > -local output cmd pat
> > -local -a var
> > +local output cmd pat pre
> > +local -a builtin_precommands var
> >  local -A opts
> >  
> > +builtin_precommands=(- builtin eval exec nocorrect noglob time)
> 
> May I suggest a comment here documenting the semantics of this variable?
> For example, why doesn't it list the 'command' precommand (presumably
> becaus that one doesn't preserve builtins, but this info should be in
> the comment, not in the list archives)?

How's
+# Precommands which allow the command to be builtin (unlike command and sudo).
+local -ar builtin_precommands=(- builtin eval exec nocorrect noglob time)

Which of course raises the point that I need to add precommands+=(sudo)
to _sudo (and most users of _normal).

> (And since I'm replying already, style nit: the array could be declared
> readonly.)

Indeed; fixed.

> >  (( $+_cmd_variant )) || typeset -gA _cmd_variant
> >  
> >  zparseopts -D -A opts b: c: r:
> > @@ -13,19 +15,27 @@ while [[ $1 = *=* ]]; do
> >    var+=( "${1%%\=*}" "${1#*=}" )
> >    shift
> >  done
> > -if (( $+_cmd_variant[$opts[-c]] )); then
> > -  (( $+opts[-r] )) && eval "${opts[-r]}=${_cmd_variant[$opts[-c]]}"
> > -  [[ $_cmd_variant[$opts[-c]] = "$1" ]] && return 1
> > +
> > +if (( ${#precommands:|builtin_precommands} )); then
> > +  pre=command
> > +elif (( $+opts[-b] && ( $precommands[(r)builtin] || $+builtins[$opts[-c]] ) )); then
> > +  (( $+opts[-r] )) && eval "${opts[-r]}=$opts[-b]"
> 
> Should that be «"${opts[-r]}=${(q)opts[-b]}"» with quoting to counter
> the eval?  (Also with the preëxisting assignments-in-eval in other lines)

I was planning on switching them all to ${(P)opts[-r]::=...} next.

Thanks for the review!