fix: NUL-delimit check-ignore; count untracked lines in chunks

Author: nikolay-e

crates/diffctx-native/src/git.rs

@@ -831,10 +831,18 @@ pub fn find_ignored_paths(repo_root: &Path, rel_paths: &[String]) -> FxHashSet<S
     // below turned that into "nothing is ignored" — silently disabling the
     // `.diffctx/ignore` contract exactly when the repo is large enough for it
     // to matter. stdin has no such ceiling.
+    // `-z` on BOTH sides. Line-delimited input splits a path containing a
+    // newline into two phantom queries: git then answers about `secret` and
+    // `name.py` while the real path `secret\nname.py` never gets a verdict, so
+    // the lookup below misses it and a file the user declared ignored is
+    // published. Verified against git directly — line mode reports the
+    // truncated stem, NUL mode reports the whole path. The rest of this module
+    // is already `-z` throughout.
     let mut args: Vec<String> = vec![
         "check-ignore".into(),
         "--no-index".into(),
         "-v".into(),
+        "-z".into(),
         "--stdin".into(),
     ];
     if let Some(ref path) = temp_excludes {
@@ -848,7 +856,7 @@ pub fn find_ignored_paths(repo_root: &Path, rel_paths: &[String]) -> FxHashSet<S
     // matches that nobody is draining, we block on the write, and the pipeline
     // hangs until the git timeout fires. A file hands the feeding to the kernel,
     // which needs no second thread to stay correct.
-    let query_file = write_private_temp_file(&queries.join("\n"));
+    let query_file = write_private_temp_file(&format!("{}\0", queries.join("\0")));
 
     let result = (|| -> Result<FxHashSet<String>> {
         let Some(ref query_path) = query_file else {
@@ -875,12 +883,7 @@ pub fn find_ignored_paths(repo_root: &Path, rel_paths: &[String]) -> FxHashSet<S
         let stdout = String::from_utf8_lossy(&output.stdout);
         let excludes_source = temp_excludes.as_ref().map(|p| p.display().to_string());
 
-        let mut rules: rustc_hash::FxHashMap<String, String> = rustc_hash::FxHashMap::default();
-        for line in stdout.lines() {
-            if let Some((rule, path)) = parse_verbose_ignore_match(line) {
-                rules.insert(path, rule);
-            }
-        }
+        let rules = parse_verbose_ignore_records(&stdout);
 
         Ok(rel_paths
             .iter()
@@ -949,9 +952,32 @@ pub fn find_ignored_paths(repo_root: &Path, rel_paths: &[String]) -> FxHashSet<S
 /// containing a tab, so the last tab is always the separator. Splitting from
 /// the left mis-parses a pattern that itself contains a tab, and the resulting
 /// lookup miss reports an ignored file as not ignored — i.e. it leaks.
-fn parse_verbose_ignore_match(line: &str) -> Option<(String, String)> {
-    let (rule, path) = line.rsplit_once('\t')?;
-    Some((rule.to_string(), unquote_c_style(path)))
+/// `check-ignore -v -z` emits four NUL-separated fields per match —
+/// `<source>\0<line>\0<pattern>\0<path>\0` — and never quotes the path,
+/// because NUL cannot appear in one.
+///
+/// The text format this replaced (`<source>:<line>:<pattern>\t<path>`) had to
+/// guess where the pattern ended and the path began, and C-quoting made a path
+/// containing a tab or a newline ambiguous. Field-delimited records remove the
+/// guess entirely.
+fn parse_verbose_ignore_records(stdout: &str) -> rustc_hash::FxHashMap<String, String> {
+    let mut rules: rustc_hash::FxHashMap<String, String> = rustc_hash::FxHashMap::default();
+    let fields: Vec<&str> = stdout.split('\0').collect();
+    // A trailing NUL leaves an empty final element; chunks of four skip it.
+    for record in fields.chunks(4) {
+        if record.len() < 4 {
+            break;
+        }
+        let (source, line, pattern, path) = (record[0], record[1], record[2], record[3]);
+        if path.is_empty() {
+            continue;
+        }
+        // The rule identity keeps the text format's shape: callers compare it
+        // against `format!("{excludes_source}:")` to tell a diffctx-declared
+        // exclusion from a gitignore one.
+        rules.insert(path.to_string(), format!("{source}:{line}:{pattern}"));
+    }
+    rules
 }
 
 fn ancestor_dirs(rel: &str) -> Vec<String> {
@@ -1276,25 +1302,52 @@ mod tests {
         }
     }
 
-    // --- parse_verbose_ignore_match: last-tab split survives a tab in the pattern ---
+    // --- parse_verbose_ignore_records: NUL fields remove every delimiter guess ---
+
+    #[test]
+    fn a_tab_in_the_pattern_no_longer_needs_disambiguating() {
+        // The text format put the pattern and the path on one line separated by
+        // a tab, so a pattern containing a tab had to be split from the right
+        // and hoped for. Fields make it unambiguous.
+        let stdout = ".gitignore\x003\x00foo\tbar\x00some/real/path.txt\x00";
+        let rules = parse_verbose_ignore_records(stdout);
+        assert_eq!(
+            rules.get("some/real/path.txt").map(String::as_str),
+            Some(".gitignore:3:foo\tbar")
+        );
+    }
+
+    /// The leak that forced `-z`. A newline in a filename split the query into
+    /// two phantom paths, git answered about the stem, the real path never got
+    /// a verdict, and a file the user declared ignored was published.
+    #[test]
+    fn a_newline_in_the_path_survives_as_one_record() {
+        let stdout = "excl\x001\x00secret*\x00secret\nname.py\x00";
+        let rules = parse_verbose_ignore_records(stdout);
+        assert_eq!(rules.len(), 1);
+        assert_eq!(
+            rules.get("secret\nname.py").map(String::as_str),
+            Some("excl:1:secret*")
+        );
+    }
 
     #[test]
-    fn parse_verbose_ignore_match_splits_on_last_tab_not_first() {
-        // The pattern itself ("foo\tbar") contains a literal tab. Splitting
-        // from the left would take "foo" as the path, silently reporting an
-        // ignored file as not ignored.
-        let line = ".gitignore:3:foo\tbar\tsome/real/path.txt";
-        let (rule, path) = parse_verbose_ignore_match(line).expect("parse");
-        assert_eq!(path, "some/real/path.txt");
-        assert_eq!(rule, ".gitignore:3:foo\tbar");
+    fn several_records_and_a_trailing_nul_parse_cleanly() {
+        let stdout = ".gitignore\x001\x00*.log\x00a.log\x00.gitignore\x002\x00*.tmp\x00b/c.tmp\x00";
+        let rules = parse_verbose_ignore_records(stdout);
+        assert_eq!(rules.len(), 2);
+        assert!(rules.contains_key("a.log"));
+        assert!(rules.contains_key("b/c.tmp"));
     }
 
     #[test]
-    fn parse_verbose_ignore_match_unquotes_c_style_path() {
-        let line = ".gitignore:1:*.log\t\"weird\\tfile.log\"";
-        let (rule, path) = parse_verbose_ignore_match(line).expect("parse");
-        assert_eq!(path, "weird\tfile.log");
-        assert_eq!(rule, ".gitignore:1:*.log");
+    fn a_truncated_final_record_is_dropped_not_half_read() {
+        // Killed mid-write, the last record is short. Reading three fields as
+        // four would key a rule under a pattern.
+        let stdout = ".gitignore\x001\x00*.log\x00a.log\x00.gitignore\x002\x00*.tmp\x00";
+        let rules = parse_verbose_ignore_records(stdout);
+        assert_eq!(rules.len(), 1);
+        assert!(rules.contains_key("a.log"));
     }
 
     // --- anchor_diffctx_ignore_line: 4 reachable outputs, root vs nested, negation ---

crates/diffctx-native/src/pipeline.rs

@@ -122,33 +122,68 @@ pub fn build_diff_context_locate(
 /// Line count for an untracked file, or `None` when it is not readable UTF-8
 /// text — the same rejection `read_to_string` gave, so binaries stay excluded.
 ///
-/// Counted through a reader rather than by materialising the file. Untracked
-/// files are scanned before any size filter applies (`max_changed_file_size` is
-/// enforced later, in fragmentation), so a dirty tree holding one multi-GB log
-/// allocated the whole thing here just to reach `.lines().count()`. Memory is
-/// now bounded by the buffer instead of the file size, and the count is
-/// unchanged: `str::lines` and `BufRead::lines` agree on `\n` splitting, on
-/// stripping a trailing `\r`, and on not counting a trailing newline as a line.
+/// Untracked files are scanned before any size filter applies
+/// (`max_changed_file_size` is enforced later, in fragmentation), so a dirty
+/// tree holding one multi-GB log used to allocate all of it here just to reach
+/// `.lines().count()`.
+///
+/// Counted over fixed byte chunks rather than by line. `BufReader::lines()`
+/// bounds nothing on its own: it allocates each line, and a minified bundle or
+/// a single-line JSON dump is one line hundreds of megabytes long — exactly the
+/// shape this was supposed to stop loading. The buffer is the only allocation
+/// that scales.
+///
+/// UTF-8 is validated as it streams, with the incomplete tail of one chunk
+/// carried into the next, so a multi-byte character split across a chunk
+/// boundary is not mistaken for the invalid byte that rejects a binary.
 ///
 /// Counting rather than size-gating keeps this bit-identical: an oversized file
-/// still gets the same hunk it always did. Skipping it outright would also be
-/// safe for the changed-files list (that comes from the untracked scan, not from
-/// hunk emission — a binary is listed today with no hunk at all), but it would
-/// drop a hunk the rest of the pipeline has seen since forever, and nothing
-/// here needs that risk. What remains unbounded is time, not memory: a
-/// multi-GB file is still streamed end to end.
+/// still gets the same hunk it always did, and the count matches `str::lines`
+/// (both split on `\n` and neither counts a trailing newline as a line).
 fn count_text_lines(path: &Path) -> Option<u32> {
-    use std::io::BufRead;
-
-    let file = std::fs::File::open(path).ok()?;
-    let mut count: u32 = 0;
-    for line in std::io::BufReader::new(file).lines() {
-        // Invalid UTF-8 rejects the whole file, as `read_to_string` did — a
-        // partial count would invent a hunk length for a binary.
-        line.ok()?;
-        count = count.saturating_add(1);
+    use std::io::Read;
+
+    const CHUNK: usize = 64 * 1024;
+
+    let mut file = std::fs::File::open(path).ok()?;
+    let mut buf = vec![0u8; CHUNK];
+    let mut carry: Vec<u8> = Vec::new();
+    let mut newlines: u32 = 0;
+    let mut last_byte: Option<u8> = None;
+
+    loop {
+        let read = file.read(&mut buf).ok()?;
+        if read == 0 {
+            break;
+        }
+        carry.extend_from_slice(&buf[..read]);
+        let valid_upto = match std::str::from_utf8(&carry) {
+            Ok(_) => carry.len(),
+            // A truncated character at the end of a chunk is not an error yet;
+            // anything else is a binary and rejects the file, as before.
+            Err(e) if e.error_len().is_none() => e.valid_up_to(),
+            Err(_) => return None,
+        };
+        newlines = newlines
+            .saturating_add(carry[..valid_upto].iter().filter(|b| **b == b'\n').count() as u32);
+        if let Some(&b) = carry[..valid_upto].last() {
+            last_byte = Some(b);
+        }
+        carry.drain(..valid_upto);
+    }
+
+    // Trailing bytes that never completed a character mean the file ends
+    // mid-sequence — invalid UTF-8, same verdict as `read_to_string`.
+    if !carry.is_empty() {
+        return None;
     }
-    Some(count)
+    // `str::lines` does not count a trailing newline as starting a line, and
+    // counts a final unterminated line as one.
+    Some(match last_byte {
+        None => 0,
+        Some(b'\n') => newlines,
+        Some(_) => newlines.saturating_add(1),
+    })
 }
 
 /// Heavy phase: clone/parse/fragment/discover/tokenize/score. Independent