Repository navigation
Fix passthru output when no lines match - #128
Merged
Merged
Conversation
Contributor
There was a problem hiding this comment.
Copilot review overview
Review tier: Balanced
Findings: 1
New issues introduced by this change (3)
| Severity | Finding |
|---|---|
 |
tgrep-cli/tests/ripgrep_compat.rs — This test does not exercise the indexed path: naming a file makes run set bypass_index and call… |
|
tgrep-cli/tests/ripgrep_compat.rs — Passing this explicit file also triggers bypass_index before server discovery… |
 |
tgrep-cli/src/serve.rs — MatchAll applies to the server's entire index, because the request does not carry the CLI's… |
What changed in this PR
Fixes --passthru so eligible text files are emitted even without matches while preserving exit status and binary behavior.
Changes:
- Uses full indexed-file enumeration for passthru.
- Emits no-hit files through context output.
- Adds local, indexed, binary, and server-oriented tests.
| File | Description |
|---|---|
tgrep-cli/src/search.rs |
Handles no-hit passthru output and exit status. |
tgrep-cli/src/serve.rs |
Adds server-side full-file passthru handling. |
tgrep-cli/tests/ripgrep_compat.rs |
Adds passthru regression coverage. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Contributor
There was a problem hiding this comment.
Copilot review overview
Review tier: Balanced
Findings: 1
Pre-existing issues (3)
| Severity | Finding |
|---|---|
|
tgrep-cli/src/serve.rs — MatchAll applies to the server's entire index, because the request does not carry the CLI's… View comment |
|
tgrep-cli/tests/ripgrep_compat.rs — Passing this explicit file also triggers bypass_index before server discovery… View comment |
|
tgrep-cli/tests/ripgrep_compat.rs — This test does not exercise the indexed path: naming a file makes run set bypass_index and call… View comment |
Suppressed comments (4)
tgrep-cli/tests/ripgrep_compat.rs:2730
- A file target bypasses server delegation entirely (
search.rs:492-528), so the running daemon is never queried and the “Server unreachable” assertion passes vacuously. Target the one-file root directory instead so this exercises the new server context-row response and client exit-status calculation.
path.to_str().unwrap(),
tgrep-cli/tests/ripgrep_compat.rs:2743
- Because
pathresolves to a file, the indexed and server iterations both bypass their advertised paths and run brute-force search (search.rs:492-528). Point the shared target atrootso the test actually validates-m 0through local-index and server request handling.
let path = root.join("hello.rs");
let path = path.to_str().unwrap();
tgrep-cli/tests/ripgrep_compat.rs:2596
- This target is a single file, so
search::runsetsbypass_indexand executes the brute-force path (search.rs:492-528). As a result, this test does not cover the changed local-index candidate selection or the no-matchFileOutcomehandling it is named for. Use a directory target and assert that all eligible files are emitted while the process still exits 1.
path.to_str().unwrap(),
tgrep-cli/src/serve.rs:1294
- Making passthru a
MatchAllserver query causes the daemon to search its entire index even for a subtree request, materialize every file line into nested vectors, serialize one large JSON response, and only then let the client discard out-of-scope rows (serve.rs:1514-1554,search.rs:692-734). For large repositories this makes memory and network usage scale with the full repository and can exhaust the daemon/client. Bypass server delegation for effective passthru, or add server-side scope plus streaming/bounded responses.
let plan = if passthru || invert_match || encoding.may_differ_from_index() {
query::QueryPlan::MatchAll
Co-authored-by: Copilot App <[email protected]>
Co-authored-by: Copilot App <[email protected]>
Co-authored-by: Copilot App <[email protected]> Copilot-Session: c4fb5038-6356-4740-aab0-2216d80e27bd
Shengyu Fu (shengyfu)
force-pushed
the
shengyfu-fix-passthru-no-match
branch
from
September 2, 2026 23:50
82673a2 to
9956009
Compare
Contributor
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Server bypass can omit live additions and emit tombstoned or newly ignored files from a stale on-disk index.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review tier: Balanced
Findings: 1
New issues introduced by this change (1)
| Severity | Finding |
|---|---|
|
tgrep-cli/src/search.rs — Bypassing the daemon here makes --passthru discover candidates only from the on-disk… |
Issues resolved since last review (3)
| Severity | Finding |
|---|---|
|
tgrep-cli/src/serve.rs — MatchAll applies to the server's entire index, because the request does not carry the CLI's… View resolved comment |
|
tgrep-cli/tests/ripgrep_compat.rs — Passing this explicit file also triggers bypass_index before server discovery… View resolved comment |
|
tgrep-cli/tests/ripgrep_compat.rs — This test does not exercise the indexed path: naming a file makes run set bypass_index and call… View resolved comment |
| // never reports. | ||
| if !opts.no_index | ||
| && !bypass_index | ||
| && !opts.effective_passthru() |
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.
Summary
--passthrusearches through every eligible indexed fileTesting
cargo test --test ripgrep_compat passthru -- --nocapturecargo test --test ripgrep_compat new_flags_agree_across_search_paths -- --nocapturecargo fmt --all -- --checkcargo clippy --workspace --all-targets -- -D warningscargo test --workspaceCloses #126