term: use an argument vector for termopen().

Old behaviour: termopen('cmd') would run `&shell &shcf "cmd"`, which
caused the functional tests to fail on some systems due to the process
not "owning" the terminal. Also, it is inconsistent with jobstart().

Modify termopen() so that &shell is not invoked, but maintain the old
behaviour with :terminal. Factor the common code for building the
argument vector from jobstart() and modify the functional tests to call
termopen() instead of :terminal (fixes #2354).

Also:
 * Add a 'name' option for termopen() so that `:terminal {cmd}` produces
   a buffer named "term//{cwd}/{cmd}" and termopen() users can customize
   the name.
 * Update the documentation.
 * Add functional tests for `:terminal` sinse its behaviour now differs
   from termopen(). Add "test/functional/fixtures/shell-test.c" and move
   "test/functional/job/tty-test.c" there, too.

Helped-by: Justin M. Keyes <@justinmk>

CMakeLists.txt

@@ -296,7 +296,7 @@ get_compile_flags(NVIM_VERSION_CFLAGS)
 
 add_subdirectory(test/includes)
 add_subdirectory(config)
-add_subdirectory(test/functional/job) # compile pty test program
+add_subdirectory(test/functional/fixtures) # compile pty/shell test programs
 
 
 # Setup some test-related bits.  We do this after going down the tree because we

runtime/doc/eval.txt

@@ -6299,15 +6299,17 @@ tempname()					*tempname()* *temp-file-name*
 		For MS-Windows forward slashes are used when the 'shellslash'
 		option is set or when 'shellcmdflag' starts with '-'.
 
-termopen({command}[, {opts}])			{Nvim} *termopen()*
+termopen({argv}[, {opts}])			{Nvim} *termopen()*
-		Spawns {command} using the shell in a new pseudo-terminal
+		Spawns {argv}(list) in a new pseudo-terminal session connected
-		session connected to the current buffer.  This function fails
+		to the current buffer.  This function fails if the current
-		if the current buffer is modified (all buffer contents are
+		buffer is modified (all buffer contents are destroyed).
-		destroyed).  The {opts} dict is similar to the one passed to
+
-		|jobstart()|, but the `pty`, `width`, `height`, and `TERM` fields are
+		The {opts} dict is similar to the one passed to |jobstart()|,
-		ignored: `height`/`width` are taken from the current window and
+		but the `pty`, `width`, `height`, and `TERM` fields are
-		$TERM is set to "xterm-256color".  Returns the same values as
+		ignored: `height`/`width` are taken from the current window
-		|jobstart()|.
+		and `$TERM` is set to "xterm-256color".  An additional option,
+		`name`, can be used to give the terminal a specific name.
+		Returns the same values as |jobstart()|.
 
 		See |nvim-terminal-emulator| for more information.
 

runtime/doc/various.txt

@@ -223,11 +223,18 @@ g8			Print the hex values of the bytes used in the
 :sh[ell]		Removed. |vim-differences| {Nvim}
 
 						  *:term* *:terminal*
-:term[inal][!] {cmd}	Spawns {command} using the current value of 'shell'
+:term[inal][!] {cmd}	Spawns {cmd} using the current value of 'shell' and
-			in a new terminal buffer.  This is equivalent to: >
+			'shellcmdflag' in a new terminal buffer.  This is
+			equivalent to: >
 
-			      :enew | call termopen('{cmd}') | startinsert
+			      :enew
+			      :call termopen([&sh, &shcf, '{cmd}'],
+                                              \{'name':'{cmd}'})
+			      :startinsert
 <
+			If no {cmd} is given, 'shellcmdflag' will not be sent
+			to |termopen()|.
+
 			Like |:enew|, it will fail if the current buffer is
 			modified, but can be forced with "!".  See |termopen()|
 			and |nvim-terminal-emulator| for more information.

src/nvim/eval.c

@@ -10779,6 +10779,40 @@ static void f_jobresize(typval_T *argvars, typval_T *rettv)
   rettv->vval.v_number = 1;
 }
 
+static char **list_to_argv(list_T *args)
+{
+  for (listitem_T *arg = args->lv_first; arg != NULL; arg = arg->li_next) {
+    if (arg->li_tv.v_type != VAR_STRING) {
+      EMSG(_(e_invarg));
+      return NULL;
+    }
+  }
+
+  int argc = args->lv_len;
+  if (!argc) {
+    EMSG(_("Argument vector must have at least one item"));
+    return NULL;
+  }
+
+  assert(args->lv_first);
+
+  const char_u *exe = get_tv_string(&args->lv_first->li_tv);
+  if (!os_can_exe(exe, NULL)) {
+    // String is not executable
+    EMSG2(e_jobexe, exe);
+    return NULL;
+  }
+  
+  // Build the argument vector
+  int i = 0;
+  char **argv = xcalloc(argc + 1, sizeof(char *));
+  for (listitem_T *arg = args->lv_first; arg != NULL; arg = arg->li_next) {
+    argv[i++] = xstrdup((char *) get_tv_string(&arg->li_tv));
+  }
+
+  return argv;
+}
+
 // "jobstart()" function
 static void f_jobstart(typval_T *argvars, typval_T *rettv)
 {
@@ -10796,28 +10830,9 @@ static void f_jobstart(typval_T *argvars, typval_T *rettv)
     return;
   }
 
-  list_T *args = argvars[0].vval.v_list;
+  char **argv = list_to_argv(argvars[0].vval.v_list);
-  // Assert that all list items are strings
+  if (!argv) {
-  for (listitem_T *arg = args->lv_first; arg != NULL; arg = arg->li_next) {
+    return;  // Did error message in list_to_argv.
-    if (arg->li_tv.v_type != VAR_STRING) {
-      EMSG(_(e_invarg));
-      return;
-    }
-  }
-
-  int argc = args->lv_len;
-  if (!argc) {
-    EMSG(_("Argument vector must have at least one item"));
-    return;
-  }
-
-  assert(args->lv_first);
- 
-  const char_u *exe = get_tv_string(&args->lv_first->li_tv);
-  if (!os_can_exe(exe, NULL)) {
-    // String is not executable
-    EMSG2(e_jobexe, exe);
-    return;
   }
 
   dict_T *job_opts = NULL;
@@ -10829,13 +10844,6 @@ static void f_jobstart(typval_T *argvars, typval_T *rettv)
     }
   }
 
-  // Build the argument vector
-  int i = 0;
-  char **argv = xcalloc(argc + 1, sizeof(char *));
-  for (listitem_T *arg = args->lv_first; arg != NULL; arg = arg->li_next) {
-    argv[i++] = xstrdup((char *) get_tv_string(&arg->li_tv));
-  }
-
   JobOptions opts = common_job_options(argv, on_stdout, on_stderr, on_exit,
       job_opts);
 
@@ -15155,13 +15163,18 @@ static void f_termopen(typval_T *argvars, typval_T *rettv)
     return;
   }
 
-  if (argvars[0].v_type != VAR_STRING
+  if (argvars[0].v_type != VAR_LIST
       || (argvars[1].v_type != VAR_DICT && argvars[1].v_type != VAR_UNKNOWN)) {
     // Wrong argument types
     EMSG(_(e_invarg));
     return;
   }
 
+  char **argv = list_to_argv(argvars[0].vval.v_list);
+  if (!argv) {
+    return;  // Did error message in list_to_argv.
+  }
+
   ufunc_T *on_stdout = NULL, *on_stderr = NULL, *on_exit = NULL;
   dict_T *job_opts = NULL;
   if (argvars[1].v_type == VAR_DICT) {
@@ -15171,7 +15184,6 @@ static void f_termopen(typval_T *argvars, typval_T *rettv)
     }
   }
 
-  char **argv = shell_build_argv((char *)argvars[0].vval.v_string, NULL);
   JobOptions opts = common_job_options(argv, on_stdout, on_stderr, on_exit,
       job_opts);
   opts.pty = true;
@@ -15197,10 +15209,16 @@ static void f_termopen(typval_T *argvars, typval_T *rettv)
     cwd = (char *)argvars[1].vval.v_string;
   }
   int pid = job_pid(job);
+
+  // Get the desired name of the buffer.
+  char *name = job_opts ?
+    (char *)get_dict_string(job_opts, (char_u *)"name", false) : NULL;
+  if (!name) {
+    name = argv[0];
+  }
   char buf[1024];
   // format the title with the pid to conform with the term:// URI 
-  snprintf(buf, sizeof(buf), "term://%s//%d:%s", cwd, pid,
+  snprintf(buf, sizeof(buf), "term://%s//%d:%s", cwd, pid, name);
-      (char *)argvars[0].vval.v_string);
   // at this point the buffer has no terminal instance associated yet, so unset
   // the 'swapfile' option to ensure no swap file will be created
   curbuf->b_p_swf = false;

src/nvim/ex_docmd.c

@@ -9410,9 +9410,26 @@ static void ex_folddo(exarg_T *eap)
 
 static void ex_terminal(exarg_T *eap)
 {
+  char *name = NULL;
   char cmd[512];
-  snprintf(cmd, sizeof(cmd), ":enew%s | call termopen('%s') | startinsert",
+  if (strcmp((char *)eap->arg, "") == 0) {
-      eap->forceit==TRUE ? "!" : "",
+    snprintf(cmd, sizeof(cmd), "['%s']", (char *)p_sh);
-      strcmp((char *)eap->arg, "") ? (char *)eap->arg : (char *)p_sh);
+    name = (char *)p_sh;
-  do_cmdline_cmd((uint8_t *)cmd);
+  } else {
+    // Escape quotes and slashes so they get sent literally.
+    name = (char *)vim_strsave_escaped(eap->arg, (char_u *)"\"\\");
+    snprintf(cmd, sizeof(cmd), "['%s','%s',\"%s\"]",
+             (char *)p_sh, (char *)p_shcf, name);
+  }
+
+  char ex_cmd[512];
+  snprintf(ex_cmd, sizeof(ex_cmd),
+           ":enew%s | call termopen(%s, {'name':\"%s\"}) | startinsert",
+           eap->forceit==TRUE ? "!" : "", cmd, name);
+
+  do_cmdline_cmd((uint8_t *)ex_cmd);
+
+  if (name != (char *)p_sh) {
+    xfree(name);
+  }
 }

test/functional/fixtures/CMakeLists.txt

@@ -1,2 +1,4 @@
 add_executable(tty-test tty-test.c)
 target_link_libraries(tty-test ${LIBUV_LIBRARIES})
+
+add_executable(shell-test shell-test.c)

test/functional/fixtures/shell-test.c

@@ -0,0 +1,25 @@
+// A simple implementation of a shell for testing
+// `termopen([&sh, &shcf, '{cmd'}])` and `termopen([&sh])`.
+//
+// If launched with no arguments, prints "ready $ ", otherwise prints
+// "ready $ {cmd}\n".
+
+#include <stdio.h>
+#include <string.h>
+
+int main(int argc, char **argv)
+{
+  fprintf(stderr, "ready $ ");
+
+  if (argc == 3) {
+    // argv should be {"terminal-test", "EXE", "prog args..."}
+    if (strcmp(argv[1], "EXE") != 0) {
+      fprintf(stderr, "first argument must be 'EXE'\n");
+      return 2;
+    }
+
+    fprintf(stderr, "%s\n", argv[2]);
+  }
+
+  return 0;
+}

test/functional/terminal/cursor_spec.lua

@@ -144,7 +144,7 @@ describe('cursor with customized highlighting', function()
       [6] = {foreground = 130},
     })
     screen:attach(false)
-    execute('term "' ..nvim_dir.. '/tty-test"')
+    execute('call termopen(["'..nvim_dir..'/tty-test"]) | startinsert')
   end)
 
   it('overrides the default highlighting', function()

test/functional/terminal/ex_terminal_spec.lua

@@ -0,0 +1,61 @@
+local helpers = require('test.functional.helpers')
+local Screen = require('test.functional.ui.screen')
+local clear, wait, nvim = helpers.clear, helpers.wait, helpers.nvim
+local nvim_dir = helpers.nvim_dir
+local execute, source = helpers.execute, helpers.source
+local eq, neq = helpers.eq, helpers.neq
+
+describe(':terminal', function()
+  local screen
+
+  before_each(function()
+    clear()
+    screen = Screen.new(50, 7)
+    screen:attach(false)
+    nvim('set_option', 'shell', nvim_dir..'/shell-test')
+    nvim('set_option', 'shellcmdflag', 'EXE')
+
+  end)
+
+  it('with no argument, acts like termopen()', function()
+    execute('terminal')
+    wait()
+    screen:expect([[
+      ready $                                           |
+      [Program exited, press any key to close]          |
+                                                        |
+                                                        |
+                                                        |
+                                                        |
+      -- TERMINAL --                                    |
+    ]])
+  end)
+
+  it('executes a given command through the shell', function()
+    execute('terminal echo hi')
+    wait()
+    screen:expect([[
+      ready $ echo hi                                   |
+                                                        |
+      [Program exited, press any key to close]          |
+                                                        |
+                                                        |
+                                                        |
+      -- TERMINAL --                                    |
+    ]])
+  end)
+
+  it('allows quotes and slashes', function()
+    execute([[terminal echo 'hello' \ "world"]])
+    wait()
+    screen:expect([[
+      ready $ echo 'hello' \ "world"                    |
+                                                        |
+      [Program exited, press any key to close]          |
+                                                        |
+                                                        |
+                                                        |
+      -- TERMINAL --                                    |
+    ]])
+  end)
+end)

test/functional/terminal/helpers.lua

@@ -56,7 +56,7 @@ local function screen_setup(extra_height)
   -- tty-test puts the terminal into raw mode and echoes all input. tests are
   -- done by feeding it with terminfo codes to control the display and
   -- verifying output with screen:expect.
-  execute('term ' ..nvim_dir.. '/tty-test')
+  execute('enew | call termopen(["'..nvim_dir..'/tty-test"]) | startinsert')
   -- wait for "tty ready" to be printed before each test or the terminal may
   -- still be in canonical mode(will echo characters for example)
   --

test/functional/terminal/highlight_spec.lua

@@ -27,7 +27,7 @@ describe('terminal window highlighting', function()
       [8] = {background = 11}
     })
     screen:attach(false)
-    execute('term "' ..nvim_dir.. '/tty-test"')
+    execute('enew | call termopen(["'..nvim_dir..'/tty-test"]) | startinsert')
     screen:expect([[
       tty ready                                         |
                                                         |
@@ -133,7 +133,7 @@ describe('terminal window highlighting with custom palette', function()
     })
     screen:attach(true)
     nvim('set_var', 'terminal_color_3', '#123456')
-    execute('term "' ..nvim_dir.. '/tty-test"')
+    execute('enew | call termopen(["'..nvim_dir..'/tty-test"]) | startinsert')
     screen:expect([[
       tty ready                                         |
                                                         |

test/functional/terminal/scrollback_spec.lua

@@ -332,7 +332,7 @@ describe('terminal prints more lines than the screen height and exits', function
     clear()
     local screen = Screen.new(50, 7)
     screen:attach(false)
-    execute('term ' ..nvim_dir.. '/tty-test 10')
+    execute('call termopen(["'..nvim_dir..'/tty-test", "10"]) | startinsert')
     wait()
     screen:expect([[
       line6                                             |