From da129416f12eed18aa601a0bb952f0f0009552d7 Mon Sep 17 00:00:00 2001 From: hugocasa Date: Fri, 28 Aug 2026 16:32:18 +0200 Subject: [PATCH] fix(cli): stop the migration guard misreading its own git history `gitRecordedDatatableMigrationPaths` decides whether a data table migration the working tree no longer has was ever tracked, and a push keeps the migration when the answer is no. Two ways it answered wrongly: `core.quotePath` is on by default, so a path holding a non-ASCII byte came back C-quoted ("migrations/datatable/dt/1_caf\303\251.up.sql") and matched nothing the caller held, reading as never recorded. `git log HEAD` fails on a repository with no commits, which fell into "its history could not be read" and told the user to check that git runs correctly. Git runs fine; there is nothing committed yet, and that is what it now says. Both are guarded by a temp-repo test, since the answer is a property of git's output rather than of any logic testable without it. Co-Authored-By: Claude Opus 5 --- cli/src/utils/git.ts | 27 ++++++- cli/test/git_recorded_migrations_unit.test.ts | 76 +++++++++++++++++++ 2 files changed, 102 insertions(+), 1 deletion(-) create mode 100644 cli/test/git_recorded_migrations_unit.test.ts diff --git a/cli/src/utils/git.ts b/cli/src/utils/git.ts index 5ca06f427a..77f187c312 100644 --- a/cli/src/utils/git.ts +++ b/cli/src/utils/git.ts @@ -156,9 +156,34 @@ export function gitRecordedDatatableMigrationPaths(): RecordedMigrationPaths { } const prefix = (prefixOut.stdout ?? "").trim(); + // `git log HEAD` fails on a repository with no commits, which is not the same as + // git being broken and must not be reported as such. + const head = spawnSync("git", ["rev-parse", "--verify", "--quiet", "HEAD"], { + encoding: "utf8", + stdio: "pipe", + }); + if ((head.status ?? 1) !== 0) { + return { + kind: "unknown", + reason: "this repository has no commits yet", + remedy: "Commit the migrations you sync", + }; + } + + // `core.quotePath` (on by default) C-quotes any path with a non-ASCII byte, which + // matches nothing the caller holds and reads as "never tracked". const r = spawnSync( "git", - ["log", "HEAD", "--format=", "--name-only", "--", "migrations/datatable"], + [ + "-c", + "core.quotePath=false", + "log", + "HEAD", + "--format=", + "--name-only", + "--", + "migrations/datatable", + ], { encoding: "utf8", stdio: "pipe", maxBuffer: 64 * 1024 * 1024 }, ); if ((r.status ?? 1) !== 0) { diff --git a/cli/test/git_recorded_migrations_unit.test.ts b/cli/test/git_recorded_migrations_unit.test.ts new file mode 100644 index 0000000000..d087c14bde --- /dev/null +++ b/cli/test/git_recorded_migrations_unit.test.ts @@ -0,0 +1,76 @@ +/** + * `gitRecordedDatatableMigrationPaths` is what a push consults before deleting a data + * table migration the working tree no longer has: a path in history is a real deletion, + * a path never recorded is one this clone may simply never have synced. Both failures + * pinned here made it answer "never recorded" or "git is broken" about a repository + * that was neither, so a migration the user committed would have been kept and the + * remedy offered would have been the wrong one. + * + * Runs real git in a temp repo — the answer is a property of git's output, not of any + * logic that could be tested without it. + */ + +import { afterEach, beforeEach, describe, expect, test } from "bun:test"; +import { execFileSync } from "node:child_process"; +import * as fs from "node:fs"; +import * as os from "node:os"; +import * as path from "node:path"; +import { gitRecordedDatatableMigrationPaths } from "../src/utils/git.ts"; + +let dir: string; +let cwd: string; + +const git = (...args: string[]) => + execFileSync("git", args, { cwd: dir, encoding: "utf8", stdio: "pipe" }); + +const commitMigration = (name: string) => { + const rel = `migrations/datatable/dt/${name}`; + fs.mkdirSync(path.join(dir, path.dirname(rel)), { recursive: true }); + fs.writeFileSync(path.join(dir, rel), "select 1;\n"); + git("add", "-A"); + git("commit", "-m", `add ${name}`); + return rel; +}; + +beforeEach(() => { + cwd = process.cwd(); + dir = fs.mkdtempSync(path.join(os.tmpdir(), "wmill-git-")); + git("init", "-q", "-b", "main"); + git("config", "user.email", "t@t.dev"); + git("config", "user.name", "t"); + process.chdir(dir); +}); + +afterEach(() => { + process.chdir(cwd); + fs.rmSync(dir, { recursive: true, force: true }); +}); + +describe("gitRecordedDatatableMigrationPaths", () => { + test("a repository with no commits is not a broken one", () => { + const r = gitRecordedDatatableMigrationPaths(); + expect(r.kind).toBe("unknown"); + if (r.kind === "unknown") { + expect(r.reason).toBe("this repository has no commits yet"); + } + }); + + test("records a path with a non-ASCII byte unquoted", () => { + // core.quotePath would return "migrations/datatable/dt/1_caf\303\251.up.sql", + // which matches no path the caller holds and reads as never recorded. + const rel = commitMigration("20260101000000_café.up.sql"); + const r = gitRecordedDatatableMigrationPaths(); + expect(r.kind).toBe("known"); + if (r.kind === "known") expect(r.paths.has(rel)).toBe(true); + }); + + test("a committed migration stays recorded after its deletion is committed", () => { + const rel = commitMigration("20260101000000_a.up.sql"); + fs.rmSync(path.join(dir, rel)); + git("add", "-A"); + git("commit", "-m", "delete it"); + const r = gitRecordedDatatableMigrationPaths(); + expect(r.kind).toBe("known"); + if (r.kind === "known") expect(r.paths.has(rel)).toBe(true); + }); +});