Conversation
|
✅ OpenCodeReview: Review complete: 0 finding(s) across 2 selected item(s). |
|
|
|
@LiRunGuo Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution. |
|
#1306 is merged, please rebase main |
git grep refuses to run when -P cannot compile the pattern, so a term
like `Set("gorm:` failed the whole call and cost the model a round trip.
Retry it as a fixed string, keep the rest of the arguments, and say in
the result that the matches are literal.
|
@LiRunGuo ping again |
The note was reworded when the global result limit landed (alibaba#1306), but the tools guide and the tool description still quoted the old per-file wording. Describe the per-file max-count 101 probe and the global 100 match cap as they now behave.
5b73802 to
aa545d9
Compare
|
Sorry for the delay, and thanks for the ping. Rebased onto Resolving the #1306 conflict was small: it moved the truncation note to after the parse loop, and the literal note from this PR now sits directly above it, so both can appear in one result. The retry inherits the new While reviewing that merge I noticed the docs had drifted from #1306's merged behavior — the limit is now a global 100-match cap with a per-file Re-verified after the rebase: On the CLA: I will sign it as soon as the link is available to me. CI is waiting on maintainer approval for the fork run. |
Description
code_searchhands the model'ssearch_textstraight togit grep -Pwhenuse_perl_regexpistrue. If the term is not valid PCRE, git refuses to compile the pattern and exits 128 without searching anything:#1340 measured 31 of 348 and 11 of 236
code_searchcalls failing this way in single runs (about 9%). Each failure costs the model a round trip, and the model usually falls back to reading files in chunks — the term is almost always code text (Set("gorm:) that merely happens to be unbalanced as a regex.This PR retries the same search as a fixed string when git rejects the pattern, keeping every other argument (case sensitivity, pathspec, ref,
--untracked/--no-index), and opens the result with a note that repeats git's reason:Three points about the design:
builtin/grep.cregisters the origin string"-e option"for a pattern passed with-e, andgrep.c'scompile_regexp_failed()renders the failure as<origin>, '<pattern>': <reason>from a literal format string that is not translated. I checked the marker against git source at v2.20.0, v2.30.0, v2.39.0, v2.45.0 and v2.50.0 — it is unchanged. It is also specific: git diagnoses an unresolvable ref (unable to resolve revision) and an invalid pathspec (Invalid pathspec magic) before it compiles patterns, so this marker only ever appears when the pattern itself is the problem.No matches foundstays byte-identical when no fallback happened, and gains the note when it did, so the model learns its term was treated literally.Second commit: doc drift found while rebasing
Rebasing onto
main(now at189be5b, which includes #1306) was a small merge: #1306 moved the truncation note to after the parse loop, and this PR's literal note now sits directly above it, so both notes can appear in one result.Reviewing that merge surfaced stale documentation. #1306 changed the limit from per-file
--max-count 100to a global 100-match cap with a--max-count 101per-file probe, and reworded the note — but the tools guide in all five locales (pages/src/content/docs/*/tools.md) and thecode_searchdescription ininternal/config/toolsconfig/tools.jsonstill quoted the old per-file wording and the old note text. The second commit brings those six files in line with the merged behavior.Type of Change
How Has This Been Tested?
make testpasses locally (race, all packages)make checkpasses (license headers, English-only check,go mod tidy,gofmt -s,go vet)make coveragepasses: above the 90% threshold; the new helpers are at 100%git grepin workspace mode, commit mode (ref + pathspec carried into the retry), and a non-git directory (where the rejection only appears on the--no-indexretry), plus no-match, valid-regex-stays-regex, non-pattern-errors-still-error, and table tests for the marker and reason parsingTestGitGrep_ResultLimitfrom fix(tool): enforce the global code search result limit #1306 still passes on the rebased tree; the retry inherits its--max-counthandling unchangedTestGitGrep_PerlRegexp_InvalidPattern_ReturnsErrorasserted the old behavior (invalid pattern → error) and is replaced by the fallback tests.Checklist
go fmt,go vet)AI disclosure: the implementation, tests and documentation edits were drafted with Codex (GPT-5, OpenAI) and reviewed line by line before pushing; no commit trailer attributes it to a model. The zh/ja/ko/ru wording in
pages/src/content/docs/*/tools.mdwas drafted the same way and is technically accurate, but I am not a native speaker of those languages — corrections are welcome. The CLA is not signed yet; I will sign as soon as the bot posts the link.Related Issues
Closes #1340
One thing I deliberately left out: a git built without PCRE2 (
-Punsupported). That failure has a different message and different semantics, and I could not reproduce it here.