From a40b4c20da83c079c37d6fe6dc1b2eb0a6437e27 Mon Sep 17 00:00:00 2001 From: Xavier Morel Date: Wed, 29 Aug 2018 16:51:53 +0200 Subject: [PATCH] [ADD] runbot_merge: flag to disable rebase before merge rebase-and-merge (or squash-merge if pr.commits == 1) remains default, but there are use cases like forward ports (merge branch X into branch X+1 so that fixes to X are available in X+1) where we really really don't want to rebase the source. This commits implements two alternative merge methods: If the PR and its target are ~disjoint, perform a straight merge (same as old default mode). However if the head of the PR has two parents *and* one of these parents is a commit of the target, assume this is a merge commit to fix a conflict (common during forward ports as X+1 will have changed independently from and incompatibly with X in some ways). In that case, merge by copying the PR's head atop the target (basically rebase just that commit, only updating the link to the parent which is part of target so that it points to the head of target instead of whatever it was previously). --- runbot_merge/models/pull_requests.py | 54 ++++++++- runbot_merge/tests/fake_github/__init__.py | 35 +++--- runbot_merge/tests/remote.py | 17 ++- runbot_merge/tests/test_basic.py | 127 +++++++++++++++++++-- runbot_merge/tests/test_multirepo.py | 1 + 5 files changed, 191 insertions(+), 43 deletions(-) diff --git a/runbot_merge/models/pull_requests.py b/runbot_merge/models/pull_requests.py index 8f7e0181..a8f10cd4 100644 --- a/runbot_merge/models/pull_requests.py +++ b/runbot_merge/models/pull_requests.py @@ -369,6 +369,7 @@ class PullRequests(models.Model): ) message = fields.Text(required=True) squash = fields.Boolean(default=False) + rebase = fields.Boolean(default=True) delegates = fields.Many2many('res.partner', help="Delegate reviewers, not intrinsically reviewers but can review this PR") priority = fields.Selection([ @@ -440,6 +441,8 @@ class PullRequests(models.Model): elif name in ('p', 'priority'): if param in ('0', '1', '2'): return ('priority', int(param)) + elif name == 'rebase': + return ('rebase', flag != '-') return None @@ -459,6 +462,9 @@ class PullRequests(models.Model): p(riority)=2|1|0 sets the priority to normal (2), pressing (1) or urgent (0). Lower-priority PRs are selected first and batched together. + rebase+/- + Whether the PR should be rebased-and-merged (the default) or just + merged normally. """ assert self, "parsing commands must be executed in an actual PR" @@ -530,6 +536,9 @@ 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 _logger.info( "%s %s(%s) on %s:%s by %s (%s)", @@ -889,19 +898,52 @@ class Batch(models.Model): pr.repository.name, pr.number, pr.target.name, pr.squash ) + target = 'tmp.{}'.format(pr.target.name) + suffix = '\n\ncloses {pr.repository.name}#{pr.number}'.format(pr=pr) try: - # FIXME: !rebase - - target = 'tmp.{}'.format(pr.target.name) - suffix = '\n\ncloses {pr.repository.name}#{pr.number}'.format(pr=pr) + # nb: pr_commits is oldest to newest so pr.head is pr_commits[-1] pr_commits = gh.commits(pr.number) - if len(pr_commits) == 1: + rebase_and_merge = pr.rebase + squash = rebase_and_merge and len(pr_commits) == 1 + if squash: pr_commits[0]['commit']['message'] += suffix new_heads[pr] = gh.rebase(pr.number, target, commits=pr_commits) - else: + elif rebase_and_merge: msg = pr.message + suffix 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: + # 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}) + copy = gh('post', 'git/commits', json={ + 'message': pr_head['commit']['message'] + suffix, + '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: + # otherwise do a regular merge + msg = pr.message + suffix + new_heads[pr] = gh.merge(pr.head, target, msg)['sha'] except (exceptions.MergeError, AssertionError) as e: _logger.exception("Failed to merge %s:%s into staging branch (error: %s)", pr.repository.name, pr.number, e) pr.state = 'error' diff --git a/runbot_merge/tests/fake_github/__init__.py b/runbot_merge/tests/fake_github/__init__.py index 69fcbdfe..2faf54cf 100644 --- a/runbot_merge/tests/fake_github/__init__.py +++ b/runbot_merge/tests/fake_github/__init__.py @@ -138,25 +138,19 @@ class Repo(object): c.statuses.append((status, context, description)) self.notify('status', self.name, context, status, c.id) - def make_commit(self, ref, message, author, committer=None, tree=None, changes=None): - assert (tree is None) ^ (changes is None), \ - "a commit must provide either a full tree or changes to the previous tree" + def make_commit(self, ref, message, author, committer=None, tree=None): + assert tree, "a commit must provide either a full tree" - branch = False - if ref is None: - pids = [] - else: - pid = ref - if not re.match(r'[0-9a-f]{40}', ref): - pid = self.refs[ref] - branch = True - parent = self.objects[pid] - pids = [pid] + refs = ref or [] + if not isinstance(refs, list): + refs = [ref] - if tree is None: - # TODO? - tid = self._update_tree(parent.tree, changes) - elif type(tree) is type(u''): + pids = [ + ref if re.match(r'[0-9a-f]{40}', ref) else self.refs[ref] + for ref in refs + ] + + if type(tree) is type(u''): assert isinstance(self.objects.get(tree), dict) tid = tree else: @@ -164,8 +158,8 @@ class Repo(object): c = Commit(tid, message, author, committer or author, parents=pids) self.objects[c.id] = c - if branch: - self.refs[ref] = c.id + if refs and refs[0] != pids[0]: + self.refs[refs[0]] = c.id return c.id def _save_tree(self, t): @@ -268,11 +262,10 @@ class Repo(object): def _create_commit(self, r): body = json.loads(r.body) - [parent] = body.get('parents') or [None] author = body.get('author') or {'name': 'default', 'email': 'default', 'date': 'Z'} try: sha = self.make_commit( - ref=parent, + ref=(body.get('parents')), message=body['message'], author=author, committer=body.get('committer') or author, diff --git a/runbot_merge/tests/remote.py b/runbot_merge/tests/remote.py index ddc1efbd..9e4db608 100644 --- a/runbot_merge/tests/remote.py +++ b/runbot_merge/tests/remote.py @@ -452,11 +452,11 @@ class Repo: assert 200 <= r.status_code < 300, r.json() wait_for_hook() - def make_commit(self, ref, message, author, committer=None, tree=None, changes=None): + def make_commit(self, ref, message, author, committer=None, tree=None): assert tree, "not supporting changes/updates" assert not (author or committer) - if ref is None: + if not ref: # None / [] # apparently github refuses to create trees/commits in empty repos # using the regular API... [(path, contents)] = tree.items() @@ -469,7 +469,11 @@ class Repo: assert 200 <= r.status_code < 300, r.json() return r.json()['commit']['sha'] - parent = self.get_ref(ref) + if isinstance(ref, list): + refs = ref + else: + refs = [ref] + parents = [self.get_ref(r) for r in refs] r = self._session.post('https://api.github.com/repos/{}/git/trees'.format(self.name), json={ 'tree': [ @@ -481,7 +485,7 @@ class Repo: h = r.json()['sha'] r = self._session.post('https://api.github.com/repos/{}/git/commits'.format(self.name), json={ - 'parents': [parent], + 'parents': parents, 'message': message, 'tree': h, @@ -490,8 +494,9 @@ class Repo: commit_sha = r.json()['sha'] - if parent != ref: - self.update_ref(ref, commit_sha) + # if the first parent is an actual ref (rather than a hash) update it + if parents[0] != refs[0]: + self.update_ref(refs[0], commit_sha) else: wait_for_hook() return commit_sha diff --git a/runbot_merge/tests/test_basic.py b/runbot_merge/tests/test_basic.py index 9bded03a..0e5543cb 100644 --- a/runbot_merge/tests/test_basic.py +++ b/runbot_merge/tests/test_basic.py @@ -638,11 +638,9 @@ class TestMergeMethod: # 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 (nb: parents is - # a frozenset otherwise can't put a node in a node as tuples are - # only hashable if their contents are) - nm2 = ('M2', frozenset([('M1', frozenset([('M0', frozenset())]))])) - nb1 = ('B1', frozenset([('B0', frozenset([nm2]))])) + # then compare to the dag version of the right graph + nm2 = node('M2', node('M1', node('M0'))) + nb1 = node('B1', node('B0', nm2)) expected = ( 'title\n\nbody\n\ncloses {}#{}'.format(repo.name, prx.number), frozenset([nm2, nb1]) @@ -654,19 +652,125 @@ class TestMergeMethod: @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): - """ should be possible to flag a PR as regular-merged + """ should be possible to flag a PR as regular-merged, regardless of + its commits count + + M M<--+ + ^ ^ | + | -> | C0 + + | ^ + C0 + | + gib-+ """ - pytest.skip("todo") + m = repo.make_commit(None, "M", None, tree={'a': 'a'}) + repo.make_ref('heads/master', m) + + c0 = repo.make_commit(m, 'C0', None, tree={'a': 'b'}) + prx = repo.make_pr("gibberish", "blahblah", target='master', ctid=c0, user='user') + env['runbot_merge.project']._check_progress() + + repo.post_status(prx.head, 'success', 'legal/cla') + repo.post_status(prx.head, 'success', 'ci/runbot') + prx.post_comment('hansen r+ rebase-', 'reviewer') + env['runbot_merge.project']._check_progress() + + repo.post_status('heads/staging.master', 'success', 'ci/runbot') + repo.post_status('heads/staging.master', 'success', 'legal/cla') + env['runbot_merge.project']._check_progress() + + master = repo.commit('heads/master') + assert master.parents == [m, prx.head], \ + "master's parents should be the old master & the PR head" + + m = node('M') + c0 = node('C0', m) + expected = node('gibberish\n\nblahblah\n\ncloses {}#{}'.format(repo.name, prx.number), m, c0) + assert log_to_node(repo.log('heads/master')), expected def test_pr_mergehead(self, repo, env): """ 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 + + rankdir="BT" + M2 -> M1 + C0 -> M1 + C1 -> C0 + C1 -> M2 + + C1 [label = "\\N / MERGE"] """ - pytest.skip("todo") + m1 = repo.make_commit(None, "M1", None, tree={'a': '0'}) + m2 = repo.make_commit(m1, "M2", None, tree={'a': '1'}) + repo.make_ref('heads/master', m2) + + c0 = repo.make_commit(m1, 'C0', None, tree={'a': '0', 'b': '2'}) + c1 = repo.make_commit([c0, m2], 'C1', None, tree={'a': '1', 'b': '2'}) + prx = repo.make_pr("T", "TT", target='master', ctid=c1, user='user') + env['runbot_merge.project']._check_progress() + + repo.post_status(prx.head, 'success', 'legal/cla') + repo.post_status(prx.head, 'success', 'ci/runbot') + prx.post_comment('hansen r+ rebase-', 'reviewer') + env['runbot_merge.project']._check_progress() + + repo.post_status('heads/staging.master', 'success', 'ci/runbot') + repo.post_status('heads/staging.master', 'success', 'legal/cla') + env['runbot_merge.project']._check_progress() + + master = repo.commit('heads/master') + assert master.parents == [m2, c0] + m1 = node('M1') + expected = node('C1', node('C0', m1), node('M2', m1)) + assert log_to_node(repo.log('heads/master')), expected + + def test_pr_mergehead_nonmember(self, repo, env): + """ if the head of the PR is a merge commit but none of the parents is + in the target, merge normally + + rankdir="BT" + M2 -> M1 + B0 -> M1 + C0 -> M1 + C1 -> C0 + C1 -> B0 + + MERGE -> M2 + MERGE -> C1 + """ + m1 = repo.make_commit(None, "M1", None, tree={'a': '0'}) + m2 = repo.make_commit(m1, "M2", None, tree={'a': '1'}) + repo.make_ref('heads/master', m2) + + b0 = repo.make_commit(m1, 'B0', None, tree={'a': '0', 'bb': 'bb'}) + + c0 = repo.make_commit(m1, 'C0', None, tree={'a': '0', 'b': '2'}) + c1 = repo.make_commit([c0, b0], 'C1', None, tree={'a': '0', 'b': '2', 'bb': 'bb'}) + prx = repo.make_pr("T", "TT", target='master', ctid=c1, user='user') + env['runbot_merge.project']._check_progress() + + repo.post_status(prx.head, 'success', 'legal/cla') + repo.post_status(prx.head, 'success', 'ci/runbot') + prx.post_comment('hansen r+ rebase-', 'reviewer') + env['runbot_merge.project']._check_progress() + + repo.post_status('heads/staging.master', 'success', 'ci/runbot') + repo.post_status('heads/staging.master', 'success', 'legal/cla') + env['runbot_merge.project']._check_progress() + + master = repo.commit('heads/master') + assert master.parents == [m2, c1] + assert repo.read_tree(master) == {'a': b'1', 'b': b'2', 'bb': b'bb'} + + m1 = node('M1') + expected = node( + 'T\n\nTT\n\ncloses {}#{}'.format(repo.name, prx.number), + node('M2', m1), + node('C1', node('C0', m1), node('B0', m1)) + ) + assert log_to_node(repo.log('heads/master')), expected @pytest.mark.xfail(reason="removed support for squash+ command") def test_force_squash_merge(self, repo, env): @@ -1323,6 +1427,9 @@ class TestUnknownPR: env['runbot_merge.project']._check_progress() assert pr.staging_id +def node(name, *children): + assert type(name) is str + return name, frozenset(children) def log_to_node(log): log = list(log) nodes = {} diff --git a/runbot_merge/tests/test_multirepo.py b/runbot_merge/tests/test_multirepo.py index b06a704e..8f9a20e4 100644 --- a/runbot_merge/tests/test_multirepo.py +++ b/runbot_merge/tests/test_multirepo.py @@ -202,6 +202,7 @@ def test_ff_fail(env, project, repo_a, repo_b): # add second commit blocking FF cn = repo_b.make_commit('heads/master', 'second', None, tree={'a': 'b_0', 'b': 'other'}) + assert repo_b.commit('heads/master').id == cn repo_a.post_status('heads/staging.master', 'success', 'ci/runbot') repo_a.post_status('heads/staging.master', 'success', 'legal/cla')