fix(fcontent): output improvements — exit code + path deduplication#5
Conversation
Pretty mode fell off the end of the script without an explicit exit, causing the last arithmetic test (( _METACHAR_WARNING == 1 )) to set the exit code to 1 when no warning was needed. JSON and paths modes already had exit 0; pretty was the only mode missing it.
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughThe changes enhance output formatting for ripgrep matches in fcontent by introducing intelligent path printing. When processing filename:line:content formatted lines, the code now prints full paths for new files and baselines for repeated files, with per-line formatting tied to a QUIET flag and a fallback mechanism for unmatched formats. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@fcontent`:
- Around line 475-478: The regex ^(.+):([0-9]+):(.*) is greedy and mis-parses
lines (corrupting _cur_file/_cur_line/_cur_text), so replace the re-parsing here
with a lookup into the data you already parsed earlier (the pass that sets those
values around lines 393-398); build an associative array (e.g., declare -A
parsed_lines) during that first pass keyed by the original rg line (or its
unique id) to store filename and line number, and here populate
_cur_file/_cur_line/_cur_text from that map instead of re-running the greedy
regex — keep the same variable names (_cur_file, _cur_line, _cur_text) so
downstream deduplication and basename logic continue working.
- Around line 473-504: The deduplication logic using _PREV_FILE fails when rg
output is not grouped by file (e.g., when users pass --rg-args sorting), so
replace the consecutive-only tracker with a persistent set: declare -A
_SEEN_FILES and mark files as seen when you first print the full-path line; then
for each MATCH_LINES entry (same loop using variables _cur_file, _cur_line,
_cur_text and QUIET), check _SEEN_FILES["$_cur_file"] to decide whether to print
the full path (and set _SEEN_FILES["$_cur_file"]=1) or print the basename-only
line; keep the existing fallback branch for non-matching lines unchanged.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
fcontent
🧰 Additional context used
📓 Path-based instructions (1)
fcontent
⚙️ CodeRabbit configuration file
fcontent: Focus on: rg invocation safety (args handling), stdin file list handling,
truncation accounting (SHOWN_MATCHES/TOTAL_FILES), and JSON/pretty output
correctness. Avoid changes that could leak private paths in examples.
Files:
fcontent
🔇 Additional comments (2)
fcontent (2)
507-507: Exit code fix is correct and necessary.Without this
exit 0, when_METACHAR_WARNINGis0, line 506's(( 0 )) && ...evaluates to exit code 1. Whileset -ewon't trigger (failure is in a&&list), the EXIT trap on line 324 captures$?, so the script previously exited with code 1 on successful searches that had no metachar warning. This correctly resolves that.
482-486: Quiet vs non-quiet indentation is inconsistent for visual alignment.In non-quiet mode, new-file lines get 5-space indent (
" ", line 485) while same-file lines get 7-space indent (" ", line 493). The 2-space difference visually groups repeated matches under their file, which seems intentional. Just confirming this is deliberate, since it's not documented.Also applies to: 490-494
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
fcontent
Outdated
| _PREV_FILE="" | ||
| for line in "${MATCH_LINES[@]}"; do | ||
| if (( QUIET == 1 )); then | ||
| echo "$line" | ||
| if [[ "$line" =~ ^(.+):([0-9]+):(.*) ]]; then | ||
| _cur_file="${BASH_REMATCH[1]}" | ||
| _cur_line="${BASH_REMATCH[2]}" | ||
| _cur_text="${BASH_REMATCH[3]}" | ||
| if [[ "$_cur_file" != "$_PREV_FILE" ]]; then | ||
| # New file — show full path | ||
| _PREV_FILE="$_cur_file" | ||
| if (( QUIET == 1 )); then | ||
| echo "$line" | ||
| else | ||
| echo " ${_cur_file}:${_cur_line}:${_cur_text}" | ||
| fi | ||
| else | ||
| # Same file — show only basename:line:content | ||
| _basename="${_cur_file##*/}" | ||
| if (( QUIET == 1 )); then | ||
| echo "${_basename}:${_cur_line}:${_cur_text}" | ||
| else | ||
| echo " ${_basename}:${_cur_line}:${_cur_text}" | ||
| fi | ||
| fi | ||
| else | ||
| echo " $line" | ||
| # Fallback: line doesn't match expected format | ||
| if (( QUIET == 1 )); then | ||
| echo "$line" | ||
| else | ||
| echo " $line" | ||
| fi | ||
| fi | ||
| done |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Deduplication assumes rg output is grouped by file — breaks with --rg-args "--sort path" or --rg-args "--sort-files".
The _PREV_FILE tracker only deduplicates consecutive matches from the same file. Default rg groups matches by file, so this works for normal usage. However, if a user passes sorting or other reordering flags via --rg-args, matches from the same file could become non-contiguous, causing basename-only lines to appear without a preceding full-path line for context.
This is a minor edge case since --rg-args is an advanced option, but worth noting. An associative array (declare -A _SEEN_FILES) would handle arbitrary ordering if you want robustness:
Sketch using associative array
-_PREV_FILE=""
+declare -A _SEEN_FILES
for line in "${MATCH_LINES[@]}"; do
if [[ "$line" =~ ^(.+):([0-9]+):(.*) ]]; then
_cur_file="${BASH_REMATCH[1]}"
_cur_line="${BASH_REMATCH[2]}"
_cur_text="${BASH_REMATCH[3]}"
- if [[ "$_cur_file" != "$_PREV_FILE" ]]; then
- # New file — show full path
- _PREV_FILE="$_cur_file"
+ if [[ -z "${_SEEN_FILES[$_cur_file]+x}" ]]; then
+ _SEEN_FILES[$_cur_file]=1
if (( QUIET == 1 )); then
echo "$line"
else📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| _PREV_FILE="" | |
| for line in "${MATCH_LINES[@]}"; do | |
| if (( QUIET == 1 )); then | |
| echo "$line" | |
| if [[ "$line" =~ ^(.+):([0-9]+):(.*) ]]; then | |
| _cur_file="${BASH_REMATCH[1]}" | |
| _cur_line="${BASH_REMATCH[2]}" | |
| _cur_text="${BASH_REMATCH[3]}" | |
| if [[ "$_cur_file" != "$_PREV_FILE" ]]; then | |
| # New file — show full path | |
| _PREV_FILE="$_cur_file" | |
| if (( QUIET == 1 )); then | |
| echo "$line" | |
| else | |
| echo " ${_cur_file}:${_cur_line}:${_cur_text}" | |
| fi | |
| else | |
| # Same file — show only basename:line:content | |
| _basename="${_cur_file##*/}" | |
| if (( QUIET == 1 )); then | |
| echo "${_basename}:${_cur_line}:${_cur_text}" | |
| else | |
| echo " ${_basename}:${_cur_line}:${_cur_text}" | |
| fi | |
| fi | |
| else | |
| echo " $line" | |
| # Fallback: line doesn't match expected format | |
| if (( QUIET == 1 )); then | |
| echo "$line" | |
| else | |
| echo " $line" | |
| fi | |
| fi | |
| done | |
| _SEEN_FILES=() | |
| declare -A _SEEN_FILES | |
| for line in "${MATCH_LINES[@]}"; do | |
| if [[ "$line" =~ ^(.+):([0-9]+):(.*) ]]; then | |
| _cur_file="${BASH_REMATCH[1]}" | |
| _cur_line="${BASH_REMATCH[2]}" | |
| _cur_text="${BASH_REMATCH[3]}" | |
| if [[ -z "${_SEEN_FILES[$_cur_file]+x}" ]]; then | |
| # New file — show full path | |
| _SEEN_FILES[$_cur_file]=1 | |
| if (( QUIET == 1 )); then | |
| echo "$line" | |
| else | |
| echo " ${_cur_file}:${_cur_line}:${_cur_text}" | |
| fi | |
| else | |
| # Same file — show only basename:line:content | |
| _basename="${_cur_file##*/}" | |
| if (( QUIET == 1 )); then | |
| echo "${_basename}:${_cur_line}:${_cur_text}" | |
| else | |
| echo " ${_basename}:${_cur_line}:${_cur_text}" | |
| fi | |
| fi | |
| else | |
| # Fallback: line doesn't match expected format | |
| if (( QUIET == 1 )); then | |
| echo "$line" | |
| else | |
| echo " $line" | |
| fi | |
| fi | |
| done |
🤖 Prompt for AI Agents
In `@fcontent` around lines 473 - 504, The deduplication logic using _PREV_FILE
fails when rg output is not grouped by file (e.g., when users pass --rg-args
sorting), so replace the consecutive-only tracker with a persistent set: declare
-A _SEEN_FILES and mark files as seen when you first print the full-path line;
then for each MATCH_LINES entry (same loop using variables _cur_file, _cur_line,
_cur_text and QUIET), check _SEEN_FILES["$_cur_file"] to decide whether to print
the full path (and set _SEEN_FILES["$_cur_file"]=1) or print the basename-only
line; keep the existing fallback branch for non-matching lines unchanged.
fcontent
Outdated
| if [[ "$line" =~ ^(.+):([0-9]+):(.*) ]]; then | ||
| _cur_file="${BASH_REMATCH[1]}" | ||
| _cur_line="${BASH_REMATCH[2]}" | ||
| _cur_text="${BASH_REMATCH[3]}" |
There was a problem hiding this comment.
Greedy .+ regex can mismatch when file content starts with digits:.
^(.+):([0-9]+):(.*) is greedy, so for rg output like /path/file:10:42:actual content, the regex backtracks and captures group 1 as /path/file:10, group 2 as 42, group 3 as actual content — wrong file and line number.
This is a pre-existing pattern (same regex on line 395), so not introduced here. But since this PR adds deduplication that depends on _cur_file being correct, the mismatch would now cause visible bugs: the wrong "file" string means deduplication groups incorrectly and basenames are derived from a corrupted path.
A non-greedy or anchored approach is hard in bash regex. One pragmatic fix: reuse the filename extracted on line 393-398 (which already parsed the same lines) by building an associative map during that pass instead of re-parsing here. Or accept the known limitation with a comment.
🤖 Prompt for AI Agents
In `@fcontent` around lines 475 - 478, The regex ^(.+):([0-9]+):(.*) is greedy and
mis-parses lines (corrupting _cur_file/_cur_line/_cur_text), so replace the
re-parsing here with a lookup into the data you already parsed earlier (the pass
that sets those values around lines 393-398); build an associative array (e.g.,
declare -A parsed_lines) during that first pass keyed by the original rg line
(or its unique id) to store filename and line number, and here populate
_cur_file/_cur_line/_cur_text from that map instead of re-running the greedy
regex — keep the same variable names (_cur_file, _cur_line, _cur_text) so
downstream deduplication and basename logic continue working.
Cwilliams333
force-pushed
the
fix/fcontent-output-improvements
branch
from
b48ad93 to
7a39524
Compare
|
Hey — nice work on the path dedup and the exit 0 fix. Both are solid. The One change we'd like before merging: swap Why: The consecutive-only tracker breaks if someone passes
Here's the sketch: -_PREV_FILE=""
+declare -A _SEEN_FILES
for line in "${MATCH_LINES[@]}"; do
if [[ "$line" =~ ^([^:]+):([0-9]+):(.*) ]]; then
_cur_file="${BASH_REMATCH[1]}"
_cur_line="${BASH_REMATCH[2]}"
_cur_text="${BASH_REMATCH[3]}"
- if [[ "$_cur_file" != "$_PREV_FILE" ]]; then
- _PREV_FILE="$_cur_file"
+ if [[ -z "${_SEEN_FILES[$_cur_file]+x}" ]]; then
+ _SEEN_FILES[$_cur_file]=1Everything else stays the same. Should be a quick swap. |
When multiple matches occur in the same file, only show the full path
for the first match. Subsequent matches show basename:line:content only.
This reduces token usage when the same file has many hits, making
output more compact for AI agent consumption.
Before:
/very/long/path/to/file.py:10:match1
/very/long/path/to/file.py:15:match2
/very/long/path/to/file.py:20:match3
After:
/very/long/path/to/file.py:10:match1
file.py:15:match2
file.py:20:match3
Cwilliams333
force-pushed
the
fix/fcontent-output-improvements
branch
from
7a39524 to
4623d40
Compare
|
Done — swapped Now handles non-contiguous file ordering (e.g., -_PREV_FILE=""
+declare -A _SEEN_FILES
for line in "${MATCH_LINES[@]}"; do
if [[ "$line" =~ ^([^:]+):([0-9]+):(.*) ]]; then
_cur_file="${BASH_REMATCH[1]}"
- if [[ "$_cur_file" != "$_PREV_FILE" ]]; then
- _PREV_FILE="$_cur_file"
+ if [[ -z "${_SEEN_FILES[$_cur_file]+x}" ]]; then
+ _SEEN_FILES[$_cur_file]=1Ready for re-review. |
2 participants
Summary
Two fixes for
fcontentoutput handling:exit 0in pretty output mode — script was falling through without explicit exitbasename:line:contentonlyWhy
The path deduplication reduces token usage when AI agents consume fcontent output. Same file with many hits no longer repeats the full path on every line.
Before:
After:
Test plan
-q) also dedupes correctly