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

Before #12937, the only way to specify the `$TERM` for a pty job was
through the `TERM` key in the job's opts dict.  This was shuttled to the
child process throug a special field on the PtyProcess object and
injected into the environment after forking.

Now that we have a proper way to specify the environment for a job, we
can simply ensure that the env dict has a proper `TERM` set and avoid
the extra shuttling of data around.

This deprecates the use of the `TERM` option, but will still honor it if
present, although at a lower priority than a `TERM` present in the env
dict.

This also fixes #13874 because we're no longer trying to overwrite
`TERM` in the env dict with the special pty `term_name`.  Doing so
raises an internal error because of the existing key which, under
certain circumstances, would cause the "hit enter" prompt.  However,
since the child process had already forked, there was no way for the
user to acknowledge the prompt and we would just hang there.

7 files changed, 30 insertions, 27 deletions

diff --git a/runtime/doc/eval.txt b/runtime/doc/eval.txt
index 384bdd63a4..0343998fe8 100644
--- a/runtime/doc/eval.txt
+++ b/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
diff --git a/src/nvim/channel.c b/src/nvim/channel.c
index 5628ede2e6..09a34ca9fe 100644
--- a/src/nvim/channel.c
+++ b/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;
diff --git a/src/nvim/eval/funcs.c b/src/nvim/eval/funcs.c
index 1a7bbf2d0e..8788d7ea88 100644
--- a/src/nvim/eval/funcs.c
+++ b/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;
   }
diff --git a/src/nvim/event/process.c b/src/nvim/event/process.c
index 8e9964bd37..b93d6cc0ab 100644
--- a/src/nvim/event/process.c
+++ b/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);
   }
diff --git a/src/nvim/os/pty_process_unix.c b/src/nvim/os/pty_process_unix.c
index 348a139e79..d794969ab5 100644
--- a/src/nvim/os/pty_process_unix.c
+++ b/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);
diff --git a/src/nvim/os/pty_process_unix.h b/src/nvim/os/pty_process_unix.h
index f7c57b3839..8c822eafad 100644
--- a/src/nvim/os/pty_process_unix.h
+++ b/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;
diff --git a/src/nvim/os/pty_process_win.h b/src/nvim/os/pty_process_win.h
index 8ad5ba7286..f8ec79a3d6 100644
--- a/src/nvim/os/pty_process_win.h
+++ b/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;