mirror of
https://github.com/alibaba/open-code-review.git
synced 2026-08-29 03:51:47 +00:00
0c44f1049e
Some checks are pending
CI / test (push) Waiting to run
CI / windows (push) Waiting to run
CI / cross-compile (amd64, darwin) (push) Waiting to run
CI / cross-compile (amd64, windows) (push) Waiting to run
CI / cross-compile (arm64, darwin) (push) Waiting to run
CI / cross-compile (arm64, linux) (push) Waiting to run
CI / cross-compile (arm64, windows) (push) Waiting to run
CodeQL Advanced / Analyze (go) (push) Waiting to run
CodeQL Advanced / Analyze (actions) (push) Waiting to run
CodeQL Advanced / Analyze (javascript-typescript) (push) Waiting to run
* fix(diff): surface git's own message when a diff command fails
GetDiff runs git through runGit, which captures stdout and stderr together,
then discarded that output on every failure path. A user whose `git show`
failed saw only:
Error: review failed: load diffs: get diffs: git show failed: exit status 129
The exit status alone cannot distinguish an unsupported option from a bad
revision or a permission problem, so diagnosing #972 meant asking the reporter
to re-run the command by hand to see what git actually said.
Quote git's output in the error for the three diff-producing paths (range,
commit, workspace-tracked). The same failure now reads:
... git show failed: exit status 129: error: unknown option `diff-merges=first-parent'
Output is capped, keeping the tail, because runGit's combined output means a
command that failed partway through carries a prefix of real diff along with
the diagnosis. The cap cuts on a rune boundary: git speaks the user's locale,
and #972 came from a Japanese-language Windows install, so a byte-wise cut
would replace a confusing error with an unreadable one.
The other runGit callers deliberately swallow errors and fall back, so they
are left alone.
* test(diff): pin which command speaks when both workspace diffs fail
Addresses review feedback on the two-stage fallback in workspaceTrackedDiff.
Reaching `git diff --staged` means `git diff HEAD` already failed, and in
the case the fallback exists for -- a repository with no commits -- it failed
with "bad revision 'HEAD'", which is expected rather than diagnostic.
Surfacing both would put that benign message ahead of the one describing what
actually blocked the review, so the behavior is deliberate and now has a test
saying so.
* fix(diff): quote stderr, not combined output, when git fails
Review feedback: a `git show` killed mid-write contributed a 2036-byte tail
made entirely of diff content, with no diagnosis anywhere in it. SIGKILL
leaves stderr empty, so keeping the tail kept repository source -- and
whatever that source contains.
That string does not stay local. reviewResultError hands it to
span.RecordError (review_cmd.go:259), and signal.NotifyContext (:99) puts
Ctrl-C on the path that reaches it, so the leak had a route to whatever
telemetry backend is configured. classifyItemError guards the run manifest
against raw error text for the same reason.
Add runGitSplit and give the three diff-producing callers stderr alone. Git
writes its diagnosis to stderr by construction, since die() writes there, so
this loses nothing a reader wants and cannot carry diff. Mirrors runGitGrep in
internal/tool/code_search.go, cancellation guard included: a signalled process
reports the signal rather than the reason, so a cancelled run now says
"context deadline exceeded" instead of "signal: killed" -- which also lets
classifyItemError reach its timeout class instead of the generic provider one.
The three failure modes this PR targets write to stderr, so their messages are
byte-identical and #972 still gets its diagnosis.
workspaceTrackedDiff returns stderr separately rather than overloading its
first return value, which also retires the dual-meaning the earlier review
flagged.
Also fix the regression test asserting "fatal:"/"error:", which git
translates: under zh_CN the line opens with a translated prefix, so it passed
only because CI runs in English. Anchor on the object name instead, the one
part no locale rewrites -- the same pairing isNotGitRepoError uses.
* docs(diff): correct the reasons recorded around gitFailure
Two comments described mechanisms that are not there.
The cancellation assertion in TestGetDiff_CancelledMidWriteReportsCancellation
credited classifyItemError with reading the error's type. It never sees it:
GetDiff is reached through loadDiffs, whose failure is recorded at agent.go:284
as a fixed SetRunFailure(RunFailureInput, "failed to resolve review input")
without inspecting err. classifyItemError has a single call site, agent.go:712,
for per-item subtask errors. Neither reviewResultError nor main.go branches on
the type either, so nothing downstream observes it today.
What the assertion actually holds is runGitSplit's cancellation guard: the leak
assertion above it passes either way, since quoting stderr alone keeps stdout
out of the error, so without this second assertion the guard could be removed
silently. Verified by dropping the guard -- only this assertion fails.
gitDiagLimit and gitFailure still justified themselves by runGit's combined
output, which no caller passes anymore. The ceiling and the keep-the-tail rule
both survive on stderr, but for a different reason: die() exits the process, so
the fatal is last, behind any warnings. Parameter renamed out -> stderr to match
what the three call sites pass.
No behavior change. make test, go vet, gofmt, english-only and coverage (94.0%)
all pass.
---------
Co-authored-by: kite <lizhengfeng.lzf@alibaba-inc.com>
|
||
|---|---|---|
| .. | ||
| first_line_test.go | ||
| git.go | ||
| git_error_test.go | ||
| git_resolve_test.go | ||
| git_test.go | ||
| gitignore.go | ||
| gitignore_test.go | ||
| hunk.go | ||
| hunk_test.go | ||
| parser.go | ||
| parser_test.go | ||
| relocate_across_files_test.go | ||
| relocation.go | ||
| relocation_test.go | ||
| resolver.go | ||
| resolver_test.go | ||
| workspace_file.go | ||
| workspace_file_test.go | ||