[IMP] forwardport: automatically approve freeze backfills

if the corresponding master PR was approved. That the backfill needs
approval can be considered a race condition: its dual is approved
and *might* have been merged before the freeze, but was not, so a
backfill was needed.

Copying over the approval is a convenience feature with pretty much no
actual risk I can think of.

Fixes #884

forwardport/models/forwardport.py

@@ -108,20 +108,32 @@ class ForwardPortTasks(models.Model, Queue):
         )
         # insert new batch in ancestry sequence
         if self.source == 'insert':
-            self.env['runbot_merge.batch'].search([
-                ('parent_id', '=', batch.id),
-                ('id', '!=', newbatch.id),
-            ]).parent_id = newbatch.id
-            # insert new PRs in ancestry sequence unless conflict (= no parent)
-            for pr in newbatch.prs:
-                if not pr.parent_id:
-                    break
-                newchild = pr.search([
-                    ('parent_id', '=', pr.parent_id.id),
-                    ('id', '!=', pr.id),
-                ])
-                if newchild:
-                    newchild.parent_id = pr.id
+            self._process_insert(batch, newbatch)
+
+    def _process_insert(self, batch, newbatch):
+        self.env['runbot_merge.batch'].search([
+            ('parent_id', '=', batch.id),
+            ('id', '!=', newbatch.id),
+        ]).parent_id = newbatch.id
+        # insert new PRs in ancestry sequence unless conflict (= no parent)
+        for pr in newbatch.prs:
+            next_target = pr._find_next_target()
+            if not next_target:
+                continue
+
+            # should have one since it was inserted before an other PR?
+            descendant = pr.search([
+                ('target', '=', next_target.id),
+                ('source_id', '=', pr.source_id.id),
+            ])
+
+            # copy the reviewing of the "descendant" (even if detached) to this pr
+            if reviewer := descendant.reviewed_by:
+                pr.reviewed_by = reviewer
+
+            # replace parent_id *if not detached*
+            if descendant.parent_id:
+                descendant.parent_id = pr.id
 
     def _complete_batches(self):
         source = pr = self.pr_id

forwardport/tests/test_weird.py

@@ -750,9 +750,14 @@ def test_freeze(env, config, make_repo, users):
     """Freeze:
 
     - should not forward-port the freeze PRs themselves
+    - unmerged forward ports need to be backfilled
+    - if the tip of the forward port is approved, the backfilled forward port
+      should also be
     """
-    prod, _ = make_basic(env, config, make_repo)
+    prod, _ = make_basic(env, config, make_repo, statuses='default')
     project = env['runbot_merge.project'].search([])
+
+
     # branches here are "a" (older), "b", and "c" (master)
     with prod:
         [root, _] = prod.make_commits(
@@ -762,6 +767,22 @@ def test_freeze(env, config, make_repo, users):
             ref='heads/b'
         )
         prod.make_commits(root, Commit('other', tree={'f': '1'}), ref='heads/c')
+
+    # region PR which is forward ported but the FPs are not merged (they are approved)
+    with prod:
+        prod.make_commits("a", Commit("stuff", tree={'x': '0'}), ref="heads/abranch")
+        p = prod.make_pr(target='a', head='abranch')
+        p.post_comment("hansen r+ fw=skipci", config['role_reviewer']['token'])
+        prod.post_status('abranch', 'success')
+    env.run_crons()
+    with prod:
+        prod.post_status('staging.a', 'success')
+    env.run_crons()
+    pr_a_id, pr_b_id, pr_c_id = pr_ids = env['runbot_merge.pull_requests'].search([], order='number')
+    assert len(pr_ids) == 3, \
+        "should have created two forward ports, one in b and one in c (/ master)"
+    # endregion
+
     with prod:
         prod.make_commits(
             'c',
@@ -771,6 +792,15 @@ def test_freeze(env, config, make_repo, users):
         release = prod.make_pr(target='c', head='release-1.1')
     env.run_crons()
 
+    # approve pr_c_id but don't actually merge it before freezing
+    with prod:
+        prod.post_status(pr_b_id.head, 'success')
+        prod.post_status(pr_c_id.head, 'success')
+        prod.get_pr(pr_c_id.number).post_comment('hansen r+', config['role_reviewer']['token'])
+    # review comment should be handled eagerly
+    assert pr_b_id.reviewed_by
+    assert pr_c_id.reviewed_by
+
     w = project.action_prepare_freeze()
     assert w['res_model'] == 'runbot_merge.project.freeze'
     w_id = env[w['res_model']].browse([w['res_id']])
@@ -781,6 +811,8 @@ def test_freeze(env, config, make_repo, users):
     assert not w_id.errors
     w_id.action_freeze()
 
+    assert project.branch_ids.mapped('name') == ['c', 'post-b', 'b', 'a']
+
     # re-enable forward-port cron after freeze
     _, cron_id = env['ir.model.data'].check_object_reference('forwardport', 'port_forward', context={'active_test': False})
     env['ir.cron'].browse([cron_id]).active = True
@@ -792,9 +824,35 @@ def test_freeze(env, config, make_repo, users):
 
     assert release_id.state == 'merged'
     assert not env['runbot_merge.pull_requests'].search([
-        ('state', '!=', 'merged')
+        ('source_id', '=', release_id.id),
     ]), "the release PRs should not be forward-ported"
 
+    assert env['runbot_merge.stagings'].search_count([]) == 2,\
+        "b and c forward ports should be staged since they were ready before freeze"
+
+    # an intermediate PR should have been created
+    pr_inserted = env['runbot_merge.pull_requests'].search([
+        ('source_id', '=', pr_a_id.id),
+        ('target.name', '=', 'post-b'),
+    ])
+    assert pr_inserted, "an intermediate PR should have been reinsered in the sequence"
+    assert pr_c_id.parent_id == pr_inserted
+    assert pr_inserted.parent_id == pr_b_id
+
+    assert pr_inserted.reviewed_by == pr_c_id.reviewed_by,\
+        "review state should have been copied over from c (master)"
+    with prod:
+        prod.post_status(pr_inserted.head, 'success')
+        prod.post_status('staging.b', 'success')
+        prod.post_status('staging.c', 'success')
+    env.run_crons()
+    with prod:
+        prod.post_status('staging.post-b', 'success')
+    env.run_crons()
+
+    assert env['runbot_merge.pull_requests'].search_count([('state', '=', 'merged')]) \
+        == len(['release', 'initial', 'fw-b', 'fw-post-b', 'fw-c'])
+
 def test_missing_magic_ref(env, config, make_repo):
     """There are cases where github fails to create / publish or fails to update
     the magic refs in refs/pull/*.