Split canonicalize function in two for missing and existing behavior (#1576)

* Split allow missing logic in two functions

* Replace use of old canonicalize

crates/nu-cli/src/path.rs

@@ -25,13 +25,7 @@ pub fn normalize(path: impl AsRef<Path>) -> PathBuf {
     normalized
 }
 
-pub struct AllowMissing(pub bool);
-
-pub fn canonicalize<P, Q>(
-    relative_to: P,
-    path: Q,
-    allow_missing: AllowMissing,
-) -> io::Result<PathBuf>
+fn canonicalize_core<P, Q>(relative_to: P, path: Q) -> PathBuf
 where
     P: AsRef<Path>,
     Q: AsRef<Path>,
@@ -57,7 +51,7 @@ where
         (relative_to.as_ref().to_path_buf(), path)
     };
 
-    let path = if path.is_relative() {
+    if path.is_relative() {
         let mut result = relative_to;
         path.components().for_each(|component| match component {
             Component::ParentDir => {
@@ -70,26 +64,44 @@ where
         result
     } else {
         path
-    };
+    }
+}
 
-    let path = match std::fs::read_link(&path) {
+pub fn canonicalize_existing<P, Q>(relative_to: P, path: Q) -> io::Result<PathBuf>
+where
+    P: AsRef<Path>,
+    Q: AsRef<Path>,
+{
+    let canonicalized = canonicalize_core(relative_to, path);
+    let path = match std::fs::read_link(&canonicalized) {
         Ok(resolved) => resolved,
         Err(e) => {
-            // We are here if path doesn't exist or isn't a symlink
-            if allow_missing.0 || path.exists() {
-                // Return if we allow missing paths or if the path
-                // actually exists, but wasn't a symlink
-                path
+            if canonicalized.exists() {
+                canonicalized
             } else {
                 return Err(e);
             }
         }
     };
 
-    // De-UNC paths
     Ok(dunce::simplified(&path).to_path_buf())
 }
 
+#[allow(dead_code)]
+pub fn canonicalize_missing<P, Q>(relative_to: P, path: Q) -> PathBuf
+where
+    P: AsRef<Path>,
+    Q: AsRef<Path>,
+{
+    let canonicalized = canonicalize_core(relative_to, path);
+    let path = match std::fs::read_link(&canonicalized) {
+        Ok(resolved) => resolved,
+        Err(_) => canonicalized,
+    };
+
+    dunce::simplified(&path).to_path_buf()
+}
+
 #[cfg(test)]
 mod tests {
     use super::*;
@@ -106,62 +118,56 @@ mod tests {
     }
 
     #[test]
-    fn canonicalize_two_dots_and_allow_missing() -> io::Result<()> {
-        let relative_to = Path::new("/foo/bar"); // does not exists
+    fn canonicalize_missing_two_dots() {
+        let relative_to = Path::new("/foo/bar");
         let path = Path::new("..");
 
         assert_eq!(
-            PathBuf::from("/foo"),
-            canonicalize(relative_to, path, AllowMissing(true))?
+            PathBuf::from("/foo"), // missing path
+            canonicalize_missing(relative_to, path)
         );
-
-        Ok(())
     }
 
     #[test]
-    fn canonicalize_three_dots_and_allow_missing() -> io::Result<()> {
-        let relative_to = Path::new("/foo/bar/baz"); // missing path
+    fn canonicalize_missing_three_dots() {
+        let relative_to = Path::new("/foo/bar/baz");
         let path = Path::new("...");
 
         assert_eq!(
-            PathBuf::from("/foo"),
-            canonicalize(relative_to, path, AllowMissing(true))?
+            PathBuf::from("/foo"), // missing path
+            canonicalize_missing(relative_to, path)
         );
-
-        Ok(())
     }
 
     #[test]
-    fn canonicalize_three_dots_with_redundant_dot_and_allow_missing() -> io::Result<()> {
-        let relative_to = Path::new("/foo/bar/baz"); // missing path
+    fn canonicalize_missing_three_dots_with_redundant_dot() {
+        let relative_to = Path::new("/foo/bar/baz");
         let path = Path::new("./...");
 
         assert_eq!(
-            PathBuf::from("/foo"),
-            canonicalize(relative_to, path, AllowMissing(true))?
+            PathBuf::from("/foo"), // missing path
+            canonicalize_missing(relative_to, path)
         );
-
-        Ok(())
     }
 
     #[test]
-    fn canonicalize_three_dots_and_disallow_missing() -> io::Result<()> {
-        let relative_to = Path::new("/foo/bar/"); // root is not missing
+    fn canonicalize_existing_three_dots() -> io::Result<()> {
+        let relative_to = Path::new("/foo/bar/");
         let path = Path::new("...");
 
         assert_eq!(
-            PathBuf::from("/"),
-            canonicalize(relative_to, path, AllowMissing(false))?
+            PathBuf::from("/"), // existing path
+            canonicalize_existing(relative_to, path)?
         );
 
         Ok(())
     }
 
     #[test]
-    fn canonicalize_three_dots_and_disallow_missing_should_fail() {
-        let relative_to = Path::new("/foo/bar/baz"); // foo is missing
+    fn canonicalize_existing_three_dots_should_fail() {
+        let relative_to = Path::new("/foo/bar/baz"); // '/foo' is missing
         let path = Path::new("...");
 
-        assert!(canonicalize(relative_to, path, AllowMissing(false)).is_err());
+        assert!(canonicalize_existing(relative_to, path).is_err());
     }
 }

crates/nu-cli/src/shell/filesystem_shell.rs

@@ -5,7 +5,7 @@ use crate::commands::mkdir::MkdirArgs;
 use crate::commands::mv::MoveArgs;
 use crate::commands::rm::RemoveArgs;
 use crate::data::dir_entry_dict;
-use crate::path::{canonicalize, normalize, AllowMissing};
+use crate::path::{canonicalize_existing, normalize};
 use crate::prelude::*;
 use crate::shell::completer::NuCompleter;
 use crate::shell::shell::Shell;
@@ -188,14 +188,13 @@ impl Shell for FilesystemShell {
                 if target == Path::new("-") {
                     PathBuf::from(&self.last_path)
                 } else {
-                    let path =
-                        canonicalize(self.path(), target, AllowMissing(false)).map_err(|_| {
-                            ShellError::labeled_error(
-                                "Cannot change to directory",
-                                "directory not found",
-                                &v.tag,
-                            )
-                        })?;
+                    let path = canonicalize_existing(self.path(), target).map_err(|_| {
+                        ShellError::labeled_error(
+                            "Cannot change to directory",
+                            "directory not found",
+                            &v.tag,
+                        )
+                    })?;
 
                     if !path.is_dir() {
                         return Err(ShellError::labeled_error(