diff --git a/forwardport/models/project.py b/forwardport/models/project.py index 3801b8b9..fe2d8ce9 100644 --- a/forwardport/models/project.py +++ b/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 diff --git a/forwardport/tests/test_simple.py b/forwardport/tests/test_simple.py index 61ed69e0..0c06b819 100644 --- a/forwardport/tests/test_simple.py +++ b/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 diff --git a/runbot_merge/models/pull_requests.py b/runbot_merge/models/pull_requests.py index d4407f18..00a4ea1a 100644 --- a/runbot_merge/models/pull_requests.py +++ b/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)