[ADD] *: fw=skipmerge

Skipmerge creates forward-ports before the source PR is even merged.

- In a break from the norm, skipmerge will create forwardports even in
  the face of conflicts.
- It will also not *detach* pull requests in case of conflicts, this
  is so the source PR can be updated and the update correctly cascades
  through the stack (likewise for any intermediate PR though *that*
  will detach as usual).

Note that this doesn't really look at outstandings, especially as they
were recently updated, so it might need to be fixed up in case of
freakout, but I feel like that should not be too much of an issue, the
authors will just get their FW reminders earlier than usual. If that's
a hassle we can always update the reminder job to ignore forward ports
whose source is not merged I guess.

Fixes #418

forwardport/tests/test_limit.py

@@ -229,6 +229,7 @@ Currently available commands:
 |`fw=no`|does not forward-port this PR|
 |`fw=default`|forward-ports this PR normally|
 |`fw=skipci`|does not wait for a forward-port's statuses to succeed before creating the next one|
+|`fw=skipmerge`|does not wait for the source to be merged before creating forward ports|
 |`up to <branch>`|only ports this PR forward to the specified branch (included)|
 |`merge`|integrate the PR with a simple merge commit, using the PR description as message|
 |`rebase-merge`|rebases the PR on top of the target branch the integrates with a merge commit, using the PR description as message|

forwardport/tests/test_skipmerge.py

@@ -0,0 +1,316 @@
+import re
+from datetime import datetime, timedelta
+
+from utils import make_basic, Commit, to_pr, REF_PATTERN, seen, matches
+
+
+def test_base(env, config, make_repo, users, page):
+    prod, other = make_basic(env, config, make_repo, statuses='default')
+
+    b_head = prod.commit('b')
+    c_head = prod.commit('c')
+    with prod:
+        # create PR as a user with no access to prod (or other)
+        prod.make_commits(
+            'a',
+            Commit('p_0', tree={'x': '1'}),
+            ref='heads/hugechange'
+        )
+        pr0 = prod.make_pr(target='a', title="super important change", head='hugechange')
+        pr0.post_comment('hansen fw=skipmerge', config['role_reviewer']['token'])
+    pr0_id = to_pr(env, pr0)
+    assert not pr0_id.merge_date, \
+        "PR obviously shouldn't have a merge date before being merged"
+    assert pr0_id.batch_id.fw_policy == 'skipmerge'
+
+    env.run_crons()
+
+    pr0_id, pr1_id, pr2_id = env['runbot_merge.pull_requests'].search([], order='number')
+    pr2_id.reminder_next = datetime.now() - timedelta(days=1)
+    env.run_crons('forwardport.reminder')
+
+    assert pr0_id.number == pr0.number
+    assert pr0.comments == [
+        (users['reviewer'], 'hansen fw=skipmerge'),
+        seen(env, pr0, users),
+        (users['user'], "Starting forward-port. Not waiting for merge to create followup forward-ports."),
+    ]
+
+    assert pr1_id.parent_id == pr0_id
+    assert pr1_id.source_id == pr0_id
+    other_owner = other.name.split('/')[0]
+    assert re.match(other_owner + ':' + REF_PATTERN.format(target='b', source='hugechange'), pr1_id.label), \
+        "check that FP PR was created in FP target repo"
+    assert pr1_id.ping == f"@{users['user']} ", "not reviewed yet so ping should only include author"
+    assert prod.read_tree(prod.commit(pr1_id.head)) == {
+        'f': 'c',
+        'g': 'b',
+        'x': '1'
+    }
+    pr1 = prod.get_pr(pr1_id.number)
+    assert pr1.comments == [
+        seen(env, pr1, users),
+        (users['user'], """\
+This PR targets b and is part of the forward-port chain. Further PRs will be created up to c.
+
+More info at https://github.com/odoo/odoo/wiki/Mergebot#forward-port
+""")]
+
+    assert pr2_id.parent_id == pr1_id
+    assert pr2_id.source_id == pr0_id
+    assert re.match(REF_PATTERN.format(target='c', source='hugechange'), pr2_id.refname), \
+        "check that FP PR was created in FP target repo"
+    assert prod.read_tree(prod.commit(pr2_id.head)) == {
+        'f': 'c',
+        'g': 'a',
+        'h': 'a',
+        'x': '1'
+    }
+    pr2 = prod.get_pr(pr2_id.number)
+    assert pr2.comments == [
+        seen(env, pr2, users),
+        (users['user'], """\
+@%s this PR targets c and is the last of the forward-port chain containing:
+* %s
+
+To merge the full chain, use
+> @hansen r+
+
+More info at https://github.com/odoo/odoo/wiki/Mergebot#forward-port
+""" % (
+            users['user'],
+            pr1_id.display_name,
+    )),
+        (users['user'], "@%s this forward port of %s is awaiting action (not merged or closed)." % (
+            users['user'],
+            pr0_id.display_name,
+        ))
+    ]
+
+    with prod:
+        prod.post_status(pr0_id.head, 'success')
+        prod.post_status(pr1_id.head, 'success')
+        prod.post_status(pr2_id.head, 'success')
+
+        pr2.post_comment('hansen r+', config['role_reviewer']['token'])
+
+    env.run_crons()
+
+    assert pr0_id.staging_id
+    assert pr1_id.staging_id
+    assert pr2_id.staging_id
+    # three branches so should have three stagings
+    assert len(pr0_id.staging_id | pr1_id.staging_id | pr2_id.staging_id) == 3
+    # validate
+    with prod:
+        prod.post_status('staging.a', 'success')
+        prod.post_status('staging.b', 'success')
+        prod.post_status('staging.c', 'success')
+
+    # and trigger merge
+    env.run_crons()
+    assert all(p.state == 'merged' for p in [pr0_id, pr1_id, pr2_id])
+
+    head_a = prod.commit('a')
+    assert head_a.message == f"""\
+p_0
+
+closes {pr0_id.display_name}
+
+Signed-off-by: {pr0_id.reviewed_by.formatted_email}\
+"""
+
+    old_b = prod.read_tree(b_head)
+    head_b = prod.commit('b')
+    assert head_b.message == f"""\
+p_0
+
+closes {pr1_id.display_name}
+
+X-original-commit: {pr0_id.head}
+Signed-off-by: {pr1_id.reviewed_by.formatted_email}\
+""", "since the previous PR is not merged we don't know what its final commit is (?)"
+    b_tree = prod.read_tree(head_b)
+    assert b_tree == {**old_b, 'x': '1'}
+
+    old_c = prod.read_tree(c_head)
+    head_c = prod.commit('c')
+    assert head_c.message == f"""\
+p_0
+
+closes {pr2_id.display_name}
+
+X-original-commit: {pr0_id.head}
+Signed-off-by: {pr2_id.reviewed_by.formatted_email}\
+"""
+    c_tree = prod.read_tree(head_c)
+    assert c_tree == {**old_c, 'x': '1'}
+
+    # check that we didn't just smash the original trees
+    assert prod.read_tree(prod.commit('a')) != b_tree != c_tree
+
+    assert pr2_id.parent_id == pr1_id
+    assert pr1_id.parent_id == pr0_id
+
+def test_conflict_recovery_source(env, config, make_repo, users, page):
+    """ Source recovery is when a forward port is in conflict and we update
+    *the source* in such a way that the fp doesn't conflict anymore. This should
+    resume forward-porting.
+    """
+    prod, _ = make_basic(env, config, make_repo, statuses='default')
+
+    with prod:
+        prod.make_commits("b", Commit("xxx", tree={'x': '1'}), ref='heads/b')
+        prod.make_commits("c", Commit("xxx", tree={'x': '2'}), ref='heads/c')
+        prod.make_commits(
+            'a',
+            Commit('p_0', tree={'x': '0'}),
+            ref='heads/hugechange'
+        )
+        pr0 = prod.make_pr(target='a', title="super important change", head='hugechange')
+        pr0.post_comment('hansen fw=skipmerge', config['role_reviewer']['token'])
+    env.run_crons()
+
+    pr0_id, pr1_id, pr2_id = env['runbot_merge.pull_requests'].search([], order='number')
+    assert prod.read_tree(prod.commit(pr1_id.head)) == {
+        'f': 'c',
+        'g': 'b',
+        'x': matches('''\
+<<<\x3c<<< b
+1
+||||||| $$
+=======
+0
+>>>\x3e>>> $$
+''')
+    }
+    assert prod.read_tree(prod.commit(pr2_id.head)) == {
+        'f': 'c',
+        'g': 'a',
+        'h': 'a',
+        'x': matches('''\
+<<<\x3c<<< c
+2
+||||||| $$
+=======
+0
+>>>\x3e>>> $$
+''')
+    }
+    assert pr1_id.parent_id and pr2_id.parent_id
+
+    pr1 = prod.get_pr(pr1_id.number)
+    assert "CONFLICT (add/add)" in pr1.comments[1][1]
+
+    # Oh no, conflict! But turns out we can implement the fix differently <smort>
+    with prod:
+        prod.make_commits("a", Commit("workaround", tree={"y": "1"}), ref="heads/hugechange", make=False)
+    assert prod.read_tree(prod.commit(pr0_id.head)) == {
+        'f': 'e',
+        'y': '1',
+    }
+
+    env.run_crons()
+
+    assert env['runbot_merge.pull_requests'].search_count([]) == 3,\
+        "check that we have not created separate new versions of the prs"
+    assert prod.read_tree(prod.commit(pr1_id.head)) == {
+        'f': 'c',
+        'g': 'b',
+        'x': '1',
+        'y': '1',
+    }
+    assert prod.read_tree(prod.commit(pr2_id.head)) == {
+        'f': 'c',
+        'g': 'a',
+        'h': 'a',
+        'x': '2',
+        'y': '1',
+    }
+
+def test_conflict_recovery_manual(env, config, make_repo, users, page):
+    """ Manual recover is when a forward port is in conflict and we update
+    *that forward port*. This should resume forward porting when skipmerge.
+    """
+    prod, _ = make_basic(env, config, make_repo, statuses='default')
+
+    with prod:
+        prod.make_commits("b", Commit("xxx", tree={'x': '1'}), ref='heads/b')
+        prod.make_commits("c", Commit("xxx", tree={'x': '1'}), ref='heads/c')
+        prod.make_commits(
+            'a',
+            Commit('p_0', tree={'x': '0'}),
+            ref='heads/hugechange'
+        )
+        pr0 = prod.make_pr(target='a', title="super important change", head='hugechange')
+        pr0.post_comment('hansen fw=skipmerge', config['role_reviewer']['token'])
+    env.run_crons()
+
+    _pr0_id, pr1_id, pr2_id = env['runbot_merge.pull_requests'].search([], order='number')
+    assert prod.read_tree(prod.commit(pr1_id.head)) == {
+        'f': 'c',
+        'g': 'b',
+        'x': matches('''\
+<<<\x3c<<< b
+1
+||||||| $$
+=======
+0
+>>>\x3e>>> $$
+''')
+    }
+    assert prod.read_tree(prod.commit(pr2_id.head)) == {
+        'f': 'c',
+        'g': 'a',
+        'h': 'a',
+        'x': matches('''\
+<<<\x3c<<< c
+1
+||||||| $$
+=======
+0
+>>>\x3e>>> $$
+''')
+    }
+    assert pr1_id.parent_id and pr2_id.parent_id, "conflicts on skipmerge should not detach"
+
+    pr1 = prod.get_pr(pr1_id.number)
+    assert "CONFLICT (add/add)" in pr1.comments[1][1]
+    assert "CONFLICT (add/add)" in prod.get_pr(pr2_id.number).comments[1][1]
+
+
+    prb_repo, prb_ref = pr1.branch
+    with prb_repo:
+        prb_repo.make_commits(
+            prod.commit("b").id,
+            Commit("thing", tree={'x': 'yyy'}),
+            ref=f'heads/{prb_ref}',
+            make=False,
+        )
+    assert not pr1_id.parent_id, "manual update should still de-parent even on skipmerge"
+    pr1_head = pr1_id.head
+    assert prod.read_tree(prod.commit(pr1_head)) == {
+        'f': 'c',
+        'g': 'b',
+        'x': 'yyy',
+    }
+    env.run_crons()
+    assert env['runbot_merge.pull_requests'].search_count([]) == 3,\
+        "check that we have not created a separate new version of pr3"
+
+    assert prod.read_tree(prod.commit(pr2_id.head)) == {
+        'f': 'c',
+        'g': 'a',
+        'h': 'a',
+        'x': 'yyy',
+    }
+
+
+    with prod:
+        prod.make_commits(
+            'a',
+            Commit('p_0', tree={'x': '42'}),
+            ref='heads/hugechange'
+        )
+    env.run_crons()
+    assert pr1.head == pr1_head, "since PR1 is detached the update to pr0 should not propagate"

runbot_merge/changelog/2025-02/merge-method-skipmerge.md

@@ -0,0 +1 @@
+IMP: allow creating forward ports *before* merging the source pull request

runbot_merge/models/batch.py

@@ -70,6 +70,7 @@ class Batch(models.Model):
         ('no', "Do not port forward"),
         ('default', "Default"),
         ('skipci', "Skip CI"),
+        ('skipmerge', "Skip merge"),
     ], required=True, default="default", string="Forward Port Policy", tracking=True)
 
     merge_date = fields.Datetime(tracking=True)
@@ -316,6 +317,8 @@ class Batch(models.Model):
 
         refname = self.genealogy_ids[0].name.split(':', 1)[-1]
         new_branch = f'{target.name}-{refname}-{self.id}-fw'
+        _logger.info("Forward-porting %s to %s (using branch %r)", self, target.name, new_branch)
+
         conflicts = {}
         for pr in prs:
             repo = git.get_local(pr.repository)
@@ -365,18 +368,20 @@ class Batch(models.Model):
             _logger.info("Created forward-port PR %s", new_pr)
             new_batch |= new_pr
 
+            report_conflicts = has_conflicts and self.fw_policy != 'skipmerge'
+
             # allows PR author to close or skipci
             new_pr.write({
                 'merge_method': pr.merge_method,
                 'source_id': source.id,
                 # only link to previous PR of sequence if cherrypick passed
-                'parent_id': pr.id if not has_conflicts else False,
+                'parent_id': pr.id if not report_conflicts else False,
                 'detach_reason': "conflicts:\n{}".format('\n\n'.join(
                     f"{out}\n{err}".strip()
                     for _, out, err, _ in filter(None, conflicts.values())
-                )) if has_conflicts else None,
+                )) if report_conflicts else None,
             })
-            if has_conflicts and pr.parent_id and pr.state not in ('merged', 'closed'):
+            if report_conflicts and pr.parent_id and pr.state not in ('merged', 'closed'):
                 self.env.ref('runbot_merge.forwardport.failure.conflict')._send(
                     repository=pr.repository,
                     pull_request=pr.number,
@@ -398,6 +403,7 @@ class Batch(models.Model):
 
         new_batch = new_batch.batch_id
         new_batch.parent_id = self
+        new_batch.fw_policy = self.fw_policy
         # try to schedule followup
         new_batch._schedule_fp_followup()
         return new_batch
@@ -407,6 +413,7 @@ class Batch(models.Model):
         # if the PR has a parent and is CI-validated, enqueue the next PR
         scheduled = self.browse(())
         for batch in self:
+            force_fw = force_fw or batch.fw_policy == 'skipmerge'
             prs = ', '.join(batch.prs.mapped('display_name'))
             _logger.info('Checking if forward-port %s (%s)', batch, prs)
             # in cas of conflict or update individual PRs will "lose" their
@@ -417,10 +424,10 @@ class Batch(models.Model):
             # same thing as all the PRs having a source, kinda, but cheaper,
             # it's not entirely true as technically the user could have added a
             # PR to the forward ported batch
-            if not (batch.parent_id and force_fw or all(p.parent_id for p in batch.prs)):
+            if not (batch.parent_id and (force_fw or all(p.parent_id for p in batch.prs))):
                 _logger.info('-> no parent %s (%s)', batch, prs)
                 continue
-            if not force_fw and batch.source.fw_policy != 'skipci' \
+            if not force_fw and batch.source.fw_policy not in ('skipci', 'skipmerge') \
                     and (invalid := batch.prs.filtered(lambda p: p.status != 'success')):
                 _logger.info(
                     '-> wrong state %s (%s)',

runbot_merge/models/commands.py

@@ -192,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()}'
@@ -203,6 +203,7 @@ class FW(enum.Enum):
         yield str(cls.DEFAULT), "forward-ports this PR normally"
         if is_reviewer:
             yield str(cls.SKIPCI), "does not wait for a forward-port's statuses to succeed before creating the next one"
+            yield str(cls.SKIPMERGE), "does not wait for the source to be merged before creating forward ports"
 
 
 @dataclass

runbot_merge/models/pull_requests.py

@@ -935,23 +935,28 @@ For your own safety I've ignored *everything in your entire comment*.
                 case commands.FW():
                     message = None
                     match command:
+                        case _ if self.batch_id.fw_policy == 'skipmerge' and command != commands.FW.SKIPMERGE:
+                            msg = "a pull request set to skip merge can not be reverted to other fw methods"
                         case commands.FW.NO if is_author or source_author:
                             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:
                             message = "Not waiting for CI to create followup forward-ports."
-                        # case commands.FW.SKIPMERGE if is_reviewer:
-                        #     message = "Not waiting for merge to create followup forward-ports."
+                        case commands.FW.SKIPMERGE if is_reviewer:
+                            if self.source_id or self.merge_date:
+                                msg = "the source is already merged"
+                            else:
+                                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:
+                        elif not (self.merge_date or command == commands.FW.SKIPMERGE):
+                            "if the PR is not merged or bypassing merge, it'll be fw'd normally"
+                        elif command != commands.FW.SKIPMERGE and (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"

runbot_merge/tests/test_basic.py

@@ -3577,6 +3577,7 @@ Currently available commands:
 |`fw=no`|does not forward-port this PR|
 |`fw=default`|forward-ports this PR normally|
 |`fw=skipci`|does not wait for a forward-port's statuses to succeed before creating the next one|
+|`fw=skipmerge`|does not wait for the source to be merged before creating forward ports|
 |`up to <branch>`|only ports this PR forward to the specified branch (included)|
 |`merge`|integrate the PR with a simple merge commit, using the PR description as message|
 |`rebase-merge`|rebases the PR on top of the target branch the integrates with a merge commit, using the PR description as message|
@@ -3607,6 +3608,7 @@ Currently available commands:
 |`fw=no`|does not forward-port this PR|
 |`fw=default`|forward-ports this PR normally|
 |`fw=skipci`|does not wait for a forward-port's statuses to succeed before creating the next one|
+|`fw=skipmerge`|does not wait for the source to be merged before creating forward ports|
 |`up to <branch>`|only ports this PR forward to the specified branch (included)|
 |`merge`|integrate the PR with a simple merge commit, using the PR description as message|
 |`rebase-merge`|rebases the PR on top of the target branch the integrates with a merge commit, using the PR description as message|

runbot_merge/tests/test_oddities.py

@@ -332,6 +332,7 @@ Currently available commands for @{user}:
 |`fw=no`|does not forward-port this PR|
 |`fw=default`|forward-ports this PR normally|
 |`fw=skipci`|does not wait for a forward-port's statuses to succeed before creating the next one|
+|`fw=skipmerge`|does not wait for the source to be merged before creating forward ports|
 |`up to <branch>`|only ports this PR forward to the specified branch (included)|
 |`merge`|integrate the PR with a simple merge commit, using the PR description as message|
 |`rebase-merge`|rebases the PR on top of the target branch the integrates with a merge commit, using the PR description as message|