From eeaca50dee7edc66b330b97cc846d6b06ee872a2 Mon Sep 17 00:00:00 2001 From: Kangaxx-0 <85712372+Kangaxx-0@users.noreply.github.com> Date: Sun, 17 Jul 2022 14:30:33 -0700 Subject: [PATCH] Conditionally disable expansion for external command (#6014) * Fix 5978 * Add unit test for explicit glob * Format * Expansion vs none-expansion * Add unit tests * Fix format.. * Add debug message for MacOS * Fix UT on Mac and add tests for windows * cleanup * clean up windows test * single and double qoutes tests * format... * Save format. * Add log to failed windows unit tests * try `touch` a file * PS or CMD * roll back some change * format * Remove log and test case * Add unit test comments * Fix Co-authored-by: Frank --- crates/nu-command/src/system/run_external.rs | 17 +- .../nu-command/tests/commands/run_external.rs | 216 ++++++++++++++++++ crates/nu-parser/src/parser.rs | 4 +- 3 files changed, 227 insertions(+), 10 deletions(-) diff --git a/crates/nu-command/src/system/run_external.rs b/crates/nu-command/src/system/run_external.rs index 355d52213a..6e134cf530 100644 --- a/crates/nu-command/src/system/run_external.rs +++ b/crates/nu-command/src/system/run_external.rs @@ -402,7 +402,7 @@ impl ExternalCommand { /// Spawn a command without shelling out to an external shell pub fn spawn_simple_command(&self, cwd: &str) -> Result { - let head = trim_enclosing_quotes(&self.name.item); + let (head, _) = trim_enclosing_quotes(&self.name.item); let head = nu_path::expand_to_real_path(head) .to_string_lossy() .to_string(); @@ -410,8 +410,9 @@ impl ExternalCommand { let mut process = std::process::Command::new(&head); for arg in self.args.iter() { + let (trimmed_args, run_glob_expansion) = trim_enclosing_quotes(&arg.item); let mut arg = Spanned { - item: remove_quotes(trim_enclosing_quotes(&arg.item)), + item: remove_quotes(trimmed_args), span: arg.span, }; @@ -421,7 +422,7 @@ impl ExternalCommand { let cwd = PathBuf::from(cwd); - if arg.item.contains('*') { + if arg.item.contains('*') && run_glob_expansion { if let Ok((prefix, matches)) = nu_engine::glob_from(&arg, &cwd, self.name.span, None) { @@ -516,14 +517,14 @@ fn shell_arg_escape(arg: &str) -> String { } } -fn trim_enclosing_quotes(input: &str) -> String { +fn trim_enclosing_quotes(input: &str) -> (String, bool) { let mut chars = input.chars(); match (chars.next(), chars.next_back()) { - (Some('"'), Some('"')) => chars.collect(), - (Some('\''), Some('\'')) => chars.collect(), - (Some('`'), Some('`')) => chars.collect(), - _ => input.to_string(), + (Some('"'), Some('"')) => (chars.collect(), false), + (Some('\''), Some('\'')) => (chars.collect(), false), + (Some('`'), Some('`')) => (chars.collect(), true), + _ => (input.to_string(), true), } } diff --git a/crates/nu-command/tests/commands/run_external.rs b/crates/nu-command/tests/commands/run_external.rs index 7071050910..2717e06b50 100644 --- a/crates/nu-command/tests/commands/run_external.rs +++ b/crates/nu-command/tests/commands/run_external.rs @@ -1,3 +1,5 @@ +use nu_test_support::fs::Stub::EmptyFile; +use nu_test_support::playground::Playground; use nu_test_support::{nu, pipeline}; #[test] @@ -13,3 +15,217 @@ fn better_empty_redirection() { assert!(!actual.out.contains('2')); } + +#[cfg(not(windows))] +#[test] +fn explicit_glob() { + Playground::setup("external with explicit glob", |dirs, sandbox| { + sandbox.with_files(vec![ + EmptyFile("D&D_volume_1.txt"), + EmptyFile("D&D_volume_2.txt"), + EmptyFile("foo.sh"), + ]); + + let actual = nu!( + cwd: dirs.test(), pipeline( + r#" + ^ls | glob '*.txt' | length + "# + )); + + assert_eq!(actual.out, "2"); + }) +} + +#[cfg(not(windows))] +#[test] +fn bare_word_expand_path_glob() { + Playground::setup("bare word should do the expansion", |dirs, sandbox| { + sandbox.with_files(vec![ + EmptyFile("D&D_volume_1.txt"), + EmptyFile("D&D_volume_2.txt"), + EmptyFile("foo.sh"), + ]); + + let actual = nu!( + cwd: dirs.test(), pipeline( + r#" + ^ls *.txt + "# + )); + + assert!(actual.out.contains("D&D_volume_1.txt")); + assert!(actual.out.contains("D&D_volume_2.txt")); + }) +} + +#[cfg(not(windows))] +#[test] +fn backtick_expand_path_glob() { + Playground::setup("backtick should do the expansion", |dirs, sandbox| { + sandbox.with_files(vec![ + EmptyFile("D&D_volume_1.txt"), + EmptyFile("D&D_volume_2.txt"), + EmptyFile("foo.sh"), + ]); + + let actual = nu!( + cwd: dirs.test(), pipeline( + r#" + ^ls `*.txt` + "# + )); + + assert!(actual.out.contains("D&D_volume_1.txt")); + assert!(actual.out.contains("D&D_volume_2.txt")); + }) +} + +#[cfg(not(windows))] +#[test] +fn single_quote_does_not_expand_path_glob() { + Playground::setup("single quote do not run the expansion", |dirs, sandbox| { + sandbox.with_files(vec![ + EmptyFile("D&D_volume_1.txt"), + EmptyFile("D&D_volume_2.txt"), + EmptyFile("foo.sh"), + ]); + + let actual = nu!( + cwd: dirs.test(), pipeline( + r#" + ^ls '*.txt' + "# + )); + + assert!(actual.err.contains("No such file or directory")); + }) +} + +#[cfg(not(windows))] +#[test] +fn double_quote_does_not_expand_path_glob() { + Playground::setup("double quote do not run the expansion", |dirs, sandbox| { + sandbox.with_files(vec![ + EmptyFile("D&D_volume_1.txt"), + EmptyFile("D&D_volume_2.txt"), + EmptyFile("foo.sh"), + ]); + + let actual = nu!( + cwd: dirs.test(), pipeline( + r#" + ^ls "*.txt" + "# + )); + + assert!(actual.err.contains("No such file or directory")); + }) +} + +#[cfg(windows)] +#[test] +fn explicit_glob_windows() { + Playground::setup("external with explicit glob", |dirs, sandbox| { + sandbox.with_files(vec![ + EmptyFile("D&D_volume_1.txt"), + EmptyFile("D&D_volume_2.txt"), + EmptyFile("foo.sh"), + ]); + + let actual = nu!( + cwd: dirs.test(), pipeline( + r#" + ^dir | glob '*.txt' | length + "# + )); + + assert_eq!(actual.out, "2"); + }) +} + +#[cfg(windows)] +#[test] +fn bare_word_expand_path_glob_windows() { + Playground::setup("bare word should do the expansion", |dirs, sandbox| { + sandbox.with_files(vec![ + EmptyFile("D&D_volume_1.txt"), + EmptyFile("D&D_volume_2.txt"), + EmptyFile("foo.sh"), + ]); + + let actual = nu!( + cwd: dirs.test(), pipeline( + r#" + ^dir *.txt + "# + )); + + assert!(actual.out.contains("D&D_volume_1.txt")); + assert!(actual.out.contains("D&D_volume_2.txt")); + }) +} + +#[cfg(windows)] +#[test] +// This test case might fail based on the running shell on Windows - CMD vs PowerShell, the reason is +// +// Test command 1 - `dir * ` +// Test command 2 - `dir '*'` +// Test command 3 - `dir "*"` +// +// In CMD, command 2 and 3 will give you an error of 'File Not Found' +// In Poweshell, all three commands will do the path expansion with any errors whatsoever +// +// With current Windows CI build(Microsoft Windows 2022 with version 10.0.20348), +// the unit test runs agaisnt PowerShell +fn double_quote_does_not_expand_path_glob_windows() { + Playground::setup("double quote do not run the expansion", |dirs, sandbox| { + sandbox.with_files(vec![ + EmptyFile("D&D_volume_1.txt"), + EmptyFile("D&D_volume_2.txt"), + EmptyFile("foo.sh"), + ]); + + let actual = nu!( + cwd: dirs.test(), pipeline( + r#" + dir "*.txt" + "# + )); + assert!(actual.out.contains("D&D_volume_1.txt")); + assert!(actual.out.contains("D&D_volume_2.txt")); + }) +} + +#[cfg(windows)] +#[test] +// This test case might fail based on the running shell on Windows - CMD vs PowerShell, the reason is +// +// Test command 1 - `dir * ` +// Test command 2 - `dir '*'` +// Test command 3 - `dir "*"` +// +// In CMD, command 2 and 3 will give you an error of 'File Not Found' +// In Poweshell, all three commands will do the path expansion with any errors whatsoever +// +// With current Windows CI build(Microsoft Windows 2022 with version 10.0.20348), +// the unit test runs agaisnt PowerShell +fn single_quote_does_not_expand_path_glob_windows() { + Playground::setup("single quote do not run the expansion", |dirs, sandbox| { + sandbox.with_files(vec![ + EmptyFile("D&D_volume_1.txt"), + EmptyFile("D&D_volume_2.txt"), + EmptyFile("foo.sh"), + ]); + + let actual = nu!( + cwd: dirs.test(), pipeline( + r#" + dir '*.txt' + "# + )); + assert!(actual.out.contains("D&D_volume_1.txt")); + assert!(actual.out.contains("D&D_volume_2.txt")); + }); +} diff --git a/crates/nu-parser/src/parser.rs b/crates/nu-parser/src/parser.rs index 29a675ae43..d48d4083d5 100644 --- a/crates/nu-parser/src/parser.rs +++ b/crates/nu-parser/src/parser.rs @@ -305,8 +305,8 @@ pub fn parse_external_call( error = error.or(err); args.push(arg); } else { - let (contents, err) = unescape_unquote_string(contents, *span); - error = error.or(err); + // Eval stage trims the quotes, so we don't have to do the same thing when parsing. + let contents = String::from_utf8_lossy(contents).to_string(); args.push(Expression { expr: Expr::String(contents),