From 421cf9238073b1e1aee0b048dc36a759ee3161be Mon Sep 17 00:00:00 2001 From: ridiculousfish Date: Sun, 9 Jun 2019 13:48:07 -0700 Subject: [PATCH] Use a generation count for uvars exports Because an exported universal variable must be exported in all variable stacks, explicit invalidation is infeasible. Switch the universal variables to a generation count. --- src/env.cpp | 11 ++---- src/env_universal_common.cpp | 66 ++++++++++++++++++++---------------- src/env_universal_common.h | 15 +++++--- 3 files changed, 50 insertions(+), 42 deletions(-) diff --git a/src/env.cpp b/src/env.cpp index 0a6479aa0..d5798f4b9 100644 --- a/src/env.cpp +++ b/src/env.cpp @@ -565,6 +565,9 @@ class env_scoped_impl_t : public environment_t { /// Invoke a function on the current (nonzero) export generations, in order. template void enumerate_generations(const Func &func) const { + // Our uvars generation count doesn't come from next_export_generation(), so always supply + // it even if it's 0. + func(uvars() ? uvars()->get_export_generation() : 0); if (globals_->exports()) func(globals_->export_gen); for (auto node = locals_; node; node = node->next) { if (node->exports()) func(node->export_gen); @@ -1085,10 +1088,6 @@ void env_stack_impl_t::set_universal(const wcstring &key, wcstring_list_t val, env_var_t new_var{val, varflags}; uvars()->set(key, new_var); - if (new_var.exports() || (oldvar && oldvar->exports())) { - // TODO: we need to use a generation count here, other parsers care about this. - export_array_.reset(); - } } int env_stack_impl_t::set(const wcstring &key, env_mode_flags_t mode, wcstring_list_t val, @@ -1160,10 +1159,6 @@ int env_stack_impl_t::remove(const wcstring &key, int mode, bool *out_needs_uvar if (!uvars()) return false; auto flags = uvars()->get_flags(key); if (!flags) return false; - if (*flags & env_var_t::flag_export) { - // TODO: need to use a generation count here. - export_array_.reset(); - } *out_needs_uvar_sync = true; return uvars()->remove(key); }; diff --git a/src/env_universal_common.cpp b/src/env_universal_common.cpp index 15721e327..4bad356ed 100644 --- a/src/env_universal_common.cpp +++ b/src/env_universal_common.cpp @@ -265,36 +265,31 @@ maybe_t env_universal_t::get_flags(const wcstring &n return none(); } -void env_universal_t::set_internal(const wcstring &key, const env_var_t &var, bool overwrite) { +void env_universal_t::set_internal(const wcstring &key, const env_var_t &var) { ASSERT_IS_LOCKED(lock); - if (!overwrite && this->modified.find(key) != this->modified.end()) { - // This value has been modified and we're not overwriting it. Skip it. - return; - } - env_var_t &entry = vars[key]; if (entry != var) { entry = var; - - // If we are overwriting, then this is now modified. - if (overwrite) { - this->modified.insert(key); - } + this->modified.insert(key); + if (entry.exports()) export_generation += 1; } } void env_universal_t::set(const wcstring &key, env_var_t var) { scoped_lock locker(lock); - this->set_internal(key, std::move(var), true /* overwrite */); + this->set_internal(key, std::move(var)); } bool env_universal_t::remove_internal(const wcstring &key) { ASSERT_IS_LOCKED(lock); - size_t erased = this->vars.erase(key); - if (erased > 0) { + auto iter = this->vars.find(key); + if (iter != this->vars.end()) { + if (iter->second.exports()) export_generation += 1; + this->vars.erase(iter); this->modified.insert(key); + return true; } - return erased > 0; + return false; } bool env_universal_t::remove(const wcstring &key) { @@ -317,27 +312,27 @@ 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 { +// new vars. Update our exports generation. +void env_universal_t::generate_callbacks_and_update_exports(const var_table_t &new_vars, + callback_data_list_t &callbacks) { // 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; - + for (const auto &kv : this->vars) { + const wcstring &key = kv.first; // Skip modified values. - if (this->modified.find(key) != this->modified.end()) { + if (this->modified.count(key)) { continue; } // If the value is not present in new_vars, it has been erased. - if (new_vars.find(key) == new_vars.end()) { + if (new_vars.count(key) == 0) { callbacks.push_back(callback_data_t(key, none())); + if (kv.second.exports()) export_generation += 1; } } // Construct callbacks for newly inserted or changed values. - for (var_table_t::const_iterator iter = new_vars.begin(); iter != new_vars.end(); ++iter) { - const wcstring &key = iter->first; + for (const auto &kv : new_vars) { + const wcstring &key = kv.first; // Skip modified values. if (this->modified.find(key) != this->modified.end()) { @@ -345,10 +340,15 @@ void env_universal_t::generate_callbacks(const var_table_t &new_vars, } // See if the value has changed. - const env_var_t &new_entry = iter->second; + const env_var_t &new_entry = kv.second; var_table_t::const_iterator existing = this->vars.find(key); - if (existing == this->vars.end() || existing->second.exports() != new_entry.exports() || - existing->second != new_entry) { + + bool old_exports = (existing != this->vars.end() && existing->second.exports()); + bool export_changed = (old_exports != new_entry.exports()); + if (export_changed) { + export_generation += 1; + } + if (existing == this->vars.end() || export_changed || existing->second != new_entry) { // Value has changed. callbacks.push_back(callback_data_t(key, new_entry.as_string())); } @@ -393,8 +393,8 @@ void env_universal_t::load_from_fd(int fd, callback_data_list_t &callbacks) { ok_to_save = false; } - // Announce changes. - this->generate_callbacks(new_vars, callbacks); + // Announce changes and update our exports generation. + this->generate_callbacks_and_update_exports(new_vars, callbacks); // Acquire the new variables. this->acquire_variables(new_vars); @@ -427,6 +427,12 @@ bool env_universal_t::load_from_path(const wcstring &path, callback_data_list_t std::string tmp = wcs2string(path); return load_from_path(tmp, callbacks); } + +uint64_t env_universal_t::get_export_generation() const { + scoped_lock locker(lock); + return export_generation; +} + /// Serialize the contents to a string. std::string env_universal_t::serialize_with_vars(const var_table_t &vars) { std::string storage; diff --git a/src/env_universal_common.h b/src/env_universal_common.h index 471c7dbba..c9553edfc 100644 --- a/src/env_universal_common.h +++ b/src/env_universal_common.h @@ -28,7 +28,7 @@ struct callback_data_t { bool is_erase() const { return !val.has_value(); } }; -typedef std::vector callback_data_list_t; +typedef std::vector callback_data_list_t; // List of fish universal variable formats. // This is exposed for testing. @@ -49,6 +49,9 @@ class env_universal_t { // Path that we save to. If empty, use the default. wcstring explicit_vars_path; + // A generation count which is incremented every time an exported variable is modified. + uint64_t export_generation{1}; + // Whether it's OK to save. This may be set to false if we discover that a future version of // fish wrote the uvars contents. bool ok_to_save{true}; @@ -58,7 +61,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 env_var_t &var, bool overwrite); + void set_internal(const wcstring &key, const env_var_t &var); bool remove_internal(const wcstring &name); // Functions concerned with saving. @@ -71,8 +74,9 @@ class env_universal_t { file_id_t last_read_file = kInvalidFileID; // 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; + // the new vars. Also update our exports generation count as necessary. + void generate_callbacks_and_update_exports(const var_table_t &new_vars, + callback_data_list_t &callbacks); // Given a variable table, copy unmodified values into self. May destructively modify // vars_to_acquire. @@ -127,6 +131,9 @@ class env_universal_t { /// Exposed for testing only. bool is_ok_to_save() const { return ok_to_save; } + + /// Access the export generation. + uint64_t get_export_generation() const; }; /// The "universal notifier" is an object responsible for broadcasting and receiving universal