From 85a78900239d793e3fd62dcfbf7914f9e223a5ea Mon Sep 17 00:00:00 2001 From: Xavier Morel Date: Fri, 18 Aug 2023 13:51:18 +0200 Subject: [PATCH] [CHG] runbot_merge: switch staging from github API to local It has been a consideration for a while, but the pain of subtly interacting with git via the ignominous CLI kept it back. Then ~~the fire nation attacked~~ github got more and more tight-fisted (and in some ways less reliable) with their API. Staging pretty much just interacts with the git database, so it's both a facultative github operator (it can just interact with git directly) and a big consumer of API requests (because the git database endpoints are very low level so it takes quite a bit of work to do anything especially when high-level operations like rebase have to be replicated by hand). Furthermore, an issue has also been noticed which can be attributed to using the github API (and that API's reliability getting worse): in some cases github will fail to propagate a ref update / reset, so when staging 2 PRs it's possible that the second one is merged on top of the temporary branch of the first one, yielding a kinda broken commit (in that it's a merge commit with a broken error message) instead of the rebase / squash commit we expected. As it turns out it's a very old issue but only happened very early so was misattributed and not (sufficiently) guarded against: - 41bd82244bb976bbd4d4be5e7bd792417c7dae6b (October 8th 2018) was spotted but thought to be a mergebot issue (might have been one of the opportunities where ref-checks were added though I can't find any reference to the commit in the runbot repo). - 2be25052e147b151d1d8a5bc73cceb351586ce03 (October 15th, 2019) was missed (or ignored). - 5a9fe7a7d05a9df7186072a7bffd60c6b428fd0e (July 31st, 2023) was spotted, but happened at a moment where everything kinda broke because of github rate-limiting ref updates, so the forensics were difficult and it was attributed to rate limiting issues. - f10d03bf0f2e8f88f62a5d8356b84f714196130f (August 24th, 2023) broke the camel's back (and the head block): the logs were not too interspersed with other garbage and pretty clear that github ack'd a ref update, returned the correct oid when checking the ref, then returned the wrong oid when fetching it later on. No Working Copy =============== The working copy turns out to not be necessary, the plumbing commands we *need* work just fine on a bare repository. Working without a WC means we had to reimplement the high level operations (rebase) by hand much as we'd done previously, *but* we needed to do that anyway as git doesn't seem to provide any way to retrieve the mapping when rebasing/cherrypicking, and cherrypicking by commit doesn't work well as it can't really find the *merge base* it needs. Forward-porting can almost certainly be implemented similarly (with some overhead), issue #803 has been opened to keep track of the idea. No TMP ====== The `tmp.` branches are no more, the process of creating stagings is based entirely around oids, if staging something fails we can just abandon the oids (they'll be collected by the weekly GC), we only need to update the staging branches at the very end of the process. This simplifies things a fair bit. For now we have stopped checking for visibility / backoff as we're pushing via git, hopefully it is a more reliable reference than the API. Commmit Message Formatting ========================== There's some unfortunate churn in the test, as the handling of trailing newlines differs between github's APIs and git itself. Fixes #247 PS: It might be a good idea to use pygit2 instead of the CLI eventually, the library is typed which is nice, and it avoids shelling out although that's really unlikely to be a major cost. --- conftest.py | 3 + mergebot_test_utils/utils.py | 7 +- runbot_merge/git.py | 118 ++++++++++- runbot_merge/models/stagings_create.py | 273 +++++++++++-------------- runbot_merge/tests/test_basic.py | 104 +++++----- runbot_merge/tests/test_multirepo.py | 12 +- 6 files changed, 307 insertions(+), 210 deletions(-) diff --git a/conftest.py b/conftest.py index a244f2af7..3d936e56d 100644 --- a/conftest.py +++ b/conftest.py @@ -537,6 +537,9 @@ def __init__(self, session, fullname, repos): self.hook = False repos.append(self) + def __repr__(self): + return f'' + @property def owner(self): return self.name.split('/')[0] diff --git a/mergebot_test_utils/utils.py b/mergebot_test_utils/utils.py index b87a23455..fae0d208f 100644 --- a/mergebot_test_utils/utils.py +++ b/mergebot_test_utils/utils.py @@ -8,7 +8,8 @@ closes {repo}#{number} -{headers}Signed-off-by: {name} <{email}>""" +{headers}Signed-off-by: {name} <{email}> +""" # target branch '-' source branch '-' base64 unique '-fw' REF_PATTERN = r'{target}-{source}-[a-zA-Z0-9_-]{{4}}-fw' @@ -136,6 +137,4 @@ def to_pr(env, pr): return pr def part_of(label, pr_id, *, separator='\n\n'): - """ Adds the "part-of" pseudo-header in the footer. - """ - return f'{label}{separator}Part-of: {pr_id.display_name}' + return f'{label}{separator}Part-of: {pr_id.display_name}\n' diff --git a/runbot_merge/git.py b/runbot_merge/git.py index 91297f9e0..09061933c 100644 --- a/runbot_merge/git.py +++ b/runbot_merge/git.py @@ -1,12 +1,14 @@ import dataclasses import itertools import logging +import os import pathlib import resource import subprocess -from typing import Optional, TypeVar +from typing import Optional, TypeVar, Union, Sequence, Tuple, Dict from odoo.tools.appdirs import user_cache_dir +from .github import MergeError, PrCommit _logger = logging.getLogger(__name__) @@ -17,6 +19,7 @@ def source_url(repository, prefix: str) -> str: repository.name, ) +Authorship = Union[Tuple[str, str], Tuple[str, str, str]] def get_local(repository, prefix: Optional[str]) -> 'Optional[Repo]': repos_dir = pathlib.Path(user_cache_dir('mergebot')) @@ -104,6 +107,119 @@ def clone(self, to: str, branch: Optional[str] = None) -> Self: ) return Repo(to) + def rebase(self, dest: str, commits: Sequence[PrCommit]) -> Tuple[str, Dict[str, str]]: + """Implements rebase by hand atop plumbing so: + + - we can work without a working copy + - we can track individual commits (and store the mapping) + + It looks like `--merge-base` is not sufficient for `merge-tree` to + correctly keep track of history, so it loses contents. Therefore + implement in two passes as in the github version. + """ + repo = self.stdout().with_config(text=True, check=False) + + logger = _logger.getChild('rebase') + logger.debug("rebasing %s on %s (reset=%s, commits=%s)", + self._repo, dest, len(commits)) + if not commits: + raise MergeError("PR has no commits") + + new_trees = [] + parent = dest + for original in commits: + if len(original['parents']) != 1: + raise MergeError( + f"commits with multiple parents ({original['sha']}) can not be rebased, " + "either fix the branch to remove merges or merge without " + "rebasing") + + new_trees.append(check(repo.merge_tree(parent, original['sha'])).stdout.strip()) + parent = check(repo.commit_tree( + tree=new_trees[-1], + parents=[parent, original['sha']], + message=f'temp rebase {original["sha"]}', + )).stdout.strip() + + mapping = {} + for original, tree in zip(commits, new_trees): + authorship = check(repo.show('--no-patch', '--pretty="%an%n%ae%n%ai%n%cn%n%ce"', original['sha'])) + author_name, author_email, author_date, committer_name, committer_email =\ + authorship.stdout.splitlines() + + c = check(repo.commit_tree( + tree=tree, + parents=[dest], + message=original['commit']['message'], + author=(author_name, author_email, author_date), + committer=(committer_name, committer_email), + )).stdout.strip() + + logger.debug('copied %s to %s (parent: %s)', original['sha'], c, dest) + dest = mapping[original['sha']] = c + + return dest, mapping + + def merge(self, c1: str, c2: str, msg: str, *, author: Tuple[str, str]) -> str: + repo = self.stdout().with_config(text=True, check=False) + + t = repo.merge_tree(c1, c2) + if t.returncode: + raise MergeError(t.stderr) + + c = self.commit_tree( + tree=t.stdout.strip(), + message=msg, + parents=[c1, c2], + author=author, + ) + if c.returncode: + raise MergeError(c.stderr) + return c.stdout.strip() + + def commit_tree( + self, *, tree: str, message: str, + parents: Sequence[str] = (), + author: Optional[Authorship] = None, + committer: Optional[Authorship] = None, + ) -> subprocess.CompletedProcess: + authorship = {} + if author: + authorship['GIT_AUTHOR_NAME'] = author[0] + authorship['GIT_AUTHOR_EMAIL'] = author[1] + if len(author) > 2: + authorship['GIT_AUTHOR_DATE'] = author[2] + if committer: + authorship['GIT_COMMITTER_NAME'] = committer[0] + authorship['GIT_COMMITTER_EMAIL'] = committer[1] + if len(committer) > 2: + authorship['GIT_COMMITTER_DATE'] = committer[2] + + return self.with_config( + stdout=subprocess.PIPE, + text=True, + env={ + **os.environ, + **authorship, + # we don't want git to use the timezone of the machine it's + # running on: previously it used the timezone configured in + # github (?), which I think / assume defaults to a generic UTC + 'TZ': 'UTC', + } + )._run( + 'commit-tree', + tree, + '-m', message, + *itertools.chain.from_iterable(('-p', p) for p in parents), + ) + +def check(p: subprocess.CompletedProcess) -> subprocess.CompletedProcess: + if not p.returncode: + return p + + _logger.info("rebase failed at %s\nstdout:\n%s\nstderr:\n%s", p.args, p.stdout, p.stderr) + raise MergeError(p.stderr or 'merge conflict') + @dataclasses.dataclass class GitCommand: diff --git a/runbot_merge/models/stagings_create.py b/runbot_merge/models/stagings_create.py index b00342dc6..544712a76 100644 --- a/runbot_merge/models/stagings_create.py +++ b/runbot_merge/models/stagings_create.py @@ -6,10 +6,9 @@ import logging import os import re -import tempfile from difflib import Differ -from itertools import count, takewhile -from pathlib import Path +from itertools import takewhile +from operator import itemgetter from typing import Dict, Union, Optional, Literal, Callable, Iterator, Tuple, List, TypeAlias import requests @@ -17,7 +16,6 @@ from odoo import api, models, fields from odoo.tools import OrderedSet -from odoo.tools.appdirs import user_cache_dir from .pull_requests import Branch, Stagings, PullRequests, Repository, Batch from .. import exceptions, utils, github, git @@ -40,7 +38,7 @@ class StagingSlice: """ gh: github.GH head: str - working_copy: git.Repo + repo: git.Repo StagingState: TypeAlias = Dict[Repository, StagingSlice] @@ -82,43 +80,7 @@ def try_staging(branch: Branch) -> Optional[Stagings]: else: # p=2 batched_prs = [pr_ids for _, pr_ids in takewhile(lambda r: r[0] == priority, rows)] - with contextlib.ExitStack() as cleanup: - return stage_into(branch, batched_prs, cleanup) - - -def ready_prs(for_branch: Branch) -> List[Tuple[int, PullRequests]]: - env = for_branch.env - env.cr.execute(""" - SELECT - min(pr.priority) as priority, - array_agg(pr.id) AS match - FROM runbot_merge_pull_requests pr - WHERE pr.target = any(%s) - -- exclude terminal states (so there's no issue when - -- deleting branches & reusing labels) - AND pr.state != 'merged' - AND pr.state != 'closed' - GROUP BY - pr.target, - CASE - WHEN pr.label SIMILAR TO '%%:patch-[[:digit:]]+' - THEN pr.id::text - ELSE pr.label - END - HAVING - bool_or(pr.state = 'ready') or bool_or(pr.priority = 0) - ORDER BY min(pr.priority), min(pr.id) - """, [for_branch.ids]) - browse = env['runbot_merge.pull_requests'].browse - return [(p, browse(ids)) for p, ids in env.cr.fetchall()] - - -def stage_into( - branch: Branch, - batched_prs: List[PullRequests], - cleanup: contextlib.ExitStack, -) -> Optional[Stagings]: - original_heads, staging_state = staging_setup(branch, batched_prs, cleanup) + original_heads, staging_state = staging_setup(branch, batched_prs) staged = stage_batches(branch, batched_prs, staging_state) @@ -146,18 +108,22 @@ def stage_into( # (with a uniquifier to ensure we don't hit a previous version of # the same) to ensure the staging head is new and we're building # everything - tree = it.gh.commit(it.head)['tree'] + project = branch.project_id uniquifier = base64.b64encode(os.urandom(12)).decode('ascii') - dummy_head = it.gh('post', 'git/commits', json={ - 'tree': tree['sha'], - 'parents': [it.head], - 'message': f'''\ + dummy_head = it.repo.with_config(check=True).commit_tree( + # somewhat exceptionally, `commit-tree` wants an actual tree + # not a tree-ish + tree=f'{it.head}^{{tree}}', + parents=[it.head], + author=(project.github_name, project.github_email), + message=f'''\ force rebuild uniquifier: {uniquifier} For-Commit-Id: {it.head} ''', - }).json()['sha'] + ).stdout.strip() + # see above, ideally we don't need to mark the real head as # `to_check` because it's an old commit but `DO UPDATE` is necessary # for `RETURNING` to work, and it doesn't really hurt (maybe) @@ -187,34 +153,17 @@ def stage_into( 'heads': heads, 'commits': commits, }) - # create staging branch from tmp - token = branch.project_id.github_token - for repo in branch.project_id.repo_ids.having_branch(branch): - it = staging_state[repo] + for repo, it in staging_state.items(): _logger.info( "%s: create staging for %s:%s at %s", branch.project_id.name, repo.name, branch.name, it.head ) - refname = 'staging.{}'.format(branch.name) - it.gh.set_ref(refname, it.head) - - i = count() - @utils.backoff(delays=WAIT_FOR_VISIBILITY, exc=TimeoutError) - def wait_for_visibility(): - if check_visibility(repo, refname, it.head, token): - _logger.info( - "[repo] updated %s:%s to %s: ok (at %d/%d)", - repo.name, refname, it.head, - next(i), len(WAIT_FOR_VISIBILITY) - ) - return - _logger.warning( - "[repo] updated %s:%s to %s: failed (at %d/%d)", - repo.name, refname, it.head, - next(i), len(WAIT_FOR_VISIBILITY) - ) - raise TimeoutError("Staged head not updated after %d seconds" % sum(WAIT_FOR_VISIBILITY)) + it.repo.stdout(False).check(True).push( + '-f', + git.source_url(repo, 'github'), + f'{it.head}:refs/heads/staging.{branch.name}', + ) _logger.info("Created staging %s (%s) to %s", st, ', '.join( '%s[%s]' % (batch, batch.prs) @@ -223,10 +172,36 @@ def wait_for_visibility(): return st +def ready_prs(for_branch: Branch) -> List[Tuple[int, PullRequests]]: + env = for_branch.env + env.cr.execute(""" + SELECT + min(pr.priority) as priority, + array_agg(pr.id) AS match + FROM runbot_merge_pull_requests pr + WHERE pr.target = any(%s) + -- exclude terminal states (so there's no issue when + -- deleting branches & reusing labels) + AND pr.state != 'merged' + AND pr.state != 'closed' + GROUP BY + pr.target, + CASE + WHEN pr.label SIMILAR TO '%%:patch-[[:digit:]]+' + THEN pr.id::text + ELSE pr.label + END + HAVING + bool_or(pr.state = 'ready') or bool_or(pr.priority = 0) + ORDER BY min(pr.priority), min(pr.id) + """, [for_branch.ids]) + browse = env['runbot_merge.pull_requests'].browse + return [(p, browse(ids)) for p, ids in env.cr.fetchall()] + + def staging_setup( target: Branch, batched_prs: List[PullRequests], - cleanup: contextlib.ExitStack ) -> Tuple[Dict[Repository, str], StagingState]: """Sets up the staging: @@ -235,14 +210,11 @@ def staging_setup( - generates working copy for each repository with the target branch """ all_prs: PullRequests = target.env['runbot_merge.pull_requests'].concat(*batched_prs) - cache_dir = user_cache_dir('mergebot') staging_state = {} original_heads = {} for repo in target.project_id.repo_ids.having_branch(target): gh = repo.github() head = gh.head(target.name) - # create tmp staging branch - gh.set_ref('tmp.{}'.format(target.name), head) source = git.get_local(repo, 'github') source.fetch( @@ -255,14 +227,8 @@ def staging_setup( f'+refs/heads/{target.name}:refs/heads/{target.name}', *(pr.head for pr in all_prs if pr.repository == repo) ) - Path(cache_dir, repo.name).parent.mkdir(parents=True, exist_ok=True) - d = cleanup.enter_context(tempfile.TemporaryDirectory( - prefix=f'{repo.name}-{target.name}-staging', - dir=cache_dir, - )) - working_copy = source.clone(d, branch=target.name) original_heads[repo] = head - staging_state[repo] = StagingSlice(gh=gh, head=head, working_copy=working_copy) + staging_state[repo] = StagingSlice(gh=gh, head=head, repo=source.stdout().with_config(text=True, check=False)) return original_heads, staging_state @@ -343,39 +309,29 @@ def read_line() -> Optional[bytes]: def stage_batch(env: api.Environment, prs: PullRequests, staging: StagingState) -> Batch: - """ - Updates meta[*][head] on success + """Stages the batch represented by the ``prs`` recordset, onto the + current corresponding staging heads. + + Alongside returning the newly created batch, updates ``staging[*].head`` + in-place on success. On failure, the heads should not be touched. """ new_heads: Dict[PullRequests, str] = {} pr_fields = env['runbot_merge.pull_requests']._fields for pr in prs: - gh = staging[pr.repository].gh - + info = staging[pr.repository] _logger.info( "Staging pr %s for target %s; method=%s", pr.display_name, pr.target.name, pr.merge_method or (pr.squash and 'single') or None ) - target = 'tmp.{}'.format(pr.target.name) - original_head = gh.head(target) try: - try: - method, new_heads[pr] = stage(pr, gh, target, related_prs=(prs - pr)) - _logger.info( - "Staged pr %s to %s by %s: %s -> %s", - pr.display_name, pr.target.name, method, - original_head, new_heads[pr] - ) - except Exception: - # reset the head which failed, as rebase() may have partially - # updated it (despite later steps failing) - gh.set_ref(target, original_head) - # then reset every previous update - for to_revert in new_heads.keys(): - it = staging[to_revert.repository] - it.gh.set_ref('tmp.{}'.format(to_revert.target.name), it.head) - raise + method, new_heads[pr] = stage(pr, info, related_prs=(prs - pr)) + _logger.info( + "Staged pr %s to %s by %s: %s -> %s", + pr.display_name, pr.target.name, method, + info.head, new_heads[pr] + ) except github.MergeError as e: raise exceptions.MergeError(pr) from e except exceptions.Mismatch as e: @@ -419,9 +375,9 @@ def format_for_difflib(items: Iterator[Tuple[str, object]]) -> Iterator[str]: Method = Literal['merge', 'rebase-merge', 'rebase-ff', 'squash'] -def stage(pr: PullRequests, gh: github.GH, target: str, related_prs: PullRequests) -> Tuple[Method, str]: +def stage(pr: PullRequests, info: StagingSlice, related_prs: PullRequests) -> Tuple[Method, str]: # nb: pr_commits is oldest to newest so pr.head is pr_commits[-1] - _, prdict = gh.pr(pr.number) + _, prdict = info.gh.pr(pr.number) commits = prdict['commits'] method: Method = pr.merge_method or ('rebase-ff' if commits == 1 else None) if commits > 50 and method.startswith('rebase'): @@ -431,7 +387,7 @@ def stage(pr: PullRequests, gh: github.GH, target: str, related_prs: PullRequest pr, "Merging PRs of 250 or more commits is not supported " "(https://developer.github.com/v3/pulls/#list-commits-on-a-pull-request)" ) - pr_commits = gh.commits(pr.number) + pr_commits = info.gh.commits(pr.number) for c in pr_commits: if not (c['commit']['author']['email'] and c['commit']['committer']['email']): raise exceptions.Unmergeable( @@ -475,7 +431,7 @@ def stage(pr: PullRequests, gh: github.GH, target: str, related_prs: PullRequest if pr.reviewed_by and pr.reviewed_by.name == pr.reviewed_by.github_login: # XXX: find other trigger(s) to sync github name? - gh_name = gh.user(pr.reviewed_by.github_login)['name'] + gh_name = info.gh.user(pr.reviewed_by.github_login)['name'] if gh_name: pr.reviewed_by.name = gh_name @@ -488,43 +444,46 @@ def stage(pr: PullRequests, gh: github.GH, target: str, related_prs: PullRequest fn = stage_rebase_ff case 'squash': fn = stage_squash - return method, fn(pr, gh, target, pr_commits, related_prs=related_prs) + return method, fn(pr, info, pr_commits, related_prs=related_prs) -def stage_squash(pr: PullRequests, gh: github.GH, target: str, commits: List[github.PrCommit], related_prs: PullRequests) -> str: +def stage_squash(pr: PullRequests, info: StagingSlice, commits: List[github.PrCommit], related_prs: PullRequests) -> str: msg = pr._build_merge_message(pr, related_prs=related_prs) - authorship = {} authors = { (c['commit']['author']['name'], c['commit']['author']['email']) for c in commits } if len(authors) == 1: - name, email = authors.pop() - authorship['author'] = {'name': name, 'email': email} + author = authors.pop() else: msg.headers.extend(sorted( ('Co-Authored-By', "%s <%s>" % author) for author in authors )) + author = (pr.repository.project_id.github_name, pr.repository.project_id.github_email) committers = { (c['commit']['committer']['name'], c['commit']['committer']['email']) for c in commits } - if len(committers) == 1: - name, email = committers.pop() - authorship['committer'] = {'name': name, 'email': email} # should committers also be added to co-authors? - - original_head = gh.head(target) - merge_tree = gh.merge(pr.head, target, 'temp merge')['tree']['sha'] - head = gh('post', 'git/commits', json={ - **authorship, - 'message': str(msg), - 'tree': merge_tree, - 'parents': [original_head], - }).json()['sha'] - gh.set_ref(target, head) + committer = committers.pop() if len(committers) == 1 else None + + r = info.repo.merge_tree(info.head, pr.head) + if r.returncode: + raise exceptions.MergeError(pr, r.stderr) + merge_tree = r.stdout.strip() + + r = info.repo.commit_tree( + tree=merge_tree, + parents=[info.head], + message=str(msg), + author=author, + committer=committer or author, + ) + if r.returncode: + raise exceptions.MergeError(pr, r.stderr) + head = r.stdout.strip() commits_map = {c['sha']: head for c in commits} commits_map[''] = head @@ -532,25 +491,30 @@ def stage_squash(pr: PullRequests, gh: github.GH, target: str, commits: List[git return head -def stage_rebase_ff(pr: PullRequests, gh: github.GH, target: str, commits: List[github.PrCommit], related_prs: PullRequests) -> str: +def stage_rebase_ff(pr: PullRequests, info: StagingSlice, commits: List[github.PrCommit], related_prs: PullRequests) -> str: # updates head commit with PR number (if necessary) then rebases # on top of target msg = pr._build_merge_message(commits[-1]['commit']['message'], related_prs=related_prs) commits[-1]['commit']['message'] = str(msg) add_self_references(pr, commits[:-1]) - head, mapping = gh.rebase(pr.number, target, commits=commits) + head, mapping = info.repo.rebase(info.head, commits=commits) pr.commits_map = json.dumps({**mapping, '': head}) return head -def stage_rebase_merge(pr: PullRequests, gh: github.GH, target: str, commits: List[github.PrCommit], related_prs: PullRequests) -> str : +def stage_rebase_merge(pr: PullRequests, info: StagingSlice, commits: List[github.PrCommit], related_prs: PullRequests) -> str : add_self_references(pr, commits) - h, mapping = gh.rebase(pr.number, target, reset=True, commits=commits) + h, mapping = info.repo.rebase(info.head, commits=commits) msg = pr._build_merge_message(pr, related_prs=related_prs) - merge_head = gh.merge(h, target, str(msg))['sha'] + + project = pr.repository.project_id + merge_head= info.repo.merge( + info.head, h, str(msg), + author=(project.github_name, project.github_email), + ) pr.commits_map = json.dumps({**mapping, '': merge_head}) return merge_head -def stage_merge(pr: PullRequests, gh: github.GH, target: str, commits: List[github.PrCommit], related_prs: PullRequests) -> str: +def stage_merge(pr: PullRequests, info: StagingSlice, commits: List[github.PrCommit], related_prs: PullRequests) -> str: pr_head = commits[-1] # oldest to newest base_commit = None head_parents = {p['sha'] for p in pr_head['parents']} @@ -573,26 +537,37 @@ def stage_merge(pr: PullRequests, gh: github.GH, target: str, commits: List[gith 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}) + t = info.repo.merge_tree(info.head, pr_head['sha']) + if t.returncode: + raise exceptions.MergeError(pr, t.stderr) + merge_tree = t.stdout.strip() + new_parents = [info.head] + list(head_parents - {base_commit}) msg = pr._build_merge_message(pr_head['commit']['message'], related_prs=related_prs) - copy = gh('post', 'git/commits', json={ - 'message': str(msg), - 'tree': merge_tree, - 'author': pr_head['commit']['author'], - 'committer': pr_head['commit']['committer'], - 'parents': new_parents, - }).json() - gh.set_ref(target, copy['sha']) + + d2t = itemgetter('name', 'email', 'date') + c = info.repo.commit_tree( + tree=merge_tree, + parents=new_parents, + message=str(msg), + author=d2t(pr_head['commit']['author']), + committer=d2t(pr_head['commit']['committer']), + ) + if c.returncode: + raise exceptions.MergeError(pr, c.stderr) + copy = c.stdout.strip() + # merge commit *and old PR head* map to the pr head replica - commits_map[''] = commits_map[pr_head['sha']] = copy['sha'] + commits_map[''] = commits_map[pr_head['sha']] = copy pr.commits_map = json.dumps(commits_map) - return copy['sha'] + return copy else: # otherwise do a regular merge msg = pr._build_merge_message(pr) - merge_head = gh.merge(pr.head, target, str(msg))['sha'] + project = pr.repository.project_id + merge_head = info.repo.merge( + info.head, pr.head, str(msg), + author=(project.github_name, project.github_email), + ) # and the merge commit is the normal merge head commits_map[''] = merge_head pr.commits_map = json.dumps(commits_map) @@ -707,10 +682,10 @@ def __setattr__(self, name, value): def __str__(self): if not self.headers: - return self.body + '\n' + return self.body.rstrip() + '\n' - with io.StringIO(self.body) as msg: - msg.write(self.body) + with io.StringIO() as msg: + msg.write(self.body.rstrip()) msg.write('\n\n') # https://git.wiki.kernel.org/index.php/CommitMessageConventions # seems to mostly use capitalised names (rather than title-cased) diff --git a/runbot_merge/tests/test_basic.py b/runbot_merge/tests/test_basic.py index ccc8e36fc..815e6c8ba 100644 --- a/runbot_merge/tests/test_basic.py +++ b/runbot_merge/tests/test_basic.py @@ -1,7 +1,6 @@ import datetime import itertools import json -import textwrap import time from unittest import mock @@ -124,7 +123,7 @@ def test_trivial_flow(env, repo, page, users, config): 'b': 'a second file', } assert master.message == "gibberish\n\nblahblah\n\ncloses {repo.name}#1"\ - "\n\nSigned-off-by: {reviewer.formatted_email}"\ + "\n\nSigned-off-by: {reviewer.formatted_email}\n"\ .format(repo=repo, reviewer=get_partner(env, users['reviewer'])) class TestCommitMessage: @@ -149,7 +148,7 @@ def test_commit_simple(self, env, repo, users, config): master = repo.commit('heads/master') assert master.message == "simple commit message\n\ncloses {repo.name}#1"\ - "\n\nSigned-off-by: {reviewer.formatted_email}"\ + "\n\nSigned-off-by: {reviewer.formatted_email}\n"\ .format(repo=repo, reviewer=get_partner(env, users['reviewer'])) def test_commit_existing(self, env, repo, users, config): @@ -174,7 +173,7 @@ def test_commit_existing(self, env, repo, users, config): master = repo.commit('heads/master') # closes #1 is already present, should not modify message assert master.message == "simple commit message that closes #1"\ - "\n\nSigned-off-by: {reviewer.formatted_email}"\ + "\n\nSigned-off-by: {reviewer.formatted_email}\n"\ .format(reviewer=get_partner(env, users['reviewer'])) def test_commit_other(self, env, repo, users, config): @@ -199,7 +198,7 @@ def test_commit_other(self, env, repo, users, config): master = repo.commit('heads/master') # closes on another repositoy, should modify the commit message assert master.message == "simple commit message that closes odoo/enterprise#1\n\ncloses {repo.name}#1"\ - "\n\nSigned-off-by: {reviewer.formatted_email}"\ + "\n\nSigned-off-by: {reviewer.formatted_email}\n"\ .format(repo=repo, reviewer=get_partner(env, users['reviewer'])) def test_commit_wrong_number(self, env, repo, users, config): @@ -224,7 +223,7 @@ def test_commit_wrong_number(self, env, repo, users, config): master = repo.commit('heads/master') # closes on another repositoy, should modify the commit message assert master.message == "simple commit message that closes #11\n\ncloses {repo.name}#1"\ - "\n\nSigned-off-by: {reviewer.formatted_email}"\ + "\n\nSigned-off-by: {reviewer.formatted_email}\n"\ .format(repo=repo, reviewer=get_partner(env, users['reviewer'])) def test_commit_delegate(self, env, repo, users, config): @@ -254,7 +253,7 @@ def test_commit_delegate(self, env, repo, users, config): master = repo.commit('heads/master') assert master.message == "simple commit message\n\ncloses {repo.name}#1"\ - "\n\nSigned-off-by: {reviewer.formatted_email}"\ + "\n\nSigned-off-by: {reviewer.formatted_email}\n"\ .format(repo=repo, reviewer=get_partner(env, users['other'])) def test_commit_coauthored(self, env, repo, users, config): @@ -292,7 +291,8 @@ def test_commit_coauthored(self, env, repo, users, config): closes {repo.name}#1 Signed-off-by: {reviewer.formatted_email} -Co-authored-by: Bob """.format( +Co-authored-by: Bob +""".format( repo=repo, reviewer=get_partner(env, users['reviewer']) ) @@ -701,9 +701,9 @@ def test_ff_failure_batch(env, repo, users, config): reviewer = get_partner(env, users["reviewer"]).formatted_email assert messages == { 'initial', 'NO!', - part_of('a1', pr_a), part_of('a2', pr_a), f'A\n\ncloses {pr_a.display_name}\n\nSigned-off-by: {reviewer}', - part_of('b1', pr_b), part_of('b2', pr_b), f'B\n\ncloses {pr_b.display_name}\n\nSigned-off-by: {reviewer}', - part_of('c1', pr_c), part_of('c2', pr_c), f'C\n\ncloses {pr_c.display_name}\n\nSigned-off-by: {reviewer}', + part_of('a1', pr_a), part_of('a2', pr_a), f'A\n\ncloses {pr_a.display_name}\n\nSigned-off-by: {reviewer}\n', + part_of('b1', pr_b), part_of('b2', pr_b), f'B\n\ncloses {pr_b.display_name}\n\nSigned-off-by: {reviewer}\n', + part_of('c1', pr_c), part_of('c2', pr_c), f'C\n\ncloses {pr_c.display_name}\n\nSigned-off-by: {reviewer}\n', } class TestPREdition: @@ -1532,7 +1532,7 @@ def test_pr_rebase_merge(self, repo, env, users, config): nb1 = node(part_of('B1', pr_id), node(part_of('B0', pr_id), nm2)) reviewer = get_partner(env, users["reviewer"]).formatted_email merge_head = ( - f'title\n\nbody\n\ncloses {pr_id.display_name}\n\nSigned-off-by: {reviewer}', + f'title\n\nbody\n\ncloses {pr_id.display_name}\n\nSigned-off-by: {reviewer}\n', frozenset([nm2, nb1]) ) assert staging == merge_head @@ -1628,7 +1628,7 @@ def test_pr_rebase_ff(self, repo, env, users, config): # then compare to the dag version of the right graph nm2 = node('M2', node('M1', node('M0'))) reviewer = get_partner(env, users["reviewer"]).formatted_email - nb1 = node(f'B1\n\ncloses {pr_id.display_name}\n\nSigned-off-by: {reviewer}', + nb1 = node(f'B1\n\ncloses {pr_id.display_name}\n\nSigned-off-by: {reviewer}\n', node(part_of('B0', pr_id), nm2)) assert staging == nb1 @@ -1718,7 +1718,7 @@ def test_pr_force_merge_single_commit(self, repo, env, users, config): c0 = node('C0', m) reviewer = get_partner(env, users["reviewer"]).formatted_email expected = node('gibberish\n\nblahblah\n\ncloses {}#{}' - '\n\nSigned-off-by: {}'.format(repo.name, prx.number, reviewer), m, c0) + '\n\nSigned-off-by: {}\n'.format(repo.name, prx.number, reviewer), m, c0) assert log_to_node(repo.log('heads/master')), expected pr = env['runbot_merge.pull_requests'].search([ ('repository.name', '=', repo.name), @@ -1760,7 +1760,7 @@ def test_unrebase_emptymessage(self, repo, env, users, config): c0 = node('C0', m) reviewer = get_partner(env, users["reviewer"]).formatted_email expected = node('gibberish\n\ncloses {}#{}' - '\n\nSigned-off-by: {}'.format(repo.name, prx.number, reviewer), m, c0) + '\n\nSigned-off-by: {}\n'.format(repo.name, prx.number, reviewer), m, c0) assert log_to_node(repo.log('heads/master')), expected @pytest.mark.parametrize('separator', [ @@ -1790,15 +1790,15 @@ def test_pr_message_break(self, repo, env, users, config, separator): env.run_crons() head = repo.commit('heads/master') - assert head.message == textwrap.dedent(f"""\ - title + assert head.message == f"""\ +title - first +first - closes {repo.name}#{pr.number} +closes {repo.name}#{pr.number} - Signed-off-by: {reviewer} - """).strip(), "should not contain the content which follows the thematic break" +Signed-off-by: {reviewer} +""", "should not contain the content which follows the thematic break" def test_pr_message_setex_title(self, repo, env, users, config): """ should not break on a proper SETEX-style title """ @@ -1830,21 +1830,21 @@ def test_pr_message_setex_title(self, repo, env, users, config): env.run_crons() head = repo.commit('heads/master') - assert head.message == textwrap.dedent(f"""\ - title + assert head.message == f"""\ +title - Title - --- - This is some text +Title +--- +This is some text - Title 2 - ------- - This is more text +Title 2 +------- +This is more text - closes {repo.name}#{pr.number} +closes {repo.name}#{pr.number} - Signed-off-by: {reviewer} - """).strip(), "should not break the SETEX titles" +Signed-off-by: {reviewer} +""", "should not break the SETEX titles" def test_rebase_no_edit(self, repo, env, users, config): """ Only the merge messages should be de-breaked @@ -1867,17 +1867,17 @@ def test_rebase_no_edit(self, repo, env, users, config): env.run_crons() head = repo.commit('heads/master') - assert head.message == textwrap.dedent(f"""\ - Commit + assert head.message == f"""\ +Commit - first - *** - second +first +*** +second - closes {repo.name}#{pr.number} +closes {repo.name}#{pr.number} - Signed-off-by: {reviewer} - """).strip(), "squashed / rebased messages should not be stripped" +Signed-off-by: {reviewer} +""", "squashed / rebased messages should not be stripped" def test_title_no_edit(self, repo, env, users, config): """The first line of a commit message should not be taken in account for @@ -1909,13 +1909,15 @@ def test_title_no_edit(self, repo, env, users, config): closes {pr_id.display_name} -Signed-off-by: {reviewer}""" +Signed-off-by: {reviewer} +""" assert repo.commit(staging_head.parents[0]).message == f"""\ Some: thing is odd -Part-of: {pr_id.display_name}""" +Part-of: {pr_id.display_name} +""" def test_pr_mergehead(self, repo, env, config): """ if the head of the PR is a merge commit and one of the parents is @@ -2000,7 +2002,7 @@ def test_pr_mergehead_nonmember(self, repo, env, users, config): m1 = node('M1') reviewer = get_partner(env, users["reviewer"]).formatted_email expected = node( - 'T\n\nTT\n\ncloses {}#{}\n\nSigned-off-by: {}'.format(repo.name, prx.number, reviewer), + 'T\n\nTT\n\ncloses {}#{}\n\nSigned-off-by: {}\n'.format(repo.name, prx.number, reviewer), node('M2', m1), node('C1', node('C0', m1), node('B0', m1)) ) @@ -2076,7 +2078,7 @@ def test_squash_merge(self, repo, env, config, users): closes {pr1_id.display_name} -Signed-off-by: {get_partner(env, users["reviewer"]).formatted_email}\ +Signed-off-by: {get_partner(env, users["reviewer"]).formatted_email} """ assert one['commit']['committer']['name'] == a_user['name'] assert one['commit']['committer']['email'] == a_user['email'] @@ -2107,7 +2109,7 @@ def test_squash_merge(self, repo, env, config, users): Signed-off-by: {get_partner(env, users["reviewer"]).formatted_email} Co-authored-by: {a_user['name']} <{a_user['email']}> -Co-authored-by: {other_user['name']} <{other_user['email']}>\ +Co-authored-by: {other_user['name']} <{other_user['email']}> """ assert repo.read_tree(repo.commit(two['sha'])) == { 'a': '0', @@ -2667,12 +2669,12 @@ def test_staging_batch(self, env, repo, users, config): staging = log_to_node(log) reviewer = get_partner(env, users["reviewer"]).formatted_email p1 = node( - 'title PR1\n\nbody PR1\n\ncloses {}\n\nSigned-off-by: {}'.format(pr1.display_name, reviewer), + 'title PR1\n\nbody PR1\n\ncloses {}\n\nSigned-off-by: {}\n'.format(pr1.display_name, reviewer), node('initial'), node(part_of('commit_PR1_01', pr1), node(part_of('commit_PR1_00', pr1), node('initial'))) ) p2 = node( - 'title PR2\n\nbody PR2\n\ncloses {}\n\nSigned-off-by: {}'.format(pr2.display_name, reviewer), + 'title PR2\n\nbody PR2\n\ncloses {}\n\nSigned-off-by: {}\n'.format(pr2.display_name, reviewer), p1, node(part_of('commit_PR2_01', pr2), node(part_of('commit_PR2_00', pr2), p1)) ) @@ -2707,12 +2709,12 @@ def test_staging_batch_norebase(self, env, repo, users, config): reviewer = get_partner(env, users["reviewer"]).formatted_email p1 = node( - 'title PR1\n\nbody PR1\n\ncloses {}#{}\n\nSigned-off-by: {}'.format(repo.name, pr1.number, reviewer), + 'title PR1\n\nbody PR1\n\ncloses {}#{}\n\nSigned-off-by: {}\n'.format(repo.name, pr1.number, reviewer), node('initial'), node('commit_PR1_01', node('commit_PR1_00', node('initial'))) ) p2 = node( - 'title PR2\n\nbody PR2\n\ncloses {}#{}\n\nSigned-off-by: {}'.format(repo.name, pr2.number, reviewer), + 'title PR2\n\nbody PR2\n\ncloses {}#{}\n\nSigned-off-by: {}\n'.format(repo.name, pr2.number, reviewer), p1, node('commit_PR2_01', node('commit_PR2_00', node('initial'))) ) @@ -2741,8 +2743,8 @@ def test_staging_batch_squash(self, env, repo, users, config): staging = log_to_node(log) reviewer = get_partner(env, users["reviewer"]).formatted_email - expected = node('commit_PR2_00\n\ncloses {}#{}\n\nSigned-off-by: {}'.format(repo.name, pr2.number, reviewer), - node('commit_PR1_00\n\ncloses {}#{}\n\nSigned-off-by: {}'.format(repo.name, pr1.number, reviewer), + expected = node('commit_PR2_00\n\ncloses {}#{}\n\nSigned-off-by: {}\n'.format(repo.name, pr2.number, reviewer), + node('commit_PR1_00\n\ncloses {}#{}\n\nSigned-off-by: {}\n'.format(repo.name, pr1.number, reviewer), node('initial'))) assert staging == expected diff --git a/runbot_merge/tests/test_multirepo.py b/runbot_merge/tests/test_multirepo.py index 576984261..908b7db44 100644 --- a/runbot_merge/tests/test_multirepo.py +++ b/runbot_merge/tests/test_multirepo.py @@ -435,7 +435,7 @@ def test_merge_fail(env, project, repo_a, repo_b, users, config): ) env.run_crons() - s2 = to_pr(env, pr2a) | to_pr(env, pr2b) + pr2a_id, pr2b_id = s2 = to_pr(env, pr2a) | to_pr(env, pr2b) st = env['runbot_merge.stagings'].search([]) assert set(st.batch_ids.prs.ids) == set(s2.ids) @@ -453,12 +453,14 @@ def test_merge_fail(env, project, repo_a, repo_b, users, config): c['commit']['message'] for c in repo_a.log('heads/staging.master') ] == [ - """commit_do-b-thing_00 + f"""\ +commit_do-b-thing_00 -closes %s +closes {pr2a_id.display_name} -Related: %s -Signed-off-by: %s""" % (s2[0].display_name, s2[1].display_name, reviewer), +Related: {pr2b_id.display_name} +Signed-off-by: {reviewer} +""", 'initial' ], "dummy commit + squash-merged PR commit + root commit"