Fix ls relative path & command argument path expansion (#757)

* Switch to short-names when the path is a relative_path (a dir) and exit with an error if the path does not exist

* Remove debugging print line

* Show relative filenames... It does not work yet for ls ../

* Try something else to fix relative paths... it works, but the ../ code part is not very pretty

* Add canonicalize check and remove code clones

* Fix the canonicalize_with issue pointed out by kubouch. Not sure the prefix_str is what kubouch suggested

* Fix the canonicalize_with issue pointed out by kubouch. Not sure the prefix_str is what kubouch suggested

* Add single-dot expansion to nu-path

* Move value path expansion from parser to eval

Fixes #745

* Remove single dot expansion from parser

It is not necessary since it will get expanded anyway in the eval.

* Fix ls to display globs with relative paths

* Use pathdiff crate to get relative paths for ls

Co-authored-by: Stefan Stanciulescu <contact@stefanstanciulescu.com>

Cargo.lock

@@ -1954,6 +1954,7 @@ dependencies = [
  "nu-table",
  "nu-term-grid",
  "num 0.4.0",
+ "pathdiff",
  "polars",
  "quick-xml 0.22.0",
  "rand",
@@ -2430,6 +2431,12 @@ dependencies = [
  "regex",
 ]
 
+[[package]]
+name = "pathdiff"
+version = "0.2.1"
+source = "registry+https://github.com/rust-lang/crates.io-index"
+checksum = "8835116a5c179084a830efb3adc117ab007512b535bc1a21c991d3b32a6b44dd"
+
 [[package]]
 name = "percent-encoding"
 version = "2.1.0"

crates/nu-command/Cargo.toml

@@ -25,6 +25,7 @@ nu-color-config = { path = "../nu-color-config" }
 url = "2.2.1"
 csv = "1.1.3"
 glob = "0.3.0"
+pathdiff = "0.2.1"
 Inflector = "0.11"
 thiserror = "1.0.29"
 sysinfo = "0.22.2"

crates/nu-command/src/filesystem/ls.rs

@@ -1,6 +1,9 @@
 use chrono::{DateTime, Utc};
+use pathdiff::diff_paths;
+
 use nu_engine::env::current_dir;
 use nu_engine::CallExt;
+use nu_path::{canonicalize_with, expand_path_with};
 use nu_protocol::ast::Call;
 use nu_protocol::engine::{Command, EngineState, Stack};
 use nu_protocol::{
@@ -15,7 +18,6 @@ use std::path::PathBuf;
 #[derive(Clone)]
 pub struct Ls;
 
-//NOTE: this is not a real implementation :D. It's just a simple one to test with until we port the real one.
 impl Command for Ls {
     fn name(&self) -> &str {
         "ls"
@@ -43,6 +45,7 @@ impl Command for Ls {
                 "Only print the file names and not the path",
                 Some('s'),
             )
+            .switch("full-paths", "display paths as absolute paths", Some('f'))
             // .switch(
             //     "du",
             //     "Display the apparent directory size in place of the directory metadata size",
@@ -61,54 +64,69 @@ impl Command for Ls {
         let all = call.has_flag("all");
         let long = call.has_flag("long");
         let short_names = call.has_flag("short-names");
+        let full_paths = call.has_flag("full-paths");
 
         let call_span = call.head;
+        let cwd = current_dir(engine_state, stack)?;
 
-        let (pattern, prefix) = if let Some(result) =
+        let pattern_arg = call.opt::<Spanned<String>>(engine_state, stack, 0)?;
-            call.opt::<Spanned<String>>(engine_state, stack, 0)?
-        {
-            let path = PathBuf::from(&result.item);
 
-            let (mut path, prefix) = if path.is_relative() {
+        let pattern = if let Some(arg) = pattern_arg {
-                let cwd = current_dir(engine_state, stack)?;
+            let path = PathBuf::from(arg.item);
-                (cwd.join(path), Some(cwd))
+            let path = if path.is_relative() {
+                expand_path_with(path, &cwd)
             } else {
-                (path, None)
+                path
             };
 
-            if path.is_dir() {
+            if path.to_string_lossy().contains('*') {
-                if permission_denied(&path) {
+                // Path is a glob pattern => do not check for existence
-                    #[cfg(unix)]
+                path
-                    let error_msg = format!(
+            } else {
-                        "The permissions of {:o} do not allow access for this user",
+                let path = if let Ok(p) = canonicalize_with(path, &cwd) {
-                        path.metadata()
+                    p
-                            .expect("this shouldn't be called since we already know there is a dir")
+                } else {
-                            .permissions()
+                    return Err(ShellError::DirectoryNotFound(arg.span));
-                            .mode()
+                };
-                            & 0o0777
-                    );
-                    #[cfg(not(unix))]
-                    let error_msg = String::from("Permission denied");
-                    return Err(ShellError::SpannedLabeledError(
-                        "Permission denied".into(),
-                        error_msg,
-                        result.span,
-                    ));
-                }
-                if is_empty_dir(&path) {
-                    return Ok(PipelineData::new(call_span));
-                }
 
                 if path.is_dir() {
-                    path = path.join("*");
+                    if permission_denied(&path) {
+                        #[cfg(unix)]
+                        let error_msg = format!(
+                            "The permissions of {:o} do not allow access for this user",
+                            path.metadata()
+                                .expect(
+                                    "this shouldn't be called since we already know there is a dir"
+                                )
+                                .permissions()
+                                .mode()
+                                & 0o0777
+                        );
+
+                        #[cfg(not(unix))]
+                        let error_msg = String::from("Permission denied");
+
+                        return Err(ShellError::SpannedLabeledError(
+                            "Permission denied".into(),
+                            error_msg,
+                            arg.span,
+                        ));
+                    }
+
+                    if is_empty_dir(&path) {
+                        return Ok(PipelineData::new(call_span));
+                    }
+
+                    path.join("*")
+                } else {
+                    path
                 }
             }
-
-            (path.to_string_lossy().to_string(), prefix)
         } else {
-            let cwd = current_dir(engine_state, stack)?;
+            cwd.join("*")
-            (cwd.join("*").to_string_lossy().to_string(), Some(cwd))
+        }
-        };
+        .to_string_lossy()
+        .to_string();
 
         let glob = glob::glob(&pattern).map_err(|err| {
             nu_protocol::ShellError::SpannedLabeledError(
@@ -141,14 +159,13 @@ impl Command for Ls {
                     }
 
                     let display_name = if short_names {
-                        path.file_name().and_then(|s| s.to_str())
+                        path.file_name().map(|os| os.to_string_lossy().to_string())
-                    } else if let Some(pre) = &prefix {
+                    } else if full_paths {
-                        match path.strip_prefix(pre) {
+                        Some(path.to_string_lossy().to_string())
-                            Ok(stripped) => stripped.to_str(),
-                            Err(_) => path.to_str(),
-                        }
                     } else {
-                        path.to_str()
+                        diff_paths(&path, &cwd)
+                            .or_else(|| Some(path.clone()))
+                            .map(|p| p.to_string_lossy().to_string())
                     }
                     .ok_or_else(|| {
                         ShellError::SpannedLabeledError(
@@ -161,8 +178,7 @@ impl Command for Ls {
                     match display_name {
                         Ok(name) => {
                             let entry =
-                                dir_entry_dict(&path, name, metadata.as_ref(), call_span, long);
+                                dir_entry_dict(&path, &name, metadata.as_ref(), call_span, long);
-
                             match entry {
                                 Ok(value) => Some(value),
                                 Err(err) => Some(Value::Error { error: err }),

crates/nu-engine/src/eval.rs

@@ -2,6 +2,7 @@ use std::cmp::Ordering;
 use std::collections::HashMap;
 use std::io::Write;
 
+use nu_path::expand_path_with;
 use nu_protocol::ast::{Block, Call, Expr, Expression, Operator, Statement};
 use nu_protocol::engine::{EngineState, Stack};
 use nu_protocol::{
@@ -375,14 +376,24 @@ pub fn eval_expression(
             val: s.clone(),
             span: expr.span,
         }),
-        Expr::Filepath(s) => Ok(Value::String {
+        Expr::Filepath(s) => {
-            val: s.clone(),
+            let cwd = current_dir_str(engine_state, stack)?;
-            span: expr.span,
+            let path = expand_path_with(s, cwd);
-        }),
+
-        Expr::GlobPattern(s) => Ok(Value::String {
+            Ok(Value::String {
-            val: s.clone(),
+                val: path.to_string_lossy().to_string(),
-            span: expr.span,
+                span: expr.span,
-        }),
+            })
+        }
+        Expr::GlobPattern(s) => {
+            let cwd = current_dir_str(engine_state, stack)?;
+            let path = expand_path_with(s, cwd);
+
+            Ok(Value::String {
+                val: path.to_string_lossy().to_string(),
+                span: expr.span,
+            })
+        }
         Expr::Signature(_) => Ok(Value::Nothing { span: expr.span }),
         Expr::Garbage => Ok(Value::Nothing { span: expr.span }),
         Expr::Nothing => Ok(Value::Nothing { span: expr.span }),

crates/nu-parser/src/parser.rs

@@ -1616,20 +1616,15 @@ pub fn parse_filepath(
     working_set: &mut StateWorkingSet,
     span: Span,
 ) -> (Expression, Option<ParseError>) {
-    let cwd = working_set.get_cwd();
-
     let bytes = working_set.get_span_contents(span);
     let bytes = trim_quotes(bytes);
     trace!("parsing: filepath");
 
     if let Ok(token) = String::from_utf8(bytes.into()) {
-        let filepath = nu_path::expand_path_with(token, cwd);
+        trace!("-- found {}", token);
-        let filepath = filepath.to_string_lossy().to_string();
-        trace!("-- found {}", filepath);
-
         (
             Expression {
-                expr: Expr::Filepath(filepath),
+                expr: Expr::Filepath(token),
                 span,
                 ty: Type::String,
                 custom_completion: None,
@@ -1849,15 +1844,10 @@ pub fn parse_glob_pattern(
     let bytes = trim_quotes(bytes);
 
     if let Ok(token) = String::from_utf8(bytes.into()) {
-        let cwd = working_set.get_cwd();
         trace!("-- found {}", token);
-
-        let filepath = nu_path::expand_path_with(token, cwd);
-        let filepath = filepath.to_string_lossy().to_string();
-
         (
             Expression {
-                expr: Expr::GlobPattern(filepath),
+                expr: Expr::GlobPattern(token),
                 span,
                 ty: Type::String,
                 custom_completion: None,

crates/nu-protocol/src/value/from_value.rs

@@ -1,14 +1,11 @@
-// use std::path::PathBuf;
-
 use std::path::PathBuf;
 use std::str::FromStr;
 
-use chrono::{DateTime, FixedOffset};
-// use nu_path::expand_path;
 use crate::ast::{CellPath, PathMember};
 use crate::engine::CaptureBlock;
 use crate::ShellError;
 use crate::{Range, Spanned, Value};
+use chrono::{DateTime, FixedOffset};
 
 pub trait FromValue: Sized {
     fn from_value(v: &Value) -> Result<Self, ShellError>;