From 72bf5898d36ea40a77d66a1970b875e6fb4e4b0c Mon Sep 17 00:00:00 2001 From: ridiculousfish Date: Sat, 2 Nov 2019 16:46:33 -0700 Subject: [PATCH] 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 | 40 ++++++--------------------- src/env.h | 3 -- src/env_dispatch.cpp | 6 ---- tests/checks/colon-delimited-var.fish | 26 +++++++++++++++++ 4 files changed, 35 insertions(+), 40 deletions(-) create mode 100644 tests/checks/colon-delimited-var.fish diff --git a/src/env.cpp b/src/env.cpp index 38b590fa1..064eef09d 100644 --- a/src/env.cpp +++ b/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) { diff --git a/src/env.h b/src/env.h index 873bfeaa6..a14b0dcdb 100644 --- a/src/env.h +++ b/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. diff --git a/src/env_dispatch.cpp b/src/env_dispatch.cpp index 424b44bad..2e91b4d94 100644 --- a/src/env_dispatch.cpp +++ b/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 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); diff --git a/tests/checks/colon-delimited-var.fish b/tests/checks/colon-delimited-var.fish new file mode 100644 index 000000000..8eeaac956 --- /dev/null +++ b/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