Prevent merging on outstanding fixup commits

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

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

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 %}

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.

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.
 

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(

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))