From 6655e0ea5ba699bbe2d56a0d9951f91a9c1ad48d Mon Sep 17 00:00:00 2001 From: xmo-odoo Date: Mon, 26 Nov 2018 10:28:13 +0100 Subject: [PATCH] [ADD] runbot_merge: better integration controls Closes #48 --- runbot_merge/models/pull_requests.py | 241 +++++++++++++++------------ runbot_merge/tests/test_basic.py | 191 +++++++++++++++++---- runbot_merge/tests/test_multirepo.py | 3 +- 3 files changed, 297 insertions(+), 138 deletions(-) diff --git a/runbot_merge/models/pull_requests.py b/runbot_merge/models/pull_requests.py index 57f3f20a..efe18265 100644 --- a/runbot_merge/models/pull_requests.py +++ b/runbot_merge/models/pull_requests.py @@ -286,8 +286,13 @@ class Branch(models.Model): THEN pr.id::text ELSE pr.label END - HAVING (bool_or(pr.priority = 0) AND NOT bool_or(pr.state = 'error')) - OR bool_and(pr.state = 'ready') + HAVING + -- all PRs in a group need to specify their merge method + bool_and(pr.squash or pr.merge_method IS NOT NULL) + AND ( + (bool_or(pr.priority = 0) AND NOT bool_or(pr.state = 'error')) + OR bool_and(pr.state = 'ready') + ) ORDER BY min(pr.priority), min(pr.id) """, [self.id]) # result: [(priority, [(repo_id, pr_id) for repo in repos] @@ -401,7 +406,12 @@ class PullRequests(models.Model): ) message = fields.Text(required=True) squash = fields.Boolean(default=False) - rebase = fields.Boolean(default=True) + merge_method = fields.Selection([ + ('merge', "merge directly, using the PR as merge commit message"), + ('rebase-merge', "rebase and merge, using the PR as merge commit message"), + ('rebase-ff', "rebase and fast-forward"), + ], default=False) + method_warned = fields.Boolean(default=False) delegates = fields.Many2many('res.partner', help="Delegate reviewers, not intrinsically reviewers but can review this PR") priority = fields.Selection([ @@ -463,33 +473,31 @@ class PullRequests(models.Model): }) def _parse_command(self, commandstring): - m = re.match(r'(\w+)(?:([+-])|=(.*))?', commandstring) - if not m: - return None - - name, flag, param = m.groups() - if name == 'retry': - return ('retry', None) - elif name in ('r', 'review'): - if flag == '+': - return ('review', True) - elif flag == '-': - return ('review', False) - elif name == 'delegate': - if flag == '+': - return ('delegate', True) - elif param: - return ('delegate', [ - p.lstrip('#@') - for p in param.split(',') - ]) - elif name in ('p', 'priority'): - if param in ('0', '1', '2'): - return ('priority', int(param)) - elif name == 'rebase': - return ('rebase', flag != '-') - - return None + for m in re.finditer( + r'(\S+?)(?:([+-])|=(\S*))?(?:\s|$)', + commandstring, + ): + name, flag, param = m.groups() + if name == 'retry': + yield ('retry', None) + elif name in ('r', 'review'): + if flag == '+': + yield ('review', True) + elif flag == '-': + yield ('review', False) + elif name == 'delegate': + if flag == '+': + yield ('delegate', True) + elif param: + yield ('delegate', [ + p.lstrip('#@') + for p in param.split(',') + ]) + elif name in ('p', 'priority'): + if param in ('0', '1', '2'): + yield ('priority', int(param)) + elif any(name == k for k, _ in type(self).merge_method.selection): + yield ('method', name) def _parse_commands(self, author, comment, login): """Parses a command string prefixed by Project::github_prefix. @@ -523,9 +531,7 @@ class PullRequests(models.Model): commands = dict( ps for m in self.repository.project_id._find_commands(comment) - for c in m.strip().split() - for ps in [self._parse_command(c)] - if ps is not None + for ps in self._parse_command(m) ) if not commands: @@ -613,10 +619,10 @@ class PullRequests(models.Model): self.repository.name, self.number, author.github_login, self.target.name, ) - elif command == 'rebase': - # anyone can rebase- their PR I guess? - self.rebase = param - ok = True + elif command == 'method': + if is_admin: + self.merge_method = param + ok = True _logger.info( "%s %s(%s) on %s:%s by %s (%s)", @@ -772,6 +778,95 @@ class PullRequests(models.Model): if commit: self.env.cr.commit() + # send feedback for multi-commit PRs without a merge_method (which + # we've not warned yet) + for r in self.search([ + ('state', '=', 'ready'), + ('squash', '=', False), + ('merge_method', '=', False), + ('method_warned', '=', False), + ]): + self.env['runbot_merge.pull_requests.feedback'].create({ + 'repository': r.repository.id, + 'pull_request': r.number, + 'message': "Because this PR has multiple commits, I need to know how to merge it:\n\n" + ''.join( + '* `%s` to %s\n' % pair + for pair in type(self).merge_method.selection + ) + }) + r.method_warned = True + if commit: + self.env.cr.commit() + + def _build_merge_message(self, message): + m = re.search(r'( |{repository})#{pr.number}\b'.format( + pr=self, + repository=self.repository.name.replace('/', '\\/') + ), message) + if m: + return message + return message + '\n\ncloses {pr.repository.name}#{pr.number}'.format(pr=self) + + def _stage(self, gh, target): + # nb: pr_commits is oldest to newest so pr.head is pr_commits[-1] + _, prdict = gh.pr(self.number) + commits = prdict['commits'] + method = self.merge_method or ('rebase-ff' if commits == 1 else None) + assert commits < 50 or not method.startswith('rebase'), \ + "rebasing a PR or more than 50 commits is a tad excessive" + assert commits < 250, "merging PRs of 250+ commits is not supported (https://developer.github.com/v3/pulls/#list-commits-on-a-pull-request)" + pr_commits = gh.commits(self.number) + + # NOTE: lost merge v merge/copy distinction (head being + # a merge commit reused instead of being re-merged) + return method, getattr(self, '_stage_' + method.replace('-', '_'))(gh, target, pr_commits) + + def _stage_rebase_ff(self, gh, target, commits): + # updates head commit with PR number (if necessary) then rebases + # on top of target + msg = self._build_merge_message(commits[-1]['commit']['message']) + commits[-1]['commit']['message'] = msg + return gh.rebase(self.number, target, commits=commits) + + def _stage_rebase_merge(self, gh, target, commits): + msg = self._build_merge_message(self.message) + h = gh.rebase(self.number, target, reset=True, commits=commits) + return gh.merge(h, target, msg)['sha'] + + def _stage_merge(self, gh, target, commits): + pr_head = commits[-1] # oldest to newest + base_commit = None + head_parents = {p['sha'] for p in pr_head['parents']} + if len(head_parents) > 1: + # look for parent(s?) of pr_head not in PR, means it's + # from target (so we merged target in pr) + merge = head_parents - {c['sha'] for c in commits} + assert len(merge) <= 1, \ + ">1 parent from base in PR's head is not supported" + if len(merge) == 1: + [base_commit] = merge + + if base_commit: + # replicate pr_head with base_commit replaced by + # the current head + original_head = gh.head(target) + merge_tree = gh.merge(pr_head['sha'], target, 'temp merge')['tree']['sha'] + new_parents = [original_head] + list(head_parents - {base_commit}) + msg = self._build_merge_message(pr_head['commit']['message']) + copy = gh('post', 'git/commits', json={ + 'message': msg, + 'tree': merge_tree, + 'author': pr_head['commit']['author'], + 'committer': pr_head['commit']['committer'], + 'parents': new_parents, + }).json() + gh.set_ref(target, copy['sha']) + return copy['sha'] + else: + # otherwise do a regular merge + msg = self._build_merge_message(self.message) + return gh.merge(self.head, target, msg)['sha'] + # state changes on reviews RPLUS = { 'opened': 'approved', @@ -1194,15 +1289,6 @@ class Batch(models.Model): :return: () or Batch object (if all prs successfully staged) """ - - def build_message(message, pr): - m = re.search(r'( |{repository})#{pr.number}\b'.format( - pr=pr, repository=pr.repository.name.replace('/', '\\/')), - message) - if m: - return message - return message + '\n\ncloses {pr.repository.name}#{pr.number}'.format(pr=pr) - new_heads = {} for pr in prs: gh = meta[pr.repository]['gh'] @@ -1215,65 +1301,12 @@ class Batch(models.Model): target = 'tmp.{}'.format(pr.target.name) original_head = gh.head(target) try: - # nb: pr_commits is oldest to newest so pr.head is pr_commits[-1] - _, prdict = gh.pr(pr.number) - commits = prdict['commits'] - assert commits < 50 or not pr.rebase, \ - "rebasing a PR or more than 50 commits is a tad excessive" - assert commits < 250, "merging PRs of 250+ commits is not supported (https://developer.github.com/v3/pulls/#list-commits-on-a-pull-request)" - pr_commits = gh.commits(pr.number) - rebase_and_merge = pr.rebase - squash = rebase_and_merge and commits == 1 - if squash: - method = 'squash' - msg = build_message(pr_commits[0]['commit']['message'], pr) - pr_commits[0]['commit']['message'] = msg - new_heads[pr] = gh.rebase(pr.number, target, commits=pr_commits) - elif rebase_and_merge: - method = 'rebase & merge' - msg = build_message(pr.message, pr) - h = gh.rebase(pr.number, target, reset=True, commits=pr_commits) - new_heads[pr] = gh.merge(h, target, msg)['sha'] - else: - pr_head = pr_commits[-1] # pr_commits is oldest to newest - base_commit = None - head_parents = {p['sha'] for p in pr_head['parents']} - if len(head_parents) > 1: - # look for parent(s?) of pr_head not in PR, means it's - # from target (so we merged target in pr) - merge = head_parents - {c['sha'] for c in pr_commits} - assert len(merge) <= 1, \ - ">1 parent from base in PR's head is not supported" - if len(merge) == 1: - [base_commit] = merge - - if base_commit: - method = 'merge/copy' - # replicate pr_head with base_commit replaced by - # the current head - original_head = gh.head(target) - merge_tree = gh.merge(pr_head['sha'], target, 'temp merge')['tree']['sha'] - new_parents = [original_head] + list(head_parents - {base_commit}) - msg = build_message(pr_head['commit']['message'], pr) - copy = gh('post', 'git/commits', json={ - 'message': msg, - 'tree': merge_tree, - 'author': pr_head['commit']['author'], - 'committer': pr_head['commit']['committer'], - 'parents': new_parents, - }).json() - gh.set_ref(target, copy['sha']) - new_heads[pr] = copy['sha'] - else: - method = 'merge' - # otherwise do a regular merge - msg = build_message(pr.message, pr) - new_heads[pr] = gh.merge(pr.head, target, msg)['sha'] - new_head = gh.head(target) + method, new_heads[pr] = pr._stage(gh, target) _logger.info( - "Staged pr %s:%s by %s to %s; %s %s -> %s", - pr.repository.name, pr.number, method, new_heads[pr], - target, original_head, new_head + "Staged pr %s:%s to %s by %s: %s -> %s", + pr.repository.name, pr.number, + pr.target.name, method, + original_head, new_heads[pr] ) except (exceptions.MergeError, AssertionError) as e: _logger.exception("Failed to merge %s:%s into staging branch (error: %s)", pr.repository.name, pr.number, e) diff --git a/runbot_merge/tests/test_basic.py b/runbot_merge/tests/test_basic.py index ac3684af..ad43598b 100644 --- a/runbot_merge/tests/test_basic.py +++ b/runbot_merge/tests/test_basic.py @@ -47,7 +47,7 @@ def test_trivial_flow(env, repo, page): env['runbot_merge.project']._send_feedback() assert pr1.labels == {'seen 🙂', 'CI 🤖'} - pr1.post_comment('hansen r+', 'reviewer') + pr1.post_comment('hansen r+ rebase-merge', 'reviewer') assert pr.state == 'ready' # can't check labels here as running the cron will stage it @@ -240,7 +240,7 @@ def test_staging_conflict(env, repo): pr1 = repo.make_pr("gibberish", "blahblah", target='master', ctid=c1, user='user') repo.post_status(c1, 'success', 'legal/cla') repo.post_status(c1, 'success', 'ci/runbot') - pr1.post_comment("hansen r+", "reviewer") + pr1.post_comment("hansen r+ rebase-merge", "reviewer") env['runbot_merge.project']._check_progress() pr1 = env['runbot_merge.pull_requests'].search([ ('repository.name', '=', repo.name), @@ -254,7 +254,7 @@ def test_staging_conflict(env, repo): pr2 = repo.make_pr('gibberish', 'blahblah', target='master', ctid=c3, user='user') repo.post_status(c3, 'success', 'legal/cla') repo.post_status(c3, 'success', 'ci/runbot') - pr2.post_comment('hansen r+', "reviewer") + pr2.post_comment('hansen r+ rebase-merge', "reviewer") env['runbot_merge.project']._check_progress() env['runbot_merge.project']._send_feedback() p_2 = env['runbot_merge.pull_requests'].search([ @@ -292,14 +292,14 @@ def test_staging_concurrent(env, repo): pr1 = repo.make_pr('t1', 'b1', target='1.0', ctid=c11, user='user') repo.post_status(pr1.head, 'success', 'ci/runbot') repo.post_status(pr1.head, 'success', 'legal/cla') - pr1.post_comment('hansen r+', "reviewer") + pr1.post_comment('hansen r+ rebase-merge', "reviewer") c20 = repo.make_commit(m, 'CCC', None, tree={'m': 'm', 'c': 'c'}) c21 = repo.make_commit(c20, 'DDD', None, tree={'m': 'm', 'c': 'c', 'd': 'd'}) pr2 = repo.make_pr('t2', 'b2', target='2.0', ctid=c21, user='user') repo.post_status(pr2.head, 'success', 'ci/runbot') repo.post_status(pr2.head, 'success', 'legal/cla') - pr2.post_comment('hansen r+', "reviewer") + pr2.post_comment('hansen r+ rebase-merge', "reviewer") env['runbot_merge.project']._check_progress() pr1 = env['runbot_merge.pull_requests'].search([ @@ -325,7 +325,7 @@ def test_staging_merge_fail(env, repo, users): prx = repo.make_pr('title', 'body', target='master', ctid=c2, user='user') repo.post_status(prx.head, 'success', 'ci/runbot') repo.post_status(prx.head, 'success', 'legal/cla') - prx.post_comment('hansen r+', "reviewer") + prx.post_comment('hansen r+ rebase-merge', "reviewer") env['runbot_merge.project']._check_progress() env['runbot_merge.project']._send_feedback() @@ -336,7 +336,7 @@ def test_staging_merge_fail(env, repo, users): assert pr1.state == 'error' assert prx.labels == {'seen 🙂', 'error 🙅'} assert prx.comments == [ - (users['reviewer'], 'hansen r+'), + (users['reviewer'], 'hansen r+ rebase-merge'), (users['user'], re_matches('^Unable to stage PR')), ] @@ -352,7 +352,7 @@ def test_staging_ci_timeout(env, repo, users): prx = repo.make_pr('title', 'body', target='master', ctid=c2, user='user') repo.post_status(prx.head, 'success', 'ci/runbot') repo.post_status(prx.head, 'success', 'legal/cla') - prx.post_comment('hansen r+', "reviewer") + prx.post_comment('hansen r+ rebase-merge', "reviewer") env['runbot_merge.project']._check_progress() pr1 = env['runbot_merge.pull_requests'].search([ @@ -377,7 +377,7 @@ def test_staging_ci_failure_single(env, repo, users): prx = repo.make_pr('title', 'body', target='master', ctid=c2, user='user') repo.post_status(prx.head, 'success', 'ci/runbot') repo.post_status(prx.head, 'success', 'legal/cla') - prx.post_comment('hansen r+', "reviewer") + prx.post_comment('hansen r+ rebase-merge', "reviewer") env['runbot_merge.project']._check_progress() assert env['runbot_merge.pull_requests'].search([ ('repository.name', '=', repo.name), @@ -394,7 +394,7 @@ def test_staging_ci_failure_single(env, repo, users): ]).state == 'error' assert prx.comments == [ - (users['reviewer'], 'hansen r+'), + (users['reviewer'], 'hansen r+ rebase-merge'), (users['user'], 'Staging failed: ci/runbot') ] @@ -408,7 +408,7 @@ def test_ff_failure(env, repo, users): prx = repo.make_pr('title', 'body', target='master', ctid=c2, user='user') repo.post_status(prx.head, 'success', 'legal/cla') repo.post_status(prx.head, 'success', 'ci/runbot') - prx.post_comment('hansen r+', "reviewer") + prx.post_comment('hansen r+ rebase-merge', "reviewer") env['runbot_merge.project']._check_progress() assert env['runbot_merge.pull_requests'].search([ ('repository.name', '=', repo.name), @@ -440,21 +440,21 @@ def test_ff_failure_batch(env, repo, users): A = repo.make_pr('A', None, target='master', ctid=a2, user='user', label='A') repo.post_status(A.head, 'success', 'legal/cla') repo.post_status(A.head, 'success', 'ci/runbot') - A.post_comment('hansen r+', "reviewer") + A.post_comment('hansen r+ rebase-merge', "reviewer") b1 = repo.make_commit(m, 'b1', None, tree={'m': 'm', 'b': '1'}) b2 = repo.make_commit(b1, 'b2', None, tree={'m': 'm', 'b': '2'}) B = repo.make_pr('B', None, target='master', ctid=b2, user='user', label='B') repo.post_status(B.head, 'success', 'legal/cla') repo.post_status(B.head, 'success', 'ci/runbot') - B.post_comment('hansen r+', "reviewer") + B.post_comment('hansen r+ rebase-merge', "reviewer") c1 = repo.make_commit(m, 'c1', None, tree={'m': 'm', 'c': '1'}) c2 = repo.make_commit(c1, 'c2', None, tree={'m': 'm', 'c': '2'}) C = repo.make_pr('C', None, target='master', ctid=c2, user='user', label='C') repo.post_status(C.head, 'success', 'legal/cla') repo.post_status(C.head, 'success', 'ci/runbot') - C.post_comment('hansen r+', "reviewer") + C.post_comment('hansen r+ rebase-merge', "reviewer") env['runbot_merge.project']._check_progress() messages = [ @@ -586,7 +586,7 @@ def test_forward_port(env, repo): pr = repo.make_pr('PR', None, target='master', ctid=head, user='user') repo.post_status(pr.head, 'success', 'legal/cla') repo.post_status(pr.head, 'success', 'ci/runbot') - pr.post_comment('hansen rebase- r+', "reviewer") + pr.post_comment('hansen r+ merge', "reviewer") env['runbot_merge.project']._check_progress() st = repo.commit('heads/staging.master') @@ -721,7 +721,7 @@ class TestRetry: prx = repo.make_pr('title', 'body', target='master', ctid=c2, user='user') repo.post_status(prx.head, 'success', 'ci/runbot') repo.post_status(prx.head, 'success', 'legal/cla') - prx.post_comment('hansen r+ delegate=%s' % users['other'], "reviewer") + prx.post_comment('hansen r+ delegate=%s rebase-merge' % users['other'], "reviewer") env['runbot_merge.project']._check_progress() assert env['runbot_merge.pull_requests'].search([ ('repository.name', '=', repo.name), @@ -783,7 +783,7 @@ class TestRetry: prx = repo.make_pr('title', 'body', target='master', ctid=c2, user='user') repo.post_status(prx.head, 'success', 'ci/runbot') repo.post_status(prx.head, 'success', 'legal/cla') - prx.post_comment('hansen r+ delegate=%s' % users['other'], "reviewer") + prx.post_comment('hansen r+ delegate=%s rebase-merge' % users['other'], "reviewer") env['runbot_merge.project']._check_progress() assert env['runbot_merge.pull_requests'].search([ ('repository.name', '=', repo.name), @@ -813,7 +813,9 @@ class TestMergeMethod: if event['pull_request']['commits'] == 1, "squash" (/rebase); otherwise regular merge """ - def test_staging_merge_squash(self, repo, env): + def test_pr_single_commit(self, repo, env): + """ If single commit, default to rebase & FF + """ m = repo.make_commit(None, 'initial', None, tree={'m': 'm'}) m2 = repo.make_commit(m, 'second', None, tree={'m': 'm', 'm2': 'm2'}) repo.make_ref('heads/master', m2) @@ -856,7 +858,7 @@ class TestMergeMethod: ]).state == 'merged' assert prx.state == 'closed' - def test_pr_update_unsquash(self, repo, env): + def test_pr_update_to_many_commits(self, repo, env): """ If a PR starts with 1 commit and a second commit is added, the PR should be unflagged as squash @@ -876,7 +878,7 @@ class TestMergeMethod: prx.push(repo.make_commit(c1, 'second2', None, tree={'m': 'c2'})) assert not pr.squash, "a PR with a single commit should not be squashed" - def test_pr_reset_squash(self, repo, env): + def test_pr_reset_to_single_commit(self, repo, env): """ If a PR starts at >1 commits and is reset back to 1, the PR should be re-flagged as squash @@ -897,8 +899,67 @@ class TestMergeMethod: prx.push(repo.make_commit(m, 'fixup', None, tree={'m': 'c2'})) assert pr.squash, "a PR with a single commit should be squashed" + def test_pr_no_method(self, repo, env, users): + """ a multi-repo PR should not be staged by default, should also get + feedback indicating a merge method is necessary + """ + m0 = repo.make_commit(None, 'M0', None, tree={'m': '0'}) + m1 = repo.make_commit(m0, 'M1', None, tree={'m': '1'}) + m2 = repo.make_commit(m1, 'M2', None, tree={'m': '2'}) + repo.make_ref('heads/master', m2) + + b0 = repo.make_commit(m1, 'B0', None, tree={'m': '1', 'b': '0'}) + b1 = repo.make_commit(b0, 'B1', None, tree={'m': '1', 'b': '1'}) + prx = repo.make_pr('title', 'body', target='master', ctid=b1, user='user') + repo.post_status(prx.head, 'success', 'legal/cla') + repo.post_status(prx.head, 'success', 'ci/runbot') + prx.post_comment('hansen r+', "reviewer") + + run_crons(env) + assert not env['runbot_merge.pull_requests'].search([ + ('repository.name', '=', repo.name), + ('number', '=', prx.number), + ]).staging_id + + assert prx.comments == [ + (users['reviewer'], 'hansen r+'), + (users['user'], """Because this PR has multiple commits, I need to know how to merge it: + +* `merge` to merge directly, using the PR as merge commit message +* `rebase-merge` to rebase and merge, using the PR as merge commit message +* `rebase-ff` to rebase and fast-forward +"""), + ] + + def test_pr_method_no_review(self, repo, env): + """ Configuring the method should be idependent from the review + """ + m0 = repo.make_commit(None, 'M0', None, tree={'m': '0'}) + m1 = repo.make_commit(m0, 'M1', None, tree={'m': '1'}) + m2 = repo.make_commit(m1, 'M2', None, tree={'m': '2'}) + repo.make_ref('heads/master', m2) + + b0 = repo.make_commit(m1, 'B0', None, tree={'m': '1', 'b': '0'}) + b1 = repo.make_commit(b0, 'B1', None, tree={'m': '1', 'b': '1'}) + prx = repo.make_pr('title', 'body', target='master', ctid=b1, user='user') + pr = env['runbot_merge.pull_requests'].search([ + ('repository.name', '=', repo.name), + ('number', '=', prx.number), + ]) + repo.post_status(prx.head, 'success', 'legal/cla') + repo.post_status(prx.head, 'success', 'ci/runbot') + + prx.post_comment('hansen rebase-merge', "reviewer") + assert pr.merge_method == 'rebase-merge' + + prx.post_comment('hansen merge', "reviewer") + assert pr.merge_method == 'merge' + + prx.post_comment('hansen rebase-ff', "reviewer") + assert pr.merge_method == 'rebase-ff' + def test_pr_rebase_merge(self, repo, env): - """ a multi-commit PR should be rebased & merged by default + """ test result on rebase-merge left: PR right: post-merge result @@ -940,7 +1001,7 @@ class TestMergeMethod: prx = repo.make_pr('title', 'body', target='master', ctid=b1, user='user') repo.post_status(prx.head, 'success', 'legal/cla') repo.post_status(prx.head, 'success', 'ci/runbot') - prx.post_comment('hansen r+', "reviewer") + prx.post_comment('hansen r+ rebase-merge', "reviewer") env['runbot_merge.project']._check_progress() @@ -971,11 +1032,72 @@ class TestMergeMethod: final_tree = repo.read_tree(repo.commit('heads/master')) assert final_tree == {'m': b'2', 'b': b'1'}, "sanity check of final tree" + def test_pr_rebase_ff(self, repo, env): + """ test result on rebase-merge + + left: PR + right: post-merge result + + +------+ +------+ + | M0 | | M0 | + +--^---+ +--^---+ + | | + | | + +--+---+ +--+---+ + +----> M1 <--+ | M1 <--+ + | +------+ | +------+ | + | | | + | | | + +--+---+ +---+---+ +------+ +---+---+ + | B0 | | M2 | | B0 +------> M2 | + +--^---+ +-------+ +--^---+ +---^---+ + | | + +--+---+ +--+---+ + PR | B1 | | B1 | + +------+ +--^---+ + """ + m0 = repo.make_commit(None, 'M0', None, tree={'m': '0'}) + m1 = repo.make_commit(m0, 'M1', None, tree={'m': '1'}) + m2 = repo.make_commit(m1, 'M2', None, tree={'m': '2'}) + repo.make_ref('heads/master', m2) + + b0 = repo.make_commit(m1, 'B0', None, tree={'m': '1', 'b': '0'}) + b1 = repo.make_commit(b0, 'B1', None, tree={'m': '1', 'b': '1'}) + prx = repo.make_pr('title', 'body', target='master', ctid=b1, user='user') + repo.post_status(prx.head, 'success', 'legal/cla') + repo.post_status(prx.head, 'success', 'ci/runbot') + prx.post_comment('hansen r+ rebase-ff', "reviewer") + + env['runbot_merge.project']._check_progress() + + # create a dag (msg:str, parents:set) from the log + staging = log_to_node(repo.log('heads/staging.master')) + # then compare to the dag version of the right graph + nm2 = node('M2', node('M1', node('M0'))) + nb1 = node('B1\n\ncloses {}#{}'.format(repo.name, prx.number), node('B0', nm2)) + expected = node(re_matches('^force rebuild'), nb1) + assert staging == expected + + repo.post_status('heads/staging.master', 'success', 'legal/cla') + repo.post_status('heads/staging.master', 'success', 'ci/runbot') + env['runbot_merge.project']._check_progress() + + assert env['runbot_merge.pull_requests'].search([ + ('repository.name', '=', repo.name), + ('number', '=', prx.number), + ]).state == 'merged' + + # check that the dummy commit is not in the final master + master = log_to_node(repo.log('heads/master')) + assert master == nb1 + final_tree = repo.read_tree(repo.commit('heads/master')) + assert final_tree == {'m': b'2', 'b': b'1'}, "sanity check of final tree" + @pytest.mark.skip(reason="what do if the PR contains merge commits???") def test_pr_contains_merges(self, repo, env): pass - def test_pr_unrebase(self, repo, env): + def test_pr_force_merge_single_commit(self, repo, env): """ should be possible to flag a PR as regular-merged, regardless of its commits count @@ -995,7 +1117,7 @@ class TestMergeMethod: repo.post_status(prx.head, 'success', 'legal/cla') repo.post_status(prx.head, 'success', 'ci/runbot') - prx.post_comment('hansen r+ rebase-', 'reviewer') + prx.post_comment('hansen r+ merge', 'reviewer') env['runbot_merge.project']._check_progress() repo.post_status('heads/staging.master', 'success', 'ci/runbot') @@ -1024,7 +1146,7 @@ class TestMergeMethod: repo.post_status(prx.head, 'success', 'legal/cla') repo.post_status(prx.head, 'success', 'ci/runbot') - prx.post_comment('hansen r+ rebase-', 'reviewer') + prx.post_comment('hansen r+ merge', 'reviewer') env['runbot_merge.project']._check_progress() repo.post_status('heads/staging.master', 'success', 'ci/runbot') @@ -1063,7 +1185,7 @@ class TestMergeMethod: repo.post_status(prx.head, 'success', 'legal/cla') repo.post_status(prx.head, 'success', 'ci/runbot') - prx.post_comment('hansen r+ rebase-', 'reviewer') + prx.post_comment('hansen r+ merge', 'reviewer') env['runbot_merge.project']._check_progress() repo.post_status('heads/staging.master', 'success', 'ci/runbot') @@ -1103,7 +1225,7 @@ class TestMergeMethod: repo.post_status(prx.head, 'success', 'legal/cla') repo.post_status(prx.head, 'success', 'ci/runbot') - prx.post_comment('hansen r+ rebase-', 'reviewer') + prx.post_comment('hansen r+ merge', 'reviewer') env['runbot_merge.project']._check_progress() repo.post_status('heads/staging.master', 'success', 'ci/runbot') @@ -1406,7 +1528,10 @@ class TestBatching(object): for context, result in statuses: repo.post_status(c, result, context) if reviewer: - pr.post_comment('hansen r+', reviewer) + pr.post_comment( + 'hansen r+%s' % (' rebase-merge' if len(trees) > 1 else ''), + reviewer + ) return pr def _get(self, env, repo, number): @@ -1456,16 +1581,16 @@ class TestBatching(object): repo.make_ref('heads/master', m) pr1 = self._pr(repo, 'PR1', [{'a': 'AAA'}, {'b': 'BBB'}]) - pr1.post_comment('hansen rebase-', 'reviewer') + pr1.post_comment('hansen merge', 'reviewer') pr2 = self._pr(repo, 'PR2', [{'c': 'CCC'}, {'d': 'DDD'}]) - pr2.post_comment('hansen rebase-', 'reviewer') + pr2.post_comment('hansen merge', 'reviewer') env['runbot_merge.project']._check_progress() pr1 = self._get(env, repo, pr1.number) assert pr1.staging_id - assert not pr1.rebase + assert pr1.merge_method == 'merge' pr2 = self._get(env, repo, pr2.number) - assert not pr2.rebase + assert pr2.merge_method == 'merge' assert pr1.staging_id assert pr2.staging_id assert pr1.staging_id == pr2.staging_id @@ -1565,7 +1690,7 @@ class TestBatching(object): # no statuses run on PR0s pr01 = self._pr(repo, 'Urgent1', [{'n': 'n'}, {'o': 'o'}], reviewer=None, statuses=[]) - pr01.post_comment('hansen priority=0', 'reviewer') + pr01.post_comment('hansen priority=0 rebase-merge', 'reviewer') p_01 = self._get(env, repo, pr01.number) assert p_01.state == 'opened' assert p_01.priority == 0 diff --git a/runbot_merge/tests/test_multirepo.py b/runbot_merge/tests/test_multirepo.py index 10e04561..4cb6bf90 100644 --- a/runbot_merge/tests/test_multirepo.py +++ b/runbot_merge/tests/test_multirepo.py @@ -450,7 +450,8 @@ def test_urgent(env, repo_a, repo_b): pr_b = make_pr(repo_b, 'B', [{'b1': 'b'}, {'b2': 'b'}], label='batch', reviewer=None, statuses=[]) pr_c = make_pr(repo_a, 'C', [{'c1': 'c', 'c2': 'c'}]) - pr_b.post_comment('hansen p=0', 'reviewer') + pr_a.post_comment('hansen rebase-merge', 'reviewer') + pr_b.post_comment('hansen rebase-merge p=0', 'reviewer') env['runbot_merge.project']._check_progress() # should have batched pr_a and pr_b despite neither being reviewed or