From 80881c75f90cf12bc372c53ec8cece22237f0091 Mon Sep 17 00:00:00 2001 From: WindSoilder Date: Fri, 1 Dec 2023 01:52:02 +0800 Subject: [PATCH] When using redirection, if a command generates non-zero exit code, the script should stop running (#11191) # Description Fixes: #11153 To make sure scripts stop from running on non-zero exit code, we need to invoke `might_consume_external_result` on `PipelineData::ExternalStream`, so it can tell nushell if this command exists with non-zero exit code. And this pr also adjusts some test cases. # User-Facing Changes ```nushell ^false out> /dev/null; print "ok" ``` After this pr, it shouldn't print ok. # Tests + Formatting Done --- .../nu-command/tests/commands/redirection.rs | 49 ++++++++++--------- crates/nu-engine/src/eval.rs | 49 ++++++++----------- 2 files changed, 47 insertions(+), 51 deletions(-) diff --git a/crates/nu-command/tests/commands/redirection.rs b/crates/nu-command/tests/commands/redirection.rs index e55cb557a..2dff56e0e 100644 --- a/crates/nu-command/tests/commands/redirection.rs +++ b/crates/nu-command/tests/commands/redirection.rs @@ -161,29 +161,32 @@ fn same_target_redirection_with_too_much_stderr_not_hang_nushell() { #[test] fn redirection_keep_exit_codes() { - Playground::setup( - "redirection should keep exit code the same", - |dirs, sandbox| { - let script_body = r#"exit 10"#; - #[cfg(not(windows))] - let output = { - sandbox.with_files(vec![FileWithContent("test.sh", script_body)]); - nu!( - cwd: dirs.test(), - "bash test.sh out> out.txt err> err.txt; echo $env.LAST_EXIT_CODE" - ) - }; - #[cfg(windows)] - let output = { - sandbox.with_files(vec![FileWithContent("test.bat", script_body)]); - nu!( - cwd: dirs.test(), - "cmd /D /c test.bat out> out.txt err> err.txt; echo $env.LAST_EXIT_CODE" - ) - }; - assert_eq!(output.out, "10") - }, - ) + let out = nu!("do -i { nu --testbin fail e> a.txt } | complete | get exit_code"); + // needs to use contains "1", because it complete will output `Some(RawStream)`. + assert!(out.out.contains('1')); +} + +#[test] +fn redirection_with_non_zero_exit_code_should_stop_from_running() { + Playground::setup("redirection with non zero exit code", |dirs, _| { + for redirection in ["o>", "o>>", "e>", "e>>", "o+e>", "o+e>>"] { + let output = nu!( + cwd: dirs.test(), + &format!("nu --testbin fail {redirection} log.txt; echo 3") + ); + assert!(!output.out.contains('3')); + } + }); + + Playground::setup("redirection with non zero exit code", |dirs, _| { + for (out, err) in [("o>", "e>"), ("o>>", "e>"), ("o>", "e>>"), ("o>>", "e>>")] { + let output = nu!( + cwd: dirs.test(), + &format!("nu --testbin fail {out} log.txt {err} err_log.txt; echo 3") + ); + assert!(!output.out.contains('3')); + } + }) } #[test] diff --git a/crates/nu-engine/src/eval.rs b/crates/nu-engine/src/eval.rs index 6e3bc9083..46a8c5095 100644 --- a/crates/nu-engine/src/eval.rs +++ b/crates/nu-engine/src/eval.rs @@ -828,17 +828,17 @@ fn eval_element_with_input( // save is internal command, normally it exists with non-ExternalStream // but here in redirection context, we make it returns ExternalStream // So nu handles exit_code correctly - ( - PipelineData::ExternalStream { - stdout: None, - stderr: None, - exit_code, - span: *span, - metadata: None, - trim_end_newline: false, - }, - false, - ) + // + // Also, we don't want to run remaining commands if this command exits with non-zero + // exit code, so we need to consume and check exit_code too + might_consume_external_result(PipelineData::ExternalStream { + stdout: None, + stderr: None, + exit_code, + span: *span, + metadata: None, + trim_end_newline: false, + }) }) } Some(out_stream) => { @@ -854,7 +854,7 @@ fn eval_element_with_input( input, )); - Ok(( + Ok(might_consume_external_result( PipelineData::ExternalStream { stdout: out_stream, stderr: None, @@ -863,7 +863,6 @@ fn eval_element_with_input( metadata: None, trim_end_newline: false, }, - false, )) } } @@ -903,17 +902,14 @@ fn eval_element_with_input( // save is internal command, normally it exists with non-ExternalStream // but here in redirection context, we make it returns ExternalStream // So nu handles exit_code correctly - ( - PipelineData::ExternalStream { - stdout: None, - stderr: None, - exit_code, - span: *out_span, - metadata: None, - trim_end_newline: false, - }, - false, - ) + might_consume_external_result(PipelineData::ExternalStream { + stdout: None, + stderr: None, + exit_code, + span: *out_span, + metadata: None, + trim_end_newline: false, + }) }) } else { Err(ShellError::CommandNotFound { span: *out_span }) @@ -1098,9 +1094,6 @@ pub fn eval_block( ); match (eval_result, redirect_stderr) { - (Ok((pipeline_data, _)), true) => { - input = pipeline_data; - } (Err(error), true) => { input = PipelineData::Value( Value::error( @@ -1110,7 +1103,7 @@ pub fn eval_block( None, ) } - (output, false) => { + (output, _) => { let output = output?; input = output.0; // external command may runs to failed