From 460a1c8f874b503cb4140619838be483600f00ce Mon Sep 17 00:00:00 2001 From: Wind Date: Mon, 6 May 2024 14:01:32 +0800 Subject: [PATCH] Allow ls works inside dir with [] brackets (#12625) # Description Fixes: #12429 To fix the issue, we need to pass the `input pattern` itself to `glob_from` function, but currently on latest main, nushell pass `expanded path of input pattern` to `glob_from` function. It causes globbing failed if expanded path includes `[]` brackets. It's a pity that I have to duplicate `nu_engine::glob_from` function into `ls`, because `ls` might convert from `NuGlob::NotExpand` to `NuGlob::Expand`, in that case, `nu_engine::glob_from` won't work if user want to ls for a directory which includes tilde: ``` mkdir "~abc" ls "~abc" ``` So I need to duplicate `glob_from` function and pass original `expand_tilde` information. # User-Facing Changes Nan # Tests + Formatting Done # After Submitting Nan --- crates/nu-command/src/filesystem/ls.rs | 105 +++++++++++-------------- crates/nu-command/tests/commands/ls.rs | 53 ++++++++++++- 2 files changed, 97 insertions(+), 61 deletions(-) diff --git a/crates/nu-command/src/filesystem/ls.rs b/crates/nu-command/src/filesystem/ls.rs index dd1b9770d2..a89cf04ed6 100644 --- a/crates/nu-command/src/filesystem/ls.rs +++ b/crates/nu-command/src/filesystem/ls.rs @@ -1,9 +1,10 @@ use super::util::get_rest_for_glob_pattern; use crate::{DirBuilder, DirInfo}; use chrono::{DateTime, Local, LocalResult, TimeZone, Utc}; +use nu_engine::glob_from; #[allow(deprecated)] use nu_engine::{command_prelude::*, env::current_dir}; -use nu_glob::{MatchOptions, Pattern}; +use nu_glob::MatchOptions; use nu_path::expand_to_real_path; use nu_protocol::{DataSource, NuGlob, PipelineMetadata}; use pathdiff::diff_paths; @@ -31,6 +32,8 @@ struct Args { call_span: Span, } +const GLOB_CHARS: &[char] = &['*', '?', '[']; + impl Command for Ls { fn name(&self) -> &str { "ls" @@ -235,24 +238,20 @@ fn ls_for_one_pattern( } }; - // it indicates we need to append an extra '*' after pattern for listing given directory - // Example: 'ls directory' -> 'ls directory/*' - let mut extra_star_under_given_directory = false; - let (path, p_tag, absolute_path, quoted) = match pattern_arg { + let mut just_read_dir = false; + let p_tag: Span = pattern_arg.as_ref().map(|p| p.span).unwrap_or(call_span); + let (pattern_arg, absolute_path) = match pattern_arg { Some(pat) => { - let p_tag = pat.span; - let expanded = nu_path::expand_path_with( - pat.item.as_ref(), - &cwd, - matches!(pat.item, NuGlob::Expand(..)), - ); + // expand with cwd here is only used for checking + let tmp_expanded = + nu_path::expand_path_with(pat.item.as_ref(), &cwd, pat.item.is_expand()); // Avoid checking and pushing "*" to the path when directory (do not show contents) flag is true - if !directory && expanded.is_dir() { - if permission_denied(&expanded) { + if !directory && tmp_expanded.is_dir() { + if permission_denied(&tmp_expanded) { #[cfg(unix)] let error_msg = format!( "The permissions of {:o} do not allow access for this user", - expanded + tmp_expanded .metadata() .expect("this shouldn't be called since we already know there is a dir") .permissions() @@ -269,10 +268,10 @@ fn ls_for_one_pattern( inner: vec![], }); } - if is_empty_dir(&expanded) { + if is_empty_dir(&tmp_expanded) { return Ok(Box::new(vec![].into_iter())); } - extra_star_under_given_directory = true; + just_read_dir = !(pat.item.is_expand() && pat.item.as_ref().contains(GLOB_CHARS)); } // it's absolute path if: @@ -282,67 +281,44 @@ fn ls_for_one_pattern( // path. let absolute_path = Path::new(pat.item.as_ref()).is_absolute() || (pat.item.is_expand() && expand_to_real_path(pat.item.as_ref()).is_absolute()); - ( - expanded, - p_tag, - absolute_path, - matches!(pat.item, NuGlob::DoNotExpand(_)), - ) + (pat.item, 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, false) + (NuGlob::Expand(".".to_string()), false) } else if is_empty_dir(&cwd) { return Ok(Box::new(vec![].into_iter())); } else { - (PathBuf::from("*"), call_span, false, false) + (NuGlob::Expand("*".to_string()), false) } } }; - let hidden_dir_specified = is_hidden_dir(&path); - // when it's quoted, we need to escape our glob pattern(but without the last extra - // start which may be added under given directory) - // so we can do ls for a file or directory like `a[123]b` - let path = if quoted { - let p = path.display().to_string(); - let mut glob_escaped = Pattern::escape(&p); - if extra_star_under_given_directory { - glob_escaped.push(std::path::MAIN_SEPARATOR); - glob_escaped.push('*'); - } - glob_escaped + let hidden_dir_specified = is_hidden_dir(pattern_arg.as_ref()); + let path = pattern_arg.into_spanned(p_tag); + let (prefix, paths) = if just_read_dir { + let expanded = nu_path::expand_path_with(path.item.as_ref(), &cwd, path.item.is_expand()); + let paths = read_dir(&expanded)?; + // just need to read the directory, so prefix is path itself. + (Some(expanded), paths) } else { - let mut p = path.display().to_string(); - if extra_star_under_given_directory { - p.push(std::path::MAIN_SEPARATOR); - p.push('*'); - } - p - }; - - let glob_path = Spanned { - // use NeedExpand, the relative escaping logic is handled previously - item: NuGlob::Expand(path.clone()), - span: p_tag, - }; - - let glob_options = if all { - None - } else { - let glob_options = MatchOptions { - recursive_match_hidden_dir: false, - ..Default::default() + let glob_options = if all { + None + } else { + let glob_options = MatchOptions { + recursive_match_hidden_dir: false, + ..Default::default() + }; + Some(glob_options) }; - Some(glob_options) + glob_from(&path, &cwd, call_span, 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 { - error: format!("No matches found for {}", &path), + error: format!("No matches found for {:?}", path.item), msg: "Pattern, file or folder not found".into(), span: Some(p_tag), help: Some("no matches found".into()), @@ -904,3 +880,14 @@ mod windows_helper { false } } + +#[allow(clippy::type_complexity)] +fn read_dir( + f: &Path, +) -> Result> + Send>, ShellError> { + let iter = f.read_dir()?.map(|d| { + d.map(|r| r.path()) + .map_err(|e| ShellError::IOError { msg: e.to_string() }) + }); + Ok(Box::new(iter)) +} diff --git a/crates/nu-command/tests/commands/ls.rs b/crates/nu-command/tests/commands/ls.rs index 8ccdcfaf93..5354c9ccba 100644 --- a/crates/nu-command/tests/commands/ls.rs +++ b/crates/nu-command/tests/commands/ls.rs @@ -85,13 +85,13 @@ fn lists_regular_files_in_special_folder() { assert_eq!(actual.out, "2"); let actual = nu!( cwd: dirs.test().join("abcd/*"), format!(r#"ls ../* | length"#)); - assert_eq!(actual.out, "3"); + assert_eq!(actual.out, "2"); let actual = nu!( cwd: dirs.test().join("abcd/?"), format!(r#"ls -D ../* | length"#)); assert_eq!(actual.out, "2"); let actual = nu!( cwd: dirs.test().join("abcd/?"), format!(r#"ls ../* | length"#)); - assert_eq!(actual.out, "3"); + assert_eq!(actual.out, "2"); }) } @@ -237,6 +237,12 @@ fn list_files_from_two_parents_up_using_multiple_dots() { ); assert_eq!(actual.out, "5"); + + let actual = nu!( + cwd: dirs.test().join("foo/bar"), + r#"ls ... | sort-by name | get name.0 | str replace -a '\' '/'"# + ); + assert_eq!(actual.out, "../../andres.xml"); }) } @@ -759,3 +765,46 @@ fn list_with_multiple_path() { assert_eq!(actual.out, "0"); }) } + +#[test] +fn list_inside_glob_metachars_dir() { + Playground::setup("list_files_inside_glob_metachars_dir", |dirs, sandbox| { + let sub_dir = "test[]"; + sandbox + .within(sub_dir) + .with_files(&[EmptyFile("test_file.txt")]); + + let actual = nu!( + cwd: dirs.test().join(sub_dir), + "ls test_file.txt | get name.0 | path basename", + ); + assert!(actual.out.contains("test_file.txt")); + }); +} + +#[test] +fn list_inside_tilde_glob_metachars_dir() { + Playground::setup( + "list_files_inside_tilde_glob_metachars_dir", + |dirs, sandbox| { + let sub_dir = "~test[]"; + sandbox + .within(sub_dir) + .with_files(&[EmptyFile("test_file.txt")]); + + // need getname.0 | path basename because the output path + // might be too long to output as a single line. + let actual = nu!( + cwd: dirs.test().join(sub_dir), + "ls test_file.txt | get name.0 | path basename", + ); + assert!(actual.out.contains("test_file.txt")); + + let actual = nu!( + cwd: dirs.test(), + "ls '~test[]' | get name.0 | path basename" + ); + assert!(actual.out.contains("test_file.txt")); + }, + ); +}