ci/github-script/lint-commits: error when conventional commit format is used (#495531)

.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',

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<ReturnType<InstanceType<import('@actions/github/lib/utils').GitHub>["rest"]["pulls"]["get"]>>["data"]
+ *  repoPath?: string,
+ * }} GetCommitMessagesForPRProps
+ *
+ * @returns {Promise<{
+ *  subject: string,
+ *  sha: string,
+ *  changedPaths: string[],
+ *  changedPathSegments: Set<string>,
+ * }[]>}
+ */
+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 }

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!`)
     }