diff --git a/src/builtin_argparse.cpp b/src/builtin_argparse.cpp index 8ac8fa515..bfaf4ce29 100644 --- a/src/builtin_argparse.cpp +++ b/src/builtin_argparse.cpp @@ -645,7 +645,8 @@ static void set_argparse_result_vars(argparse_cmd_opts_t &opts) { if (!opt_spec->num_seen) continue; if (opt_spec->short_flag_valid) { - env_set(var_name_prefix + opt_spec->short_flag, ENV_LOCAL, opt_spec->vals); + env_set(var_name_prefix + opt_spec->short_flag, ENV_LOCAL, opt_spec->vals, + opt_spec->long_flag.empty()); } if (!opt_spec->long_flag.empty()) { // We do a simple replacement of all non alphanum chars rather than calling diff --git a/src/builtin_set.cpp b/src/builtin_set.cpp index e075ee738..8d900f8b1 100644 --- a/src/builtin_set.cpp +++ b/src/builtin_set.cpp @@ -284,8 +284,8 @@ static int my_env_path_setup(const wchar_t *cmd, const wchar_t *key, //!OCLINT( /// Call env_set. If this is a path variable, e.g. PATH, validate the elements. On error, print a /// description of the problem to stderr. -static int my_env_set(const wchar_t *cmd, const wchar_t *key, int scope, - const wcstring_list_t &list, io_streams_t &streams) { +static int my_env_set(const wchar_t *cmd, const wchar_t *key, int scope, wcstring_list_t &list, + io_streams_t &streams) { int retval; if (is_path_variable(key)) { diff --git a/src/env.cpp b/src/env.cpp index 6a848d288..abbd4b083 100644 --- a/src/env.cpp +++ b/src/env.cpp @@ -1029,17 +1029,18 @@ static env_node_t *env_get_node(const wcstring &key) { return env; } -/// Sets the variable with the specified name to a single value. -int env_set_one(const wcstring &key, env_mode_flags_t mode, const wcstring &val) { - wcstring_list_t vals({ - val, - }); - return env_set(key, mode, vals); -} +static int set_umask(const wcstring *single_val, wcstring_list_t *list_val) { + long mask = -1; + if (single_val && !single_val->empty()) { + mask = fish_wcstol(single_val->c_str(), NULL, 8); + } else if (list_val && list_val->size() == 1 && !(*list_val)[0].empty()) { + mask = fish_wcstol((*list_val)[0].c_str(), NULL, 8); + } -/// Sets the variable with the specified name to no (i.e., zero) values. -int env_set_empty(const wcstring &key, env_mode_flags_t mode) { - return env_set(key, mode, wcstring_list_t()); + if (errno || mask > 0777 || mask < 0) return ENV_INVALID; + // Do not actually create a umask variable. On env_get() it will be calculated. + umask(mask); + return ENV_OK; } /// Set the value of the environment variable whose name matches key to val. @@ -1048,7 +1049,6 @@ int env_set_empty(const wcstring &key, env_mode_flags_t mode) { /// caller afterwards /// /// \param key The key -/// \param val The value /// \param var_mode The type of the variable. Can be any combination of ENV_GLOBAL, ENV_LOCAL, /// ENV_EXPORT and ENV_USER. If mode is ENV_DEFAULT, the current variable space is searched and the /// current mode is used. If no current variable with the same name is found, ENV_LOCAL is assumed. @@ -1061,18 +1061,18 @@ int env_set_empty(const wcstring &key, env_mode_flags_t mode) { /// * ENV_SCOPE, the variable cannot be set in the given scope. This applies to readonly/electric /// variables set from the local or universal scopes, or set as exported. /// * ENV_INVALID, the variable value was invalid. This applies only to special variables. -int env_set(const wcstring &key, env_mode_flags_t var_mode, const wcstring_list_t &vals) { +static int env_set_internal(const wcstring &key, env_mode_flags_t var_mode, bool swap_vals, + const wcstring *single_val, wcstring_list_t *list_val) { ASSERT_IS_MAIN_THREAD(); bool has_changed_old = vars_stack().has_changed_exported; int done = 0; - if (!vals.empty() && (key == L"PWD" || key == L"HOME")) { + if (single_val && !single_val->empty() && (key == L"PWD" || key == L"HOME")) { // Canonicalize our path; if it changes, recurse and try again. - assert(vals.size() == 1); - wcstring val_canonical = vals[0]; + wcstring val_canonical = *single_val; path_make_canonical(val_canonical); - if (vals[0] != val_canonical) { - return env_set_one(key, var_mode, val_canonical); + if (*single_val != val_canonical) { + return env_set_internal(key, var_mode, swap_vals, &val_canonical, nullptr); } } @@ -1087,18 +1087,8 @@ int env_set(const wcstring &key, env_mode_flags_t var_mode, const wcstring_list_ return ENV_PERM; } - if (key == L"umask") { - // Set the new umask. - if (!vals.empty() && vals[0].size()) { - long mask = fish_wcstol(vals[0].c_str(), NULL, 8); - if (!errno && mask <= 0777 && mask >= 0) { - umask(mask); - // Do not actually create a umask variable, on env_get, it will be calculated - // dynamically. - return ENV_OK; - } - } - return ENV_INVALID; + if (key == L"umask") { // set new umask + return set_umask(single_val, list_val); } if (var_mode & ENV_UNIVERSAL) { @@ -1115,7 +1105,13 @@ int env_set(const wcstring &key, env_mode_flags_t var_mode, const wcstring_list_ new_export = old_export; } if (uvars()) { - uvars()->set(key, vals, new_export); + if (list_val) { + uvars()->set(key, *list_val, new_export); + } else { + wcstring_list_t vals; + if (single_val) vals.push_back(*single_val); + uvars()->set(key, vals, new_export); + } env_universal_barrier(); if (old_export || new_export) { vars_stack().mark_changed_exported(); @@ -1164,7 +1160,13 @@ int env_set(const wcstring &key, env_mode_flags_t var_mode, const wcstring_list_ exportv = uvars()->get_export(key); } - uvars()->set(key, vals, exportv); + if (list_val) { + uvars()->set(key, *list_val, exportv); + } else { + wcstring_list_t vals; + if (single_val) vals.push_back(*single_val); + uvars()->set(key, vals, exportv); + } env_universal_barrier(); done = 1; @@ -1184,7 +1186,15 @@ int env_set(const wcstring &key, env_mode_flags_t var_mode, const wcstring_list_ has_changed_new = true; } - var.set_vals(vals); + if (single_val) { + var.set_val(*single_val); + } else if (list_val) { + if (swap_vals) { + var.swap_vals(*list_val); + } else { + var.set_vals(*list_val); + } + } if (var_mode & ENV_EXPORT) { // The new variable is exported. var.exportv = true; @@ -1215,6 +1225,22 @@ int env_set(const wcstring &key, env_mode_flags_t var_mode, const wcstring_list_ return ENV_OK; } +/// Sets the variable with the specified name to the given values. The `vals` will be set to an +/// empty list on return since its content will be swapped into the `env_var_t` that is created. +int env_set(const wcstring &key, env_mode_flags_t mode, wcstring_list_t &vals, bool swap_vals) { + return env_set_internal(key, mode, swap_vals, nullptr, &vals); +} + +/// Sets the variable with the specified name to a single value. +int env_set_one(const wcstring &key, env_mode_flags_t mode, const wcstring &val) { + return env_set_internal(key, mode, false, &val, nullptr); +} + +/// Sets the variable with the specified name without any (i.e., zero) values. +int env_set_empty(const wcstring &key, env_mode_flags_t mode) { + return env_set_internal(key, mode, false, nullptr, nullptr); +} + /// Attempt to remove/free the specified key/value pair from the specified map. /// /// \return zero if the variable was not found, non-zero otherwise diff --git a/src/env.h b/src/env.h index 65e68cf3f..9864f2243 100644 --- a/src/env.h +++ b/src/env.h @@ -116,12 +116,13 @@ class env_var_t { const wcstring get_name() const { return name; } + void set_vals(wcstring_list_t &l) { vals = l; } + void swap_vals(wcstring_list_t &l) { vals.swap(l); } void set_val(const wcstring &s) { vals = { s, }; } - void set_vals(const wcstring_list_t &l) { vals = l; } env_var_t &operator=(const env_var_t &var) { this->vals = var.vals; @@ -156,8 +157,10 @@ wcstring_list_t decode_serialized(const wcstring &s); /// Gets the variable with the specified name, or env_var_t::missing_var if it does not exist. env_var_t env_get(const wcstring &key, env_mode_flags_t mode = ENV_DEFAULT); -/// Sets the variable with the specified name to the given values. -int env_set(const wcstring &key, env_mode_flags_t mode, const wcstring_list_t &vals); +/// Sets the variable with the specified name to the given values. The `vals` will be set to an +/// empty list on return since its content will be swapped into the `env_var_t` that is created. +int env_set(const wcstring &key, env_mode_flags_t mode, wcstring_list_t &vals, + bool swap_vals = true); /// Sets the variable with the specified name to a single value. int env_set_one(const wcstring &key, env_mode_flags_t mode, const wcstring &val); diff --git a/src/env_universal_common.cpp b/src/env_universal_common.cpp index 570c8b08b..83bfb96ea 100644 --- a/src/env_universal_common.cpp +++ b/src/env_universal_common.cpp @@ -280,7 +280,7 @@ bool env_universal_t::get_export(const wcstring &name) const { return result; } -void env_universal_t::set_internal(const wcstring &key, const wcstring_list_t &vals, bool exportv, +void env_universal_t::set_internal(const wcstring &key, wcstring_list_t &vals, bool exportv, bool overwrite) { ASSERT_IS_LOCKED(lock); if (!overwrite && this->modified.find(key) != this->modified.end()) { @@ -290,7 +290,7 @@ void env_universal_t::set_internal(const wcstring &key, const wcstring_list_t &v env_var_t &entry = vars[key]; if (entry.exportv != exportv || entry.as_list() != vals) { - entry.set_vals(vals); + entry.swap_vals(vals); entry.exportv = exportv; // If we are overwriting, then this is now modified. @@ -300,7 +300,7 @@ void env_universal_t::set_internal(const wcstring &key, const wcstring_list_t &v } } -void env_universal_t::set(const wcstring &key, const wcstring_list_t &vals, bool exportv) { +void env_universal_t::set(const wcstring &key, wcstring_list_t &vals, bool exportv) { scoped_lock locker(lock); this->set_internal(key, vals, exportv, true /* overwrite */); } @@ -839,7 +839,7 @@ void env_universal_t::parse_message_internal(const wcstring &msgstr, var_table_t env_var_t &entry = (*vars)[key]; entry.exportv = exportv; wcstring_list_t values = decode_serialized(val); - entry.set_vals(values); + entry.swap_vals(values); } } else { debug(1, PARSE_ERR, msg); diff --git a/src/env_universal_common.h b/src/env_universal_common.h index 2755d3897..d071d5943 100644 --- a/src/env_universal_common.h +++ b/src/env_universal_common.h @@ -44,8 +44,7 @@ class env_universal_t { bool load_from_path(const wcstring &path, callback_data_list_t &callbacks); void load_from_fd(int fd, callback_data_list_t &callbacks); - void set_internal(const wcstring &key, const wcstring_list_t &val, bool exportv, - bool overwrite); + void set_internal(const wcstring &key, wcstring_list_t &val, bool exportv, bool overwrite); bool remove_internal(const wcstring &name); // Functions concerned with saving. @@ -78,7 +77,7 @@ class env_universal_t { bool get_export(const wcstring &name) const; // Sets a variable. - void set(const wcstring &key, const wcstring_list_t &val, bool exportv); + void set(const wcstring &key, wcstring_list_t &val, bool exportv); // Removes a variable. Returns true if it was found, false if not. bool remove(const wcstring &name); diff --git a/src/fish_tests.cpp b/src/fish_tests.cpp index c1a33d7f2..1bb340c99 100644 --- a/src/fish_tests.cpp +++ b/src/fish_tests.cpp @@ -2543,11 +2543,10 @@ static int test_universal_helper(int x) { for (int j = 0; j < UVARS_PER_THREAD; j++) { const wcstring key = format_string(L"key_%d_%d", x, j); const wcstring val = format_string(L"val_%d_%d", x, j); - uvars.set(key, - wcstring_list_t({ + wcstring_list_t vals({ val, - }), - false); + }); + uvars.set(key, vals, false); bool synced = uvars.sync(callbacks); if (!synced) { err(L"Failed to sync universal variables after modification"); @@ -2611,77 +2610,47 @@ static void test_universal_callbacks() { if (system("mkdir -p test/fish_uvars_test/")) err(L"mkdir failed"); env_universal_t uvars1(UVARS_TEST_PATH); env_universal_t uvars2(UVARS_TEST_PATH); + wcstring_list_t vals; // Put some variables into both. - uvars1.set(L"alpha", - wcstring_list_t({ - L"1", - }), - false); - uvars1.set(L"beta", - wcstring_list_t({ - L"1", - }), - false); - uvars1.set(L"delta", - wcstring_list_t({ - L"1", - }), - false); - uvars1.set(L"epsilon", - wcstring_list_t({ - L"1", - }), - false); - uvars1.set(L"lambda", - wcstring_list_t({ - L"1", - }), - false); - uvars1.set(L"kappa", - wcstring_list_t({ - L"1", - }), - false); - uvars1.set(L"omicron", - wcstring_list_t({ - L"1", - }), - false); + vals.push_back(L"a1"); + uvars1.set(L"alpha", vals, false); + assert(vals.size() == 0); + vals.push_back(L"b1"); + uvars1.set(L"beta", vals, false); + vals.push_back(L"d1"); + uvars1.set(L"delta", vals, false); + vals.push_back(L"e1"); + uvars1.set(L"epsilon", vals, false); + vals.push_back(L"l1"); + uvars1.set(L"lambda", vals, false); + vals.push_back(L"k1"); + uvars1.set(L"kappa", vals, false); + vals.push_back(L"o1"); + uvars1.set(L"omicron", vals, false); uvars1.sync(callbacks); uvars2.sync(callbacks); // Change uvars1. - uvars1.set(L"alpha", - wcstring_list_t({ - L"2", - }), - false); // changes value - uvars1.set(L"beta", - wcstring_list_t({ - L"1", - }), - true); // changes export - uvars1.remove(L"delta"); // erases value - uvars1.set(L"epsilon", - wcstring_list_t({ - L"1", - }), - false); // changes nothing + vals.push_back(L"a2"); + uvars1.set(L"alpha", vals, false); // changes value + vals.clear(); // clear the old value + vals.push_back(L"b2"); + uvars1.set(L"beta", vals, true); // changes export + + uvars1.remove(L"delta"); // erases value + + vals.clear(); // clear the old value + vals.push_back(L"e1"); + uvars1.set(L"epsilon", vals, false); // changes nothing uvars1.sync(callbacks); // Change uvars2. It should treat its value as correct and ignore changes from uvars1. - uvars2.set(L"lambda", - wcstring_list_t({ - L"1", - }), - false); // same value - uvars2.set(L"kappa", - wcstring_list_t({ - L"2", - }), - false); // different value + vals.push_back(L"l2"); + uvars2.set(L"lambda", vals, false); // same value + vals.push_back(L"k2"); + uvars2.set(L"kappa", vals, false); // different value // Now see what uvars2 sees. callbacks.clear(); @@ -2694,10 +2663,10 @@ static void test_universal_callbacks() { do_test(callbacks.size() == 3); do_test(callbacks.at(0).type == SET); do_test(callbacks.at(0).key == L"alpha"); - do_test(callbacks.at(0).val == L"2"); + do_test(callbacks.at(0).val == L"a2"); do_test(callbacks.at(1).type == SET_EXPORT); do_test(callbacks.at(1).key == L"beta"); - do_test(callbacks.at(1).val == L"1"); + do_test(callbacks.at(1).val == L"b2"); do_test(callbacks.at(2).type == ERASE); do_test(callbacks.at(2).key == L"delta"); do_test(callbacks.at(2).val == L""); diff --git a/src/function.cpp b/src/function.cpp index e0e028a88..06b295e03 100644 --- a/src/function.cpp +++ b/src/function.cpp @@ -345,6 +345,7 @@ void function_prepare_environment(const wcstring &name, const wchar_t *const *ar // It should be impossible for the var to be missing since we're inheriting it from an outer // scope. So we now die horribly if it is missing. assert(!it->second.missing()); - env_set(it->first, ENV_LOCAL | ENV_USER, it->second.as_const_list()); + wcstring_list_t vals = it->second.as_const_list(); // we need a copy + env_set(it->first, ENV_LOCAL | ENV_USER, vals); // because this mutates the list } }