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

[kangasta]

trafficstars

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]

All committers have signed the CLA.

[marckhouzam]

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]

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: https://github.com/marckhouzam/cobra-completion-testing/pull/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...

[github-actions[bot]]

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]

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]

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

[github-actions[bot]]

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]

Adding a comment to keep this from being closed.

[marckhouzam]

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]

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 (https://github.com/UpCloudLtd/upcloud-cli/pull/204), but it would be nice to get rid of that.