[IMP] runbot_merge: commit message edition abstraction

Prepares for more complex edition operations on the forwardbot side

* split out the pseudo-headers from the message body
* don't separate the co-authored-by headers from the others, seems
  unnecessary, we just need to ensure they're at the end so github
  doesn't miss them (/it)

runbot_merge/models/pull_requests.py

@@ -1,6 +1,6 @@
 import base64
-import collections
 import datetime
+import io
 import json
 import logging
 import os
@@ -11,9 +11,11 @@ import time
 from itertools import takewhile
 
 import requests
+from werkzeug.datastructures import Headers
 
 from odoo import api, fields, models, tools
 from odoo.exceptions import ValidationError
+from odoo.tools import OrderedSet
 
 from .. import github, exceptions, controllers, utils
 
@@ -989,34 +991,27 @@ class PullRequests(models.Model):
             if commit:
                 self.env.cr.commit()
 
+    def _parse_commit_message(self, message):
+        """ Parses a commit message to split out the pseudo-headers (which
+        should be at the end) from the body, and serialises back with a
+        predefined pseudo-headers ordering.
+        """
+        return Message.from_message(message)
+
     def _build_merge_message(self, message):
         # handle co-authored commits (https://help.github.com/articles/creating-a-commit-with-multiple-authors/)
-        original = message.splitlines()
-        lines = []
-        coauthors = []
-        for line in original:
-            if line.startswith('Co-authored-by:'):
-                # remove all empty lines before C-A-B
-                coauthors.append(line)
-                while lines and not lines[-1]:
-                    lines.pop()
-                continue
-
-            lines.append(line.strip())
-
-        m = re.search(r'( |{repository})#{pr.number}\b'.format(
+        m = self._parse_commit_message(message)
+        pattern = r'( |{repository})#{pr.number}\b'.format(
             pr=self,
             repository=self.repository.name.replace('/', '\\/')
-        ), message)
-        if not m:
-            lines.extend(['', 'closes {pr.repository.name}#{pr.number}'.format(pr=self)])
-        if self.reviewed_by:
-            lines.extend(['', 'Signed-off-by: {}'.format(self.reviewed_by.formatted_email)])
+        )
+        if not re.search(pattern, m.body):
+            m.body += '\n\ncloses {pr.repository.name}#{pr.number}'.format(pr=self)
 
-        if coauthors:
-            lines.extend(['', ''])
-            lines.extend(coauthors)
-        return '\n'.join(lines)
+        if self.reviewed_by:
+            m.headers.add('signed-off-by', self.reviewed_by.formatted_email)
+
+        return str(m)
 
     def _stage(self, gh, target):
         # nb: pr_commits is oldest to newest so pr.head is pr_commits[-1]
@@ -1684,3 +1679,64 @@ def parse_refs_smart(read):
             break # empty list (no refs)
         m = refline.match(line)
         yield m[1].decode(), m[2].decode()
+
+HEADER = re.compile('^([A-Za-z-]+): (.*)$')
+class Message:
+    @classmethod
+    def from_message(cls, msg):
+        in_headers = True
+        headers = []
+        body = []
+        for line in reversed(msg.splitlines()):
+            if not line:
+                if not in_headers and body and body[-1]:
+                    body.append(line)
+                continue
+
+            h = HEADER.match(line)
+            if h:
+                # c-a-b = special case from an existing test, not sure if actually useful?
+                if in_headers or h.group(1).lower() == 'co-authored-by':
+                    headers.append(h.groups())
+                    continue
+
+            body.append(line)
+            in_headers = False
+
+        return cls('\n'.join(reversed(body)), Headers(reversed(headers)))
+
+    def __init__(self, body, headers=None):
+        self.body = body
+        self.headers = headers or Headers()
+
+    def __setattr__(self, name, value):
+        # make sure stored body is always stripped
+        if name == 'body':
+            value = value and value.strip()
+        super().__setattr__(name, value)
+
+    def __str__(self):
+        if not self.headers:
+            return self.body + '\n'
+
+        with io.StringIO(self.body) as msg:
+            msg.write(self.body)
+            msg.write('\n\n')
+            # https://git.wiki.kernel.org/index.php/CommitMessageConventions
+            # seems to mostly use capitalised names (rather than title-cased)
+            keys = list(OrderedSet(k.capitalize() for k in self.headers.keys()))
+            # c-a-b must be at the very end otherwise github doesn't see it
+            keys.sort(key=lambda k: k == 'Co-authored-by')
+            for k in keys:
+                for v in self.headers.getlist(k):
+                    msg.write(k)
+                    msg.write(': ')
+                    msg.write(v)
+                    msg.write('\n')
+
+            return msg.getvalue()
+
+    def sub(self, pattern, repl, *, flags):
+        """ Performs in-place replacements on the body
+        """
+        self.body = re.sub(pattern, repl, self.body, flags=flags)

runbot_merge/tests/fake_github/__init__.py

@@ -733,7 +733,7 @@ class Commit(object):
     __slots__ = ['tree', 'message', 'author', 'committer', 'parents', 'statuses']
     def __init__(self, tree, message, author, committer, parents):
         self.tree = tree
-        self.message = message
+        self.message = message.strip()
         self.author = Author.from_(author) or Author('', '', '')
         self.committer = Author.from_(committer) or self.author
         self.parents = parents

runbot_merge/tests/test_basic.py

@@ -226,7 +226,12 @@ class TestCommitMessage:
         """
         c1 = repo.make_commit(None, 'first!', None, tree={'f': 'm1'})
         repo.make_ref('heads/master', c1)
-        c2 = repo.make_commit(c1, 'simple commit message\n\n\nCo-authored-by: Bob <bob@example.com>\n\nFixes a thing', None, tree={'f': 'm2'})
+        c2 = repo.make_commit(c1, '''simple commit message
+
+
+Co-authored-by: Bob <bob@example.com>
+
+Fixes a thing''', None, tree={'f': 'm2'})
 
         prx = repo.make_pr('title', 'body', target='master', ctid=c2, user='user')
         repo.post_status(prx.head, 'success', 'ci/runbot')
@@ -240,10 +245,17 @@ class TestCommitMessage:
         run_crons(env)
 
         master = repo.commit('heads/master')
-        assert master.message == "simple commit message\n\nFixes a thing\n\ncloses {repo.name}#1"\
-                                 "\n\nSigned-off-by: {reviewer.formatted_email}"\
-                                 "\n\n\nCo-authored-by: Bob <bob@example.com>"\
-                                 .format(repo=repo, reviewer=get_partner(env, users['reviewer']))
+        assert master.message == """simple commit message
+
+Fixes a thing
+
+closes {repo.name}#1
+
+Signed-off-by: {reviewer.formatted_email}
+Co-authored-by: Bob <bob@example.com>""".format(
+            repo=repo,
+            reviewer=get_partner(env, users['reviewer'])
+        )
 
 class TestWebhookSecurity:
     def test_no_secret(self, env, project, repo):