From 628a47e6dc5380746a37a85c79872c7e90bd4053 Mon Sep 17 00:00:00 2001 From: Yassine Haouzane <81616372+YassineHaouzane@users.noreply.github.com> Date: Fri, 7 Jul 2023 03:16:17 +0200 Subject: [PATCH] Fix: update engine_state when history.isolation is true (#9268) (#9616) # Description Fixes #9268, the issue was due to the fact that when `history_isolation` was set to true the engine_state's `session_id` wasn't updated. This PR mainly refactors the code that updates the line_editor's history information into one function `update_line_editor_history` and also updates the engine_state's `session_id` when `history_isolation` is set to `true` by calling the function `store_history_id_in_engine`. # User-Facing Changes None since it's a bug fix. # Tests + Formatting I tried to extract the block that was updating the line_editor's `session_id` to make it easier to add a test. The test checks that after a call to the function `update_line_editor_history`, the `engine_state` and the returned `line_editor` do have the same `session_id`. --- crates/nu-cli/src/repl.rs | 89 ++++++++++++++++++++++++++++----------- 1 file changed, 65 insertions(+), 24 deletions(-) diff --git a/crates/nu-cli/src/repl.rs b/crates/nu-cli/src/repl.rs index e80ed8def..0e70d5927 100644 --- a/crates/nu-cli/src/repl.rs +++ b/crates/nu-cli/src/repl.rs @@ -7,7 +7,7 @@ use crate::{ }; use crossterm::cursor::SetCursorStyle; use log::{trace, warn}; -use miette::{IntoDiagnostic, Result}; +use miette::{ErrReport, IntoDiagnostic, Result}; use nu_cmd_base::util::get_guaranteed_cwd; use nu_color_config::StyleComputer; use nu_command::hook::eval_hook; @@ -20,9 +20,13 @@ use nu_protocol::{ Value, }; use nu_utils::utils::perf; -use reedline::{CursorConfig, DefaultHinter, EditCommand, Emacs, SqliteBackedHistory, Vi}; +use reedline::{ + CursorConfig, DefaultHinter, EditCommand, Emacs, FileBackedHistory, HistorySessionId, Reedline, + SqliteBackedHistory, Vi, +}; use std::{ io::{self, Write}, + path::Path, sync::atomic::Ordering, time::Instant, }; @@ -47,7 +51,7 @@ pub fn evaluate_repl( entire_start_time: Instant, ) -> Result<()> { use nu_command::hook; - use reedline::{FileBackedHistory, Reedline, Signal}; + use reedline::Signal; let use_color = engine_state.get_config().use_ansi_coloring; // Guard against invocation without a connected terminal. @@ -91,11 +95,7 @@ pub fn evaluate_repl( let mut line_editor = Reedline::create(); // Now that reedline is created, get the history session id and store it in engine_state - let hist_sesh = line_editor - .get_history_session_id() - .map(i64::from) - .unwrap_or(0); - engine_state.history_session_id = hist_sesh; + store_history_id_in_engine(engine_state, &line_editor); perf( "setup reedline", start_time, @@ -127,22 +127,8 @@ pub fn evaluate_repl( engine_state.config.history_file_format, ); if let Some(history_path) = history_path.as_deref() { - let history: Box = match engine_state.config.history_file_format { - HistoryFileFormat::PlainText => Box::new( - FileBackedHistory::with_file( - config.max_history_size as usize, - history_path.to_path_buf(), - ) - .into_diagnostic()?, - ), - HistoryFileFormat::Sqlite => Box::new( - SqliteBackedHistory::with_file(history_path.to_path_buf()).into_diagnostic()?, - ), - }; - line_editor = line_editor - .with_history_session_id(history_session_id) - .with_history_exclusion_prefix(Some(" ".into())) - .with_history(history); + line_editor = + update_line_editor_history(engine_state, history_path, line_editor, history_session_id)? }; perf( "setup history", @@ -729,6 +715,44 @@ pub fn evaluate_repl( Ok(()) } +fn store_history_id_in_engine(engine_state: &mut EngineState, line_editor: &Reedline) { + let session_id = line_editor + .get_history_session_id() + .map(i64::from) + .unwrap_or(0); + + engine_state.history_session_id = session_id; +} + +fn update_line_editor_history( + engine_state: &mut EngineState, + history_path: &Path, + line_editor: Reedline, + history_session_id: Option, +) -> Result { + let config = engine_state.get_config(); + let history: Box = match engine_state.config.history_file_format { + HistoryFileFormat::PlainText => Box::new( + FileBackedHistory::with_file( + config.max_history_size as usize, + history_path.to_path_buf(), + ) + .into_diagnostic()?, + ), + HistoryFileFormat::Sqlite => { + Box::new(SqliteBackedHistory::with_file(history_path.to_path_buf()).into_diagnostic()?) + } + }; + let line_editor = line_editor + .with_history_session_id(history_session_id) + .with_history_exclusion_prefix(Some(" ".into())) + .with_history(history); + + store_history_id_in_engine(engine_state, &line_editor); + + Ok(line_editor) +} + fn map_nucursorshape_to_cursorshape(shape: NuCursorShape) -> SetCursorStyle { match shape { NuCursorShape::Block => SetCursorStyle::SteadyBlock, @@ -800,3 +824,20 @@ fn looks_like_path_windows_drive_path_works() { assert_eq!(looks_like_path("F:\\some_dir"), on_windows); assert_eq!(looks_like_path("G:/some_dir"), on_windows); } + +#[test] +fn are_session_ids_in_sync() { + let engine_state = &mut EngineState::new(); + let history_path_o = + crate::config_files::get_history_path("nushell", engine_state.config.history_file_format); + assert!(history_path_o.is_some()); + let history_path = history_path_o.as_deref().unwrap(); + let line_editor = reedline::Reedline::create(); + let history_session_id = reedline::Reedline::create_history_session_id(); + let line_editor = + update_line_editor_history(engine_state, history_path, line_editor, history_session_id); + assert_eq!( + i64::from(line_editor.unwrap().get_history_session_id().unwrap()), + engine_state.history_session_id + ); +}