diff --git a/src/git.ts b/src/git.ts index 8cb08752..aa5f646b 100644 --- a/src/git.ts +++ b/src/git.ts @@ -12,7 +12,7 @@ export interface GitCommandResult { export interface GitEligibility { ok: boolean; gitRoot?: string; - reason?: "not_git" | "no_head"; + reason?: "not_git"; message?: string; } @@ -42,17 +42,6 @@ export async function getGitEligibility(cwd: string): Promise { } const gitRoot = (await git(cwd, ["rev-parse", "--show-toplevel"])).stdout.trim(); - try { - await git(gitRoot, ["rev-parse", "--verify", "--quiet", "HEAD^{commit}"]); - } catch { - return { - ok: false, - gitRoot, - reason: "no_head", - message: "repository has no HEAD commit", - }; - } - return { ok: true, gitRoot }; } diff --git a/src/review-checkpoints.test.ts b/src/review-checkpoints.test.ts index 0c2aeb7b..d2f1c868 100644 --- a/src/review-checkpoints.test.ts +++ b/src/review-checkpoints.test.ts @@ -1,6 +1,6 @@ import assert from "node:assert/strict"; import { execFile } from "node:child_process"; -import { mkdtemp, rm, writeFile } from "node:fs/promises"; +import { mkdir, mkdtemp, rename, rm, writeFile } from "node:fs/promises"; import { tmpdir } from "node:os"; import { join } from "node:path"; import test, { type TestContext } from "node:test"; @@ -50,6 +50,82 @@ test("show_changes reports and advances the last-shown checkpoint", async (t) => assert.equal(afterReviewed.patch, ""); }); +test("a nested workspace only reviews changes inside its workspace root", async (t) => { + const repositoryRoot = await committedRepository(t); + const workspaceRoot = join(repositoryRoot, "packages", "app"); + const siblingRoot = join(repositoryRoot, "packages", "other"); + await mkdir(workspaceRoot, { recursive: true }); + await mkdir(siblingRoot, { recursive: true }); + await writeFile(join(workspaceRoot, "app.txt"), "app\n"); + await writeFile(join(siblingRoot, "other.txt"), "other\n"); + await git(repositoryRoot, ["add", "."]); + await git(repositoryRoot, ["commit", "-m", "Add packages"]); + + const manager = createReviewCheckpointManager(); + await manager.initializeWorkspace({ workspaceId: "ws_nested", root: workspaceRoot }); + + await writeFile(join(workspaceRoot, "app.txt"), "app changed\n"); + await writeFile(join(siblingRoot, "other.txt"), "other changed\n"); + + const review = await manager.reviewChanges({ + workspaceId: "ws_nested", + root: workspaceRoot, + markReviewed: false, + }); + + assert.deepEqual(review.files.map((file) => file.path), ["app.txt"]); + assert.match(review.patch, /app changed/); + assert.doesNotMatch(review.patch, /other changed/); +}); + +test("binary changes use a renderable review patch instead of a Git binary patch", async (t) => { + const root = await committedRepository(t); + await writeFile(join(root, "asset.bin"), Buffer.from([0, 1, 2, 3])); + await git(root, ["add", "asset.bin"]); + await git(root, ["commit", "-m", "Add binary asset"]); + + const manager = createReviewCheckpointManager(); + await manager.initializeWorkspace({ workspaceId: "ws_binary", root }); + await writeFile(join(root, "asset.bin"), Buffer.from([0, 1, 9, 3])); + + const review = await manager.reviewChanges({ + workspaceId: "ws_binary", + root, + markReviewed: false, + }); + + assert.deepEqual(review.files.map((file) => file.path), ["asset.bin"]); + assert.match(review.patch, /Binary files/); + assert.doesNotMatch(review.patch, /GIT binary patch/); +}); + +test("review metadata preserves pure renames from the rendered patch", async (t) => { + const root = await committedRepository(t); + await writeFile(join(root, "before.txt"), "same content\n"); + await git(root, ["add", "before.txt"]); + await git(root, ["commit", "-m", "Add rename source"]); + + const manager = createReviewCheckpointManager(); + await manager.initializeWorkspace({ workspaceId: "ws_rename", root }); + await rename(join(root, "before.txt"), join(root, "after.txt")); + + const review = await manager.reviewChanges({ + workspaceId: "ws_rename", + root, + markReviewed: false, + }); + + assert.deepEqual(review.files, [ + { + path: "after.txt", + previousPath: "before.txt", + type: "rename-pure", + additions: 0, + removals: 0, + }, + ]); +}); + test("review checkpoints survive a manager restart", async (t) => { const root = await committedRepository(t); const manager = createReviewCheckpointManager(); @@ -173,27 +249,21 @@ test("a concurrent review rejects a different root after initialization", async } }); -test("an unborn repository becomes reviewable after its first commit", async (t) => { +test("an unborn repository is reviewable without creating a commit", async (t) => { const root = await unbornRepository(t); const manager = createReviewCheckpointManager(); await manager.initializeWorkspace({ workspaceId: "ws_unborn", root }); - await assert.rejects( - () => manager.reviewChanges({ workspaceId: "ws_unborn", root }), - /repository has no HEAD commit/, - ); + await writeFile(join(root, "README.md"), "first file\n"); - await writeFile(join(root, "README.md"), "first commit\n"); - await git(root, ["add", "README.md"]); - await git(root, ["commit", "-m", "Initial commit"]); - - const afterFirstCommit = await manager.reviewChanges({ + const review = await manager.reviewChanges({ workspaceId: "ws_unborn", root, markReviewed: false, }); - assert.equal(afterFirstCommit.summary.files, 0); - assert.equal(afterFirstCommit.patch, ""); + assert.deepEqual(review.files.map((file) => file.path), ["README.md"]); + assert.equal(review.files[0]?.type, "new"); + assert.match(review.patch, /first file/); }); async function committedRepository(t: TestContext): Promise { diff --git a/src/review-checkpoints.ts b/src/review-checkpoints.ts index 0fd8bf36..ec998154 100644 --- a/src/review-checkpoints.ts +++ b/src/review-checkpoints.ts @@ -1,6 +1,7 @@ import { mkdtemp, rm } from "node:fs/promises"; import { tmpdir } from "node:os"; import { join } from "node:path"; +import { parsePatchFiles } from "@pierre/diffs"; import { git, getGitEligibility, safeWorkspaceRefSegment } from "./git.js"; export type ReviewSince = "last_shown" | "workspace_open"; @@ -47,6 +48,7 @@ export interface ReviewCheckpointManager { } const REVIEW_REF_PREFIX = "refs/devspace/review"; +const REVIEW_DIFF_MAX_BUFFER = 10_000_000; export function createReviewCheckpointManager(): ReviewCheckpointManager { const states = new Map(); @@ -107,14 +109,21 @@ export function createReviewCheckpointManager(): ReviewCheckpointManager { const baselineRef = effectiveSince === "workspace_open" ? state.openRef : state.baselineRef; const baseline = (await git(state.gitRoot, ["rev-parse", "--verify", `${baselineRef}^{commit}`])).stdout.trim(); - const current = await createWorkingTreeSnapshot(state.gitRoot); - const patch = (await git(state.gitRoot, ["diff", "--binary", "--no-color", baseline, current], { - maxBuffer: 50 * 1024 * 1024, + const current = await createWorkingTreeSnapshot(state.gitRoot, state.root); + const patch = (await git(state.root, [ + "diff", + "--relative", + "--patch", + "--find-renames", + "--no-color", + "--no-ext-diff", + "--no-textconv", + baseline, + current, + ], { + maxBuffer: REVIEW_DIFF_MAX_BUFFER, })).stdout; - const numstat = (await git(state.gitRoot, ["diff", "--numstat", "-z", baseline, current], { - maxBuffer: 50 * 1024 * 1024, - })).stdout; - const files = parseNumstat(numstat); + const files = parseReviewFiles(patch); const summary = summarizeFiles(files); if (markReviewed) { @@ -139,6 +148,47 @@ export function createReviewCheckpointManager(): ReviewCheckpointManager { }; } +function parseReviewFiles(patch: string): ReviewFile[] { + if (patch.length === 0) return []; + + try { + return parsePatchFiles(patch, "review", true).flatMap((parsedPatch) => + parsedPatch.files.map((file) => { + const stats = file.hunks.reduce( + (total, hunk) => ({ + additions: total.additions + hunk.additionLines, + removals: total.removals + hunk.deletionLines, + }), + { additions: 0, removals: 0 }, + ); + + return { + path: file.name, + previousPath: file.prevName, + type: reviewFileType(file.type), + ...stats, + }; + }), + ); + } catch (error) { + const detail = error instanceof Error ? error.message : String(error); + throw new Error(`Review diff could not be rendered: ${detail}`); + } +} + +function reviewFileType(type: string): ReviewFile["type"] { + switch (type) { + case "rename-pure": + case "rename-changed": + case "new": + case "deleted": + case "change": + return type; + default: + return "change"; + } +} + function assertWorkspaceRoot( state: WorkspaceReviewState | undefined, workspaceId: string, @@ -175,7 +225,7 @@ async function initializeWorkspaceState( ]); if (!openCommit && !baselineCommit) { - const initialCommit = await createWorkingTreeSnapshot(eligibility.gitRoot); + const initialCommit = await createWorkingTreeSnapshot(eligibility.gitRoot, root); await git(eligibility.gitRoot, ["update-ref", state.openRef, initialCommit]); await git(eligibility.gitRoot, ["update-ref", state.baselineRef, initialCommit]); state.openRefAvailable = true; @@ -215,17 +265,18 @@ function reviewRefs( }; } -async function createWorkingTreeSnapshot(gitRoot: string): Promise { +async function createWorkingTreeSnapshot(gitRoot: string, workspaceRoot: string): Promise { const tempDir = await mkdtemp(join(tmpdir(), "devspace-review-index-")); const indexPath = join(tempDir, "index"); const env = checkpointEnv(indexPath); try { - await git(gitRoot, ["read-tree", "HEAD"], { env }); - await git(gitRoot, ["add", "-A", "--", "."], { env }); + if (await commitForRef(gitRoot, "HEAD")) { + await git(gitRoot, ["read-tree", "HEAD"], { env }); + } + await git(workspaceRoot, ["add", "-A", "--", "."], { env }); const tree = (await git(gitRoot, ["write-tree"], { env })).stdout.trim(); - const parent = (await git(gitRoot, ["rev-parse", "--verify", "HEAD^{commit}"])).stdout.trim(); - return (await git(gitRoot, ["commit-tree", tree, "-p", parent, "-m", "DevSpace review snapshot"], { env })).stdout.trim(); + return (await git(gitRoot, ["commit-tree", tree, "-m", "DevSpace review snapshot"], { env })).stdout.trim(); } finally { await rm(tempDir, { recursive: true, force: true }); } @@ -241,56 +292,6 @@ function checkpointEnv(indexPath: string): NodeJS.ProcessEnv { }; } -function parseNumstat(output: string): ReviewFile[] { - const fields = output.split("\0").filter((field) => field.length > 0); - const files: ReviewFile[] = []; - - for (let index = 0; index < fields.length;) { - const header = fields[index++] ?? ""; - const parts = header.split("\t"); - const additions = parseStatNumber(parts[0]); - const removals = parseStatNumber(parts[1]); - - if (parts.length >= 3) { - const path = parts[2] ?? ""; - if (path) files.push({ path, type: fileType(path, undefined, additions, removals), additions, removals }); - continue; - } - - const previousPath = fields[index++]; - const path = fields[index++]; - if (!path) continue; - - files.push({ - path, - previousPath, - type: fileType(path, previousPath, additions, removals), - additions, - removals, - }); - } - - return files; -} - -function parseStatNumber(value: string | undefined): number { - if (!value || value === "-") return 0; - const parsed = Number(value); - return Number.isFinite(parsed) ? parsed : 0; -} - -function fileType( - path: string, - previousPath: string | undefined, - additions: number, - removals: number, -): ReviewFile["type"] { - if (previousPath) return additions === 0 && removals === 0 ? "rename-pure" : "rename-changed"; - if (additions > 0 && removals === 0) return "new"; - if (additions === 0 && removals > 0) return "deleted"; - return "change"; -} - function summarizeFiles(files: ReviewFile[]): ReviewSummary { return files.reduce( (summary, file) => ({ diff --git a/src/ui/review-payload.tsx b/src/ui/review-payload.tsx index 455e5472..546c55ae 100644 --- a/src/ui/review-payload.tsx +++ b/src/ui/review-payload.tsx @@ -25,6 +25,11 @@ interface MountedPayload { unmount(): void; } +interface ParsedReviewPatch { + files: FileDiffMetadata[]; + error?: string; +} + export function mountReviewPayload( container: HTMLElement, options: PayloadRendererOptions, @@ -50,13 +55,15 @@ function ReviewPayload({ }: PayloadRendererOptions) { const patch = card.payload?.patch; const themeType: ThemeType = hostContext?.theme === "light" ? "light" : "dark"; - const files = useMemo(() => parseFiles(patch), [patch]); + const parsedPatch = useMemo(() => parseFiles(patch), [patch]); + const files = parsedPatch.files; const visibleFiles = typeof visibleFileCount === "number" ? files.slice(0, visibleFileCount) : files; const [openFiles, setOpenFiles] = useState(() => new Set()); if (errorMessage) return ; + if (parsedPatch.error) return ; if (!patch) return ; if (files.length === 0) return ; @@ -177,9 +184,19 @@ function fileChangeSymbol(kind: FileChangeKind): string { } } -function parseFiles(patch: string | undefined): FileDiffMetadata[] { - if (!patch) return []; - return parsePatchFiles(patch, "review", true).flatMap((parsedPatch) => parsedPatch.files); +function parseFiles(patch: string | undefined): ParsedReviewPatch { + if (!patch) return { files: [] }; + + try { + return { + files: parsePatchFiles(patch, "review", true).flatMap((parsedPatch) => parsedPatch.files), + }; + } catch { + return { + files: [], + error: "Review diff could not be rendered.", + }; + } } function diffStats(fileDiff: FileDiffMetadata): { additions: number; removals: number } {