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 <v-frankz@microsoft.com>

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<std::process::Command, ShellError> {
-        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(), false),
-        (Some('\''), Some('\'')) => chars.collect(),
+        (Some('\''), Some('\'')) => (chars.collect(), false),
-        (Some('`'), Some('`')) => chars.collect(),
+        (Some('`'), Some('`')) => (chars.collect(), true),
-        _ => input.to_string(),
+        _ => (input.to_string(), true),
     }
 }
 

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"));
+    });
+}

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);
+            // Eval stage trims the quotes, so we don't have to do the same thing when parsing.
-            error = error.or(err);
+            let contents = String::from_utf8_lossy(contents).to_string();
 
             args.push(Expression {
                 expr: Expr::String(contents),