[RFC] win: defaults: 'shellcmdflag', 'shellxquote' #7343

janlazo · 2017-10-01T01:49:42Z

Wrapping a command in double quotes allows cmd.exe to safely dequote
the entire command as if the user entered the entire command in an interactive prompt.
This reduces the need to escape nested and uneven double quotes.

set shellcmdflag=/s\ /c

makes this behaviour consistent.

Use case for this using cygwin's echo.exe to compare with cmd.exe's internal echo command.
Without this patch, cmd.exe cannot use echo.exe even if it is wrapped in double quotes.

Example:

:: internal echo
> cmd /s /c " echo foo\:bar" "
foo\:bar"

:: cygwin echo.exe
> cmd /s /c " "echo" foo\:bar" "
foo:bar

janlazo · 2017-10-01T02:45:58Z

All test failures are because of set shellxquote=\"

janlazo · 2017-10-01T03:08:37Z

It's not chainable (ie cmd /s /c "command_A" && cmd /s /c "command_B").
sxe is required to escape double quotes correctly.

> cmd /s /c ^"^"echo^" foo\:bar^"^" && cmd /s /c ^"echo foo\:bar^"^"
foo:bar
foo\:bar"

justinmk · 2017-10-01T21:10:12Z

If the tests pass we can try this out...

janlazo · 2017-10-02T01:46:24Z

I don't know why the QuickBuild keeps failing. I'm experienced only with Travis and Appveyor so I don't know what to do here.

janlazo · 2017-10-02T05:00:06Z

The gf test wasn't updated for this breaking change https://github.com/neovim/neovim/wiki/Following-HEAD#20170821 to start terminal in normal mode.

janlazo · 2017-10-02T07:24:45Z

I squashed test commits because the appveyor build passed. First time with Lua so any feedback is appreciated.

The suggested defaults are for handling basic commands only. Need more test cases for special characters in cmd.exe, especially ^ and \. I mentioned setting shellxquote but I think it should be noop and shellescape should handle this instead instead of every Vim plugin creating their own shellescape() for Windows support. I prefer that it behaves similar to fzf#shellescape so it is associated with shell only and that shellslash shouldn't affect shellescape.

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: - interactive prompt - batchfile - command name fzf#shellescape in the Vim plugin can handle only the batchfile.

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.

justinmk · 2017-10-04T07:32:37Z

test/functional/terminal/ex_terminal_spec.lua

@@ -105,9 +105,15 @@ describe(':terminal (with fake shell)', function()

  it('executes a given command through the shell', function()
    terminal_with_fake_shell('echo hi')
-    screen:expect([[
+    local escaped = [[


Instead of this, can we force the old shellxquote for these tests? Because it's not important for these tests to test the default value of shellxquote. Then we avoid lots of churn and also makes it easier to rollback if the new default doesn't work out for some reason.

Yes, because the affected test commands do not require any non-trivial escaping and have 0-1 double quotes. The new shellxquote is for commands with 2+ double quotes.

justinmk · 2017-10-04T07:36:18Z

test/functional/ex_cmds/bang_filter_spec.lua

@@ -4,8 +4,6 @@ local helpers = require('test.functional.helpers')(after_each)
 local feed, command, clear = helpers.feed, helpers.command, helpers.clear
 local mkdir, write_file, rmdir = helpers.mkdir, helpers.write_file, helpers.rmdir

-if helpers.pending_win32(pending) then return end


janlazo · 2017-10-04T15:13:21Z

@justinmk Do you want a separate PR for the tests to handle the old shellxquote value to compare with the new default? I will enable more shell-related tests either way.

justinmk · 2017-10-04T15:20:33Z

@janlazo Not really, maybe I'm confused by #7343 (comment)

Can't we do command('set ...') to force the old value, to avoid the drastic (and unncessary) changes to those tests?

janlazo · 2017-10-04T16:05:01Z

The output of cmd.exe /? mentions 2 cases.

Case 1 is for commands using up to 2 double quotes. These double quotes are useful for escaping a path with special characters mentioned at the bottom of cmd.exe /?. If the users wants to reserve the double quotes to escape something else, they can use DOS shortnames for absolute paths.
However, cmd.exe will dequote as usual on the first double quote it gets, finds the last double quote before a redirection (haven't tested for pipe yet), and pass everything between these 2 double quotes as a single token. See junegunn/fzf#916 (comment) for examples. It doesn't work for external programs that share the same name as built-in commands in cmd.exe. cmd /c "echo" "hello" fails because it thinks echo" "hello is the executable. Since it's unreliable to use 2+ double quotes unless the user knows what they're doing and have checked the entire command and the environment variables (ie Does %ENV_VAR% contain any double quotes?), I don't recommend relying on this case in general.

Case 2 is for forcing this dequoting behaviour such that cmd.exe will run the entire command as is.
/s flag guarantees this such that cmd.exe /s /c "C:\path to neovim\bin\nvim.exe" fails because that never works in an interactive session; cmd.exe thinks that C:\path is the executable and the rest are the arguments, separated by spaces. However, cmd.exe /s /c ""C:\path to neovim\bin\nvim.exe"" works because the double-quote escaping for nvim.exe is retained.

I want to enable more tests to test my assumptions for Case 2 and eventually construct a shellescape() function that uses ^.

justinmk · 2017-10-04T17:35:33Z

Ok. Can the changes to test/functional/terminal/ex_terminal_spec.lua be avoided or not ? :)

janlazo · 2017-10-04T18:15:40Z

Yes. I'll work on it tonight.

janlazo · 2018-03-21T21:30:23Z

@justinmk fugitive will be more consistent after this PR because the result of running the command on a cmd.exe session or with termopen (or jobstart/system for non-interactive commands) gives the same result. If the command fails to run, then that's fugitive's problem because of its custom shellescape or invalid arguments.

justinmk · 2018-03-21T22:17:07Z

test/functional/eval/system_spec.lua

@@ -354,6 +360,8 @@ describe('systemlist()', function()
      eq(5, eval('v:shell_error'))
      eval([[systemlist('this-should-not-exist')]])
      eq(1, eval('v:shell_error'))
+      eval([[systemlist('"ping" "-n" "1" "127.0.0.1"')]])
+      eq(0, eval('v:shell_error'))


Is this to test #7698? Doesn't it add 1 second to each test?

This tests #7698 because /c doesn't support quoted programs with quoted arguments (ie. cmd /c "vim" "-u" "NONE"). ping -n 1 doesn't add 1 second because the first request runs immediately. -n 1 guarantees 1 request only because, by default, it sends up to 4 requests (ie -n 4).

Ok, still it would be clearer if we could instead check the output of something like echo or cat.

eq(..., [[systemlist('"echo" "foo" "bar" "baz"')]])

justinmk · 2018-03-23T06:57:35Z

Looks like more worked! Is there a reason echo can't be used?

janlazo · 2018-03-23T12:06:44Z

@justinmk echo.exe is in the C:\msys64\usr\bin or in bin/ folders of msysgit (or git-for-windows) so it's not available by default in Windows 7, 8.

I'll test with nested cmd.exe echo or powershell echo with escaped arguments.

Test on system() and systemlist()

justinmk · 2018-03-24T13:11:18Z

Can't we just use the examples given in #7698 ? It's not clear if these other elaborate examples with powershell are actually testing the problem from that issue. Let's keep it simple.

janlazo · 2018-03-24T15:22:02Z

That's covered by more.com and ping.exe since cmd.exe can't run them with an empty PATH (ie. set PATH=) unless the working directory is C:\Windows\system32.
The elaborate examples are for testing if the commands remain escaped in a nested shell.

Double-quoting cat in the existing tests for #7698 shouldn't be a problem for sh so I'll update the tests.

justinmk · 2018-03-24T16:18:44Z

test/functional/eval/system_spec.lua

+      eq('"a b"\r', eval([[get(systemlist('cmd /s/c "cmd /s/c "cmd /s/c "echo "a b""""'), 0, '')]]))
+      eq(0, eval('v:shell_error'))
+      eq('"a b"\r', eval([[get(systemlist('powershell -NoProfile -NoLogo -ExecutionPolicy RemoteSigned -Command echo ''\^"a b\^"'''), 0, '')]]))
+      eq(0, eval('v:shell_error'))


These new tests aren't relevant to this sets v:shell_error test group.

If we really need to check v:shell_error for these quote/escape tests, just do it in the other tests. I don't think we need to, though.

We need to keep the tests readable where possible.

justinmk · 2018-03-24T16:20:30Z

Assuming the build passes, are you done with changes?

janlazo · 2018-03-24T17:11:15Z

Yes, if it's okay to keep the redundant tests that you mentioned.

justinmk · 2018-03-24T20:40:28Z

test/functional/eval/system_spec.lua

@@ -464,7 +491,7 @@ describe('systemlist()', function()
    after_each(delete_file(fname))

    it('replaces NULs by newline characters', function()
-      eq({'part1\npart2\npart3'}, eval('systemlist("cat '..fname..'")'))
+      eq({'part1\npart2\npart3'}, eval([[systemlist('"cat" "]]..fname..[["')]]))


Is quoting like this now required, or is this change just to test #7698 ?

It's not required but #7698 had it. Other tests using cat aren't quoted and work fine.

justinmk · 2018-03-24T20:42:41Z

test/functional/eval/system_spec.lua

@@ -239,6 +254,8 @@ describe('system()', function()
      end
    end)
    it('to backgrounded command does not crash', function()
+      -- cmd.exe doesn't background a command with &
+      if iswin() then return end
      -- This is indeterminate, just exercise the codepath. May get E5677.
      feed_command('call system("echo -n echoed &")')


So this test now fails? (Even if it was useless before, just want to confirm that the new default now causes this kind of thing to fail.)

It's passes before and after this change. I disabled but the test isn't doing what it is described to do.

closes #7698 Wrapping a command in double-quotes allows cmd.exe to safely dequote the entire command as if the user entered the entire command in an interactive prompt. This reduces the need to escape nested and uneven double quotes. The `/s` flag of cmd.exe makes the behaviour more reliable: :set shellcmdflag=/s\ /c Before this patch, cmd.exe cannot use cygwin echo.exe (as opposed to cmd.exe `echo` builtin) even if it is wrapped in double quotes. Example: :: internal echo > cmd /s /c " echo foo\:bar" " foo\:bar" :: cygwin echo.exe > cmd /s /c " "echo" foo\:bar" " foo:bar

justinmk · 2018-03-24T21:11:18Z

Merged.

I removed the tests from the sets v:shell_error group. If you think we still need them let me know.

FEATURES: 3cc7ebf #7234 built-in VimL expression parser 6a7c904 #4419 implement <Cmd> key to invoke command in any mode b836328 #7679 'startup: treat stdin as text instead of commands' 58b210e :digraphs : highlight with hl-SpecialKey #2690 7a13611 #8276 'startup: Let `-s -` read from stdin' 1e71978 events: VimSuspend, VimResume #8280 1e7d5e8 #6272 'stdpath()' f96d99a #8247 server: introduce --listen e8c39f7 #8226 insert-mode: interpret unmapped META as ESC 98e7112 msg: do not scroll entire screen (#8088) f72630b #8055 let negative 'writedelay' show all redraws 5d2dd2e win: has("wsl") on Windows Subsystem for Linux #7330 a4f6cec cmdline: CmdlineEnter and CmdlineLeave autocommands (#7422) 207b7ca #6844 channels: support buffered output and bytes sockets/stdio API: f85cbea #7917 API: buffer updates 418abfc #6743 API: list information about all channels/jobs. 36b2e3f #8375 API: nvim_get_commands 273d2cd #8329 API: Make nvim_set_option() update `:verbose set …` 8d40b36 #8371 API: more reliable/descriptive VimL errors ebb1acb #8353 API: nvim_call_dict_function 9f994bb #8004 API: nvim_list_uis 3405704 #7520 API/UI: forward option updates to UIs 911b1e4 #7821 API: improve nvim_command_output WINDOWS OS: 9cefd83 #8084, #8516 build/win: support MSVC ee4e1fd win: Fix reading content from stdin (#8267) TUI: ffb8904 #8309 TUI: add support for mouse release events in urxvt 8d5a46e #8081 TUI: implement "standout" attribute 6071637 TUI: support TERM=konsole-256color 67848c0 #7653 TUI: report TUI info with -V3 ('verbose' >= 3) 3d0ee17 TUI/rxvt: enable focus-reporting d109f56 #7640 TUI: 'term' option: reflect effective terminal behavior FIXES: ed6a113 #8273 'job-control: avoid kill-timer race' 4e02f1a #8107 'jobs: separate process-group' 451c48a terminal: flush vterm output buffer on pty output #8486 5d6732f :checkhealth fixes #8335 53f11dc #8218 'Fix errors reported by PVS' d05712f inccommand: pause :terminal redraws (#8307) 51af911 inccommand: do not execute trailing commands #8256 84359a4 terminal: resize to the max dimensions (#8249) d49c1dd #8228 Make vim_fgets() return the same values as in Vim 60e96a4 screen: winhl=Normal:Background should not override syntax (#8093) 0c59ac1 #5908 'shada: Also save numbered marks' ba87a2c cscope: ignore EINTR while reading the prompt (#8079) b1412dc #7971 ':terminal Enter/Leave should not increment jumplist' 3a5721e TUI: libtermkey: force CSI driver for mouse input #7948 6ff13d7 #7720 TUI: faster startup 1c6e956 #7862 TUI: fix resize-related segfaults a58c909 #7676 TUI: always hide cursor when flushing, never flush buffers during unibilium output 303e1df #7624 TUI: disable BCE almost always 249bdb0 #7761 mark: Make sure that jumplist item will not have zero lnum 6f41ce0 #7704 macOS: Set $LANG based on the system locale a043899 #7633 'Retry fgets on EINTR' CHANGES: ad60927 #8304 default to 'nofsync' f3f1970 #8035 defaults: 'fillchars' a6052c7 #7984 defaults: sidescroll=1 b69fa86 #7888 defaults: enable cscopeverbose 7c4bb23 defaults: do :filetype stuff unless explicitly "off" 2aa308c #5658 'Apply :lmap in macros' 8ce6393 terminal: Leave 'relativenumber' alone (#8360) e46534b #4486 refactor: Remove maxmem, maxmemtot options 131aad9 win: defaults: 'shellcmdflag', 'shellxquote' #7343 c57d315 #8031 jobwait(): return -2 on interrupt also with timeout 6452831 clipboard: macOS: fallback to tmux if pbcopy is broken #7940 300d365 #7919 Make 'langnoremap' apply directly after a map ada1956 #7880 'lua/executor: Remove lightuserdata' INTERNAL: de0a954 #7806 internal statistics for list impl dee78a4 #7708 rewrite internal list impl

janlazo force-pushed the windows_shell_cmd branch from a558f18 to 42bdf92 Compare October 1, 2017 01:55

marvim added the WIP label Oct 1, 2017

janlazo force-pushed the windows_shell_cmd branch from 42bdf92 to 4a80a78 Compare October 1, 2017 13:54

justinmk added the platform:windows label Oct 1, 2017

justinmk added this to the 0.2.2 milestone Oct 1, 2017

janlazo force-pushed the windows_shell_cmd branch from 142b538 to 90cf20f Compare October 2, 2017 07:10

janlazo mentioned this pull request Oct 2, 2017

Fix ExecCommandWith for cmd.exe in Windows junegunn/fzf#1072

Closed

janlazo changed the title ~~[WIP] win: defaults: 'shellcmdflag', 'shellxquote'~~ [RFC] win: defaults: 'shellcmdflag', 'shellxquote' Oct 2, 2017

marvim added RFC and removed WIP labels Oct 2, 2017

janlazo mentioned this pull request Oct 3, 2017

Windows: command line args spec #4928

Closed

janlazo force-pushed the windows_shell_cmd branch 2 times, most recently from 68c2696 to 1bcd19b Compare October 4, 2017 04:52

justinmk reviewed Oct 4, 2017

View reviewed changes

janlazo force-pushed the windows_shell_cmd branch from 70bed4f to 65686d0 Compare October 4, 2017 23:21

janlazo changed the title ~~[RFC] win: defaults: 'shellcmdflag', 'shellxquote'~~ [WIP] win: defaults: 'shellcmdflag', 'shellxquote' Oct 4, 2017

janlazo added 2 commits March 21, 2018 17:47

fixup: doc: use backticks for sample commands

6bc5ba9

test: win: quoted ping works in system,systemlist

424abbc

justinmk reviewed Mar 21, 2018

View reviewed changes

janlazo force-pushed the windows_shell_cmd branch from a149ef6 to 424abbc Compare March 22, 2018 16:53

test: win: try more.com on system()

a1c5a8a

janlazo force-pushed the windows_shell_cmd branch from 50c2c73 to a1c5a8a Compare March 22, 2018 19:57

test: win: try more.com on $COMSPEC and systemlist

eee9c3e

janlazo added 2 commits March 24, 2018 00:50

test: win: nested cmd /s/c for system,systemlist

a376955

test: win: add escaped powershell command

31ac97c

Test on system() and systemlist()

janlazo force-pushed the windows_shell_cmd branch from f7e0d05 to 31ac97c Compare March 24, 2018 05:10

fixup: escape echo argument

1c864a6

janlazo added 2 commits March 24, 2018 11:35

test: win: silently return on background test

34f70ac

test: win: escape cat and arguments (neovim#7698)

b9a2cce

justinmk reviewed Mar 24, 2018

View reviewed changes

justinmk closed this Mar 24, 2018

justinmk removed the RFC label Mar 24, 2018

janlazo mentioned this pull request Mar 24, 2018

[RDY]test: win: background command with start /b /wait #8171

Merged

justinmk mentioned this pull request Apr 22, 2018

Strange ui output on windows equalsraf/neovim-qt#304

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[RFC] win: defaults: 'shellcmdflag', 'shellxquote' #7343

[RFC] win: defaults: 'shellcmdflag', 'shellxquote' #7343

janlazo commented Oct 1, 2017 •

edited

Loading

janlazo commented Oct 1, 2017

janlazo commented Oct 1, 2017 •

edited

Loading

justinmk commented Oct 1, 2017

janlazo commented Oct 2, 2017

janlazo commented Oct 2, 2017

janlazo commented Oct 2, 2017 •

edited

Loading

justinmk Oct 4, 2017

janlazo Oct 4, 2017

justinmk Oct 4, 2017

janlazo commented Oct 4, 2017

justinmk commented Oct 4, 2017

janlazo commented Oct 4, 2017 •

edited

Loading

justinmk commented Oct 4, 2017

janlazo commented Oct 4, 2017

janlazo commented Mar 21, 2018

justinmk Mar 21, 2018

janlazo Mar 21, 2018 •

edited

Loading

justinmk Mar 22, 2018

justinmk commented Mar 23, 2018

janlazo commented Mar 23, 2018

justinmk commented Mar 24, 2018

janlazo commented Mar 24, 2018 •

edited

Loading

justinmk Mar 24, 2018 •

edited

Loading

justinmk commented Mar 24, 2018

janlazo commented Mar 24, 2018

justinmk Mar 24, 2018

janlazo Mar 25, 2018

justinmk Mar 24, 2018 •

edited

Loading

janlazo Mar 25, 2018

justinmk commented Mar 24, 2018

[RFC] win: defaults: 'shellcmdflag', 'shellxquote' #7343

[RFC] win: defaults: 'shellcmdflag', 'shellxquote' #7343

Conversation

janlazo commented Oct 1, 2017 • edited Loading

janlazo commented Oct 1, 2017

janlazo commented Oct 1, 2017 • edited Loading

justinmk commented Oct 1, 2017

janlazo commented Oct 2, 2017

janlazo commented Oct 2, 2017

janlazo commented Oct 2, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

janlazo commented Oct 4, 2017

justinmk commented Oct 4, 2017

janlazo commented Oct 4, 2017 • edited Loading

justinmk commented Oct 4, 2017

janlazo commented Oct 4, 2017

janlazo commented Mar 21, 2018

Choose a reason for hiding this comment

janlazo Mar 21, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

justinmk commented Mar 23, 2018

janlazo commented Mar 23, 2018

justinmk commented Mar 24, 2018

janlazo commented Mar 24, 2018 • edited Loading

justinmk Mar 24, 2018 • edited Loading

Choose a reason for hiding this comment

justinmk commented Mar 24, 2018

janlazo commented Mar 24, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

justinmk Mar 24, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

justinmk commented Mar 24, 2018

janlazo commented Oct 1, 2017 •

edited

Loading

janlazo commented Oct 1, 2017 •

edited

Loading

janlazo commented Oct 2, 2017 •

edited

Loading

janlazo commented Oct 4, 2017 •

edited

Loading

janlazo Mar 21, 2018 •

edited

Loading

janlazo commented Mar 24, 2018 •

edited

Loading

justinmk Mar 24, 2018 •

edited

Loading

justinmk Mar 24, 2018 •

edited

Loading