[IMP] runbot_merge, forwardport: better signed-off-by handling

Remove original-signed-off-by, doesn't actually seem useful given the
semantics of signed-off-by according to the kernel doc'. Plus it
didn't actually work as the intent was to keep the signoff of the
original PR in the forward-port, but that signoff is not part of the
commit we're cherrypicking (it gets added on the fly when the commit
is merged).

Therefore explicitly get the ack-chain into the PR: when merging an FP
PR, try to integrate the signoff of the original PR, that of the final
FP pr, and while at it that of the last explicit update in the commit
chain (e.g. in case there's been a conflict or something).

Fixes #284

forwardport/models/project.py

@@ -906,18 +906,31 @@ stderr:
             new = configured.stdout().rev_parse('HEAD').stdout.decode()
             logger.info('%s: success -> %s', commit_sha, new)
 
+    def _build_merge_message(self, message, related_prs=()):
+        msg = super()._build_merge_message(message, related_prs=related_prs)
+
+        # ensures all reviewers in the review path are on the PR in order:
+        # original reviewer, then last conflict reviewer, then current PR
+        reviewers = (self | self._get_root() | self.source_id)\
+            .mapped('reviewed_by.formatted_email')
+
+        sobs = msg.headers.getlist('signed-off-by')
+        msg.headers.remove('signed-off-by')
+        msg.headers.extend(
+            ('signed-off-by', signer)
+            for signer in sobs
+            if signer not in reviewers
+        )
+        msg.headers.extend(
+            ('signed-off-by', reviewer)
+            for reviewer in reversed(reviewers)
+        )
+
+        return msg
+
     def _make_fp_message(self, commit):
         cmap = json.loads(self.commits_map)
         msg = self._parse_commit_message(commit['commit']['message'])
-        # original signed-off-er should be retained but didn't necessarily
-        # sign off here, so convert signed-off-by to something else
-        sob = msg.headers.getlist('signed-off-by')
-        if sob:
-            msg.headers.remove('signed-off-by')
-            msg.headers.extend(
-                ('original-signed-off-by', v)
-                for v in sob
-            )
         # write the *merged* commit as "original", not the PR's
         msg.headers['x-original-commit'] = cmap.get(commit['sha'], commit['sha'])
         # don't stringify so caller can still perform alterations

forwardport/tests/test_simple.py

@@ -869,9 +869,23 @@ def test_access_rights(env, config, make_repo, users, author, reviewer, delegate
     if success:
         assert pr1.staging_id and pr2.staging_id,\
             "%s should have approved FP of PRs by %s" % (reviewer, author)
+        st = prod.commit('staging.b')
+        c = prod.commit(st.parents[0])
+        # Should be signed-off by both original reviewer and forward port reviewer
+        original_signoff = signoff(config['role_user'], c.message)
+        forward_signoff = signoff(config['role_' + reviewer], c.message)
+        assert c.message.index(original_signoff) <= c.message.index(forward_signoff),\
+            "Check that FP approver is after original PR approver as that's " \
+            "the review path for the PR"
     else:
         assert not (pr1.staging_id or pr2.staging_id),\
             "%s should *not* have approved FP of PRs by %s" % (reviewer, author)
+def signoff(conf, message):
+    for n in filter(None, [conf.get('name'), conf.get('user')]):
+        signoff = 'Signed-off-by: ' + n
+        if signoff in message:
+            return signoff
+    raise AssertionError("Failed to find signoff by %s in %s" % (conf, message))
 
 def test_batched(env, config, make_repo, users):
     """ Tests for projects with multiple repos & sync'd branches. Batches

runbot_merge/models/pull_requests.py

@@ -1160,7 +1160,7 @@ class PullRequests(models.Model):
         if self.reviewed_by:
             m.headers.add('signed-off-by', self.reviewed_by.formatted_email)
 
-        return str(m)
+        return m
 
     def _stage(self, gh, target, related_prs=()):
         # nb: pr_commits is oldest to newest so pr.head is pr_commits[-1]
@@ -1190,7 +1190,7 @@ class PullRequests(models.Model):
         # updates head commit with PR number (if necessary) then rebases
         # on top of target
         msg = self._build_merge_message(commits[-1]['commit']['message'], related_prs=related_prs)
-        commits[-1]['commit']['message'] = msg
+        commits[-1]['commit']['message'] = str(msg)
         head, mapping = gh.rebase(self.number, target, commits=commits)
         self.commits_map = json.dumps({**mapping, '': head})
         return head
@@ -1198,7 +1198,7 @@ class PullRequests(models.Model):
     def _stage_rebase_merge(self, gh, target, commits, related_prs=()):
         msg = self._build_merge_message(self.message, related_prs=related_prs)
         h, mapping = gh.rebase(self.number, target, reset=True, commits=commits)
-        merge_head = gh.merge(h, target, msg)['sha']
+        merge_head = gh.merge(h, target, str(msg))['sha']
         self.commits_map = json.dumps({**mapping, '': merge_head})
         return merge_head
 
@@ -1224,7 +1224,7 @@ class PullRequests(models.Model):
             new_parents = [original_head] + list(head_parents - {base_commit})
             msg = self._build_merge_message(pr_head['commit']['message'], related_prs=related_prs)
             copy = gh('post', 'git/commits', json={
-                'message': msg,
+                'message': str(msg),
                 'tree': merge_tree,
                 'author': pr_head['commit']['author'],
                 'committer': pr_head['commit']['committer'],
@@ -1238,7 +1238,7 @@ class PullRequests(models.Model):
         else:
             # otherwise do a regular merge
             msg = self._build_merge_message(self.message)
-            merge_head = gh.merge(self.head, target, msg)['sha']
+            merge_head = gh.merge(self.head, target, str(msg))['sha']
             # and the merge commit is the normal merge head
             commits_map[''] = merge_head
             self.commits_map = json.dumps(commits_map)