Public: anyone can read this; the crew can change it.
A push to main alone starts the recombine pass, after the hook has answered #10057
Check · Review · Land · Live done — Landed and live in production.
mason/forge:mason/10040-a-push-to-main-alone-starts-the-recombin → main 4 commits fork 4 threads · 4 unresolved
5 files
changed +68
−7
src/core/recombine.ts +14 −1
| @@ -2,7 +2,8 @@ | ||
| 2 | 2 | * Bring every open kept-fork pull's `combined/N` up to date with the default |
| 3 | 3 | * branch (#9012). One pass per repository at a time: a call made during a |
| 4 | 4 | * pass sets one "again" flag, and when the pass ends exactly one more runs, |
| 5 | * reading the tip then. Nothing calls this yet; a hook will. | |
| 5 | * reading the tip then. The post-receive hook starts one, off its answer, for | |
| 6 | * a push that moves the default branch (`http/server.ts`). | |
| 6 | 7 | */ |
| 7 | 8 | |
| 8 | 9 | import type { Database } from "bun:sqlite"; |
| @@ -22,6 +23,17 @@ export interface RecombineForge { | ||
| 22 | 23 | } |
| 23 | 24 | |
| 24 | 25 | const running = new Map<string, { again: boolean; done: Promise<number> }>(); |
| 26 | const started = new Map<string, number>(); | |
| 27 | ||
| 28 | /** How many runs have been started for the repository: what a test counts. */ | |
| 29 | export function runsStarted(repoId: string): number { | |
| 30 | return started.get(repoId) ?? 0; | |
| 31 | } | |
| 32 | ||
| 33 | /** Resolves when the run now going (if any) has ended; starts and joins none. */ | |
| 34 | export async function idle(repoId: string): Promise<void> { | |
| 35 | await running.get(repoId)?.done; | |
| 36 | } | |
| 25 | 37 | |
| 26 | 38 | /** One pass: the tip is read once, each pull combined against it. */ |
| 27 | 39 | async function pass(forge: RecombineForge, repoId: string): Promise<void> { |
| @@ -61,6 +73,7 @@ export function recombine(forge: RecombineForge, repoId: string): Promise<number | ||
| 61 | 73 | current.again = true; |
| 62 | 74 | return current.done; |
| 63 | 75 | } |
| 76 | started.set(repoId, runsStarted(repoId) + 1); | |
| 64 | 77 | const state = { again: false, done: Promise.resolve(0) }; |
| 65 | 78 | state.done = (async () => { |
| 66 | 79 | let passes = 0; |
src/http/server.ts +15
| @@ -68,6 +68,7 @@ import { | ||
| 68 | 68 | getRepoById, |
| 69 | 69 | } from "../core/repos.ts"; |
| 70 | 70 | import { trackPullHead } from "../core/pulls.ts"; |
| 71 | import { recombine } from "../core/recombine.ts"; | |
| 71 | 72 | import { createRepo, repositoryLimitMessage, resolveOwner } from "../actions/repos.ts"; |
| 72 | 73 | import { sessionOf } from "../core/sessions.ts"; |
| 73 | 74 | import { scopeAllows, verifyToken } from "../core/tokens.ts"; |
| @@ -2052,6 +2053,20 @@ async function handleHook(app: App, request: Request): Promise<Response> { | ||
| 2052 | 2053 | error: String(error), |
| 2053 | 2054 | }), |
| 2054 | 2055 | ); |
| 2056 | // A push that moves the default branch brings every kept-fork pull's | |
| 2057 | // `combined/N` up to date (`core/recombine.ts`). Off the hook's answer: | |
| 2058 | // the pusher never waits for it, and never hears of its errors. | |
| 2059 | const tip = `refs/heads/${repo.defaultBranch}`; | |
| 2060 | if (input.updates.some((u) => u.ref === tip && !ZERO_OID.test(u.newOid))) | |
| 2061 | void recombine( | |
| 2062 | { db: app.db, reposRoot: app.reposRoot, log: app.log }, | |
| 2063 | repo.id, | |
| 2064 | ).catch((error) => | |
| 2065 | app.log.warn("could not recombine after the push", { | |
| 2066 | repo: `${repo.ownerName}/${repo.name}`, | |
| 2067 | error: String(error), | |
| 2068 | }), | |
| 2069 | ); | |
| 2055 | 2070 | // Every push is scanned for secrets and advisories (`checks/scan.ts`, |
| 2056 | 2071 | // #9593). Off the hook's answer too: a finding is recorded, not refused. |
| 2057 | 2072 | void queueScanPush(app, repo, input.updates).catch((error) => |
src/web/pages/pull.ts +1 −1
| @@ -1054,7 +1054,7 @@ export async function pullFilesPage(ctx: Ctx): Promise<Response> { | ||
| 1054 | 1054 | ${holdCard(ctx, repo, pull, { act: false })} |
| 1055 | 1055 | ${teamsCard(teamVerdicts(ctx.app.db, { ...pull, headOid: status.headOid }))} |
| 1056 | 1056 | ${mergeBox(ctx, repo, pull, status, mayMerge, by, check, isApp, tier, stale)} |
| 1057 | ${slicesCard(base, thread, slices, status.ahead, ends, pull.state === "open" && pull.mergedAt === null, mayLand)} | |
| 1057 | ${slicesCard(base, thread, slices, ends.length > 0 ? ends.length : status.ahead, ends, pull.state === "open" && pull.mergedAt === null, mayLand)} | |
Unrelated to the issue's boxes: a count fix to the slices card, bundled into a wiring pull. It is a consequence of this change, so keep it, but say so in the description and make sure test/pull-slices.test.ts covers the combined-merge-commit case. |
||
| 1058 | 1058 | ${await combinedCard(dir, pull, status.headOid, status.baseOid)} |
| 1059 | 1059 | <div class="split"> |
| 1060 | 1060 | <div> |
test/combined-fork.test.ts +4 −1
| @@ -185,7 +185,10 @@ describe("a conflicting fork pull is combined on our side (#9012)", () => { | ||
| 185 | 185 | await Bun.write(join(a, "four.txt"), "four\n"); |
| 186 | 186 | await git(["add", "."], a); |
| 187 | 187 | await git(["commit", "-m", "alice's four"], a); |
| 188 | await git(["push", "origin", "main"], a); | |
| 188 | // Moved past the hook (a push to main would recombine at once, #10040), so | |
| 189 | // the preview stays built on the old main and the card must say so. | |
| 190 | await git(["push", "origin", "main:refs/heads/four"], a); | |
| 191 | await git(["update-ref", "refs/heads/main", await git(["rev-parse", "HEAD"], a)], bare); | |
| 189 | 192 | const stale = await (await call(`/alice/app/pulls/${number}`, alice)).text(); |
| 190 | 193 | expect(stale).toContain(`Stale: main has moved since ${aliceMain.slice(0, 10)}`); |
| 191 | 194 | expect(await screenText()).toContain("Combined with main · stale"); |
test/combined-recombine.test.ts +34 −4
| @@ -8,7 +8,7 @@ import { serve, type Servers } from "../src/http/server.ts"; | ||
| 8 | 8 | import { gitClientEnv } from "./helpers/git-client.ts"; |
| 9 | 9 | import { proveAll } from "./helpers/verified.ts"; |
| 10 | 10 | import { promote } from "./helpers/members.ts"; |
| 11 | import { recombine } from "../src/core/recombine.ts"; | |
| 11 | import { idle, recombine, runsStarted } from "../src/core/recombine.ts"; | |
| 12 | 12 | import { getRepo } from "../src/core/repos.ts"; |
| 13 | 13 | import { repoDir } from "../src/git/repos.ts"; |
| 14 | 14 | import { tokenFor } from "./helpers/tokens.ts"; |
| @@ -126,8 +126,14 @@ async function moveMain(file: string): Promise<string> { | ||
| 126 | 126 | return git(["rev-parse", "HEAD"], a); |
| 127 | 127 | } |
| 128 | 128 | |
| 129 | /** The pass the push itself starts (the hook began it before it answered), awaited. */ | |
| 130 | async function settled(main: string): Promise<string> { | |
| 131 | await idle(repoId); | |
| 132 | return main; | |
| 133 | } | |
| 134 | ||
| 129 | 135 | test("main moves, and every combined branch has the new main as a parent", async () => { |
| 130 | const main = await moveMain("m1.txt"); | |
| 136 | const main = await settled(await moveMain("m1.txt")); | |
| 131 | 137 | expect(await recombine(forge(), repoId)).toBe(1); |
| 132 | 138 | for (const n of [first, second]) expect((await parents(`refs/heads/combined/${n}`))[1]).toBe(main); |
| 133 | 139 | }); |
| @@ -135,7 +141,7 @@ test("main moves, and every combined branch has the new main as a parent", async | ||
| 135 | 141 | test("one pull that cannot combine is logged and skipped, the others still follow", async () => { |
| 136 | 142 | await git(["update-ref", "-d", `refs/pull/${first}/head`], bare); |
| 137 | 143 | const before = await git(["rev-parse", `refs/heads/combined/${first}`], bare); |
| 138 | const main = await moveMain("m2.txt"); | |
| 144 | const main = await settled(await moveMain("m2.txt")); | |
| 139 | 145 | warnings.length = 0; |
| 140 | 146 | await recombine(forge(), repoId); |
| 141 | 147 | expect(warnings.length).toBe(1); |
| @@ -144,8 +150,32 @@ test("one pull that cannot combine is logged and skipped, the others still follo | ||
| 144 | 150 | }); |
| 145 | 151 | |
| 146 | 152 | test("calls made during a pass share it: three at once run at most two passes", async () => { |
| 147 | await moveMain("m3.txt"); | |
| 153 | await settled(await moveMain("m3.txt")); | |
| 148 | 154 | const counts = await Promise.all([recombine(forge(), repoId), recombine(forge(), repoId), recombine(forge(), repoId)]); |
| 149 | 155 | expect(Math.max(...counts)).toBeLessThanOrEqual(2); |
| 150 | 156 | expect(Math.max(...counts)).toBe(2); |
| 151 | 157 | }); |
| 158 | ||
| 159 | test("a push to main alone recombines every combined branch, with no call made", async () => { | |
| 160 | const main = await moveMain("m4.txt"); | |
| 161 | await idle(repoId); | |
| 162 | expect((await parents(`refs/heads/combined/${second}`))[1]).toBe(main); | |
| 163 | }); | |
| 164 | ||
| 165 | test("a push to another branch, or a delete of the default branch, starts no pass", async () => { | |
outdated
Polling loop with Bun.sleep(50) up to 5s, plus settled() adding a fixed Bun.sleep(100). The extra 100ms guess at 'the pass has finished' is a race. Wait on a real signal (e.g. await the recombine promise, or poll until no pass is running) instead of a timed guess. |
||
| 166 | const runs = runsStarted(repoId); | |
| 167 | await git(["checkout", "-q", "-b", "side"], a); | |
| 168 | await git(["push", "origin", "side"], a); | |
| 169 | await git(["checkout", "-q", "main"], a); | |
| 170 | await idle(repoId); | |
| 171 | expect(runsStarted(repoId)).toBe(runs); | |
| 172 | // The forge lets a bare repository's checked-out branch be deleted only if git does. | |
outdated
A fixed Bun.sleep(300) then asserting nothing changed proves little: a slow pass would land after the check and the test would still pass. Use a deterministic check (e.g. count log lines or passes started, or push to main afterwards and wait for that pass, then assert the side push added none). |
||
| 173 | await git(["config", "receive.denyDeleteCurrent", "ignore"], bare); | |
| 174 | await git(["push", "origin", ":refs/heads/main"], a); | |
| 175 | await idle(repoId); | |
| 176 | expect(runsStarted(repoId)).toBe(runs); | |
| 177 | // The pass a push to main starts is counted: the check above can see one. | |
| 178 | await git(["push", "origin", "main"], a); | |
| 179 | await idle(repoId); | |
| 180 | expect(runsStarted(repoId)).toBe(runs + 1); | |
| 181 | }); |
No test covers a delete of the default branch starting no pass (box 2 names deletes). Add a case that deletes main and asserts no pass starts.