ls will return error if no files/folders match path/pattern (#1286)

* `ls` will return error if no files/folders match path/pattern

* Revert changes to src/data/files.rs

* Add a name_only flag to dir_entry_dict

Add name_only flag to indicate if the caller only cares about filenames
or wants the whole path

* Update ls changes from feedback

* Little cleanup

* Resolve merge conflicts

* lints

src/data/files.rs

@@ -8,9 +8,25 @@ pub(crate) fn dir_entry_dict(
     tag: impl Into<Tag>,
     full: bool,
     with_symlink_targets: bool,
+    name_only: bool,
 ) -> Result<Value, ShellError> {
-    let mut dict = TaggedDictBuilder::new(tag);
+    let tag = tag.into();
-    dict.insert_untagged("name", UntaggedValue::string(filename.to_string_lossy()));
+    let mut dict = TaggedDictBuilder::new(&tag);
+
+    let name = if name_only {
+        filename.file_name().and_then(|s| s.to_str())
+    } else {
+        filename.to_str()
+    }
+    .ok_or_else(|| {
+        ShellError::labeled_error(
+            format!("Invalid file name: {:}", filename.to_string_lossy()),
+            "invalid file name",
+            tag,
+        )
+    })?;
+
+    dict.insert_untagged("name", UntaggedValue::string(name));
 
     if metadata.is_dir() {
         dict.insert_untagged("type", UntaggedValue::string("Dir"));

src/shell/filesystem_shell.rs

@@ -14,7 +14,7 @@ use nu_parser::ExpandContext;
 use nu_protocol::{Primitive, ReturnSuccess, UntaggedValue};
 use rustyline::completion::FilenameCompleter;
 use rustyline::hint::{Hinter, HistoryHinter};
-use std::path::{Path, PathBuf};
+use std::path::PathBuf;
 use std::sync::atomic::Ordering;
 use trash as SendToTrash;
 
@@ -96,132 +96,60 @@ impl Shell for FilesystemShell {
         }: LsArgs,
         context: &RunnablePerItemContext,
     ) -> Result<OutputStream, ShellError> {
-        let cwd = self.path();
-        let mut full_path = PathBuf::from(self.path());
-
-        if let Some(value) = &path {
-            full_path.push((*value).as_ref())
-        }
-
         let ctrl_c = context.ctrl_c.clone();
         let name_tag = context.name.clone();
 
-        //If it's not a glob, try to display the contents of the entry if it's a directory
+        let (path, p_tag) = match path {
-        let lossy_path = full_path.to_string_lossy();
+            Some(p) => {
-        if !lossy_path.contains('*') && !lossy_path.contains('?') {
+                let p_tag = p.tag;
-            let entry = Path::new(&full_path);
+                let mut p = p.item;
-            if entry.is_dir() {
+                if p.is_dir() {
-                let entries = std::fs::read_dir(&entry);
+                    if is_dir_empty(&p) {
-                let entries = match entries {
+                        return Ok(OutputStream::empty());
-                    Err(e) => {
-                        if let Some(s) = path {
-                            return Err(ShellError::labeled_error(
-                                e.to_string(),
-                                e.to_string(),
-                                s.tag(),
-                            ));
-                        } else {
-                            return Err(ShellError::labeled_error(
-                                e.to_string(),
-                                e.to_string(),
-                                name_tag,
-                            ));
-                        }
                     }
-                    Ok(o) => o,
+                    p.push("*");
-                };
+                }
-                let mut entries = entries.collect::<Vec<Result<std::fs::DirEntry, _>>>();
+                (p, p_tag)
-                entries.sort_by(|x, y| match (x, y) {
-                    (Ok(entry1), Ok(entry2)) => entry1
-                        .path()
-                        .to_string_lossy()
-                        .to_lowercase()
-                        .cmp(&entry2.path().to_string_lossy().to_lowercase()),
-                    (Err(_), Ok(_)) => std::cmp::Ordering::Greater,
-                    (Ok(_), Err(_)) => std::cmp::Ordering::Greater,
-                    _ => std::cmp::Ordering::Equal,
-                });
-                let stream = async_stream! {
-                    for entry in entries {
-                        if ctrl_c.load(Ordering::SeqCst) {
-                            break;
-                        }
-                        if let Ok(entry) = entry {
-                            let filepath = entry.path();
-                            if let Ok(metadata) = std::fs::symlink_metadata(&filepath) {
-                                let mut filename = if let Ok(fname) = filepath.strip_prefix(&cwd) {
-                                    fname
-                                } else {
-                                    Path::new(&filepath)
-                                };
-
-                                if short_names {
-                                    filename = if let Some(fname) = filename.file_name() {
-                                        Path::new(fname)
-                                    } else {
-                                        let fname = filename
-                                            .file_name()
-                                            .or_else(|| Path::new(filename).file_name())
-                                            .unwrap_or(filename.as_os_str());
-                                        Path::new(fname)
-                                    }
-                                }
-
-                                let value = dir_entry_dict(filename, &metadata, &name_tag, full, with_symlink_targets)?;
-                                yield ReturnSuccess::value(value);
-                            }
-                        }
-                    }
-                };
-                return Ok(stream.to_output_stream());
             }
-        }
+            None => {
-
+                if is_dir_empty(&self.path().into()) {
-        let entries = match glob::glob(&full_path.to_string_lossy()) {
+                    return Ok(OutputStream::empty());
-            Ok(files) => files,
-            Err(_) => {
-                if let Some(source) = path {
-                    return Err(ShellError::labeled_error(
-                        "Invalid pattern",
-                        "Invalid pattern",
-                        source.tag(),
-                    ));
                 } else {
-                    return Err(ShellError::untagged_runtime_error("Invalid pattern."));
+                    (PathBuf::from("./*"), context.name.clone())
                 }
             }
         };
 
-        // Enumerate the entries from the glob and add each
+        let mut paths = match glob::glob(&path.to_string_lossy()) {
+            Ok(g) => Ok(g),
+            Err(e) => Err(ShellError::labeled_error("Glob error", e.msg, &p_tag)),
+        }?
+        .peekable();
+
+        if paths.peek().is_none() {
+            return Err(ShellError::labeled_error(
+                "Invalid File or Pattern",
+                "Invalid File or Pattern",
+                &p_tag,
+            ));
+        }
+
         let stream = async_stream! {
-            for entry in entries {
+            for path in paths {
                 if ctrl_c.load(Ordering::SeqCst) {
                     break;
                 }
-                if let Ok(entry) = entry {
+                match path {
-                    if let Ok(metadata) = std::fs::symlink_metadata(&entry) {
+                    Ok(p) => match std::fs::symlink_metadata(&p) {
-                        let mut filename = if let Ok(fname) = entry.strip_prefix(&cwd) {
+                        Ok(m) => {
-                            fname
+                            match dir_entry_dict(&p, &m, name_tag.clone(), full, short_names, with_symlink_targets) {
-                        } else {
+                                Ok(d) => yield ReturnSuccess::value(d),
-                             Path::new(&entry)
+                                Err(e) => yield Err(e)
-                        };
-
-                        if short_names {
-                            filename = if let Some(fname) = filename.file_name() {
-                                Path::new(fname)
-                            } else {
-                                let fname = filename
-                                            .file_name()
-                                            .or_else(|| Path::new(filename).file_name())
-                                            .unwrap_or(filename.as_os_str());
-                                Path::new(fname)
                             }
                         }
-
+                        Err(e) => yield Err(ShellError::from(e))
-                        if let Ok(value) = dir_entry_dict(filename, &metadata, &name_tag, full, with_symlink_targets) {
-                            yield ReturnSuccess::value(value);
-                        }
                     }
+                    Err(e) => yield Err(e.into_error().into()),
                 }
             }
         };
@@ -1196,3 +1124,10 @@ impl Shell for FilesystemShell {
         self.hinter.hint(line, pos, ctx)
     }
 }
+
+fn is_dir_empty(d: &PathBuf) -> bool {
+    match d.read_dir() {
+        Err(_e) => true,
+        Ok(mut s) => s.next().is_none(),
+    }
+}