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.

- 🟢 `toolkit fmt`
- 🟢 `toolkit clippy`
- 🟢 `toolkit test`
- 🟢 `toolkit test stdlib`

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") {

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| {

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<Item = Component> {
     }))
 }
 
-#[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

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};

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<Path> {
+    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\"))
+        );
+    }
+}