Merge pull request neovim#13876 from jamessan/pty-term

fix(pty): Always use $TERM from the job's env dict

Author: jamessan

runtime/doc/eval.txt

@@ -5607,7 +5607,6 @@ jobstart({cmd}[, {opts}])				*jobstart()*
 			      before invoking `on_stderr`. |channel-buffered|
 		  stdout_buffered: (boolean) Collect data until EOF (stream
 			      closed) before invoking `on_stdout`. |channel-buffered|
-		  TERM:	      (string) Sets the `pty` $TERM environment variable.
 		  width:      (number) Width of the `pty` terminal.
 
 		{opts} is passed as |self| dictionary to the callback; the

src/nvim/channel.c

@@ -292,7 +292,6 @@ static void close_cb(Stream *stream, void *data)
 ///                  directory if `cwd` is NULL
 /// @param[in]  pty_width  Width of the pty, ignored if `pty` is false
 /// @param[in]  pty_height  Height of the pty, ignored if `pty` is false
-/// @param[in]  term_name  `$TERM` for the pty
 /// @param[in]  env  Nvim's configured environment is used if this is NULL,
 ///                  otherwise defines all environment variables
 /// @param[out]  status_out  0 for invalid arguments, > 0 for the channel id,
@@ -304,8 +303,7 @@ Channel *channel_job_start(char **argv, CallbackReader on_stdout,
                            bool pty, bool rpc, bool overlapped, bool detach,
                            const char *cwd,
                            uint16_t pty_width, uint16_t pty_height,
-                           char *term_name, dict_T *env,
-                           varnumber_T *status_out)
+                           dict_T *env, varnumber_T *status_out)
 {
   assert(cwd == NULL || os_isdir_executable(cwd));
 
@@ -318,7 +316,9 @@ Channel *channel_job_start(char **argv, CallbackReader on_stdout,
     if (detach) {
       EMSG2(_(e_invarg2), "terminal/pty job cannot be detached");
       shell_free_argv(argv);
-      xfree(term_name);
+      if (env) {
+        tv_dict_free(env);
+      }
       channel_destroy_early(chan);
       *status_out = 0;
       return NULL;
@@ -330,9 +330,6 @@ Channel *channel_job_start(char **argv, CallbackReader on_stdout,
     if (pty_height > 0) {
       chan->stream.pty.height = pty_height;
     }
-    if (term_name) {
-      chan->stream.pty.term_name = term_name;
-    }
   } else {
     chan->stream.uv = libuv_process_init(&main_loop, chan);
   }
@@ -362,9 +359,6 @@ Channel *channel_job_start(char **argv, CallbackReader on_stdout,
     if (proc->env) {
       tv_dict_free(proc->env);
     }
-    if (proc->type == kProcessTypePty) {
-      xfree(chan->stream.pty.term_name);
-    }
     channel_destroy_early(chan);
     *status_out = proc->status;
     return NULL;

src/nvim/eval/funcs.c

@@ -4912,7 +4912,8 @@ static const char *required_env_vars[] = {
 
 static dict_T *create_environment(const dictitem_T *job_env,
                                   const bool clear_env,
-                                  const bool pty)
+                                  const bool pty,
+                                  const char * const pty_term_name)
 {
   dict_T * env = tv_dict_alloc();
 
@@ -4946,6 +4947,18 @@ static dict_T *create_environment(const dictitem_T *job_env,
     }
   }
 
+  // For a pty, we need a sane $TERM set.  We can't rely on nvim's environment,
+  // because the child process is going to be communicating with nvim, not the
+  // parent terminal.  Set a sane default, but let the user override it in the
+  // job's environment if they want.
+  if (pty) {
+    dictitem_T *dv = tv_dict_find(env, S_LEN("TERM"));
+    if (dv) {
+      tv_dict_item_remove(env, dv);
+    }
+    tv_dict_add_str(env, S_LEN("TERM"), pty_term_name);
+  }
+
   if (job_env) {
     tv_dict_extend(env, job_env->di_tv.vval.v_dict, "force");
   }
@@ -5055,20 +5068,25 @@ static void f_jobstart(typval_T *argvars, typval_T *rettv, FunPtr fptr)
     }
   }
 
-  env = create_environment(job_env, clear_env, pty);
-
   uint16_t width = 0, height = 0;
   char *term_name = NULL;
 
   if (pty) {
     width = (uint16_t)tv_dict_get_number(job_opts, "width");
     height = (uint16_t)tv_dict_get_number(job_opts, "height");
-    term_name = tv_dict_get_string(job_opts, "TERM", true);
+    // Legacy method, before env option existed, to specify $TERM.  No longer
+    // documented, but still usable to avoid breaking scripts.
+    term_name = tv_dict_get_string(job_opts, "TERM", false);
+    if (!term_name) {
+      term_name = "ansi";
+    }
   }
 
+  env = create_environment(job_env, clear_env, pty, term_name);
+
   Channel *chan = channel_job_start(argv, on_stdout, on_stderr, on_exit, pty,
                                     rpc, overlapped, detach, cwd, width, height,
-                                    term_name, env, &rettv->vval.v_number);
+                                    env, &rettv->vval.v_number);
   if (chan) {
     channel_create_event(chan, NULL);
   }
@@ -7511,7 +7529,7 @@ static void f_rpcstart(typval_T *argvars, typval_T *rettv, FunPtr fptr)
   Channel *chan = channel_job_start(argv, CALLBACK_READER_INIT,
                                     CALLBACK_READER_INIT, CALLBACK_NONE,
                                     false, true, false, false, NULL, 0, 0,
-                                    NULL, NULL, &rettv->vval.v_number);
+                                    NULL, &rettv->vval.v_number);
   if (chan) {
     channel_create_event(chan, NULL);
   }
@@ -10618,7 +10636,7 @@ static void f_termopen(typval_T *argvars, typval_T *rettv, FunPtr fptr)
     }
   }
 
-  env = create_environment(job_env, clear_env, pty);
+  env = create_environment(job_env, clear_env, pty, "xterm-256color");
 
   const bool rpc = false;
   const bool overlapped = false;
@@ -10627,8 +10645,7 @@ static void f_termopen(typval_T *argvars, typval_T *rettv, FunPtr fptr)
   Channel *chan = channel_job_start(argv, on_stdout, on_stderr, on_exit,
                                     pty, rpc, overlapped, detach, cwd,
                                     term_width, curwin->w_height_inner,
-                                    xstrdup("xterm-256color"), env,
-                                    &rettv->vval.v_number);
+                                    env, &rettv->vval.v_number);
   if (rettv->vval.v_number <= 0) {
     return;
   }

src/nvim/event/process.c

@@ -270,9 +270,6 @@ static void process_close_event(void **argv)
 {
   Process *proc = argv[0];
   shell_free_argv(proc->argv);
-  if (proc->type == kProcessTypePty) {
-    xfree(((PtyProcess *)proc)->term_name);
-  }
   if (proc->cb) {  // "on_exit" for jobstart(). See channel_job_start().
     proc->cb(proc, proc->status, proc->data);
   }

src/nvim/os/pty_process_unix.c

@@ -182,8 +182,6 @@ static void init_child(PtyProcess *ptyproc)
   char *prog = ptyproc->process.argv[0];
 
   assert(proc->env);
-  tv_dict_add_str(proc->env, S_LEN("TERM"),
-                  ptyproc->term_name ? ptyproc->term_name : "ansi");
   environ = tv_dict_to_env(proc->env);
   execvp(prog, proc->argv);
   ELOG("execvp failed: %s: %s", strerror(errno), prog);

src/nvim/os/pty_process_unix.h

@@ -17,7 +17,6 @@ static inline PtyProcess pty_process_init(Loop *loop, void *data)
 {
   PtyProcess rv;
   rv.process = process_init(loop, kProcessTypePty, data);
-  rv.term_name = NULL;
   rv.width = 80;
   rv.height = 24;
   rv.tty_fd = -1;

src/nvim/os/pty_process_win.h

@@ -37,7 +37,6 @@ static inline PtyProcess pty_process_init(Loop *loop, void *data)
 {
   PtyProcess rv;
   rv.process = process_init(loop, kProcessTypePty, data);
-  rv.term_name = NULL;
   rv.width = 80;
   rv.height = 24;
   rv.object.winpty = NULL;