From dde59b9fe6239a7a718173e297ef726a6a054613 Mon Sep 17 00:00:00 2001 From: Xavier Morel Date: Mon, 2 Mar 2020 08:54:58 +0100 Subject: [PATCH] [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 | 31 ++++++++++++++++++++-------- forwardport/tests/test_simple.py | 14 +++++++++++++ runbot_merge/models/pull_requests.py | 10 ++++----- 3 files changed, 41 insertions(+), 14 deletions(-) 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)