From 3b8d392548263d94e74fa6f8b4c3c9fa25ba6e90 Mon Sep 17 00:00:00 2001
From: Xavier Morel <xmo@odoo.com>
Date: Fri, 23 Aug 2024 11:38:22 +0200
Subject: [PATCH 01/45] [FIX] pytest warnings

Backport of d7a78f89d0877e1688f999e1d188fc519e5049d5

- `choice` is not a proper type
- markers should be declared
---
 conftest.py | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/conftest.py b/conftest.py
index 1357fdf3..539cf272 100644
--- a/conftest.py
+++ b/conftest.py
@@ -86,7 +86,7 @@ def pytest_addoption(parser):
     parser.addoption('--coverage', action='store_true')
 
     parser.addoption(
-        '--tunnel', action="store", type="choice", choices=['', 'ngrok', 'localtunnel'], default='',
+        '--tunnel', action="store", choices=['', 'ngrok', 'localtunnel'], default='',
         help="Which tunneling method to use to expose the local Odoo server "
              "to hook up github's webhook. ngrok is more reliable, but "
              "creating a free account is necessary to avoid rate-limiting "
@@ -104,6 +104,11 @@ def pytest_configure(config):
         "markers",
         "expect_log_errors(reason): allow and require tracebacks in the log",
     )
+    config.addinivalue_line(
+        "markers",
+        "defaultstatuses: use the statuses `default` rather than `ci/runbot,legal/cla`",
+    )
+
 
 def pytest_unconfigure(config):
     if not is_manager(config):

From 1d106f552d46722129d68477b626a0a1aeb3494c Mon Sep 17 00:00:00 2001
From: Xavier Morel <xmo@odoo.com>
Date: Thu, 5 Sep 2024 13:02:10 +0200
Subject: [PATCH 02/45] [FIX] runbot_merge: missing feedback on fw r+

In some cases, feedback to the PR author that an r+ is redundant went
missing.

This turns out to be due to the convolution of the handling of
approval on forward-port, and the fact that the target PR is treated
exactly like its ancestors: if the PR is already approved the approval
is not even attempted (and so no feedback if it's incorrect).

Straighten up this bit and add a special case for the PR being
commented on, it should have the usual feedback if in error or already
commented on.

Furthermore, update `PullRequests._pr_acl` to kinda work out of the
box for forward-port: if the current PR is a forward port,
`is_reviewer` should check delegation on all ancestors, there doesn't
seem to be any reason to split "source_reviewer", "parent_reviewer",
and "is_reviewer".

Fixes #939
---
 forwardport/tests/test_simple.py     |  1 +
 forwardport/tests/test_weird.py      | 58 +++++++++++++++++++
 runbot_merge/models/commands.py      | 10 +++-
 runbot_merge/models/pull_requests.py | 86 ++++++++++------------------
 4 files changed, 98 insertions(+), 57 deletions(-)

diff --git a/forwardport/tests/test_simple.py b/forwardport/tests/test_simple.py
index 634a25c0..187a1688 100644
--- a/forwardport/tests/test_simple.py
+++ b/forwardport/tests/test_simple.py
@@ -692,6 +692,7 @@ More info at https://github.com/odoo/odoo/wiki/Mergebot#forward-port
 '''.format_map(users)),
         (users['other'], 'hansen r+')
     ]
+    assert pr2_id.reviewed_by
 
 
 def test_redundant_approval(env, config, make_repo, users):
diff --git a/forwardport/tests/test_weird.py b/forwardport/tests/test_weird.py
index 19feda24..e2fd04a7 100644
--- a/forwardport/tests/test_weird.py
+++ b/forwardport/tests/test_weird.py
@@ -136,6 +136,64 @@ def test_failed_staging(env, config, make_repo):
     assert pr3_id.state == 'ready'
     assert pr3_id.staging_id
 
+def test_fw_retry(env, config, make_repo, users):
+    prod, _ = make_basic(env, config, make_repo, statuses='default')
+    other_token = config['role_other']['token']
+    fork = prod.fork(token=other_token)
+    with prod, fork:
+        fork.make_commits('a', Commit('c', tree={'a': '0'}), ref='heads/abranch')
+        pr1 = prod.make_pr(
+            title="whatever",
+            target='a',
+            head=f'{fork.owner}:abranch',
+            token=other_token,
+        )
+        prod.post_status(pr1.head, 'success')
+        pr1.post_comment('hansen r+', config['role_reviewer']['token'])
+    env.run_crons()
+
+    other_partner = env['res.partner'].search([('github_login', '=', users['other'])])
+    assert len(other_partner) == 1
+    other_partner.email = "foo@example.com"
+
+    with prod:
+        prod.post_status('staging.a', 'success')
+    env.run_crons()
+
+    _pr1_id, pr2_id = env['runbot_merge.pull_requests'].search([], order='number')
+    pr2 = prod.get_pr(pr2_id.number)
+    with prod:
+        prod.post_status(pr2_id.head, 'success')
+        pr2.post_comment('hansen r+', other_token)
+    env.run_crons()
+    assert not pr2_id.blocked
+
+    with prod:
+        prod.post_status('staging.b', 'failure')
+    env.run_crons()
+
+    assert pr2_id.error
+    with prod:
+        pr2.post_comment('hansen r+', other_token)
+    env.run_crons()
+    assert pr2_id.state == 'error'
+    with prod:
+        pr2.post_comment('hansen retry', other_token)
+    env.run_crons()
+    assert pr2_id.state == 'ready'
+
+    assert pr2.comments == [
+        seen(env, pr2, users),
+        (users['user'], "This PR targets b and is part of the forward-port chain. Further PRs will be created up to c.\n\nMore info at https://github.com/odoo/odoo/wiki/Mergebot#forward-port\n"),
+        (users['other'], 'hansen r+'),
+        (users['user'], "@{other} @{reviewer} staging failed: default".format_map(users)),
+
+        (users['other'], 'hansen r+'),
+        (users['user'], "This PR is already reviewed, it's in error, you might want to `retry` it instead (if you have already confirmed the error is not legitimate)."),
+
+        (users['other'], 'hansen retry'),
+    ]
+
 class TestNotAllBranches:
     """ Check that forward-ports don't behave completely insanely when not all
     branches are supported on all repositories.
diff --git a/runbot_merge/models/commands.py b/runbot_merge/models/commands.py
index fdee31c6..9e3c2963 100644
--- a/runbot_merge/models/commands.py
+++ b/runbot_merge/models/commands.py
@@ -72,6 +72,14 @@ class Approve:
             return f"r={ids}"
         return 'review+'
 
+    def __contains__(self, item):
+        if self.ids is None:
+            return True
+        return item in self.ids
+
+    def fmt(self):
+        return ", ".join(f"#{n:d}" for n in (self.ids or ()))
+
     @classmethod
     def help(cls, _: bool) -> Iterator[Tuple[str, str]]:
         yield "r(eview)+", "approves the PR, if it's a forwardport also approves all non-detached parents"
@@ -184,7 +192,7 @@ class FW(enum.Enum):
     DEFAULT = enum.auto()
     NO = enum.auto()
     SKIPCI = enum.auto()
-    SKIPMERGE = enum.auto()
+    # SKIPMERGE = enum.auto()
 
     def __str__(self) -> str:
         return f'fw={self.name.lower()}'
diff --git a/runbot_merge/models/pull_requests.py b/runbot_merge/models/pull_requests.py
index c2b243bb..cb7670e2 100644
--- a/runbot_merge/models/pull_requests.py
+++ b/runbot_merge/models/pull_requests.py
@@ -679,14 +679,14 @@ class PullRequests(models.Model):
             })
 
         is_admin, is_reviewer, is_author = self._pr_acl(author)
-        _source_admin, source_reviewer, source_author = self.source_id._pr_acl(author)
+        source_author = self.source_id._pr_acl(author).is_author
         # nota: 15.0 `has_group` completely doesn't work if the recordset is empty
         super_admin = is_admin and author.user_ids and author.user_ids.has_group('runbot_merge.group_admin')
 
         help_list: list[type(commands.Command)] = list(filter(None, [
             commands.Help,
 
-            (self.source_id and (source_author or source_reviewer) or is_reviewer) and not self.reviewed_by and commands.Approve,
+            (is_reviewer or (self.source_id and source_author)) and not self.reviewed_by and commands.Approve,
             (is_author or source_author) and self.reviewed_by and commands.Reject,
             (is_author or source_author) and self.error and commands.Retry,
 
@@ -768,55 +768,21 @@ For your own safety I've ignored *everything in your entire comment*.
             match command:
                 case commands.Approve() if self.draft:
                     msg = "draft PRs can not be approved."
-                case commands.Approve() if self.source_id:
-                    # rules are a touch different for forwardport PRs:
-                    valid = lambda _: True if command.ids is None else lambda n: n in command.ids
-                    _, source_reviewer, source_author = self.source_id._pr_acl(author)
-
-                    ancestors = list(self._iter_ancestors())
-                    # - reviewers on the original can approve any forward port
-                    if source_reviewer:
-                        approveable = ancestors
-                    elif source_author:
-                        # give full review rights on all forwardports (attached
-                        # or not) to original author
-                        approveable = ancestors
+                case commands.Approve() if self.source_id and (source_author or is_reviewer):
+                    if selected := [p for p in self._iter_ancestors() if p.number in command]:
+                        for pr in selected:
+                            # ignore already reviewed PRs, unless it's the one
+                            # being r+'d, this means ancestors in error will not
+                            # be warned about
+                            if pr == self or not pr.reviewed_by:
+                                pr._approve(author, login)
                     else:
-                        # between the first merged ancestor and self
-                        mergeors = list(itertools.dropwhile(
-                            lambda p: p.state != 'merged',
-                            reversed(ancestors),
-                        ))
-                        # between the first ancestor the current user can review and self
-                        reviewors = list(itertools.dropwhile(
-                            lambda p: not p._pr_acl(author).is_reviewer,
-                            reversed(ancestors),
-                        ))
-
-                        # source author can approve any descendant of a merged
-                        # forward port (or source), people with review rights
-                        # to a forward port have review rights to its
-                        # descendants, if both apply use the most favorable
-                        # (largest number of PRs)
-                        if source_author and len(mergeors) > len(reviewors):
-                            approveable = mergeors
-                        else:
-                            approveable = reviewors
-
-                    if approveable:
-                        for pr in approveable:
-                            if not (pr.state in RPLUS and valid(pr.number)):
-                                continue
-                            msg = pr._approve(author, login)
-                            if msg:
-                                break
-                    else:
-                        msg = f"you can't {command} you silly little bean."
+                        msg = f"tried to approve PRs {command.fmt()} but no such PR is an ancestors of {self.number}"
                 case commands.Approve() if is_reviewer:
-                    if command.ids is not None and command.ids != [self.number]:
-                        msg = f"tried to approve PRs {command.ids} but the current PR is {self.number}"
-                    else:
+                    if command.ids is None or command.ids == [self.number]:
                         msg = self._approve(author, login)
+                    else:
+                        msg = f"tried to approve PRs {command.fmt()} but the current PR is {self.number}"
                 case commands.Reject() if is_author or source_author:
                     if self.batch_id.skipchecks or self.reviewed_by:
                         if self.error:
@@ -916,10 +882,10 @@ For your own safety I've ignored *everything in your entire comment*.
                             message = "Disabled forward-porting."
                         case commands.FW.DEFAULT if is_author or source_author:
                             message = "Waiting for CI to create followup forward-ports."
-                        case commands.FW.SKIPCI if is_reviewer or source_reviewer:
+                        case commands.FW.SKIPCI if is_reviewer:
                             message = "Not waiting for CI to create followup forward-ports."
-                        case commands.FW.SKIPMERGE if is_reviewer or source_reviewer:
-                            message = "Not waiting for merge to create followup forward-ports."
+                        # case commands.FW.SKIPMERGE if is_reviewer:
+                        #     message = "Not waiting for merge to create followup forward-ports."
                         case _:
                             msg = f"you don't have the right to {command}."
 
@@ -1142,7 +1108,7 @@ For your own safety I've ignored *everything in your entire comment*.
             kw['bodies'] = {p.id: html_escape(message) for p in self}
         return super()._message_log_batch(**kw)
 
-    def _pr_acl(self, user):
+    def _pr_acl(self, user) -> ACL:
         if not self:
             return ACL(False, False, False)
 
@@ -1151,10 +1117,18 @@ For your own safety I've ignored *everything in your entire comment*.
             ('repository_id', '=', self.repository.id),
             ('review', '=', True) if self.author != user else ('self_review', '=', True),
         ]) == 1
-        is_reviewer = is_admin or self in user.delegate_reviewer
-        # TODO: should delegate reviewers be able to retry PRs?
-        is_author = is_reviewer or self.author == user
-        return ACL(is_admin, is_reviewer, is_author)
+        if is_admin:
+            return ACL(True, True, True)
+
+        # delegate on source = delegate on PR
+        if self.source_id and self.source_id in user.delegate_reviewer:
+            return ACL(False, True, True)
+        # delegate on any ancestors ~ delegate on PR (maybe should be any descendant of source?)
+        if any(p in user.delegate_reviewer for p in self._iter_ancestors()):
+            return ACL(False, True, True)
+
+        # user is probably always False on a forward port
+        return ACL(False, False, self.author == user)
 
     def _validate(self, statuses):
         # could have two PRs (e.g. one open and one closed) at least

From 017b8d4bb04588d5760de21a149b1fa0e38b2126 Mon Sep 17 00:00:00 2001
From: Xavier Morel <xmo@odoo.com>
Date: Fri, 6 Sep 2024 12:33:51 +0200
Subject: [PATCH 03/45] [FIX] forwardport: don't post reminder on PRs waiting
 to be staged

The FW reminder is useful to remind people of the outstanding forward
ports they need to process, as taking too long can be an issue.

They are, however, not useful if the developer has already done
everything, the PR is ready and unblocked, but the mergebot has fallen
behind and has a hard time catching up. In that case there is nothing
for the developer to do, so pinging them is not productive, it's only
frustrating.

Fixes #930
---
 forwardport/models/project.py    |  9 +++----
 forwardport/tests/test_simple.py | 43 +++++++++++++++++++++-----------
 2 files changed, 32 insertions(+), 20 deletions(-)

diff --git a/forwardport/models/project.py b/forwardport/models/project.py
index 72630453..c3d4806d 100644
--- a/forwardport/models/project.py
+++ b/forwardport/models/project.py
@@ -484,6 +484,8 @@ stderr:
             ('source_id', '!=', False),
             # active
             ('state', 'not in', ['merged', 'closed']),
+            # not ready
+            ('blocked', '!=', False),
             ('source_id.merge_date', '<', cutoff),
         ], order='source_id, id'), lambda p: p.source_id)
 
@@ -498,11 +500,8 @@ stderr:
                 continue
             source.reminder_backoff_factor += 1
 
-            # only keep the PRs which don't have an attached descendant)
-            pr_ids = {p.id for p in prs}
-            for pr in prs:
-                pr_ids.discard(pr.parent_id.id)
-            for pr in (p for p in prs if p.id in pr_ids):
+            # only keep the PRs which don't have an attached descendant
+            for pr in set(prs).difference(p.parent_id for p in prs):
                 self.env.ref('runbot_merge.forwardport.reminder')._send(
                     repository=pr.repository,
                     pull_request=pr.number,
diff --git a/forwardport/tests/test_simple.py b/forwardport/tests/test_simple.py
index 187a1688..0c4260c2 100644
--- a/forwardport/tests/test_simple.py
+++ b/forwardport/tests/test_simple.py
@@ -251,7 +251,7 @@ def test_empty(env, config, make_repo, users):
     """ Cherrypick of an already cherrypicked (or separately implemented)
     commit -> conflicting pr.
     """
-    prod, other = make_basic(env, config, make_repo)
+    prod, other = make_basic(env, config, make_repo, statuses="default")
     # merge change to b
     with prod:
         [p_0] = prod.make_commits(
@@ -259,13 +259,11 @@ def test_empty(env, config, make_repo, users):
             ref='heads/early'
         )
         pr0 = prod.make_pr(target='b', head='early')
-        prod.post_status(p_0, 'success', 'legal/cla')
-        prod.post_status(p_0, 'success', 'ci/runbot')
+        prod.post_status(p_0, 'success')
         pr0.post_comment('hansen r+', config['role_reviewer']['token'])
     env.run_crons()
     with prod:
-        prod.post_status('staging.b', 'success', 'legal/cla')
-        prod.post_status('staging.b', 'success', 'ci/runbot')
+        prod.post_status('staging.b', 'success')
 
     # merge same change to a afterwards
     with prod:
@@ -274,15 +272,13 @@ def test_empty(env, config, make_repo, users):
             ref='heads/late'
         )
         pr1 = prod.make_pr(target='a', head='late')
-        prod.post_status(p_1, 'success', 'legal/cla')
-        prod.post_status(p_1, 'success', 'ci/runbot')
+        prod.post_status(p_1, 'success')
         pr1.post_comment('hansen r+', config['role_reviewer']['token'])
     env.run_crons()
     with prod:
-        prod.post_status('staging.a', 'success', 'legal/cla')
-        prod.post_status('staging.a', 'success', 'ci/runbot')
-
+        prod.post_status('staging.a', 'success')
     env.run_crons()
+
     assert prod.read_tree(prod.commit('a')) == {
         'f': 'e',
         'x': '0',
@@ -315,7 +311,8 @@ def test_empty(env, config, make_repo, users):
     }
 
     with prod:
-        validate_all([prod], [fp_id.head, fail_id.head])
+        prod.post_status(fp_id.head, 'success')
+        prod.post_status(fail_id.head, 'success')
     env.run_crons()
 
     # should not have created any new PR
@@ -379,12 +376,28 @@ More info at https://github.com/odoo/odoo/wiki/Mergebot#forward-port
         (users['reviewer'], 'hansen r+'),
         seen(env, pr1, users),
     ]
-    assert fail_pr.comments == [
-        seen(env, fail_pr, users),
-        conflict,
+    assert fail_pr.comments[2:] == [awaiting]*2,\
+        "message should not be triggered on closed PR"
+
+    with prod:
+        fail_pr.open()
+    with prod:
+        prod.post_status(fail_id.head, 'success')
+    assert fail_id.state == 'validated'
+    env.run_crons('forwardport.reminder', context={'forwardport_updated_before': FAKE_PREV_WEEK})
+    assert fail_pr.comments[2:] == [awaiting]*3, "check that message triggers again"
+
+    with prod:
+        fail_pr.post_comment('hansen r+', config['role_reviewer']['token'])
+    assert fail_id.state == 'ready'
+    env.run_crons('forwardport.reminder', context={'forwardport_updated_before': FAKE_PREV_WEEK})
+    assert fail_pr.comments[2:] == [
         awaiting,
         awaiting,
-    ], "each cron run should trigger a new message"
+        awaiting,
+        (users['reviewer'], "hansen r+"),
+    ],"if a PR is ready (unblocked), the reminder should not trigger as "\
+        "we're likely waiting for the mergebot"
 
 def test_partially_empty(env, config, make_repo):
     """ Check what happens when only some commits of the PR are now empty

From 64f9dcbc228e73f2d0acbf39c475db47672874ce Mon Sep 17 00:00:00 2001
From: Xavier Morel <xmo@odoo.com>
Date: Fri, 6 Sep 2024 13:04:13 +0200
Subject: [PATCH 04/45] [FIX] *: unnecessary warning on r- of forward port

Reminding users that `r-` on a forward port only unreviews *that*
forwardport is useful, as `r+;r-` is not a no-op (all preceding
siblings are still reviewed).

However it is useless if all siblings are not approved or already
merged. So avoid sending the warning in that case.

Fixes #934
---
 forwardport/tests/test_simple.py     | 8 ++++++++
 runbot_merge/models/pull_requests.py | 8 +++++---
 2 files changed, 13 insertions(+), 3 deletions(-)

diff --git a/forwardport/tests/test_simple.py b/forwardport/tests/test_simple.py
index 0c4260c2..7ec2601c 100644
--- a/forwardport/tests/test_simple.py
+++ b/forwardport/tests/test_simple.py
@@ -629,6 +629,14 @@ def test_disapproval(env, config, make_repo, users):
                         "sibling forward ports may have to be unapproved "
                         "individually."),
     ]
+    with prod:
+        pr2.post_comment('hansen r-', token=config['role_other']['token'])
+    env.run_crons(None)
+    assert pr2_id.state == 'validated'
+    assert pr2.comments[2:] == [
+        (users['other'], "hansen r+"),
+        (users['other'], "hansen r-"),
+    ], "shouldn't have a warning on r- because it's the only approved PR of the chain"
 
 def test_delegate_fw(env, config, make_repo, users):
     """If a user is delegated *on a forward port* they should be able to approve
diff --git a/runbot_merge/models/pull_requests.py b/runbot_merge/models/pull_requests.py
index cb7670e2..4bb2edd3 100644
--- a/runbot_merge/models/pull_requests.py
+++ b/runbot_merge/models/pull_requests.py
@@ -796,10 +796,12 @@ For your own safety I've ignored *everything in your entire comment*.
                                 pull_request=self.number,
                                 format_args={'user': login, 'pr': self},
                             )
-                        if self.source_id:
+                        if self.source_id.forwardport_ids.filtered(
+                            lambda p: p.reviewed_by and p.state not in ('merged', 'closed')
+                        ):
                             feedback("Note that only this forward-port has been"
-                                     " unapproved, sibling forward ports may "
-                                     "have to be unapproved individually.")
+                                     " unapproved, sibling forward ports may"
+                                     " have to be unapproved individually.")
                         self.unstage("unreviewed (r-) by %s", login)
                     else:
                         msg = "r- makes no sense in the current PR state."

From 146564a90a7c4ee6bc111380886e2e1ec1dabe7b Mon Sep 17 00:00:00 2001
From: Xavier Morel <xmo@odoo.com>
Date: Fri, 6 Sep 2024 13:16:37 +0200
Subject: [PATCH 05/45] [FIX] runbot_merge: set staging_end on all terminations

Rather than only setting `staging_end` on status change, set it when
the staging gets deactivated. This way cancelling a staging (whether
explicitely or via a PR update) will also end it, so will a staging
timeout, etc..., rather than keep the counter running.

Fixes #931
---
 runbot_merge/models/pull_requests.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/runbot_merge/models/pull_requests.py b/runbot_merge/models/pull_requests.py
index 4bb2edd3..b4e7047f 100644
--- a/runbot_merge/models/pull_requests.py
+++ b/runbot_merge/models/pull_requests.py
@@ -2049,6 +2049,7 @@ class Stagings(models.Model):
                 ._trigger(fields.Datetime.to_datetime(timeout))
 
         if vals.get('active') is False:
+            vals['staging_end'] = fields.Datetime.now()
             self.env.ref("runbot_merge.staging_cron")._trigger()
 
         return super().write(vals)
@@ -2141,7 +2142,6 @@ class Stagings(models.Model):
             s.state = st
             if s.state != 'pending':
                 self.env.ref("runbot_merge.merge_cron")._trigger()
-                s.staging_end = fields.Datetime.now()
             if update_timeout_limit:
                 s.timeout_limit = datetime.datetime.now() + datetime.timedelta(minutes=s.target.project_id.ci_timeout)
                 self.env.ref("runbot_merge.merge_cron")._trigger(s.timeout_limit)

From d0723499a2d71fe1ed55d55d60cf7362351837c6 Mon Sep 17 00:00:00 2001
From: Xavier Morel <xmo@odoo.com>
Date: Fri, 6 Sep 2024 13:51:55 +0200
Subject: [PATCH 06/45] [IMP] runbot_merge: stage by first ready

This is an approximation under the assumption that stored computes
update the `write_date`, and that there's not much else that will be
computed on a batch.

Eventually it might be a good idea for this to be a proper field,
computed alongside the unblocking of the batch.

Fixes #932
---
 runbot_merge/models/stagings_create.py | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/runbot_merge/models/stagings_create.py b/runbot_merge/models/stagings_create.py
index 55955a0a..75464045 100644
--- a/runbot_merge/models/stagings_create.py
+++ b/runbot_merge/models/stagings_create.py
@@ -195,11 +195,15 @@ def ready_batches(for_branch: Branch) -> Tuple[bool, Batch]:
     # staged through priority *and* through split.
     split_ids = for_branch.split_ids.batch_ids.ids
     env.cr.execute("""
-        SELECT max(priority)
-        FROM runbot_merge_batch
-        WHERE blocked IS NULL AND target = %s AND NOT id = any(%s)
+        SELECT exists (
+            SELECT FROM runbot_merge_batch
+            WHERE priority = 'alone'
+              AND blocked IS NULL
+              AND target = %s
+              AND NOT id = any(%s)
+        )
     """, [for_branch.id, split_ids])
-    alone = env.cr.fetchone()[0] == 'alone'
+    [alone] = env.cr.fetchone()
 
     return (
         alone,
@@ -208,7 +212,7 @@ def ready_batches(for_branch: Branch) -> Tuple[bool, Batch]:
             ('blocked', '=', False),
             ('priority', '=', 'alone') if alone else (1, '=', 1),
             ('id', 'not in', split_ids),
-        ], order="priority DESC, id ASC"),
+        ], order="priority DESC, write_date ASC, id ASC"),
     )
 
 

From 60188063f8857991014463020bfc0cd884e06187 Mon Sep 17 00:00:00 2001
From: Xavier Morel <xmo@odoo.com>
Date: Fri, 6 Sep 2024 15:09:08 +0200
Subject: [PATCH 07/45] [FIX] *: ensure I don't get bollocked up again by tags

Today (or really a month ago) I learned: when giving git a symbolic
ref (e.g. a ref name), if it's ambiguous then

1. If `$GIT_DIR/$name` exists, that is what you mean (this is usually
   useful only for `HEAD`, `FETCH_HEAD`, `ORIG_HEAD`, `MERGE_HEAD`,
   `REBASE_HEAD`, `REVERT_HEAD`, `CHERRY_PICK_HEAD`, `BISECT_HEAD` and
   `AUTO_MERGE`)
2. otherwise, `refs/$name` if it exists
3. otherwise, `refs/tags/$name` if it exists
4. otherwise, `refs/heads/$name` if it exists
5. otherwise, `refs/remotes/$name` if it exists
6. otherwise, `refs/remotes/$name/HEAD` if it exists

This means if a tag and a branch have the same name and only the name
is provided (not the full ref), git will select the tag, which gets
very confusing for the mergebot as it now tries to rebase onto the tag
(which because that's not fun otherwise was not even on the branch of
the same name).

Fix by providing full refs to `rev-parse` when trying to retrieve the
head of the target branches. And as a defense in depth opportunity,
also exclude tags when fetching refs by spec: apparently fetching a
specific commit does not trigger the retrieval of tags, but any sort
of spec will see the tags come along for the ride even if the tags are
not in any way under the fetched ref e.g. `refs/heads/*` will very
much retrieve the tags despite them being located at `refs/tags/*`.

Fixes #922
---
 forwardport/models/project.py                  | 5 +++--
 runbot_merge/models/project_freeze/__init__.py | 2 +-
 runbot_merge/models/stagings_create.py         | 3 ++-
 3 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/forwardport/models/project.py b/forwardport/models/project.py
index c3d4806d..0031b61b 100644
--- a/forwardport/models/project.py
+++ b/forwardport/models/project.py
@@ -362,7 +362,8 @@ stderr:
 {err}
 """
 
-            target_head = source.stdout().rev_parse(target_branch.name).stdout.decode().strip()
+            target_head = source.stdout().rev_parse(f"refs/heads/{target_branch.name}")\
+                .stdout.decode().strip()
             commit = conf.commit_tree(
                 tree=tree.stdout.decode().splitlines(keepends=False)[0],
                 parents=[target_head],
@@ -386,7 +387,7 @@ stderr:
         logger = _logger.getChild(str(self.id)).getChild('cherrypick')
 
         # target's head
-        head = repo.stdout().rev_parse(branch).stdout.decode().strip()
+        head = repo.stdout().rev_parse(f"refs/heads/{branch}").stdout.decode().strip()
 
         commits = self.commits()
         logger.info(
diff --git a/runbot_merge/models/project_freeze/__init__.py b/runbot_merge/models/project_freeze/__init__.py
index 0a39ceae..7af53f27 100644
--- a/runbot_merge/models/project_freeze/__init__.py
+++ b/runbot_merge/models/project_freeze/__init__.py
@@ -217,7 +217,7 @@ class FreezeWizard(models.Model):
             for r in self.project_id.repo_ids
         }
         for repo, copy in repos.items():
-            copy.fetch(git.source_url(repo), '+refs/heads/*:refs/heads/*')
+            copy.fetch(git.source_url(repo), '+refs/heads/*:refs/heads/*', no_tags=True)
         all_prs = self.release_pr_ids.pr_id | self.bump_pr_ids.pr_id
         for pr in all_prs:
             repos[pr.repository].fetch(
diff --git a/runbot_merge/models/stagings_create.py b/runbot_merge/models/stagings_create.py
index 75464045..08734958 100644
--- a/runbot_merge/models/stagings_create.py
+++ b/runbot_merge/models/stagings_create.py
@@ -244,7 +244,8 @@ def staging_setup(
             # be hooked only to "proper" remote-tracking branches
             # (in `refs/remotes`), it doesn't seem to work here
             f'+refs/heads/{target.name}:refs/heads/{target.name}',
-            *(pr.head for pr in by_repo.get(repo, []))
+            *(pr.head for pr in by_repo.get(repo, [])),
+            no_tags=True,
         )
         original_heads[repo] = head
         staging_state[repo] = StagingSlice(gh=gh, head=head, repo=source.stdout().with_config(text=True, check=False))

From 10ca096d867463a57b1de1e5943115c047fe1238 Mon Sep 17 00:00:00 2001
From: Xavier Morel <xmo@odoo.com>
Date: Mon, 16 Sep 2024 12:46:44 +0200
Subject: [PATCH 08/45] [FIX] forwardport: cron create prototype

Apparently when I added these to trigger the corresponding cron the
lack of decorator caused them to not work at all, at least when
invoked from the interface, consequently I notably wasn't able to
force a forward port via creating one such task when trying to work
around #953
---
 forwardport/models/forwardport.py | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/forwardport/models/forwardport.py b/forwardport/models/forwardport.py
index e805e59a..6f9320e1 100644
--- a/forwardport/models/forwardport.py
+++ b/forwardport/models/forwardport.py
@@ -1,9 +1,7 @@
 # -*- coding: utf-8 -*-
 import builtins
-import contextlib
 import logging
 import re
-import uuid
 from contextlib import ExitStack
 from datetime import datetime, timedelta
 
@@ -11,7 +9,7 @@ import requests
 import sentry_sdk
 from dateutil import relativedelta
 
-from odoo import fields, models
+from odoo import api, fields, models
 from odoo.addons.runbot_merge import git
 from odoo.addons.runbot_merge.github import GH
 
@@ -77,6 +75,7 @@ class ForwardPortTasks(models.Model, Queue):
     retry_after = fields.Datetime(required=True, default='1900-01-01 01:01:01')
     pr_id = fields.Many2one('runbot_merge.pull_requests')
 
+    @api.model_create_multi
     def create(self, vals_list):
         self.env.ref('forwardport.port_forward')._trigger()
         return super().create(vals_list)
@@ -270,6 +269,7 @@ class UpdateQueue(models.Model, Queue):
     original_root = fields.Many2one('runbot_merge.pull_requests')
     new_root = fields.Many2one('runbot_merge.pull_requests')
 
+    @api.model_create_multi
     def create(self, vals_list):
         self.env.ref('forwardport.updates')._trigger()
         return super().create(vals_list)
@@ -359,6 +359,7 @@ class DeleteBranches(models.Model, Queue):
 
     pr_id = fields.Many2one('runbot_merge.pull_requests')
 
+    @api.model_create_multi
     def create(self, vals_list):
         self.env.ref('forwardport.remover')._trigger(datetime.now() - MERGE_AGE)
         return super().create(vals_list)

From fe7cd8e1f08aa6e1e30bf0b09ef8c54c08e89ed4 Mon Sep 17 00:00:00 2001
From: Xavier Morel <xmo@odoo.com>
Date: Mon, 16 Sep 2024 12:48:42 +0200
Subject: [PATCH 09/45] [IMP] runbot_merge: name PR correctly on staging
 success

Logging PRs by id is unusual, unreadable, and inconvenient.
---
 runbot_merge/models/pull_requests.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/runbot_merge/models/pull_requests.py b/runbot_merge/models/pull_requests.py
index b4e7047f..4c175194 100644
--- a/runbot_merge/models/pull_requests.py
+++ b/runbot_merge/models/pull_requests.py
@@ -2298,7 +2298,7 @@ class Stagings(models.Model):
                 prs = self.mapped('batch_ids.prs')
                 logger.info(
                     "%s FF successful, marking %s as merged",
-                    self, prs
+                    self, prs.mapped('display_name'),
                 )
                 self.batch_ids.merge_date = fields.Datetime.now()
 

From 851656bec0140056f3f426d6360158e9b0d7948c Mon Sep 17 00:00:00 2001
From: Xavier Morel <xmo@odoo.com>
Date: Mon, 16 Sep 2024 12:49:23 +0200
Subject: [PATCH 10/45] [IMP] runbot_merge: set status on skipchecks & use that

- rather than enumerate states, forward-porting should just check if
  the statuses are successful on a PR
- for the same consistency reasons explained in
  f97502e503cb52c0000ad9b153c4ddb7482c3a6d, `skipchecks` should force
  the status of a PR to `success`: it very odd that a PR would be
  ready without being validated...
---
 runbot_merge/models/batch.py         | 2 +-
 runbot_merge/models/pull_requests.py | 5 ++++-
 runbot_merge/tests/test_oddities.py  | 2 +-
 3 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/runbot_merge/models/batch.py b/runbot_merge/models/batch.py
index d920802f..1a96075e 100644
--- a/runbot_merge/models/batch.py
+++ b/runbot_merge/models/batch.py
@@ -432,7 +432,7 @@ class Batch(models.Model):
                 _logger.info('-> no parent %s (%s)', batch, prs)
                 continue
             if not force_fw and batch.source.fw_policy != 'skipci' \
-                    and (invalid := batch.prs.filtered(lambda p: p.state not in ['validated', 'ready'])):
+                    and (invalid := batch.prs.filtered(lambda p: p.status != 'success')):
                 _logger.info(
                     '-> wrong state %s (%s)',
                     batch,
diff --git a/runbot_merge/models/pull_requests.py b/runbot_merge/models/pull_requests.py
index 4c175194..dd27fe36 100644
--- a/runbot_merge/models/pull_requests.py
+++ b/runbot_merge/models/pull_requests.py
@@ -1163,7 +1163,7 @@ For your own safety I've ignored *everything in your entire comment*.
         super().modified(fnames, create, before)
 
     @api.depends(
-        'statuses', 'overrides', 'target', 'parent_id',
+        'statuses', 'overrides', 'target', 'parent_id', 'skipchecks',
         'repository.status_ids.context',
         'repository.status_ids.branch_filter',
         'repository.status_ids.prs',
@@ -1173,6 +1173,9 @@ For your own safety I've ignored *everything in your entire comment*.
             statuses = {**json.loads(pr.statuses), **pr._get_overrides()}
 
             pr.statuses_full = json.dumps(statuses, indent=4)
+            if pr.skipchecks:
+                pr.status = 'success'
+                continue
 
             st = 'success'
             for ci in pr.repository.status_ids._for_pr(pr):
diff --git a/runbot_merge/tests/test_oddities.py b/runbot_merge/tests/test_oddities.py
index 6bee3696..e0f530b5 100644
--- a/runbot_merge/tests/test_oddities.py
+++ b/runbot_merge/tests/test_oddities.py
@@ -260,7 +260,7 @@ def test_force_ready(env, repo, config):
     pr_id.skipchecks = True
 
     assert pr_id.state == 'ready'
-    assert pr_id.status == 'pending'
+    assert pr_id.status == 'success'
     reviewer = env['res.users'].browse([env._uid]).partner_id
     assert pr_id.reviewed_by == reviewer
 

From a046cf2f7c1d5451fd0897a678f72520a5d9b169 Mon Sep 17 00:00:00 2001
From: Xavier Morel <xmo@odoo.com>
Date: Tue, 17 Sep 2024 10:29:01 +0200
Subject: [PATCH 11/45] [FIX] *: UX around fw=no

The UX around the split of limit and forward port policy (and
especially moving "don't forward port" to the policy) was not really
considered and caused confusion for both me and devs: after having
disabled forward porting, updating the limit would not restore it, but
there would be no indication of such an issue.

This caused odoo/enterprise#68916 to not be forward ported at merge
(despite looking for all the world like it should be), and while
updating the limit post-merge did force a forward-port that
inconsistency was just as jarring (also not helped by being unable to
create an fw batch via the backend UI because reasons, see
10ca096d867463a57b1de1e5943115c047fe1238).

Fix this along most axis:

- Notify the user and reset the fw policy if the limit is updated
  while `fw=no`.
- Trigger a forward port if the fw policy is updated (from `no`) on a
  merged PR, currently only sources.
- Add check & warning to limit update process so it does *not* force a
  port (though maybe it should under the assumption that we're
  updating the limit anyway? We'll see).

Fixes #953
---
 forwardport/tests/test_limit.py      | 78 ++++++++++++++++++++++++++++
 runbot_merge/models/commands.py      |  2 +-
 runbot_merge/models/pull_requests.py | 54 +++++++++++++++----
 runbot_merge/tests/test_oddities.py  |  1 +
 4 files changed, 124 insertions(+), 11 deletions(-)

diff --git a/forwardport/tests/test_limit.py b/forwardport/tests/test_limit.py
index de563e3d..e5bfcd3e 100644
--- a/forwardport/tests/test_limit.py
+++ b/forwardport/tests/test_limit.py
@@ -76,6 +76,84 @@ def test_ignore(env, config, make_repo, users):
         (users['user'], "Disabled forward-porting."),
     ]
 
+    with prod:
+        pr.post_comment('hansen up to c', config['role_reviewer']['token'])
+    env.run_crons()
+
+    assert env['runbot_merge.pull_requests'].search([]) == pr_id,\
+        "should still not have created a forward port"
+    assert pr.comments[6:] == [
+        (users['reviewer'], 'hansen up to c'),
+        (users['user'], "@%s can not forward-port, policy is 'no' on %s" % (
+            users['reviewer'],
+            pr_id.display_name,
+        ))
+    ]
+
+    assert pr_id.limit_id == env['runbot_merge.branch'].search([('name', '=', 'c')])
+    pr_id.limit_id = False
+
+    with prod:
+        pr.post_comment("hansen fw=default", config['role_reviewer']['token'])
+    env.run_crons()
+
+    assert pr.comments[8:] == [
+        (users['reviewer'], "hansen fw=default"),
+        (users['user'], "Starting forward-port. Waiting for CI to create followup forward-ports.")
+    ]
+
+    assert env['runbot_merge.pull_requests'].search([('source_id', '=', pr_id.id)]),\
+        "should finally have created a forward port"
+
+
+def test_unignore(env, config, make_repo, users):
+    env['res.partner'].create({'name': users['other'], 'github_login': users['other'], 'email': 'other@example.org'})
+
+    prod, _ = make_basic(env, config, make_repo, statuses="default")
+    token = config['role_other']['token']
+    fork = prod.fork(token=token)
+    with prod, fork:
+        [c] = fork.make_commits('a', Commit('c', tree={'0': '0'}), ref='heads/mybranch')
+        pr = prod.make_pr(target='a', head=f'{fork.owner}:mybranch', title="title", token=token)
+        prod.post_status(c, 'success')
+    env.run_crons()
+
+    pr_id = to_pr(env, pr)
+    assert pr_id.batch_id.fw_policy == 'default'
+
+    with prod:
+        pr.post_comment('hansen fw=no', token)
+    env.run_crons(None)
+    assert pr_id.batch_id.fw_policy == 'no'
+
+    with prod:
+        pr.post_comment('hansen fw=default', token)
+    env.run_crons(None)
+    assert pr_id.batch_id.fw_policy == 'default'
+
+    with prod:
+        pr.post_comment('hansen fw=no', token)
+    env.run_crons(None)
+    with prod:
+        pr.post_comment('hansen up to c', token)
+    env.run_crons(None)
+    assert pr_id.batch_id.fw_policy == 'default'
+
+    assert pr.comments == [
+        seen(env, pr, users),
+        (users['other'], "hansen fw=no"),
+        (users['user'], "Disabled forward-porting."),
+        (users['other'], "hansen fw=default"),
+        (users['user'], "Waiting for CI to create followup forward-ports."),
+        (users['other'], "hansen fw=no"),
+        (users['user'], "Disabled forward-porting."),
+        (users['other'], "hansen up to c"),
+        (users['user'], "Forward-porting to 'c'. Re-enabled forward-porting "\
+                        "(you should use `fw=default` to re-enable forward "\
+                        "porting after disabling)."
+        ),
+    ]
+
 def test_disable(env, config, make_repo, users):
     """ Checks behaviour if the limit target is disabled:
 
diff --git a/runbot_merge/models/commands.py b/runbot_merge/models/commands.py
index 9e3c2963..53c268d4 100644
--- a/runbot_merge/models/commands.py
+++ b/runbot_merge/models/commands.py
@@ -200,8 +200,8 @@ class FW(enum.Enum):
     @classmethod
     def help(cls, is_reviewer: bool) -> Iterator[Tuple[str, str]]:
         yield str(cls.NO), "does not forward-port this PR"
+        yield str(cls.DEFAULT), "forward-ports this PR normally"
         if is_reviewer:
-            yield str(cls.DEFAULT), "forward-ports this PR normally"
             yield str(cls.SKIPCI), "does not wait for a forward-port's statuses to succeed before creating the next one"
 
 
diff --git a/runbot_merge/models/pull_requests.py b/runbot_merge/models/pull_requests.py
index dd27fe36..74d04261 100644
--- a/runbot_merge/models/pull_requests.py
+++ b/runbot_merge/models/pull_requests.py
@@ -9,6 +9,7 @@ import json
 import logging
 import re
 import time
+from enum import IntEnum
 from functools import reduce
 from operator import itemgetter
 from typing import Optional, Union, List, Iterator, Tuple
@@ -879,6 +880,7 @@ For your own safety I've ignored *everything in your entire comment*.
                 case commands.Close() if source_author:
                     feedback(close=True)
                 case commands.FW():
+                    message = None
                     match command:
                         case commands.FW.NO if is_author or source_author:
                             message = "Disabled forward-porting."
@@ -890,10 +892,26 @@ For your own safety I've ignored *everything in your entire comment*.
                         #     message = "Not waiting for merge to create followup forward-ports."
                         case _:
                             msg = f"you don't have the right to {command}."
+                    if message:
+                        # TODO: feedback?
+                        if self.source_id:
+                            "if the pr is not a source, ignore (maybe?)"
+                        elif not self.merge_date:
+                            "if the PR is not merged, it'll be fw'd normally"
+                        elif self.batch_id.fw_policy != 'no' or command == commands.FW.NO:
+                            "if the policy is not being flipped from no to something else, nothing to do"
+                        elif branch_key(self.limit_id) <= branch_key(self.target):
+                            "if the limit is lower than current (old style ignore) there's nothing to do"
+                        else:
+                            message = f"Starting forward-port. {message}"
+                            self.env['forwardport.batches'].create({
+                                'batch_id': self.batch_id.id,
+                                'source': 'merge',
+                            })
 
-                    if not msg:
                         (self.source_id or self).batch_id.fw_policy = command.name.lower()
                         feedback(message=message)
+
                 case commands.Limit(branch) if is_author:
                     if branch is None:
                         feedback(message="'ignore' is deprecated, use 'fw=no' to disable forward porting.")
@@ -901,7 +919,7 @@ For your own safety I've ignored *everything in your entire comment*.
                     for p in self.batch_id.prs:
                         ping, m = p._maybe_update_limit(limit)
 
-                        if ping and p == self:
+                        if ping is Ping.ERROR and p == self:
                             msg = m
                         else:
                             if ping:
@@ -934,31 +952,37 @@ For your own safety I've ignored *everything in your entire comment*.
             feedback(message=f"@{login}{rejections}{footer}")
         return 'rejected'
 
-    def _maybe_update_limit(self, limit: str) -> Tuple[bool, str]:
+    def _maybe_update_limit(self, limit: str) -> Tuple[Ping, str]:
         limit_id = self.env['runbot_merge.branch'].with_context(active_test=False).search([
             ('project_id', '=', self.repository.project_id.id),
             ('name', '=', limit),
         ])
         if not limit_id:
-            return True, f"there is no branch {limit!r}, it can't be used as a forward port target."
+            return Ping.ERROR, f"there is no branch {limit!r}, it can't be used as a forward port target."
 
         if limit_id != self.target and not limit_id.active:
-            return True, f"branch {limit_id.name!r} is disabled, it can't be used as a forward port target."
+            return Ping.ERROR, f"branch {limit_id.name!r} is disabled, it can't be used as a forward port target."
 
         # not forward ported yet, just acknowledge the request
         if not self.source_id and self.state != 'merged':
             self.limit_id = limit_id
             if branch_key(limit_id) <= branch_key(self.target):
-                return False, "Forward-port disabled (via limit)."
+                return Ping.NO, "Forward-port disabled (via limit)."
             else:
-                return False, f"Forward-porting to {limit_id.name!r}."
+                suffix = ''
+                if self.batch_id.fw_policy == 'no':
+                    self.batch_id.fw_policy = 'default'
+                    suffix = " Re-enabled forward-porting (you should use "\
+                             "`fw=default` to re-enable forward porting "\
+                             "after disabling)."
+                return Ping.NO, f"Forward-porting to {limit_id.name!r}.{suffix}"
 
         # if the PR has been forwardported
         prs = (self | self.forwardport_ids | self.source_id | self.source_id.forwardport_ids)
         tip = max(prs, key=pr_key)
         # if the fp tip was closed it's fine
         if tip.state == 'closed':
-            return True, f"{tip.display_name} is closed, no forward porting is going on"
+            return Ping.ERROR, f"{tip.display_name} is closed, no forward porting is going on"
 
         prs.limit_id = limit_id
 
@@ -977,7 +1001,7 @@ For your own safety I've ignored *everything in your entire comment*.
             except psycopg2.errors.LockNotAvailable:
                 # row locked = port occurring and probably going to succeed,
                 # so next(real_limit) likely a done deal already
-                return True, (
+                return Ping.ERROR, (
                     f"Forward port of {tip.display_name} likely already "
                     f"ongoing, unable to cancel, close next forward port "
                     f"when it completes.")
@@ -988,6 +1012,9 @@ For your own safety I've ignored *everything in your entire comment*.
             # forward porting was previously stopped at tip, and we want it to
             # resume
             if tip.state == 'merged':
+                if tip.batch_id.source.fw_policy == 'no':
+                    # hack to ping the user but not rollback the transaction
+                    return Ping.YES, f"can not forward-port, policy is 'no' on {(tip.source_id or tip).display_name}"
                 self.env['forwardport.batches'].create({
                     'batch_id': tip.batch_id.id,
                     'source': 'fp' if tip.parent_id else 'merge',
@@ -1022,7 +1049,7 @@ For your own safety I've ignored *everything in your entire comment*.
             for p in (self.source_id | root) - self
         ])
 
-        return False, msg
+        return Ping.NO, msg
 
 
     def _find_next_target(self) -> Optional[Branch]:
@@ -1631,6 +1658,13 @@ For your own safety I've ignored *everything in your entire comment*.
             'batch_id': batch.create({}).id,
         })
 
+
+class Ping(IntEnum):
+    NO = 0
+    YES = 1
+    ERROR = 2
+
+
 # ordering is a bit unintuitive because the lowest sequence (and name)
 # is the last link of the fp chain, reasoning is a bit more natural the
 # other way around (highest object is the last), especially with Python
diff --git a/runbot_merge/tests/test_oddities.py b/runbot_merge/tests/test_oddities.py
index e0f530b5..530a34d3 100644
--- a/runbot_merge/tests/test_oddities.py
+++ b/runbot_merge/tests/test_oddities.py
@@ -348,6 +348,7 @@ Currently available commands for @{user}:
 |-|-|
 |`help`|displays this help|
 |`fw=no`|does not forward-port this PR|
+|`fw=default`|forward-ports this PR normally|
 |`up to <branch>`|only ports this PR forward to the specified branch (included)|
 |`check`|fetches or refreshes PR metadata, resets mergebot state|
 

From c8a06601a7c223cdfc9da23f2c799b4c660f274b Mon Sep 17 00:00:00 2001
From: Xavier Morel <xmo@odoo.com>
Date: Wed, 18 Sep 2024 15:19:13 +0200
Subject: [PATCH 12/45] [FIX] *: unstage on status going from success to
 failure

And unconditionally unstage when the HEAD of a PR is synchronised.

While a rebuild on a PR which was previously staged can be a false
positive (e.g. because it hit a non-derministic test the second time
around) it can also be legitimate e.g. auto-rebase of an old PR to
check it out. In that case the PR should be unstaged.

Furthermore, as soon as the PR gets rebuilt it goes back into
`approved` (because the status goes to pending and currently there's
no great way to suppress that in the rebuild case without also fucking
it up for the sync case). This would cause a sync on the PR to be
missed (in that it would not unstage the PR), which is broken. Fix
that by not checking the state of the PR beforehand, it seems to be an
unnecessary remnant of older code, and not really an optimisation (or
at least one likely not worth bothering with, especially as we then
proceed to perform a full PR validation anyway).

Fixes #950
---
 forwardport/tests/test_weird.py      |  5 +-
 mergebot_test_utils/utils.py         | 14 +++++
 runbot_merge/controllers/__init__.py |  3 -
 runbot_merge/models/pull_requests.py | 14 ++++-
 runbot_merge/tests/test_staging.py   | 94 +++++++++++++++++++++++++++-
 5 files changed, 121 insertions(+), 9 deletions(-)

diff --git a/forwardport/tests/test_weird.py b/forwardport/tests/test_weird.py
index e2fd04a7..b7feab33 100644
--- a/forwardport/tests/test_weird.py
+++ b/forwardport/tests/test_weird.py
@@ -3,7 +3,7 @@ from datetime import datetime, timedelta
 
 import pytest
 
-from utils import seen, Commit, to_pr, make_basic
+from utils import seen, Commit, to_pr, make_basic, prevent_unstaging
 
 
 def test_no_token(env, config, make_repo):
@@ -935,7 +935,8 @@ def test_missing_magic_ref(env, config, make_repo):
     pr_id = to_pr(env, pr)
     assert pr_id.staging_id
 
-    pr_id.head = '0'*40
+    with prevent_unstaging(pr_id.staging_id):
+        pr_id.head = '0'*40
     with prod:
         prod.post_status('staging.a', 'success', 'legal/cla')
         prod.post_status('staging.a', 'success', 'ci/runbot')
diff --git a/mergebot_test_utils/utils.py b/mergebot_test_utils/utils.py
index c5473e93..e5c24fbd 100644
--- a/mergebot_test_utils/utils.py
+++ b/mergebot_test_utils/utils.py
@@ -1,4 +1,5 @@
 # -*- coding: utf-8 -*-
+import contextlib
 import itertools
 import re
 import time
@@ -200,3 +201,16 @@ Signed-off-by: {pr_id.reviewed_by.formatted_email}"""
 def ensure_one(records):
     assert len(records) == 1
     return records
+
+
+@contextlib.contextmanager
+def prevent_unstaging(st) -> None:
+    # hack: `Stagings.cancel` only cancels *active* stagings,
+    # so if we disable the staging, do a thing, then re-enable
+    # then things should work out
+    assert st and st.active, "preventing unstaging of not-a-staging is not useful"
+    st.active = False
+    try:
+        yield
+    finally:
+        st.active = True
diff --git a/runbot_merge/controllers/__init__.py b/runbot_merge/controllers/__init__.py
index b8005979..5dac8125 100644
--- a/runbot_merge/controllers/__init__.py
+++ b/runbot_merge/controllers/__init__.py
@@ -285,9 +285,6 @@ def handle_pr(env, event):
             _logger.error("PR %s sync %s -> %s => failure (closed)", pr_obj.display_name, pr_obj.head, pr['head']['sha'])
             return "It's my understanding that closed/merged PRs don't get sync'd"
 
-        if pr_obj.state == 'ready':
-            pr_obj.unstage("updated by %s", event['sender']['login'])
-
         _logger.info(
             "PR %s sync %s -> %s by %s => reset to 'open' and squash=%s",
             pr_obj.display_name,
diff --git a/runbot_merge/models/pull_requests.py b/runbot_merge/models/pull_requests.py
index 74d04261..577866cb 100644
--- a/runbot_merge/models/pull_requests.py
+++ b/runbot_merge/models/pull_requests.py
@@ -1164,7 +1164,7 @@ For your own safety I've ignored *everything in your entire comment*.
         # temporarily on the same head, or on the same head with different
         # targets
         updateable = self.filtered(lambda p: not p.merge_date)
-        updateable.statuses = statuses
+        updateable.statuses = statuses or '{}'
         for pr in updateable:
             if pr.status == "failure":
                 statuses = json.loads(pr.statuses_full)
@@ -1212,6 +1212,9 @@ For your own safety I've ignored *everything in your entire comment*.
                     break
                 if v == 'pending':
                     st = 'pending'
+            if pr.status != 'failure' and st == 'failure':
+                pr.unstage("had CI failure after staging")
+
             pr.status = st
 
     @api.depends(
@@ -1346,7 +1349,7 @@ For your own safety I've ignored *everything in your entire comment*.
 
         pr = super().create(vals)
         c = self.env['runbot_merge.commit'].search([('sha', '=', pr.head)])
-        pr._validate(c.statuses or '{}')
+        pr._validate(c.statuses)
 
         if pr.state not in ('closed', 'merged'):
             self.env.ref('runbot_merge.pr.created')._send(
@@ -1423,8 +1426,13 @@ For your own safety I've ignored *everything in your entire comment*.
 
         newhead = vals.get('head')
         if newhead:
+            if pid := self.env.cr.precommit.data.get('change-author'):
+                writer = self.env['res.partner'].browse(pid)
+            else:
+                writer = self.env.user.partner_id
+            self.unstage("updated by %s", writer.github_login or writer.name)
             c = self.env['runbot_merge.commit'].search([('sha', '=', newhead)])
-            self._validate(c.statuses or '{}')
+            self._validate(c.statuses)
         return w
 
     def _check_linked_prs_statuses(self, commit=False):
diff --git a/runbot_merge/tests/test_staging.py b/runbot_merge/tests/test_staging.py
index c1aeeeeb..7dc98713 100644
--- a/runbot_merge/tests/test_staging.py
+++ b/runbot_merge/tests/test_staging.py
@@ -1,4 +1,4 @@
-from utils import Commit, to_pr
+from utils import Commit, to_pr, make_basic, prevent_unstaging
 
 
 def test_staging_disabled_branch(env, project, repo, config):
@@ -26,3 +26,95 @@ def test_staging_disabled_branch(env, project, repo, config):
         "master is allowed to stage, should be staged"
     assert not to_pr(env, other_pr).staging_id, \
         "other is *not* allowed to stage, should not be staged"
+
+
+def test_staged_failure(env, config, repo, users):
+    """If a PR is staged and gets a new CI failure, it should be unstaged
+
+    This was an issue with odoo/odoo#165931 which got rebuilt and that triggered
+    a failure, which made the PR !ready but kept the staging going. So the PR
+    ended up in an odd state of being both staged and not ready.
+
+    And while the CI failure it got was a false positive, it was in fact the
+    problematic PR breaking that staging.
+
+    More relevant the runbot's "automatic rebase" mode sends CI to the original
+    commits so receiving legitimate failures after staging very much makes
+    sense e.g. an old PR is staged, the staging starts failing, somebody notices
+    the outdated PR and triggers autorebase, which fails (because something
+    incompatible was merged in the meantime), the PR *should* be unstaged.
+    """
+    with repo:
+        repo.make_commits(None, Commit("master", tree={'a': '1'}), ref="heads/master")
+
+        repo.make_commits('master', Commit('c', tree={'a': 'b'}), ref="heads/mybranch")
+        pr = repo.make_pr(target='master', head='mybranch')
+        repo.post_status('mybranch', 'success')
+        pr.post_comment('hansen r+', config['role_reviewer']['token'])
+    env.run_crons()
+
+    pr_id = to_pr(env, pr)
+    staging = pr_id.staging_id
+    assert staging, "pr should be staged"
+
+    with repo:
+        # started rebuild, nothing should happen
+        repo.post_status('mybranch', 'pending')
+    env.run_crons()
+    assert pr_id.staging_id
+    # can't find a clean way to keep this "ready" when transitioning from
+    # success to pending *without updating the head*, at least not without
+    # adding a lot of contextual information around `_compute_statuses`
+    # assert pr_id.state == 'ready'
+
+    with repo:
+        # rebuild failed omg!
+        repo.post_status('mybranch', 'failure')
+    env.run_crons()
+
+    assert pr_id.status == 'failure'
+    assert pr_id.state == 'approved'
+
+    assert not pr_id.staging_id, "pr should be unstaged"
+    assert staging.state == "cancelled"
+    assert staging.reason == f"{pr_id.display_name} had CI failure after staging"
+
+
+def test_update_unready(env, config, repo, users):
+    """Less likely with `test_staged_failure` fixing most of the issue, but
+    clearly the assumption that a staged PR will be `ready` is not strictly
+    enforced.
+
+    As such, updating the PR should try to `unstage` it no matter what state
+    it's in, this will lead to slightly higher loads on sync but loads on the
+    mergebot are generally non-existent outside of the git maintenance cron,
+    and there are doubtless other optimisations missing, or that (and other
+    items) can be done asynchronously.
+    """
+    with repo:
+        repo.make_commits(None, Commit("master", tree={'a': '1'}), ref="heads/master")
+
+        repo.make_commits('master', Commit('c', tree={'a': 'b'}), ref="heads/mybranch")
+        pr = repo.make_pr(target='master', head='mybranch')
+        repo.post_status('mybranch', 'success')
+        pr.post_comment('hansen r+', config['role_reviewer']['token'])
+
+        [c2] = repo.make_commits('master', Commit('c', tree={'a': 'c'}))
+    env.run_crons()
+
+    pr_id = to_pr(env, pr)
+    staging = pr_id.staging_id
+    assert staging, "pr should be staged"
+
+    with prevent_unstaging(pr_id.staging_id):
+        pr_id.overrides = '{"default": {"state": "failure"}}'
+    assert pr_id.state == "approved"
+    assert pr_id.staging_id, "pr should not have been unstaged because we cheated"
+
+    with repo:
+        repo.update_ref("heads/mybranch", c2, force=True)
+    env.run_crons()
+
+    assert not pr_id.staging_id, "pr should be unstaged"
+    assert staging.state == "cancelled"
+    assert staging.reason == f"{pr_id.display_name} updated by {users['user']}"

From 154e610bbc49853fd2ecc8a926c2976d54891fc8 Mon Sep 17 00:00:00 2001
From: Xavier Morel <xmo@odoo.com>
Date: Thu, 19 Sep 2024 12:17:59 +0200
Subject: [PATCH 13/45] [IMP] *: modernize `TestPRUpdate`

- Switch to just `default` ci.
- Autouse fixture to init the master branch.
- Switch to `make_commits`.
- Merge `test_reopen_update` and `test_update_closed_revalidate` into
  `test_update_closed`: the former did basically nothing useful and
  the latter could easily be folded into `test_update_closed` by just
  validating the new commit.
---
 forwardport/tests/test_weird.py  |   8 +-
 runbot_merge/tests/test_basic.py | 252 ++++++++++++-------------------
 2 files changed, 96 insertions(+), 164 deletions(-)

diff --git a/forwardport/tests/test_weird.py b/forwardport/tests/test_weird.py
index b7feab33..9577f690 100644
--- a/forwardport/tests/test_weird.py
+++ b/forwardport/tests/test_weird.py
@@ -920,13 +920,12 @@ def test_missing_magic_ref(env, config, make_repo):
     Emulate this behaviour by updating the PR with a commit which lives in the
     repo but has no ref.
     """
-    prod, _ = make_basic(env, config, make_repo)
+    prod, _ = make_basic(env, config, make_repo, statuses='default')
     a_head = prod.commit('refs/heads/a')
     with prod:
         [c] = prod.make_commits(a_head.id, Commit('x', tree={'x': '0'}), ref='heads/change')
         pr = prod.make_pr(target='a', head='change')
-        prod.post_status(c, 'success', 'legal/cla')
-        prod.post_status(c, 'success', 'ci/runbot')
+        prod.post_status(c, 'success')
         pr.post_comment('hansen r+', config['role_reviewer']['token'])
     env.run_crons()
 
@@ -938,8 +937,7 @@ def test_missing_magic_ref(env, config, make_repo):
     with prevent_unstaging(pr_id.staging_id):
         pr_id.head = '0'*40
     with prod:
-        prod.post_status('staging.a', 'success', 'legal/cla')
-        prod.post_status('staging.a', 'success', 'ci/runbot')
+        prod.post_status('staging.a', 'success')
     env.run_crons()
 
     assert not pr_id.staging_id
diff --git a/runbot_merge/tests/test_basic.py b/runbot_merge/tests/test_basic.py
index 18d75d73..e59b8bf4 100644
--- a/runbot_merge/tests/test_basic.py
+++ b/runbot_merge/tests/test_basic.py
@@ -2191,83 +2191,53 @@ Co-authored-by: {other_user['name']} <{other_user['email']}>\
         }
 
 
-class TestPRUpdate(object):
+class TestPRUpdate:
     """ Pushing on a PR should update the HEAD except for merged PRs, it
     can have additional effect (see individual tests)
     """
+    @pytest.fixture(autouse=True)
+    def master(self, repo):
+        with repo:
+            [m] = repo.make_commits(None, Commit('initial', tree={'m': 'm'}), ref="heads/master")
+        return m
+
     def test_update_opened(self, env, repo):
         with repo:
-            m = repo.make_commit(None, 'initial', None, tree={'m': 'm'})
-            repo.make_ref('heads/master', m)
-
-            c = repo.make_commit(m, 'fist', None, tree={'m': 'c1'})
-            prx = repo.make_pr(title='title', body='body', target='master', head=c)
+            [c] = repo.make_commits("master", Commit('fist', tree={'m': 'c1'}))
+            prx = repo.make_pr(target='master', head=c)
 
         pr = to_pr(env, prx)
         assert pr.head == c
         # alter & push force PR entirely
         with repo:
-            c2 = repo.make_commit(m, 'first', None, tree={'m': 'cc'})
-            repo.update_ref(prx.ref, c2, force=True)
-        assert pr.head == c2
-
-    def test_reopen_update(self, env, repo, config):
-        with repo:
-            m = repo.make_commit(None, 'initial', None, tree={'m': 'm'})
-            repo.make_ref('heads/master', m)
-
-            c = repo.make_commit(m, 'fist', None, tree={'m': 'c1'})
-            prx = repo.make_pr(title='title', body='body', target='master', head=c)
-            prx.post_comment("hansen r+", config['role_reviewer']['token'])
-
-        pr = to_pr(env, prx)
-        assert pr.state == 'approved'
-        assert pr.reviewed_by
-        with repo:
-            prx.close()
-        assert pr.state == 'closed'
-        assert pr.head == c
-        assert not pr.reviewed_by
-
-        with repo:
-            prx.open()
-        assert pr.state == 'opened'
-        assert not pr.reviewed_by
-
-        with repo:
-            c2 = repo.make_commit(c, 'first', None, tree={'m': 'cc'})
+            [c2] = repo.make_commits("master", Commit('first', tree={'m': 'cc'}))
             repo.update_ref(prx.ref, c2, force=True)
         assert pr.head == c2
 
+    @pytest.mark.defaultstatuses
     def test_update_validated(self, env, repo):
         """ Should reset to opened
         """
         with repo:
-            m = repo.make_commit(None, 'initial', None, tree={'m': 'm'})
-            repo.make_ref('heads/master', m)
-
-            c = repo.make_commit(m, 'fist', None, tree={'m': 'c1'})
-            prx = repo.make_pr(title='title', body='body', target='master', head=c)
-            repo.post_status(prx.head, 'success', 'legal/cla')
-            repo.post_status(prx.head, 'success', 'ci/runbot')
+            [c] = repo.make_commits("master", Commit('first', tree={'m': 'c1'}))
+            pr = repo.make_pr(target='master', head=c)
+            repo.post_status(c, 'success')
         env.run_crons()
-        pr = to_pr(env, prx)
-        assert pr.head == c
-        assert pr.state == 'validated'
+
+        pr_id = to_pr(env, pr)
+        assert pr_id.head == c
+        assert pr_id.state == 'validated'
 
         with repo:
-            c2 = repo.make_commit(m, 'first', None, tree={'m': 'cc'})
-            repo.update_ref(prx.ref, c2, force=True)
-        assert pr.head == c2
-        assert pr.state == 'opened'
+            [c2] = repo.make_commits("master", Commit('first', tree={'m': 'cc'}))
+            repo.update_ref(pr.ref, c2, force=True)
+        assert pr_id.head == c2
+        assert pr_id.state == 'opened'
 
     def test_update_approved(self, env, repo, config):
         with repo:
-            m = repo.make_commit(None, 'initial', None, tree={'m': 'm'})
-            repo.make_ref('heads/master', m)
-
-            c = repo.make_commit(m, 'fist', None, tree={'m': 'c1'})
-            prx = repo.make_pr(title='title', body='body', target='master', head=c)
+            [c] = repo.make_commits("master", Commit('fist', tree={'m': 'c1'}))
+            prx = repo.make_pr(target='master', head=c)
             prx.post_comment('hansen r+', config['role_reviewer']['token'])
 
         pr = to_pr(env, prx)
@@ -2275,22 +2245,19 @@ class TestPRUpdate(object):
         assert pr.state == 'approved'
 
         with repo:
-            c2 = repo.make_commit(c, 'first', None, tree={'m': 'cc'})
+            [c2] = repo.make_commits("master", Commit('first', tree={'m': 'cc'}))
             repo.update_ref(prx.ref, c2, force=True)
         assert pr.head == c2
         assert pr.state == 'opened'
 
+    @pytest.mark.defaultstatuses
     def test_update_ready(self, env, repo, config):
         """ Should reset to opened
         """
         with repo:
-            m = repo.make_commit(None, 'initial', None, tree={'m': 'm'})
-            repo.make_ref('heads/master', m)
-
-            c = repo.make_commit(m, 'fist', None, tree={'m': 'c1'})
-            prx = repo.make_pr(title='title', body='body', target='master', head=c)
-            repo.post_status(prx.head, 'success', 'legal/cla')
-            repo.post_status(prx.head, 'success', 'ci/runbot')
+            [c] = repo.make_commits("master", Commit('fist', tree={'m': 'c1'}))
+            prx = repo.make_pr(target='master', head=c)
+            repo.post_status(prx.head, 'success')
             prx.post_comment('hansen r+', config['role_reviewer']['token'])
         env.run_crons()
         pr = to_pr(env, prx)
@@ -2298,22 +2265,19 @@ class TestPRUpdate(object):
         assert pr.state == 'ready'
 
         with repo:
-            c2 = repo.make_commit(c, 'first', None, tree={'m': 'cc'})
+            [c2] = repo.make_commits(c, Commit('first', tree={'m': 'cc'}))
             repo.update_ref(prx.ref, c2, force=True)
         assert pr.head == c2
         assert pr.state == 'opened'
 
+    @pytest.mark.defaultstatuses
     def test_update_staged(self, env, repo, config):
         """ Should cancel the staging & reset PR to opened
         """
         with repo:
-            m = repo.make_commit(None, 'initial', None, tree={'m': 'm'})
-            repo.make_ref('heads/master', m)
-
-            c = repo.make_commit(m, 'fist', None, tree={'m': 'c1'})
-            prx = repo.make_pr(title='title', body='body', target='master', head=c)
-            repo.post_status(prx.head, 'success', 'legal/cla')
-            repo.post_status(prx.head, 'success', 'ci/runbot')
+            [c] = repo.make_commits("master", Commit('fist', tree={'m': 'c1'}))
+            prx = repo.make_pr(target='master', head=c)
+            repo.post_status(prx.head, 'success')
             prx.post_comment('hansen r+', config['role_reviewer']['token'])
 
         env.run_crons()
@@ -2322,33 +2286,27 @@ class TestPRUpdate(object):
         assert pr.staging_id
 
         with repo:
-            c2 = repo.make_commit(c, 'first', None, tree={'m': 'cc'})
+            [c2] = repo.make_commits(c, Commit('first', tree={'m': 'cc'}))
             repo.update_ref(prx.ref, c2, force=True)
         assert pr.head == c2
         assert pr.state == 'opened'
         assert not pr.staging_id
         assert not env['runbot_merge.stagings'].search([])
 
+    @pytest.mark.defaultstatuses
     def test_split(self, env, repo, config):
         """ Should remove the PR from its split, and possibly delete the split
         entirely.
         """
         with repo:
-            m = repo.make_commit(None, 'initial', None, tree={'m': 'm'})
-            repo.make_ref('heads/master', m)
-
-            c = repo.make_commit(m, 'first', None, tree={'m': 'm', '1': '1'})
-            repo.make_ref('heads/p1', c)
-            prx1 = repo.make_pr(title='t1', body='b1', target='master', head='p1')
-            repo.post_status(prx1.head, 'success', 'legal/cla')
-            repo.post_status(prx1.head, 'success', 'ci/runbot')
+            repo.make_commits("master", Commit('first', tree={'1': '1'}), ref="heads/p1")
+            prx1 = repo.make_pr(target='master', head='p1')
+            repo.post_status(prx1.head, 'success')
             prx1.post_comment('hansen r+', config['role_reviewer']['token'])
 
-            c = repo.make_commit(m, 'first', None, tree={'m': 'm', '2': '2'})
-            repo.make_ref('heads/p2', c)
-            prx2 = repo.make_pr(title='t2', body='b2', target='master', head='p2')
-            repo.post_status(prx2.head, 'success', 'legal/cla')
-            repo.post_status(prx2.head, 'success', 'ci/runbot')
+            [c] = repo.make_commits("master", Commit('first', tree={'2': '2'}), ref="heads/p2")
+            prx2 = repo.make_pr(target='master', head='p2')
+            repo.post_status(prx2.head, 'success')
             prx2.post_comment('hansen r+', config['role_reviewer']['token'])
         env.run_crons()
 
@@ -2359,7 +2317,7 @@ class TestPRUpdate(object):
         s0 = pr1.staging_id
 
         with repo:
-            repo.post_status('heads/staging.master', 'failure', 'ci/runbot')
+            repo.post_status('heads/staging.master', 'failure')
         env.run_crons()
 
         assert pr1.staging_id and pr1.staging_id != s0, "pr1 should have been re-staged"
@@ -2369,22 +2327,20 @@ class TestPRUpdate(object):
         assert env['runbot_merge.split'].search([])
 
         with repo:
-            repo.update_ref(prx2.ref, repo.make_commit(c, 'second', None, tree={'m': 'm', '2': '22'}), force=True)
+            [c2] = repo.make_commits(c, Commit('second', tree={'2': '22'}))
+            repo.update_ref(prx2.ref, c2, force=True)
         # probably not necessary ATM but...
         env.run_crons()
 
         assert pr2.state == 'opened', "state should have been reset"
         assert not env['runbot_merge.split'].search([]), "there should be no split left"
 
+    @pytest.mark.defaultstatuses
     def test_update_error(self, env, repo, config):
         with repo:
-            m = repo.make_commit(None, 'initial', None, tree={'m': 'm'})
-            repo.make_ref('heads/master', m)
-
-            c = repo.make_commit(m, 'fist', None, tree={'m': 'c1'})
-            prx = repo.make_pr(title='title', body='body', target='master', head=c)
-            repo.post_status(prx.head, 'success', 'legal/cla')
-            repo.post_status(prx.head, 'success', 'ci/runbot')
+            [c] = repo.make_commits("master", Commit('fist', tree={'m': 'c1'}))
+            prx = repo.make_pr(target='master', head=c)
+            repo.post_status(prx.head, 'success')
             prx.post_comment('hansen r+', config['role_reviewer']['token'])
         env.run_crons()
         pr = to_pr(env, prx)
@@ -2392,24 +2348,25 @@ class TestPRUpdate(object):
         assert pr.staging_id
 
         with repo:
-            repo.post_status('staging.master', 'success', 'legal/cla')
-            repo.post_status('staging.master', 'failure', 'ci/runbot')
+            repo.post_status('staging.master', 'failure')
         env.run_crons()
         assert not pr.staging_id
         assert pr.state == 'error'
 
         with repo:
-            c2 = repo.make_commit(c, 'first', None, tree={'m': 'cc'})
+            [c2] = repo.make_commits(c, Commit('first', tree={'m': 'cc'}))
             repo.update_ref(prx.ref, c2, force=True)
         assert pr.head == c2
         assert pr.state == 'opened'
 
     def test_unknown_pr(self, env, repo):
         with repo:
-            m = repo.make_commit(None, 'initial', None, tree={'m': 'm'})
+            [m, c] = repo.make_commits(
+                None,
+                Commit('initial', tree={'m': 'm'}),
+                Commit('first', tree={'m': 'c1'}),
+            )
             repo.make_ref('heads/1.0', m)
-
-            c = repo.make_commit(m, 'first', None, tree={'m': 'c1'})
             prx = repo.make_pr(title='title', body='body', target='1.0', head=c)
         assert not env['runbot_merge.pull_requests'].search([('number', '=', prx.number)])
 
@@ -2418,27 +2375,24 @@ class TestPRUpdate(object):
         })
 
         with repo:
-            c2 = repo.make_commit(c, 'second', None, tree={'m': 'c2'})
+            [c2] = repo.make_commits(c, Commit('second', tree={'m': 'c2'}))
             repo.update_ref(prx.ref, c2, force=True)
 
         assert not env['runbot_merge.pull_requests'].search([('number', '=', prx.number)])
 
+    @pytest.mark.defaultstatuses
     def test_update_to_ci(self, env, repo):
         """ If a PR is updated to a known-valid commit, it should be
         validated
         """
         with repo:
-            m = repo.make_commit(None, 'initial', None, tree={'m': 'm'})
-            repo.make_ref('heads/master', m)
-
-            c = repo.make_commit(m, 'fist', None, tree={'m': 'c1'})
-            c2 = repo.make_commit(m, 'first', None, tree={'m': 'cc'})
-            repo.post_status(c2, 'success', 'legal/cla')
-            repo.post_status(c2, 'success', 'ci/runbot')
+            [c] = repo.make_commits("master", Commit('fist', tree={'m': 'c1'}))
+            [c2] = repo.make_commits("master", Commit('first', tree={'m': 'cc'}))
+            repo.post_status(c2, 'success')
         env.run_crons()
 
         with repo:
-            prx = repo.make_pr(title='title', body='body', target='master', head=c)
+            prx = repo.make_pr(target='master', head=c)
         pr = to_pr(env, prx)
         assert pr.head == c
         assert pr.state == 'opened'
@@ -2448,6 +2402,7 @@ class TestPRUpdate(object):
         assert pr.head == c2
         assert pr.state == 'validated'
 
+    @pytest.mark.defaultstatuses
     def test_update_missed(self, env, repo, config, users):
         """ Sometimes github's webhooks don't trigger properly, a branch's HEAD
         does not get updated and we might e.g. attempt to merge a PR despite it
@@ -2467,10 +2422,9 @@ class TestPRUpdate(object):
             repo.make_ref('heads/somethingelse', c)
 
             [c] = repo.make_commits(
-                'heads/master', repo.Commit('title \n\nbody', tree={'a': '1'}), ref='heads/abranch')
+                'master', repo.Commit('title \n\nbody', tree={'a': '1'}), ref='heads/abranch')
             pr = repo.make_pr(target='master', head='abranch')
-            repo.post_status(pr.head, 'success', 'legal/cla')
-            repo.post_status(pr.head, 'success', 'ci/runbot')
+            repo.post_status(pr.head, 'success')
             pr.post_comment('hansen r+', config['role_reviewer']['token'])
 
         env.run_crons()
@@ -2486,8 +2440,7 @@ class TestPRUpdate(object):
             # to the PR *actually* having more than 1 commit and thus needing
             # a configuration
             [c2] = repo.make_commits('heads/master', repo.Commit('c2', tree={'a': '2'}))
-            repo.post_status(c2, 'success', 'legal/cla')
-            repo.post_status(c2, 'success', 'ci/runbot')
+            repo.post_status(c2, 'success')
             repo.update_ref(pr.ref, c2, force=True)
 
         other = env['runbot_merge.branch'].create({
@@ -2596,61 +2549,43 @@ Please check and re-approve.
             f"Updated target, squash, message. Updated {pr_id.display_name} to ready. Updated to {c2}."
         )
 
-    def test_update_closed(self, env, repo):
+    @pytest.mark.defaultstatuses
+    def test_update_closed(self, env, repo, config):
         with repo:
-            [m] = repo.make_commits(None, repo.Commit('initial', tree={'m': 'm'}), ref='heads/master')
-
-            [c] = repo.make_commits(m, repo.Commit('first', tree={'m': 'm3'}), ref='heads/abranch')
-            prx = repo.make_pr(title='title', body='body', target='master', head=c)
+            [c] = repo.make_commits("master", repo.Commit('first', tree={'m': 'm3'}), ref='heads/abranch')
+            pr = repo.make_pr(target='master', head=c)
+            pr.post_comment("hansen r+", config['role_reviewer']['token'])
         env.run_crons()
-        pr = to_pr(env, prx)
-        assert pr.state == 'opened'
-        assert pr.head == c
-        assert pr.squash
+
+        pr_id = to_pr(env, pr)
+        assert pr_id.state == 'approved'
+        assert pr_id.head == c
+        assert pr_id.squash
+        assert pr_id.reviewed_by
 
         with repo:
-            prx.close()
-
-        with repo:
-            c2 = repo.make_commit(c, 'xxx', None, tree={'m': 'm4'})
-            repo.update_ref(prx.ref, c2)
-
+            pr.close()
         assert pr.state == 'closed'
         assert pr.head == c
-        assert pr.squash
+        assert not pr_id.reviewed_by
+        assert pr_id.squash
 
         with repo:
-            prx.open()
-        assert pr.state == 'opened'
-        assert pr.head == c2
-        assert not pr.squash
+            [c2] = repo.make_commits(c, Commit('xxx', tree={'m': 'm4'}))
+            repo.update_ref(pr.ref, c2)
+            repo.post_status(c2, "success")
+
+        assert pr_id.state == 'closed'
+        assert pr_id.head == c
+        assert not pr_id.reviewed_by
+        assert pr_id.squash
 
-    def test_update_closed_revalidate(self, env, repo):
-        """ The PR should be validated on opening and reopening in case there's
-        already a CI+ stored (as the CI might never trigger unless explicitly
-        re-requested)
-        """
         with repo:
-            m = repo.make_commit(None, 'initial', None, tree={'m': 'm'})
-            repo.make_ref('heads/master', m)
-
-            c = repo.make_commit(m, 'fist', None, tree={'m': 'c1'})
-            repo.post_status(c, 'success', 'legal/cla')
-            repo.post_status(c, 'success', 'ci/runbot')
-            prx = repo.make_pr(title='title', body='body', target='master', head=c)
-
-        env.run_crons()
-        pr = to_pr(env, prx)
-        assert pr.state == 'validated', \
-            "if a PR is created on a CI'd commit, it should be validated immediately"
-
-        with repo: prx.close()
-        assert pr.state == 'closed'
-
-        with repo: prx.open()
-        assert pr.state == 'validated', \
-            "if a PR is reopened and had a CI'd head, it should be validated immediately"
-
+            pr.open()
+        assert pr_id.state == 'validated'
+        assert pr_id.head == c2
+        assert not pr_id.reviewed_by
+        assert not pr_id.squash
 
 class TestBatching(object):
     def _pr(self, repo, prefix, trees, *, target='master', user, reviewer,
@@ -2970,7 +2905,6 @@ class TestBatching(object):
             pr0.post_comment('hansen NOW!', config['role_reviewer']['token'])
         env.run_crons()
 
-        # TODO: maybe just deactivate stagings instead of deleting them when canceling?
         assert not p_1.staging_id
         assert to_pr(env, pr0).staging_id
 

From e309e1a3a2043b0ef2e18aeaa25b3d20ed9cd269 Mon Sep 17 00:00:00 2001
From: Xavier Morel <xmo@odoo.com>
Date: Fri, 20 Sep 2024 12:17:17 +0200
Subject: [PATCH 14/45] [FIX] runbot_merge: don't over-bump timeout

By updating the staging timeout every time we run `_compute_state` and
still have a `pending` status, we'd actually bump the timeout *on
every success CI* except for the last one. Which was never the
intention and can add an hour or two to the mergebot-side timeout.

Instead, add an `updated_at` attribute to statuses (name taken from
the webhook payload even though we don't use that), read it when we
see `pending` required statuses, and update the staging timeout based
on that if necessary.

That way as long as we don't get *new* pending statuses, the timeout
doesn't get updated.

Fixes #952
---
 runbot_merge/controllers/__init__.py |  4 ++-
 runbot_merge/models/pull_requests.py | 46 ++++++++++++++++------------
 runbot_merge/tests/test_basic.py     | 23 +++++++++-----
 3 files changed, 46 insertions(+), 27 deletions(-)

diff --git a/runbot_merge/controllers/__init__.py b/runbot_merge/controllers/__init__.py
index 5dac8125..17b8b005 100644
--- a/runbot_merge/controllers/__init__.py
+++ b/runbot_merge/controllers/__init__.py
@@ -2,6 +2,7 @@ import hashlib
 import hmac
 import logging
 import json
+from datetime import datetime
 
 import sentry_sdk
 import werkzeug.exceptions
@@ -358,7 +359,8 @@ def handle_status(env, event):
         event['context']: {
             'state': event['state'],
             'target_url': event['target_url'],
-            'description': event['description']
+            'description': event['description'],
+            'updated_at': datetime.now().isoformat(timespec='seconds'),
         }
     })
     # create status, or merge update into commit *unless* the update is already
diff --git a/runbot_merge/models/pull_requests.py b/runbot_merge/models/pull_requests.py
index 577866cb..971ba99c 100644
--- a/runbot_merge/models/pull_requests.py
+++ b/runbot_merge/models/pull_requests.py
@@ -2133,6 +2133,7 @@ class Stagings(models.Model):
         if not self.env.user.has_group('runbot_merge.status'):
             raise AccessError("You are not allowed to post a status.")
 
+        now = datetime.datetime.now().isoformat(timespec='seconds')
         for s in self:
             if not s.target.project_id.staging_rpc:
                 continue
@@ -2145,6 +2146,7 @@ class Stagings(models.Model):
                 'state': status,
                 'target_url': target_url,
                 'description': description,
+                'updated_at': now,
             }
             s.statuses_cache = json.dumps(st)
 
@@ -2158,39 +2160,45 @@ class Stagings(models.Model):
         "heads.repository_id.status_ids.context",
     )
     def _compute_state(self):
-        for s in self:
-            if s.state != 'pending':
+        for staging in self:
+            if staging.state != 'pending':
                 continue
 
             # maps commits to the statuses they need
             required_statuses = [
-                (h.commit_id.sha, h.repository_id.status_ids._for_staging(s).mapped('context'))
-                for h in s.heads
+                (h.commit_id.sha, h.repository_id.status_ids._for_staging(staging).mapped('context'))
+                for h in staging.heads
             ]
-            cmap = json.loads(s.statuses_cache)
+            cmap = json.loads(staging.statuses_cache)
 
-            update_timeout_limit = False
-            st = 'success'
+            last_pending = ""
+            state = 'success'
             for head, reqs in required_statuses:
                 statuses = cmap.get(head) or {}
-                for v in map(lambda n: statuses.get(n, {}).get('state'), reqs):
-                    if st == 'failure' or v in ('error', 'failure'):
-                        st = 'failure'
+                for status in (statuses.get(n, {}) for n in reqs):
+                    v = status.get('state')
+                    if state == 'failure' or v in ('error', 'failure'):
+                        state = 'failure'
                     elif v is None:
-                        st = 'pending'
+                        state = 'pending'
                     elif v == 'pending':
-                        st = 'pending'
-                        update_timeout_limit = True
+                        state = 'pending'
+                        last_pending = max(last_pending, status.get('updated_at', ''))
                     else:
                         assert v == 'success'
 
-            s.state = st
-            if s.state != 'pending':
+            staging.state = state
+            if staging.state != 'pending':
                 self.env.ref("runbot_merge.merge_cron")._trigger()
-            if update_timeout_limit:
-                s.timeout_limit = datetime.datetime.now() + datetime.timedelta(minutes=s.target.project_id.ci_timeout)
-                self.env.ref("runbot_merge.merge_cron")._trigger(s.timeout_limit)
-                _logger.debug("%s got pending status, bumping timeout to %s (%s)", self, s.timeout_limit, cmap)
+
+            if last_pending:
+                timeout = datetime.datetime.fromisoformat(last_pending) \
+                      + datetime.timedelta(minutes=staging.target.project_id.ci_timeout)
+
+                if timeout > staging.timeout_limit:
+                    staging.timeout_limit = timeout
+                    self.env.ref("runbot_merge.merge_cron")._trigger(timeout)
+                    _logger.debug("%s got pending status, bumping timeout to %s", staging, timeout)
 
     def action_cancel(self):
         w = self.env['runbot_merge.stagings.cancel'].create({
diff --git a/runbot_merge/tests/test_basic.py b/runbot_merge/tests/test_basic.py
index e59b8bf4..440518e6 100644
--- a/runbot_merge/tests/test_basic.py
+++ b/runbot_merge/tests/test_basic.py
@@ -605,7 +605,6 @@ def test_staging_ci_timeout(env, repo, config, page, update_op: Callable[[int],
     assert dangerbox
     assert dangerbox[0].text == 'timed out (>60 minutes)'
 
-@pytest.mark.defaultstatuses
 def test_timeout_bump_on_pending(env, repo, config):
     with repo:
         [m, c] = repo.make_commits(
@@ -615,8 +614,9 @@ def test_timeout_bump_on_pending(env, repo, config):
         )
         repo.make_ref('heads/master', m)
 
-        prx = repo.make_pr(title='title', body='body', target='master', head=c)
-        repo.post_status(prx.head, 'success')
+        prx = repo.make_pr(target='master', head=c)
+        repo.post_status(prx.head, 'success', 'ci/runbot')
+        repo.post_status(prx.head, 'success', 'legal/cla')
         prx.post_comment('hansen r+', config['role_reviewer']['token'])
     env.run_crons()
 
@@ -624,9 +624,18 @@ def test_timeout_bump_on_pending(env, repo, config):
     old_timeout = odoo.fields.Datetime.to_string(datetime.datetime.now() - datetime.timedelta(days=15))
     st.timeout_limit = old_timeout
     with repo:
-        repo.post_status('staging.master', 'pending')
+        repo.post_status('staging.master', 'pending', 'ci/runbot')
     env.run_crons(None)
-    assert st.timeout_limit > old_timeout
+    assert st.timeout_limit > old_timeout, "receiving a pending status should bump the timeout"
+
+    st.timeout_limit = old_timeout
+    # clear the statuses cache to remove the memoized status times
+    st.statuses_cache = "{}"
+    st.commit_ids.statuses = "{}"
+    with repo:
+        repo.post_status('staging.master', 'success', 'legal/cla')
+    env.run_crons(None)
+    assert st.timeout_limit == old_timeout, "receiving a success status should *not* bump the timeout"
 
 def test_staging_ci_failure_single(env, repo, users, config, page):
     """ on failure of single-PR staging, mark & notify failure
@@ -3327,8 +3336,8 @@ class TestUnknownPR:
 
         c = env['runbot_merge.commit'].search([('sha', '=', prx.head)])
         assert json.loads(c.statuses) == {
-            'legal/cla': {'state': 'success', 'target_url': None, 'description': None},
-            'ci/runbot': {'state': 'success', 'target_url': 'http://example.org/wheee', 'description': None}
+            'legal/cla': {'state': 'success', 'target_url': None, 'description': None, 'updated_at': matches("$$")},
+            'ci/runbot': {'state': 'success', 'target_url': 'http://example.org/wheee', 'description': None, 'updated_at': matches("$$")}
         }
         assert prx.comments == [
             seen(env, prx, users),

From 98868b520046f4dae28507b64ea96298cd6fdd04 Mon Sep 17 00:00:00 2001
From: Xavier Morel <xmo@odoo.com>
Date: Tue, 24 Sep 2024 09:53:32 +0200
Subject: [PATCH 15/45] [IMP] runbot_merge: add notifications on inactive
 branch interactions

Add warnings when trying to send comments / commands to PRs targeting
inactive branches.

This was missing leading to confusion, as one warning is clearly not
enough.

Fixes #941
---
 runbot_merge/models/pull_requests.py       |  44 +++++---
 runbot_merge/tests/test_disabled_branch.py | 114 +++++++++++----------
 2 files changed, 91 insertions(+), 67 deletions(-)

diff --git a/runbot_merge/models/pull_requests.py b/runbot_merge/models/pull_requests.py
index 971ba99c..d04e2a72 100644
--- a/runbot_merge/models/pull_requests.py
+++ b/runbot_merge/models/pull_requests.py
@@ -620,26 +620,46 @@ class PullRequests(models.Model):
             return json.loads(self.overrides)
         return {}
 
-    def _get_or_schedule(self, repo_name, number, *, target=None, closing=False):
+    def _get_or_schedule(self, repo_name, number, *, target=None, closing=False) -> PullRequests | None:
         repo = self.env['runbot_merge.repository'].search([('name', '=', repo_name)])
         if not repo:
-            return
-
-        if target and not repo.project_id._has_branch(target):
-            self.env.ref('runbot_merge.pr.fetch.unmanaged')._send(
-                repository=repo,
-                pull_request=number,
-                format_args={'repository': repo, 'branch': target, 'number': number}
+            source = self.env['runbot_merge.events_sources'].search([('repository', '=', repo_name)])
+            _logger.warning(
+                "Got a PR notification for unknown repository %s (source %s)",
+                repo_name, source,
             )
             return
 
-        pr = self.search([
-            ('repository', '=', repo.id),
-            ('number', '=', number,)
-        ])
+        if target:
+            b = self.env['runbot_merge.branch'].with_context(active_test=False).search([
+                ('project_id', '=', repo.project_id.id),
+                ('name', '=', target),
+            ])
+            tmpl = None if b.active \
+                else 'runbot_merge.handle.branch.inactive' if b\
+                else 'runbot_merge.pr.fetch.unmanaged'
+        else:
+            tmpl = None
+
+        pr = self.search([('repository', '=', repo.id), ('number', '=', number)])
+        if pr and not pr.target.active:
+            tmpl = 'runbot_merge.handle.branch.inactive'
+            target = pr.target.name
+
+        if tmpl and not closing:
+            self.env.ref(tmpl)._send(
+                repository=repo,
+                pull_request=number,
+                format_args={'repository': repo_name, 'branch': target, 'number': number},
+            )
+
         if pr:
             return pr
 
+        # if the branch is unknown or inactive, no need to fetch the PR
+        if tmpl:
+            return
+
         Fetch = self.env['runbot_merge.fetch_job']
         if Fetch.search([('repository', '=', repo.id), ('number', '=', number)]):
             return
diff --git a/runbot_merge/tests/test_disabled_branch.py b/runbot_merge/tests/test_disabled_branch.py
index 88e5e490..eaf2e9f6 100644
--- a/runbot_merge/tests/test_disabled_branch.py
+++ b/runbot_merge/tests/test_disabled_branch.py
@@ -1,8 +1,11 @@
 import pytest
 
-from utils import seen, Commit, pr_page
+from utils import seen, Commit, pr_page, to_pr
 
-def test_existing_pr_disabled_branch(env, project, make_repo, setreviewers, config, users, page):
+
+pytestmark = pytest.mark.defaultstatuses
+
+def test_existing_pr_disabled_branch(env, project, repo, config, users, page):
     """ PRs to disabled branches are ignored, but what if the PR exists *before*
     the branch is disabled?
     """
@@ -10,20 +13,11 @@ def test_existing_pr_disabled_branch(env, project, make_repo, setreviewers, conf
     # new work
     assert env['base'].run_crons()
 
-    repo = make_repo('repo')
-    project.branch_ids.sequence = 0
     project.write({'branch_ids': [
+        (1, project.branch_ids.id, {'sequence': 0}),
         (0, 0, {'name': 'other', 'sequence': 1}),
         (0, 0, {'name': 'other2', 'sequence': 2}),
     ]})
-    repo_id = env['runbot_merge.repository'].create({
-        'project_id': project.id,
-        'name': repo.name,
-        'status_ids': [(0, 0, {'context': 'status'})],
-        'group_id': False,
-    })
-    setreviewers(*project.repo_ids)
-    env['runbot_merge.events_sources'].create({'repository': repo.name})
 
     with repo:
         [m] = repo.make_commits(None, Commit('root', tree={'a': '1'}), ref='heads/master')
@@ -32,14 +26,11 @@ def test_existing_pr_disabled_branch(env, project, make_repo, setreviewers, conf
 
         [c] = repo.make_commits(ot, Commit('wheee', tree={'b': '2'}))
         pr = repo.make_pr(title="title", body='body', target='other', head=c)
-        repo.post_status(c, 'success', 'status')
+        repo.post_status(c, 'success')
         pr.post_comment('hansen r+', config['role_reviewer']['token'])
     env.run_crons()
 
-    pr_id = env['runbot_merge.pull_requests'].search([
-        ('repository', '=', repo_id.id),
-        ('number', '=', pr.number),
-    ])
+    pr_id = to_pr(env, pr)
     branch_id = pr_id.target
     assert pr_id.staging_id
     staging_id = branch_id.active_staging_id
@@ -47,9 +38,6 @@ def test_existing_pr_disabled_branch(env, project, make_repo, setreviewers, conf
 
     # staging of `pr` should have generated a staging branch
     _ = repo.get_ref('heads/staging.other')
-    # stagings should not need a tmp branch anymore, so this should not exist
-    with pytest.raises(AssertionError, match=r'Not Found'):
-        repo.get_ref('heads/tmp.other')
 
     # disable branch "other"
     branch_id.active = False
@@ -74,7 +62,7 @@ def test_existing_pr_disabled_branch(env, project, make_repo, setreviewers, conf
     [target] = p.cssselect('table tr.bg-info')
     assert 'inactive' in target.classes
     assert target[0].text_content() == "other"
-
+    env.run_crons()
     assert pr.comments == [
         (users['reviewer'], "hansen r+"),
         seen(env, pr, users),
@@ -82,37 +70,38 @@ def test_existing_pr_disabled_branch(env, project, make_repo, setreviewers, conf
     ]
 
     with repo:
-        [c2] = repo.make_commits(ot, Commit('wheee', tree={'b': '3'}))
-        repo.update_ref(pr.ref, c2, force=True)
+        [c2] = repo.make_commits(ot, Commit('wheee', tree={'b': '3'}), ref=pr.ref, make=False)
+    env.run_crons()
+    assert pr.comments[3] == (
+        users['user'],
+        "This PR targets the disabled branch {repository}:{target}, it needs to be retargeted before it can be merged.".format(
+            repository=repo.name,
+            target="other",
+        )
+    )
     assert pr_id.head == c2, "pr should be aware of its update"
 
     with repo:
         pr.base = 'other2'
-        repo.post_status(c2, 'success', 'status')
+        repo.post_status(c2, 'success')
         pr.post_comment('hansen rebase-ff r+', config['role_reviewer']['token'])
     env.run_crons()
 
+    assert pr.comments[4:] == [
+        (users['reviewer'], 'hansen rebase-ff r+'),
+        (users['user'], "Merge method set to rebase and fast-forward."),
+    ]
+
     assert pr_id.state == 'ready'
     assert pr_id.target == env['runbot_merge.branch'].search([('name', '=', 'other2')])
     assert pr_id.staging_id
 
     # staging of `pr` should have generated a staging branch
     _ = repo.get_ref('heads/staging.other2')
-    # stagings should not need a tmp branch anymore, so this should not exist
-    with pytest.raises(AssertionError, match=r'Not Found'):
-        repo.get_ref('heads/tmp.other2')
 
-def test_new_pr_no_branch(env, project, make_repo, setreviewers, users):
+def test_new_pr_no_branch(env, project, repo, users):
     """ A new PR to an *unknown* branch should be ignored and warn
     """
-    repo = make_repo('repo')
-    repo_id = env['runbot_merge.repository'].create({
-        'project_id': project.id,
-        'name': repo.name,
-        'status_ids': [(0, 0, {'context': 'status'})]
-    })
-    setreviewers(*project.repo_ids)
-    env['runbot_merge.events_sources'].create({'repository': repo.name})
 
     with repo:
         [m] = repo.make_commits(None, Commit('root', tree={'a': '1'}), ref='heads/master')
@@ -123,30 +112,18 @@ def test_new_pr_no_branch(env, project, make_repo, setreviewers, users):
     env.run_crons()
 
     assert not env['runbot_merge.pull_requests'].search([
-        ('repository', '=', repo_id.id),
+        ('repository', '=', project.repo_ids.id),
         ('number', '=', pr.number),
     ]), "the PR should not have been created in the backend"
     assert pr.comments == [
         (users['user'], "This PR targets the un-managed branch %s:other, it needs to be retargeted before it can be merged." % repo.name),
     ]
 
-def test_new_pr_disabled_branch(env, project, make_repo, setreviewers, users):
+def test_new_pr_disabled_branch(env, project, repo, users):
     """ A new PR to a *disabled* branch should be accepted (rather than ignored)
     but should warn
     """
-    repo = make_repo('repo')
-    repo_id = env['runbot_merge.repository'].create({
-        'project_id': project.id,
-        'name': repo.name,
-        'status_ids': [(0, 0, {'context': 'status'})]
-    })
-    env['runbot_merge.branch'].create({
-        'project_id': project.id,
-        'name': 'other',
-        'active': False,
-    })
-    setreviewers(*project.repo_ids)
-    env['runbot_merge.events_sources'].create({'repository': repo.name})
+    project.write({'branch_ids': [(0, 0, {'name': 'other', 'active': False})]})
 
     with repo:
         [m] = repo.make_commits(None, Commit('root', tree={'a': '1'}), ref='heads/master')
@@ -156,13 +133,40 @@ def test_new_pr_disabled_branch(env, project, make_repo, setreviewers, users):
         pr = repo.make_pr(title="title", body='body', target='other', head=c)
     env.run_crons()
 
-    pr_id = env['runbot_merge.pull_requests'].search([
-        ('repository', '=', repo_id.id),
-        ('number', '=', pr.number),
-    ])
+    pr_id = to_pr(env, pr)
     assert pr_id, "the PR should have been created in the backend"
     assert pr_id.state == 'opened'
     assert pr.comments == [
         (users['user'], "This PR targets the disabled branch %s:other, it needs to be retargeted before it can be merged." % repo.name),
         seen(env, pr, users),
     ]
+
+def test_review_disabled_branch(env, project, repo, users, config):
+    with repo:
+        [m] = repo.make_commits(None, Commit("init", tree={'m': 'm'}), ref='heads/master')
+
+        [c] = repo.make_commits(m, Commit('pr', tree={'m': 'n'}))
+        pr = repo.make_pr(target="master", head=c)
+    env.run_crons()
+    target = project.branch_ids
+    target.active = False
+    env.run_crons()
+    with repo:
+        pr.post_comment("A normal comment", config['role_other']['token'])
+    with repo:
+        pr.post_comment("hansen r+", config['role_reviewer']['token'])
+    env.run_crons()
+
+    assert pr.comments == [
+        seen(env, pr, users),
+        (users['user'], "@{user} the target branch {target!r} has been disabled, you may want to close this PR.".format(
+            **users,
+            target=target.name,
+        )),
+        (users['other'], "A normal comment"),
+        (users['reviewer'], "hansen r+"),
+        (users['user'], "This PR targets the disabled branch {repository}:{target}, it needs to be retargeted before it can be merged.".format(
+            repository=repo.name,
+            target=target.name,
+        )),
+    ]

From 26882c42aad3c61c2ea988414c30f08e86561ad9 Mon Sep 17 00:00:00 2001
From: Xavier Morel <xmo@odoo.com>
Date: Fri, 27 Sep 2024 12:36:02 +0200
Subject: [PATCH 16/45] [FIX] warning in test logs

---
 conftest.py | 1 +
 1 file changed, 1 insertion(+)

diff --git a/conftest.py b/conftest.py
index 539cf272..3226d824 100644
--- a/conftest.py
+++ b/conftest.py
@@ -477,6 +477,7 @@ class IrCron(models.Model):
         (mod / '__manifest__.py').write_text(pprint.pformat({
             'name': 'dummy saas_worker',
             'version': '1.0',
+            'license': 'BSD-0-Clause',
         }), encoding='utf-8')
         (mod / 'util.py').write_text("""\
 def from_role(*_, **__):

From ef22529620afbae3bfe4a398de4a2416a511a7b7 Mon Sep 17 00:00:00 2001
From: Xavier Morel <xmo@odoo.com>
Date: Fri, 27 Sep 2024 12:36:50 +0200
Subject: [PATCH 17/45] [ADD] support for recursive tree to the test GH proxy

---
 conftest.py | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/conftest.py b/conftest.py
index 3226d824..7163552f 100644
--- a/conftest.py
+++ b/conftest.py
@@ -773,19 +773,20 @@ class Repo:
             parents=[p['sha'] for p in gh_commit['parents']],
         )
 
-    def read_tree(self, commit):
+    def read_tree(self, commit: Commit, *, recursive=False) -> dict[str, str]:
         """ read tree object from commit
-
-        :param Commit commit:
-        :rtype: Dict[str, str]
         """
-        r = self._session.get('https://api.github.com/repos/{}/git/trees/{}'.format(self.name, commit.tree))
+        r = self._session.get(
+            'https://api.github.com/repos/{}/git/trees/{}'.format(self.name, commit.tree),
+            params={'recursive': '1'} if recursive else None,
+        )
         assert 200 <= r.status_code < 300, r.text
 
         # read tree's blobs
         tree = {}
         for t in r.json()['tree']:
-            assert t['type'] == 'blob', "we're *not* doing recursive trees in test cases"
+            if t['type'] != 'blob':
+                continue # only keep blobs
             r = self._session.get('https://api.github.com/repos/{}/git/blobs/{}'.format(self.name, t['sha']))
             assert 200 <= r.status_code < 300, r.text
             tree[t['path']] = base64.b64decode(r.json()['content']).decode()

From 8f27773f8d158d53a7ac37197b48f173d0c5411c Mon Sep 17 00:00:00 2001
From: Xavier Morel <xmo@odoo.com>
Date: Fri, 27 Sep 2024 12:37:49 +0200
Subject: [PATCH 18/45] [IMP] forwardport: surfacing of modify/delete conflicts

Given branch A, and branch B forked from it. If B removes a file which
a PR to A later modifies, on forward port Git generates a
modify/delete conflict (as in one side modifies a file which the
other deleted).

So far so good, except while it does notify the caller of the issue
the modified file is just dumped as-is into the working copy (or
commit), which essentially resurrects it.

This is an issue, *especially* as the file is already part of a
commit (rather tan just a U local file), if the developer fixes the
conflict "in place" rather than re-doing the forward-port from scratch
it's easy to miss the reincarnated file (and possibly the changes that
were part of it too), which at best leaves parasitic dead code in the
working copy. There is also no easy way for the runbot to check it as
adding unimported standalone files while rare is not unknown
e.g. utility scripts (to say nothing of JS where we can't really track
the usages / imports at all).

To resolve this issue, during conflict generation post-process
modify/delete to insert artificial conflict markers, the file should
be syntactically invalid so linters / checkers should complain, and
the minimal check has a step looking for conflict markers which should
fire and prevent merging the PR.

Fixes #896
---
 forwardport/models/project.py       |  22 +++++-
 forwardport/tests/test_conflicts.py | 114 ++++++++++++++++++++++++----
 runbot_merge/git.py                 |  44 ++++++++++-
 3 files changed, 161 insertions(+), 19 deletions(-)

diff --git a/forwardport/models/project.py b/forwardport/models/project.py
index 0031b61b..f26ebfd5 100644
--- a/forwardport/models/project.py
+++ b/forwardport/models/project.py
@@ -346,7 +346,7 @@ class PullRequests(models.Model):
                 '--merge-base', commits[0]['parents'][0]['sha'],
                 target_branch.name,
                 root.head,
-            )
+            ).stdout.decode().splitlines(keepends=False)[0]
             # if there was a single commit, reuse its message when committing
             # the conflict
             if len(commits) == 1:
@@ -362,10 +362,28 @@ stderr:
 {err}
 """
 
+            # if a file is modified by the original PR and added by the forward
+            # port / conflict it's a modify/delete conflict: the file was
+            # deleted in the target branch, and the update (modify) in the
+            # original PR causes it to be added back
+            original_modified = set(conf.diff(
+                "--diff-filter=M", "--name-only",
+                "--merge-base", root.target.name,
+                root.head,
+            ).stdout.decode().splitlines(keepends=False))
+            conflict_added = set(conf.diff(
+                "--diff-filter=A", "--name-only",
+                target_branch.name,
+                tree,
+            ).stdout.decode().splitlines(keepends=False))
+            if modify_delete := (conflict_added & original_modified):
+                # rewrite the tree with conflict markers added to modify/deleted blobs
+                tree = conf.modify_delete(tree, modify_delete)
+
             target_head = source.stdout().rev_parse(f"refs/heads/{target_branch.name}")\
                 .stdout.decode().strip()
             commit = conf.commit_tree(
-                tree=tree.stdout.decode().splitlines(keepends=False)[0],
+                tree=tree,
                 parents=[target_head],
                 message=str(msg),
                 author=author,
diff --git a/forwardport/tests/test_conflicts.py b/forwardport/tests/test_conflicts.py
index 3c0bbda6..f6e23bbf 100644
--- a/forwardport/tests/test_conflicts.py
+++ b/forwardport/tests/test_conflicts.py
@@ -262,7 +262,7 @@ def test_massive_conflict(env, config, make_repo):
 
 
 def test_conflict_deleted(env, config, make_repo):
-    prod, other = make_basic(env, config, make_repo)
+    prod, other = make_basic(env, config, make_repo, statuses="default")
     # remove f from b
     with prod:
         prod.make_commits(
@@ -277,18 +277,12 @@ def test_conflict_deleted(env, config, make_repo):
             ref='heads/conflicting'
         )
         pr = prod.make_pr(target='a', head='conflicting')
-        prod.post_status(p_0, 'success', 'legal/cla')
-        prod.post_status(p_0, 'success', 'ci/runbot')
+        prod.post_status(p_0, 'success')
         pr.post_comment('hansen r+', config['role_reviewer']['token'])
 
     env.run_crons()
     with prod:
-        prod.post_status('staging.a', 'success', 'legal/cla')
-        prod.post_status('staging.a', 'success', 'ci/runbot')
-
-    env.run_crons()
-    # wait a bit for PR webhook... ?
-    time.sleep(5)
+        prod.post_status('staging.a', 'success')
     env.run_crons()
 
     # should have created a new PR
@@ -300,9 +294,14 @@ def test_conflict_deleted(env, config, make_repo):
         'g': 'c',
     }
     assert pr1.state == 'opened'
-    # NOTE: no actual conflict markers because pr1 essentially adds f de-novo
     assert prod.read_tree(prod.commit(pr1.head)) == {
-        'f': 'xxx',
+        'f': matches("""\
+<<<\x3c<<< $$
+||||||| $$
+=======
+xxx
+>>>\x3e>>> $$
+"""),
         'g': 'c',
     }
 
@@ -333,6 +332,89 @@ def test_conflict_deleted(env, config, make_repo):
     }
     assert pr1.state == 'opened', "state should be open still"
 
+
+def test_conflict_deleted_deep(env, config, make_repo):
+    """ Same as the previous one but files are deeper than toplevel, and we only
+    want to see if the conflict post-processing works.
+    """
+    # region: setup
+    prod = make_repo("test")
+    env['runbot_merge.events_sources'].create({'repository': prod.name})
+    with prod:
+        [a, b] = prod.make_commits(
+            None,
+            Commit("a", tree={
+                "foo/bar/baz": "1",
+                "foo/bar/qux": "1",
+                "corge/grault": "1",
+            }),
+            Commit("b", tree={"foo/bar/qux": "2"}, reset=True),
+        )
+        prod.make_ref("heads/a", a)
+        prod.make_ref("heads/b", b)
+
+    project = env['runbot_merge.project'].create({
+        'name': "test",
+        'github_token': config['github']['token'],
+        'github_prefix': 'hansen',
+        'fp_github_token': config['github']['token'],
+        'fp_github_name': 'herbert',
+        'branch_ids': [
+            (0, 0, {'name': 'a', 'sequence': 100}),
+            (0, 0, {'name': 'b', 'sequence': 80}),
+        ],
+        "repo_ids": [
+            (0, 0, {
+                'name': prod.name,
+                'required_statuses': "default",
+                'fp_remote_target': prod.fork().name,
+                'group_id': False,
+            })
+        ]
+    })
+    env['res.partner'].search([
+        ('github_login', '=', config['role_reviewer']['user'])
+    ]).write({
+        'review_rights': [(0, 0, {'repository_id': project.repo_ids.id, 'review': True})]
+    })
+    # endregion
+
+    with prod:
+        prod.make_commits(
+            'a',
+            Commit("c", tree={
+                "foo/bar/baz": "2",
+                "corge/grault": "insert funny number",
+            }),
+            ref="heads/conflicting",
+        )
+        pr = prod.make_pr(target='a', head='conflicting')
+        prod.post_status("conflicting", "success")
+        pr.post_comment("hansen r+", config['role_reviewer']['token'])
+    env.run_crons()
+
+    with prod:
+        prod.post_status('staging.a', 'success')
+    env.run_crons()
+    _, pr1 = env['runbot_merge.pull_requests'].search([], order='number')
+    assert prod.read_tree(prod.commit(pr1.head), recursive=True) == {
+        "foo/bar/qux": "2",
+        "foo/bar/baz": """\
+<<<<<<< HEAD
+||||||| MERGE BASE
+=======
+2
+>>>>>>> FORWARD PORTED
+""",
+        "corge/grault": """\
+<<<<<<< HEAD
+||||||| MERGE BASE
+=======
+insert funny number
+>>>>>>> FORWARD PORTED
+"""
+    }, "the conflicting file should have had conflict markers fixed in"
+
 def test_multiple_commits_same_authorship(env, config, make_repo):
     """ When a PR has multiple commits by the same author and its
     forward-porting triggers a conflict, the resulting (squashed) conflict
@@ -436,13 +518,13 @@ def test_multiple_commits_different_authorship(env, config, make_repo, users, ro
         "author set to the bot but an empty email"
     assert get(c.committer) == (bot, '')
 
-    assert re.match(r'''<<<\x3c<<< HEAD
+    assert prod.read_tree(c)['g'] == matches('''<<<\x3c<<< b
 b
-|||||||| parent of [\da-f]{7,}.*
+||||||| $$
 =======
 2
->>>\x3e>>> [\da-f]{7,}.*
-''', prod.read_tree(c)['g'])
+>>>\x3e>>> $$
+''')
 
     # I'd like to fix the conflict so everything is clean and proper *but*
     # github's API apparently rejects creating commits with an empty email.
@@ -459,7 +541,7 @@ b
 
     assert pr2.comments == [
         seen(env, pr2, users),
-        (users['user'], matches('@%s @%s $$CONFLICT' % (users['user'], users['reviewer']))),
+        (users['user'], matches('@%(user)s @%(reviewer)s $$CONFLICT' % users)),
         (users['reviewer'], 'hansen r+'),
         (users['user'], f"@{users['user']} @{users['reviewer']} unable to stage: "
                         "All commits must have author and committer email, "
diff --git a/runbot_merge/git.py b/runbot_merge/git.py
index caaa6e5d..d9937329 100644
--- a/runbot_merge/git.py
+++ b/runbot_merge/git.py
@@ -6,7 +6,8 @@ import pathlib
 import resource
 import stat
 import subprocess
-from typing import Optional, TypeVar, Union, Sequence, Tuple, Dict
+from operator import methodcaller
+from typing import Optional, TypeVar, Union, Sequence, Tuple, Dict, Iterable
 
 from odoo.tools.appdirs import user_cache_dir
 from .github import MergeError, PrCommit
@@ -245,6 +246,47 @@ class Repo:
             *itertools.chain.from_iterable(('-p', p) for p in parents),
         )
 
+
+    def modify_delete(self, tree: str, files: Iterable[str]) -> str:
+        """Updates ``files`` in ``tree`` to add conflict markers to show them
+        as being modify/delete-ed, rather than have only the original content.
+
+        This is because having just content in a valid file is easy to miss,
+        causing file resurrections as they get committed rather than re-removed.
+
+        TODO: maybe extract the diff information compared to before they were removed? idk
+        """
+        # FIXME: either ignore or process binary files somehow (how does git show conflicts in binary files?)
+        repo = self.stdout().with_config(stderr=None, text=True, check=False, encoding="utf-8")
+        for f in files:
+            contents = repo.cat_file("-p", f"{tree}:{f}").stdout
+            # decorate the contents as if HEAD and BASE are empty
+            oid = repo\
+                .with_config(input=f"""\
+<<<\x3c<<< HEAD
+||||||| MERGE BASE
+=======
+{contents}
+>>>\x3e>>> FORWARD PORTED
+""")\
+                .hash_object("-w", "--stdin", "--path", f)\
+                .stdout.strip()
+
+            # we need to rewrite every tree from the parent of `f`
+            while f:
+                f, _, local = f.rpartition("/")
+                # tree to update, `{tree}:` works as an alias for tree
+                lstree = repo.ls_tree(f"{tree}:{f}").stdout.splitlines(keepends=False)
+                new_tree = "".join(
+                    # tab before name is critical to the format
+                    f"{mode} {typ} {oid if name == local else sha}\t{name}\n"
+                    for mode, typ, sha, name in map(methodcaller("split", None, 3), lstree)
+                )
+                oid = repo.with_config(input=new_tree, check=True).mktree().stdout.strip()
+            tree = oid
+        return tree
+
+
 def check(p: subprocess.CompletedProcess) -> subprocess.CompletedProcess:
     if not p.returncode:
         return p

From 430ccab2cbf1c57b351412f3b54030ad565bbef5 Mon Sep 17 00:00:00 2001
From: Xavier Morel <xmo@odoo.com>
Date: Fri, 27 Sep 2024 12:53:51 +0200
Subject: [PATCH 19/45] [IMP] runbot_merge: suppress view validation warning

This is a dumb false positive, kill it.
---
 runbot_merge/models/__init__.py   |  1 +
 runbot_merge/models/ir_ui_view.py | 17 +++++++++++++++++
 2 files changed, 18 insertions(+)
 create mode 100644 runbot_merge/models/ir_ui_view.py

diff --git a/runbot_merge/models/__init__.py b/runbot_merge/models/__init__.py
index f15f98f2..c1cee75c 100644
--- a/runbot_merge/models/__init__.py
+++ b/runbot_merge/models/__init__.py
@@ -1,4 +1,5 @@
 from . import ir_actions
+from . import ir_ui_view
 from . import res_partner
 from . import project
 from . import pull_requests
diff --git a/runbot_merge/models/ir_ui_view.py b/runbot_merge/models/ir_ui_view.py
new file mode 100644
index 00000000..fa915348
--- /dev/null
+++ b/runbot_merge/models/ir_ui_view.py
@@ -0,0 +1,17 @@
+from odoo import models
+
+
+class View(models.Model):
+    _inherit = 'ir.ui.view'
+
+    def _log_view_warning(self, msg, node):
+        """The view validator is dumb and triggers a warning because there's a
+        `field.btn`, even though making a `field[widget=url]` (which renders as
+        a link) look like a button is perfectly legitimate.
+
+        Suppress that warning.
+        """
+        if node.tag == 'field' and node.get('widget') == 'url' and "button/submit/reset" in msg:
+            return
+
+        super()._log_view_warning(msg, node)

From aac987f2bbea3164e60a94bb2460c3a2e2f0ac31 Mon Sep 17 00:00:00 2001
From: Xavier Morel <xmo@odoo.com>
Date: Fri, 27 Sep 2024 14:13:43 +0200
Subject: [PATCH 20/45] [FIX] runbot_merge: dashboard display nits

- fix staging reasons containing escaped quotes (would render as
  `&#32;` to the end user)
- remove extra spacing around PR title @title/popovers
- simplify a few view conditionals through judicious use of `t-elif`
  and nesting
- make `staging_end` non-computed as it's not computed anymore, just
  set if and when the staging gets disabled
  (146564a90a7c4ee6bc111380886e2e1ec1dabe7b)
---
 runbot_merge/models/pull_requests.py |  2 +-
 runbot_merge/views/templates.xml     | 42 +++++++++++++---------------
 2 files changed, 21 insertions(+), 23 deletions(-)

diff --git a/runbot_merge/models/pull_requests.py b/runbot_merge/models/pull_requests.py
index d04e2a72..810eb673 100644
--- a/runbot_merge/models/pull_requests.py
+++ b/runbot_merge/models/pull_requests.py
@@ -2048,7 +2048,7 @@ class Stagings(models.Model):
     active = fields.Boolean(default=True)
 
     staged_at = fields.Datetime(default=fields.Datetime.now, index=True)
-    staging_end = fields.Datetime(store=True, compute='_compute_state')
+    staging_end = fields.Datetime(store=True)
     staging_duration = fields.Float(compute='_compute_duration')
     timeout_limit = fields.Datetime(store=True, compute='_compute_timeout_limit')
     reason = fields.Text("Reason for final state (if any)")
diff --git a/runbot_merge/views/templates.xml b/runbot_merge/views/templates.xml
index 2dff930f..2ac80f83 100644
--- a/runbot_merge/views/templates.xml
+++ b/runbot_merge/views/templates.xml
@@ -5,21 +5,18 @@
     </function>
 
     <template id="link-pr" name="create a link to `pr`">
+        <t t-set="title"
+           t-value="pr.message.split('\n', 1)[0] if pr.repository.group_id &lt;= env.user.groups_id else ''"/>
         <t t-set="title">
-            <t t-if="pr.repository.group_id &lt;= env.user.groups_id">
-                <t t-out="pr.message.split('\n', 1)[0]"/>
-            </t>
-        </t>
-        <t t-set="title">
-          <t t-if="title.strip() and pr.blocked" >
-            <t t-out="title.strip()"/>: <t t-out="pr.blocked"/>
+          <t t-if="title and pr.blocked" >
+            <t t-out="title"/>: <t t-out="pr.blocked"/>
           </t>
           <t t-else="">
-            <t t-out="pr.blocked or title.strip()"/>
+            <t t-out="pr.blocked or title"/>
           </t>
         </t>
         <a t-attf-href="https://github.com/{{ pr.repository.name }}/pull/{{ pr.number }}"
-           t-att-title="title"
+           t-att-title="title.strip()"
            t-att-target="target or None"
            t-att-class="classes or None"
         ><t t-esc="pr.display_name"/></a>
@@ -177,14 +174,15 @@
                     <t t-if="staging_index >= 4">d-none d-lg-block</t>
                 </t>
                 <t t-set="title">
-                    <t t-if="staging.state == 'ff_failed'">fast forward failed (<t t-esc="staging.reason"/>)</t>
-                    <t t-if="staging.state == 'pending'">last status</t>
+                    <t t-if="staging.state == 'success'"/>
+                    <t t-else="">
+                        <t t-if="staging.state == 'pending'">last status</t
+                        ><t t-elif="staging.state == 'ff_failed'">fast forward failed (<t t-out="(staging.reason or '').replace('\'', '')"/>)</t
+                        ><t t-else="" t-out="(staging.reason or '').replace('\'', '')"
+                        /> at <t t-out="(staging.staging_end or staging.write_date).replace(microsecond=0)"/>Z
+                    </t>
                 </t>
-                <!-- separate concatenation to avoid having line-break in title as some browsers trigger it -->
-                <!-- write-date may have microsecond precision, strip that information -->
-                <!-- use write-date under assumption that a staging is last modified when it ends -->
-                <t t-set="title"><t t-esc="title.strip() or staging.reason"/> at <t t-esc="staging.write_date.replace(microsecond=0)"/>Z</t>
-                <li t-attf-class="staging {{stateclass.strip()}} {{decorationclass.strip()}}" t-att-title="title">
+                <li t-attf-class="staging {{stateclass.strip()}} {{decorationclass.strip()}}" t-att-title="title.strip() or None">
                     <ul class="list-unstyled">
                         <li t-foreach="staging.batch_ids" t-as="batch" class="batch">
                             <t t-esc="batch.prs[:1].label"/>
@@ -241,14 +239,14 @@
                             </t>
                         </t>
                         <t t-set="title">
-                            <t t-if="staging.state == 'canceled'">Cancelled:
-                                <t t-esc="staging.reason"/>
+                            <t t-if="staging.state == 'ff_failed'">
+                                Fast Forward Failed
                             </t>
-                            <t t-if="staging.state == 'ff_failed'">Fast
-                                Forward Failed
+                            <t t-elif="staging.state == 'canceled'">
+                                Cancelled<t t-if="staging.reason">: <t t-out="staging.reason.replace('\'', '')"/></t>
                             </t>
-                            <t t-if="staging.state not in ('canceled', 'ff_failed')">
-                                <t t-esc="staging.reason"/>
+                            <t t-else="">
+                                <t t-out="(staging.reason or '').replace('\'', '')"/>
                             </t>
                         </t>
                         <tr t-att-class="stateclass"

From 6a1b77b92c6a3ca7fe1251389345a1ff2c7d7d90 Mon Sep 17 00:00:00 2001
From: Xavier Morel <xmo@odoo.com>
Date: Wed, 2 Oct 2024 12:14:09 +0200
Subject: [PATCH 21/45] [ADD] runbot_merge: support for unstaged patches

Unstaged changes can be useful or necessary for some tasks
e.g. absolute emergency (where even faking the state of a staging is
not really desirable, if that's even possible anymore), or changes
which are so broad they're difficult to stage (e.g. t10s updates).

Add a new object which serves as a queue for patch to direct-apply,
with support for either text patches (udiff style out of git show or
format-patch) or commits to cherry-pick. In the former case, the part
of the show / format-patch before the diff itself is used for the
commit metadata (author, committer, dates, message) whereas for the
commit version the commit itself is reused as-is.

Applied patches are simply disabled for traceability.

Fixes #926
---
 conftest.py                               |  25 +-
 runbot_merge/git.py                       |  54 ++--
 runbot_merge/models/__init__.py           |   1 +
 runbot_merge/models/patcher.py            | 311 ++++++++++++++++++++++
 runbot_merge/models/project.py            |  12 +
 runbot_merge/security/ir.model.access.csv |   4 +-
 runbot_merge/security/security.xml        |   8 +-
 runbot_merge/tests/test_patching.py       | 240 +++++++++++++++++
 runbot_merge/views/queues.xml             |  75 +++++-
 9 files changed, 695 insertions(+), 35 deletions(-)
 create mode 100644 runbot_merge/models/patcher.py
 create mode 100644 runbot_merge/tests/test_patching.py

diff --git a/conftest.py b/conftest.py
index 7163552f..38794fb4 100644
--- a/conftest.py
+++ b/conftest.py
@@ -810,6 +810,11 @@ class Repo:
         r = self._session.patch('https://api.github.com/repos/{}/git/refs/{}'.format(self.name, name), json={'sha': commit, 'force': force})
         assert r.ok, r.text
 
+    def delete_ref(self, name):
+        assert self.hook
+        r = self._session.delete(f'https://api.github.com/repos/{self.name}/git/refs/{name}')
+        assert r.ok, r.text
+
     def protect(self, branch):
         assert self.hook
         r = self._session.put('https://api.github.com/repos/{}/branches/{}/protection'.format(self.name, branch), json={
@@ -1244,13 +1249,27 @@ class LabelsProxy(collections.abc.MutableSet):
 
 class Environment:
     def __init__(self, port, db):
-        self._uid = xmlrpc.client.ServerProxy('http://localhost:{}/xmlrpc/2/common'.format(port)).authenticate(db, 'admin', 'admin', {})
-        self._object = xmlrpc.client.ServerProxy('http://localhost:{}/xmlrpc/2/object'.format(port))
+        self._port = port
         self._db = db
+        self._uid = None
+        self._password = None
+        self._object = xmlrpc.client.ServerProxy(f'http://localhost:{port}/xmlrpc/2/object')
+        self.login('admin', 'admin')
+
+    def with_user(self, login, password):
+        env = copy.copy(self)
+        env.login(login, password)
+        return env
+
+    def login(self, login, password):
+        self._password = password
+        self._uid = xmlrpc.client.ServerProxy(
+            f'http://localhost:{self._port}/xmlrpc/2/common'
+        ).authenticate(self._db, login, password, {})
 
     def __call__(self, model, method, *args, **kwargs):
         return self._object.execute_kw(
-            self._db, self._uid, 'admin',
+            self._db, self._uid, self._password,
             model, method,
             args, kwargs
         )
diff --git a/runbot_merge/git.py b/runbot_merge/git.py
index d9937329..8f2732a8 100644
--- a/runbot_merge/git.py
+++ b/runbot_merge/git.py
@@ -7,7 +7,8 @@ import resource
 import stat
 import subprocess
 from operator import methodcaller
-from typing import Optional, TypeVar, Union, Sequence, Tuple, Dict, Iterable
+from typing import Optional, TypeVar, Union, Sequence, Tuple, Dict
+from collections.abc import Iterable, Mapping, Callable
 
 from odoo.tools.appdirs import user_cache_dir
 from .github import MergeError, PrCommit
@@ -76,6 +77,7 @@ class Repo:
         config.setdefault('stderr', subprocess.PIPE)
         self._config = config
         self._params = ()
+        self.runner = subprocess.run
 
     def __getattr__(self, name: str) -> 'GitCommand':
         return GitCommand(self, name.replace('_', '-'))
@@ -86,7 +88,7 @@ class Repo:
             + tuple(itertools.chain.from_iterable(('-c', p) for p in self._params + ALWAYS))\
             + args
         try:
-            return subprocess.run(args, preexec_fn=_bypass_limits, **opts)
+            return self.runner(args, preexec_fn=_bypass_limits, **opts)
         except subprocess.CalledProcessError as e:
             stream = e.stderr or e.stdout
             if stream:
@@ -246,30 +248,14 @@ class Repo:
             *itertools.chain.from_iterable(('-p', p) for p in parents),
         )
 
-
-    def modify_delete(self, tree: str, files: Iterable[str]) -> str:
-        """Updates ``files`` in ``tree`` to add conflict markers to show them
-        as being modify/delete-ed, rather than have only the original content.
-
-        This is because having just content in a valid file is easy to miss,
-        causing file resurrections as they get committed rather than re-removed.
-
-        TODO: maybe extract the diff information compared to before they were removed? idk
-        """
+    def update_tree(self, tree: str, files: Mapping[str, Callable[[Self, str], str]]) -> str:
         # FIXME: either ignore or process binary files somehow (how does git show conflicts in binary files?)
         repo = self.stdout().with_config(stderr=None, text=True, check=False, encoding="utf-8")
-        for f in files:
-            contents = repo.cat_file("-p", f"{tree}:{f}").stdout
-            # decorate the contents as if HEAD and BASE are empty
-            oid = repo\
-                .with_config(input=f"""\
-<<<\x3c<<< HEAD
-||||||| MERGE BASE
-=======
-{contents}
->>>\x3e>>> FORWARD PORTED
-""")\
-                .hash_object("-w", "--stdin", "--path", f)\
+        for f, c in files.items():
+            new_contents = c(repo, f)
+            oid = repo \
+                .with_config(input=new_contents) \
+                .hash_object("-w", "--stdin", "--path", f) \
                 .stdout.strip()
 
             # we need to rewrite every tree from the parent of `f`
@@ -286,6 +272,26 @@ class Repo:
             tree = oid
         return tree
 
+    def modify_delete(self, tree: str, files: Iterable[str]) -> str:
+        """Updates ``files`` in ``tree`` to add conflict markers to show them
+        as being modify/delete-ed, rather than have only the original content.
+
+        This is because having just content in a valid file is easy to miss,
+        causing file resurrections as they get committed rather than re-removed.
+
+        TODO: maybe extract the diff information compared to before they were removed? idk
+        """
+        def rewriter(r: Self, f: str) -> str:
+            contents = r.cat_file("-p", f"{tree}:{f}").stdout
+            return f"""\
+<<<\x3c<<< HEAD
+||||||| MERGE BASE
+=======
+{contents}
+>>>\x3e>>> FORWARD PORTED
+"""
+        return self.update_tree(tree, dict.fromkeys(files, rewriter))
+
 
 def check(p: subprocess.CompletedProcess) -> subprocess.CompletedProcess:
     if not p.returncode:
diff --git a/runbot_merge/models/__init__.py b/runbot_merge/models/__init__.py
index c1cee75c..4a25ecd4 100644
--- a/runbot_merge/models/__init__.py
+++ b/runbot_merge/models/__init__.py
@@ -4,6 +4,7 @@ from . import res_partner
 from . import project
 from . import pull_requests
 from . import batch
+from . import patcher
 from . import project_freeze
 from . import stagings_create
 from . import staging_cancel
diff --git a/runbot_merge/models/patcher.py b/runbot_merge/models/patcher.py
new file mode 100644
index 00000000..f007db0f
--- /dev/null
+++ b/runbot_merge/models/patcher.py
@@ -0,0 +1,311 @@
+""" Implements direct (unstaged) patching.
+
+Useful for massive data changes which are a pain to merge normally but very
+unlikely to break things (e.g. i18n), fixes so urgent staging is an unacceptable
+overhead, or FBI backdoors oh wait forget about that last one.
+"""
+from __future__ import annotations
+
+import logging
+import pathlib
+import re
+import subprocess
+import tarfile
+import tempfile
+from dataclasses import dataclass
+from email import message_from_string, policy
+from email.utils import parseaddr
+from typing import Union
+
+from odoo import models, fields, api
+from odoo.exceptions import ValidationError
+from odoo.tools.mail import plaintext2html
+
+from .pull_requests import Branch
+from .. import git
+
+_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<file_from>\S+)(:?\s.*)?\n
+\+\+\+\x20b/(?P<file_to>\S+)(:?\s.*)?\n
+@@\x20-(\d+(,\d+)?)\x20\+(\d+(,\d+)?)\x20@@ # trailing garbage
+""", re.VERBOSE)
+
+
+Authorship = Union[None, tuple[str, str], tuple[str, str, str]]
+@dataclass
+class ParseResult:
+    kind: str
+    author: Authorship
+    committer: Authorship
+    message: str
+    patch: str
+
+
+def expect(line: str, starts_with: str, message: str) -> str:
+    if not line.startswith(starts_with):
+        raise ValidationError(message)
+    return line
+
+
+def parse_show(p: Patch) -> ParseResult:
+    # headers are Author, Date or Author, AuthorDate, Commit, CommitDate
+    # commit message is indented 4 spaces
+    lines = iter(p.patch.splitlines(keepends=True))
+    if not next(lines).startswith("commit "):
+        raise ValidationError("Invalid patch")
+    name, email = parseaddr(
+        expect(next(lines), "Author:", "Missing author")
+            .split(maxsplit=1)[1])
+    date: str = next(lines)
+    header, date = date.split(maxsplit=1)
+    author = (name, email, date)
+    if header.startswith("Date:"):
+        committer = author
+    elif header.startswith("AuthorDate:"):
+        commit = expect(next(lines), "Commit:", "Missing committer")
+        commit_date = expect(next(lines), "CommitDate:", "Missing commit date")
+        name, email = parseaddr(commit.split(maxsplit=1)[1])
+        committer = (name, email, commit_date.split(maxsplit=1)[1])
+    else:
+        raise ValidationError(
+            "Invalid patch: expected 'Date:' or 'AuthorDate:' pseudo-header, "
+            f"found {header}.\nOnly 'medium' and 'fuller' formats are supported")
+
+    # skip possible extra headers before the message
+    while next(lines) != ' \n':
+        continue
+
+    body = []
+    while (l := next(lines)) != ' \n':
+        body.append(l.removeprefix('    '))
+
+    # remainder should be the patch
+    patch = "".join(
+        line for line in lines
+        if not line.startswith("git --diff ")
+        if not line.startswith("index ")
+    )
+    return ParseResult(kind="show", author=author, committer=committer, message="".join(body).rstrip(), patch=patch)
+
+
+def parse_format_patch(p: Patch) -> ParseResult:
+    m = message_from_string(p.patch, policy=policy.default)
+    if m.is_multipart():
+        raise ValidationError("multipart patches are not supported.")
+
+    name, email = parseaddr(m['from'])
+    author = (name, email, m['date'])
+    msg = m['subject'].removeprefix('[PATCH] ')
+    body, _, rest = m.get_content().partition('---\n')
+    if body:
+        msg += '\n\n' + body
+
+    # split off the signature, per RFC 3676 § 4.3.
+    # leave the diffstat in as it *should* not confuse tooling?
+    patch, _, _ = rest.rpartition("-- \n")
+    # git (diff, show, format-patch) adds command and index headers to every
+    # file header, which patch(1) chokes on, strip them... but maybe this should
+    # extract the udiff sections instead?
+    patch = re.sub(
+        "^(git --diff .*|index .*)\n",
+        "",
+        patch,
+        flags=re.MULTILINE,
+    )
+    return ParseResult(kind="format-patch", author=author, committer=author, message=msg, patch=patch)
+
+
+class PatchFailure(Exception):
+    pass
+
+
+class Patch(models.Model):
+    _name = "runbot_merge.patch"
+    _inherit = ['mail.thread']
+    _description = "Unstaged direct-application patch"
+
+    active = fields.Boolean(default=True, tracking=True)
+    repository = fields.Many2one('runbot_merge.repository', required=True, tracking=True)
+    target = fields.Many2one('runbot_merge.branch', required=True, tracking=True)
+    commit = fields.Char(size=40, string="commit to cherry-pick, must be in-network", tracking=True)
+
+    patch = fields.Text(string="unified diff to apply", tracking=True)
+    format = fields.Selection([
+        ("format-patch", "format-patch"),
+        ("show", "show"),
+    ], compute="_compute_patch_meta")
+    message = fields.Text(compute="_compute_patch_meta")
+
+    _sql_constraints = [
+        ('patch_contents_either', 'check ((commit is null) != (patch is null))', 'Either the commit or patch must be set, and not both.'),
+    ]
+
+    @api.depends("patch")
+    def _compute_patch_meta(self) -> None:
+        for p in self:
+            if r := p._parse_patch():
+                p.format = r.kind
+                p.message = r.message
+            else:
+                p.format = False
+                p.message = False
+
+    def _parse_patch(self) -> ParseResult | None:
+        if not self.patch:
+            return None
+
+        if self.patch.startswith("commit "):
+            return parse_show(self)
+        elif self.patch.startswith("From "):
+            return parse_format_patch(self)
+        else:
+            raise ValidationError("Only `git show` and `git format-patch` formats are supported")
+
+    def _auto_init(self):
+        super()._auto_init()
+        self.env.cr.execute("""
+        CREATE INDEX IF NOT EXISTS runbot_merge_patch_active
+            ON runbot_merge_patch (target) WHERE active
+        """)
+
+    @api.model_create_multi
+    def create(self, vals_list):
+        if any(vals.get('active') is not False for vals in vals_list):
+            self.env.ref("runbot_merge.staging_cron")._trigger()
+        return super().create(vals_list)
+
+    def write(self, vals):
+        if vals.get("active") is not False:
+            self.env.ref("runbot_merge.staging_cron")._trigger()
+        return super().write(vals)
+
+    @api.constrains('patch')
+    def _validate_patch(self):
+        for p in self:
+            patch = p._parse_patch()
+            if not patch:
+                continue
+
+            has_files = False
+            for m in FILE_PATTERN.finditer(patch.patch):
+                has_files = True
+                if m['file_from'] != m['file_to']:
+                    raise ValidationError("Only patches updating a file in place are supported, not creation, removal, or renaming.")
+            if not has_files:
+                raise ValidationError("Patches should have files they patch, found none.")
+
+    def _apply_patches(self, target: Branch) -> bool:
+        patches = self.search([('target', '=', target.id)], order='id asc')
+        if not patches:
+            return True
+
+        commits = {}
+        repos = {}
+        for p in patches:
+            repos[p.repository] = git.get_local(p.repository).check(True)
+            commits.setdefault(p.repository, set())
+            if p.commit:
+                commits[p.repository].add(p.commit)
+
+        for patch in patches:
+            patch.active = False
+            r = repos[patch.repository]
+            remote = git.source_url(patch.repository)
+            if (cs := commits.pop(patch.repository, None)) is not None:
+                # first time encountering a repo, fetch the branch and commits
+                r.fetch(remote, f"+refs/heads/{target.name}:refs/heads/{target.name}", *cs, no_tags=True)
+
+            _logger.info(
+                "Applying %s to %r (in %s)",
+                patch,
+                patch.target.display_name,
+                patch.repository.name,
+            )
+            try:
+                c = patch._apply_commit(r) if patch.commit else 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"
+                patch.message_post(body=plaintext2html(e), subject=subject)
+                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}")
+            ## one of the repos is out of consistency, loop around to new staging?
+            if res.returncode:
+                _logger.warning(
+                    "Unable to push result of %s\nout:\n%s\nerr:\n%s",
+                    patch.id,
+                    res.stdout,
+                    res.stderr,
+                )
+                return False
+
+        return True
+
+    def _apply_commit(self, r: git.Repo) -> str:
+        r = r.check(True).stdout().with_config(encoding="utf-8")
+        # TODO: maybe use git-rev-list instead?
+        sha = r.show('--no-patch', '--pretty=%H', 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)
+        if res.returncode:
+            _conflict_info, _, informational = res.stdout.partition('\n\n')
+            raise PatchFailure(informational)
+
+        return r.commit_tree(
+            tree=res.stdout.strip(),
+            parents=[sha],
+            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:
+        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"),
+        )
+        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)
+        archiver.runner = subprocess.Popen
+        with archiver.archive(self.target.name, *files) as out, \
+             tarfile.open(fileobj=out.stdout, mode='r|') as tf,\
+             tempfile.TemporaryDirectory() as tmpdir:
+            tf.extractall(tmpdir)
+            patch = subprocess.run(
+                ['patch', '-p1', '-d', tmpdir],
+                input=p.patch,
+                stdout=subprocess.PIPE,
+                stderr=subprocess.PIPE,
+                encoding='utf-8',
+            )
+            if patch.returncode:
+                raise PatchFailure(patch.stdout)
+            new_tree = r.update_tree(self.target.name, files)
+
+        sha = r.stdout().with_config(encoding='utf-8')\
+            .show('--no-patch', '--pretty=%H', self.target.name)\
+            .stdout.strip()
+        return r.commit_tree(
+            tree=new_tree,
+            parents=[sha],
+            message=p.message,
+            author=p.author,
+            committer=p.committer,
+        ).stdout.strip()
diff --git a/runbot_merge/models/project.py b/runbot_merge/models/project.py
index 55210ebb..629a4f58 100644
--- a/runbot_merge/models/project.py
+++ b/runbot_merge/models/project.py
@@ -147,6 +147,18 @@ class Project(models.Model):
             ('staging_enabled', '=', True),
             ('project_id.staging_enabled', '=', True),
         ]):
+            try:
+                with self.env.cr.savepoint():
+                    if not self.env['runbot_merge.patch']._apply_patches(branch):
+                        self.env.ref("runbot_merge.staging_cron")._trigger()
+                        return
+
+            except Exception:
+                _logger.exception("Failed to apply patches to branch %r", branch.name)
+            else:
+                if commit:
+                    self.env.cr.commit()
+
             try:
                 with self.env.cr.savepoint(), \
                     sentry_sdk.start_span(description=f'create staging {branch.name}') as span:
diff --git a/runbot_merge/security/ir.model.access.csv b/runbot_merge/security/ir.model.access.csv
index 76bbf09d..0e8b0e8d 100644
--- a/runbot_merge/security/ir.model.access.csv
+++ b/runbot_merge/security/ir.model.access.csv
@@ -30,5 +30,5 @@ access_runbot_merge_pull_requests,User access to PR,model_runbot_merge_pull_requ
 access_runbot_merge_pull_requests_feedback,Users have no reason to access feedback,model_runbot_merge_pull_requests_feedback,,0,0,0,0
 access_runbot_merge_review_rights_2,Users can see partners,model_res_partner_review,base.group_user,1,0,0,0
 access_runbot_merge_review_override_2,Users can see partners,model_res_partner_override,base.group_user,1,0,0,0
-runbot_merge.access_runbot_merge_pull_requests_feedback_template,access_runbot_merge_pull_requests_feedback_template,runbot_merge.model_runbot_merge_pull_requests_feedback_template,base.group_system,1,1,0,0
-
+access_runbot_merge_pull_requests_feedback_template,access_runbot_merge_pull_requests_feedback_template,runbot_merge.model_runbot_merge_pull_requests_feedback_template,base.group_system,1,1,0,0
+access_runbot_merge_patch,Patcher access,runbot_merge.model_runbot_merge_patch,runbot_merge.group_patcher,1,1,1,0
diff --git a/runbot_merge/security/security.xml b/runbot_merge/security/security.xml
index d1ea1c79..668b2f02 100644
--- a/runbot_merge/security/security.xml
+++ b/runbot_merge/security/security.xml
@@ -1,9 +1,15 @@
 <odoo>
+    <record model="res.groups" id="group_patcher">
+        <field name="name">Mergebot Patcher</field>
+    </record>
     <record model="res.groups" id="group_admin">
         <field name="name">Mergebot Administrator</field>
     </record>
     <record model="res.groups" id="base.group_system">
-        <field name="implied_ids" eval="[(4, ref('runbot_merge.group_admin'))]"/>
+        <field name="implied_ids" eval="[
+            (4, ref('runbot_merge.group_admin')),
+            (4, ref('runbot_merge.group_patcher')),
+        ]"/>
     </record>
     <record model="res.groups" id="status">
         <field name="name">Mergebot Status Sender</field>
diff --git a/runbot_merge/tests/test_patching.py b/runbot_merge/tests/test_patching.py
new file mode 100644
index 00000000..aa76f8e0
--- /dev/null
+++ b/runbot_merge/tests/test_patching.py
@@ -0,0 +1,240 @@
+import xmlrpc.client
+
+import pytest
+
+from utils import Commit
+
+# basic udiff / show style patch, updates `b` from `1` to `2`
+BASIC_UDIFF = """\
+commit 0000000000000000000000000000000000000000
+Author: 3 Discos Down <bar@example.org>
+Date:   2021-04-24T17:09:14Z
+ 
+    whop
+    
+    whop whop
+ 
+diff --git a/b b/b
+index 000000000000..000000000000 100644
+--- a/b
++++ b/b
+@@ -1,1 +1,1 @@
+-1
++2
+"""
+
+FIELD_MAPPING = {
+    'boolean': ('old_value_integer', 'new_value_integer'),
+    'date': ('old_value_datetime', 'new_value_datetime'),
+    'datetime': ('old_value_datetime', 'new_value_datetime'),
+    'char': ('old_value_char', 'new_value_char'),
+    'float': ('old_value_float', 'new_value_float'),
+    'integer': ('old_value_integer', 'new_value_integer'),
+    'monetary': ('old_value_float', 'new_value_float'),
+    'text': ('old_value_text', 'new_value_text'),
+}
+def read_tracking_value(tv):
+    field_id = tv.field_id if 'field_id' in tv else tv.field
+    field_type = field_id.field_type if 'field_type' in field_id else field_id.ttype
+    old_field_name, new_field_name = FIELD_MAPPING[field_type]
+    return field_id.name, tv[old_field_name], tv[new_field_name]
+
+
+@pytest.fixture(autouse=True)
+def _setup(repo):
+    with repo:
+        [c, _] = repo.make_commits(
+            None,
+            Commit("a", tree={"a": "1", "b": "1\n"}),
+            Commit("b", tree={"a": "2"}),
+            ref="heads/master",
+        )
+        repo.make_ref("heads/x", c)
+
+@pytest.mark.parametrize("group,access", [
+    ('base.group_portal', False),
+    ('base.group_user', False),
+    ('runbot_merge.group_patcher', True),
+    ('runbot_merge.group_admin', False),
+    ('base.group_system', True),
+])
+def test_patch_acl(env, project, group, access):
+    g = env.ref(group)
+    assert g._name == 'res.groups'
+    env['res.users'].create({
+        'name': 'xxx',
+        'login': 'xxx',
+        'password': 'xxx',
+        'groups_id': [(6, 0, [g.id])],
+    })
+    env2 = env.with_user('xxx', 'xxx')
+    def create():
+        return env2['runbot_merge.patch'].create({
+            'target': project.branch_ids.id,
+            'repository': project.repo_ids.id,
+            'patch': BASIC_UDIFF,
+        })
+    if access:
+        create()
+    else:
+        pytest.raises(xmlrpc.client.Fault, create)\
+            .match("You are not allowed to create")
+
+def test_apply_commit(env, project, repo, users):
+    with repo:
+        [c] = repo.make_commits("x", Commit("c", tree={"b": "2"}, author={
+            'name': "Henry Hoover",
+            "email": "dustsuckinghose@example.org",
+        }), ref="heads/abranch")
+        repo.delete_ref('heads/abranch')
+
+    p = env['runbot_merge.patch'].create({
+        'target': project.branch_ids.id,
+        'repository': project.repo_ids.id,
+        'commit': c,
+    })
+
+    env.run_crons()
+
+    HEAD = repo.commit('master')
+    assert repo.read_tree(HEAD) == {
+        'a': '2',
+        'b': '2',
+    }
+    assert HEAD.message == "c"
+    assert HEAD.author['name'] == "Henry Hoover"
+    assert HEAD.author['email'] == "dustsuckinghose@example.org"
+    assert not p.active
+
+def test_commit_conflict(env, project, repo, users):
+    with repo:
+        [c] = repo.make_commits("x", Commit("x", tree={"b": "3"}))
+        repo.make_commits("master", Commit("c", tree={"b": "2"}), ref="heads/master", make=False)
+
+    p = env['runbot_merge.patch'].create({
+        'target': project.branch_ids.id,
+        'repository': project.repo_ids.id,
+        'commit': c,
+    })
+
+    env.run_crons()
+
+    HEAD = repo.commit('master')
+    assert repo.read_tree(HEAD) == {
+        'a': '2',
+        'b': '2',
+    }
+    assert not p.active
+    assert [(
+        m.subject,
+        m.body,
+        list(map(read_tracking_value, m.tracking_value_ids)),
+    )
+        for m in reversed(p.message_ids)
+    ] == [
+        (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>\
+""",
+            [],
+        ),
+        (False, '', [('active', 1, 0)]),
+    ]
+
+def test_apply_udiff(env, project, repo, users):
+    p = env['runbot_merge.patch'].create({
+        'target': project.branch_ids.id,
+        'repository': project.repo_ids.id,
+        'patch': BASIC_UDIFF,
+    })
+
+    env.run_crons()
+
+    HEAD = repo.commit('master')
+    assert repo.read_tree(HEAD) == {
+        'a': '2',
+        'b': '2\n',
+    }
+    assert HEAD.message == "whop\n\nwhop whop"
+    assert HEAD.author['name'] == "3 Discos Down"
+    assert HEAD.author['email'] == "bar@example.org"
+    assert not p.active
+
+
+def test_apply_format_patch(env, project, repo, users):
+    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 <bar@example.org>
+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
+""",
+    })
+
+    env.run_crons()
+
+    HEAD = repo.commit('master')
+    assert repo.read_tree(HEAD) == {
+        'a': '2',
+        'b': '2\n',
+    }
+    assert HEAD.message == "whop\n\nwhop whop"
+    assert HEAD.author['name'] == "3 Discos Down"
+    assert HEAD.author['email'] == "bar@example.org"
+    assert not p.active
+
+def test_patch_conflict(env, project, repo, users):
+    p = env['runbot_merge.patch'].create({
+        'target': project.branch_ids.id,
+        'repository': project.repo_ids.id,
+        'patch': BASIC_UDIFF,
+    })
+    with repo:
+        repo.make_commits('master', Commit('cccombo breaker', tree={'b': '3'}), ref='heads/master', make=False)
+
+    env.run_crons()
+
+    HEAD = repo.commit('master')
+    assert HEAD.message == 'cccombo breaker'
+    assert repo.read_tree(HEAD) == {
+        'a': '2',
+        'b': '3',
+    }
+    assert not p.active
+    assert [(
+        m.subject,
+        m.body,
+        list(map(read_tracking_value, m.tracking_value_ids)),
+    )
+        for m in reversed(p.message_ids)
+    ] == [(
+        False,
+        '<p>Unstaged direct-application patch created</p>',
+        [],
+    ), (
+        "Unable to apply patch",
+        "<p>patching file b<br>Hunk #1 FAILED at 1.<br>1 out of 1 hunk FAILED -- saving rejects to file b.rej<br></p>",
+        [],
+    ), (
+        False, '', [('active', 1, 0)]
+    )]
diff --git a/runbot_merge/views/queues.xml b/runbot_merge/views/queues.xml
index a72571f3..e18abaa1 100644
--- a/runbot_merge/views/queues.xml
+++ b/runbot_merge/views/queues.xml
@@ -81,17 +81,82 @@
         </field>
     </record>
 
-    <menuitem name="Queues" id="menu_queues" parent="runbot_merge_menu"/>
+    <record id="action_patches" model="ir.actions.act_window">
+        <field name="name">Patches</field>
+        <field name="res_model">runbot_merge.patch</field>
+    </record>
+    <record id="search_patch" model="ir.ui.view">
+        <field name="name">Patches Search</field>
+        <field name="model">runbot_merge.patch</field>
+        <field name="arch" type="xml">
+            <search>
+                <filter string="Inactive" name="active" domain="[('active', '=', False)]"/>
+                <field name="target"/>
+                <field name="repository"/>
+            </search>
+        </field>
+    </record>
+    <record id="tree_patch" model="ir.ui.view">
+        <field name="name">Patches List</field>
+        <field name="model">runbot_merge.patch</field>
+        <field name="arch" type="xml">
+            <tree>
+                <field name="id"/>
+                <field name="repository"/>
+                <field name="target"/>
+            </tree>
+        </field>
+    </record>
+    <record id="form_patch" model="ir.ui.view">
+        <field name="name">Patches Form</field>
+        <field name="model">runbot_merge.patch</field>
+        <field name="arch" type="xml">
+            <form>
+                <sheet>
+                    <group>
+                        <group>
+                            <field name="repository"/>
+                            <field name="target"/>
+                        </group>
+                        <group>
+                            <field name="active"/>
+                        </group>
+                    </group>
+                    <group attrs="{'invisible': [
+                        ('commit', '=', False),
+                        ('patch', '!=', False),
+                    ]}">
+                        <group colspan="4">
+                            <field name="commit"/>
+                        </group>
+                    </group>
+                    <group attrs="{'invisible': [
+                        ('patch', '=', False),
+                        ('commit', '!=', False),
+                    ]}">
+                        <group colspan="4">
+                            <field name="format" colspan="4"/>
+                            <field name="patch" widget="ace"/>
+                                <!-- no diff/patch mode support -->
+                                <!-- options="{'mode': 'patch'}"/> -->
+                            <field name="message" colspan="4"/>
+                        </group>
+                    </group>
+                </sheet>
+            </form>
+        </field>
+    </record>
+
+    <menuitem name="Queues" id="menu_queues" parent="runbot_merge_menu">
         <menuitem name="Splits" id="menu_queues_splits"
-                  parent="menu_queues"
                   action="action_splits"/>
         <menuitem name="Feedback" id="menu_queues_feedback"
-                  parent="menu_queues"
                   action="action_feedback"/>
         <menuitem name="Tagging" id="menu_queues_tagging"
-                  parent="menu_queues"
                   action="action_tagging"/>
         <menuitem name="Fetches" id="menu_fetches"
-                  parent="menu_queues"
                   action="action_fetches"/>
+        <menuitem name="Patches" id="menu_patches"
+                  action="action_patches"/>
+    </menuitem>
 </odoo>

From 3a8b4684da23f10bb8f76d1222f8f637d0eba1a5 Mon Sep 17 00:00:00 2001
From: Xavier Morel <xmo@odoo.com>
Date: Mon, 7 Oct 2024 08:03:36 +0200
Subject: [PATCH 22/45] [IMP] conftest: support for mapped(fn)

---
 conftest.py | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/conftest.py b/conftest.py
index 38794fb4..3d65f1ed 100644
--- a/conftest.py
+++ b/conftest.py
@@ -1425,6 +1425,9 @@ class Model:
         )
 
     def mapped(self, path):
+        if callable(path):
+            return [path(r) for r in self]
+
         field, *rest = path.split('.', 1)
         descr = self._fields[field]
         if descr['type'] in ('many2one', 'one2many', 'many2many'):

From a45db1e089550d58edaf36a73ef55d77b3407190 Mon Sep 17 00:00:00 2001
From: Xavier Morel <xmo@odoo.com>
Date: Mon, 7 Oct 2024 08:04:25 +0200
Subject: [PATCH 23/45] [IMP] conftest: support for a more generic current_date

---
 conftest.py | 1 +
 1 file changed, 1 insertion(+)

diff --git a/conftest.py b/conftest.py
index 3d65f1ed..a6465680 100644
--- a/conftest.py
+++ b/conftest.py
@@ -437,6 +437,7 @@ class Base(models.AbstractModel):
     _inherit = 'base'
 
     def run_crons(self):
+        builtins.current_date = self.env.context.get('current_date')
         builtins.forwardport_merged_before = self.env.context.get('forwardport_merged_before')
         builtins.forwardport_updated_before = self.env.context.get('forwardport_updated_before')
         self.env['ir.cron']._process_jobs(self.env.cr.dbname)

From 4215be770d219fc04e3bb38366f01374be06807a Mon Sep 17 00:00:00 2001
From: Xavier Morel <xmo@odoo.com>
Date: Mon, 7 Oct 2024 08:06:03 +0200
Subject: [PATCH 24/45] [IMP] runbot_merge: move read_tracking_value to utils

And use it in test_trivial_flow instead of the kinda half-assed manual
version.
---
 mergebot_test_utils/utils.py        | 15 +++++++++++++++
 runbot_merge/tests/test_basic.py    | 29 ++++++++++++++---------------
 runbot_merge/tests/test_patching.py | 18 +-----------------
 3 files changed, 30 insertions(+), 32 deletions(-)

diff --git a/mergebot_test_utils/utils.py b/mergebot_test_utils/utils.py
index e5c24fbd..3baa3102 100644
--- a/mergebot_test_utils/utils.py
+++ b/mergebot_test_utils/utils.py
@@ -3,6 +3,7 @@ import contextlib
 import itertools
 import re
 import time
+import typing
 
 from lxml import html
 
@@ -214,3 +215,17 @@ def prevent_unstaging(st) -> None:
         yield
     finally:
         st.active = True
+
+
+TYPE_MAPPING = {
+    'boolean': 'integer',
+    'date': 'datetime',
+    'monetary': 'float',
+    'selection': 'char',
+    'many2one': 'char',
+}
+def read_tracking_value(tv) -> tuple[str, typing.Any, typing.Any]:
+    field_id = tv.field_id if 'field_id' in tv else tv.field
+    field_type = field_id.field_type if 'field_type' in field_id else field_id.ttype
+    t = TYPE_MAPPING.get(field_type) or field_type
+    return field_id.name, tv[f"old_value_{t}"], tv[f"new_value_{t}"]
diff --git a/runbot_merge/tests/test_basic.py b/runbot_merge/tests/test_basic.py
index 440518e6..afbc491f 100644
--- a/runbot_merge/tests/test_basic.py
+++ b/runbot_merge/tests/test_basic.py
@@ -11,7 +11,8 @@ import requests
 from lxml import html
 
 import odoo
-from utils import _simple_init, seen, matches, get_partner, Commit, pr_page, to_pr, part_of, ensure_one
+from utils import _simple_init, seen, matches, get_partner, Commit, pr_page, to_pr, part_of, ensure_one, read_tracking_value
+
 
 @pytest.fixture(autouse=True)
 def _configure_statuses(request, project, repo):
@@ -157,31 +158,29 @@ def test_trivial_flow(env, repo, page, users, config):
 
     # reverse because the messages are in newest-to-oldest by default
     # (as that's how you want to read them)
-    messages = reversed([
-        (m.author_id.display_name, m.body, list(zip(
-            m.tracking_value_ids.get_old_display_value(),
-            m.tracking_value_ids.get_new_display_value(),
-        )))
-        for m in pr_id.message_ids
-    ])
+    messages = pr_id.message_ids[::-1].mapped(lambda m: (
+        m.author_id.display_name,
+        m.body,
+        list(map(read_tracking_value, m.tracking_value_ids)),
+    ))
 
     assert list(messages) == [
         (users['user'], '<p>Pull Request created</p>', []),
-        (users['user'], '', [(c1, c2)]),
-        ('OdooBot', f'<p>statuses changed on {c2}</p>', [('Opened', 'Validated')]),
+        (users['user'], '', [('head', c1, c2)]),
+        ('OdooBot', f'<p>statuses changed on {c2}</p>', [('state', 'Opened', 'Validated')]),
         # reviewer approved changing the state and setting reviewer as reviewer
         # plus set merge method
         ('Reviewer', '', [
-            ('Validated', 'Ready'),
-            ('', 'rebase and merge, using the PR as merge commit message'),
-            ('', 'Reviewer'),
+            ('state', 'Validated', 'Ready'),
+            ('merge_method', '', 'rebase and merge, using the PR as merge commit message'),
+            ('reviewed_by', '', 'Reviewer'),
         ]),
         # staging succeeded
         (matches('$$'), f'<p>staging {st.id} succeeded</p>', [
             # set merge date
-            (False, pr_id.merge_date + 'Z'),
+            ('merge_date', False, pr_id.merge_date),
             # updated state
-            ('Ready', 'Merged'),
+            ('state', 'Ready', 'Merged'),
         ]),
     ]
 
diff --git a/runbot_merge/tests/test_patching.py b/runbot_merge/tests/test_patching.py
index aa76f8e0..2774599a 100644
--- a/runbot_merge/tests/test_patching.py
+++ b/runbot_merge/tests/test_patching.py
@@ -2,7 +2,7 @@ import xmlrpc.client
 
 import pytest
 
-from utils import Commit
+from utils import Commit, read_tracking_value
 
 # basic udiff / show style patch, updates `b` from `1` to `2`
 BASIC_UDIFF = """\
@@ -23,22 +23,6 @@ index 000000000000..000000000000 100644
 +2
 """
 
-FIELD_MAPPING = {
-    'boolean': ('old_value_integer', 'new_value_integer'),
-    'date': ('old_value_datetime', 'new_value_datetime'),
-    'datetime': ('old_value_datetime', 'new_value_datetime'),
-    'char': ('old_value_char', 'new_value_char'),
-    'float': ('old_value_float', 'new_value_float'),
-    'integer': ('old_value_integer', 'new_value_integer'),
-    'monetary': ('old_value_float', 'new_value_float'),
-    'text': ('old_value_text', 'new_value_text'),
-}
-def read_tracking_value(tv):
-    field_id = tv.field_id if 'field_id' in tv else tv.field
-    field_type = field_id.field_type if 'field_type' in field_id else field_id.ttype
-    old_field_name, new_field_name = FIELD_MAPPING[field_type]
-    return field_id.name, tv[old_field_name], tv[new_field_name]
-
 
 @pytest.fixture(autouse=True)
 def _setup(repo):

From 20a4e97b0504e543672b1525fa745c420b436c51 Mon Sep 17 00:00:00 2001
From: Xavier Morel <xmo@odoo.com>
Date: Mon, 7 Oct 2024 08:07:59 +0200
Subject: [PATCH 25/45] [CHG] runbot_merge: make merge method non-blocking

Because of the false negatives due to github's reordering of events on
retargeting, blocking merge methods can be rather frustrating or the
user as what's happening and how to solve it isn't clear in that case.

Keep the warnings and all, but remove the blocking factor: if a PR
doesn't have a merge method and is not single-commit, just skip it on
staging. This way, PRs which are actually single-commit will stage
fine even if the mergebot thinks they shouldn't be.

Fixes #957
---
 runbot_merge/controllers/__init__.py   |  8 ++-
 runbot_merge/exceptions.py             |  5 +-
 runbot_merge/models/batch.py           |  4 +-
 runbot_merge/models/pull_requests.py   | 34 ++++++++----
 runbot_merge/models/stagings_create.py | 20 +++++--
 runbot_merge/tests/test_basic.py       | 73 +++++++++++++++++++++++++-
 runbot_merge/tests/test_multirepo.py   |  6 ++-
 7 files changed, 131 insertions(+), 19 deletions(-)

diff --git a/runbot_merge/controllers/__init__.py b/runbot_merge/controllers/__init__.py
index 17b8b005..969331ea 100644
--- a/runbot_merge/controllers/__init__.py
+++ b/runbot_merge/controllers/__init__.py
@@ -2,7 +2,7 @@ import hashlib
 import hmac
 import logging
 import json
-from datetime import datetime
+from datetime import datetime, timedelta
 
 import sentry_sdk
 import werkzeug.exceptions
@@ -294,6 +294,12 @@ def handle_pr(env, event):
             event['sender']['login'],
             pr['commits'] == 1
         )
+        if pr['base']['ref'] != pr_obj.target.name:
+            env['runbot_merge.fetch_job'].create({
+                'repository': pr_obj.repository.id,
+                'number': pr_obj.number,
+                'commits_at': datetime.now() + timedelta(minutes=5),
+            })
 
         pr_obj.write({
             'reviewed_by': False,
diff --git a/runbot_merge/exceptions.py b/runbot_merge/exceptions.py
index 4ef79f2e..5b4c30c5 100644
--- a/runbot_merge/exceptions.py
+++ b/runbot_merge/exceptions.py
@@ -5,4 +5,7 @@ class FastForwardError(Exception):
 class Mismatch(MergeError):
     pass
 class Unmergeable(MergeError):
-    ...
+    pass
+
+class Skip(MergeError):
+    pass
diff --git a/runbot_merge/models/batch.py b/runbot_merge/models/batch.py
index 1a96075e..494ded57 100644
--- a/runbot_merge/models/batch.py
+++ b/runbot_merge/models/batch.py
@@ -195,7 +195,7 @@ class Batch(models.Model):
 
     @api.depends(
         "merge_date",
-        "prs.error", "prs.draft", "prs.squash", "prs.merge_method",
+        "prs.error", "prs.draft",
         "skipchecks",
         "prs.status", "prs.reviewed_by", "prs.target",
     )
@@ -208,7 +208,7 @@ class Batch(models.Model):
             elif len(targets := batch.prs.mapped('target')) > 1:
                 batch.blocked = f"Multiple target branches: {', '.join(targets.mapped('name'))!r}"
             elif blocking := batch.prs.filtered(
-                lambda p: p.error or p.draft or not (p.squash or p.merge_method)
+                lambda p: p.error or p.draft
             ):
                 batch.blocked = "Pull request(s) %s blocked." % ', '.join(blocking.mapped('display_name'))
             elif not batch.skipchecks and (unready := batch.prs.filtered(
diff --git a/runbot_merge/models/pull_requests.py b/runbot_merge/models/pull_requests.py
index 810eb673..2f16266a 100644
--- a/runbot_merge/models/pull_requests.py
+++ b/runbot_merge/models/pull_requests.py
@@ -1,6 +1,7 @@
 from __future__ import annotations
 
 import ast
+import builtins
 import collections
 import contextlib
 import datetime
@@ -106,7 +107,7 @@ All substitutions are tentatively applied sequentially to the input.
             self._cr, 'runbot_merge_unique_repo', self._table, ['name'])
         return res
 
-    def _load_pr(self, number, *, closing=False):
+    def _load_pr(self, number, *, closing=False, squash=False):
         gh = self.github()
 
         # fetch PR object and handle as *opened*
@@ -133,6 +134,10 @@ All substitutions are tentatively applied sequentially to the input.
             ('number', '=', number),
         ])
         if pr_id:
+            if squash:
+                pr_id.squash = pr['commits'] == 1
+                return
+
             sync = controllers.handle_pr(self.env, {
                 'action': 'synchronize',
                 'pull_request': pr,
@@ -583,8 +588,6 @@ class PullRequests(models.Model):
 
     @api.depends(
         'batch_id.prs.draft',
-        'batch_id.prs.squash',
-        'batch_id.prs.merge_method',
         'batch_id.prs.state',
         'batch_id.skipchecks',
     )
@@ -592,12 +595,11 @@ class PullRequests(models.Model):
         self.blocked = False
         requirements = (
             lambda p: not p.draft,
-            lambda p: p.squash or p.merge_method,
             lambda p: p.state == 'ready' \
                   or p.batch_id.skipchecks \
                  and all(pr.state != 'error' for pr in p.batch_id.prs)
         )
-        messages = ('is in draft', 'has no merge method', 'is not ready')
+        messages = ('is in draft', 'is not ready')
         for pr in self:
             if pr.state in ('merged', 'closed'):
                 continue
@@ -834,6 +836,10 @@ For your own safety I've ignored *everything in your entire comment*.
                         pull_request=self.number,
                         format_args={'new_method': explanation, 'pr': self, 'user': login},
                     )
+                    # if the merge method is the only thing preventing (but not
+                    # *blocking*) staging, trigger a staging
+                    if self.state == 'ready':
+                        self.env.ref("runbot_merge.staging_cron")._trigger()
                 case commands.Retry() if is_author or source_author:
                     if self.error:
                         self.error = False
@@ -2497,31 +2503,41 @@ class FetchJob(models.Model):
     repository = fields.Many2one('runbot_merge.repository', required=True)
     number = fields.Integer(required=True, group_operator=None)
     closing = fields.Boolean(default=False)
+    commits_at = fields.Datetime(index="btree_not_null")
 
     @api.model_create_multi
     def create(self, vals_list):
-        self.env.ref('runbot_merge.fetch_prs_cron')._trigger()
+        now = fields.Datetime.now()
+        self.env.ref('runbot_merge.fetch_prs_cron')._trigger({
+            fields.Datetime.to_datetime(
+                vs.get('commits_at') or now
+            )
+            for vs in vals_list
+        })
         return super().create(vals_list)
 
     def _check(self, commit=False):
         """
         :param bool commit: commit after each fetch has been executed
         """
+        now = getattr(builtins, 'current_date', None) or fields.Datetime.to_string(datetime.datetime.now())
         while True:
-            f = self.search([], limit=1)
+            f = self.search([
+                '|', ('commits_at', '=', False), ('commits_at', '<=', now)
+            ], limit=1)
             if not f:
                 return
 
+            f.active = False
             self.env.cr.execute("SAVEPOINT runbot_merge_before_fetch")
             try:
-                f.repository._load_pr(f.number, closing=f.closing)
+                f.repository._load_pr(f.number, closing=f.closing, squash=bool(f.commits_at))
             except Exception:
                 self.env.cr.execute("ROLLBACK TO SAVEPOINT runbot_merge_before_fetch")
                 _logger.exception("Failed to load pr %s, skipping it", f.number)
             finally:
                 self.env.cr.execute("RELEASE SAVEPOINT runbot_merge_before_fetch")
 
-            f.active = False
             if commit:
                 self.env.cr.commit()
 
diff --git a/runbot_merge/models/stagings_create.py b/runbot_merge/models/stagings_create.py
index 08734958..ca517395 100644
--- a/runbot_merge/models/stagings_create.py
+++ b/runbot_merge/models/stagings_create.py
@@ -262,6 +262,10 @@ def stage_batches(branch: Branch, batches: Batch, staging_state: StagingState) -
             break
         try:
             staged |= stage_batch(env, batch, staging_state)
+        except exceptions.Skip:
+            # skip silently because the PR will be retried on every staging
+            # which is going to be ultra spammy
+            pass
         except exceptions.MergeError as e:
             pr = e.args[0]
             _logger.info("Failed to stage %s into %s", pr.display_name, branch.name)
@@ -419,9 +423,19 @@ def stage(pr: PullRequests, info: StagingSlice, related_prs: PullRequests) -> Tu
         invalid['target'] = branch.id
         diff.append(('Target branch', pr.target.name, branch.name))
 
-    if pr.squash != commits == 1:
-        invalid['squash'] = commits == 1
-        diff.append(('Single commit', pr.squash, commits == 1))
+    if not method:
+        if not pr.method_warned:
+            pr.env.ref('runbot_merge.pr.merge_method')._send(
+                repository=pr.repository,
+                pull_request=pr.number,
+                format_args={'pr': pr, 'methods': ''.join(
+                    '* `%s` to %s\n' % pair
+                    for pair in type(pr).merge_method.selection
+                    if pair[0] != 'squash'
+                )},
+            )
+            pr.method_warned = True
+        raise exceptions.Skip()
 
     msg = utils.make_message(prdict)
     if pr.message != msg:
diff --git a/runbot_merge/tests/test_basic.py b/runbot_merge/tests/test_basic.py
index afbc491f..9fee6f6f 100644
--- a/runbot_merge/tests/test_basic.py
+++ b/runbot_merge/tests/test_basic.py
@@ -2211,7 +2211,7 @@ class TestPRUpdate:
 
     def test_update_opened(self, env, repo):
         with repo:
-            [c] = repo.make_commits("master", Commit('fist', tree={'m': 'c1'}))
+            [c] = repo.make_commits("master", Commit('first', tree={'m': 'c1'}))
             prx = repo.make_pr(target='master', head=c)
 
         pr = to_pr(env, prx)
@@ -2595,6 +2595,77 @@ Please check and re-approve.
         assert not pr_id.reviewed_by
         assert not pr_id.squash
 
+    @pytest.mark.defaultstatuses
+    def test_update_incorrect_commits_count(self, port, env, project, repo, config, users):
+        """This is not a great test but it aims to kinda sorta simulate the
+        behaviour when a user retargets and updates a PR at about the same time:
+        github can send the hooks in the wrong order, which leads to the correct
+        base and head but can lead to the wrong squash status.
+        """
+        project.write({
+            'branch_ids': [(0, 0, {
+                'name': 'xxx',
+            })]
+        })
+        with repo:
+            [c] = repo.make_commits("master", Commit("c", tree={"m": "n"}), ref="heads/thing")
+            pr = repo.make_pr(target='master', head='thing')
+
+        pr_id = to_pr(env, pr)
+        pr_id.head = '0'*40
+        with requests.Session() as s:
+            r = s.post(
+                f"http://localhost:{port}/runbot_merge/hooks",
+                headers={
+                    "X-Github-Event": "pull_request",
+                },
+                json={
+                    'action': 'synchronize',
+                    'sender': {
+                        'login': users['user'],
+                    },
+                    'repository': {
+                        'full_name': repo.name,
+                    },
+                    'pull_request': {
+                        'number': pr.number,
+                        'head': {'sha': c},
+                        'title': "c",
+                        'commits': 40123,
+                        'base': {
+                            'ref': 'xxx',
+                            'repo': {
+                                'full_name': repo.name,
+                            },
+                        }
+                    }
+                }
+            )
+            r.raise_for_status()
+        assert pr_id.head == c, "the head should have been updated"
+        assert not pr_id.squash, "the wrong count should be used"
+
+        with repo:
+            pr.post_comment("hansen r+", config['role_reviewer']['token'])
+            repo.post_status(c, 'success')
+        env.run_crons()
+        assert not pr_id.blocked
+        assert pr_id.message_ids[::-1].mapped(lambda m: (
+            ((m.subject or '') + '\n\n' + m.body).strip(),
+            list(map(read_tracking_value, m.tracking_value_ids)),
+        )) == [
+            ('<p>Pull Request created</p>', []),
+            ('', [('head', c, '0'*40)]),
+            ('', [('head', '0'*40, c), ('squash', 1, 0)]),
+            ('', [('state', 'Opened', 'Approved'), ('reviewed_by', '', 'Reviewer')]),
+            (f'<p>statuses changed on {c}</p>', [('state', 'Approved', 'Ready')]),
+        ]
+        assert pr_id.staging_id
+        with repo:
+            repo.post_status('staging.master', 'success')
+        env.run_crons()
+        assert pr_id.merge_date
+
 class TestBatching(object):
     def _pr(self, repo, prefix, trees, *, target='master', user, reviewer,
             statuses=(('ci/runbot', 'success'), ('legal/cla', 'success'))
diff --git a/runbot_merge/tests/test_multirepo.py b/runbot_merge/tests/test_multirepo.py
index 0efb7cfc..52e9bb2b 100644
--- a/runbot_merge/tests/test_multirepo.py
+++ b/runbot_merge/tests/test_multirepo.py
@@ -806,10 +806,12 @@ class TestBlocked:
 
         p = to_pr(env, pr)
         assert p.state == 'ready'
-        assert p.blocked
+        assert not p.blocked
+        assert not p.staging_id
 
         with repo_a: pr.post_comment('hansen rebase-merge', config['role_reviewer']['token'])
-        assert not p.blocked
+        env.run_crons()
+        assert p.staging_id
 
     def test_linked_closed(self, env, repo_a, repo_b, config):
         with repo_a, repo_b:

From 5748c086e5722eb1958c56ca10d44d2988ecc7ba Mon Sep 17 00:00:00 2001
From: Xavier Morel <xmo@odoo.com>
Date: Wed, 16 Oct 2024 12:17:30 +0200
Subject: [PATCH 26/45] [FIX] runbot_merge: status of closed PRs in extant
 batches

If a PR is closed but part of an ongoing batch, the change in status
of the batch might be reflected on the PR still:

- if a PR is closed and the batch gets staged, the PR shows up as
  being staged
- if the PR is merged then the batch gets merged, the PR shows up as
  merged

Fixes #914

Also remove the unused `_tagstate` helper property.
---
 runbot_merge/models/pull_requests.py | 22 +++++++++++-----------
 1 file changed, 11 insertions(+), 11 deletions(-)

diff --git a/runbot_merge/models/pull_requests.py b/runbot_merge/models/pull_requests.py
index 2f16266a..6d1ca7f5 100644
--- a/runbot_merge/models/pull_requests.py
+++ b/runbot_merge/models/pull_requests.py
@@ -431,10 +431,16 @@ class PullRequests(models.Model):
     staging_id = fields.Many2one('runbot_merge.stagings', compute='_compute_staging', inverse=readonly, readonly=True, store=True)
     staging_ids = fields.Many2many('runbot_merge.stagings', string="Stagings", compute='_compute_stagings', inverse=readonly, readonly=True, context={"active_test": False})
 
-    @api.depends('batch_id.batch_staging_ids.runbot_merge_stagings_id.active')
+    @api.depends(
+        'closed',
+        'batch_id.batch_staging_ids.runbot_merge_stagings_id.active',
+    )
     def _compute_staging(self):
         for p in self:
-            p.staging_id = p.batch_id.staging_ids.filtered('active')
+            if p.closed:
+                p.staging_id = False
+            else:
+                p.staging_id = p.batch_id.staging_ids.filtered('active')
 
     @api.depends('batch_id.batch_staging_ids.runbot_merge_stagings_id')
     def _compute_stagings(self):
@@ -1250,10 +1256,10 @@ For your own safety I've ignored *everything in your entire comment*.
     )
     def _compute_state(self):
         for pr in self:
-            if pr.batch_id.merge_date:
-                pr.state = 'merged'
-            elif pr.closed:
+            if pr.closed:
                 pr.state = "closed"
+            elif pr.batch_id.merge_date:
+                pr.state = 'merged'
             elif pr.error:
                 pr.state = "error"
             elif pr.batch_id.skipchecks: # skipchecks behaves as both approval and status override
@@ -1338,12 +1344,6 @@ For your own safety I've ignored *everything in your entire comment*.
                          "ON runbot_merge_pull_requests "
                          "USING hash (head)")
 
-    @property
-    def _tagstate(self):
-        if self.state == 'ready' and self.staging_id.heads:
-            return 'staged'
-        return self.state
-
     def _get_batch(self, *, target, label):
         batch = self.env['runbot_merge.batch']
         if not re.search(r':patch-\d+$', label):

From 640392dc201db5a6e425630c747c84061c552181 Mon Sep 17 00:00:00 2001
From: Xavier Morel <xmo@odoo.com>
Date: Fri, 18 Oct 2024 10:09:19 +0200
Subject: [PATCH 27/45] [FIX] significantly speed up local testing

The mergebot tests have always been pretty gentle on system load which
is nice, however it's just looking at the list of longest tests that I
realised / re-membered the hook wait duration is 10 seconds for the
benefit of github, which doesn't really matter locally. This means on
interaction / cron-heavy tests the test might only be using on the
order of 10% CPU or something, that is a waste of time.

TBF this is easily compensated by increasing the concurrency of the
test suite (e.g. from 16 to 32 when I switched machine, but it seems
as if not more sensible to lower the webhook wait delay to something
more reasonable. 1s seems to be a good fit here, on my new computer at
n=16 it leads to the test suite running in 15mn at 600% CPU (which is
pretty good on a 6/12 CPU as it loads the system heavily but doesn't
completely bog it down).

Reducing it to 0.5s, the test suite takes the same duration but CPU
load increases to 770%, and errors creep up, likely a mix of
concurrency issues in the DB and dummy-central sending webhooks too
slowly as we compete with it for CPU resources (could actually make
sense to restrict the number of threads tokio can use). Reducing
concurrency could make this work better, but I think at this point
we're in a pretty good state, it's even somewhat reasonable to run the
test suite sequentially (taking about 1h10 but being functionally
invisible in terms of load).
---
 conftest.py | 35 +++++++++++++++++++++++++----------
 1 file changed, 25 insertions(+), 10 deletions(-)

diff --git a/conftest.py b/conftest.py
index a6465680..cb06646f 100644
--- a/conftest.py
+++ b/conftest.py
@@ -78,6 +78,18 @@ import requests
 NGROK_CLI = [
     'ngrok', 'start', '--none', '--region', 'eu',
 ]
+# When an operation can trigger webhooks, the test suite has to wait *some time*
+# in the hope that the webhook(s) will have been dispatched by the end as github
+# provides no real webhook feedback (e.g. an event stream).
+#
+# This also acts as a bit of a rate limiter, as github has gotten more and more
+# angry with that. Especially around event-generating limits.
+#
+# TODO: explore https://docs.github.com/en/rest/using-the-rest-api/rate-limits-for-the-rest-api
+#       and see if it would be possible to be a better citizen (e.g. add test
+#       retry / backoff using GH tighter GH integration)
+WEBHOOK_WAIT_TIME = 10  # seconds
+LOCAL_WEBHOOK_WAIT_TIME = 1
 
 def pytest_addoption(parser):
     parser.addoption('--addons-path')
@@ -98,7 +110,12 @@ def pytest_addoption(parser):
 def is_manager(config):
     return not hasattr(config, 'workerinput')
 
-def pytest_configure(config):
+def pytest_configure(config: pytest.Config) -> None:
+    global WEBHOOK_WAIT_TIME
+    # no tunnel => local setup, no need to wait as much
+    if not config.getoption('--tunnel'):
+        WEBHOOK_WAIT_TIME = LOCAL_WEBHOOK_WAIT_TIME
+
     sys.path.insert(0, os.path.join(os.path.dirname(__file__), 'mergebot_test_utils'))
     config.addinivalue_line(
         "markers",
@@ -109,7 +126,6 @@ def pytest_configure(config):
         "defaultstatuses: use the statuses `default` rather than `ci/runbot,legal/cla`",
     )
 
-
 def pytest_unconfigure(config):
     if not is_manager(config):
         return
@@ -126,7 +142,7 @@ def _set_socket_timeout():
     socket.setdefaulttimeout(120.0)
 
 @pytest.fixture(scope="session")
-def config(pytestconfig):
+def config(pytestconfig: pytest.Config) -> dict[str, dict[str, str]]:
     """ Flat version of the pytest config file (pytest.ini), parses to a
     simple dict of {section: {key: value}}
 
@@ -211,7 +227,7 @@ def users(partners, rolemap):
     return {k: v['login'] for k, v in rolemap.items()}
 
 @pytest.fixture(scope='session')
-def tunnel(pytestconfig, port):
+def tunnel(pytestconfig: pytest.Config, port: int):
     """ Creates a tunnel to localhost:<port> using ngrok or localtunnel, should yield the
     publicly routable address & terminate the process at the end of the session
     """
@@ -368,8 +384,8 @@ def db(request, module, dbcache, tmpdir):
     if not request.config.getoption('--no-delete'):
         subprocess.run(['dropdb', rundb], check=True)
 
-def wait_for_hook(n=1):
-    time.sleep(10 * n)
+def wait_for_hook():
+    time.sleep(WEBHOOK_WAIT_TIME)
 
 def wait_for_server(db, port, proc, mod, timeout=120):
     """ Polls for server to be response & have installed our module.
@@ -999,11 +1015,11 @@ class Repo:
                 return
 
     def __enter__(self):
-        self.hook = 1
+        self.hook = True
         return self
     def __exit__(self, *args):
-        wait_for_hook(self.hook)
-        self.hook = 0
+        wait_for_hook()
+        self.hook = False
     class Commit:
         def __init__(self, message, *, author=None, committer=None, tree, reset=False):
             self.id = None
@@ -1137,7 +1153,6 @@ class PR:
             headers=headers
         )
         assert 200 <= r.status_code < 300, r.text
-        wait_for_hook()
 
     def delete_comment(self, cid, token=None):
         assert self.repo.hook

From 632763d39020fa43753cc3394187f646e1d7c452 Mon Sep 17 00:00:00 2001
From: Xavier Morel <xmo@odoo.com>
Date: Tue, 22 Oct 2024 10:50:09 +0200
Subject: [PATCH 28/45] [CHG] runbot_merge: move labels cron to triggered

Missed it during the previous pass, probably because it's in the
middle of `pull_requests.py`. It's a classic template for triggered
crons since the model is just a queue of actions for the cron.
---
 runbot_merge/data/merge_cron.xml     | 2 +-
 runbot_merge/models/pull_requests.py | 2 ++
 runbot_merge/tests/test_by_branch.py | 2 --
 3 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/runbot_merge/data/merge_cron.xml b/runbot_merge/data/merge_cron.xml
index aa9de63e..a728acc5 100644
--- a/runbot_merge/data/merge_cron.xml
+++ b/runbot_merge/data/merge_cron.xml
@@ -38,7 +38,7 @@
     <field name="state">code</field>
     <field name="code">model._send()</field>
     <field name="interval_number">10</field>
-    <field name="interval_type">minutes</field>
+    <field name="interval_type">hours</field>
     <field name="numbercall">-1</field>
     <field name="doall" eval="False"/>
     <field name="priority">70</field>
diff --git a/runbot_merge/models/pull_requests.py b/runbot_merge/models/pull_requests.py
index 6d1ca7f5..2c97c21a 100644
--- a/runbot_merge/models/pull_requests.py
+++ b/runbot_merge/models/pull_requests.py
@@ -1758,6 +1758,8 @@ class Tagging(models.Model):
             values['tags_remove'] = json.dumps(list(values['tags_remove']))
         if not isinstance(values.get('tags_add', ''), str):
             values['tags_add'] = json.dumps(list(values['tags_add']))
+        if values:
+            self.env.ref('runbot_merge.labels_cron')._trigger()
         return super().create(values)
 
     def _send(self):
diff --git a/runbot_merge/tests/test_by_branch.py b/runbot_merge/tests/test_by_branch.py
index 9134a4e2..4d64ca43 100644
--- a/runbot_merge/tests/test_by_branch.py
+++ b/runbot_merge/tests/test_by_branch.py
@@ -149,7 +149,6 @@ def test_pseudo_version_tag(env, project, make_repo, setreviewers, config):
     with repo:
         repo.post_status('staging.master', 'success', 'ci')
     env.run_crons() # should merge staging
-    env.run_crons('runbot_merge.labels_cron') # update labels
     assert pr_id.state == 'merged'
     assert pr.labels >= {'2.1'}
 
@@ -170,6 +169,5 @@ def test_pseudo_version_tag(env, project, make_repo, setreviewers, config):
     with repo:
         repo.post_status('staging.master', 'success', 'ci')
     env.run_crons() # should merge staging
-    env.run_crons('runbot_merge.labels_cron') # update labels
     assert pr_id.state == 'merged'
     assert pr.labels >= {'post-bonk'}

From d9e6d394485a297044a938fe623d2a735c93d4e8 Mon Sep 17 00:00:00 2001
From: Xavier Morel <xmo@odoo.com>
Date: Tue, 22 Oct 2024 10:51:30 +0200
Subject: [PATCH 29/45] [IMP] mergebot_test_utils: minor style fixes

---
 mergebot_test_utils/utils.py | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/mergebot_test_utils/utils.py b/mergebot_test_utils/utils.py
index 3baa3102..50976518 100644
--- a/mergebot_test_utils/utils.py
+++ b/mergebot_test_utils/utils.py
@@ -134,7 +134,7 @@ def make_basic(
     prod = make_repo(reponame)
     env['runbot_merge.events_sources'].create({'repository': prod.name})
     with prod:
-        a_0, a_1, a_2, a_3, a_4, = prod.make_commits(
+        _0, _1, a_2, _3, _4, = prod.make_commits(
             None,
             Commit("0", tree={'f': 'a'}),
             Commit("1", tree={'f': 'b'}),
@@ -143,7 +143,7 @@ def make_basic(
             Commit("4", tree={'f': 'e'}),
             ref='heads/a',
         )
-        b_1, b_2 = prod.make_commits(
+        b_1, _2 = prod.make_commits(
             a_2,
             Commit('11', tree={'g': 'a'}),
             Commit('22', tree={'g': 'b'}),

From ed1f084c4f7d6463b7ec84f74127c1322917280f Mon Sep 17 00:00:00 2001
From: Xavier Morel <xmo@odoo.com>
Date: Tue, 22 Oct 2024 10:51:47 +0200
Subject: [PATCH 30/45] [IMP] runbot_merge: style fixes

- replace manual token_urlsafe by actual token_urlsafe
- make conditional right side up and more readable
- replace match by fullmatch, should not change anything since we end
  with a greedy universal match but is slightly more explicit
---
 runbot_merge/models/batch.py | 12 ++++--------
 1 file changed, 4 insertions(+), 8 deletions(-)

diff --git a/runbot_merge/models/batch.py b/runbot_merge/models/batch.py
index 494ded57..5e7dbbfb 100644
--- a/runbot_merge/models/batch.py
+++ b/runbot_merge/models/batch.py
@@ -1,10 +1,8 @@
 from __future__ import annotations
 
-import base64
-import contextlib
 import logging
-import os
 import re
+import secrets
 from collections import defaultdict
 from collections.abc import Iterator
 
@@ -323,9 +321,7 @@ class Batch(models.Model):
         new_branch = '%s-%s-%s-fw' % (
             target.name,
             base.refname,
-            # avoid collisions between fp branches (labels can be reused
-            # or conflict especially as we're chopping off the owner)
-            base64.urlsafe_b64encode(os.urandom(3)).decode()
+            secrets.token_urlsafe(3),
         )
         conflicts = {}
         for pr in prs:
@@ -352,11 +348,11 @@ class Batch(models.Model):
                 for p in root | source
             )
 
-            title, body = re.match(r'(?P<title>[^\n]+)\n*(?P<body>.*)', message, flags=re.DOTALL).groups()
+            title, body = re.fullmatch(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}:{new_branch}',
-                'title': '[FW]' + (' ' if title[0] != '[' else '') + title,
+                'title': '[FW]' + ('' if title[0] == '[' else ' ') + title,
                 'body': body
             })
             if not r.ok:

From cf4d1629074eb9d976f4bed1feb546d75964d290 Mon Sep 17 00:00:00 2001
From: Xavier Morel <xmo@odoo.com>
Date: Tue, 22 Oct 2024 10:53:45 +0200
Subject: [PATCH 31/45] [ADD] *: PR backport wizard

This is not a full user-driven backport thingie for now, just one
admins can use to facilitate thing and debug issues with the
system. May eventually graduate to a frontend feature.

Fixes #925
---
 forwardport/models/forwardport.py         |   4 +-
 forwardport/models/project.py             |  22 +++-
 forwardport/tests/test_backport.py        | 117 ++++++++++++++++++
 runbot_merge/models/__init__.py           |   1 +
 runbot_merge/models/backport/__init__.py  | 140 ++++++++++++++++++++++
 runbot_merge/models/batch.py              |   2 +-
 runbot_merge/security/ir.model.access.csv |   1 +
 7 files changed, 280 insertions(+), 7 deletions(-)
 create mode 100644 forwardport/tests/test_backport.py
 create mode 100644 runbot_merge/models/backport/__init__.py

diff --git a/forwardport/models/forwardport.py b/forwardport/models/forwardport.py
index 6f9320e1..5e28e5e0 100644
--- a/forwardport/models/forwardport.py
+++ b/forwardport/models/forwardport.py
@@ -202,7 +202,7 @@ class ForwardPortTasks(models.Model, Queue):
             # 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)
+            conflict, head = source._create_port_branch(repo, target, forward=True)
             repo.push(git.fw_url(pr.repository), f'{head}:refs/heads/{ref}')
 
             remote_target = repository.fp_remote_target
@@ -302,7 +302,7 @@ class UpdateQueue(models.Model, Queue):
                     return
 
                 repo = git.get_local(previous.repository)
-                conflicts, new_head = previous._create_fp_branch(repo, child.target)
+                conflicts, new_head = previous._create_port_branch(repo, child.target, forward=True)
 
                 if conflicts:
                     _, out, err, _ = conflicts
diff --git a/forwardport/models/project.py b/forwardport/models/project.py
index f26ebfd5..00faa469 100644
--- a/forwardport/models/project.py
+++ b/forwardport/models/project.py
@@ -34,6 +34,8 @@ from odoo.addons.runbot_merge import git, utils
 from odoo.addons.runbot_merge.models.pull_requests import Branch
 from odoo.addons.runbot_merge.models.stagings_create import Message
 
+Conflict = tuple[str, str, str, list[str]]
+
 DEFAULT_DELTA = dateutil.relativedelta.relativedelta(days=3)
 
 _logger = logging.getLogger('odoo.addons.forwardport')
@@ -287,17 +289,29 @@ class PullRequests(models.Model):
         return sorted(commits, key=lambda c: idx[c['sha']])
 
 
-    def _create_fp_branch(self, source, target_branch):
+    def _create_port_branch(
+            self,
+            source: git.Repo,
+            target_branch: Branch,
+            *,
+            forward: bool,
+    ) -> tuple[typing.Optional[Conflict], str]:
         """ Creates a forward-port for the current PR to ``target_branch`` under
         ``fp_branch_name``.
 
-        :param target_branch: the branch to port forward to
-        :rtype: (None | (str, str, str, list[commit]), Repo)
+        :param source: the git repository to work with / in
+        :param target_branch: the branch to port ``self`` to
+        :param forward: whether this is a forward (``True``) or a back
+                        (``False``) port
+        :returns: a conflict if one happened, and the head of the port branch
+                  (either a succcessful port of the entire `self`, or a conflict
+                  commit)
         """
         logger = _logger.getChild(str(self.id))
         root = self.root_id
         logger.info(
-            "Forward-porting %s (%s) to %s",
+            "%s %s (%s) to %s",
+            "Forward-porting" if forward else "Back-porting",
             self.display_name, root.display_name, target_branch.name
         )
         fetch = source.with_config(stdout=subprocess.PIPE, stderr=subprocess.STDOUT).fetch()
diff --git a/forwardport/tests/test_backport.py b/forwardport/tests/test_backport.py
new file mode 100644
index 00000000..dbfd8408
--- /dev/null
+++ b/forwardport/tests/test_backport.py
@@ -0,0 +1,117 @@
+from xmlrpc.client import Fault
+
+import pytest
+
+from utils import make_basic, Commit, to_pr, seen
+
+
+@pytest.fixture
+def repo(env, config, make_repo):
+    repo, _ = make_basic(env, config, make_repo, statuses="default")
+    return repo
+
+@pytest.fixture
+def pr_id(env, repo, config):
+    with repo:
+        repo.make_commits('c', Commit("c", tree={'x': '1'}), ref='heads/aref')
+        pr = repo.make_pr(target='c', head='aref')
+        repo.post_status('aref', 'success')
+        pr.post_comment('hansen r+', config['role_reviewer']['token'])
+    env.run_crons()
+    with repo:
+        repo.post_status('staging.c', 'success')
+    env.run_crons()
+    pr_id = to_pr(env, pr)
+    assert pr_id.merge_date
+    return pr_id
+
+@pytest.fixture
+def backport_id(env, pr_id):
+    action = pr_id.backport()
+    backport_id = env[action['res_model']].browse([action['res_id']])
+    assert backport_id._name == 'runbot_merge.pull_requests.backport'
+    assert backport_id
+    return backport_id
+
+def test_golden_path(env, repo, config, pr_id, backport_id, users):
+    branch_a, branch_b, _branch_c = env['runbot_merge.branch'].search([], order='name')
+    backport_id.target = branch_a.id
+    act2 = backport_id.action_apply()
+    env.run_crons()  # run cron to update labels
+
+    _, bp_id = env['runbot_merge.pull_requests'].search([], order='number')
+    assert bp_id.limit_id == branch_b
+    assert bp_id._name == act2['res_model']
+    assert bp_id.id == act2['res_id']
+    bp_head = repo.commit(bp_id.head)
+    assert repo.read_tree(bp_head) == {
+        'f': 'e',
+        'x': '1',
+    }
+    assert bp_head.message == f"""c
+
+X-original-commit: {pr_id.head}\
+"""
+    assert bp_id.message == f"[Backport] c\n\nBackport of {pr_id.display_name}"
+    assert repo.get_pr(bp_id.number).labels == {"backport"}
+
+    # check that the backport can actually be merged and forward-ports successfully...
+    with repo:
+        repo.post_status(bp_id.head, 'success')
+        repo.get_pr(bp_id.number).post_comment("hansen r+", config['role_reviewer']['token'])
+    env.run_crons()
+    with repo:
+        repo.post_status('staging.a', 'success')
+    env.run_crons()
+    _pr, _backport, fw_id = env['runbot_merge.pull_requests'].search([], order='number')
+    fw_pr = repo.get_pr(fw_id.number)
+    assert fw_pr.comments == [
+        seen(env, fw_pr, users),
+        (users['user'], '''\
+@{user} @{reviewer} this PR targets b and is the last of the forward-port chain.
+
+To merge the full chain, use
+> @hansen r+
+
+More info at https://github.com/odoo/odoo/wiki/Mergebot#forward-port
+'''.format_map(users)),
+    ]
+
+def test_conflict(env, repo, config, backport_id):
+    with repo:
+        repo.make_commits('a', Commit('conflict', tree={'x': '-1'}), ref='heads/a', make=False)
+
+    branch_a, _branch_b, _branch_c = env['runbot_merge.branch'].search([], order='name')
+    backport_id.target = branch_a.id
+    with pytest.raises(Fault) as exc:
+        backport_id.action_apply()
+    assert exc.value.faultString == """\
+backport conflict:
+
+Auto-merging x
+CONFLICT (add/add): Merge conflict in x
+"""
+
+def test_target_error(env, config, backport_id):
+    branch_a, _branch_b, branch_c = env['runbot_merge.branch'].search([], order='name')
+    with pytest.raises(Fault) as exc:
+        backport_id.action_apply()
+    assert exc.value.faultString == "A backport needs a backport target"
+
+    backport_id.target = branch_c.id
+    with pytest.raises(Fault) as exc:
+        backport_id.action_apply()
+    assert exc.value.faultString == "The backport branch needs to be before the source's branch (got 'c' and 'c')"
+
+    backport_id.target = branch_a.id
+    backport_id.action_apply()
+
+@pytest.mark.skip(
+    reason="Currently no way to make just the PR creation fail, swapping the "
+           "fp_github_token for an invalid one breaks git itself"
+)
+def test_pr_fail(env, config, repo, pr_id, backport_id):
+    backport_id.target = env['runbot_merge.branch'].search([], order='name', limit=1).id
+    with pytest.raises(Fault) as exc:
+        backport_id.action_apply()
+    assert exc.value.faultString == 'Backport PR creation failure: '
diff --git a/runbot_merge/models/__init__.py b/runbot_merge/models/__init__.py
index 4a25ecd4..54fd25d5 100644
--- a/runbot_merge/models/__init__.py
+++ b/runbot_merge/models/__init__.py
@@ -8,5 +8,6 @@ from . import patcher
 from . import project_freeze
 from . import stagings_create
 from . import staging_cancel
+from . import backport
 from . import events_sources
 from . import crons
diff --git a/runbot_merge/models/backport/__init__.py b/runbot_merge/models/backport/__init__.py
new file mode 100644
index 00000000..ac58ce29
--- /dev/null
+++ b/runbot_merge/models/backport/__init__.py
@@ -0,0 +1,140 @@
+import logging
+import re
+import secrets
+import subprocess
+
+import requests
+
+from odoo import models, fields
+from odoo.exceptions import UserError
+
+from ..batch import Batch
+from ..project import Project
+from ..pull_requests import Repository
+from ... import git
+
+_logger = logging.getLogger(__name__)
+
+
+class PullRequest(models.Model):
+    _inherit = 'runbot_merge.pull_requests'
+
+    id: int
+    display_name: str
+    project: Project
+    repository: Repository
+    batch_id: Batch
+
+    def backport(self) -> dict:
+        if len(self) != 1:
+            raise UserError(f"Backporting works one PR at a time, got {len(self)}")
+
+        if len(self.batch_id.prs) > 1:
+            raise UserError("Automatic backport of multi-pr batches is not currently supported")
+
+        if not self.project.fp_github_token:
+            raise UserError(f"Can not backport {self.display_name}: no token on project {self.project.display_name}")
+
+        if not self.repository.fp_remote_target:
+            raise UserError(f"Can not backport {self.display_name}: no remote on {self.project.display_name}")
+
+        w = self.env['runbot_merge.pull_requests.backport'].create({
+            'pr_id': self.id,
+        })
+        return {
+            'type': 'ir.actions.act_window',
+            'name': f"Backport of {self.display_name}",
+            'views': [('form', False)],
+            'target': 'new',
+            'res_model': w._name,
+            'res_id': w.id,
+        }
+
+class PullRequestBackport(models.TransientModel):
+    _name = 'runbot_merge.pull_requests.backport'
+    _rec_name = 'pr_id'
+
+    pr_id = fields.Many2one('runbot_merge.pull_requests', required=True)
+    target = fields.Many2one('runbot_merge.branch')
+
+    def action_apply(self) -> dict:
+        if not self.target:
+            raise UserError("A backport needs a backport target")
+
+        project = self.pr_id.project
+        branches = project._forward_port_ordered().ids
+        source = self.pr_id.source_id or self.pr_id
+        source_idx = branches.index(source.target.id)
+        if branches.index(self.target.id) >= source_idx:
+            raise UserError(
+                "The backport branch needs to be before the source's branch "
+                f"(got {self.target.name!r} and {source.target.name!r})"
+            )
+
+        _logger.info(
+            "backporting %s (on %s) to %s",
+            self.pr_id.display_name,
+            self.pr_id.target.name,
+            self.target.name,
+        )
+
+        bp_branch = "%s-%s-%s-bp" % (
+            self.target.name,
+            self.pr_id.refname,
+            secrets.token_urlsafe(3),
+        )
+        repo_id = self.pr_id.repository
+        repo = git.get_local(repo_id)
+
+        old_map = self.pr_id.commits_map
+        self.pr_id.commits_map = "{}"
+        conflict, head = self.pr_id._create_port_branch(repo, self.target, forward=False)
+        self.pr_id.commits_map = old_map
+
+        if conflict:
+            feedback = "\n".join(filter(None, conflict[1:3]))
+            raise UserError(f"backport conflict:\n\n{feedback}")
+        repo.push(git.fw_url(repo_id), f"{head}:refs/heads/{bp_branch}")
+
+        self.env.cr.execute('LOCK runbot_merge_pull_requests IN SHARE MODE')
+
+        owner, _repo = repo_id.fp_remote_target.split('/', 1)
+        message = source.message + f"\n\nBackport of {self.pr_id.display_name}"
+        title, body = re.fullmatch(r'(?P<title>[^\n]+)\n*(?P<body>.*)', message, flags=re.DOTALL).groups()
+
+        r = requests.post(
+            f'https://api.github.com/repos/{repo_id.name}/pulls',
+            headers={'Authorization': f'token {project.fp_github_token}'},
+            json={
+                'base': self.target.name,
+                'head': f'{owner}:{bp_branch}',
+                'title': '[Backport]' + ('' if title[0] == '[' else ' ') + title,
+                'body': body
+            }
+        )
+        if not r.ok:
+            raise UserError(f"Backport PR creation failure: {r.text}")
+
+        backport = self.env['runbot_merge.pull_requests']._from_gh(r.json())
+        _logger.info("Created backport %s for %s", backport.display_name, self.pr_id.display_name)
+
+        backport.write({
+            'merge_method': self.pr_id.merge_method,
+            # the backport's own forwardport should stop right before the
+            # original PR by default
+            'limit_id': branches[source_idx - 1],
+        })
+        self.env['runbot_merge.pull_requests.tagging'].create({
+            'repository': repo_id.id,
+            'pull_request': backport.number,
+            'tags_add': ['backport'],
+        })
+        # scheduling fp followup probably doesn't make sense since we don't copy the fw_policy...
+
+        return {
+            'type': 'ir.actions.act_window',
+            'name': "new backport",
+            'views': [('form', False)],
+            'res_model': backport._name,
+            'res_id': backport.id,
+        }
diff --git a/runbot_merge/models/batch.py b/runbot_merge/models/batch.py
index 5e7dbbfb..df5cff03 100644
--- a/runbot_merge/models/batch.py
+++ b/runbot_merge/models/batch.py
@@ -326,7 +326,7 @@ class Batch(models.Model):
         conflicts = {}
         for pr in prs:
             repo = git.get_local(pr.repository)
-            conflicts[pr], head = pr._create_fp_branch(repo, target)
+            conflicts[pr], head = pr._create_port_branch(repo, target, forward=True)
 
             repo.push(git.fw_url(pr.repository), f"{head}:refs/heads/{new_branch}")
 
diff --git a/runbot_merge/security/ir.model.access.csv b/runbot_merge/security/ir.model.access.csv
index 0e8b0e8d..13949597 100644
--- a/runbot_merge/security/ir.model.access.csv
+++ b/runbot_merge/security/ir.model.access.csv
@@ -32,3 +32,4 @@ access_runbot_merge_review_rights_2,Users can see partners,model_res_partner_rev
 access_runbot_merge_review_override_2,Users can see partners,model_res_partner_override,base.group_user,1,0,0,0
 access_runbot_merge_pull_requests_feedback_template,access_runbot_merge_pull_requests_feedback_template,runbot_merge.model_runbot_merge_pull_requests_feedback_template,base.group_system,1,1,0,0
 access_runbot_merge_patch,Patcher access,runbot_merge.model_runbot_merge_patch,runbot_merge.group_patcher,1,1,1,0
+access_runbot_merge_backport_admin,Admin access to backport wizard,model_runbot_merge_pull_requests_backport,runbot_merge.group_admin,1,1,1,0

From 2174d7da31089371b6b93dddc326333ff14c36dd Mon Sep 17 00:00:00 2001
From: Xavier Morel <xmo@odoo.com>
Date: Tue, 22 Oct 2024 13:12:28 +0200
Subject: [PATCH 32/45] [IMP] runbot_merge: optimise edited event

For the longest time Github's `change` key was borked when
transitioning a description to and from empty. They fixed that during
2023, which I already saw and impacted on
DC (xmo-odoo/dummy_central@1ebed9d4183d34fa412f6556aedde019803a15a0
and xmo-odoo/dummy_central@937e87c2a4383f5ffcda6756443b082d2bde14bd)
but this had yet to be taken in account by the mergebot.

This is now done, the code is functionally reverted to what it was
before I realised `changes` was hosed and moved off of it in
3da18741963e80b34fd98055ecb6bc1483d09c35.

Fixes #743
---
 runbot_merge/controllers/__init__.py | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/runbot_merge/controllers/__init__.py b/runbot_merge/controllers/__init__.py
index 969331ea..eaaec321 100644
--- a/runbot_merge/controllers/__init__.py
+++ b/runbot_merge/controllers/__init__.py
@@ -210,12 +210,8 @@ def handle_pr(env, event):
                 updates['target'] = branch.id
                 updates['squash'] = pr['commits'] == 1
 
-        # turns out github doesn't bother sending a change key if the body is
-        # changing from empty (None), therefore ignore that entirely, just
-        # generate the message and check if it changed
-        message = utils.make_message(pr)
-        if message != pr_obj.message:
-            updates['message'] = message
+        if 'title' in event['changes'] or 'body' in event['changes']:
+            updates['message'] = utils.make_message(pr)
 
         _logger.info("update: %s = %s (by %s)", pr_obj.display_name, updates, event['sender']['login'])
         if updates:

From 2fea31883074b1c1f7c39d6119a124578ccb1ff6 Mon Sep 17 00:00:00 2001
From: Xavier Morel <xmo@odoo.com>
Date: Tue, 22 Oct 2024 13:57:58 +0200
Subject: [PATCH 33/45] [IMP] runbot_merge: hide concurrent update errors

As far as I can tell they are properly handled:

- In `handle_status` we let the http layer retry the query, which
  pretty much always succeeds.
- In `Commit.notify`, we rollback the application of the current
  commit, meaning it'll be processed by the next run of the cron,
  which also seems to succeed every time (that is going through the
  log I pretty much never notice the same commit being serialization
  failure'd twice in a row).

  Which we can trigger for faster action, this last item is not
  entirely necessary as statuses should generally come in fast and
  especially if we have concurrency errors, but it can't hurt.

This means the only genuine issue is... sql_db logging a "bad query"
every time there's a serialization failure.

In `handle_status`, just suppress the message outright, if there's an
error other than serialization the http / dispatch layer should catch
and log it.

In `Commit._notify` things are slightly more difficult as the execute
is implicit (`flush` -> `_write` -> `execute`) so we can't pass the
flag by parameter. One option would be to set and unset
`_default_log_exception`, but it would either be a bit dodgy or it
would require using a context manager and increasing the indentation
level (or using a custom context manager).

Instead just `mute_logger` the fucking thing. It's a bit brutish and
mostly used in tests, but not just, and feels like the least bad
option here...

Closes #805
---
 runbot_merge/controllers/__init__.py |  2 +-
 runbot_merge/models/pull_requests.py | 11 ++++++++---
 2 files changed, 9 insertions(+), 4 deletions(-)

diff --git a/runbot_merge/controllers/__init__.py b/runbot_merge/controllers/__init__.py
index eaaec321..10d815ef 100644
--- a/runbot_merge/controllers/__init__.py
+++ b/runbot_merge/controllers/__init__.py
@@ -374,7 +374,7 @@ def handle_status(env, event):
             SET to_check = true,
                 statuses = c.statuses::jsonb || EXCLUDED.statuses::jsonb
             WHERE NOT c.statuses::jsonb @> EXCLUDED.statuses::jsonb
-    """, [event['sha'], status_value])
+    """, [event['sha'], status_value], log_exceptions=False)
     env.ref("runbot_merge.process_updated_commits")._trigger()
 
     return 'ok'
diff --git a/runbot_merge/models/pull_requests.py b/runbot_merge/models/pull_requests.py
index 2c97c21a..4893444c 100644
--- a/runbot_merge/models/pull_requests.py
+++ b/runbot_merge/models/pull_requests.py
@@ -23,7 +23,7 @@ from markupsafe import Markup
 from odoo import api, fields, models, tools, Command
 from odoo.exceptions import AccessError, UserError
 from odoo.osv import expression
-from odoo.tools import html_escape, Reverse
+from odoo.tools import html_escape, Reverse, mute_logger
 from . import commands
 from .utils import enum, readonly, dfm
 
@@ -1978,9 +1978,11 @@ class Commit(models.Model):
             self.env.ref("runbot_merge.process_updated_commits")._trigger()
         return r
 
+    @mute_logger('odoo.sql_db')
     def _notify(self):
         Stagings = self.env['runbot_merge.stagings']
         PRs = self.env['runbot_merge.pull_requests']
+        serialization_failures = False
         # chances are low that we'll have more than one commit
         for c in self.search([('to_check', '=', True)]):
             sha = c.sha
@@ -1999,15 +2001,18 @@ class Commit(models.Model):
                 if stagings:
                     stagings._notify(c)
             except psycopg2.errors.SerializationFailure:
-                _logger.info("Failed to apply commit %s (%s): serialization failure", c, sha)
+                serialization_failures = True
+                _logger.info("Failed to apply commit %s: serialization failure", sha)
                 self.env.cr.rollback()
             except Exception:
-                _logger.exception("Failed to apply commit %s (%s)", c, sha)
+                _logger.exception("Failed to apply commit %s", sha)
                 self.env.cr.rollback()
             else:
                 self.env.cr.precommit.data['change-message'] = \
                     f"statuses changed on {sha}"
                 self.env.cr.commit()
+        if serialization_failures:
+            self.env.ref("runbot_merge.process_updated_commits")._trigger()
 
     _sql_constraints = [
         ('unique_sha', 'unique (sha)', 'no duplicated commit'),

From e7716f8b778bd0859dda92a24f1e1bd47b4c1f1e Mon Sep 17 00:00:00 2001
From: Xavier Morel <xmo@odoo.com>
Date: Tue, 22 Oct 2024 15:05:48 +0200
Subject: [PATCH 34/45] [FIX] *: fw=no reflection in the PR dashboard

Like limit, fw=no should restrict the table length, in this case to
just the current branch (as we're not forward porting at all).

Before this, `no` would not be applied as a limit visually, the table
would still go up to the main branch which is very confusing.

Fixes #962
---
 forwardport/tests/test_limit.py  | 11 ++++++++++-
 runbot_merge/views/templates.xml |  2 ++
 2 files changed, 12 insertions(+), 1 deletion(-)

diff --git a/forwardport/tests/test_limit.py b/forwardport/tests/test_limit.py
index e5bfcd3e..7df9d9e5 100644
--- a/forwardport/tests/test_limit.py
+++ b/forwardport/tests/test_limit.py
@@ -1,3 +1,4 @@
+import lxml.html
 import pytest
 
 from utils import seen, Commit, make_basic, to_pr
@@ -106,7 +107,7 @@ def test_ignore(env, config, make_repo, users):
         "should finally have created a forward port"
 
 
-def test_unignore(env, config, make_repo, users):
+def test_unignore(env, config, make_repo, users, page):
     env['res.partner'].create({'name': users['other'], 'github_login': users['other'], 'email': 'other@example.org'})
 
     prod, _ = make_basic(env, config, make_repo, statuses="default")
@@ -121,11 +122,19 @@ def test_unignore(env, config, make_repo, users):
     pr_id = to_pr(env, pr)
     assert pr_id.batch_id.fw_policy == 'default'
 
+    doc = lxml.html.fromstring(page(f'/{prod.name}/pull/{pr.number}'))
+    assert len(doc.cssselect("table > tbody > tr")) == 3, \
+        "the overview table should have as many rows as there are tables"
+
     with prod:
         pr.post_comment('hansen fw=no', token)
     env.run_crons(None)
     assert pr_id.batch_id.fw_policy == 'no'
 
+    doc = lxml.html.fromstring(page(f'/{prod.name}/pull/{pr.number}'))
+    assert len(doc.cssselect("table > tbody > tr")) == 1, \
+        "if fw=no, there should be just one row"
+
     with prod:
         pr.post_comment('hansen fw=default', token)
     env.run_crons(None)
diff --git a/runbot_merge/views/templates.xml b/runbot_merge/views/templates.xml
index 2ac80f83..9b5bea4b 100644
--- a/runbot_merge/views/templates.xml
+++ b/runbot_merge/views/templates.xml
@@ -465,6 +465,8 @@ if not genealogy:
     repos = pr.repository
 elif all(p.state in ('merged', 'closed') for p in genealogy[-1].all_prs):
     branches = (project.branch_ids & targets)[::-1]
+elif pr.batch_id.fw_policy == 'no':
+    branches = pr.target
 else:
     # if the tip of the genealogy is not closed, extend to the furthest limit,
     # keeping branches which are active or have an associated batch / PR

From 11f2231e828b30ccc58058416e9913a220c13485 Mon Sep 17 00:00:00 2001
From: Xavier Morel <xmo@odoo.com>
Date: Tue, 22 Oct 2024 15:08:00 +0200
Subject: [PATCH 35/45] [FIX] runbot_merge: missing model description

---
 runbot_merge/models/backport/__init__.py | 1 +
 1 file changed, 1 insertion(+)

diff --git a/runbot_merge/models/backport/__init__.py b/runbot_merge/models/backport/__init__.py
index ac58ce29..ddc89c7a 100644
--- a/runbot_merge/models/backport/__init__.py
+++ b/runbot_merge/models/backport/__init__.py
@@ -52,6 +52,7 @@ class PullRequest(models.Model):
 
 class PullRequestBackport(models.TransientModel):
     _name = 'runbot_merge.pull_requests.backport'
+    _description = "PR backport wizard"
     _rec_name = 'pr_id'
 
     pr_id = fields.Many2one('runbot_merge.pull_requests', required=True)

From 5b94dcce35a524102986b1b562864065e659e5a9 Mon Sep 17 00:00:00 2001
From: Xavier Morel <xmo@odoo.com>
Date: Tue, 29 Oct 2024 13:13:59 +0100
Subject: [PATCH 36/45] [FIX] runbot_merge: reset markdown renderer

pymarkdown's footnotes plugin *saves footnotes across invocations by
default*. Even if I understand the documented use case it seems wild
that it's not opt-in...

Anyway disable that resetting all internal state. Thanks rfr for the
inital report that things were looking odd.
---
 runbot_merge/models/utils.py | 1 +
 1 file changed, 1 insertion(+)

diff --git a/runbot_merge/models/utils.py b/runbot_merge/models/utils.py
index a66fe0b4..50740c59 100644
--- a/runbot_merge/models/utils.py
+++ b/runbot_merge/models/utils.py
@@ -29,6 +29,7 @@ def dfm(repository: str, text: str) -> Markup:
     """
     t = DFM_CONTEXT_REPO.set(repository)
     try:
+        dfm_renderer.reset()
         return Markup(dfm_renderer.convert(escape(text)))
     finally:
         DFM_CONTEXT_REPO.reset(t)

From a12e593fbabcf31bbf8660cf2a20ea6f343b7657 Mon Sep 17 00:00:00 2001
From: Xavier Morel <xmo@odoo.com>
Date: Mon, 18 Nov 2024 09:48:48 +0100
Subject: [PATCH 37/45] [FIX] runbot_merge: backport wizard

- fix incorrect view specs (the action id comes first)
- add a wizard form and hook it into the PR, completely forgot to do
  that
- usability improvements: filter branches to be in the same project as
  the PR being backported, and older than the current PR's branch

The latter is a somewhat incomplete condition: ideally we'd want to
only allow selecting branches preceding the target of the *source* of
the PR being backported, that way we don't risk errors when
backporting forward-ports (the condition should be checked in the
final action but still).

Also we're only filtering by sequence, so we're missing the name part
of the ordering, hence if multiple branches have the same sequence we
may not allow selecting some of the preceding branches.
---
 runbot_merge/__manifest__.py             |  1 +
 runbot_merge/models/backport/__init__.py | 12 ++++++++----
 runbot_merge/models/backport/views.xml   | 23 +++++++++++++++++++++++
 3 files changed, 32 insertions(+), 4 deletions(-)
 create mode 100644 runbot_merge/models/backport/views.xml

diff --git a/runbot_merge/__manifest__.py b/runbot_merge/__manifest__.py
index a21ce712..a00eb8ba 100644
--- a/runbot_merge/__manifest__.py
+++ b/runbot_merge/__manifest__.py
@@ -19,6 +19,7 @@
         'views/templates.xml',
         'models/project_freeze/views.xml',
         'models/staging_cancel/views.xml',
+        'models/backport/views.xml',
     ],
     'assets': {
         'web.assets_frontend': [
diff --git a/runbot_merge/models/backport/__init__.py b/runbot_merge/models/backport/__init__.py
index ddc89c7a..7be4652a 100644
--- a/runbot_merge/models/backport/__init__.py
+++ b/runbot_merge/models/backport/__init__.py
@@ -1,7 +1,6 @@
 import logging
 import re
 import secrets
-import subprocess
 
 import requests
 
@@ -44,7 +43,7 @@ class PullRequest(models.Model):
         return {
             'type': 'ir.actions.act_window',
             'name': f"Backport of {self.display_name}",
-            'views': [('form', False)],
+            'views': [(False, 'form')],
             'target': 'new',
             'res_model': w._name,
             'res_id': w.id,
@@ -56,7 +55,12 @@ class PullRequestBackport(models.TransientModel):
     _rec_name = 'pr_id'
 
     pr_id = fields.Many2one('runbot_merge.pull_requests', required=True)
-    target = fields.Many2one('runbot_merge.branch')
+    project_id = fields.Many2one(related='pr_id.repository.project_id')
+    source_seq = fields.Integer(related='pr_id.target.sequence')
+    target = fields.Many2one(
+        'runbot_merge.branch',
+        domain="[('project_id', '=', project_id), ('sequence', '>', source_seq)]",
+    )
 
     def action_apply(self) -> dict:
         if not self.target:
@@ -135,7 +139,7 @@ class PullRequestBackport(models.TransientModel):
         return {
             'type': 'ir.actions.act_window',
             'name': "new backport",
-            'views': [('form', False)],
+            'views': [(False, 'form')],
             'res_model': backport._name,
             'res_id': backport.id,
         }
diff --git a/runbot_merge/models/backport/views.xml b/runbot_merge/models/backport/views.xml
new file mode 100644
index 00000000..ec34d972
--- /dev/null
+++ b/runbot_merge/models/backport/views.xml
@@ -0,0 +1,23 @@
+<odoo>
+    <record id="runbot_merge_backport_form" model="ir.ui.view">
+        <field name="name">Backport Wizard</field>
+        <field name="model">runbot_merge.pull_requests.backport</field>
+        <field name="arch" type="xml">
+            <form>
+                <field name="pr_id" invisible="1"/>
+                <field name="project_id" invisible="1"/>
+                <field name="source_seq" invisible="1"/>
+                <field name="target" class="w-100"/>
+            </form>
+        </field>
+    </record>
+
+    <record id="runbot_merge_backport_pr_action" model="ir.actions.server">
+        <field name="name">Perform backport from current PR</field>
+        <field name="type">ir.actions.server</field>
+        <field name="model_id" ref="model_runbot_merge_pull_requests"/>
+        <field name="binding_model_id" ref="model_runbot_merge_pull_requests"/>
+        <field name="state">code</field>
+        <field name="code">action = record.backport()</field>
+    </record>
+</odoo>

From 5441ba12ae62bf634e59beb55607f64635c8631e Mon Sep 17 00:00:00 2001
From: Xavier Morel <xmo@odoo.com>
Date: Mon, 18 Nov 2024 12:37:44 +0100
Subject: [PATCH 38/45] [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<file_from>\S+)(:?\s.*)?\n
-\+\+\+\x20b/(?P<file_to>\S+)(:?\s.*)?\n
+---\x20(?P<prefix_a>a/)?(?P<file_from>\S+)(:?\s.*)?\n
+\+\+\+\x20(?P<prefix_b>b/)?(?P<file_to>\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 <bar@example.org>
+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 <bar@example.org>
+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 <bar@example.org>
-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, '<p>Unstaged direct-application patch created</p>', []),
+        (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 @@
                         </group>
                     </group>
                 </sheet>
+                <div class="oe_chatter">
+                    <field name="message_follower_ids" widget="mail_followers"/>
+                    <field name="message_ids" widget="mail_thread"/>
+                </div>
             </form>
         </field>
     </record>

From c974f510366e00e023773cfe13669528a6f8ee6f Mon Sep 17 00:00:00 2001
From: Xavier Morel <xmo@odoo.com>
Date: Mon, 18 Nov 2024 13:09:23 +0100
Subject: [PATCH 39/45] [IMP] runbot_merge: trigger staging if re-enabled for
 branch

If staging gets re-enabled on a branch (or the branch itself gets
re-enabled), immediately run a staging cron as there may already be
PRs waiting, and no trigger enqueued: cron triggers have no payload,
they just get removed when the cron runs which means if a bunch of PRs
become ready for branch B with staging disabled, the cron is going to
run, it's going to stage nothing on that branch (because staging is
disabled) then it's going to delete all the triggers.

Fixes #979
---
 runbot_merge/models/pull_requests.py | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/runbot_merge/models/pull_requests.py b/runbot_merge/models/pull_requests.py
index 4893444c..c0bf5f4e 100644
--- a/runbot_merge/models/pull_requests.py
+++ b/runbot_merge/models/pull_requests.py
@@ -315,6 +315,13 @@ class Branch(models.Model):
                 'message': tmpl._format(pr=pr),
             } for pr in actives.prs])
             self.env.ref('runbot_merge.branch_cleanup')._trigger()
+
+        if (
+            (vals.get('staging_enabled') is True and not all(self.mapped('staging_enabled')))
+         or (vals.get('active') is True and not all(self.mapped('active')))
+        ):
+            self.env.ref('runbot_merge.staging_cron')._trigger()
+
         super().write(vals)
         return True
 

From fbfb96be53d126efdabdd7e4d232adb6c783ad82 Mon Sep 17 00:00:00 2001
From: Xavier Morel <xmo@odoo.com>
Date: Mon, 18 Nov 2024 13:52:27 +0100
Subject: [PATCH 40/45] [IMP] runbot_merge: ping commenter when fetching PR due
 to comment

If a comment causes an unknown PR to be fetched, it's a bit odd to
ping the author (and possibly reviewer) anyway as they're not super
concerned (and technically we could be ignoring the purported /
attempted reviewer).

So if a fetch job was created because of a comment, remember the
comment author and ping *them* instead of using the default ping
policy.

Fixes #981
---
 runbot_merge/controllers/__init__.py          |  4 ++-
 ..._merge.pull_requests.feedback.template.csv |  2 +-
 runbot_merge/models/pull_requests.py          | 33 ++++++++++++++++---
 runbot_merge/tests/test_basic.py              |  4 +--
 4 files changed, 35 insertions(+), 8 deletions(-)

diff --git a/runbot_merge/controllers/__init__.py b/runbot_merge/controllers/__init__.py
index 10d815ef..dce688ed 100644
--- a/runbot_merge/controllers/__init__.py
+++ b/runbot_merge/controllers/__init__.py
@@ -431,7 +431,9 @@ def _handle_comment(env, repo, issue, comment, target=None):
     if not repository.project_id._find_commands(comment['body'] or ''):
         return "No commands, ignoring"
 
-    pr = env['runbot_merge.pull_requests']._get_or_schedule(repo, issue, target=target)
+    pr = env['runbot_merge.pull_requests']._get_or_schedule(
+        repo, issue, target=target, commenter=comment['user']['login'],
+    )
     if not pr:
         return "Unknown PR, scheduling fetch"
 
diff --git a/runbot_merge/data/runbot_merge.pull_requests.feedback.template.csv b/runbot_merge/data/runbot_merge.pull_requests.feedback.template.csv
index eab2a241..b140ec43 100644
--- a/runbot_merge/data/runbot_merge.pull_requests.feedback.template.csv
+++ b/runbot_merge/data/runbot_merge.pull_requests.feedback.template.csv
@@ -16,7 +16,7 @@ runbot_merge.pr.load.unmanaged,"Branch `{pr[base][ref]}` is not within my remit,
 
 pr: pull request (github object)
 Repository: repository object (???)"
-runbot_merge.pr.load.fetched,"{pr.ping}I didn't know about this PR and had to retrieve its information, you may have to re-approve it as I didn't see previous commands.","Notifies that we did retrieve an unknown PR (either by request or as side effect of an interaction).
+runbot_merge.pr.load.fetched,"{ping}I didn't know about this PR and had to retrieve its information, you may have to re-approve it as I didn't see previous commands.","Notifies that we did retrieve an unknown PR (either by request or as side effect of an interaction).
 
 Pr: pr object we just created"
 runbot_merge.pr.branch.disabled,"{pr.ping}the target branch {pr.target.name!r} has been disabled, you may want to close this PR.","Notifies that the target branch for this PR was deactivated.
diff --git a/runbot_merge/models/pull_requests.py b/runbot_merge/models/pull_requests.py
index c0bf5f4e..d301523f 100644
--- a/runbot_merge/models/pull_requests.py
+++ b/runbot_merge/models/pull_requests.py
@@ -107,7 +107,14 @@ All substitutions are tentatively applied sequentially to the input.
             self._cr, 'runbot_merge_unique_repo', self._table, ['name'])
         return res
 
-    def _load_pr(self, number, *, closing=False, squash=False):
+    def _load_pr(
+        self,
+        number: int,
+        *,
+        closing: bool = False,
+        squash: bool = False,
+        ping: str | None = None,
+    ):
         gh = self.github()
 
         # fetch PR object and handle as *opened*
@@ -238,7 +245,10 @@ All substitutions are tentatively applied sequentially to the input.
         self.env.ref('runbot_merge.pr.load.fetched')._send(
             repository=self,
             pull_request=number,
-            format_args={'pr': pr_id},
+            format_args={
+                'pr': pr_id,
+                'ping': ping or pr_id.ping,
+            },
         )
 
     def having_branch(self, branch):
@@ -635,7 +645,15 @@ class PullRequests(models.Model):
             return json.loads(self.overrides)
         return {}
 
-    def _get_or_schedule(self, repo_name, number, *, target=None, closing=False) -> PullRequests | None:
+    def _get_or_schedule(
+            self,
+            repo_name: str,
+            number: int,
+            *,
+            target: str | None = None,
+            closing: bool = False,
+            commenter: str | None = None,
+    ) -> PullRequests | None:
         repo = self.env['runbot_merge.repository'].search([('name', '=', repo_name)])
         if not repo:
             source = self.env['runbot_merge.events_sources'].search([('repository', '=', repo_name)])
@@ -682,6 +700,7 @@ class PullRequests(models.Model):
             'repository': repo.id,
             'number': number,
             'closing': closing,
+            'commenter': commenter,
         })
 
     def _iter_ancestors(self) -> Iterator[PullRequests]:
@@ -2518,6 +2537,7 @@ class FetchJob(models.Model):
     number = fields.Integer(required=True, group_operator=None)
     closing = fields.Boolean(default=False)
     commits_at = fields.Datetime(index="btree_not_null")
+    commenter = fields.Char()
 
     @api.model_create_multi
     def create(self, vals_list):
@@ -2545,7 +2565,12 @@ class FetchJob(models.Model):
             f.active = False
             self.env.cr.execute("SAVEPOINT runbot_merge_before_fetch")
             try:
-                f.repository._load_pr(f.number, closing=f.closing, squash=bool(f.commits_at))
+                f.repository._load_pr(
+                    f.number,
+                    closing=f.closing,
+                    squash=bool(f.commits_at),
+                    ping=f.commenter and f'@{f.commenter} ',
+                )
             except Exception:
                 self.env.cr.execute("ROLLBACK TO SAVEPOINT runbot_merge_before_fetch")
                 _logger.exception("Failed to load pr %s, skipping it", f.number)
diff --git a/runbot_merge/tests/test_basic.py b/runbot_merge/tests/test_basic.py
index 9fee6f6f..fa1e72f0 100644
--- a/runbot_merge/tests/test_basic.py
+++ b/runbot_merge/tests/test_basic.py
@@ -3414,7 +3414,7 @@ class TestUnknownPR:
             (users['reviewer'], 'hansen r+'),
             (users['reviewer'], 'hansen r+'),
             seen(env, prx, users),
-            (users['user'], f"@{users['user']} I didn't know about this PR and had to "
+            (users['user'], f"@{users['reviewer']} I didn't know about this PR and had to "
                             "retrieve its information, you may have to "
                             "re-approve it as I didn't see previous commands."),
         ]
@@ -3470,7 +3470,7 @@ class TestUnknownPR:
             # reviewer is set because fetch replays all the comments (thus
             # setting r+ and reviewer) but then syncs the head commit thus
             # unsetting r+ but leaving the reviewer
-            (users['user'], f"@{users['user']} I didn't know about this PR and had to retrieve "
+            (users['user'], f"@{users['reviewer']} I didn't know about this PR and had to retrieve "
                             "its information, you may have to re-approve it "
                             "as I didn't see previous commands."),
         ]

From 3fe29ba8f6b491c01ec6a0514f8dd650157ad8ba Mon Sep 17 00:00:00 2001
From: Xavier Morel <xmo@odoo.com>
Date: Mon, 18 Nov 2024 14:18:25 +0100
Subject: [PATCH 41/45] [IMP] forwardport: batch list

Since b45ecf08f955478cc0e0e76177c1223c8dfc0898 forwardport batches
which fail have a delay set in order to avoid spamming. However that
delay was not displayed anywhere, which made things confusing as the
batch would not get run even after creating new triggers.

Show the delay if it's set (to a value later than now), as a relative
delta for clarity (as normally the delay is in minutes so a full blown
date is difficult to read / aprehend), and allow viewing and setting
it in the form view.

Fixes #982
---
 forwardport/data/queues.xml       | 10 ++++++++--
 forwardport/models/forwardport.py | 11 +++++++++++
 2 files changed, 19 insertions(+), 2 deletions(-)

diff --git a/forwardport/data/queues.xml b/forwardport/data/queues.xml
index ba011344..8d552a2a 100644
--- a/forwardport/data/queues.xml
+++ b/forwardport/data/queues.xml
@@ -11,6 +11,7 @@
             <tree>
                 <field name="source"/>
                 <field name="batch_id"/>
+                <field name="retry_after_relative" string="Retry In"/>
             </tree>
         </field>
     </record>
@@ -20,8 +21,13 @@
         <field name="arch" type="xml">
             <form>
                 <group>
-                    <group><field name="source"/></group>
-                    <group><field name="batch_id"/></group>
+                    <group>
+                        <field name="source"/>
+                        <field name="batch_id"/>
+                    </group>
+                    <group>
+                        <field name="retry_after"/>
+                    </group>
                 </group>
             </form>
         </field>
diff --git a/forwardport/models/forwardport.py b/forwardport/models/forwardport.py
index 5e28e5e0..c8824167 100644
--- a/forwardport/models/forwardport.py
+++ b/forwardport/models/forwardport.py
@@ -7,6 +7,7 @@ from datetime import datetime, timedelta
 
 import requests
 import sentry_sdk
+from babel.dates import format_timedelta
 from dateutil import relativedelta
 
 from odoo import api, fields, models
@@ -73,6 +74,7 @@ class ForwardPortTasks(models.Model, Queue):
         ('complete', 'Complete ported batches'),
     ], required=True)
     retry_after = fields.Datetime(required=True, default='1900-01-01 01:01:01')
+    retry_after_relative = fields.Char(compute="_compute_retry_after_relative")
     pr_id = fields.Many2one('runbot_merge.pull_requests')
 
     @api.model_create_multi
@@ -91,6 +93,15 @@ class ForwardPortTasks(models.Model, Queue):
             ('retry_after', '<=', fields.Datetime.to_string(fields.Datetime.now())),
         ]
 
+    @api.depends('retry_after')
+    def _compute_retry_after_relative(self):
+        now = fields.Datetime.now()
+        for t in self:
+            if t.retry_after <= now:
+                t.retry_after_relative = ""
+            else:
+                t.retry_after_relative = format_timedelta(t.retry_after - now, locale=t.env.lang)
+
     def _on_failure(self):
         super()._on_failure()
         self.retry_after = fields.Datetime.to_string(fields.Datetime.now() + timedelta(minutes=30))

From 63a0ee90b224493efd93dc8927dda1bf8bbd8118 Mon Sep 17 00:00:00 2001
From: Xavier Morel <xmo@odoo.com>
Date: Mon, 18 Nov 2024 14:45:21 +0100
Subject: [PATCH 42/45] [ADD] runbot_merge: views from custom

Add a few views / view extensions set as custom on the production
mergebot which I never remembered to implement in the actual source.
---
 runbot_merge/views/mergebot.xml | 10 ++++++++++
 runbot_merge/views/queues.xml   |  2 +-
 2 files changed, 11 insertions(+), 1 deletion(-)

diff --git a/runbot_merge/views/mergebot.xml b/runbot_merge/views/mergebot.xml
index 66888469..78df490f 100644
--- a/runbot_merge/views/mergebot.xml
+++ b/runbot_merge/views/mergebot.xml
@@ -115,6 +115,7 @@
                 <field name="state"/>
                 <field name="author"/>
                 <field name="reviewed_by"/>
+                <field name="write_date"/>
             </tree>
         </field>
     </record>
@@ -380,6 +381,15 @@
         <field name="res_model">runbot_merge.commit</field>
         <field name="view_mode">tree,form</field>
     </record>
+    <record id="runbot_merge_commits_search" model="ir.ui.view">
+        <field name="name">commits search</field>
+        <field name="model">runbot_merge.commit</field>
+        <field name="arch" type="xml">
+            <search>
+                <field name="sha" operator="="/>
+            </search>
+        </field>
+    </record>
     <record id="runbot_merge_commits_tree" model="ir.ui.view">
         <field name="name">commits list</field>
         <field name="model">runbot_merge.commit</field>
diff --git a/runbot_merge/views/queues.xml b/runbot_merge/views/queues.xml
index 6bec187e..1514bec4 100644
--- a/runbot_merge/views/queues.xml
+++ b/runbot_merge/views/queues.xml
@@ -23,7 +23,7 @@
         <field name="res_model">runbot_merge.pull_requests.feedback</field>
     </record>
     <record id="tree_feedback" model="ir.ui.view">
-        <field name="name">Feedback</field>
+        <field name="name">Feedback List</field>
         <field name="model">runbot_merge.pull_requests.feedback</field>
         <field name="arch" type="xml">
             <tree>

From 4563fc5fc062507c7d451096438b58c6275aa2d8 Mon Sep 17 00:00:00 2001
From: Xavier Morel <xmo@odoo.com>
Date: Mon, 18 Nov 2024 14:49:17 +0100
Subject: [PATCH 43/45] [FIX] runbot_merge: dashboard image is branch created
 via SQL

If a branch is created via SQL directly, one might forget to set its
`write_date`, leading the computation of the `last_modified` to be
*really* unhappy as it's asked to compare a bunch of datetime objects
to `False` in order to compute the `max`.

Substitute missing `write_date` with `datetime.min` so they are
extremely unlikely to influence the computation until and unless the
branch gets updated.
---
 runbot_merge/controllers/dashboard.py | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/runbot_merge/controllers/dashboard.py b/runbot_merge/controllers/dashboard.py
index 9cdc55d8..ab484727 100644
--- a/runbot_merge/controllers/dashboard.py
+++ b/runbot_merge/controllers/dashboard.py
@@ -4,6 +4,7 @@ from __future__ import annotations
 import base64
 import collections
 import colorsys
+import datetime
 import hashlib
 import io
 import json
@@ -160,7 +161,7 @@ def raster_render(pr):
     # last-modified should be in RFC2822 format, which is what
     # email.utils.formatdate does (sadly takes a timestamp but...)
     last_modified = formatdate(max((
-        o.write_date
+        o.write_date or datetime.datetime.min
         for o in chain(
             project,
             repos,

From c4bdb75d9c589c7b5a6cedd02da57390f838a91c Mon Sep 17 00:00:00 2001
From: Xavier Morel <xmo@odoo.com>
Date: Tue, 19 Nov 2024 10:15:25 +0100
Subject: [PATCH 44/45] [FIX] runbot_merge: `EmailMessage.get_content`
 misbehaves

`get_content` round-trips the text part through `ascii` with
`error=replace`, so if the input is not ascii it screws up
tremendously, which leads to either failing to apply patches (the more
likely situation) or corrupting the patches.

`get_payload`, if called without `decode`, pretty much just returns
the payload unless it needs a decoding pass (e.g. because it contains
raw surrogates, but that should not be an issue for us). So this is
really what we want.

While at it, increase `patch`'s verbosity in case it can give us more
info.
---
 runbot_merge/models/patcher.py      | 6 +++---
 runbot_merge/tests/test_patching.py | 4 ++--
 2 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/runbot_merge/models/patcher.py b/runbot_merge/models/patcher.py
index 199dd02d..d7ad0dfd 100644
--- a/runbot_merge/models/patcher.py
+++ b/runbot_merge/models/patcher.py
@@ -99,7 +99,7 @@ def parse_format_patch(p: Patch) -> ParseResult:
     name, email = parseaddr(m['from'])
     author = (name, email, m['date'])
     msg = re.sub(r'^\[PATCH( \d+/\d+)?\] ', '', m['subject'])
-    body, _, rest = m.get_content().partition('---\n')
+    body, _, rest = m.get_payload().partition('---\n')
     if body:
         msg += '\n\n' + body
 
@@ -300,14 +300,14 @@ class Patch(models.Model):
              tempfile.TemporaryDirectory() as tmpdir:
             tf.extractall(tmpdir)
             patch = subprocess.run(
-                ['patch', f'-p{prefix}', '-d', tmpdir],
+                ['patch', f'-p{prefix}', '--directory', tmpdir, '--verbose'],
                 input=p.patch,
                 stdout=subprocess.PIPE,
                 stderr=subprocess.PIPE,
                 encoding='utf-8',
             )
             if patch.returncode:
-                raise PatchFailure(patch.stdout)
+                raise PatchFailure("\n---------\n".join(filter(None, [p.patch, patch.stdout.strip(), patch.stderr.strip()])))
             new_tree = r.update_tree(self.target.name, files)
 
         sha = r.stdout().with_config(encoding='utf-8')\
diff --git a/runbot_merge/tests/test_patching.py b/runbot_merge/tests/test_patching.py
index 020fbae4..2f2c8b64 100644
--- a/runbot_merge/tests/test_patching.py
+++ b/runbot_merge/tests/test_patching.py
@@ -2,7 +2,7 @@ import xmlrpc.client
 
 import pytest
 
-from utils import Commit, read_tracking_value
+from utils import Commit, read_tracking_value, matches
 
 # basic udiff / show style patch, updates `b` from `1` to `2`
 BASIC_UDIFF = """\
@@ -255,7 +255,7 @@ def test_patch_conflict(env, project, repo, users):
         [],
     ), (
         "Unable to apply patch",
-        "<p>patching file b<br>Hunk #1 FAILED at 1.<br>1 out of 1 hunk FAILED -- saving rejects to file b.rej<br></p>",
+        matches("$$"),  # feedback from patch can vary
         [],
     ), (
         False, '', [('active', 1, 0)]

From 667aa69f5b5224a22ca5a1acf2518b44f3832101 Mon Sep 17 00:00:00 2001
From: Xavier Morel <xmo@odoo.com>
Date: Tue, 19 Nov 2024 14:58:41 +0100
Subject: [PATCH 45/45] [FIX] runbot_merge, forwardport: create_single is
 deprecated

Update a bunch of `create` overrides to work in batch. Also fix a few
`super()` calls unnecessarily in legacy style.
---
 forwardport/models/project.py        |  68 +++++++++-----
 runbot_merge/models/pull_requests.py | 135 +++++++++++++++------------
 2 files changed, 121 insertions(+), 82 deletions(-)

diff --git a/forwardport/models/project.py b/forwardport/models/project.py
index 00faa469..89fa5102 100644
--- a/forwardport/models/project.py
+++ b/forwardport/models/project.py
@@ -185,31 +185,55 @@ class PullRequests(models.Model):
 
     reminder_backoff_factor = fields.Integer(default=-4, group_operator=None)
 
-    @api.model_create_single
-    def create(self, vals):
-        # PR opened event always creates a new PR, override so we can precreate PRs
-        existing = self.search([
-            ('repository', '=', vals['repository']),
-            ('number', '=', vals['number']),
-        ])
-        if existing:
-            return existing
+    @api.model_create_multi
+    def create(self, vals_list):
+        # we're doing a lot of weird processing so fast-path this case even if
+        # unlikely
+        if not vals_list:
+            return self.browse()
 
-        if vals.get('parent_id') and 'source_id' not in vals:
-            vals['source_id'] = self.browse(vals['parent_id']).root_id.id
-        pr = super().create(vals)
+        existings = []
+        to_create = []
+        for vals in vals_list:
+            # PR opened event always creates a new PR, override so we can precreate PRs
+            if existing := self.search([
+                ('repository', '=', vals['repository']),
+                ('number', '=', vals['number']),
+            ]):
+                existings.append(existing.id)
+            else:
+                existings.append(None)
+                to_create.append(vals)
+                if vals.get('parent_id') and 'source_id' not in vals:
+                    vals['source_id'] = self.browse(vals['parent_id']).root_id.id
 
-        # added a new PR to an already forward-ported batch: port the PR
-        if self.env['runbot_merge.batch'].search_count([
-            ('parent_id', '=', pr.batch_id.id),
-        ]):
-            self.env['forwardport.batches'].create({
-                'batch_id': pr.batch_id.id,
-                'source': 'complete',
-                'pr_id': pr.id,
-            })
+        # no PR to create, return the existing ones
+        if not to_create:
+            return self.browse(existings)
 
-        return pr
+        prs = super().create(to_create)
+
+        for pr in prs:
+            # added a new PR to an already forward-ported batch: port the PR
+            if self.env['runbot_merge.batch'].search_count([
+                ('parent_id', '=', pr.batch_id.id),
+            ]):
+                self.env['forwardport.batches'].create({
+                    'batch_id': pr.batch_id.id,
+                    'source': 'complete',
+                    'pr_id': pr.id,
+                })
+
+        # no existing PR, return all the created ones
+        if len(to_create) == len(vals_list):
+            return prs
+
+        # merge existing and created
+        prs = iter(prs)
+        return self.browse((
+            existing or next(prs).id
+            for existing in existings
+        ))
 
     def write(self, vals):
         # if the PR's head is updated, detach (should split off the FP lines as this is not the original code)
diff --git a/runbot_merge/models/pull_requests.py b/runbot_merge/models/pull_requests.py
index e6fa6dd6..4ab04aa5 100644
--- a/runbot_merge/models/pull_requests.py
+++ b/runbot_merge/models/pull_requests.py
@@ -82,15 +82,16 @@ class Repository(models.Model):
 All substitutions are tentatively applied sequentially to the input.
 """)
 
-    @api.model
-    def create(self, vals):
-        if 'status_ids' in vals:
-            return super().create(vals)
+    @api.model_create_multi
+    def create(self, vals_list):
+        for vals in vals_list:
+            if 'status_ids' in vals:
+                continue
 
-        st = vals.pop('required_statuses', 'legal/cla,ci/runbot')
-        if st:
-            vals['status_ids'] = [(0, 0, {'context': c}) for c in st.split(',')]
-        return super().create(vals)
+            if st := vals.pop('required_statuses', 'legal/cla,ci/runbot'):
+                vals['status_ids'] = [(0, 0, {'context': c}) for c in st.split(',')]
+
+        return super().create(vals_list)
 
     def write(self, vals):
         st = vals.pop('required_statuses', None)
@@ -102,7 +103,7 @@ All substitutions are tentatively applied sequentially to the input.
         return github.GH(self.project_id[token_field], self.name)
 
     def _auto_init(self):
-        res = super(Repository, self)._auto_init()
+        res = super()._auto_init()
         tools.create_unique_index(
             self._cr, 'runbot_merge_unique_repo', self._table, ['name'])
         return res
@@ -294,7 +295,7 @@ class Branch(models.Model):
     staging_enabled = fields.Boolean(default=True)
 
     def _auto_init(self):
-        res = super(Branch, self)._auto_init()
+        res = super()._auto_init()
         tools.create_unique_index(
             self._cr, 'runbot_merge_unique_branch_per_repo',
             self._table, ['name', 'project_id'])
@@ -1359,36 +1360,44 @@ For your own safety I've ignored *everything in your entire comment*.
             ])
         return batch or batch.create({})
 
-    @api.model
-    def create(self, vals):
-        batch = self._get_batch(target=vals['target'], label=vals['label'])
-        vals['batch_id'] = batch.id
-        if 'limit_id' not in vals:
-            limits = {p.limit_id for p in batch.prs}
-            if len(limits) == 1:
-                vals['limit_id'] = limits.pop().id
-            elif limits:
-                repo = self.env['runbot_merge.repository'].browse(vals['repository'])
-                _logger.warning(
-                    "Unable to set limit on %s#%s: found multiple limits in batch (%s)",
-                    repo.name, vals['number'],
-                    ', '.join(
-                        f'{p.display_name} => {p.limit_id.name}'
-                        for p in batch.prs
+    @api.model_create_multi
+    def create(self, vals_list):
+        batches = {}
+        for vals in vals_list:
+            batch_key = vals['target'], vals['label']
+            batch = batches.get(batch_key)
+            if batch is None:
+                batch = batches[batch_key] = self._get_batch(target=vals['target'], label=vals['label'])
+            vals['batch_id'] = batch.id
+
+            if 'limit_id' not in vals:
+                limits = {p.limit_id for p in batch.prs}
+                if len(limits) == 1:
+                    vals['limit_id'] = limits.pop().id
+                elif limits:
+                    repo = self.env['runbot_merge.repository'].browse(vals['repository'])
+                    _logger.warning(
+                        "Unable to set limit on %s#%s: found multiple limits in batch (%s)",
+                        repo.name, vals['number'],
+                        ', '.join(
+                            f'{p.display_name} => {p.limit_id.name}'
+                            for p in batch.prs
+                        )
                     )
+
+        prs = super().create(vals_list)
+
+        for pr in prs:
+            c = self.env['runbot_merge.commit'].search([('sha', '=', pr.head)])
+            pr._validate(c.statuses)
+
+            if pr.state not in ('closed', 'merged'):
+                self.env.ref('runbot_merge.pr.created')._send(
+                    repository=pr.repository,
+                    pull_request=pr.number,
+                    format_args={'pr': pr},
                 )
-
-        pr = super().create(vals)
-        c = self.env['runbot_merge.commit'].search([('sha', '=', pr.head)])
-        pr._validate(c.statuses)
-
-        if pr.state not in ('closed', 'merged'):
-            self.env.ref('runbot_merge.pr.created')._send(
-                repository=pr.repository,
-                pull_request=pr.number,
-                format_args={'pr': pr},
-            )
-        return pr
+        return prs
 
     def _from_gh(self, description, author=None, branch=None, repo=None):
         if repo is None:
@@ -1457,11 +1466,14 @@ For your own safety I've ignored *everything in your entire comment*.
 
         newhead = vals.get('head')
         if newhead:
-            if pid := self.env.cr.precommit.data.get('change-author'):
-                writer = self.env['res.partner'].browse(pid)
-            else:
-                writer = self.env.user.partner_id
-            self.unstage("updated by %s", writer.github_login or writer.name)
+            authors = self.env.cr.precommit.data.get(f'mail.tracking.author.{self._name}', {})
+            for p in self:
+                if pid := authors.get(p.id):
+                    writer = self.env['res.partner'].browse(pid)
+                else:
+                    writer = self.env.user.partner_id
+                p.unstage("updated by %s", writer.github_login or writer.name)
+            # this can be batched
             c = self.env['runbot_merge.commit'].search([('sha', '=', newhead)])
             self._validate(c.statuses)
         return w
@@ -1754,18 +1766,20 @@ class Tagging(models.Model):
     tags_remove = fields.Char(required=True, default='[]')
     tags_add = fields.Char(required=True, default='[]')
 
-    def create(self, values):
-        if values.pop('state_from', None):
-            values['tags_remove'] = ALL_TAGS
-        if 'state_to' in values:
-            values['tags_add'] = _TAGS[values.pop('state_to')]
-        if not isinstance(values.get('tags_remove', ''), str):
-            values['tags_remove'] = json.dumps(list(values['tags_remove']))
-        if not isinstance(values.get('tags_add', ''), str):
-            values['tags_add'] = json.dumps(list(values['tags_add']))
-        if values:
+    @api.model_create_multi
+    def create(self, vals_list):
+        for values in vals_list:
+            if values.pop('state_from', None):
+                values['tags_remove'] = ALL_TAGS
+            if 'state_to' in values:
+                values['tags_add'] = _TAGS[values.pop('state_to')]
+            if not isinstance(values.get('tags_remove', ''), str):
+                values['tags_remove'] = json.dumps(list(values['tags_remove']))
+            if not isinstance(values.get('tags_add', ''), str):
+                values['tags_add'] = json.dumps(list(values['tags_add']))
+        if any(vals_list):
             self.env.ref('runbot_merge.labels_cron')._trigger()
-        return super().create(values)
+        return super().create(vals_list)
 
     def _send(self):
         # noinspection SqlResolve
@@ -1969,16 +1983,17 @@ class Commit(models.Model):
     commit_ids = fields.Many2many('runbot_merge.stagings', relation='runbot_merge_stagings_commits', column2='staging_id', column1='commit_id')
     pull_requests = fields.One2many('runbot_merge.pull_requests', compute='_compute_prs')
 
-    @api.model_create_single
-    def create(self, values):
-        values['to_check'] = True
-        r = super(Commit, self).create(values)
+    @api.model_create_multi
+    def create(self, vals_list):
+        for values in vals_list:
+            values['to_check'] = True
+        r = super().create(vals_list)
         self.env.ref("runbot_merge.process_updated_commits")._trigger()
         return r
 
     def write(self, values):
         values.setdefault('to_check', True)
-        r = super(Commit, self).write(values)
+        r = super().write(values)
         if values['to_check']:
             self.env.ref("runbot_merge.process_updated_commits")._trigger()
         return r
@@ -2023,7 +2038,7 @@ class Commit(models.Model):
     ]
 
     def _auto_init(self):
-        res = super(Commit, self)._auto_init()
+        res = super()._auto_init()
         self._cr.execute("""
             CREATE INDEX IF NOT EXISTS runbot_merge_unique_statuses
             ON runbot_merge_commit