refactor(diff): normalize git argument ordering in workspaceTrackedDiff#4
refactor(diff): normalize git argument ordering in workspaceTrackedDiff#4chethanuk wants to merge 1 commit into
Conversation
codeant-ai
Bot
added
the
size:XS
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
3fe6215 to
cafa137
Compare
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/diff/git.go`:
- Around line 264-276: Update the git diff handling around runGit to return the
git diff HEAD result whenever err == nil, removing the out != "" condition.
Preserve the context cancellation check and staged-diff fallback for errors,
including repositories without a HEAD.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 58aa6f56-70f5-48ad-a56a-8e72497bdf5e
📒 Files selected for processing (7)
internal/diff/git.gointernal/diff/git_test.gointernal/llm/providers.gointernal/llm/providers_test.gopages/src/i18n/en.tspages/src/i18n/ja.tspages/src/i18n/zh.ts
| out, err := p.runGit(ctx, "-c", "core.quotepath=false", "diff", "--no-ext-diff", "--no-textconv", "--find-renames", "--src-prefix=a/", "--dst-prefix=b/", "--no-color", "-U"+fmt.Sprint(DiffContextLines), "--end-of-options", "HEAD", "--") | ||
| if err == nil && out != "" { | ||
| return out, nil | ||
| } | ||
| if ctx.Err() != nil { | ||
| return "", ctx.Err() | ||
| } | ||
| return p.runGit(ctx, "-c", "core.quotepath=false", "diff", "--no-ext-diff", "--no-textconv", "--find-renames", "--src-prefix=a/", "--dst-prefix=b/", "--staged", "--no-color", "-U"+fmt.Sprint(DiffContextLines), "--") | ||
| // Fall back to the staged diff when `git diff HEAD` errored or was empty. This is | ||
| // not redundant with the call above: in a repository with no commits yet there is no | ||
| // HEAD, so `git diff HEAD` fails with "bad revision 'HEAD'", but `git diff --staged` | ||
| // still surfaces staged changes by diffing the index against the empty tree — the only | ||
| // way to review a workspace before its first commit. | ||
| return p.runGit(ctx, "-c", "core.quotepath=false", "diff", "--no-ext-diff", "--no-textconv", "--find-renames", "--src-prefix=a/", "--dst-prefix=b/", "--no-color", "-U"+fmt.Sprint(DiffContextLines), "--staged", "--") |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify the behavior of git diff HEAD vs --staged when WT matches HEAD but index differs.
repo=$(mktemp -d)
cd "$repo"
git init -q
git config user.email "test@example.com"
git config user.name "Test User"
git config commit.gpgsign false
echo "A" > file.txt
git add file.txt
git commit -m "Initial commit"
echo "B" > file.txt
git add file.txt
echo "A" > file.txt
echo "--- git diff HEAD ---"
git diff HEAD
echo "--- git diff --staged ---"
git diff --stagedRepository: chethanuk/open-code-review
Length of output: 492
Drop the out != "" check here. git diff HEAD already errors when HEAD is missing, and on a clean tree it returns empty output; if the index has staged-only changes, this fallback will surface those staged diffs even though the working tree matches HEAD. If the fallback is only for repos without commits, err == nil is enough.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/diff/git.go` around lines 264 - 276, Update the git diff handling
around runGit to return the git diff HEAD result whenever err == nil, removing
the out != "" condition. Preserve the context cancellation check and staged-diff
fallback for errors, including repositories without a HEAD.
…or workspace diff Follow-up to alibaba#381, which normalized the argument ordering in workspaceTrackedDiff (options -> positional ref -> --) but stopped short of the --end-of-options guard. This adds it before HEAD in the first runGit call, bringing workspace mode fully in line with the canonical range/commit/ merge-base calls in this file (git.go:119/126/256), which already require git >= 2.24. The --staged fallback call is untouched: it has no positional ref (only the -- pathspec separator), so --end-of-options would guard nothing there. Also documents why the --staged fallback is load-bearing (repos with no commits have no HEAD, so `git diff HEAD` fails while `git diff --staged` still surfaces staged changes against the empty tree) and pins it with TestWorkspaceDiffNoCommitsUsesStagedFallback. Closes alibaba#374
cafa137 to
3126e26
Compare
|
Warning Review limit reached
Next review available in: 55 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
✨ 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 |
1 participant
Description
Normalizes the git argument ordering in
workspaceTrackedDiff(internal/diff/git.go) so bothrunGitinvocations follow the same convention as every other diff command in the file: option flags first, then positional refs (HEAD), then--. No behavioral change to the produced diff — git accepts either ordering — this is purely for consistency and maintainability.The reordered calls use
--end-of-optionsbefore the positional ref, the same guard the range/commit modes in this file already use — so workspace mode now shares their existing git ≥ 2.24 floor (released Nov 2019). Old and new orderings were verified to produce byte-identical output.While touching this function, the fallback from
git diff HEADtogit diff --stagedgained a clarifying comment and a regression test: the fallback is load-bearing for repositories with no commits yet, whereHEADdoes not resolve (git diff HEADfails withbad revision 'HEAD') butgit diff --stagedstill surfaces staged changes by diffing the index against the empty tree.TestWorkspaceDiffNoCommitsUsesStagedFallbackpins this: it stages a file in a commit-less repository and asserts the workspace diff surfaces it.Type of Change
How Has This Been Tested?
make testpasses locallyNew regression test
TestWorkspaceDiffNoCommitsUsesStagedFallbackfails if the--stagedfallback is removed and passes with the reordered arguments, demonstrating the reorder preserves behavior including the no-commit edge case.make check(tidy + fmt + vet) also passes.Checklist
go fmt,go vet)Related Issues
Closes alibaba#374
Summary by CodeRabbit
New Features
OLLAMA_API_KEY.Bug Fixes
Documentation