From a1ae2c8d1d20ed0afcfff977d6c8340ca3840a62 Mon Sep 17 00:00:00 2001 From: Ammar Date: Sun, 19 Oct 2025 17:50:19 -0500 Subject: [PATCH 1/4] =?UTF-8?q?=F0=9F=A4=96=20Fix=20code=20review:=20elimi?= =?UTF-8?q?nate=20inverse=20deltas=20when=20branch=20is=20behind=20base?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Problem: When a branch is behind origin/main and includeUncommitted is enabled, the Code Review panel would show inverse deltas (deletions) of commits that landed on origin/main after branching. This happened because toggling 'Uncommitted' switched between: - git diff origin/main...HEAD (stable merge-base) - git diff origin/main (two-dot, compares tip to working dir) The two-dot diff changed the comparison point, showing irrelevant work. Solution: Use explicit merge-base when includeUncommitted is true: - git diff $(git merge-base origin/main HEAD) This maintains a stable merge-base regardless of the uncommitted flag and produces a single unified diff (no duplicate hunks from concatenation). Test Coverage: - Added test case for branch behind base ref scenario - Verifies no inverse deltas appear from commits on base ref - All 15 diffParser tests pass _Generated with `cmux`_ --- .../RightSidebar/CodeReview/ReviewPanel.tsx | 27 ++++++- src/utils/git/diffParser.test.ts | 73 +++++++++++++++++++ 2 files changed, 97 insertions(+), 3 deletions(-) diff --git a/src/components/RightSidebar/CodeReview/ReviewPanel.tsx b/src/components/RightSidebar/CodeReview/ReviewPanel.tsx index 01c62a578..fa728ac0d 100644 --- a/src/components/RightSidebar/CodeReview/ReviewPanel.tsx +++ b/src/components/RightSidebar/CodeReview/ReviewPanel.tsx @@ -241,6 +241,19 @@ interface DiagnosticInfo { * Shared logic between numstat (file tree) and diff (hunks) commands * Exported for testing * + * Git diff semantics: + * - `git diff A...HEAD` (three-dot): Shows commits on current branch since branching from A + * → Uses merge-base(A, HEAD) as comparison point, so changes to A after branching don't appear + * - `git diff $(git merge-base A HEAD)`: Shows all changes from branch point to working directory + * → Includes both committed changes on the branch AND uncommitted working directory changes + * → Single unified diff (no duplicate hunks from concatenation) + * - `git diff HEAD`: Shows only uncommitted changes (working directory vs HEAD) + * - `git diff --staged`: Shows only staged changes (index vs HEAD) + * + * The key insight: When includeUncommitted is true, we compare from the merge-base directly + * to the working directory. This gives a stable comparison point (doesn't change when base + * ref moves forward) while including both committed and uncommitted work in a single diff. + * * @param diffBase - Base reference ("main", "HEAD", "--staged") * @param includeUncommitted - Include uncommitted working directory changes * @param pathFilter - Optional path filter (e.g., ' -- "src/foo.ts"') @@ -265,9 +278,17 @@ export function buildGitDiffCommand( return `git diff HEAD${flags}${pathFilter}`; } - // Branch diff: two-dot includes uncommitted, three-dot excludes - const range = includeUncommitted ? diffBase : `${diffBase}...HEAD`; - return `git diff ${range}${flags}${pathFilter}`; + // Branch diff: use three-dot for committed only, or merge-base for committed+uncommitted + if (includeUncommitted) { + // Use merge-base to get a unified diff from branch point to working directory + // This includes both committed changes on the branch AND uncommitted working changes + // Single command avoids duplicate hunks from concatenation + // Stable comparison point: merge-base doesn't change when diffBase ref moves forward + return `git diff $(git merge-base ${diffBase} HEAD)${flags}${pathFilter}`; + } else { + // Three-dot: committed changes only (merge-base to HEAD) + return `git diff ${diffBase}...HEAD${flags}${pathFilter}`; + } } export const ReviewPanel: React.FC = ({ diff --git a/src/utils/git/diffParser.test.ts b/src/utils/git/diffParser.test.ts index f7e2c93fb..d25980ad2 100644 --- a/src/utils/git/diffParser.test.ts +++ b/src/utils/git/diffParser.test.ts @@ -468,4 +468,77 @@ function greet(name) { expect(allContent.includes("staged")).toBe(true); expect(allContent.includes("unstaged")).toBe(true); }); + + it("should not show inverse deltas when branch is behind base ref", () => { + // Scenario: Branch A is 3 commits behind origin/main + // When reviewing with includeUncommitted=true, should NOT show inverse deltas + // from the 3 commits that landed on origin/main after branching + execSync("git reset --hard HEAD", { cwd: testRepoPath }); + + // Create a "main" branch and add 3 commits + execSync("git checkout -b test-main", { cwd: testRepoPath }); + writeFileSync(join(testRepoPath, "main-file.txt"), "Initial content\n"); + execSync("git add main-file.txt && git commit -m 'Initial on main'", { cwd: testRepoPath }); + + // Branch off from here (this is the merge-base) + execSync("git checkout -b feature-branch", { cwd: testRepoPath }); + + // Add commits on feature branch + writeFileSync(join(testRepoPath, "feature-file.txt"), "Feature content\n"); + execSync("git add feature-file.txt && git commit -m 'Add feature file'", { + cwd: testRepoPath, + }); + + // Simulate origin/main moving forward (3 commits ahead) + execSync("git checkout test-main", { cwd: testRepoPath }); + writeFileSync(join(testRepoPath, "main-file.txt"), "Initial content\nCommit Y\n"); + execSync("git add main-file.txt && git commit -m 'Commit Y on main'", { + cwd: testRepoPath, + }); + + writeFileSync(join(testRepoPath, "main-file.txt"), "Initial content\nCommit Y\nCommit Z\n"); + execSync("git add main-file.txt && git commit -m 'Commit Z on main'", { + cwd: testRepoPath, + }); + + writeFileSync(join(testRepoPath, "main-file.txt"), "Initial content\nCommit Y\nCommit Z\nCommit W\n"); + execSync("git add main-file.txt && git commit -m 'Commit W on main'", { + cwd: testRepoPath, + }); + + // Back to feature branch + execSync("git checkout feature-branch", { cwd: testRepoPath }); + + // Add uncommitted changes to feature branch + writeFileSync(join(testRepoPath, "feature-file.txt"), "Feature content\nUncommitted change\n"); + + // Now review with includeUncommitted=true, base=test-main + const gitCommand = buildGitDiffCommand("test-main", true, "", "diff"); + const diffOutput = execSync(gitCommand, { cwd: testRepoPath, encoding: "utf-8" }); + const fileDiffs = parseDiff(diffOutput); + + // Should show only feature-file.txt (the file we added/modified) + // Should NOT show main-file.txt as deletions (inverse deltas) + const featureFile = fileDiffs.find((f) => f.filePath === "feature-file.txt"); + const mainFile = fileDiffs.find((f) => f.filePath === "main-file.txt"); + + expect(featureFile).toBeDefined(); + expect(mainFile).toBeUndefined(); // Should NOT appear + + // Verify we see both committed and uncommitted changes in feature-file.txt + const allHunks = extractAllHunks(fileDiffs); + const allContent = allHunks.map((h) => h.content).join("\n"); + + expect(allContent.includes("Feature content")).toBe(true); + expect(allContent.includes("Uncommitted change")).toBe(true); + + // Critically: should NOT show any deletions from Commit Y, Z, W + expect(allContent.includes("Commit Y")).toBe(false); + expect(allContent.includes("Commit Z")).toBe(false); + expect(allContent.includes("Commit W")).toBe(false); + + // Cleanup + execSync("git checkout test-main --force", { cwd: testRepoPath }); + execSync("git branch -D feature-branch", { cwd: testRepoPath }); + }); }); From 5e3a41e74d1e4ed40641ee4dea857153762400e6 Mon Sep 17 00:00:00 2001 From: Ammar Date: Sun, 19 Oct 2025 18:01:20 -0500 Subject: [PATCH 2/4] docs: add diagram to test for clarity --- src/utils/git/diffParser.test.ts | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/src/utils/git/diffParser.test.ts b/src/utils/git/diffParser.test.ts index d25980ad2..ddb7c3e3d 100644 --- a/src/utils/git/diffParser.test.ts +++ b/src/utils/git/diffParser.test.ts @@ -471,8 +471,17 @@ function greet(name) { it("should not show inverse deltas when branch is behind base ref", () => { // Scenario: Branch A is 3 commits behind origin/main - // When reviewing with includeUncommitted=true, should NOT show inverse deltas - // from the 3 commits that landed on origin/main after branching + // + // Git history: + // test-main: Initial---Y---Z---W (3 commits ahead) + // \ + // feature: Feature (committed) + uncommitted changes + // + // Problem: Old behavior with includeUncommitted=true used two-dot diff, + // comparing W to working directory, showing Y, Z, W as inverse deltas. + // + // Expected: Should only show feature branch changes (committed + uncommitted), + // NOT inverse deltas from Y, Z, W commits that landed on test-main. execSync("git reset --hard HEAD", { cwd: testRepoPath }); // Create a "main" branch and add 3 commits From c24cba3f367098bc9ba643e46b76c615d35e406a Mon Sep 17 00:00:00 2001 From: Ammar Date: Sun, 19 Oct 2025 18:03:27 -0500 Subject: [PATCH 3/4] test: verify both includeUncommitted modes don't show inverse deltas Expanded test to verify that BOTH includeUncommitted=false (three-dot) and includeUncommitted=true (merge-base) correctly avoid showing inverse deltas when branch is behind base ref. This provides comprehensive coverage of both code paths and documents the key difference: committed-only vs committed+uncommitted. --- src/utils/git/diffParser.test.ts | 30 +++++++++++++++++++++++++++++- 1 file changed, 29 insertions(+), 1 deletion(-) diff --git a/src/utils/git/diffParser.test.ts b/src/utils/git/diffParser.test.ts index ddb7c3e3d..de7266bc8 100644 --- a/src/utils/git/diffParser.test.ts +++ b/src/utils/git/diffParser.test.ts @@ -521,7 +521,35 @@ function greet(name) { // Add uncommitted changes to feature branch writeFileSync(join(testRepoPath, "feature-file.txt"), "Feature content\nUncommitted change\n"); - // Now review with includeUncommitted=true, base=test-main + // Test 1: includeUncommitted=false (committed only, uses three-dot) + const gitCommandCommittedOnly = buildGitDiffCommand("test-main", false, "", "diff"); + const diffOutputCommittedOnly = execSync(gitCommandCommittedOnly, { + cwd: testRepoPath, + encoding: "utf-8", + }); + const fileDiffsCommittedOnly = parseDiff(diffOutputCommittedOnly); + + const featureFileCommittedOnly = fileDiffsCommittedOnly.find( + (f) => f.filePath === "feature-file.txt" + ); + const mainFileCommittedOnly = fileDiffsCommittedOnly.find((f) => f.filePath === "main-file.txt"); + + expect(featureFileCommittedOnly).toBeDefined(); + expect(mainFileCommittedOnly).toBeUndefined(); // No inverse deltas + + const hunksCommittedOnly = extractAllHunks(fileDiffsCommittedOnly); + const contentCommittedOnly = hunksCommittedOnly.map((h) => h.content).join("\n"); + + // Should show committed feature work + expect(contentCommittedOnly.includes("Feature content")).toBe(true); + // Should NOT show uncommitted changes (key difference from includeUncommitted=true) + expect(contentCommittedOnly.includes("Uncommitted change")).toBe(false); + // Should NOT show inverse deltas from main + expect(contentCommittedOnly.includes("Commit Y")).toBe(false); + expect(contentCommittedOnly.includes("Commit Z")).toBe(false); + expect(contentCommittedOnly.includes("Commit W")).toBe(false); + + // Test 2: includeUncommitted=true (committed + uncommitted, uses merge-base) const gitCommand = buildGitDiffCommand("test-main", true, "", "diff"); const diffOutput = execSync(gitCommand, { cwd: testRepoPath, encoding: "utf-8" }); const fileDiffs = parseDiff(diffOutput); From 2c5a02a5c3b65328861be0a79907c3f211cd8b70 Mon Sep 17 00:00:00 2001 From: Ammar Date: Sun, 19 Oct 2025 18:06:44 -0500 Subject: [PATCH 4/4] style: fix prettier formatting --- src/utils/git/diffParser.test.ts | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/src/utils/git/diffParser.test.ts b/src/utils/git/diffParser.test.ts index de7266bc8..a60fac14d 100644 --- a/src/utils/git/diffParser.test.ts +++ b/src/utils/git/diffParser.test.ts @@ -510,7 +510,10 @@ function greet(name) { cwd: testRepoPath, }); - writeFileSync(join(testRepoPath, "main-file.txt"), "Initial content\nCommit Y\nCommit Z\nCommit W\n"); + writeFileSync( + join(testRepoPath, "main-file.txt"), + "Initial content\nCommit Y\nCommit Z\nCommit W\n" + ); execSync("git add main-file.txt && git commit -m 'Commit W on main'", { cwd: testRepoPath, }); @@ -532,7 +535,9 @@ function greet(name) { const featureFileCommittedOnly = fileDiffsCommittedOnly.find( (f) => f.filePath === "feature-file.txt" ); - const mainFileCommittedOnly = fileDiffsCommittedOnly.find((f) => f.filePath === "main-file.txt"); + const mainFileCommittedOnly = fileDiffsCommittedOnly.find( + (f) => f.filePath === "main-file.txt" + ); expect(featureFileCommittedOnly).toBeDefined(); expect(mainFileCommittedOnly).toBeUndefined(); // No inverse deltas