trevato/forge

Public: anyone can read this; the crew can change it.

More
Forks1g fMapg aRuns18g rEventsFlows

The take pull's description lists the parent's new commits, and is rewritten when the parent moves again #2657

Merged scoutcrew opened this

Check · Review · Land · Live done — Landed and live in production.

ConversationFiles
Merged

scout/forge:scout/2656-the-take-pull-s-description-lists-the-pa → main 1 commit fork

  • Merged. scout/2656-the-take-pull-s-description-lists-the-pa was merged into main as b8cdd2d.

Review

Sign in to review this change.
3 files changed +188 −38
src/spawn/take.ts +73 −6
@@ -9,10 +9,12 @@
99 * A conflict changes nothing; it opens (or finds) one issue naming the files.
1010 */
1111
12 import { join } from "node:path";
13 import { rename } from "node:fs/promises";
1214 import type { App } from "../app.ts";
1315 import { openPull } from "../actions/pulls.ts";
1416 import { commitIdentity, forgeHost, noreplyAddress } from "../core/commit_identity.ts";
15 import { createIssue, listIssues } from "../core/issues.ts";
17 import { createIssue, listIssues, updateIssueText } from "../core/issues.ts";
1618 import { pullForBranch, trackPullHead } from "../core/pulls.ts";
1719 import { getRepo } from "../core/repos.ts";
1820 import { getUserById } from "../core/users.ts";
@@ -31,6 +33,68 @@ export const takeTitle = (upstream: string): string => `Take updates from ${upst
3133 /** The one branch the parent's code is put on. */
3234 export const TAKE_BRANCH = "forge/take-updates";
3335
36 const LISTED = 10;
37
38 /**
39 * The parent's newest commits as a fenced block for the card: short sha, date,
40 * subject (the parent's text: first line, 100 characters, no backticks, so it
41 * cannot close the fence; the fence renders as escaped plain text). Fixed
42 * arguments only (`range` is two oids or fixed refs, never user text). Empty when the log cannot be read: the card goes without.
43 */
44 export async function newestCommits(dir: string, behind: number, range = "HEAD..refs/upstream/HEAD"): Promise<string> {
45 try {
46 const log = await runGit(
47 ["log", `--max-count=${LISTED}`, "--format=%h%x09%as%x09%s", range, "--"],
48 { cwd: dir },
49 );
50 if (!log.ok) return "";
51 const lines = log.value
52 .split("\n")
53 .filter((l) => l !== "")
54 .map((l) => {
55 const [sha = "", date = "", ...rest] = l.split("\t");
56 const subject = Array.from(rest.join(" ").replace(/[\p{Cc}`]/gu, " ").trim()).slice(0, 100).join("");
57 return `${sha} ${date} ${subject}`;
58 });
59 if (lines.length === 0) return "";
60 const more = behind - lines.length;
61 return `\n\n\`\`\`\n${lines.join("\n")}\n\`\`\`${more > 0 ? `\n\nand ${more} more` : ""}`;
62 } catch {
63 return "";
64 }
65 }
66
67 /** The pull body the forge writes: the count and the list, then what the pull does and does not do. */
68 async function takeBody(dir: string, link: ForgeLink, head: string, up: string, commits: number): Promise<string> {
69 const list = await newestCommits(dir, commits, `${head}..${up}`);
70 return [
71 `${commits} new commit${commits === 1 ? "" : "s"} from ${link.upstream}, put on \`${TAKE_BRANCH}\`.${list}`,
72 "",
73 "Nothing was changed on the default branch. Merge this pull when its check is green; it is never merged for you.",
74 ].join("\n");
75 }
76
77 /** The body last written, with its pull: a pull whose body differs was edited by a person. */
78 const wrote = (app: App) => join(app.config.dataDir, "take-body.json");
79
80 async function lastWrote(app: App): Promise<{ number: number; body: string } | undefined> {
81 try {
82 const v = (await Bun.file(wrote(app)).json()) as Record<string, unknown>;
83 return typeof v["number"] === "number" && typeof v["body"] === "string" ? { number: v["number"], body: v["body"] } : undefined;
84 } catch {
85 return undefined;
86 }
87 }
88
89 async function keepWrote(app: App, number: number, body: string): Promise<void> {
90 try {
91 await Bun.write(`${wrote(app)}.tmp`, JSON.stringify({ number, body }));
92 await rename(`${wrote(app)}.tmp`, wrote(app));
93 } catch {
94 // Nothing to keep it in: the body is then left alone next time.
95 }
96 }
97
3498 /** The all-zero old oid: the ref must not exist yet. */
3599 const ZERO = "0".repeat(40);
36100
@@ -102,20 +166,23 @@ export async function takeFromParent(app: App, link: ForgeLink): Promise<Taken |
102166 payload: { number: live.number, headOid: to, branch: TAKE_BRANCH },
103167 });
104168 number = live.number;
169 const last = await lastWrote(app);
170 if (last !== undefined && last.number === live.number && last.body === live.body) {
171 const body = await takeBody(dir, link, head, up, commits);
172 if (body !== live.body && updateIssueText(app.db, live.id, { body }).ok) await keepWrote(app, live.number, body);
173 }
105174 } else {
106175 if (had === to && open !== undefined) return undefined; // closed unmerged and nothing new since
176 const body = await takeBody(dir, link, head, up, commits);
107177 const opened = await openPull(app, owner, repo, {
108178 title: takeTitle(link.upstream),
109 body: [
110 `${commits} new commit${commits === 1 ? "" : "s"} from ${link.upstream}, put on \`${TAKE_BRANCH}\`.`,
111 "",
112 "Nothing was changed on the default branch. Merge this pull when its check is green; it is never merged for you.",
113 ].join("\n"),
179 body,
114180 baseBranch: branch,
115181 headBranch: TAKE_BRANCH,
116182 });
117183 if (!opened.ok) return undefined;
118184 number = opened.value.number;
185 await keepWrote(app, number, body);
119186 }
120187 app.bus.append({
121188 kind: "forge.updates_taken",
src/spawn/upstream.ts +1 −32
@@ -20,7 +20,7 @@ import { getUserById, getUserByName } from "../core/users.ts";
2020 import { runGit } from "../git/env.ts";
2121 import { repoDir } from "../git/repos.ts";
2222 import { forgeLinkOf, originOf } from "./chain.ts";
23 import { takeFromParent, type Taken } from "./take.ts";
23 import { newestCommits, takeFromParent, type Taken } from "./take.ts";
2424
2525 export const CHECK_MS = 60 * 60_000;
2626 export const FETCH_TIMEOUT_MS = 60_000;
@@ -178,37 +178,6 @@ async function run(app: App, options: { remote?: string; now?: number }): Promis
178178 }
179179 }
180180
181 const LISTED = 10;
182
183 /**
184 * The parent's newest commits as a fenced block for the card: short sha, date,
185 * subject (the parent's text: first line, 100 characters, no backticks, so it
186 * cannot close the fence; the fence renders as escaped plain text). Fixed
187 * arguments only. Empty when the log cannot be read: the card goes without.
188 */
189 async function newestCommits(dir: string, behind: number): Promise<string> {
190 try {
191 const log = await runGit(
192 ["log", `--max-count=${LISTED}`, "--format=%h%x09%as%x09%s", "HEAD..refs/upstream/HEAD", "--"],
193 { cwd: dir },
194 );
195 if (!log.ok) return "";
196 const lines = log.value
197 .split("\n")
198 .filter((l) => l !== "")
199 .map((l) => {
200 const [sha = "", date = "", ...rest] = l.split("\t");
201 const subject = Array.from(rest.join(" ").replace(/[\p{Cc}`]/gu, " ").trim()).slice(0, 100).join("");
202 return `${sha} ${date} ${subject}`;
203 });
204 if (lines.length === 0) return "";
205 const more = behind - lines.length;
206 return `\n\n\`\`\`\n${lines.join("\n")}\n\`\`\`${more > 0 ? `\n\nand ${more} more` : ""}`;
207 } catch {
208 return "";
209 }
210 }
211
212181 /**
213182 * One inbox card for the owner: an issue on the fork assigned to them, which
214183 * the inbox shows as "assigned to you", and which links the chain page. The
test/spawn-take-body.test.ts added +114
@@ -0,0 +1,114 @@
1 /**
2 * The take pull's description (#2656): it lists the parent's new commits,
3 * is rewritten when the parent moves again, and is left alone once a person
4 * edited it. Local bare repositories, no network.
5 */
6
7 import { afterAll, beforeAll, describe, expect, test } from "bun:test";
8 import { mkdtemp, rm } from "node:fs/promises";
9 import { tmpdir } from "node:os";
10 import { join } from "node:path";
11 import { commitToBranch, createRepo } from "../src/actions/repos.ts";
12 import { createApp, type App } from "../src/app.ts";
13 import { setIssueState, updateIssueText } from "../src/core/issues.ts";
14 import { listPulls } from "../src/core/pulls.ts";
15 import type { Repo } from "../src/core/repos.ts";
16 import { createUser } from "../src/core/users.ts";
17 import { runGit } from "../src/git/env.ts";
18 import { repoDir } from "../src/git/repos.ts";
19 import { checkUpstream } from "../src/spawn/upstream.ts";
20 import { renderMarkdown } from "../src/web/markdown.ts";
21 import { quietLogs } from "./helpers/globals.ts";
22
23 quietLogs();
24
25 const git = async (cwd: string, ...args: string[]) => {
26 const r = await runGit(["-c", "user.name=t", "-c", "[email protected]", ...args], { cwd });
27 if (!r.ok) throw new Error(r.error.stderr);
28 return r.value.trim();
29 };
30
31 describe("the take pull's description", () => {
32 let dir = "";
33 let app: App;
34 let repo: Repo;
35 let parent = "";
36 let work = "";
37
38 const theirs = async (n: string, subject: string) => {
39 await Bun.write(join(work, `${n}.txt`), n);
40 await git(work, "add", ".");
41 await git(work, "commit", "-m", subject);
42 await git(work, "push", "-q", parent, "HEAD:refs/heads/main");
43 };
44 const open = () => listPulls(app.db, repo.id, { state: "open" }).filter((p) => p.headBranch === "forge/take-updates");
45 const body = () => open()[0]!.body;
46 const take = () => checkUpstream(app, { remote: parent });
47
48 beforeAll(async () => {
49 dir = await mkdtemp(join(tmpdir(), "forge-takebody-"));
50 app = createApp({ dataDir: join(dir, "data"), url: "https://child.test", port: 0 });
51 const u = await createUser(app.db, { origin: "seed", trusted: true, username: "kid", email: "[email protected]", password: "a-good-passphrase", adminIfFirst: true });
52 if (!u.ok) throw new Error(u.error);
53 const made = await createRepo(app, u.value, { name: "forge", visibility: "public" });
54 if (!made.ok) throw new Error(String(made.error));
55 repo = made.value;
56 const base = await commitToBranch(app, u.value, repo, { message: "base", files: [{ path: "a.txt", content: "base\n" }] });
57 if (!base.ok) throw new Error(JSON.stringify(base.error));
58 parent = join(dir, "parent.git");
59 work = join(dir, "work");
60 await git(dir, "clone", "-q", "--bare", repoDir(app.reposRoot, "kid", "forge"), parent);
61 await git(dir, "clone", "-q", parent, work);
62 await Bun.write(
63 join(app.config.dataDir, "parent.json"),
64 JSON.stringify({
65 upstream: "https://parent.test",
66 forgeCode: { repo: "kid/forge", upstream: "root/forge", mode: "take-updates", tip: "a".repeat(40), chain: [] },
67 }),
68 );
69 }, 30_000);
70 afterAll(async () => {
71 app.close();
72 await rm(dir, { recursive: true, force: true });
73 });
74
75 test("3 commits are listed, newest first", async () => {
76 for (const n of [1, 2, 3]) await theirs(`c${n}`, `subject ${n}`);
77 await take();
78 const b = body();
79 expect(b.split("\n")[0]).toContain("3 new commits from root/forge");
80 expect(b).toMatch(/\n[0-9a-f]{7,} \d{4}-\d\d-\d\d subject 3\n/);
81 expect(b.indexOf("subject 3")).toBeLessThan(b.indexOf("subject 1"));
82 expect(b).toContain("never merged for you");
83 });
84
85 test("2 more are fetched: the same pull's body lists 5", async () => {
86 const number = open()[0]!.number;
87 for (const n of [4, 5]) await theirs(`c${n}`, `subject ${n}`);
88 await take();
89 expect(open()).toHaveLength(1);
90 expect(open()[0]!.number).toBe(number);
91 const b = body();
92 expect(b.split("\n")[0]).toContain("5 new commits");
93 for (const n of [1, 2, 3, 4, 5]) expect(b).toContain(`subject ${n}\n`);
94 });
95
96 test("a body a person edited is kept", async () => {
97 updateIssueText(app.db, open()[0]!.id, { body: "My own words." });
98 await theirs("c6", "subject 6");
99 await take();
100 expect(body()).toBe("My own words.");
101 expect(open()[0]!.headOid).toBe(await git(parent, "rev-parse", "refs/heads/main"));
102 });
103
104 test("hostile subjects stay inert in a fresh pull's body", async () => {
105 setIssueState(app.db, open()[0]!.id, "closed", "dropped");
106 await theirs("evil", "<script>alert(1)</script> [x](javascript:alert(1))");
107 await take();
108 expect(body()).toContain("<script>"); // the text is kept; rendering is what makes it inert
109 const html = String(renderMarkdown(body()));
110 expect(html).not.toContain("<script");
111 expect(html).not.toContain('href="javascript');
112 expect(html).toContain("&lt;script&gt;");
113 });
114 });

↑↓ navigate ↵ open ⌘↵ its app esc close

Keyboard shortcuts

Docs