[FIX] runbot_merge: PR splits should be updated on PR state change

On a PR being updated, closed or unreviewed, if it is part of an
active staging that staging would get cancelled (yay). However, if the
PR was part of a pending *split*, then the split would *not* get
cancelled / updated (to remove the PR from it), and the PR could go on
to get staged as if everything were right in the world which is an
issue.

It doesn't look like it actually happened (at least I got no echo of
it), but it almost did at least once.

fixes #160

runbot_merge/controllers/__init__.py

@@ -153,7 +153,7 @@ def handle_pr(env, event):
             return "It's my understanding that closed/merged PRs don't get sync'd"
 
         if pr_obj.state == 'ready':
-            pr_obj.staging_id.cancel(
+            pr_obj.unstage(
                 "PR %s:%s updated by %s",
                 pr_obj.repository.name, pr_obj.number,
                 event['sender']['login']
@@ -201,7 +201,7 @@ def handle_pr(env, event):
                 'state_from': res[1] if not pr_obj.staging_id else 'staged',
                 'state_to': 'closed',
             })
-            pr_obj.staging_id.cancel(
+            pr_obj.unstage(
                 "PR %s:%s closed by %s",
                 pr_obj.repository.name, pr_obj.number,
                 event['sender']['login']

runbot_merge/models/pull_requests.py

@@ -698,11 +698,7 @@ class PullRequests(models.Model):
                     newstate = RMINUS.get(self.state)
                     if newstate:
                         self.state = newstate
-                        if self.staging_id:
-                            self.staging_id.cancel(
-                                "unreview (r-) by %s",
-                                author.github_login
-                            )
+                        self.unstage("unreview (r-) by %s", author.github_login)
                         ok = True
                     else:
                         msg = "r- makes no sense in the current PR state."
@@ -1040,6 +1036,23 @@ class PullRequests(models.Model):
             self.commits_map = json.dumps(commits_map)
             return merge_head
 
+    def unstage(self, reason, *args):
+        """ If the PR is staged, cancel the staging. If the PR is split and
+        waiting, remove it from the split (possibly delete the split entirely)
+        """
+        split_batches = self.with_context(active_test=False).mapped('batch_ids').filtered('split_id')
+        if len(split_batches) > 1:
+            _logger.warn("Found a PR linked with more than one split batch: %s (%s)", self, split_batches)
+        for b in split_batches:
+            if len(b.split_id.batch_ids) == 1:
+                # only the batch of this PR -> delete split
+                b.split_id.unlink()
+            else:
+                # else remove this batch from the split
+                b.split_id = False
+
+        self.staging_id.cancel(reason, *args)
+
 # state changes on reviews
 RPLUS = {
     'opened': 'approved',

runbot_merge/tests/remote.py

@@ -348,8 +348,8 @@ class Model:
         ids = self._env(self._model, 'exists', self._ids)
         return Model(self._env, self._model, ids)
 
-    def search(self, domain):
-        ids = self._env(self._model, 'search', domain)
+    def search(self, domain, **kw):
+        ids = self._env(self._model, 'search', domain, **kw)
         return Model(self._env, self._model, ids)
 
     def create(self, values):

runbot_merge/tests/test_basic.py

@@ -1633,6 +1633,49 @@ class TestPRUpdate(object):
         assert not pr.staging_id
         assert not env['runbot_merge.stagings'].search([])
 
+    def test_split(self, env, repo):
+        """ Should remove the PR from its split, and possibly delete the split
+        entirely.
+        """
+        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'})
+        prx1 = repo.make_pr('t1', 'b1', target='master', ctid=c, user='user', label='p1')
+        repo.post_status(prx1.head, 'success', 'legal/cla')
+        repo.post_status(prx1.head, 'success', 'ci/runbot')
+        prx1.post_comment('hansen r+', user='reviewer')
+
+        c = repo.make_commit(m, 'first', None, tree={'m': 'm', '2': '2'})
+        prx2 = repo.make_pr('t2', 'b2', target='master', ctid=c, user='user', label='p2')
+        repo.post_status(prx2.head, 'success', 'legal/cla')
+        repo.post_status(prx2.head, 'success', 'ci/runbot')
+        prx2.post_comment('hansen r+', user='reviewer')
+
+        run_crons(env)
+
+        pr1, pr2 = env['runbot_merge.pull_requests'].search([], order='number')
+        assert pr1.number == prx1.number
+        assert pr2.number == prx2.number
+        assert pr1.staging_id == pr2.staging_id
+        s0 = pr1.staging_id
+
+        repo.post_status('heads/staging.master', 'failure', 'ci/runbot')
+        run_crons(env)
+
+        assert pr1.staging_id and pr1.staging_id != s0, "pr1 should have been re-staged"
+        assert not pr2.staging_id, "pr2 should not"
+        # TODO: remote doesn't currently handle env context so can't mess
+        #       around using active_test=False
+        assert env['runbot_merge.split'].search([])
+
+        prx2.push(repo.make_commit(c, 'second', None, tree={'m': 'm', '2': '22'}))
+        # probably not necessary ATM but...
+        run_crons(env)
+
+        assert pr2.state == 'opened', "state should have been reset"
+        assert not env['runbot_merge.split'].search([]), "there should be no split left"
+
     def test_update_error(self, env, repo):
         m = repo.make_commit(None, 'initial', None, tree={'m': 'm'})
         repo.make_ref('heads/master', m)
@@ -2404,6 +2447,49 @@ class TestRMinus:
         assert pr.staging_id == st
         assert pr.state == 'ready'
 
+    def test_split(self, env, repo):
+        """ Should remove the PR from its split, and possibly delete the split
+        entirely.
+        """
+        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'})
+        prx1 = repo.make_pr('t1', 'b1', target='master', ctid=c, user='user', label='p1')
+        repo.post_status(prx1.head, 'success', 'legal/cla')
+        repo.post_status(prx1.head, 'success', 'ci/runbot')
+        prx1.post_comment('hansen r+', user='reviewer')
+
+        c = repo.make_commit(m, 'first', None, tree={'m': 'm', '2': '2'})
+        prx2 = repo.make_pr('t2', 'b2', target='master', ctid=c, user='user', label='p2')
+        repo.post_status(prx2.head, 'success', 'legal/cla')
+        repo.post_status(prx2.head, 'success', 'ci/runbot')
+        prx2.post_comment('hansen r+', user='reviewer')
+
+        run_crons(env)
+
+        pr1, pr2 = env['runbot_merge.pull_requests'].search([], order='number')
+        assert pr1.number == prx1.number
+        assert pr2.number == prx2.number
+        assert pr1.staging_id == pr2.staging_id
+        s0 = pr1.staging_id
+
+        repo.post_status('heads/staging.master', 'failure', 'ci/runbot')
+        run_crons(env)
+
+        assert pr1.staging_id and pr1.staging_id != s0, "pr1 should have been re-staged"
+        assert not pr2.staging_id, "pr2 should not"
+        # TODO: remote doesn't currently handle env context so can't mess
+        #       around using active_test=False
+        assert env['runbot_merge.split'].search([])
+
+        # prx2 was actually a terrible idea!
+        prx2.post_comment('hansen r-', user='reviewer')
+        # probably not necessary ATM but...
+        run_crons(env)
+
+        assert pr2.state == 'validated', "state should have been reset"
+        assert not env['runbot_merge.split'].search([]), "there should be no split left"
 
 class TestComments:
     def test_address_method(self, repo, env):