From f19a3cb7a3d034c49f8528a640cdf2f86bb10bf0 Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Wed, 8 Jul 2026 01:20:12 -0700 Subject: [PATCH] fix(selfhost): distinguish missing-ref and detached-HEAD errors in update script selfhost-update.sh's ff-only merge failure previously printed the same "diverged history" message for two unrelated causes: a genuine non-fast-forward divergence, and a bad SELFHOST_UPDATE_BRANCH/ SELFHOST_UPDATE_REMOTE override pointing at a ref that doesn't exist at all (git merge --ff-only fails the same way for both). Add an explicit `git rev-parse --verify` check right after the fetch so a missing ref gets its own distinct, more actionable error instead of being misreported as a divergence. Also give a checkout in a detached HEAD state (where `git rev-parse --abbrev-ref HEAD` literally returns "HEAD") its own message instead of the generic branch-mismatch one, which read oddly ("currently on 'HEAD', expected 'main'") and suggested a nonsensical SELFHOST_UPDATE_BRANCH=HEAD override. Also fix an inaccurate comment in test/unit/docs-selfhost-git-deploy-hygiene.test.ts's tracked-file-shadowing test: .gitignore has no effect on files git already tracks (verified: `git add -A` still stages a modification to an already-tracked-but-now-ignored file). The real risk the test guards against is the opposite direction -- a future PR adding a genuinely new tracked file whose name happens to match the pattern would have it silently excluded from `git status`'s untracked list and from `git add -A`/`git add .`, not "untracked on reclone" as the comment previously and incorrectly claimed. Add test coverage for both new error paths (detached HEAD, and SELFHOST_UPDATE_BRANCH naming a branch the remote doesn't have) to selfhost-update-script.test.ts. Both nits were flagged by the Gittensory Orb review on #4151 (merged). Closes #4156 --- scripts/selfhost-update.sh | 12 ++++++++ .../docs-selfhost-git-deploy-hygiene.test.ts | 13 ++++++--- test/unit/selfhost-update-script.test.ts | 28 +++++++++++++++++++ 3 files changed, 49 insertions(+), 4 deletions(-) diff --git a/scripts/selfhost-update.sh b/scripts/selfhost-update.sh index d9c5878382..5c269a65d0 100755 --- a/scripts/selfhost-update.sh +++ b/scripts/selfhost-update.sh @@ -46,6 +46,11 @@ fi cd "$SCRIPT_DIR/.." current_branch="$(git rev-parse --abbrev-ref HEAD)" +if [ "$current_branch" = "HEAD" ]; then + echo "error: checkout is in a detached HEAD state, expected to be on '$BRANCH' -- checkout" \ + "$BRANCH first (this script only updates a branch-tracking checkout)" >&2 + exit 1 +fi if [ "$current_branch" != "$BRANCH" ]; then echo "error: currently on '$current_branch', expected '$BRANCH' -- checkout $BRANCH first, or" \ "set SELFHOST_UPDATE_BRANCH=$current_branch if that is deliberate" >&2 @@ -61,6 +66,13 @@ fi echo "selfhost update: fetching $REMOTE" git fetch "$REMOTE" +if ! git rev-parse --verify --quiet "$REMOTE/$BRANCH" >/dev/null; then + echo "error: $REMOTE/$BRANCH does not exist after fetching $REMOTE -- check" \ + "SELFHOST_UPDATE_REMOTE/SELFHOST_UPDATE_BRANCH for a typo, or confirm $REMOTE actually has a" \ + "'$BRANCH' branch" >&2 + exit 1 +fi + echo "selfhost update: fast-forwarding $BRANCH to $REMOTE/$BRANCH" if ! git merge --ff-only "$REMOTE/$BRANCH"; then echo "error: $BRANCH could not be fast-forwarded to $REMOTE/$BRANCH -- local history has" \ diff --git a/test/unit/docs-selfhost-git-deploy-hygiene.test.ts b/test/unit/docs-selfhost-git-deploy-hygiene.test.ts index b30949a888..79efe59430 100644 --- a/test/unit/docs-selfhost-git-deploy-hygiene.test.ts +++ b/test/unit/docs-selfhost-git-deploy-hygiene.test.ts @@ -31,10 +31,15 @@ describe("self-host git-deploy hygiene (#1660)", () => { }); it("does not shadow any file actually tracked in the repo", () => { - // The real regression concern: a future PR could add a legitimately-tracked file whose name - // happens to match `*.bak-*` or `*.backup-*`, which would silently untrack it the moment - // someone re-clones. Ask git itself, rather than approximating the glob in JS, since git's - // own matcher is the one that actually enforces these patterns. + // The real regression concern: .gitignore has no effect on a file git already tracks -- it + // keeps being tracked, staged, and diffed normally forever, ignore pattern or not (verified: + // `git add -A` still stages a modification to an already-tracked-but-now-ignored file). The + // actual risk is the opposite direction -- a future PR that genuinely intends to add a NEW + // tracked file whose name happens to match `*.bak-*` or `*.backup-*` would have that file + // silently excluded from `git status`'s untracked list and from `git add -A`/`git add .`, so + // it could go uncommitted without anyone noticing (an explicit `git add ` at least warns + // and needs `-f`; a broad add just skips it quietly). Ask git itself, rather than approximating + // the glob in JS, since git's own matcher is the one that actually enforces these patterns. const result = spawnSync("git", ["ls-files"], { encoding: "utf8" }); expect(result.status).toBe(0); const trackedFiles = result.stdout.split("\n").filter(Boolean); diff --git a/test/unit/selfhost-update-script.test.ts b/test/unit/selfhost-update-script.test.ts index 8746209817..620873511d 100644 --- a/test/unit/selfhost-update-script.test.ts +++ b/test/unit/selfhost-update-script.test.ts @@ -182,6 +182,34 @@ describe("selfhost-update.sh", () => { expect(readCallLog(callLog)).toBe(""); }); + it("refuses a detached HEAD with a distinct message instead of the generic branch mismatch", () => { + const { seedDir, checkoutDir, callLog } = createSandbox(); + advanceOrigin(seedDir, "advance for detached-head"); + git(["checkout", "-q", "--detach", "HEAD"], checkoutDir); + + const result = run(checkoutDir, callLog); + + expect(result.status).not.toBe(0); + expect(result.stderr).toContain("detached HEAD state"); + expect(result.stderr).not.toContain("currently on 'HEAD'"); + expect(readCallLog(callLog)).toBe(""); + }); + + it("gives a distinct error when SELFHOST_UPDATE_BRANCH names a branch the remote doesn't have", () => { + const { checkoutDir, callLog } = createSandbox(); + // The local checkout must actually be on the named branch for the branch-mismatch check to + // pass, so this exercises the *next* guard: the branch exists locally but has no upstream + // counterpart to fast-forward from. + git(["checkout", "-q", "-b", "no-such-branch-upstream"], checkoutDir); + + const result = run(checkoutDir, callLog, { SELFHOST_UPDATE_BRANCH: "no-such-branch-upstream" }); + + expect(result.status).not.toBe(0); + expect(result.stderr).toContain("origin/no-such-branch-upstream does not exist"); + expect(result.stderr).not.toContain("could not be fast-forwarded"); + expect(readCallLog(callLog)).toBe(""); + }); + it("accepts a non-default branch when SELFHOST_UPDATE_BRANCH names it explicitly", () => { const { seedDir, checkoutDir, callLog } = createSandbox(); git(["checkout", "-q", "-b", "release"], seedDir);