diff --git a/conftest.py b/conftest.py index a244f2af..3d936e56 100644 --- a/conftest.py +++ b/conftest.py @@ -537,6 +537,9 @@ class Repo: 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 b87a2345..fae0d208 100644 --- a/mergebot_test_utils/utils.py +++ b/mergebot_test_utils/utils.py @@ -8,7 +8,8 @@ MESSAGE_TEMPLATE = """{message} 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 91297f9e..09061933 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 @@ class Repo: ) 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 b00342dc..544712a7 100644 --- a/runbot_merge/models/stagings_create.py +++ b/runbot_merge/models/stagings_create.py @@ -6,10 +6,9 @@ import json 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 werkzeug.datastructures import Headers 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 @@ For-Commit-Id: {it.head} '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 @@ For-Commit-Id: {it.head} 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 @@ UNCHECKABLE = ['merge_method', 'overrides', 'draft'] 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? + committer = committers.pop() if len(committers) == 1 else None - 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) + 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 @@ class Message: 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 ccc8e36f..815e6c8b 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 @@ class TestCommitMessage: 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 @@ class TestCommitMessage: 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 @@ class TestCommitMessage: 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 @@ class TestCommitMessage: 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 @@ class TestCommitMessage: 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 @@ Fixes a thing 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 @@ commits, I need to know how to merge it: 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 @@ commits, I need to know how to merge it: # 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 @@ commits, I need to know how to merge it: 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 @@ commits, I need to know how to merge it: 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 @@ commits, I need to know how to merge it: 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 @@ removed 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 @@ removed 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 @@ thing: thong 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 @@ Part-of: {pr_id.display_name}""" 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 @@ Part-of: {pr_id.display_name}""" 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 @@ closes {pr2_id.display_name} 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 @@ class TestBatching(object): 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 @@ class TestBatching(object): 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 @@ class TestBatching(object): 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 57698426..908b7db4 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"