Improve shelling out (#1273)

Improvements to shelling out

Cargo.lock

@@ -552,12 +552,6 @@ dependencies = [
  "crossbeam-utils 0.7.0",
 ]
 
-[[package]]
-name = "crossbeam-utils"
-version = "0.5.0"
-source = "registry+https://github.com/rust-lang/crates.io-index"
-checksum = "677d453a17e8bd2b913fa38e8b9cf04bcdbb5be790aa294f2389661d72036015"
-
 [[package]]
 name = "crossbeam-utils"
 version = "0.6.6"
@@ -2301,6 +2295,7 @@ dependencies = [
  "unicode-xid",
  "url",
  "users",
+ "which",
 ]
 
 [[package]]
@@ -3802,11 +3797,11 @@ checksum = "8ea5119cdb4c55b55d432abb513a0429384878c15dde60cc77b1c99de1a95a6a"
 
 [[package]]
 name = "subprocess"
-version = "0.1.18"
+version = "0.1.20"
 source = "registry+https://github.com/rust-lang/crates.io-index"
-checksum = "28fc0f40f0c0da73339d347aa7d6d2b90341a95683a47722bc4eebed71ff3c00"
+checksum = "68713fc0f9d941642c1e020d622e6421dfe09e8891ddd4bfa2109fda9a40431d"
 dependencies = [
- "crossbeam-utils 0.5.0",
+ "crossbeam-utils 0.7.0",
  "libc",
  "winapi 0.3.8",
 ]
@@ -4452,6 +4447,16 @@ dependencies = [
  "nom 4.2.3",
 ]
 
+[[package]]
+name = "which"
+version = "3.1.0"
+source = "registry+https://github.com/rust-lang/crates.io-index"
+checksum = "5475d47078209a02e60614f7ba5e645ef3ed60f771920ac1906d7c1cc65024c8"
+dependencies = [
+ "failure",
+ "libc",
+]
+
 [[package]]
 name = "widestring"
 version = "0.4.0"

Cargo.toml

@@ -101,10 +101,11 @@ nom_locate = "1.0.0"
 nom-tracable = "0.4.1"
 unicode-xid = "0.2.0"
 serde_ini = "0.2.0"
-subprocess = "0.1.18"
+subprocess = "0.1.20"
 pretty-hex = "0.1.1"
 hex = "0.4"
 tempfile = "3.1.0"
+which = "3.1.0"
 ichwh = "0.3"
 textwrap = {version = "0.11.0", features = ["term_size"]}
 shellexpand = "1.1.1"

src/commands/classified/external.rs

@@ -89,6 +89,14 @@ pub(crate) async fn run_external_command(
 ) -> Result<Option<InputStream>, ShellError> {
     trace!(target: "nu::run::external", "-> {}", command.name);
 
+    if !did_find_command(&command.name) {
+        return Err(ShellError::labeled_error(
+            "Command not found",
+            "command not found",
+            &command.name_tag,
+        ));
+    }
+
     if command.has_it_argument() {
         run_with_iterator_arg(command, context, input, is_last).await
     } else {
@@ -333,13 +341,23 @@ async fn spawn(
                     }
                 }
 
+                // We can give an error when we see a non-zero exit code, but this is different
+                // than what other shells will do.
+                let cfg = crate::data::config::config(Tag::unknown());
+                if let Ok(cfg) = cfg {
+                    if cfg.contains_key("nonzero_exit_errors") {
                         yield Ok(Value {
-                    value: UntaggedValue::Error(ShellError::labeled_error(
+                            value: UntaggedValue::Error(
+                                ShellError::labeled_error(
                                     "External command failed",
                                     "command failed",
-                            &name_tag)),
+                                    &name_tag,
-                    tag: name_tag
+                                )
+                            ),
+                            tag: name_tag,
                         });
+                    }
+                }
                 return;
             }
 
@@ -381,6 +399,11 @@ async fn spawn(
                     None => futures_timer::Delay::new(std::time::Duration::from_millis(10)).await,
                     Some(status) => {
                         if !status.success() {
+                            // We can give an error when we see a non-zero exit code, but this is different
+                            // than what other shells will do.
+                            let cfg = crate::data::config::config(Tag::unknown());
+                            if let Ok(cfg) = cfg {
+                                if cfg.contains_key("nonzero_exit_errors") {
                                     yield Ok(Value {
                                         value: UntaggedValue::Error(
                                             ShellError::labeled_error(
@@ -392,6 +415,8 @@ async fn spawn(
                                         tag: name_tag,
                                     });
                                 }
+                            }
+                        }
                         break;
                     }
                 }
@@ -408,6 +433,27 @@ async fn spawn(
     }
 }
 
+fn did_find_command(name: &str) -> bool {
+    #[cfg(not(windows))]
+    {
+        which::which(name).is_ok()
+    }
+
+    #[cfg(windows)]
+    {
+        if which::which(name).is_ok() {
+            true
+        } else {
+            let cmd_builtins = [
+                "call", "cls", "color", "date", "dir", "echo", "find", "hostname", "pause",
+                "start", "time", "title", "ver", "copy", "mkdir", "rename", "rd", "rmdir", "type",
+            ];
+
+            cmd_builtins.contains(&name)
+        }
+    }
+}
+
 fn expand_tilde<SI: ?Sized, P, HD>(input: &SI, home_dir: HD) -> std::borrow::Cow<str>
 where
     SI: AsRef<str>,
@@ -459,70 +505,60 @@ fn shell_os_paths() -> Vec<std::path::PathBuf> {
 mod tests {
     use super::{
         add_quotes, argument_contains_whitespace, argument_is_quoted, expand_tilde, remove_quotes,
-        run_external_command, Context, OutputStream,
+        run_external_command, Context,
     };
     use futures::executor::block_on;
-    use futures::stream::TryStreamExt;
     use nu_errors::ShellError;
-    use nu_protocol::{UntaggedValue, Value};
     use nu_test_support::commands::ExternalBuilder;
 
-    async fn read(mut stream: OutputStream) -> Option<Value> {
+    // async fn read(mut stream: OutputStream) -> Option<Value> {
-        match stream.try_next().await {
+    //     match stream.try_next().await {
-            Ok(val) => {
+    //         Ok(val) => {
-                if let Some(val) = val {
+    //             if let Some(val) = val {
-                    val.raw_value()
+    //                 val.raw_value()
-                } else {
+    //             } else {
-                    None
+    //                 None
-                }
+    //             }
-            }
+    //         }
-            Err(_) => None,
+    //         Err(_) => None,
-        }
+    //     }
-    }
+    // }
 
     async fn non_existent_run() -> Result<(), ShellError> {
         let cmd = ExternalBuilder::for_name("i_dont_exist.exe").build();
 
         let mut ctx = Context::basic().expect("There was a problem creating a basic context.");
 
-        let stream = run_external_command(cmd, &mut ctx, None, false)
+        assert!(run_external_command(cmd, &mut ctx, None, false)
-            .await?
+            .await
-            .expect("There was a problem running the external command.");
+            .is_err());
-
-        match read(stream.into()).await {
-            Some(Value {
-                value: UntaggedValue::Error(_),
-                ..
-            }) => {}
-            None | _ => panic!("Apparently a command was found (It's not supposed to be found)"),
-        }
 
         Ok(())
     }
 
-    async fn failure_run() -> Result<(), ShellError> {
+    // async fn failure_run() -> Result<(), ShellError> {
-        let cmd = ExternalBuilder::for_name("fail").build();
+    //     let cmd = ExternalBuilder::for_name("fail").build();
 
-        let mut ctx = Context::basic().expect("There was a problem creating a basic context.");
+    //     let mut ctx = Context::basic().expect("There was a problem creating a basic context.");
-        let stream = run_external_command(cmd, &mut ctx, None, false)
+    //     let stream = run_external_command(cmd, &mut ctx, None, false)
-            .await?
+    //         .await?
-            .expect("There was a problem running the external command.");
+    //         .expect("There was a problem running the external command.");
 
-        match read(stream.into()).await {
+    //     match read(stream.into()).await {
-            Some(Value {
+    //         Some(Value {
-                value: UntaggedValue::Error(_),
+    //             value: UntaggedValue::Error(_),
-                ..
+    //             ..
-            }) => {}
+    //         }) => {}
-            None | _ => panic!("Command didn't fail."),
+    //         None | _ => panic!("Command didn't fail."),
-        }
+    //     }
 
-        Ok(())
+    //     Ok(())
-    }
+    // }
 
-    #[test]
+    // #[test]
-    fn identifies_command_failed() -> Result<(), ShellError> {
+    // fn identifies_command_failed() -> Result<(), ShellError> {
-        block_on(failure_run())
+    //     block_on(failure_run())
-    }
+    // }
 
     #[test]
     fn identifies_command_not_found() -> Result<(), ShellError> {

tests/shell/pipeline/commands/external.rs

@@ -1,14 +1,14 @@
 use nu_test_support::{nu, nu_error};
 
-#[test]
+// #[test]
-fn shows_error_for_command_that_fails() {
+// fn shows_error_for_command_that_fails() {
-    let actual = nu_error!(
+//     let actual = nu_error!(
-        cwd: ".",
+//         cwd: ".",
-        "fail"
+//         "fail"
-    );
+//     );
 
-    assert!(actual.contains("External command failed"));
+//     assert!(actual.contains("External command failed"));
-}
+// }
 
 #[test]
 fn shows_error_for_command_not_found() {
@@ -17,7 +17,7 @@ fn shows_error_for_command_not_found() {
         "ferris_is_not_here.exe"
     );
 
-    assert!(actual.contains("External command failed"));
+    assert!(actual.contains("Command not found"));
 }
 
 mod it_evaluation {