diff --git a/CHANGELOG.md b/CHANGELOG.md index 34e2085..adf7a4d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,42 @@ # Changelog +## Unreleased + +### Fixed — data loss + +- **`gwt rm` could half-delete a worktree, and then could not finish the job.** + With a dev server still running, `git worktree remove` deletes files while the + process recreates them, the final `rmdir` fails — and git has already + deregistered the worktree by then. The retry answers `No worktree matching`, + and the half-emptied directory can only be cleared by hand. Observed on a + worktree with 51 live processes: 144 KB of orphans left behind, including the + repository's `CLAUDE.md` and `eslint.config.mjs`. + + `rm` now looks for processes running under the worktree first, names them, and + refuses — the same shape as the existing dirty-tree guard, which tells you what + to do rather than doing it for you. It prints the `pkill` to run. + + **There is deliberately no flag to override it**, and `--force` does not. A + dirty tree is data you may be willing to lose; a live process is a mechanism + that corrupts the removal itself. The check is also a heuristic on command + lines, so a false positive is possible — refusing one costs a minute of + confusion, and stopping one would cost a process that mattered. + +### Fixed + +- **A branch created with `--from` inherited the base as its upstream.** `git + worktree add -b fix/x origin/main` sets `origin/main` as the upstream, + so a bare `git push` refuses — and suggests `git push origin HEAD:main`. Copy + that suggestion on a tired afternoon and the fix lands on the default branch. + `--from` now passes `--no-track`, and the same `git push` suggests + `git push --set-upstream origin fix/x` instead. + +- **`--from` branched off a stale base.** `add` fetched only when the branch + already existed, so creating a new branch from `--from main` used whatever the + local ref happened to be — weeks behind on a repository left alone, with + nothing said. It now fetches the base first, and when a local base is behind + its remote it says by how many commits and suggests `--from origin/`. + ## 0.8.4 Nothing to reinstall — the shell wrapper is unchanged. `gwt sync-env` copies diff --git a/src/commands/add.ts b/src/commands/add.ts index efe39ba..4398b51 100644 --- a/src/commands/add.ts +++ b/src/commands/add.ts @@ -1,6 +1,7 @@ import { intro, outro, log, select, isCancel } from "@clack/prompts"; import pc from "picocolors"; import { + fetchBase, getRepoRoot, getWorktreePath, worktreeExists, @@ -97,6 +98,10 @@ export async function commandAdd( if (from) { log.info(`Creating from base branch: ${from}`); + if (!branchAlreadyExists) { + log.step("Fetching base…"); + fetchBase(from); + } } log.step("Creating git worktree…"); diff --git a/src/commands/rm.ts b/src/commands/rm.ts index 446aecf..9aff02a 100644 --- a/src/commands/rm.ts +++ b/src/commands/rm.ts @@ -10,6 +10,7 @@ import { resolveWorktree } from "../lib/resolveWorktree.js"; import { runCommands } from "../lib/setup.js"; import { requestCd } from "../lib/shellCd.js"; import { resolveConfig, REPO_CONFIG_FILE } from "../lib/repoConfig.js"; +import { processesIn, describeProcesses } from "../lib/processes.js"; export async function commandRm( query?: string, @@ -34,6 +35,29 @@ export async function commandRm( process.exit(1); } + // Refused, never offered as a prompt — the same shape as the dirty-tree guard + // above. Two reasons. Terminating someone's dev servers is not this command's + // business: it removes a worktree, and it has no idea whether one of those + // processes is a debugger someone is standing in. And the detection is a + // heuristic on command lines, so a false positive is possible — refusing one + // costs a minute of confusion, killing one costs a process that mattered. + // + // There is deliberately no flag to override it. A dirty tree is data you may + // be willing to lose; a live process is a mechanism that corrupts the removal + // itself, and no amount of certainty makes that safe. + const running = processesIn(worktreePath); + if (running.length > 0) { + const names = describeProcesses(running); + log.error( + `${running.length} process(es) still running in this worktree` + + (names.length > 0 ? ` (${names.join(", ")})` : "") + + `.\n Removing now deletes files underneath them, and git leaves the` + + `\n directory half-emptied — a state gwt cannot repair.` + + `\n Stop them first: pkill -f ${worktreePath}`, + ); + process.exit(1); + } + // clack cannot open a prompt without a TTY — it dies with a raw // `uv_tty_init returned EINVAL`. Removal is destructive, so a script that // cannot be asked has to say up front that it means it. diff --git a/src/lib/git.ts b/src/lib/git.ts index a3cb364..f0de61a 100644 --- a/src/lib/git.ts +++ b/src/lib/git.ts @@ -1,6 +1,7 @@ import { execFileSync } from "child_process"; import path from "path"; import fs from "fs"; +import { log } from "@clack/prompts"; export interface WorktreeEntry { path: string; @@ -132,12 +133,56 @@ export function addWorktree( from?: string, ): void { const root = getRepoRoot(); + // --no-track when branching off a base: git would otherwise set the new + // branch's upstream to that base, and a bare `git push` then fails with + // "the upstream branch of your current branch does not match the name of your + // current branch" and helpfully suggests `git push origin HEAD:production`. + // Copy that on a tired afternoon and a fix lands on the default branch. const args = branchExists(branch) ? ["worktree", "add", worktreePath, branch] - : ["worktree", "add", worktreePath, "-b", branch, ...(from ? [from] : [])]; + : [ + "worktree", + "add", + worktreePath, + ...(from ? ["--no-track"] : []), + "-b", + branch, + ...(from ? [from] : []), + ]; runVisible("git", args, root); } +/** + * Refreshes the remote counterpart of `base`, and says so when a local base is + * behind it. + * + * `add` only fetched when the branch already existed, so creating a new branch + * from `--from production` used whatever the local ref happened to be — weeks + * stale on a repository left alone, silently. + */ +export function fetchBase(base: string): void { + const root = getRepoRoot(); + const remoteless = base.startsWith("origin/") ? base.slice("origin/".length) : base; + try { + runVisible("git", ["fetch", "origin", remoteless], root); + } catch { + // A base with no remote counterpart is legitimate: a local-only branch, or + // a tag. Creating from it is still what was asked. + return; + } + + if (base.startsWith("origin/")) return; + const remote = `origin/${remoteless}`; + if (!succeeds("git", ["rev-parse", "--verify", remote], root)) return; + const behind = run("git", ["rev-list", "--count", `${base}..${remote}`], root); + if (behind !== "0") { + log.warn( + `Local '${base}' is ${behind} commit(s) behind ${remote}. ` + + `Pass --from ${remote} to branch from the remote instead.`, + ); + } +} + export function removeWorktree(worktreePath: string): void { const root = getRepoRoot(); runVisible("git", ["worktree", "remove", worktreePath, "--force"], root); diff --git a/src/lib/processes.ts b/src/lib/processes.ts new file mode 100644 index 0000000..69e0ce6 --- /dev/null +++ b/src/lib/processes.ts @@ -0,0 +1,60 @@ +import { execFileSync } from "child_process"; + +// `git worktree remove` deletes files one by one, so anything still writing +// under the worktree races it: the files go, a live process recreates one, and +// the final rmdir fails. git has already deregistered the worktree by then, so +// `gwt rm` answers "No worktree matching" on the retry and the half-emptied +// directory can only be finished off by hand. Refusing up front is the only +// version of this that stays repairable. + +// pgrep matches an extended regular expression against the full command line, +// and a path is full of characters that mean something there — a worktree for +// `release/1.0.0` would otherwise match paths it does not own. +function escapeForRegex(value: string): string { + return value.replace(/[.*+?^${}()|[\]\\]/g, "\\$&"); +} + +/** + * PIDs of processes whose command line mentions `worktreePath`. + * + * A dev server started inside a worktree carries the path in its argv — every + * `node /node_modules/...` shows up, and so does the Nx daemon. It is + * a heuristic rather than a file-handle audit, but it is the one that found 51 + * live processes under a worktree that `git worktree remove` had just started + * deleting. + */ +export function processesIn(worktreePath: string): number[] { + let out: string; + try { + out = execFileSync("pgrep", ["-f", "--", escapeForRegex(worktreePath)], { + encoding: "utf-8", + stdio: ["ignore", "pipe", "ignore"], + }); + } catch { + // pgrep exits 1 when nothing matches, and 127 when it is not installed. + // Neither is a reason to block a removal. + return []; + } + + const own = new Set([process.pid, process.ppid]); + return out + .split("\n") + .map((line) => Number(line.trim())) + .filter((pid) => Number.isInteger(pid) && pid > 0 && !own.has(pid)); +} + +/** Command names for `pids`, deduplicated, for a message a human can act on. */ +export function describeProcesses(pids: number[]): string[] { + const names = pids.map((pid) => { + try { + const comm = execFileSync("ps", ["-o", "comm=", "-p", String(pid)], { + encoding: "utf-8", + stdio: ["ignore", "pipe", "ignore"], + }).trim(); + return comm.split("/").pop() || "?"; + } catch { + return "?"; + } + }); + return [...new Set(names)].filter((name) => name !== "?"); +} diff --git a/tests/add-from-base.test.ts b/tests/add-from-base.test.ts new file mode 100644 index 0000000..d22de1a --- /dev/null +++ b/tests/add-from-base.test.ts @@ -0,0 +1,75 @@ +import { test, describe } from "node:test"; +import assert from "node:assert/strict"; +import path from "node:path"; +import { tmpRepoWithRemote, runCli, git } from "./helpers/fixtures.ts"; + +function siblingOf(repo: string, branch: string): string { + return path.join( + path.dirname(repo), + `${path.basename(repo)}-${branch.replace(/\//g, "-")}`, + ); +} + +function upstreamOf(worktree: string): string | null { + try { + return git(worktree, "rev-parse", "--abbrev-ref", "@{upstream}").trim(); + } catch { + return null; + } +} + +describe("gwt add --from", () => { + // Branching off a remote ref makes git set that ref as the upstream, and a + // bare `git push` then refuses with a suggestion to push to the base branch + // itself — `git push origin HEAD:main`. On a support fix that is one tired + // copy-paste away from landing on the default branch. + test("leaves the new branch without an upstream", () => { + const { repo } = tmpRepoWithRemote(); + + const result = runCli(["add", "fix/x", "--from", "origin/main"], { cwd: repo }); + assert.equal(result.code, 0, result.output); + + assert.equal( + upstreamOf(siblingOf(repo, "fix/x")), + null, + "a branch created from a base must not inherit the base as its upstream", + ); + }); + + test("still tracks normally when no base is given", () => { + const { repo } = tmpRepoWithRemote(); + const result = runCli(["add", "plain"], { cwd: repo }); + assert.equal(result.code, 0, result.output); + // No base, no -b from a remote ref: nothing to inherit either way, but the + // worktree has to exist and be usable. + assert.match(git(repo, "worktree", "list"), /plain/); + }); + + // `add` only fetched when the branch already existed, so a new branch created + // from `--from main` used whatever the local ref happened to be. + test("fetches the base, so the worktree starts from the remote's tip", () => { + const { repo, origin } = tmpRepoWithRemote(); + + // Someone else pushes while this clone is not looking. + const other = path.join(path.dirname(repo), "other"); + git(path.dirname(repo), "clone", "-q", origin, other); + git(other, "config", "user.email", "them@example.com"); + git(other, "config", "user.name", "Them"); + // The bare origin's HEAD still points at whatever init.defaultBranch was + // when it was created, which is not `main` on every runner — so the clone + // may land nowhere. Name the branch rather than trusting the checkout. + git(other, "checkout", "-q", "-B", "main", "origin/main"); + git(other, "commit", "-q", "--allow-empty", "-m", "landed after our last fetch"); + git(other, "push", "-q", "origin", "main"); + + const result = runCli(["add", "fix/y", "--from", "origin/main"], { cwd: repo }); + assert.equal(result.code, 0, result.output); + + const log = git(siblingOf(repo, "fix/y"), "log", "--oneline"); + assert.match( + log, + /landed after our last fetch/, + "the base was refreshed before branching", + ); + }); +}); diff --git a/tests/rm-live-worktree.test.ts b/tests/rm-live-worktree.test.ts new file mode 100644 index 0000000..f00847f --- /dev/null +++ b/tests/rm-live-worktree.test.ts @@ -0,0 +1,85 @@ +import { test, describe, after } from "node:test"; +import assert from "node:assert/strict"; +import { spawn, type ChildProcess } from "node:child_process"; +import { tmpRepo, tmpRepoWithRemote, runCli, git } from "./helpers/fixtures.ts"; +import path from "node:path"; +import { processesIn, describeProcesses } from "../dist/lib/processes.js"; + +const spawned: ChildProcess[] = []; +after(() => { + for (const child of spawned) child.kill("SIGKILL"); +}); + +/** A process whose argv carries `dir` — the shape a dev server started there has. */ +function sleeperIn(dir: string): ChildProcess { + const child = spawn( + process.execPath, + ["-e", `setTimeout(() => {}, 60000); // ${path.join(dir, "node_modules", "marker")}`], + { stdio: "ignore", detached: false }, + ); + spawned.push(child); + return child; +} + +describe("processesIn", () => { + test("finds a process whose command line carries the worktree path", async () => { + const dir = tmpRepo("live"); + assert.deepEqual(processesIn(dir), [], "clean before anything runs"); + + const child = sleeperIn(dir); + await new Promise((r) => setTimeout(r, 300)); + + const found = processesIn(dir); + assert.ok(found.includes(child.pid!), `expected ${child.pid} in ${found}`); + assert.ok(!found.includes(process.pid), "never reports the running gwt itself"); + assert.ok(describeProcesses(found).length > 0, "names the processes for the message"); + }); + + test("an unrelated directory is unaffected", async () => { + const mine = tmpRepo("mine"); + const other = tmpRepo("other"); + sleeperIn(mine); + await new Promise((r) => setTimeout(r, 300)); + assert.deepEqual(processesIn(other), []); + }); +}); + +describe("gwt rm with a live worktree", () => { + // The failure this guards: `git worktree remove` deletes files under a running + // process, the final rmdir fails, and git has already deregistered the + // worktree — so the retry says "No worktree matching" and the half-emptied + // directory can only be finished by hand. + // Refused, not prompted — the same shape as the dirty-tree guard, and for a + // stronger reason: the detection is a heuristic, so a false positive must cost + // a confused user rather than a killed process. + test("refuses, and --force does not override it", async () => { + const { repo } = tmpRepoWithRemote(); + runCli(["add", "doomed"], { cwd: repo }); + const worktree = path.join(path.dirname(repo), `${path.basename(repo)}-doomed`); + + sleeperIn(worktree); + await new Promise((r) => setTimeout(r, 300)); + + const result = runCli(["rm", "doomed", "--force"], { cwd: repo }); + assert.equal(result.code, 1, "exits non-zero rather than half-removing"); + assert.match(result.output, /still running/); + assert.match(result.output, /pkill -f/, "says how to clear it"); + assert.doesNotMatch( + result.output, + /Stop them and remove|Stopped \d+ process/, + "never offers to kill them", + ); + + const listed = git(repo, "worktree", "list"); + assert.match(listed, /doomed/, "the worktree is still registered"); + }); + + test("removes normally once nothing is running there", () => { + const { repo } = tmpRepoWithRemote(); + runCli(["add", "quiet"], { cwd: repo }); + + const result = runCli(["rm", "quiet", "--force"], { cwd: repo }); + assert.equal(result.code, 0, result.output); + assert.doesNotMatch(git(repo, "worktree", "list"), /quiet/); + }); +});