Merge #5910 from justinmk/win32-jobstart

Windows: fix jobstart()

8 files changed, 117 insertions, 39 deletions

diff --git a/.ci/build.bat b/.ci/build.bat
index c2f560fb7c..d21957718d 100644
--- a/.ci/build.bat
+++ b/.ci/build.bat
@@ -17,7 +17,14 @@ set PATH=C:\Program Files (x86)\CMake\bin\cpack.exe;%PATH%
 
 :: Build third-party dependencies
 C:\msys64\usr\bin\bash -lc "pacman --verbose --noconfirm -Su" || goto :error
-C:\msys64\usr\bin\bash -lc "pacman --verbose --noconfirm --needed -S mingw-w64-%ARCH%-cmake mingw-w64-%ARCH%-perl mingw-w64-%ARCH%-python2 mingw-w64-%ARCH%-diffutils gperf" || goto :error
+C:\msys64\usr\bin\bash -lc "pacman --verbose --noconfirm --needed -S mingw-w64-%ARCH%-cmake mingw-w64-%ARCH%-perl mingw-w64-%ARCH%-diffutils gperf" || goto :error
+
+:: Use Appveyor's python
+set PATH=C:\Python27;C:\Python27\Scripts;%PATH%
+set PATH=C:\Python35;C:\Python35\Scripts;%PATH%
+copy c:\Python35\python.exe c:\Python35\python3.exe
+pip2 install neovim || goto error
+pip3 install neovim || goto error
 
 mkdir .deps
 cd .deps
diff --git a/CMakeLists.txt b/CMakeLists.txt
index cb8302deb7..fdd6f0ed79 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -489,10 +489,9 @@ if(BUSTED_PRG)
     ${CMAKE_BINARY_DIR}/test/config/paths.lua)
 
   set(UNITTEST_PREREQS nvim-test unittest-headers)
-  if(WIN32)
-    set(FUNCTIONALTEST_PREREQS nvim shell-test)
-  else()
-    set(FUNCTIONALTEST_PREREQS nvim tty-test shell-test)
+  set(FUNCTIONALTEST_PREREQS nvim printargs-test shell-test)
+  if(NOT WIN32)
+    list(APPEND FUNCTIONALTEST_PREREQS tty-test)
   endif()
   set(BENCHMARK_PREREQS nvim tty-test)
 
diff --git a/runtime/doc/eval.txt b/runtime/doc/eval.txt
index 78124debe1..69c8d0285a 100644
--- a/runtime/doc/eval.txt
+++ b/runtime/doc/eval.txt
@@ -4666,11 +4666,16 @@ jobstart({cmd}[, {opts}])				{Nvim} *jobstart()*
 		Spawns {cmd} as a job.  If {cmd} is a |List| it is run
 		directly.  If {cmd} is a |String| it is processed like this: >
 		  :call jobstart(split(&shell) + split(&shellcmdflag) + ['{cmd}'])
-<		NOTE: read |shell-unquoting| before constructing any lists 
-		with 'shell' or 'shellcmdflag' options.  The above call is 
-		only written to show the idea, one needs to perform unquoting 
-		and do split taking quotes into account.
-
+<		NOTE: This only shows the idea; see |shell-unquoting| before
+		constructing lists with 'shell' or 'shellcmdflag'.
+
+		NOTE: On Windows if {cmd} is a List, cmd[0] must be a valid
+		executable (.exe, .com).  If the executable is in $PATH it can
+		be called by name, with or without an extension: >
+		   :call jobstart(['ping', 'neovim.io'])
+<		If it is a path (not a name), it must include the extension: >
+		   :call jobstart(['System32\ping.exe', 'neovim.io'])
+<
 		{opts} is a dictionary with these keys:
 		  on_stdout: stdout event handler (function name or |Funcref|)
 		  on_stderr: stderr event handler (function name or |Funcref|)
@@ -7099,9 +7104,8 @@ synstack({lnum}, {col})					*synstack()*
 		valid positions.
 
 system({cmd} [, {input}])				*system()* *E677*
-		Get the output of the shell command {cmd} as a |string|.  {cmd}
-		will be run the same as in |jobstart()|.  See |systemlist()|
-		to get the output as a |List|.
+		Get the output of {cmd} as a |string| (use |systemlist()| to
+		get a |List|). {cmd} is treated exactly as in |jobstart()|.
 		Not to be used for interactive commands.
 
 		If {input} is a string it is written to a pipe and passed as
diff --git a/src/nvim/eval.c b/src/nvim/eval.c
index 17e89e5757..1688a565c1 100644
--- a/src/nvim/eval.c
+++ b/src/nvim/eval.c
@@ -16994,8 +16994,12 @@ static void get_system_output_as_rettv(typval_T *argvars, typval_T *rettv,
   }
 
   // get shell command to execute
-  char **argv = tv_to_argv(&argvars[0], NULL, NULL);
+  bool executable = true;
+  char **argv = tv_to_argv(&argvars[0], NULL, &executable);
   if (!argv) {
+    if (!executable) {
+      set_vim_var_nr(VV_SHELL_ERROR, (long)-1);
+    }
     xfree(input);
     return;  // Already did emsg.
   }
diff --git a/src/nvim/event/libuv_process.c b/src/nvim/event/libuv_process.c
index a68badcc8f..907187aa17 100644
--- a/src/nvim/event/libuv_process.c
+++ b/src/nvim/event/libuv_process.c
@@ -19,8 +19,7 @@ bool libuv_process_spawn(LibuvProcess *uvproc)
   Process *proc = (Process *)uvproc;
   uvproc->uvopts.file = proc->argv[0];
   uvproc->uvopts.args = proc->argv;
-  uvproc->uvopts.flags = UV_PROCESS_WINDOWS_HIDE
-                  | UV_PROCESS_WINDOWS_VERBATIM_ARGUMENTS;
+  uvproc->uvopts.flags = UV_PROCESS_WINDOWS_HIDE;
   if (proc->detach) {
       uvproc->uvopts.flags |= UV_PROCESS_DETACHED;
   }
diff --git a/test/functional/eval/system_spec.lua b/test/functional/eval/system_spec.lua
index 6393477260..d5845132dd 100644
--- a/test/functional/eval/system_spec.lua
+++ b/test/functional/eval/system_spec.lua
@@ -1,12 +1,10 @@
 local helpers = require('test.functional.helpers')(after_each)
-local eq, clear, eval, execute, feed, nvim =
-  helpers.eq, helpers.clear, helpers.eval, helpers.execute, helpers.feed,
-  helpers.nvim
+local eq, call, clear, eval, execute, feed, nvim =
+  helpers.eq, helpers.call, helpers.clear, helpers.eval, helpers.execute,
+  helpers.feed, helpers.nvim
 
 local Screen = require('test.functional.ui.screen')
 
-if helpers.pending_win32(pending) then return end
-
 local function create_file_with_nuls(name)
   return function()
     feed('ipart1<C-V>000part2<C-V>000part3<ESC>:w '..name..'<CR>')
@@ -31,7 +29,70 @@ end
 describe('system()', function()
   before_each(clear)
 
-  it('sets the v:shell_error variable', function()
+  describe('command passed as a List', function()
+    local printargs_path = helpers.nvim_dir..'/printargs-test'
+      .. (helpers.os_name() == 'windows' and '.exe' or '')
+
+    it('sets v:shell_error if cmd[0] is not executable', function()
+      call('system', { 'this-should-not-exist' })
+      eq(-1, eval('v:shell_error'))
+    end)
+
+    it('parameter validation does NOT modify v:shell_error', function()
+      -- 1. Call system() with invalid parameters.
+      -- 2. Assert that v:shell_error was NOT set.
+      execute('call system({})')
+      eq('E475: Invalid argument: expected String or List', eval('v:errmsg'))
+      eq(0, eval('v:shell_error'))
+      execute('call system([])')
+      eq('E474: Invalid argument', eval('v:errmsg'))
+      eq(0, eval('v:shell_error'))
+
+      -- Provoke a non-zero v:shell_error.
+      call('system', { 'this-should-not-exist' })
+      local old_val = eval('v:shell_error')
+      eq(-1, old_val)
+
+      -- 1. Call system() with invalid parameters.
+      -- 2. Assert that v:shell_error was NOT modified.
+      execute('call system({})')
+      eq(old_val, eval('v:shell_error'))
+      execute('call system([])')
+      eq(old_val, eval('v:shell_error'))
+    end)
+
+    it('quotes arguments correctly #5280', function()
+      local out = call('system',
+        { printargs_path, [[1]], [[2 "3]], [[4 ' 5]], [[6 ' 7']] })
+
+      eq(0, eval('v:shell_error'))
+      eq([[arg1=1;arg2=2 "3;arg3=4 ' 5;arg4=6 ' 7';]], out)
+
+      out = call('system', { printargs_path, [['1]], [[2 "3]] })
+      eq(0, eval('v:shell_error'))
+      eq([[arg1='1;arg2=2 "3;]], out)
+
+      out = call('system', { printargs_path, "A\nB" })
+      eq(0, eval('v:shell_error'))
+      eq("arg1=A\nB;", out)
+    end)
+
+    it('calls executable in $PATH', function()
+      if 0 == eval("executable('python')") then pending("missing `python`") end
+      eq("foo\n", eval([[system(['python', '-c', 'print("foo")'])]]))
+      eq(0, eval('v:shell_error'))
+    end)
+
+    it('does NOT run in shell', function()
+      if helpers.os_name() ~= 'windows' then
+        eq("* $PATH %PATH%\n", eval("system(['echo', '*', '$PATH', '%PATH%'])"))
+      end
+    end)
+  end)
+
+  if helpers.pending_win32(pending) then return end
+
+  it('sets v:shell_error', function()
     eval([[system("sh -c 'exit'")]])
     eq(0, eval('v:shell_error'))
     eval([[system("sh -c 'exit 1'")]])
@@ -158,7 +219,7 @@ describe('system()', function()
     end)
   end)
 
-  describe('passing number as input', function()
+  describe('input passed as Number', function()
     it('stringifies the input', function()
       eq('1', eval('system("cat", 1)'))
     end)
@@ -175,8 +236,8 @@ describe('system()', function()
     end)
   end)
 
-  describe('passing list as input', function()
-    it('joins list items with linefeed characters', function()
+  describe('input passed as List', function()
+    it('joins List items with linefeed characters', function()
       eq('line1\nline2\nline3',
         eval("system('cat -', ['line1', 'line2', 'line3'])"))
     end)
@@ -185,7 +246,7 @@ describe('system()', function()
     -- is inconsistent and is a good reason for the existence of the
     -- `systemlist()` function, where input and output map to the same
     -- characters(see the following tests with `systemlist()` below)
-    describe('with linefeed characters inside list items', function()
+    describe('with linefeed characters inside List items', function()
       it('converts linefeed characters to NULs', function()
         eq('l1\001p2\nline2\001a\001b\nl3',
           eval([[system('cat -', ["l1\np2", "line2\na\nb", 'l3'])]]))
@@ -202,7 +263,7 @@ describe('system()', function()
 
   describe("with a program that doesn't close stdout", function()
     if not xclip then
-      pending('skipped (missing xclip)', function() end)
+      pending('missing `xclip`', function() end)
     else
       it('will exit properly after passing input', function()
         eq('', eval([[system('xclip -i -selection clipboard', 'clip-data')]]))
@@ -210,18 +271,12 @@ describe('system()', function()
       end)
     end
   end)
-
-  describe('command passed as a list', function()
-    it('does not execute &shell', function()
-      eq('* $NOTHING ~/file',
-         eval("system(['echo', '-n', '*', '$NOTHING', '~/file'])"))
-    end)
-  end)
 end)
 
+if helpers.pending_win32(pending) then return end
+
 describe('systemlist()', function()
-  -- behavior is similar to `system()` but it returns a list instead of a
-  -- string.
+  -- Similar to `system()`, but returns List instead of String.
   before_each(clear)
 
   it('sets the v:shell_error variable', function()
@@ -334,14 +389,14 @@ describe('systemlist()', function()
     end)
   end)
 
-  describe('passing list as input', function()
+  describe('input passed as List', function()
     it('joins list items with linefeed characters', function()
       eq({'line1', 'line2', 'line3'},
         eval("systemlist('cat -', ['line1', 'line2', 'line3'])"))
     end)
 
     -- Unlike `system()` which uses SOH to represent NULs, with `systemlist()`
-    -- input and ouput are the same
+    -- input and ouput are the same.
     describe('with linefeed characters inside list items', function()
       it('converts linefeed characters to NULs', function()
         eq({'l1\np2', 'line2\na\nb', 'l3'},
@@ -381,7 +436,7 @@ describe('systemlist()', function()
 
   describe("with a program that doesn't close stdout", function()
     if not xclip then
-      pending('skipped (missing xclip)', function() end)
+      pending('missing `xclip`', function() end)
     else
       it('will exit properly after passing input', function()
         eq({}, eval(
diff --git a/test/functional/fixtures/CMakeLists.txt b/test/functional/fixtures/CMakeLists.txt
index 70aee6efa9..8537ea390f 100644
--- a/test/functional/fixtures/CMakeLists.txt
+++ b/test/functional/fixtures/CMakeLists.txt
@@ -2,3 +2,4 @@ add_executable(tty-test tty-test.c)
 target_link_libraries(tty-test ${LIBUV_LIBRARIES})
 
 add_executable(shell-test shell-test.c)
+add_executable(printargs-test printargs-test.c)
diff --git a/test/functional/fixtures/printargs-test.c b/test/functional/fixtures/printargs-test.c
new file mode 100644
index 0000000000..2c25cf8447
--- /dev/null
+++ b/test/functional/fixtures/printargs-test.c
@@ -0,0 +1,9 @@
+#include <stdio.h>
+
+int main(int argc, char **argv)
+{
+  for (int i=1; i<argc; i++) {
+    printf("arg%d=%s;", i, argv[i]);
+  }
+  return 0;
+}