[FIX] runbot_merge: sync PR target and message on check

Previously the mergebot would only sync the head commit, but synching
more is useful.

Also update the final sanity check on staging:

- as with check, update the message & target branch
- reset PR state and post a message when updating message instead of
  doing so silently

Note: maybe only fail the staging if the message is updated *and*
relevant to staging (aka there's a merge method and it's not
`rebase`)?

Fixes #680

forwardport/models/project_freeze.py

@@ -15,8 +15,12 @@ class FreezeWizard(models.Model):
         self.env.ref('forwardport.port_forward').active = False
         return r
 
+    def action_freeze(self):
+        return super(FreezeWizard, self.with_context(forwardport_keep_disabled=True))\
+            .action_freeze()
+
     def unlink(self):
         r = super().unlink()
-        if not self.search_count([]):
+        if not (self.env.context.get('forwardport_keep_disabled') or self.search_count([])):
             self.env.ref('forwardport.port_forward').active = True
         return r

forwardport/tests/test_weird.py

@@ -761,7 +761,10 @@ def test_retarget_after_freeze(env, config, make_repo, users):
     assert job
 
     # fuck up yo life: retarget the existing FP PR to the new branch
-    port_id.target = new_branch.id
+    port_pr = prod.get_pr(port_id.number)
+    with prod:
+        port_pr.base = 'bprime'
+    assert port_id.target == new_branch
 
     env.run_crons('forwardport.port_forward')
     assert not job.exists(), "job should have succeeded and apoptosed"
@@ -771,7 +774,6 @@ def test_retarget_after_freeze(env, config, make_repo, users):
     assert env['runbot_merge.pull_requests'].search([('state', 'not in', ('merged', 'closed'))]) == port_id
 
     # merge the retargered PR
-    port_pr = prod.get_pr(port_id.number)
     with prod:
         prod.post_status(port_pr.head, 'success', 'ci/runbot')
         prod.post_status(port_pr.head, 'success', 'legal/cla')

runbot_merge/controllers/__init__.py

@@ -137,11 +137,12 @@ def handle_pr(env, event):
         if message != pr_obj.message:
             updates['message'] = message
 
-        _logger.info("update: %s#%d = %s (by %s)", repo.name, pr['number'], updates, event['sender']['login'])
+        _logger.info("update: %s = %s (by %s)", pr_obj.display_name, updates, event['sender']['login'])
         if updates:
-            pr_obj.write(updates)
+            # copy because it updates the `updates` dict internally
-            return 'Updated {}'.format(pr_obj.id)
+            pr_obj.write(dict(updates))
-        return "Nothing to update ({})".format(event['changes'].keys())
+            return 'Updated {}'.format(', '.join(updates))
+        return "Nothing to update ({})".format(', '.join(event['changes'].keys()))
 
     message = None
     if not branch:
@@ -209,7 +210,7 @@ def handle_pr(env, event):
             'head': pr['head']['sha'],
             'squash': pr['commits'] == 1,
         })
-        return 'Updated {} to {}'.format(pr_obj.display_name, pr_obj.head)
+        return f'Updated to {pr_obj.head}'
 
     if event['action'] == 'ready_for_review':
         pr_obj.draft = False

runbot_merge/models/pull_requests.py

@@ -121,22 +121,31 @@ All substitutions are tentatively applied sequentially to the input.
             })
             return
 
-        # if the PR is already loaded, check... if the heads match?
+        # if the PR is already loaded, force sync a few attributes
         pr_id = self.env['runbot_merge.pull_requests'].search([
             ('repository.name', '=', pr['base']['repo']['full_name']),
             ('number', '=', number),
         ])
         if pr_id:
-            # TODO: edited, maybe (requires crafting a 'changes' object)
+            sync = controllers.handle_pr(self.env, {
-            r = controllers.handle_pr(self.env, {
                 'action': 'synchronize',
                 'pull_request': pr,
                 'sender': {'login': self.project_id.github_prefix}
             })
+            edit = controllers.handle_pr(self.env, {
+                'action': 'edited',
+                'pull_request': pr,
+                'changes': {
+                    'base': {'ref': {'from': pr_id.target.name}},
+                    'title': {'from': pr_id.message.splitlines()[0]},
+                    'body': {'from', ''.join(pr_id.message.splitlines(keepends=True)[2:])},
+                },
+                'sender': {'login': self.project_id.github_prefix},
+            })
             feedback({
                 'repository': pr_id.repository.id,
                 'pull_request': number,
-                'message': r,
+                'message': f"{edit}. {sync}.",
             })
             return
 
@@ -1154,7 +1163,7 @@ class PullRequests(models.Model):
                         old_target.display_name, pr.target.display_name,
                     )
                 old_message = prev[pr.id]['message']
-                if pr.merge_method in ('merge', 'rebase-merge') and pr.message != old_message:
+                if pr.merge_method not in (False, 'rebase-ff') and pr.message != old_message:
                     pr.unstage("merge message updated")
         return w
 
@@ -1301,9 +1310,32 @@ class PullRequests(models.Model):
                     f"missing email on {c['sha']} indicates the authorship is "
                     f"most likely incorrect."
                 )
+
+        # sync and signal possibly missed updates
+        invalid = {}
         pr_head = pr_commits[-1]['sha']
-        if pr_head != self.head:
+        if self.head != pr_head:
-            raise exceptions.Mismatch(self.head, pr_head, commits == 1)
+            invalid['head'] = pr_head
+        if self.squash != commits == 1:
+            invalid['squash'] = commits == 1
+
+        msg = f'{prdict["title"]}\n\n{prdict.get("body") or ""}'.strip()
+        if self.message != msg:
+            invalid['message'] = msg
+
+        if self.target.name != prdict['base']['ref']:
+            branch = self.env['runbot_merge.branch'].with_context(active_test=False).search([
+                ('name', '=', prdict['base']['ref']),
+                ('project_id', '=', self.repository.project_id.id),
+            ])
+            if not branch:
+                self.unlink()
+                raise exceptions.Unmergeable(self, "While staging, found this PR had been retargeted to an un-managed branch.")
+            invalid['target'] = branch.id
+
+        if invalid:
+            self.write({**invalid, 'state': 'opened'})
+            raise exceptions.Mismatch(invalid)
 
         if self.reviewed_by and self.reviewed_by.name == self.reviewed_by.github_login:
             # XXX: find other trigger(s) to sync github name?
@@ -1311,11 +1343,6 @@ class PullRequests(models.Model):
             if gh_name:
                 self.reviewed_by.name = gh_name
 
-        # update pr message in case an update was missed
-        msg = f'{prdict["title"]}\n\n{prdict.get("body") or ""}'.strip()
-        if self.message != msg:
-            self.message = msg
-
         # NOTE: lost merge v merge/copy distinction (head being
         #       a merge commit reused instead of being re-merged)
         return method, getattr(self, '_stage_' + method.replace('-', '_'))(
@@ -2123,6 +2150,7 @@ class Batch(models.Model):
         :return: () or Batch object (if all prs successfully staged)
         """
         new_heads = {}
+        pr_fields = self.env['runbot_merge.pull_requests']._fields
         for pr in prs:
             gh = meta[pr.repository]['gh']
 
@@ -2154,24 +2182,27 @@ class Batch(models.Model):
             except github.MergeError:
                 raise exceptions.MergeError(pr)
             except exceptions.Mismatch as e:
-                old_head, new_head, to_squash = e.args
-                pr.write({
-                    'state': 'opened',
-                    'squash': to_squash,
-                    'head': new_head,
-                })
                 _logger.warning(
-                    "head mismatch on %s: had %s but found %s",
+                    "data mismatch on %s: %s",
-                    pr.display_name, old_head, new_head
+                    pr.display_name, e.args[0]
                 )
                 self.env['runbot_merge.pull_requests.feedback'].create({
                     'repository': pr.repository.id,
                     'pull_request': pr.number,
-                    'message': "%swe apparently missed an update to this PR "
+                    'message': """\
-                               "and tried to stage it in a state which "
+%swe apparently missed updates to this PR and tried to stage it in a state 
-                               "might not have been approved. PR has been "
+which might not have been approved.
-                               "updated to %s, please check and approve or "
+
-                               "re-approve." % (pr.ping(), new_head)
+The properties %s were not correctly synchronized and have been updated.
+
+Note that we are unable to check the properties %s.
+
+Please check and re-approve.
+""" % (
+                        pr.ping(),
+                        ', '.join(pr_fields[f].string for f in e.args[0]),
+                        ', '.join(pr_fields[f].string for f in UNCHECKABLE),
+                    )
                 })
                 return self.env['runbot_merge.batch']
 
@@ -2183,6 +2214,8 @@ class Batch(models.Model):
             'prs': [(4, pr.id, 0) for pr in prs],
         })
 
+UNCHECKABLE = ['merge_method', 'overrides', 'draft']
+
 class FetchJob(models.Model):
     _name = _description = 'runbot_merge.fetch_job'
 

runbot_merge/tests/test_basic.py

@@ -2389,26 +2389,23 @@ class TestPRUpdate(object):
         assert pr.head == c2
         assert pr.state == 'validated'
 
-    def test_update_missed(self, env, repo, config):
+    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
         now being unreviewed or failing CI or somesuch.
 
-        This is not a super frequent occurrence, and possibly not the most
+        Therefore during the staging process we should check what we can, reject
-        problematic issue ever (e.g. if the branch doesn't CI it's not going to
+        the staging if cricical properties were found to mismatch, and notify
-        pass staging, though we might still be staging a branch which had been
+        the pull request.
-        unreviewed).
 
-        So during the staging process, the heads should be checked, and the PR
+        The PR should then be reset to open (and transition to validated on its
-        will not be staged if the heads don't match (though it'll be reset to
+        own if the existing or new head has valid statuses), we don't want to
-        open, rather than put in an error state as technically there's no
+        put it in an error state as technically there's no error, just something
-        failure, we just want to notify users that something went odd with the
+        which went a bit weird.
-        mergebot).
-
-        TODO: other cases / situations where we want to update the head?
         """
         with repo:
-            repo.make_commits(None, repo.Commit('m', tree={'a': '0'}), ref='heads/master')
+            [c] = repo.make_commits(None, repo.Commit('m', tree={'a': '0'}), ref='heads/master')
+            repo.make_ref('heads/somethingelse', c)
 
             [c] = repo.make_commits(
                 'heads/master', repo.Commit('c', tree={'a': '1'}), ref='heads/abranch')
@@ -2433,11 +2430,18 @@ class TestPRUpdate(object):
             repo.post_status(c2, 'success', 'ci/runbot')
             repo.update_ref(pr.ref, c2, force=True)
 
+        other = env['runbot_merge.branch'].create({
+            'name': 'somethingelse',
+            'project_id': env['runbot_merge.project'].search([]).id,
+        })
+
         # we missed the update notification so the db should still be at c and
         # in a "ready" state
         pr_id.write({
             'head': c,
             'state': 'ready',
+            'message': "Something else",
+            'target': other.id,
         })
 
         env.run_crons()
@@ -2445,13 +2449,33 @@ class TestPRUpdate(object):
         # the PR should not get merged, and should be updated
         assert pr_id.state == 'validated'
         assert pr_id.head == c2
+        assert pr_id.message == 'c'
+        assert pr_id.target.name == 'master'
+        assert pr.comments[-1] == (users['user'], """\
+@{} @{} we apparently missed updates to this PR and tried to stage it in a state 
+which might not have been approved.
 
-        pr_id.write({'head': c, 'state': 'ready'})
+The properties Head, Message, Target were not correctly synchronized and have been updated.
+
+Note that we are unable to check the properties Merge Method, Overrides, Draft.
+
+Please check and re-approve.
+""".format(users['user'], users['reviewer']))
+
+        pr_id.write({
+            'head': c,
+            'state': 'ready',
+            'message': "Something else",
+            'target': other.id,
+        })
         with repo:
             pr.post_comment('hansen check')
         env.run_crons()
         assert pr_id.state == 'validated'
         assert pr_id.head == c2
+        assert pr_id.message == 'c' # the commit's message was used for the PR
+        assert pr_id.target.name == 'master'
+        assert pr.comments[-1] == (users['user'], f"Updated target, squash, message. Updated to {c2}.")
 
     def test_update_closed(self, env, repo):
         with repo:

runbot_merge/tests/test_oddities.py

@@ -63,40 +63,6 @@ def test_name_search(env):
     assert PRs.name_search('repo') == [pr2, pr0, pr1]
     assert PRs.name_search('repo#1959') == [pr1]
 
-def test_message_desync(env, project, make_repo, users, setreviewers, config):
-    """If the PR message gets desync'd (github misses sending an update), the
-    merge message should still match what's on github rather than what's in the
-    db
-    """
-    repo = make_repo('repo')
-    env['runbot_merge.repository'].create({
-        'project_id': project.id,
-        'name': repo.name,
-        'status_ids': [(0, 0, {'context': 'status'})]
-    })
-    setreviewers(*project.repo_ids)
-
-    with repo:
-        [m] = repo.make_commits(None, Commit('root', tree={'a': '1'}), ref='heads/master')
-
-        [c] = repo.make_commits('master', Commit('whee', tree={'b': '2'}))
-        pr = repo.make_pr(title='title', body='body', target='master', head=c)
-        repo.post_status(c, 'success', 'status')
-    env.run_crons()
-
-    pr_id = to_pr(env, pr)
-    assert pr_id.message == 'title\n\nbody'
-    pr_id.message = "xxx"
-
-    with repo:
-        pr.post_comment('hansen merge r+', config['role_reviewer']['token'])
-    env.run_crons()
-
-    st = repo.commit('staging.master')
-    assert st.message.startswith('title\n\nbody'),\
-        "the stored PR message should have been ignored when staging"
-    assert st.parents == [m, c], "check the staging's ancestry is the right one"
-
 def test_unreviewer(env, project, port):
     repo = env['runbot_merge.repository'].create({
         'project_id': project.id,