From a1384b386803130a12f9d09d6ff6970f833cd468 Mon Sep 17 00:00:00 2001 From: Xavier Morel Date: Tue, 9 Oct 2018 15:01:45 +0200 Subject: [PATCH] [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 | 12 +++- runbot_merge/tests/fake_github/__init__.py | 66 ++++++++++++++++------ runbot_merge/tests/remote.py | 10 +++- runbot_merge/tests/test_basic.py | 11 +++- 4 files changed, 75 insertions(+), 24 deletions(-) diff --git a/runbot_merge/github.py b/runbot_merge/github.py index c6702656..9f158482 100644 --- a/runbot_merge/github.py +++ b/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() diff --git a/runbot_merge/tests/fake_github/__init__.py b/runbot_merge/tests/fake_github/__init__.py index 5b1f1600..e6ceed53 100644 --- a/runbot_merge/tests/fake_github/__init__.py +++ b/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, { - "sha": sha, - "author": author, - "committer": body.get('committer') or author, - "message": body['message'], - "tree": {"sha": body['tree']}, - "parents": [{"sha": sha}], - }) + return (201, self._read_commit(r, sha)[1]) + 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, - "committer": c.committer, + "author": c.author.to_json(), + "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.committer = committer or author + self.author = Author.from_(author) 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, - "committer": self.committer, + "author": self.author.to_json(), + "committer": self.committer.to_json(), "message": self.message, "tree": {"sha": self.tree}, }, diff --git a/runbot_merge/tests/remote.py b/runbot_merge/tests/remote.py index fb99905c..9601382e 100644 --- a/runbot_merge/tests/remote.py +++ b/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'] diff --git a/runbot_merge/tests/test_basic.py b/runbot_merge/tests/test_basic.py index de5f89f6..a9cd6fcd 100644 --- a/runbot_merge/tests/test_basic.py +++ b/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'}) - b1 = repo.make_commit(b0, 'B1', None, tree={'m': '1', 'b': '1'}) + # test commit ordering issue while at it: github sorts commits on + # 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')