mirror of
https://github.com/odoo/runbot.git
synced 2025-03-15 15:35:46 +07:00
[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.
This commit is contained in:
Xavier Morel
parent
da9e5daf0c
commit
3c88e033f6
@ -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("<p>{}</p>").format(body),
|
||||
r.stdout and Markup("<p>stdout:</p>\n<pre>\n{}</pre>").format(r.stdout),
|
||||
r.stderr and Markup("<p>stderr:</p>\n<pre>\n{}</pre>").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("<pre>{}</pre>").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,
|
||||
|
||||
@ -184,10 +184,7 @@ def test_commit_conflict(env, project, repo, users):
|
||||
(False, '<p>Unstaged direct-application patch created</p>', []),
|
||||
(
|
||||
"Unable to apply patch",
|
||||
"""\
|
||||
<p>Auto-merging b<br>\
|
||||
CONFLICT (content): Merge conflict in b<br></p>\
|
||||
""",
|
||||
"<pre>Auto-merging b\nCONFLICT (content): Merge conflict in b\n</pre>",
|
||||
[],
|
||||
),
|
||||
(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] == [
|
||||
"<p>Unstaged direct-application patch created</p>",
|
||||
"<p>Unable to apply patch: commit 0123456789012345678901234567890123456789 not found.</p>",
|
||||
matches('''\
|
||||
<p>Commit 0123456789012345678901234567890123456789 not found</p>
|
||||
<p>stderr:</p>
|
||||
<pre>
|
||||
error: Unable to find 0123456789012345678901234567890123456789 under $repo$
|
||||
Cannot obtain needed object 0123456789012345678901234567890123456789
|
||||
error: fetch failed.
|
||||
</pre>\
|
||||
'''),
|
||||
]
|
||||
|
||||
def test_apply_udiff(env, project, repo, users):
|
||||
|
||||
Loading…
Reference in New Issue
Block a user