huggingface / huggingface/serge

Consider ripgrep for the grep tool instead of git grep

Open
#94 0 comments 0 reactions 0 assignees View on GitHub
Dominant language
Python
Stars
51
Forks
10
Avg merge
11h 32m
Merged PRs (30d)
36

Description

### Context

The `grep` tool in `reviewbot/tools.py` shells out to `git grep`. Fixing three
silent-wrong-answer bugs in it recently meant hand-rolling behaviour that
ripgrep provides directly, which is worth weighing as a follow-up rather than
more patches on top of `git grep`:

- **Per-file vs. overall result caps.** `git grep --max-count` is per *file*
with no signal in the output, so the overall cap has to be enforced by
streaming stdout and stopping one line past it just to learn whether more
matches exist. `rg --json` reports match counts and a `summary` event
directly, so "is this the complete set?" stops being an inference.
- **Long match lines.** A minified bundle line could spend the whole 8 KB
output budget, so match lines are now clipped at 300 chars by hand.
`rg --max-columns=N --max-columns-preview` does this natively.
- **Regex dialect.** `-E` is POSIX ERE: `\d`, `\w`, `\b`, `(?...)` match
nothing rather than erroring, so a model writing PCRE by habit gets a
confident `no matches`. That now needs `-P` plus a runtime fallback for gits
built without PCRE2, plus a note explaining the dialect. `rg` is Rust-regex
by default — `\d`, `\b`, non-greedy all work, with no build-dependent
behaviour to detect.
- **Path scoping.** The `path` argument is currently passed to git as a
*pathspec*, so glob metacharacters get pathspec semantics rather than being a
literal path. `rg -g ` separates the two.

### What `git grep` is doing well

Worth keeping in mind, because it is the reason for the current design: the
tracked-file set is a free ignore list. It skips `.git` internals, vendored
directories and generated-but-gitignored output, which is roughly what
`DENY_DIR_NAMES` exists to enforce for `read_file` / `list_dir`. `rg` respects
`.gitignore` by default and lands in nearly the same place, but the two sets are
not identical — `rg` also sees untracked non-ignored files (which is what we
want for `/tasks`, and what `--untracked` was just added for).

### Cost

- One more binary in the image (`apt-get install -y ripgrep`, or the static
release tarball to pin a version).
- `rg` must be optional or probed: the tool is also exercised in unit tests and
local runs where it may be absent, so a `git grep` fallback path would have
to stay.
- `--json` output means parsing events instead of `path:line:text`, so
`_stream_grep` changes shape (arguably for the better — the per-line
formatting becomes explicit rather than inherited from git's output format).

### Suggested scope

1. Probe for `rg` once per process; keep the `git grep` implementation as the
fallback so nothing regresses where `rg` is missing.
2. Use `--json` for exact match counts, `--max-columns` for line clipping,
`-g` for path scoping.
3. Keep the current output contract exactly as it is: `path:line:text`, an
explicit note when the result is truncated, and no note when it is complete
(the model is told it can count an unannotated result and rely on the total).
4. Keep the `DENY_DIR_NAMES` filter on match paths — grep should not return a
path `read_file` will refuse to open, whichever engine found it.

Not urgent: the current implementation is correct now. This is about deleting
workarounds, and about the regex dialect no longer depending on how git was
built.

Contributor guide

No contributing guide indexed for this repository

Research direction

Start in reviewbot/tools.py, especially the grep implementation and _stream_grep, then inspect the existing unit tests and how local runs provide external binaries. Compare the proposed rg JSON, clipping, path scoping, fallback, and DENY_DIR_NAMES requirements with the current output contract. Done means rg is used when available, git grep remains a working fallback, and existing tests still pass with unchanged output and truncation behavior.

Written by the indexing model from the issue text.

Assessment

Tech stack
git, python
Domain
tooling
Issue type
Feature
Difficulty
4/5
Estimated time
3-5 days
Activity status
Active
Clarity
Mostly clear
Newbie friendliness
48/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.