Skip to content

fix(grep): adjust the command to fall through if the output would already be as small as possible#1456

Merged
aeppling merged 1 commit intortk-ai:developfrom
JBF1991:fix-grep-passthrough
May 1, 2026
Merged

fix(grep): adjust the command to fall through if the output would already be as small as possible#1456
aeppling merged 1 commit intortk-ai:developfrom
JBF1991:fix-grep-passthrough

Conversation

@JBF1991
Copy link
Copy Markdown
Contributor

@JBF1991 JBF1991 commented Apr 22, 2026

Summary

Fix rtk grep compatibility with standard grep format flags (-c, -l, -L, -o, -Z). These flags change output shape so fundamentally that RTK's grouped formatter produced garbage or ignored the flag entirely.

Root Cause

Test plan

All tests pass:

$ rtk cargo test --all
cargo test: 1680 passed, 6 ignored

Format flags now produce raw output:

Flag Command Output
-c rtk grep -c "fn run" src/cmds/system/grep_cmd.rs 1
-l rtk grep -l "fn run" src/cmds/system/grep_cmd.rs src/cmds/system/grep_cmd.rs
-L rtk grep -L "nonexistent" src/cmds/system/grep_cmd.rs src/cmds/system/grep_cmd.rs
-o rtk grep -o "fn" src/cmds/system/grep_cmd.rs fn\nfn\nfn...

Normal path unchanged:

$ rtk grep "fn run" src/cmds/system/grep_cmd.rs
1 matches in 1F:

[file] src/cmds/system/grep_cmd.rs (1):
    12: pub fn run(
  • cargo fmt --all && cargo clippy --all-targets && cargo test
  • Manual testing: rtk <command> output inspected

Important: All PRs must target the develop branch (not master).
See CONTRIBUTING.md for details.

@aeppling aeppling self-assigned this Apr 22, 2026
Copy link
Copy Markdown
Contributor

@aeppling aeppling left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Blockers

1. Missing --no-ignore-vcs in passthrough path

The duplicated rg command construction forgot --no-ignore-vcs (added by #791 to fix false negatives on gitignored files).

Result: rtk grep "pattern" . searches gitignored files, rtk grep -c "pattern" . does not. Same pattern, different results depending on output flag.

2. -l / --files-with-matches is dead code

Clap captures -l as max_len (#[arg(short = 'l', ...)]) before it reaches extra_args. Users get a parse error, not file listing. Same class of bug this PR fixed for -c.

Root cause: both bugs come from duplicating the command construction

The passthrough path copies ~25 lines of rg command setup instead of reusing it. Fix both bugs at once by moving the has_format_flag check to after execution, before formatting:

// existing rg command construction (unchanged, includes --no-ignore-vcs)
let result = exec_capture(&mut rg_cmd).or_else(|_| { /* grep fallback */ })?;

if has_format_flag(extra_args) {
    print!("{}", result.stdout);
    if !result.stderr.is_empty() {
        eprintln!("{}", result.stderr.trim());
    }
    timer.track_passthrough(...);
    return Ok(result.exit_code);
}

// existing formatter (unchanged)

No duplication, no flag drift, --no-ignore-vcs inherited automatically.

@JBF1991
Copy link
Copy Markdown
Contributor Author

JBF1991 commented Apr 26, 2026

I wasn't aware of #791 when I pushed this up. Sorry! I will update it accordingly.

@JBF1991 JBF1991 force-pushed the fix-grep-passthrough branch from 67ba4b9 to c873c6d Compare April 26, 2026 01:16
@JBF1991 JBF1991 force-pushed the fix-grep-passthrough branch from c873c6d to 021827c Compare April 26, 2026 01:39
@JBF1991
Copy link
Copy Markdown
Contributor Author

JBF1991 commented Apr 26, 2026

@aeppling I went ahead and made the change with slight tweaks to avoid empty spaces in the db when there was a pass-through.

@aeppling
Copy link
Copy Markdown
Contributor

aeppling commented May 1, 2026

LGTM, thanks for contributing @JBF1991

Will be released during the week

@aeppling aeppling merged commit 09e1c0a into rtk-ai:develop May 1, 2026
11 checks passed
@aeppling aeppling mentioned this pull request May 3, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants