From 1b1aa637fead9013fd112a6fe4f5d3c3ca17fdbd Mon Sep 17 00:00:00 2001 From: Xavier Morel Date: Fri, 23 Aug 2019 16:16:30 +0200 Subject: [PATCH] [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 | 104 ++++++++++++++++----- runbot_merge/tests/fake_github/__init__.py | 2 +- runbot_merge/tests/test_basic.py | 22 ++++- 3 files changed, 98 insertions(+), 30 deletions(-) diff --git a/runbot_merge/models/pull_requests.py b/runbot_merge/models/pull_requests.py index 9f1f082a..96f4c7bd 100644 --- a/runbot_merge/models/pull_requests.py +++ b/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) diff --git a/runbot_merge/tests/fake_github/__init__.py b/runbot_merge/tests/fake_github/__init__.py index b31c5660..cba86221 100644 --- a/runbot_merge/tests/fake_github/__init__.py +++ b/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 diff --git a/runbot_merge/tests/test_basic.py b/runbot_merge/tests/test_basic.py index ef6dc4b8..7229f3be 100644 --- a/runbot_merge/tests/test_basic.py +++ b/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 \n\nFixes a thing', None, tree={'f': 'm2'}) + c2 = repo.make_commit(c1, '''simple commit message + + +Co-authored-by: Bob + +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 "\ - .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 """.format( + repo=repo, + reviewer=get_partner(env, users['reviewer']) + ) class TestWebhookSecurity: def test_no_secret(self, env, project, repo):