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

kangasta · 2022-06-23T21:48:02Z

This PR would escape bash v2 completions when there are only one completion left and use line-break to separate completions when passing them to compgen. This should fix issues described in #1740 and only affect completions with special characters, such as whitespace.

CLAassistant · 2022-06-23T21:48:14Z

All committers have signed the CLA.

marckhouzam · 2022-06-27T05:22:02Z

Thanks @kangasta for the contribution 🎉
I will need to take the time to give this proper attention but I will not have time for the next couple of weeks. I'll get back to this as soon as I can. Thanks for your patience.

marckhouzam · 2022-08-15T02:12:39Z

Ok, I've started looking into this and I started by running some tests and the PR seems to work as expected.

Next step was to add a test to the bash v2 testing of cobra-completion-testing: marckhouzam/cobra-completion-testing#22. So now we can see that if a single completion has a space, the space character is not escaped. Running the same test with the PR shows that the space character is escaped. I think I have to add a couple more tests to cover each possible scenario, so I will keep working on it.

One issue is that the changes cause a slow down in the completion logic. I'm a little surprised because the changes are isolated to when there is a single completion left, so I'll need to dig into it.

More to come...

kangasta · 2022-08-16T09:26:18Z

bash_completionsV2.go

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


Could this cause the performance loss?

This also feels bit clumsy to me, but I was not able to get completions with whitespace to be separate indices of COMPREPLY without printf here.

It's almost certainly here. One starting point for debugging would be to just with strategically placed times, e.g. time compgen_words=....

printf -v "arr[index]" ... in a loop would avoid a subshell, but the net result could be even slower as this doesn't create subshells in a loop.

I don't think we need printf at all. Wouldn't something like this work?

IFS=$'\n' read -ra COMPREPLY -d '' < <(IFS=$'\n'; compgen -W "${completions[*]}" -- "$cur")

IFS=$'\n' compgen ... sets IFS for just the compgen call.

IFS=$'\n'; compgen ... sets IFS for both the compgen call and the "${completions[*]}" string

And since that change is in a subshell, it won't affect IFS for the rest of the script

You can see what this does with this simpler example:

$ arr=( 1 2 3 ) $ IFS=$'\n' echo "${arr[*]}" $ IFS=$'\n'; echo "${arr[*]}"

scop · 2022-08-20T09:44:50Z

bash_completionsV2.go

@@ -220,7 +220,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")
+        compgen_words=$(printf "%%s\n" "${completions[@]}")


compgen_words should be made local.

scop · 2022-08-20T09:45:52Z

bash_completionsV2.go

+        IFS=$'\n' read -ra COMPREPLY -d '' < <(IFS=$'\n' compgen -W "${compgen_words}" -- "$cur")
+
+        # If there is a single completion left, escape the completion
+        if [ ${#COMPREPLY[*]} -eq 1 ]; then


Style, see PR #1702

Suggested change

if [ ${#COMPREPLY[*]} -eq 1 ]; then

if ((${#COMPREPLY[*]} == 1)); then

scop · 2022-08-20T09:48:05Z

bash_completionsV2.go

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


I think we can eliminate a subshell here.

Suggested change

COMPREPLY[0]=$(printf %%q "${COMPREPLY[0]}")

printf -v "COMPREPLY[0]" %%q "${COMPREPLY[0]}"

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.

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

scop · 2022-08-20T09:48:46Z

bash_completionsV2.go

    if [ ${#COMPREPLY[*]} -eq 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}")


As above

Suggested change

COMPREPLY[0]=$(printf %%q "${comp}")

printf -v "COMPREPLY[0]" %%q "$comp"

github-actions · 2022-10-25T00:16:10Z

The Cobra project currently lacks enough contributors to adequately respond to all PRs. This bot triages issues and PRs according to the following rules:

After 60d of inactivity, lifecycle/stale is applied. - After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied and the PR is closed.
You can:
Make a comment to remove the stale label and show your support. The 60 days reset. - If a PR has lifecycle/rotten and is closed, comment and ask maintainers if they'd be interseted in reopening.

This ensures completions with whitespace are completed as a single argument. Signed-off-by: Toni Kangas <toni.kangas@upcloud.com>

This ensures that completions with whitespace are treated as single completion. Signed-off-by: Toni Kangas <toni.kangas@upcloud.com>

kangasta · 2022-10-27T10:58:59Z

Rebased this on top of main

@marckhouzam how big of a performance loss this caused? One option to get this forward could be to split the fix so that optimization for completions without \t, from where the performance loss likely comes, would be addresses in separate PR.

Or is there something I could help with on the testing side?

marckhouzam · 2022-10-27T11:06:16Z

I'll try to get back to this PR next week. Thanks for your patience.

github-actions · 2022-12-28T00:08:18Z

The Cobra project currently lacks enough contributors to adequately respond to all PRs. This bot triages issues and PRs according to the following rules:

After 60d of inactivity, lifecycle/stale is applied. - After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied and the PR is closed.
You can:
Make a comment to remove the stale label and show your support. The 60 days reset. - If a PR has lifecycle/rotten and is closed, comment and ask maintainers if they'd be interested in reopening.

kangasta · 2022-12-29T11:56:41Z

Adding a comment to keep this from being closed.

marckhouzam · 2023-10-30T14:51:24Z

Wow, I completely lost track of this. Sorry @kangasta.
The slow down is about 3 to 4 times slower.

I just ran make bash in https://github.com/marckhouzam/cobra-completion-testing

kangasta · 2024-02-06T14:55:59Z

Yeah, I have not much ideas on how to optimize this further. There were comments from @scop about unnecessary subshells, but those would require dropping support for older bash versions.

We have a hack in upctl to bypass this issue that replaces spaces with non-breaking spaces (UpCloudLtd/upcloud-cli#204), but it would be nice to get rid of that.

JeffFaer · 2024-02-28T16:21:43Z

bash_completionsV2.go

@@ -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")


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

…haracters. Special characters include whitepace, so this is more general than spf13#1743. I added some test cases to cobra-completion-testing. This PR makes them pass. It also doesn't trip any of the performance regression tests.

JeffFaer · 2024-03-21T04:02:09Z

bash_completionsV2.go

@@ -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")" )


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

…haracters. Special characters include whitepace, so this is more general than spf13#1743. I added some test cases to cobra-completion-testing. This PR makes them pass. It also doesn't trip any of the performance regression tests. I'm happy to submit those tests as a PR as well. - https://github.com/JeffFaer/cobra-completion-testing/tree/special_characters - JeffFaer/cobra-completion-testing@52254c1

…haracters. Special characters include whitepace, so this is more general than spf13#1743. This should also fix spf13#1740. I added some test cases to cobra-completion-testing. This PR makes them pass. It also doesn't trip any of the performance regression tests. I'm happy to submit those tests as a PR as well. - https://github.com/JeffFaer/cobra-completion-testing/tree/special_characters - JeffFaer/cobra-completion-testing@52254c1

github-actions bot added the size/S Denotes a PR that chanes 10-24 lines label Jun 23, 2022

marckhouzam added the area/shell-completion All shell completions label Aug 14, 2022

marckhouzam mentioned this pull request Aug 15, 2022

Test that completions can contain a space for bash marckhouzam/cobra-completion-testing#22

Open

kangasta commented Aug 16, 2022

View reviewed changes

scop mentioned this pull request Aug 20, 2022

Shell completions with whitespace seem to work differently on bash vs. other supported shells #1740

Open

scop suggested changes Aug 20, 2022

View reviewed changes

kangasta force-pushed the fix/bash-v2-completion-with-whitespace branch from ac82162 to 5f27234 Compare August 25, 2022 14:23

github-actions bot added the lifecycle/stale label Oct 25, 2022

kangasta added 2 commits October 27, 2022 13:54

Escape bash completion when there is only one possible completion

fbc5c65

This ensures completions with whitespace are completed as a single argument. Signed-off-by: Toni Kangas <toni.kangas@upcloud.com>

Use line-break as field separator in compgen words input

3f2dad7

This ensures that completions with whitespace are treated as single completion. Signed-off-by: Toni Kangas <toni.kangas@upcloud.com>

kangasta force-pushed the fix/bash-v2-completion-with-whitespace branch from 5f27234 to 3f2dad7 Compare October 27, 2022 10:54

github-actions bot removed the area/shell-completion All shell completions label Oct 27, 2022

github-actions bot removed the lifecycle/stale label Oct 28, 2022

github-actions bot added the lifecycle/stale label Dec 28, 2022

github-actions bot removed the lifecycle/stale label Dec 30, 2022

marckhouzam added this to the 1.8.0 milestone Apr 5, 2023

maxlandon mentioned this pull request Aug 19, 2023

Proposal: Freeze the completion API #2019

Open

marckhouzam modified the milestones: 1.8.0, 1.9.0 Nov 15, 2023

JeffFaer reviewed Feb 28, 2024

View reviewed changes

JeffFaer reviewed Mar 21, 2024

View reviewed changes

JeffFaer mentioned this pull request Mar 21, 2024

fix(bash): Fix bash completion for suggestions that contain special characters. #2126

Open

marckhouzam removed this from the 1.8.1 milestone Nov 4, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

kangasta commented Jun 23, 2022

CLAassistant commented Jun 23, 2022 •

edited

Loading

marckhouzam commented Jun 27, 2022

marckhouzam commented Aug 15, 2022

kangasta Aug 16, 2022

scop Aug 20, 2022

JeffFaer Feb 28, 2024

scop Aug 20, 2022

kangasta Aug 25, 2022

scop Aug 20, 2022

kangasta Aug 25, 2022

scop Aug 20, 2022

scop Aug 20, 2022

marckhouzam Aug 20, 2022

scop Aug 20, 2022

github-actions bot commented Oct 25, 2022

kangasta commented Oct 27, 2022

marckhouzam commented Oct 27, 2022

github-actions bot commented Dec 28, 2022

kangasta commented Dec 29, 2022

marckhouzam commented Oct 30, 2023

kangasta commented Feb 6, 2024

JeffFaer Feb 28, 2024

JeffFaer Mar 21, 2024

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

	if [ ${#COMPREPLY[*]} -eq 1 ]; then
	if ((${#COMPREPLY[*]} == 1)); then

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

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

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

Are you sure you want to change the base?

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

Conversation

kangasta commented Jun 23, 2022

CLAassistant commented Jun 23, 2022 • edited Loading

marckhouzam commented Jun 27, 2022

marckhouzam commented Aug 15, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Oct 25, 2022

kangasta commented Oct 27, 2022

marckhouzam commented Oct 27, 2022

github-actions bot commented Dec 28, 2022

kangasta commented Dec 29, 2022

marckhouzam commented Oct 30, 2023

kangasta commented Feb 6, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CLAassistant commented Jun 23, 2022 •

edited

Loading