From 3f4519d60549c88c303ba80a5e3a328e96103e0f Mon Sep 17 00:00:00 2001 From: Xavier Morel Date: Thu, 30 May 2024 10:55:40 +0200 Subject: [PATCH] [CHG] runbot_merge: add signoff & related to all commits if rebased. Untouched commits (straight merge) remain unalterated, but all rebased or squashed commits now get signoff and `Related` headers added on top of the already previously added `part-of`. Implement by generalising `_build_merge_message` to `_build_message` and having `add_self_references` delegate to it, removes some of the redundancy / differential handling. Update the `part_of` helper to also add the S-O-B header to the PR, although it currently does not reference the entire forward port chain. Fixes #876 --- forwardport/models/project.py | 22 ----------------- mergebot_test_utils/utils.py | 5 +++- runbot_merge/models/pull_requests.py | 25 ++++++++++++++++--- runbot_merge/models/stagings_create.py | 34 +++++++++++++------------- runbot_merge/tests/test_basic.py | 3 ++- 5 files changed, 44 insertions(+), 45 deletions(-) diff --git a/forwardport/models/project.py b/forwardport/models/project.py index 43379eb1..a4099d05 100644 --- a/forwardport/models/project.py +++ b/forwardport/models/project.py @@ -519,28 +519,6 @@ stderr: prev = configured.stdout().rev_parse('HEAD').stdout.decode() logger.info('%s: success -> %s', commit_sha, prev) - 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.root_id | 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 = Message.from_message(commit['commit']['message']) diff --git a/mergebot_test_utils/utils.py b/mergebot_test_utils/utils.py index c69b3a3c..cba96114 100644 --- a/mergebot_test_utils/utils.py +++ b/mergebot_test_utils/utils.py @@ -182,7 +182,10 @@ def to_pr(env, pr): def part_of(label, pr_id, *, separator='\n\n'): """ Adds the "part-of" pseudo-header in the footer. """ - return f'{label}{separator}Part-of: {pr_id.display_name}' + return f"""\ +{label}{separator}\ +Part-of: {pr_id.display_name} +Signed-off-by: {pr_id.reviewed_by.formatted_email}""" def ensure_one(records): assert len(records) == 1 diff --git a/runbot_merge/models/pull_requests.py b/runbot_merge/models/pull_requests.py index 90da29f6..1a174aef 100644 --- a/runbot_merge/models/pull_requests.py +++ b/runbot_merge/models/pull_requests.py @@ -1339,19 +1339,36 @@ class PullRequests(models.Model): if commit: self.env.cr.commit() - def _build_merge_message(self, message: Union['PullRequests', str], related_prs=()) -> 'Message': + def _build_message(self, message: Union['PullRequests', str], related_prs: 'PullRequests' = (), merge: bool = True) -> 'Message': # handle co-authored commits (https://help.github.com/articles/creating-a-commit-with-multiple-authors/) m = Message.from_message(message) if not is_mentioned(message, self): - m.body += f'\n\ncloses {self.display_name}' + if merge: + m.body += f'\n\ncloses {self.display_name}' + else: + m.headers.pop('Part-Of', None) + m.headers.add('Part-Of', self.display_name) for r in related_prs: if not is_mentioned(message, r, full_reference=True): m.headers.add('Related', r.display_name) - if self.reviewed_by: - m.headers.add('signed-off-by', self.reviewed_by.formatted_email) + # 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.root_id | self.source_id)\ + .mapped('reviewed_by.formatted_email') + sobs = m.headers.getlist('signed-off-by') + m.headers.remove('signed-off-by') + m.headers.extend( + ('signed-off-by', signer) + for signer in sobs + if signer not in reviewers + ) + m.headers.extend( + ('signed-off-by', reviewer) + for reviewer in reversed(reviewers) + ) return m def unstage(self, reason, *args): diff --git a/runbot_merge/models/stagings_create.py b/runbot_merge/models/stagings_create.py index 1f620154..ec3d99b3 100644 --- a/runbot_merge/models/stagings_create.py +++ b/runbot_merge/models/stagings_create.py @@ -454,7 +454,7 @@ def stage(pr: PullRequests, info: StagingSlice, related_prs: PullRequests) -> Tu return method, new_head def stage_squash(pr: PullRequests, info: StagingSlice, commits: List[github.PrCommit], related_prs: PullRequests) -> str: - msg = pr._build_merge_message(pr, related_prs=related_prs) + msg = pr._build_message(pr, related_prs=related_prs) authors = { (c['commit']['author']['name'], c['commit']['author']['email']) @@ -499,11 +499,7 @@ def stage_squash(pr: PullRequests, info: StagingSlice, commits: List[github.PrCo return head def stage_rebase_ff(pr: PullRequests, info: StagingSlice, commits: List[github.PrCommit], related_prs: PullRequests) -> str: - # updates head commit with PR number (if necessary) then rebases - # on top of target - msg = pr._build_merge_message(commits[-1]['commit']['message'], related_prs=related_prs) - commits[-1]['commit']['message'] = str(msg) - add_self_references(pr, commits[:-1]) + add_self_references(pr, commits, related_prs=related_prs, merge=commits[-1]) _logger.debug("rebasing %s on %s (commits=%s)", pr.display_name, info.head, len(commits)) @@ -512,11 +508,11 @@ def stage_rebase_ff(pr: PullRequests, info: StagingSlice, commits: List[github.P return head def stage_rebase_merge(pr: PullRequests, info: StagingSlice, commits: List[github.PrCommit], related_prs: PullRequests) -> str : - add_self_references(pr, commits) + add_self_references(pr, commits, related_prs=related_prs) _logger.debug("rebasing %s on %s (commits=%s)", pr.display_name, info.head, len(commits)) h, mapping = info.repo.rebase(info.head, commits=commits) - msg = pr._build_merge_message(pr, related_prs=related_prs) + msg = pr._build_message(pr, related_prs=related_prs) project = pr.repository.project_id merge_head= info.repo.merge( @@ -554,7 +550,7 @@ def stage_merge(pr: PullRequests, info: StagingSlice, commits: List[github.PrCom raise exceptions.MergeError(pr, t.stderr) merge_tree = t.stdout.strip() new_parents = [info.head] + list(head_parents - {base_commit}) - msg = pr._build_merge_message(pr_head['commit']['message'], related_prs=related_prs) + msg = pr._build_message(pr_head['commit']['message'], related_prs=related_prs) d2t = itemgetter('name', 'email', 'date') c = info.repo.commit_tree( @@ -574,7 +570,7 @@ def stage_merge(pr: PullRequests, info: StagingSlice, commits: List[github.PrCom return copy else: # otherwise do a regular merge - msg = pr._build_merge_message(pr) + msg = pr._build_message(pr) project = pr.repository.project_id merge_head = info.repo.merge( info.head, pr.head, str(msg), @@ -595,17 +591,21 @@ def is_mentioned(message: Union[PullRequests, str], pr: PullRequests, *, full_re pattern = fr'( |\b{repository})#{pr.number}\b' return bool(re.search(pattern, message if isinstance(message, str) else message.message)) -def add_self_references(pr: PullRequests, commits: List[github.PrCommit]): +def add_self_references( + pr: PullRequests, + commits: List[github.PrCommit], + related_prs: PullRequests, + merge: Optional[github.PrCommit] = None, +): """Adds a footer reference to ``self`` to all ``commits`` if they don't already refer to the PR. """ for c in (c['commit'] for c in commits): - if not is_mentioned(c['message'], pr): - message = c['message'] - m = Message.from_message(message) - m.headers.pop('Part-Of', None) - m.headers.add('Part-Of', pr.display_name) - c['message'] = str(m) + c['message'] = str(pr._build_message( + c['message'], + related_prs=related_prs, + merge=merge and c['url'] == merge['commit']['url'], + )) BREAK = re.compile(r''' [ ]{0,3} # 0-3 spaces of indentation diff --git a/runbot_merge/tests/test_basic.py b/runbot_merge/tests/test_basic.py index 3b616cb8..afa7a3a9 100644 --- a/runbot_merge/tests/test_basic.py +++ b/runbot_merge/tests/test_basic.py @@ -1965,7 +1965,8 @@ Some: thing is odd -Part-of: {pr_id.display_name}""" +Part-of: {pr_id.display_name} +Signed-off-by: {reviewer}""" def test_pr_mergehead(self, repo, env, config): """ if the head of the PR is a merge commit and one of the parents is