From 8204cc4f2831ead9419e790a2744f8c50abd1d3c Mon Sep 17 00:00:00 2001 From: Darren Schroeder <343840+fdncred@users.noreply.github.com> Date: Fri, 4 Feb 2022 14:32:13 -0600 Subject: [PATCH] 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 | 83 ++++++++++++++++++++++---- crates/nu-command/tests/commands/ls.rs | 16 ++--- 2 files changed, 75 insertions(+), 24 deletions(-) diff --git a/crates/nu-command/src/filesystem/ls.rs b/crates/nu-command/src/filesystem/ls.rs index 55cab1af9d..87cddb16f3 100644 --- a/crates/nu-command/src/filesystem/ls.rs +++ b/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, - ShellError, Signature, Span, Spanned, SyntaxShape, Value, + Category, DataSource, IntoInterruptiblePipelineData, IntoPipelineData, PipelineData, + 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::>(engine_state, stack, 0)?; - let pattern = if let Some(pattern) = pattern_arg { - pattern - } else { - Spanned { - item: cwd.join("*").to_string_lossy().to_string(), - span: call_span, + let (path, p_tag) = match pattern_arg { + 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 { + (PathBuf::from("./*"), 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) -> 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) -> bool { + match dir.as_ref().read_dir() { + Err(_) => true, + Ok(mut s) => s.next().is_none(), + } +} + fn is_hidden_dir(dir: impl AsRef) -> bool { #[cfg(windows)] { diff --git a/crates/nu-command/tests/commands/ls.rs b/crates/nu-command/tests/commands/ls.rs index eda52a6cda..91f9a1393f 100644 --- a/crates/nu-command/tests/commands/ls.rs +++ b/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",