diff --git a/web/src/lib/api/gitmirror.test.ts b/web/src/lib/api/gitmirror.test.ts index 43e8db44d..42cfd3f4d 100644 --- a/web/src/lib/api/gitmirror.test.ts +++ b/web/src/lib/api/gitmirror.test.ts @@ -106,11 +106,16 @@ describe("gitmirror diff mappers", () => { expect(diffFileName(fileDiff("", "added.ts"))).toBe("added.ts"); }); - it("reads the change kind off the two paths", () => { + it("reads the change kind off the two sides", () => { expect(diffFileKind(fileDiff("a.ts", "a.ts"))).toBe("changed"); expect(diffFileKind(fileDiff("", "a.ts"))).toBe("new"); expect(diffFileKind(fileDiff("a.ts", ""))).toBe("deleted"); expect(diffFileKind(fileDiff("a.ts", "b.ts"))).toBe("renamed"); + + // what gitmirror actually sends: both sides named, the absent one a null oid + const absent = src("a.ts", { oid: "0".repeat(40), size: 0 }); + expect(diffFileKind({ ...fileDiff("a.ts", "a.ts"), lhsSrc: absent })).toBe("new"); + expect(diffFileKind({ ...fileDiff("a.ts", "a.ts"), rhsSrc: absent })).toBe("deleted"); }); it("counts line pairs: one side is pure, both sides is a replacement", () => { diff --git a/web/src/lib/api/gitmirror.ts b/web/src/lib/api/gitmirror.ts index 67a278d1c..faa83692d 100644 --- a/web/src/lib/api/gitmirror.ts +++ b/web/src/lib/api/gitmirror.ts @@ -45,12 +45,17 @@ export const mergeCheck = ( export type MirrorFileKind = "new" | "deleted" | "renamed" | "changed"; -// a side with no path is a side that doesn't exist: the file was added or deleted +// an add or delete carries the same path on both sides — gix names both resources +// after the change's location — so the side that doesn't exist is the one whose oid +// is null, not the one whose path is empty +export const diffSideExists = (src: ShTangledGitDefs.DiffSrc): boolean => + src.path !== "" && !/^0+$/.test(src.oid); + export const diffFileName = (file: MirrorFileDiff): string => file.rhsSrc.path || file.lhsSrc.path; export const diffFileKind = (file: MirrorFileDiff): MirrorFileKind => { - if (!file.lhsSrc.path) return "new"; - if (!file.rhsSrc.path) return "deleted"; + if (!diffSideExists(file.lhsSrc)) return "new"; + if (!diffSideExists(file.rhsSrc)) return "deleted"; return file.lhsSrc.path === file.rhsSrc.path ? "changed" : "renamed"; }; diff --git a/web/src/lib/api/pullDiff.test.ts b/web/src/lib/api/pullDiff.test.ts index 7a9138216..fa1d76fbe 100644 --- a/web/src/lib/api/pullDiff.test.ts +++ b/web/src/lib/api/pullDiff.test.ts @@ -13,6 +13,10 @@ const src = (path: string, extra: Partial = {}) => ({ ...extra }); +// gitmirror names both sides of an add or delete after the same path and marks the +// side that doesn't exist with a null oid +const absent = (path: string) => src(path, { oid: "0".repeat(40), size: 0 }); + const json = (body: unknown) => new Response(JSON.stringify(body), { status: 200, @@ -61,18 +65,29 @@ describe("loadPullDiff", () => { hasSyntacticChanges: false }, { - lhsSrc: src(""), + lhsSrc: absent("new.ts"), rhsSrc: src("new.ts"), hunks: [{ novelLhs: [], novelRhs: [0], lines: [{ rhs: 0 }] }], hasSyntacticChanges: false + }, + { + lhsSrc: src("gone.ts"), + rhsSrc: absent("gone.ts"), + hunks: [{ novelLhs: [0], novelRhs: [], lines: [{ lhs: 0 }] }], + hasSyntacticChanges: false } ], - { "base:a.ts": "one\n", "head:a.ts": "two\nthree\n", "head:new.ts": "hello\n" } + { + "base:a.ts": "one\n", + "head:a.ts": "two\nthree\n", + "head:new.ts": "hello\n", + "base:gone.ts": "bye\n" + } ); const diff = await loadPullDiff(ctx, git, params, "unified"); - expect(diff.stat).toEqual({ insertions: 3, deletions: 1, files_changed: 2 }); + expect(diff.stat).toEqual({ insertions: 3, deletions: 2, files_changed: 3 }); expect(diff.truncated).toBe(0); expect(diff.files[0]).toMatchObject({ name: "a.ts", @@ -87,7 +102,15 @@ describe("loadPullDiff", () => { oldFile: { contents: "" }, newFile: { contents: "hello\n" } }); - expect(calls.filter((path) => path.endsWith("repo.blob"))).toHaveLength(3); + expect(diff.files[2]).toMatchObject({ + name: "gone.ts", + kind: "deleted", + oldFile: { contents: "bye\n" }, + newFile: { contents: "" } + }); + // two for the changed file, one each for the sides that exist: the absent side + // is never fetched, it would 404 + expect(calls.filter((path) => path.endsWith("repo.blob"))).toHaveLength(4); // the server prerenders each pair so the first paint ships highlighted expect(diff.prerendered?.["a.ts→a.ts"]).toContain("three"); }); @@ -155,9 +178,16 @@ describe("loadPullInterdiff", () => { rhsSrc: src("b.ts"), hunks: [], hasSyntacticChanges: false + }, + // an added file has no left side to inline in the first place + { + lhsSrc: absent("c.ts"), + rhsSrc: src("c.ts"), + hunks: [{ novelLhs: [], novelRhs: [0], lines: [{ rhs: 0 }] }], + hasSyntacticChanges: false } ], - { "v2head:a.ts": "two\n", "v2head:b.ts": "two\n" } + { "v2head:a.ts": "two\n", "v2head:b.ts": "two\n", "v2head:c.ts": "new\n" } ); const diff = await loadPullInterdiff(ctx, git, interdiffParams, "unified"); @@ -167,7 +197,12 @@ describe("loadPullInterdiff", () => { newFile: { name: "a.ts", contents: "two\n" } }); expect(diff.files[1].note).toBe("Contents could not be loaded."); + expect(diff.files[2]).toMatchObject({ + kind: "new", + oldFile: { contents: "" }, + newFile: { name: "c.ts", contents: "new\n" } + }); // one blob per file, the right side only - expect(calls.filter((path) => path.endsWith("repo.blob"))).toHaveLength(2); + expect(calls.filter((path) => path.endsWith("repo.blob"))).toHaveLength(3); }); }); diff --git a/web/src/lib/api/pullDiff.ts b/web/src/lib/api/pullDiff.ts index 119972b04..cb38b00ad 100644 --- a/web/src/lib/api/pullDiff.ts +++ b/web/src/lib/api/pullDiff.ts @@ -4,6 +4,7 @@ import { blob } from "./gitclient"; import { diffFileKind, diffFileStat, + diffSideExists, getDiff, getInterdiff, type MirrorFileDiff, @@ -44,9 +45,14 @@ export interface PullDiff { // the diff endpoint answers metadata only — `diffSrc` is path/oid/size/flags and // a hunk is aligned line numbers — so the text comes from the blob endpoint and // pierre diffs the two sides itself -const sideContents = async (git: GitTarget, ref: string, path: string): Promise => { - if (!path) return ""; - const output = await blob(git, { ref, path }, MAX_INLINE_SIZE).catch(() => null); +const sideContents = async ( + git: GitTarget, + ref: string, + src: MirrorFileDiff["lhsSrc"] +): Promise => { + // a side that doesn't exist at this ref is an empty side, not a failed fetch + if (!diffSideExists(src)) return ""; + const output = await blob(git, { ref, path: src.path }, MAX_INLINE_SIZE).catch(() => null); if (!output || output.encoding !== "utf-8") return null; return output.content ?? ""; }; @@ -120,8 +126,8 @@ export const loadPullDiff = async ( // the left side is read at baseCommit, while gitmirror diffs from // merge_base(base, head). identical when base is the merge base, which // is what a pull version stores - sideContents(git, params.baseCommit, file.lhsSrc.path), - sideContents(git, params.headCommit, file.rhsSrc.path) + sideContents(git, params.baseCommit, file.lhsSrc), + sideContents(git, params.headCommit, file.rhsSrc) ]) ); }; @@ -137,8 +143,8 @@ export const loadPullInterdiff = async ( ): Promise => { const { diffs } = await getInterdiff(ctx, params); return buildDiff(diffs, style, async (file) => [ - file.lhsSrc.path ? (file.lhsSrc.content ?? null) : "", - await sideContents(git, params.headCommit2, file.rhsSrc.path) + diffSideExists(file.lhsSrc) ? (file.lhsSrc.content ?? null) : "", + await sideContents(git, params.headCommit2, file.rhsSrc) ]); }; diff --git a/web/src/lib/components/repo/PierreDiff.svelte b/web/src/lib/components/repo/PierreDiff.svelte index 14fbdb695..54aaffd18 100644 --- a/web/src/lib/components/repo/PierreDiff.svelte +++ b/web/src/lib/components/repo/PierreDiff.svelte @@ -1,7 +1,7 @@