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..fcccfeacd75e --- /dev/null +++ b/ci/github-script/get-pr-commit-details.js @@ -0,0 +1,101 @@ +// @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) +} + +/** + * 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"] + * repoPath?: string, + * }} GetCommitMessagesForPRProps + * + * @returns {Promise<{ + * subject: string, + * sha: string, + * changedPaths: string[], + * changedPathSegments: Set, + * }[]>} + */ +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: [ + '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) => { + // Subject first, then a blank line, then filenames. + const result = ( + await runGit({ + args: ['log', '--format=%s', '--name-only', '-1', sha], + repoPath, + core, + quiet: true, + }) + ).stdout.split('\n') + + const subject = result[0] + + const changedPaths = result.slice(2, -1) + + const changedPathSegments = new Set( + changedPaths.flatMap((path) => path.split('/')), + ) + + return { + sha, + subject, + changedPaths, + changedPathSegments, + } + }), + ) +} + +module.exports = { getCommitDetailsForPR } diff --git a/ci/github-script/lint-commits.js b/ci/github-script/lint-commits.js index 21d709cadcbe..4ef8b6f6ab04 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,84 +46,70 @@ 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({ 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 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 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 (!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.', @@ -153,15 +118,25 @@ 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) } + 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!`) }