* [PATCH] _pick_variant: Update builtin check @ 2019-03-20 2:05 Matthew Martin 2019-03-20 3:38 ` Matthew Martin 0 siblings, 1 reply; 7+ messages in thread From: Matthew Martin @ 2019-03-20 2:05 UTC (permalink / raw) To: zsh-workers Move the builtin check prior to the cache lookup and don't cache builtins if -b is provided. This corrects the result if a user uses both a builtin and external version of a command in one shell session. The command may be builtin only if all precommands are builtin-preserving which are enumerated in builtin_precommands. Since a builtin-preserving precommand must be itself builtin, I figure it's easier to maintain as a white list. This corrects the case Daniel brought up: command chmod <tab>. I'll split out the _builtin change before commit. Wasn't large enough to get its own mail but necessary if anyone is testing. - Matthew Martin --- Completion/Base/Utility/_pick_variant | 15 +++++++++------ Completion/Zsh/Command/_builtin | 2 ++ 2 files changed, 11 insertions(+), 6 deletions(-) diff --git a/Completion/Base/Utility/_pick_variant b/Completion/Base/Utility/_pick_variant index 9099e3599..f4ceb7462 100644 --- a/Completion/Base/Utility/_pick_variant +++ b/Completion/Base/Utility/_pick_variant @@ -1,9 +1,11 @@ #autoload local output cmd pat -local -a var +local -a builtin_precommands var local -A opts +builtin_precommands=(- builtin eval exec nocorrect noglob time) + (( $+_cmd_variant )) || typeset -gA _cmd_variant zparseopts -D -A opts b: c: r: @@ -13,15 +15,16 @@ 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 (( $+opts[-b] && ! ${#precommands:|builtin_precommands} && + ( $+builtins[$opts[-c]] || $precommands[(I)builtin] ) )); then + (( $+opts[-r] )) && eval "${opts[-r]}=$opts[-b]" return 0 fi -if [[ $+opts[-b] -eq 1 && -n $builtins[$opts[-c]] ]]; then - _cmd_variant[$opts[-c]]=$opts[-b] +if (( $+_cmd_variant[$opts[-c]] )); then (( $+opts[-r] )) && eval "${opts[-r]}=${_cmd_variant[$opts[-c]]}" + [[ $_cmd_variant[$opts[-c]] = "$1" ]] && return 1 return 0 fi diff --git a/Completion/Zsh/Command/_builtin b/Completion/Zsh/Command/_builtin index 9fb6acf7b..a77af9879 100644 --- a/Completion/Zsh/Command/_builtin +++ b/Completion/Zsh/Command/_builtin @@ -1,5 +1,7 @@ #compdef builtin +precommands+=(builtin) + if (( $CURRENT > 2 )); then shift words (( CURRENT -- )) -- 2.21.0 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] _pick_variant: Update builtin check 2019-03-20 2:05 [PATCH] _pick_variant: Update builtin check Matthew Martin @ 2019-03-20 3:38 ` Matthew Martin 2019-03-20 12:52 ` Matthew Martin 0 siblings, 1 reply; 7+ messages in thread From: Matthew Martin @ 2019-03-20 3:38 UTC (permalink / raw) To: zsh-workers On Tue, Mar 19, 2019 at 09:05:13PM -0500, Matthew Martin wrote: > Move the builtin check prior to the cache lookup and don't cache > builtins if -b is provided. This corrects the result if a user uses both > a builtin and external version of a command in one shell session. > > The command may be builtin only if all precommands are > builtin-preserving which are enumerated in builtin_precommands. > Since a builtin-preserving precommand must be itself builtin, I figure > it's easier to maintain as a white list. This corrects the case Daniel > brought up: command chmod <tab>. I've realized that I was only testing on a system that uses the fallback variant and the patch will give incorrect results on GNU systems since _call_program may call a builtin. I'll rework the patch (although I'm open to suggestions for fixing it). Sorry for the noise. ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] _pick_variant: Update builtin check 2019-03-20 3:38 ` Matthew Martin @ 2019-03-20 12:52 ` Matthew Martin 2019-03-20 13:05 ` Daniel Shahaf 0 siblings, 1 reply; 7+ messages in thread From: Matthew Martin @ 2019-03-20 12:52 UTC (permalink / raw) To: zsh-workers On Tue, Mar 19, 2019 at 10:38:15PM -0500, Matthew Martin wrote: > On Tue, Mar 19, 2019 at 09:05:13PM -0500, Matthew Martin wrote: > > Move the builtin check prior to the cache lookup and don't cache > > builtins if -b is provided. This corrects the result if a user uses both > > a builtin and external version of a command in one shell session. > > > > The command may be builtin only if all precommands are > > builtin-preserving which are enumerated in builtin_precommands. > > Since a builtin-preserving precommand must be itself builtin, I figure > > it's easier to maintain as a white list. This corrects the case Daniel > > brought up: command chmod <tab>. > > I've realized that I was only testing on a system that uses the fallback > variant and the patch will give incorrect results on GNU systems since > _call_program may call a builtin. I'll rework the patch (although I'm > open to suggestions for fixing it). 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]]) If command is specified, set pre=command to force _call_program to run the command. Eligible for caching. If -b is passed and (builtin is specified or the command is a builtin), return the -b variant. Ineligible for caching. If builtin is specified, set pre=builtin to force _call_program to run the builtin. Ineligible for caching. This leaves 3 cases: - command not specified, builtin not specified, -b passed, is not builtin - command not specified, builtin not specified, -b not passed, is not builtin - command not specified, builtin not specified, -b not passed, is builtin In all cases, fall through to _call_program with no prefix set. In the first two cases, the command is not a builtin, so they are clearly caching eligible. In the last case, I'm unsure if caching should be allowed, but one would hope there are few builtins that have a completer that uses _pick_variant and does not pass -b. Since this case should be rare, I've omitted it from the cache eligibility check. - Matthew Martin --- Completion/Base/Utility/_pick_variant | 28 ++++++++++++++++++--------- Completion/Zsh/Command/_builtin | 2 ++ 2 files changed, 21 insertions(+), 9 deletions(-) diff --git a/Completion/Base/Utility/_pick_variant b/Completion/Base/Utility/_pick_variant index 9099e3599..22faa8381 100644 --- a/Completion/Base/Utility/_pick_variant +++ 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) + (( $+_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]" 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 diff --git a/Completion/Zsh/Command/_builtin b/Completion/Zsh/Command/_builtin index 9fb6acf7b..a77af9879 100644 --- a/Completion/Zsh/Command/_builtin +++ b/Completion/Zsh/Command/_builtin @@ -1,5 +1,7 @@ #compdef builtin +precommands+=(builtin) + if (( $CURRENT > 2 )); then shift words (( CURRENT -- )) -- 2.21.0 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] _pick_variant: Update builtin check 2019-03-20 12:52 ` Matthew Martin @ 2019-03-20 13:05 ` Daniel Shahaf 2019-03-21 0:16 ` Matthew Martin 0 siblings, 1 reply; 7+ messages in thread From: Daniel Shahaf @ 2019-03-20 13:05 UTC (permalink / raw) To: zsh-workers 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] _pick_variant: Update builtin check 2019-03-20 13:05 ` Daniel Shahaf @ 2019-03-21 0:16 ` Matthew Martin 2019-03-21 11:57 ` Daniel Shahaf 0 siblings, 1 reply; 7+ messages in thread From: Matthew Martin @ 2019-03-21 0:16 UTC (permalink / raw) To: Daniel Shahaf; +Cc: zsh-workers 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! ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] _pick_variant: Update builtin check 2019-03-21 0:16 ` Matthew Martin @ 2019-03-21 11:57 ` Daniel Shahaf 2019-03-21 12:40 ` Matthew Martin 0 siblings, 1 reply; 7+ messages in thread From: Daniel Shahaf @ 2019-03-21 11:57 UTC (permalink / raw) To: zsh-workers Matthew Martin wrote on Thu, 21 Mar 2019 00:17 +00:00: > 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: > > > +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) I understand what you mean in the context of this thread, but I think the two uses of the term 'command' might be confusing to someone who opens the file in a year or three. How about: # Precommands which allow their wrapped command to be a builtin. # All of these are necessarily builtins or reserved words themselves, but not all builtin precommands are listed here: for one, the 'command' builtin is excluded. Cheers, Daniel ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] _pick_variant: Update builtin check 2019-03-21 11:57 ` Daniel Shahaf @ 2019-03-21 12:40 ` Matthew Martin 0 siblings, 0 replies; 7+ messages in thread From: Matthew Martin @ 2019-03-21 12:40 UTC (permalink / raw) To: Daniel Shahaf; +Cc: zsh-workers On Thu, Mar 21, 2019 at 07:57:38AM -0400, Daniel Shahaf wrote: > I understand what you mean in the context of this thread, but I think > the two uses of the term 'command' might be confusing to someone who > opens the file in a year or three. How about: > > # Precommands which allow their wrapped command to be a builtin. > # All of these are necessarily builtins or reserved words themselves, but not all builtin precommands are listed here: for one, the 'command' builtin is excluded. Works for me. Unless there are other comments I'll commit later today. - Matthew Martin From 06741ae0671c4a29d2700afa46470a8024ddbefb Mon Sep 17 00:00:00 2001 From: Matthew Martin <phy1729@gmail.com> Date: Tue, 19 Mar 2019 20:42:35 -0500 Subject: [PATCH] _pick_variant: Update builtin check --- Completion/Base/Utility/_pick_variant | 30 ++++++++++++++++++++------- 1 file changed, 22 insertions(+), 8 deletions(-) diff --git a/Completion/Base/Utility/_pick_variant b/Completion/Base/Utility/_pick_variant index 9099e3599..872e8f583 100644 --- a/Completion/Base/Utility/_pick_variant +++ b/Completion/Base/Utility/_pick_variant @@ -1,9 +1,15 @@ #autoload -local output cmd pat +local output cmd pat pre local -a var local -A opts +# Precommands which allow their wrapped command to be a builtin. +# All of these are necessarily builtins or reserved words themselves, +# but not all builtin precommands are listed here: +# for one, the 'command' builtin is excluded. +local -ar builtin_precommands=(- builtin eval exec nocorrect noglob time) + (( $+_cmd_variant )) || typeset -gA _cmd_variant zparseopts -D -A opts b: c: r: @@ -13,19 +19,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[(I)builtin] || $+builtins[$opts[-c]] ) )); then + (( $+opts[-r] )) && eval "${opts[-r]}=$opts[-b]" return 0 +elif (( $precommands[(I)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 +50,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 -- 2.21.0 ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2019-03-21 12:41 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-03-20 2:05 [PATCH] _pick_variant: Update builtin check Matthew Martin 2019-03-20 3:38 ` Matthew Martin 2019-03-20 12:52 ` Matthew Martin 2019-03-20 13:05 ` Daniel Shahaf 2019-03-21 0:16 ` Matthew Martin 2019-03-21 11:57 ` Daniel Shahaf 2019-03-21 12:40 ` Matthew Martin
Code repositories for project(s) associated with this public inbox https://git.vuxu.org/mirror/zsh/ This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).