From bcb7ef48b622d91acdb7aa2e9f61b5a80dfd3d81 Mon Sep 17 00:00:00 2001 From: Piotr Kufel Date: Fri, 11 Oct 2024 05:51:50 -0700 Subject: [PATCH] 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 --- .../nu-cli/src/commands/history/history_.rs | 30 ++++--------- crates/nu-cli/src/config_files.rs | 45 +++++-------------- crates/nu-cli/src/repl.rs | 19 +++----- .../nu-command/src/env/config/config_reset.rs | 8 +--- crates/nu-path/src/helpers.rs | 32 +++++++------ crates/nu-path/src/lib.rs | 2 +- crates/nu-protocol/src/config/history.rs | 19 ++++++++ crates/nu-protocol/src/eval_const.rs | 7 +-- src/config_files.rs | 20 +++------ src/main.rs | 9 +--- src/run.rs | 32 ++++--------- tests/repl/test_config_path.rs | 4 +- 12 files changed, 82 insertions(+), 145 deletions(-) diff --git a/crates/nu-cli/src/commands/history/history_.rs b/crates/nu-cli/src/commands/history/history_.rs index fc1ed1b516..f5289e24a4 100644 --- a/crates/nu-cli/src/commands/history/history_.rs +++ b/crates/nu-cli/src/commands/history/history_.rs @@ -39,27 +39,15 @@ impl Command for History { ) -> Result { let head = call.head; - let Some(history) = engine_state.history_config() else { - return Ok(PipelineData::empty()); - }; - - // 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() { + if let Some(history) = engine_state.history_config() { + // 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) }); + }; 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> = 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 = 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 = 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()) } } diff --git a/crates/nu-cli/src/config_files.rs b/crates/nu-cli/src/config_files.rs index 998686d870..7627252acc 100644 --- a/crates/nu-cli/src/config_files.rs +++ b/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( - engine_state: &mut EngineState, - plugin_file: Option>, - storage_path: &str, -) { +pub fn read_plugin_file(engine_state: &mut EngineState, plugin_file: Option>) { 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( - engine_state: &mut EngineState, - plugin_file: Option>, - storage_path: &str, -) { +pub fn add_plugin_file(engine_state: &mut EngineState, plugin_file: Option>) { 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 { - 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| { - dir.push(storage_path); - nu_path::canonicalize_with(dir, &cwd).ok() - }) else { + let Some(config_dir) = + nu_path::nu_config_dir().and_then(|dir| nu_path::canonicalize_with(dir, &cwd).ok()) + else { return false; }; diff --git a/crates/nu-cli/src/repl.rs b/crates/nu-cli/src/repl.rs index 718f85f9b6..ed2b3a2510 100644 --- a/crates/nu-cli/src/repl.rs +++ b/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>, load_std_lib: Option>, 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 { } } -fn get_line_editor( - engine_state: &mut EngineState, - nushell_path: &str, - use_color: bool, -) -> Result { +fn get_line_editor(engine_state: &mut EngineState, use_color: bool) -> Result { 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 = - crate::config_files::get_history_path("nushell", history.file_format).unwrap(); + let history_path = history.file_path().unwrap(); let line_editor = reedline::Reedline::create(); let history_session_id = reedline::Reedline::create_history_session_id(); let line_editor = update_line_editor_history( diff --git a/crates/nu-command/src/env/config/config_reset.rs b/crates/nu-command/src/env/config/config_reset.rs index a185072d67..8cf45d21bc 100644 --- a/crates/nu-command/src/env/config/config_reset.rs +++ b/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() { - Some(path) => path, - None => { - return Err(ShellError::ConfigDirNotFound { span: None }); - } + let Some(config_path) = nu_path::nu_config_dir() else { + 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"); diff --git a/crates/nu-path/src/helpers.rs b/crates/nu-path/src/helpers.rs index b36d59971e..931b3f9d07 100644 --- a/crates/nu-path/src/helpers.rs +++ b/crates/nu-path/src/helpers.rs @@ -1,3 +1,5 @@ +use std::path::PathBuf; + use crate::AbsolutePathBuf; pub fn home_dir() -> Option { @@ -6,27 +8,29 @@ pub fn home_dir() -> Option { /// Return the data directory for the current platform or XDG_DATA_HOME if specified. pub fn data_dir() -> Option { - std::env::var("XDG_DATA_HOME") - .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)) + configurable_dir_path("XDG_DATA_HOME", dirs::data_dir) } /// Return the cache directory for the current platform or XDG_CACHE_HOME if specified. pub fn cache_dir() -> Option { - std::env::var("XDG_CACHE_HOME") - .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)) + configurable_dir_path("XDG_CACHE_HOME", dirs::cache_dir) } -/// Return the config directory for the current platform or XDG_CONFIG_HOME if specified. -pub fn config_dir() -> Option { - std::env::var("XDG_CONFIG_HOME") +/// Return the nushell config directory. +pub fn nu_config_dir() -> Option { + 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, +) -> Option { + 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)) } diff --git a/crates/nu-path/src/lib.rs b/crates/nu-path/src/lib.rs index 524a6aed4e..cf31a5789f 100644 --- a/crates/nu-path/src/lib.rs +++ b/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}; diff --git a/crates/nu-protocol/src/config/history.rs b/crates/nu-protocol/src/config/history.rs index 56418ae0a1..911a05ab16 100644 --- a/crates/nu-protocol/src/config/history.rs +++ b/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 { + 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 { diff --git a/crates/nu-protocol/src/eval_const.rs b/crates/nu-protocol/src/eval_const.rs index 26750b867c..695c044a79 100644 --- a/crates/nu-protocol/src/eval_const.rs +++ b/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() { - Some(mut path) => { - path.push("nushell"); - Ok(canonicalize_path(engine_state, path.as_ref())) - } + let config_path = match nu_path::nu_config_dir() { + Some(path) => Ok(canonicalize_path(engine_state, path.as_ref())), None => Err(Value::error( ShellError::ConfigDirNotFound { span: Some(span) }, span, diff --git a/src/config_files.rs b/src/config_files.rs index 1dd2a4b66d..7631a951bc 100644 --- a/src/config_files.rs +++ b/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() { - config_path.push(NUSHELL_FOLDER); - + } else if let Some(mut config_path) = nu_path::nu_config_dir() { // 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() { - config_path.push(NUSHELL_FOLDER); + if let Some(mut config_path) = nu_path::nu_config_dir() { 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() { - config_path.push(NUSHELL_FOLDER); - !config_path.exists() - } else { - false - }; + let ask_to_create_config = nu_path::nu_config_dir().map_or(false, |p| !p.exists()); 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| { - p.push(NUSHELL_FOLDER); + None => nu_path::nu_config_dir().map(|p| { let mut p = canonicalize_with(&p, cwd).unwrap_or(p.into()); p.push(default_config_name); canonicalize_with(&p, cwd).unwrap_or(p) diff --git a/src/main.rs b/src/main.rs index 839c32b66a..b7c5a6a173 100644 --- a/src/main.rs +++ b/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() { - 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() - }; - + let nushell_config_path: PathBuf = nu_path::nu_config_dir().map(Into::into).unwrap_or_default(); if let Ok(xdg_config_home) = std::env::var("XDG_CONFIG_HOME") { if !xdg_config_home.is_empty() { if nushell_config_path diff --git a/src/run.rs b/src/run.rs index 16dbe0a5b6..84b5f7df4a 100644 --- a/src/run.rs +++ b/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() { - config_path.push(NUSHELL_FOLDER); - !config_path.exists() - } else { - false - }; - + let mut stack = Stack::new(); 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() { - config_path.push(config_files::NUSHELL_FOLDER); - !config_path.exists() - } else { - false - }; + let ask_to_create_config = nu_path::nu_config_dir().map_or(false, |p| !p.exists()); // 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() { - config_path.push(config_files::NUSHELL_FOLDER); - !config_path.exists() - } else { - false - }; + let ask_to_create_config = nu_path::nu_config_dir().map_or(false, |p| !p.exists()); #[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, diff --git a/tests/repl/test_config_path.rs b/tests/repl/test_config_path.rs index 6bee043765..c812f3404c 100644 --- a/tests/repl/test_config_path.rs +++ b/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"); - test_config_path_helper(playground, config_dir.join("nushell")); + let config_dir = nu_path::nu_config_dir().expect("Could not get config directory"); + test_config_path_helper(playground, config_dir); }); }