fix escaping for terminal completions for zsh/bash/fish #251989
Conversation
|
Working on pwsh now |
meganrogge
changed the title
|
Just realized maybe we want to handle escaping where this gets inserted instead of changing the label? Yes, going with that instead |
|
something weird is happening w pwsh . It gets escaped properly and sent to the terminal, but then the inserted text is wrong 
|
|
pwsh seems involved, skipping for now |
meganrogge
changed the title
src/vs/workbench/contrib/terminalContrib/suggest/browser/terminalCompletionService.ts
Outdated
Show resolved
Hide resolved
| export function escapeTerminalCompletionLabel(label: string, shellType: TerminalShellType | undefined, pathSeparator: string): string { | ||
| // Only escape for bash/zsh/fish; PowerShell and cmd have different rules | ||
| if (!shellType || shellType === WindowsShellType.CommandPrompt || shellType === GeneralShellType.PowerShell) { | ||
| return label; | ||
| } | ||
|
|
||
| // Bash/zsh/fish escaping | ||
| const specialCharsSet = new Set(['[', ']', '(', ')', '{', '}', `'`, '"', '\\', '$', '`', ';', '&', '|', '<', '>', '*', '?', '~', '#', '=', '%', '!']); | ||
| const specialCharsRegex = /[\[\]\(\)\{\}'"\\$\`;&|<>*?~#=%!]/; | ||
| if (!specialCharsRegex.test(label)) { | ||
| return label; | ||
| } | ||
| return label.split('').map(c => specialCharsSet.has(c) ? '\\' + c : c).join(''); | ||
| } |
There was a problem hiding this comment.
I don't think we can do this generally, these characters may be perfectly valid in the context and this would likely introduce a host of other problems.
The safest approach here is to escape on the completion item in the provider, so escape for the git branch generator and for paths when we know it's safe to do. We also want the label to represent exactly what will be typed and this changes that.
There was a problem hiding this comment.
ok, re
We also want the label to represent exactly what will be typed and this changes that.
That's how I initially implemented it. Then changed the approach because it looked a bit ugly
There was a problem hiding this comment.
we resolve the paths in core, not the extension, so cannot do this for just git specifically. What is a case when this is not safe or desirable?
There was a problem hiding this comment.
For git it would need to be in the post processing of the generator:
meganrogge
force-pushed
the
scornful-hoverfly
branch
from
6d9c5e5 to
6cd435c
Compare
| return label; | ||
| } | ||
|
|
||
| const specialCharsSet = new Set(['[', ']', '(', ')', '{', '}', `'`, '"', '\\', '$', '`', ';', '&', '|', '<', '>', '*', '?', '~', '#', '=', '%', '!']); |
There was a problem hiding this comment.
We should verify that all of these are actually needed here and have tests for each case. For example in bash I don't think we should use the escape for = since it works without it:
Starting with less that we know is wrong and adding over time would be the better approach than trying to cover everything and maybe introducing problems.
100% agree that it's more ugly with the escapes, so we should only use them when needed.
fixes #249024