From 6d7c728471d44853fc9f09dad006eb4e44c65441 Mon Sep 17 00:00:00 2001 From: Xavier Morel Date: Tue, 28 Aug 2018 15:42:28 +0200 Subject: [PATCH] [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 | 77 ++++++++++++------ runbot_merge/models/pull_requests.py | 25 +++--- runbot_merge/tests/fake_github/__init__.py | 49 ++++++++---- runbot_merge/tests/remote.py | 15 ++-- runbot_merge/tests/test_basic.py | 91 +++++++++++++++++++++- 5 files changed, 198 insertions(+), 59 deletions(-) diff --git a/runbot_merge/github.py b/runbot_merge/github.py index b2eaf696..4f93123f 100644 --- a/runbot_merge/github.py +++ b/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): - if not squash: - r = self('post', 'merges', json={ - 'base': dest, - 'head': sha, - 'commit_message': message, - }, check={409: exceptions.MergeError}) - r = r.json() - return dict(r['commit'], sha=r['sha']) + def merge(self, sha, dest, message): + r = self('post', 'merges', json={ + 'base': dest, + 'head': sha, + 'commit_message': message, + }, check={409: exceptions.MergeError}) + r = r.json() + return dict(r['commit'], sha=r['sha']) - current_head = self.head(dest) - tree = self.merge(sha, dest, "temp")['tree']['sha'] - c = self('post', 'git/commits', json={ - 'message': message, - 'tree': tree, - 'parents': [current_head], - 'author': author, - }, check={409: exceptions.MergeError}).json() - self.set_ref(dest, c['sha']) - return c + def rebase(self, pr, dest, reset=False, commits=None): + """ Rebase pr's commits on top of dest, updates dest unless ``reset`` + is set. + + Returns the hash of the rebased head. + """ + original_head = self.head(dest) + if commits is None: + commits = self.commits(pr) + + 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 diff --git a/runbot_merge/models/pull_requests.py b/runbot_merge/models/pull_requests.py index 13961e97..8f7e0181 100644 --- a/runbot_merge/models/pull_requests.py +++ b/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'] - except exceptions.MergeError: - _logger.exception("Failed to merge %s:%s into staging branch", pr.repository.name, pr.number) + # FIXME: !rebase + + 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)") diff --git a/runbot_merge/tests/fake_github/__init__.py b/runbot_merge/tests/fake_github/__init__.py index 547c1df5..69fcbdfe 100644 --- a/runbot_merge/tests/fake_github/__init__.py +++ b/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, { - "sha": c.id, - "commit": { - "author": c.author, - "committer": c.committer, - "message": body['commit_message'], - "tree": {"sha": tid}, - }, - "parents": [{"sha": target}, {"sha": sha}] - }) + return (201, c.to_json()) _handlers = [ ('POST', r'git/refs', _create_ref), @@ -512,6 +511,7 @@ class Repo(object): ('GET', r'pulls/(?P\d+)', _read_pr), ('PATCH', r'pulls/(?P\d+)', _edit_pr), ('GET', r'pulls/(?P\d+)/reviews', _read_pr_reviews), + ('GET', r'pulls/(?P\d+)/commits', _read_pr_commits), ('POST', r'issues/(?P\d+)/labels', _add_labels), ('DELETE', r'issues/(?P\d+)/labels/(?P