From 709b2479d98db033ac2644d27dec2341ab2be68a Mon Sep 17 00:00:00 2001 From: Devyn Cairns Date: Sat, 4 May 2024 02:38:37 -0700 Subject: [PATCH] Fix trailing slash in PWD set by `cd` (#12760) # Description Fixes #12758. #12662 introduced a bug where calling `cd` with a path with a trailing slash would cause `PWD` to be set to a path including a trailing slash, which is not allowed. This adds a helper to `nu_path` to remove this, and uses it in the `cd` command to clean it up before setting `PWD`. # Tests + Formatting I added some tests to make sure we don't regress on this in the future. - :green_circle: `toolkit fmt` - :green_circle: `toolkit clippy` - :green_circle: `toolkit test` - :green_circle: `toolkit test stdlib` --- crates/nu-command/src/filesystem/cd.rs | 3 + crates/nu-command/tests/commands/cd.rs | 27 +++++++ crates/nu-path/src/components.rs | 17 +--- crates/nu-path/src/lib.rs | 2 + crates/nu-path/src/trailing_slash.rs | 108 +++++++++++++++++++++++++ 5 files changed, 142 insertions(+), 15 deletions(-) create mode 100644 crates/nu-path/src/trailing_slash.rs diff --git a/crates/nu-command/src/filesystem/cd.rs b/crates/nu-command/src/filesystem/cd.rs index 50ad0b35a1..f90e9bfd2a 100644 --- a/crates/nu-command/src/filesystem/cd.rs +++ b/crates/nu-command/src/filesystem/cd.rs @@ -100,6 +100,9 @@ impl Command for Cd { } }; + // Strip the trailing slash from the new path. This is required for PWD. + let path = nu_path::strip_trailing_slash(&path); + // Set OLDPWD. // We're using `Stack::get_env_var()` instead of `EngineState::cwd()` to avoid a conversion roundtrip. if let Some(oldpwd) = stack.get_env_var(engine_state, "PWD") { diff --git a/crates/nu-command/tests/commands/cd.rs b/crates/nu-command/tests/commands/cd.rs index fa9ea382f2..c7c30528e8 100644 --- a/crates/nu-command/tests/commands/cd.rs +++ b/crates/nu-command/tests/commands/cd.rs @@ -26,6 +26,16 @@ fn filesystem_change_from_current_directory_using_relative_path() { }) } +#[test] +fn filesystem_change_from_current_directory_using_relative_path_with_trailing_slash() { + Playground::setup("cd_test_1_slash", |dirs, _| { + // Intentionally not using correct path sep because this should work on Windows + let actual = nu!( cwd: dirs.root(), "cd cd_test_1_slash/; $env.PWD"); + + assert_eq!(PathBuf::from(actual.out), *dirs.test()); + }) +} + #[test] fn filesystem_change_from_current_directory_using_absolute_path() { Playground::setup("cd_test_2", |dirs, _| { @@ -42,6 +52,23 @@ fn filesystem_change_from_current_directory_using_absolute_path() { }) } +#[test] +fn filesystem_change_from_current_directory_using_absolute_path_with_trailing_slash() { + Playground::setup("cd_test_2", |dirs, _| { + let actual = nu!( + cwd: dirs.test(), + r#" + cd '{}{}' + $env.PWD + "#, + dirs.formats().display(), + std::path::MAIN_SEPARATOR_STR, + ); + + assert_eq!(PathBuf::from(actual.out), dirs.formats()); + }) +} + #[test] fn filesystem_switch_back_to_previous_working_directory() { Playground::setup("cd_test_3", |dirs, sandbox| { diff --git a/crates/nu-path/src/components.rs b/crates/nu-path/src/components.rs index b7f60598c2..9cb74dc373 100644 --- a/crates/nu-path/src/components.rs +++ b/crates/nu-path/src/components.rs @@ -24,15 +24,13 @@ //! `Path::join()`. It works because `PathBuf::push("")` will add a trailing //! slash when the original path doesn't have one. -#[cfg(unix)] -use std::os::unix::ffi::OsStrExt; -#[cfg(windows)] -use std::os::windows::ffi::OsStrExt; use std::{ ffi::OsStr, path::{Component, Path}, }; +use crate::trailing_slash::has_trailing_slash; + /// Like `Path::components()`, but produces an extra empty component at the end /// when `path` contains a trailing slash. /// @@ -63,17 +61,6 @@ pub fn components(path: &Path) -> impl Iterator { })) } -#[cfg(windows)] -fn has_trailing_slash(path: &Path) -> bool { - let last = path.as_os_str().encode_wide().last(); - last == Some(b'\\' as u16) || last == Some(b'/' as u16) -} -#[cfg(unix)] -fn has_trailing_slash(path: &Path) -> bool { - let last = path.as_os_str().as_bytes().last(); - last == Some(&b'/') -} - #[cfg(test)] mod test { //! We'll go through every variant of Component, with or without trailing diff --git a/crates/nu-path/src/lib.rs b/crates/nu-path/src/lib.rs index e4baf36e76..6c064d4b57 100644 --- a/crates/nu-path/src/lib.rs +++ b/crates/nu-path/src/lib.rs @@ -4,8 +4,10 @@ pub mod dots; pub mod expansions; mod helpers; mod tilde; +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::{config_dir, config_dir_old, home_dir}; pub use tilde::expand_tilde; +pub use trailing_slash::{has_trailing_slash, strip_trailing_slash}; diff --git a/crates/nu-path/src/trailing_slash.rs b/crates/nu-path/src/trailing_slash.rs new file mode 100644 index 0000000000..4daf5d45ef --- /dev/null +++ b/crates/nu-path/src/trailing_slash.rs @@ -0,0 +1,108 @@ +use std::{ + borrow::Cow, + path::{Path, PathBuf}, +}; + +/// Strip any trailing slashes from a non-root path. This is required in some contexts, for example +/// for the `PWD` environment variable. +pub fn strip_trailing_slash(path: &Path) -> Cow { + if has_trailing_slash(path) { + // If there are, the safest thing to do is have Rust parse the path for us and build it + // again. This will correctly handle a root directory, but it won't add the trailing slash. + let mut out = PathBuf::with_capacity(path.as_os_str().len()); + out.extend(path.components()); + Cow::Owned(out) + } else { + // The path is safe and doesn't contain any trailing slashes. + Cow::Borrowed(path) + } +} + +/// `true` if the path has a trailing slash, including if it's the root directory. +#[cfg(windows)] +pub fn has_trailing_slash(path: &Path) -> bool { + use std::os::windows::ffi::OsStrExt; + + let last = path.as_os_str().encode_wide().last(); + last == Some(b'\\' as u16) || last == Some(b'/' as u16) +} + +/// `true` if the path has a trailing slash, including if it's the root directory. +#[cfg(unix)] +pub fn has_trailing_slash(path: &Path) -> bool { + use std::os::unix::ffi::OsStrExt; + + let last = path.as_os_str().as_bytes().last(); + last == Some(&b'/') +} + +#[cfg(test)] +mod tests { + use super::*; + + #[cfg_attr(not(unix), ignore = "only for Unix")] + #[test] + fn strip_root_unix() { + assert_eq!(Path::new("/"), strip_trailing_slash(Path::new("/"))); + } + + #[cfg_attr(not(unix), ignore = "only for Unix")] + #[test] + fn strip_non_trailing_unix() { + assert_eq!( + Path::new("/foo/bar"), + strip_trailing_slash(Path::new("/foo/bar")) + ); + } + + #[cfg_attr(not(unix), ignore = "only for Unix")] + #[test] + fn strip_trailing_unix() { + assert_eq!( + Path::new("/foo/bar"), + strip_trailing_slash(Path::new("/foo/bar/")) + ); + } + + #[cfg_attr(not(windows), ignore = "only for Windows")] + #[test] + fn strip_root_windows() { + assert_eq!(Path::new(r"C:\"), strip_trailing_slash(Path::new(r"C:\"))); + } + + #[cfg_attr(not(windows), ignore = "only for Windows")] + #[test] + fn strip_non_trailing_windows() { + assert_eq!( + Path::new(r"C:\foo\bar"), + strip_trailing_slash(Path::new(r"C:\foo\bar")) + ); + } + + #[cfg_attr(not(windows), ignore = "only for Windows")] + #[test] + fn strip_non_trailing_windows_unc() { + assert_eq!( + Path::new(r"\\foo\bar"), + strip_trailing_slash(Path::new(r"\\foo\bar")) + ); + } + + #[cfg_attr(not(windows), ignore = "only for Windows")] + #[test] + fn strip_trailing_windows() { + assert_eq!( + Path::new(r"C:\foo\bar"), + strip_trailing_slash(Path::new(r"C:\foo\bar\")) + ); + } + + #[cfg_attr(not(windows), ignore = "only for Windows")] + #[test] + fn strip_trailing_windows_unc() { + assert_eq!( + Path::new(r"\\foo\bar"), + strip_trailing_slash(Path::new(r"\\foo\bar\")) + ); + } +}