Repository: reubeno/brush
Pull request #490 (2025-05-12 18:12:52)

feat(mapfile): implement `-n`, `-d`, `-s`

github-actions[bot]: # Performance Benchmark Report
| Benchmark name                         | Baseline (μs) |  Test/PR (μs) |    Delta (μs) | Delta %         |
| -------------------------------------- | ------------- | ------------- | ------------- | --------------- |
| `clone_shell_object                  ` | `   18.34 μs` | `   18.16 μs` | `   -0.18 μs` | `🟢 -0.98%    ` |
| `eval_arithmetic                     ` | `    0.17 μs` | `    0.17 μs` | `    0.00 μs` | `⚪  Unchanged` |
| `expand_one_string                   ` | `    1.79 μs` | `    1.82 μs` | `    0.03 μs` | `⚪  Unchanged` |
| `for_loop                            ` | `   22.46 μs` | `   23.49 μs` | `    1.03 μs` | `🟠 +4.57%    ` |
| `function_call                       ` | `    2.41 μs` | `    2.33 μs` | `   -0.08 μs` | `⚪  Unchanged` |
| `instantiate_shell                   ` | `   57.20 μs` | `   57.47 μs` | `    0.26 μs` | `⚪  Unchanged` |
| `instantiate_shell_with_init_scripts ` | `23467.09 μs` | `23764.13 μs` | `  297.05 μs` | `🟠 +1.27%    ` |
| `parse_bash_completion               ` | ` 1667.97 μs` | ` 1681.78 μs` | `   13.80 μs` | `⚪  Unchanged` |
| `parse_sample_script                 ` | `    1.77 μs` | `    1.82 μs` | `    0.04 μs` | `🟠 +2.43%    ` |
| `run_echo_builtin_command            ` | `   15.36 μs` | `   15.35 μs` | `   -0.01 μs` | `⚪  Unchanged` |
| `run_one_external_command            ` | ` 6747.02 μs` | ` 2072.85 μs` | `-4674.17 μs` | `🟢 -69.28%   ` |
| `tokenize_sample_script              ` | `    2.88 μs` | `    2.90 μs` | `    0.03 μs` | `⚪  Unchanged` |

# Code Coverage Report: Only Changed Files listed

| Package | Base Coverage | New Coverage | Difference |
| ------- | ------------- | ------------ | ---------- |
| brush-core/src/builtins/mapfile.rs | 🟢 76.84% | 🟢 76.19% | 🔴 -0.65% |
| **Overall Coverage** | **🟢 74.9%** | **🟢 74.89%** | **🔴 -0.01%** |

_Minimum allowed coverage is_ `70%`_, this run produced_ `74.89%`

# Test Summary: bash-completion test suite
| Outcome            | Count                   | Percentage |
| ------------------ | ----------------------: | ---------: |
| ✅ Pass            | 1296            | <span style='color:green'>61.45</span> |
| ❗️ Error           | 228           | <span style='color:red'>10.81</span> |
| ❌ Fail            | 199            | <span style='color:red'>9.44</span> |
| ⏩ Skip            | 371            | 17.59 |
| ❎ Expected Fail   | 14   | 0.66 |
| ✔️ Unexpected Pass | 1 | <span style='color:red'>0.05</span> |
| 📊 Total           | 2109           | 100.00 |

<!-- Sticky Pull Request Comment -->
Elsie19: I sort of over-engineered `LineCount`. I can revert the last commit if you want, I just think (barring line count), it looks and feels much nicer to use.
reubeno: > I sort of over-engineered `LineCount`. I can revert the last commit if you want, I just think (barring line count), it looks and feels much nicer to use.

I agree it looks nicer to use in the core implementation logic of the builtin. I also agree it might be nice to simplify the `LineCount` implementation a bit.

What do you think about using `Range`s to reduce the custom code, e.g.:

```rust
use either;

/// Iterates from 0 up to (but not including) the given `count` value;
/// as a special case, if `count` is 0, continues iterating indefinitely.
fn count_to(count: i32) -> impl Iterator<Item = i32> {
    if count == 0 {
        either::Left(0..)
    } else {
        either::Right(0..count)
    }
}
```

I had to use the `either` crate (or else `Box<dyn...>`) because the indefinite range and the limited range have different concrete types and I placed it in a separate function, but hopefully you get the idea.

_I was also idly wondering whether we might consider placing the actual body of the read loop in an iterator and then be able to use adaptors like `.take(max_count)` to implement the `-n` constraint. With that said, I didn't think too deeply into that and am not sure if we'd be happier with the result._

(Also, shout-out for adding `default_value = "MAPFILE"`. That's a good compat fix!)
Elsie19: I don't think adding an extra crate just to reduce the line count in one file is particularly useful.

We could also maybe move `LineCount` into `OpenFile` to abstract it away from `mapfile` as well.

I was also thinking about moving the read loop into the iterator. I was actually wondering why it *wasn't* the case in the first place!
reubeno: > I don't think adding an extra crate just to reduce the line count in one file is particularly useful.

In general, I agree with your hesitance. In this specific case, `either` is something I'd looked at multiple times previously, seems reasonably well supported, doesn't have any dependencies of its own (when used with only default features), and provides usability value. I think it would be acceptable to pull it in here, but I'm okay with other options too.

> We could also maybe move LineCount into OpenFile to abstract it away from mapfile as well.

I'd be fine--and would probably prefer--it living in its own `.rs` file if we don't go with something like the `count_to` function above; I just would want it to be `pub(crate)` and not export it.

> I was actually wondering why it wasn't the case in the first place! I think the implementation of mapfile could be a lot more readable if it was rewritten.

I don't think there was a particular reason. We should be mindful of the fallibility of iterating and make sure we can reasonably implement `-C` / `-c` options for sending lines to a callback in batch... but assuming those seem fine, please feel free to propose a refactoring that you think will be easier to read and maintain.
Elsie19: I think that should come in a later PR. I will add tests and undraft this.
Elsie19: Actually I might need to rework all of mapfile, it's not very futureproof and I can't get `-n` to work.
reubeno: > On a somewhat related note, I think you should look at https://github.com/bminor/bash/tree/master/tests. This would *really* uncover any implementation bugs.

Yep, it's on our list to figure out how to use the core bash tests. (We've got an issue somewhere that briefly mentions this, I think?)

As context, my initial hesitance had been due to licensing concern. Throughout brush's development, I haven't looked at bash sources or taken any code from its code base to avoid issues in that space. But with that said, we should figure out how to at least run the tests without importing them into this repo--perhaps similar to what projects like uutils have done or like what we do with the bash-completion tests. 

I've also separately reached out to POSIX to inquire about getting a temporary license to their official conformance tests. That will likely require a fair bit of work to make sure we really have the right conditionals around all non-POSIX extensions.
Elsie19: > I've also separately reached out to POSIX to inquire about getting a temporary license to their official conformance tests. That will likely require a fair bit of work to make sure we really have the right conditionals around all non-POSIX extensions.

I think that's a wonderful idea!
Elsie19: Ugh I still forgot tests, hold on...
Elsie19: Concurrently with doing tests, I do have something I'd like fixed if possible:

```rs
let orig_term_attr = setup_terminal_settings(&input_file)?;

let mut entries = vec![];
let mut buf_reader = BufReader::new(input_file.clone());
```

Can you think of any ways to remove the clone in `BufReader`? I know that `OpenFile` is small, but it never hurts to remove any copying that isn't strictly necessary.
reubeno: Thanks for adding some basic tests. I skimmed through the change quickly and had a few broader questions... (I can look more closely within the next ~day or so.)

Is there a reason this change shifts to using `BufReader`? Note that the original implementation wasn't ideal, but it was intentionally reading a single character at a time (without any explicit buffering) to ensure that we don't read too many characters from the underlying file/stream--i.e., that we stop reading as soon as we read the final character. With `BufReader` there's a risk that it will read a larger chunk from the underlying input file, even if we've only read a strict subset of the characters from it.

Also, do we need to bring back some of the special Ctrl-C/Ctrl-D handling? It's hard to test those without a more interactive/pty-style test, but can you at least confirm that they still work as expected?
Elsie19: > Thanks for adding some basic tests. I skimmed through the change quickly and had a few broader questions... (I can look more closely within  Is there a reason this change shifts to using `BufReader`? Note that the original implementation wasn't ideal, but it was intentionally reading a single character at a time (without any explicit buffering) to ensure that we don't read too many characters from the underlying file/stream--i.e., that we stop reading as soon as we read the final character. With `BufReader` there's a risk that it will read a larger chunk from the underlying input file, even if we've only read a strict subset of the characters from it.

> It can be excessively inefficient to work directly with a Read instance. For example, every call to read on TcpStream results in a system call. A BufReader<R> performs large, infrequent reads on the underlying Read and maintains an in-memory buffer of the results.

https://doc.rust-lang.org/std/io/struct.BufReader.html

> Also, do we need to bring back some of the special Ctrl-C/Ctrl-D handling? It's hard to test those without a more interactive/pty-style test, but can you at least confirm that they still work as expected?

Oh yes I should add that back! Thanks for reminding me.

reubeno: I'm familiar with the benefits of buffered reads :smile:. I'd absolutely support using it if we could sort out the semantics -- but what I'm saying is that there's a functional requirement for builtins like `read`, `readarray`/`mapfile`, etc. to read no more bytes from the underlying OS file stream than they are supposed to consume. Otherwise, another command running in the pipeline/context afterwards may experience data loss (because the `BufReader` would have already read in those bytes in its buffer, but discarded them when it was dropped).

In case it helps, here's a contrived example of a scenario I'm concerned with:

* User runs something like: `seq 1 3 | (mapfile -n 1; lolcat; echo $MAPFILE)`
* `brush` will establish a single pipe that `seq` writes into, and from which the `mapfile`/`lolcat` subshell will read from.
* `seq` will write `1\n2\n3\n` into the writer end of the pipe.
* `mapfile` runs, with its `stdin` mapped to the reader end of the pipe, and:
  * creates a `BufReader` atop `stdin` (the pipe reader)
  * reads from the `BufReader`
  * under the hood, the implementation of `BufReader` reads _some number of bytes it decides_ from the pipe and fills its buffer; it then returns to its caller the number of bytes requested. Those bytes it read are now only in its buffer and no longer present in the pipe for future reads.
  * sees `1` and then `\n` and realizes it's read enough; it sets `MAPFILE` to `"1"`
  * (implicitly) drops the `BufReader`, which results in the `BufReader`'s internal buffer getting dropped on the floor, along with the bytes that are still in it
* `lolcat` runs, with its `stdin` mapped to the same reader end of the pipe
  * it reads from its `stdin` and *doesn't* see `2`, because `2` had already been read into the `BufReader` buffer above and that byte is no longer present in the pipe

Expected output is something like:

![image](https://github.com/user-attachments/assets/69aa99e7-a742-4cbb-9d37-37713eb7f508)

With reads buffered in this way, it's entirely possible the `BufReader` could read *all* the bytes from the pipe, and `lolcat` would see nothing -- losing the remaining bytes.
Elsie19: Oh I wasn't aware of that, but good catch. I'll rework it later. The reason I wanted to use `BufReader` was because it could do `.read_until`, which I thought would be easier to work with.
Elsie19: Also I cannot find the flag `-r` in `lolcat`. Is that a custom command?
reubeno: Thanks for the update; I got a bit busy yesterday but should be able to get to this during the weekend.

I'd like to put together a new release sometime next week and include this (and 2 other pending changes) in it, assuming it's ready.
Elsie19: It's ready!
Elsie19: I fixed all your comments, but I removed `-C`/`-c` support (for now, that can be a later PR).
Elsie19: > Changes look good, thanks for incorporating the feedback. 2 final comments -- these two commented out lines should be removable at this point, right?

Yes, they can be removed.

Patch:
diff --git a/brush-core/src/builtins/mapfile.rs b/brush-core/src/builtins/mapfile.rs
index 8a274d9..87769fc 100644
--- a/brush-core/src/builtins/mapfile.rs
+++ b/brush-core/src/builtins/mapfile.rs
@@ -12,15 +12,15 @@ pub(crate) struct MapFileCommand {
     delimiter: String,
 
     /// Maximum number of entries to read (0 means no limit).
-    #[arg(short = 'n', default_value = "0")]
+    #[arg(short = 'n', default_value_t = 0)]
     max_count: i64,
 
     /// Index into array at which to start assignment.
-    #[arg(short = 'O')]
-    origin: Option<i64>,
+    #[arg(short = 'O', default_value_t = 0)]
+    origin: i64,
 
     /// Number of initial entries to skip.
-    #[arg(short = 's', default_value = "0")]
+    #[arg(short = 's', default_value_t = 0, value_parser = clap::value_parser!(i64).range(0..))]
     skip_count: i64,
 
     /// Whether or not to remove the delimiter from each read line.
@@ -28,7 +28,7 @@ pub(crate) struct MapFileCommand {
     remove_delimiter: bool,
 
     /// File descriptor to read from (defaults to stdin).
-    #[arg(short = 'u', default_value = "0")]
+    #[arg(short = 'u', default_value_t = 0)]
     fd: u32,
 
     /// Name of function to call for each group of lines.
@@ -36,10 +36,11 @@ pub(crate) struct MapFileCommand {
     callback: Option<String>,
 
     /// Number of lines to pass the callback for each group.
-    #[arg(short = 'c', default_value = "5000")]
+    #[arg(short = 'c', default_value_t = 5000, value_parser = clap::value_parser!(i64).range(1..))]
     callback_group_size: i64,
 
     /// Name of array to read into.
+    #[arg(default_value = "MAPFILE")]
     array_var_name: String,
 }
 
@@ -48,27 +49,13 @@ impl builtins::Command for MapFileCommand {
         &self,
         context: commands::ExecutionContext<'_>,
     ) -> Result<crate::builtins::ExitCode, error::Error> {
-        if self.delimiter != "\n" {
-            // This will require reading a single char at a time and stoping as soon as
-            // the delimiter is hit.
-            return error::unimp("mapfile with non-newline delimiter not yet implemented");
-        }
-
-        if self.max_count != 0 {
-            return error::unimp("mapfile -n is not yet implemented");
-        }
-
-        if self.origin.is_some() {
+        if self.origin != 0 {
             // This will require merging into a potentially already-existing array.
             return error::unimp("mapfile -O is not yet implemented");
         }
 
-        if self.skip_count != 0 {
-            return error::unimp("mapfile -s is not yet implemented");
-        }
-
-        if self.callback.is_some() {
-            return error::unimp("mapfile -C is not yet implemented");
+        if self.callback_group_size != 5000 || self.callback.is_some() {
+            return error::unimp("mapfile -C/-c is not yet implemented");
         }
 
         let input_file = context
@@ -97,42 +84,53 @@ impl MapFileCommand {
         &self,
         mut input_file: openfiles::OpenFile,
     ) -> Result<variables::ArrayLiteral, error::Error> {
-        let mut entries = vec![];
-
         let orig_term_attr = setup_terminal_settings(&input_file)?;
 
-        let mut current_entry = String::new();
-        let mut buffer: [u8; 1] = [0; 1]; // 1-byte buffer
+        let mut entries = vec![];
+        let mut read_count = 0;
+        let max_count = self.max_count.try_into()?;
+        let delimiter = self.delimiter.chars().next().unwrap_or('\n') as u8;
+
+        let mut buf = [0u8; 1];
+
+        while max_count == 0 || entries.len() < max_count {
+            let mut line = vec![];
+            let mut saw_delimiter = false;
+
+            loop {
+                match input_file.read(&mut buf) {
+                    Ok(0) => break,                                         // End of input
+                    Ok(1) if buf[0] == b'\x03' => break,                    // Ctrl+C
+                    Ok(1) if buf[0] == b'\x04' && line.is_empty() => break, // Ctrl+D
+                    Ok(1) => {
+                        let byte = buf[0];
+                        line.push(byte);
+                        if byte == delimiter {
+                            saw_delimiter = true;
+                            break;
+                        }
+                    }
+                    Ok(_) => unreachable!("input can only be 0, 1, or error"),
+                    Err(e) => return Err(e.into()),
+                }
+            }
 
-        loop {
-            // TODO: Figure out how to restore terminal settings on error?
-            let n = input_file.read(&mut buffer)?;
-            if n == 0 {
-                // EOF reached.
+            if line.is_empty() && !saw_delimiter {
                 break;
             }
 
-            let ch = buffer[0] as char;
+            if read_count < self.skip_count {
+                read_count += 1;
+                continue;
+            }
 
-            // Check for Ctrl+C.
-            if ch == '\x03' {
-                break;
-            // Ctrl+D is EOF *if* there's no entry in progress.
-            } else if ch == '\x04' && current_entry.is_empty() {
-                break;
+            if self.remove_delimiter && line.ends_with(&[delimiter]) {
+                line.pop();
             }
 
-            // Check for a delimiting newline char.
-            // TODO: Support other delimiters.
-            if ch == '\n' {
-                if !self.remove_delimiter {
-                    current_entry.push(ch);
-                }
+            let line_str = String::from_utf8_lossy(&line).to_string();
 
-                entries.push((None, std::mem::take(&mut current_entry)));
-            } else {
-                current_entry.push(ch);
-            }
+            entries.push((None, line_str));
         }
 
         if let Some(orig_term_attr) = &orig_term_attr {


Test patch:
diff --git a/brush-shell/tests/cases/builtins/mapfile.yaml b/brush-shell/tests/cases/builtins/mapfile.yaml
index 60e9b18..a129636 100644
--- a/brush-shell/tests/cases/builtins/mapfile.yaml
+++ b/brush-shell/tests/cases/builtins/mapfile.yaml
@@ -8,3 +8,19 @@ cases:
     stdin: |
       readarray -t myarray < /dev/null
       (echo "hello"; echo "there") | (readarray -t myarray && declare -p myarray)
+  - name: "mapfile -d"
+    stdin: |
+      mapfile -t -d 'Z' myarray < <(echo -ne "helloZthereZgeneralZkenobi")
+      [[ "${myarray[*]}" == "hello there general kenobi" ]]
+  - name: "mapfile no array name"
+    stdin: |
+      mapfile -t < <(echo -ne "woo\nah!")
+      [[ "${MAPFILE[*]}" == "woo ah!" ]]
+  - name: "mapfile -n"
+    stdin: |
+      mapfile -t -n 2 -d 'Z' myarray < <(echo -ne "helloZthereZgeneralZkenobi")
+      [[ "${myarray[*]}" == "hello there" ]]
+  - name: "mapfile -s"
+    stdin: |
+      mapfile -t -n 2 -s 1 -d 'Z' myarray < <(echo -ne "helloZthereZgeneralZkenobi")
+      [[ "${myarray[*]}" == "there general" ]]