Build a commit validation layer into CI []

.git-hooks/pre-push.py

@@ -36,7 +36,6 @@
 import json
 import os
 import platform
-import re
 import shutil
 import subprocess
 import sys
@@ -97,10 +96,6 @@ def _is_excluded_file_path(path_str: str) -> bool:
     path_str.startswith(prefix) for prefix in EXCLUDES_PATH_PREFIXES
   )
 
-COMMIT_SUBJECT_POLICY_REGEX = re.compile(
-  r'^.*\[(GH #[0-9]+|(EDG[cfjp]+fe/([0-9]+))|,)*\].*$'
-)
-
 TOOL_PATH_GIT = shutil.which('git')
 
 def run_git(args, *, check = True, text = True):
@@ -201,11 +196,10 @@ def check_commits(self):
       print_indented(show_output, '  ')
       print('')
 
-      subject = git_output(['show', '--no-patch', '--format=%s', commit])
-      if COMMIT_SUBJECT_POLICY_REGEX.match(subject) is None:
-        print(
-          "  [POLICY] Commit message doesn't contain valid PR number (or [])."
-        )
+      message = git_output(['show', '--no-patch', '--format=%B', commit])
+      problem = edgchanges.check_commit_subject(message)
+      if problem is not None:
+        print(f"  [POLICY] Commit subject: {problem}.")
         sys.exit(1)
 
       first_new_commit = commit

.github/workflows/changes-fragments.yml

@@ -5,8 +5,10 @@
 #
 # Check the pending Changes entries (*/pending_changes/*.txt) with
 # `edg-collect-changes --check` (format, spelling, typos, line length), so a
-# bad entry is caught before the nightly collect-changes job meets it, and run
-# the unit tests of the tools involved.
+# bad entry is caught before the nightly collect-changes job meets it.  This
+# checks the entries, not the tools; their unit tests are dev-tools-tests.yml.
+# The tool paths below are triggers only so that a change to the checks is
+# tried against the entries already pending.
 
 name: Pending Changes Entries
 
@@ -21,7 +23,6 @@ on:
       - dev_tools/bin/edg-collect-changes
       - dev_tools/pylibs/edgchanges/**
       - dev_tools/pylibs/edgutil/**
-      - dev_tools/tests/**
   pull_request:
     paths:
       - '**/pending_changes/**'
@@ -30,7 +31,6 @@ on:
       - dev_tools/bin/edg-collect-changes
       - dev_tools/pylibs/edgchanges/**
       - dev_tools/pylibs/edgutil/**
-      - dev_tools/tests/**
 
 permissions:
   contents: read
@@ -56,9 +56,6 @@ jobs:
           sudo apt-get update
           sudo apt-get install -y aspell aspell-en
 
-      - name: Run unit tests
-        run: python3 -m unittest discover -s dev_tools/tests -v
-
       - name: Check pending Changes entries
         run: |
           status=0

.github/workflows/commit-subjects.yml

@@ -0,0 +1,66 @@
+# Part of the EDG Compiler Project, under the Apache License v2.0 with LLVM
+# Exceptions.
+# See https://edgcpp.org/LICENSE.txt for license information.
+# SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+#
+# Check the subjects of the new commits pushed to main and of every pull
+# request's commits with edg-check-commit-subjects (the same policy the
+# pre-push hook applies, from edgchanges).  Only commits with a violation are
+# reported.
+#
+# A pull request is checked from its base commit to its head commit.  A push
+# is checked from the previous tip of main.  If that tip is unavailable (e.g.
+# after a force-push), the job fails rather than check nothing.
+#
+# Merge commits are rejected too.  A fork that wants them sets the repository
+# variable ALLOW_MERGE_COMMITS to 'true' (Settings > Secrets and variables >
+# Actions > Variables).
+
+name: Commit Subjects
+
+on:
+  push:
+    branches:
+      - main
+  pull_request:
+
+permissions:
+  contents: read
+
+jobs:
+  check:
+    name: check-commit-subjects
+    runs-on: ubuntu-latest
+    timeout-minutes: 10
+    env:
+      PYTHONPATH: ${{ github.workspace }}/dev_tools/pylibs
+    steps:
+      - name: Check out repository
+        uses: actions/checkout@v7
+        with:
+          fetch-depth: 0
+          ref: ${{ github.event.pull_request.head.sha || github.sha }}
+
+      - name: Check commit subjects
+        env:
+          EVENT_NAME: ${{ github.event_name }}
+          PR_BASE_SHA: ${{ github.event.pull_request.base.sha }}
+          PUSH_BEFORE_SHA: ${{ github.event.before }}
+          ALLOW_MERGE_COMMITS: ${{ vars.ALLOW_MERGE_COMMITS }}
+        run: |
+          if [[ "${EVENT_NAME}" == pull_request ]]; then
+            range=("${PR_BASE_SHA}..HEAD")
+          elif [[ "${PUSH_BEFORE_SHA}" != 0000000000000000000000000000000000000000 ]] &&
+               git cat-file -e "${PUSH_BEFORE_SHA}^{commit}" 2> /dev/null; then
+            range=("${PUSH_BEFORE_SHA}..HEAD")
+          else
+            echo "::error::No previous tip of the branch to compare against" \
+                 "(push before: ${PUSH_BEFORE_SHA}); the pushed commits" \
+                 "were not checked."
+            exit 1
+          fi
+          options=()
+          if [[ "${ALLOW_MERGE_COMMITS}" == true ]]; then
+            options+=(--allow-merges)
+          fi
+          dev_tools/bin/edg-check-commit-subjects "${options[@]}" "${range[@]}"

.github/workflows/dev-tools-tests.yml

@@ -0,0 +1,55 @@
+# Part of the EDG Compiler Project, under the Apache License v2.0 with LLVM
+# Exceptions.
+# See https://edgcpp.org/LICENSE.txt for license information.
+# SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+#
+# Run the unit tests of the dev_tools (dev_tools/tests).  Checking the pending
+# Changes entries themselves is changes-fragments.yml.
+
+name: Dev Tools Tests
+
+on:
+  push:
+    branches:
+      - main
+    paths:
+      - .github/workflows/dev-tools-tests.yml
+      - dev_tools/bin/edg-check-commit-subjects
+      - dev_tools/bin/edg-check-file-changes
+      - dev_tools/bin/edg-collect-changes
+      - dev_tools/pylibs/edgchanges/**
+      - dev_tools/pylibs/edgutil/**
+      - dev_tools/tests/**
+  pull_request:
+    paths:
+      - .github/workflows/dev-tools-tests.yml
+      - dev_tools/bin/edg-check-commit-subjects
+      - dev_tools/bin/edg-check-file-changes
+      - dev_tools/bin/edg-collect-changes
+      - dev_tools/pylibs/edgchanges/**
+      - dev_tools/pylibs/edgutil/**
+      - dev_tools/tests/**
+
+permissions:
+  contents: read
+
+jobs:
+  unit-tests:
+    name: dev-tools-unit-tests
+    runs-on: ubuntu-latest
+    timeout-minutes: 10
+    env:
+      PYTHONPATH: ${{ github.workspace }}/dev_tools/pylibs
+    steps:
+      - name: Check out repository
+        uses: actions/checkout@v7
+
+      # The same dictionary as the dev-env image
+      # (dev_tools/docker/dev-env/setup-scripts/install-software.sh).
+      - name: Install aspell
+        run: |
+          sudo apt-get update
+          sudo apt-get install -y aspell aspell-en
+
+      - name: Run unit tests
+        run: python3 -m unittest discover -s dev_tools/tests -v

dev_tools/bin/edg-check-commit-subjects

@@ -0,0 +1,155 @@
+#!/usr/bin/env python3
+
+# Part of the EDG Compiler Project, under the Apache License v2.0 with LLVM
+# Exceptions.
+# See https://edgcpp.org/LICENSE.txt for license information.
+# SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+
+'''Check a range of commits against the commit policy: the subject (see
+edgchanges.check_commit_subject), and no merge commits.
+
+The arguments are handed to git log, so any revision range it accepts works:
+
+  edg-check-commit-subjects origin/main..HEAD
+  edg-check-commit-subjects main HEAD ^origin/main
+
+A merge commit is a violation by itself; --allow-merges (for forks that use
+merge commits) skips merge commits instead, subject and all.
+
+Only commits with a violation are reported.  The exit status is 1 if any
+commit was reported, and 0 otherwise.
+'''
+
+import argparse
+import shutil
+import subprocess
+import sys
+
+from typing import List, NamedTuple, Optional
+
+import edgchanges
+import edgutil
+
+TOOL_PATH_GIT = shutil.which('git')
+
+Commit = NamedTuple(
+  'Commit',
+  [('hash', str), ('is_merge', bool), ('message', str)]
+)
+
+def _list_commits(log_args: List[str]) -> List[Commit]:
+  '''Return the commits git rev-list selects.
+
+  Commit messages are arbitrary bytes, so nothing here depends on their
+  content: rev-list (hashes only) feeds cat-file --batch, which gives the size
+  of each object it prints.'''
+  assert TOOL_PATH_GIT is not None
+  rev_list_proc = subprocess.Popen(
+    [TOOL_PATH_GIT, 'rev-list', *log_args],
+    stdout = subprocess.PIPE,
+    stderr = subprocess.PIPE
+  )
+  cat_file_proc = subprocess.Popen(
+    [TOOL_PATH_GIT, 'cat-file', '--batch'],
+    stdin = rev_list_proc.stdout,
+    stdout = subprocess.PIPE,
+    stderr = subprocess.PIPE
+  )
+  # Leave the pipe to cat-file, so rev-list sees SIGPIPE if it exits early.
+  assert rev_list_proc.stdout is not None
+  rev_list_proc.stdout.close()
+  data, cat_file_stderr = cat_file_proc.communicate()
+  assert rev_list_proc.stderr is not None
+  rev_list_stderr = rev_list_proc.stderr.read()
+  rev_list_proc.stderr.close()
+
+  for returncode, stderr in (
+    (rev_list_proc.wait(), rev_list_stderr),
+    (cat_file_proc.returncode, cat_file_stderr)
+  ):
+    if returncode != 0:
+      raise RuntimeError(stderr.decode('utf-8', errors = 'replace').strip())
+
+  commits = []
+  position = 0
+  while position < len(data):
+    line_end = data.index(b'\n', position)
+    header = data[position:line_end].split()
+    if len(header) != 3 or header[1] != b'commit':
+      raise RuntimeError(f"unexpected object: {data[position:line_end]!r}")
+    content_start = line_end + 1
+    content_end = content_start + int(header[2])
+    # The message follows the first blank line, after the headers.
+    headers, _, message = (
+      data[content_start:content_end].partition(b'\n\n')
+    )
+    parent_count = sum(
+      1 for line in headers.split(b'\n') if line.startswith(b'parent ')
+    )
+    commits.append(
+      Commit(
+        header[0].decode(),
+        parent_count > 1,
+        message.decode('utf-8', errors = 'replace')
+      )
+    )
+    position = content_end + 1
+  return commits
+
+def check_commits(log_args: List[str], *, allow_merges: bool) -> int:
+  '''Report each commit selected by log_args that violates the policy; return
+  the number of such commits.'''
+  violations = 0
+  for commit in _list_commits(log_args):
+    if commit.is_merge and allow_merges:
+      continue
+
+    problem: Optional[str]
+    if commit.is_merge:
+      problem = 'merge commits are not allowed'
+    else:
+      problem = edgchanges.check_commit_subject(commit.message)
+    if problem is not None:
+      print(f"{commit.hash[:12]} {commit.message.partition(chr(10))[0]}")
+      print(f"  {problem}")
+      violations += 1
+  return violations
+
+def _parse_args() -> argparse.Namespace:
+  parser = argparse.ArgumentParser(
+    description = (
+      'Report the commits in a range that violate the commit policy '
+      '(subject format, no merge commits).'
+    )
+  )
+  parser.add_argument(
+    '--allow-merges',
+    action = 'store_true',
+    help = 'permit merge commits (and skip them) instead of reporting them'
+  )
+  parser.add_argument(
+    'log_args',
+    nargs = '+',
+    metavar = 'GIT_LOG_ARG',
+    help = 'arguments for git log, e.g. a range such as origin/main..HEAD'
+  )
+  return parser.parse_args()
+
+def main() -> int:
+  args = _parse_args()
+  if TOOL_PATH_GIT is None:
+    edgutil.eprint('error: git not found')
+    return 1
+
+  try:
+    violations = check_commits(
+      args.log_args,
+      allow_merges = args.allow_merges
+    )
+  except RuntimeError as ex:
+    edgutil.eprint(f"error: {ex}")
+    return 1
+  return 1 if violations != 0 else 0
+
+if __name__ == '__main__':
+  sys.exit(main())