Clean up how PATH and CDPATH munging occurs

PATH and CDPATH have special behavior around empty elements. Express this
directly in env_stack_t::set rather than via variable dispatch; this is
cleaner.

src/env.cpp

@@ -133,27 +133,6 @@ static export_generation_t next_export_generation() {
     return ++*val;
 }
 
-/// Some env vars contain a list of paths where an empty path element is equivalent to ".".
-/// Unfortunately that convention causes problems for fish scripts. So this function replaces the
-/// empty path element with an explicit ".". See issue #3914.
-void fix_colon_delimited_var(const wcstring &var_name, env_stack_t &vars) {
-    const auto paths = vars.get(var_name);
-    if (paths.missing_or_empty()) return;
-
-    // See if there's any empties.
-    const wcstring empty = wcstring();
-    const wcstring_list_t &strs = paths->as_list();
-    if (contains(strs, empty)) {
-        // Copy the list and replace empties with L"."
-        wcstring_list_t newstrs = strs;
-        std::replace(newstrs.begin(), newstrs.end(), empty, wcstring(L"."));
-        int retval = vars.set(var_name, ENV_DEFAULT | ENV_USER, std::move(newstrs));
-        if (retval != ENV_OK) {
-            FLOGF(error, L"fix_colon_delimited_var failed unexpectedly with retval %d", retval);
-        }
-    }
-}
-
 const wcstring_list_t &env_var_t::as_list() const { return *vals_; }
 
 wchar_t env_var_t::get_delimiter() const {
@@ -231,14 +210,6 @@ void misc_init() {
     }
 }
 
-/// Ensure the content of the magic path env vars is reasonable. Specifically, that empty path
-/// elements are converted to explicit "." to make the vars easier to use in fish scripts.
-static void init_path_vars() {
-    // Do not replace empties in MATHPATH - see #4158.
-    fix_colon_delimited_var(L"PATH", env_stack_t::globals());
-    fix_colon_delimited_var(L"CDPATH", env_stack_t::globals());
-}
-
 /// Make sure the PATH variable contains something.
 static void setup_path() {
     auto &vars = env_stack_t::globals();
@@ -297,8 +268,6 @@ void env_init(const struct config_paths_t *paths /* or NULL */) {
     path_get_data(user_data_dir);
     vars.set_one(FISH_USER_DATA_DIR, ENV_GLOBAL, user_data_dir);
 
-    init_path_vars();
-
     // Set up the USER and PATH variables
     setup_path();
 
@@ -1268,6 +1237,15 @@ int env_stack_t::set(const wcstring &key, env_mode_flags_t mode, wcstring_list_t
         path_make_canonical(vals.front());
     }
 
+    // Hacky stuff around PATH and CDPATH: #3914.
+    // Not MANPATH; see #4158.
+    // Replace empties with dot. Note we ignore pathvar here.
+    if (key == L"PATH" || key == L"CDPATH") {
+        auto munged_vals = colon_split(vals);
+        std::replace(munged_vals.begin(), munged_vals.end(), wcstring(L""), wcstring(L"."));
+        vals = std::move(munged_vals);
+    }
+
     bool needs_uvar_sync = false;
     auto ret = acquire_impl()->set(key, mode, std::move(vals), &needs_uvar_sync);
     if (ret == ENV_OK) {

src/env.h

@@ -322,9 +322,6 @@ bool term_supports_setting_title();
 /// Gets a path appropriate for runtime storage
 wcstring env_get_runtime_path();
 
-/// Replace empty path elements with "." - see #3914.
-void fix_colon_delimited_var(const wcstring &var_name, env_stack_t &vars);
-
 /// A wrapper around setenv() and unsetenv() which use a lock.
 /// In general setenv() and getenv() are highly incompatible with threads. This makes it only
 /// slightly safer.

src/env_dispatch.cpp

@@ -252,10 +252,6 @@ static void handle_tz_change(const wcstring &var_name, env_stack_t &vars) {
     handle_timezone(var_name.c_str(), vars);
 }
 
-static void handle_magic_colon_var_change(const wcstring &var_name, env_stack_t &vars) {
-    fix_colon_delimited_var(var_name, vars);
-}
-
 static void handle_locale_change(const environment_t &vars) {
     init_locale(vars);
     // We need to re-guess emoji width because the locale might have changed to a multibyte one.
@@ -301,8 +297,6 @@ static std::unique_ptr<const var_dispatch_table_t> create_dispatch_table() {
         var_dispatch_table->add(var_name, handle_curses_change);
     }
 
-    var_dispatch_table->add(L"PATH", handle_magic_colon_var_change);
-    var_dispatch_table->add(L"CDPATH", handle_magic_colon_var_change);
     var_dispatch_table->add(L"fish_term256", handle_fish_term_change);
     var_dispatch_table->add(L"fish_term24bit", handle_fish_term_change);
     var_dispatch_table->add(L"fish_escape_delay_ms", update_wait_on_escape_ms);

tests/checks/colon-delimited-var.fish

@@ -0,0 +1,26 @@
+# RUN: env PATH="a::b" CDPATH="d::e" MANPATH="x::y" %fish %s
+
+# In PATH and CDPATH, empty elements are treated the same as "."
+# In fish we replace them explicitly. Ensure that works.
+# Do not replace empties in MATHPATH - see #4158.
+
+echo "$PATH"
+# CHECK: a:.:b
+
+echo "$CDPATH"
+# CHECK: d:.:e
+
+echo "$MANPATH"
+# CHECK: x::y
+
+set PATH abc '' def
+echo "$PATH"
+# CHECK: abc:.:def
+
+set CDPATH '' qqq
+echo "$CDPATH"
+# CHECK: .:qqq
+
+set MANPATH 123 '' 456
+echo "$MANPATH"
+# CHECK: 123::456