[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
This commit is contained in:
Xavier Morel 2023-10-09 13:52:30 +02:00
parent 2cd3fb8999
commit b21fbaf9cc
4 changed files with 145 additions and 2 deletions

View File

@ -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):

View File

@ -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

View File

@ -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)

View File

@ -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.")
]