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

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

Conversation

JeffFaer
Copy link

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

CLAassistant commented Mar 21, 2024

CLA assistant check
All committers have signed the CLA.

@ncw
Copy link
Contributor

ncw commented Sep 6, 2024

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
Copy link
Collaborator

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

@marckhouzam
Copy link
Collaborator

@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
Copy link
Author

@marckhouzam
Copy link
Collaborator

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 <jeffrey.faer@gmail.com>
…uoting.

Signed-off-by: Jeffrey Faer <jeffrey.faer@gmail.com>
They're not going through compgen so they don't need it.

Signed-off-by: Jeffrey Faer <jeffrey.faer@gmail.com>
Signed-off-by: Jeffrey Faer <jeffrey.faer@gmail.com>
@JeffFaer JeffFaer force-pushed the quote_bash_completions branch from ec43e79 to 9771cb0 Compare December 5, 2024 04:39
@JeffFaer
Copy link
Author

JeffFaer commented Dec 5, 2024

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

@JeffFaer
Copy link
Author

JeffFaer commented Dec 5, 2024

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 <jeffrey.faer@gmail.com>
@JeffFaer
Copy link
Author

JeffFaer commented Dec 5, 2024

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

Signed-off-by: Jeffrey Faer <jeffrey.faer@gmail.com>
The other ones should already have been quoted by the completion script.

Signed-off-by: Jeffrey Faer <jeffrey.faer@gmail.com>
@JeffFaer JeffFaer force-pushed the quote_bash_completions branch from 398ef9f to 7a9725b Compare December 6, 2024 05:03
@marckhouzam
Copy link
Collaborator

@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
Copy link
Author

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


63)
# Type: Listing completions after successive tabs
__%[1]s_handle_standard_completion_case
Copy link
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
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)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Shell completions with whitespace seem to work differently on bash vs. other supported shells
4 participants