Skip to content

Commit d363128

Browse files
authored
🤖 Fix code review: eliminate inverse deltas when branch is behind base (#340)
## Problem When a branch is behind `origin/main` and the "Uncommitted" checkbox is enabled in Code Review, inverse deltas (deletions) appear for commits that landed on `origin/main` after branching. **Root cause:** Toggling "Uncommitted" switched between different git diff modes: - `git diff origin/main...HEAD` (three-dot, stable merge-base) when uncommitted=false - `git diff origin/main` (two-dot, tip comparison) when uncommitted=true The two-dot diff compares the tip of `origin/main` to the working directory, showing commits Y, Z, W as deletions since they exist on main but not in the feature branch. ## Solution Use explicit `git merge-base` when `includeUncommitted` is true: ```bash # includeUncommitted=false (committed only) git diff origin/main...HEAD # includeUncommitted=true (committed + uncommitted) git diff $(git merge-base origin/main HEAD) ``` **Benefits:** - **Stable merge-base:** Doesn't change when base ref moves forward - **Single unified diff:** Avoids duplicate hunks from concatenation - **Clear semantics:** "Uncommitted" literally means "include working directory changes" ## Test Coverage Added comprehensive test: "should not show inverse deltas when branch is behind base ref" - Creates feature branch from main - Adds 3 commits to main after branching (simulates branch falling behind) - Adds committed + uncommitted changes to feature branch - Verifies: Shows only feature changes, NOT inverse deltas from main All 662 tests pass ✓ _Generated with `cmux`_
1 parent 41ed015 commit d363128

File tree

2 files changed

+139
-3
lines changed

2 files changed

+139
-3
lines changed

‎src/components/RightSidebar/CodeReview/ReviewPanel.tsx‎

Lines changed: 24 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -243,6 +243,19 @@ interface DiagnosticInfo {
243243
* Shared logic between numstat (file tree) and diff (hunks) commands
244244
* Exported for testing
245245
*
246+
* Git diff semantics:
247+
* - `git diff A...HEAD` (three-dot): Shows commits on current branch since branching from A
248+
* → Uses merge-base(A, HEAD) as comparison point, so changes to A after branching don't appear
249+
* - `git diff $(git merge-base A HEAD)`: Shows all changes from branch point to working directory
250+
* → Includes both committed changes on the branch AND uncommitted working directory changes
251+
* → Single unified diff (no duplicate hunks from concatenation)
252+
* - `git diff HEAD`: Shows only uncommitted changes (working directory vs HEAD)
253+
* - `git diff --staged`: Shows only staged changes (index vs HEAD)
254+
*
255+
* The key insight: When includeUncommitted is true, we compare from the merge-base directly
256+
* to the working directory. This gives a stable comparison point (doesn't change when base
257+
* ref moves forward) while including both committed and uncommitted work in a single diff.
258+
*
246259
* @param diffBase - Base reference ("main", "HEAD", "--staged")
247260
* @param includeUncommitted - Include uncommitted working directory changes
248261
* @param pathFilter - Optional path filter (e.g., ' -- "src/foo.ts"')
@@ -267,9 +280,17 @@ export function buildGitDiffCommand(
267280
return `git diff HEAD${flags}${pathFilter}`;
268281
}
269282

270-
// Branch diff: two-dot includes uncommitted, three-dot excludes
271-
const range = includeUncommitted ? diffBase : `${diffBase}...HEAD`;
272-
return `git diff ${range}${flags}${pathFilter}`;
283+
// Branch diff: use three-dot for committed only, or merge-base for committed+uncommitted
284+
if (includeUncommitted) {
285+
// Use merge-base to get a unified diff from branch point to working directory
286+
// This includes both committed changes on the branch AND uncommitted working changes
287+
// Single command avoids duplicate hunks from concatenation
288+
// Stable comparison point: merge-base doesn't change when diffBase ref moves forward
289+
return `git diff $(git merge-base ${diffBase} HEAD)${flags}${pathFilter}`;
290+
} else {
291+
// Three-dot: committed changes only (merge-base to HEAD)
292+
return `git diff ${diffBase}...HEAD${flags}${pathFilter}`;
293+
}
273294
}
274295

275296
export const ReviewPanel: React.FC<ReviewPanelProps> = ({

‎src/utils/git/diffParser.test.ts‎

Lines changed: 115 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -468,4 +468,119 @@ function greet(name) {
468468
expect(allContent.includes("staged")).toBe(true);
469469
expect(allContent.includes("unstaged")).toBe(true);
470470
});
471+
472+
it("should not show inverse deltas when branch is behind base ref", () => {
473+
// Scenario: Branch A is 3 commits behind origin/main
474+
//
475+
// Git history:
476+
// test-main: Initial---Y---Z---W (3 commits ahead)
477+
// \
478+
// feature: Feature (committed) + uncommitted changes
479+
//
480+
// Problem: Old behavior with includeUncommitted=true used two-dot diff,
481+
// comparing W to working directory, showing Y, Z, W as inverse deltas.
482+
//
483+
// Expected: Should only show feature branch changes (committed + uncommitted),
484+
// NOT inverse deltas from Y, Z, W commits that landed on test-main.
485+
execSync("git reset --hard HEAD", { cwd: testRepoPath });
486+
487+
// Create a "main" branch and add 3 commits
488+
execSync("git checkout -b test-main", { cwd: testRepoPath });
489+
writeFileSync(join(testRepoPath, "main-file.txt"), "Initial content\n");
490+
execSync("git add main-file.txt && git commit -m 'Initial on main'", { cwd: testRepoPath });
491+
492+
// Branch off from here (this is the merge-base)
493+
execSync("git checkout -b feature-branch", { cwd: testRepoPath });
494+
495+
// Add commits on feature branch
496+
writeFileSync(join(testRepoPath, "feature-file.txt"), "Feature content\n");
497+
execSync("git add feature-file.txt && git commit -m 'Add feature file'", {
498+
cwd: testRepoPath,
499+
});
500+
501+
// Simulate origin/main moving forward (3 commits ahead)
502+
execSync("git checkout test-main", { cwd: testRepoPath });
503+
writeFileSync(join(testRepoPath, "main-file.txt"), "Initial content\nCommit Y\n");
504+
execSync("git add main-file.txt && git commit -m 'Commit Y on main'", {
505+
cwd: testRepoPath,
506+
});
507+
508+
writeFileSync(join(testRepoPath, "main-file.txt"), "Initial content\nCommit Y\nCommit Z\n");
509+
execSync("git add main-file.txt && git commit -m 'Commit Z on main'", {
510+
cwd: testRepoPath,
511+
});
512+
513+
writeFileSync(
514+
join(testRepoPath, "main-file.txt"),
515+
"Initial content\nCommit Y\nCommit Z\nCommit W\n"
516+
);
517+
execSync("git add main-file.txt && git commit -m 'Commit W on main'", {
518+
cwd: testRepoPath,
519+
});
520+
521+
// Back to feature branch
522+
execSync("git checkout feature-branch", { cwd: testRepoPath });
523+
524+
// Add uncommitted changes to feature branch
525+
writeFileSync(join(testRepoPath, "feature-file.txt"), "Feature content\nUncommitted change\n");
526+
527+
// Test 1: includeUncommitted=false (committed only, uses three-dot)
528+
const gitCommandCommittedOnly = buildGitDiffCommand("test-main", false, "", "diff");
529+
const diffOutputCommittedOnly = execSync(gitCommandCommittedOnly, {
530+
cwd: testRepoPath,
531+
encoding: "utf-8",
532+
});
533+
const fileDiffsCommittedOnly = parseDiff(diffOutputCommittedOnly);
534+
535+
const featureFileCommittedOnly = fileDiffsCommittedOnly.find(
536+
(f) => f.filePath === "feature-file.txt"
537+
);
538+
const mainFileCommittedOnly = fileDiffsCommittedOnly.find(
539+
(f) => f.filePath === "main-file.txt"
540+
);
541+
542+
expect(featureFileCommittedOnly).toBeDefined();
543+
expect(mainFileCommittedOnly).toBeUndefined(); // No inverse deltas
544+
545+
const hunksCommittedOnly = extractAllHunks(fileDiffsCommittedOnly);
546+
const contentCommittedOnly = hunksCommittedOnly.map((h) => h.content).join("\n");
547+
548+
// Should show committed feature work
549+
expect(contentCommittedOnly.includes("Feature content")).toBe(true);
550+
// Should NOT show uncommitted changes (key difference from includeUncommitted=true)
551+
expect(contentCommittedOnly.includes("Uncommitted change")).toBe(false);
552+
// Should NOT show inverse deltas from main
553+
expect(contentCommittedOnly.includes("Commit Y")).toBe(false);
554+
expect(contentCommittedOnly.includes("Commit Z")).toBe(false);
555+
expect(contentCommittedOnly.includes("Commit W")).toBe(false);
556+
557+
// Test 2: includeUncommitted=true (committed + uncommitted, uses merge-base)
558+
const gitCommand = buildGitDiffCommand("test-main", true, "", "diff");
559+
const diffOutput = execSync(gitCommand, { cwd: testRepoPath, encoding: "utf-8" });
560+
const fileDiffs = parseDiff(diffOutput);
561+
562+
// Should show only feature-file.txt (the file we added/modified)
563+
// Should NOT show main-file.txt as deletions (inverse deltas)
564+
const featureFile = fileDiffs.find((f) => f.filePath === "feature-file.txt");
565+
const mainFile = fileDiffs.find((f) => f.filePath === "main-file.txt");
566+
567+
expect(featureFile).toBeDefined();
568+
expect(mainFile).toBeUndefined(); // Should NOT appear
569+
570+
// Verify we see both committed and uncommitted changes in feature-file.txt
571+
const allHunks = extractAllHunks(fileDiffs);
572+
const allContent = allHunks.map((h) => h.content).join("\n");
573+
574+
expect(allContent.includes("Feature content")).toBe(true);
575+
expect(allContent.includes("Uncommitted change")).toBe(true);
576+
577+
// Critically: should NOT show any deletions from Commit Y, Z, W
578+
expect(allContent.includes("Commit Y")).toBe(false);
579+
expect(allContent.includes("Commit Z")).toBe(false);
580+
expect(allContent.includes("Commit W")).toBe(false);
581+
582+
// Cleanup
583+
execSync("git checkout test-main --force", { cwd: testRepoPath });
584+
execSync("git branch -D feature-branch", { cwd: testRepoPath });
585+
});
471586
});

0 commit comments

Comments
 (0)