diff --git a/.github/PULL_REQUEST_TEMPLATE.md b/.github/PULL_REQUEST_TEMPLATE.md index 87b695bab..1db3bb824 100644 --- a/.github/PULL_REQUEST_TEMPLATE.md +++ b/.github/PULL_REQUEST_TEMPLATE.md @@ -40,7 +40,7 @@ https://open.inf.is/docs/handbook/style/commit-messages/ -- in short: a category emoji, then optionally an action, then `：` (U+FF1A), then what the change does, in 50 characters or fewer and with no `#NNNN` on the end. -_Copy and paste one of the following emoji into description_ -- copy +_Copy and paste one of the following emoji into the title_ -- copy rather than type, since some have a lookalike spelling that is not the one recognized here. diff --git a/build/shared/commit-message.mts b/build/shared/commit-message.mts index 1ee7b90e4..cdbe1726f 100644 --- a/build/shared/commit-message.mts +++ b/build/shared/commit-message.mts @@ -120,7 +120,7 @@ const TRAILER_LINE = /^(?[A-Za-z][\w-]*):[ \t]*(?.*)$/; const ASSISTED_BY_VALUE = /^[^\s:]+:\S+( \S+)*$/; /** git folds a trailer whose value runs onto an indented line beneath it. */ -const CONTINUATION_LINE = /^\s/; +export const CONTINUATION_LINE = /^\s/; /** * Each known token with the pattern that also matches it misspelt with a @@ -423,7 +423,19 @@ export function validateCommitMessage(message: string) { problems.push('the line after the subject has to be blank'); } - for (const line of rest) { + // Where the trailer block starts, or past the end when there is none. A + // trailer is one line by construction here: git would read a folded value, + // but `readTrailers` keeps only the token line, so wrapping a long + // `Signed-off-by:` to fit would put the author out of reach of the sign-off + // check. The block is exempt from the width limit rather than made to fit + // inside it, which is also what the limit is for -- prose that is read. + const paragraphs = paragraphsOf(rest); + const blockStart = + trailerBlockOf(paragraphs).length > 0 + ? rest.length - (paragraphs.at(-1)?.length ?? 0) + : rest.length; + + for (const [index, line] of rest.entries()) { // A line of dashes is why the trailers in this project have been going // unread: `---` is where git stops looking for them, and any longer run // splits the block in two so that only the half below it counts. @@ -439,7 +451,11 @@ export function validateCommitMessage(message: string) { // No grapheme is fewer than one code unit, so a line of BODY_MAX code // units or fewer is within the limit whatever it is made of, and the // segmenter need not see it. - if (line.length > BODY_MAX && /\s/.test(line.trim())) { + if ( + index < blockStart && + line.length > BODY_MAX && + /\s/.test(line.trim()) + ) { const width = countGraphemes(line); if (width > BODY_MAX) { diff --git a/build/shared/commit-message.test.mts b/build/shared/commit-message.test.mts index 67ec3ea17..44fd73dd6 100644 --- a/build/shared/commit-message.test.mts +++ b/build/shared/commit-message.test.mts @@ -11,6 +11,7 @@ import { readFile } from 'node:fs/promises'; import { describe, test } from 'node:test'; import { ACTIONS, + BODY_MAX, CATEGORIES, readTrailers, validateCommitMessage, @@ -366,6 +367,38 @@ describe('the vocabulary', () => { }); }); +describe('the width limit', () => { + test('leaves a trailer that cannot be wrapped alone', () => { + // Folding this to fit would put the author on a continuation line, where + // `readTrailers` does not look and so `checkSignOff` could not match it. + const long = + 'Signed-off-by: Christopher Alexander Montgomery ' + + ''; + ok(long.length > BODY_MAX); + deepStrictEqual( + validateCommitMessage(`🏗️🔧：fix it\n\nA body line.\n\n${long}`), + [] + ); + }); + + test('still holds prose to it, in the same message', () => { + const problems = validateCommitMessage( + `🏗️🔧：fix it\n\n${'word '.repeat(20)}end\n\n` + + 'Signed-off-by: Derek Lewis ' + ); + deepStrictEqual(problems.length, 1); + match(problems[0] ?? '', /the limit is 72/); + }); + + test('holds a long line in a paragraph that only looks like trailers', () => { + // Not a block, so git reads no trailers in it and it is prose after all. + const problems = validateCommitMessage( + `🏗️🔧：fix it\n\nSigned-off-by: Derek Lewis \n${'word '.repeat(20)}end` + ); + ok(problems.some((problem) => /the limit is 72/.test(problem))); + }); +}); + describe('a message written to be slow', () => { // A commit message comes from whoever opened the pull request, and the // commit queue reads it holding credentials that can write here. How long diff --git a/build/shared/landing.mts b/build/shared/landing.mts index 2017880fb..3fc414449 100644 --- a/build/shared/landing.mts +++ b/build/shared/landing.mts @@ -11,7 +11,11 @@ * the repositories in this organization do not agree on that yet. */ -import { linesOf, TRAILER_ORDER } from '@openinf/portal/build/commit-message'; +import { + CONTINUATION_LINE, + linesOf, + TRAILER_ORDER, +} from '@openinf/portal/build/commit-message'; /** One commit's message, split into the parts a landed message reuses. */ export type CommitParts = { @@ -34,9 +38,26 @@ export function partsOfMessage(message: string): CommitParts { const body: string[] = []; const trailers: string[] = []; + // A folded trailer travels with the line it belongs to. Read on its own an + // indented continuation carries no token, so it would be filed as body and + // stranded there -- and `Co-authored-by:` folded over two lines would land + // with the address left behind, which is attribution quietly lost. + let inTrailer = false; + for (const line of rest) { - if (TRAILER_ORDER.includes(tokenOf(line))) trailers.push(line); - else body.push(line); + if (TRAILER_ORDER.includes(tokenOf(line))) { + trailers.push(line); + inTrailer = true; + } else if ( + inTrailer && + CONTINUATION_LINE.test(line) && + line.trim() !== '' + ) { + trailers[trailers.length - 1] += `\n${line}`; + } else { + body.push(line); + inTrailer = false; + } } while (body.at(-1)?.trim() === '') body.pop(); diff --git a/build/shared/landing.test.mts b/build/shared/landing.test.mts index e1fb0d25e..d82b4b8af 100644 --- a/build/shared/landing.test.mts +++ b/build/shared/landing.test.mts @@ -344,3 +344,48 @@ describe('checksVerdict and its own run', () => { ); }); }); + +describe('a folded trailer', () => { + const folded = [ + '🏗️🔧：fix it', + '', + 'A body line.', + '', + 'Co-authored-by: Jane Doe', + ' ', + 'Signed-off-by: Derek Lewis ', + ].join('\n'); + + test('is one trailer, not a trailer and a stranded line', () => { + const parts = partsOfMessage(folded); + deepStrictEqual(parts.body, ['A body line.']); + deepStrictEqual(parts.trailers, [ + 'Co-authored-by: Jane Doe\n ', + 'Signed-off-by: Derek Lewis ', + ]); + }); + + test('lands with its value still attached to it', () => { + const message = composeLandingMessage( + [partsOfMessage(folded)], + 'https://example.com/pull/1' + ); + deepStrictEqual(message.split('\n'), [ + 'A body line.', + '', + 'Co-authored-by: Jane Doe', + ' ', + 'Signed-off-by: Derek Lewis ', + 'PR-URL: https://example.com/pull/1', + ]); + deepStrictEqual(validateCommitMessage(`🏗️🔧：fix it\n\n${message}`), []); + }); + + test('does not swallow an indented body line after prose', () => { + const parts = partsOfMessage( + '🏗️🔧：fix it\n\nA body line.\n an indented one\n\nPR-URL: x' + ); + deepStrictEqual(parts.body, ['A body line.', ' an indented one']); + deepStrictEqual(parts.trailers, ['PR-URL: x']); + }); +}); diff --git a/build/tasks/land-pull-request.mts b/build/tasks/land-pull-request.mts index b1d77a93a..9acf29211 100644 --- a/build/tasks/land-pull-request.mts +++ b/build/tasks/land-pull-request.mts @@ -232,15 +232,25 @@ try { // do with a stranger's commits is to not have them on disk at all. // GitHub returns them oldest first, which is the order to read them in, // and merges are dropped since their messages say nothing. - const messages: string[] = JSON.parse( - gh( - 'api', - '--paginate', - `repos/${repository()}/pulls/${number}/commits`, - '--jq', - '[.[] | select(.parents | length < 2) | .commit.message]' - ) - ); + // + // `--jq` beside `--paginate` filters each page and prints the results + // one after another, so a branch past the first page of commits would + // hand `JSON.parse` several arrays in a row and land nothing. `--slurp` + // gathers the pages themselves into one array, and the filtering that + // `--jq` was doing happens here instead. + const pages: { parents: unknown[]; commit: { message: string } }[][] = + JSON.parse( + gh( + 'api', + '--paginate', + '--slurp', + `repos/${repository()}/pulls/${number}/commits` + ) + ); + const messages = pages + .flat() + .filter((commit) => commit.parents.length < 2) + .map((commit) => commit.commit.message); const parts = messages.map((message) => partsOfMessage(message)); const message = composeLandingMessage( parts,