Fix ExecCommandWith for cmd.exe in Windows #1072
Conversation
Close junegunn#1018 Run the command as is in cmd.exe with no parsing and escaping. Explicity set cmd.SysProcAttr so execCommand does not escape the command. Technically, the command should be escaped with ^ for special characters, including ". This allows cmd.exe commands to be chained together. See neovim/neovim#7343 (comment) However, this requires a new shellescape function that is specific to one of the following: - interactive prompt - batchfile - command name fzf#shellescape in the Vim plugin can handle only the batchfile.
janlazo
force-pushed
the
go_windows_ExecCommand
branch
from
921e6c5
to
0d7234e
Compare
|
Thanks. First off, let's make it clear that fzf is a command-line program that can be used with or without Vim (many users of fzf do not even use Vim) and there is no way we can enforce anything on bind/preview expressions users specify. The specifics of Having said that, I have a few comments.
|
|
I mentioned As for your questions,
For reference, I'm trying to avoid case 1, mentioned in |
|
What is "interactive prompt"? EDIT: Nevermind, I get it, you meant |
|
I see what you mean now for the double-quoted string for the placeholder. I need a crude shellescape for the command after all. |
|
Besides |
|
Is |
|
I kept |
I think you're right. Also, it seems that it was a mistake to use According to https://golang.org/pkg/strconv/#Quote
I don't think we want this behavior in this context. |
quoteEntry uses strconv.Quote in Windows but this escapes more than \ and "
Use strings.Replace to handle only \ and ".
- double all backslashes
- double-quote the entry, escaping all inner double quotes with backslash
src/terminal.go
Outdated
| return strconv.Quote(strings.Replace(entry, "\"", "\\\"", -1)) | ||
| escaped := strings.Replace(entry, "\\", "\\\\", -1) | ||
| escaped = "\"" + strings.Replace(escaped, "\"", "\\\"", -1) + "\"" | ||
| r, _ := regexp.Compile("[&|<>()@^%\"]") |
There was a problem hiding this comment.
Let's use raw string literals (using backticks) in these lines to make them more readable.
|
What about parsing Line 270 in f317730 |
|
What about it? I don't see how it's related to the current discussion. |
|
The parser is incompatible with |
|
Anyway, I can escape |
|
If there's no issues with the changes, then I will update the docs for |
Yes, I'd like to leave this as an open issue, since 1. I believe it's rare to have non-trivial options in $FZF_DEFAULT_OPTS, 2. and it's easy to come up with a sh-compatible version of it.
What needs to be mentioned? |
The user has to do the escaping themselves because fzf will run their command as is on cmd.exe, not |
Can you give an example where escaping is needed?
Umm, is |
fzf unsets |
|
Thanks for the comment. But I was also curious how a command would look like after manual escaping. So the commands shown in https://github.com/junegunn/fzf/wiki/Windows#relative-filepaths will have to change?
This is the point I don't understand. |
|
Are we discussing Cygwin environment? |
For cmd.exe only for consistency with this PR (ie. add
No,
For |
By default, no. |
|
Thanks for the clarification and for being patient with me. So for the users running fzf on cmd.exe, there's no confusion, but Cygwin or bash users should be aware that SHELL is not used by fzf and FZF_DEFAULT_COMMAND should be written for cmd.exe. You can briefly mention this on the README page or we can just add a link to the Windows wiki page. |
|
We can update the Windows wiki page and then add a link in the README and Cygwin wiki page to consolidate the explanation for why I don't mind doing more work to clarify the changes introduced. |
|
On second thought, cmd /v:ON /s /c "set /p my_prompt= & fzf --prompt !my_prompt!"Use case is a short for loop on the preview command |
|
We should probably add terminal_test.go and implement test cases for the Windows version of quoteEntry. |
|
Will it run on Travis? I thought |
|
if util.IsWindows() {
return quoteEntryCmd(entry)
}
return ... |
|
Can you add a few test cases? |
|
I can add unescaped commands from https://github.com/junegunn/fzf/wiki/Windows#relative-filepaths, some shell commands I used in neovim/neovim#7343, and basic statements in batchfiles and powershell scripts. It helps if I can isolate these test cases into one file so I can test with package main
import (
"fmt"
"os/exec"
"syscall"
"strings"
"regexp"
)
func main() {
tests := [...]string{
"\"",
"\\",
"\\\"",
"\"\\\\\\\"",
"&|<>()@^%!",
"%USERPROFILE%",
"C:\\Program Files (x86)\\",
}
for _, v := range tests {
escaped := Escape(v)
out, _ := ExecCommandWith("echo " + escaped).Output()
out2, _ := ExecCommandWith("\"echo\" " + escaped).Output()
fmt.Printf("%s\n%s\n%s%s\n", v, escaped, out, out2)
}
}
func Escape(entry string) string {
escaped := strings.Replace(entry, `\`, `\\`, -1)
escaped = `"` + strings.Replace(escaped, `"`, `\"`, -1) + `"`
r, _ := regexp.Compile(`[&|<>()@^%!"]`)
return r.ReplaceAllStringFunc(escaped, func (match string) string {
return "^" + match
})
}
func ExecCommandWith(command string) *exec.Cmd {
cmd := exec.Command("cmd")
cmd.SysProcAttr = &syscall.SysProcAttr{
HideWindow: false,
CmdLine: fmt.Sprintf(` /s /c "%s"`, command),
CreationFlags: 0,
}
return cmd
} |
|
Functional test cases for diff --git a/src/terminal.go b/src/terminal.go
index 1890a2a..98d837c 100644
--- a/src/terminal.go
+++ b/src/terminal.go
@@ -1103,14 +1103,18 @@ func keyMatch(key int, event tui.Event) bool {
event.Type == tui.Mouse && key == tui.DoubleClick && event.MouseEvent.Double
}
+func quoteEntryCmd(entry string) string {
+ escaped := strings.Replace(entry, `\`, `\\`, -1)
+ escaped = `"` + strings.Replace(escaped, `"`, `\"`, -1) + `"`
+ r, _ := regexp.Compile(`[&|<>()@^%!"]`)
+ return r.ReplaceAllStringFunc(escaped, func(match string) string {
+ return "^" + match
+ })
+}
+
func quoteEntry(entry string) string {
if util.IsWindows() {
- escaped := strings.Replace(entry, `\`, `\\`, -1)
- escaped = `"` + strings.Replace(escaped, `"`, `\"`, -1) + `"`
- r, _ := regexp.Compile(`[&|<>()@^%!"]`)
- return r.ReplaceAllStringFunc(escaped, func (match string) string {
- return "^" + match
- })
+ return quoteEntryCmd(entry)
}
return "'" + strings.Replace(entry, "'", "'\\''", -1) + "'"
}
diff --git a/src/terminal_test.go b/src/terminal_test.go
index d42d2b8..60f2b1a 100644
--- a/src/terminal_test.go
+++ b/src/terminal_test.go
@@ -91,3 +91,22 @@ func TestReplacePlaceholder(t *testing.T) {
result = replacePlaceholder("echo {}/{1}/{3}/{2..3}", true, Delimiter{regex: regex}, false, "query", items1)
check("echo ' foo'\\''bar baz'/'f'/'r b'/''\\''bar b'")
}
+
+func TestQuoteEntryCmd(t *testing.T) {
+ tests := map[string]string{
+ `"`: `^"\^"^"`,
+ `\`: `^"\\^"`,
+ `\"`: `^"\\\^"^"`,
+ `"\\\"`: `^"\^"\\\\\\\^"^"`,
+ `&|<>()@^%!`: `^"^&^|^<^>^(^)^@^^^%^!^"`,
+ `%USERPROFILE%`: `^"^%USERPROFILE^%^"`,
+ `C:\Program Files (x86)\`: `^"C:\\Program Files ^(x86^)\\^"`,
+ }
+
+ for input, expected := range tests {
+ escaped := quoteEntryCmd(input)
+ if escaped != expected {
+ t.Errorf("Input: %s, expected: %s, actual %s", input, expected, escaped)
+ }
+ }
+}This is all I need for now. Maybe you could add a few more test cases to the map? I'll leave it up to you. |
Close #1018 Run the command as is in cmd.exe with no parsing and escaping. Explicity set cmd.SysProcAttr so execCommand does not escape the command. Technically, the command should be escaped with ^ for special characters, including ". This allows cmd.exe commands to be chained together. See neovim/neovim#7343 (comment) This commit also updates quoteEntry to use strings.Replace instead of strconv.Quote which escapes more than \ and ".
|
Closed by c4185e8. I added the test cases to your commits and pushed to master. Let me know if there's anything else you want to address. Thanks! |
|
I will send another PR to add more test cases from the wiki to the map. I was busy this week so I didn't have time to include your changes myself so thanks for closing this with your changes. @junegunn can you update the README or wiki for me to mention |
|
No hurries. I already did update the wiki page, so please take a look. https://github.com/junegunn/fzf/wiki/Windows I'll also add a link to the page on the README page. |
|
That's fine. Thanks you. |
Close #1018
Run the command as is in cmd.exe with no parsing and escaping.
Explicity set cmd.SysProcAttr so execCommand does not escape the command.
Technically, the command should be escaped with ^ for special characters, including ".
This allows cmd.exe commands to be chained together.
See neovim/neovim#7343 (comment)
However, this requires a new shellescape function that is specific to one of the following:
fzf#shellescape in the Vim plugin can handle only the batchfile.