Repository: reubeno/brush
Pull request #375 (2025-02-02 22:11:11)

fix(redirection): allow continuing past redir errors
Redirection failure is an error that will cause the command to fail, but subsequent commands in a compound command, say, should be executed normally.

Resolves #341.

github-actions[bot]: ## Test Results
    2 files      9 suites   1m 48s ⏱️
  669 tests   669 ✅ 0 💤 0 ❌
1 324 runs  1 324 ✅ 0 💤 0 ❌

Results for commit 0395611b.

[test-results]:data:application/gzip;base64,H4sIAF7wn2cC/13MyQ6DIBSF4VcxrLtgkKkv0+AVElKVhmFl+u5Foq12+f0nOStyfrIJ3Tt661AqPjfoirFEk31YKglWNdQpb6MQ+tAjFYD/9PSvmvA3OOOnS7AxhriXWJbtkzDa7zo+L+n32Xy6bD4/QphnnysQZpoLQgZQjI5COWqNBNeDllhzZcQglZJccvT+AOhJ6WMHAQAA

github-actions[bot]: # Performance Benchmark Report
| Benchmark name                         | Baseline (μs) |  Test/PR (μs) |    Delta (μs) | Delta %         |
| -------------------------------------- | ------------- | ------------- | ------------- | --------------- |
| `clone_shell_object                  ` | `   18.16 μs` | `   18.38 μs` | `    0.22 μs` | `🟠 +1.21%    ` |
| `eval_arithmetic                     ` | `    0.17 μs` | `    0.17 μs` | `   -0.00 μs` | `⚪  Unchanged` |
| `expand_one_string                   ` | `    1.94 μs` | `    1.95 μs` | `    0.01 μs` | `⚪  Unchanged` |
| `for_loop                            ` | `   22.08 μs` | `   22.61 μs` | `    0.52 μs` | `🟠 +2.37%    ` |
| `function_call                       ` | `    2.59 μs` | `    2.83 μs` | `    0.24 μs` | `⚪  Unchanged` |
| `instantiate_shell                   ` | `   57.36 μs` | `   57.05 μs` | `   -0.31 μs` | `⚪  Unchanged` |
| `instantiate_shell_with_init_scripts ` | `23881.84 μs` | `24572.13 μs` | `  690.30 μs` | `⚪  Unchanged` |
| `parse_bash_completion               ` | ` 1672.68 μs` | ` 1840.33 μs` | `  167.65 μs` | `🟠 +10.02%   ` |
| `parse_sample_script                 ` | `    1.74 μs` | `    1.74 μs` | `    0.00 μs` | `⚪  Unchanged` |
| `run_echo_builtin_command            ` | `   15.91 μs` | `   15.54 μs` | `   -0.37 μs` | `⚪  Unchanged` |
| `run_one_external_command            ` | ` 2205.13 μs` | ` 2457.86 μs` | `  252.73 μs` | `🟠 +11.46%   ` |
| `tokenize_sample_script              ` | `    2.77 μs` | `    2.88 μs` | `    0.10 μs` | `🟠 +3.78%    ` |

# Code Coverage Report: Only Changed Files listed

| Package | Base Coverage | New Coverage | Difference |
| ------- | ------------- | ------------ | ---------- |
| brush-core/src/builtins/mapfile.rs | 🟢 76.6% | 🟢 76.84% | 🟢 0.24% |
| brush-core/src/builtins/read.rs | 🟠 66.87% | 🟠 66.47% | 🔴 -0.4% |
| brush-core/src/commands.rs | 🟢 87.21% | 🟢 86.97% | 🔴 -0.24% |
| brush-core/src/interp.rs | 🟢 91.58% | 🟢 91.68% | 🟢 0.1% |
| brush-core/src/jobs.rs | 🔴 37.07% | 🔴 43.1% | 🟢 6.03% |
| **Overall Coverage** | **🟢 77.89%** | **🟢 77.98%** | **🟢 0.09%** |

_Minimum allowed coverage is_ `70%`_, this run produced_ `77.98%`
<!-- Sticky Pull Request Comment -->

Patch:
diff --git a/brush-core/src/builtins/mapfile.rs b/brush-core/src/builtins/mapfile.rs
index cb26ba2..8a274d9 100644
--- a/brush-core/src/builtins/mapfile.rs
+++ b/brush-core/src/builtins/mapfile.rs
@@ -72,6 +72,7 @@ impl builtins::Command for MapFileCommand {
         }
 
         let input_file = context
+            .params
             .fd(self.fd)
             .ok_or_else(|| error::Error::BadFileDescriptor(self.fd))?;
 
diff --git a/brush-core/src/builtins/read.rs b/brush-core/src/builtins/read.rs
index ab4f6a5..09247eb 100644
--- a/brush-core/src/builtins/read.rs
+++ b/brush-core/src/builtins/read.rs
@@ -82,6 +82,7 @@ impl builtins::Command for ReadCommand {
         let input_stream = if let Some(fd_num) = self.fd_num_to_read {
             let fd_num = fd_num as u32;
             context
+                .params
                 .fd(fd_num)
                 .ok_or_else(|| error::Error::BadFileDescriptor(fd_num))?
         } else {
diff --git a/brush-core/src/commands.rs b/brush-core/src/commands.rs
index 9b56fc1..3c6ddfd 100644
--- a/brush-core/src/commands.rs
+++ b/brush-core/src/commands.rs
@@ -115,27 +115,17 @@ pub struct ExecutionContext<'a> {
 impl ExecutionContext<'_> {
     /// Returns the standard input file; usable with `write!` et al.
     pub fn stdin(&self) -> openfiles::OpenFile {
-        self.fd(0).unwrap()
+        self.params.stdin()
     }
 
     /// Returns the standard output file; usable with `write!` et al.
     pub fn stdout(&self) -> openfiles::OpenFile {
-        self.fd(1).unwrap()
+        self.params.stdout()
     }
 
     /// Returns the standard error file; usable with `write!` et al.
     pub fn stderr(&self) -> openfiles::OpenFile {
-        self.fd(2).unwrap()
-    }
-
-    /// Returns the file descriptor with the given number.
-    #[allow(clippy::unwrap_in_result)]
-    pub fn fd(&self, fd: u32) -> Option<openfiles::OpenFile> {
-        self.params
-            .open_files
-            .files
-            .get(&fd)
-            .map(|f| f.try_dup().unwrap())
+        self.params.stderr()
     }
 
     pub(crate) fn should_cmd_lead_own_process_group(&self) -> bool {
diff --git a/brush-core/src/interp.rs b/brush-core/src/interp.rs
index 9885fe8..afc77b1 100644
--- a/brush-core/src/interp.rs
+++ b/brush-core/src/interp.rs
@@ -114,6 +114,29 @@ pub struct ExecutionParameters {
     pub process_group_policy: ProcessGroupPolicy,
 }
 
+impl ExecutionParameters {
+    /// Returns the standard input file; usable with `write!` et al.
+    pub fn stdin(&self) -> openfiles::OpenFile {
+        self.fd(0).unwrap()
+    }
+
+    /// Returns the standard output file; usable with `write!` et al.
+    pub fn stdout(&self) -> openfiles::OpenFile {
+        self.fd(1).unwrap()
+    }
+
+    /// Returns the standard error file; usable with `write!` et al.
+    pub fn stderr(&self) -> openfiles::OpenFile {
+        self.fd(2).unwrap()
+    }
+
+    /// Returns the file descriptor with the given number.
+    #[allow(clippy::unwrap_in_result)]
+    pub fn fd(&self, fd: u32) -> Option<openfiles::OpenFile> {
+        self.open_files.files.get(&fd).map(|f| f.try_dup().unwrap())
+    }
+}
+
 #[derive(Clone, Debug, Default)]
 /// Policy for how to manage spawned external processes.
 pub enum ProcessGroupPolicy {
@@ -910,12 +933,16 @@ impl ExecuteInPipeline for ast::SimpleCommand {
         {
             match item {
                 CommandPrefixOrSuffixItem::IoRedirect(redirect) => {
-                    if setup_redirect(context.shell, &mut params, redirect)
-                        .await?
-                        .is_none()
-                    {
-                        // Something went wrong.
-                        return Ok(CommandSpawnResult::ImmediateExit(1));
+                    match setup_redirect(context.shell, &mut params, redirect).await {
+                        Err(e) => {
+                            writeln!(params.stderr(), "error: {e}")?;
+                            return Ok(CommandSpawnResult::ImmediateExit(1));
+                        }
+                        Ok(None) => {
+                            // Something went wrong.
+                            return Ok(CommandSpawnResult::ImmediateExit(1));
+                        }
+                        Ok(_) => (),
                     }
                 }
                 CommandPrefixOrSuffixItem::ProcessSubstitution(kind, subshell_command) => {


Test patch:
diff --git a/brush-shell/tests/cases/redirection.yaml b/brush-shell/tests/cases/redirection.yaml
index 89707c1..9058546 100644
--- a/brush-shell/tests/cases/redirection.yaml
+++ b/brush-shell/tests/cases/redirection.yaml
@@ -109,3 +109,8 @@ cases:
     ignore_stderr: true
     stdin: |
       ( : $(echo hi >&3); ) 3>output.txt
+
+  - name: "Redirection failure"
+    ignore_stderr: true
+    stdin: |
+      echo hi > /non-existent-dir/file.txt; echo following-command