From b1d3278de197e998458a3f8a6c83fcb56a623c9f Mon Sep 17 00:00:00 2001 From: Xavier Morel Date: Fri, 5 Jul 2024 13:32:02 +0200 Subject: [PATCH] [CHG] forwardport: perform forward porting without working copies The goal is to reduce maintenance and odd disk interactions & concurrency issues, by not creating concurrent clones, not having to push forks back in the repository, etc... it also removes the need to cleanup "scratch" working copies though that looks not to have been an issue in a while. The work is done on isolated objects without using or mutating refs, so even concurrent work should not be a problem. This turns out to not be any more verbose (less so if anything) than using `cherry-pick`, as that is not really designed for scripted / non-interactive use, or for squashing commits thereafter. Working directly with trees and commits is quite a bit cleaner even without a ton of helpers. Much of the credit goes to Julia Evans for [their investigation of 3-way merges as the underpinnings of cherry-picking][3-way merge], this would have been a lot more difficult if I'd had to rediscover the merge-base trick independently. A few things have been changed by this: - The old trace/stderr from cherrypick has disappeared as it's generated by cherrypick, but for a non-interactive use it's kinda useless anyway so I probably should have looked into removing it earlier (I think the main use was investigation of the inflateinit issue). - Error on emptied commits has to be hand-rolled as `merge-tree` couldn't care less, this is not hard but is a bit annoying. - `merge-tree`'s conflict information only references raw commits, which makes sense, but requires updating a bunch of tests. Then again so does the fact that it *usually* doesn't send anything to stderr, so that's usually disappearing. Conveniently `merge-tree` merges the conflict marker directly in the files / tree so we don't have to mess about moving them back out of the repository and into the working copy as I assume cherry-pick does, which means we don't have to try and commit them back in ether. That is a huge part of the gain over faffing about with the working copy. Fixes #847 [3-way merge]: https://jvns.ca/blog/2023/11/10/how-cherry-pick-and-revert-work/ --- forwardport/models/forwardport.py | 177 +++++++------- forwardport/models/project.py | 216 ++++++++---------- forwardport/tests/test_batches.py | 9 +- forwardport/tests/test_conflicts.py | 15 +- forwardport/tests/test_updates.py | 25 +- runbot_merge/git.py | 17 +- runbot_merge/models/batch.py | 11 +- runbot_merge/models/crons/git_maintenance.py | 2 +- .../models/project_freeze/__init__.py | 14 +- runbot_merge/models/stagings_create.py | 6 +- 10 files changed, 220 insertions(+), 272 deletions(-) diff --git a/forwardport/models/forwardport.py b/forwardport/models/forwardport.py index 54521621..a0604da7 100644 --- a/forwardport/models/forwardport.py +++ b/forwardport/models/forwardport.py @@ -161,94 +161,94 @@ class ForwardPortTasks(models.Model, Queue): # only then create the PR objects # TODO: maybe do that after making forward-port WC-less, so all the # branches can be pushed atomically at once - with contextlib.ExitStack() as s: - for descendant in self.batch_id.descendants(): - target = pr._find_next_target() - if target is None: - _logger.info("Will not forward-port %s: no next target", pr.display_name) - return + for descendant in self.batch_id.descendants(): + target = pr._find_next_target() + if target is None: + _logger.info("Will not forward-port %s: no next target", pr.display_name) + return - if PullRequests.search_count([ - ('source_id', '=', source.id), - ('target', '=', target.id), - ('state', 'not in', ('closed', 'merged')), - ]): - _logger.warning("Will not forward-port %s: already ported", pr.display_name) - return + if PullRequests.search_count([ + ('source_id', '=', source.id), + ('target', '=', target.id), + ('state', 'not in', ('closed', 'merged')), + ]): + _logger.warning("Will not forward-port %s: already ported", pr.display_name) + return - if target != descendant.target: - self.env['runbot_merge.pull_requests.feedback'].create({ - 'repository': repository.id, - 'pull_request': source.id, - 'token_field': 'fp_github_token', - 'message': """\ + if target != descendant.target: + self.env['runbot_merge.pull_requests.feedback'].create({ + 'repository': repository.id, + 'pull_request': source.id, + 'token_field': 'fp_github_token', + 'message': """\ {pr.ping}unable to port this PR forwards due to inconsistency: goes from \ {pr.target.name} to {next_target.name} but {batch} ({batch_prs}) targets \ {batch.target.name}. """.format(pr=pr, next_target=target, batch=descendant, batch_prs=', '.join(descendant.mapped('prs.display_name'))) - }) - return - - ref = descendant.prs[:1].refname - # NOTE: ports the new source everywhere instead of porting each - # PR to the next step as it does not *stop* on conflict - conflict, working_copy = source._create_fp_branch(target, ref, s) - working_copy.push('target', ref) - - remote_target = repository.fp_remote_target - owner, _ = remote_target.split('/', 1) - message = source.message + f"\n\nForward-Port-Of: {pr.display_name}" - - title, body = re.match(r'(?P[^\n]+)\n*(?P<body>.*)', message, flags=re.DOTALL).groups() - r = gh.post(f'https://api.github.com/repos/{pr.repository.name}/pulls', json={ - 'base': target.name, - 'head': f'{owner}:{ref}', - 'title': '[FW]' + (' ' if title[0] != '[' else '') + title, - 'body': body }) - if not r.ok: - _logger.warning("Failed to create forward-port PR for %s, deleting branches", pr.display_name) - # delete all the branches this should automatically close the - # PRs if we've created any. Using the API here is probably - # simpler than going through the working copies - d = gh.delete(f'https://api.github.com/repos/{remote_target}/git/refs/heads/{ref}') - if d.ok: - _logger.info("Deleting %s:%s=success", remote_target, ref) - else: - _logger.warning("Deleting %s:%s=%s", remote_target, ref, d.text) - raise RuntimeError(f"Forwardport failure: {pr.display_name} ({r.text})") + return - new_pr = PullRequests._from_gh(r.json()) - _logger.info("Created forward-port PR %s", new_pr) - new_pr.write({ - 'batch_id': descendant.id, # should already be set correctly but... - 'merge_method': pr.merge_method, - 'source_id': source.id, - # only link to previous PR of sequence if cherrypick passed - # FIXME: apply parenting of siblings? Apply parenting *to* siblings? - 'parent_id': pr.id if not conflict else False, - 'detach_reason': "{1}\n{2}".format(*conflict).strip() if conflict else None, - }) + ref = descendant.prs[:1].refname + # NOTE: ports the new source everywhere instead of porting each + # PR to the next step as it does not *stop* on conflict + repo = git.get_local(source.repository) + conflict, head = source._create_fp_branch(repo, target) + repo.push(git.fw_url(pr.repository), f'{head}:refs/heads/{ref}') - if conflict: - self.env.ref('runbot_merge.forwardport.failure.conflict')._send( - repository=pr.repository, - pull_request=pr.number, - token_field='fp_github_token', - format_args={'source': source, 'pr': pr, 'new': new_pr, 'footer': FOOTER}, - ) - new_pr._fp_conflict_feedback(pr, {pr: conflict}) + remote_target = repository.fp_remote_target + owner, _ = remote_target.split('/', 1) + message = source.message + f"\n\nForward-Port-Of: {pr.display_name}" - labels = ['forwardport'] - if conflict: - labels.append('conflict') - self.env['runbot_merge.pull_requests.tagging'].create({ - 'repository': new_pr.repository.id, - 'pull_request': new_pr.number, - 'tags_add': labels, - }) + title, body = re.match(r'(?P<title>[^\n]+)\n*(?P<body>.*)', message, flags=re.DOTALL).groups() + r = gh.post(f'https://api.github.com/repos/{pr.repository.name}/pulls', json={ + 'base': target.name, + 'head': f'{owner}:{ref}', + 'title': '[FW]' + (' ' if title[0] != '[' else '') + title, + 'body': body + }) + if not r.ok: + _logger.warning("Failed to create forward-port PR for %s, deleting branches", pr.display_name) + # delete all the branches this should automatically close the + # PRs if we've created any. Using the API here is probably + # simpler than going through the working copies + d = gh.delete(f'https://api.github.com/repos/{remote_target}/git/refs/heads/{ref}') + if d.ok: + _logger.info("Deleting %s:%s=success", remote_target, ref) + else: + _logger.warning("Deleting %s:%s=%s", remote_target, ref, d.text) + raise RuntimeError(f"Forwardport failure: {pr.display_name} ({r.text})") - pr = new_pr + new_pr = PullRequests._from_gh(r.json()) + _logger.info("Created forward-port PR %s", new_pr) + new_pr.write({ + 'batch_id': descendant.id, # should already be set correctly but... + 'merge_method': pr.merge_method, + 'source_id': source.id, + # only link to previous PR of sequence if cherrypick passed + # FIXME: apply parenting of siblings? Apply parenting *to* siblings? + 'parent_id': pr.id if not conflict else False, + 'detach_reason': "{1}\n{2}".format(*conflict).strip() if conflict else None, + }) + + if conflict: + self.env.ref('runbot_merge.forwardport.failure.conflict')._send( + repository=pr.repository, + pull_request=pr.number, + token_field='fp_github_token', + format_args={'source': source, 'pr': pr, 'new': new_pr, 'footer': FOOTER}, + ) + new_pr._fp_conflict_feedback(pr, {pr: conflict}) + + labels = ['forwardport'] + if conflict: + labels.append('conflict') + self.env['runbot_merge.pull_requests.tagging'].create({ + 'repository': new_pr.repository.id, + 'pull_request': new_pr.number, + 'tags_add': labels, + }) + + pr = new_pr class UpdateQueue(models.Model, Queue): _name = 'forwardport.updates' @@ -286,8 +286,9 @@ class UpdateQueue(models.Model, Queue): ) return - conflicts, working_copy = previous._create_fp_branch( - child.target, child.refname, s) + repo = git.get_local(previous.repository) + conflicts, new_head = previous._create_fp_branch(repo, child.target) + if conflicts: _, out, err, _ = conflicts self.env.ref('runbot_merge.forwardport.updates.conflict.parent')._send( @@ -308,9 +309,8 @@ class UpdateQueue(models.Model, Queue): }, ) - new_head = working_copy.stdout().rev_parse(child.refname).stdout.decode().strip() - commits_count = int(working_copy.stdout().rev_list( - f'{child.target.name}..{child.refname}', + commits_count = int(repo.stdout().rev_list( + f'{child.target.name}..{new_head}', count=True ).stdout.decode().strip()) old_head = child.head @@ -320,16 +320,11 @@ class UpdateQueue(models.Model, Queue): # 'state': 'opened', 'squash': commits_count == 1, }) - # push the new head to the local cache: in some cases github - # doesn't propagate revisions fast enough so on the next loop we - # can't find the revision we just pushed - dummy_branch = str(uuid.uuid4()) - ref = git.get_local(previous.repository, 'fp_github') - working_copy.push(ref._directory, f'{new_head}:refs/heads/{dummy_branch}') - ref.branch('--delete', '--force', dummy_branch) # then update the child's branch to the new head - working_copy.push(f'--force-with-lease={child.refname}:{old_head}', - 'target', child.refname) + repo.push( + f'--force-with-lease={child.refname}:{old_head}', + git.fw_url(child.repository), + f"{new_head}:refs/heads/{child.refname}") # committing here means github could technically trigger its # webhook before sending a response, but committing before diff --git a/forwardport/models/project.py b/forwardport/models/project.py index 5c664c25..31e0ec9c 100644 --- a/forwardport/models/project.py +++ b/forwardport/models/project.py @@ -19,9 +19,7 @@ import json import logging import operator import subprocess -import tempfile import typing -from pathlib import Path import dateutil.relativedelta import requests @@ -30,7 +28,6 @@ from odoo import models, fields, api from odoo.exceptions import UserError from odoo.osv import expression from odoo.tools.misc import topological_sort, groupby -from odoo.tools.appdirs import user_cache_dir from odoo.addons.base.models.res_partner import Partner from odoo.addons.runbot_merge import git, utils from odoo.addons.runbot_merge.models.pull_requests import Branch @@ -289,17 +286,11 @@ class PullRequests(models.Model): return sorted(commits, key=lambda c: idx[c['sha']]) - def _create_fp_branch(self, target_branch, fp_branch_name, cleanup): + def _create_fp_branch(self, source, target_branch): """ Creates a forward-port for the current PR to ``target_branch`` under ``fp_branch_name``. :param target_branch: the branch to port forward to - :param fp_branch_name: the name of the branch to create the FP under - :param ExitStack cleanup: so the working directories can be cleaned up - :return: A pair of an optional conflict information and a repository. If - present the conflict information is composed of the hash of the - conflicting commit, the stderr and stdout of the failed - cherrypick and a list of all PR commit hashes :rtype: (None | (str, str, str, list[commit]), Repo) """ logger = _logger.getChild(str(self.id)) @@ -308,104 +299,52 @@ class PullRequests(models.Model): "Forward-porting %s (%s) to %s", self.display_name, root.display_name, target_branch.name ) - source = git.get_local(self.repository, 'fp_github') - r = source.with_config(stdout=subprocess.PIPE, stderr=subprocess.STDOUT).fetch() - logger.info("Updated cache repo %s:\n%s", source._directory, r.stdout.decode()) + tree = source.with_config(stdout=subprocess.PIPE, stderr=subprocess.STDOUT).fetch() + logger.info("Updated cache repo %s:\n%s", source._directory, tree.stdout.decode()) - logger.info("Create working copy...") - cache_dir = user_cache_dir('forwardport') - # PullRequest.display_name is `owner/repo#number`, so `owner` becomes a - # directory, `TemporaryDirectory` only creates the leaf, so we need to - # make sure `owner` exists in `cache_dir`. - Path(cache_dir, root.repository.name).parent.mkdir(parents=True, exist_ok=True) - working_copy = source.clone( - cleanup.enter_context( - tempfile.TemporaryDirectory( - prefix=f'{root.display_name}-to-{target_branch.name}', - dir=cache_dir)), - branch=target_branch.name - ) - - r = working_copy.with_config(stdout=subprocess.PIPE, stderr=subprocess.STDOUT) \ - .fetch(git.source_url(self.repository, 'fp_github'), root.head) + tree = source.with_config(stdout=subprocess.PIPE, stderr=subprocess.STDOUT) \ + .fetch(git.source_url(self.repository), root.head) logger.info( - "Fetched head of %s into %s:\n%s", + "Fetched head of %s (%s):\n%s", root.display_name, - working_copy._directory, - r.stdout.decode() + root.head, + tree.stdout.decode() ) - if working_copy.check(False).cat_file(e=root.head).returncode: + if source.check(False).cat_file(e=root.head).returncode: raise ForwardPortError( f"During forward port of {self.display_name}, unable to find " f"expected head of {root.display_name} ({root.head})" ) - project_id = self.repository.project_id - # add target remote - working_copy.remote( - 'add', 'target', - 'https://{p.fp_github_token}@github.com/{r.fp_remote_target}'.format( - r=self.repository, - p=project_id - ) - ) - logger.info("Create FP branch %s in %s", fp_branch_name, working_copy._directory) - working_copy.checkout(b=fp_branch_name) - try: - root._cherry_pick(working_copy) - return None, working_copy + return None, root._cherry_pick(source, target_branch.name) except CherrypickError as e: h, out, err, commits = e.args - # using git diff | git apply -3 to get the entire conflict set - # turns out to not work correctly: in case files have been moved - # / removed (which turns out to be a common source of conflicts - # when forward-porting) it'll just do nothing to the working copy - # so the "conflict commit" will be empty - # switch to a squashed-pr branch - working_copy.check(True).checkout('-bsquashed', root.head) # commits returns oldest first, so youngest (head) last head_commit = commits[-1]['commit'] to_tuple = operator.itemgetter('name', 'email') - def to_dict(term, vals): - return {'GIT_%s_NAME' % term: vals[0], 'GIT_%s_EMAIL' % term: vals[1], 'GIT_%s_DATE' % term: vals[2]} authors, committers = set(), set() - for c in (c['commit'] for c in commits): - authors.add(to_tuple(c['author'])) - committers.add(to_tuple(c['committer'])) - fp_authorship = (project_id.fp_github_name, '', '') + for commit in (c['commit'] for c in commits): + authors.add(to_tuple(commit['author'])) + committers.add(to_tuple(commit['committer'])) + fp_authorship = (self.repository.project_id.fp_github_name, '', '') author = fp_authorship if len(authors) != 1 \ else authors.pop() + (head_commit['author']['date'],) committer = fp_authorship if len(committers) != 1 \ else committers.pop() + (head_commit['committer']['date'],) - conf = working_copy.with_config(env={ - **to_dict('AUTHOR', author), - **to_dict('COMMITTER', committer), - 'GIT_COMMITTER_DATE': '', - }) - # squash to a single commit - conf.reset('--soft', commits[0]['parents'][0]['sha']) - conf.commit(a=True, message="temp") - squashed = conf.stdout().rev_parse('HEAD').stdout.strip().decode() - - # switch back to the PR branch - conf.checkout(fp_branch_name) - # cherry-pick the squashed commit to generate the conflict - conf.with_params( + conf = source.with_params( 'merge.renamelimit=0', 'merge.renames=copies', 'merge.conflictstyle=zdiff3' - ) \ - .with_config(check=False) \ - .cherry_pick(squashed, no_commit=True, strategy="ort") - status = conf.stdout().status(short=True, untracked_files='no').stdout.decode() - if err.strip(): - err = err.rstrip() + '\n----------\nstatus:\n' + status - else: - err = 'status:\n' + status + ).with_config(stdout=subprocess.PIPE, stderr=subprocess.PIPE) + tree = conf.with_config(check=False).merge_tree( + '--merge-base', commits[0]['parents'][0]['sha'], + target_branch.name, + root.head, + ) # if there was a single commit, reuse its message when committing # the conflict if len(commits) == 1: @@ -421,77 +360,107 @@ stderr: {err} """ - conf.with_config(input=str(msg).encode()) \ - .commit(all=True, allow_empty=True, file='-') + target_head = source.stdout().rev_parse(target_branch.name).stdout.decode().strip() + commit = conf.commit_tree( + tree=tree.stdout.decode().splitlines(keepends=False)[0], + parents=[target_head], + message=str(msg), + author=author, + committer=committer[:2], + ) + assert commit.returncode == 0,\ + f"commit failed\n\n{commit.stdout.decode()}\n\n{commit.stderr.decode}" + hh = commit.stdout.strip() - return (h, out, err, [c['sha'] for c in commits]), working_copy + return (h, out, err, [c['sha'] for c in commits]), hh - def _cherry_pick(self, working_copy): - """ Cherrypicks ``self`` into the working copy + def _cherry_pick(self, repo: git.Repo, branch: Branch) -> str: + """ Cherrypicks ``self`` into ``branch`` - :return: ``True`` if the cherrypick was successful, ``False`` otherwise + :return: the HEAD of the forward-port is successful + :raises CherrypickError: in case of conflict """ # <xxx>.cherrypick.<number> logger = _logger.getChild(str(self.id)).getChild('cherrypick') - # original head so we can reset - original_head = working_copy.stdout().rev_parse('HEAD').stdout.decode().strip() + # target's head + head = repo.stdout().rev_parse(branch).stdout.decode().strip() commits = self.commits() - logger.info("%s: copy %s commits to %s\n%s", self, len(commits), original_head, '\n'.join( - '- %s (%s)' % (c['sha'], c['commit']['message'].splitlines()[0]) - for c in commits - )) + logger.info( + "%s: copy %s commits to %s (%s)%s", + self, len(commits), branch, head, ''.join( + '\n- %s: %s' % (c['sha'], c['commit']['message'].splitlines()[0]) + for c in commits + ) + ) - conf_base = working_copy.with_params( + conf = repo.with_params( 'merge.renamelimit=0', 'merge.renames=copies', ).with_config( stdout=subprocess.PIPE, stderr=subprocess.PIPE, - check=False + check=False, ) for commit in commits: commit_sha = commit['sha'] - # config (global -c) or commit options don't really give access to - # setting dates - cm = commit['commit'] # get the "git" commit object rather than the "github" commit resource - env = { - 'GIT_AUTHOR_NAME': cm['author']['name'], - 'GIT_AUTHOR_EMAIL': cm['author']['email'], - 'GIT_AUTHOR_DATE': cm['author']['date'], - 'GIT_COMMITTER_NAME': cm['committer']['name'], - 'GIT_COMMITTER_EMAIL': cm['committer']['email'], - } - configured = working_copy.with_config(env=env) + # merge-tree is a bit stupid and gets confused when the options + # follow the parameters + r = conf.merge_tree('--merge-base', commit['parents'][0]['sha'], head, commit_sha) + new_tree = r.stdout.decode().splitlines(keepends=False)[0] + if r.returncode: + # For merge-tree the stdout on conflict is of the form + # + # oid of toplevel tree + # conflicted file info+ + # + # informational messages+ + # + # to match cherrypick we only want the informational messages, + # so strip everything else + r.stdout = r.stdout.split(b'\n\n')[-1] + else: + # By default cherry-pick fails if a non-empty commit becomes + # empty (--empty=stop), also it fails when cherrypicking already + # empty commits which I didn't think we prevented but clearly we + # do...? + parent_tree = conf.rev_parse(f'{head}^{{tree}}').stdout.decode().strip() + if parent_tree == new_tree: + r.returncode = 1 + r.stdout = f"You are currently cherry-picking commit {commit_sha}.".encode() + r.stderr = b"The previous cherry-pick is now empty, possibly due to conflict resolution." - conf = conf_base.with_config(env={**env, 'GIT_TRACE': 'true'}) - # first try with default / low renamelimit - r = conf.cherry_pick(commit_sha, strategy="ort") logger.debug("Cherry-picked %s: %s\n%s\n%s", commit_sha, r.returncode, r.stdout.decode(), _clean_rename(r.stderr.decode())) - - if r.returncode: # pick failed, reset and bail + if r.returncode: # pick failed, bail # try to log inflateInit: out of memory errors as warning, they - # seem to return the status code 128 + # seem to return the status code 128 (nb: may not work anymore + # with merge-tree, idk) logger.log( logging.WARNING if r.returncode == 128 else logging.INFO, "forward-port of %s (%s) failed at %s", self, self.display_name, commit_sha) - configured.reset('--hard', original_head) + raise CherrypickError( commit_sha, r.stdout.decode(), _clean_rename(r.stderr.decode()), commits ) + # get the "git" commit object rather than the "github" commit resource + cc = conf.commit_tree( + tree=new_tree, + parents=[head], + message=str(self._make_fp_message(commit)), + author=map_author(commit['commit']['author']), + committer=map_committer(commit['commit']['committer']), + ) + if cc.returncode: + raise CherrypickError(commit_sha, cc.stdout.decode(), cc.stderr.decode(), commits) - msg = self._make_fp_message(commit) + head = cc.stdout.strip() + logger.info('%s -> %s', commit_sha, head) - # replace existing commit message with massaged one - configured \ - .with_config(input=str(msg).encode())\ - .commit(amend=True, file='-') - result = configured.stdout().rev_parse('HEAD').stdout.decode() - logger.info('%s: success -> %s', commit_sha, result) + return head def _make_fp_message(self, commit): cmap = json.loads(self.commits_map) @@ -541,6 +510,11 @@ stderr: format_args={'pr': pr, 'source': source}, ) + +map_author = operator.itemgetter('name', 'email', 'date') +map_committer = operator.itemgetter('name', 'email') + + class Stagings(models.Model): _inherit = 'runbot_merge.stagings' diff --git a/forwardport/tests/test_batches.py b/forwardport/tests/test_batches.py index b0b7155a..2ecab8b6 100644 --- a/forwardport/tests/test_batches.py +++ b/forwardport/tests/test_batches.py @@ -381,7 +381,7 @@ def test_add_to_forward_port_conflict(env, config, make_repo, users): assert pr2_c.comments == [ seen(env, pr2_c, users), # should have conflicts - (users['user'], matches("""@{user} cherrypicking of pull request {previous.display_name} failed. + (users['user'], """@{user} cherrypicking of pull request {previous.display_name} failed. stdout: ``` @@ -390,11 +390,6 @@ CONFLICT (add/add): Merge conflict in b ``` -stderr: -``` -$$ -``` - Either perform the forward-port manually (and push to this branch, proceeding as usual) or close this PR (maybe?). In the former case, you may want to edit this PR message as well. @@ -402,5 +397,5 @@ In the former case, you may want to edit this PR message as well. :warning: after resolving this conflict, you will need to merge it via @{project.github_prefix}. More info at https://github.com/odoo/odoo/wiki/Mergebot#forward-port -""".format(project=project, previous=pr2_b_id, **users))) +""".format(project=project, previous=pr2_b_id, **users)) ] diff --git a/forwardport/tests/test_conflicts.py b/forwardport/tests/test_conflicts.py index 030e534b..7011b1b3 100644 --- a/forwardport/tests/test_conflicts.py +++ b/forwardport/tests/test_conflicts.py @@ -62,17 +62,17 @@ def test_conflict(env, config, make_repo, users): assert prod.read_tree(c) == { 'f': 'c', 'g': 'a', - 'h': matches('''<<<\x3c<<< HEAD + 'h': matches('''<<<\x3c<<< $$ a -||||||| parent of $$ (temp) +||||||| $$ ======= xxx ->>>\x3e>>> $$ (temp) +>>>\x3e>>> $$ '''), } assert prc.comments == [ seen(env, prc, users), - (users['user'], matches( + (users['user'], f'''@{users['user']} @{users['reviewer']} cherrypicking of pull request {pra_id.display_name} failed. stdout: @@ -82,11 +82,6 @@ CONFLICT (add/add): Merge conflict in h ``` -stderr: -``` -$$ -``` - Either perform the forward-port manually (and push to this branch, proceeding as usual) or close this PR (maybe?). In the former case, you may want to edit this PR message as well. @@ -94,7 +89,7 @@ In the former case, you may want to edit this PR message as well. :warning: after resolving this conflict, you will need to merge it via @{project.github_prefix}. More info at https://github.com/odoo/odoo/wiki/Mergebot#forward-port -''')) +''') ] prb = prod.get_pr(prb_id.number) diff --git a/forwardport/tests/test_updates.py b/forwardport/tests/test_updates.py index d68c6c92..b13cd16a 100644 --- a/forwardport/tests/test_updates.py +++ b/forwardport/tests/test_updates.py @@ -445,12 +445,12 @@ def test_subsequent_conflict(env, make_repo, config, users): assert repo.read_tree(repo.commit(pr3_id.head)) == { 'f': 'c', 'g': 'a', - 'h': matches('''<<<\x3c<<< HEAD + 'h': matches('''<<<\x3c<<< $$ a -||||||| parent of $$ (temp) +||||||| $$ ======= conflict! ->>>\x3e>>> $$ (temp) +>>>\x3e>>> $$ '''), 'x': '0', } @@ -470,7 +470,7 @@ conflict! # 1. link to status page # 2. forward-port chain thing assert repo.get_pr(pr3_id.number).comments[2:] == [ - (users['user'], matches(f'''\ + (users['user'], f'''\ @{users['user']} @{users['reviewer']} WARNING: the update of {pr2_id.display_name} to {pr2_id.head} has caused a \ conflict in this pull request, data may have been lost. @@ -478,20 +478,5 @@ stdout: ``` Auto-merging h CONFLICT (add/add): Merge conflict in h -``` - -stderr: -``` -$$ trace: built-in: git cherry-pick {pr2_id.head} --strategy ort -error: could not apply {pr2_id.head[:7]}... newfiles -hint: After resolving the conflicts, mark them with -hint: "git add/rm <pathspec>", then run -hint: "git cherry-pick --continue". -hint: You can instead skip this commit with "git cherry-pick --skip". -hint: To abort and get back to the state before "git cherry-pick", -hint: run "git cherry-pick --abort". -hint: Disable this message with "git config advice.mergeConflict false" ----------- -status: -```''')) +```'''), ] diff --git a/runbot_merge/git.py b/runbot_merge/git.py index f44c1cd2..caaa6e5d 100644 --- a/runbot_merge/git.py +++ b/runbot_merge/git.py @@ -13,16 +13,21 @@ from .github import MergeError, PrCommit _logger = logging.getLogger(__name__) - -def source_url(repository, prefix: str) -> str: +def source_url(repository) -> str: return 'https://{}@github.com/{}'.format( - repository.project_id[f'{prefix}_token'], + repository.project_id.github_token, repository.name, ) +def fw_url(repository) -> str: + return 'https://{}@github.com/{}'.format( + repository.project_id.fp_github_token, + repository.fp_remote_target, + ) + Authorship = Union[Tuple[str, str], Tuple[str, str, str]] -def get_local(repository, prefix: Optional[str]) -> 'Optional[Repo]': +def get_local(repository, *, clone: bool = True) -> 'Optional[Repo]': repos_dir = pathlib.Path(user_cache_dir('mergebot')) repos_dir.mkdir(parents=True, exist_ok=True) # NB: `repository.name` is `$org/$name` so this will be a subdirectory, probably @@ -30,9 +35,9 @@ def get_local(repository, prefix: Optional[str]) -> 'Optional[Repo]': if repo_dir.is_dir(): return git(repo_dir) - elif prefix: + elif clone: _logger.info("Cloning out %s to %s", repository.name, repo_dir) - subprocess.run(['git', 'clone', '--bare', source_url(repository, prefix), str(repo_dir)], check=True) + subprocess.run(['git', 'clone', '--bare', source_url(repository), str(repo_dir)], check=True) # bare repos don't have fetch specs by default, and fetching *into* # them is a pain in the ass, configure fetch specs so `git fetch` # works properly diff --git a/runbot_merge/models/batch.py b/runbot_merge/models/batch.py index c85eb4f7..de70d67b 100644 --- a/runbot_merge/models/batch.py +++ b/runbot_merge/models/batch.py @@ -13,7 +13,7 @@ from psycopg2 import sql from odoo import models, fields, api from .utils import enum - +from .. import git _logger = logging.getLogger(__name__) FOOTER = '\nMore info at https://github.com/odoo/odoo/wiki/Mergebot#forward-port\n' @@ -330,12 +330,11 @@ class Batch(models.Model): base64.urlsafe_b64encode(os.urandom(3)).decode() ) conflicts = {} - with contextlib.ExitStack() as s: - for pr in prs: - conflicts[pr], working_copy = pr._create_fp_branch( - target, new_branch, s) + for pr in prs: + repo = git.get_local(pr.repository) + conflicts[pr], head = pr._create_fp_branch(repo, target) - working_copy.push('target', new_branch) + repo.push(git.fw_url(pr.repository), f"{head}:refs/heads/{new_branch}") gh = requests.Session() gh.headers['Authorization'] = 'token %s' % proj.fp_github_token diff --git a/runbot_merge/models/crons/git_maintenance.py b/runbot_merge/models/crons/git_maintenance.py index 2b0e2182..29dd028a 100644 --- a/runbot_merge/models/crons/git_maintenance.py +++ b/runbot_merge/models/crons/git_maintenance.py @@ -24,7 +24,7 @@ class GC(models.TransientModel): # run on all repos with a forwardport target (~ forwardport enabled) for repo in self.env['runbot_merge.repository'].search([]): - repo_git = get_local(repo, prefix=None) + repo_git = get_local(repo, clone=False) if not repo_git: continue diff --git a/runbot_merge/models/project_freeze/__init__.py b/runbot_merge/models/project_freeze/__init__.py index 3c645969..0a39ceae 100644 --- a/runbot_merge/models/project_freeze/__init__.py +++ b/runbot_merge/models/project_freeze/__init__.py @@ -213,15 +213,15 @@ class FreezeWizard(models.Model): gh_sessions = {r: r.github() for r in self.project_id.repo_ids} repos: Dict[Repository, git.Repo] = { - r: git.get_local(r, 'github').check(False) + r: git.get_local(r).check(False) for r in self.project_id.repo_ids } for repo, copy in repos.items(): - copy.fetch(git.source_url(repo, 'github'), '+refs/heads/*:refs/heads/*') + copy.fetch(git.source_url(repo), '+refs/heads/*:refs/heads/*') all_prs = self.release_pr_ids.pr_id | self.bump_pr_ids.pr_id for pr in all_prs: repos[pr.repository].fetch( - git.source_url(pr.repository, 'github'), + git.source_url(pr.repository), pr.head, ) @@ -282,7 +282,7 @@ class FreezeWizard(models.Model): repo_id = rel.repository_id if repos[repo_id].push( - git.source_url(repo_id, 'github'), + git.source_url(repo_id), f'{rel_heads[repo_id]}:refs/heads/{self.branch_name}', ).returncode: failure = ('create', repo_id.name, self.branch_name) @@ -294,7 +294,7 @@ class FreezeWizard(models.Model): for bump in self.bump_pr_ids: repo_id = bump.repository_id if repos[repo_id].push( - git.source_url(repo_id, 'github'), + git.source_url(repo_id), f'{bump_heads[repo_id]}:refs/heads/{master_name}' ).returncode: failure = ('fast-forward', repo_id.name, master_name) @@ -310,7 +310,7 @@ class FreezeWizard(models.Model): for prev_id in to_revert: if repos[prev_id].push( '-f', - git.source_url(prev_id, 'github'), + git.source_url(prev_id), f'{prevs[prev_id]}:refs/heads/{master_name}', ).returncode: failures.append(prev_id.name) @@ -323,7 +323,7 @@ class FreezeWizard(models.Model): for prev_id in to_delete: if repos[prev_id].push( - git.source_url(prev_id, 'github'), + git.source_url(prev_id), f':refs/heads/{self.branch_name}' ).returncode: failures.append(prev_id.name) diff --git a/runbot_merge/models/stagings_create.py b/runbot_merge/models/stagings_create.py index ddc9c3d4..2005416b 100644 --- a/runbot_merge/models/stagings_create.py +++ b/runbot_merge/models/stagings_create.py @@ -177,7 +177,7 @@ For-Commit-Id: {it.head} ) it.repo.stdout(False).check(True).push( '-f', - git.source_url(repo, 'github'), + git.source_url(repo), f'{it.head}:refs/heads/staging.{branch.name}', ) @@ -231,9 +231,9 @@ def staging_setup( gh = repo.github() head = gh.head(target.name) - source = git.get_local(repo, 'github') + source = git.get_local(repo) source.fetch( - git.source_url(repo, 'github'), + git.source_url(repo), # a full refspec is necessary to ensure we actually fetch the ref # (not just the commit it points to) and update it. # `git fetch $remote $branch` seems to work locally, but it might