Skip to content

fix(search): treat -f/--file as a pattern source, not a positional pa… - #3666

Closed
dwin-gharibi wants to merge 1 commit into
rtk-ai:developfrom
dwin-gharibi:fix/grep-pattern-file-flag
Closed

dwin-gharibi wants to merge 1 commit into
rtk-ai:developfrom
dwin-gharibi:fix/grep-pattern-file-flag

Conversation

@dwin-gharibi

Copy link
Copy Markdown

Closes #3665.

Summary

  • Treat -f / --file as a pattern source in extract_pattern_path, exactly like
    -e / --regexp: when one is present every positional is a path, not a pattern.
  • Fixes rtk grep -f patterns.txt app.log silently returning no matches (exit 1) where real
    grep returns matches — and, with two files, returning matches from the wrong file.
  • The flag is tracked during the arg scan rather than by inspecting flags afterwards, so
    another option's value that happens to read -f cannot be mistaken for it.

With no positional pattern left, run() takes its existing patterns.is_empty() passthrough
branch and executes the user's command verbatim — correct output, no compression on this form.
Grouping -f searches is a follow-up; correctness first.

Test plan

  • cargo fmt --all && cargo clippy --all-targets && cargo test
  • Manual testing: rtk <command> output inspected

Unit tests (added, TDD — red before green)

src/cmds/system/search.rs:

  • pattern_file_flag_leaves_positionals_as_paths — all four spellings (-f F, --file F,
    --file=F, -fF) yield no positional pattern and keep the search path.
    Fails on develop with: ["-f", "pats.txt", "hay.txt"]: a pattern file must not turn a positional into the pattern, got ["hay.txt"].
  • pattern_file_flag_is_forwarded_to_the_engine — -f and its value still reach the engine.
  • positional_pattern_still_wins_without_a_pattern_source — guards the ordinary form.

Behavioural check against real grep

Every case below now produces byte-identical output and exit code to GNU grep:

Command Before After
grep -f patterns.txt app.log no output, exit 1 3 lines, exit 0 — identical
grep --file patterns.txt app.log no output, exit 1 identical
grep --file=patterns.txt app.log no output, exit 1 identical
grep -fpatterns.txt app.log no output, exit 1 identical
grep -f patterns.txt app.log second.log wrong file's match 4 lines, identical
grep -n -f patterns.txt app.log no output, exit 1 identical

Controls confirming ordinary grep is untouched: grep -n ERROR, grep -c ERROR,
grep -e ERROR -e WARN, grep ERROR a b — all identical before and after.

…ttern

grep and rg both read patterns from a file with -f/--file, the file-based
twin of -e. extract_pattern_path classified them as generic value-taking
flags, so the first positional -- the file to search -- was consumed as the
pattern and the path list was left empty, making the engine read stdin.

rtk grep -f patterns.txt app.log returned no output and exit 1 where grep
returns three matches; with two files it returned matches from the wrong
file. Both are silent: nothing in the exit code or stderr distinguishes them
from a genuine no-match.

Track the flag during the arg scan rather than inspecting flags afterwards,
so another option's value cannot be mistaken for it.
Copilot AI lite review requested due to automatic review settings August 23, 2026 18:03

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@devnulled

Copy link
Copy Markdown
Contributor

This also fixes the rg engine, where the bug is worse than the grep case
described here — I built the branch and measured it. Flagging because the rg
half isn't mentioned in the description or pinned by a test, and it's the half
that fails silently.

On grep, -f stealing the path gives an empty result and exit 1. On rg it gives
a larger wrong answer at exit 0, because rg recurses the working
directory when the path list comes back empty. Same exit code as a correct run,
plausible output, wrong result set.

Fixture — three files; the question asked is "which lines of hay.txt match the
patterns in pats.txt":

./hay.txt        alpha FOO one / beta bar / gamma FOO three
./pats.txt       FOO
./sub/other.txt  unrelated FOO in another file

Three-way, all four -f spellings, rg engine:

$ rg -f pats.txt hay.txt                    # real ripgrep 15.2.0
alpha FOO one
gamma FOO three
exit=0

$ rtk rg -f pats.txt hay.txt                # v0.48.0
FOO                                         <-- from pats.txt, not searched
alpha FOO one
gamma FOO three
unrelated FOO in another file               <-- from sub/, not searched
exit=0

$ rtk rg -f pats.txt hay.txt                # this branch
alpha FOO one
gamma FOO three
exit=0

The extra lines come from files the caller never named — and note it matched
inside pats.txt itself, so the pattern file contaminated its own results.

Every form reaches parity with the real engine on this branch, and every one of
them is broken on v0.48.0:

form engine v0.48.0 this branch
-f pats.txt hay.txt rg 4 lines, exit 0 parity
--file pats.txt hay.txt rg 4 lines, exit 0 parity
--file=pats.txt hay.txt rg 4 lines, exit 0 parity
-fpats.txt hay.txt rg 4 lines, exit 0 parity
-f pats.txt hay.txt grep empty, exit 1 parity
--file pats.txt hay.txt grep empty, exit 1 parity
-fpats.txt hay.txt grep empty, exit 1 parity

cargo test --all on the branch: 2707 passed, 0 failed.

Measured on debug builds of 88a50e9 (this branch) and fde0a8f (v0.48.0),
macOS, against /opt/homebrew/bin/rg and /usr/bin/grep by absolute path. That
last detail matters for anyone re-running it: in some agent shells grep is a
shell function routing to a bundled ugrep, and rg gets rewritten by rtk's own
hook, so a differential using bare names quietly compares rtk against rtk.

The one gap: pattern_file_flag_leaves_positionals_as_paths exercises
extract_pattern_path directly, so nothing pins the rg-side end-to-end
behavior. Since the rg failure is exit 0 with plausible output, a regression
there can't be caught by watching for an error — a case asserting that
rtk rg -f pats.txt hay.txt returns exactly the engine's own lines would close
it. Killing mutation for such a test: revert the condition on line 252 to
!e_patterns.is_empty().

@KuSh

KuSh commented Sep 19, 2026

Copy link
Copy Markdown
Collaborator

Thanks for the precise diagnosis and the four-spelling test table. This landed on develop in #3681 (merged 2026-09-12): extract_pattern_path now tracks a patterns_from_file flag during the scan and, as in your patch, treats every positional as a path when -f or --file is present, with a test covering -f F, --file F, -fF and --file=F. That PR also rebuilt the function on the shared argument tokenizer, so the hand-rolled flag scan your hunks edit is no longer in the file and they can no longer apply. Issue #3665 is still open only because it has not been verified against a release yet. Closing this, but please comment or reopen if you find a form that develop still gets wrong.

@KuSh KuSh closed this Sep 19, 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.

rtk grep -f patterns.txt file searches the wrong input and reports no matches

4 participants