diff --git a/src/history/history.rs b/src/history/history.rs index baf83476e..5de3b9a12 100644 --- a/src/history/history.rs +++ b/src/history/history.rs @@ -295,6 +295,9 @@ enum DeletionScope { struct HistoryImpl { /// The name of this list. Used for picking a suitable filename and for switching modes. name: WString, + /// Optional custom directory for the history file. If None, uses path_get_data(). + /// Primarily for testing. + custom_directory: Option, /// New items. Note that these are NOT discarded on save. We need to keep these around so we can /// distinguish between items in our history and items in the history of other shells that were /// started after we were started. @@ -333,7 +336,11 @@ fn history_file_path(&self) -> std::io::Result> { return Ok(None); } - let Some(mut path) = path_get_data() else { + let mut path = if let Some(custom_dir) = &self.custom_directory { + custom_dir.clone() + } else if let Some(data_path) = path_get_data() { + data_path + } else { return Err(std::io::Error::new( std::io::ErrorKind::NotFound, "Error obtaining data directory. This is a manually constructed error which does not indicate why this happened.", @@ -343,7 +350,11 @@ fn history_file_path(&self) -> std::io::Result> { path.push('/'); path.push_utfstr(&self.name); path.push_utfstr(L!("_history")); - if let Some(canonicalized_path) = wrealpath(&path) { + + // For custom directories, skip wrealpath since file may not exist yet + if self.custom_directory.is_some() { + Ok(Some(path)) + } else if let Some(canonicalized_path) = wrealpath(&path) { Ok(Some(canonicalized_path)) } else { Err(std::io::Error::other(format!( @@ -736,9 +747,10 @@ fn save_unless_disabled(&mut self) { self.save(vacuum); } - fn new(name: WString) -> Self { + fn new(name: WString, custom_directory: Option) -> Self { Self { name, + custom_directory, new_items: vec![], first_unwritten_new_item_index: 0, has_pending_item: false, @@ -1210,18 +1222,26 @@ pub fn add_commandline(&self, s: WString) { imp.add(item, false, true); } - pub fn new(name: &wstr) -> Arc { - Arc::new(Self(Mutex::new(HistoryImpl::new(name.to_owned())))) + /// Creates a new History with a custom directory path. + /// The history file will be stored at `{directory}/{name}_history`. + /// If the directory is None, it will be stored at path_get_data(). + pub fn new(name: &wstr, directory: Option) -> Arc { + Arc::new(Self(Mutex::new(HistoryImpl::new( + name.to_owned(), + directory, + )))) } - /// Returns history with the given name, creating it if necessary. + /// Returns the history with the given name, creating it if necessary, using the default data directory. + /// This uses the HISTORIES global collection. Note it is possible to create a history without + /// placing it into this collection. pub fn with_name(name: &wstr) -> Arc { let mut histories = HISTORIES.lock().unwrap(); if let Some(hist) = histories.get(name) { Arc::clone(hist) } else { - let hist = Self::new(name); + let hist = Self::new(name, None); histories.insert(name.to_owned(), Arc::clone(&hist)); hist } @@ -1786,7 +1806,7 @@ mod tests { History, HistoryItem, HistorySearch, PathList, PersistenceMode, SearchDirection, SearchFlags, SearchType, VACUUM_FREQUENCY, }; - use crate::common::{ESCAPE_TEST_CHAR, ScopeGuard, osstr2wcstring, wcs2bytes, wcs2osstring}; + use crate::common::{ESCAPE_TEST_CHAR, osstr2wcstring, wcs2bytes, wcs2osstring}; use crate::env::{EnvMode, EnvSetMode, EnvStack}; use crate::fs::{LockedFile, WriteMethod}; use crate::path::path_get_data; @@ -1799,8 +1819,7 @@ mod tests { use std::collections::VecDeque; use std::io::BufReader; use std::sync::Arc; - use std::time::UNIX_EPOCH; - use std::time::{Duration, SystemTime}; + use std::time::{Duration, SystemTime, UNIX_EPOCH}; fn history_contains(history: &History, txt: &wstr) -> bool { for i in 1.. { @@ -2010,9 +2029,10 @@ fn generate_history_lines(item_count: usize, idx: usize) -> Vec { result } - fn pound_on_history(item_count: usize, idx: usize) -> Arc { + #[allow(clippy::ref_option)] + fn write_history_entries(dir: &Option, item_count: usize, idx: usize) -> Arc { // Called in child thread to modify history. - let hist = History::new(L!("race_test")); + let hist = History::new(L!("race_test"), dir.clone()); let hist_lines = generate_history_lines(item_count, idx); for line in hist_lines { hist.add_commandline(line); @@ -2022,23 +2042,16 @@ fn pound_on_history(item_count: usize, idx: usize) -> Arc { } #[test] - #[serial] fn test_history_races() { - let _cleanup = test_init(); + // Place history in a temp directory. + let tmpdir = fish_tempfile::new_dir().unwrap(); + let hist_dir = Some(osstr2wcstring(tmpdir.path())); - let tmp_path = std::env::current_dir() - .unwrap() - .join("history-races-test-balloon"); - std::fs::write(&tmp_path, []).unwrap(); - let _cleanup = ScopeGuard::new((), |()| { - std::fs::remove_file(&tmp_path).unwrap(); - }); - if LockedFile::new( - crate::fs::LockingMode::Exclusive(WriteMethod::RenameIntoPlace), - &osstr2wcstring(&tmp_path), - ) - .is_err() - { + // Skip tests if we can't get an exclusive lock on a file in that directory. + let tmp_balloon = tmpdir.path().join("history-races-test-balloon"); + std::fs::write(&tmp_balloon, []).unwrap(); + let mode = crate::fs::LockingMode::Exclusive(WriteMethod::RenameIntoPlace); + if LockedFile::new(mode, &osstr2wcstring(&tmp_balloon)).is_err() { return; } @@ -2052,12 +2065,13 @@ fn test_history_races() { const ITEM_COUNT: usize = 256; // Ensure history is clear. - History::new(L!("race_test")).clear(); + History::new(L!("race_test"), hist_dir.clone()).clear(); let mut children = Vec::with_capacity(RACE_COUNT); for i in 0..RACE_COUNT { + let hist_dir = hist_dir.clone(); children.push(std::thread::spawn(move || { - pound_on_history(ITEM_COUNT, i); + write_history_entries(&hist_dir, ITEM_COUNT, i); })); } @@ -2074,7 +2088,7 @@ fn test_history_races() { time_barrier(); // Ensure that we got sane, sorted results. - let hist = History::new(L!("race_test")); + let hist = History::new(L!("race_test"), hist_dir.clone()); // History is enumerated from most recent to least // Every item should be the last item in some array @@ -2128,25 +2142,26 @@ fn test_history_races() { } #[test] - #[serial] fn test_history_external_rewrites() { - let _cleanup = test_init(); + // Place history in a temp directory. + let tmpdir = fish_tempfile::new_dir().unwrap(); + let hist_dir = Some(osstr2wcstring(tmpdir.path())); // Write some history to disk. { - let hist = pound_on_history(VACUUM_FREQUENCY / 2, 0); + let hist = write_history_entries(&hist_dir, VACUUM_FREQUENCY / 2, 0); hist.add_commandline("needle".into()); hist.save(); } std::thread::sleep(Duration::from_secs(1)); // Read history from disk. - let hist = History::new(L!("race_test")); + let hist = History::new(L!("race_test"), hist_dir.clone()); assert_eq!(hist.item_at_index(1).unwrap().str(), "needle"); // Add items until we rewrite the file. // In practice this might be done by another shell. - pound_on_history(VACUUM_FREQUENCY, 0); + write_history_entries(&hist_dir, VACUUM_FREQUENCY, 0); for i in 1.. { if hist.item_at_index(i).unwrap().str() == "needle" { @@ -2156,16 +2171,21 @@ fn test_history_external_rewrites() { } #[test] - #[serial] fn test_history_merge() { - let _cleanup = test_init(); + let tmpdir = fish_tempfile::new_dir().unwrap(); + let hist_dir = Some(osstr2wcstring(tmpdir.path())); + // In a single fish process, only one history is allowed to exist with the given name But it's // common to have multiple history instances with the same name active in different processes, // e.g. when you have multiple shells open. We try to get that right and merge all their history // together. Test that case. const COUNT: usize = 3; let name = L!("merge_test"); - let hists = [History::new(name), History::new(name), History::new(name)]; + let hists = [ + History::new(name, hist_dir.clone()), + History::new(name, hist_dir.clone()), + History::new(name, hist_dir.clone()), + ]; let texts = [L!("History 1"), L!("History 2"), L!("History 3")]; let alt_texts = [ L!("History Alt 1"), @@ -2204,7 +2224,7 @@ fn test_history_merge() { // Make a new history. It should contain everything. The time_barrier() is so that the timestamp // is newer, since we only pick up items whose timestamp is before the birth stamp. time_barrier(); - let everything = History::new(name); + let everything = History::new(name, hist_dir.clone()); for text in texts { assert!(history_contains(&everything, text)); }