From 98e199f7b50933104adf42a55eeeec0faeb7f005 Mon Sep 17 00:00:00 2001 From: JT <547158+jntrnr@users.noreply.github.com> Date: Fri, 29 Jul 2022 11:00:54 +1200 Subject: [PATCH] Move `ls` back to last-known-good state (#6175) * revert the recent ls changes * cargo fmt --- crates/nu-command/src/filesystem/ls.rs | 424 ++++++++----------------- crates/nu-command/tests/commands/ls.rs | 22 -- 2 files changed, 134 insertions(+), 312 deletions(-) diff --git a/crates/nu-command/src/filesystem/ls.rs b/crates/nu-command/src/filesystem/ls.rs index 0fa94e1cc1..02263cc7a3 100644 --- a/crates/nu-command/src/filesystem/ls.rs +++ b/crates/nu-command/src/filesystem/ls.rs @@ -1,17 +1,15 @@ use crate::DirBuilder; use crate::DirInfo; use chrono::{DateTime, Local, LocalResult, TimeZone, Utc}; -use itertools::Itertools; use nu_engine::env::current_dir; use nu_engine::CallExt; use nu_glob::MatchOptions; use nu_path::expand_to_real_path; use nu_protocol::ast::Call; use nu_protocol::engine::{Command, EngineState, Stack}; -use nu_protocol::IntoPipelineData; use nu_protocol::{ - Category, DataSource, Example, IntoInterruptiblePipelineData, PipelineData, PipelineMetadata, - ShellError, Signature, Span, Spanned, SyntaxShape, Value, + Category, DataSource, Example, IntoInterruptiblePipelineData, IntoPipelineData, PipelineData, + PipelineMetadata, ShellError, Signature, Span, Spanned, SyntaxShape, Value, }; use pathdiff::diff_paths; @@ -40,11 +38,7 @@ impl Command for Ls { fn signature(&self) -> nu_protocol::Signature { Signature::build("ls") // Using a string instead of a glob pattern shape so it won't auto-expand - .rest( - "pattern(s)", - SyntaxShape::String, - "the glob pattern(s) to use", - ) + .optional("pattern", SyntaxShape::String, "the glob pattern to use") .switch("all", "Show hidden files", Some('a')) .switch( "long", @@ -87,23 +81,19 @@ impl Command for Ls { let call_span = call.head; let cwd = current_dir(engine_state, stack)?; - let mut shell_errors: Vec = vec![]; - let pattern_args: Vec> = call.rest(engine_state, stack, 0)?; - let glob_results = if !pattern_args.is_empty() { - pattern_args - .into_iter() - .flat_map(|pattern_arg| { - let mut path = expand_to_real_path(pattern_arg.clone().item); - let p_tag = pattern_arg.span; - let cwd = cwd.clone(); - let ctrl_c = ctrl_c.clone(); + let pattern_arg: Option> = call.opt(engine_state, stack, 0)?; - let expanded = nu_path::expand_path_with(&path, &cwd); - // Avoid checking and pushing "*" to the path when directory (do not show contents) flag is true - if !directory && expanded.is_dir() { - if permission_denied(&path) { - #[cfg(unix)] - let error_msg = format!( + let (path, p_tag, absolute_path) = match pattern_arg { + Some(p) => { + let p_tag = p.span; + let mut p = expand_to_real_path(p.item); + + let expanded = nu_path::expand_path_with(&p, &cwd); + // Avoid checking and pushing "*" to the path when directory (do not show contents) flag is true + if !directory && expanded.is_dir() { + if permission_denied(&p) { + #[cfg(unix)] + let error_msg = format!( "The permissions of {:o} do not allow access for this user", expanded .metadata() @@ -114,225 +104,94 @@ impl Command for Ls { .mode() & 0o0777 ); - #[cfg(not(unix))] - let error_msg = String::from("Permission denied"); - shell_errors.push(ShellError::GenericError( - "Permission denied".to_string(), - error_msg, - Some(p_tag), - None, - Vec::new(), - )); - } - if is_empty_dir(&expanded) { - return Vec::from([Value::nothing(call_span)]).into_iter(); - } - path.push("*"); - } - - let absolute_path = path.is_absolute(); - - let hidden_dir_specified = is_hidden_dir(&path); - - let glob_path = Spanned { - item: path.display().to_string(), - span: p_tag, - }; - - let glob_options = if all { - None - } else { - let mut glob_options = MatchOptions::new(); - glob_options.recursive_match_hidden_dir = false; - Some(glob_options) - }; - let (prefix, paths) = - match nu_engine::glob_from(&glob_path, &cwd, call_span, glob_options) { - Ok((prefix, paths)) => (prefix, paths), - Err(e) => { - shell_errors.push(e); - return Vec::from([Value::nothing(call_span)]).into_iter(); - } - }; - let mut paths_peek = paths.peekable(); - if paths_peek.peek().is_none() { - shell_errors.push(ShellError::GenericError( - format!("No matches found for {}", &path.display().to_string()), - "".to_string(), + #[cfg(not(unix))] + let error_msg = String::from("Permission denied"); + return Err(ShellError::GenericError( + "Permission denied".to_string(), + error_msg, + Some(p_tag), None, - Some("no matches found".to_string()), Vec::new(), )); } - - let mut hidden_dirs = vec![]; - - paths_peek - .into_iter() - .filter_map(move |x| match x { - Ok(path) => { - let metadata = match std::fs::symlink_metadata(&path) { - Ok(metadata) => Some(metadata), - Err(_) => None, - }; - if path_contains_hidden_folder(&path, &hidden_dirs) { - return None; - } - - if !all && !hidden_dir_specified && is_hidden_dir(&path) { - if path.is_dir() { - hidden_dirs.push(path); - } - return None; - } - - let display_name = if short_names { - path.file_name().map(|os| os.to_string_lossy().to_string()) - } else if full_paths || absolute_path { - Some(path.to_string_lossy().to_string()) - } else if let Some(prefix) = &prefix { - if let Ok(remainder) = path.strip_prefix(&prefix) { - if directory { - // When the path is the same as the cwd, path_diff should be "." - let path_diff = if let Some(path_diff_not_dot) = - diff_paths(&path, &cwd) - { - let path_diff_not_dot = - path_diff_not_dot.to_string_lossy(); - if path_diff_not_dot.is_empty() { - ".".to_string() - } else { - path_diff_not_dot.to_string() - } - } else { - path.to_string_lossy().to_string() - }; - - Some(path_diff) - } else { - let new_prefix = - if let Some(pfx) = diff_paths(&prefix, &cwd) { - pfx - } else { - prefix.to_path_buf() - }; - - Some( - new_prefix - .join(remainder) - .to_string_lossy() - .to_string(), - ) - } - } else { - Some(path.to_string_lossy().to_string()) - } - } else { - Some(path.to_string_lossy().to_string()) - } - .ok_or_else(|| { - ShellError::GenericError( - format!("Invalid file name: {:}", path.to_string_lossy()), - "invalid file name".into(), - Some(call_span), - None, - Vec::new(), - ) - }); - - match display_name { - Ok(name) => { - let entry = dir_entry_dict( - &path, - &name, - metadata.as_ref(), - call_span, - long, - du, - ctrl_c.clone(), - ); - match entry { - Ok(value) => Some(value), - Err(err) => Some(Value::Error { error: err }), - } - } - Err(err) => Some(Value::Error { error: err }), - } - } - _ => Some(Value::Nothing { span: call_span }), - }) - .collect_vec() - .into_iter() - }) - .collect_vec() - } else { - let (path, p_tag, absolute_path) = if directory { - (PathBuf::from("."), call_span, false) - } else if is_empty_dir(current_dir(engine_state, stack)?) { - return Ok(Value::nothing(call_span).into_pipeline_data()); - } else { - (PathBuf::from("./*"), call_span, false) - }; - - let hidden_dir_specified = is_hidden_dir(&path); - - let glob_path = Spanned { - item: path.display().to_string(), - span: p_tag, - }; - - let glob_options = if all { - None - } else { - let mut glob_options = MatchOptions::new(); - glob_options.recursive_match_hidden_dir = false; - Some(glob_options) - }; - let (prefix, paths) = nu_engine::glob_from(&glob_path, &cwd, call_span, glob_options)?; - - let mut paths_peek = paths.peekable(); - if paths_peek.peek().is_none() { - return Err(ShellError::GenericError( - format!("No matches found for {}", &path.display().to_string()), - "".to_string(), - None, - Some("no matches found".to_string()), - Vec::new(), - )); + if is_empty_dir(&expanded) { + return Ok(Value::nothing(call_span).into_pipeline_data()); + } + p.push("*"); + } + let absolute_path = p.is_absolute(); + (p, p_tag, absolute_path) } + None => { + // Avoid pushing "*" to the default path when directory (do not show contents) flag is true + if directory { + (PathBuf::from("."), call_span, false) + } else if is_empty_dir(current_dir(engine_state, stack)?) { + return Ok(Value::nothing(call_span).into_pipeline_data()); + } else { + (PathBuf::from("./*"), call_span, false) + } + } + }; - let mut hidden_dirs = vec![]; + let hidden_dir_specified = is_hidden_dir(&path); - paths_peek - .into_iter() - .filter_map(move |x| match x { - Ok(path) => { - let metadata = match std::fs::symlink_metadata(&path) { - Ok(metadata) => Some(metadata), - Err(_) => None, - }; - if path_contains_hidden_folder(&path, &hidden_dirs) { - return None; + let glob_path = Spanned { + item: path.display().to_string(), + span: p_tag, + }; + + let glob_options = if all { + None + } else { + let mut glob_options = MatchOptions::new(); + glob_options.recursive_match_hidden_dir = false; + Some(glob_options) + }; + let (prefix, paths) = nu_engine::glob_from(&glob_path, &cwd, call_span, glob_options)?; + + let mut paths_peek = paths.peekable(); + if paths_peek.peek().is_none() { + return Err(ShellError::GenericError( + format!("No matches found for {}", &path.display().to_string()), + "".to_string(), + None, + Some("no matches found".to_string()), + Vec::new(), + )); + } + + let mut hidden_dirs = vec![]; + + Ok(paths_peek + .into_iter() + .filter_map(move |x| match x { + Ok(path) => { + let metadata = match std::fs::symlink_metadata(&path) { + Ok(metadata) => Some(metadata), + Err(_) => None, + }; + if path_contains_hidden_folder(&path, &hidden_dirs) { + return None; + } + + if !all && !hidden_dir_specified && is_hidden_dir(&path) { + if path.is_dir() { + hidden_dirs.push(path); } + return None; + } - if !all && !hidden_dir_specified && is_hidden_dir(&path) { - if path.is_dir() { - hidden_dirs.push(path); - } - return None; - } - - let display_name = if short_names { - path.file_name().map(|os| os.to_string_lossy().to_string()) - } else if full_paths || absolute_path { - Some(path.to_string_lossy().to_string()) - } else if let Some(prefix) = &prefix { - if let Ok(remainder) = path.strip_prefix(&prefix) { - if directory { - // When the path is the same as the cwd, path_diff should be "." - let path_diff = if let Some(path_diff_not_dot) = - diff_paths(&path, &cwd) - { + let display_name = if short_names { + path.file_name().map(|os| os.to_string_lossy().to_string()) + } else if full_paths || absolute_path { + Some(path.to_string_lossy().to_string()) + } else if let Some(prefix) = &prefix { + if let Ok(remainder) = path.strip_prefix(&prefix) { + if directory { + // When the path is the same as the cwd, path_diff should be "." + let path_diff = + if let Some(path_diff_not_dot) = diff_paths(&path, &cwd) { let path_diff_not_dot = path_diff_not_dot.to_string_lossy(); if path_diff_not_dot.is_empty() { ".".to_string() @@ -343,63 +202,53 @@ impl Command for Ls { path.to_string_lossy().to_string() }; - Some(path_diff) - } else { - let new_prefix = if let Some(pfx) = diff_paths(&prefix, &cwd) { - pfx - } else { - prefix.to_path_buf() - }; - - Some(new_prefix.join(remainder).to_string_lossy().to_string()) - } + Some(path_diff) } else { - Some(path.to_string_lossy().to_string()) + let new_prefix = if let Some(pfx) = diff_paths(&prefix, &cwd) { + pfx + } else { + prefix.to_path_buf() + }; + + Some(new_prefix.join(remainder).to_string_lossy().to_string()) } } else { Some(path.to_string_lossy().to_string()) } - .ok_or_else(|| { - ShellError::GenericError( - format!("Invalid file name: {:}", path.to_string_lossy()), - "invalid file name".into(), - Some(call_span), - None, - Vec::new(), - ) - }); - - match display_name { - Ok(name) => { - let entry = dir_entry_dict( - &path, - &name, - metadata.as_ref(), - call_span, - long, - du, - ctrl_c.clone(), - ); - match entry { - Ok(value) => Some(value), - Err(err) => Some(Value::Error { error: err }), - } - } - Err(err) => Some(Value::Error { error: err }), - } + } else { + Some(path.to_string_lossy().to_string()) } - _ => Some(Value::Nothing { span: call_span }), - }) - .collect_vec() - }; + .ok_or_else(|| { + ShellError::GenericError( + format!("Invalid file name: {:}", path.to_string_lossy()), + "invalid file name".into(), + Some(call_span), + None, + Vec::new(), + ) + }); - if !shell_errors.is_empty() { - return Err(shell_errors.pop().expect("Vec pop error")); - } - - Ok(glob_results - .into_iter() - .filter(|result| !matches!(result, Value::Nothing { .. })) + match display_name { + Ok(name) => { + let entry = dir_entry_dict( + &path, + &name, + metadata.as_ref(), + call_span, + long, + du, + ctrl_c.clone(), + ); + match entry { + Ok(value) => Some(value), + Err(err) => Some(Value::Error { error: err }), + } + } + Err(err) => Some(Value::Error { error: err }), + } + } + _ => Some(Value::Nothing { span: call_span }), + }) .into_pipeline_data_with_metadata( PipelineMetadata { data_source: DataSource::Ls, @@ -430,11 +279,6 @@ impl Command for Ls { example: "ls *.rs", result: None, }, - Example { - description: "List all rust files and all toml files", - example: "ls *.rs *.toml", - result: None, - }, Example { description: "List all files and directories whose name do not contain 'bar'", example: "ls -s | where name !~ bar", diff --git a/crates/nu-command/tests/commands/ls.rs b/crates/nu-command/tests/commands/ls.rs index 93d09ea9ee..f5708df52d 100644 --- a/crates/nu-command/tests/commands/ls.rs +++ b/crates/nu-command/tests/commands/ls.rs @@ -335,28 +335,6 @@ fn lists_files_including_starting_with_dot() { }) } -#[test] -fn lists_regular_files_using_multiple_asterisk_wildcards() { - Playground::setup("ls_test_10", |dirs, sandbox| { - sandbox.with_files(vec![ - EmptyFile("los.txt"), - EmptyFile("tres.txt"), - EmptyFile("amigos.txt"), - EmptyFile("arepas.clu"), - ]); - - let actual = nu!( - cwd: dirs.test(), pipeline( - r#" - ls *.txt *.clu - | length - "# - )); - - assert_eq!(actual.out, "4"); - }) -} - #[test] fn list_all_columns() { Playground::setup("ls_test_all_columns", |dirs, sandbox| {