Close ripgrep parity gaps found by differential testing - #100
Merged
Conversation
Implements P0 items 1/3/6 and all P1 items from the tgrep-vs-ripgrep gap analysis, factors the duplicated match pipeline into a shared module, and fixes the correctness bugs found while validating. Correctness (silent wrong answers): - Remove the unconditional 1 MiB file cap from search; it is now --max-filesize, default unlimited when searching and 1 MiB when indexing. - Build the trigram query plan from every pattern, not just the first, so -e/-f no longer silently drop candidates on the indexed and server paths. - Bypass the trigram plan for -v: inverted matching selects files where some line does *not* match, so plan-narrowed candidates were the wrong set. - Decode files lossily instead of skipping invalid UTF-8, on both the local and server paths. - Apply --max-filesize on the indexed, explicit-file and server paths. - Report binary files over the server, and still count them under -c. - -c over the server counts distinct matching lines, not context rows. - Resolve -f/--file patterns client-side so they survive the RPC hop. - Limit -m/--max-count by match, not by line, so -U -m 1 cannot print a partial multiline match. - Suppress the -- context separator when no context was requested. - Validate --max-filesize once up front instead of swallowing parse errors. Tooling compatibility: - ripgrep-compatible --json envelope (begin/match/context/end/summary). - --vimgrep emits one row per match; --color always highlights matches. - ripgrep exit codes: 0 matched, 1 no match, 2 error. - Free -L for --follow and move --files-without-match to its long name. - Strip Windows verbatim \\?\ prefixes from displayed paths. - New flags: -s, -a/--text, -I, -., -L/--follow, --no-messages, --iglob, --glob-case-insensitive, --multiline-dotall, --max-filesize. - Globs are case-sensitive by default, matching ripgrep. Refactor: - Move the whole matching pipeline (SearchMatcher, MatchOptions, Emit, FileMatches, collect_hits) into tgrep-cli/src/matching.rs. search.rs and serve.rs now share it and differ only in how they render results. Verified: 325 tests pass, clippy and fmt clean, and server/local-index/ brute-force output agrees across 24 flag combinations. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 4e9d52fa-e7ec-4e5d-b9a9-f33c8cf582a7
Closes three of the remaining gaps between tgrep and ripgrep so tgrep can stand in for ripgrep on large repos. File types: replace the 12-entry ad-hoc table with ripgrep's verbatim 219-group definition set, plus a TypeDefs builder and TypeFilter. The CLI gains repeatable -t, -T/--type-not, --type-add, --type-clear, and a --type-list that reflects clears and adds. Globs match the basename with literal_separator(true), and an unknown type exits 2, both matching ripgrep. Encoding: new tgrep-core/src/encoding.rs wraps encoding_rs with BOM sniffing and an EncodingMode of Auto/None/Explicit, wired into every read and binary-detection site so UTF-16 files are searched instead of being reported as binary. Adds -E/--encoding and --no-encoding. The index is always built in Auto mode, so a non-Auto encoding makes trigram filtering unsound and also misses BOM-less UTF-16 files that were classified binary at index time; such a request therefore bypasses both the index and the server rather than falling back to MatchAll. Flags: wire ~45 further ripgrep flags through all three search paths (brute force, local index, and server), including --sort/--sortr/ --sort-files, --count-matches, --include-zero, --passthru, --replace, --stop-on-nonmatch, --pretty, --engine, --regex-size-limit, and --dfa-size-limit. Arguments are now validated once up front through Cli::resolve, so bad input exits 2 instead of being ignored. -z/--search-zip is rejected explicitly instead of silently reporting no matches, and --crlf, --mmap, --no-config, and --colors are accepted as documented no-ops. Along the way: - --regex-size-limit now errors on CompiledTooBig instead of silently falling back to the backtracking engine. - apply_max_columns returned None when no -M was set, suppressing every output line. - --count-matches returned nothing over the server; the client now sums spans per file. - --sort orders by path components on all three paths, so `src.rs`, `src/lib.rs`, and `srcx.rs` agree with ripgrep's Path::cmp rather than a raw string compare. - Time-based --sort uses sort_by_cached_key; sort_by_key re-read file metadata on every comparison. - --column no longer fabricates column 1 on context lines. - clap's derive builds every argument in one generated stack frame, which overflowed the main thread's stack in debug builds once the argument count grew, so the CLI body runs on a 32 MiB worker thread. Adds ~48 integration tests covering file types, encoding, the new flags, the no-op flags, and cross-path agreement. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 4e9d52fa-e7ec-4e5d-b9a9-f33c8cf582a7
Verified tgrep against a real ripgrep 15.2.0 binary with a differential harness covering 107 cases across all three search paths (brute force, local index, and server). Every expectation below was confirmed by running the same arguments through rg rather than derived from its source. Behaviour fixes: - Path display now pushes results onto the path the user typed, so the argument survives verbatim and only the appended remainder uses the native separator. - -n and -H follow ripgrep's context-dependent defaults: line numbers only on a TTY, filenames unless exactly one file was named. - With -e/-f, every positional argument is a path. - --index-path with a subdirectory or file argument no longer returns nothing, and --max-depth is honoured on the indexed and server paths. - Binary files follow ripgrep's visibility model: silently skipped when reached by traversal, reported with a note when named explicitly, and promoted to that note by the new --binary flag. FileOutcome replaces a bool so a skipped file cannot leak into --files-without-match. - --column, -b and --vimgrep report source byte offsets rather than offsets into lossily decoded text. - -M/--max-columns measures the line including its terminator and no longer drops over-long context lines. Fixes found in review of the above: - to_source_offset could underflow and panic when an offset landed inside a U+FFFD, which --replace on a file containing invalid UTF-8 reliably triggered. Offsets inside a replacement now clamp to it. - --replace reports positions in the rewritten line, so -o accumulates each replacement's length delta instead of reusing original offsets. - Column mapping and the max-columns terminator are shared by the local and server paths through one helper, so they cannot drift. Adds tgrep-cli/tests/ripgrep_parity.rs, a regression suite recording the verified ripgrep behaviour for each divergence, and documents the remaining invalid-UTF-8 limitations in the README. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 4e9d52fa-e7ec-4e5d-b9a9-f33c8cf582a7
Contributor
There was a problem hiding this comment.
Pull request overview
This PR tightens behavioral parity between tgrep and ripgrep (rg 15.2.0) by aligning CLI semantics, walking/filtering behavior, encoding/offset handling, and output formatting across local, indexed, and server-backed search paths, backed by an extensive new regression suite.
Changes:
- Align output behavior with ripgrep: path display rules, context-dependent defaults (
-n/-H),--column/-b/--vimgrepbyte-accurate offsets,-M/--max-columnssemantics, and ripgrep-like JSON stream framing/stats. - Expand file discovery and filtering parity: richer walker options (max depth, filesystem boundaries, ignore-file variants), glob/iglob behavior, and a ripgrep-derived
--typedefinition table with--type-add/--type-clear. - Add new core capabilities and coverage: encoding support (
-E/--encoding), multi-pattern query planning, binary visibility model, and a large ripgrep-parity test suite.
Show a summary per file
| File | Description |
|---|---|
| tgrep-core/src/walker.rs | Extends walking options (depth, FS boundaries, ignore-file controls, size cap modeling) and adds skipped_too_large accounting. |
| tgrep-core/src/query.rs | Adds build_multi_pattern_plan to union plans across all patterns and adds unit tests. |
| tgrep-core/src/live.rs | Switches live indexing to operate on decoded-for-index text for binary detection/upserts. |
| tgrep-core/src/lib.rs | Exposes new encoding module. |
| tgrep-core/src/filetypes.rs | Replaces hardcoded type list with a ripgrep-derived table and adds full --type* machinery. |
| tgrep-core/src/encoding.rs | Adds ripgrep-like encoding parsing/decoding, BOM handling, and lossy fixup mapping for byte-accurate offsets. |
| tgrep-core/src/builder.rs | Introduces configurable max indexed file size and indexes decoded text for parity with search semantics. |
| tgrep-core/Cargo.toml | Adds globset and encoding_rs dependencies. |
| tgrep-cli/tests/ripgrep_parity.rs | New large regression suite pinning verified rg behaviors across features and search paths. |
| tgrep-cli/src/walkcount.rs | Updates reporting to include “too large” skip category. |
| tgrep-cli/src/serve.rs | Unifies server matching with shared matcher; adds encoding, type filters, glob behavior, max filesize, binary notes, and decoded-file caching. |
| tgrep-cli/src/output.rs | Major output parity work: path display model, byte columns/spans, max-columns rules, binary notes, JSON begin/end/summary, and highlighting. |
| tgrep-cli/src/matching.rs | Centralizes matching/regex-engine selection, multiline/span grouping, replace-coordinate logic, and emission model. |
| tgrep-cli/src/main.rs | Expands CLI surface to match rg (types, glob modes, encoding, binary/text, replace, limits, sorting, etc.) and refines defaults/exit codes. |
| tgrep-cli/src/index.rs | Refactors tgrep index invocation into an options struct and adds max file size support. |
| tgrep-cli/src/glob_filter.rs | Adds --iglob and case-sensitivity controls; keeps separator/literal behavior consistent with ripgrep. |
| README.md | Documents new parity rules (defaults, binary model, encoding, file walking, size limits, exit codes). |
| Cargo.lock | Locks new dependency versions (encoding_rs, globset). |
Review details
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
- Files reviewed: 19/20 changed files
- Comments generated: 4
- Review effort level: Lite
Wire up --no-ignore-messages, which parsed but was never read: the walker was handed --no-messages for both. ripgrep gates ignore-file errors on both flags, so pass the pair. With the flag reachable, make it worth having. ripgrep also reports ignore files it could not parse that were found during the walk; the `ignore` crate hangs those on the entry rather than yielding them as an error, so they were dropped. Report them, rendered relative to the search root so Windows does not surface a `\\?\C:\...` path. The --ignore-file message no longer repeats the path, which the crate's error already carries. Verified against rg 15.2.0 that a malformed ignore file does not change the exit code: ripgrep's `ignore_message!` deliberately skips the "errored" flag that `err_message!` sets, so a reported parse error still exits 0 when a match was found. Documented, and pinned by tests plus four differential cases. Reject --regex-size-limit/--dfa-size-limit values that do not fit a usize instead of truncating with `as`, matching ripgrep's human_readable_usize. Fixed the same cast in the server's request parsing, and gave the shared size parser the flag name so a bad --regex-size-limit no longer reports a problem with --max-filesize. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 4e9d52fa-e7ec-4e5d-b9a9-f33c8cf582a7
Contributor
There was a problem hiding this comment.
Review details
Suppressed comments (3)
Previously missed (2) — in code that hasn't changed since the last review.
tgrep-cli/src/output.rs:459
- In JSON output,
lines.textis always emitted with a trailing\n, even when the source line had no terminator (final line) or had\r\n. This breaks ripgrep JSON parity and makes consumers see text that doesn’t match the underlying file.
This issue also appears on line 541 of the same file.
"lines": { "text": format!("{content}\n") },
tgrep-cli/src/output.rs:327
- Per-file JSON
endstats useself.started.elapsed(), butstartedis initialized once inOutputWriter::newand never reset per file. In ripgrep JSON,end.data.stats.elapsedis per-file elapsed, while the overall elapsed belongs in the finalsummarymessage; with the current code, per-file elapsed will monotonically increase and won’t reflect the time spent searching each file.
"stats": self.file_stats.to_json(self.started.elapsed()),
tgrep-cli/src/output.rs:541
- In JSON output, match
lines.textis always emitted with a trailing\n. For ripgrep parity (and accurate round-tripping), this should reflect the original line terminator (\n,\r\n, or none for an unterminated final line /-o).
"lines": { "text": format!("{content}\n") },
- Files reviewed: 19/20 changed files
- Comments generated: 1
- Review effort level: Lite
no_heading_outputs_flat_format searched an absolute fixture path and then asserted that splitn(3, ':') yielded three fields. On Windows the drive-letter colon supplied the third field, so the test passed for the wrong reason; on macOS and Linux the same assertion failed because there is no such colon and --no-heading does not imply line numbers when stdout is not a terminal. Run from inside the fixture with relative arguments, matching the pattern the neighbouring vimgrep tests already use, and assert what ripgrep actually prints: file:content for --no-heading, and file:line:content once -n is given. Both shapes were verified byte-for-byte against rg 15.2.0. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 4e9d52fa-e7ec-4e5d-b9a9-f33c8cf582a7
Contributor
There was a problem hiding this comment.
Review details
Suppressed comments (3)
Previously missed (3) — in code that hasn't changed since the last review.
tgrep-cli/src/main.rs:1127
report_missing_pathhard-codes a Windows-specific NotFound message/errno ("The system cannot find the path specified. (os error 3)"). On non-Windows platforms this will print the wrong error text and error code, and it won’t match what the OS would actually report for the missing path. Prefer formatting the realstd::io::Errorfrom a failed filesystem call so the message is correct on every platform.
fn report_missing_path(path: &std::path::Path, no_messages: bool) {
ERRORED.store(true, std::sync::atomic::Ordering::SeqCst);
if !no_messages {
eprintln!(
"tgrep: {}: The system cannot find the path specified. (os error 3)",
path.display()
);
}
tgrep-core/src/filetypes.rs:63
TypeDefs::clearcurrently removes only the exact key passed in. Because built-in types are inserted once per alias (e.g.batandbatch), clearing one alias leaves the other alias defined, so--type-clear batwon’t actually clear the whole ripgrep type group as described (and--type-listwill still show the remaining alias). Clearing should remove all names/aliases that belong to the same built-in group.
/// Apply `--type-clear NAME`.
pub fn clear(&mut self, name: &str) {
self.map.remove(name);
}
tgrep-core/src/encoding.rs:195
lossy_utf8tracks cumulative decoded-vs-source length changes in ausize(gained) and clampsreplacedwithmin(REPLACEMENT_LEN). If an invalid UTF-8 subsequence is longer than 3 bytes (U+FFFD’s UTF-8 width), the decoded output can be shorter than the source, which requires representing a negative shift. With the currentmin(...)clamp, such cases would be mapped incorrectly byLossyFixups::to_source_offset(offsets after the replacement would be too small).
let replaced = err.error_len().unwrap_or(rest.len() - valid);
shifts.push((out.len(), {
gained += REPLACEMENT_LEN - replaced.min(REPLACEMENT_LEN);
gained
}));
- Files reviewed: 19/20 changed files
- Comments generated: 0 new
- Review effort level: Lite
A binary file matched with --json produced a begin/end pair with no match events, matches: 0 and binary_offset: null, so the hit was indistinguishable from a clean file that matched nothing. ripgrep has no "binary file matches" note in JSON: it emits the matching lines as ordinary match events and puts the offset of the first NUL on the file's end message, stopping bytes_searched at that offset. Verified against rg 15.2.0, whose output tgrep now reproduces field for field. The local path records the offset on the writer and falls through to the normal emission instead of returning early. The daemon only ever returned a marker for a binary file, so the client asks for the lines too via a new binary_lines request flag, defaulted off so an older server or client behaves as before. The rows are dropped along with the marker when the file was only reached by traversal, which otherwise would have leaked its raw contents. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 4e9d52fa-e7ec-4e5d-b9a9-f33c8cf582a7
Contributor
There was a problem hiding this comment.
Review details
Suppressed comments (4)
Previously missed (4) — in code that hasn't changed since the last review.
tgrep-cli/src/main.rs:1127
report_missing_pathhardcodes a Windows-specific OS error message ("The system cannot find the path specified. (os error 3)"). On non-Windows platforms this produces an incorrect/misleading diagnostic and diverges from ripgrep’s platform-native error text. Prefer printing the actualstd::io::Errorfrom a failing filesystem call so the message matches the host OS.
fn report_missing_path(path: &std::path::Path, no_messages: bool) {
ERRORED.store(true, std::sync::atomic::Ordering::SeqCst);
if !no_messages {
eprintln!(
"tgrep: {}: The system cannot find the path specified. (os error 3)",
path.display()
);
}
tgrep-cli/src/matching.rs:555
- In
--only-matchingmode,--max-countcurrently limits the number of matching lines discovered (viacollect_hits), but this loop still emits all matches on each matching line. That means-o -m 1can print multiple match rows if the first matching line contains multiple hits, which violates ripgrep’s "stop after N matches" semantics in only-matching mode.
for &(s, e) in &hit.spans {
let text = &line[s..e];
on_emit(Emit::Match {
line_number: hit.idx + 1,
content: Cow::Borrowed(text),
tgrep-core/src/filetypes.rs:63
TypeDefs::clearremoves only the provided key. Because aliases (e.g.py/python,bat/batch) are stored as separate entries inTypeDefs::map,--type-clear pywill leavepythondefined (and similarly--type-add py:...will only updatepy). That breaks the expected invariant that aliases remain equivalent once user customizations are applied, and can cause surprising differences depending on which alias the user typed.
/// Apply `--type-clear NAME`.
pub fn clear(&mut self, name: &str) {
self.map.remove(name);
}
tgrep-cli/src/output.rs:345
- In JSON mode, per-file
endmessages reportstats.elapsedusingself.started.elapsed()(time since the writer was created). That makes per-fileelapsedgrow cumulatively across files instead of reflecting time spent on that particular file; the overall duration is already represented by thesummary.elapsed_totalobject.
"path": { "text": self.display_path(&file) },
"binary_offset": binary_offset,
"stats": self.file_stats.to_json(self.started.elapsed()),
},
});
- Files reviewed: 19/20 changed files
- Comments generated: 0 new
- Review effort level: Lite
ripgrep's unrestricted levels are -u = --no-ignore, -uu = +--hidden and -uuu = +--binary. tgrep's help text said the same, but the implementation set --text for the third level, so -uuu printed a binary file's raw lines where rg prints "binary file matches (found ...)". Confirmed against rg 15.2.0. Setting �inary rather than ext also keeps -uuu on the brute-force path, since bypass_index already keys off the resolved flag. Documents the four server/index tuning flags that had no README entry, and adds the long --unrestricted spelling to the flag table. Every long flag the CLI accepts is now mentioned in the README. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 4e9d52fa-e7ec-4e5d-b9a9-f33c8cf582a7
Contributor
There was a problem hiding this comment.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
tgrep-cli/src/main.rs:1131
report_missing_pathhard-codes a Windows-only error string ("The system cannot find the path specified. (os error 3)"). On non-Windows (and for other failure modes like permission denied), this will print the wrong message and diverges from ripgrep-style diagnostics, which surface the OS-provided error text.
Consider formatting the actual std::io::Error from metadata and printing that instead.
fn report_missing_path(path: &std::path::Path, no_messages: bool) {
ERRORED.store(true, std::sync::atomic::Ordering::SeqCst);
if !no_messages {
eprintln!(
"tgrep: {}: The system cannot find the path specified. (os error 3)",
path.display()
);
}
- Files reviewed: 19/20 changed files
- Comments generated: 1
- Review effort level: Lite
ripgrep's -m limits matching lines. Its multiline searcher reports one unit per contiguous block of lines that matches cover, and everything inside that block comes with it, so three matches on one line are a single unit and all three are still reported. tgrep's multiline branch instead capped the match spans themselves, which under-reported any line holding more than one match: \$ printf 'foo foo foo\nfoo bar\n' > m.txt \$ rg -U -m1 --vimgrep foo m.txt # 3 rows \$ tgrep -U -m1 --vimgrep foo m.txt # 1 row Count line blocks instead. Keeping whole spans means a match straddling a line boundary still prints in full rather than being clipped mid-pattern, which was the reason the limit was applied to spans in the first place. The line-oriented branch already counted lines and is unchanged. find_spans' limit argument now has no callers, so drop it. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 4e9d52fa-e7ec-4e5d-b9a9-f33c8cf582a7
Contributor
There was a problem hiding this comment.
Review details
Suppressed comments (2)
Previously missed (2) — in code that hasn't changed since the last review.
tgrep-cli/src/main.rs:1129
report_missing_pathhardcodes a Windows-only error string ("os error 3" / "The system cannot find..."). On non-Windows platforms this diverges from the OS error ripgrep prints (and from the actual underlying error), which makes stderr misleading.
eprintln!(
"tgrep: {}: The system cannot find the path specified. (os error 3)",
path.display()
);
tgrep-cli/src/main.rs:1118
- In
--filesmode, missing paths setERRORED, butlist_filesstill returnsOk(()). That meanstgrep --files <missing>can exit 0 even though an I/O error occurred (and possibly printed), whereas the rest of the CLI usesERROREDto produce exit code 2.
}
Ok(())
- Files reviewed: 19/20 changed files
- Comments generated: 0 new
- Review effort level: Lite
--vimgrep exists to give an editor one jump target per match, so ripgrep reports a match that crosses a line boundary only on the line it starts on. tgrep printed a row for every line the match covered: \$ printf 'a foo\nbar foo\nbaz foo\nqux foo\n' > ml.txt \$ rg -U --vimgrep '(?s)foo.*?foo' ml.txt # 2 rows \$ tgrep -U --vimgrep '(?s)foo.*?foo' ml.txt # 4 rows Clip each span to the end of its starting line when --vimgrep is on. Plain -U is untouched and still prints every line a match covers, which is also what ripgrep does. The server does its own matching, so --vimgrep now travels over the protocol alongside the other match options. It defaults to false, so a client and server on different versions still agree. Two rg behaviours are deliberately not copied, both cases where it names a column that isn't on the line it prints: -U --column repeats one column for every line of a match, and -U --vimgrep -o can report column 19 of a 7-character line. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 4e9d52fa-e7ec-4e5d-b9a9-f33c8cf582a7
Contributor
There was a problem hiding this comment.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
tgrep-cli/src/output.rs:345
- Per-file JSON
endmessages currently useself.started.elapsed()forstats.elapsed, which makes each file’sstats.elapsedcumulative since the writer was created (later files will report a larger elapsed that includes earlier files). ripgrep’s JSON format reportsstats.elapsedper-file, while the finalsummarycarries the total elapsed (elapsed_total). Track a per-file startInstant(set when emittingbegin) and use its elapsed infinish_json_file().
let msg = serde_json::json!({
"type": "end",
"data": {
"path": { "text": self.display_path(&file) },
"binary_offset": binary_offset,
"stats": self.file_stats.to_json(self.started.elapsed()),
},
});
- Files reviewed: 19/20 changed files
- Comments generated: 1
- Review effort level: Lite
Re-ran all 12 benchmark workflows on this branch and regenerated every published figure in BENCHMARKS.md and README.md from the resulting 18 artifacts. tgrep still wins all 18 cells. Geometric means move slightly, to 12.6x on Windows, 8.4x on macOS and 2.5x on Linux, and the headline peak moves from 55x to 58x (gecko-dev on macOS). The differences from the previous set are runner variance rather than a change in behaviour: ripgrep's own baselines move between sessions, most visibly on macOS. Also corrected two things the refresh exposed: - Ratios near 1.0 were published to one decimal, so kubernetes/Linux read as "1.1x" when it measured 1.06x — a 4% overstatement in tgrep's favour. Cells below 2x now carry two decimals. - The kernel narrative quoted a single before/after pair for ripgrep's Linux total. It now quotes the band across all six runs we have (28-46s), which is the actual evidence that ripgrep is insensitive to the query set, and the macOS caution now compares only same-query-set runs (388s, 495s, 500s) instead of conflating the old and new suites. Every number in both files is machine-checked against the raw artifacts. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 4e9d52fa-e7ec-4e5d-b9a9-f33c8cf582a7
Contributor
There was a problem hiding this comment.
Review details
Suppressed comments (1)
tgrep-cli/src/output.rs:572
- In ripgrep-compatible JSON,
stats.matchescounts the number of submatches, not the number of emitted match events. For-v/--invert-match,submatchesis empty and should contribute 0 matches;max(1)inflatesstats.matchesand makes it disagree with ripgrep (0 matches but non-zero matched lines).
self.file_stats.matched_lines += 1;
self.file_stats.matches += submatches.len().max(1) as u64;
- Files reviewed: 21/22 changed files
- Comments generated: 2
- Review effort level: Lite
Both crates inherit `version.workspace = true`, so the single root manifest edit covers tgrep-core and tgrep-cli; Cargo.lock is regenerated to match. The index format version in tgrep-core/src/meta.rs is deliberately left at 2. It is independent of the crate version, and bumping it would invalidate every existing on-disk index. Verified: release build reports `tgrep 1.0.2`, the full suite passes, and an indexed search over this repo returns results byte-identical to ripgrep. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 4e9d52fa-e7ec-4e5d-b9a9-f33c8cf582a7
Contributor
There was a problem hiding this comment.
Review details
Suppressed comments (1)
tgrep-cli/src/output.rs:572
- In JSON mode,
stats.matchesshould reflect the number of emittedsubmatches. Usingsubmatches.len().max(1)makesstats.matches(and thereforesearches_with_match) inconsistent with the actual payload for cases where amatchevent has zero submatches (e.g.,-v/--invert-match, where lines are selected but there are no match ranges).
self.file_stats.matched_lines += 1;
self.file_stats.matches += submatches.len().max(1) as u64;
- Files reviewed: 22/23 changed files
- Comments generated: 0 new
- Review effort level: Lite
`--no-require-git` is declared `global = true`, so `index` and `serve`
accepted it, but neither passed it down to the walk. Only the search path
read it. The flag was therefore silently inert on the two commands that
build the index:
tgrep index --no-require-git -> 56 files (flag dropped)
tgrep -l --no-require-git -> 5 files (flag honoured)
Besides ignoring the user's request, that left the index and the search
disagreeing about which files exist. Plumb the flag through BuildOptions,
WalkOptions and ServeOptions to every walk site.
`walk_file_metadata` never set `require_git` at all, so it silently
defaulted to gated. It feeds stale-file detection, so had it stayed gated
while the build honoured the flag, every ignored file would have looked
new on each server start. Its two boolean parameters became MetaWalkOptions
rather than a third bare bool, since adjacent booleans are easy to
transpose at a call site. `handle_reload` moves to build_index_with_options
so a reload cannot quietly change ignore semantics either.
The default stays git-gated: verified against rg 15.2.0 on the same
fixture, which also indexes 56 files without a `.git` and 5 with one. That
is ripgrep's behaviour and is not the bug.
The gate was, however, completely silent. On a non-git enlistment a
repo-root .gitignore does nothing, and the only symptom is an index far
larger than expected -- which is how this surfaced, on a 289k-file
Perforce tree. Print a warning naming the cause and the flag that fixes
it. It stays quiet inside a git repo, when no .gitignore exists, and when
the user has already passed --no-require-git or --no-ignore.
Adds 8 tests. Each was mutation-tested: reverting a change fails exactly
the test that covers it and no others.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 4e9d52fa-e7ec-4e5d-b9a9-f33c8cf582a7
Someone indexing a Perforce or Source Depot tree has no way to tell why their root .gitignore did nothing. Document the gate where they would look -- under Build the index -- along with the warning tgrep now prints and the --no-require-git escape hatch. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 4e9d52fa-e7ec-4e5d-b9a9-f33c8cf582a7
Contributor
There was a problem hiding this comment.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
tgrep-cli/src/main.rs:1120
report_missing_pathsets the globalERROREDflag, butlist_filesalways returnsOk(())even if one or more search paths were missing. That makestgrep --files <missing>exit 0 instead of 2, unlikerun_searchand unlike the intended ripgrep-compatible behavior described inreport_missing_path.
}
Ok(())
- Files reviewed: 23/24 changed files
- Comments generated: 0 new
- Review effort level: Lite
Contributor
There was a problem hiding this comment.
Review details
Suppressed comments (2)
Previously missed (1) — in code that hasn't changed since the last review.
tgrep-cli/src/main.rs:30
- The comment above
ERROREDis inaccurate:ERROREDis only set inreport_missing_path(hard IO errors), not “when anything is reported to stderr”. This is confusing because many non-fatal messages are printed to stderr (--stats, warnings, ignore-file parse errors) without settingERRORED, and ripgrep’smessagesmodule distinguishes between fatalerr_message!and non-fatalignore_message!.
/// Set when anything is reported to stderr, mirroring ripgrep's `messages`
/// module, which drives the exit code.
static ERRORED: AtomicBool = AtomicBool::new(false);
tgrep-core/src/walker.rs:452
walk_file_metadatastill hard-codes a 1 MiB size cap (DEFAULT_MAX_FILE_SIZE). Now that indexing can be configured viaBuildOptions::max_file_size/--max-filesize, this metadata walk can drift from the index’s file set (e.g., an index built with a higher limit will write stamps for >1 MiB files, but a later stale check that useswalk_file_metadatawill skip them and may treat them as deleted). Consider plumbing the effective max-file-size intoMetaWalkOptions(or otherwise ensuring the metadata walk uses the same limit as the index build) so stale detection stays consistent.
if let Ok(meta) = entry.metadata() {
if meta.len() > DEFAULT_MAX_FILE_SIZE {
return ignore::WalkState::Continue;
- Files reviewed: 23/24 changed files
- Comments generated: 0 new
- Review effort level: Lite
`walk_file_metadata` hard-coded `DEFAULT_MAX_FILE_SIZE` while the indexing
walk took a configurable `max_file_size`. The startup stale check treats an
indexed file missing from that walk as deleted, so serving an index built
with a raised `--max-filesize` silently evicted every file above 1 MiB --
and flushed the eviction to disk, so the loss survived a restart.
Reproduced end to end before fixing:
tgrep index . --index-path idx --max-filesize 10M
-> Found 2 text files
tgrep serve --index-path idx
-> stale check: 0 changed, 0 new, 1 deleted
tgrep -l needle --index-path idx -> small.txt
rg -l needle -> small.txt, big.txt
`--max-filesize` is `global = true`, so `serve` accepted it and dropped it
-- the same shape as the `--no-require-git` bug in the previous commit.
Plumb it through ServeOptions and ServerState to the bootstrap build, the
background build and both metadata walks, so every walk a server runs
shares one cap.
`MetaWalkOptions::default` is hand-written because a derived Default would
make `max_file_size` `None`, and `None` means *no limit* -- inverting the
default rather than matching `WalkOptions::default`.
Reported by Copilot review on walker.rs; confirmed real rather than taken
on trust.
Tests: a core test over the three cap settings, a Default-parity test, and
a server-level regression test that builds with a raised cap, serves with
it, and asserts the large file is still searchable. The server test waits
for the stale check to finish before searching -- without that it can
outrun the check and pass whether or not the bug is present. All three
were mutation-tested: restoring the hard-coded cap fails them with
["small.txt"] against ["big.txt", "small.txt"].
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 4e9d52fa-e7ec-4e5d-b9a9-f33c8cf582a7
Contributor
There was a problem hiding this comment.
Review details
Suppressed comments (2)
Previously missed (1) — in code that hasn't changed since the last review.
tgrep-cli/src/output.rs:386
write_context_separatorclonescontext_separator(self.config.context_separator.clone()) on every call. Since this runs per emitted line, it creates avoidable allocations; borrowing theStringviaas_deref()is sufficient.
This issue also appears on line 521 of the same file.
let Some(sep) = self.config.context_separator.clone() else {
tgrep-cli/src/output.rs:523
write_matchcopies the (possibly large) match line into a newString(content.to_string()) even thoughm.contentalready owns the data andtrim_adjustreturns a&str. This adds an extra allocation and full memcpy per matching line, which can be a noticeable overhead on high-match-volume searches.
let (content, spans) = self.trim_adjust(&m.content, &m.spans);
let content = content.to_string();
match self.config.format {
- Files reviewed: 24/25 changed files
- Comments generated: 0 new
- Review effort level: Lite
An inverted search prints the lines the pattern did *not* hit, so those lines carry no match spans. We were forcing every emitted match event to count as at least one match, which reported `matches: 2` for a `-v` search where ripgrep 15.2.0 reports `matches: 0`. The two counters have to move together. `searches_with_match` was keyed off `matches`, so simply dropping the `.max(1)` would have taken it to 0 and claimed the file produced no output when it plainly did. It is now keyed off `matched_lines`, which counts emitted lines and is non-zero exactly when the file wrote something. Verified against real ripgrep: `rg --json -v alpha a.txt` reports matches 0, matched_lines 2, searches_with_match 1; tgrep now agrees. All single-file and directory JSON stats cases in the differential harness match. Note the `.max(1)` in matching.rs's `match_count()` is *not* the same bug and is deliberately left alone: `rg --count-matches -v` reports the matched-line count, not 0, so that clamp is what keeps `--count-matches` correct. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 4e9d52fa-e7ec-4e5d-b9a9-f33c8cf582a7
Contributor
There was a problem hiding this comment.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
tgrep-cli/src/output.rs:442
--max-columns-previewtruncation currently useslimitas a byte count intocontent, butlimitis defined (and checked above) ascontent.len() + terminator_len. For terminated lines (LF/CRLF), this means the preview can includeterminator_lenextra bytes compared to the limit semantics, which is inconsistent with the “including terminator” rule and can diverge from ripgrep’s preview length.
Use limit - terminator_len (saturating at 0) when computing the preview cut point.
if self.config.max_columns_preview {
let cut = floor_boundary(content, limit);
return Cow::Owned(format!(
"{} [... omitted end of long line]",
&content[..cut]
));
- Files reviewed: 24/25 changed files
- Comments generated: 0 new
- Review effort level: Lite
msftsiwei
approved these changes
Aug 21, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Replacing ripgrep with tgrep on large repos means tgrep has to behave the same way. This closes the behavioural gaps between the two.
Rather than reason about parity from ripgrep's source, I installed ripgrep 15.2.0 locally and built a differential-testing harness that runs identical arguments through both binaries and diffs the output. Every expectation below was confirmed empirically. Several of these divergences were invisible from reading the code.
Verification
rgDivergences fixed
src/→src/main.rsbutsrc→src\main.rs.-nand-Hdefaults are context-dependent. Line numbers only when stdout is a terminal (--column/--vimgrep/-pimply them;-band-A/-B/-Cdo not); filenames unless exactly one file was named. This is the most user-visible change —tgrep needle . | cut -d: -f1now behaves like ripgrep.-e/-f— once either supplies a pattern, every positional argument is a path.--index-pathwith a subdirectory or file argument silently returned nothing.--max-depthwas ignored on the indexed and server paths.-l,-cand-L); one named explicitly reports a note;--binary(new flag) promotes traversal to the explicit behaviour. A 3-stateFileOutcomereplaces aboolso "skipped" can no longer leak into--files-without-matchas "no match".--column/-b/--vimgrepreported offsets into lossily-decoded text rather than source bytes (a repaired byte becomes a 3-byte U+FFFD, so every column after it was wrong).-M/--max-columnsmeasures the line including its terminator — verified across LF, CRLF and unterminated final lines — and no longer silently drops over-long context lines. Message wording now matches ([Omitted long matching line]).--filesomitted binary files.Bugs caught reviewing the above
A review pass over the change set found three genuine issues, all fixed and covered by tests:
to_source_offsetunderflowed when an offset landed inside a U+FFFD, which--replaceon any file containing invalid UTF-8 reliably triggered. In release builds it silently wrapped a column to 0; through the daemon it killed a rayon worker and made the client fall back with a misleading "server unreachable".-roffsets. ripgrep reports positions in the rewritten line, so-omust accumulate each replacement's length delta — rg gives3, 11where tgrep gave3, 8.-Mterminator boundary, above.Fixing (1) properly meant reworking the replace-coordinate model: columns stay in source terms and are corrected by an explicit shift, so
-ron a latin-1 file now reports rg's column 6 rather than 8.Column mapping and the max-columns terminator are now shared by the local and server paths through a single helper, so the three search paths cannot drift apart again.
Known limits, documented in the README
tgrep decodes before searching — that's what makes the trigram index possible — so on lines that aren't valid UTF-8 a pattern can match the substituted U+FFFD, which ripgrep (searching raw bytes) never does.
--jsonlikewise emitslines.textwith substitutions where ripgrep emits base64lines.bytes. Both follow from the design rather than being oversights; columns are clamped so output stays well-formed either way.Notes for reviewers
tgrep-cli/tests/ripgrep_parity.rsis new: a regression suite organised into 13 sections, each commented with thergbehaviour it pins so expectations can be re-verified without re-deriving them.main(including Bound posting decode by index.bin, not by untrusted lookup.bin fields #99); no file overlap, and the full suite plus the harness were re-run after rebasing.Follow-up: a search regression this PR introduced, and the benchmark refresh that caught it
Re-running the benchmark workflows on this branch showed search was ~1.3x slower than
main. That was real, and caused by this PR. Three separate causes, all fixed in9f48525:--vimgrep/-o/-r/--column,collect_hitsstarted callingfind_iteron every matching line — all spans plus aVecallocation per line — wheremaincalledfind_startand stopped at the first match. Most searches never look at spans past the first.{type,file,line,content}to also carryspans,columns,offsetandterm. The client parsed and then discarded them.lossy_utf8dids.to_string()even when the input was already valid UTF-8.The fix makes the extra work opt-in. The client computes
wants_match_detail()/wants_position_detail()from the actual flags and sendsdetailandpositionsin the request; the server skips computing and serializing what wasn't asked for, andMatchOptions::all_spansgates the local path the same way. Both params parse withunwrap_or(true), so an older client against a newer server still gets everything.Measured with an alternating min-of-N harness (single runs were useless — this machine has ~24% background load, and the same binary on the same query read 152ms and 360ms minutes apart): main 8,978ms / before 11,888ms (1.32x) / after 9,802ms (1.09x). The residual is fixed per-process startup. Re-verified with 506 tests and 378/378 differential checks against real
rg.Benchmarks refreshed (
9e3fec3)All 12 workflows were re-run on the fixed branch, with a control sweep on
mainat the same time — GitHub runner hardware drifts too much to trust a comparison against previously committed numbers. BENCHMARKS.md and README.md now carry the measured 18-cell matrix, and every published figure was cross-checked against the raw artifacts.Geometric mean speedup vs ripgrep: 12.4x Windows, 9.2x macOS, 2.7x Linux (the linux row was re-measured later in this PR — see below).
Earlier revisions of this description reported torvalds/linux as the one cell where ripgrep won (~1.3x on Linux). That finding was real, and the section below explains what it turned out to be measuring.
Follow-up: the
--statsbug, and making the kernel benchmark measure search--statswas counting the wrong thing, twiceThe note at the end of the previous revision — that
--statsreports a count taken before the path filter — was right, but understated. It was two bugs.--statsread the server'snum_matches, which is the number of rows the server built for the whole indexed tree. The client scopes those rows before printing, so:tgrep --stats "fn main" tgrep-core/srcprinted 6 matches and then claimed 90;-C 2inflated the same search from 90 to 445.The fix counts the rows the client actually prints, summing spans per row and de-duplicating
(file, line)for the matched-line total, so the stats and the output cannot disagree by construction.The subtle case is
-v: an inverted match prints lines that did not match, so ripgrep reports 0 matches with a non-zero matched-line count. My first attempt usedspans.len().max(1)and got this wrong; a 18-case differential sweep againstrg 15.2.0caught it. All 18 now agree exactly, plus 8 more cross-checked on the Linux kernel. 9 unit tests cover the counting logic.num_matchesstays on the wire — 23 integration assertions read it — and is now documented as a whole-tree row count.Two more parity bugs, from review
Both were flagged as previously missed, both confirmed against real
rgbefore fixing, and both have regression tests that were verified to fail when the old behaviour is restored.tgrepalways printedThe system cannot find the path specified. (os error 3)— wrong on Unix, and wrong on Windows too when the missing thing is a file (os error 2, different wording). It now asks the OS and uses ripgrep's phrasing; both cases are byte-identical torgapart from the binary name.stats.elapsedwas cumulative. It was read from the writer's overall start, so per-file times climbed monotonically (5.3ms → 9.6ms over 14 files) wherergreports an unordered 0.05–0.25ms each.The kernel query set was measuring result delivery, not search
The suite's queries were generic tokens that match most of the kernel. Measured against a local checkout, all 102:
^#define\s+[A-Z_]+)At roughly 5µs to serialize, ship, deserialize and print each match, those queries were timing tgrep's IPC throughput rather than its search. Replaced with queries a kernel developer would actually run —
devm_platform_ioremap_resource,netif_napi_add,blk_mq_alloc_tag_set— chosen from measured selectivity rather than guessed, in the spirit of the chromium set. All 102 were checked to compile underrg, and a sample was cross-checked againstrg --statson the full tree (7 of 8 exact, worst divergence 0.11%).Re-measured
Linux and macOS were run twice; the tables quote the more conservative run.
The number worth reading is not the ratio but the control: ripgrep's Linux total barely moved (45.4s → 42.3s — it scans every file whichever pattern it gets), while tgrep's fell from 60.2s to 7.5s. The gap between those two facts is the per-match delivery cost, isolated. Conversely, macOS ripgrep totals shifted a lot between measurement sessions (96s–221s before, ~497s now on an unchanged repo), so that column carries runner variance the Linux column does not — worth knowing before quoting 27x.
tgrep now wins all 18 cells, so Where tgrep loses no longer describes the matrix. It is replaced by What decides the margin, which keeps the match-volume finding — it is the useful part — and cites the old kernel numbers as its evidence rather than quietly dropping the case where tgrep lost.
All 198 published figures were re-checked against the raw artifacts by the verification script. Rewriting its range assertions exposed that three of them had been dead code, counting toward the total without asserting anything; those are now real checks, and the script was re-mutation-tested (22 mismatches reported on deliberately corrupted data).
Follow-up: binary offsets, and a final benchmark refresh
368b294— binary offsets are positions in the file, not in the repaired textCopilot flagged that the server's binary marker used an offset into the lossily-decoded text rather than the file. That was right, and the problem was broader than the comment: the local search path — the one users actually hit by default — had the identical bug, and
stats.bytes_searchedwas wrong on the same principle.Verified against real
rg 15.2.0on a fixture ofFF FF+NEEDLE\n+NUL:binary_offsetbytes_searched(with NUL)bytes_searched(no NUL, 14-byte file)LossyFixups::to_source_offsetalready existed and was already applied to columns,--byte-offsetandabsolute_offset— these two were simply the ones that missed the convention. Fixed at all three sites.Worth recording: one of the four regression tests I wrote passed with the fix reverted. Mutation testing caught it. Root cause is that
bypass_indexis set for--binaryand for named-file invocations, so the server's binary marker is not currently reachable end-to-end from the CLI; that path is now covered by a direct unit test onsearch_file_matchesinstead of a vacuous integration test. The remaining three fail individually when their corresponding fix is reverted, and each fix has exactly one failing test group.42f3fb3— benchmark refreshAll 12 workflows re-run on the branch. tgrep still wins all 18 cells; nothing regressed.
Geometric means: 12.6x Windows, 8.4x macOS, 2.5x Linux. Headline peak 55x → 58x.
Two corrections the refresh forced, both against tgrep's favour:
1.1xwhen it measured1.06x— a 4% overstatement. The verification script's own 3% tolerance is what caught it. Cells below 2x now carry two decimals.45.4s → 42.3s) as evidence that ripgrep is insensitive to the query set. That claim is better supported by the band across all six runs we now have, 28–46s, so it says that instead. The macOS variance caution previously compared runs from different query sets (96s vs 500s), which was not a like-for-like comparison; it now cites the three same-suite runs (388s, 495s, 500s — a 1.3x spread).The verification script was extended to cover the hand-written kernel prose — the run-to-run ratios, the ripgrep band, the tgrep old/new bands and the macOS spread are all now re-derived from the raw artifacts rather than trusted. 214 published figures are machine-checked, and the new checks were mutation-tested: six deliberate corruptions produced exactly six mismatches, one per check.