[FIX] runbot_merge: iterate commits in topological order

Previously, runbot_merge assumed github would return commits in
topological order (from base to head of PR). However as in the UI
github sorts commits using the author's date field, so depending on
rebasing/cherrypick/... it's possible to have the head of the commit
be "younger" than the rest. In that case robodoo will try to merge
it *first*, then attempt to merge the rest on top of it (-ish, it'd
probably make a hash of it if that worked), at which point github
replies with a 204 (nothing to merge) because the PR head has already
included everything which topologically precedes it.

Fix: re-sort commits topologically when fetching the PR's log. That
way they're rebased in the proper order and correctly linked to one
another.

Example problematic PR: odoo/enterprise#2794, the commits are

773aef03a59d50b33221d7cdcdf54cd0cbe0c914
    author.date: 2018-10-01T14:58:38Z
879547c8dd37e7f413a97393a82f92377785b50b (parent: 773aef03)
    author.date: 2018-10-01T12:02:08Z

Because 879547c8 is "older" than 773aef03, github returns it first,
both in the UI and via the API.

Also fixed up support for committer & author metadata in fake_github
so the local tests would both expose the issue properly and allow
fixing it.

runbot_merge/github.py

@@ -5,6 +5,7 @@ import logging
 
 import requests
 
+from odoo.tools import topological_sort
 from . import exceptions
 
 _logger = logging.getLogger(__name__)
@@ -191,7 +192,16 @@ class GH(object):
         """ Returns a PR's commits oldest first (that's what GH does &
         is what we want)
         """
-        return list(self.commits_lazy(pr))
+        commits = list(self.commits_lazy(pr))
+        # map shas to the position the commit *should* have
+        idx =  {
+            c: i
+            for i, c in enumerate(topological_sort({
+                c['sha']: [p['sha'] for p in c['parents']]
+                for c in commits
+            }))
+        }
+        return sorted(commits, key=lambda c: idx[c['sha']])
 
     def statuses(self, h):
         r = self('get', 'commits/{}/status'.format(h)).json()

runbot_merge/tests/fake_github/__init__.py

@@ -1,4 +1,5 @@
 import collections
+import datetime
 import hashlib
 import hmac
 import io
@@ -266,35 +267,29 @@ class Repo(object):
 
     def _create_commit(self, r):
         body = json.loads(r.body)
-        author = body.get('author') or {'name': 'default', 'email': 'default', 'date': 'Z'}
+        author = body.get('author')
         try:
             sha = self.make_commit(
-                ref=(body.get('parents')),
+                ref=body.get('parents'),
                 message=body['message'],
                 author=author,
-                committer=body.get('committer') or author,
+                committer=body.get('committer'),
                 tree=body['tree']
             )
         except (KeyError, AssertionError):
             # either couldn't find the parent or couldn't find the tree
             return (404, None)
 
-        return (201, {
+        return (201, self._read_commit(r, sha)[1])
-            "sha": sha,
+
-            "author": author,
-            "committer": body.get('committer') or author,
-            "message": body['message'],
-            "tree": {"sha": body['tree']},
-            "parents": [{"sha": sha}],
-        })
     def _read_commit(self, _, sha):
         c = self.objects.get(sha)
         if not isinstance(c, Commit):
             return (404, None)
         return (200, {
             "sha": sha,
-            "author": c.author,
+            "author": c.author.to_json(),
-            "committer": c.committer,
+            "committer": c.committer.to_json(),
             "message": c.message,
             "tree": {"sha": c.tree},
             "parents": [{"sha": p} for p in c.parents],
@@ -437,7 +432,13 @@ class Repo(object):
             nextlink = url.replace(query=url_encode(dict(qs, page=page+1)))
             headers['Link'] = '<%s>; rel="next"' % str(nextlink)
 
-        commits = [c.to_json() for c in pr.commits[offset:limit]]
+        commits = [
+            c.to_json()
+            for c in sorted(
+                pr.commits,
+                key=lambda c: (c.author.date, c.committer.date)
+            )[offset:limit]
+        ]
         body = io.BytesIO(json.dumps(commits).encode('utf-8'))
 
         return responses.HTTPResponse(
@@ -627,13 +628,42 @@ class PR(Issue):
         self.comments.append((user, "REVIEW %s\n\n%s " % (state, body)))
         self.repo.notify('pull_request_review', state, self, user, body)
 
+FMT = '%Y-%m-%dT%H:%M:%SZ'
+class Author(object):
+    __slots__ = ['name', 'email', 'date']
+
+    def __init__(self, name, email, date):
+        self.name = name
+        self.email = email
+        self.date = date or datetime.datetime.now().strftime(FMT)
+
+    @classmethod
+    def from_(cls, d):
+        if not d:
+            return None
+        return Author(**d)
+
+    def to_json(self):
+        return {
+            'name': self.name,
+            'email': self.email,
+            'date': self.date,
+        }
+
+    def __str__(self):
+        return '%s <%s> %d Z' % (
+            self.name,
+            self.email,
+            int(datetime.datetime.strptime(self.date, FMT).timestamp())
+        )
+
 class Commit(object):
     __slots__ = ['tree', 'message', 'author', 'committer', 'parents', 'statuses']
     def __init__(self, tree, message, author, committer, parents):
         self.tree = tree
         self.message = message
-        self.author = author
+        self.author = Author.from_(author) or Author('', '', '')
-        self.committer = committer or author
+        self.committer = Author.from_(committer) or self.author
         self.parents = parents
         self.statuses = []
 
@@ -645,8 +675,8 @@ class Commit(object):
         return {
             "sha": self.id,
             "commit": {
-                "author": self.author,
+                "author": self.author.to_json(),
-                "committer": self.committer,
+                "committer": self.committer.to_json(),
                 "message": self.message,
                 "tree": {"sha": self.tree},
             },

runbot_merge/tests/remote.py

@@ -453,7 +453,6 @@ class Repo:
 
     def make_commit(self, ref, message, author, committer=None, tree=None, wait=True):
         assert tree, "not supporting changes/updates"
-        assert not (author or committer)
 
         if not ref: # None / []
             # apparently github refuses to create trees/commits in empty repos
@@ -483,12 +482,17 @@ class Repo:
         assert 200 <= r.status_code < 300, r.json()
         h = r.json()['sha']
 
-        r = self._session.post('https://api.github.com/repos/{}/git/commits'.format(self.name), json={
+        data = {
             'parents': parents,
             'message': message,
             'tree': h,
+        }
+        if author:
+            data['author'] = author
+        if committer:
+            data['committer'] = committer
 
-        })
+        r = self._session.post('https://api.github.com/repos/{}/git/commits'.format(self.name), json=data)
         assert 200 <= r.status_code < 300, r.json()
 
         commit_sha = r.json()['sha']

runbot_merge/tests/test_basic.py

@@ -2,6 +2,7 @@ import datetime
 import json
 import re
 import time
+import requests
 
 import pytest
 
@@ -818,8 +819,14 @@ class TestMergeMethod:
         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'})
+        # test commit ordering issue while at it: github sorts commits on
-        b1 = repo.make_commit(b0, 'B1', None, tree={'m': '1', 'b': '1'})
+        # author.date instead of doing so topologically which is absolutely
+        # not what we want
+        committer = {'name': 'a', 'email': 'a', 'date': '2018-10-08T11:48:43Z'}
+        author0 = {'name': 'a', 'email': 'a', 'date': '2018-10-01T14:58:38Z'}
+        author1 = {'name': 'a', 'email': 'a', 'date': '2015-10-01T14:58:38Z'}
+        b0 = repo.make_commit(m1, 'B0', author=author0, committer=committer, tree={'m': '1', 'b': '0'})
+        b1 = repo.make_commit(b0, 'B1', author=author1, committer=committer, 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')