From 5cf3617eef9ce848d19d8d06c22e47586889df36 Mon Sep 17 00:00:00 2001 From: Xavier Morel Date: Tue, 12 Jan 2021 12:24:34 +0100 Subject: [PATCH] [IMP] runbot_merge: split out content after separator when creating commit Currently when creating *merges* the commit message is the concatenation of the PR title and the PR body. However it can be convenient to include description test which would not be included in the commit message e.g. PR template stuff. "Thematic breaks" seem like a good way to do this: the commit message will only include the content preceding the first thematic break, everything else is ignored (except headings which are not ignored, double negations FTW). Might be that that's a bit excessive and we should only ignore what follows the *last* thematic break. Or that there needs to be a more specific marker. We'll see. Fixes #443. --- runbot_merge/models/pull_requests.py | 43 ++++++++++++++ runbot_merge/tests/test_basic.py | 84 ++++++++++++++++++++++++++++ 2 files changed, 127 insertions(+) diff --git a/runbot_merge/models/pull_requests.py b/runbot_merge/models/pull_requests.py index 089bf5c5..9449b028 100644 --- a/runbot_merge/models/pull_requests.py +++ b/runbot_merge/models/pull_requests.py @@ -2041,19 +2041,62 @@ def parse_refs_smart(read): m = refline.match(line) yield m[1].decode(), m[2].decode() +BREAK = re.compile(r''' + ^ + [ ]{0,3} # 0-3 spaces of indentation + # followed by a sequence of three or more matching -, _, or * characters, + # each followed optionally by any number of spaces or tabs + # so needs to start with a _, - or *, then have at least 2 more such + # interspersed with any number of spaces or tabs + ([*_-]) + ([ \t]*\1){2,} + [ \t]* + $ +''', flags=re.VERBOSE) +SETEX_UNDERLINE = re.compile(r''' + ^ + [ ]{0,3} # no more than 3 spaces indentation + [-=]+ # a sequence of = characters or a sequence of - characters + [ ]* # any number of trailing spaces + $ + # we don't care about "a line containing a single -" because we want to + # disambiguate SETEX headings from thematic breaks, and thematic breaks have + # 3+ -. Doesn't look like GH interprets `- - -` as a line so yay... +''', flags=re.VERBOSE) HEADER = re.compile('^([A-Za-z-]+): (.*)$') class Message: @classmethod def from_message(cls, msg): in_headers = True + maybe_setex = None headers = [] body = [] for line in reversed(msg.splitlines()): + if maybe_setex: + # NOTE: actually slightly more complicated: it's a SETEX heading + # only if preceding line(s) can be interpreted as a + # paragraph so e.g. a title followed by a line of dashes + # would indeed be a break, but this should be good enough + # for now, if we need more we'll need a full-blown + # markdown parser probably + if line: # actually a SETEX title -> add underline to body then process current + body.append(maybe_setex) + else: # actually break, remove body then process current + body = [] + maybe_setex = None + if not line: if not in_headers and body and body[-1]: body.append(line) continue + if BREAK.match(line): + if SETEX_UNDERLINE.match(line): + maybe_setex = line + else: + body = [] + continue + h = HEADER.match(line) if h: # c-a-b = special case from an existing test, not sure if actually useful? diff --git a/runbot_merge/tests/test_basic.py b/runbot_merge/tests/test_basic.py index 18497d8f..f53be763 100644 --- a/runbot_merge/tests/test_basic.py +++ b/runbot_merge/tests/test_basic.py @@ -2,6 +2,7 @@ import datetime import itertools import json import re +import textwrap from unittest import mock import pytest @@ -1632,6 +1633,89 @@ class TestMergeMethod: '\n\nSigned-off-by: {}'.format(repo.name, prx.number, reviewer), m, c0) assert log_to_node(repo.log('heads/master')), expected + @pytest.mark.parametrize('separator', [ + '***', '___', '\n---', + '*'*12, '\n----------------', + '- - -', ' ** ** **' + ]) + def test_pr_message_break(self, repo, env, users, config, separator): + """ If the PR message contains a "thematic break", only the part before + should be included in the merge commit's message. + """ + reviewer = get_partner(env, users["reviewer"]).formatted_email + with repo: + root = repo.make_commits(None, Commit("root", tree={'a': 'a'}), ref='heads/master') + + repo.make_commits(root, Commit('C', tree={'a': 'b'}), ref=f'heads/change') + pr = repo.make_pr(title="title", body=f'first\n{separator}\nsecond', + target='master', head=f'change') + repo.post_status(pr.head, 'success', 'legal/cla') + repo.post_status(pr.head, 'success', 'ci/runbot') + pr.post_comment('hansen r+ merge', config['role_reviewer']['token']) + env.run_crons() + + with repo: + repo.post_status('heads/staging.master', 'success', 'ci/runbot') + repo.post_status('heads/staging.master', 'success', 'legal/cla') + env.run_crons() + + head = repo.commit('heads/master') + assert head.message == textwrap.dedent(f"""\ + title + + first + + closes {repo.name}#{pr.number} + + Signed-off-by: {reviewer} + """).strip(), "should not contain the content which follows the thematic break" + + def test_pr_message_setex_title(self, repo, env, users, config): + """ should not break on a proper SETEX-style title """ + reviewer = get_partner(env, users["reviewer"]).formatted_email + with repo: + root = repo.make_commits(None, Commit("root", tree={'a': 'a'}), ref='heads/master') + + repo.make_commits(root, Commit('C', tree={'a': 'b'}), ref=f'heads/change') + pr = repo.make_pr(title="title", body="""\ +Title +--- +This is some text + +Title 2 +------- +This is more text +*** +removed +""", + target='master', head=f'change') + repo.post_status(pr.head, 'success', 'legal/cla') + repo.post_status(pr.head, 'success', 'ci/runbot') + pr.post_comment('hansen r+ merge', config['role_reviewer']['token']) + env.run_crons() + + with repo: + repo.post_status('heads/staging.master', 'success', 'ci/runbot') + repo.post_status('heads/staging.master', 'success', 'legal/cla') + env.run_crons() + + head = repo.commit('heads/master') + assert head.message == textwrap.dedent(f"""\ + title + + Title + --- + This is some text + + Title 2 + ------- + This is more text + + closes {repo.name}#{pr.number} + + Signed-off-by: {reviewer} + """).strip(), "should not break the SETEX titles" + def test_pr_mergehead(self, repo, env, config): """ if the head of the PR is a merge commit and one of the parents is in the target, replicate the merge commit instead of merging