From 5441ba12ae62bf634e59beb55607f64635c8631e Mon Sep 17 00:00:00 2001 From: Xavier Morel Date: Mon, 18 Nov 2024 12:37:44 +0100 Subject: [PATCH] [FIX] runbot_merge: format_patch if `--no-prefix` Turns out you can configure format-patch with `--no-prefix` and some people (*cough cough* mat) have that in their standard setup, so the assumption of needing to strip 1 level of prefix does not necessarily hold. Also fix a few more issues: - some people (*cough cough* still mat) also use `-n` by default, which adds the series sequence (`n/m`) even for a single patch, handle that correctly - logging patch application errors is pretty useful when patching fails and I'm trying to get the information via logs, do that - especially when I decide to add error messages to tracking *but forgot to show the chatter by default*, fix that as well The commit-based patcher worked first try, and patch-based would have worked too if not for those meddling kids. In the future it might be a good idea to reify the stripping level (`-p`) on the patch object though, and maybe provide a computed preview of the list of files to patch, so issues are easier for the operator to diagnose. --- runbot_merge/models/patcher.py | 29 ++++++---- runbot_merge/tests/test_patching.py | 84 +++++++++++++++++++++-------- runbot_merge/views/queues.xml | 4 ++ 3 files changed, 85 insertions(+), 32 deletions(-) diff --git a/runbot_merge/models/patcher.py b/runbot_merge/models/patcher.py index f007db0f..199dd02d 100644 --- a/runbot_merge/models/patcher.py +++ b/runbot_merge/models/patcher.py @@ -28,8 +28,8 @@ _logger = logging.getLogger(__name__) FILE_PATTERN = re.compile(r""" # paths with spaces don't work well as the path can be followed by a timestamp # (in an unspecified format?) ----\x20a/(?P\S+)(:?\s.*)?\n -\+\+\+\x20b/(?P\S+)(:?\s.*)?\n +---\x20(?Pa/)?(?P\S+)(:?\s.*)?\n +\+\+\+\x20(?Pb/)?(?P\S+)(:?\s.*)?\n @@\x20-(\d+(,\d+)?)\x20\+(\d+(,\d+)?)\x20@@ # trailing garbage """, re.VERBOSE) @@ -98,7 +98,7 @@ def parse_format_patch(p: Patch) -> ParseResult: name, email = parseaddr(m['from']) author = (name, email, m['date']) - msg = m['subject'].removeprefix('[PATCH] ') + msg = re.sub(r'^\[PATCH( \d+/\d+)?\] ', '', m['subject']) body, _, rest = m.get_content().partition('---\n') if body: msg += '\n\n' + body @@ -225,12 +225,16 @@ class Patch(models.Model): patch.repository.name, ) try: - c = patch._apply_commit(r) if patch.commit else patch._apply_patch(r) + if patch.commit: + c = patch._apply_commit(r) + else: + c = patch._apply_patch(r) 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) continue # `.` is the local "remote", so this updates target to c @@ -276,10 +280,17 @@ class Patch(models.Model): def _apply_patch(self, r: git.Repo) -> str: p = self._parse_patch() - files = dict.fromkeys( - (m['file_to'] for m in FILE_PATTERN.finditer(p.patch)), - lambda _r, f: pathlib.Path(tmpdir, f).read_text(encoding="utf-8"), - ) + files = {} + def reader(_r, f): + return pathlib.Path(tmpdir, f).read_text(encoding="utf-8") + + prefix = 0 + for m in FILE_PATTERN.finditer(p.patch): + if not prefix and m['prefix_a'] and m['prefix_b']: + prefix = 1 + + files[m['file_to']] = reader + archiver = r.stdout(True) # 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) @@ -289,7 +300,7 @@ class Patch(models.Model): tempfile.TemporaryDirectory() as tmpdir: tf.extractall(tmpdir) patch = subprocess.run( - ['patch', '-p1', '-d', tmpdir], + ['patch', f'-p{prefix}', '-d', tmpdir], input=p.patch, stdout=subprocess.PIPE, stderr=subprocess.PIPE, diff --git a/runbot_merge/tests/test_patching.py b/runbot_merge/tests/test_patching.py index 2774599a..020fbae4 100644 --- a/runbot_merge/tests/test_patching.py +++ b/runbot_merge/tests/test_patching.py @@ -23,6 +23,51 @@ index 000000000000..000000000000 100644 +2 """ +FORMAT_PATCH_XMO = """\ +From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001 +From: 3 Discos Down +Date: Sat, 24 Apr 2021 17:09:14 +0000 +Subject: [PATCH] [I18N] whop + +whop whop +--- + b | 2 +- + 1 file changed, 1 insertion(+), 1 deletion(-) + +diff --git a/b b/b +index 000000000000..000000000000 100644 +--- a/b ++++ b/b +@@ -1,1 +1,1 @@ +-1 ++2 +-- +2.46.2 +""" + +# slightly different format than the one I got, possibly because older? +FORMAT_PATCH_MAT = """\ +From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001 +From: 3 Discos Down +Date: Sat, 24 Apr 2021 17:09:14 +0000 +Subject: [PATCH 1/1] [I18N] whop + +whop whop +--- + b | 2 +- + 1 file changed, 1 insertion(+), 1 deletion(-) + +diff --git b b +index 000000000000..000000000000 100644 +--- b ++++ b +@@ -1,1 +1,1 @@ +-1 ++2 +-- +2.34.1 +""" + @pytest.fixture(autouse=True) def _setup(repo): @@ -148,41 +193,34 @@ def test_apply_udiff(env, project, repo, users): assert not p.active -def test_apply_format_patch(env, project, repo, users): +@pytest.mark.parametrize('patch', [ + pytest.param(FORMAT_PATCH_XMO, id='xmo'), + pytest.param(FORMAT_PATCH_MAT, id='mat'), +]) +def test_apply_format_patch(env, project, repo, users, patch): p = env['runbot_merge.patch'].create({ 'target': project.branch_ids.id, 'repository': project.repo_ids.id, - 'patch': """\ -From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001 -From: 3 Discos Down -Date: Sat, 24 Apr 2021 17:09:14 +0000 -Subject: [PATCH] whop - -whop whop ---- - b | 2 +- - 1 file changed, 1 insertion(+), 1 deletion(-) - -diff --git a/b b/b -index 000000000000..000000000000 100644 ---- a/b -+++ b/b -@@ -1,1 +1,1 @@ --1 -+2 --- -2.46.2 -""", + 'patch': patch, }) env.run_crons() + bot = env['res.users'].browse((1,)) + assert p.message_ids[::-1].mapped(lambda m: ( + m.author_id.display_name, + m.body, + list(map(read_tracking_value, m.tracking_value_ids)), + )) == [ + (p.create_uid.partner_id.display_name, '

Unstaged direct-application patch created

', []), + (bot.partner_id.display_name, "", [('active', 1, 0)]), + ] HEAD = repo.commit('master') assert repo.read_tree(HEAD) == { 'a': '2', 'b': '2\n', } - assert HEAD.message == "whop\n\nwhop whop" + assert HEAD.message == "[I18N] whop\n\nwhop whop" assert HEAD.author['name'] == "3 Discos Down" assert HEAD.author['email'] == "bar@example.org" assert not p.active diff --git a/runbot_merge/views/queues.xml b/runbot_merge/views/queues.xml index e18abaa1..6bec187e 100644 --- a/runbot_merge/views/queues.xml +++ b/runbot_merge/views/queues.xml @@ -143,6 +143,10 @@ +
+ + +