[IMP] *: add closed PR to the branch deleter

Also reduce the grace period for merged PR branches to 1 week (from
2), and go through the on-disk repository instead of the github API.

Technically it might even be possible to do this un bulk, though that
seems annoying in case of failure (e.g. because somebody else deleted
the branch previously).

Fixes #1082

forwardport/models/forwardport.py

@@ -15,10 +15,9 @@ from dateutil import relativedelta
 
 from odoo import api, fields, models
 from odoo.addons.runbot_merge import git
-from odoo.addons.runbot_merge.github import GH
 
 # how long a merged PR survives
-MERGE_AGE = relativedelta.relativedelta(weeks=2)
+MERGE_AGE = relativedelta.relativedelta(weeks=1)
 FOOTER = '\nMore info at https://github.com/odoo/odoo/wiki/Mergebot#forward-port\n'
 
 _logger = logging.getLogger(__name__)
@@ -379,7 +378,7 @@ class DeleteBranches(models.Model, Queue):
     _name = 'forwardport.branch_remover'
     _description = "Removes branches of merged PRs"
 
-    pr_id = fields.Many2one('runbot_merge.pull_requests')
+    pr_id = fields.Many2one('runbot_merge.pull_requests', index=True)
 
     @api.model_create_multi
     def create(self, vals_list):
@@ -389,7 +388,11 @@ class DeleteBranches(models.Model, Queue):
     def _search_domain(self):
         cutoff = getattr(builtins, 'forwardport_merged_before', None) \
              or fields.Datetime.to_string(datetime.now() - MERGE_AGE)
-        return [('pr_id.merge_date', '<', cutoff)]
+        return [
+            '|', ('pr_id.merge_date', '<', cutoff),
+                 '&', ('pr_id.closed', '=', True),
+                      ('pr_id.write_date', '<', cutoff),
+        ]
 
     def _process_item(self):
         _deleter.info(
@@ -398,8 +401,8 @@ class DeleteBranches(models.Model, Queue):
             self.pr_id.label
         )
 
-        if self.pr_id.state != 'merged':
-            _deleter.info('✘ PR is not "merged" (got %s)', self.pr_id.state)
+        if self.pr_id.state not in ('merged', 'closed'):
+            _deleter.info('✘ PR is active (%s)', self.pr_id.state)
             return
 
         repository = self.pr_id.repository
@@ -414,34 +417,17 @@ class DeleteBranches(models.Model, Queue):
             _deleter.info('✘ PR owner != FP target owner (%s)', repo_owner)
             return # probably don't have access to arbitrary repos
 
-        github = GH(token=repository.project_id.fp_github_token, repo=fp_remote)
-        refurl = 'git/refs/heads/' + branch
-        ref = github('get', refurl, check=False)
-        if ref.status_code != 200:
-            _deleter.info("✘ branch already deleted (%s)", ref.json())
-            return
-
-        ref = ref.json()
-        if isinstance(ref, list):
+        r = git.get_local(repository).check(False).push(
+            git.fw_url(repository),
+            '--delete', branch,
+            f'--force-with-lease={branch}:{self.pr_id.head}',
+        )
+        if r.returncode:
             _deleter.info(
-                "✘ got a fuzzy match (%s), branch probably deleted",
-                ', '.join(r['ref'] for r in ref)
-            )
-            return
-
-        if ref['object']['sha'] != self.pr_id.head:
-            _deleter.info(
-                "✘ branch %s head mismatch, expected %s, got %s",
+                '✘ failed to delete branch %s of PR %s:\n%s',
                 self.pr_id.label,
-                self.pr_id.head,
-                ref['object']['sha']
+                self.pr_id.display_name,
+                r.stderr.decode(),
             )
-            return
-
-        r = github('delete', refurl, check=False)
-        assert r.status_code == 204, \
-            "Tried to delete branch %s of %s, got %s" % (
-                branch, self.pr_id.display_name,
-                r.json()
-            )
-        _deleter.info('✔ deleted branch %s of PR %s', self.pr_id.label, self.pr_id.display_name)
+        else:
+            _deleter.info('✔ deleted branch %s of PR %s', self.pr_id.label, self.pr_id.display_name)

forwardport/tests/test_simple.py

@@ -1105,7 +1105,8 @@ class TestBranchDeletion:
 
         # check that the branches still exist
         assert other.get_ref('heads/abranch') == pr_heads[0]
-        assert other.get_ref('heads/bbranch') == pr_heads[1]
+        with pytest.raises(AssertionError, match="Not Found"):
+            other.get_ref('heads/bbranch')
         assert other.get_ref('heads/cbranch') == pr_heads[2]
         assert other.get_ref('heads/dbranch') == pr_heads[3]
 

runbot_merge/models/pull_requests.py

@@ -1499,14 +1499,20 @@ For your own safety I've ignored *everything in your entire comment*.
                 if merge_method not in (False, 'rebase-ff') and pr.message != vals['message']:
                     pr.unstage("merge message updated")
 
+        # FIXME: remove condition once mergebot and forwardbot modules are merged
+        remover = self.env.get('forwardport.branch_remover')
         match vals.get('closed'):
             case True if not self.closed:
                 vals['reviewed_by'] = False
+                if remover is not None:
+                    remover.create([{'pr_id': self.id}])
             case False if self.closed and not self.batch_id:
                 vals['batch_id'] = self._get_batch(
                     target=vals.get('target') or self.target.id,
                     label=vals.get('label') or self.label,
                 )
+                if remover is not None:
+                    remover.search([('pr_id', '=', self.id)]).unlink()
         w = super().write(vals)
 
         newhead = vals.get('head')