Redesign custom canonicalize, enable multiple dots expansion earlier. (#1588)

* Expand n dots early where tilde was also expanded.

* Remove normalize, not needed.
New function absolutize, doesn't follow links neither checks existence.
Renamed canonicalize_existing to canonicalize, works as expected.

* Remove normalize usages, change canonicalize.

* Treat strings as paths

crates/nu-cli/src/path.rs

@@ -1,37 +1,14 @@
 use std::io;
 use std::path::{Component, Path, PathBuf};
 
-pub fn normalize(path: impl AsRef<Path>) -> PathBuf {
+pub fn absolutize<P, Q>(relative_to: P, path: Q) -> PathBuf
-    let mut normalized = PathBuf::new();
-    for component in path.as_ref().components() {
-        match component {
-            Component::Normal(normal) => {
-                if let Some(normal) = normal.to_str() {
-                    if normal.chars().all(|c| c == '.') {
-                        for _ in 0..(normal.len() - 1) {
-                            normalized.push("..");
-                        }
-                    } else {
-                        normalized.push(normal);
-                    }
-                } else {
-                    normalized.push(normal);
-                }
-            }
-            c => normalized.push(c.as_os_str()),
-        }
-    }
-
-    normalized
-}
-
-fn canonicalize_core<P, Q>(relative_to: P, path: Q) -> PathBuf
 where
     P: AsRef<Path>,
     Q: AsRef<Path>,
 {
-    let path = normalize(path);
+    let path = relative_to.as_ref().join(path);
-    let (relative_to, path) = if path.is_absolute() {
+
+    let (relative_to, path) = {
         let components: Vec<_> = path.components().collect();
         let separator = components
             .iter()
@@ -47,11 +24,9 @@ where
         } else {
             (relative_to.as_ref().to_path_buf(), path)
         }
-    } else {
-        (relative_to.as_ref().to_path_buf(), path)
     };
 
-    if path.is_relative() {
+    let path = if path.is_relative() {
         let mut result = relative_to;
         path.components().for_each(|component| match component {
             Component::ParentDir => {
@@ -64,15 +39,17 @@ where
         result
     } else {
         path
-    }
+    };
+
+    dunce::simplified(&path).to_path_buf()
 }
 
-pub fn canonicalize_existing<P, Q>(relative_to: P, path: Q) -> io::Result<PathBuf>
+pub fn canonicalize<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 canonicalized = absolutize(relative_to, path);
     let path = match std::fs::read_link(&canonicalized) {
         Ok(resolved) => resolved,
         Err(e) => {
@@ -87,87 +64,40 @@ where
     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::*;
     use std::io;
 
     #[test]
-    fn normalize_three_dots() {
+    fn absolutize_two_dots() {
-        assert_eq!(PathBuf::from("../.."), normalize("..."));
-    }
-
-    #[test]
-    fn normalize_three_dots_with_redundant_dot() {
-        assert_eq!(PathBuf::from("./../.."), normalize("./..."));
-    }
-
-    #[test]
-    fn canonicalize_missing_two_dots() {
         let relative_to = Path::new("/foo/bar");
         let path = Path::new("..");
 
         assert_eq!(
             PathBuf::from("/foo"), // missing path
-            canonicalize_missing(relative_to, path)
+            absolutize(relative_to, path)
         );
     }
 
     #[test]
-    fn canonicalize_missing_three_dots() {
+    fn canonicalize_should_succeed() -> io::Result<()> {
-        let relative_to = Path::new("/foo/bar/baz");
+        let relative_to = Path::new("/foo/bar");
-        let path = Path::new("...");
+        let path = Path::new("../..");
-
-        assert_eq!(
-            PathBuf::from("/foo"), // missing path
-            canonicalize_missing(relative_to, path)
-        );
-    }
-
-    #[test]
-    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"), // missing path
-            canonicalize_missing(relative_to, path)
-        );
-    }
-
-    #[test]
-    fn canonicalize_existing_three_dots() -> io::Result<()> {
-        let relative_to = Path::new("/foo/bar/");
-        let path = Path::new("...");
 
         assert_eq!(
             PathBuf::from("/"), // existing path
-            canonicalize_existing(relative_to, path)?
+            canonicalize(relative_to, path)?,
         );
 
         Ok(())
     }
 
     #[test]
-    fn canonicalize_existing_three_dots_should_fail() {
+    fn canonicalize_should_fail() {
         let relative_to = Path::new("/foo/bar/baz"); // '/foo' is missing
-        let path = Path::new("...");
+        let path = Path::new("../..");
 
-        assert!(canonicalize_existing(relative_to, path).is_err());
+        assert!(canonicalize(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_existing, normalize};
+use crate::path::canonicalize;
 use crate::prelude::*;
 use crate::shell::completer::NuCompleter;
 use crate::shell::shell::Shell;
@@ -105,7 +105,7 @@ impl Shell for FilesystemShell {
         let (path, p_tag) = match path {
             Some(p) => {
                 let p_tag = p.tag;
-                let mut p = normalize(p.item);
+                let mut p = p.item;
                 if p.is_dir() {
                     if is_empty_dir(&p) {
                         return Ok(OutputStream::empty());
@@ -188,7 +188,7 @@ impl Shell for FilesystemShell {
                 if target == Path::new("-") {
                     PathBuf::from(&self.last_path)
                 } else {
-                    let path = canonicalize_existing(self.path(), target).map_err(|_| {
+                    let path = canonicalize(self.path(), target).map_err(|_| {
                         ShellError::labeled_error(
                             "Cannot change to directory",
                             "directory not found",
@@ -255,8 +255,8 @@ impl Shell for FilesystemShell {
         let name_tag = name;
 
         let path = Path::new(path);
-        let source = normalize(path.join(&src.item));
+        let source = path.join(&src.item);
-        let mut destination = normalize(path.join(&dst.item));
+        let mut destination = path.join(&dst.item);
 
         let sources: Vec<_> = match glob::glob(&source.to_string_lossy()) {
             Ok(files) => files.collect(),
@@ -535,7 +535,7 @@ impl Shell for FilesystemShell {
         }
 
         for dir in directories.iter() {
-            let create_at = normalize(path.join(&dir.item));
+            let create_at = path.join(&dir.item);
 
             let dir_res = std::fs::create_dir_all(create_at);
             if let Err(reason) = dir_res {
@@ -559,8 +559,8 @@ impl Shell for FilesystemShell {
         let name_tag = name;
 
         let path = Path::new(path);
-        let source = normalize(path.join(&src.item));
+        let source = path.join(&src.item);
-        let mut destination = normalize(path.join(&dst.item));
+        let mut destination = path.join(&dst.item);
 
         let sources: Vec<_> = match glob::glob(&source.to_string_lossy()) {
             Ok(files) => files.collect(),
@@ -978,7 +978,7 @@ impl Shell for FilesystemShell {
                 ));
             }
 
-            let path = normalize(path.join(&target.item));
+            let path = path.join(&target.item);
             match glob::glob(&path.to_string_lossy()) {
                 Ok(files) => {
                     for file in files {

crates/nu-parser/src/lib.rs

@@ -1,6 +1,7 @@
 mod files;
 mod lite_parse;
 mod parse;
+mod path;
 mod shapes;
 mod signature;
 

crates/nu-parser/src/parse.rs

@@ -1,6 +1,7 @@
 use std::path::Path;
 
 use crate::lite_parse::{lite_parse, LiteCommand, LitePipeline};
+use crate::path::expand_path;
 use crate::signature::SignatureRegistry;
 use nu_errors::{ArgumentError, ParseError};
 use nu_protocol::hir::{
@@ -373,7 +374,7 @@ fn parse_arg(
         }
         SyntaxShape::Pattern => {
             let trimmed = trim_quotes(&lite_arg.item);
-            let expanded = shellexpand::tilde(&trimmed).to_string();
+            let expanded = expand_path(&trimmed);
             (
                 SpannedExpression::new(Expression::pattern(expanded), lite_arg.span),
                 None,
@@ -385,7 +386,7 @@ fn parse_arg(
         SyntaxShape::Unit => parse_unit(&lite_arg),
         SyntaxShape::Path => {
             let trimmed = trim_quotes(&lite_arg.item);
-            let expanded = shellexpand::tilde(&trimmed).to_string();
+            let expanded = expand_path(&trimmed);
             let path = Path::new(&expanded);
             (
                 SpannedExpression::new(Expression::FilePath(path.to_path_buf()), lite_arg.span),
@@ -844,7 +845,7 @@ pub fn classify_pipeline(
             commands.push(ClassifiedCommand::Internal(internal_command))
         } else {
             let trimmed = trim_quotes(&lite_cmd.name.item);
-            let name = shellexpand::tilde(&trimmed).to_string();
+            let name = expand_path(&trimmed);
             // This is an external command we should allow arguments to pass through with minimal parsing
             commands.push(ClassifiedCommand::External(ExternalCommand {
                 name,

crates/nu-parser/src/path.rs

@@ -0,0 +1,52 @@
+use std::path::{Component, Path, PathBuf};
+
+fn expand_ndots(path: &str) -> String {
+    let path = Path::new(path);
+    let mut expanded = PathBuf::new();
+
+    for component in path.components() {
+        match component {
+            Component::Normal(normal) => {
+                if let Some(normal) = normal.to_str() {
+                    if normal.chars().all(|c| c == '.') {
+                        for _ in 0..(normal.len() - 1) {
+                            expanded.push("..");
+                        }
+                    } else {
+                        expanded.push(normal);
+                    }
+                } else {
+                    expanded.push(normal);
+                }
+            }
+
+            c => expanded.push(c.as_os_str()),
+        }
+    }
+
+    expanded.to_string_lossy().to_string()
+}
+
+pub fn expand_path(path: &str) -> String {
+    let tilde_expansion = shellexpand::tilde(path);
+    expand_ndots(&tilde_expansion)
+}
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+
+    #[test]
+    fn expand_in_relative_path() {
+        let expected = Path::new("../..");
+        let expanded = PathBuf::from(expand_path("..."));
+        assert_eq!(expected, &expanded);
+    }
+
+    #[test]
+    fn expand_in_absolute_path() {
+        let expected = Path::new("/foo/../..");
+        let expanded = PathBuf::from(expand_path("/foo/..."));
+        assert_eq!(expected, &expanded);
+    }
+}