fix ls and ls tests (#931)

* fix `ls` and ls tests

* tweak to ls so it doesn't scream on empty dirs

* clippy

* reworked `ls` to put in what was left out

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

@@ -6,8 +6,8 @@ use nu_engine::CallExt;
 use nu_protocol::ast::Call;
 use nu_protocol::engine::{Command, EngineState, Stack};
 use nu_protocol::{
-    Category, DataSource, IntoInterruptiblePipelineData, PipelineData, PipelineMetadata,
+    Category, DataSource, IntoInterruptiblePipelineData, IntoPipelineData, PipelineData,
-    ShellError, Signature, Span, Spanned, SyntaxShape, Value,
+    PipelineMetadata, ShellError, Signature, Span, Spanned, SyntaxShape, Value,
 };
 use pathdiff::diff_paths;
 #[cfg(unix)]
@@ -67,27 +67,70 @@ impl Command for Ls {
         let full_paths = call.has_flag("full-paths");
         let du = call.has_flag("du");
         let ctrl_c = engine_state.ctrlc.clone();
-
         let call_span = call.head;
         let cwd = current_dir(engine_state, stack)?;
-
         let pattern_arg = call.opt::<Spanned<String>>(engine_state, stack, 0)?;
 
-        let pattern = if let Some(pattern) = pattern_arg {
+        let (path, p_tag) = match pattern_arg {
-            pattern
+            Some(p) => {
+                let p_tag = p.span;
+                let mut p = PathBuf::from(p.item);
+                if p.is_dir() {
+                    if permission_denied(&p) {
+                        #[cfg(unix)]
+                        let error_msg = format!(
+                            "The permissions of {:o} do not allow access for this user",
+                            p.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".to_string(),
+                            error_msg,
+                            p_tag,
+                        ));
+                    }
+                    if is_empty_dir(&p) {
+                        return Ok(Value::nothing(call_span).into_pipeline_data());
+                    }
+                    p.push("*");
+                }
+                (p, p_tag)
+            }
+            None => {
+                if is_empty_dir(current_dir(engine_state, stack)?) {
+                    return Ok(Value::nothing(call_span).into_pipeline_data());
                 } else {
-            Spanned {
+                    (PathBuf::from("./*"), call_span)
-                item: cwd.join("*").to_string_lossy().to_string(),
+                }
-                span: call_span,
             }
         };
 
-        let (prefix, glob) = nu_engine::glob_from(&pattern, &cwd, call_span)?;
+        let hidden_dir_specified = is_hidden_dir(&path);
+
+        let glob_path = Spanned {
+            item: path.display().to_string(),
+            span: p_tag,
+        };
+        let (prefix, paths) = nu_engine::glob_from(&glob_path, &cwd, call_span)?;
+
+        let mut paths_peek = paths.peekable();
+        if paths_peek.peek().is_none() {
+            return Err(ShellError::LabeledError(
+                format!("No matches found for {}", &path.display().to_string()),
+                "no matches found".to_string(),
+            ));
+        }
 
-        let hidden_dir_specified = is_hidden_dir(&pattern.item);
         let mut hidden_dirs = vec![];
 
-        Ok(glob
+        Ok(paths_peek
             .into_iter()
             .filter_map(move |x| match x {
                 Ok(path) => {
@@ -163,6 +206,20 @@ 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_empty_dir(dir: impl AsRef<Path>) -> bool {
+    match dir.as_ref().read_dir() {
+        Err(_) => true,
+        Ok(mut s) => s.next().is_none(),
+    }
+}
+
 fn is_hidden_dir(dir: impl AsRef<Path>) -> bool {
     #[cfg(windows)]
     {

crates/nu-command/tests/commands/ls.rs

@@ -67,8 +67,6 @@ fn lists_regular_files_using_question_mark_wildcard() {
     })
 }
 
-// FIXME: jt: needs more work
-#[ignore]
 #[test]
 fn lists_all_files_in_directories_from_stream() {
     Playground::setup("ls_test_4", |dirs, sandbox| {
@@ -90,7 +88,7 @@ fn lists_all_files_in_directories_from_stream() {
             r#"
                 echo dir_a dir_b
                 | each { ls $it }
-                | length
+                | flatten | length
             "#
         ));
 
@@ -115,8 +113,6 @@ fn does_not_fail_if_glob_matches_empty_directory() {
     })
 }
 
-// FIXME: jt: needs more work
-#[ignore]
 #[test]
 fn fails_when_glob_doesnt_match() {
     Playground::setup("ls_test_5", |dirs, sandbox| {
@@ -292,8 +288,6 @@ fn lists_files_including_starting_with_dot() {
     })
 }
 
-// FIXME: jt: needs more work
-#[ignore]
 #[test]
 fn list_all_columns() {
     Playground::setup("ls_test_all_columns", |dirs, sandbox| {
@@ -306,14 +300,14 @@ fn list_all_columns() {
         // Normal Operation
         let actual = nu!(
             cwd: dirs.test(),
-            "ls | get | to md"
+            "ls | columns | to md"
         );
         let expected = ["name", "type", "size", "modified"].join("");
         assert_eq!(actual.out, expected, "column names are incorrect for ls");
         // Long
         let actual = nu!(
             cwd: dirs.test(),
-            "ls -l | get | to md"
+            "ls -l | columns | to md"
         );
         let expected = {
             #[cfg(unix)]
@@ -322,10 +316,10 @@ fn list_all_columns() {
                     "name",
                     "type",
                     "target",
-                    "num_links",
-                    "inode",
                     "readonly",
                     "mode",
+                    "num_links",
+                    "inode",
                     "uid",
                     "group",
                     "size",