Fix: update engine_state when history.isolation is true (#9268) (#9616)

<!--
if this PR closes one or more issues, you can automatically link the PR
with
them by using one of the [*linking
keywords*](https://docs.github.com/en/issues/tracking-your-work-with-issues/linking-a-pull-request-to-an-issue#linking-a-pull-request-to-an-issue-using-a-keyword),
e.g.
- this PR should close #xxxx
- fixes #xxxx

you can also mention related issues, PRs or discussions!
-->

# 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`.
<!--
Thank you for improving Nushell. Please, check our [contributing
guide](../CONTRIBUTING.md) and talk to the core team before making major
changes.

Description of your pull request goes here. **Provide examples and/or
screenshots** if your changes affect the user experience.
-->

# User-Facing Changes
<!-- List of all changes that impact the user experience here. This
helps us keep track of breaking changes. -->
None since it's a bug fix.

# Tests + Formatting
<!--
Don't forget to add tests that cover your changes.

Make sure you've run and fixed any issues with these commands:

- `cargo fmt --all -- --check` to check standard code formatting (`cargo
fmt --all` applies these changes)
- `cargo clippy --workspace -- -D warnings -D clippy::unwrap_used -A
clippy::needless_collect -A clippy::result_large_err` to check that
you're using the standard code style
- `cargo test --workspace` to check that all tests pass
- `cargo run -- crates/nu-std/tests/run.nu` to run the tests for the
standard library

> **Note**
> from `nushell` you can also use the `toolkit` as follows
> ```bash
> use toolkit.nu # or use an `env_change` hook to activate it
automatically
> toolkit check pr
> ```
-->

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

@@ -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
+    store_history_id_in_engine(engine_state, &line_editor);
-        .get_history_session_id()
-        .map(i64::from)
-        .unwrap_or(0);
-    engine_state.history_session_id = hist_sesh;
     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<dyn reedline::History> = match engine_state.config.history_file_format {
+        line_editor =
-            HistoryFileFormat::PlainText => Box::new(
+            update_line_editor_history(engine_state, history_path, line_editor, history_session_id)?
-                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);
     };
     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<HistorySessionId>,
+) -> Result<Reedline, ErrReport> {
+    let config = engine_state.get_config();
+    let history: Box<dyn reedline::History> = 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
+    );
+}