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

Jump to bottom

Add similar whitespace escape logic to bash v2 completions than in other completions #1743

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Add similar whitespace escape logic to bash v2 completions than in other completions #1743

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
fbc5c65
Escape bash completion when there is only one possible completion
kangasta Jun 23, 2022
3f2dad7
Use line-break as field separator in compgen words input
kangasta Jun 23, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
15 changes: 11 additions & 4 deletions bash_completionsV2.go
View file
Open in desktop
Expand Up @@ -216,7 +216,7 @@ __%[1]s_handle_completion_types() {
comp=${comp%%%%$tab*}
# Only consider the completions that match
if [[ $comp == "$cur"* ]]; then
COMPREPLY+=("$comp")
COMPREPLY+=( "$(printf %%q "$comp")" )
Copy link

Choose a reason for hiding this comment

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

If I'm reading cobra-completion-testing right, this line is the performance regression. We're creating a subshell to quote each individual completion

We could instead batch all of the completions up into another array, and then quote them all at once kinda like how we're doing for the input to this loop in the first place

fi
done < <(printf "%%s\n" "${completions[@]}")
;;
Expand All @@ -233,7 +233,14 @@ __%[1]s_handle_standard_completion_case() {

# Short circuit to optimize if we don't have descriptions
if [[ "${completions[*]}" != *$tab* ]]; then
IFS=$'\n' read -ra COMPREPLY -d '' < <(compgen -W "${completions[*]}" -- "$cur")
local compgen_words=$(printf "%%s\n" "${completions[@]}")
IFS=$'\n' read -ra COMPREPLY -d '' < <(IFS=$'\n' compgen -W "${compgen_words}" -- "$cur")
Copy link

Choose a reason for hiding this comment

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

If we want to support other bash special characters, we need more quoting:

local compgen_words="$(printf "%%q\n" "${completions[@]}")"
cur="$(printf "%%q" "${cur}")"
IFS=$'\n' read -ra COMPREPLY -d '' < <(IFS=$'\n' compgen -W "${compgen_words}" -- "${cur}")

compgen's -W honors shell quoting, so a completion like foo\>bar would become foo>bar to compgen. We also have to quote ${cur} because compgen appears to honor shell quoting after checking to make sure each word begins with ${cur}

Similarly, we need quoting up above: https://github.com/spf13/cobra/blob/main/bash_completionsV2.go#L62-L84

$ args=( echo "foo\>bar" )
$ eval "${args[*]}"
foo>bar

eval also honors shell quoting, so if we want to pass the actual quoted value through to the __complete call, we need to quote the args in requestComp

$ args=( echo "foo\>bar" )
$ eval "$(printf "%q " "${args[@]}")"
foo\>bar


# If there is a single completion left, escape the completion
if ((${#COMPREPLY[*]} == 1)); then
COMPREPLY[0]=$(printf %%q "${COMPREPLY[0]}")
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we can eliminate a subshell here.

Suggested change
COMPREPLY[0]=$(printf %%q "${COMPREPLY[0]}")
printf -v "COMPREPLY[0]" %%q "${COMPREPLY[0]}"

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, actually printf -v to an array index is a bash 4.1+ thing, so as/if older compatibility is needed, then scratch this and similar comments of mine elsewhere.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah, we try to keep it working with bash 3, as much as possible.

fi

return 0
fi

Expand All @@ -252,12 +259,12 @@ __%[1]s_handle_standard_completion_case() {
fi
done < <(printf "%%s\n" "${completions[@]}")

# If there is a single completion left, remove the description text
# If there is a single completion left, remove the description text and escape the completion
if ((${#COMPREPLY[*]} == 1)); then
__%[1]s_debug "COMPREPLY[0]: ${COMPREPLY[0]}"
comp="${COMPREPLY[0]%%%%$tab*}"
__%[1]s_debug "Removed description from single completion, which is now: ${comp}"
COMPREPLY[0]=$comp
COMPREPLY[0]=$(printf %%q "${comp}")
Copy link
Contributor

Choose a reason for hiding this comment

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

As above

Suggested change
COMPREPLY[0]=$(printf %%q "${comp}")
printf -v "COMPREPLY[0]" %%q "$comp"

else # Format the descriptions
__%[1]s_format_comp_descriptions $longest
fi
Expand Down