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.

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(bash): Fix bash completion for suggestions that contain special characters. #2126

Closed
wants to merge 8 commits into from
71 changes: 57 additions & 14 deletions bash_completionsV2.go
Original file line number Diff line number Diff line change
Expand Up @@ -173,8 +173,9 @@ __%[1]s_process_completion_results() {
__%[1]s_handle_completion_types
fi

__%[1]s_handle_special_char "$cur" :
__%[1]s_handle_special_char "$cur" =
__%[1]s_handle_wordbreaks "$cur"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

At surface value, this line looked wrong to me because I didn't think it was correct to handle wordbreaks like this when the user requested menu-completion. But after some manual testing I'm pleasantly surprised to find that this is correct, and menu-completion does still expect the same behavior as "regular" completion.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I didn't understand why this change was needed so for safety, I did not take it.


__%[1]s_debug "The final COMPREPLY: $(printf "%%s\n" "${COMPREPLY[@]}")"

# Print the activeHelp statements before we finish
if ((${#activeHelp[*]} != 0)); then
Expand Down Expand Up @@ -224,20 +225,28 @@ __%[1]s_handle_completion_types() {
# completions at once on the command-line we must remove the descriptions.
# https://github.com/spf13/cobra/issues/1508
local tab=$'\t' comp
while IFS='' read -r comp; do
for comp in "${completions[@]}"; do
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not sure why this was needed so I didn't take it.

[[ -z $comp ]] && continue
# Strip any description
comp=${comp%%%%$tab*}
# Only consider the completions that match
if [[ $comp == "$cur"* ]]; then
Copy link
Collaborator

Choose a reason for hiding this comment

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

We need to do the escaping before we filter on "$cur" so that when the user typed bash1\ s it still matches the (escaped) completion

COMPREPLY+=("$comp")
fi
done < <(printf "%%s\n" "${completions[@]}")
fi
done

__%[1]s_escape_compreply
;;

63)
Copy link
Collaborator

Choose a reason for hiding this comment

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

This was elegant. However, because it was still required to escape characters for COMP_TYPE==9 so that we could filter completion on what the user typed, I went with a different approach.

To avoid showing escaped characters in the menu list, I didn't escape the completions when there were more than one. We want to insert in the command-line the escape characters, and since we only insert in the command-line when there is a single completion (except for COMP_TYPE 37 or 42), that seemed like a good way to know when to escape or not.

# Type: Listing completions after successive tabs
__%[1]s_handle_standard_completion_case
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not escaping here so that the list of results on multiple tabs doesn't include any extra escape characters...is that the behavior you're expecting?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Could you give an example of the behaviour you are referring too?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

testprog prefix special-chars bash<tab><tab>

The list of options it reports is not escaped (i.e. bash4>redirect), but if you were to do bash4<tab> you'd get the escaped version (bash4\>redirect)

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we should escape it in the list. I will do that in #1743

Copy link
Collaborator

Choose a reason for hiding this comment

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

Hmm, I notice that zsh and fish don't show the escaping in their list. I'll think about it...

Copy link
Collaborator

Choose a reason for hiding this comment

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

I agree with you that we should not escape the completions when printing them as a menu

;;

*)
# Type: complete (normal completion)
__%[1]s_handle_standard_completion_case
__%[1]s_escape_compreply
Copy link
Collaborator

Choose a reason for hiding this comment

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

I liked the elegance of trying to escape the characters at the end. But it turned out we also needed to escape them from within __%[1]s_handle_standard_completion_case, so I decided to do everything from within that function.

;;
esac
}
Expand All @@ -247,7 +256,13 @@ __%[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")
# compgen's -W option respects shell quoting, so we need to escape.
local compgen_words="$(__%[1]s_escape_suggestions "${completions[@]}")"
if [[ "${BASH_VERSION}" = 3* ]]; then
# bash3 compgen doesn't handle escaped # symbols correctly.
compgen_words="${compgen_words//\\#/#}"
Copy link
Collaborator

Choose a reason for hiding this comment

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

In the end I decided not to escape the # since the shell passes to the go program without problems.

fi
IFS=$'\n' read -ra COMPREPLY -d '' < <(IFS=$'\n'; compgen -W "${compgen_words}" -- "${cur}")
Copy link
Collaborator

Choose a reason for hiding this comment

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

I ended-up using the logic you wrote here (except for the BASH 3 stuff).
I gave you credit in the commit and put you as a co-author.

return 0
fi

Expand All @@ -271,21 +286,49 @@ __%[1]s_handle_standard_completion_case() {
__%[1]s_debug "COMPREPLY[0]: ${COMPREPLY[0]}"
Copy link
Collaborator

Choose a reason for hiding this comment

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

In the loop above, we need to do the escaping before we filter on "$cur" so that when the user typed bash1\ s it still matches the (escaped) completion

comp="${COMPREPLY[0]%%%%$tab*}"
__%[1]s_debug "Removed description from single completion, which is now: ${comp}"
COMPREPLY[0]=$comp
COMPREPLY[0]="${comp}"
else # Format the descriptions
__%[1]s_format_comp_descriptions $longest
fi
}

__%[1]s_handle_special_char()
__%[1]s_escape_compreply() {
IFS=$'\n' read -ra COMPREPLY -d '' < <(__%[1]s_escape_suggestions "${COMPREPLY[@]}")
}

__%[1]s_escape_suggestions() {
if (( $# == 0 )); then
return
fi
local suggestions=( "$@" )

IFS=$'\n' read -ra suggestions -d '' < <(printf "%%q\n" "${suggestions[@]}")

# Additionally escape # symbols.
local suggestion
for suggestion in "${suggestions[@]}"; do
echo "${suggestion//#/\\#}"
done
}

__%[1]s_handle_wordbreaks()
{
if ((${#COMPREPLY[@]} == 0)); then
return
fi

local comp="$1"
local char=$2
if [[ "$comp" == *${char}* && "$COMP_WORDBREAKS" == *${char}* ]]; then
local word=${comp%%"${comp##*${char}}"}
local idx=${#COMPREPLY[*]}
while ((--idx >= 0)); do
COMPREPLY[idx]=${COMPREPLY[idx]#"$word"}
local i prefix
for ((i=0; i < ${#comp}; i++)); do
local char="${comp:$i:1}"
if [[ "${COMP_WORDBREAKS}" == *"${char}"* ]]; then
prefix="${comp::$i+1}"
fi
done

if [[ -n "${prefix}" ]]; then
for ((i=0; i < ${#COMPREPLY[@]}; i++)); do
COMPREPLY[i]=${COMPREPLY[i]#$prefix}
done
fi
}
Expand Down
Loading