diff --git a/src/env/environment.rs b/src/env/environment.rs index dee847260..dcfd11261 100644 --- a/src/env/environment.rs +++ b/src/env/environment.rs @@ -8,7 +8,6 @@ use crate::common::{str2wcstring, unescape_string, wcs2zstring, UnescapeStringStyle}; use crate::env::{EnvMode, EnvVar, Statuses}; use crate::env_dispatch::{env_dispatch_init, env_dispatch_var_change}; -use crate::env_universal_common::CallbackDataList; use crate::event::Event; use crate::flog::FLOG; use crate::global_safety::RelaxedAtomicBool; @@ -351,22 +350,23 @@ pub fn universal_sync(&self, always: bool) -> Vec { } UVARS_LOCALLY_MODIFIED.store(false); - let mut callbacks = CallbackDataList::new(); - let changed = uvars().sync(&mut callbacks); + let (changed, callbacks) = uvars().sync(); if changed { default_notifier().post_notification(); } // React internally to changes to special variables like LANG, and populate on-variable events. let mut result = Vec::new(); - for callback in callbacks { - let name = callback.key; - env_dispatch_var_change(&name, self); - let evt = if callback.val.is_none() { - Event::variable_erase(name) - } else { - Event::variable_set(name) - }; - result.push(evt); + if let Some(callbacks) = callbacks { + for callback in callbacks { + let name = callback.key; + env_dispatch_var_change(&name, self); + let evt = if callback.val.is_none() { + Event::variable_erase(name) + } else { + Event::variable_set(name) + }; + result.push(evt); + } } result } @@ -792,8 +792,7 @@ pub fn env_init(paths: Option<&ConfigPaths>, do_uvars: bool, default_paths: bool UVAR_SCOPE_IS_GLOBAL.store(true); } else { // Set up universal variables using the default path. - let mut callbacks = CallbackDataList::new(); - uvars().initialize(&mut callbacks); + let callbacks = uvars().initialize().unwrap_or_default(); for callback in callbacks { env_dispatch_var_change(&callback.key, vars); } diff --git a/src/env_universal_common.rs b/src/env_universal_common.rs index b241591d6..cc41338b7 100644 --- a/src/env_universal_common.rs +++ b/src/env_universal_common.rs @@ -154,34 +154,32 @@ pub fn get_table(&self) -> &VarTable { /// Initialize this uvars for the default path. /// This should be called at most once on any given instance. - pub fn initialize(&mut self, callbacks: &mut CallbackDataList) { + pub fn initialize(&mut self) -> Option { // Set do_flock to false immediately if the default variable path is on a remote filesystem. // See #7968. if path_get_config_remoteness() == DirRemoteness::remote { self.do_flock = false; } - self.initialize_at_path(callbacks, default_vars_path()); + self.initialize_at_path(default_vars_path()) } /// Initialize a this uvars for a given path. /// This is exposed for testing only. - pub fn initialize_at_path(&mut self, callbacks: &mut CallbackDataList, path: WString) { + pub fn initialize_at_path(&mut self, path: WString) -> Option { if path.is_empty() { - return; + return None; } assert!(!self.initialized(), "Already initialized"); self.vars_path = path; - if self.load_from_path(callbacks) { - // Successfully loaded from our normal path. - } + self.load_from_path() } /// Reads and writes variables at the correct path. Returns true if modified variables were /// written. - pub fn sync(&mut self, callbacks: &mut CallbackDataList) -> bool { + pub fn sync(&mut self) -> (bool, Option) { if !self.initialized() { - return false; + return (false, None); } FLOG!(uvar_file, "universal log sync"); @@ -216,9 +214,9 @@ pub fn sync(&mut self, callbacks: &mut CallbackDataList) -> bool { // with fire anyways. // If we have no changes, just load. if self.modified.is_empty() { - self.load_from_path_narrow(callbacks); + let callbacks = self.load_from_path_narrow(); FLOG!(uvar_file, "universal log no modifications"); - return false; + return (false, callbacks); } let directory = wdirname(&self.vars_path).to_owned(); @@ -228,16 +226,16 @@ pub fn sync(&mut self, callbacks: &mut CallbackDataList) -> bool { // Open the file. let Some(mut vars_file) = self.open_and_acquire_lock() else { FLOG!(uvar_file, "universal log open_and_acquire_lock() failed"); - return false; + return (false, None); }; // Read from it. - self.load_from_file(&mut vars_file, callbacks); + let callbacks = self.load_from_file(&mut vars_file); if self.ok_to_save { - self.save(&directory) + (self.save(&directory), callbacks) } else { - true + (true, callbacks) } } @@ -364,38 +362,38 @@ fn initialized(&self) -> bool { !self.vars_path.is_empty() } - fn load_from_path(&mut self, callbacks: &mut CallbackDataList) -> bool { + fn load_from_path(&mut self) -> Option { self.narrow_vars_path = wcs2zstring(&self.vars_path); - self.load_from_path_narrow(callbacks) + self.load_from_path_narrow() } - fn load_from_path_narrow(&mut self, callbacks: &mut CallbackDataList) -> bool { + fn load_from_path_narrow(&mut self) -> Option { // Check to see if the file is unchanged. We do this again in load_from_file, but this avoids // opening the file unnecessarily. if self.last_read_file_id != INVALID_FILE_ID && file_id_for_path_narrow(&self.narrow_vars_path) == self.last_read_file_id { FLOG!(uvar_file, "universal log sync elided based on fast stat()"); - return true; + return None; } let Ok(mut file) = open_cloexec(&self.narrow_vars_path, OFlag::O_RDONLY, Mode::empty()) else { - return false; + return None; }; FLOG!(uvar_file, "universal log reading from file"); - self.load_from_file(&mut file, callbacks); - true + self.load_from_file(&mut file) } // Load environment variables from the opened [`File`] `file`. It must be mutable because we // will read from the underlying fd. - fn load_from_file(&mut self, file: &mut File, callbacks: &mut CallbackDataList) { + fn load_from_file(&mut self, file: &mut File) -> Option { // Get the dev / inode. let current_file_id = file_id_for_file(file); if current_file_id == self.last_read_file_id { FLOG!(uvar_file, "universal log sync elided based on fstat()"); + None } else { // Read a variables table from the file. let mut new_vars = VarTable::new(); @@ -408,11 +406,12 @@ fn load_from_file(&mut self, file: &mut File, callbacks: &mut CallbackDataList) } // Announce changes and update our exports generation. - self.generate_callbacks_and_update_exports(&new_vars, callbacks); + let callbacks = self.generate_callbacks_and_update_exports(&new_vars); // Acquire the new variables. self.acquire_variables(new_vars); self.last_read_file_id = current_file_id; + Some(callbacks) } } @@ -568,11 +567,8 @@ fn move_new_vars_file_into_place(&mut self, src: &wstr, dst: &wstr) -> bool { // Given a variable table, generate callbacks representing the difference between our vars and // the new vars. Also update our exports generation count as necessary. - fn generate_callbacks_and_update_exports( - &mut self, - new_vars: &VarTable, - callbacks: &mut CallbackDataList, - ) { + fn generate_callbacks_and_update_exports(&mut self, new_vars: &VarTable) -> CallbackDataList { + let mut callbacks = CallbackDataList::new(); // Construct callbacks for erased values. for (key, value) in &self.vars { // Skip modified values. @@ -616,6 +612,7 @@ fn generate_callbacks_and_update_exports( }); } } + callbacks } // Given a variable table, copy unmodified values into self. @@ -628,8 +625,7 @@ fn acquire_variables(&mut self, mut vars_to_acquire: VarTable) { vars_to_acquire.remove(key); } Some(src) => { - // 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. + // The value has been modified. Copy it over. vars_to_acquire.insert(key.clone(), src.clone()); } } diff --git a/src/tests/env_universal_common.rs b/src/tests/env_universal_common.rs index 71341cefd..e01acd275 100644 --- a/src/tests/env_universal_common.rs +++ b/src/tests/env_universal_common.rs @@ -3,7 +3,7 @@ use crate::common::ScopeGuard; use crate::common::ENCODE_DIRECT_BASE; use crate::env::{EnvVar, EnvVarFlags, VarTable}; -use crate::env_universal_common::{CallbackDataList, EnvUniversal, UvarFormat}; +use crate::env_universal_common::{EnvUniversal, UvarFormat}; use crate::reader::{reader_pop, reader_push, ReaderConfig}; use crate::tests::prelude::*; use crate::threads::{iothread_drain_all, iothread_perform}; @@ -15,15 +15,14 @@ fn test_universal_helper(x: usize) { let _cleanup = test_init(); - let mut callbacks = CallbackDataList::new(); let mut uvars = EnvUniversal::new(); - uvars.initialize_at_path(&mut callbacks, UVARS_TEST_PATH.to_owned()); + uvars.initialize_at_path(UVARS_TEST_PATH.to_owned()); for j in 0..UVARS_PER_THREAD { let key = sprintf!("key_%d_%d", x, j); let val = sprintf!("val_%d_%d", x, j); uvars.set(&key, EnvVar::new(val, EnvVarFlags::empty())); - let synced = uvars.sync(&mut callbacks); + let (synced, _) = uvars.sync(); assert!( synced, "Failed to sync universal variables after modification" @@ -32,7 +31,7 @@ fn test_universal_helper(x: usize) { // Last step is to delete the first key. uvars.remove(&sprintf!("key_%d_%d", x, 0)); - let synced = uvars.sync(&mut callbacks); + let (synced, _) = uvars.sync(); assert!(synced, "Failed to sync universal variables after deletion"); } @@ -54,8 +53,7 @@ fn test_universal() { iothread_drain_all(&mut reader); let mut uvars = EnvUniversal::new(); - let mut callbacks = CallbackDataList::new(); - uvars.initialize_at_path(&mut callbacks, UVARS_TEST_PATH.to_owned()); + uvars.initialize_at_path(UVARS_TEST_PATH.to_owned()); for i in 0..threads { for j in 0..UVARS_PER_THREAD { @@ -219,11 +217,25 @@ fn test_universal_parsing_legacy() { fn test_universal_callbacks() { let _cleanup = test_init(); std::fs::create_dir_all("test/fish_uvars_test/").unwrap(); - let mut callbacks = CallbackDataList::new(); let mut uvars1 = EnvUniversal::new(); let mut uvars2 = EnvUniversal::new(); - uvars1.initialize_at_path(&mut callbacks, UVARS_TEST_PATH.to_owned()); - uvars2.initialize_at_path(&mut callbacks, UVARS_TEST_PATH.to_owned()); + let mut callbacks = uvars1 + .initialize_at_path(UVARS_TEST_PATH.to_owned()) + .unwrap_or_default(); + callbacks.append( + &mut uvars2 + .initialize_at_path(UVARS_TEST_PATH.to_owned()) + .unwrap_or_default(), + ); + + macro_rules! sync { + ($uvars:expr) => { + let (_, cb_opt) = $uvars.sync(); + if let Some(mut cb) = cb_opt { + callbacks.append(&mut cb); + } + }; + } let noflags = EnvVarFlags::empty(); @@ -236,8 +248,8 @@ fn test_universal_callbacks() { uvars1.set(L!("kappa"), EnvVar::new(L!("1").to_owned(), noflags)); // uvars1.set(L!("omicron"), EnvVar::new(L!("1").to_owned(), noflags)); // - uvars1.sync(&mut callbacks); - uvars2.sync(&mut callbacks); + sync!(uvars1); + sync!(uvars2); // Change uvars1. uvars1.set(L!("alpha"), EnvVar::new(L!("2").to_owned(), noflags)); // changes value @@ -247,7 +259,7 @@ fn test_universal_callbacks() { ); // changes export uvars1.remove(L!("delta")); // erases value uvars1.set(L!("epsilon"), EnvVar::new(L!("1").to_owned(), noflags)); // changes nothing - uvars1.sync(&mut callbacks); + sync!(uvars1); // Change uvars2. It should treat its value as correct and ignore changes from uvars1. uvars2.set(L!("lambda"), EnvVar::new(L!("1").to_owned(), noflags)); // same value @@ -255,7 +267,7 @@ fn test_universal_callbacks() { // Now see what uvars2 sees. callbacks.clear(); - uvars2.sync(&mut callbacks); + sync!(uvars2); // Sort them to get them in a predictable order. callbacks.sort_by(|a, b| a.key.cmp(&b.key)); @@ -308,12 +320,12 @@ fn test_universal_ok_to_save() { "UVARS_TEST_PATH should be readable" ); - let mut cbs = CallbackDataList::new(); let mut uvars = EnvUniversal::new(); - uvars.initialize_at_path(&mut cbs, UVARS_TEST_PATH.to_owned()); + uvars + .initialize_at_path(UVARS_TEST_PATH.to_owned()) + .unwrap_or_default(); assert!(!uvars.is_ok_to_save(), "Should not be OK to save"); - uvars.sync(&mut cbs); - cbs.clear(); + uvars.sync(); assert!(!uvars.is_ok_to_save(), "Should still not be OK to save"); uvars.set( L!("SOMEVAR"),