From 3c88e033f647a7ef534996f502c7532123057def Mon Sep 17 00:00:00 2001 From: Xavier Morel Date: Fri, 14 Mar 2025 13:42:02 +0100 Subject: [PATCH] [FIX] runbot_merge: make patcher not rely on branches This was the root cause of the incident of Feb 13/14: because the patcher pushed to the local branch before pushing to the remote failing to push to the remote would leave the local ref broken, as `fetch("refs/heads/*:refs/heads/*")` apparently does not do non-ff updates (which does make some sense I guess). So in this case a staging finished, was pushed to the remote, then git delayed the read side just enough that when the patcher looked up the target it got the old commit. It applied a patch on top of that, tried to push, and got a failure (non-ff update), which led the local and remote branches divergent, and caused any further update of the local reference branches to fail, thus every forward port to be blocked. Using symbolic branches during patching was completely dumb (and updating the local branch unnecessary), so switch the entire thing to using just commits, and update a bunch of error reporting while at it. --- runbot_merge/models/patcher.py | 113 +++++++++++++++++----------- runbot_merge/tests/test_patching.py | 15 ++-- 2 files changed, 79 insertions(+), 49 deletions(-) diff --git a/runbot_merge/models/patcher.py b/runbot_merge/models/patcher.py index d558271f..33218147 100644 --- a/runbot_merge/models/patcher.py +++ b/runbot_merge/models/patcher.py @@ -20,6 +20,8 @@ from email import message_from_string, policy from email.utils import parseaddr from typing import Union +from markupsafe import Markup + from odoo import models, fields, api from odoo.exceptions import ValidationError from odoo.tools.mail import plaintext2html @@ -242,34 +244,49 @@ class Patch(models.Model): if not has_files: raise ValidationError("Patches should have files they patch, found none.") + def _notify(self, subject: str | None, body: str, r: subprocess.CompletedProcess[str]) -> None: + self.message_post( + subject=subject, + body=Markup("\n").join(filter(None, [ + Markup("

{}

").format(body), + r.stdout and Markup("

stdout:

\n
\n{}
").format(r.stdout), + r.stderr and Markup("

stderr:

\n
\n{}
").format(r.stderr), + ])), + ) + def _apply_patches(self, target: Branch) -> bool: patches = self.search([('target', '=', target.id)], order='id asc') selected = len(patches) if not selected: return True - commits = collections.defaultdict(set) - repos = {} - for p in patches: - if p.repository not in repos: - repos[p.repository] = git.get_local(p.repository).check(True) - if p.commit: - commits[p.repository].add(p) + repo_info = { + r: { + 'local': git.get_local(r).check(True).with_config(encoding="utf-8"), + 'with_commit': self.browse(), + 'target_head': None, + } + for r in patches.repository + } + for p in patches.filtered('commit'): + repo_info[p.repository]['with_commit'] |= p - for repo, ps in commits.items(): - r = repos[repo].check(False) + for repo, info in repo_info.items(): + r = info['local'] remote = git.source_url(repo) - if r.fetch( + if r.check(False).fetch( remote, f"+refs/heads/{target.name}:refs/heads/{target.name}", - *(p.commit for p in ps), + *info['with_commit'].mapped('commit'), no_tags=True, ).returncode: r.fetch(remote, f"+refs/heads/{target.name}:refs/heads/{target.name}", no_tags=True) - for p in ps: - if r.fetch(remote, p.commit, no_tags=True).returncode: + for p in info['with_commit']: + if (res := r.check(False).fetch(remote, p.commit, no_tags=True)).returncode: patches -= p - p.message_post(body=f"Unable to apply patch: commit {p.commit} not found.") + p._notify(None, f"Commit {p.commit} not found", res) + info['target_head'] = r.stdout().rev_list('-1', target.name).stdout.strip() + # if some of the commits are not available (yet) schedule a new staging # in case this is a low traffic period (so there might not be staging # triggers every other minute @@ -278,77 +295,88 @@ class Patch(models.Model): for patch in patches: patch.active = False - r = repos[patch.repository] - remote = git.source_url(patch.repository) + info = repo_info[patch.repository] + r = info['local'] + sha = info['target_head'] _logger.info( - "Applying %s to %r (in %s)", + "Applying %s to %s:%r (%s@%s)", patch, + patch.repository.name, patch.target.display_name, patch.repository.name, + sha, ) - info = r.check(True).stdout().with_config(encoding="utf-8") - t = info.show('--no-patch', '--pretty=%T', patch.target.name).stdout.strip() + # this tree should be available locally since we got `sha` from the + # local commit + t = r.get_tree(sha) try: if patch.commit: - c = patch._apply_commit(r) + c = patch._apply_commit(r, sha) else: - c = patch._apply_patch(r) - if t == info.show('--no-patch', '--pretty=%T', c).stdout.strip(): - raise PatchFailure( + c = patch._apply_patch(r, sha) + if t == r.get_tree(c): + raise PatchFailure(Markup( "Patch results in an empty commit when applied, " "it is likely a duplicate of a merged commit." - ) + )) except Exception as e: if isinstance(e, PatchFailure): subject = "Unable to apply patch" else: subject = "Unknown error while trying to apply patch" _logger.error("%s:\n%s", subject, str(e)) - patch.message_post(body=plaintext2html(e), subject=subject) + patch.message_post( + subject=subject, + # hack in order to get a formatted message from line 320 but + # a pre from git + # TODO: do better + body=e.args[0] if isinstance(e.args[0], Markup) else Markup("
{}
").format(e), + ) continue - # `.` is the local "remote", so this updates target to c - r.fetch(".", f"{c}:{target.name}") # push patch by patch, avoids sync issues and in most cases we have 0~1 patches res = r.check(False).stdout()\ .with_config(encoding="utf-8")\ - .push(remote, f"{target.name}:{target.name}") + .push( + git.source_url(patch.repository), + f"{c}:{target.name}", + f"--force-with-lease={target.name}:{sha}", + ) ## one of the repos is out of consistency, loop around to new staging? if res.returncode: + patch._notify(None, f"Unable to push result ({c})", res) _logger.warning( - "Unable to push result of %s\nout:\n%s\nerr:\n%s", - patch.id, - res.stdout, - res.stderr, + "Unable to push result of %s (%s)\nout:\n%s\nerr:\n%s", + patch, c, res.stdout, res.stderr, ) - return False + else: + info['target_head'] = c return True - def _apply_commit(self, r: git.Repo) -> str: + def _apply_commit(self, r: git.Repo, parent: str) -> str: r = r.check(True).stdout().with_config(encoding="utf-8") - sha = r.rev_list('-1', self.target.name).stdout.strip() target = r.show('--no-patch', '--pretty=%an%n%ae%n%ai%n%cn%n%ce%n%ci%n%B', self.commit) # retrieve metadata of cherrypicked commit author_name, author_email, author_date, committer_name, committer_email, committer_date, body =\ target.stdout.strip().split("\n", 6) - res = r.check(False).merge_tree(sha, self.commit) + res = r.check(False).merge_tree(parent, self.commit) if res.returncode: _conflict_info, _, informational = res.stdout.partition('\n\n') raise PatchFailure(informational) return r.commit_tree( tree=res.stdout.strip(), - parents=[sha], + parents=[parent], message=body.strip(), author=(author_name, author_email, author_date), committer=(committer_name, committer_email, committer_date), ).stdout.strip() - def _apply_patch(self, r: git.Repo) -> str: + def _apply_patch(self, r: git.Repo, parent: str) -> str: p = self._parse_patch() def reader(_r, f): return pathlib.Path(tmpdir, f).read_text(encoding="utf-8") @@ -364,7 +392,7 @@ class Patch(models.Model): read.add(m['file_from']) patched[m['file_to']] = reader - archiver = r.stdout(True) + archiver = r.stdout(True).with_config(encoding=None) # if the parent is checked then we can't get rid of the kwarg and popen doesn't support it archiver._config.pop('check', None) archiver.runner = subprocess.Popen @@ -373,7 +401,7 @@ class Patch(models.Model): # if there's no file to *update*, `archive` will extract the entire # tree which is unnecessary if read: - out = stack.enter_context(archiver.archive(self.target.name, *read)) + out = stack.enter_context(archiver.archive(parent, *read)) tf = stack.enter_context(tarfile.open(fileobj=out.stdout, mode='r|')) tf.extractall(tmpdir) patch = subprocess.run( @@ -387,12 +415,9 @@ class Patch(models.Model): raise PatchFailure("\n---------\n".join(filter(None, [p.patch, patch.stdout.strip(), patch.stderr.strip()]))) new_tree = r.update_tree(self.target.name, patched) - sha = r.stdout().with_config(encoding='utf-8')\ - .rev_list('-1', self.target.name)\ - .stdout.strip() return r.commit_tree( tree=new_tree, - parents=[sha], + parents=[parent], message=p.message, author=p.author, committer=p.committer, diff --git a/runbot_merge/tests/test_patching.py b/runbot_merge/tests/test_patching.py index 6867f2cf..daae982f 100644 --- a/runbot_merge/tests/test_patching.py +++ b/runbot_merge/tests/test_patching.py @@ -184,10 +184,7 @@ def test_commit_conflict(env, project, repo, users): (False, '

Unstaged direct-application patch created

', []), ( "Unable to apply patch", - """\ -

Auto-merging b
\ -CONFLICT (content): Merge conflict in b

\ -""", + "
Auto-merging b\nCONFLICT (content): Merge conflict in b\n
", [], ), (False, '', [('active', 1, 0)]), @@ -222,7 +219,15 @@ def test_apply_not_found(env, project, repo, users): assert p2.active assert p2.message_ids.mapped('body')[::-1] == [ "

Unstaged direct-application patch created

", - "

Unable to apply patch: commit 0123456789012345678901234567890123456789 not found.

", + matches('''\ +

Commit 0123456789012345678901234567890123456789 not found

+

stderr:

+
+error: Unable to find 0123456789012345678901234567890123456789 under $repo$
+Cannot obtain needed object 0123456789012345678901234567890123456789
+error: fetch failed.
+
\ +'''), ] def test_apply_udiff(env, project, repo, users):