From d93a6abb2b37bf111a4bab21d195d63bf7681f17 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ville=20Skytt=C3=A4?= Date: Mon, 10 Apr 2023 16:41:00 +0300 Subject: [PATCH 01/16] refactor(__load_completion): improve variable naming --- bash_completion | 30 +++++++++++++++--------------- 1 file changed, 15 insertions(+), 15 deletions(-) diff --git a/bash_completion b/bash_completion index 094bf2b9f72..e40ea65f9da 100644 --- a/bash_completion +++ b/bash_completion @@ -2496,9 +2496,9 @@ complete -F _minimal '' __load_completion() { - local cmd="${1##*/}" dir compfile + local cmd=$1 cmdname=${1##*/} dir compfile local -a paths - [[ $cmd ]] || return 1 + [[ $cmdname ]] || return 1 local -a dirs=() @@ -2523,9 +2523,9 @@ __load_completion() dirs+=(./completions) fi - # 3) From bin directories extracted from $(realpath "$cmd") and PATH + # 3) From bin directories extracted from $(realpath "$cmdname") and PATH local ret - _comp_realcommand "$1" && paths=("${ret%/*}") || paths=() + _comp_realcommand "$cmd" && paths=("${ret%/*}") || paths=() _comp_split -aF : paths "$PATH" for dir in "${paths[@]%/}"; do if [[ -d $dir && $dir == ?*/@(bin|sbin) ]]; then @@ -2539,10 +2539,10 @@ __load_completion() dirs+=("${paths[@]/%//bash-completion/completions}") local backslash= - if [[ $cmd == \\* ]]; then - cmd=${cmd:1} + if [[ $cmdname == \\* ]]; then + cmdname=${cmdname:1} # If we already have a completion for the "real" command, use it - $(complete -p "$cmd" 2>/dev/null || echo false) "\\$cmd" && return 0 + $(complete -p "$cmdname" 2>/dev/null || echo false) "\\$cmdname" && return 0 backslash=\\ fi @@ -2551,7 +2551,7 @@ __load_completion() for dir in "${dirs[@]}"; do [[ -d $dir ]] || continue - for compfile in "$cmd" "$cmd.bash"; do + for compfile in "$cmdname" "$cmdname.bash"; do compfile="$dir/$compfile" # Avoid trying to source dirs as long as we support bash < 4.3 # to avoid an fd leak; https://bugzilla.redhat.com/903540 @@ -2560,31 +2560,31 @@ __load_completion() [[ $compfile == */.?(.) ]] || echo "bash_completion: $compfile: is a directory" >&2 elif [[ -e $compfile ]] && . "$compfile"; then - [[ $backslash ]] && $(complete -p "$cmd") "\\$cmd" + [[ $backslash ]] && $(complete -p "$cmdname") "\\$cmdname" return 0 fi done done - # Search fallback completions named "_$cmd" + # Search fallback completions named "_$cmdname" for dir in "${dirs[@]}"; do [[ -d $dir ]] || continue - compfile="$dir/_$cmd" + compfile="$dir/_$cmdname" # Avoid trying to source dirs as long as we support bash < 4.3 # to avoid an fd leak; https://bugzilla.redhat.com/903540 if [[ -d $compfile ]]; then # Do not warn with . or .. (especially the former is common) [[ $compfile == */.?(.) ]] || echo "bash_completion: $compfile: is a directory" >&2 - elif [[ -e $compfile ]] && . "$compfile" "$cmd"; then - [[ $backslash ]] && $(complete -p "$cmd") "\\$cmd" + elif [[ -e $compfile ]] && . "$compfile" "$cmdname"; then + [[ $backslash ]] && $(complete -p "$cmdname") "\\$cmdname" return 0 fi done # Look up simple "xspec" completions - [[ -v _xspecs[$cmd] ]] && - complete -F _filedir_xspec "$cmd" "$backslash$cmd" && return 0 + [[ -v _xspecs[$cmdname] ]] && + complete -F _filedir_xspec "$cmdname" "$backslash$cmdname" && return 0 return 1 } From f6a2211486eb6f921015e1cb795736cf5b49f7cc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ville=20Skytt=C3=A4?= Date: Mon, 10 Apr 2023 16:57:04 +0300 Subject: [PATCH 02/16] refactor(__load_completion): handle backslash always at start of command Previously, we handled it at the start of the command's _basename_. Even though the backslash doesn't make a practical difference with regards to aliases with paths, it seems more logical and clearer to handle it this way. When invoked as just the basename, this does not change anything. --- bash_completion | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/bash_completion b/bash_completion index e40ea65f9da..43b4426d073 100644 --- a/bash_completion +++ b/bash_completion @@ -2500,6 +2500,14 @@ __load_completion() local -a paths [[ $cmdname ]] || return 1 + local backslash= + if [[ $cmd == \\* ]]; then + cmd=${cmd:1} + # If we already have a completion for the "real" command, use it + $(complete -p "$cmd" 2>/dev/null || echo false) "\\$cmd" && return 0 + backslash=\\ + fi + local -a dirs=() # Lookup order: @@ -2538,14 +2546,6 @@ __load_completion() _comp_split -F : paths "${XDG_DATA_DIRS:-/usr/local/share:/usr/share}" dirs+=("${paths[@]/%//bash-completion/completions}") - local backslash= - if [[ $cmdname == \\* ]]; then - cmdname=${cmdname:1} - # If we already have a completion for the "real" command, use it - $(complete -p "$cmdname" 2>/dev/null || echo false) "\\$cmdname" && return 0 - backslash=\\ - fi - # For loading 3rd party completions wrapped in shopt reset local IFS=$' \t\n' From e01c4a1e2716fdc2fc7ac471948f24fa9c69e8ca Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ville=20Skytt=C3=A4?= Date: Mon, 10 Apr 2023 17:19:06 +0300 Subject: [PATCH 03/16] fix(__load_completion): pass full path to fallback completion loaders Passing just the basename results in failures when a command invoked (with a path) is not in `$PATH`. Also facilitates correct command instance being used to generate the completion in our fallback completion loaders. Completions generated by different instances of the command might vary. --- bash_completion | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/bash_completion b/bash_completion index 43b4426d073..d1eae534a76 100644 --- a/bash_completion +++ b/bash_completion @@ -2576,7 +2576,7 @@ __load_completion() # Do not warn with . or .. (especially the former is common) [[ $compfile == */.?(.) ]] || echo "bash_completion: $compfile: is a directory" >&2 - elif [[ -e $compfile ]] && . "$compfile" "$cmdname"; then + elif [[ -e $compfile ]] && . "$compfile" "$cmd" "$@"; then [[ $backslash ]] && $(complete -p "$cmdname") "\\$cmdname" return 0 fi From 9e076543de380e08467e9aa45951c172826f8022 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ville=20Skytt=C3=A4?= Date: Mon, 10 Apr 2023 17:23:23 +0300 Subject: [PATCH 04/16] feat(__load_completion): source all completions with same args For consistency and completeness, e.g. arg #'s > 1 for fallback loaders, and the same (possibly processed) command. --- bash_completion | 49 +++++++++++++++++++------------------------------ 1 file changed, 19 insertions(+), 30 deletions(-) diff --git a/bash_completion b/bash_completion index d1eae534a76..702a39b3073 100644 --- a/bash_completion +++ b/bash_completion @@ -2549,39 +2549,28 @@ __load_completion() # For loading 3rd party completions wrapped in shopt reset local IFS=$' \t\n' - for dir in "${dirs[@]}"; do - [[ -d $dir ]] || continue - for compfile in "$cmdname" "$cmdname.bash"; do - compfile="$dir/$compfile" - # Avoid trying to source dirs as long as we support bash < 4.3 - # to avoid an fd leak; https://bugzilla.redhat.com/903540 - if [[ -d $compfile ]]; then - # Do not warn with . or .. (especially the former is common) - [[ $compfile == */.?(.) ]] || - echo "bash_completion: $compfile: is a directory" >&2 - elif [[ -e $compfile ]] && . "$compfile"; then - [[ $backslash ]] && $(complete -p "$cmdname") "\\$cmdname" - return 0 - fi + # Look up and source + shift + local prefix + for prefix in "" _; do # Regular from all dirs first, then fallbacks + for dir in "${dirs[@]}"; do + [[ -d $dir ]] || continue + for compfile in "$prefix$cmdname" "$prefix$cmdname.bash"; do + compfile="$dir/$compfile" + # Avoid trying to source dirs as long as we support bash < 4.3 + # to avoid an fd leak; https://bugzilla.redhat.com/903540 + if [[ -d $compfile ]]; then + # Do not warn with . or .. (especially the former is common) + [[ $compfile == */.?(.) ]] || + echo "bash_completion: $compfile: is a directory" >&2 + elif [[ -e $compfile ]] && . "$compfile" "$cmd" "$@"; then + [[ $backslash ]] && $(complete -p "$cmdname") "\\$cmdname" + return 0 + fi + done done done - # Search fallback completions named "_$cmdname" - for dir in "${dirs[@]}"; do - [[ -d $dir ]] || continue - compfile="$dir/_$cmdname" - # Avoid trying to source dirs as long as we support bash < 4.3 - # to avoid an fd leak; https://bugzilla.redhat.com/903540 - if [[ -d $compfile ]]; then - # Do not warn with . or .. (especially the former is common) - [[ $compfile == */.?(.) ]] || - echo "bash_completion: $compfile: is a directory" >&2 - elif [[ -e $compfile ]] && . "$compfile" "$cmd" "$@"; then - [[ $backslash ]] && $(complete -p "$cmdname") "\\$cmdname" - return 0 - fi - done - # Look up simple "xspec" completions [[ -v _xspecs[$cmdname] ]] && complete -F _filedir_xspec "$cmdname" "$backslash$cmdname" && return 0 From 6b8d4a5b5963c1af9710099b146a201f55fa6b87 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ville=20Skytt=C3=A4?= Date: Mon, 10 Apr 2023 17:28:22 +0300 Subject: [PATCH 05/16] feat(__load_completion): pass absolute path when sourcing and available Mostly for better handling of completions loaded via relative paths, in case the completion registers it for the exact command passed to it. --- bash_completion | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/bash_completion b/bash_completion index 702a39b3073..7e4bfa7dfe2 100644 --- a/bash_completion +++ b/bash_completion @@ -2508,6 +2508,13 @@ __load_completion() backslash=\\ fi + # Try to resolve real path to $cmd, and make it absolute + local ret realcmd= + if _comp_realcommand "$cmd"; then + realcmd=$ret + cmd=${realcmd%/*}/$cmdname # basename could be an alias + fi + local -a dirs=() # Lookup order: @@ -2531,9 +2538,8 @@ __load_completion() dirs+=(./completions) fi - # 3) From bin directories extracted from $(realpath "$cmdname") and PATH - local ret - _comp_realcommand "$cmd" && paths=("${ret%/*}") || paths=() + # 3) From bin directories extracted from path to command, and $PATH + [[ $realcmd ]] && paths=("${realcmd%/*}") || paths=() _comp_split -aF : paths "$PATH" for dir in "${paths[@]%/}"; do if [[ -d $dir && $dir == ?*/@(bin|sbin) ]]; then From 42b2c0211cb18a98e7cd4727a5aa9a69e3fa1f31 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ville=20Skytt=C3=A4?= Date: Mon, 10 Apr 2023 20:55:01 +0300 Subject: [PATCH 06/16] perf(__load_completion): reduce number of `stat` calls --- bash_completion | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/bash_completion b/bash_completion index 7e4bfa7dfe2..74c5f060301 100644 --- a/bash_completion +++ b/bash_completion @@ -2542,9 +2542,8 @@ __load_completion() [[ $realcmd ]] && paths=("${realcmd%/*}") || paths=() _comp_split -aF : paths "$PATH" for dir in "${paths[@]%/}"; do - if [[ -d $dir && $dir == ?*/@(bin|sbin) ]]; then + [[ $dir == ?*/@(bin|sbin) ]] && dirs+=("${dir%/*}/share/bash-completion/completions") - fi done # 4) From XDG_DATA_DIRS or system dirs (e.g. /usr/share, /usr/local/share): @@ -2557,10 +2556,14 @@ __load_completion() # Look up and source shift - local prefix + local i prefix compspec for prefix in "" _; do # Regular from all dirs first, then fallbacks - for dir in "${dirs[@]}"; do - [[ -d $dir ]] || continue + for i in ${!dirs[*]}; do + dir=${dirs[i]} + if [[ ! -d $dir ]]; then + unset -v 'dirs[i]' + continue + fi for compfile in "$prefix$cmdname" "$prefix$cmdname.bash"; do compfile="$dir/$compfile" # Avoid trying to source dirs as long as we support bash < 4.3 From f16dd8049850a30cb298c2c450bf02f1e4451d72 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ville=20Skytt=C3=A4?= Date: Mon, 10 Apr 2023 22:09:54 +0300 Subject: [PATCH 07/16] feat(__load_completion): check that a completion was actually set On successful return, at least the full path to the command in question should have a completion set. If the command invoked was given without a path, the pathless one should have one set, too. --- bash_completion | 28 ++++++++++++++++--- .../share/bash-completion/completions/cmd1 | 1 + .../share/bash-completion/completions/cmd2 | 1 + .../userdir1/completions/cmd1 | 1 + .../userdir2/completions/cmd2 | 1 + test/t/unit/test_unit_load_completion.py | 14 ++++++++++ 6 files changed, 42 insertions(+), 4 deletions(-) diff --git a/bash_completion b/bash_completion index 74c5f060301..134bd3d1f8f 100644 --- a/bash_completion +++ b/bash_completion @@ -2509,7 +2509,7 @@ __load_completion() fi # Try to resolve real path to $cmd, and make it absolute - local ret realcmd= + local ret realcmd="" origcmd=$cmd if _comp_realcommand "$cmd"; then realcmd=$ret cmd=${realcmd%/*}/$cmdname # basename could be an alias @@ -2551,7 +2551,8 @@ __load_completion() _comp_split -F : paths "${XDG_DATA_DIRS:-/usr/local/share:/usr/share}" dirs+=("${paths[@]/%//bash-completion/completions}") - # For loading 3rd party completions wrapped in shopt reset + # Set up default $IFS in case loaded completions depend on it, + # as well as for $compspec invocation below. local IFS=$' \t\n' # Look up and source @@ -2573,8 +2574,27 @@ __load_completion() [[ $compfile == */.?(.) ]] || echo "bash_completion: $compfile: is a directory" >&2 elif [[ -e $compfile ]] && . "$compfile" "$cmd" "$@"; then - [[ $backslash ]] && $(complete -p "$cmdname") "\\$cmdname" - return 0 + # At least $cmd is expected to have a completion set when + # we return successfully; see if it already does + if compspec=$(complete -p "$cmd" 2>/dev/null); then + local -a extspecs=() + # $cmd is the case in which we do backslash processing + [[ $backslash ]] && extspecs+=("$backslash$cmd") + # If invoked without path, that one should be set, too + # ...but let's not overwrite an existing one, if any + [[ $origcmd != */* ]] && + ! complete -p "$origcmd" &>/dev/null && + extspecs+=("$origcmd") + ((${#extspecs[*]} != 0)) && $compspec "${extspecs[@]}" + return 0 + fi + # If not, see if we got one for $cmdname + if compspec=$(complete -p "$cmdname" 2>/dev/null); then + # Use that for $cmd too, if we have a full path to it + [[ $cmd == /* ]] && $compspec "$cmd" + return 0 + fi + # Nothing expected was set, continue lookup fi done done diff --git a/test/fixtures/__load_completion/prefix1/share/bash-completion/completions/cmd1 b/test/fixtures/__load_completion/prefix1/share/bash-completion/completions/cmd1 index e2f434e7b47..378a6e3fd3f 100644 --- a/test/fixtures/__load_completion/prefix1/share/bash-completion/completions/cmd1 +++ b/test/fixtures/__load_completion/prefix1/share/bash-completion/completions/cmd1 @@ -1 +1,2 @@ echo 'cmd1: sourced from prefix1' +complete -C true "$1" diff --git a/test/fixtures/__load_completion/prefix1/share/bash-completion/completions/cmd2 b/test/fixtures/__load_completion/prefix1/share/bash-completion/completions/cmd2 index 8549e626e3a..167ad624ecd 100644 --- a/test/fixtures/__load_completion/prefix1/share/bash-completion/completions/cmd2 +++ b/test/fixtures/__load_completion/prefix1/share/bash-completion/completions/cmd2 @@ -1 +1,2 @@ echo 'cmd2: sourced from prefix1' +complete -C true "$1" diff --git a/test/fixtures/__load_completion/userdir1/completions/cmd1 b/test/fixtures/__load_completion/userdir1/completions/cmd1 index 5e88e908e19..b26bf1fe393 100644 --- a/test/fixtures/__load_completion/userdir1/completions/cmd1 +++ b/test/fixtures/__load_completion/userdir1/completions/cmd1 @@ -1 +1,2 @@ echo 'cmd1: sourced from userdir1' +complete -C true "$1" diff --git a/test/fixtures/__load_completion/userdir2/completions/cmd2 b/test/fixtures/__load_completion/userdir2/completions/cmd2 index 135d084a23f..667989bb687 100644 --- a/test/fixtures/__load_completion/userdir2/completions/cmd2 +++ b/test/fixtures/__load_completion/userdir2/completions/cmd2 @@ -1 +1,2 @@ echo 'cmd2: sourced from userdir2' +complete -C true "$1" diff --git a/test/t/unit/test_unit_load_completion.py b/test/t/unit/test_unit_load_completion.py index be22a1ea78c..1a6a798dc2a 100644 --- a/test/t/unit/test_unit_load_completion.py +++ b/test/t/unit/test_unit_load_completion.py @@ -37,12 +37,26 @@ def test_PATH_1(self, bash): bash, "__load_completion cmd2", want_output=True ) assert output.strip() == "cmd2: sourced from prefix1" + output = assert_bash_exec( + bash, "complete -p cmd2", want_output=True + ) + assert " cmd2" in output + output = assert_bash_exec( + bash, 'complete -p "$PWD/prefix1/sbin/cmd2"', want_output=True + ) + assert "/prefix1/sbin/cmd2" in output def test_cmd_path_1(self, bash): + assert_bash_exec(bash, "complete -r cmd1 || :", want_output=None) output = assert_bash_exec( bash, "__load_completion prefix1/bin/cmd1", want_output=True ) assert output.strip() == "cmd1: sourced from prefix1" + output = assert_bash_exec( + bash, 'complete -p "$PWD/prefix1/bin/cmd1"', want_output=True + ) + assert "/prefix1/bin/cmd1" in output + assert_bash_exec(bash, "! complete -p cmd1", want_output=None) output = assert_bash_exec( bash, "__load_completion prefix1/sbin/cmd2", want_output=True ) From 3f479c173b9bd2619962a47b120e48acd54d6c0b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ville=20Skytt=C3=A4?= Date: Tue, 11 Apr 2023 22:28:21 +0300 Subject: [PATCH 08/16] docs(__load_completion): elaborate on preserving basename --- bash_completion | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/bash_completion b/bash_completion index 134bd3d1f8f..bfe084f7834 100644 --- a/bash_completion +++ b/bash_completion @@ -2512,7 +2512,10 @@ __load_completion() local ret realcmd="" origcmd=$cmd if _comp_realcommand "$cmd"; then realcmd=$ret - cmd=${realcmd%/*}/$cmdname # basename could be an alias + # Retain original basename, target $realcmd might behave differently + # depending on what it gets invoked as, so we should not install + # a completion for a different one for it. For example `busybox`. + cmd=${realcmd%/*}/$cmdname fi local -a dirs=() From 95eef92e5f6934c395f82720589f9c2a1dce1f73 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ville=20Skytt=C3=A4?= Date: Tue, 11 Apr 2023 22:37:48 +0300 Subject: [PATCH 09/16] refactor(fallbacks): remove checks if a completion got installed `__load_completion` takes care of those now. --- completions/_cargo | 4 ---- completions/_gh | 4 ---- completions/_golangci-lint | 4 ---- completions/_nox | 2 -- completions/_ruff | 4 ---- completions/_rustup | 4 ---- completions/_yq | 4 ---- 7 files changed, 26 deletions(-) diff --git a/completions/_cargo b/completions/_cargo index fcb5e2781df..b5b86ad799c 100644 --- a/completions/_cargo +++ b/completions/_cargo @@ -6,8 +6,4 @@ local rustup="${1%cargo}rustup" # use rustup from same dir eval -- "$("$rustup" completions bash cargo 2>/dev/null)" -{ - complete -p "$1" || complete -p "${1##*/}" -} &>/dev/null - # ex: filetype=sh diff --git a/completions/_gh b/completions/_gh index 7b0daefed8b..8a0376dd33f 100644 --- a/completions/_gh +++ b/completions/_gh @@ -5,8 +5,4 @@ eval -- "$("$1" completion --shell bash 2>/dev/null)" -{ - complete -p "$1" || complete -p "${1##*/}" -} &>/dev/null - # ex: filetype=sh diff --git a/completions/_golangci-lint b/completions/_golangci-lint index 830fa536aa0..40fc5c37b6c 100644 --- a/completions/_golangci-lint +++ b/completions/_golangci-lint @@ -6,8 +6,4 @@ eval -- "$("$1" completion bash 2>/dev/null)" -{ - complete -p "$1" || complete -p "${1##*/}" -} &>/dev/null - # ex: filetype=sh diff --git a/completions/_nox b/completions/_nox index 9bef453c0ab..40b8bb182c1 100644 --- a/completions/_nox +++ b/completions/_nox @@ -10,6 +10,4 @@ eval -- "$( register-python-argcomplete3 --shell bash "$1" 2>/dev/null )" -complete -p "$1" &>/dev/null - # ex: filetype=sh diff --git a/completions/_ruff b/completions/_ruff index 71f7c0c6524..b5fd1a5381c 100644 --- a/completions/_ruff +++ b/completions/_ruff @@ -5,8 +5,4 @@ eval -- "$("$1" generate-shell-completion bash 2>/dev/null)" -{ - complete -p "$1" || complete -p "${1##*/}" -} &>/dev/null - # ex: filetype=sh diff --git a/completions/_rustup b/completions/_rustup index 0f2fe6e4056..1bcf44da7b3 100644 --- a/completions/_rustup +++ b/completions/_rustup @@ -4,8 +4,4 @@ eval -- "$("$1" completions bash rustup 2>/dev/null)" -{ - complete -p "$1" || complete -p "${1##*/}" -} &>/dev/null - # ex: filetype=sh diff --git a/completions/_yq b/completions/_yq index ff662bf9ede..c357bd9c440 100644 --- a/completions/_yq +++ b/completions/_yq @@ -5,8 +5,4 @@ eval -- "$("$1" shell-completion bash 2>/dev/null)" -{ - complete -p "$1" || complete -p "${1##*/}" -} &>/dev/null - # ex: filetype=sh From a44ba64b74bb1b6d1875665224590e48874c9d84 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ville=20Skytt=C3=A4?= Date: Tue, 11 Apr 2023 22:39:16 +0300 Subject: [PATCH 10/16] refactor(_vault): do not install for command basename `__load_completion` takes care of that now when appropriate. --- completions/_vault | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/completions/_vault b/completions/_vault index 468236f5827..25abbc20a07 100644 --- a/completions/_vault +++ b/completions/_vault @@ -3,6 +3,6 @@ # # This serves as a fallback in case the completion is not installed otherwise. -type "$1" &>/dev/null && complete -C "\"$1\" 2>/dev/null" "$1" "${1##*/}" +type "$1" &>/dev/null && complete -C "\"$1\" 2>/dev/null" "$1" # ex: filetype=sh From 03bdac41019e432d48ab887a11b4b95c19a9455b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ville=20Skytt=C3=A4?= Date: Tue, 11 Apr 2023 22:52:00 +0300 Subject: [PATCH 11/16] perf(__load_completion): avoid a subshell on completion install check https://github.com/scop/bash-completion/pull/924#discussion_r1163232244 Co-authored-by: Koichi Murase --- bash_completion | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/bash_completion b/bash_completion index bfe084f7834..2d19ae67788 100644 --- a/bash_completion +++ b/bash_completion @@ -2592,7 +2592,7 @@ __load_completion() return 0 fi # If not, see if we got one for $cmdname - if compspec=$(complete -p "$cmdname" 2>/dev/null); then + if [[ $cmdname != "$cmd" ]] && compspec=$(complete -p "$cmdname" 2>/dev/null); then # Use that for $cmd too, if we have a full path to it [[ $cmd == /* ]] && $compspec "$cmd" return 0 From 28b587a47058395fdacb7eafe866685af1e74e3c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ville=20Skytt=C3=A4?= Date: Tue, 11 Apr 2023 23:29:59 +0300 Subject: [PATCH 12/16] feat(_comp_abspath): new utility function, use in `_comp_realcommand` --- bash_completion | 30 ++++++++++++++++++++---------- 1 file changed, 20 insertions(+), 10 deletions(-) diff --git a/bash_completion b/bash_completion index 2d19ae67788..89cdcd8a302 100644 --- a/bash_completion +++ b/bash_completion @@ -1659,6 +1659,25 @@ _fstypes() [[ $fss ]] && COMPREPLY+=($(compgen -W "$fss" -- "$cur")) } +# Get absolute path to a file, with rudimentary canonicalization. +# No symlink resolution or existence checks are done; +# see `_comp_realcommand` for those. +# @param $1 The file +# @var[out] ret The path +_comp_abspath() +{ + ret=$1 + case $ret in + /*) ;; + ../*) ret=$PWD/${ret:3} ;; + *) ret=$PWD/$ret ;; + esac + while [[ $ret == */./* ]]; do + ret=${ret//\/.\//\/} + done + ret=${ret//+(\/)/\/} +} + # Get real command. # Command is the filename of command in PATH with possible symlinks resolved # (if resolve tooling available), empty string if command not found. @@ -1677,16 +1696,7 @@ _comp_realcommand() elif type -p readlink >/dev/null; then ret=$(readlink -f "$file") else - ret=$file - if [[ $ret == */* ]]; then - if [[ $ret == ./* ]]; then - ret=$PWD/${file:2} - elif [[ $ret == ../* ]]; then - ret=$PWD/${file:3} - elif [[ $ret != /* ]]; then - ret=$PWD/$file - fi - fi + _comp_abspath "$file" fi } From e4ede514249c4250dd43139f54fa3c46554a1dbc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ville=20Skytt=C3=A4?= Date: Tue, 11 Apr 2023 23:45:31 +0300 Subject: [PATCH 13/16] fix(__load_completion): don't resolve symlinks to invoked command This might make the path we install completions for different from the invoked path so that the completion would fail to actually load after setup. The final path segment might also have a different name in the resolved symlink, and we should not install it for the resolved name, because the target's functionality might vary based on the invocation name. Simply replacing the final path segment with the original basename could yield a path that does not exist, even though the original invoked one does. https://github.com/scop/bash-completion/pull/924#discussion_r1163249029 --- bash_completion | 15 ++++++--------- 1 file changed, 6 insertions(+), 9 deletions(-) diff --git a/bash_completion b/bash_completion index 89cdcd8a302..6baae8a2fb7 100644 --- a/bash_completion +++ b/bash_completion @@ -2518,14 +2518,11 @@ __load_completion() backslash=\\ fi - # Try to resolve real path to $cmd, and make it absolute - local ret realcmd="" origcmd=$cmd - if _comp_realcommand "$cmd"; then - realcmd=$ret - # Retain original basename, target $realcmd might behave differently - # depending on what it gets invoked as, so we should not install - # a completion for a different one for it. For example `busybox`. - cmd=${realcmd%/*}/$cmdname + # Resolve absolute path to $cmd + local ret pathcmd origcmd=$cmd + if pathcmd=$(type -P "$cmd"); then + _comp_abspath "$pathcmd" + cmd=$ret fi local -a dirs=() @@ -2552,7 +2549,7 @@ __load_completion() fi # 3) From bin directories extracted from path to command, and $PATH - [[ $realcmd ]] && paths=("${realcmd%/*}") || paths=() + [[ $cmd == */* ]] && paths=("${cmd%/*}") || paths=() _comp_split -aF : paths "$PATH" for dir in "${paths[@]%/}"; do [[ $dir == ?*/@(bin|sbin) ]] && From 61982cbcf5cb283dfcf6b7863127fd1dda2e5985 Mon Sep 17 00:00:00 2001 From: Koichi Murase Date: Wed, 12 Apr 2023 07:24:32 +0900 Subject: [PATCH 14/16] fix(__load_completion): search completions in share/ from real path MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Ville Skyttä --- bash_completion | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/bash_completion b/bash_completion index 6baae8a2fb7..31d62b685f6 100644 --- a/bash_completion +++ b/bash_completion @@ -2548,8 +2548,12 @@ __load_completion() dirs+=(./completions) fi - # 3) From bin directories extracted from path to command, and $PATH - [[ $cmd == */* ]] && paths=("${cmd%/*}") || paths=() + # 3) From bin directories extracted from the specified path to the command, + # the real path to the command, and $PATH + paths=() + [[ $cmd == */* ]] && paths+=("${cmd%/*}") + local ret + _comp_realcommand "$cmd" && paths+=("${ret%/*}") _comp_split -aF : paths "$PATH" for dir in "${paths[@]%/}"; do [[ $dir == ?*/@(bin|sbin) ]] && From 6b75d64667446d11c89cd36836d29b5d40c76d82 Mon Sep 17 00:00:00 2001 From: Koichi Murase Date: Wed, 12 Apr 2023 07:27:11 +0900 Subject: [PATCH 15/16] fix(__load_completion): check if `cmd` is absolute for bin dir --- bash_completion | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/bash_completion b/bash_completion index 31d62b685f6..6046334ab72 100644 --- a/bash_completion +++ b/bash_completion @@ -2551,7 +2551,7 @@ __load_completion() # 3) From bin directories extracted from the specified path to the command, # the real path to the command, and $PATH paths=() - [[ $cmd == */* ]] && paths+=("${cmd%/*}") + [[ $cmd == /* ]] && paths+=("${cmd%/*}") local ret _comp_realcommand "$cmd" && paths+=("${ret%/*}") _comp_split -aF : paths "$PATH" From 01e1ae56f7d931f72fe456c8284e22e30583e978 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ville=20Skytt=C3=A4?= Date: Mon, 17 Apr 2023 22:43:09 +0300 Subject: [PATCH 16/16] test(_comp_abspath): add some unit tests --- test/t/unit/Makefile.am | 1 + test/t/unit/test_unit_abspath.py | 67 ++++++++++++++++++++++++++++++++ 2 files changed, 68 insertions(+) create mode 100644 test/t/unit/test_unit_abspath.py diff --git a/test/t/unit/Makefile.am b/test/t/unit/Makefile.am index 1db106cc6dd..8f8e5671959 100644 --- a/test/t/unit/Makefile.am +++ b/test/t/unit/Makefile.am @@ -1,4 +1,5 @@ EXTRA_DIST = \ + test_unit_abspath.py \ test_unit_command_offset.py \ test_unit_count_args.py \ test_unit_deprecate_func.py \ diff --git a/test/t/unit/test_unit_abspath.py b/test/t/unit/test_unit_abspath.py new file mode 100644 index 00000000000..ff356ccae43 --- /dev/null +++ b/test/t/unit/test_unit_abspath.py @@ -0,0 +1,67 @@ +import pytest + +from conftest import assert_bash_exec + + +@pytest.mark.bashcomp( + cmd=None, cwd="shared", ignore_env=r"^\+declare -f __tester$" +) +class TestUnitAbsPath: + @pytest.fixture + def functions(self, bash): + assert_bash_exec( + bash, + ( + "__tester() { " + "local ret; " + '_comp_abspath "$1"; ' + 'printf %s "$ret"; ' + "}" + ), + ) + + def test_non_pollution(self, bash): + """Test environment non-pollution, detected at teardown.""" + assert_bash_exec( + bash, + "foo() { local ret=; _comp_abspath bar; }; foo; unset -f foo", + want_output=None, + ) + + def test_absolute(self, bash, functions): + output = assert_bash_exec( + bash, + "__tester /foo/bar", + want_output=True, + want_newline=False, + ) + assert output.strip() == "/foo/bar" + + def test_relative(self, bash, functions): + output = assert_bash_exec( + bash, + "__tester foo/bar", + want_output=True, + want_newline=False, + ) + assert output.strip().endswith("/shared/foo/bar") + + def test_cwd(self, bash, functions): + output = assert_bash_exec( + bash, + "__tester ./foo/./bar", + want_output=True, + want_newline=False, + ) + assert output.strip().endswith("/shared/foo/bar") + + def test_parent(self, bash, functions): + output = assert_bash_exec( + bash, + "__tester ../shared/foo/bar", + want_output=True, + want_newline=False, + ) + assert output.strip().endswith( + "/shared/foo/bar" + ) and not output.strip().endswith("../shared/foo/bar")