diff --git a/CHANGELOG.rst b/CHANGELOG.rst index 390582f8d..494f72fe9 100644 --- a/CHANGELOG.rst +++ b/CHANGELOG.rst @@ -20,6 +20,7 @@ Improved terminal support Other improvements ------------------ - History is no longer corrupted with NUL bytes when fish receives SIGTERM or SIGHUP (:issue:`10300`). +- Private mode in-memory history (``set fish_history``) is no longer shared with :doc:`builtin read ` (:issue:`12662`). - :doc:`fish_update_completions ` now handles groff ``\X'...'`` device control escapes, fixing completion generation for man pages produced by help2man 1.50 and later (such as coreutils 9.10). - Improve user experience when removing history entries via the :doc:`web-based config `. - :doc:`funced ` will no longer lose work if there are parse errors multiple times without new changes to the file. diff --git a/src/builtins/history.rs b/src/builtins/history.rs index a69426b37..1888a5aa3 100644 --- a/src/builtins/history.rs +++ b/src/builtins/history.rs @@ -2,7 +2,7 @@ use crate::builtins::error::Error; use crate::history::in_private_mode; -use crate::history::{self, History, history_session_id}; +use crate::history::{self, History, history_id}; use crate::reader::commandline_get_state; use crate::{err_fmt, err_str}; @@ -256,7 +256,7 @@ pub fn history(parser: &Parser, streams: &mut IoStreams, args: &mut [&wstr]) -> // from webconfig.py. let history = commandline_get_state(true) .history - .unwrap_or_else(|| History::new(&history_session_id(parser.vars()))); + .unwrap_or_else(|| History::new(history_id(parser.vars()))); // If a history command hasn't already been specified via a flag check the first word. // Note that this can be simplified after we eliminate allowing subcommands as flags. diff --git a/src/builtins/read.rs b/src/builtins/read.rs index dbdb7f421..53af17ba6 100644 --- a/src/builtins/read.rs +++ b/src/builtins/read.rs @@ -6,6 +6,7 @@ common::valid_var_name, env::{EnvMode, EnvVar, EnvVarFlags, Environment as _, READ_BYTE_LIMIT}, err_fmt, err_str, + history::{HistoryId, MemoryHistoryId}, input_common::{DecodeState, InvalidPolicy, decode_utf8}, nix::isatty, parse_execution::varname_error, @@ -261,7 +262,11 @@ fn read_interactive( let old_modes = set_shell_modes_temporarily(inputfd); // Keep in-memory history only. - reader_push(parser, L!(""), conf); + reader_push( + parser, + HistoryId::Memory(MemoryHistoryId::BuiltinRead), + conf, + ); let _modifiable_commandline = parser.scope().readonly_commandline.then(|| { parser.push_scope(|s| { s.readonly_commandline = false; diff --git a/src/builtins/set.rs b/src/builtins/set.rs index f518db6f4..59ee37312 100644 --- a/src/builtins/set.rs +++ b/src/builtins/set.rs @@ -6,7 +6,7 @@ err_fmt, err_str, event::{self, Event}, expand::{expand_escape_string, expand_escape_variable}, - history::{History, history_session_id}, + history::{History, history_id}, parse_execution::varname_error, parser::ParserEnvSetMode, wutil::wcstoi::wcstoi_partial, @@ -567,7 +567,7 @@ fn list(opts: &Options, parser: &Parser, streams: &mut IoStreams) -> BuiltinResu if !names_only { let mut val = WString::new(); if opts.shorten_ok && key == "history" { - let history = History::new(&history_session_id(parser.vars())); + let history = History::new(history_id(parser.vars())); for i in 1..history.size() { if val.len() >= 64 { break; diff --git a/src/complete.rs b/src/complete.rs index 87ad762b6..500ebf3f7 100644 --- a/src/complete.rs +++ b/src/complete.rs @@ -12,7 +12,7 @@ }, flog::{flog, flogf}, function, - history::{History, history_session_id}, + history::{History, history_id}, localization::{LocalizableString, localizable_string}, operation_context::OperationContext, parse_constants::SourceRange, @@ -1675,7 +1675,7 @@ fn complete_variable(&mut self, s: &wstr, start_offset: usize) -> bool { // $history can be huge, don't put all of it in the completion description; see // #6288. if env_name == "history" { - let history = History::new(&history_session_id(self.ctx.vars())); + let history = History::new(history_id(self.ctx.vars())); for i in 1..std::cmp::min(history.size(), 64) { if i > 1 { desc.push(' '); diff --git a/src/env/environment_impl.rs b/src/env/environment_impl.rs index d0635b0ac..3ba1d2ad7 100644 --- a/src/env/environment_impl.rs +++ b/src/env/environment_impl.rs @@ -5,7 +5,7 @@ use crate::env_universal_common::EnvUniversal; use crate::flog::flog; use crate::global_safety::RelaxedAtomicBool; -use crate::history::{History, history_session_id_from_var}; +use crate::history::{History, history_id_from_var}; use crate::kill::kill_entries; use crate::null_terminated_array::OwningNullTerminatedArray; use crate::portable_atomic::AtomicU64; @@ -373,8 +373,8 @@ fn try_get_computed(&self, key: &wstr) -> Option { } let history = commandline_get_state(true).history.unwrap_or_else(|| { let fish_history_var = self.getf(L!("fish_history"), EnvMode::default()); - let session_id = history_session_id_from_var(fish_history_var); - History::new(&session_id) + let history_id = history_id_from_var(fish_history_var); + History::new(history_id) }); Some(EnvVar::new_from_name_vec( L!("history"), diff --git a/src/env_dispatch.rs b/src/env_dispatch.rs index 49b4b4e22..4b27a404f 100644 --- a/src/env_dispatch.rs +++ b/src/env_dispatch.rs @@ -231,8 +231,8 @@ fn handle_term_size_change(vars: &EnvStack) { } fn handle_fish_history_change(vars: &EnvStack) { - let session_id = crate::history::history_session_id(vars); - reader_change_history(&session_id); + let history_id = crate::history::history_id(vars); + reader_change_history(history_id); } fn handle_fish_cursor_selection_mode_change(vars: &EnvStack) { diff --git a/src/expand.rs b/src/expand.rs index 7f7c9e7e7..6d1665e88 100644 --- a/src/expand.rs +++ b/src/expand.rs @@ -13,7 +13,7 @@ complete::{CompleteFlags, Completion, CompletionList, CompletionReceiver}, env::{EnvVar, Environment}, exec::exec_subshell_for_expand, - history::{History, history_session_id}, + history::{History, history_id}, operation_context::OperationContext, parse_constants::{ParseError, ParseErrorCode, ParseErrorList, SOURCE_LOCATION_UNKNOWN}, parse_util::{MaybeParentheses, expand_variable_error, locate_cmdsubst_range}, @@ -613,7 +613,7 @@ fn expand_variables( let mut history = None; let mut var = None; if var_name == "history" { - history = Some(History::new(&history_session_id(vars))); + history = Some(History::new(history_id(vars))); } else if var_name.as_char_slice() != [VARIABLE_EXPAND_EMPTY] { var = vars.get(var_name); } diff --git a/src/history/history.rs b/src/history/history.rs index b2f8e4f9a..44919ca46 100644 --- a/src/history/history.rs +++ b/src/history/history.rs @@ -276,7 +276,19 @@ fn merge(&mut self, item: &HistoryItem) -> bool { } } -static HISTORIES: Mutex>> = Mutex::new(BTreeMap::new()); +#[derive(Clone, Eq, Ord, PartialEq, PartialOrd)] +pub enum MemoryHistoryId { + PrivateMode, + BuiltinRead, +} + +#[derive(Clone, Eq, Ord, PartialEq, PartialOrd)] +pub enum HistoryId { + Memory(MemoryHistoryId), + Disk { session_id: WString }, +} + +static HISTORIES: Mutex>> = Mutex::new(BTreeMap::new()); /// When deleting, whether the deletion should be only for this session or for all sessions. #[derive(Clone, Copy, PartialEq, Eq)] @@ -1218,9 +1230,14 @@ pub fn add_commandline(&self, s: WString) { /// 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(). - fn new_with_directory(name: &wstr, directory: Option) -> Arc { + fn new_with_directory(id: HistoryId, directory: Option) -> Arc { Arc::new(Self(Mutex::new(HistoryImpl::new( - name.to_owned(), + match id { + HistoryId::Memory(_) => WString::new(), + HistoryId::Disk { + session_id: filename, + } => filename, + }, directory, )))) } @@ -1228,14 +1245,14 @@ fn new_with_directory(name: &wstr, directory: Option) -> Arc { /// 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 new(name: &wstr) -> Arc { + pub fn new(id: HistoryId) -> Arc { let mut histories = HISTORIES.lock().unwrap(); - if let Some(hist) = histories.get(name) { + if let Some(hist) = histories.get(&id) { Arc::clone(hist) } else { - let hist = Self::new_with_directory(name, None); - histories.insert(name.to_owned(), Arc::clone(&hist)); + let hist = Self::new_with_directory(id.clone(), None); + histories.insert(id, Arc::clone(&hist)); hist } } @@ -1693,17 +1710,23 @@ pub fn save_all() { } /// Return the prefix for the files to be used for command and read history. -pub fn history_session_id(vars: &dyn Environment) -> WString { - history_session_id_from_var(vars.get(L!("fish_history"))) +pub fn history_id(vars: &dyn Environment) -> HistoryId { + history_id_from_var(vars.get(L!("fish_history"))) } -pub fn history_session_id_from_var(history_name_var: Option) -> WString { +pub fn history_id_from_var(history_name_var: Option) -> HistoryId { + use HistoryId::*; + let default = || Disk { + session_id: DFLT_FISH_HISTORY_SESSION_ID.to_owned(), + }; let Some(var) = history_name_var else { - return DFLT_FISH_HISTORY_SESSION_ID.to_owned(); + return default(); }; let session_id = var.as_string(); - if session_id.is_empty() || valid_var_name(&session_id) { - session_id + if session_id.is_empty() { + Memory(MemoryHistoryId::PrivateMode) + } else if valid_var_name(&session_id) { + Disk { session_id } } else { flog!( error, @@ -1713,7 +1736,7 @@ pub fn history_session_id_from_var(history_name_var: Option) -> WString DFLT_FISH_HISTORY_SESSION_ID ), ); - DFLT_FISH_HISTORY_SESSION_ID.to_owned() + default() } } @@ -1803,6 +1826,7 @@ mod tests { common::ESCAPE_TEST_CHAR, env::{EnvMode, EnvSetMode, EnvStack}, fs::{LockedFile, WriteMethod}, + history::HistoryId, prelude::*, tests::prelude::test_init, }; @@ -1834,7 +1858,12 @@ fn history_contains(history: &History, txt: &wstr) -> bool { // Helper to create a history with a custom directory, for testing. fn create_test_history(name: &wstr, custom_dir: &wstr) -> Arc { - History::new_with_directory(name, Some(custom_dir.to_owned())) + History::new_with_directory( + HistoryId::Disk { + session_id: name.to_owned(), + }, + Some(custom_dir.to_owned()), + ) } fn random_string(rng: &mut ThreadRng) -> WString { @@ -2292,7 +2321,6 @@ fn test_history_path_detection() { // Regression test for #7582. // Temporary directory for the history files. let hist_tmpdir = fish_tempfile::new_dir().unwrap(); - let hist_dir = Some(osstr2wcstring(hist_tmpdir.path())); // Temporary directory for the files we will detect. let tmpdir = fish_tempfile::new_dir().unwrap(); @@ -2309,7 +2337,8 @@ fn test_history_path_detection() { test_vars.set_one(L!("PWD"), global_mode, wdir_path.clone()); test_vars.set_one(L!("HOME"), global_mode, wdir_path.clone()); - let history = History::new_with_directory(L!("path_detection"), hist_dir); + let history = + create_test_history(L!("path_detection"), &osstr2wcstring(hist_tmpdir.path())); history.clear(); assert_eq!(history.size(), 0); history.add_pending_with_file_detection( diff --git a/src/reader/reader.rs b/src/reader/reader.rs index f4503cad5..5d764288d 100644 --- a/src/reader/reader.rs +++ b/src/reader/reader.rs @@ -47,8 +47,8 @@ parse_text_face_for_highlight, }, history::{ - History, HistorySearch, PersistenceMode, SearchDirection, SearchFlags, SearchType, - history_session_id, in_private_mode, + History, HistoryId, HistorySearch, MemoryHistoryId, PersistenceMode, SearchDirection, + SearchFlags, SearchType, history_id, in_private_mode, }, input_common::{ BackgroundColorQuery, CharEvent, CharInputStyle, CursorPositionQuery, @@ -369,8 +369,11 @@ pub fn current_data() -> Option<&'static mut ReaderData> { use fish_widestring::word_char::{WordCharClass, is_blank}; /// Add a new reader to the reader stack. -/// If `history_name` is empty, then save history in-memory only; do not write it to disk. -pub fn reader_push<'a>(parser: &'a Parser, history_name: &wstr, conf: ReaderConfig) -> Reader<'a> { +pub fn reader_push<'a>( + parser: &'a Parser, + history_id: HistoryId, + conf: ReaderConfig, +) -> Reader<'a> { assert_is_main_thread(); let inputfd = conf.inputfd; let input_data = if !parser.interactive_initialized.swap(true) { @@ -398,7 +401,7 @@ pub fn reader_push<'a>(parser: &'a Parser, history_name: &wstr, conf: ReaderConf } else { InputData::new(inputfd, *parser.blocking_query_timeout.borrow()) }; - let hist = History::new(history_name); + let hist = History::new(history_id); hist.resolve_pending(); let data = ReaderData::new(input_data, hist, conf, reader_data_stack().is_empty()); reader_data_stack().push(data); @@ -427,7 +430,7 @@ pub fn fake_scoped_reader<'a>(parser: &'a Parser) -> impl ScopeGuarding : fooba # CHECK: prompt 5> : fooba # CHECK: foobar foobaz + +isolated-tmux send-keys C-u C-l \ + 'read' Enter read-input Enter +tmux-sleep +isolated-tmux send-keys \ + 'set fish_history' Enter \ + 'true some command; read' Enter +tmux-sleep +isolated-tmux send-keys C-p +tmux-sleep +isolated-tmux capture-pane -p +# CHECK: prompt 5> read +# CHECK: read> read-input +# CHECK: read-input⏎ +# CHECK: prompt 6> set fish_history +# CHECK: prompt 6> true some command; read +# CHECK: read> read-input