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..a60fac14d 100644 --- a/src/utils/git/diffParser.test.ts +++ b/src/utils/git/diffParser.test.ts @@ -468,4 +468,119 @@ 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 + // + // 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 + 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"); + + // 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); + + // 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 }); + }); });