Fix powershell escaping #2647

rashil2000 · 2021-10-27T07:52:09Z

Fix what was left in #2641

rashil2000 · 2021-10-27T07:57:02Z

stinos · 2021-10-27T09:53:20Z

src/terminal_windows.go

@@ -37,7 +37,7 @@ func quoteEntry(entry string) string {
 			return "^" + match
 		})
 	} else if strings.Contains(shell, "pwsh") || strings.Contains(shell, "powershell") {
-		escaped := strings.Replace(entry, `"`, `""`, -1)
+		escaped := strings.Replace(entry, `"`, `\"`, -1)


Why was this needed (perhaps add a test showing why)? Would it be an option to escape with a backtick instead?

This was actually motivated by your PR (#2438) itself.

I have tried escaping quotes for powershell commands, using backticks in cmd shell. It didn't work for me.

Right. My PR was more a POC actually and the quoting wasn't well-researched nor well-tested. Thoughts for now (I might be missing something though, didn't look into this in detail):

escaping depends on which type of string is used in PS. So echo '\"' yields \" whereas echo "\"" is not a complete command

when you enter echo 'bar"' in PS it's going to show bar" and that is exactly as expected. If I get it correctly this change would mean that if somehow fzf encouters 'bar"' it is going to change that into 'bar\"' (?) but that is really a different string.

escaping depends on which type of string is used in PS. So echo '"' yields " whereas echo """ is not a complete command

Right, this is because \ is not an escape character in PS,

when you enter echo 'bar"' in PS it's going to show bar" and that is exactly as expected. If I get it correctly this change would mean that if somehow fzf encouters 'bar"' it is going to change that into 'bar"' (?) but that is really a different string.

Hmm, yes. Would you recommend removing the escaped := strings.Replace(entry, ", ", -1) statement altogether?

Fzf for windows has two layers of escaping. First layer of escaping is done for shells, i.e. ^ for cmd, "" for powershell, *nix gets by with its single quotes just fine. After the first layer, you are basically done and you could call it a day. However, some applications that are commonly used with fzf require some characters to be escaped with backslash. Hence there is second layer of application specific backslash-escaping, i.e. \". This is usability enhancement.

@stinos Here is a test case for backslash-escaping in cmd:

fzf/src/terminal_test.go

Lines 314 to 315 in edac982

// example of mandatorily escaped double quote in the output, otherwise `rg -- ""C:\\test.txt""` is not matching for the double quotes around the path

{give{`rg -- {}`, ``, newItems(`"C:\test.txt"`)}, want{output: `rg -- ^"\^"C:\\test.txt\^"^"`}},

if somehow fzf encouters 'bar"' it is going to change that into 'bar"' (?) but that is really a different string

Yeah. You would see this output when you run fzf --preview "rg {}" and the {} string of value bar" would get escaped to bar\". Note that the backslash is required for the ripgrep to work as expected.

@vovcacik right, thanks for the explanation, I didn't realize that. It does ring a bell though and probably this is what cause me problems in the past; I think I was trying to use something like Read-History | fzf --bind=enter:execute({}) so like plain Ctrl-R but allowing to execute lines at will but that never really worked out as expected, didn't look into it because IIRC dlv.exe gave up on me somehow.

Anyway if I understand it correctly fzf will never pass the {} for execute/preview as-is then, right? Or is there a placeholder flag or similar which means 'just pass as-is to execute/preview/...' ?

I get that this is for convenience, but it's also a bit counterintuitive; I mean when I'm in PS I know I have to use git grep 'bar\"' (or 'bar"""' or probably some other forms work as well) because that is just how PS executes external commands.

Also ' does not need being doubled then probably?

Anyway if I understand it correctly fzf will never pass the {} for execute/preview as-is then, right?

Nope, it will aggressively escape the {} contents to play it safe. @junegunn usually recommends the use of {f} which will create a temp file (script) that will contain the {} value as-is as you say. The full example would then be Read-History | fzf --bind=enter:execute(cmd /c {f}) (but it is broken at the moment, the file is missing .bat extension).

Also ' does not need being doubled then probably?

~~Not sure about that. But I didn't have any problems with single quotes when writing tests for this PR (rashil2000#1)~~
It is needed, and it works fine in this PR (see c7473f8)

The full example would then be Read-History | fzf --bind=enter:execute(cmd /c {f})

Ok that's useful. It would rather be Read-History | fzf --bind=enter:execute(Get-Content {f} | Invoke-Expression) though, to execute PS history via PS, not cmd.

It is needed

Oh right because the complete command is wrapped in ', missed that.

@stinos, @rashil2000 the current implementation is actually what you guys want. The consistency with ripgrep (from cmd) would require additional level of escaping (as pointed out in #2647 (comment) and vovcacik@5e774bc)

src/util/util_windows.go

vovcacik · 2021-10-27T20:25:30Z

To settle down this PR, I have written tests for it. The tests are not exhaustive, but it will make the PR consistent with how fzf works in cmd console. Maybe we should have write the tests first 😅

stinos · 2021-10-28T08:08:05Z

consistent with how fzf works in cmd console

Probably to late to revert this decision, but my 2 cents again: this is Powershell, not cmd, they're very different so the more I think about it, the more it feels wrong to try and make them consistent. I get that for cmd the quoting gets applied because it's needed like 99% of the time anyway. But that isn't the case in PS: it has a ton of builtin commands and for those the quoting of " is wrong. Sure the tests show that it's going to work with ripgrep, but if you'd be piping to ripgrep you probably wouldn't be using PS but cmd directly just because it's faster. Also cmd is Windows-only, Powershell is not, so this probably introduce differences between how it gets called depending on OS.

Anyway, I feel like this should at least be documented. Have the manual state explicitly that on Windows fzf is always going to insert \ for quotes and also mention the reason why (which is https://docs.microsoft.com/en-us/powershell/module/microsoft.powershell.core/about/about_parsing?view=powershell-7.1#passing-arguments-that-contain-quote-characters). E.g. the tests have a comment like 'rg is not matching for the double quotes' but that is not really accurate: it's just that rg will never even see those quotes in its arguments because the process API strips them.

vovcacik · 2021-10-28T19:26:18Z

@janlazo Do you remember why vim-plug escapes " and \ to \", \\ respectively? Ref: L2203-L2205. I don't use the plugin myself.

janlazo · 2021-10-29T01:11:23Z

From Neovim's :h jobstart,

- {cmd} is collapsed to a string of quoted args as expected
  by CommandLineToArgvW https://msdn.microsoft.com/bb776391
  unless cmd[0] is some form of "cmd.exe".

https://docs.microsoft.com/en-gb/windows/win32/api/shellapi/nf-shellapi-commandlinetoargvw?redirectedfrom=MSDN

CommandLineToArgvW has a special interpretation of backslash characters when they are followed by a quotation mark character ("). This interpretation assumes that any preceding argument is a valid file system path, or else it may behave unpredictably.

This special interpretation controls the "in quotes" mode tracked by the parser. When this mode is off, whitespace terminates the current argument. When on, whitespace is added to the argument like all other characters.

2n backslashes followed by a quotation mark produce n backslashes followed by begin/end quote. This does not become part of the parsed argument, but toggles the "in quotes" mode.
(2n) + 1 backslashes followed by a quotation mark again produce n backslashes followed by a quotation mark literal ("). This does not toggle the "in quotes" mode.
n backslashes not followed by a quotation mark simply produce n backslashes.

vovcacik · 2021-10-29T19:27:22Z

@janlazo Thanks! That explains the implementation of s:shellesc_ps1. ¹ Fzf is not subject to the rules specified in CommandLineToArgvW, so it looks like internal requirement of the plugin. Correct me if I am wrong please.

Is there any scenario that would cause issues when the quoteEntry method wouldn't escape " with backslash. Note that quoteEntry escapes strings only when expanding fzf's placeholders (like {}, {q} etc). There is a gap in my understanding that would connect something like fzf --bind "enter:execute(nvim {}) and the jobstart command from vim/nvim.

I am asking for this because @stinos is proposing to use powershell's native quoting rules and if that would not break any workflows I think @junegunn could actually like it more (the rules are closer to how fzf escapes strings on *nix).

I am comparing the two if branches in L1422 and L1432 ↩

janlazo · 2021-10-31T17:13:09Z

Any Windows program that executes external commands is subject to CommandLineToArgW.
golang does it by default.
golang doesn't special-case cmd.exe so cmdline is non-empty when the shell is cmd.exe.

Command passed to jobstart must be escaped. Editor plugin doesn't have to escape commands that fzf (binary) will execute internally. When executing the expanded command, inner " must be escaped (on Windows).

Backtick escaping seems to specific to powershell. cmd.exe doesn't special-case backticks. It can work for the value passed to -Command. I wouldn't use it when dealing with external programs.

vovcacik · 2021-11-01T21:34:47Z

New test suite added, with docs. It drops one level of escaping in favor of using powershell's native escaping. Good way to see the difference is to compare those two commits: old and new.

junegunn · 2021-11-02T00:38:45Z

@vovcacik Thanks for the test cases!
@rashil2000 Can you merge @vovcacik's PR to your branch? Let's wrap this up.

Parsers included: - go parser (well, this is easily dealt with using `` strings) - win32 (shell-api) parser - powershell parser (for powershell commands) - powershell parsing rules for calling native commands - internal parsers of select regex applications (like grep)

junegunn · 2021-11-02T06:28:06Z

Rebased to tidy up the commits.

junegunn · 2021-11-02T06:56:45Z

Merged, thanks!

rashil2000 mentioned this pull request Oct 27, 2021

Respect SHELL env var on Windows (#2638) #2641

Merged

stinos reviewed Oct 27, 2021

View reviewed changes

src/util/util_windows.go Outdated Show resolved Hide resolved

stinos reviewed Oct 27, 2021

View reviewed changes

src/util/util_windows.go Show resolved Hide resolved

vovcacik mentioned this pull request Oct 27, 2021

Test the Powershell escaping rashil2000/fzf#1

Closed

vovcacik mentioned this pull request Nov 1, 2021

Test the Powershell native escaping rashil2000/fzf#2

Merged

rashil2000 and others added 3 commits November 2, 2021 15:26

Fix powershell escaping

0dc308d

Test escaping of powershell commands on Windows

cbd183e

junegunn force-pushed the pwsh-escaping branch from c5016bf to 121f5c8 Compare November 2, 2021 06:27

junegunn merged commit e0dd2be into junegunn:master Nov 2, 2021

rashil2000 deleted the pwsh-escaping branch November 2, 2021 06:57

junegunn mentioned this pull request Apr 21, 2024

Add --with-shell for shelling out with different command and flags #3746

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix powershell escaping #2647

Fix powershell escaping #2647

rashil2000 commented Oct 27, 2021

rashil2000 commented Oct 27, 2021

stinos Oct 27, 2021

rashil2000 Oct 27, 2021

stinos Oct 27, 2021

rashil2000 Oct 27, 2021

vovcacik Oct 27, 2021 •

edited

stinos Oct 27, 2021

vovcacik Oct 27, 2021 •

edited

stinos Oct 28, 2021

vovcacik Nov 1, 2021

vovcacik commented Oct 27, 2021

stinos commented Oct 28, 2021

vovcacik commented Oct 28, 2021

janlazo commented Oct 29, 2021 •

edited

vovcacik commented Oct 29, 2021

janlazo commented Oct 31, 2021

vovcacik commented Nov 1, 2021

junegunn commented Nov 2, 2021

junegunn commented Nov 2, 2021

junegunn commented Nov 2, 2021

	// example of mandatorily escaped double quote in the output, otherwise `rg -- ""C:\\test.txt""` is not matching for the double quotes around the path
	{give{`rg -- {}`, ``, newItems(`"C:\test.txt"`)}, want{output: `rg -- ^"\^"C:\\test.txt\^"^"`}},

Fix powershell escaping #2647

Fix powershell escaping #2647

Conversation

rashil2000 commented Oct 27, 2021

rashil2000 commented Oct 27, 2021

stinos Oct 27, 2021

Choose a reason for hiding this comment

rashil2000 Oct 27, 2021

Choose a reason for hiding this comment

stinos Oct 27, 2021

Choose a reason for hiding this comment

rashil2000 Oct 27, 2021

Choose a reason for hiding this comment

vovcacik Oct 27, 2021 • edited

Choose a reason for hiding this comment

stinos Oct 27, 2021

Choose a reason for hiding this comment

vovcacik Oct 27, 2021 • edited

Choose a reason for hiding this comment

stinos Oct 28, 2021

Choose a reason for hiding this comment

vovcacik Nov 1, 2021

Choose a reason for hiding this comment

vovcacik commented Oct 27, 2021

stinos commented Oct 28, 2021

vovcacik commented Oct 28, 2021

janlazo commented Oct 29, 2021 • edited

vovcacik commented Oct 29, 2021

Footnotes

janlazo commented Oct 31, 2021

vovcacik commented Nov 1, 2021

junegunn commented Nov 2, 2021

junegunn commented Nov 2, 2021

junegunn commented Nov 2, 2021

vovcacik Oct 27, 2021 •

edited

vovcacik Oct 27, 2021 •

edited

janlazo commented Oct 29, 2021 •

edited