Public: anyone can read this; the crew can change it.
Files page: checks phase under 3 ms and no git spawns for branch tips on a hit #6169
Check · Review · Land · Live done — Landed and live in production.
scout/forge:scout/6153-files-page-checks-phase-under-3-ms-and-n → main 4 commits fork 5 threads · 5 unresolved
5 files
changed +279
−16
src/core/pulls.ts +11 −3
| @@ -12,6 +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 { refTip } from "../git/read.ts"; | |
| 15 | 16 | import { readTips } from "../git/tips.ts"; |
| 16 | 17 | import { |
| 17 | 18 | aheadCount, |
| @@ -761,12 +762,19 @@ export async function pullStatus( | ||
| 761 | 762 | pull: Pull, |
| 762 | 763 | gate?: ReviewGate, |
| 763 | 764 | ): Promise<PullStatus> { |
| 764 | const baseOid = | |
| 765 | (await resolveOid(dir, `refs/heads/${pull.baseBranch}`)) ?? pull.baseOid; | |
| 766 | 765 | const headRef = pull.crossRepo |
| 767 | 766 | ? `refs/pull/${pull.number}/head` |
| 768 | 767 | : `refs/heads/${pull.headBranch}`; |
| 769 | const headOid = (await resolveOid(dir, headRef)) ?? pull.headOid; | |
| 768 | // The tips come from the ref files (no process); git is asked only when | |
| 769 | // they cannot be read that way. | |
| 770 | const tip = async (ref: string): Promise<string | undefined> => | |
| 771 | (await refTip(dir, ref)) ?? (await resolveOid(dir, ref)); | |
| 772 | const [liveBase, liveHead] = await Promise.all([ | |
| 773 | tip(`refs/heads/${pull.baseBranch}`), | |
| 774 | tip(headRef), | |
| 775 | ]); | |
| 776 | const baseOid = liveBase ?? pull.baseOid; | |
| 777 | const headOid = liveHead ?? pull.headOid; | |
| 770 | 778 | const reviews = gate === undefined ? null : reviewGate(gate, pull, headOid); |
| 771 | 779 | const { ahead, mergeable: clean, conflicts } = await mergeReading( |
| 772 | 780 | dir, |
src/git/read.ts +28
| @@ -8,6 +8,7 @@ | ||
| 8 | 8 | import { defect } from "../kernel/error.ts"; |
| 9 | 9 | import { err, ok, type Result } from "../kernel/result.ts"; |
| 10 | 10 | import { gitEnv, runGit, runGitRaw, type GitFailure } from "./env.ts"; |
| 11 | import { isSafeRef } from "./guard.ts"; | |
| 11 | 12 | |
| 12 | 13 | export type ObjectType = "blob" | "tree" | "commit" | "tag"; |
| 13 | 14 | |
| @@ -556,3 +557,30 @@ export async function listPaths( | ||
| 556 | 557 | if (all.at(-1) === "") all.pop(); |
| 557 | 558 | return ok({ paths: all.slice(0, cap), truncated: all.length > cap }); |
| 558 | 559 | } |
| 560 | ||
| 561 | /** | |
| 562 | * The commit a ref names, read from the ref files themselves: no process. | |
| 563 | * A loose ref wins over `packed-refs`, as in git. Anything this does not | |
| 564 | * fully understand (a symbolic ref, a missing file, an odd line) answers | |
| 565 | * undefined and the caller asks git. | |
| 566 | */ | |
| 567 | export async function refTip(dir: string, ref: string): Promise<string | undefined> { | |
Good guard order (safe ref, refs/ prefix, no ..). Add test cases for a symbolic ref and a garbled loose file so the fallback to git is proven. |
||
| 568 | if (!isSafeRef(ref) || !ref.startsWith("refs/") || ref.includes("..")) return undefined; | |
| 569 | const oid = /^[0-9a-f]{40}([0-9a-f]{24})?$/; | |
| 570 | try { | |
| 571 | const loose = (await Bun.file(`${dir}/${ref}`).text()).trim(); | |
| 572 | return oid.test(loose) ? loose : undefined; | |
| 573 | } catch { | |
| 574 | // not loose: it may be packed | |
| 575 | } | |
| 576 | try { | |
| 577 | const packed = await Bun.file(`${dir}/packed-refs`).text(); | |
| 578 | for (const line of packed.split("\n")) { | |
| 579 | if (line.endsWith(` ${ref}`) && oid.test(line.slice(0, line.length - ref.length - 1))) | |
| 580 | return line.slice(0, line.length - ref.length - 1); | |
| 581 | } | |
| 582 | } catch { | |
| 583 | // no packed-refs | |
| 584 | } | |
| 585 | return undefined; | |
| 586 | } |
src/web/pages/pull.ts +29 −13
| @@ -54,8 +54,7 @@ import { | ||
| 54 | 54 | } from "../../core/reviews.ts"; |
| 55 | 55 | import { getUsersByIds } from "../../core/users.ts"; |
| 56 | 56 | import { canWrite, getRepoById, type Repo } from "../../core/repos.ts"; |
| 57 | import { resolveOid } from "../../git/merge.ts"; | |
| 58 | import { commitLog } from "../../git/read.ts"; | |
| 57 | import { commitLog, readerFor } from "../../git/read.ts"; | |
| 59 | 58 | import { wallOf } from "../../contract/policy.ts"; |
| 60 | 59 | import type { Ctx } from "../../http/context.ts"; |
| 61 | 60 | import { |
| @@ -303,6 +302,28 @@ export function checksLine( | ||
| 303 | 302 | </p>`; |
| 304 | 303 | } |
| 305 | 304 | |
| 305 | /** Whether a head declares a fast tier: a commit's files never change. */ | |
| 306 | const fastByHead = new Map<string, boolean>(); | |
| 307 | export async function declaresFast(dir: string, head: string): Promise<boolean> { | |
| 308 | const known = fastByHead.get(head); | |
| 309 | if (known !== undefined) return known; | |
| 310 | const [forgeJson, packageJson] = await Promise.all([ | |
| 311 | readFileAt(dir, head, "forge.json"), | |
| 312 | readFileAt(dir, head, "package.json"), | |
| 313 | ]); | |
| 314 | const declared = parseCheck(forgeJson, packageJson); | |
outdated
A head with neither file is never cached, so every view of such a pull re-reads from git, which is the cost this pull is removing. Cache the negative once the commit is known to exist, for example by checking the tree is readable, rather than by whether a file came back. |
||
| 315 | const fast = declared.ok && declared.value?.fast != null; | |
| 316 | // Nothing read: remember that only for a commit known to exist (its tree | |
outdated
A |
||
| 317 | // reads); an unreadable one may simply not have landed yet. | |
| 318 | if (forgeJson === undefined && packageJson === undefined) { | |
| 319 | const tree = await readerFor(dir).object(`${head}^{tree}`); | |
| 320 | if (!tree.ok) return fast; | |
| 321 | } | |
| 322 | if (fastByHead.size >= 500) fastByHead.delete(fastByHead.keys().next().value!); | |
| 323 | fastByHead.set(head, fast); | |
| 324 | return fast; | |
| 325 | } | |
| 326 | ||
| 306 | 327 | /** |
| 307 | 328 | * The tiers' words for a pull whose check declares a fast tier (#1198); |
| 308 | 329 | * nothing when it does not, or when no check has run on this head. |
| @@ -314,12 +335,7 @@ async function tierOf( | ||
| 314 | 335 | ): Promise<string> { |
| 315 | 336 | const run = check.run; |
| 316 | 337 | if (run === null || run.kind !== "check") return ""; |
| 317 | const [forgeJson, packageJson] = await Promise.all([ | |
| 318 | readFileAt(dir, head, "forge.json"), | |
| 319 | readFileAt(dir, head, "package.json"), | |
| 320 | ]); | |
| 321 | const declared = parseCheck(forgeJson, packageJson); | |
| 322 | if (!declared.ok || declared.value?.fast == null) return ""; | |
| 338 | if (!(await declaresFast(dir, head))) return ""; | |
| 323 | 339 | const state: TierState | undefined = |
| 324 | 340 | check.state === "ok" |
| 325 | 341 | ? "green" |
| @@ -843,11 +859,11 @@ export async function pullFilesPage(ctx: Ctx): Promise<Response> { | ||
| 843 | 859 | const check = tick("checks", () => checkSummary(ctx.app.db, pull.id, status.headOid)); |
| 844 | 860 | const isApp = getAppByRepo(ctx.app.db, repo.id) !== undefined; |
| 845 | 861 | const tier = await lap("checks", () => tierOf(dir, status.headOid, check)); |
| 846 | const stale = await lap("checks", async () => | |
| 862 | // `status.baseOid` is the live base tip already: no second reading of it. | |
| 863 | const stale = | |
| 847 | 864 | pull.state === "open" && pull.mergedAt === null |
| 848 | ? checkStale(check.run, await resolveOid(dir, `refs/heads/${pull.baseBranch}`)) | |
| 849 | : false, | |
| 850 | ); | |
| 865 | ? checkStale(check.run, status.baseOid) | |
| 866 | : false; | |
| 851 | 867 | const threads = await lap("reviews", () => pullThreads(ctx.app, pull)); |
| 852 | 868 | const threadViews = threadStateViews(ctx, repo, threads); |
| 853 | 869 | const unresolved = threads.filter((t) => t.resolvedAt === null).length; |
| @@ -1395,7 +1411,7 @@ export async function pullChecksLineFor( | ||
| 1395 | 1411 | const tier = await tierOf(dir, status.headOid, check); |
| 1396 | 1412 | const stale = |
| 1397 | 1413 | pull.state === "open" && pull.mergedAt === null |
| 1398 | ? checkStale(check.run, await resolveOid(dir, `refs/heads/${pull.baseBranch}`)) | |
| 1414 | ? checkStale(check.run, status.baseOid) | |
| 1399 | 1415 | : false; |
| 1400 | 1416 | return pullChecksLine(ctx, repo, pull, status, check, isApp, tier, stale); |
| 1401 | 1417 | } |
test/pull-tips.test.ts added +153
| @@ -0,0 +1,153 @@ | ||
| 1 | import { expect, 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 { pullStatus, type Pull } from "../src/core/pulls.ts"; | |
| 6 | import { declaresFast } from "../src/web/pages/pull.ts"; | |
| 7 | import { runGit } from "../src/git/env.ts"; | |
| 8 | import { gitClientEnv } from "./helpers/git-client.ts"; | |
| 9 | ||
| 10 | const env = gitClientEnv({ | |
| 11 | GIT_AUTHOR_NAME: "X", | |
| 12 | GIT_AUTHOR_EMAIL: "[email protected]", | |
| 13 | GIT_COMMITTER_NAME: "X", | |
| 14 | GIT_COMMITTER_EMAIL: "[email protected]", | |
| 15 | }); | |
| 16 | ||
| 17 | async function git(cwd: string, ...args: string[]): Promise<string> { | |
| 18 | const p = Bun.spawn(["git", ...args], { cwd, env, stdout: "pipe", stderr: "pipe" }); | |
| 19 | const out = await new Response(p.stdout).text(); | |
| 20 | expect(await p.exited).toBe(0); | |
| 21 | return out.trim(); | |
| 22 | } | |
| 23 | ||
| 24 | const pullAt = (base: string, head: string, o: Partial<Pull> = {}): Pull => | |
| 25 | ({ | |
| 26 | number: 1, | |
| 27 | baseBranch: "main", | |
| 28 | headBranch: "feature", | |
| 29 | baseOid: base, | |
| 30 | headOid: head, | |
| 31 | crossRepo: false, | |
| 32 | ...o, | |
| 33 | }) as Pull; | |
| 34 | ||
| 35 | async function repo(): Promise<{ root: string; gd: string; one: string; two: string }> { | |
| 36 | const root = await mkdtemp(join(tmpdir(), "pulltips-")); | |
| 37 | await git(root, "init", "-q", "-b", "main"); | |
| 38 | await git(root, "commit", "-q", "--allow-empty", "-m", "one"); | |
| 39 | const one = await git(root, "rev-parse", "HEAD"); | |
| 40 | await git(root, "checkout", "-q", "-b", "feature"); | |
| 41 | await Bun.write(join(root, "f.txt"), "x\n"); | |
| 42 | await git(root, "add", "f.txt"); | |
| 43 | await git(root, "commit", "-q", "-m", "two"); | |
| 44 | const two = await git(root, "rev-parse", "HEAD"); | |
| 45 | return { root, gd: join(root, ".git"), one, two }; | |
| 46 | } | |
| 47 | ||
| 48 | test("pullStatus answers the live tips: loose, packed, and through git for a symbolic or garbled ref", async () => { | |
| 49 | const { root, gd, one, two } = await repo(); | |
| 50 | try { | |
| 51 | // The snapshot on the pull is stale on purpose: the answer must be live. | |
| 52 | const stale = pullAt("0".repeat(40), "1".repeat(40)); | |
| 53 | let s = await pullStatus(gd, stale); | |
| 54 | expect([s.baseOid, s.headOid, s.ahead, s.mergeable]).toEqual([one, two, 1, true]); | |
| 55 | ||
| 56 | await git(root, "pack-refs", "--all"); | |
| 57 | s = await pullStatus(gd, stale); | |
| 58 | expect([s.baseOid, s.headOid]).toEqual([one, two]); | |
| 59 | ||
| 60 | // A symbolic loose ref is not for us to read: git resolves it. | |
| 61 | await Bun.write(join(gd, "refs/heads/feature"), "ref: refs/heads/main\n"); | |
| 62 | s = await pullStatus(gd, stale); | |
| 63 | expect(s.headOid).toBe(one); | |
| 64 | ||
| 65 | // A garbled one is not an oid: git refuses it, the pull's snapshot stands. | |
| 66 | await Bun.write(join(gd, "refs/heads/feature"), "garbage\n"); | |
| 67 | s = await pullStatus(gd, pullAt(one, two)); | |
| 68 | expect(s.headOid).toBe(two); | |
| 69 | } finally { | |
| 70 | await rm(root, { recursive: true, force: true }); | |
| 71 | } | |
| 72 | }); | |
| 73 | ||
| 74 | test("pullStatus spawns nothing on a hit: live tips with no git on the PATH", async () => { | |
| 75 | const { root, gd, one, two } = await repo(); | |
| 76 | const nogit = await mkdtemp(join(tmpdir(), "pulltips-nogit-")); | |
| 77 | const path = process.env.PATH; | |
| 78 | const without = async (work: () => Promise<void>): Promise<void> => { | |
| 79 | process.env.PATH = nogit; | |
| 80 | try { | |
| 81 | await work(); | |
| 82 | } finally { | |
| 83 | process.env.PATH = path; | |
| 84 | } | |
| 85 | }; | |
| 86 | try { | |
| 87 | const stale = pullAt("0".repeat(40), "1".repeat(40)); | |
| 88 | await pullStatus(gd, stale); // the merge reading is remembered by oid | |
| 89 | // Control: with no git, a spawn fails, so a spawn below would be seen. | |
| 90 | await without(async () => { | |
| 91 | const probe = await runGit(["rev-parse", "HEAD"], { cwd: root }).then( | |
| 92 | (r) => r.ok, | |
| 93 | () => false, | |
| 94 | ); | |
| 95 | expect(probe).toBe(false); | |
| 96 | }); | |
| 97 | await without(async () => { | |
| 98 | const s = await pullStatus(gd, stale); | |
| 99 | expect([s.baseOid, s.headOid]).toEqual([one, two]); | |
| 100 | }); | |
| 101 | await git(root, "pack-refs", "--all"); | |
| 102 | await pullStatus(gd, stale); | |
| 103 | await without(async () => { | |
| 104 | const s = await pullStatus(gd, stale); | |
| 105 | expect([s.baseOid, s.headOid]).toEqual([one, two]); | |
| 106 | }); | |
| 107 | } finally { | |
| 108 | process.env.PATH = path; | |
| 109 | await rm(root, { recursive: true, force: true }); | |
| 110 | await rm(nogit, { recursive: true, force: true }); | |
| 111 | } | |
| 112 | }); | |
| 113 | ||
| 114 | test("declaresFast remembers a commit with neither file, once its tree reads", async () => { | |
| 115 | const { root, gd, two } = await repo(); | |
| 116 | const elsewhere = await mkdtemp(join(tmpdir(), "pulltips-empty-")); | |
| 117 | try { | |
| 118 | await git(elsewhere, "init", "-q", "--bare"); | |
| 119 | expect(await declaresFast(gd, two)).toBe(false); | |
| 120 | // Remembered: a repository that cannot supply the commit is not asked. | |
| 121 | expect(await declaresFast(elsewhere, two)).toBe(false); | |
| 122 | // An unreadable commit is not remembered as false. | |
| 123 | const ghost = "a".repeat(40); | |
| 124 | expect(await declaresFast(elsewhere, ghost)).toBe(false); | |
| 125 | expect(await declaresFast(gd, ghost)).toBe(false); | |
| 126 | } finally { | |
| 127 | await rm(root, { recursive: true, force: true }); | |
| 128 | await rm(elsewhere, { recursive: true, force: true }); | |
| 129 | } | |
| 130 | }); | |
| 131 | ||
| 132 | test("declaresFast is stable across calls and not poisoned by a failed first read", async () => { | |
| 133 | const { root, gd } = await repo(); | |
| 134 | try { | |
| 135 | await Bun.write( | |
| 136 | join(root, "forge.json"), | |
| 137 | JSON.stringify({ check: { run: "a", fast: "b" } }), | |
| 138 | ); | |
| 139 | await git(root, "add", "forge.json"); | |
| 140 | await git(root, "commit", "-q", "-m", "check"); | |
| 141 | const head = await git(root, "rev-parse", "HEAD"); | |
| 142 | const empty = await mkdtemp(join(tmpdir(), "pulltips-empty-")); | |
| 143 | await git(empty, "init", "-q", "--bare"); | |
| 144 | // The commit cannot be read yet (a repository without it): false, and not remembered. | |
| 145 | expect(await declaresFast(empty, head)).toBe(false); | |
| 146 | expect(await declaresFast(gd, head)).toBe(true); | |
| 147 | // Remembered once read: the same answer with nowhere to read it from. | |
| 148 | expect(await declaresFast(empty, head)).toBe(true); | |
| 149 | expect(await declaresFast(gd, head)).toBe(true); | |
| 150 | } finally { | |
| 151 | await rm(root, { recursive: true, force: true }); | |
| 152 | } | |
| 153 | }); |
test/ref-tip.test.ts added +58
| @@ -0,0 +1,58 @@ | ||
| 1 | import { expect, 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 { refTip } from "../src/git/read.ts"; | |
| 6 | import { gitClientEnv } from "./helpers/git-client.ts"; | |
| 7 | ||
| 8 | const env = gitClientEnv({ | |
| 9 | GIT_AUTHOR_NAME: "X", | |
| 10 | GIT_AUTHOR_EMAIL: "[email protected]", | |
| 11 | GIT_COMMITTER_NAME: "X", | |
| 12 | GIT_COMMITTER_EMAIL: "[email protected]", | |
| 13 | }); | |
| 14 | ||
| 15 | async function git(cwd: string, ...args: string[]): Promise<string> { | |
| 16 | const p = Bun.spawn(["git", ...args], { cwd, env, stdout: "pipe", stderr: "pipe" }); | |
| 17 | const out = await new Response(p.stdout).text(); | |
| 18 | expect(await p.exited).toBe(0); | |
| 19 | return out.trim(); | |
| 20 | } | |
| 21 | ||
Only refTip is covered. Add coverage that fails without the pullStatus tip reading and without the declaresFast cache. |
||
| 22 | test("refTip reads loose and packed refs without git, and refuses what it cannot read", async () => { | |
| 23 | const dir = await mkdtemp(join(tmpdir(), "reftip-")); | |
| 24 | try { | |
| 25 | await git(dir, "init", "-q", "-b", "main"); | |
| 26 | const gd = join(dir, ".git"); | |
| 27 | await git(dir, "commit", "-q", "--allow-empty", "-m", "one"); | |
| 28 | const one = await git(dir, "rev-parse", "HEAD"); | |
| 29 | await git(dir, "branch", "feature"); | |
| 30 | expect(await refTip(gd, "refs/heads/main")).toBe(one); | |
| 31 | await git(dir, "pack-refs", "--all"); | |
| 32 | expect(await refTip(gd, "refs/heads/feature")).toBe(one); | |
| 33 | await git(dir, "commit", "-q", "--allow-empty", "-m", "two"); | |
| 34 | const two = await git(dir, "rev-parse", "HEAD"); | |
| 35 | // a loose ref shadows the packed one | |
| 36 | expect(await refTip(gd, "refs/heads/main")).toBe(two); | |
| 37 | expect(await refTip(gd, "refs/heads/nope")).toBeUndefined(); | |
| 38 | expect(await refTip(gd, "refs/heads/../../x")).toBeUndefined(); | |
| 39 | expect(await refTip(gd, "HEAD")).toBeUndefined(); | |
| 40 | } finally { | |
| 41 | await rm(dir, { recursive: true, force: true }); | |
| 42 | } | |
| 43 | }); | |
| 44 | ||
| 45 | test("refTip answers undefined for a symbolic or garbled loose ref, so the caller asks git", async () => { | |
| 46 | const dir = await mkdtemp(join(tmpdir(), "reftip-")); | |
| 47 | try { | |
| 48 | await git(dir, "init", "-q", "-b", "main"); | |
| 49 | await git(dir, "commit", "-q", "--allow-empty", "-m", "one"); | |
| 50 | const gd = join(dir, ".git"); | |
| 51 | await Bun.write(join(gd, "refs/heads/sym"), "ref: refs/heads/main\n"); | |
| 52 | await Bun.write(join(gd, "refs/heads/junk"), "not an oid\n"); | |
| 53 | expect(await refTip(gd, "refs/heads/sym")).toBeUndefined(); | |
| 54 | expect(await refTip(gd, "refs/heads/junk")).toBeUndefined(); | |
| 55 | } finally { | |
| 56 | await rm(dir, { recursive: true, force: true }); | |
| 57 | } | |
| 58 | }); |
new above
offline
This is the behaviour the issue is about. Add a test that runs pullStatus with git unavailable (empty PATH / failing git) and still gets the live tips; today the suite passes with the old rev-parse path restored.