Allow external args to expand globs (#839)

* Allow external args to expand globs

* WIP

* A bit of cleanups and refactor to glob_from

* oops, add file

Cargo.lock

@@ -1999,6 +1999,7 @@ name = "nu-engine"
 version = "0.1.0"
 dependencies = [
  "chrono",
+ "glob",
  "itertools",
  "nu-path",
  "nu-protocol",

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

@@ -3,7 +3,6 @@ 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::{
@@ -13,7 +12,7 @@ use nu_protocol::{
 
 #[cfg(unix)]
 use std::os::unix::fs::PermissionsExt;
-use std::path::{Component, PathBuf};
+use std::path::PathBuf;
 
 #[derive(Clone)]
 pub struct Ls;
@@ -71,82 +70,18 @@ impl Command for Ls {
 
         let pattern_arg = call.opt::<Spanned<String>>(engine_state, stack, 0)?;
 
-        let (prefix, pattern) = if let Some(arg) = pattern_arg {
+        let pattern = if let Some(pattern) = pattern_arg {
-            let path = PathBuf::from(arg.item);
+            pattern
-            let path = if path.is_relative() {
-                expand_path_with(path, &cwd)
-            } else {
-                path
-            };
-
-            if path.to_string_lossy().contains('*') {
-                // Path is a glob pattern => do not check for existence
-                // Select the longest prefix until the first '*'
-                let mut p = PathBuf::new();
-                for c in path.components() {
-                    if let Component::Normal(os) = c {
-                        if os.to_string_lossy().contains('*') {
-                            break;
-                        }
-                    }
-                    p.push(c);
-                }
-                (Some(p), path)
-            } else {
-                let path = if let Ok(p) = canonicalize_with(path, &cwd) {
-                    p
-                } else {
-                    return Err(ShellError::DirectoryNotFound(arg.span));
-                };
-
-                if path.is_dir() {
-                    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));
-                    }
-
-                    (Some(path.clone()), path.join("*"))
-                } else {
-                    (path.parent().map(|parent| parent.to_path_buf()), path)
-                }
-            }
         } else {
-            (Some(cwd.clone()), cwd.join("*"))
+            Spanned {
+                item: cwd.join("*").to_string_lossy().to_string(),
+                span: call_span,
+            }
         };
 
-        let pattern = pattern.to_string_lossy().to_string();
+        let (prefix, glob) = nu_engine::glob_from(&pattern, &cwd, call_span)?;
 
-        let glob = glob::glob(&pattern).map_err(|err| {
+        let hidden_dir_specified = is_hidden_dir(&pattern.item);
-            nu_protocol::ShellError::SpannedLabeledError(
-                "Error extracting glob pattern".into(),
-                err.to_string(),
-                call.head,
-            )
-        })?;
-
-        let hidden_dir_specified = is_hidden_dir(&pattern);
         let mut hidden_dirs = vec![];
 
         Ok(glob
@@ -218,13 +153,6 @@ impl Command for Ls {
     }
 }
 
-fn permission_denied(dir: impl AsRef<Path>) -> bool {
-    match dir.as_ref().read_dir() {
-        Err(e) => matches!(e.kind(), std::io::ErrorKind::PermissionDenied),
-        Ok(_) => false,
-    }
-}
-
 fn is_hidden_dir(dir: impl AsRef<Path>) -> bool {
     #[cfg(windows)]
     {
@@ -248,13 +176,6 @@ fn is_hidden_dir(dir: impl AsRef<Path>) -> bool {
     }
 }
 
-fn is_empty_dir(dir: impl AsRef<Path>) -> bool {
-    match dir.as_ref().read_dir() {
-        Err(_) => true,
-        Ok(mut s) => s.next().is_none(),
-    }
-}
-
 fn path_contains_hidden_folder(path: &Path, folders: &[PathBuf]) -> bool {
     let path_str = path.to_str().expect("failed to read path");
     if folders

crates/nu-command/src/system/run_external.rs

@@ -1,5 +1,6 @@
 use std::collections::HashMap;
 use std::io::{BufRead, BufReader, Write};
+use std::path::PathBuf;
 use std::process::{Command as CommandSys, Stdio};
 use std::sync::atomic::Ordering;
 use std::sync::mpsc;
@@ -7,11 +8,12 @@ use std::sync::mpsc;
 use nu_engine::env_to_strings;
 use nu_protocol::engine::{EngineState, Stack};
 use nu_protocol::{ast::Call, engine::Command, ShellError, Signature, SyntaxShape, Value};
-use nu_protocol::{ByteStream, Category, Config, PipelineData, Spanned};
+use nu_protocol::{ByteStream, Category, Config, PipelineData, Span, Spanned};
 
 use itertools::Itertools;
 
 use nu_engine::CallExt;
+use pathdiff::diff_paths;
 use regex::Regex;
 
 const OUTPUT_BUFFER_SIZE: usize = 1024;
@@ -57,13 +59,19 @@ impl Command for External {
         let mut args_strs = vec![];
 
         for arg in args {
+            let span = if let Ok(span) = arg.span() {
+                span
+            } else {
+                Span { start: 0, end: 0 }
+            };
+
             if let Ok(s) = arg.as_string() {
-                args_strs.push(s);
+                args_strs.push(Spanned { item: s, span });
-            } else if let Value::List { vals, .. } = arg {
+            } else if let Value::List { vals, span } = arg {
                 // Interpret a list as a series of arguments
                 for val in vals {
                     if let Ok(s) = val.as_string() {
-                        args_strs.push(s);
+                        args_strs.push(Spanned { item: s, span });
                     } else {
                         return Err(ShellError::ExternalCommand(
                             "Cannot convert argument to a string".into(),
@@ -95,7 +103,7 @@ impl Command for External {
 
 pub struct ExternalCommand<'call> {
     pub name: Spanned<String>,
-    pub args: Vec<String>,
+    pub args: Vec<Spanned<String>>,
     pub last_expression: bool,
     pub env_vars: HashMap<String, String>,
     pub call: &'call Call,
@@ -113,7 +121,7 @@ impl<'call> ExternalCommand<'call> {
         let ctrlc = engine_state.ctrlc.clone();
 
         let mut process = if let Some(d) = self.env_vars.get("PWD") {
-            let mut process = self.create_command(d);
+            let mut process = self.create_command(d)?;
             process.current_dir(d);
             process
         } else {
@@ -248,26 +256,26 @@ impl<'call> ExternalCommand<'call> {
         }
     }
 
-    fn create_command(&self, cwd: &str) -> CommandSys {
+    fn create_command(&self, cwd: &str) -> Result<CommandSys, ShellError> {
         // in all the other cases shell out
         if cfg!(windows) {
             //TODO. This should be modifiable from the config file.
             // We could give the option to call from powershell
             // for minimal builds cwd is unused
             if self.name.item.ends_with(".cmd") || self.name.item.ends_with(".bat") {
-                self.spawn_cmd_command()
+                Ok(self.spawn_cmd_command())
             } else {
                 self.spawn_simple_command(cwd)
             }
         } else if self.name.item.ends_with(".sh") {
-            self.spawn_sh_command()
+            Ok(self.spawn_sh_command())
         } else {
             self.spawn_simple_command(cwd)
         }
     }
 
     /// Spawn a command without shelling out to an external shell
-    fn spawn_simple_command(&self, cwd: &str) -> std::process::Command {
+    fn spawn_simple_command(&self, cwd: &str) -> Result<std::process::Command, ShellError> {
         let head = trim_enclosing_quotes(&self.name.item);
         let head = if head.starts_with('~') || head.starts_with("..") {
             nu_path::expand_path_with(head, cwd)
@@ -293,32 +301,92 @@ impl<'call> ExternalCommand<'call> {
 
         let mut process = std::process::Command::new(&new_head);
 
-        for arg in &self.args {
+        for arg in self.args.iter() {
-            let arg = trim_enclosing_quotes(arg);
+            let arg = Spanned {
-            let arg = if arg.starts_with('~') || arg.starts_with("..") {
+                item: trim_enclosing_quotes(&arg.item),
-                nu_path::expand_path_with(arg, cwd)
+                span: arg.span,
-                    .to_string_lossy()
-                    .to_string()
-            } else {
-                arg
             };
 
-            let new_arg;
+            let cwd = PathBuf::from(cwd);
 
-            #[cfg(windows)]
+            if arg.item.contains('*') {
-            {
+                if let Ok((prefix, matches)) = nu_engine::glob_from(&arg, &cwd, self.name.span) {
-                new_arg = arg.replace("\\", "\\\\");
+                    let matches: Vec<_> = matches.collect();
+
+                    // Following shells like bash, if we can't expand a glob pattern, we don't assume an empty arg
+                    // Instead, we throw an error. This helps prevent issues with things like `ls unknowndir/*` accidentally
+                    // listening the current directory.
+                    if matches.is_empty() {
+                        return Err(ShellError::FileNotFoundCustom(
+                            "pattern not found".to_string(),
+                            arg.span,
+                        ));
+                    }
+                    for m in matches {
+                        if let Ok(arg) = m {
+                            let arg = if let Some(prefix) = &prefix {
+                                if let Ok(remainder) = arg.strip_prefix(&prefix) {
+                                    let new_prefix = if let Some(pfx) = diff_paths(&prefix, &cwd) {
+                                        pfx
+                                    } else {
+                                        prefix.to_path_buf()
+                                    };
+
+                                    new_prefix.join(remainder).to_string_lossy().to_string()
+                                } else {
+                                    arg.to_string_lossy().to_string()
+                                }
+                            } else {
+                                arg.to_string_lossy().to_string()
+                            };
+                            let new_arg;
+
+                            #[cfg(windows)]
+                            {
+                                new_arg = arg.replace("\\", "\\\\");
+                            }
+
+                            #[cfg(not(windows))]
+                            {
+                                new_arg = arg;
+                            }
+
+                            process.arg(&new_arg);
+                        } else {
+                            let new_arg;
+
+                            #[cfg(windows)]
+                            {
+                                new_arg = arg.item.replace("\\", "\\\\");
+                            }
+
+                            #[cfg(not(windows))]
+                            {
+                                new_arg = arg.item.clone();
+                            }
+
+                            process.arg(&new_arg);
+                        }
+                    }
+                }
+            } else {
+                let new_arg;
+
+                #[cfg(windows)]
+                {
+                    new_arg = arg.item.replace("\\", "\\\\");
+                }
+
+                #[cfg(not(windows))]
+                {
+                    new_arg = arg.item;
+                }
+
+                process.arg(&new_arg);
             }
-
-            #[cfg(not(windows))]
-            {
-                new_arg = arg;
-            }
-
-            process.arg(&new_arg);
         }
 
-        process
+        Ok(process)
     }
 
     /// Spawn a cmd command with `cmd /c args...`
@@ -330,7 +398,7 @@ impl<'call> ExternalCommand<'call> {
             // Clean the args before we use them:
             // https://stackoverflow.com/questions/1200235/how-to-pass-a-quoted-pipe-character-to-cmd-exe
             // cmd.exe needs to have a caret to escape a pipe
-            let arg = arg.replace("|", "^|");
+            let arg = arg.item.replace("|", "^|");
             process.arg(&arg);
         }
         process
@@ -338,8 +406,11 @@ impl<'call> ExternalCommand<'call> {
 
     /// Spawn a sh command with `sh -c args...`
     fn spawn_sh_command(&self) -> std::process::Command {
-        let joined_and_escaped_arguments =
+        let joined_and_escaped_arguments = self
-            self.args.iter().map(|arg| shell_arg_escape(arg)).join(" ");
+            .args
+            .iter()
+            .map(|arg| shell_arg_escape(&arg.item))
+            .join(" ");
         let cmd_with_args = vec![self.name.item.clone(), joined_and_escaped_arguments].join(" ");
         let mut process = std::process::Command::new("sh");
         process.arg("-c").arg(cmd_with_args);

crates/nu-engine/Cargo.toml

@@ -8,6 +8,7 @@ nu-protocol = { path = "../nu-protocol", features = ["plugin"] }
 nu-path = { path = "../nu-path" }
 itertools = "0.10.1"
 chrono = { version="0.4.19", features=["serde"] }
+glob = "0.3.0"
 
 [features]
 plugin = []

crates/nu-engine/src/glob_from.rs

@@ -0,0 +1,121 @@
+#[cfg(unix)]
+use std::os::unix::fs::PermissionsExt;
+use std::path::{Component, Path, PathBuf};
+
+use nu_path::{canonicalize_with, expand_path_with};
+use nu_protocol::{ShellError, Span, Spanned};
+
+/// This function is like `glob::glob` from the `glob` crate, except it is relative to a given cwd.
+///
+/// It returns a tuple of two values: the first is an optional prefix that the expanded filenames share.
+/// This prefix can be removed from the front of each value to give an approximation of the relative path
+/// to the user
+///
+/// The second of the two values is an iterator over the matching filepaths.
+#[allow(clippy::type_complexity)]
+pub fn glob_from(
+    pattern: &Spanned<String>,
+    cwd: &Path,
+    span: Span,
+) -> Result<
+    (
+        Option<PathBuf>,
+        Box<dyn Iterator<Item = Result<PathBuf, ShellError>> + Send>,
+    ),
+    ShellError,
+> {
+    let path = PathBuf::from(&pattern.item);
+    let path = if path.is_relative() {
+        expand_path_with(path, cwd)
+    } else {
+        path
+    };
+
+    let (prefix, pattern) = if path.to_string_lossy().contains('*') {
+        // Path is a glob pattern => do not check for existence
+        // Select the longest prefix until the first '*'
+        let mut p = PathBuf::new();
+        for c in path.components() {
+            if let Component::Normal(os) = c {
+                if os.to_string_lossy().contains('*') {
+                    break;
+                }
+            }
+            p.push(c);
+        }
+        (Some(p), path)
+    } else {
+        let path = if let Ok(p) = canonicalize_with(path, &cwd) {
+            p
+        } else {
+            return Err(ShellError::DirectoryNotFound(pattern.span));
+        };
+
+        if path.is_dir() {
+            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,
+                    pattern.span,
+                ));
+            }
+
+            if is_empty_dir(&path) {
+                return Ok((Some(path), Box::new(vec![].into_iter())));
+            }
+
+            (Some(path.clone()), path.join("*"))
+        } else {
+            (path.parent().map(|parent| parent.to_path_buf()), path)
+        }
+    };
+
+    let pattern = pattern.to_string_lossy().to_string();
+
+    let glob = glob::glob(&pattern).map_err(|err| {
+        nu_protocol::ShellError::SpannedLabeledError(
+            "Error extracting glob pattern".into(),
+            err.to_string(),
+            span,
+        )
+    })?;
+
+    Ok((
+        prefix,
+        Box::new(glob.map(move |x| match x {
+            Ok(v) => Ok(v),
+            Err(err) => Err(nu_protocol::ShellError::SpannedLabeledError(
+                "Error extracting glob pattern".into(),
+                err.to_string(),
+                span,
+            )),
+        })),
+    ))
+}
+
+fn permission_denied(dir: impl AsRef<Path>) -> bool {
+    match dir.as_ref().read_dir() {
+        Err(e) => matches!(e.kind(), std::io::ErrorKind::PermissionDenied),
+        Ok(_) => false,
+    }
+}
+
+fn is_empty_dir(dir: impl AsRef<Path>) -> bool {
+    match dir.as_ref().read_dir() {
+        Err(_) => true,
+        Ok(mut s) => s.next().is_none(),
+    }
+}

crates/nu-engine/src/lib.rs

@@ -3,9 +3,11 @@ pub mod column;
 mod documentation;
 pub mod env;
 mod eval;
+mod glob_from;
 
 pub use call_ext::CallExt;
 pub use column::get_columns;
 pub use documentation::{generate_docs, get_brief_help, get_documentation, get_full_help};
 pub use env::*;
 pub use eval::{eval_block, eval_expression, eval_operator};
+pub use glob_from::glob_from;