Reduce duplication in history path construction (#13475)

# Description
Currently there is a bit of chaos regarding construction of history file
paths. Various pieces of code across a number of crates reimplement the
same/similar logic:
- There is `get_history_path`, but it requires a directory parameter (it
really just joins it with a file name).
- Some places use a const for the directory parameter, others use a
string literal - in all cases the value seems to be `"nushell"`.
- Some places assume the `"nushell"` value, other plumb it down from
close to the top of the call stack.
- Some places use a constant for history file names while others assume
it.

This PR tries to make it so that the history/config path format is
defined in a single places and so dependencies on it are easier to
follow:
- It removes `get_history_path` and adds a `file_path` method to
`HistoryConfig` instead (an extra motivation being, this is a convenient
place that can be used from all creates that need a history file path)
- Adds a `nu_config_dir` function that returns the nushell configuration
directory.
- Updates existing code to rely on the above, effectively removing
duplicate uses of `"nushell"` and `NUSHELL_FOLDER` and assumptions about
file names associated with different history formats

# User-Facing Changes
None

crates/nu-cli/src/commands/history/history_.rs

@@ -39,27 +39,15 @@ impl Command for History {
     ) -> Result<PipelineData, ShellError> {
         let head = call.head;
 
-        let Some(history) = engine_state.history_config() else {
+        if let Some(history) = engine_state.history_config() {
-            return Ok(PipelineData::empty());
+            // todo for sqlite history this command should be an alias to `open ~/.config/nushell/history.sqlite3 | get history`
-        };
+            let Some(history_path) = history.file_path() else {
-
+                return Err(ShellError::ConfigDirNotFound { span: Some(head) });
-        // todo for sqlite history this command should be an alias to `open ~/.config/nushell/history.sqlite3 | get history`
+            };
-        if let Some(config_path) = nu_path::config_dir() {
             let clear = call.has_flag(engine_state, stack, "clear")?;
             let long = call.has_flag(engine_state, stack, "long")?;
             let signals = engine_state.signals().clone();
 
-            let mut history_path = config_path;
-            history_path.push("nushell");
-            match history.file_format {
-                HistoryFileFormat::Sqlite => {
-                    history_path.push("history.sqlite3");
-                }
-                HistoryFileFormat::Plaintext => {
-                    history_path.push("history.txt");
-                }
-            }
-
             if clear {
                 let _ = std::fs::remove_file(history_path);
                 // TODO: FIXME also clear the auxiliary files when using sqlite
@@ -67,17 +55,16 @@ impl Command for History {
             } else {
                 let history_reader: Option<Box<dyn ReedlineHistory>> = match history.file_format {
                     HistoryFileFormat::Sqlite => {
-                        SqliteBackedHistory::with_file(history_path.clone().into(), None, None)
+                        SqliteBackedHistory::with_file(history_path.clone(), None, None)
                             .map(|inner| {
                                 let boxed: Box<dyn ReedlineHistory> = Box::new(inner);
                                 boxed
                             })
                             .ok()
                     }
-
                     HistoryFileFormat::Plaintext => FileBackedHistory::with_file(
                         history.max_size as usize,
-                        history_path.clone().into(),
+                        history_path.clone(),
                     )
                     .map(|inner| {
                         let boxed: Box<dyn ReedlineHistory> = Box::new(inner);
@@ -85,7 +72,6 @@ impl Command for History {
                     })
                     .ok(),
                 };
-
                 match history.file_format {
                     HistoryFileFormat::Plaintext => Ok(history_reader
                         .and_then(|h| {
@@ -126,7 +112,7 @@ impl Command for History {
                 }
             }
         } else {
-            Err(ShellError::ConfigDirNotFound { span: Some(head) })
+            Ok(PipelineData::empty())
         }
     }
 

crates/nu-cli/src/config_files.rs

@@ -5,7 +5,7 @@ use nu_path::canonicalize_with;
 use nu_protocol::{engine::StateWorkingSet, ParseError, PluginRegistryFile, Spanned};
 use nu_protocol::{
     engine::{EngineState, Stack},
-    report_shell_error, HistoryFileFormat, PipelineData,
+    report_shell_error, PipelineData,
 };
 #[cfg(feature = "plugin")]
 use nu_utils::perf;
@@ -16,15 +16,8 @@ const PLUGIN_FILE: &str = "plugin.msgpackz";
 #[cfg(feature = "plugin")]
 const OLD_PLUGIN_FILE: &str = "plugin.nu";
 
-const HISTORY_FILE_TXT: &str = "history.txt";
-const HISTORY_FILE_SQLITE: &str = "history.sqlite3";
-
 #[cfg(feature = "plugin")]
-pub fn read_plugin_file(
+pub fn read_plugin_file(engine_state: &mut EngineState, plugin_file: Option<Spanned<String>>) {
-    engine_state: &mut EngineState,
-    plugin_file: Option<Spanned<String>>,
-    storage_path: &str,
-) {
     use nu_protocol::ShellError;
     use std::path::Path;
 
@@ -52,7 +45,7 @@ pub fn read_plugin_file(
     let mut start_time = std::time::Instant::now();
     // Reading signatures from plugin registry file
     // The plugin.msgpackz file stores the parsed signature collected from each registered plugin
-    add_plugin_file(engine_state, plugin_file.clone(), storage_path);
+    add_plugin_file(engine_state, plugin_file.clone());
     perf!(
         "add plugin file to engine_state",
         start_time,
@@ -70,8 +63,7 @@ pub fn read_plugin_file(
                     log::warn!("Plugin file not found: {}", plugin_path.display());
 
                     // Try migration of an old plugin file if this wasn't a custom plugin file
-                    if plugin_file.is_none() && migrate_old_plugin_file(engine_state, storage_path)
+                    if plugin_file.is_none() && migrate_old_plugin_file(engine_state) {
-                    {
                         let Ok(file) = std::fs::File::open(&plugin_path) else {
                             log::warn!("Failed to load newly migrated plugin file");
                             return;
@@ -159,11 +151,7 @@ pub fn read_plugin_file(
 }
 
 #[cfg(feature = "plugin")]
-pub fn add_plugin_file(
+pub fn add_plugin_file(engine_state: &mut EngineState, plugin_file: Option<Spanned<String>>) {
-    engine_state: &mut EngineState,
-    plugin_file: Option<Spanned<String>>,
-    storage_path: &str,
-) {
     use std::path::Path;
 
     use nu_protocol::report_parse_error;
@@ -189,9 +177,8 @@ pub fn add_plugin_file(
                     ),
                 );
             }
-        } else if let Some(mut plugin_path) = nu_path::config_dir() {
+        } else if let Some(plugin_path) = nu_path::nu_config_dir() {
             // Path to store plugins signatures
-            plugin_path.push(storage_path);
             let mut plugin_path =
                 canonicalize_with(&plugin_path, &cwd).unwrap_or(plugin_path.into());
             plugin_path.push(PLUGIN_FILE);
@@ -235,19 +222,8 @@ pub fn eval_config_contents(
     }
 }
 
-pub(crate) fn get_history_path(storage_path: &str, mode: HistoryFileFormat) -> Option<PathBuf> {
-    nu_path::config_dir().map(|mut history_path| {
-        history_path.push(storage_path);
-        history_path.push(match mode {
-            HistoryFileFormat::Plaintext => HISTORY_FILE_TXT,
-            HistoryFileFormat::Sqlite => HISTORY_FILE_SQLITE,
-        });
-        history_path.into()
-    })
-}
-
 #[cfg(feature = "plugin")]
-pub fn migrate_old_plugin_file(engine_state: &EngineState, storage_path: &str) -> bool {
+pub fn migrate_old_plugin_file(engine_state: &EngineState) -> bool {
     use nu_protocol::{
         PluginExample, PluginIdentity, PluginRegistryItem, PluginRegistryItemData, PluginSignature,
         ShellError,
@@ -260,10 +236,9 @@ pub fn migrate_old_plugin_file(engine_state: &EngineState, storage_path: &str) -
         return false;
     };
 
-    let Some(config_dir) = nu_path::config_dir().and_then(|mut dir| {
+    let Some(config_dir) =
-        dir.push(storage_path);
+        nu_path::nu_config_dir().and_then(|dir| nu_path::canonicalize_with(dir, &cwd).ok())
-        nu_path::canonicalize_with(dir, &cwd).ok()
+    else {
-    }) else {
         return false;
     };
 

crates/nu-cli/src/repl.rs

@@ -50,7 +50,6 @@ use sysinfo::System;
 pub fn evaluate_repl(
     engine_state: &mut EngineState,
     stack: Stack,
-    nushell_path: &str,
     prerun_command: Option<Spanned<String>>,
     load_std_lib: Option<Spanned<String>>,
     entire_start_time: Instant,
@@ -97,7 +96,7 @@ pub fn evaluate_repl(
 
     unique_stack.set_last_exit_code(0, Span::unknown());
 
-    let mut line_editor = get_line_editor(engine_state, nushell_path, use_color)?;
+    let mut line_editor = get_line_editor(engine_state, use_color)?;
     let temp_file = temp_dir().join(format!("{}.nu", uuid::Uuid::new_v4()));
 
     if let Some(s) = prerun_command {
@@ -213,7 +212,7 @@ pub fn evaluate_repl(
             }
             Err(_) => {
                 // line_editor is lost in the error case so reconstruct a new one
-                line_editor = get_line_editor(engine_state, nushell_path, use_color)?;
+                line_editor = get_line_editor(engine_state, use_color)?;
             }
         }
     }
@@ -243,11 +242,7 @@ fn unescape_for_vscode(text: &mut std::str::Chars) -> Option<char> {
     }
 }
 
-fn get_line_editor(
+fn get_line_editor(engine_state: &mut EngineState, use_color: bool) -> Result<Reedline> {
-    engine_state: &mut EngineState,
-    nushell_path: &str,
-    use_color: bool,
-) -> Result<Reedline> {
     let mut start_time = std::time::Instant::now();
     let mut line_editor = Reedline::create();
 
@@ -258,7 +253,7 @@ fn get_line_editor(
     if let Some(history) = engine_state.history_config() {
         start_time = std::time::Instant::now();
 
-        line_editor = setup_history(nushell_path, engine_state, line_editor, history)?;
+        line_editor = setup_history(engine_state, line_editor, history)?;
 
         perf!("setup history", start_time, use_color);
     }
@@ -1121,7 +1116,6 @@ fn flush_engine_state_repl_buffer(engine_state: &mut EngineState, line_editor: &
 /// Setup history management for Reedline
 ///
 fn setup_history(
-    nushell_path: &str,
     engine_state: &mut EngineState,
     line_editor: Reedline,
     history: HistoryConfig,
@@ -1133,7 +1127,7 @@ fn setup_history(
         None
     };
 
-    if let Some(path) = crate::config_files::get_history_path(nushell_path, history.file_format) {
+    if let Some(path) = history.file_path() {
         return update_line_editor_history(
             engine_state,
             path,
@@ -1409,8 +1403,7 @@ fn trailing_slash_looks_like_path() {
 fn are_session_ids_in_sync() {
     let engine_state = &mut EngineState::new();
     let history = engine_state.history_config().unwrap();
-    let history_path =
+    let history_path = history.file_path().unwrap();
-        crate::config_files::get_history_path("nushell", history.file_format).unwrap();
     let line_editor = reedline::Reedline::create();
     let history_session_id = reedline::Reedline::create_history_session_id();
     let line_editor = update_line_editor_history(

crates/nu-command/src/env/config/config_reset.rs

@@ -45,13 +45,9 @@ impl Command for ConfigReset {
         let only_env = call.has_flag(engine_state, stack, "env")?;
         let no_backup = call.has_flag(engine_state, stack, "without-backup")?;
         let span = call.head;
-        let mut config_path = match nu_path::config_dir() {
+        let Some(config_path) = nu_path::nu_config_dir() else {
-            Some(path) => path,
+            return Err(ShellError::ConfigDirNotFound { span: None });
-            None => {
-                return Err(ShellError::ConfigDirNotFound { span: None });
-            }
         };
-        config_path.push("nushell");
         if !only_env {
             let mut nu_config = config_path.clone();
             nu_config.push("config.nu");

crates/nu-path/src/helpers.rs

@@ -1,3 +1,5 @@
+use std::path::PathBuf;
+
 use crate::AbsolutePathBuf;
 
 pub fn home_dir() -> Option<AbsolutePathBuf> {
@@ -6,27 +8,29 @@ pub fn home_dir() -> Option<AbsolutePathBuf> {
 
 /// Return the data directory for the current platform or XDG_DATA_HOME if specified.
 pub fn data_dir() -> Option<AbsolutePathBuf> {
-    std::env::var("XDG_DATA_HOME")
+    configurable_dir_path("XDG_DATA_HOME", dirs::data_dir)
-        .ok()
-        .and_then(|path| AbsolutePathBuf::try_from(path).ok())
-        .or_else(|| dirs::data_dir().and_then(|path| AbsolutePathBuf::try_from(path).ok()))
-        .map(|path| path.canonicalize().map(Into::into).unwrap_or(path))
 }
 
 /// Return the cache directory for the current platform or XDG_CACHE_HOME if specified.
 pub fn cache_dir() -> Option<AbsolutePathBuf> {
-    std::env::var("XDG_CACHE_HOME")
+    configurable_dir_path("XDG_CACHE_HOME", dirs::cache_dir)
-        .ok()
-        .and_then(|path| AbsolutePathBuf::try_from(path).ok())
-        .or_else(|| dirs::cache_dir().and_then(|path| AbsolutePathBuf::try_from(path).ok()))
-        .map(|path| path.canonicalize().map(Into::into).unwrap_or(path))
 }
 
-/// Return the config directory for the current platform or XDG_CONFIG_HOME if specified.
+/// Return the nushell config directory.
-pub fn config_dir() -> Option<AbsolutePathBuf> {
+pub fn nu_config_dir() -> Option<AbsolutePathBuf> {
-    std::env::var("XDG_CONFIG_HOME")
+    configurable_dir_path("XDG_CONFIG_HOME", dirs::config_dir).map(|mut p| {
+        p.push("nushell");
+        p
+    })
+}
+
+fn configurable_dir_path(
+    name: &'static str,
+    dir: impl FnOnce() -> Option<PathBuf>,
+) -> Option<AbsolutePathBuf> {
+    std::env::var(name)
         .ok()
         .and_then(|path| AbsolutePathBuf::try_from(path).ok())
-        .or_else(|| dirs::config_dir().and_then(|path| AbsolutePathBuf::try_from(path).ok()))
+        .or_else(|| dir().and_then(|path| AbsolutePathBuf::try_from(path).ok()))
         .map(|path| path.canonicalize().map(Into::into).unwrap_or(path))
 }

crates/nu-path/src/lib.rs

@@ -11,7 +11,7 @@ mod trailing_slash;
 
 pub use components::components;
 pub use expansions::{canonicalize_with, expand_path_with, expand_to_real_path, locate_in_dirs};
-pub use helpers::{cache_dir, config_dir, data_dir, home_dir};
+pub use helpers::{cache_dir, data_dir, home_dir, nu_config_dir};
 pub use path::*;
 pub use tilde::expand_tilde;
 pub use trailing_slash::{has_trailing_slash, strip_trailing_slash};

crates/nu-protocol/src/config/history.rs

@@ -9,6 +9,16 @@ pub enum HistoryFileFormat {
     Plaintext,
 }
 
+impl HistoryFileFormat {
+    pub fn default_file_name(self) -> std::path::PathBuf {
+        match self {
+            HistoryFileFormat::Plaintext => "history.txt",
+            HistoryFileFormat::Sqlite => "history.sqlite3",
+        }
+        .into()
+    }
+}
+
 impl FromStr for HistoryFileFormat {
     type Err = &'static str;
 
@@ -29,6 +39,15 @@ pub struct HistoryConfig {
     pub isolation: bool,
 }
 
+impl HistoryConfig {
+    pub fn file_path(&self) -> Option<std::path::PathBuf> {
+        nu_path::nu_config_dir().map(|mut history_path| {
+            history_path.push(self.file_format.default_file_name());
+            history_path.into()
+        })
+    }
+}
+
 impl Default for HistoryConfig {
     fn default() -> Self {
         Self {

crates/nu-protocol/src/eval_const.rs

@@ -34,11 +34,8 @@ pub(crate) fn create_nu_constant(engine_state: &EngineState, span: Span) -> Valu
 
     let mut record = Record::new();
 
-    let config_path = match nu_path::config_dir() {
+    let config_path = match nu_path::nu_config_dir() {
-        Some(mut path) => {
+        Some(path) => Ok(canonicalize_path(engine_state, path.as_ref())),
-            path.push("nushell");
-            Ok(canonicalize_path(engine_state, path.as_ref()))
-        }
         None => Err(Value::error(
             ShellError::ConfigDirNotFound { span: Some(span) },
             span,

src/config_files.rs

@@ -17,7 +17,6 @@ use std::{
     sync::Arc,
 };
 
-pub(crate) const NUSHELL_FOLDER: &str = "nushell";
 const CONFIG_FILE: &str = "config.nu";
 const ENV_FILE: &str = "env.nu";
 const LOGINSHELL_FILE: &str = "login.nu";
@@ -48,9 +47,7 @@ pub(crate) fn read_config_file(
                 report_shell_error(engine_state, &e);
             }
         }
-    } else if let Some(mut config_path) = nu_path::config_dir() {
+    } else if let Some(mut config_path) = nu_path::nu_config_dir() {
-        config_path.push(NUSHELL_FOLDER);
-
         // Create config directory if it does not exist
         if !config_path.exists() {
             if let Err(err) = std::fs::create_dir_all(&config_path) {
@@ -141,8 +138,7 @@ pub(crate) fn read_loginshell_file(engine_state: &mut EngineState, stack: &mut S
     );
 
     // read and execute loginshell file if exists
-    if let Some(mut config_path) = nu_path::config_dir() {
+    if let Some(mut config_path) = nu_path::nu_config_dir() {
-        config_path.push(NUSHELL_FOLDER);
         config_path.push(LOGINSHELL_FILE);
 
         warn!("loginshell_file: {}", config_path.display());
@@ -268,16 +264,11 @@ pub(crate) fn setup_config(
         &config_file, &env_file, is_login_shell
     );
 
-    let ask_to_create_config = if let Some(mut config_path) = nu_path::config_dir() {
+    let ask_to_create_config = nu_path::nu_config_dir().map_or(false, |p| !p.exists());
-        config_path.push(NUSHELL_FOLDER);
-        !config_path.exists()
-    } else {
-        false
-    };
 
     let result = catch_unwind(AssertUnwindSafe(|| {
         #[cfg(feature = "plugin")]
-        read_plugin_file(engine_state, plugin_file, NUSHELL_FOLDER);
+        read_plugin_file(engine_state, plugin_file);
 
         read_config_file(engine_state, stack, env_file, true, ask_to_create_config);
         read_config_file(
@@ -315,8 +306,7 @@ pub(crate) fn set_config_path(
     );
     let config_path = match config_file {
         Some(s) => canonicalize_with(&s.item, cwd).ok(),
-        None => nu_path::config_dir().map(|mut p| {
+        None => nu_path::nu_config_dir().map(|p| {
-            p.push(NUSHELL_FOLDER);
             let mut p = canonicalize_with(&p, cwd).unwrap_or(p.into());
             p.push(default_config_name);
             canonicalize_with(&p, cwd).unwrap_or(p)

src/main.rs

@@ -98,14 +98,7 @@ fn main() -> Result<()> {
     // Set default NU_LIB_DIRS and NU_PLUGIN_DIRS here before the env.nu is processed. If
     // the env.nu file exists, these values will be overwritten, if it does not exist, or
     // there is an error reading it, these values will be used.
-    let nushell_config_path = if let Some(mut path) = nu_path::config_dir() {
+    let nushell_config_path: PathBuf = nu_path::nu_config_dir().map(Into::into).unwrap_or_default();
-        path.push("nushell");
-        path.into()
-    } else {
-        // Not really sure what to default this to if nu_path::config_dir() returns None
-        std::path::PathBuf::new()
-    };
-
     if let Ok(xdg_config_home) = std::env::var("XDG_CONFIG_HOME") {
         if !xdg_config_home.is_empty() {
             if nushell_config_path

src/run.rs

@@ -1,6 +1,6 @@
 use crate::{
     command,
-    config_files::{self, setup_config, NUSHELL_FOLDER},
+    config_files::{self, setup_config},
 };
 use log::trace;
 #[cfg(feature = "plugin")]
@@ -21,16 +21,11 @@ pub(crate) fn run_commands(
     entire_start_time: std::time::Instant,
 ) {
     trace!("run_commands");
-    let mut stack = Stack::new();
+
     let start_time = std::time::Instant::now();
+    let ask_to_create_config = nu_path::nu_config_dir().map_or(false, |p| !p.exists());
 
-    let ask_to_create_config = if let Some(mut config_path) = nu_path::config_dir() {
+    let mut stack = Stack::new();
-        config_path.push(NUSHELL_FOLDER);
-        !config_path.exists()
-    } else {
-        false
-    };
-
     if stack.has_env_var(engine_state, "NU_DISABLE_IR") {
         stack.use_ir = false;
     }
@@ -42,7 +37,7 @@ pub(crate) fn run_commands(
     // if the --no-config-file(-n) flag is passed, do not load plugin, env, or config files
     if parsed_nu_cli_args.no_config_file.is_none() {
         #[cfg(feature = "plugin")]
-        read_plugin_file(engine_state, parsed_nu_cli_args.plugin_file, NUSHELL_FOLDER);
+        read_plugin_file(engine_state, parsed_nu_cli_args.plugin_file);
 
         perf!("read plugins", start_time, use_color);
 
@@ -63,12 +58,7 @@ pub(crate) fn run_commands(
         perf!("read env.nu", start_time, use_color);
 
         let start_time = std::time::Instant::now();
-        let ask_to_create_config = if let Some(mut config_path) = nu_path::config_dir() {
+        let ask_to_create_config = nu_path::nu_config_dir().map_or(false, |p| !p.exists());
-            config_path.push(config_files::NUSHELL_FOLDER);
-            !config_path.exists()
-        } else {
-            false
-        };
 
         // If we have a config file parameter *OR* we have a login shell parameter, read the config file
         if parsed_nu_cli_args.config_file.is_some() || parsed_nu_cli_args.login_shell.is_some() {
@@ -140,14 +130,9 @@ pub(crate) fn run_file(
     // if the --no-config-file(-n) flag is passed, do not load plugin, env, or config files
     if parsed_nu_cli_args.no_config_file.is_none() {
         let start_time = std::time::Instant::now();
-        let ask_to_create_config = if let Some(mut config_path) = nu_path::config_dir() {
+        let ask_to_create_config = nu_path::nu_config_dir().map_or(false, |p| !p.exists());
-            config_path.push(config_files::NUSHELL_FOLDER);
-            !config_path.exists()
-        } else {
-            false
-        };
         #[cfg(feature = "plugin")]
-        read_plugin_file(engine_state, parsed_nu_cli_args.plugin_file, NUSHELL_FOLDER);
+        read_plugin_file(engine_state, parsed_nu_cli_args.plugin_file);
         perf!("read plugins", start_time, use_color);
 
         let start_time = std::time::Instant::now();
@@ -230,7 +215,6 @@ pub(crate) fn run_repl(
     let ret_val = evaluate_repl(
         engine_state,
         stack,
-        NUSHELL_FOLDER,
         parsed_nu_cli_args.execute,
         parsed_nu_cli_args.no_std_lib,
         entire_start_time,

tests/repl/test_config_path.rs

@@ -140,8 +140,8 @@ fn test_config_path_helper(
 #[test]
 fn test_default_config_path() {
     Playground::setup("default_config_path", |_, playground| {
-        let config_dir = nu_path::config_dir().expect("Could not get config directory");
+        let config_dir = nu_path::nu_config_dir().expect("Could not get config directory");
-        test_config_path_helper(playground, config_dir.join("nushell"));
+        test_config_path_helper(playground, config_dir);
     });
 }