From ce3b2e0e947e8bb62b7ef164cd030c1adbf60709 Mon Sep 17 00:00:00 2001 From: Derek Lewis Date: Sat, 15 Aug 2026 01:54:01 +0000 Subject: [PATCH] =?UTF-8?q?=F0=9F=8F=97=EF=B8=8F=E2=9C=A8=EF=BC=9Aland=20a?= =?UTF-8?q?=20pull=20request=20as=20one=20good=20commit?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The half-step before a commit queue: the message building, under test and in version control, still driven by hand. `nps land ` squashes a pull request into one commit whose message is composed from the commits it contains -- every message kept whole, the trailers gathered into the paragraph git reads, and `PR-URL` added -- then holds that message to the same rules a commit answers to before merging. It earned its keep already: it refused twice while landing the last nine pull requests, once for an over-wide line and once for a `Fixes:` left stranded where git would not have read it. The composing lives apart from the rules on purpose. Keeping every commit's words and gathering its trailers is the same job whatever house style a repository writes subjects in, and the repositories in this organization do not agree on that yet. Signed-off-by: Derek Lewis Assisted-by: Claude-Code:claude-opus-5 --- build/shared/landing.mts | 94 ++++++++++++++++ build/shared/landing.test.mts | 177 ++++++++++++++++++++++++++++++ build/tasks/land-pull-request.mts | 177 ++++++++++++++++++++++++++++++ package-scripts.yml | 3 + package.json | 1 + 5 files changed, 452 insertions(+) create mode 100644 build/shared/landing.mts create mode 100644 build/shared/landing.test.mts create mode 100644 build/tasks/land-pull-request.mts diff --git a/build/shared/landing.mts b/build/shared/landing.mts new file mode 100644 index 000000000..1b3d9ede5 --- /dev/null +++ b/build/shared/landing.mts @@ -0,0 +1,94 @@ +/** + * @file Building the commit message a pull request lands as. + * @author The OpenINF Authors & Friends + * @license MIT OR Apache-2.0 OR BlueOak-1.0.0 + * @module {type ES6Module} build/shared/landing + * + * Nothing here knows what a subject is supposed to look like. Composing a + * landed message -- keeping every commit's words, gathering the trailers into + * the one paragraph git reads, adding the pull request it came through -- is + * the same job whatever house style a repository writes its subjects in, and + * the repositories in this organization do not agree on that yet. + */ + +import { TRAILER_ORDER } from '@openinf/portal/build/commit-message'; + +/** One commit's message, split into the parts a landed message reuses. */ +export type CommitParts = { + subject: string; + body: string[]; + trailers: string[]; +}; + +const tokenOf = (line: string) => line.match(/^([A-Za-z][\w-]*):/)?.[1] ?? ''; + +/** + * Splits a commit message into the parts a landed message reuses. Trailers + * are lifted out wherever they were written, because a squashed message can + * only have one trailer block and it has to be at the end. + * @param {string} message One commit's whole message. + * @returns {CommitParts} Its subject, its body, and the trailers it carried. + */ +export function partsOfMessage(message: string): CommitParts { + const [subject = '', ...rest] = message + .replace(/[\r\n]+$/, '') + .split(/\r?\n/); + const body: string[] = []; + const trailers: string[] = []; + + for (const line of rest) { + if (TRAILER_ORDER.includes(tokenOf(line))) trailers.push(line); + else body.push(line); + } + + while (body.at(-1)?.trim() === '') body.pop(); + while (body.at(0)?.trim() === '') body.shift(); + + return { subject, body, trailers }; +} + +/** + * Builds the message a pull request should land as. One commit lands as + * itself. Several land as one, every message kept whole so that nothing + * written down is lost -- each subject becomes a heading in the body, the + * first included, since a pull request title describes the whole and need not + * be any single commit's subject. + * @param {CommitParts[]} parts Each commit on the branch, oldest first. + * @param {string} prUrl The pull request the commits are landing through. + * @returns {string} Everything below the subject line. + */ +export function composeLandingMessage(parts: CommitParts[], prUrl: string) { + const paragraphs = + parts.length === 1 + ? (parts[0]?.body ?? []) + : parts.flatMap((part, index) => [ + ...(index === 0 ? [] : ['']), + part.subject, + '', + ...part.body, + ]); + + const rank = (line: string) => TRAILER_ORDER.indexOf(tokenOf(line)); + const gathered = [ + ...new Set([...parts.flatMap((part) => part.trailers), `PR-URL: ${prUrl}`]), + ].sort((one, other) => rank(one) - rank(other)); + + // `Fixes:` says everything `Refs:` would about the same issue. + const fixed = new Set( + gathered + .filter((line) => line.startsWith('Fixes:')) + .map((line) => line.slice('Fixes:'.length).trim()) + ); + + return [ + ...paragraphs, + '', + ...gathered.filter( + (line) => + !( + line.startsWith('Refs:') && + fixed.has(line.slice('Refs:'.length).trim()) + ) + ), + ].join('\n'); +} diff --git a/build/shared/landing.test.mts b/build/shared/landing.test.mts new file mode 100644 index 000000000..0da47c21e --- /dev/null +++ b/build/shared/landing.test.mts @@ -0,0 +1,177 @@ +/** + * @file Tests for building the message a pull request lands as. + * @author The OpenINF Authors & Friends + * @license MIT OR Apache-2.0 OR BlueOak-1.0.0 + * @module {type ES6Module} build/shared/landing.test + */ + +import { deepStrictEqual, match, ok } from 'node:assert/strict'; +import { execFileSync } from 'node:child_process'; +import { describe, test } from 'node:test'; +import { validateCommitMessage } from '@openinf/portal/build/commit-message'; +import { + composeLandingMessage, + partsOfMessage, +} from '@openinf/portal/build/landing'; + +const URL_ = 'https://github.com/OpenINF/openinf.github.io/pull/1234'; + +describe('partsOfMessage', () => { + test('separates subject, body and trailers', () => { + deepStrictEqual( + partsOfMessage( + '🏗️🔧：fix it\n\nWhy it needed fixing.\n\nSigned-off-by: A \n' + ), + { + subject: '🏗️🔧：fix it', + body: ['Why it needed fixing.'], + trailers: ['Signed-off-by: A '], + } + ); + }); + + test('lifts a trailer out of the middle of a body', () => { + // A squashed message can only have one trailer block, at the end, so one + // written half way up has to be found wherever it is. + const { body, trailers } = partsOfMessage( + '🏗️🔧：fix it\n\nRefs: https://x/1\n\nMore explanation.' + ); + + deepStrictEqual(trailers, ['Refs: https://x/1']); + deepStrictEqual(body, ['More explanation.']); + }); + + test('reads a message written with carriage returns', () => { + deepStrictEqual( + partsOfMessage('🏗️🔧：fix it\r\n\r\nBody.\r\n\r\nFixes: https://x/1\r\n') + .trailers, + ['Fixes: https://x/1'] + ); + }); +}); + +describe('composeLandingMessage', () => { + test('a single commit lands as itself, plus where it came from', () => { + const parts = [partsOfMessage('🏗️🔧：fix it\n\nWhy.\n\nRefs: https://x/1')]; + + // `PR-URL` comes before `Refs` in the documented order, so the trailer + // the commit carried moves below the one the landing adds. + deepStrictEqual( + composeLandingMessage(parts, URL_), + `Why.\n\nPR-URL: ${URL_}\nRefs: https://x/1` + ); + }); + + test('several commits keep every word, subjects as headings', () => { + const parts = [ + partsOfMessage('🏗️✨：the first thing\n\nWhy the first.'), + partsOfMessage('🏗️🔧：the second thing\n\nWhy the second.'), + ]; + + deepStrictEqual( + composeLandingMessage(parts, URL_), + [ + '🏗️✨：the first thing', + '', + 'Why the first.', + '', + '🏗️🔧：the second thing', + '', + 'Why the second.', + '', + `PR-URL: ${URL_}`, + ].join('\n') + ); + }); + + test('gathers scattered trailers into one block, in order', () => { + const parts = [ + partsOfMessage('🏗️✨：one\n\nA.\n\nFixes: https://x/9'), + partsOfMessage( + '🏗️🔧：two\n\nB.\n\nAssisted-by: Claude-Code:claude-opus-5\nSigned-off-by: D ' + ), + ]; + const message = composeLandingMessage(parts, URL_); + const block = message.slice(message.lastIndexOf('\n\n') + 2).split('\n'); + + deepStrictEqual(block, [ + 'Signed-off-by: D ', + 'Assisted-by: Claude-Code:claude-opus-5', + `PR-URL: ${URL_}`, + 'Fixes: https://x/9', + ]); + }); + + test('keeps one copy of a trailer both commits carried', () => { + const signed = 'Signed-off-by: D '; + const parts = [ + partsOfMessage(`🏗️✨：one\n\nA.\n\n${signed}`), + partsOfMessage(`🏗️🔧：two\n\nB.\n\n${signed}`), + ]; + + deepStrictEqual( + composeLandingMessage(parts, URL_) + .split('\n') + .filter((l) => l === signed).length, + 1 + ); + }); + + test('drops a Refs that duplicates a Fixes', () => { + const parts = [ + partsOfMessage('🏗️✨：one\n\nA.\n\nRefs: https://x/9'), + partsOfMessage('🏗️🔧：two\n\nB.\n\nFixes: https://x/9'), + ]; + const message = composeLandingMessage(parts, URL_); + + ok(message.includes('Fixes: https://x/9')); + ok(!message.includes('Refs: https://x/9')); + }); +}); + +describe('the message that comes out', () => { + test('passes the rules a commit answers to', () => { + const parts = [ + partsOfMessage( + '🏗️✨：one\n\nA reason.\n\nSigned-off-by: D \nAssisted-by: Claude-Code:claude-opus-5' + ), + partsOfMessage('🏗️🔧：two\n\nAnother reason.'), + ]; + const subject = '🏗️✨：land two things at once'; + + deepStrictEqual( + validateCommitMessage( + `${subject}\n\n${composeLandingMessage(parts, URL_)}` + ), + [] + ); + }); + + test('ends in a block git reads as trailers', () => { + const parts = [ + partsOfMessage('🏗️✨：one\n\nA reason.\n\nSigned-off-by: D '), + ]; + const message = `🏗️✨：one\n\n${composeLandingMessage(parts, URL_)}`; + const parsed = execFileSync('git', ['interpret-trailers', '--parse'], { + encoding: 'utf8', + input: message, + }) + .split('\n') + .filter(Boolean); + + deepStrictEqual(parsed, ['Signed-off-by: D ', `PR-URL: ${URL_}`]); + }); + + test('a body that would be too wide is still reported', () => { + // The rules are applied to the composed message, not to the commits it + // came from, so an over-wide line cannot slip through the join. + const parts = [partsOfMessage(`🏗️✨：one\n\n${'word '.repeat(20)}`)]; + + match( + validateCommitMessage( + `🏗️✨：one\n\n${composeLandingMessage(parts, URL_)}` + ).join(), + /the limit is 72/ + ); + }); +}); diff --git a/build/tasks/land-pull-request.mts b/build/tasks/land-pull-request.mts new file mode 100644 index 000000000..8941b58ed --- /dev/null +++ b/build/tasks/land-pull-request.mts @@ -0,0 +1,177 @@ +/** + * @file Land a pull request as one commit, with a message worth keeping. + * @author The OpenINF Authors & Friends + * @license MIT OR Apache-2.0 OR BlueOak-1.0.0 + * @module {type ES6Module} build/tasks/land-pull-request + * + * Run as `nps land `, or `nps "land --dry-run"` to see the + * message without landing anything. Squash merging through the GitHub user + * interface would take the pull request body, or nothing, and leave the + * trailers where git cannot read them; this builds the message instead, holds + * it to the same rules a commit answers to, and only then merges. + */ + +import { execFileSync } from 'node:child_process'; +import { validateCommitMessage } from '@openinf/portal/build/commit-message'; +import { + composeLandingMessage, + partsOfMessage, +} from '@openinf/portal/build/landing'; + +/** What this task needs to know about a pull request. */ +type PullRequest = { + title: string; + base: string; + head: string; + state: string; + draft: boolean; + mergeableState: string; +}; + +const REPOSITORY = 'OpenINF/openinf.github.io'; +const DEFAULT_BRANCH = 'live'; + +const [number, ...flags] = process.argv.slice(2); +const dryRun = flags.includes('--dry-run'); + +const gh = (...args: string[]) => + execFileSync('gh', args, { encoding: 'utf8', maxBuffer: 1 << 24 }).trim(); +const git = (...args: string[]) => + execFileSync('git', args, { encoding: 'utf8', maxBuffer: 1 << 24 }).trim(); + +/** + * Reads the pull request, waiting for GitHub to work out whether it merges. + * The answer is `unknown` for a moment after anything lands on the base, and + * acting on that would be acting on a stale one. + * @param {string} pull The pull request number. + * @returns {Promise} The fields this task needs. + */ +const readPull = async (pull: string): Promise => { + const fetchPull = () => + JSON.parse( + gh( + 'api', + `repos/${REPOSITORY}/pulls/${pull}`, + '--jq', + '{title, base: .base.ref, head: .head.sha, state, draft, mergeableState: .mergeable_state}' + ) + ); + let found = fetchPull(); + + // Only an open pull request is worth waiting on. A closed one reports + // `unknown` for ever, and waiting twenty seconds to say so helps nobody. + for ( + let attempt = 0; + attempt < 10 && + found.state === 'open' && + !found.draft && + found.mergeableState === 'unknown'; + attempt += 1 + ) { + await new Promise((resolve) => setTimeout(resolve, 2000)); + found = fetchPull(); + } + + return found; +}; + +/** + * Says why a pull request cannot be landed as it stands. + * @param {PullRequest} pull What GitHub reports about it. + * @returns {string} The reason, or an empty string if there is none. + */ +const refuse = (pull: PullRequest) => { + if (pull.draft || pull.state !== 'open') { + return `it is ${pull.draft ? 'a draft' : pull.state}`; + } + + if (pull.mergeableState === 'dirty') { + return `it conflicts with ${DEFAULT_BRANCH}; rebase the branch onto it and push first`; + } + + if (pull.base !== DEFAULT_BRANCH) { + return `it targets ${pull.base}; retarget it, or land its base first`; + } + + if (pull.mergeableState === 'unknown') { + return 'GitHub has not worked out whether it merges yet; try again shortly'; + } + + return ''; +}; + +if (number === undefined || !/^\d+$/.test(number)) { + console.error('Usage: nps "land [--dry-run]"'); + process.exitCode = 1; +} else { + const pull = await readPull(number); + const reason = refuse(pull); + + if (reason !== '') { + console.error(`#${number} cannot be landed: ${reason}.`); + process.exitCode = 1; + } else { + // Oldest first, so the landed message reads in the order the work was + // done rather than the order git lists it. + const shas = git( + 'rev-list', + '--reverse', + '--no-merges', + `origin/${DEFAULT_BRANCH}..${pull.head}` + ) + .split('\n') + .filter(Boolean); + const parts = shas.map((sha) => + partsOfMessage(git('log', '-1', '--format=%B', sha)) + ); + const message = composeLandingMessage( + parts, + `https://github.com/${REPOSITORY}/pull/${number}` + ); + const problems = validateCommitMessage(`${pull.title}\n\n${message}`); + const rule = '='.repeat(72); + + console.log(`${rule}\n${pull.title}\n\n${message}\n${rule}`); + console.log( + `#${number}: ${shas.length} commit${shas.length === 1 ? '' : 's'}, ${pull.mergeableState}` + ); + + if (shas.length === 0) { + console.error(`\n#${number} has no commits over ${DEFAULT_BRANCH}.`); + process.exitCode = 1; + } else if (problems.length > 0) { + console.error( + '\nThe message this would land does not pass its own rules:' + ); + for (const problem of problems) console.error(` ${problem}`); + process.exitCode = 1; + } else if (dryRun) { + console.log('\nDry run; nothing landed.'); + } else { + const merged = JSON.parse( + gh( + 'api', + '-X', + 'PUT', + `repos/${REPOSITORY}/pulls/${number}/merge`, + '-f', + 'merge_method=squash', + '-f', + `commit_title=${pull.title}`, + '-f', + `commit_message=${message}`, + '-f', + `sha=${pull.head}`, + '--jq', + '{merged, sha}' + ) + ); + + if (merged.merged === true) console.log(`\nLanded as ${merged.sha}`); + else { + console.error(`\n#${number} did not merge.`); + process.exitCode = 1; + } + } + } +} diff --git a/package-scripts.yml b/package-scripts.yml index c58790260..84ea5962c 100644 --- a/package-scripts.yml +++ b/package-scripts.yml @@ -39,6 +39,9 @@ scripts: toml: node build/tasks/format/format-toml.mts ts: node build/tasks/format/format-ts.mts yaml: node build/tasks/format/format-yaml.mts + # Squash a pull request into one commit with a message built from the + # commits it contains. Takes a number, and --dry-run to see it first. + land: node build/tasks/land-pull-request.mts build: nps compile.buildPortal test: nps verify.all start: eleventy --serve diff --git a/package.json b/package.json index affd224a6..0227bb74a 100644 --- a/package.json +++ b/package.json @@ -11,6 +11,7 @@ "exports": { "./build/commit-message": "./build/shared/commit-message.mts", "./build/constants": "./build/shared/constants.mts", + "./build/landing": "./build/shared/landing.mts", "./build/utils": "./build/utils.mts" }, "homepage": "open.inf.is",