From be8c1dc0066cd1034a6b110a622f47b516bfe029 Mon Sep 17 00:00:00 2001 From: Devyn Cairns Date: Mon, 3 Jun 2024 00:38:55 -0700 Subject: Fix `run_external::expand_glob()` to return paths that are PWD-relative but reflect the original intent (#13028) # Description Fix #13021 This changes the `expand_glob()` function to use `nu_engine::glob_from()` so that absolute paths are actually preserved, rather than being made relative to the provided parent. This preserves the intent of whoever wrote the original path/glob, and also makes it so that tilde always produces absolute paths. I also made `expand_glob()` handle Ctrl-C so that it can be interrupted. cc @YizhePKU # Tests + Formatting No additional tests here... but that might be a good idea. --- crates/nu-command/src/system/run_external.rs | 141 +++++++++++++++++++-------- 1 file changed, 98 insertions(+), 43 deletions(-) diff --git a/crates/nu-command/src/system/run_external.rs b/crates/nu-command/src/system/run_external.rs index a200ce75d..d4645e87e 100644 --- a/crates/nu-command/src/system/run_external.rs +++ b/crates/nu-command/src/system/run_external.rs @@ -4,7 +4,7 @@ use nu_protocol::{ ast::{Expr, Expression}, did_you_mean, process::ChildProcess, - ByteStream, OutDest, + ByteStream, NuGlob, OutDest, }; use nu_system::ForegroundChild; use nu_utils::IgnoreCaseExt; @@ -13,7 +13,7 @@ use std::{ io::Write, path::{Path, PathBuf}, process::Stdio, - sync::Arc, + sync::{atomic::AtomicBool, Arc}, thread, }; @@ -155,6 +155,9 @@ impl Command for External { } }; + // Log the command we're about to run in case it's useful for debugging purposes. + log::trace!("run-external spawning: {command:?}"); + // Spawn the child process. On Unix, also put the child process to // foreground if we're in an interactive session. #[cfg(windows)] @@ -232,6 +235,7 @@ pub fn eval_arguments_from_call( stack: &mut Stack, call: &Call, ) -> Result>, ShellError> { + let ctrlc = &engine_state.ctrlc; let cwd = engine_state.cwd(Some(stack))?; let mut args: Vec> = vec![]; for (expr, spread) in call.rest_iter(1) { @@ -240,7 +244,7 @@ pub fn eval_arguments_from_call( // glob-expansion, and inner-quotes-removal, in that order. for arg in eval_argument(engine_state, stack, expr, spread)? { let tilde_expanded = expand_tilde(&arg); - for glob_expanded in expand_glob(&tilde_expanded, &cwd, expr.span)? { + for glob_expanded in expand_glob(&tilde_expanded, &cwd, expr.span, ctrlc)? { let inner_quotes_removed = remove_inner_quotes(&glob_expanded); args.push(inner_quotes_removed.into_owned().into_spanned(expr.span)); } @@ -321,37 +325,75 @@ fn expand_tilde(arg: &str) -> String { /// /// Note: This matches the default behavior of Bash, but is known to be /// error-prone. We might want to change this behavior in the future. -fn expand_glob(arg: &str, cwd: &Path, span: Span) -> Result, ShellError> { - let Ok(paths) = nu_glob::glob_with_parent(arg, nu_glob::MatchOptions::default(), cwd) else { +fn expand_glob( + arg: &str, + cwd: &Path, + span: Span, + interrupt: &Option>, +) -> Result, ShellError> { + const GLOB_CHARS: &[char] = &['*', '?', '[']; + + // Don't expand something that doesn't include the GLOB_CHARS + if !arg.contains(GLOB_CHARS) { + return Ok(vec![arg.into()]); + } + + // We must use `nu_engine::glob_from` here, in order to ensure we get paths from the correct + // dir + let glob = NuGlob::Expand(arg.to_owned()).into_spanned(span); + let Ok((_prefix, paths)) = nu_engine::glob_from(&glob, cwd, span, None) else { + // If an error occurred, return the original input return Ok(vec![arg.into()]); }; - let mut result = vec![]; - for path in paths { - let path = path.map_err(|err| ShellError::IOErrorSpanned { - msg: format!("{}: {:?}", err.path().display(), err.error()), - span, - })?; - // Strip PWD from the resulting paths if possible. - let path_stripped = if let Ok(remainder) = path.strip_prefix(cwd) { - // If stripping PWD results in an empty path, return `.` instead. - if remainder.components().next().is_none() { - Path::new(".") + // If the first component of the original `arg` string path was '.', that should be preserved + let relative_to_dot = Path::new(arg).starts_with("."); + + let paths = paths + // Skip over glob failures. These are usually just inaccessible paths. + .flat_map(|path_result| match path_result { + Ok(path) => Some(path), + Err(err) => { + // But internally log them just in case we need to debug this. + log::warn!("Error in run_external::expand_glob(): {}", err); + None + } + }) + // Make the paths relative to the cwd + .map(|path| { + path.strip_prefix(cwd) + .map(|path| path.to_owned()) + .unwrap_or(path) + }) + // Add './' to relative paths if the original pattern had it + .map(|path| { + if relative_to_dot && path.is_relative() { + Path::new(".").join(path) } else { - remainder + path } - } else { - &path - }; - let path_string = path_stripped.to_string_lossy().to_string(); - result.push(path_string); - } + }) + // Convert the paths returned to UTF-8 strings. + // + // FIXME: this fails to return the correct results for non-UTF-8 paths, but we don't support + // those in Nushell yet. + .map(|path| path.to_string_lossy().into_owned()) + // Abandon if ctrl-c is pressed + .map(|path| { + if !nu_utils::ctrl_c::was_pressed(interrupt) { + Ok(path) + } else { + Err(ShellError::InterruptedByUser { span: Some(span) }) + } + }) + .collect::, ShellError>>()?; - if result.is_empty() { - result.push(arg.to_string()); + if !paths.is_empty() { + Ok(paths) + } else { + // If we failed to match, return the original input + Ok(vec![arg.into()]) } - - Ok(result) } /// Transforms `--option="value"` into `--option=value`. `value` can be quoted @@ -607,6 +649,7 @@ fn escape_cmd_argument(arg: &Spanned) -> Result, ShellError mod test { use super::*; use nu_protocol::ast::ListItem; + use nu_test_support::{fs::Stub, playground::Playground}; #[test] fn test_remove_quotes() { @@ -669,26 +712,38 @@ mod test { #[test] fn test_expand_glob() { - let tempdir = tempfile::tempdir().unwrap(); - let cwd = tempdir.path(); - std::fs::File::create(cwd.join("a.txt")).unwrap(); - std::fs::File::create(cwd.join("b.txt")).unwrap(); + Playground::setup("test_expand_glob", |dirs, play| { + play.with_files(&[Stub::EmptyFile("a.txt"), Stub::EmptyFile("b.txt")]); - let actual = expand_glob("*.txt", cwd, Span::unknown()).unwrap(); - let expected = &["a.txt", "b.txt"]; - assert_eq!(actual, expected); + let cwd = dirs.test(); - let actual = expand_glob("'*.txt'", cwd, Span::unknown()).unwrap(); - let expected = &["'*.txt'"]; - assert_eq!(actual, expected); + let actual = expand_glob("*.txt", cwd, Span::unknown(), &None).unwrap(); + let expected = &["a.txt", "b.txt"]; + assert_eq!(actual, expected); - let actual = expand_glob(cwd.to_str().unwrap(), cwd, Span::unknown()).unwrap(); - let expected = &["."]; - assert_eq!(actual, expected); + let actual = expand_glob("./*.txt", cwd, Span::unknown(), &None).unwrap(); + let expected = vec![ + Path::new(".").join("a.txt").to_string_lossy().into_owned(), + Path::new(".").join("b.txt").to_string_lossy().into_owned(), + ]; + assert_eq!(actual, expected); - let actual = expand_glob("[*.txt", cwd, Span::unknown()).unwrap(); - let expected = &["[*.txt"]; - assert_eq!(actual, expected); + let actual = expand_glob("'*.txt'", cwd, Span::unknown(), &None).unwrap(); + let expected = &["'*.txt'"]; + assert_eq!(actual, expected); + + let actual = expand_glob(".", cwd, Span::unknown(), &None).unwrap(); + let expected = &["."]; + assert_eq!(actual, expected); + + let actual = expand_glob("./a.txt", cwd, Span::unknown(), &None).unwrap(); + let expected = &["./a.txt"]; + assert_eq!(actual, expected); + + let actual = expand_glob("[*.txt", cwd, Span::unknown(), &None).unwrap(); + let expected = &["[*.txt"]; + assert_eq!(actual, expected); + }) } #[test] -- cgit v1.2.3-70-g09d2