diff --git a/.git-hooks/pre-push.py b/.git-hooks/pre-push.py index 27f447753a..b1ca14545b 100755 --- a/.git-hooks/pre-push.py +++ b/.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): @@ -182,7 +177,7 @@ def check_commits(self): As commits are walked, check to make sure they match formatting guidance. Returns the oldest new commit hash, or None if there are no new commits. - Exits immediately if a commit subject fails the PR-number policy. + Exits immediately if a commit message fails the policy. ''' first_new_commit = None @@ -201,11 +196,15 @@ 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 [])." - ) + # Read bytes: text mode would turn any \r into \n, hiding it from the + # check. Don't strip: that would also remove \r, \x0c, etc. at the ends + # of the message. + message = run_git( + ['show', '--no-patch', '--format=%B', commit], text = False + ).stdout.decode('utf-8', errors = 'replace') + problem = edgchanges.check_commit_message(message) + if problem is not None: + print(f" [POLICY] Commit message: {problem}.") sys.exit(1) first_new_commit = commit diff --git a/.github/workflows/acknowledg-deploy.yml b/.github/workflows/acknowledg-deploy.yml index e13f938a9b..276dfc4f67 100644 --- a/.github/workflows/acknowledg-deploy.yml +++ b/.github/workflows/acknowledg-deploy.yml @@ -53,6 +53,8 @@ jobs: steps: - name: Check out repository uses: actions/checkout@v7 + with: + persist-credentials: false - name: Set up Node uses: actions/setup-node@v7 @@ -123,6 +125,8 @@ jobs: steps: - name: Check out repository uses: actions/checkout@v7 + with: + persist-credentials: false - name: Set up uv uses: astral-sh/setup-uv@v10.3.0 diff --git a/.github/workflows/acknowledg-forge-deploy.yml b/.github/workflows/acknowledg-forge-deploy.yml index c1292cd5b4..bc6700e35a 100644 --- a/.github/workflows/acknowledg-forge-deploy.yml +++ b/.github/workflows/acknowledg-forge-deploy.yml @@ -67,6 +67,8 @@ jobs: steps: - name: Check out repository uses: actions/checkout@v7 + with: + persist-credentials: false - name: Set up uv uses: astral-sh/setup-uv@v10.3.0 diff --git a/.github/workflows/acknowledg-forge.yml b/.github/workflows/acknowledg-forge.yml index dce391b24c..37e5a4a736 100644 --- a/.github/workflows/acknowledg-forge.yml +++ b/.github/workflows/acknowledg-forge.yml @@ -52,6 +52,8 @@ jobs: steps: - name: Check out repository uses: actions/checkout@v7 + with: + persist-credentials: false - name: Set up uv uses: astral-sh/setup-uv@v10.3.0 @@ -73,6 +75,8 @@ jobs: steps: - name: Check out repository uses: actions/checkout@v7 + with: + persist-credentials: false - name: Set up uv uses: astral-sh/setup-uv@v10.3.0 diff --git a/.github/workflows/build-and-test-ci.yml b/.github/workflows/build-and-test-ci.yml index 6888e76c18..ffbfc5df3b 100644 --- a/.github/workflows/build-and-test-ci.yml +++ b/.github/workflows/build-and-test-ci.yml @@ -43,6 +43,9 @@ on: - '**/pending_changes/**' pull_request: +permissions: + contents: read + concurrency: group: ${{ github.workflow }}-${{ github.event.pull_request.number || github.sha }} cancel-in-progress: true @@ -77,6 +80,8 @@ jobs: steps: - name: Check out repository uses: actions/checkout@v7 + with: + persist-credentials: false - name: Configure linux-gcc-debug env: @@ -120,6 +125,8 @@ jobs: steps: - name: Check out repository uses: actions/checkout@v7 + with: + persist-credentials: false - name: Configure linux-i686-gcc-debug env: @@ -170,6 +177,8 @@ jobs: steps: - name: Check out repository uses: actions/checkout@v7 + with: + persist-credentials: false - name: Download linux-gcc-debug build uses: actions/download-artifact@v7 @@ -283,6 +292,8 @@ jobs: steps: - name: Check out repository uses: actions/checkout@v7 + with: + persist-credentials: false - name: Download linux-i686-gcc-debug build uses: actions/download-artifact@v7 @@ -436,6 +447,8 @@ jobs: steps: - name: Check out repository uses: actions/checkout@v7 + with: + persist-credentials: false - name: Download per-config coverage reports continue-on-error: true @@ -502,6 +515,8 @@ jobs: steps: - name: Check out repository uses: actions/checkout@v7 + with: + persist-credentials: false - name: Configure linux-gcc-release env: diff --git a/.github/workflows/changes-fragments.yml b/.github/workflows/changes-fragments.yml index a1fef0c2ed..1d66f33989 100644 --- a/.github/workflows/changes-fragments.yml +++ b/.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 @@ -48,6 +48,8 @@ jobs: steps: - name: Check out repository uses: actions/checkout@v7 + with: + persist-credentials: false # The same dictionary as the dev-env image # (dev_tools/docker/dev-env/setup-scripts/install-software.sh). @@ -56,9 +58,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 diff --git a/.github/workflows/commit-messages.yml b/.github/workflows/commit-messages.yml new file mode 100644 index 0000000000..e53e781033 --- /dev/null +++ b/.github/workflows/commit-messages.yml @@ -0,0 +1,79 @@ +# 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 messages of the new commits pushed to main and of every pull +# request's commits with edg-check-commit-messages (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. +# +# The checker and the commits come from one checkout of github.sha, the same +# commit GitHub loads this workflow from. For a pull request that is the merge +# commit, so the checker is there even if the pull request's branch predates +# it. +# +# 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 Messages + +on: + push: + branches: + - main + pull_request: + +permissions: + contents: read + +jobs: + check: + name: check-commit-messages + runs-on: ubuntu-latest + timeout-minutes: 10 + env: + PYTHONPATH: ${{ github.workspace }}/dev_tools/pylibs + steps: + - name: Check out repository + uses: actions/checkout@v7 + with: + # The checker is pull-request code; keep the token away from it. + # Nothing here needs authenticated git. + persist-credentials: false + fetch-depth: 0 + # Only commit messages are read, so skip the blobs and check out + # just the checker. + filter: blob:none + sparse-checkout: dev_tools + ref: ${{ github.sha }} + + - name: Check commit messages + env: + EVENT_NAME: ${{ github.event_name }} + PR_BASE_SHA: ${{ github.event.pull_request.base.sha }} + PR_HEAD_SHA: ${{ github.event.pull_request.head.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}..${PR_HEAD_SHA}") + 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-messages "${options[@]}" "${range[@]}" diff --git a/.github/workflows/dev-tools-tests.yml b/.github/workflows/dev-tools-tests.yml new file mode 100644 index 0000000000..d39164e490 --- /dev/null +++ b/.github/workflows/dev-tools-tests.yml @@ -0,0 +1,57 @@ +# 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-messages + - 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-messages + - 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 + with: + persist-credentials: false + + # 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 diff --git a/.github/workflows/documentation.yml b/.github/workflows/documentation.yml index 28db564634..baebcd9b7e 100644 --- a/.github/workflows/documentation.yml +++ b/.github/workflows/documentation.yml @@ -52,6 +52,8 @@ jobs: steps: - name: Check out repository uses: actions/checkout@v7 + with: + persist-credentials: false # The published site uses the dirhtml builder, so check that one. - name: Build HTML diff --git a/.github/workflows/issue-templates.yml b/.github/workflows/issue-templates.yml index 479836c3d3..a5fcb9e6ce 100644 --- a/.github/workflows/issue-templates.yml +++ b/.github/workflows/issue-templates.yml @@ -39,6 +39,8 @@ jobs: steps: - name: Check out repository uses: actions/checkout@v7 + with: + persist-credentials: false - name: Compare issue and discussion forms run: python3 dev_tools/bin/edg-check-issue-templates diff --git a/.github/workflows/strict-build-check.yml b/.github/workflows/strict-build-check.yml index 36655159e7..d35a4188f6 100644 --- a/.github/workflows/strict-build-check.yml +++ b/.github/workflows/strict-build-check.yml @@ -18,6 +18,9 @@ on: - '**/pending_changes/**' pull_request: +permissions: + contents: read + concurrency: group: ${{ github.workflow }}-${{ github.event.pull_request.number || github.sha }} cancel-in-progress: true @@ -30,6 +33,8 @@ jobs: steps: - name: Check out repository uses: actions/checkout@v7 + with: + persist-credentials: false - name: Install Ninja run: choco install ninja -y @@ -166,6 +171,8 @@ jobs: steps: - name: Check out repository uses: actions/checkout@v7 + with: + persist-credentials: false - name: Configure linux-gcc-release env: @@ -191,6 +198,8 @@ jobs: steps: - name: Check out repository uses: actions/checkout@v7 + with: + persist-credentials: false - name: Configure linux-gcc-debug env: @@ -216,6 +225,8 @@ jobs: steps: - name: Check out repository uses: actions/checkout@v7 + with: + persist-credentials: false - name: Configure linux-clang-release env: @@ -241,6 +252,8 @@ jobs: steps: - name: Check out repository uses: actions/checkout@v7 + with: + persist-credentials: false - name: Configure linux-clang-debug env: diff --git a/.github/workflows/typecheck-python.yml b/.github/workflows/typecheck-python.yml index 67e90c948a..fecde86e66 100644 --- a/.github/workflows/typecheck-python.yml +++ b/.github/workflows/typecheck-python.yml @@ -20,6 +20,9 @@ on: - '**/pending_changes/**' pull_request: +permissions: + contents: read + concurrency: group: ${{ github.workflow }}-${{ github.event.pull_request.number || github.sha }} cancel-in-progress: true @@ -50,6 +53,8 @@ jobs: steps: - name: Check out repository uses: actions/checkout@v7 + with: + persist-credentials: false - name: Set up Python ${{ matrix.python-version }} if: ${{ !matrix.use-docker }} diff --git a/dev_tools/bin/edg-check-commit-messages b/dev_tools/bin/edg-check-commit-messages new file mode 100755 index 0000000000..d1a643ceb4 --- /dev/null +++ b/dev_tools/bin/edg-check-commit-messages @@ -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 message (see +edgchanges.check_commit_message), and no merge commits. + +The arguments are handed to git log, so any revision range it accepts works: + + edg-check-commit-messages origin/main..HEAD + edg-check-commit-messages main HEAD ^origin/main + +A merge commit is a violation by itself; --allow-merges (for forks that use +merge commits) skips merge commits instead, message 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_message(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 ' + '(message 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()) diff --git a/dev_tools/pylibs/edgchanges/__init__.py b/dev_tools/pylibs/edgchanges/__init__.py index 154dd39aec..bb1110b9d5 100644 --- a/dev_tools/pylibs/edgchanges/__init__.py +++ b/dev_tools/pylibs/edgchanges/__init__.py @@ -47,6 +47,15 @@ TAG_REGEX = re.compile(r'^GH #[0-9]+$') +# The internal issue tracker form of a tag; accepted in commit messages but +# not in Changes entries. +EDG_TAG_REGEX = re.compile(r'^EDG[cfjp]+fe/[0-9]+$') + +# The bracketed group at the end of a commit subject, which may be followed by +# the closing quotes of a git revert subject (Revert "...", nested to any +# depth); see check_commit_message. +_SUBJECT_TAGS_REGEX = re.compile(r'\[([^\[\]]*)\]"*$') + # A problem with an entry's format; line_no is None for the whole file. FormatError = NamedTuple( 'FormatError', @@ -60,6 +69,47 @@ def is_fragment_path(path_str: str) -> bool: return (path.suffix == FRAGMENT_SUFFIX and path.parent.name == PENDING_CHANGES_DIR_NAME) +def _is_subject_tag(tag: str) -> bool: + return (TAG_REGEX.match(tag) is not None or + EDG_TAG_REGEX.match(tag) is not None) + +def check_commit_message(message: str) -> Optional[str]: + '''Return a description of the problem with a commit message, or None if + it's acceptable. + + The subject is the first line, which must stand alone (be followed by a + blank line, if anything) and end with a bracketed group that is either empty + (for work with no issue) or a comma-separated list of tags: "GH #N" (as in a + Changes entry) or the internal form "EDGcpfe/N". Earlier bracketed text + (e.g. "[[always_inline]]") is not checked. Nothing in the message may break + a line other than \n.''' + # Keep the history friendly to simple line-based tooling: only \n may break + # a line (git's rule), so splitlines and split('\n') agree. This also + # rejects CRLF, which git's default cleanup never leaves in a message. + # The trailing newlines are dropped because only split yields their empty + # final element. + text = message.rstrip('\n') + lines = text.split('\n') + if text != '' and lines != text.splitlines(): + return ('message must not contain line-break characters other than' + ' newline (e.g. carriage return)') + + subject = lines[0] + # Like git, count a whitespace-only line as blank. + if len(lines) > 1 and lines[1].strip() != '': + return 'subject must be a single line; wrap the body after a blank line' + + match = _SUBJECT_TAGS_REGEX.search(subject) + if match is None: + return "subject must end with a tag list (use e.g. '[GH #123]', or '[]')" + + group = match.group(1) + if group != '' and not all( + _is_subject_tag(tag.strip()) for tag in group.split(',')): + return f"invalid tag list '[{group}]'; use e.g. '[GH #123]', or '[]'" + + return None + def target_changes_path(fragment_path: Path) -> Path: '''Return the Changes file the entry at fragment_path will be collected into (the one beside its pending_changes/ directory).''' diff --git a/dev_tools/tests/test_edg_check_commit_messages.py b/dev_tools/tests/test_edg_check_commit_messages.py new file mode 100644 index 0000000000..3d53310edc --- /dev/null +++ b/dev_tools/tests/test_edg_check_commit_messages.py @@ -0,0 +1,185 @@ +# 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 + +'''Tests for the commit policy: edgchanges.check_commit_message (shared with +the pre-push hook) and edg-check-commit-messages. + +Run with: python3 -m unittest discover -s dev_tools/tests +(with dev_tools/pylibs on PYTHONPATH). +''' + +import os +import subprocess +import sys +import tempfile +import unittest + +import edgchanges + +from pathlib import Path + +DEV_TOOLS_DIR = Path(__file__).resolve().parents[1] +MESSAGES_TOOL_PATH = DEV_TOOLS_DIR / 'bin' / 'edg-check-commit-messages' + +class CommitMessageTests(unittest.TestCase): + '''The commit message policy, shared by the pre-push hook and + edg-check-commit-messages.''' + + def test_valid_messages(self) -> None: + for message in ( + 'Fix thing [GH #213]', + 'Fix thing [EDGcpfe/29032]', + 'Fix bug [EDGcpfe/29055,EDGcpfe/28999]', + 'Fix bug [GH #1, GH #2]', + 'Mixed [nope] and [GH #7]', + 'No issue []', + 'Drop [[always_inline]] on setjmp [GH #176]', + 'Fix thing [GH #1]\n', + # git revert and Reapply subjects, nested ones included. + 'Revert "Fix thing [GH #1]"\n\nThis reverts commit 0123abcd.', + 'Reapply "Fix thing [EDGcpfe/28806]"', + 'Revert "Revert "Fix thing [GH #1]""', + 'Revert "Fix thing []"', + 'Fix thing [GH #1]\n\nBody mentions [nope]', + # Trailing newlines (git show adds some) don't change the verdict. + 'Fix thing [GH #1]\n ', + 'Fix thing [GH #1]\n \nBody', + ): + with self.subTest(message = message): + self.assertIsNone(edgchanges.check_commit_message(message)) + self.assertIsNone( + edgchanges.check_commit_message(message.rstrip('\n')) + ) + + def test_invalid_messages(self) -> None: + for message in ( + '', + 'No tag at all', + 'Bad ref [GH 213]', + 'Comma junk [,,]', + 'Run together [GH #1GH #2]', + 'Unclosed [GH #1', + 'Last wins [GH #7] but [nope]', + 'Tag not at the end [GH #1] of the subject', + 'Wrapped subject\n[GH #1]', + # Trailing whitespace on the subject isn't tolerated. + 'Fix thing [GH #1] ', + 'Fix thing [GH #1] \n\nBody', + 'Revert "Fix thing [GH #1]" ', + 'Revert "Fix thing"', + 'Revert "Fix thing [GH 1]"', + 'Revert "Fix thing [GH #1] and more"', + 'Wrapped subject [GH #1]\ncontinued', + 'No tag here\n\nBut the body has [GH #1]', + # Anything but \n that splitlines treats as a line break, anywhere. + 'Fix thing [GH #1]\r\n\r\nBody', + 'Fix thing [GH #1]\n\nBody\rmore', + 'Fix\x0cthing [GH #1]', + 'Fix thing [GH #1]\n\nBody\u2028more', + ): + with self.subTest(message = message): + self.assertIsNotNone(edgchanges.check_commit_message(message)) + self.assertIsNotNone( + edgchanges.check_commit_message(message.rstrip('\n')) + ) + + def test_tool_reports_only_violations(self) -> None: + with tempfile.TemporaryDirectory() as tmp_dir_str: + git = ['git', '-C', tmp_dir_str] + subprocess.run(git + ['init', '-q'], check = True) + for subject in ('Good [GH #1]', 'Bad one', 'Also good []\n\nBody'): + subprocess.run( + git + ['-c', 'user.name=t', '-c', 'user.email=t@example.com', + 'commit', '-q', '--allow-empty', '-m', subject], + check = True + ) + def run_tool(*args: str) -> 'subprocess.CompletedProcess[str]': + return subprocess.run( + [sys.executable, str(MESSAGES_TOOL_PATH), *args], + cwd = tmp_dir_str, + capture_output = True, + text = True, + env = dict(os.environ, PYTHONPATH = str(DEV_TOOLS_DIR / 'pylibs')) + ) + + result = run_tool('HEAD') + self.assertEqual(result.returncode, 1) + self.assertIn('Bad one', result.stdout) + self.assertNotIn('Good', result.stdout) + + result = run_tool('HEAD~1..HEAD') + self.assertEqual(result.returncode, 0) + self.assertEqual(result.stdout, '') + + # A merge commit is reported even with a good subject, unless merges + # are allowed. + commit = ['-c', 'user.name=t', '-c', 'user.email=t@example.com'] + subprocess.run(git + ['checkout', '-q', '-b', 'side', 'HEAD~1'], + check = True) + subprocess.run( + git + commit + ['commit', '-q', '--allow-empty', '-m', 'Side [GH #2]'], + check = True + ) + subprocess.run(git + ['checkout', '-q', '-'], check = True) + subprocess.run( + git + commit + ['merge', '-q', '--no-ff', 'side', + '-m', 'Merge side [GH #2]'], + check = True + ) + result = run_tool('HEAD~1..HEAD') + self.assertEqual(result.returncode, 1) + self.assertIn('Merge side', result.stdout) + self.assertIn('merge commits are not allowed', result.stdout) + + result = run_tool('--allow-merges', 'HEAD~1..HEAD') + self.assertEqual(result.returncode, 0) + self.assertEqual(result.stdout, '') + + def test_tool_hostile_messages(self) -> None: + '''Messages containing the characters a naive parser might use as + delimiters can't hide a violation or break the tool.''' + with tempfile.TemporaryDirectory() as tmp_dir_str: + git = ['git', '-C', tmp_dir_str] + subprocess.run(git + ['init', '-q'], check = True) + tree = subprocess.run( + git + ['hash-object', '-t', 'tree', '-w', '--stdin'], + input = b'', capture_output = True, check = True + ).stdout.decode().strip() + ident = b'author t 0 +0000\ncommitter t 0 +0000\n' + parent = '' + for message in ( + b'Wrapped [GH #1]\x01\ncontinued', + b'Fake\x01deadbeef\x00\x00Good [GH #1]\x01', + b'Embedded NUL [GH #1]\n\nbody\x00\x01more', + ): + content = b'tree ' + tree.encode() + b'\n' + if parent != '': + content += b'parent ' + parent.encode() + b'\n' + content += ident + b'\n' + message + parent = subprocess.run( + [ + *git, + 'hash-object', + '-t', + 'commit', + '-w', + '--literally', + '--stdin' + ], + input = content, capture_output = True, check = True + ).stdout.decode().strip() + result = subprocess.run( + [sys.executable, str(MESSAGES_TOOL_PATH), parent], + cwd = tmp_dir_str, + capture_output = True, + text = True, + env = dict(os.environ, PYTHONPATH = str(DEV_TOOLS_DIR / 'pylibs')) + ) + self.assertEqual(result.returncode, 1) + self.assertEqual(result.stderr, '') + # Only the first and second commits violate the policy (the wrapped + # subject, and the fake-record one that doesn't end in a tag). + self.assertEqual(result.stdout.count('\n '), 2) + self.assertIn('single line', result.stdout) diff --git a/dev_tools/typecheck-python.sh b/dev_tools/typecheck-python.sh index 60c7817375..2719027da9 100755 --- a/dev_tools/typecheck-python.sh +++ b/dev_tools/typecheck-python.sh @@ -57,6 +57,7 @@ pushd bin/ > /dev/null check_types_strict edg-bench-run-delta check_types_strict edg-bench-run-save check_types edg-cmakedef + check_types_strict edg-check-commit-messages check_types_strict edg-collect-changes check_types_strict edg-docker-bench check_types_strict edg-docker-test