From 7fb96f506a9c47ae3dc465f134f8b895f68d7683 Mon Sep 17 00:00:00 2001 From: Thomas Flament Date: Mon, 16 Feb 2026 16:58:40 +0100 Subject: [PATCH] Prevent merging on outstanding fixup commits Issue: PTFE-3031 --- bert_e/exceptions.py | 6 ++ bert_e/lib/git.py | 9 +++ bert_e/templates/fixup_commit_detected.md | 17 +++++ bert_e/tests/test_bert_e.py | 86 +++++++++++++++++++++++ bert_e/workflow/gitwaterflow/__init__.py | 26 ++++++- bert_e/workflow/gitwaterflow/commands.py | 5 ++ bert_e/workflow/gitwaterflow/utils.py | 5 ++ 7 files changed, 153 insertions(+), 1 deletion(-) create mode 100644 bert_e/templates/fixup_commit_detected.md diff --git a/bert_e/exceptions.py b/bert_e/exceptions.py index fb93bdcf..8e642909 100644 --- a/bert_e/exceptions.py +++ b/bert_e/exceptions.py @@ -273,6 +273,12 @@ class QueueBuildFailedMessage(TemplateException): template = "queue_build_failed.md" +class FixupCommitDetected(TemplateException): + code = 137 + template = "fixup_commit_detected.md" + status = "failure" + + # internal exceptions class UnableToSendEmail(InternalException): code = 201 diff --git a/bert_e/lib/git.py b/bert_e/lib/git.py index 8908a42b..c33b7613 100644 --- a/bert_e/lib/git.py +++ b/bert_e/lib/git.py @@ -302,6 +302,7 @@ def __init__(self, repo, sha1, author=None, parents=None): self._repo = repo self.sha1 = sha1 self._author = author + self._message = None try: self._parents = [Commit(repo, parent) for parent in parents] except TypeError: @@ -321,6 +322,14 @@ def author(self): ) return self._author + @property + def message(self): + if self._message is None: + self._message = self._repo.cmd( + 'git log -1 --pretty="%%s" %s', self.sha1 + ).strip() + return self._message + @property def is_merge(self): return len(self.parents) > 1 diff --git a/bert_e/templates/fixup_commit_detected.md b/bert_e/templates/fixup_commit_detected.md new file mode 100644 index 00000000..31a15b4d --- /dev/null +++ b/bert_e/templates/fixup_commit_detected.md @@ -0,0 +1,17 @@ +{% extends "message.md" %} + +{% block title -%} +Fixup commits detected +{% endblock %} + +{% block message %} +The following commits on branch `{{ src_branch }}` appear to be intended for +interactive rebase and must be squashed before merging: + +{% for commit in fixup_commits %} +- `{{ commit.sha1[:12] }}` {{ commit.message }} +{% endfor %} + +Please squash these commits using `git rebase -i` and force-push the result +to your branch. +{% endblock %} diff --git a/bert_e/tests/test_bert_e.py b/bert_e/tests/test_bert_e.py index 90009e56..ea72ae2b 100644 --- a/bert_e/tests/test_bert_e.py +++ b/bert_e/tests/test_bert_e.py @@ -784,6 +784,7 @@ class RepositoryTests(unittest.TestCase): bypass_all = [ 'bypass_author_approval', 'bypass_build_status', + 'bypass_fixup_check', 'bypass_incompatible_branch', 'bypass_jira_check', 'bypass_peer_approval', @@ -4779,6 +4780,91 @@ def test_admin_self_bypass(self): with self.assertRaises(exns.SuccessMessage): self.handle(pr.id, backtrace=True) + def test_fixup_commit_detected(self): + pr = self.create_pr('bugfix/TEST-00501', 'development/4.3') + add_file_to_branch( + self.gitrepo, 'bugfix/TEST-00501', 'base_file') + self.gitrepo.cmd('git checkout bugfix/TEST-00501') + self.gitrepo.cmd('echo fixup > fixup_file') + self.gitrepo.cmd('git add fixup_file') + self.gitrepo.cmd( + 'git commit -m "fixup! adds base_file file on ' + 'bugfix/TEST-00501"') + self.gitrepo.cmd('git push origin bugfix/TEST-00501') + + with self.assertRaises(exns.FixupCommitDetected): + self.handle(pr.id, + options=self.bypass_all_but(['bypass_fixup_check']), + backtrace=True) + + def test_squash_commit_detected(self): + pr = self.create_pr('bugfix/TEST-00502', 'development/4.3') + self.gitrepo.cmd('git checkout bugfix/TEST-00502') + self.gitrepo.cmd('echo squash > squash_file') + self.gitrepo.cmd('git add squash_file') + self.gitrepo.cmd( + 'git commit -m "squash! initial commit"') + self.gitrepo.cmd('git push origin bugfix/TEST-00502') + + with self.assertRaises(exns.FixupCommitDetected): + self.handle(pr.id, + options=self.bypass_all_but(['bypass_fixup_check']), + backtrace=True) + + def test_amend_commit_detected(self): + pr = self.create_pr('bugfix/TEST-00503', 'development/4.3') + self.gitrepo.cmd('git checkout bugfix/TEST-00503') + self.gitrepo.cmd('echo amend > amend_file') + self.gitrepo.cmd('git add amend_file') + self.gitrepo.cmd( + 'git commit -m "amend! initial commit"') + self.gitrepo.cmd('git push origin bugfix/TEST-00503') + + with self.assertRaises(exns.FixupCommitDetected): + self.handle(pr.id, + options=self.bypass_all_but(['bypass_fixup_check']), + backtrace=True) + + def test_fixup_commit_bypass(self): + pr = self.create_pr('bugfix/TEST-00504', 'development/4.3') + self.gitrepo.cmd('git checkout bugfix/TEST-00504') + self.gitrepo.cmd('echo fixup > fixup_file') + self.gitrepo.cmd('git add fixup_file') + self.gitrepo.cmd( + 'git commit -m "fixup! initial commit"') + self.gitrepo.cmd('git push origin bugfix/TEST-00504') + + with self.assertRaises(exns.SuccessMessage): + self.handle(pr.id, + options=self.bypass_all, + backtrace=True) + + def test_fixup_commit_bypass_via_comment(self): + pr = self.create_pr('bugfix/TEST-00505', 'development/4.3') + self.gitrepo.cmd('git checkout bugfix/TEST-00505') + self.gitrepo.cmd('echo fixup > fixup_file') + self.gitrepo.cmd('git add fixup_file') + self.gitrepo.cmd( + 'git commit -m "fixup! initial commit"') + self.gitrepo.cmd('git push origin bugfix/TEST-00505') + + pr_admin = self.admin_bb.get_pull_request(pull_request_id=pr.id) + pr_admin.add_comment( + '@%s bypass_fixup_check' % self.args.robot_username) + + with self.assertRaises(exns.SuccessMessage): + self.handle(pr.id, + options=self.bypass_all_but(['bypass_fixup_check']), + backtrace=True) + + def test_no_fixup_commits_passes(self): + pr = self.create_pr('bugfix/TEST-00506', 'development/4.3') + + with self.assertRaises(exns.SuccessMessage): + self.handle(pr.id, + options=self.bypass_all, + backtrace=True) + class TestQueueing(RepositoryTests): """Tests which validate all things related to the merge queue. diff --git a/bert_e/workflow/gitwaterflow/__init__.py b/bert_e/workflow/gitwaterflow/__init__.py index 7bda9d6c..369b12d6 100644 --- a/bert_e/workflow/gitwaterflow/__init__.py +++ b/bert_e/workflow/gitwaterflow/__init__.py @@ -31,7 +31,8 @@ ) from .utils import ( bypass_incompatible_branch, bypass_peer_approval, - bypass_author_approval, bypass_leader_approval, bypass_build_status + bypass_author_approval, bypass_leader_approval, bypass_build_status, + bypass_fixup_check ) from .commands import setup # noqa from .integration import (check_integration_branches, @@ -196,6 +197,7 @@ def _handle_pull_request(job: PullRequestJob): notify_integration_data(job, wbranches, child_prs) check_approvals(job) + check_fixup_commits(job) check_build_status(job, wbranches) interactive = job.settings.interactive @@ -621,6 +623,28 @@ def check_approvals(job): ) +def check_fixup_commits(job): + if bypass_fixup_check(job): + return + + FIXUP_PREFIXES = ('fixup! ', 'squash! ', 'amend! ') + + commits = list( + job.git.src_branch.get_commit_diff(job.git.dst_branch) + ) + fixup_commits = [ + c for c in commits + if any(c.message.startswith(prefix) for prefix in FIXUP_PREFIXES) + ] + + if fixup_commits: + raise messages.FixupCommitDetected( + src_branch=job.git.src_branch.name, + fixup_commits=fixup_commits, + active_options=job.active_options + ) + + def check_build_status(job, wbranches): """Check the build statuses of the integration pull requests. diff --git a/bert_e/workflow/gitwaterflow/commands.py b/bert_e/workflow/gitwaterflow/commands.py index eb5b5bc2..d369c6ec 100644 --- a/bert_e/workflow/gitwaterflow/commands.py +++ b/bert_e/workflow/gitwaterflow/commands.py @@ -196,6 +196,11 @@ def setup(defaults={}): "Bypass the pull request leaders' approval", privileged=True, default=defaults.get("bypass_leader_approval", False)) + Reactor.add_option( + "bypass_fixup_check", + "Bypass the check for fixup/squash/amend commits", + privileged=True, + default=defaults.get("bypass_fixup_check", False)) # Other options Reactor.add_option( diff --git a/bert_e/workflow/gitwaterflow/utils.py b/bert_e/workflow/gitwaterflow/utils.py index bc710066..032a802c 100644 --- a/bert_e/workflow/gitwaterflow/utils.py +++ b/bert_e/workflow/gitwaterflow/utils.py @@ -27,3 +27,8 @@ def bypass_build_status(job): def bypass_jira_check(job): return (job.settings.bypass_jira_check or job.author_bypass.get('bypass_jira_check', False)) + + +def bypass_fixup_check(job): + return (job.settings.bypass_fixup_check or + job.author_bypass.get('bypass_fixup_check', False))