Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix subcommand/choices completion in bash #106

Merged
merged 2 commits into from
Oct 29, 2022
Merged

Fix subcommand/choices completion in bash #106

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
cb81d71
bash: fix subcommand completion
brentyi Sep 9, 2022
9bfda44
fix #74 again
casperdcl Oct 29, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 5 additions & 4 deletions shtab/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -352,7 +352,7 @@ def complete_bash(parser, root_prefix=None, preamble="", choice_functions=None):
local current_action_compgen_var=${current_action}_COMPGEN
current_action_compgen="${!current_action_compgen_var}"

local current_action_choices_var="${current_action}_choices"
local current_action_choices_var="${current_action}_choices[@]"
current_action_choices="${!current_action_choices_var}"

local current_action_nargs_var="${current_action}_nargs"
Expand Down Expand Up @@ -420,10 +420,11 @@ def complete_bash(parser, root_prefix=None, preamble="", choice_functions=None):
COMPREPLY=( $(compgen -W "${current_option_strings[*]}" -- "${completing_word}") )
else
# use choices & compgen
local IFS=$'\\n'
COMPREPLY=( $(compgen -W "${current_action_choices}" -- "${completing_word}") \\
$([ -n "${current_action_compgen}" ] \\
local IFS=$'\\n' # items may contain spaces, so delimit using newline
COMPREPLY=( $([ -n "${current_action_compgen}" ] \\
&& "${current_action_compgen}" "${completing_word}") )
unset IFS
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@casperdcl Re your comment on SO:

you can also unset IFS as soon as it's served its purpose. That way subsequent commands can be simplified

No it's required by the compgen -W line for the same reason I wrote on the answer.

Instead of going over them again, I'll just give you examples showing you 1. why it's not the culprit, 2. why it's required. After that, I'll point out 3. where the bug in this code is.

1. Why it's not the culprit:

$ CHOICES=(bla blue)
$ printf "[%s]\n" "${CHOICES[@]}"
[bla]
[blue]
$ compgen -W "${CHOICES[*]}" bl
bla
blue
$ IFS=$'\n'
$ compgen -W "${CHOICES[*]}" bl
bla
blue
$ unset IFS

👆 Having IFS set doesn't change the outcome.

2. Why it's required:

$ CHOICES=("big bla" "big blue")
$ printf "[%s]\n" "${CHOICES[@]}"
[big bla]
[big blue]
$ compgen -W "${CHOICES[*]}" "big bl" # Broken without IFS setting
$ IFS=$'\n'
$ compgen -W "${CHOICES[*]}" "big bl"
big bla
big blue
$ unset IFS

👆 Because IFS wasn't set before the first compgen -W, compgen treated the candate list as big, bla, blue instead of big bla and big blue.

3. Where the bug in this code is

It's because current_action_choices isn't an array. The original *_choices variable is probably an array but the following line converts it to a scalar:

local current_action_choices_var="${current_action}_choices[@]"
current_action_choices="${!current_action_choices_var}"

Instead you need to do:

 local current_action_choices_var="${current_action}_choices[@]"
-current_action_choices="${!current_action_choices_var}"
+current_action_choices=("${!current_action_choices_var}")

Here's a reproduction:

$ FOO=(a b "c d")
printf "[%s]\n" "${FOO[@]}"
[a]
[b]
[c d]
$ BAR=FOO[@]
$ echo "$BAR"
FOO[@]
$ printf "[%s]\n" "${!BAR}"
[a]
[b]
[c d]
$ BROKEN_ARRAY="${!BAR}"
$ printf "[%s]\n" "${BROKEN_ARRAY[@]}"
[a b c d]
$ PROPER_ARRAY=("${!BAR}")
$ printf "[%s]\n" "${PROPER_ARRAY[@]}"
[a]
[b]
[c d]

(e.g. no need to printf ;%q\n)

On a final note, you also wrote 👆 in your comment but printf %q has nothing to do with IFS.

It's there to change COMPREPLY to big\ bla and big\ blue instead of big bla and big blue:

$ printf "%q\n" "big bla" "big blue"
big\ bla
big\ blue

Whether it's required or not depends entirely on the command and whether big and blue should be treated as a single argument or two separate arguments. e.g. If "big blue" was a name of a file and auto-completion did cat big blue instead of cat big\ blue it wouldn't be very helpful.

COMPREPLY+=( $(compgen -W "${current_action_choices[*]}" -- "${completing_word}") )
fi

return 0
Expand Down