diff --git a/CHANGELOG.md b/CHANGELOG.md index 1571552f9..c96ff1d88 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -26,6 +26,7 @@ - Fix setting `$COLUMNS` and `$LINES` before first prompt is displayed (#4141). - `read` failures due to too much data should define the var (#4180). - multiple `read` commands in non-interactive scripts were broken in fish 2.6.0 (#4206). +- Fix regression involving universal variables with side-effects at startup such as `set -U fish_escape_delay_ms 10` (#4196). - Added completions for: - `as` (#4130). - `jest` (#4142). diff --git a/src/env.cpp b/src/env.cpp index b839300aa..9a82b044c 100644 --- a/src/env.cpp +++ b/src/env.cpp @@ -588,7 +588,8 @@ static bool variable_is_colon_delimited_var(const wcstring &str) { /// React to modifying the given variable. static void react_to_variable_change(const wcstring &key) { // Don't do any of this until `env_init()` has run. We only want to do this in response to - // variables set by the user; e.g., in a script like *config.fish* or interactively. + // variables set by the user; e.g., in a script like *config.fish* or interactively or as part + // of loading the universal variables for the first time. if (!env_initialized) return; if (var_is_locale(key)) { @@ -753,6 +754,26 @@ void misc_init() { #endif // OS_IS_MS_WINDOWS } +static void env_universal_callbacks(callback_data_list_t &callbacks) { + for (size_t i = 0; i < callbacks.size(); i++) { + const callback_data_t &data = callbacks.at(i); + universal_callback(data.type, data.key.c_str()); + } +} + +void env_universal_barrier() { + ASSERT_IS_MAIN_THREAD(); + if (!uvars()) return; + + callback_data_list_t callbacks; + bool changed = uvars()->sync(callbacks); + if (changed) { + universal_notifier_t::default_notifier().post_notification(); + } + + env_universal_callbacks(callbacks); +} + void env_init(const struct config_paths_t *paths /* or NULL */) { // These variables can not be altered directly by the user. const wchar_t *const ro_keys[] = { @@ -869,11 +890,6 @@ void env_init(const struct config_paths_t *paths /* or NULL */) { env_set_termsize(); // initialize the terminal size variables env_set_read_limit(); // initialize the read_byte_limit - // Set up universal variables. The empty string means to use the deafult path. - assert(s_universal_variables == NULL); - s_universal_variables = new env_universal_t(L""); - s_universal_variables->load(); - // Set g_use_posix_spawn. Default to true. env_var_t use_posix_spawn = env_get_string(L"fish_use_posix_spawn"); g_use_posix_spawn = @@ -882,11 +898,23 @@ void env_init(const struct config_paths_t *paths /* or NULL */) { // Set fish_bind_mode to "default". env_set(FISH_BIND_MODE_VAR, DEFAULT_BIND_MODE, ENV_GLOBAL); + // This is somewhat subtle. At this point we consider our environment to be sufficiently + // initialized that we can react to changes to variables. Prior to doing this we expect that the + // code for setting vars that might have side-effects will do whatever + // `react_to_variable_change()` would do for that var. + env_initialized = true; + + // Set up universal variables. The empty string means to use the default path. + assert(s_universal_variables == NULL); + s_universal_variables = new env_universal_t(L""); + callback_data_list_t callbacks; + s_universal_variables->load(callbacks); + env_universal_callbacks(callbacks); + // Now that the global scope is fully initialized, add a toplevel local scope. This same local // scope will persist throughout the lifetime of the fish process, and it will ensure that `set // -l` commands run at the command-line don't affect the global scope. env_push(false); - env_initialized = true; } /// Search all visible scopes in order for the specified key. Return the first scope in which it was @@ -1482,23 +1510,6 @@ env_vars_snapshot_t::env_vars_snapshot_t(const wchar_t *const *keys) { } } -void env_universal_barrier() { - ASSERT_IS_MAIN_THREAD(); - if (uvars()) { - callback_data_list_t changes; - bool changed = uvars()->sync(&changes); - if (changed) { - universal_notifier_t::default_notifier().post_notification(); - } - - // Post callbacks. - for (size_t i = 0; i < changes.size(); i++) { - const callback_data_t &data = changes.at(i); - universal_callback(data.type, data.key.c_str()); - } - } -} - env_vars_snapshot_t::env_vars_snapshot_t() {} // The "current" variables are not a snapshot at all, but instead trampoline to env_get_string, etc. diff --git a/src/env_universal_common.cpp b/src/env_universal_common.cpp index 9c9268162..b813ffc77 100644 --- a/src/env_universal_common.cpp +++ b/src/env_universal_common.cpp @@ -346,9 +346,7 @@ wcstring_list_t env_universal_t::get_names(bool show_exported, bool show_unexpor // Given a variable table, generate callbacks representing the difference between our vars and the // new vars. void env_universal_t::generate_callbacks(const var_table_t &new_vars, - callback_data_list_t *callbacks) const { - assert(callbacks != NULL); - + callback_data_list_t &callbacks) const { // Construct callbacks for erased values. for (var_table_t::const_iterator iter = this->vars.begin(); iter != this->vars.end(); ++iter) { const wcstring &key = iter->first; @@ -360,7 +358,7 @@ void env_universal_t::generate_callbacks(const var_table_t &new_vars, // If the value is not present in new_vars, it has been erased. if (new_vars.find(key) == new_vars.end()) { - callbacks->push_back(callback_data_t(ERASE, key, L"")); + callbacks.push_back(callback_data_t(ERASE, key, L"")); } } @@ -379,7 +377,7 @@ void env_universal_t::generate_callbacks(const var_table_t &new_vars, if (existing == this->vars.end() || existing->second.exportv != new_entry.exportv || existing->second.val != new_entry.val) { // Value has changed. - callbacks->push_back( + callbacks.push_back( callback_data_t(new_entry.exportv ? SET_EXPORT : SET, key, new_entry.val)); } } @@ -408,7 +406,7 @@ void env_universal_t::acquire_variables(var_table_t *vars_to_acquire) { this->vars = std::move(*vars_to_acquire); } -void env_universal_t::load_from_fd(int fd, callback_data_list_t *callbacks) { +void env_universal_t::load_from_fd(int fd, callback_data_list_t &callbacks) { ASSERT_IS_LOCKED(lock); assert(fd >= 0); // Get the dev / inode. @@ -420,9 +418,7 @@ void env_universal_t::load_from_fd(int fd, callback_data_list_t *callbacks) { var_table_t new_vars = this->read_message_internal(fd); // Announce changes. - if (callbacks != NULL) { - this->generate_callbacks(new_vars, callbacks); - } + this->generate_callbacks(new_vars, callbacks); // Acquire the new variables. this->acquire_variables(&new_vars); @@ -430,7 +426,7 @@ void env_universal_t::load_from_fd(int fd, callback_data_list_t *callbacks) { } } -bool env_universal_t::load_from_path(const wcstring &path, callback_data_list_t *callbacks) { +bool env_universal_t::load_from_path(const wcstring &path, callback_data_list_t &callbacks) { ASSERT_IS_LOCKED(lock); // Check to see if the file is unchanged. We do this again in load_from_fd, but this avoids @@ -507,12 +503,11 @@ bool env_universal_t::move_new_vars_file_into_place(const wcstring &src, const w return ret == 0; } -bool env_universal_t::load() { - callback_data_list_t callbacks; +bool env_universal_t::load(callback_data_list_t &callbacks) { const wcstring vars_path = explicit_vars_path.empty() ? default_vars_path() : explicit_vars_path; scoped_lock locker(lock); - bool success = load_from_path(vars_path, &callbacks); + bool success = load_from_path(vars_path, callbacks); if (!success && !tried_renaming && errno == ENOENT) { // We failed to load, because the file was not found. Older fish used the hostname only. Try // moving the filename based on the hostname into place; if that succeeds try again. @@ -523,10 +518,11 @@ bool env_universal_t::load() { const wcstring hostname_path = wdirname(vars_path) + L'/' + hostname_id; if (0 == wrename(hostname_path, vars_path)) { // We renamed - try again. - success = this->load(); + success = this->load(callbacks); } } } + return success; } @@ -645,7 +641,7 @@ bool env_universal_t::open_and_acquire_lock(const wcstring &path, int *out_fd) { // Returns true if modified variables were written, false if not. (There may still be variable // changes due to other processes on a false return). -bool env_universal_t::sync(callback_data_list_t *callbacks) { +bool env_universal_t::sync(callback_data_list_t &callbacks) { debug(5, L"universal log sync"); scoped_lock locker(lock); // Our saving strategy: diff --git a/src/env_universal_common.h b/src/env_universal_common.h index 4eb63dadb..a73a57c43 100644 --- a/src/env_universal_common.h +++ b/src/env_universal_common.h @@ -41,8 +41,8 @@ class env_universal_t { mutable pthread_mutex_t lock; bool tried_renaming; - bool load_from_path(const wcstring &path, callback_data_list_t *callbacks); - void load_from_fd(int fd, callback_data_list_t *callbacks); + 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 &val, bool exportv, bool overwrite); bool remove_internal(const wcstring &name); @@ -58,7 +58,7 @@ class env_universal_t { // Given a variable table, generate callbacks representing the difference between our vars and // the new vars. - void generate_callbacks(const var_table_t &new_vars, callback_data_list_t *callbacks) const; + 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 // vars_to_acquire. @@ -87,11 +87,11 @@ class env_universal_t { wcstring_list_t get_names(bool show_exported, bool show_unexported) const; /// Loads variables at the correct path. - bool load(); + bool load(callback_data_list_t &callbacks); /// Reads and writes variables at the correct path. Returns true if modified variables were /// written. - bool sync(callback_data_list_t *callbacks); + bool sync(callback_data_list_t &callbacks); }; /// The "universal notifier" is an object responsible for broadcasting and receiving universal diff --git a/src/fish_tests.cpp b/src/fish_tests.cpp index 0a51d5ab2..49390d2ba 100644 --- a/src/fish_tests.cpp +++ b/src/fish_tests.cpp @@ -2621,12 +2621,13 @@ static void test_input() { #define UVARS_TEST_PATH L"test/fish_uvars_test/varsfile.txt" static int test_universal_helper(int x) { + callback_data_list_t callbacks; env_universal_t uvars(UVARS_TEST_PATH); 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, val, false); - bool synced = uvars.sync(NULL); + bool synced = uvars.sync(callbacks); if (!synced) { err(L"Failed to sync universal variables after modification"); } @@ -2634,7 +2635,7 @@ static int test_universal_helper(int x) { // Last step is to delete the first key. uvars.remove(format_string(L"key_%d_%d", x, 0)); - bool synced = uvars.sync(NULL); + bool synced = uvars.sync(callbacks); if (!synced) { err(L"Failed to sync universal variables after deletion"); } @@ -2652,7 +2653,8 @@ static void test_universal() { iothread_drain_all(); env_universal_t uvars(UVARS_TEST_PATH); - bool loaded = uvars.load(); + callback_data_list_t callbacks; + bool loaded = uvars.load(callbacks); if (!loaded) { err(L"Failed to load universal variables"); } @@ -2685,6 +2687,8 @@ static bool callback_data_less_than(const callback_data_t &a, const callback_dat static void test_universal_callbacks() { say(L"Testing universal callbacks"); + callback_data_list_t 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); @@ -2698,23 +2702,23 @@ static void test_universal_callbacks() { uvars1.set(L"kappa", L"1", false); uvars1.set(L"omicron", L"1", false); - uvars1.sync(NULL); - uvars2.sync(NULL); + uvars1.sync(callbacks); + uvars2.sync(callbacks); // Change uvars1. uvars1.set(L"alpha", L"2", false); // changes value uvars1.set(L"beta", L"1", true); // changes export uvars1.remove(L"delta"); // erases value uvars1.set(L"epsilon", L"1", false); // changes nothing - uvars1.sync(NULL); + uvars1.sync(callbacks); // Change uvars2. It should treat its value as correct and ignore changes from uvars1. uvars2.set(L"lambda", L"1", false); // same value uvars2.set(L"kappa", L"2", false); // different value // Now see what uvars2 sees. - callback_data_list_t callbacks; - uvars2.sync(&callbacks); + callbacks.clear(); + uvars2.sync(callbacks); // Sort them to get them in a predictable order. std::sort(callbacks.begin(), callbacks.end(), callback_data_less_than); diff --git a/src/input_common.cpp b/src/input_common.cpp index 08a86534f..3ccb2324b 100644 --- a/src/input_common.cpp +++ b/src/input_common.cpp @@ -60,7 +60,6 @@ static int (*interrupt_handler)(); void input_common_init(int (*ih)()) { interrupt_handler = ih; - update_wait_on_escape_ms(); } void input_common_destroy() {}