diff --git a/src/core/loaders.test.ts b/src/core/loaders.test.ts index 1d7947b0..db4fabb6 100644 --- a/src/core/loaders.test.ts +++ b/src/core/loaders.test.ts @@ -993,4 +993,125 @@ describe("loadAppBootstrap", () => { expect(bootstrap.changeset.files[0]?.path.endsWith("after.ts")).toBe(true); expect(bootstrap.changeset.files[0]?.stats.additions).toBeGreaterThan(0); }); + + test("loads patch text emitted with diff.noprefix=true (e.g. from `hunk pager` stdin)", async () => { + const bootstrap = await loadAppBootstrap({ + kind: "patch", + text: [ + "diff --git src/example.ts src/example.ts", + "index 0000000..1111111 100644", + "--- src/example.ts", + "+++ src/example.ts", + "@@ -1,1 +1,2 @@", + " const value = 1;", + "+const added = 2;", + ].join("\n"), + options: { mode: "auto" }, + }); + + expect(bootstrap.changeset.files).toHaveLength(1); + expect(bootstrap.changeset.files[0]).toMatchObject({ + path: "src/example.ts", + metadata: { name: "src/example.ts", type: "change" }, + }); + expect(bootstrap.changeset.files[0]?.stats.additions).toBe(1); + }); + + test("loads noprefix rename patches by recovering the rename pair from the headers", async () => { + const bootstrap = await loadAppBootstrap({ + kind: "patch", + text: [ + "diff --git old/path.ts new/path.ts", + "similarity index 100%", + "rename from old/path.ts", + "rename to new/path.ts", + ].join("\n"), + options: { mode: "auto" }, + }); + + expect(bootstrap.changeset.files).toHaveLength(1); + expect(bootstrap.changeset.files[0]).toMatchObject({ + path: "new/path.ts", + previousPath: "old/path.ts", + metadata: { type: "rename-pure" }, + }); + }); + + test("loads quoted noprefix patch text emitted for escaped git paths", async () => { + const dir = createTempRepo("hunk-patch-quoted-noprefix-"); + const fileName = "src\tfile.txt"; + + writeFileSync(join(dir, fileName), "one\n"); + git(dir, "add", "."); + git(dir, "commit", "-m", "initial"); + + writeFileSync(join(dir, fileName), "two\n"); + const patchText = git(dir, "-c", "diff.noprefix=true", "diff", "--", fileName); + + expect(patchText).toContain('diff --git "src\\tfile.txt" "src\\tfile.txt"'); + + const bootstrap = await loadAppBootstrap({ + kind: "patch", + text: patchText, + options: { mode: "auto" }, + }); + + expect(bootstrap.changeset.files).toHaveLength(1); + expect(bootstrap.changeset.files[0]).toMatchObject({ + path: "src\\tfile.txt", + metadata: { name: "src\\tfile.txt", type: "change" }, + }); + expect(bootstrap.changeset.files[0]?.stats).toEqual({ additions: 1, deletions: 1 }); + }); + + test("does not mangle a deleted SQL `-- comment` line in a noprefix patch", async () => { + // The original source line `-- drop table users;` (a SQL comment) is encoded in a unified + // diff deletion as `--- drop table users;` — three dashes (one for the deletion marker, + // two from the comment) and a space. That looks identical to a `--- a/path` file header + // on its own, so the noprefix prefix-restorer must stop rewriting `--- ` lines once the + // `+++ ` line of the current block has been emitted. + const bootstrap = await loadAppBootstrap({ + kind: "patch", + text: [ + "diff --git db/schema.sql db/schema.sql", + "index 0000000..1111111 100644", + "--- db/schema.sql", + "+++ db/schema.sql", + "@@ -1,3 +1,2 @@", + " CREATE TABLE users (id INT);", + "--- drop table users;", + " CREATE TABLE posts (id INT);", + ].join("\n"), + options: { mode: "auto" }, + }); + + expect(bootstrap.changeset.files).toHaveLength(1); + const file = bootstrap.changeset.files[0]!; + expect(file.path).toBe("db/schema.sql"); + expect(file.stats.deletions).toBe(1); + // The deleted content must round-trip as `-- drop table users;` (the original SQL line), + // not as `-- a/drop table users;` (the corruption produced when the rewriter is still + // active inside the hunk body). + expect(file.metadata.deletionLines).toContain("-- drop table users;\n"); + expect(file.metadata.deletionLines.some((line) => line.includes("a/"))).toBe(false); + }); + + test("leaves correctly prefixed patches untouched even when paths sit inside an `a/` directory", async () => { + const bootstrap = await loadAppBootstrap({ + kind: "patch", + text: [ + "diff --git a/a/inner.ts b/a/inner.ts", + "index 0000000..1111111 100644", + "--- a/a/inner.ts", + "+++ b/a/inner.ts", + "@@ -1,1 +1,2 @@", + " const x = 1;", + "+const y = 2;", + ].join("\n"), + options: { mode: "auto" }, + }); + + expect(bootstrap.changeset.files).toHaveLength(1); + expect(bootstrap.changeset.files[0]?.path).toBe("a/inner.ts"); + }); }); diff --git a/src/core/loaders.ts b/src/core/loaders.ts index a18def17..4f5d1232 100644 --- a/src/core/loaders.ts +++ b/src/core/loaders.ts @@ -234,6 +234,114 @@ function buildDiffFile( }; } +/** + * Re-add Git's `a/` and `b/` path prefixes to patch headers when stdin came from a + * `git diff` that was emitted with `diff.noprefix=true` (or otherwise stripped prefixes). + * + * `@pierre/diffs` requires `a/` and `b/` on `diff --git`, `---`, and `+++` lines and throws + * a `TypeError` on the first noprefix header, which leaves the review with zero files. The + * git-backed paths force `diff.noprefix=false` when they invoke git internally; this helper + * covers the patch path (`hunk patch`, `hunk pager`) where the input was produced by an + * outer `git` process we do not control. + * + * The rewrite is scoped to header lines only: once the `+++ ` line has been emitted for a + * block we clear the flag so a deleted line whose content starts with `-- ` (e.g. a removed + * SQL/Lua/Haskell comment, which becomes `--- foo` on disk) is not mistaken for a file + * header inside the hunk body. + */ +function normalizeGitPatchPrefixes(patchText: string) { + if (!patchText.includes("diff --git ")) { + return patchText; + } + + let blockNeedsPrefix = false; + + return patchText + .split("\n") + .map((line) => { + if (line.startsWith("diff --git ")) { + const result = rewriteGitDiffHeader(line); + blockNeedsPrefix = result.changed; + return result.line; + } + + if (blockNeedsPrefix && line.startsWith("--- ")) { + return rewriteUnifiedFileLine(line, "--- ", "a/"); + } + + if (blockNeedsPrefix && line.startsWith("+++ ")) { + blockNeedsPrefix = false; + return rewriteUnifiedFileLine(line, "+++ ", "b/"); + } + + return line; + }) + .join("\n"); +} + +/** Detect prefixed/noprefix `diff --git` lines and rewrite the noprefix form into `a/X b/Y`. */ +function rewriteGitDiffHeader(line: string): { line: string; changed: boolean } { + const rest = line.slice("diff --git ".length).trimEnd(); + + const quotedMatch = rest.match(/^"((?:\\.|[^"\\])*)" "((?:\\.|[^"\\])*)"$/); + if (quotedMatch) { + const [, oldPath = "", newPath = ""] = quotedMatch; + // Pierre's git header parser does not currently handle the quoted `"a/..." "b/..."` + // form, so canonicalize quoted paths to the unquoted form even when prefixes exist. + return { + line: `diff --git ${withGitPrefix(oldPath, "a/")} ${withGitPrefix(newPath, "b/")}`, + changed: true, + }; + } + + const tokens = rest.split(" "); + + // Already prefixed: `a/X b/Y` (covers single-token and equally split multi-token paths). + if (tokens.length === 2 && tokens[0]?.startsWith("a/") && tokens[1]?.startsWith("b/")) { + return { line, changed: false }; + } + if (tokens.length >= 2 && tokens.length % 2 === 0) { + const half = tokens.length / 2; + const firstHalf = tokens.slice(0, half).join(" "); + const secondHalf = tokens.slice(half).join(" "); + if (firstHalf.startsWith("a/") && secondHalf.startsWith("b/")) { + return { line, changed: false }; + } + // Non-rename noprefix: identical halves regardless of whether the path contains spaces. + if (firstHalf === secondHalf && firstHalf.length > 0) { + return { line: `diff --git a/${firstHalf} b/${secondHalf}`, changed: true }; + } + } + + // Two-token rename without prefix and without spaces in either path. + if (tokens.length === 2 && tokens[0] && tokens[1]) { + return { line: `diff --git a/${tokens[0]} b/${tokens[1]}`, changed: true }; + } + + // Genuinely ambiguous (rename with spaces and no quoting). Leave untouched and let the + // parser surface the existing failure rather than guess at the path split. + return { line, changed: false }; +} + +/** Return a path with the expected Git side prefix while avoiding double-prefixing. */ +function withGitPrefix(path: string, prefix: "a/" | "b/") { + return path.startsWith(prefix) ? path : `${prefix}${path}`; +} + +/** Insert the canonical `a/` or `b/` prefix on a unified-diff header that is missing it. */ +function rewriteUnifiedFileLine(line: string, marker: "--- " | "+++ ", prefix: "a/" | "b/") { + const path = line.slice(marker.length); + const quotedPath = path.match(/^"((?:\\.|[^"\\])*)"(.*)$/); + const pathName = quotedPath?.[1] ?? path; + const suffix = quotedPath?.[2] ?? ""; + + if (pathName === "/dev/null" || pathName.startsWith("/dev/null\t")) { + return line; + } + + return `${marker}${withGitPrefix(pathName, prefix)}${suffix}`; +} + /** Escape only the filename characters that break unified-diff header parsing. */ function escapeUntrackedPatchPath(path: string) { return path @@ -366,8 +474,8 @@ function normalizePatchChangeset( sourceLabel: string, agentContext: AgentContext | null, ): Changeset { - const normalizedPatchText = stripGitLogMetadata( - stripTerminalControl(patchText.replaceAll("\r\n", "\n")), + const normalizedPatchText = normalizeGitPatchPrefixes( + stripGitLogMetadata(stripTerminalControl(patchText.replaceAll("\r\n", "\n"))), ); let parsedPatches: ReturnType;