Switch to using subprocess::shell (#1264)

* Switch to using `shell`

Switch to using the shell for subprocess to enable more natural shelling out.

* Update external.rs

* This is a test with .shell() for external

* El pollo loco's PR

* co co co

* Attempt to fix windows

* Fmt

* Less is more?

Co-authored-by: Andrés N. Robalino <andres@androbtech.com>

crates/nu-test-support/src/lib.rs

@@ -14,6 +14,16 @@ pub fn pipeline(commands: &str) -> String {
         .to_string()
 }
 
+pub fn shell_os_paths() -> Vec<std::path::PathBuf> {
+    let mut original_paths = vec![];
+
+    if let Some(paths) = std::env::var_os("PATH") {
+        original_paths = std::env::split_paths(&paths).collect::<Vec<_>>();
+    }
+
+    original_paths
+}
+
 #[cfg(test)]
 mod tests {
     use super::pipeline;

crates/nu-test-support/src/macros.rs

@@ -28,18 +28,25 @@ macro_rules! nu {
             $crate::fs::DisplayPath::display_path(&$path)
         );
 
-        let dummies = $crate::fs::binaries();
+        let test_bins = $crate::fs::binaries();
-        let dummies = dunce::canonicalize(&dummies).unwrap_or_else(|e| {
+        let test_bins = dunce::canonicalize(&test_bins).unwrap_or_else(|e| {
             panic!(
                 "Couldn't canonicalize dummy binaries path {}: {:?}",
-                dummies.display(),
+                test_bins.display(),
                 e
             )
         });
 
+        let mut paths = $crate::shell_os_paths();
+        paths.push(test_bins);
+
+        let paths_joined = match std::env::join_paths(paths.iter()) {
+            Ok(all) => all,
+            Err(_) => panic!("Couldn't join paths for PATH var."),
+        };
+
         let mut process = match Command::new($crate::fs::executable_path())
-            // .env_clear()
+            .env("PATH", paths_joined)
-            .env("PATH", dummies)
             .stdin(Stdio::piped())
             .stdout(Stdio::piped())
             .spawn()
@@ -103,18 +110,25 @@ macro_rules! nu_error {
             $crate::fs::DisplayPath::display_path(&$path)
         );
 
-        let dummies = $crate::fs::binaries();
+        let test_bins = $crate::fs::binaries();
-        let dummies = dunce::canonicalize(&dummies).unwrap_or_else(|e| {
+        let test_bins = dunce::canonicalize(&test_bins).unwrap_or_else(|e| {
             panic!(
                 "Couldn't canonicalize dummy binaries path {}: {:?}",
-                dummies.display(),
+                test_bins.display(),
                 e
             )
         });
 
+        let mut paths = $crate::shell_os_paths();
+        paths.push(test_bins);
+
+        let paths_joined = match std::env::join_paths(paths.iter()) {
+            Ok(all) => all,
+            Err(_) => panic!("Couldn't join paths for PATH var."),
+        };
+
         let mut process = Command::new($crate::fs::executable_path())
-            // .env_clear()
+            .env("PATH", paths_joined)
-            .env("PATH", dummies)
             .stdout(Stdio::piped())
             .stdin(Stdio::piped())
             .stderr(Stdio::piped())

src/commands/classified/external.rs

@@ -209,10 +209,26 @@ async fn run_with_stdin(
 
             let process_args = args.iter().map(|arg| {
                 let arg = expand_tilde(arg.deref(), || home_dir.as_ref());
-                if let Some(unquoted) = remove_quotes(&arg) {
+
-                    unquoted.to_string()
+                #[cfg(not(windows))]
-                } else {
+                {
-                    arg.as_ref().to_string()
+                    if argument_contains_whitespace(&arg) && argument_is_quoted(&arg) {
+                        if let Some(unquoted) = remove_quotes(&arg) {
+                            format!(r#""{}""#, unquoted)
+                        } else {
+                            arg.as_ref().to_string()
+                        }
+                    } else {
+                        arg.as_ref().to_string()
+                    }
+                }
+                #[cfg(windows)]
+                {
+                    if let Some(unquoted) = remove_quotes(&arg) {
+                        unquoted.to_string()
+                    } else {
+                        arg.as_ref().to_string()
+                    }
                 }
             }).collect::<Vec<String>>();
 
@@ -248,11 +264,24 @@ async fn spawn(
     let command = command.clone();
     let name_tag = command.name_tag.clone();
 
-    let mut process = Exec::cmd(&command.name);
+    let mut process = {
+        #[cfg(windows)]
+        {
+            let mut process = Exec::cmd("cmd");
+            process = process.arg("/c");
+            process = process.arg(&command.name);
+            for arg in args {
+                process = process.arg(&arg);
+            }
+            process
+        }
 
-    for arg in args {
+        #[cfg(not(windows))]
-        process = process.arg(&arg);
+        {
-    }
+            let cmd_with_args = vec![command.name.clone(), args.join(" ")].join(" ");
+            Exec::shell(&cmd_with_args)
+        }
+    };
 
     process = process.cwd(path);
     trace!(target: "nu::run::external", "cwd = {:?}", &path);
@@ -415,6 +444,17 @@ fn remove_quotes(argument: &str) -> Option<&str> {
     Some(&argument[1..size - 1])
 }
 
+#[allow(unused)]
+fn shell_os_paths() -> Vec<std::path::PathBuf> {
+    let mut original_paths = vec![];
+
+    if let Some(paths) = std::env::var_os("PATH") {
+        original_paths = std::env::split_paths(&paths).collect::<Vec<_>>();
+    }
+
+    original_paths
+}
+
 #[cfg(test)]
 mod tests {
     use super::{

tests/shell/pipeline/commands/external.rs

@@ -17,7 +17,7 @@ fn shows_error_for_command_not_found() {
         "ferris_is_not_here.exe"
     );
 
-    assert!(actual.contains("Command not found"));
+    assert!(actual.contains("External command failed"));
 }
 
 mod it_evaluation {