[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).
2025-03-15 23:45:44 +07:00 · 2018-08-29 16:51:53 +02:00 · 2018-08-29 16:51:53 +02:00 · a40b4c20da
commit a40b4c20da
parent 6d7c728471
5 changed files with 191 additions and 43 deletions
--- 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'
--- 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,
--- 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
--- 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 = {}
--- 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')