From a7b281292d509364c83867c4f931fd4cdd620632 Mon Sep 17 00:00:00 2001 From: Yash Thakur <45539777+ysthakur@users.noreply.github.com> Date: Sun, 10 Mar 2024 06:07:31 -0400 Subject: [PATCH] Canonicalize config dir (#12136) It turns out that my previous PR, https://github.com/nushell/nushell/pull/11999, didn't properly canonicalize `$nu.default-config-dir` in a scenario where `XDG_CONFIG_HOME` (or the equivalent on each platform) was a symlink. To remedy that, this PR makes `nu_path::config_dir()` return a canonicalized path. This probably shouldn't break anything (except maybe tests relying on the old behavior), since the canonical path will be equivalent to non-canonical paths. # User-Facing Changes A user may get a path with symlinks resolved and `..`s replaced where they previously didn't. I'm not sure where this would happen, though, and anyway, the canonical path is probably the "correct" thing to present to the user. We're using `omnipath` to make the path presentable to the user on Windows, so there's no danger of someone getting an path with `\\?` there. # Tests + Formatting The tests for config files have been updated to run the binary using the `Director` so that it has access to the `XDG_CONFIG_HOME`/`HOME` environment variables to be able to change the config directory. --- crates/nu-cli/src/config_files.rs | 3 +- crates/nu-path/src/helpers.rs | 2 +- src/tests/test_config_path.rs | 78 +++++++++++++++++++------------ 3 files changed, 49 insertions(+), 34 deletions(-) diff --git a/crates/nu-cli/src/config_files.rs b/crates/nu-cli/src/config_files.rs index 06e20ef248..49620ff5fa 100644 --- a/crates/nu-cli/src/config_files.rs +++ b/crates/nu-cli/src/config_files.rs @@ -71,8 +71,7 @@ pub fn add_plugin_file( let e = ParseError::FileNotFound(plugin_file.item, plugin_file.span); report_error(&working_set, &e); } - } else if let Some(plugin_path) = nu_path::config_dir() { - let mut plugin_path = canonicalize_with(&plugin_path, cwd).unwrap_or(plugin_path); + } else if let Some(mut plugin_path) = nu_path::config_dir() { // Path to store plugins signatures plugin_path.push(storage_path); plugin_path.push(PLUGIN_FILE); diff --git a/crates/nu-path/src/helpers.rs b/crates/nu-path/src/helpers.rs index febe8a2a7c..a3ce3b84cf 100644 --- a/crates/nu-path/src/helpers.rs +++ b/crates/nu-path/src/helpers.rs @@ -7,7 +7,7 @@ pub fn home_dir() -> Option { } pub fn config_dir() -> Option { - dirs_next::config_dir() + dirs_next::config_dir().map(|path| canonicalize(&path).unwrap_or(path)) } #[cfg(windows)] diff --git a/src/tests/test_config_path.rs b/src/tests/test_config_path.rs index bf0a74377a..85d2e4ca4b 100644 --- a/src/tests/test_config_path.rs +++ b/src/tests/test_config_path.rs @@ -1,12 +1,8 @@ use nu_test_support::nu; +use nu_test_support::playground::{Executable, Playground}; use pretty_assertions::assert_eq; use std::fs; -use std::path::Path; - -#[cfg(any(target_os = "linux", target_os = "macos"))] -use nu_test_support::playground::Playground; -#[cfg(any(target_os = "linux", target_os = "macos"))] -use std::path::PathBuf; +use std::path::{Path, PathBuf}; #[cfg(not(target_os = "windows"))] fn adjust_canonicalization>(p: P) -> String { @@ -40,76 +36,97 @@ fn setup_fake_config(playground: &mut Playground) -> PathBuf { "XDG_CONFIG_HOME", &playground.cwd().join(config_link).display().to_string(), ); - Path::new(config_link).join("nushell") + playground.cwd().join(config_link).join("nushell") } #[cfg(target_os = "macos")] { let fake_home = "fake_home"; let home_link = "home_link"; - let dir_end = "fake-home/Library/Application\\ Support/nushell"; + let dir_end = "Library/Application Support/nushell"; playground.mkdir(&format!("{fake_home}/{dir_end}")); playground.symlink(fake_home, home_link); playground.with_env( "HOME", &playground.cwd().join(home_link).display().to_string(), ); - PathBuf::from(home_link).join(dir_end) + playground.cwd().join(home_link).join(dir_end) } } -fn test_config_path_helper() { - let config_dir = nu_path::config_dir().expect("Could not get config directory"); - let config_dir_nushell = config_dir.join("nushell"); +fn run(playground: &mut Playground, command: &str) -> String { + let result = playground.pipeline(command).execute().map_err(|e| { + let outcome = e.output.map(|outcome| { + format!( + "out: '{}', err: '{}'", + String::from_utf8_lossy(&outcome.out), + String::from_utf8_lossy(&outcome.err) + ) + }); + format!( + "desc: {}, exit: {:?}, outcome: {}", + e.desc, + e.exit, + outcome.unwrap_or("empty".to_owned()) + ) + }); + String::from_utf8_lossy(&result.unwrap().out) + .trim() + .to_string() +} + +fn test_config_path_helper(playground: &mut Playground, config_dir_nushell: PathBuf) { // Create the config dir folder structure if it does not already exist if !config_dir_nushell.exists() { let _ = fs::create_dir_all(&config_dir_nushell); } - let cwd = std::env::current_dir().expect("Could not get current working directory"); let config_dir_nushell = std::fs::canonicalize(&config_dir_nushell).expect("canonicalize config dir failed"); - let actual = nu!(cwd: &cwd, "$nu.default-config-dir"); - assert_eq!(actual.out, adjust_canonicalization(&config_dir_nushell)); + let actual = run(playground, "$nu.default-config-dir"); + assert_eq!(actual, adjust_canonicalization(&config_dir_nushell)); let config_path = config_dir_nushell.join("config.nu"); // We use canonicalize here in case the config or env is symlinked since $nu.config-path is returning the canonicalized path in #8653 let canon_config_path = adjust_canonicalization(std::fs::canonicalize(&config_path).unwrap_or(config_path)); - let actual = nu!(cwd: &cwd, "$nu.config-path"); - assert_eq!(actual.out, canon_config_path); + let actual = run(playground, "$nu.config-path"); + assert_eq!(actual, canon_config_path); let env_path = config_dir_nushell.join("env.nu"); let canon_env_path = adjust_canonicalization(std::fs::canonicalize(&env_path).unwrap_or(env_path)); - let actual = nu!(cwd: &cwd, "$nu.env-path"); - assert_eq!(actual.out, canon_env_path); + let actual = run(playground, "$nu.env-path"); + assert_eq!(actual, canon_env_path); let history_path = config_dir_nushell.join("history.txt"); let canon_history_path = adjust_canonicalization(std::fs::canonicalize(&history_path).unwrap_or(history_path)); - let actual = nu!(cwd: &cwd, "$nu.history-path"); - assert_eq!(actual.out, canon_history_path); + let actual = run(playground, "$nu.history-path"); + assert_eq!(actual, canon_history_path); let login_path = config_dir_nushell.join("login.nu"); let canon_login_path = adjust_canonicalization(std::fs::canonicalize(&login_path).unwrap_or(login_path)); - let actual = nu!(cwd: &cwd, "$nu.loginshell-path"); - assert_eq!(actual.out, canon_login_path); + let actual = run(playground, "$nu.loginshell-path"); + assert_eq!(actual, canon_login_path); #[cfg(feature = "plugin")] { let plugin_path = config_dir_nushell.join("plugin.nu"); let canon_plugin_path = adjust_canonicalization(std::fs::canonicalize(&plugin_path).unwrap_or(plugin_path)); - let actual = nu!(cwd: &cwd, "$nu.plugin-path"); - assert_eq!(actual.out, canon_plugin_path); + let actual = run(playground, "$nu.plugin-path"); + assert_eq!(actual, canon_plugin_path); } } #[test] fn test_default_config_path() { - test_config_path_helper(); + 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")); + }); } /// Make the config folder a symlink to a temporary folder without any config files @@ -118,9 +135,8 @@ fn test_default_config_path() { #[test] fn test_default_symlinked_config_path_empty() { Playground::setup("symlinked_empty_config_dir", |_, playground| { - let _ = setup_fake_config(playground); - - test_config_path_helper(); + let config_dir_nushell = setup_fake_config(playground); + test_config_path_helper(playground, config_dir_nushell); }); } @@ -148,7 +164,7 @@ fn test_default_symlink_config_path_broken_symlink_config_files() { ); } - test_config_path_helper(); + test_config_path_helper(playground, fake_config_dir_nushell); }, ); } @@ -178,7 +194,7 @@ fn test_default_config_path_symlinked_config_files() { playground.symlink(empty_file, fake_config_dir_nushell.join(config_file)); } - test_config_path_helper(); + test_config_path_helper(playground, fake_config_dir_nushell); }, ); }