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[*]}"

It was important to escape all completions, not only if there was only one and it seems compgen does not persist escaped sequences, so we would have to use printf "%q" for each completions. This makes this "short circuit" if statement just as slow as when we process descriptions.

Therefore, to avoid having to maintain two code paths, I removed the "short circuit" if block.

bash_completionsV2.go

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.

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

I agree with the first part of this comment, but in the end I have removed that if block altogether.

As for the second part of the comment @JeffFaer , I think you wrote it before we discussed that the user will be typing the escape characters as well. So I don't think it applies.

…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

For the moment, printf -v seems to work and is fast.

…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

…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 Signed-off-by: Jeffrey Faer <jeffrey.faer@gmail.com>

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>

Signed-off-by: Marc Khouzam <marc.khouzam@gmail.com>

marckhouzam

I worked very hard trying to make sure #2126 this was safe to merge, and in the end I felt there were too many changes I didn't understand well enough so I tried to simplify the change as much as I could. By the time I was done and had something working, it looked a lot like this PR here! So, I felt like the fair thing to do was to come back to this PR and fix it to make it work.

Working with the new tests of marckhouzam/cobra-completion-testing#25, I was able to get this PR working as I feel we needed.

And with the use of printf -v mentioned by @scop , the performance is much better. We just need to discuss if we need to protect ourselves against printf -v not working in all shell versions.

I have pushed a new commit on top of yours; and all together I feel we got a solution.

marckhouzam · 2025-01-18T03:38:33Z

bash_completionsV2.go

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


It was important to escape all completions, not only if there was only one and it seems compgen does not persist escaped sequences, so we would have to use printf "%q" for each completions. This makes this "short circuit" if statement just as slow as when we process descriptions.

Therefore, to avoid having to maintain two code paths, I removed the "short circuit" if block.

bash_completionsV2.go

marckhouzam · 2025-01-18T03:46:12Z

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


For the moment, printf -v seems to work and is fast.

marckhouzam · 2025-01-18T03:55:17Z

bash_completionsV2.go

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


It turns out that doing the escaping only when there is a single completion is not sufficient.

# Say I have a completion that is "bash1 space" # The below works $ testprog prefix special-chars bash1[tab] $ testprog prefix special-chars bash1\ space # But this does not $ testprog prefix special-chars bash1\ s[tab]

This is because further above we do [[ $comp == "$cur"* ]] || continue but since $cur == "bash1\ s (that is what the user has typed), it won't match $comp because we haven't escaped $comp.
The solution is that we need to escape every completion before we start filtering on $cur

marckhouzam · 2025-01-18T03:59:59Z

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


I agree with the first part of this comment, but in the end I have removed that if block altogether.

As for the second part of the comment @JeffFaer , I think you wrote it before we discussed that the user will be typing the escape characters as well. So I don't think it applies.

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

bash_completionsV2.go Outdated Show resolved Hide resolved

bash_completionsV2.go Outdated Show resolved Hide resolved

bash_completionsV2.go Outdated Show resolved Hide resolved

bash_completionsV2.go Outdated Show resolved Hide resolved

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 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

kangasta and others added 3 commits January 17, 2025 23:22

Escape bash completion when there is only one possible completion

190ea5d

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

74a2762

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

Must do escaping earlier

3fb6896

Signed-off-by: Marc Khouzam <marc.khouzam@gmail.com>

marckhouzam force-pushed the fix/bash-v2-completion-with-whitespace branch from 3f2dad7 to 3fb6896 Compare January 18, 2025 04:22

marckhouzam reviewed Jan 18, 2025

View reviewed changes

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

marckhouzam Jan 18, 2025

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

marckhouzam Jan 18, 2025

JeffFaer Mar 21, 2024

marckhouzam Jan 18, 2025

marckhouzam left a comment

marckhouzam Jan 18, 2025

marckhouzam Jan 18, 2025

marckhouzam Jan 18, 2025

marckhouzam Jan 18, 2025

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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marckhouzam left a 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

CLAassistant commented Jun 23, 2022 •

edited

Loading