[CHG] runbot_merge: toggle default merge method to rebase-and-merge

After discussion with al & rco, conclusion was default PR merging method
should be rebase-and-merge for cleaner history.

Add test for that scenario (w/ test for final DAG) and implement this
change.

runbot_merge/github.py

@@ -15,13 +15,14 @@ class GH(object):
         session = self._session = requests.Session()
         session.headers['Authorization'] = 'token {}'.format(token)
 
-    def __call__(self, method, path, json=None, check=True):
+    def __call__(self, method, path, params=None, json=None, check=True):
         """
         :type check: bool | dict[int:Exception]
         """
         r = self._session.request(
             method,
             '{}/repos/{}/{}'.format(self._url, self._repo, path),
+            params=params,
             json=json
         )
         if check:
@@ -89,26 +90,48 @@ class GH(object):
                 return
         raise AssertionError("{}: {}".format(r.status_code, r.json()))
 
-    def merge(self, sha, dest, message, squash=False, author=None):
+    def merge(self, sha, dest, message):
-        if not squash:
+        r = self('post', 'merges', json={
-            r = self('post', 'merges', json={
+            'base': dest,
-                'base': dest,
+            'head': sha,
-                'head': sha,
+            'commit_message': message,
-                'commit_message': message,
+        }, check={409: exceptions.MergeError})
-            }, check={409: exceptions.MergeError})
+        r = r.json()
-            r = r.json()
+        return dict(r['commit'], sha=r['sha'])
-            return dict(r['commit'], sha=r['sha'])
 
-        current_head = self.head(dest)
+    def rebase(self, pr, dest, reset=False, commits=None):
-        tree = self.merge(sha, dest, "temp")['tree']['sha']
+        """ Rebase pr's commits on top of dest, updates dest unless ``reset``
-        c = self('post', 'git/commits', json={
+        is set.
-            'message': message,
+
-            'tree': tree,
+        Returns the hash of the rebased head.
-            'parents': [current_head],
+        """
-            'author': author,
+        original_head = self.head(dest)
-        }, check={409: exceptions.MergeError}).json()
+        if commits is None:
-        self.set_ref(dest, c['sha'])
+            commits = self.commits(pr)
-        return c
+
+        assert commits, "can't rebase a PR with no commits"
+        for c in commits:
+            assert len(c['parents']) == 1, "can't rebase commits with more than one parent"
+            tmp_msg = 'temp rebasing PR %s (%s)' % (pr, c['sha'])
+            c['new_tree'] = self.merge(c['sha'], dest, tmp_msg)['tree']['sha']
+        self.set_ref(dest, original_head)
+
+        prev = original_head
+        for c in commits:
+            copy = self('post', 'git/commits', json={
+                'message': c['commit']['message'],
+                'tree': c['new_tree'],
+                'parents': [prev],
+                'author': c['commit']['author'],
+                'committer': c['commit']['committer'],
+            }, check={409: exceptions.MergeError}).json()
+            prev = copy['sha']
+
+        if reset:
+            self.set_ref(dest, original_head)
+
+        # prev is updated after each copy so it's the rebased PR head
+        return prev
 
     # fetch various bits of issues / prs to load them
     def pr(self, number):
@@ -119,18 +142,26 @@ class GH(object):
 
     def comments(self, number):
         for page in itertools.count(1):
-            r = self('get', 'issues/{}/comments?page={}'.format(number, page))
+            r = self('get', 'issues/{}/comments'.format(number), params={'page': page})
             yield from r.json()
             if not r.links.get('next'):
                 return
 
     def reviews(self, number):
         for page in itertools.count(1):
-            r = self('get', 'pulls/{}/reviews?page={}'.format(number, page))
+            r = self('get', 'pulls/{}/reviews'.format(number), params={'page': page})
             yield from r.json()
             if not r.links.get('next'):
                 return
 
+    def commits(self, pr):
+        """ Returns a PR's commits oldest first (that's what GH does &
+        is what we want)
+        """
+        r = self('get', 'pulls/{}/commits'.format(pr), params={'per_page': PR_COMMITS_MAX})
+        assert not r.links.get('next'), "more than {} commits".format(PR_COMMITS_MAX)
+        return r.json()
+
     def statuses(self, h):
         r = self('get', 'commits/{}/status'.format(h)).json()
         return [{
@@ -138,3 +169,5 @@ class GH(object):
             'context': s['context'],
             'state': s['state'],
         } for s in r['statuses']]
+
+PR_COMMITS_MAX = 50

runbot_merge/models/pull_requests.py

@@ -888,19 +888,22 @@ class Batch(models.Model):
                 "Staging pr %s:%s for target %s; squash=%s",
                 pr.repository.name, pr.number, pr.target.name, pr.squash
             )
-            msg = pr.message
-            author=None
-            if pr.squash:
-                commit = gh.commit(pr.head)
-                msg = commit['message']
-                author = commit['author']
-
-            msg += '\n\ncloses {pr.repository.name}#{pr.number}'.format(pr=pr)
 
             try:
-                new_heads[pr] = gh.merge(pr.head, 'tmp.{}'.format(pr.target.name), msg, squash=pr.squash, author=author)['sha']
+                # FIXME: !rebase
-            except exceptions.MergeError:
+
-                _logger.exception("Failed to merge %s:%s into staging branch", pr.repository.name, pr.number)
+                target = 'tmp.{}'.format(pr.target.name)
+                suffix = '\n\ncloses {pr.repository.name}#{pr.number}'.format(pr=pr)
+                pr_commits = gh.commits(pr.number)
+                if len(pr_commits) == 1:
+                    pr_commits[0]['commit']['message'] += suffix
+                    new_heads[pr] = gh.rebase(pr.number, target, commits=pr_commits)
+                else:
+                    msg = pr.message + suffix
+                    h = gh.rebase(pr.number, target, reset=True, commits=pr_commits)
+                    new_heads[pr] = gh.merge(h, 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'
                 gh.comment(pr.number, "Unable to stage PR (merge conflict)")
 

runbot_merge/tests/fake_github/__init__.py

@@ -130,7 +130,7 @@ class Repo(object):
         while commits:
             c = commits.pop(0)
             commits.extend(self.commit(r) for r in c.parents)
-            yield c
+            yield c.to_json()
 
     def post_status(self, ref, status, context='default', description=""):
         assert status in ('error', 'failure', 'pending', 'success')
@@ -194,7 +194,7 @@ class Repo(object):
         for method, pattern, handler in sorted(self._handlers, key=lambda t: -len(t[1])):
             if method and request.method != method:
                 continue
-
+            # FIXME: remove qs from path & ensure path is entirely matched, maybe finally use proper routing?
             m = re.match(pattern, path)
             if m:
                 return handler(self, request, **m.groupdict())
@@ -377,7 +377,7 @@ class Repo(object):
             },
             'title': pr.title,
             'body': pr.body,
-            'commits': pr.commits,
+            'commits': len(pr.commits),
             'user': {'login': pr.user},
         })
 
@@ -423,6 +423,14 @@ class Repo(object):
             if r
         ])
 
+    def _read_pr_commits(self, r, number):
+        pr = self.issues.get(int(number))
+        if not isinstance(pr, PR):
+            return (404, None)
+
+        return (200, [c.to_json() for c in pr.commits])
+
+
     def _add_labels(self, r, number):
         try:
             pr = self.issues[int(number)]
@@ -482,16 +490,7 @@ class Repo(object):
         c = Commit(tid, body['commit_message'], author=None, committer=None, parents=[target, sha])
         self.objects[c.id] = c
 
-        return (201, {
+        return (201, c.to_json())
-            "sha": c.id,
-            "commit": {
-                "author": c.author,
-                "committer": c.committer,
-                "message": body['commit_message'],
-                "tree": {"sha": tid},
-            },
-            "parents": [{"sha": target}, {"sha": sha}]
-        })
 
     _handlers = [
         ('POST', r'git/refs', _create_ref),
@@ -512,6 +511,7 @@ class Repo(object):
         ('GET', r'pulls/(?P<number>\d+)', _read_pr),
         ('PATCH', r'pulls/(?P<number>\d+)', _edit_pr),
         ('GET', r'pulls/(?P<number>\d+)/reviews', _read_pr_reviews),
+        ('GET', r'pulls/(?P<number>\d+)/commits', _read_pr_commits),
 
         ('POST', r'issues/(?P<number>\d+)/labels', _add_labels),
         ('DELETE', r'issues/(?P<number>\d+)/labels/(?P<label>.+)', _remove_label),
@@ -599,8 +599,13 @@ class PR(Issue):
     def commits(self):
         store = self.repo.objects
         target = self.repo.commit('heads/%s' % self.base).id
-        return len({h for h, _ in git.walk_ancestors(store, self.head, False)}
+
-                   - {h for h, _ in git.walk_ancestors(store, target, False)})
+        base = {h for h, _ in git.walk_ancestors(store, target, False)}
+        own = [
+            h for h, _ in git.walk_ancestors(store, self.head, False)
+            if h not in base
+        ]
+        return list(map(self.repo.commit, reversed(own)))
 
     def post_review(self, state, user, body):
         self.comments.append((user, "REVIEW %s\n\n%s " % (state, body)))
@@ -620,6 +625,18 @@ class Commit(object):
     def id(self):
         return git.make_commit(self.tree, self.message, self.author, self.committer, parents=self.parents)[0]
 
+    def to_json(self):
+        return {
+            "sha": self.id,
+            "commit": {
+                "author": self.author,
+                "committer": self.committer,
+                "message": self.message,
+                "tree": {"sha": self.tree},
+            },
+            "parents": [{"sha": p} for p in self.parents]
+        }
+
     def __str__(self):
         parents = '\n'.join('parent {}'.format(p) for p in self.parents) + '\n'
         return """commit {}
@@ -733,6 +750,6 @@ class Client(werkzeug.test.Client):
             },
             'title': pr.title,
             'body': pr.body,
-            'commits': pr.commits,
+            'commits': len(pr.commits),
             'user': {'login': pr.user},
         }

runbot_merge/tests/remote.py

@@ -564,12 +564,15 @@ class Repo:
         return any(c['sha'] == sha for c in self.log(of))
 
     def log(self, ref_or_sha):
-        r = self._session.get(
+        for page in itertools.count(1):
-            'https://api.github.com/repos/{}/commits'.format(self.name),
+            r = self._session.get(
-            params={'sha': ref_or_sha}
+                'https://api.github.com/repos/{}/commits'.format(self.name),
-        )
+                params={'sha': ref_or_sha, 'page': page}
-        assert 200 <= r.status_code < 300, r.json()
+            )
-        return r.json()
+            assert 200 <= r.status_code < 300, r.json()
+            yield from r.json()
+            if not r.links.get('next'):
+                return
 
 ct = itertools.count()
 

runbot_merge/tests/test_basic.py

@@ -52,8 +52,8 @@ def test_trivial_flow(env, repo):
     assert pr1.labels == {'seen 🙂', 'CI 🤖', 'r+ 👌', 'merged 🎉'}
 
     master = repo.commit('heads/master')
-    assert master.parents == [m, pr1.head],\
+    # with default-rebase, only one parent is "known"
-        "master's parents should be the old master & the PR head"
+    assert master.parents[0] == m
     assert repo.read_tree(master) == {
         'a': b'some other content',
         'b': b'a second file',
@@ -510,7 +510,7 @@ class TestRetry:
         env['runbot_merge.project']._check_progress()
         assert pr.state == 'validated'
 
-class TestSquashing(object):
+class TestMergeMethod:
     """
     if event['pull_request']['commits'] == 1, "squash" (/rebase); otherwise
     regular merge
@@ -595,6 +595,79 @@ class TestSquashing(object):
         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_rebase_merge(self, repo, env):
+        """ a multi-commit PR should be rebased & merged by default
+
+        left: PR
+        right: post-merge result
+
+                     +------+                   +------+
+                     |  M0  |                   |  M0  |
+                     +--^---+                   +--^---+
+                        |                          |
+                        |                          |
+                     +--+---+                   +--+---+
+                +---->  M1  <--+                |  M1  <--+
+                |    +------+  |                +------+  |
+                |              |                          |
+                |              |                          |
+             +--+---+      +---+---+    +------+      +---+---+
+             |  B0  |      |  M2   |    |  B0  +------>  M2   |
+             +--^---+      +-------+    +--^---+      +---^---+
+                |                          |              |
+             +--+---+                   +--+---+          |
+          PR |  B1  |                   |  B1  |          |
+             +------+                   +--^---+          |
+                                           |          +---+---+
+                                           +----------+ merge |
+                                                      +-------+
+        """
+        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")
+
+        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 (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]))]))
+        expected = (
+            'title\n\nbody\n\ncloses {}#{}'.format(repo.name, prx.number),
+            frozenset([nm2, nb1])
+        )
+        assert staging == expected
+
+        final_tree = repo.read_tree(repo.commit('heads/staging.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):
+        """
+        """
+
+    def test_pr_unrebase(self, repo, env):
+        """ should be possible to flag a PR as regular-merged
+        """
+        pytest.skip("todo")
+
+    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
+        """
+        pytest.skip("todo")
+
     @pytest.mark.xfail(reason="removed support for squash+ command")
     def test_force_squash_merge(self, repo, env):
         m = repo.make_commit(None, 'initial', None, tree={'m': 'm'})
@@ -1215,7 +1288,7 @@ class TestUnknownPR:
        valid but unknown PRs
 
     * get statuses if head commit unknown (additional cron?)
-    * assume there are no existing r+ (?)
+    * handle any comment & review (existing PRs may enter the system on a review/r+)
     """
     def test_rplus_unknown(self, repo, env):
         m = repo.make_commit(None, 'initial', None, tree={'m': 'm'})
@@ -1249,3 +1322,13 @@ class TestUnknownPR:
 
         env['runbot_merge.project']._check_progress()
         assert pr.staging_id
+
+def log_to_node(log):
+    log = list(log)
+    nodes = {}
+    for c in reversed(log):
+        nodes[c['sha']] = (c['commit']['message'], frozenset(
+            nodes[p['sha']]
+            for p in (c['parents'])
+        ))
+    return nodes[log[0]['sha']]