From 743ab29528fdc05c963f5d56e79effa49f95c2ab Mon Sep 17 00:00:00 2001 From: Michael Daniels Date: Sun, 1 Mar 2026 10:13:18 -0500 Subject: [PATCH 1/3] ci/github-script: split getting commit details for PR into its own file --- .github/workflows/test.yml | 1 + ci/github-script/get-pr-commit-details.js | 93 +++++++++++++++++++++++ ci/github-script/lint-commits.js | 89 ++-------------------- 3 files changed, 102 insertions(+), 81 deletions(-) create mode 100644 ci/github-script/get-pr-commit-details.js diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index fef00b09a902..8c4c7e7c1921 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -77,6 +77,7 @@ jobs: 'ci/github-script/bot.js', 'ci/github-script/check-target-branch.js', 'ci/github-script/commits.js', + 'ci/github-script/get-pr-commit-details.js', 'ci/github-script/lint-commits.js', 'ci/github-script/merge.js', 'ci/github-script/prepare.js', diff --git a/ci/github-script/get-pr-commit-details.js b/ci/github-script/get-pr-commit-details.js new file mode 100644 index 000000000000..07ac10faa44f --- /dev/null +++ b/ci/github-script/get-pr-commit-details.js @@ -0,0 +1,93 @@ +// @ts-check +const { promisify } = require('node:util') +const execFile = promisify(require('node:child_process').execFile) + +/** + * @param {{ + * args: string[] + * core: import('@actions/core'), + * quiet?: boolean, + * repoPath?: string, + * }} RunGitProps + */ +async function runGit({ args, repoPath, core, quiet }) { + if (repoPath) { + args = ['-C', repoPath, ...args] + } + + if (!quiet) { + core.info(`About to run \`git ${args.map((s) => `'${s}'`).join(' ')}\``) + } + + return await execFile('git', args) +} + +/** + * GitHub's API will return a maximum of 250 commits. + * We will use it if we can, but fall back to using git locally. + * + * @param {{ + * context: import('@actions/github/lib/context').Context, + * core: import('@actions/core'), + * github: InstanceType, + * pr: Awaited["rest"]["pulls"]["get"]>>["data"] + * repoPath?: string, + * }} GetCommitMessagesForPRProps + * + * @returns {Promise<{ + * message: string, + * sha: string, + * }[]>} + */ +async function getCommitDetailsForPR({ context, core, github, pr, repoPath }) { + if (pr.commits < 250) { + return ( + await github.paginate(github.rest.pulls.listCommits, { + ...context.repo, + pull_number: pr.number, + }) + ).map((commit) => ({ message: commit.commit.message, sha: commit.sha })) + } else { + await runGit({ + args: ['fetch', `--depth=1`, 'origin', pr.base.sha], + repoPath, + core, + }) + await runGit({ + args: ['fetch', `--depth=${pr.commits + 1}`, 'origin', pr.head.sha], + repoPath, + core, + }) + + const shas = ( + await runGit({ + args: [ + 'rev-list', + `--max-count=${pr.commits}`, + `${pr.base.sha}..${pr.head.sha}`, + ], + repoPath, + core, + }) + ).stdout + .split('\n') + .map((s) => s.trim()) + .filter(Boolean) + + return Promise.all( + shas.map(async (sha) => ({ + sha, + message: ( + await runGit({ + args: ['log', '--format=%s', '-1', sha], + repoPath, + core, + quiet: true, + }) + ).stdout, + })), + ) + } +} + +module.exports = { getCommitDetailsForPR } diff --git a/ci/github-script/lint-commits.js b/ci/github-script/lint-commits.js index 21d709cadcbe..ddc422f9114c 100644 --- a/ci/github-script/lint-commits.js +++ b/ci/github-script/lint-commits.js @@ -1,27 +1,6 @@ // @ts-check const { classify } = require('../supportedBranches.js') -const { promisify } = require('node:util') -const execFile = promisify(require('node:child_process').execFile) - -/** - * @param {{ - * args: string[] - * core: import('@actions/core'), - * quiet?: boolean, - * repoPath?: string, - * }} RunGitProps - */ -async function runGit({ args, repoPath, core, quiet }) { - if (repoPath) { - args = ['-C', repoPath, ...args] - } - - if (!quiet) { - core.info(`About to run \`git ${args.map((s) => `'${s}'`).join(' ')}\``) - } - - return await execFile('git', args) -} +const { getCommitDetailsForPR } = require('./get-pr-commit-details.js') /** * @param {{ @@ -67,65 +46,13 @@ async function checkCommitMessages({ github, context, core, repoPath }) { return } - /** - * GitHub's API will return a maximum of 250 commits. - * We will use it if we can, but fall back to using git locally. - * This type is used to abstract over the differences between the two. - * @type {{ - * message: string, - * sha: string, - * }[]} - */ - let commits - - if (pr.commits < 250) { - commits = ( - await github.paginate(github.rest.pulls.listCommits, { - ...context.repo, - pull_number, - }) - ).map((commit) => ({ message: commit.commit.message, sha: commit.sha })) - } else { - await runGit({ - args: ['fetch', `--depth=1`, 'origin', pr.base.sha], - repoPath, - core, - }) - await runGit({ - args: ['fetch', `--depth=${pr.commits + 1}`, 'origin', pr.head.sha], - repoPath, - core, - }) - - const shas = ( - await runGit({ - args: [ - 'rev-list', - `--max-count=${pr.commits}`, - `${pr.base.sha}..${pr.head.sha}`, - ], - repoPath, - core, - }) - ).stdout - .split('\n') - .map((s) => s.trim()) - .filter(Boolean) - - commits = await Promise.all( - shas.map(async (sha) => ({ - sha, - message: ( - await runGit({ - args: ['log', '--format=%s', '-1', sha], - repoPath, - core, - quiet: true, - }) - ).stdout, - })), - ) - } + const commits = await getCommitDetailsForPR({ + context, + core, + github, + pr, + repoPath, + }) const failures = new Set() From d03b81d689080c079f1de188b8da1d60fcc70963 Mon Sep 17 00:00:00 2001 From: Michael Daniels Date: Fri, 6 Mar 2026 08:53:12 -0500 Subject: [PATCH 2/3] ci/github-script: don't use GH API when getting commit info, return only subject --- ci/github-script/get-pr-commit-details.js | 88 +++++++++++------------ ci/github-script/lint-commits.js | 17 ++--- 2 files changed, 46 insertions(+), 59 deletions(-) diff --git a/ci/github-script/get-pr-commit-details.js b/ci/github-script/get-pr-commit-details.js index 07ac10faa44f..c9349fa7e055 100644 --- a/ci/github-script/get-pr-commit-details.js +++ b/ci/github-script/get-pr-commit-details.js @@ -23,71 +23,63 @@ async function runGit({ args, repoPath, core, quiet }) { } /** - * GitHub's API will return a maximum of 250 commits. - * We will use it if we can, but fall back to using git locally. - * * @param {{ - * context: import('@actions/github/lib/context').Context, * core: import('@actions/core'), - * github: InstanceType, * pr: Awaited["rest"]["pulls"]["get"]>>["data"] * repoPath?: string, * }} GetCommitMessagesForPRProps * * @returns {Promise<{ - * message: string, + * subject: string, * sha: string, * }[]>} */ -async function getCommitDetailsForPR({ context, core, github, pr, repoPath }) { - if (pr.commits < 250) { - return ( - await github.paginate(github.rest.pulls.listCommits, { - ...context.repo, - pull_number: pr.number, - }) - ).map((commit) => ({ message: commit.commit.message, sha: commit.sha })) - } else { +async function getCommitDetailsForPR({ core, pr, repoPath }) { + await runGit({ + args: ['fetch', `--depth=1`, 'origin', pr.base.sha], + repoPath, + core, + }) + await runGit({ + args: ['fetch', `--depth=${pr.commits + 1}`, 'origin', pr.head.sha], + repoPath, + core, + }) + + const shas = ( await runGit({ - args: ['fetch', `--depth=1`, 'origin', pr.base.sha], - repoPath, - core, - }) - await runGit({ - args: ['fetch', `--depth=${pr.commits + 1}`, 'origin', pr.head.sha], + args: [ + 'rev-list', + `--max-count=${pr.commits}`, + `${pr.base.sha}..${pr.head.sha}`, + ], repoPath, core, }) + ).stdout + .split('\n') + .map((s) => s.trim()) + .filter(Boolean) - const shas = ( - await runGit({ - args: [ - 'rev-list', - `--max-count=${pr.commits}`, - `${pr.base.sha}..${pr.head.sha}`, - ], - repoPath, - core, - }) - ).stdout - .split('\n') - .map((s) => s.trim()) - .filter(Boolean) + return Promise.all( + shas.map(async (sha) => { + const result = ( + await runGit({ + args: ['log', '--format=%s', '--numstat', '-1', sha], + repoPath, + core, + quiet: true, + }) + ).stdout.split('\n') - return Promise.all( - shas.map(async (sha) => ({ + const subject = result[0] + + return { sha, - message: ( - await runGit({ - args: ['log', '--format=%s', '-1', sha], - repoPath, - core, - quiet: true, - }) - ).stdout, - })), - ) - } + subject, + } + }), + ) } module.exports = { getCommitDetailsForPR } diff --git a/ci/github-script/lint-commits.js b/ci/github-script/lint-commits.js index ddc422f9114c..bba4cb9ec99a 100644 --- a/ci/github-script/lint-commits.js +++ b/ci/github-script/lint-commits.js @@ -47,9 +47,7 @@ async function checkCommitMessages({ github, context, core, repoPath }) { } const commits = await getCommitDetailsForPR({ - context, core, - github, pr, repoPath, }) @@ -57,21 +55,18 @@ async function checkCommitMessages({ github, context, core, repoPath }) { const failures = new Set() for (const commit of commits) { - const message = commit.message - const firstLine = message.split('\n')[0] + const logMsgStart = `Commit ${commit.sha}'s message's subject ("${commit.subject}")` - const logMsgStart = `Commit ${commit.sha}'s message's subject ("${firstLine}")` - - if (!firstLine.includes(': ')) { + if (!commit.subject.includes(': ')) { core.error( `${logMsgStart} was detected as not meeting our guidelines because ` + - 'it does not contain a colon followed by a whitespace.' + + 'it does not contain a colon followed by a whitespace. ' + 'There are likely other issues as well.', ) failures.add(commit.sha) } - if (firstLine.endsWith('.')) { + if (commit.subject.endsWith('.')) { core.error( `${logMsgStart} was detected as not meeting our guidelines because ` + 'it ends in a period. There may be other issues as well.', @@ -80,10 +75,10 @@ async function checkCommitMessages({ github, context, core, repoPath }) { } const fixups = ['amend!', 'fixup!', 'squash!'] - if (fixups.some((s) => firstLine.startsWith(s))) { + if (fixups.some((s) => commit.subject.startsWith(s))) { core.error( `${logMsgStart} was detected as not meeting our guidelines because ` + - `it begins with "${fixups.find((s) => firstLine.startsWith(s))}". ` + + `it begins with "${fixups.find((s) => commit.subject.startsWith(s))}". ` + 'Did you forget to run `git rebase -i --autosquash`?', ) failures.add(commit.sha) From 58f002f950109ef1ffe11f4d18039a930f176ab4 Mon Sep 17 00:00:00 2001 From: Michael Daniels Date: Sun, 1 Mar 2026 10:14:27 -0500 Subject: [PATCH 3/3] ci/github-script/lint-commits: error when conventional commit format is used E.g. https://redirect.github.com/NixOS/nixpkgs/pull/495442 --- ci/github-script/get-pr-commit-details.js | 18 ++++++- ci/github-script/lint-commits.js | 63 +++++++++++++++++++++-- 2 files changed, 75 insertions(+), 6 deletions(-) diff --git a/ci/github-script/get-pr-commit-details.js b/ci/github-script/get-pr-commit-details.js index c9349fa7e055..fcccfeacd75e 100644 --- a/ci/github-script/get-pr-commit-details.js +++ b/ci/github-script/get-pr-commit-details.js @@ -23,6 +23,11 @@ async function runGit({ args, repoPath, core, quiet }) { } /** + * Gets the SHA, subject and changed files for each commit in the given PR. + * + * Don't use GitHub API at all: the "list commits on PR" endpoint has a limit + * of 250 commits and doesn't return the changed files. + * * @param {{ * core: import('@actions/core'), * pr: Awaited["rest"]["pulls"]["get"]>>["data"] @@ -32,6 +37,8 @@ async function runGit({ args, repoPath, core, quiet }) { * @returns {Promise<{ * subject: string, * sha: string, + * changedPaths: string[], + * changedPathSegments: Set, * }[]>} */ async function getCommitDetailsForPR({ core, pr, repoPath }) { @@ -63,9 +70,10 @@ async function getCommitDetailsForPR({ core, pr, repoPath }) { return Promise.all( shas.map(async (sha) => { + // Subject first, then a blank line, then filenames. const result = ( await runGit({ - args: ['log', '--format=%s', '--numstat', '-1', sha], + args: ['log', '--format=%s', '--name-only', '-1', sha], repoPath, core, quiet: true, @@ -74,9 +82,17 @@ async function getCommitDetailsForPR({ core, pr, repoPath }) { const subject = result[0] + const changedPaths = result.slice(2, -1) + + const changedPathSegments = new Set( + changedPaths.flatMap((path) => path.split('/')), + ) + return { sha, subject, + changedPaths, + changedPathSegments, } }), ) diff --git a/ci/github-script/lint-commits.js b/ci/github-script/lint-commits.js index bba4cb9ec99a..4ef8b6f6ab04 100644 --- a/ci/github-script/lint-commits.js +++ b/ci/github-script/lint-commits.js @@ -46,17 +46,60 @@ async function checkCommitMessages({ github, context, core, repoPath }) { return } - const commits = await getCommitDetailsForPR({ - core, - pr, - repoPath, - }) + const commits = await getCommitDetailsForPR({ core, pr, repoPath }) const failures = new Set() + const conventionalCommitTypes = [ + 'build', + 'chore', + 'ci', + 'doc', + 'docs', + 'feat', + 'feature', + 'fix', + 'perf', + 'refactor', + 'style', + 'test', + ] + + /** + * @param {string[]} types e.g. ["fix", "feat"] + * @param {string?} sha commit hash + */ + function makeConventionalCommitRegex(types, sha = null) { + core.info( + `${ + sha + ? `Conventional commit types for ${sha?.slice(0, 16)}` + : 'Default conventional commit types' + }: ${JSON.stringify(types)}`, + ) + + return new RegExp(`^(${types.join('|')})!?(\\(.*\\))?!?:`) + } + + // Optimize for the common case that we don't have path segments with the + // same name as a conventional commit type. + const fullConventionalCommitRegex = makeConventionalCommitRegex( + conventionalCommitTypes, + ) + for (const commit of commits) { const logMsgStart = `Commit ${commit.sha}'s message's subject ("${commit.subject}")` + // If we have a commit `perf: ...`, and we touch a file containing the path + // segment "perf", we don't want to flag this. + const filteredTypes = conventionalCommitTypes.filter( + (type) => !commit.changedPathSegments.has(type), + ) + const conventionalCommitRegex = + filteredTypes.length === conventionalCommitTypes.length + ? fullConventionalCommitRegex + : makeConventionalCommitRegex(filteredTypes, commit.sha) + if (!commit.subject.includes(': ')) { core.error( `${logMsgStart} was detected as not meeting our guidelines because ` + @@ -84,6 +127,16 @@ async function checkCommitMessages({ github, context, core, repoPath }) { failures.add(commit.sha) } + if (conventionalCommitRegex.test(commit.subject)) { + core.error( + `${logMsgStart} was detected as not meeting our guidelines because ` + + 'it seems to use conventional commit (conventionalcommits.org) ' + + 'formatting. Nixpkgs has its own, different, commit message ' + + 'formatting standards.', + ) + failures.add(commit.sha) + } + if (!failures.has(commit.sha)) { core.info(`${logMsgStart} passed our automated checks!`) }