From 760be4faec0350567ddb60cb9c8749099913e592 Mon Sep 17 00:00:00 2001 From: Vincent De Smet Date: Wed, 30 Sep 2026 18:00:45 +0800 Subject: [PATCH 1/3] fix(grep): skip binary and non-UTF-8 input instead of aborting grep read lines into a String, so the first non-UTF-8 file failed read_line with InvalidData, and the `?` in cmd_grep aborted the file loop: the command exited 1 and every later file was never scanned. Read lines as bytes and treat binary input like GNU grep: a NUL in the first buffer marks the file binary, and invalid-UTF-8 lines are matched lossily but never printed. Either way a match reports "grep: FILE: binary file matches" on stderr. Add -a/--text, -I and --binary-files=TYPE. Per-file read errors are reported and grep moves on; BrokenPipe still stops it. Fixes #123 Co-Authored-By: Claude Opus 5.5 (1M context) Claude-Session: https://claude.ai/code/session_01AXaT7zwomYh4CtqSGrCY1Y --- src/commands/grep.rs | 140 ++++++++++++++++++++-- tests/shell_integration.rs | 230 +++++++++++++++++++++++++++++++++++++ 2 files changed, 359 insertions(+), 11 deletions(-) diff --git a/src/commands/grep.rs b/src/commands/grep.rs index ba3b382..8f12202 100644 --- a/src/commands/grep.rs +++ b/src/commands/grep.rs @@ -1,3 +1,5 @@ +use std::borrow::Cow; + use crate::os; use crate::prelude::*; @@ -21,6 +23,9 @@ Options: -H, --with-filename print filename with matches -h, --no-filename suppress filename prefix -o, --only-matching show only the matching part + -a, --text process a binary file as if it were text + -I ignore binary files (treat as non-matching) + --binary-files=TYPE binary, text, or without-match -m, --max-count NUM stop after NUM matches per file -A, --after-context NUM print NUM lines after match -B, --before-context NUM print NUM lines before match @@ -29,6 +34,17 @@ Options: --exclude=GLOB skip files matching GLOB --exclude-dir=DIR skip directories matching DIR"; +/// How to treat input that looks binary (NUL byte or invalid UTF-8). +#[derive(Clone, Copy, PartialEq, Eq)] +enum BinaryFiles { + /// Report "binary file matches" instead of printing the lines. + Binary, + /// Print every line, decoding invalid bytes lossily. + Text, + /// Treat files containing NUL as if they had no matches. + WithoutMatch, +} + struct Opts { patterns: Vec, files: Vec, @@ -46,6 +62,7 @@ struct Opts { with_filename: Option, only_matching: bool, max_count: Option, + binary_files: BinaryFiles, after_context: usize, before_context: usize, include: Vec, @@ -71,6 +88,7 @@ fn parse_args(args: &[String]) -> Result, Box Result, Box opts.with_filename = Some(false), Short('o') | Long("only-matching") => opts.only_matching = true, Short('m') | Long("max-count") => opts.max_count = Some(parser.value()?.parse()?), + Short('a') | Long("text") => opts.binary_files = BinaryFiles::Text, + Short('I') => opts.binary_files = BinaryFiles::WithoutMatch, + Long("binary-files") => { + opts.binary_files = match parser.value()?.string()?.as_str() { + "binary" => BinaryFiles::Binary, + "text" => BinaryFiles::Text, + "without-match" => BinaryFiles::WithoutMatch, + other => { + return Err( + format!("invalid argument '{other}' for '--binary-files'").into() + ); + } + }; + } Short('A') | Long("after-context") => opts.after_context = parser.value()?.parse()?, Short('B') | Long("before-context") => opts.before_context = parser.value()?.parse()?, Short('C') | Long("context") => { @@ -216,13 +248,21 @@ async fn grep_reader( re: ®ex::Regex, opts: &Opts, prefix: &str, + name: &str, w: &mut os::FdWriter, + ew: &mut os::FdWriter, ) -> Result> { let mut buf_reader = BufReader::new(reader); - let mut line = String::new(); + let mut line: Vec = Vec::new(); let mut lineno: u64 = 0; let mut match_count: u64 = 0; let mut found = false; + let detect_binary = opts.binary_files != BinaryFiles::Text; + // A NUL in the first buffer marks the whole input binary; a later NUL hides only its line. + let file_binary = detect_binary && buf_reader.fill_buf().await?.contains(&0); + // A matched or context line was hidden as binary, so report it on stderr. + let mut binary_matched = false; + let summary_only = opts.quiet || opts.list || opts.list_non_matching || opts.count; let use_context = opts.before_context > 0 || opts.after_context > 0; // Ring buffer for before-context @@ -232,21 +272,42 @@ async fn grep_reader( let mut after_remaining: usize = 0; // Whether we need a "--" separator before the next context group let mut need_sep = false; + // Whether any match or context line has been written yet. + let mut printed_any = false; + + // A binary file is treated as non-matching under -I. + let skip_file = file_binary && opts.binary_files == BinaryFiles::WithoutMatch; + // Line number of the last binary line hidden from context (0 = none). + let mut last_binary_lineno: u64 = 0; loop { + if skip_file { + break; + } line.clear(); - if buf_reader.read_line(&mut line).await? == 0 { + if buf_reader.read_until(b'\n', &mut line).await? == 0 { break; } lineno += 1; - let text = line.trim_end_matches('\n').trim_end_matches('\r'); + let mut raw = line.as_slice(); + raw = raw.strip_suffix(b"\n").unwrap_or(raw); + raw = raw.strip_suffix(b"\r").unwrap_or(raw); + // Invalid UTF-8 is matched lossily but never printed unless -a. + let (text, is_binary) = match std::str::from_utf8(raw) { + Ok(s) => ( + Cow::Borrowed(s), + file_binary || (detect_binary && s.contains('\0')), + ), + Err(_) => (String::from_utf8_lossy(raw), detect_binary), + }; + let text = text.as_ref(); let matched = re.is_match(text) ^ opts.invert; if matched { found = true; match_count += 1; - if opts.quiet || opts.list || opts.list_non_matching || opts.count { + if summary_only { if let Some(max) = opts.max_count && match_count >= max { @@ -255,12 +316,34 @@ async fn grep_reader( continue; } + if is_binary { + binary_matched = true; + if use_context { + // The hidden match ends any open context and breaks the group. + after_remaining = 0; + before_buf.clear(); + need_sep = printed_any; + last_binary_lineno = lineno; + } + if file_binary || opts.max_count.is_some_and(|max| match_count >= max) { + break; + } + continue; + } + if use_context { + // A hidden binary line still inside the before window. + let binary_in_window = last_binary_lineno > 0 + && lineno - last_binary_lineno <= opts.before_context as u64; // Print separator between context groups - if (opts.before_context == 0 || !before_buf.is_empty()) && need_sep { + if (opts.before_context == 0 || !before_buf.is_empty() || binary_in_window) + && need_sep + { wprintln!(w, "--")?; } need_sep = false; + binary_matched |= binary_in_window; + last_binary_lineno = 0; // Flush before-context buffer for (bno, btext) in before_buf.drain(..) { if !prefix.is_empty() { @@ -274,6 +357,7 @@ async fn grep_reader( after_remaining = opts.after_context; } + printed_any = true; if opts.only_matching && !opts.invert { for m in re.find_iter(text) { if !prefix.is_empty() { @@ -299,9 +383,25 @@ async fn grep_reader( { break; } + } else if is_binary { + // Binary lines are never shown, but they still occupy a context slot. + if use_context { + if found && after_remaining > 0 { + after_remaining -= 1; + // -o prints no context, so the hidden line is never in play. + binary_matched |= !opts.only_matching; + if after_remaining == 0 { + need_sep = true; + } + } else { + before_buf.clear(); + last_binary_lineno = lineno; + } + } } else if use_context && found && after_remaining > 0 { // Print after-context line after_remaining -= 1; + printed_any = true; if !prefix.is_empty() { wprint!(w, "{}-", prefix)?; } @@ -336,6 +436,9 @@ async fn grep_reader( if opts.list_non_matching && !found { wprintln!(w, "{}", prefix)?; } + if binary_matched && !summary_only && opts.binary_files == BinaryFiles::Binary { + wprintln!(ew, "grep: {}: binary file matches", name)?; + } Ok(found) } @@ -377,12 +480,14 @@ async fn cmd_grep(os: &dyn Kernel, args: &[String]) -> CommandResult { let show_name = opts.with_filename.unwrap_or(multi); let mut w = io::stdout()?; + // stderr can only be taken once per process, so share one writer across files. + let mut ew = io::stderr()?; let mut any_match = false; if files.is_empty() { // Read from stdin let reader = io::stdin()?; - if grep_reader(reader, &re, &opts, "", &mut w).await? { + if grep_reader(reader, &re, &opts, "", "(standard input)", &mut w, &mut ew).await? { any_match = true; } } else { @@ -391,7 +496,6 @@ async fn cmd_grep(os: &dyn Kernel, args: &[String]) -> CommandResult { Ok(fd) => fd, Err(e) => { if !opts.quiet { - let mut ew = io::stderr()?; wprintln!(ew, "grep: {}: {}", path, e)?; } continue; @@ -399,10 +503,24 @@ async fn cmd_grep(os: &dyn Kernel, args: &[String]) -> CommandResult { }; let reader = io::take_reader(fd)?; let prefix = if show_name { path.as_str() } else { "" }; - if grep_reader(reader, &re, &opts, prefix, &mut w).await? { - any_match = true; - if opts.quiet { - return Ok(0); + match grep_reader(reader, &re, &opts, prefix, path, &mut w, &mut ew).await { + Ok(true) => { + any_match = true; + if opts.quiet { + return Ok(0); + } + } + Ok(false) => {} + Err(e) => { + // A closed stdout must stop grep, not repeat per file. + if e.downcast_ref::() + .is_some_and(|io| io.kind() == std::io::ErrorKind::BrokenPipe) + { + return Err(e); + } + if !opts.quiet { + wprintln!(ew, "grep: {}: {}", path, e)?; + } } } } diff --git a/tests/shell_integration.rs b/tests/shell_integration.rs index a7111c7..108f95a 100644 --- a/tests/shell_integration.rs +++ b/tests/shell_integration.rs @@ -1992,6 +1992,236 @@ expect!( "/tmp/gi.txt:yes" ); +// ── grep binary / non-UTF-8 input ─────────────────────────────────── + +/// Runs `cmd` in a shell with a host dir of raw-byte fixtures bound at /g. +fn grep_binary_run(tag: &str, cmd: &str) -> strands_shell::Output { + let (rt, local) = rt(); + rt.block_on(local.run_until(async { + let dir = std::env::temp_dir().join(format!("lsh_grep_bin_{tag}")); + let _ = std::fs::remove_dir_all(&dir); + std::fs::create_dir_all(&dir).unwrap(); + std::fs::write(dir.join("a.txt"), b"needle\n").unwrap(); + std::fs::write(dir.join("b.txt"), b"needle b\n").unwrap(); + std::fs::write(dir.join("bin.dat"), b"\0\xffneedle\n").unwrap(); + std::fs::write(dir.join("c.txt"), b"needle c\n").unwrap(); + std::fs::write( + dir.join("mid.txt"), + b"x\nneedle ok\n\xffneedle bad\nneedle after\n", + ) + .unwrap(); + std::fs::write( + dir.join("ctx.txt"), + b"before\n\xffbad ctx\nneedle hit\n\xffafter bad\nplain after\n", + ) + .unwrap(); + std::fs::write( + dir.join("ctxc.txt"), + b"needle a\n\xffneedle b\nafter1\nneedle c\nafter2\nafter3\n", + ) + .unwrap(); + std::fs::write(dir.join("far.txt"), b"\xffbad\nl2\nl3\nneedle\n").unwrap(); + std::fs::write( + dir.join("sep.txt"), + b"needle A\nl2\nl3\n\xffbin\nneedle B\n", + ) + .unwrap(); + for n in 1..=3 { + let big: String = (0..5000).map(|i| format!("needle {i}\n")).collect(); + std::fs::write(dir.join(format!("big{n}.txt")), big).unwrap(); + } + std::fs::write(dir.join("bin2.dat"), b"\xffneedle\nneedle\n").unwrap(); + std::fs::write(dir.join("nomatch.dat"), b"\0\xffzzz\n").unwrap(); + let mut shell = Shell::builder() + .bind_direct(dir.to_str().unwrap(), "/g") + .build() + .unwrap(); + let out = shell.run(cmd).await; + let _ = std::fs::remove_dir_all(&dir); + out + })) +} + +#[test] +fn grep_binary_file_note() { + let out = grep_binary_run("note", "grep needle /g/bin.dat"); + assert_eq!(out.stdout, ""); + assert_eq!(out.stderr, "grep: /g/bin.dat: binary file matches\n"); + assert_eq!(out.status, 0); +} + +#[test] +fn grep_recursive_continues_past_binary() { + let out = grep_binary_run("rec", "cd /g; grep -rn needle ."); + assert!(out.stdout.contains("a.txt:1:needle"), "{:?}", out.stdout); + assert!(out.stdout.contains("b.txt:1:needle b"), "{:?}", out.stdout); + assert!(out.stdout.contains("c.txt:1:needle c"), "{:?}", out.stdout); + assert!( + out.stdout.contains("mid.txt:2:needle ok"), + "{:?}", + out.stdout + ); + assert!(!out.stdout.contains("bin.dat"), "{:?}", out.stdout); + assert!( + out.stderr.contains("grep: bin.dat: binary file matches"), + "{:?}", + out.stderr + ); + assert!( + out.stderr.contains("grep: mid.txt: binary file matches"), + "{:?}", + out.stderr + ); + assert_eq!(out.status, 0); +} + +#[test] +fn grep_invalid_utf8_line_suppressed() { + let out = grep_binary_run("mid", "grep needle /g/mid.txt"); + assert_eq!(out.stdout, "needle ok\nneedle after\n"); + assert_eq!(out.stderr, "grep: /g/mid.txt: binary file matches\n"); + assert_eq!(out.status, 0); +} + +#[test] +fn grep_only_matching_skips_binary_line() { + let out = grep_binary_run("o", "grep -o needle /g/mid.txt"); + assert_eq!(out.stdout, "needle\nneedle\n"); +} + +#[test] +fn grep_i_skips_binary_file() { + let out = grep_binary_run("bi", "grep -I needle /g/bin.dat"); + assert_eq!(out.stdout, ""); + assert_eq!(out.stderr, ""); + assert_eq!(out.status, 1); +} + +#[test] +fn grep_text_flag_prints_binary_lines() { + let out = grep_binary_run("ba", "grep -a needle /g/mid.txt"); + assert!(out.stdout.contains("needle ok"), "{:?}", out.stdout); + assert!( + out.stdout.contains("\u{FFFD}needle bad"), + "{:?}", + out.stdout + ); + assert_eq!(out.stderr, ""); + assert_eq!(out.status, 0); +} + +#[test] +fn grep_binary_files_option() { + let out = grep_binary_run("bf1", "grep --binary-files=without-match needle /g/bin.dat"); + assert_eq!(out.status, 1); + let out = grep_binary_run("bf2", "grep --binary-files=text needle /g/mid.txt"); + assert!(out.stdout.contains("needle bad"), "{:?}", out.stdout); + let out = grep_binary_run("bf3", "grep --binary-files=bogus needle /g/a.txt"); + assert_ne!(out.status, 0); + assert!( + out.stderr.contains("grep: invalid argument 'bogus'"), + "{:?}", + out.stderr + ); + assert!(!out.stderr.contains("grep: grep:"), "{:?}", out.stderr); +} + +#[test] +fn grep_binary_line_occupies_context_slot() { + let out = grep_binary_run("ctxa", "grep -n -A1 needle /g/ctx.txt"); + assert_eq!(out.stdout, "3:needle hit\n"); + assert!( + out.stderr.contains("binary file matches"), + "{:?}", + out.stderr + ); + let out = grep_binary_run("ctxb", "grep -n -B1 needle /g/ctx.txt"); + assert_eq!(out.stdout, "3:needle hit\n"); + assert!( + out.stderr.contains("binary file matches"), + "{:?}", + out.stderr + ); +} + +#[test] +fn grep_matched_binary_line_resets_context() { + let out = grep_binary_run("ctxc1", "grep -n -A1 needle /g/ctxc.txt"); + assert_eq!(out.stdout, "1:needle a\n--\n4:needle c\n5-after2\n"); + let out = grep_binary_run("ctxc2", "grep -n -A2 needle /g/ctxc.txt"); + assert_eq!( + out.stdout, + "1:needle a\n--\n4:needle c\n5-after2\n6-after3\n" + ); +} + +#[test] +fn grep_distant_binary_line_gives_no_note() { + let out = grep_binary_run("far", "grep -n -B1 needle /g/far.txt"); + assert_eq!(out.stdout, "3-l3\n4:needle\n"); + assert_eq!(out.stderr, ""); +} + +#[test] +fn grep_dropped_binary_line_keeps_group_separator() { + let out = grep_binary_run("sepb", "grep -n -B1 needle /g/sep.txt"); + assert_eq!(out.stdout, "1:needle A\n--\n5:needle B\n"); + let out = grep_binary_run("sepc", "grep -n -C1 needle /g/sep.txt"); + assert_eq!(out.stdout, "1:needle A\n2-l2\n--\n5:needle B\n"); +} + +#[test] +fn grep_without_match_is_silent_for_invalid_utf8() { + let out = grep_binary_run("iq", "grep -I needle /g/mid.txt"); + assert_eq!(out.stdout, "needle ok\nneedle after\n"); + assert_eq!(out.stderr, ""); +} + +#[test] +fn grep_count_and_list_on_binary() { + let out = grep_binary_run("cl", "grep -c needle /g/bin.dat /g/mid.txt"); + assert_eq!(out.stdout, "/g/bin.dat:1\n/g/mid.txt:3\n"); + assert_eq!(out.stderr, ""); + let out = grep_binary_run("cl2", "grep -l needle /g/bin.dat /g/mid.txt"); + assert_eq!(out.stdout, "/g/bin.dat\n/g/mid.txt\n"); + assert_eq!(out.stderr, ""); + let out = grep_binary_run("cl3", "grep -q needle /g/bin.dat"); + assert_eq!(out.status, 0); + assert_eq!(out.stderr, ""); +} + +#[test] +fn grep_stdin_binary_note() { + let out = grep_binary_run("stdin", "cat /g/bin.dat | grep needle"); + assert_eq!(out.stdout, ""); + assert_eq!(out.stderr, "grep: (standard input): binary file matches\n"); + assert_eq!(out.status, 0); +} + +#[test] +fn grep_broken_pipe_stops_after_first_file() { + let out = grep_binary_run( + "bp", + "grep needle /g/big1.txt /g/big2.txt /g/big3.txt | head -n 1", + ); + assert_eq!(out.stdout, "/g/big1.txt:needle 0\n"); + assert!(!out.stderr.contains("/g/big"), "stderr: {}", out.stderr); +} + +#[test] +fn grep_no_leading_separator_before_hidden_binary_match() { + let out = grep_binary_run("sep0", "grep -A1 needle /g/bin2.dat"); + assert_eq!(out.stdout, "needle\n"); +} + +#[test] +fn grep_binary_no_match_quiet_exit_1() { + let out = grep_binary_run("nm", "grep needle /g/nomatch.dat"); + assert_eq!(out.stdout, ""); + assert_eq!(out.stderr, ""); + assert_eq!(out.status, 1); +} + // ── jq coverage ───────────────────────────────────────────────────── expect!(jq_identity, "echo '{\"a\":1}' | jq '.'", "{\n \"a\": 1\n}"); From f283019b6da8d7942ef3fb5bff8e1190c53a6054 Mon Sep 17 00:00:00 2001 From: Vincent De Smet Date: Thu, 1 Oct 2026 11:02:02 +0800 Subject: [PATCH 2/3] fix(grep): match binary input on raw bytes, closer to GNU grep Traced against the GNU grep 3.11 source (src/grep.c): - Match on raw bytes (regex::bytes), so `.` never matches an invalid byte: `a\xffb` no longer matches `a.b`. - With -o, hide only matches whose own bytes are invalid UTF-8; valid matches on such a line still print (print_line_middle). - Detect NULs in the first 96 KiB, as GNU does with INITIAL_BUFSIZE, and treat a later NUL as making the rest of the file binary. With -I the file then counts as non-matching. - -q/-l/-L stop at the first match. -a still decodes invalid bytes lossily, because captured shell output must be valid UTF-8. Co-Authored-By: Claude Opus 5.5 (1M context) Claude-Session: https://claude.ai/code/session_01AXaT7zwomYh4CtqSGrCY1Y --- src/commands/grep.rs | 71 ++++++++++++++++++++++---------------- tests/shell_integration.rs | 59 +++++++++++++++++++++++++++---- 2 files changed, 93 insertions(+), 37 deletions(-) diff --git a/src/commands/grep.rs b/src/commands/grep.rs index 8f12202..a9d1f79 100644 --- a/src/commands/grep.rs +++ b/src/commands/grep.rs @@ -1,5 +1,3 @@ -use std::borrow::Cow; - use crate::os; use crate::prelude::*; @@ -39,7 +37,7 @@ Options: enum BinaryFiles { /// Report "binary file matches" instead of printing the lines. Binary, - /// Print every line, decoding invalid bytes lossily. + /// Print every line, decoding invalid bytes lossily (captured output must be UTF-8). Text, /// Treat files containing NUL as if they had no matches. WithoutMatch, @@ -157,7 +155,9 @@ fn parse_args(args: &[String]) -> Result, Box Result> { +fn build_regex( + opts: &Opts, +) -> Result> { let combined = if opts.fixed { opts.patterns .iter() @@ -174,7 +174,7 @@ fn build_regex(opts: &Opts) -> Result( reader: R, - re: ®ex::Regex, + re: ®ex::bytes::Regex, opts: &Opts, prefix: &str, name: &str, w: &mut os::FdWriter, ew: &mut os::FdWriter, ) -> Result> { - let mut buf_reader = BufReader::new(reader); + let mut buf_reader = BufReader::with_capacity(96 * 1024, reader); let mut line: Vec = Vec::new(); let mut lineno: u64 = 0; let mut match_count: u64 = 0; let mut found = false; let detect_binary = opts.binary_files != BinaryFiles::Text; - // A NUL in the first buffer marks the whole input binary; a later NUL hides only its line. - let file_binary = detect_binary && buf_reader.fill_buf().await?.contains(&0); + // A NUL in the first 96 KiB (GNU's first buffer) marks the whole input binary; a later NUL + // does so from its line on (GNU decides per buffer there). + let mut file_binary = detect_binary && buf_reader.fill_buf().await?.contains(&0); // A matched or context line was hidden as binary, so report it on stderr. let mut binary_matched = false; let summary_only = opts.quiet || opts.list || opts.list_non_matching || opts.count; let use_context = opts.before_context > 0 || opts.after_context > 0; // Ring buffer for before-context - let mut before_buf: std::collections::VecDeque<(u64, String)> = + let mut before_buf: std::collections::VecDeque<(u64, Vec)> = std::collections::VecDeque::new(); // How many more after-context lines to print let mut after_remaining: usize = 0; @@ -292,31 +293,36 @@ async fn grep_reader( let mut raw = line.as_slice(); raw = raw.strip_suffix(b"\n").unwrap_or(raw); raw = raw.strip_suffix(b"\r").unwrap_or(raw); - // Invalid UTF-8 is matched lossily but never printed unless -a. - let (text, is_binary) = match std::str::from_utf8(raw) { - Ok(s) => ( - Cow::Borrowed(s), - file_binary || (detect_binary && s.contains('\0')), - ), - Err(_) => (String::from_utf8_lossy(raw), detect_binary), - }; - let text = text.as_ref(); - let matched = re.is_match(text) ^ opts.invert; + if detect_binary && !file_binary && raw.contains(&0) { + file_binary = true; + if opts.binary_files == BinaryFiles::WithoutMatch { + // GNU counts the whole file as non-matching; printed lines stay printed. + match_count = 0; + found = false; + break; + } + } + // Invalid UTF-8 is matched on raw bytes but never printed unless -a. + let invalid = detect_binary && std::str::from_utf8(raw).is_err(); + let is_binary = file_binary || invalid; + // -o hides only matches that are themselves invalid UTF-8. + let per_match = opts.only_matching && !opts.invert && !file_binary; + let matched = re.is_match(raw) ^ opts.invert; if matched { found = true; match_count += 1; if summary_only { - if let Some(max) = opts.max_count - && match_count >= max - { + // -q/-l/-L stop at the first match (GNU done_on_match). + let first_match_done = opts.quiet || opts.list || opts.list_non_matching; + if first_match_done || opts.max_count.is_some_and(|max| match_count >= max) { break; } continue; } - if is_binary { + if is_binary && !per_match { binary_matched = true; if use_context { // The hidden match ends any open context and breaks the group. @@ -352,21 +358,26 @@ async fn grep_reader( if opts.line_number { wprint!(w, "{}-", bno)?; } - wprintln!(w, "{}", btext)?; + wprintln!(w, "{}", String::from_utf8_lossy(&btext))?; } after_remaining = opts.after_context; } printed_any = true; if opts.only_matching && !opts.invert { - for m in re.find_iter(text) { + for m in re.find_iter(raw) { + // GNU drops the rest of the line once a match is suppressed. + if invalid && std::str::from_utf8(m.as_bytes()).is_err() { + binary_matched = true; + break; + } if !prefix.is_empty() { wprint!(w, "{}:", prefix)?; } if opts.line_number { wprint!(w, "{}:", lineno)?; } - wprintln!(w, "{}", m.as_str())?; + wprintln!(w, "{}", String::from_utf8_lossy(m.as_bytes()))?; } } else { if !prefix.is_empty() { @@ -375,7 +386,7 @@ async fn grep_reader( if opts.line_number { wprint!(w, "{}:", lineno)?; } - wprintln!(w, "{}", text)?; + wprintln!(w, "{}", String::from_utf8_lossy(raw))?; } if let Some(max) = opts.max_count @@ -408,7 +419,7 @@ async fn grep_reader( if opts.line_number { wprint!(w, "{}-", lineno)?; } - wprintln!(w, "{}", text)?; + wprintln!(w, "{}", String::from_utf8_lossy(raw))?; if after_remaining == 0 { need_sep = true; } @@ -417,7 +428,7 @@ async fn grep_reader( if after_remaining == 0 && found && !need_sep { need_sep = true; } - before_buf.push_back((lineno, text.to_string())); + before_buf.push_back((lineno, raw.to_vec())); while before_buf.len() > opts.before_context { before_buf.pop_front(); } diff --git a/tests/shell_integration.rs b/tests/shell_integration.rs index 108f95a..23d687d 100644 --- a/tests/shell_integration.rs +++ b/tests/shell_integration.rs @@ -2032,6 +2032,14 @@ fn grep_binary_run(tag: &str, cmd: &str) -> strands_shell::Output { } std::fs::write(dir.join("bin2.dat"), b"\xffneedle\nneedle\n").unwrap(); std::fs::write(dir.join("nomatch.dat"), b"\0\xffzzz\n").unwrap(); + std::fs::write(dir.join("dot.txt"), b"a\xffb\naxb\n").unwrap(); + std::fs::write(dir.join("o.txt"), b"\xff foo\n").unwrap(); + let filler: String = (0..20000).map(|i| format!("text {i}\n")).collect(); + std::fs::write( + dir.join("late.txt"), + format!("needle early\n{filler}needle \0 nul\nneedle after\n"), + ) + .unwrap(); let mut shell = Shell::builder() .bind_direct(dir.to_str().unwrap(), "/g") .build() @@ -2086,7 +2094,7 @@ fn grep_invalid_utf8_line_suppressed() { #[test] fn grep_only_matching_skips_binary_line() { let out = grep_binary_run("o", "grep -o needle /g/mid.txt"); - assert_eq!(out.stdout, "needle\nneedle\n"); + assert_eq!(out.stdout, "needle\nneedle\nneedle\n"); } #[test] @@ -2100,14 +2108,51 @@ fn grep_i_skips_binary_file() { #[test] fn grep_text_flag_prints_binary_lines() { let out = grep_binary_run("ba", "grep -a needle /g/mid.txt"); - assert!(out.stdout.contains("needle ok"), "{:?}", out.stdout); - assert!( - out.stdout.contains("\u{FFFD}needle bad"), - "{:?}", - out.stdout - ); + assert_eq!(out.stdout, "needle ok\n\u{FFFD}needle bad\nneedle after\n"); assert_eq!(out.stderr, ""); assert_eq!(out.status, 0); + // Matching still runs on raw bytes: `.` never matches the invalid byte. + let out = grep_binary_run("bal", "grep -a '^. foo' /g/o.txt"); + assert_eq!(out.stdout, ""); + assert_eq!(out.status, 1); +} + +#[test] +fn grep_dot_does_not_match_invalid_byte() { + let out = grep_binary_run("dot", "grep -c 'a.b' /g/dot.txt"); + assert_eq!(out.stdout, "1\n"); +} + +#[test] +fn grep_only_matching_prints_valid_match_on_invalid_line() { + let out = grep_binary_run("ofoo", "grep -o foo /g/o.txt"); + assert_eq!(out.stdout, "foo\n"); + assert_eq!(out.stderr, ""); +} + +#[test] +fn grep_late_nul_hides_rest_of_file() { + let out = grep_binary_run("late", "grep needle /g/late.txt"); + assert_eq!(out.stdout, "needle early\n"); + assert_eq!(out.stderr, "grep: /g/late.txt: binary file matches\n"); + let out = grep_binary_run("latei", "grep -I needle /g/late.txt"); + assert_eq!(out.stdout, "needle early\n"); + assert_eq!(out.stderr, ""); + assert_eq!(out.status, 1); + let out = grep_binary_run("latec", "grep -c needle /g/late.txt"); + assert_eq!(out.stdout, "3\n"); + let out = grep_binary_run("lateci", "grep -cI needle /g/late.txt"); + assert_eq!(out.stdout, "0\n"); + assert_eq!(out.status, 1); + // -q/-l/-L stop at the first match, before the late NUL. + let out = grep_binary_run("lateqi", "grep -qI needle /g/late.txt"); + assert_eq!(out.status, 0); + let out = grep_binary_run("lateli", "grep -lI needle /g/late.txt"); + // The file is listed (single-file -l prints an empty name, a separate pre-existing quirk). + assert!(!out.stdout.is_empty(), "{:?}", out.stdout); + assert_eq!(out.status, 0); + let out = grep_binary_run("lateLi", "grep -LI needle /g/late.txt"); + assert_eq!(out.stdout, ""); } #[test] From b1f677df40adad664b153fa09903caa66de48a34 Mon Sep 17 00:00:00 2001 From: Vincent De Smet Date: Sat, 3 Oct 2026 07:36:09 +0800 Subject: [PATCH 3/3] fix(grep): keep context lines around hidden binary lines, as GNU does Hidden (binary) lines were clearing the before-context buffer, so valid text lines the user asked for with -B/-C were dropped. They now keep their slot in the window without being written. Context output follows GNU grep's prtext/lastout model: - `--` is written only when the next group is not contiguous with the last line actually written. - A hidden match starts no after-context, and a hidden line inside the after-context ends it. - -o writes no context lines (GNU), which also restores the binary note for -o -A. Two context bugs from main are fixed along the way: a spurious `--` between adjacent groups, and -o printing context lines. Co-Authored-By: Claude Opus 5.5 (1M context) Claude-Session: https://claude.ai/code/session_01AXaT7zwomYh4CtqSGrCY1Y --- src/commands/grep.rs | 174 +++++++++++++++++-------------------- tests/shell_integration.rs | 70 ++++++++++++++- 2 files changed, 150 insertions(+), 94 deletions(-) diff --git a/src/commands/grep.rs b/src/commands/grep.rs index a9d1f79..794c936 100644 --- a/src/commands/grep.rs +++ b/src/commands/grep.rs @@ -243,6 +243,23 @@ async fn collect_files_recursive(os: &dyn Kernel, path: &str, opts: &Opts, out: } } +async fn write_context( + w: &mut os::FdWriter, + prefix: &str, + line_number: bool, + lineno: u64, + text: &[u8], +) -> Result<(), Box> { + if !prefix.is_empty() { + wprint!(w, "{}-", prefix)?; + } + if line_number { + wprint!(w, "{}-", lineno)?; + } + wprintln!(w, "{}", String::from_utf8_lossy(text))?; + Ok(()) +} + async fn grep_reader( reader: R, re: ®ex::bytes::Regex, @@ -266,20 +283,17 @@ async fn grep_reader( let summary_only = opts.quiet || opts.list || opts.list_non_matching || opts.count; let use_context = opts.before_context > 0 || opts.after_context > 0; - // Ring buffer for before-context - let mut before_buf: std::collections::VecDeque<(u64, Vec)> = + // Last B lines not yet written (hidden ones included), as (lineno, bytes, hidden). + let mut before_buf: std::collections::VecDeque<(u64, Vec, bool)> = std::collections::VecDeque::new(); - // How many more after-context lines to print - let mut after_remaining: usize = 0; - // Whether we need a "--" separator before the next context group - let mut need_sep = false; - // Whether any match or context line has been written yet. - let mut printed_any = false; + // Line number of the last line written (0 = none); GNU's lastout. + let mut last_printed: u64 = 0; + let mut used = false; + // After-context lines still owed. + let mut pending: usize = 0; // A binary file is treated as non-matching under -I. let skip_file = file_binary && opts.binary_files == BinaryFiles::WithoutMatch; - // Line number of the last binary line hidden from context (0 = none). - let mut last_binary_lineno: u64 = 0; loop { if skip_file { @@ -308,6 +322,8 @@ async fn grep_reader( // -o hides only matches that are themselves invalid UTF-8. let per_match = opts.only_matching && !opts.invert && !file_binary; let matched = re.is_match(raw) ^ opts.invert; + // Hidden lines are never written but otherwise behave like normal lines. + let hidden = is_binary && !per_match; if matched { found = true; @@ -322,71 +338,65 @@ async fn grep_reader( continue; } - if is_binary && !per_match { + if hidden && file_binary { binary_matched = true; - if use_context { - // The hidden match ends any open context and breaks the group. - after_remaining = 0; - before_buf.clear(); - need_sep = printed_any; - last_binary_lineno = lineno; - } - if file_binary || opts.max_count.is_some_and(|max| match_count >= max) { - break; - } - continue; + break; } - if use_context { - // A hidden binary line still inside the before window. - let binary_in_window = last_binary_lineno > 0 - && lineno - last_binary_lineno <= opts.before_context as u64; - // Print separator between context groups - if (opts.before_context == 0 || !before_buf.is_empty() || binary_in_window) - && need_sep - { - wprintln!(w, "--")?; + // Region start; a gap from the last written line starts a new group. + let rs = (last_printed + 1).max(lineno.saturating_sub(opts.before_context as u64)); + // GNU's `used` is set by any selected line, hidden or not. With nothing written + // and no -A, its lastout is still unset and never matches, so the group gets `--`. + let lastout_unset = last_printed == 0 && opts.after_context == 0; + if use_context && used && (lastout_unset || rs != last_printed + 1) { + wprintln!(w, "--")?; + } + used = true; + for (bno, btext, bhidden) in before_buf.drain(..) { + if bno < rs { + continue; } - need_sep = false; - binary_matched |= binary_in_window; - last_binary_lineno = 0; - // Flush before-context buffer - for (bno, btext) in before_buf.drain(..) { - if !prefix.is_empty() { - wprint!(w, "{}-", prefix)?; - } - if opts.line_number { - wprint!(w, "{}-", bno)?; + if bhidden { + binary_matched |= !opts.only_matching; + } else { + if !opts.only_matching { + write_context(w, prefix, opts.line_number, bno, &btext).await?; } - wprintln!(w, "{}", String::from_utf8_lossy(&btext))?; + last_printed = bno; } - after_remaining = opts.after_context; } - printed_any = true; - if opts.only_matching && !opts.invert { - for m in re.find_iter(raw) { - // GNU drops the rest of the line once a match is suppressed. - if invalid && std::str::from_utf8(m.as_bytes()).is_err() { - binary_matched = true; - break; + if hidden { + // A hidden match starts no after-context (GNU lastout does not advance). + binary_matched = true; + pending = 0; + } else { + if opts.only_matching && !opts.invert { + for m in re.find_iter(raw) { + // GNU drops the rest of the line once a match is suppressed. + if invalid && std::str::from_utf8(m.as_bytes()).is_err() { + binary_matched = true; + break; + } + if !prefix.is_empty() { + wprint!(w, "{}:", prefix)?; + } + if opts.line_number { + wprint!(w, "{}:", lineno)?; + } + wprintln!(w, "{}", String::from_utf8_lossy(m.as_bytes()))?; } + } else { if !prefix.is_empty() { wprint!(w, "{}:", prefix)?; } if opts.line_number { wprint!(w, "{}:", lineno)?; } - wprintln!(w, "{}", String::from_utf8_lossy(m.as_bytes()))?; - } - } else { - if !prefix.is_empty() { - wprint!(w, "{}:", prefix)?; - } - if opts.line_number { - wprint!(w, "{}:", lineno)?; + wprintln!(w, "{}", String::from_utf8_lossy(raw))?; } - wprintln!(w, "{}", String::from_utf8_lossy(raw))?; + last_printed = lineno; + pending = opts.after_context; } if let Some(max) = opts.max_count @@ -394,41 +404,21 @@ async fn grep_reader( { break; } - } else if is_binary { - // Binary lines are never shown, but they still occupy a context slot. - if use_context { - if found && after_remaining > 0 { - after_remaining -= 1; - // -o prints no context, so the hidden line is never in play. - binary_matched |= !opts.only_matching; - if after_remaining == 0 { - need_sep = true; - } - } else { - before_buf.clear(); - last_binary_lineno = lineno; + } else if pending > 0 { + if hidden { + // GNU lastout never passes a hidden line, so it spends the rest of the context. + pending = 0; + binary_matched |= !opts.only_matching; + } else { + pending -= 1; + // -o prints no context, but the line still counts as written. + if !opts.only_matching { + write_context(w, prefix, opts.line_number, lineno, raw).await?; } + last_printed = lineno; } - } else if use_context && found && after_remaining > 0 { - // Print after-context line - after_remaining -= 1; - printed_any = true; - if !prefix.is_empty() { - wprint!(w, "{}-", prefix)?; - } - if opts.line_number { - wprint!(w, "{}-", lineno)?; - } - wprintln!(w, "{}", String::from_utf8_lossy(raw))?; - if after_remaining == 0 { - need_sep = true; - } - } else if use_context { - // Buffer for before-context - if after_remaining == 0 && found && !need_sep { - need_sep = true; - } - before_buf.push_back((lineno, raw.to_vec())); + } else if opts.before_context > 0 { + before_buf.push_back((lineno, raw.to_vec(), hidden)); while before_buf.len() > opts.before_context { before_buf.pop_front(); } diff --git a/tests/shell_integration.rs b/tests/shell_integration.rs index 23d687d..066903c 100644 --- a/tests/shell_integration.rs +++ b/tests/shell_integration.rs @@ -2026,6 +2026,22 @@ fn grep_binary_run(tag: &str, cmd: &str) -> strands_shell::Output { b"needle A\nl2\nl3\n\xffbin\nneedle B\n", ) .unwrap(); + std::fs::write(dir.join("hidm.txt"), b"l1\nl2\n\xffneedle\nneedle vis\n").unwrap(); + std::fs::write( + dir.join("sep4.txt"), + b"needle A\nl2\n\xffbin\nl4\nneedle B\n", + ) + .unwrap(); + std::fs::write( + dir.join("c7.txt"), + b"MATCH\n\xffc2\nc3\nc4\nc5\n\xffMATCH\n", + ) + .unwrap(); + std::fs::write(dir.join("adj.txt"), b"needle A\nl2\nl3\nl4\nneedle B\n").unwrap(); + std::fs::write(dir.join("foo.txt"), b"foo\nbar\n").unwrap(); + std::fs::write(dir.join("ha3.txt"), b"needle\n\xffa\nb\nc\nd\ne\nneedle2\n").unwrap(); + std::fs::write(dir.join("hidstart.txt"), b"\xffneedle\nc1\nc2\nneedle\n").unwrap(); + std::fs::write(dir.join("e14.txt"), b"\xffneedle\nx\ny\nneedle\n").unwrap(); for n in 1..=3 { let big: String = (0..5000).map(|i| format!("needle {i}\n")).collect(); std::fs::write(dir.join(format!("big{n}.txt")), big).unwrap(); @@ -2215,6 +2231,37 @@ fn grep_dropped_binary_line_keeps_group_separator() { assert_eq!(out.stdout, "1:needle A\n2-l2\n--\n5:needle B\n"); } +#[test] +fn grep_hidden_lines_keep_neighbouring_before_context() { + let note = "grep: /g/hidm.txt: binary file matches\n"; + let out = grep_binary_run("hidm", "grep -B2 needle /g/hidm.txt"); + assert_eq!(out.stdout, "l1\nl2\nneedle vis\n"); + assert_eq!(out.stderr, note); + let out = grep_binary_run("sep4", "grep -B3 needle /g/sep4.txt"); + assert_eq!(out.stdout, "needle A\nl2\nl4\nneedle B\n"); + assert_eq!(out.stderr, "grep: /g/sep4.txt: binary file matches\n"); + let out = grep_binary_run("c7", "grep -C1 MATCH /g/c7.txt"); + assert_eq!(out.stdout, "MATCH\n--\nc5\n"); + assert_eq!(out.stderr, "grep: /g/c7.txt: binary file matches\n"); +} + +#[test] +fn grep_adjacent_context_groups_have_no_separator() { + let out = grep_binary_run("adj", "grep -B3 needle /g/adj.txt"); + assert_eq!(out.stdout, "needle A\nl2\nl3\nl4\nneedle B\n"); + assert_eq!(out.stderr, ""); +} + +#[test] +fn grep_only_matching_prints_no_context() { + let out = grep_binary_run("ofoo1", "grep -o -A1 foo /g/foo.txt"); + assert_eq!(out.stdout, "foo\n"); + assert_eq!(out.stderr, ""); + let out = grep_binary_run("ohid", "grep -o -A1 needle /g/hidm.txt"); + assert_eq!(out.stdout, "needle\nneedle\n"); + assert_eq!(out.stderr, ""); +} + #[test] fn grep_without_match_is_silent_for_invalid_utf8() { let out = grep_binary_run("iq", "grep -I needle /g/mid.txt"); @@ -2254,9 +2301,28 @@ fn grep_broken_pipe_stops_after_first_file() { } #[test] -fn grep_no_leading_separator_before_hidden_binary_match() { +fn grep_leading_separator_after_hidden_binary_match() { + // GNU's `used` flag is set by hidden selected lines too. let out = grep_binary_run("sep0", "grep -A1 needle /g/bin2.dat"); - assert_eq!(out.stdout, "needle\n"); + assert_eq!(out.stdout, "--\nneedle\n"); + let out = grep_binary_run("sep0b", "grep -n -A1 needle /g/hidstart.txt"); + assert_eq!(out.stdout, "--\n4:needle\n"); + let out = grep_binary_run("sep0c", "grep -n -C2 needle /g/hidstart.txt"); + assert_eq!(out.stdout, "--\n2-c1\n3-c2\n4:needle\n"); + let out = grep_binary_run("sep0d", "grep -B1 needle /g/e14.txt"); + assert_eq!(out.stdout, "--\ny\nneedle\n"); + // With -A pending, GNU's lastout starts at the first line: a contiguous group gets none. + let out = grep_binary_run("sep0e", "grep -n -C3 needle /g/hidstart.txt"); + assert_eq!(out.stdout, "2-c1\n3-c2\n4:needle\n"); +} + +#[test] +fn grep_hidden_line_ends_after_context() { + let out = grep_binary_run("ha3a", "grep -n -A3 needle /g/ha3.txt"); + assert_eq!(out.stdout, "1:needle\n--\n7:needle2\n"); + assert_eq!(out.stderr, "grep: /g/ha3.txt: binary file matches\n"); + let out = grep_binary_run("ha3c", "grep -n -C3 needle /g/ha3.txt"); + assert_eq!(out.stdout, "1:needle\n--\n4-c\n5-d\n6-e\n7:needle2\n"); } #[test]