Add --redirect-combine option to run-external (#8918)

# Description

Add option that combines both output streams to the `run-external`
command.

This allows you to do something like this:

```nushell
let res = do -i { run-external --redirect-combine <command that prints to stdout and stderr> } | complete

if $res.exit_code != 0 {
  # Only print output when command has failed.
  print "The command has failed, these are the logs:"
  print $res.stdout
}
```

# User-Facing Changes

No breaking changes, just an extra option.

# Tests + Formatting

Added a test that checks the new option

# After Submitting
<!-- If your PR had any user-facing changes, update [the
documentation](https://github.com/nushell/nushell.github.io) after the
PR is merged, if necessary. This will help us keep the docs up to date.
-->

Co-authored-by: Jelle Besseling <jelle@bigbridge.nl>

Cargo.lock

@@ -2868,6 +2868,7 @@ dependencies = [
  "num-traits",
  "once_cell",
  "open",
+ "os_pipe",
  "pathdiff",
  "percent-encoding",
  "polars",
@@ -3427,6 +3428,16 @@ dependencies = [
  "hashbrown 0.12.3",
 ]
 
+[[package]]
+name = "os_pipe"
+version = "1.1.3"
+source = "registry+https://github.com/rust-lang/crates.io-index"
+checksum = "a53dbb20faf34b16087a931834cba2d7a73cc74af2b7ef345a4c8324e2409a12"
+dependencies = [
+ "libc",
+ "windows-sys 0.45.0",
+]
+
 [[package]]
 name = "os_str_bytes"
 version = "6.5.0"

crates/nu-command/Cargo.toml

@@ -100,6 +100,7 @@ uuid = { version = "1.3.0", features = ["v4"] }
 wax = { version = "0.5.0" }
 which = { version = "4.4.0", optional = true }
 print-positions = "0.6.1"
+os_pipe = "1.1.3"
 
 [target.'cfg(windows)'.dependencies]
 winreg = "0.50.0"

crates/nu-command/src/env/config/utils.rs

@@ -71,6 +71,7 @@ pub(crate) fn gen_command(
         arg_keep_raw: vec![false; number_of_args],
         redirect_stdout: false,
         redirect_stderr: false,
+        redirect_combine: false,
         env_vars: env_vars_str,
         trim_end_newline: false,
     }

crates/nu-command/src/system/exec.rs

@@ -67,6 +67,7 @@ fn exec(
 
     let redirect_stdout = call.has_flag("redirect-stdout");
     let redirect_stderr = call.has_flag("redirect-stderr");
+    let redirect_combine = call.has_flag("redirect-combine");
     let trim_end_newline = call.has_flag("trim-end-newline");
 
     let external_command = create_external_command(
@@ -75,6 +76,7 @@ fn exec(
         call,
         redirect_stdout,
         redirect_stderr,
+        redirect_combine,
         trim_end_newline,
     )?;
 

crates/nu-command/src/system/run_external.rs

@@ -11,6 +11,7 @@ use nu_protocol::{
     SyntaxShape, Type, Value,
 };
 use nu_system::ForegroundProcess;
+use os_pipe::PipeReader;
 use pathdiff::diff_paths;
 use std::collections::HashMap;
 use std::io::{BufRead, BufReader, Read, Write};
@@ -41,6 +42,11 @@ impl Command for External {
             .input_output_types(vec![(Type::Any, Type::Any)])
             .switch("redirect-stdout", "redirect stdout to the pipeline", None)
             .switch("redirect-stderr", "redirect stderr to the pipeline", None)
+            .switch(
+                "redirect-combine",
+                "redirect both stdout and stderr combined to the pipeline (collected in stdout)",
+                None,
+            )
             .switch("trim-end-newline", "trimming end newlines", None)
             .required("command", SyntaxShape::String, "external command to run")
             .rest("args", SyntaxShape::Any, "arguments for external command")
@@ -56,14 +62,25 @@ impl Command for External {
     ) -> Result<PipelineData, ShellError> {
         let redirect_stdout = call.has_flag("redirect-stdout");
         let redirect_stderr = call.has_flag("redirect-stderr");
+        let redirect_combine = call.has_flag("redirect-combine");
         let trim_end_newline = call.has_flag("trim-end-newline");
 
+        if redirect_combine && (redirect_stdout || redirect_stderr) {
+            return Err(ShellError::ExternalCommand {
+                label: "Cannot use --redirect-combine with --redirect-stdout or --redirect-stderr"
+                    .into(),
+                help: "use either --redirect-combine or redirect a single output stream".into(),
+                span: call.head,
+            });
+        }
+
         let command = create_external_command(
             engine_state,
             stack,
             call,
             redirect_stdout,
             redirect_stderr,
+            redirect_combine,
             trim_end_newline,
         )?;
 
@@ -93,6 +110,7 @@ pub fn create_external_command(
     call: &Call,
     redirect_stdout: bool,
     redirect_stderr: bool,
+    redirect_combine: bool,
     trim_end_newline: bool,
 ) -> Result<ExternalCommand, ShellError> {
     let name: Spanned<String> = call.req(engine_state, stack, 0)?;
@@ -149,6 +167,7 @@ pub fn create_external_command(
         arg_keep_raw,
         redirect_stdout,
         redirect_stderr,
+        redirect_combine,
         env_vars: env_vars_str,
         trim_end_newline,
     })
@@ -161,6 +180,7 @@ pub struct ExternalCommand {
     pub arg_keep_raw: Vec<bool>,
     pub redirect_stdout: bool,
     pub redirect_stderr: bool,
+    pub redirect_combine: bool,
     pub env_vars: HashMap<String, String>,
     pub trim_end_newline: bool,
 }
@@ -177,10 +197,10 @@ impl ExternalCommand {
 
         let ctrlc = engine_state.ctrlc.clone();
 
-        let mut fg_process = ForegroundProcess::new(
-            self.create_process(&input, false, head)?,
-            engine_state.pipeline_externals_state.clone(),
-        );
+        #[allow(unused_mut)]
+        let (cmd, mut reader) = self.create_process(&input, false, head)?;
+        let mut fg_process =
+            ForegroundProcess::new(cmd, engine_state.pipeline_externals_state.clone());
         // mut is used in the windows branch only, suppress warning on other platforms
         #[allow(unused_mut)]
         let mut child;
@@ -211,8 +231,10 @@ impl ExternalCommand {
                         .any(|&cmd| command_name_upper == cmd);
 
                     if looks_like_cmd_internal {
+                        let (cmd, new_reader) = self.create_process(&input, true, head)?;
+                        reader = new_reader;
                         let mut cmd_process = ForegroundProcess::new(
-                            self.create_process(&input, true, head)?,
+                            cmd,
                             engine_state.pipeline_externals_state.clone(),
                         );
                         child = cmd_process.spawn();
@@ -242,9 +264,11 @@ impl ExternalCommand {
                                                     item: file_name.to_string_lossy().to_string(),
                                                     span: self.name.span,
                                                 };
+                                                let (cmd, new_reader) = new_command
+                                                    .create_process(&input, true, head)?;
+                                                reader = new_reader;
                                                 let mut cmd_process = ForegroundProcess::new(
-                                                    new_command
-                                                        .create_process(&input, true, head)?,
+                                                    cmd,
                                                     engine_state.pipeline_externals_state.clone(),
                                                 );
                                                 child = cmd_process.spawn();
@@ -419,6 +443,7 @@ impl ExternalCommand {
                 let commandname = self.name.item.clone();
                 let redirect_stdout = self.redirect_stdout;
                 let redirect_stderr = self.redirect_stderr;
+                let redirect_combine = self.redirect_combine;
                 let span = self.name.span;
                 let output_ctrlc = ctrlc.clone();
                 let stderr_ctrlc = ctrlc.clone();
@@ -441,6 +466,12 @@ impl ExternalCommand {
                                         .to_string(), span }
                             })?;
 
+                            read_and_redirect_message(stdout, stdout_tx, ctrlc)
+                        } else if redirect_combine {
+                            let stdout = reader.ok_or_else(|| {
+                                ShellError::ExternalCommand { label: "Error taking combined stdout and stderr from external".to_string(), help: "Combined redirects need access to reader pipe of an external command"
+                                        .to_string(), span }
+                            })?;
                             read_and_redirect_message(stdout, stdout_tx, ctrlc)
                         }
 
@@ -516,7 +547,7 @@ impl ExternalCommand {
                 let exit_code_receiver = ValueReceiver::new(exit_code_rx);
 
                 Ok(PipelineData::ExternalStream {
-                    stdout: if redirect_stdout {
+                    stdout: if redirect_stdout || redirect_combine {
                         Some(RawStream::new(
                             Box::new(stdout_receiver),
                             output_ctrlc.clone(),
@@ -553,7 +584,7 @@ impl ExternalCommand {
         input: &PipelineData,
         use_cmd: bool,
         span: Span,
-    ) -> Result<CommandSys, ShellError> {
+    ) -> Result<(CommandSys, Option<PipeReader>), ShellError> {
         let mut process = if let Some(d) = self.env_vars.get("PWD") {
             let mut process = if use_cmd {
                 self.spawn_cmd_command(d)
@@ -583,6 +614,13 @@ impl ExternalCommand {
 
         // If the external is not the last command, its output will get piped
         // either as a string or binary
+        let reader = if self.redirect_combine {
+            let (reader, writer) = os_pipe::pipe()?;
+            let writer_clone = writer.try_clone()?;
+            process.stdout(writer);
+            process.stderr(writer_clone);
+            Some(reader)
+        } else {
             if self.redirect_stdout {
                 process.stdout(Stdio::piped());
             }
@@ -590,6 +628,8 @@ impl ExternalCommand {
             if self.redirect_stderr {
                 process.stderr(Stdio::piped());
             }
+            None
+        };
 
         // If there is an input from the pipeline. The stdin from the process
         // is piped so it can be used to send the input information
@@ -597,7 +637,7 @@ impl ExternalCommand {
             process.stdin(Stdio::piped());
         }
 
-        Ok(process)
+        Ok((process, reader))
     }
 
     fn create_command(&self, cwd: &str) -> Result<CommandSys, ShellError> {

crates/nu-command/tests/commands/run_external.rs

@@ -320,3 +320,18 @@ fn quotes_trimmed_when_shelling_out() {
 
     assert_eq!(actual.out, "foo");
 }
+
+#[test]
+fn redirect_combine() {
+    Playground::setup("redirect_combine", |dirs, _| {
+        let actual = nu!(
+            cwd: dirs.test(), pipeline(
+            r#"
+                run-external --redirect-combine sh [-c 'echo Foo; echo >&2 Bar']
+            "#
+        ));
+
+        // Lines are collapsed in the nu! macro
+        assert_eq!(actual.out, "FooBar");
+    });
+}