From b21fbaf9cc18b4a1cbc18e12ca9703091a8ef521 Mon Sep 17 00:00:00 2001 From: Xavier Morel Date: Mon, 9 Oct 2023 13:52:30 +0200 Subject: [PATCH] [IMP] runbot_merge: prevent merging empty commits The low-level APIs used by the staging process don't do any merge check, so because of the way Git works it's possible for them to merge commits with content as empty commits, e.g. if something was merged then backported and the backport was merged on top. This should trigger a merge failure as we don't really want to merge newly empty. This is a feature which some high level commands of git support, kind-of, e.g. by default `git rebase --interactive` will ask about newly empty commits. Take care to allow merging already-empty commits, as these do have a use for signaling, freezes, .... Fixes #809 --- runbot_merge/git.py | 19 +++++ runbot_merge/github.py | 6 +- runbot_merge/models/stagings_create.py | 13 ++- runbot_merge/tests/test_oddities.py | 109 +++++++++++++++++++++++++ 4 files changed, 145 insertions(+), 2 deletions(-) diff --git a/runbot_merge/git.py b/runbot_merge/git.py index 09061933..f8bdb7f5 100644 --- a/runbot_merge/git.py +++ b/runbot_merge/git.py @@ -107,6 +107,11 @@ class Repo: ) return Repo(to) + def get_tree(self, commit_hash: str) -> str: + r = self.with_config(check=True).rev_parse(f'{commit_hash}^{{tree}}') + + return r.stdout.strip() + def rebase(self, dest: str, commits: Sequence[PrCommit]) -> Tuple[str, Dict[str, str]]: """Implements rebase by hand atop plumbing so: @@ -125,6 +130,9 @@ class Repo: if not commits: raise MergeError("PR has no commits") + prev_tree = repo.get_tree(dest) + prev_original_tree = repo.get_tree(commits[0]['parents'][0]["sha"]) + new_trees = [] parent = dest for original in commits: @@ -135,11 +143,22 @@ class Repo: "rebasing") new_trees.append(check(repo.merge_tree(parent, original['sha'])).stdout.strip()) + # allow merging empty commits, but not empty*ing* commits while merging + if prev_original_tree != original['commit']['tree']['sha']: + if new_trees[-1] == prev_tree: + raise MergeError( + f"commit {original['sha']} results in an empty tree when " + f"merged, it is likely a duplicate of a merged commit, " + f"rebase and remove." + ) + parent = check(repo.commit_tree( tree=new_trees[-1], parents=[parent, original['sha']], message=f'temp rebase {original["sha"]}', )).stdout.strip() + prev_tree = new_trees[-1] + prev_original_tree = original['commit']['tree']['sha'] mapping = {} for original, tree in zip(commits, new_trees): diff --git a/runbot_merge/github.py b/runbot_merge/github.py index 1f31e03c..2abcc495 100644 --- a/runbot_merge/github.py +++ b/runbot_merge/github.py @@ -57,8 +57,12 @@ Authorship = TypedDict('Authorship', { 'name': str, 'email': str, }) +CommitTree = TypedDict('CommitTree', { + 'sha': str, + 'url': str, +}) Commit = TypedDict('Commit', { - 'tree': str, + 'tree': CommitTree, 'url': str, 'message': str, # optional when creating a commit diff --git a/runbot_merge/models/stagings_create.py b/runbot_merge/models/stagings_create.py index 7e083e0e..382fa49d 100644 --- a/runbot_merge/models/stagings_create.py +++ b/runbot_merge/models/stagings_create.py @@ -426,7 +426,18 @@ def stage(pr: PullRequests, info: StagingSlice, related_prs: PullRequests) -> Tu fn = stage_rebase_ff case 'squash': fn = stage_squash - return method, fn(pr, info, pr_commits, related_prs=related_prs) + + pr_base_tree = info.repo.get_tree(pr_commits[0]['parents'][0]['sha']) + pr_head_tree = pr_commits[-1]['commit']['tree']['sha'] + + merge_base_tree = info.repo.get_tree(info.head) + new_head = fn(pr, info, pr_commits, related_prs=related_prs) + merge_head_tree = info.repo.get_tree(new_head) + + if pr_head_tree != pr_base_tree and merge_head_tree == merge_base_tree: + raise exceptions.MergeError(pr, f'results in an empty tree when merged, might be the duplicate of a merged PR.') + + 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) diff --git a/runbot_merge/tests/test_oddities.py b/runbot_merge/tests/test_oddities.py index d8121629..755a3312 100644 --- a/runbot_merge/tests/test_oddities.py +++ b/runbot_merge/tests/test_oddities.py @@ -143,3 +143,112 @@ def test_staging_post_update(env, project, make_repo, setreviewers, users, confi assert staging_id.statuses == [ [repo.name, 'ci/runbot', 'failure', ''], ] + +def test_merge_empty_commits(env, project, make_repo, setreviewers, users, config): + """The mergebot should allow merging already-empty commits. + """ + repo = make_repo('repo') + project.write({'repo_ids': [(0, 0, { + 'name': repo.name, + 'group_id': False, + 'required_statuses': 'default', + })]}) + setreviewers(*project.repo_ids) + + with repo: + [m] = repo.make_commits(None, Commit('initial', tree={'m': 'm'}), ref='heads/master') + + repo.make_commits(m, Commit('thing1', tree={}), ref='heads/other1') + pr1 = repo.make_pr(target='master', head='other1') + repo.post_status(pr1.head, 'success') + pr1.post_comment('hansen r+', config['role_reviewer']['token']) + + repo.make_commits(m, Commit('thing2', tree={}), ref='heads/other2') + pr2 = repo.make_pr(target='master', head='other2') + repo.post_status(pr2.head, 'success') + pr2.post_comment('hansen r+ rebase-ff', config['role_reviewer']['token']) + env.run_crons() + pr1_id = to_pr(env, pr1) + pr2_id = to_pr(env, pr2) + assert pr1_id.staging_id and pr2_id.staging_id + + with repo: + repo.post_status('staging.master', 'success') + env.run_crons() + + assert pr1_id.state == pr2_id.state == 'merged' + + # log is most-recent-first (?) + commits = list(repo.log('master')) + head = repo.commit(commits[0]['sha']) + assert repo.read_tree(head) == {'m': 'm'} + + assert commits[0]['commit']['message'].startswith('thing2') + assert commits[1]['commit']['message'].startswith('thing1') + assert commits[2]['commit']['message'] == 'initial' + +def test_merge_emptying_commits(env, project, make_repo, setreviewers, users, config): + """The mergebot should *not* allow merging non-empty commits which become + empty as part of the staging (rebasing) + """ + repo = make_repo('repo') + project.write({'repo_ids': [(0, 0, { + 'name': repo.name, + 'group_id': False, + 'required_statuses': 'default', + })]}) + setreviewers(*project.repo_ids) + + with repo: + [m, _] = repo.make_commits( + None, + Commit('initial', tree={'m': 'm'}), + Commit('second', tree={'m': 'c'}), + ref='heads/master', + ) + + [c1] = repo.make_commits(m, Commit('thing', tree={'m': 'c'}), ref='heads/branch1') + pr1 = repo.make_pr(target='master', head='branch1') + repo.post_status(pr1.head, 'success') + pr1.post_comment('hansen r+ rebase-ff', config['role_reviewer']['token']) + + [_, c2] = repo.make_commits( + m, + Commit('thing1', tree={'c': 'c'}), + Commit('thing2', tree={'m': 'c'}), + ref='heads/branch2', + ) + pr2 = repo.make_pr(target='master', head='branch2') + repo.post_status(pr2.head, 'success') + pr2.post_comment('hansen r+ rebase-ff', config['role_reviewer']['token']) + + repo.make_commits( + m, + Commit('thing1', tree={'m': 'x'}), + Commit('thing2', tree={'m': 'c'}), + ref='heads/branch3', + ) + pr3 = repo.make_pr(target='master', head='branch3') + repo.post_status(pr3.head, 'success') + pr3.post_comment('hansen r+ squash', config['role_reviewer']['token']) + env.run_crons() + + ping = f"@{users['user']} @{users['reviewer']}" + # check that first / sole commit emptying is caught + assert not to_pr(env, pr1).staging_id + assert pr1.comments[3:] == [ + (users['user'], f"{ping} unable to stage: commit {c1} results in an empty tree when merged, it is likely a duplicate of a merged commit, rebase and remove.") + ] + + # check that followup commit emptying is caught + assert not to_pr(env, pr2).staging_id + assert pr2.comments[3:] == [ + (users['user'], f"{ping} unable to stage: commit {c2} results in an empty tree when merged, it is likely a duplicate of a merged commit, rebase and remove.") + ] + + # check that emptied squashed pr is caught + pr3_id = to_pr(env, pr3) + assert not pr3_id.staging_id + assert pr3.comments[3:] == [ + (users['user'], f"{ping} unable to stage: results in an empty tree when merged, might be the duplicate of a merged PR.") + ]