Re: [PATCH] _pick_variant: Update builtin check

[Daniel Shahaf <d.s@xxxxxxxxxxxxxxxxxx>]

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)?

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

>  (( $+_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)

>    return 0
> +elif (( $precommands[(r)builtin] )); then
> +  pre=builtin
> +else
> +  # Neither builtin nor command-forcing precommand specified,
> +  # so no prefix is needed
> +  pre=
>  fi
>  
> -if [[ $+opts[-b] -eq 1 && -n $builtins[$opts[-c]] ]]; then
> -  _cmd_variant[$opts[-c]]=$opts[-b]
> +if [[ $pre != builtin ]] && (( $+_cmd_variant[$opts[-c]] )); then
>    (( $+opts[-r] )) && eval "${opts[-r]}=${_cmd_variant[$opts[-c]]}"
> +  [[ $_cmd_variant[$opts[-c]] = "$1" ]] && return 1
>    return 0
>  fi
>  
> -output="$(_call_program variant $opts[-c] "${@[2,-1]}" </dev/null 2>&1)"
> +output="$(_call_program variant $pre $opts[-c] "${@[2,-1]}" </dev/null 2>&1)"
>  
>  for cmd pat in "$var[@]"; do
>    if [[ $output = *$~pat* ]]; then
> @@ -36,6 +46,6 @@ for cmd pat in "$var[@]"; do
>  done
>  
>  (( $+opts[-r] )) && eval "${opts[-r]}=$1"
> -_cmd_variant[$opts[-c]]="$1"
> +[[ $pre != builtin ]] && _cmd_variant[$opts[-c]]="$1"
>  
>  return 1

Cheers,

Daniel