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

JeffFaer · 2024-03-21T04:16:33Z

Special characters include whitepace, so this is more general than
#1743. This should also fix #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.

CLAassistant · 2024-03-21T04:16:38Z

All committers have signed the CLA.

ncw · 2024-09-06T17:17:12Z

It would be nice to have this PR merged - many rclone users have noticed that we can't complete file names with spaces in :-(

marckhouzam · 2024-10-29T14:43:08Z

Sorry for the delay. I will be reviewing this next.

marckhouzam · 2024-10-29T14:44:06Z

@JeffFaer

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

That would be great!

JeffFaer · 2024-10-30T02:41:19Z

marckhouzam/cobra-completion-testing#25

marckhouzam · 2024-11-18T02:58:55Z

I think there may be some confusion here.
When doing completion for special characters, the characters should be automatically escaped.
So if the completion choice is a>b, doing prog [tab][tab] should yield prog a\>b.
You can rebase #1743 and try it out as it behaves like this.
You can also try doing such completion with zsh and see how it should behave.

…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 <[email protected]>

…uoting. Signed-off-by: Jeffrey Faer <[email protected]>

They're not going through compgen so they don't need it. Signed-off-by: Jeffrey Faer <[email protected]>

Signed-off-by: Jeffrey Faer <[email protected]>

JeffFaer · 2024-12-05T04:41:08Z

I've updated these changes to reflect the behavior you're looking for. It automatically escapes the last completion suggestion.

JeffFaer · 2024-12-05T05:35:23Z

Hmm there's actually a bad interaction here with COMP_WORDBREAKS. The default COMP_WORDBREAKS seems to include many special characters by default.

The bash completion script is handling a couple wordbreaks (= and :) "correctly" by removing everything up to and including the wordbreak from proposed COMPREPLYs so that readline doesn't duplicate the same prefix again. That's problematic for special characters that need to be escaped like > because that means the completion script won't be able to escape them anymore...

Signed-off-by: Jeffrey Faer <[email protected]>

JeffFaer · 2024-12-05T05:56:15Z

I took a pass at a way to address the COMP_WORDBREAKS interaction (tl;dr: remove all wordbreaks that would need to be escaped)

JeffFaer · 2024-12-05T06:03:03Z

bash_completionsV2.go

@@ -173,8 +169,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"


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.

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

Signed-off-by: Jeffrey Faer <[email protected]>

The other ones should already have been quoted by the completion script. Signed-off-by: Jeffrey Faer <[email protected]>

marckhouzam · 2024-12-27T19:58:03Z

@JeffFaer Thanks for the update! It looks really good. However, while testing with the updated program of marckhouzam/cobra-completion-testing#25 I'm noticing a subtle behaviour, that I feel is incorrect.

bash-5.2$ ./testprog prefix special-chars bash4>[tab]
bash-5.2$ ./testprog prefix special-chars bash4\>redirect

In the above case, when the user presses [tab] after the redirect symbol > they should get some file completion and not the > being escaped. In the above case, since the user did not escape the > themselves, it means they want to use it as a redirect command, so Cobra should not escape it.

If the user where to escape the > themselves, that is when Cobra should continue the shell completion:

bash-5.2$ ./testprog prefix special-chars bash4\>[tab]
# Below is what I would expect, but this is not how the PR behaves
bash-5.2$ ./testprog prefix special-chars bash4\>redirect

Comparing it to zsh completion helps see the expected behaviour.
So, to summarize, this is what we need:

bash-5.2$ ./testprog prefix special-chars bash4[tab]
bash-5.2$ ./testprog prefix special-chars bash4\>redirect

bash-5.2$ ./testprog prefix special-chars bash4\>[tab]
bash-5.2$ ./testprog prefix special-chars bash4\>redirect

bash-5.2$ ./testprog prefix special-chars bash4>[tab]
# Normal file completion should happen here

The same approach should apply to the other special characters: , |, $, ;.
For #, zsh requires it to be escaped to continue completion (bash5\#<TAB>), but I'm not sure why, but let's do the same.

…ld print them to the command line (COMP_TYPE=9)

JeffFaer · 2024-12-30T21:29:22Z

It sounds like you're looking to have special characters escaped as soon as they end up on the command line instead of after the full word has been completed. I've updated the PR to do that. PTAL. Escaping special characters as soon as they end up on the command line lets us simplify some of the wordbreak complexity, too

JeffFaer · 2024-12-30T21:30:06Z

bash_completionsV2.go

+
+    63)
+        # Type: Listing completions after successive tabs
+        __%[1]s_handle_standard_completion_case


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?

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

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)

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

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

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

marckhouzam · 2025-01-18T04:06:21Z

@JeffFaer I worked many hours trying to make sure 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 #1743. So, I feel the fair thing to do is to go back to #1743 and make it work.

I was able to do sort all this out thanks to the great tests you wrote in marckhouzam/cobra-completion-testing#25, so I want to merge that one also.

I'm going to pause working on this one and see if we can get #1743 working.
Thanks for your continued efforts.

marckhouzam · 2025-01-19T18:34:08Z

bash_completionsV2.go

@@ -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
            [[ -z $comp ]] && continue
            # Strip any description
            comp=${comp%%%%$tab*}
            # Only consider the completions that match
            if [[ $comp == "$cur"* ]]; then


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

marckhouzam · 2025-01-20T14:18:32Z

bash_completionsV2.go

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


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

marckhouzam · 2025-01-20T19:45:06Z

bash_completionsV2.go

+
+    63)
+        # Type: Listing completions after successive tabs
+        __%[1]s_handle_standard_completion_case


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

marckhouzam · 2025-01-21T03:02:39Z

bash_completionsV2.go

+        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//\\#/#}"


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

Also avoid using 'printf -v' by using "read" patterns, inspired by PR spf13#2126 from of "Jeffrey Faer". Signed-off-by: Marc Khouzam <[email protected]> Co-authored-by: Jeffrey Faer <[email protected]>

marckhouzam · 2025-01-21T03:16:52Z

bash_completionsV2.go

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


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

marckhouzam · 2025-01-21T03:20:35Z

bash_completionsV2.go

+        __%[1]s_escape_compreply
+        ;;
+
+    63)


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.

marckhouzam · 2025-01-21T03:21:54Z

bash_completionsV2.go

        ;;

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


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.

marckhouzam · 2025-01-21T03:22:59Z

bash_completionsV2.go

+            # bash3 compgen doesn't handle escaped # symbols correctly.
+            compgen_words="${compgen_words//\\#/#}"
+        fi
+        IFS=$'\n' read -ra COMPREPLY -d '' < <(IFS=$'\n'; compgen -W "${compgen_words}" -- "${cur}")


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.

marckhouzam · 2025-01-21T03:26:00Z

@JeffFaer Thanks again for your hard work. This was very tricky. The version I plan to commit is now posted in #1743 but I made you a co-author because I was able to get things working thanks to different part of your work as well.

If you can give a spin to #1743 and see if you are happy with it, it would help me make sure I didn't forget something you might have noticed during your hard work. Thanks!

I will close this PR in favor of #1743

ncw mentioned this pull request Apr 3, 2024

rclone copy creating empty directories it didn't create before rclone/rclone#7689

Closed

ncw mentioned this pull request Sep 6, 2024

completion of remote paths fails on spaces rclone/rclone#8056

Open

JeffFaer mentioned this pull request Oct 30, 2024

test(bash): Add a bunch of tests for bash special characters. marckhouzam/cobra-completion-testing#25

Merged

JeffFaer added 4 commits December 4, 2024 21:39

fix: We don't need an extra empty character when we're doing proper q…

497d36c

…uoting. Signed-off-by: Jeffrey Faer <[email protected]>

Don't quote different completion types.

76d900b

They're not going through compgen so they don't need it. Signed-off-by: Jeffrey Faer <[email protected]>

address review comments

9771cb0

Signed-off-by: Jeffrey Faer <[email protected]>

JeffFaer force-pushed the quote_bash_completions branch from ec43e79 to 9771cb0 Compare December 5, 2024 04:39

Handle wordbreaks better.

718f1d8

Signed-off-by: Jeffrey Faer <[email protected]>

JeffFaer commented Dec 5, 2024

View reviewed changes

JeffFaer added 2 commits December 4, 2024 23:04

remove line

df206d9

Signed-off-by: Jeffrey Faer <[email protected]>

Only quote the last arg.

7a9725b

The other ones should already have been quoted by the completion script. Signed-off-by: Jeffrey Faer <[email protected]>

JeffFaer force-pushed the quote_bash_completions branch from 398ef9f to 7a9725b Compare December 6, 2024 05:03

Automatically escape all special characters as soon as completion wou…

2fc6dad

…ld print them to the command line (COMP_TYPE=9)

JeffFaer commented Dec 30, 2024

View reviewed changes

marckhouzam mentioned this pull request Jan 18, 2025

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

Merged

marckhouzam reviewed Jan 21, 2025

View reviewed changes

marckhouzam closed this Jan 21, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

JeffFaer commented Mar 21, 2024

CLAassistant commented Mar 21, 2024 •

edited

Loading

ncw commented Sep 6, 2024

marckhouzam commented Oct 29, 2024

marckhouzam commented Oct 29, 2024

JeffFaer commented Oct 30, 2024

marckhouzam commented Nov 18, 2024

JeffFaer commented Dec 5, 2024

JeffFaer commented Dec 5, 2024

JeffFaer commented Dec 5, 2024

JeffFaer Dec 5, 2024

marckhouzam Jan 21, 2025

marckhouzam commented Dec 27, 2024

JeffFaer commented Dec 30, 2024

JeffFaer Dec 30, 2024

marckhouzam Dec 30, 2024

JeffFaer Dec 30, 2024

marckhouzam Jan 18, 2025

marckhouzam Jan 18, 2025

marckhouzam Jan 20, 2025

marckhouzam commented Jan 18, 2025 •

edited

Loading

marckhouzam Jan 19, 2025

marckhouzam Jan 20, 2025

marckhouzam Jan 20, 2025

marckhouzam Jan 21, 2025

marckhouzam Jan 21, 2025

marckhouzam Jan 21, 2025

marckhouzam Jan 21, 2025

marckhouzam Jan 21, 2025

marckhouzam commented Jan 21, 2025

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

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

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

Conversation

JeffFaer commented Mar 21, 2024

CLAassistant commented Mar 21, 2024 • edited Loading

ncw commented Sep 6, 2024

marckhouzam commented Oct 29, 2024

marckhouzam commented Oct 29, 2024

JeffFaer commented Oct 30, 2024

marckhouzam commented Nov 18, 2024

JeffFaer commented Dec 5, 2024

JeffFaer commented Dec 5, 2024

JeffFaer commented Dec 5, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marckhouzam commented Dec 27, 2024

JeffFaer commented Dec 30, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marckhouzam commented Jan 18, 2025 • edited Loading

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

marckhouzam commented Jan 21, 2025

CLAassistant commented Mar 21, 2024 •

edited

Loading

marckhouzam commented Jan 18, 2025 •

edited

Loading