Fix try not working with let, etc. (#13885)

# Description
Partialy addresses #13868. `try` does not catch non-zero exit code
errors from the last command in a pipeline if the result is assigned to
a variable using `let` (or `mut`).

This was fixed by adding a new `OutDest::Value` case. This is used when
the pipeline is in a "value" position. I.e., it will be collected into a
value. This ended up replacing most of the usages of `OutDest::Capture`.
So, this PR also renames `OutDest::Capture` to `OutDest::PipeSeparate`
to better fit the few remaining use cases for it.

# User-Facing Changes
Bug fix.

# Tests + Formatting
Added two tests.
This commit is contained in:
Ian Manske
2024-09-23 04:44:25 -07:00
committed by GitHub
parent 2541a712e4
commit 03ee54a4df
32 changed files with 127 additions and 98 deletions

View File

@@ -580,8 +580,8 @@ impl ByteStream {
copy_with_signals(file, dest, span, signals)?;
}
ByteStreamSource::Child(mut child) => {
// All `OutDest`s except `OutDest::Capture` will cause `stderr` to be `None`.
// Only `save`, `tee`, and `complete` set the stderr `OutDest` to `OutDest::Capture`,
// All `OutDest`s except `OutDest::PipeSeparate` will cause `stderr` to be `None`.
// Only `save`, `tee`, and `complete` set the stderr `OutDest` to `OutDest::PipeSeparate`,
// and those commands have proper simultaneous handling of stdout and stderr.
debug_assert!(child.stderr.is_none(), "stderr should not exist");
@@ -614,7 +614,7 @@ impl ByteStream {
write_to_out_dest(read, stdout, true, span, signals)?;
}
ByteStreamSource::File(file) => match stdout {
OutDest::Pipe | OutDest::Capture | OutDest::Null => {}
OutDest::Pipe | OutDest::PipeSeparate | OutDest::Value | OutDest::Null => {}
OutDest::Inherit => {
copy_with_signals(file, io::stdout(), span, signals)?;
}
@@ -970,7 +970,7 @@ fn write_to_out_dest(
signals: &Signals,
) -> Result<(), ShellError> {
match stream {
OutDest::Pipe | OutDest::Capture => return Ok(()),
OutDest::Pipe | OutDest::PipeSeparate | OutDest::Value => return Ok(()),
OutDest::Null => copy_with_signals(read, io::sink(), span, signals),
OutDest::Inherit if stdout => copy_with_signals(read, io::stdout(), span, signals),
OutDest::Inherit => copy_with_signals(read, io::stderr(), span, signals),

View File

@@ -10,13 +10,17 @@ pub enum OutDest {
/// If stdout and stderr are both set to `Pipe`,
/// then they will combined into the `stdout` of [`ChildProcess`](crate::process::ChildProcess).
Pipe,
/// Capture output to later be collected into a [`Value`](crate::Value), `Vec`, or used in some other way.
/// Redirect the stdout and/or stderr of one command as the input for the next command in the pipeline.
///
/// The output stream(s) will be available in the `stdout` or `stderr` of [`ChildProcess`](crate::process::ChildProcess).
///
/// This is similar to `Pipe` but will never combine stdout and stderr
/// or place an external command's stderr into `stdout` of [`ChildProcess`](crate::process::ChildProcess).
Capture,
PipeSeparate,
/// Signifies the result of the pipeline will be immediately collected into a value after this command.
///
/// So, it is fine to collect the stream ahead of time in the current command.
Value,
/// Ignore output.
///
/// This will forward output to the null device for the platform.
@@ -46,7 +50,7 @@ impl TryFrom<&OutDest> for Stdio {
fn try_from(out_dest: &OutDest) -> Result<Self, Self::Error> {
match out_dest {
OutDest::Pipe | OutDest::Capture => Ok(Self::piped()),
OutDest::Pipe | OutDest::PipeSeparate | OutDest::Value => Ok(Self::piped()),
OutDest::Null => Ok(Self::null()),
OutDest::Inherit => Ok(Self::inherit()),
OutDest::File(file) => Ok(file.try_clone()?.into()),

View File

@@ -167,8 +167,8 @@ impl PipelineData {
/// Writes all values or redirects all output to the current [`OutDest`]s in `stack`.
///
/// For [`OutDest::Pipe`] and [`OutDest::Capture`], this will return the `PipelineData` as is
/// without consuming input and without writing anything.
/// For [`OutDest::Pipe`] and [`OutDest::PipeSeparate`], this will return the `PipelineData` as
/// is without consuming input and without writing anything.
///
/// For the other [`OutDest`]s, the given `PipelineData` will be completely consumed
/// and `PipelineData::Empty` will be returned (assuming no errors).
@@ -178,11 +178,18 @@ impl PipelineData {
stack: &mut Stack,
) -> Result<PipelineData, ShellError> {
match (self, stack.stdout()) {
(data, OutDest::Pipe | OutDest::Capture) => return Ok(data),
(PipelineData::Empty, ..) => {}
(data, OutDest::Pipe | OutDest::PipeSeparate) => return Ok(data),
(data, OutDest::Value) => {
let metadata = data.metadata();
let span = data.span().unwrap_or(Span::unknown());
return data
.into_value(span)
.map(|val| PipelineData::Value(val, metadata));
}
(PipelineData::ByteStream(stream, ..), stdout) => {
stream.write_to_out_dests(stdout, stack.stderr())?;
}
(PipelineData::Empty, ..) => {}
(PipelineData::Value(..), OutDest::Null) => {}
(PipelineData::ListStream(stream, ..), OutDest::Null) => {
// we need to drain the stream in case there are external commands in the pipeline