diff --git a/src/builtin_set.cpp b/src/builtin_set.cpp index cad1034ae..810451033 100644 --- a/src/builtin_set.cpp +++ b/src/builtin_set.cpp @@ -296,7 +296,7 @@ static int my_env_set(const wchar_t *cmd, const wchar_t *key, const wcstring_lis // We don't check `val->empty()` because an array var with a single empty string will be // "empty". A truly empty array var is set to the special value `ENV_NULL`. auto val = list_to_array_val(list); - retval = env_set(key, *val == ENV_NULL ? NULL : val->c_str(), scope | ENV_USER); + retval = env_set(key, val->c_str(), scope | ENV_USER); switch (retval) { case ENV_OK: { retval = STATUS_CMD_OK; diff --git a/src/env.cpp b/src/env.cpp index 736a73433..63389d4ed 100644 --- a/src/env.cpp +++ b/src/env.cpp @@ -126,8 +126,8 @@ class env_node_t { /// Pointer to next level. std::unique_ptr next; - /// Returns a pointer to the given entry if present, or NULL. - const var_entry_t *find_entry(const wcstring &key); + /// Returns the given entry if present else env_var_t::missing_var. + const env_var_t find_entry(const wcstring &key); }; class variable_entry_t { @@ -174,27 +174,21 @@ struct var_stack_t { env_node_t *next_scope_to_search(env_node_t *node); const env_node_t *next_scope_to_search(const env_node_t *node) const; - // Returns the scope used for unspecified scopes - // An unspecified scope is either the topmost shadowing scope, or the global scope if none - // This implements the default behavior of 'set' + // Returns the scope used for unspecified scopes. An unspecified scope is either the topmost + // shadowing scope, or the global scope if none. This implements the default behavior of `set`. env_node_t *resolve_unspecified_scope(); }; void var_stack_t::push(bool new_scope) { std::unique_ptr node(new env_node_t(new_scope)); - // Copy local-exported variables + // Copy local-exported variables. auto top_node = top.get(); - // Only if we introduce a new shadowing scope - // i.e. not if it's just `begin; end` or "--no-scope-shadowing". - if (new_scope) { - if (!(top_node == this->global_env)) { - for (auto &var : top_node->env) { - if (var.second.exportv) { - // This should copy var - node->env.insert(var); - } - } + // Only if we introduce a new shadowing scope; i.e. not if it's just `begin; end` or + // "--no-scope-shadowing". + if (new_scope && top_node != this->global_env) { + for (auto &var : top_node->env) { + if (var.second.exportv) node->env.insert(var); } } @@ -240,8 +234,8 @@ void var_stack_t::pop() { assert(this->top != NULL); for (const auto &entry_pair : old_top->env) { - const var_entry_t &entry = entry_pair.second; - if (entry.exportv) { + const env_var_t &var = entry_pair.second; + if (var.exportv) { this->mark_changed_exported(); break; } @@ -309,13 +303,10 @@ static bool is_electric(const wcstring &key) { return env_electric.find(key.c_str()) != env_electric.end(); } -const var_entry_t *env_node_t::find_entry(const wcstring &key) { - const var_entry_t *result = NULL; - var_table_t::const_iterator where = env.find(key); - if (where != env.end()) { - result = &where->second; - } - return result; +const env_var_t env_node_t::find_entry(const wcstring &key) { + var_table_t::const_iterator entry = env.find(key); + if (entry != env.end()) return entry->second; + return env_var_t::missing_var(); } /// Return the current umask value. @@ -959,10 +950,8 @@ void env_init(const struct config_paths_t *paths /* or NULL */) { static env_node_t *env_get_node(const wcstring &key) { env_node_t *env = vars_stack().top.get(); while (env != NULL) { - if (env->find_entry(key) != NULL) { - break; - } - + env_var_t var = env->find_entry(key); + if (!var.missing()) break; env = vars_stack().next_scope_to_search(env); } return env; @@ -1059,8 +1048,8 @@ int env_set(const wcstring &key, const wchar_t *val, env_mode_flags_t var_mode) if (preexisting_node != NULL) { var_table_t::const_iterator result = preexisting_node->env.find(key); assert(result != preexisting_node->env.end()); - const var_entry_t &entry = result->second; - if (entry.exportv) { + const env_var_t &var = result->second; + if (var.exportv) { preexisting_entry_exportv = true; has_changed_new = true; } @@ -1074,7 +1063,7 @@ int env_set(const wcstring &key, const wchar_t *val, env_mode_flags_t var_mode) } else if (preexisting_node != NULL) { node = preexisting_node; if ((var_mode & (ENV_EXPORT | ENV_UNEXPORT)) == 0) { - // use existing entry's exportv + // Use existing entry's exportv status. var_mode = //!OCLINT(parameter reassignment) preexisting_entry_exportv ? ENV_EXPORT : 0; } @@ -1108,20 +1097,21 @@ int env_set(const wcstring &key, const wchar_t *val, env_mode_flags_t var_mode) if (!done) { // Set the entry in the node. Note that operator[] accesses the existing entry, or // creates a new one. - var_entry_t &entry = node->env[key]; - if (entry.exportv) { + env_var_t &var = node->env[key]; + if (var.exportv) { // This variable already existed, and was exported. has_changed_new = true; } - entry.val = val; + + var.set_val(val); if (var_mode & ENV_EXPORT) { // The new variable is exported. - entry.exportv = true; + var.exportv = true; node->exportv = true; has_changed_new = true; } else { - entry.exportv = false; - // Set the node's exportv when it changes something about exports + var.exportv = false; + // Set the node's exported when it changes something about exports // (also when it redefines a variable to not be exported). node->exportv = has_changed_old != has_changed_new; } @@ -1219,7 +1209,7 @@ int env_remove(const wcstring &key, int var_mode) { } wcstring env_var_t::as_string(void) const { - assert(!is_missing); + //assert(!is_missing); return val; } @@ -1276,12 +1266,15 @@ env_var_t env_get(const wcstring &key, env_mode_flags_t mode) { env_node_t *env = search_local ? vars_stack().top.get() : vars_stack().global_env; while (env != NULL) { - const var_entry_t *entry = env->find_entry(key); - if (entry != NULL && (entry->exportv ? search_exported : search_unexported)) { - if (entry->val == ENV_NULL) { - return env_var_t::missing_var(); - } - return entry->val; + const env_var_t var = env->find_entry(key); + if (!var.missing() && (var.exportv ? search_exported : search_unexported)) { + // The following statement is wrong because ENV_NULL is supposed to mean the var is + // set but has zero elements. Therefore we should not return missing_var. However, + // If this is changed to return `var` without the special-case then unit tests fail. + // Note that tokenize_variable_array() explicitly handles a var whose string + // representation contains only ENV_NULL. + if (var.as_string() == ENV_NULL) return env_var_t::missing_var(); + return var; } if (has_scope) { @@ -1342,8 +1335,8 @@ bool env_exist(const wchar_t *key, env_mode_flags_t mode) { var_table_t::const_iterator result = env->env.find(key); if (result != env->env.end()) { - const var_entry_t &res = result->second; - return res.exportv ? test_exported : test_unexported; + const env_var_t var = result->second; + return var.exportv ? test_exported : test_unexported; } env = vars_stack().next_scope_to_search(env); } @@ -1385,9 +1378,9 @@ static void add_key_to_string_set(const var_table_t &envs, std::set *s bool show_exported, bool show_unexported) { var_table_t::const_iterator iter; for (iter = envs.begin(); iter != envs.end(); ++iter) { - const var_entry_t &e = iter->second; + const env_var_t &var = iter->second; - if ((e.exportv && show_exported) || (!e.exportv && show_unexported)) { + if ((var.exportv && show_exported) || (!var.exportv && show_unexported)) { // Insert this key. str_set->insert(iter->first); } @@ -1440,39 +1433,39 @@ wcstring_list_t env_get_names(int flags) { } /// Get list of all exported variables. -static void get_exported(const env_node_t *n, std::map *h) { +static void get_exported(const env_node_t *n, var_table_t &h) { if (!n) return; - if (n->new_scope) + if (n->new_scope) { get_exported(vars_stack().global_env, h); - else + } else { get_exported(n->next.get(), h); + } var_table_t::const_iterator iter; for (iter = n->env.begin(); iter != n->env.end(); ++iter) { const wcstring &key = iter->first; - const var_entry_t &val_entry = iter->second; + const env_var_t var = iter->second; - if (val_entry.exportv && val_entry.val != ENV_NULL) { + if (!var.missing() && var.exportv) { // Export the variable. Don't use std::map::insert here, since we need to overwrite // existing values from previous scopes. - (*h)[key] = val_entry.val; + h[key] = var; } else { // We need to erase from the map if we are not exporting, since a lower scope may have // exported. See #2132. - h->erase(key); + h.erase(key); } } } // Given a map from key to value, add values to out of the form key=value. -static void export_func(const std::map &envs, std::vector &out) { +static void export_func(const var_table_t &envs, std::vector &out) { out.reserve(out.size() + envs.size()); - std::map::const_iterator iter; - for (iter = envs.begin(); iter != envs.end(); ++iter) { + for (auto iter = envs.begin(); iter != envs.end(); ++iter) { const wcstring &key = iter->first; const std::string &ks = wcs2string(key); - std::string vs = wcs2string(iter->second); + std::string vs = wcs2string(iter->second.as_string()); // Arrays in the value are ASCII record separator (0x1e) delimited. But some variables // should have colons. Add those. @@ -1497,11 +1490,10 @@ void var_stack_t::update_export_array_if_necessary() { if (!this->has_changed_exported) { return; } - std::map vals; debug(4, L"env_export_arr() recalc"); - - get_exported(this->top.get(), &vals); + var_table_t vals; + get_exported(this->top.get(), vals); if (uvars()) { const wcstring_list_t uni = uvars()->get_names(true, false); @@ -1544,7 +1536,7 @@ void env_set_argv(const wchar_t *const *argv) { env_set(L"argv", sb.c_str(), ENV_LOCAL); } else { - env_set(L"argv", 0, ENV_LOCAL); + env_set(L"argv", NULL, ENV_LOCAL); } } diff --git a/src/env.h b/src/env.h index b5c8131c5..aee366f6c 100644 --- a/src/env.h +++ b/src/env.h @@ -75,16 +75,19 @@ class env_var_t { bool is_missing; public: + bool exportv; // whether the variable should be exported + static env_var_t missing_var() { env_var_t result((wcstring())); result.is_missing = true; + result.exportv = false; return result; } - env_var_t(const env_var_t &x) : val(x.val), is_missing(x.is_missing) {} - env_var_t(const wcstring &x) : val(x), is_missing(false) {} - env_var_t(const wchar_t *x) : val(x), is_missing(false) {} - env_var_t() : val(L""), is_missing(false) {} + env_var_t(const env_var_t &x) : val(x.val), is_missing(x.is_missing), exportv(x.exportv) {} + env_var_t(const wcstring &x) : val(x), is_missing(false), exportv(false) {} + env_var_t(const wchar_t *x) : val(x), is_missing(false), exportv(false) {} + env_var_t() : val(L""), is_missing(false), exportv(false) {} bool empty(void) const { return val.empty(); }; bool missing(void) const { return is_missing; } @@ -96,10 +99,14 @@ class env_var_t { env_var_t &operator=(const env_var_t &v) { is_missing = v.is_missing; + exportv = v.exportv; val = v.val; return *this; } + void set_val(const wcstring &s) { val = s; is_missing = false; } + void set_val(const wchar_t *s) { val = s; is_missing = false; } + bool operator==(const env_var_t &s) const { return is_missing == s.is_missing && val == s.val; } bool operator==(const wcstring &s) const { return !is_missing && val == s; } @@ -192,15 +199,7 @@ class env_vars_snapshot_t { extern int g_fork_count; extern bool g_use_posix_spawn; -/// A variable entry. Stores the value of a variable and whether it should be exported. -struct var_entry_t { - wcstring val; // the value of the variable - bool exportv; // whether the variable should be exported - - var_entry_t() : exportv(false) {} -}; - -typedef std::map var_table_t; +typedef std::map var_table_t; extern bool term_has_xn; // does the terminal have the "eat_newline_glitch" diff --git a/src/env_universal_common.cpp b/src/env_universal_common.cpp index 00228113f..19a087ee1 100644 --- a/src/env_universal_common.cpp +++ b/src/env_universal_common.cpp @@ -273,7 +273,7 @@ env_var_t env_universal_t::get(const wcstring &name) const { env_var_t result = env_var_t::missing_var(); var_table_t::const_iterator where = vars.find(name); if (where != vars.end()) { - result = env_var_t(where->second.val); + result = where->second; } return result; } @@ -295,10 +295,10 @@ void env_universal_t::set_internal(const wcstring &key, const wcstring &val, boo return; } - var_entry_t *entry = &vars[key]; - if (entry->exportv != exportv || entry->val != val) { - entry->val = val; - entry->exportv = exportv; + env_var_t &entry = vars[key]; + if (entry.exportv != exportv || entry.as_string() != val) { + entry.set_val(val); + entry.exportv = exportv; // If we are overwriting, then this is now modified. if (overwrite) { @@ -332,8 +332,8 @@ wcstring_list_t env_universal_t::get_names(bool show_exported, bool show_unexpor var_table_t::const_iterator iter; for (iter = vars.begin(); iter != vars.end(); ++iter) { const wcstring &key = iter->first; - const var_entry_t &e = iter->second; - if ((e.exportv && show_exported) || (!e.exportv && show_unexported)) { + const env_var_t &var = iter->second; + if ((var.exportv && show_exported) || (!var.exportv && show_unexported)) { result.push_back(key); } } @@ -369,18 +369,18 @@ void env_universal_t::generate_callbacks(const var_table_t &new_vars, } // See if the value has changed. - const var_entry_t &new_entry = iter->second; + const env_var_t &new_entry = iter->second; var_table_t::const_iterator existing = this->vars.find(key); if (existing == this->vars.end() || existing->second.exportv != new_entry.exportv || - existing->second.val != new_entry.val) { + existing->second != new_entry) { // Value has changed. callbacks.push_back( - callback_data_t(new_entry.exportv ? SET_EXPORT : SET, key, new_entry.val)); + callback_data_t(new_entry.exportv ? SET_EXPORT : SET, key, new_entry.as_string())); } } } -void env_universal_t::acquire_variables(var_table_t *vars_to_acquire) { +void env_universal_t::acquire_variables(var_table_t &vars_to_acquire) { // Copy modified values from existing vars to vars_to_acquire. for (std::set::iterator iter = this->modified.begin(); iter != this->modified.end(); ++iter) { @@ -388,19 +388,19 @@ void env_universal_t::acquire_variables(var_table_t *vars_to_acquire) { var_table_t::iterator src_iter = this->vars.find(key); if (src_iter == this->vars.end()) { /* The value has been deleted. */ - vars_to_acquire->erase(key); + vars_to_acquire.erase(key); } else { // The value has been modified. Copy it over. Note we can destructively modify the // source entry in vars since we are about to get rid of this->vars entirely. - var_entry_t &src = src_iter->second; - var_entry_t &dst = (*vars_to_acquire)[key]; - dst.val = std::move(src.val); + env_var_t &src = src_iter->second; + env_var_t &dst = vars_to_acquire[key]; + dst.set_val(src.as_string()); dst.exportv = src.exportv; } } // We have constructed all the callbacks and updated vars_to_acquire. Acquire it! - this->vars = std::move(*vars_to_acquire); + this->vars = std::move(vars_to_acquire); } void env_universal_t::load_from_fd(int fd, callback_data_list_t &callbacks) { @@ -418,7 +418,7 @@ void env_universal_t::load_from_fd(int fd, callback_data_list_t &callbacks) { this->generate_callbacks(new_vars, callbacks); // Acquire the new variables. - this->acquire_variables(&new_vars); + this->acquire_variables(new_vars); last_read_file = current_file; } } @@ -464,8 +464,8 @@ bool env_universal_t::write_to_fd(int fd, const wcstring &path) { // Append the entry. Note that append_file_entry may fail, but that only affects one // variable; soldier on. const wcstring &key = iter->first; - const var_entry_t &entry = iter->second; - append_file_entry(entry.exportv ? SET_EXPORT : SET, key, entry.val, &contents, &storage); + const env_var_t &var = iter->second; + append_file_entry(var.exportv ? SET_EXPORT : SET, key, var.as_string(), &contents, &storage); // Go to next. ++iter; @@ -845,9 +845,9 @@ void env_universal_t::parse_message_internal(const wcstring &msgstr, var_table_t wcstring val; if (unescape_string(tmp + 1, &val, 0)) { - var_entry_t &entry = (*vars)[key]; + env_var_t &entry = (*vars)[key]; entry.exportv = exportv; - entry.val = std::move(val); // acquire the value + entry.set_val(val); // acquire the value } } else { debug(1, PARSE_ERR, msg); diff --git a/src/env_universal_common.h b/src/env_universal_common.h index a73a57c43..bca672ac7 100644 --- a/src/env_universal_common.h +++ b/src/env_universal_common.h @@ -60,9 +60,9 @@ class env_universal_t { // the new vars. void generate_callbacks(const var_table_t &new_vars, callback_data_list_t &callbacks) const; - // Given a variable table, copy unmodified values into self. May destructively modified + // Given a variable table, copy unmodified values into self. May destructively modify // vars_to_acquire. - void acquire_variables(var_table_t *vars_to_acquire); + void acquire_variables(var_table_t &vars_to_acquire); static void parse_message_internal(const wcstring &msg, var_table_t *vars, wcstring *storage); static var_table_t read_message_internal(int fd); diff --git a/src/fish_tests.cpp b/src/fish_tests.cpp index 09e57e091..9c539d96e 100644 --- a/src/fish_tests.cpp +++ b/src/fish_tests.cpp @@ -215,6 +215,7 @@ static void popd() { /// Test that the fish functions for converting strings to numbers work. static void test_str_to_num() { + say(L"Testing str_to_num"); const wchar_t *end; int i; long l; @@ -4368,6 +4369,9 @@ int main(int argc, char **argv) { // Set default signal handlers, so we can ctrl-C out of this. signal_reset_handlers(); + if (should_test_function("utility_functions")) test_utility_functions(); + if (should_test_function("wcstring_tok")) test_wcstring_tok(); + if (should_test_function("env_vars")) test_env_vars(); if (should_test_function("str_to_num")) test_str_to_num(); if (should_test_function("highlighting")) test_highlighting(); if (should_test_function("new_parser_ll2")) test_new_parser_ll2(); @@ -4409,15 +4413,12 @@ int main(int argc, char **argv) { if (should_test_function("autosuggestion_ignores")) test_autosuggestion_ignores(); if (should_test_function("autosuggestion_combining")) test_autosuggestion_combining(); if (should_test_function("autosuggest_suggest_special")) test_autosuggest_suggest_special(); - if (should_test_function("wcstring_tok")) test_wcstring_tok(); if (should_test_function("history")) history_tests_t::test_history(); if (should_test_function("history_merge")) history_tests_t::test_history_merge(); if (should_test_function("history_races")) history_tests_t::test_history_races(); if (should_test_function("history_formats")) history_tests_t::test_history_formats(); if (should_test_function("string")) test_string(); - if (should_test_function("env_vars")) test_env_vars(); if (should_test_function("illegal_command_exit_code")) test_illegal_command_exit_code(); - if (should_test_function("utility_functions")) test_utility_functions(); // history_tests_t::test_history_speed(); say(L"Encountered %d errors in low-level tests", err_count); diff --git a/src/highlight.cpp b/src/highlight.cpp index 2a64336f9..aefafa436 100644 --- a/src/highlight.cpp +++ b/src/highlight.cpp @@ -279,7 +279,6 @@ rgb_color_t highlight_get_color(highlight_spec_t highlight, bool is_background) // Handle modifiers. if (highlight & highlight_modifier_valid_path) { env_var_t var2 = env_get(L"fish_color_valid_path"); - const wcstring val2 = var2.missing() ? L"" : var2.c_str(); rgb_color_t result2 = parse_color(var2, is_background); if (result.is_normal()) @@ -1014,8 +1013,9 @@ static bool command_is_valid(const wcstring &cmd, enum parse_statement_decoratio if (!is_valid && command_ok) is_valid = path_get_path(cmd, NULL, vars); // Implicit cd - if (!is_valid && implicit_cd_ok) + if (!is_valid && implicit_cd_ok) { is_valid = path_can_be_implicit_cd(cmd, NULL, working_directory.c_str(), vars); + } // Return what we got. return is_valid; diff --git a/src/path.cpp b/src/path.cpp index bee6ed8b1..e2c7216af 100644 --- a/src/path.cpp +++ b/src/path.cpp @@ -48,20 +48,18 @@ static bool path_get_path_core(const wcstring &cmd, wcstring *out_path, } int err = ENOENT; - wcstring bin_path; + wcstring_list_t pathsv; if (!bin_path_var.missing()) { - bin_path = bin_path_var.as_string(); + bin_path_var.to_list(pathsv); } else { // Note that PREFIX is defined in the `Makefile` and is thus defined when this module is // compiled. This ensures we always default to "/bin", "/usr/bin" and the bin dir defined // for the fish programs. Possibly with a duplicate dir if PREFIX is empty, "/", "/usr" or // "/usr/". If the PREFIX duplicates /bin or /usr/bin that is harmless other than a trivial // amount of time testing a path we've already tested. - bin_path = *list_to_array_val(wcstring_list_t({L"/bin", L"/usr/bin", PREFIX L"/bin"})); + pathsv = wcstring_list_t({L"/bin", L"/usr/bin", PREFIX L"/bin"}); } - wcstring_list_t pathsv; - bin_path_var.to_list(pathsv); for (auto next_path : pathsv) { if (next_path.empty()) continue; append_path_component(next_path, cmd); diff --git a/tests/read.err b/tests/read.err index 676c6e2d1..d76028bc0 100644 --- a/tests/read.err +++ b/tests/read.err @@ -8,6 +8,9 @@ read: Expected at least 1 args, got only 0 read: Expected 1 args, got 0 read: Expected 1 args, got 2 +#################### +# Verify correct behavior of subcommands and splitting of input. + #################### # Test splitting input diff --git a/tests/read.in b/tests/read.in index 36cbcc98e..cd5bfa07e 100644 --- a/tests/read.in +++ b/tests/read.in @@ -3,8 +3,6 @@ # Test read builtin and IFS. # -########### -# Start by testing that invocation errors are handled correctly. logmsg Read with no vars is an error read @@ -13,19 +11,19 @@ read -a read -a v1 v2 read -a v1 -########### -# Verify correct behavior of subcommands and splitting of input. -count (echo one\ntwo) -set -l IFS \t -count (echo one\ntwo) -set -l IFS -count (echo one\ntwo) -echo [(echo -n one\ntwo)] -count (echo one\ntwo\n) -echo [(echo -n one\ntwo\n)] -count (echo one\ntwo\n\n) -echo [(echo -n one\ntwo\n\n)] -set -le IFS +logmsg Verify correct behavior of subcommands and splitting of input. +begin + count (echo one\ntwo) + set -l IFS \t + count (echo one\ntwo) + set -l IFS + count (echo one\ntwo) + echo [(echo -n one\ntwo)] + count (echo one\ntwo\n) + echo [(echo -n one\ntwo\n)] + count (echo one\ntwo\n\n) + echo [(echo -n one\ntwo\n\n)] +end function print_vars --no-scope-shadowing set -l space diff --git a/tests/read.out b/tests/read.out index 4b495bdc3..eb1c23a6d 100644 --- a/tests/read.out +++ b/tests/read.out @@ -4,6 +4,9 @@ #################### # Read with -a and anything other than exactly on var name is an error + +#################### +# Verify correct behavior of subcommands and splitting of input. 2 2 1