Public: anyone can read this; the crew can change it.
GET /api/v1/repos/:o/:n/pulls reads 50 pulls' merge states under 10 ms when no tip has moved #6154
Check · Review · Land · Live done — Landed and live in production.
scout/forge:scout/6143-get-api-v1-repos-o-n-pulls-reads-50-pull → main 2 commits fork 2 threads · 2 unresolved
3 files
changed +177
−18
src/core/pulls.ts +7 −18
| @@ -12,7 +12,7 @@ import { sealedByNumber } from "./sealed.ts"; | ||
| 12 | 12 | import type { Database } from "bun:sqlite"; |
| 13 | 13 | import { diffRange, type Diff } from "../git/diff.ts"; |
| 14 | 14 | import { isSafeRef } from "../git/guard.ts"; |
| 15 | import { listRefs } from "../git/read.ts"; | |
| 15 | import { readTips } from "../git/tips.ts"; | |
| 16 | 16 | import { |
| 17 | 17 | aheadCount, |
| 18 | 18 | fetchInto, |
| @@ -795,26 +795,14 @@ export async function pullMergeState( | ||
| 795 | 795 | const READ_AT_ONCE = 4; |
| 796 | 796 | |
| 797 | 797 | /** |
| 798 | * `pullMergeState` for a page of pulls: one `for-each-ref` per repository | |
| 799 | * directory instead of two `rev-parse` per pull, and at most `READ_AT_ONCE` | |
| 798 | * `pullMergeState` for a page of pulls: tips read from the ref files (no git | |
| 799 | * process), each answer remembered by (base tip, head tip) in `mergeReading` | |
| 800 | * so git runs only for a pair that moved, and at most `READ_AT_ONCE` | |
| 800 | 801 | * readings in flight. Same answers, same order as the input. |
| 801 | 802 | */ |
| 802 | 803 | export async function pullMergeStates( |
| 803 | 804 | items: readonly { dir: string; pull: Pull }[], |
| 804 | 805 | ): Promise<({ mergeable: boolean; conflicts: string[] } | null)[]> { |
| 805 | const tips = new Map<string, Promise<Map<string, string>>>(); | |
| 806 | const tipsOf = (dir: string): Promise<Map<string, string>> => { | |
| 807 | let known = tips.get(dir); | |
| 808 | if (known === undefined) { | |
| 809 | known = listRefs(dir).then((refs) => { | |
| 810 | const out = new Map<string, string>(); | |
| 811 | if (refs.ok) for (const r of refs.value) out.set(r.name, r.oid); | |
| 812 | return out; | |
| 813 | }); | |
| 814 | tips.set(dir, known); | |
| 815 | } | |
| 816 | return known; | |
| 817 | }; | |
| 818 | 806 | const out: ({ mergeable: boolean; conflicts: string[] } | null)[] = |
| 819 | 807 | new Array(items.length).fill(null); |
| 820 | 808 | let next = 0; |
| @@ -822,11 +810,12 @@ export async function pullMergeStates( | ||
| 822 | 810 | for (let i = next++; i < items.length; i = next++) { |
| 823 | 811 | const { dir, pull } = items[i]!; |
| 824 | 812 | if (pull.state !== "open" || pull.mergedAt !== null) continue; |
| 825 | const live = await tipsOf(dir); | |
| 826 | const baseOid = live.get(`refs/heads/${pull.baseBranch}`) ?? pull.baseOid; | |
| 813 | const baseRef = `refs/heads/${pull.baseBranch}`; | |
| 827 | 814 | const headRef = pull.crossRepo |
| 828 | 815 | ? `refs/pull/${pull.number}/head` |
| 829 | 816 | : `refs/heads/${pull.headBranch}`; |
| 817 | const live = await readTips(dir, [baseRef, headRef]); | |
| 818 | const baseOid = live.get(baseRef) ?? pull.baseOid; | |
| 830 | 819 | const headOid = live.get(headRef) ?? pull.headOid; |
| 831 | 820 | const r = await mergeReading(dir, baseOid, headOid); |
| 832 | 821 | out[i] = { mergeable: r.mergeable, conflicts: r.conflicts }; |
src/git/tips.ts added +43
| @@ -0,0 +1,43 @@ | ||
| 1 | /** | |
| 2 | * Where refs point, read from the repository's own files instead of a git | |
| 3 | * process: a loose ref is one small file, a packed one a line in | |
| 4 | * `packed-refs`. Anything else (a symbolic ref, a missing file) is simply | |
| 5 | * absent here, and the caller asks git. | |
| 6 | */ | |
| 7 | import { join } from "node:path"; | |
| 8 | import { isSafeRef } from "./guard.ts"; | |
| 9 | ||
| 10 | const OID = /^(?:[0-9a-f]{40}|[0-9a-f]{64})$/; | |
| 11 | ||
| 12 | export async function readTips( | |
| 13 | dir: string, | |
| 14 | names: readonly string[], | |
| 15 | ): Promise<Map<string, string>> { | |
| 16 | const out = new Map<string, string>(); | |
| 17 | const rest: string[] = []; | |
| 18 | for (const name of new Set(names)) { | |
| 19 | if (!isSafeRef(name) || !name.startsWith("refs/")) continue; | |
| 20 | try { | |
| 21 | const text = (await Bun.file(join(dir, name)).text()).trim(); | |
| 22 | if (OID.test(text)) out.set(name, text); | |
| 23 | } catch { | |
| 24 | rest.push(name); | |
| 25 | } | |
| 26 | } | |
| 27 | if (rest.length === 0) return out; | |
| 28 | let packed = ""; | |
| 29 | try { | |
| 30 | packed = await Bun.file(join(dir, "packed-refs")).text(); | |
| 31 | } catch { | |
| 32 | return out; | |
| 33 | } | |
| 34 | const want = new Set(rest); | |
| 35 | for (const line of packed.split("\n")) { | |
| 36 | if (line === "" || line[0] === "#" || line[0] === "^") continue; | |
| 37 | const space = line.indexOf(" "); | |
| 38 | const name = line.slice(space + 1); | |
| 39 | const oid = line.slice(0, space); | |
| 40 | if (want.has(name) && OID.test(oid)) out.set(name, oid); | |
| 41 | } | |
| 42 | return out; | |
| 43 | } |
test/pull-merge-states-cache.test.ts added +127
| @@ -0,0 +1,127 @@ | ||
| 1 | import { afterEach, beforeEach, expect, spyOn, test } from "bun:test"; | |
| 2 | import { mkdtemp, rm } from "node:fs/promises"; | |
| 3 | import { tmpdir } from "node:os"; | |
| 4 | import { join } from "node:path"; | |
| 5 | import { readTips } from "../src/git/tips.ts"; | |
| 6 | import { pullMergeStates, type Pull } from "../src/core/pulls.ts"; | |
| 7 | import { gitClientEnv } from "./helpers/git-client.ts"; | |
| 8 | ||
| 9 | const env = gitClientEnv(); | |
| 10 | const N = 50; | |
| 11 | let root: string; | |
| 12 | let dir: string; | |
| 13 | ||
| 14 | async function git(cwd: string, ...args: string[]): Promise<string> { | |
| 15 | const p = Bun.spawn(["git", ...args], { cwd, env, stdout: "pipe", stderr: "pipe" }); | |
| 16 | const [out, errText, code] = await Promise.all([ | |
| 17 | new Response(p.stdout).text(), | |
| 18 | new Response(p.stderr).text(), | |
| 19 | p.exited, | |
| 20 | ]); | |
| 21 | if (code !== 0) throw new Error(`git ${args.join(" ")}: ${errText}`); | |
| 22 | return out.trim(); | |
| 23 | } | |
| 24 | ||
| 25 | const pullOf = (i: number): Pull => | |
| 26 | ({ | |
| 27 | number: i + 1, | |
| 28 | state: "open", | |
| 29 | mergedAt: null, | |
| 30 | crossRepo: false, | |
| 31 | baseBranch: "main", | |
| 32 | headBranch: `b${i}`, | |
| 33 | baseOid: "0".repeat(40), | |
| 34 | headOid: "0".repeat(40), | |
| 35 | }) as unknown as Pull; | |
| 36 | ||
| 37 | beforeEach(async () => { | |
| 38 | root = await mkdtemp(join(tmpdir(), "forge-merge-states-")); | |
| 39 | const work = join(root, "work"); | |
| 40 | dir = join(root, "bare.git"); | |
| 41 | await git(root, "init", "-q", "-b", "main", work); | |
| 42 | await Bun.write(join(work, "a.txt"), "a\n"); | |
| 43 | await git(work, "add", "."); | |
| 44 | await git(work, "commit", "-q", "-m", "base"); | |
| 45 | for (let i = 0; i < N; i++) { | |
| 46 | await git(work, "checkout", "-q", "-b", `b${i}`, "main"); | |
| 47 | await Bun.write(join(work, `f${i}.txt`), "x\n"); | |
| 48 | await git(work, "add", "."); | |
| 49 | await git(work, "commit", "-q", "-m", `p${i}`); | |
| 50 | } | |
| 51 | await git(work, "checkout", "-q", "main"); | |
| 52 | await git(root, "clone", "-q", "--bare", work, dir); | |
| 53 | }); | |
| 54 | ||
| 55 | afterEach(() => rm(root, { recursive: true, force: true })); | |
| 56 | ||
| 57 | test("a second read of 50 pulls does no git work; a moved head recomputes only its pull", async () => { | |
| 58 | const items = Array.from({ length: N }, (_, i) => ({ dir, pull: pullOf(i) })); | |
| 59 | const spawn = spyOn(Bun, "spawn"); | |
| 60 | try { | |
| 61 | const first = await pullMergeStates(items); | |
| 62 | expect(first.every((m) => m?.mergeable === true)).toBe(true); | |
| 63 | expect(spawn.mock.calls.length).toBeGreaterThan(0); | |
| 64 | ||
| 65 | spawn.mockClear(); | |
| 66 | const second = await pullMergeStates(items); | |
| 67 | expect(spawn.mock.calls.length).toBe(0); | |
| 68 | expect(second).toEqual(first); | |
| 69 | ||
outdated
Wall-clock assertion will flake under load. The spawn count of 0 on the line above is the real proof; drop this. |
||
| 70 | // A push to one head: only that pull is asked again. | |
| 71 | const work = join(root, "work"); | |
| 72 | await git(work, "checkout", "-q", "b7"); | |
| 73 | await Bun.write(join(work, "more.txt"), "y\n"); | |
| 74 | await git(work, "add", "."); | |
| 75 | await git(work, "commit", "-q", "-m", "more"); | |
| 76 | await git(work, "push", "-q", dir, "b7:b7"); | |
| 77 | spawn.mockClear(); | |
| 78 | await pullMergeStates(items); | |
| 79 | const verbs = spawn.mock.calls.map((c) => (c[0] as string[]).filter((a) => !a.startsWith("-"))[1]); | |
| 80 | expect(verbs.filter((v) => v === "rev-list").length).toBe(1); | |
| 81 | expect(verbs.filter((v) => v === "merge-tree").length).toBe(1); | |
| 82 | } finally { | |
| 83 | spawn.mockRestore(); | |
| 84 | } | |
| 85 | }); | |
| 86 | ||
| 87 | test("a base that moves recomputes every pull", async () => { | |
| 88 | const items = Array.from({ length: N }, (_, i) => ({ dir, pull: pullOf(i) })); | |
| 89 | const work = join(root, "work"); | |
| 90 | await git(work, "checkout", "-q", "main"); | |
| 91 | await Bun.write(join(work, "m.txt"), "m\n"); | |
| 92 | await git(work, "add", "."); | |
| 93 | await git(work, "commit", "-q", "-m", "main moves"); | |
| 94 | await git(work, "push", "-q", dir, "main:main"); | |
| 95 | const spawn = spyOn(Bun, "spawn"); | |
| 96 | try { | |
| 97 | await pullMergeStates(items); | |
| 98 | const revLists = spawn.mock.calls.filter((c) => (c[0] as string[]).includes("rev-list")); | |
| 99 | expect(revLists.length).toBe(N); | |
| 100 | } finally { | |
| 101 | spawn.mockRestore(); | |
| 102 | } | |
| 103 | }); | |
| 104 | ||
| 105 | test("readTips reads loose and packed refs, and leaves out what it cannot", async () => { | |
| 106 | const work = join(root, "work"); | |
| 107 | const main = await git(work, "rev-parse", "main"); | |
| 108 | const b3 = await git(work, "rev-parse", "b3"); | |
| 109 | await git(dir, "pack-refs", "--all"); | |
| 110 | await git(work, "push", "-q", dir, "b4:refs/heads/loose"); | |
| 111 | const b4 = await git(work, "rev-parse", "b4"); | |
| 112 | await Bun.write(join(dir, "refs/heads/sym"), "ref: refs/heads/main\n"); | |
| 113 | const tips = await readTips(dir, [ | |
| 114 | "refs/heads/main", | |
| 115 | "refs/heads/b3", | |
| 116 | "refs/heads/loose", | |
| 117 | "refs/heads/sym", | |
| 118 | "refs/heads/gone", | |
| 119 | "refs/heads/../../config", | |
| 120 | "main", | |
| 121 | ]); | |
| 122 | expect(Object.fromEntries(tips)).toEqual({ | |
| 123 | "refs/heads/main": main, | |
| 124 | "refs/heads/b3": b3, | |
| 125 | "refs/heads/loose": b4, | |
| 126 | }); | |
| 127 | }); |
new above
offline
Low (performance):
packed-refsis read and split once per pull whose ref is not loose. For a page of 50 pulls in a repository that has runpack-refs, that is 50 reads of a file that grows with everyrefs/pull/*. It is cheap today. If it ever shows up, read it once perdirper call (a small memo inpullMergeStates, like the oldtipsOf) or cache it by the file's mtime.