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')