fix(terminal): never propagate $COLORTERM from outer env (#26440)

If $COLORTERM is "truecolor" but the user sets 'notermguicolors',
propagating $COLORTERM to :terminal usually doesn't work well.

4 files changed, 54 insertions, 11 deletions

diff --git a/src/nvim/eval/funcs.c b/src/nvim/eval/funcs.c
index 0054c47678..4029478072 100644
--- a/src/nvim/eval/funcs.c
+++ b/src/nvim/eval/funcs.c
@@ -3898,12 +3898,13 @@ static void f_jobresize(typval_T *argvars, typval_T *rettv, EvalFuncData fptr)
   rettv->vval.v_number = 1;
 }
 
-static const char *ignored_env_vars[] = {
+static const char *pty_ignored_env_vars[] = {
 #ifndef MSWIN
   "COLUMNS",
   "LINES",
   "TERMCAP",
   "COLORFGBG",
+  "COLORTERM",
 #endif
   NULL
 };
@@ -3943,9 +3944,9 @@ static dict_T *create_environment(const dictitem_T *job_env, const bool clear_en
       // child process.  We're removing them here so the user can still decide
       // they want to explicitly set them.
       for (size_t i = 0;
-           i < ARRAY_SIZE(ignored_env_vars) && ignored_env_vars[i];
+           i < ARRAY_SIZE(pty_ignored_env_vars) && pty_ignored_env_vars[i];
            i++) {
-        dictitem_T *dv = tv_dict_find(env, ignored_env_vars[i], -1);
+        dictitem_T *dv = tv_dict_find(env, pty_ignored_env_vars[i], -1);
         if (dv) {
           tv_dict_item_remove(env, dv);
         }
@@ -3953,10 +3954,6 @@ static dict_T *create_environment(const dictitem_T *job_env, const bool clear_en
 #ifndef MSWIN
       // Set COLORTERM to "truecolor" if termguicolors is set
       if (p_tgc) {
-        dictitem_T *dv = tv_dict_find(env, S_LEN("COLORTERM"));
-        if (dv) {
-          tv_dict_item_remove(env, dv);
-        }
         tv_dict_add_str(env, S_LEN("COLORTERM"), "truecolor");
       }
 #endif
diff --git a/test/functional/helpers.lua b/test/functional/helpers.lua
index 7ec67a2cb3..449c3ab9d8 100644
--- a/test/functional/helpers.lua
+++ b/test/functional/helpers.lua
@@ -38,8 +38,6 @@ module.nvim_argv = {
   '--cmd', module.nvim_set,
   -- Remove default mappings.
   '--cmd', 'mapclear | mapclear!',
-  -- Unset $COLORTERM so that it won't propagate to :terminal when 'notermguicolors'.
-  '--cmd', 'unlet $COLORTERM',
   -- Make screentest work after changing to the new default color scheme
   -- Source 'vim' color scheme without side effects
   -- TODO: rewrite tests
diff --git a/test/functional/terminal/buffer_spec.lua b/test/functional/terminal/buffer_spec.lua
index 7a52ee2b13..ece09bca88 100644
--- a/test/functional/terminal/buffer_spec.lua
+++ b/test/functional/terminal/buffer_spec.lua
@@ -1,8 +1,10 @@
 local helpers = require('test.functional.helpers')(after_each)
+local Screen = require('test.functional.ui.screen')
 local thelpers = require('test.functional.terminal.helpers')
 local assert_alive = helpers.assert_alive
 local feed, clear, nvim = helpers.feed, helpers.clear, helpers.nvim
 local poke_eventloop = helpers.poke_eventloop
+local nvim_prog = helpers.nvim_prog
 local eval, feed_command, source = helpers.eval, helpers.feed_command, helpers.source
 local pcall_err = helpers.pcall_err
 local eq, neq = helpers.eq, helpers.neq
@@ -559,4 +561,52 @@ describe('termopen()', function()
     eq("Vim:E11: Invalid in command-line window; <CR> executes, CTRL-C quits",
       pcall_err(funcs.termopen, "bar"))
   end)
+
+  describe('$COLORTERM value', function()
+    if skip(is_os('win'), 'Not applicable for Windows') then return end
+
+    before_each(function()
+      -- Outer value should never be propagated to :terminal
+      funcs.setenv('COLORTERM', 'wrongvalue')
+    end)
+
+    local function test_term_colorterm(expected, opts)
+      local screen = Screen.new(50, 4)
+      screen:attach()
+      funcs.termopen({
+        nvim_prog, '-u', 'NONE', '-i', 'NONE', '--headless',
+        '-c', 'echo $COLORTERM | quit',
+      }, opts)
+      screen:expect(([[
+        ^%s{MATCH:%%s+}|
+        [Process exited 0]                                |
+                                                          |
+                                                          |
+      ]]):format(expected))
+    end
+
+    describe("with 'notermguicolors'", function()
+      before_each(function()
+        command('set notermguicolors')
+      end)
+      it('is empty by default', function()
+        test_term_colorterm('')
+      end)
+      it('can be overridden', function()
+        test_term_colorterm('expectedvalue', { env = { COLORTERM = 'expectedvalue' } })
+      end)
+    end)
+
+    describe("with 'termguicolors'", function()
+      before_each(function()
+        command('set termguicolors')
+      end)
+      it('is "truecolor" by default', function()
+        test_term_colorterm('truecolor')
+      end)
+      it('can be overridden', function()
+        test_term_colorterm('expectedvalue', { env = { COLORTERM = 'expectedvalue' } })
+      end)
+    end)
+  end)
 end)
diff --git a/test/functional/terminal/tui_spec.lua b/test/functional/terminal/tui_spec.lua
index bcd2122cc7..362f1fc1ee 100644
--- a/test/functional/terminal/tui_spec.lua
+++ b/test/functional/terminal/tui_spec.lua
@@ -2213,8 +2213,6 @@ describe("TUI 't_Co' (terminal colors)", function()
 
   local function assert_term_colors(term, colorterm, maxcolors)
     clear({env={TERM=term}, args={}})
-    -- Allow overriding $COLORTERM in :terminal
-    command('set notermguicolors')
     screen = thelpers.setup_child_nvim({
       '-u', 'NONE',
       '-i', 'NONE',