From b3c272f72025b27de38c0206d0d266739a672b59 Mon Sep 17 00:00:00 2001 From: c4605 Date: Sun, 17 May 2026 12:58:10 +0200 Subject: [PATCH] Fix native diff row expansion (#940) * Fix structured diff parsing for no-prefix paths Git can emit diff --git path path when diff.noprefix is enabled. The structured diff parser only handled the default a/path b/path form, so checkout diff assembly could lose hunks for affected files and render expanded rows empty. Normalize optional a/ and b/ prefixes in both app and server parsers, and cover the no-prefix header form with regression tests. * Fix no-prefix diff path parsing --------- Co-authored-by: Mohamed Boudra --- .../app/src/utils/diff-highlighter.test.ts | 52 +++++++++++++++++++ packages/app/src/utils/diff-highlighter.ts | 43 +++++++++++---- .../src/server/utils/diff-highlighter.test.ts | 52 +++++++++++++++++++ .../src/server/utils/diff-highlighter.ts | 43 ++++++++++++--- .../server/src/utils/checkout-git.test.ts | 21 ++++++++ 5 files changed, 195 insertions(+), 16 deletions(-) diff --git a/packages/app/src/utils/diff-highlighter.test.ts b/packages/app/src/utils/diff-highlighter.test.ts index 6885a211a..76a3a63ba 100644 --- a/packages/app/src/utils/diff-highlighter.test.ts +++ b/packages/app/src/utils/diff-highlighter.test.ts @@ -185,6 +185,58 @@ describe("parseDiff", () => { expect(hunk.lines[3].content).toBe("const bar = 3;"); }); + it("parses no-prefix git diff headers", () => { + const files = parseDiff( + SIMPLE_DIFF.replace("a/example.ts b/example.ts", "example.ts example.ts") + .replace("--- a/example.ts", "--- example.ts") + .replace("+++ b/example.ts", "+++ example.ts"), + ); + + expect(files).toHaveLength(1); + expect(files[0].path).toBe("example.ts"); + expect(files[0].additions).toBe(1); + expect(files[0].deletions).toBe(1); + expect(files[0].hunks).toHaveLength(1); + }); + + it("parses paths with spaces", () => { + const files = parseDiff(SIMPLE_DIFF.replaceAll("example.ts", "file with space.ts")); + + expect(files).toHaveLength(1); + expect(files[0].path).toBe("file with space.ts"); + expect(files[0].hunks).toHaveLength(1); + }); + + it("parses no-prefix paths with spaces", () => { + const files = parseDiff( + SIMPLE_DIFF.replaceAll("example.ts", "file with space.ts") + .replace( + "a/file with space.ts b/file with space.ts", + "file with space.ts file with space.ts", + ) + .replace("--- a/file with space.ts", "--- file with space.ts") + .replace("+++ b/file with space.ts", "+++ file with space.ts"), + ); + + expect(files).toHaveLength(1); + expect(files[0].path).toBe("file with space.ts"); + expect(files[0].hunks).toHaveLength(1); + }); + + it("preserves no-prefix paths that start with a or b", () => { + const files = parseDiff( + `${SIMPLE_DIFF.replace( + "diff --git a/example.ts b/example.ts", + "diff --git a/example.ts a/example.ts", + ).replace("--- a/example.ts\n+++ b/example.ts", "--- a/example.ts\n+++ a/example.ts")} +${SIMPLE_DIFF.replaceAll("example.ts", "other.ts") + .replace("diff --git a/other.ts b/other.ts", "diff --git b/other.ts b/other.ts") + .replace("--- a/other.ts\n+++ b/other.ts", "--- b/other.ts\n+++ b/other.ts")}`, + ); + + expect(files.map((file) => file.path)).toEqual(["a/example.ts", "b/other.ts"]); + }); + it("parses a diff with multiple hunks", () => { const files = parseDiff(MULTI_HUNK_DIFF); diff --git a/packages/app/src/utils/diff-highlighter.ts b/packages/app/src/utils/diff-highlighter.ts index c179e1755..1830df603 100644 --- a/packages/app/src/utils/diff-highlighter.ts +++ b/packages/app/src/utils/diff-highlighter.ts @@ -30,14 +30,40 @@ function isDiffMetadataLine(line: string): boolean { return DIFF_METADATA_PREFIXES.some((prefix) => line.startsWith(prefix)); } -function extractDiffPath(firstLine: string): string { - const pathMatch = firstLine.match(/a\/(.*?) b\//); - if (pathMatch) { - return pathMatch[1]; +// Git's default patch headers use paired a/path and b/path prefixes, while +// diff.noprefix emits plain paths that may legitimately start with a/ or b/. +function usesDiffPathPrefixes(oldPath: string, newPath: string): boolean { + return oldPath.startsWith("a/") && newPath.startsWith("b/"); +} + +function extractPathFromMetadata(lines: string[], prefix: "--- " | "+++ "): string | null { + const line = lines.find((candidate) => candidate.startsWith(prefix)); + if (!line) { + return null; } - const newFileMatch = firstLine.match(/b\/(.+)$/); - if (newFileMatch) { - return newFileMatch[1]; + + const path = line.slice(prefix.length).replace(/\t.*$/, "").trimEnd(); + return path === "/dev/null" ? null : path; +} + +function extractDiffPath(lines: string[]): string { + const firstLine = lines[0] ?? ""; + const prefixedPathMatch = firstLine.match(/^a\/(.+) b\/(.+)$/); + if (prefixedPathMatch) { + return prefixedPathMatch[2]; + } + + const metadataPath = + extractPathFromMetadata(lines, "+++ ") ?? extractPathFromMetadata(lines, "--- "); + if (metadataPath) { + return metadataPath; + } + + const pathMatch = firstLine.match(/^(\S+)\s+(\S+)$/); + if (pathMatch) { + const [, oldPath, newPath] = pathMatch; + const path = newPath === "/dev/null" ? oldPath : newPath; + return usesDiffPathPrefixes(oldPath, newPath) ? path.slice(2) : path; } return "unknown"; } @@ -118,11 +144,10 @@ export function parseDiff(diffText: string): ParsedDiffFile[] { for (const section of fileSections) { const lines = section.split("\n"); - const firstLine = lines[0]; const isNew = section.includes("new file mode") || section.includes("--- /dev/null"); const isDeleted = section.includes("deleted file mode") || section.includes("+++ /dev/null"); - const path = extractDiffPath(firstLine); + const path = extractDiffPath(lines); const { hunks, additions, deletions } = parseHunks(lines); files.push({ path, isNew, isDeleted, additions, deletions, hunks }); diff --git a/packages/server/src/server/utils/diff-highlighter.test.ts b/packages/server/src/server/utils/diff-highlighter.test.ts index 25f4ac751..8816dd520 100644 --- a/packages/server/src/server/utils/diff-highlighter.test.ts +++ b/packages/server/src/server/utils/diff-highlighter.test.ts @@ -188,6 +188,58 @@ describe("parseDiff", () => { expect(hunk.lines[3].content).toBe("const bar = 3;"); }); + it("parses no-prefix git diff headers", () => { + const files = parseDiff( + SIMPLE_DIFF.replace("a/example.ts b/example.ts", "example.ts example.ts") + .replace("--- a/example.ts", "--- example.ts") + .replace("+++ b/example.ts", "+++ example.ts"), + ); + + expect(files).toHaveLength(1); + expect(files[0].path).toBe("example.ts"); + expect(files[0].additions).toBe(1); + expect(files[0].deletions).toBe(1); + expect(files[0].hunks).toHaveLength(1); + }); + + it("parses paths with spaces", () => { + const files = parseDiff(SIMPLE_DIFF.replaceAll("example.ts", "file with space.ts")); + + expect(files).toHaveLength(1); + expect(files[0].path).toBe("file with space.ts"); + expect(files[0].hunks).toHaveLength(1); + }); + + it("parses no-prefix paths with spaces", () => { + const files = parseDiff( + SIMPLE_DIFF.replaceAll("example.ts", "file with space.ts") + .replace( + "a/file with space.ts b/file with space.ts", + "file with space.ts file with space.ts", + ) + .replace("--- a/file with space.ts", "--- file with space.ts") + .replace("+++ b/file with space.ts", "+++ file with space.ts"), + ); + + expect(files).toHaveLength(1); + expect(files[0].path).toBe("file with space.ts"); + expect(files[0].hunks).toHaveLength(1); + }); + + it("preserves no-prefix paths that start with a or b", () => { + const files = parseDiff( + `${SIMPLE_DIFF.replace( + "diff --git a/example.ts b/example.ts", + "diff --git a/example.ts a/example.ts", + ).replace("--- a/example.ts\n+++ b/example.ts", "--- a/example.ts\n+++ a/example.ts")} +${SIMPLE_DIFF.replaceAll("example.ts", "other.ts") + .replace("diff --git a/other.ts b/other.ts", "diff --git b/other.ts b/other.ts") + .replace("--- a/other.ts\n+++ b/other.ts", "--- b/other.ts\n+++ b/other.ts")}`, + ); + + expect(files.map((file) => file.path)).toEqual(["a/example.ts", "b/other.ts"]); + }); + it("parses a diff with multiple hunks", () => { const files = parseDiff(MULTI_HUNK_DIFF); diff --git a/packages/server/src/server/utils/diff-highlighter.ts b/packages/server/src/server/utils/diff-highlighter.ts index 1d314541e..c016318e4 100644 --- a/packages/server/src/server/utils/diff-highlighter.ts +++ b/packages/server/src/server/utils/diff-highlighter.ts @@ -39,11 +39,41 @@ interface ParseAndHighlightDiffOptions { /** * Parse a unified diff into structured data */ -function extractPathFromDiffHeader(firstLine: string): string { - const pathMatch = firstLine.match(/a\/(.*?) b\//); - if (pathMatch) return pathMatch[1]; - const newFileMatch = firstLine.match(/b\/(.+)$/); - if (newFileMatch) return newFileMatch[1]; +// Git's default patch headers use paired a/path and b/path prefixes, while +// diff.noprefix emits plain paths that may legitimately start with a/ or b/. +function usesDiffPathPrefixes(oldPath: string, newPath: string): boolean { + return oldPath.startsWith("a/") && newPath.startsWith("b/"); +} + +function extractPathFromMetadata(lines: string[], prefix: "--- " | "+++ "): string | null { + const line = lines.find((candidate) => candidate.startsWith(prefix)); + if (!line) { + return null; + } + + const path = line.slice(prefix.length).replace(/\t.*$/, "").trimEnd(); + return path === "/dev/null" ? null : path; +} + +function extractPathFromDiffHeader(lines: string[]): string { + const firstLine = lines[0] ?? ""; + const prefixedPathMatch = firstLine.match(/^a\/(.+) b\/(.+)$/); + if (prefixedPathMatch) { + return prefixedPathMatch[2]; + } + + const metadataPath = + extractPathFromMetadata(lines, "+++ ") ?? extractPathFromMetadata(lines, "--- "); + if (metadataPath) { + return metadataPath; + } + + const pathMatch = firstLine.match(/^(\S+)\s+(\S+)$/); + if (pathMatch) { + const [, oldPath, newPath] = pathMatch; + const path = newPath === "/dev/null" ? oldPath : newPath; + return usesDiffPathPrefixes(oldPath, newPath) ? path.slice(2) : path; + } return "unknown"; } @@ -123,11 +153,10 @@ export function parseDiff(diffText: string): ParsedDiffFile[] { for (const section of fileSections) { const lines = section.split("\n"); - const firstLine = lines[0]; const isNew = section.includes("new file mode") || section.includes("--- /dev/null"); const isDeleted = section.includes("deleted file mode") || section.includes("+++ /dev/null"); - const path = extractPathFromDiffHeader(firstLine); + const path = extractPathFromDiffHeader(lines); const { hunks, additions, deletions } = parseSectionBody(lines); diff --git a/packages/server/src/utils/checkout-git.test.ts b/packages/server/src/utils/checkout-git.test.ts index 9c8c87301..44c5d08cf 100644 --- a/packages/server/src/utils/checkout-git.test.ts +++ b/packages/server/src/utils/checkout-git.test.ts @@ -348,6 +348,27 @@ const x = 1; expect(removedLine?.tokens).toEqual([{ text: "old comment line", style: "comment" }]); }); + it("preserves no-prefix structured paths that start with a or b", async () => { + mkdirSync(join(repoDir, "a")); + mkdirSync(join(repoDir, "b")); + commitFile(repoDir, "a/example.ts", "const value = 1;\n", "add a-prefixed path"); + commitFile(repoDir, "b/other.ts", "const value = 1;\n", "add b-prefixed path"); + commitFile(repoDir, "file with space.ts", "const value = 1;\n", "add path with space"); + execFileSync("git", ["config", "diff.noprefix", "true"], { cwd: repoDir }); + + writeFileSync(join(repoDir, "a/example.ts"), "const value = 2;\n"); + writeFileSync(join(repoDir, "b/other.ts"), "const value = 2;\n"); + writeFileSync(join(repoDir, "file with space.ts"), "const value = 2;\n"); + + const diff = await getCheckoutDiff(repoDir, { mode: "uncommitted", includeStructured: true }); + + expect(diff.structured?.map((file) => [file.path, file.hunks.length])).toEqual([ + ["a/example.ts", 1], + ["b/other.ts", 1], + ["file with space.ts", 1], + ]); + }); + it("returns checkout root metadata for normal repos", async () => { const status = await getCheckoutStatus(repoDir); expect(status.isGit).toBe(true);