[FIX] runbot_merge: a few issues with updated staging check

1cea247e6c tried to improve staging
checks to avoid staging PRs in the wrong state, however it had two
issues:

PR state
--------

The process would reset the PR's state to open, but unless the head
was being resync'd it wouldn't re-apply the statuses on the state,
leading to a PR with all-valid statuses, but a missing CI.

Message
-------

The message check didn't compose the PR message the same way PR
creation / update did (it did not trim the title and description
individually, only after concatenation), resulting in a
not-actually-existing divergence getting signaled in the case where
the PR title ends or the description starts with whitespace.

Expand relevant test, add a utility function to compose a PR message
and use it everywhere for coherence.

Also update the logging and reporting to show a diff of all the
updated items (hidden behind a `details` element).

conftest.py

@@ -834,6 +834,8 @@ class Comment(tuple):
         self._c = c
         return self
     def __getitem__(self, item):
+        if isinstance(item, int):
+            return super().__getitem__(item)
         return self._c[item]
 
 

runbot_merge/controllers/__init__.py

@@ -130,10 +130,7 @@ def handle_pr(env, event):
         # turns out github doesn't bother sending a change key if the body is
         # changing from empty (None), therefore ignore that entirely, just
         # generate the message and check if it changed
-        message = pr['title'].strip()
+        message = utils.make_message(pr)
-        body = (pr['body'] or '').strip()
-        if body:
-            message += f"\n\n{body}"
         if message != pr_obj.message:
             updates['message'] = message
 

runbot_merge/models/pull_requests.py

@@ -14,6 +14,7 @@ import pprint
 import re
 import time
 
+from difflib import Differ
 from itertools import takewhile
 
 import requests
@@ -1120,10 +1121,6 @@ class PullRequests(models.Model):
                 ('github_login', '=', description['user']['login']),
             ], limit=1)
 
-        message = description['title'].strip()
-        body = description['body'] and description['body'].strip()
-        if body:
-            message += '\n\n' + body
         return self.env['runbot_merge.pull_requests'].create({
             'state': 'opened' if description['state'] == 'open' else 'closed',
             'number': description['number'],
@@ -1133,7 +1130,7 @@ class PullRequests(models.Model):
             'repository': repo.id,
             'head': description['head']['sha'],
             'squash': description['commits'] == 1,
-            'message': message,
+            'message': utils.make_message(description),
             'draft': description['draft'],
         })
 
@@ -1313,15 +1310,11 @@ class PullRequests(models.Model):
 
         # sync and signal possibly missed updates
         invalid = {}
+        diff = []
         pr_head = pr_commits[-1]['sha']
         if self.head != pr_head:
             invalid['head'] = pr_head
-        if self.squash != commits == 1:
+            diff.append(('Head', self.head, pr_head))
-            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([
@@ -1332,10 +1325,20 @@ class PullRequests(models.Model):
                 self.unlink()
                 raise exceptions.Unmergeable(self, "While staging, found this PR had been retargeted to an un-managed branch.")
             invalid['target'] = branch.id
+            diff.append(('Target branch', self.target.name, branch.name))
+
+        if self.squash != commits == 1:
+            invalid['squash'] = commits == 1
+            diff.append(('Single commit', self.squash, commits == 1))
+
+        msg = utils.make_message(prdict)
+        if self.message != msg:
+            invalid['message'] = msg
+            diff.append(('Message', self.message, msg))
 
         if invalid:
-            self.write({**invalid, 'state': 'opened'})
+            self.write({**invalid, 'state': 'opened', 'head': pr_head})
-            raise exceptions.Mismatch(invalid)
+            raise exceptions.Mismatch(invalid, diff)
 
         if self.reviewed_by and self.reviewed_by.name == self.reviewed_by.github_login:
             # XXX: find other trigger(s) to sync github name?
@@ -2182,27 +2185,49 @@ class Batch(models.Model):
             except github.MergeError:
                 raise exceptions.MergeError(pr)
             except exceptions.Mismatch as e:
+                def format_items(items):
+                    """ Bit of a pain in the ass because difflib really wants
+                    all lines to be newline-terminated, but not all values are
+                    actual lines, and also needs to split multiline values.
+                    """
+                    for name, value in items:
+                        yield name + ':\n'
+                        if not value.endswith('\n'):
+                            value += '\n'
+                        yield from value.splitlines(keepends=True)
+                        yield '\n'
+
+                old = list(format_items((n, v) for n, v, _ in e.args[1]))
+                new = list(format_items((n, v) for n, _, v in e.args[1]))
+                diff = ''.join(Differ().compare(old, new))
                 _logger.warning(
-                    "data mismatch on %s: %s",
+                    "data mismatch on %s:\n%s",
-                    pr.display_name, e.args[0]
+                    pr.display_name, diff
                 )
                 self.env['runbot_merge.pull_requests.feedback'].create({
                     'repository': pr.repository.id,
                     'pull_request': pr.number,
                     'message': """\
-%swe apparently missed updates to this PR and tried to stage it in a state 
+{ping}we apparently missed updates to this PR and tried to stage it in a state \
 which might not have been approved.
 
-The properties %s were not correctly synchronized and have been updated.
+The properties {mismatch} were not correctly synchronized and have been updated.
 
-Note that we are unable to check the properties %s.
+<details><summary>differences</summary>
+
+```diff
+{diff}```
+</details>
+
+Note that we are unable to check the properties {unchecked}.
 
 Please check and re-approve.
-""" % (
+""".format(
-                        pr.ping(),
+    ping=pr.ping(),
-                        ', '.join(pr_fields[f].string for f in e.args[0]),
+    mismatch=', '.join(pr_fields[f].string for f in e.args[0]),
-                        ', '.join(pr_fields[f].string for f in UNCHECKABLE),
+    diff=diff,
-                    )
+    unchecked=', '.join(pr_fields[f].string for f in UNCHECKABLE),
+)
                 })
                 return self.env['runbot_merge.batch']
 

runbot_merge/tests/test_basic.py

@@ -2408,7 +2408,7 @@ class TestPRUpdate(object):
             repo.make_ref('heads/somethingelse', c)
 
             [c] = repo.make_commits(
-                'heads/master', repo.Commit('c', tree={'a': '1'}), ref='heads/abranch')
+                'heads/master', repo.Commit('title \n\nbody', tree={'a': '1'}), ref='heads/abranch')
             pr = repo.make_pr(target='master', head='abranch')
             repo.post_status(pr.head, 'success', 'legal/cla')
             repo.post_status(pr.head, 'success', 'ci/runbot')
@@ -2418,6 +2418,7 @@ class TestPRUpdate(object):
             ('number', '=', pr.number),
         ])
         env.run_crons('runbot_merge.process_updated_commits')
+        assert pr_id.message == 'title\n\nbody'
         assert pr_id.state == 'ready'
 
         # TODO: find way to somehow skip / ignore the update_ref?
@@ -2449,18 +2450,70 @@ 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.message == 'title\n\nbody'
         assert pr_id.target.name == 'master'
-        assert pr.comments[-1] == (users['user'], """\
+        assert pr.comments[-1]['body'] == """\
-@{} @{} we apparently missed updates to this PR and tried to stage it in a state 
+@{} @{} we apparently missed updates to this PR and tried to stage it in a state \
 which might not have been approved.
 
-The properties Head, Message, Target were not correctly synchronized and have been updated.
+The properties Head, Target, Message were not correctly synchronized and have been updated.
+
+<details><summary>differences</summary>
+
+```diff
+  Head:
+- {}
++ {}
+  
+  Target branch:
+- somethingelse
++ master
+  
+  Message:
+- Something else
++ title
+  
++ body
++ 
+```
+</details>
 
 Note that we are unable to check the properties Merge Method, Overrides, Draft.
 
 Please check and re-approve.
-""".format(users['user'], users['reviewer']))
+""".format(users['user'], users['reviewer'], c, c2)
+
+        # if the head commit doesn't change, that part should still be valid
+        with repo:
+            pr.post_comment('hansen r+', config['role_reviewer']['token'])
+        assert pr_id.state == 'ready'
+        pr_id.write({'message': 'wrong'})
+        env.run_crons()
+
+        assert pr_id.message == 'title\n\nbody'
+        assert pr_id.state == 'validated'
+        assert pr.comments[-1]['body'] == """\
+@{} @{} we apparently missed updates to this PR and tried to stage it in a state \
+which might not have been approved.
+
+The properties Message were not correctly synchronized and have been updated.
+
+<details><summary>differences</summary>
+
+```diff
+  Message:
+- wrong
++ title
+  
++ body
++ 
+```
+</details>
+
+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,
@@ -2473,7 +2526,7 @@ Please check and re-approve.
         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.message == 'title\n\nbody' # 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}.")
 

runbot_merge/utils.py

@@ -30,3 +30,8 @@ def backoff(func=None, *, delays=BACKOFF_DELAYS, exc=Exception):
             if delay is None:
                 raise
             time.sleep(delay)
+
+def make_message(pr_dict):
+    title = pr_dict['title'].strip()
+    body = (pr_dict.get('body') or '').strip()
+    return f'{title}\n\n{body}' if body else title