[FIX] forwardport: fix deduplication of authorship in multi-pr conflict

a45f7260fa had intended to use the
original authorship information for conflict commit even if there were
multiple commits, as long as there was only one author (/ committer)
for the entire sequence.

Sadly the deduplication was buggy as it took the *authorship date* in
account, which basically ensured commits would never be considered as
having the same authorship outside of tests (where it was possible for
commits to be created at the same second).

Related to #505
This commit is contained in:
Xavier Morel 2021-08-02 09:18:30 +02:00 committed by xmo-odoo
parent 32829cf880
commit d249417ceb
5 changed files with 336 additions and 228 deletions

View File

@ -130,16 +130,18 @@ def rolemap(request, config):
r = _rate_limited(lambda: requests.get('https://api.github.com/user', headers={'Authorization': 'token %s' % data['token']}))
r.raise_for_status()
rolemap[role] = data['user'] = r.json()['login']
user = rolemap[role] = r.json()
data['user'] = user['login']
return rolemap
@pytest.fixture
def partners(env, config, rolemap):
m = dict.fromkeys(rolemap.keys(), env['res.partner'])
for role, login in rolemap.items():
for role, u in rolemap.items():
if role in ('user', 'other'):
continue
login = u['login']
m[role] = env['res.partner'].create({
'name': config['role_' + role].get('name', login),
'github_login': login,
@ -165,7 +167,7 @@ def setreviewers(partners):
@pytest.fixture
def users(partners, rolemap):
return rolemap
return {k: v['login'] for k, v in rolemap.items()}
@pytest.fixture(scope='session')
def tunnel(pytestconfig, port):
@ -419,7 +421,7 @@ def _rate_limited(req):
q = req()
if not q.ok and q.headers.get('X-RateLimit-Remaining') == '0':
reset = int(q.headers['X-RateLimit-Reset'])
delay = round(reset - time.time() + 1.0)
delay = max(0, round(reset - time.time() + 1.0))
print("Hit rate limit, sleeping for", delay, "seconds")
time.sleep(delay)
continue

View File

@ -860,8 +860,10 @@ This PR targets %s and is part of the forward-port chain. Further PRs will be cr
root_branch = 'origin/pull/%d' % root.number
working_copy.checkout('-bsquashed', root_branch)
root_commits = root.commits()
# commits returns oldest first, so youngest (head) last
head_commit = root_commits[-1]['commit']
to_tuple = operator.itemgetter('name', 'email', 'date')
to_tuple = operator.itemgetter('name', 'email')
to_dict = lambda term, vals: {
'GIT_%s_NAME' % term: vals[0],
'GIT_%s_EMAIL' % term: vals[1],
@ -872,8 +874,10 @@ This PR targets %s and is part of the forward-port chain. Further PRs will be cr
authors.add(to_tuple(c['author']))
committers.add(to_tuple(c['committer']))
fp_authorship = (project_id.fp_github_name, project_id.fp_github_email, '')
author = authors.pop() if len(authors) == 1 else fp_authorship
committer = committers.pop() if len(committers) == 1 else fp_authorship
author = fp_authorship if len(authors) != 1\
else authors.pop() + (head_commit['author']['date'],)
committer = fp_authorship if len(committers) != 1 \
else committers.pop() + (head_commit['committer']['date'],)
conf = working_copy.with_config(env={
**to_dict('AUTHOR', author),
**to_dict('COMMITTER', committer),

View File

@ -0,0 +1,321 @@
import re
import time
from operator import itemgetter
from utils import make_basic, Commit, validate_all, re_matches, seen, REF_PATTERN, to_pr
def test_conflict(env, config, make_repo, users):
""" Create a PR to A which will (eventually) conflict with C when
forward-ported.
"""
prod, other = make_basic(env, config, make_repo)
# create a d branch
with prod:
prod.make_commits('c', Commit('1111', tree={'i': 'a'}), ref='heads/d')
project = env['runbot_merge.project'].search([])
project.write({
'branch_ids': [
(0, 0, {'name': 'd', 'fp_sequence': 4, 'fp_target': True})
]
})
# generate a conflict: create a h file in a PR to a
with prod:
[p_0] = prod.make_commits(
'a', Commit('p_0', tree={'h': 'xxx'}),
ref='heads/conflicting'
)
pr = prod.make_pr(target='a', head='conflicting')
prod.post_status(p_0, 'success', 'legal/cla')
prod.post_status(p_0, 'success', 'ci/runbot')
pr.post_comment('hansen r+', config['role_reviewer']['token'])
env.run_crons()
with prod:
prod.post_status('staging.a', 'success', 'legal/cla')
prod.post_status('staging.a', 'success', 'ci/runbot')
env.run_crons()
pra_id, prb_id = env['runbot_merge.pull_requests'].search([], order='number')
# mark pr b as OK so it gets ported to c
with prod:
validate_all([prod], [prb_id.head])
env.run_crons()
pra_id, prb_id, prc_id = env['runbot_merge.pull_requests'].search([], order='number')
# should have created a new PR
# but it should not have a parent, and there should be conflict markers
assert not prc_id.parent_id
assert prc_id.source_id == pra_id
assert prc_id.state == 'opened'
p = prod.commit(p_0)
c = prod.commit(prc_id.head)
assert c.author == p.author
# ignore date as we're specifically not keeping the original's
without_date = itemgetter('name', 'email')
assert without_date(c.committer) == without_date(p.committer)
assert prod.read_tree(c) == {
'f': 'c',
'g': 'a',
'h': re_matches(r'''<<<\x3c<<< HEAD
a
=======
xxx
>>>\x3e>>> [0-9a-f]{7,}.*
'''),
}
prb = prod.get_pr(prb_id.number)
assert prb.comments == [
seen(env, prb, users),
(users['user'], '''\
This PR targets b and is part of the forward-port chain. Further PRs will be created up to d.
More info at https://github.com/odoo/odoo/wiki/Mergebot#forward-port
'''),
(users['user'], """Ping @%s, @%s
The next pull request (%s) is in conflict. You can merge the chain up to here by saying
> @%s r+
More info at https://github.com/odoo/odoo/wiki/Mergebot#forward-port
""" % (
users['user'], users['reviewer'],
prc_id.display_name,
project.fp_github_name
))
]
# check that CI passing does not create more PRs
with prod:
validate_all([prod], [prc_id.head])
env.run_crons()
time.sleep(5)
env.run_crons()
assert pra_id | prb_id | prc_id == env['runbot_merge.pull_requests'].search([], order='number'),\
"CI passing should not have resumed the FP process on a conflicting / draft PR"
# fix the PR, should behave as if this were a normal PR
prc = prod.get_pr(prc_id.number)
pr_repo, pr_ref = prc.branch
with pr_repo:
pr_repo.make_commits(
# if just given a branch name, goes and gets it from pr_repo whose
# "b" was cloned before that branch got rolled back
'c',
Commit('h should indeed be xxx', tree={'h': 'xxx'}),
ref='heads/%s' % pr_ref,
make=False,
)
env.run_crons()
assert prod.read_tree(prod.commit(prc_id.head)) == {
'f': 'c',
'g': 'a',
'h': 'xxx',
}
assert prc_id.state == 'opened', "state should be open still"
assert ('#%d' % pra_id.number) in prc_id.message
# check that merging the fixed PR fixes the flow and restarts a forward
# port process
with prod:
prod.post_status(prc.head, 'success', 'legal/cla')
prod.post_status(prc.head, 'success', 'ci/runbot')
prc.post_comment('hansen r+', config['role_reviewer']['token'])
env.run_crons()
assert prc_id.staging_id
with prod:
prod.post_status('staging.c', 'success', 'legal/cla')
prod.post_status('staging.c', 'success', 'ci/runbot')
env.run_crons()
*_, prd_id = env['runbot_merge.pull_requests'].search([], order='number')
assert ('#%d' % pra_id.number) in prd_id.message, \
"check that source / PR A is referenced by resume PR"
assert ('#%d' % prc_id.number) in prd_id.message, \
"check that parent / PR C is referenced by resume PR"
assert prd_id.parent_id == prc_id
assert prd_id.source_id == pra_id
assert re.match(
REF_PATTERN.format(target='d', source='conflicting'),
prd_id.refname
)
assert prod.read_tree(prod.commit(prd_id.head)) == {
'f': 'c',
'g': 'a',
'h': 'xxx',
'i': 'a',
}
def test_conflict_deleted(env, config, make_repo):
prod, other = make_basic(env, config, make_repo)
# remove f from b
with prod:
prod.make_commits(
'b', Commit('33', tree={'g': 'c'}, reset=True),
ref='heads/b'
)
# generate a conflict: update f in a
with prod:
[p_0] = prod.make_commits(
'a', Commit('p_0', tree={'f': 'xxx'}),
ref='heads/conflicting'
)
pr = prod.make_pr(target='a', head='conflicting')
prod.post_status(p_0, 'success', 'legal/cla')
prod.post_status(p_0, 'success', 'ci/runbot')
pr.post_comment('hansen r+', config['role_reviewer']['token'])
env.run_crons()
with prod:
prod.post_status('staging.a', 'success', 'legal/cla')
prod.post_status('staging.a', 'success', 'ci/runbot')
env.run_crons()
# wait a bit for PR webhook... ?
time.sleep(5)
env.run_crons()
# should have created a new PR
pr0, pr1 = env['runbot_merge.pull_requests'].search([], order='number')
# but it should not have a parent
assert not pr1.parent_id
assert pr1.source_id == pr0
assert prod.read_tree(prod.commit('b')) == {
'g': 'c',
}
assert pr1.state == 'opened'
# NOTE: no actual conflict markers because pr1 essentially adds f de-novo
assert prod.read_tree(prod.commit(pr1.head)) == {
'f': 'xxx',
'g': 'c',
}
# check that CI passing does not create more PRs
with prod:
validate_all([prod], [pr1.head])
env.run_crons()
time.sleep(5)
env.run_crons()
assert pr0 | pr1 == env['runbot_merge.pull_requests'].search([], order='number'),\
"CI passing should not have resumed the FP process on a conflicting / draft PR"
# fix the PR, should behave as if this were a normal PR
get_pr = prod.get_pr(pr1.number)
pr_repo, pr_ref = get_pr.branch
with pr_repo:
pr_repo.make_commits(
# if just given a branch name, goes and gets it from pr_repo whose
# "b" was cloned before that branch got rolled back
prod.commit('b').id,
Commit('f should indeed be removed', tree={'g': 'c'}, reset=True),
ref='heads/%s' % pr_ref,
make=False,
)
env.run_crons()
assert prod.read_tree(prod.commit(pr1.head)) == {
'g': 'c',
}
assert pr1.state == 'opened', "state should be open still"
def test_multiple_commits_same_authorship(env, config, make_repo):
""" When a PR has multiple commits by the same author and its
forward-porting triggers a conflict, the resulting (squashed) conflict
commit should have the original author (same with the committer).
"""
author = {'name': 'George Pearce', 'email': 'gp@example.org'}
committer = {'name': 'G. P. W. Meredith', 'email': 'gpwm@example.org'}
prod, _ = make_basic(env, config, make_repo)
with prod:
# conflict: create `g` in `a`, using two commits
prod.make_commits(
'a',
Commit('c0', tree={'g': '1'},
author={**author, 'date': '1932-10-18T12:00:00Z'},
committer={**committer, 'date': '1932-11-02T12:00:00Z'}),
Commit('c1', tree={'g': '2'},
author={**author, 'date': '1932-11-12T12:00:00Z'},
committer={**committer, 'date': '1932-11-13T12:00:00Z'}),
ref='heads/conflicting'
)
pr = prod.make_pr(target='a', head='conflicting')
prod.post_status('conflicting', 'success', 'legal/cla')
prod.post_status('conflicting', 'success', 'ci/runbot')
pr.post_comment('hansen r+ rebase-ff', config['role_reviewer']['token'])
env.run_crons()
pr_id = to_pr(env, pr)
assert pr_id.state == 'ready'
assert pr_id.staging_id
with prod:
prod.post_status('staging.a', 'success', 'legal/cla')
prod.post_status('staging.a', 'success', 'ci/runbot')
env.run_crons()
for _ in range(20):
pr_ids = env['runbot_merge.pull_requests'].search([], order='number')
if len(pr_ids) == 2:
_ , pr2_id = pr_ids
break
time.sleep(0.5)
else:
assert 0, "timed out"
c = prod.commit(pr2_id.head)
get = itemgetter('name', 'email')
assert get(c.author) == get(author)
assert get(c.committer) == get(committer)
def test_multiple_commits_different_authorship(env, config, make_repo, users, rolemap):
""" When a PR has multiple commits by the same author and its
forward-porting triggers a conflict, the resulting (squashed) conflict
commit should have the original author (same with the committer).
"""
author = {'name': 'George Pearce', 'email': 'gp@example.org'}
committer = {'name': 'G. P. W. Meredith', 'email': 'gpwm@example.org'}
prod, _ = make_basic(env, config, make_repo)
with prod:
# conflict: create `g` in `a`, using two commits
# just swap author and committer in the commits
prod.make_commits(
'a',
Commit('c0', tree={'g': '1'},
author={**author, 'date': '1932-10-18T12:00:00Z'},
committer={**committer, 'date': '1932-11-02T12:00:00Z'}),
Commit('c1', tree={'g': '2'},
author={**committer, 'date': '1932-11-12T12:00:00Z'},
committer={**author, 'date': '1932-11-13T12:00:00Z'}),
ref='heads/conflicting'
)
pr = prod.make_pr(target='a', head='conflicting')
prod.post_status('conflicting', 'success', 'legal/cla')
prod.post_status('conflicting', 'success', 'ci/runbot')
pr.post_comment('hansen r+ rebase-ff', config['role_reviewer']['token'])
env.run_crons()
pr_id = to_pr(env, pr)
assert pr_id.state == 'ready'
assert pr_id.staging_id
with prod:
prod.post_status('staging.a', 'success', 'legal/cla')
prod.post_status('staging.a', 'success', 'ci/runbot')
env.run_crons()
for _ in range(20):
pr_ids = env['runbot_merge.pull_requests'].search([], order='number')
if len(pr_ids) == 2:
_ , pr2_id = pr_ids
break
time.sleep(0.5)
else:
assert 0, "timed out"
c = prod.commit(pr2_id.head)
get = itemgetter('name', 'email')
rm = rolemap['user']
assert get(c.author) == (rm['login'], rm['email'])
assert get(c.committer) == (rm['login'], rm['email'])

View File

@ -3,11 +3,10 @@ import collections
import re
import time
from datetime import datetime, timedelta
from operator import itemgetter
import pytest
from utils import seen, re_matches, Commit, make_basic, REF_PATTERN, MESSAGE_TEMPLATE, validate_all
from utils import seen, Commit, make_basic, REF_PATTERN, MESSAGE_TEMPLATE, validate_all
FMT = '%Y-%m-%d %H:%M:%S'
FAKE_PREV_WEEK = (datetime.now() + timedelta(days=1)).strftime(FMT)
@ -233,220 +232,6 @@ More info at https://github.com/odoo/odoo/wiki/Mergebot#forward-port
with pytest.raises(AssertionError, match="Not Found"):
other.get_ref(pr2_ref)
def test_conflict(env, config, make_repo, users):
""" Create a PR to A which will (eventually) conflict with C when
forward-ported.
"""
prod, other = make_basic(env, config, make_repo)
# create a d branch
with prod:
prod.make_commits('c', Commit('1111', tree={'i': 'a'}), ref='heads/d')
project = env['runbot_merge.project'].search([])
project.write({
'branch_ids': [
(0, 0, {'name': 'd', 'fp_sequence': 4, 'fp_target': True})
]
})
# generate a conflict: create a h file in a PR to a
with prod:
[p_0] = prod.make_commits(
'a', Commit('p_0', tree={'h': 'xxx'}),
ref='heads/conflicting'
)
pr = prod.make_pr(target='a', head='conflicting')
prod.post_status(p_0, 'success', 'legal/cla')
prod.post_status(p_0, 'success', 'ci/runbot')
pr.post_comment('hansen r+', config['role_reviewer']['token'])
env.run_crons()
with prod:
prod.post_status('staging.a', 'success', 'legal/cla')
prod.post_status('staging.a', 'success', 'ci/runbot')
env.run_crons()
pra_id, prb_id = env['runbot_merge.pull_requests'].search([], order='number')
# mark pr b as OK so it gets ported to c
with prod:
validate_all([prod], [prb_id.head])
env.run_crons()
pra_id, prb_id, prc_id = env['runbot_merge.pull_requests'].search([], order='number')
# should have created a new PR
# but it should not have a parent, and there should be conflict markers
assert not prc_id.parent_id
assert prc_id.source_id == pra_id
assert prc_id.state == 'opened'
p = prod.commit(p_0)
c = prod.commit(prc_id.head)
assert c.author == p.author
# ignore date as we're specifically not keeping the original's
without_date = itemgetter('name', 'email')
assert without_date(c.committer) == without_date(p.committer)
assert prod.read_tree(c) == {
'f': 'c',
'g': 'a',
'h': re_matches(r'''<<<\x3c<<< HEAD
a
=======
xxx
>>>\x3e>>> [0-9a-f]{7,}.*
'''),
}
prb = prod.get_pr(prb_id.number)
assert prb.comments == [
seen(env, prb, users),
(users['user'], '''\
This PR targets b and is part of the forward-port chain. Further PRs will be created up to d.
More info at https://github.com/odoo/odoo/wiki/Mergebot#forward-port
'''),
(users['user'], """Ping @%s, @%s
The next pull request (%s) is in conflict. You can merge the chain up to here by saying
> @%s r+
More info at https://github.com/odoo/odoo/wiki/Mergebot#forward-port
""" % (
users['user'], users['reviewer'],
prc_id.display_name,
project.fp_github_name
))
]
# check that CI passing does not create more PRs
with prod:
validate_all([prod], [prc_id.head])
env.run_crons()
time.sleep(5)
env.run_crons()
assert pra_id | prb_id | prc_id == env['runbot_merge.pull_requests'].search([], order='number'),\
"CI passing should not have resumed the FP process on a conflicting / draft PR"
# fix the PR, should behave as if this were a normal PR
prc = prod.get_pr(prc_id.number)
pr_repo, pr_ref = prc.branch
with pr_repo:
pr_repo.make_commits(
# if just given a branch name, goes and gets it from pr_repo whose
# "b" was cloned before that branch got rolled back
'c',
Commit('h should indeed be xxx', tree={'h': 'xxx'}),
ref='heads/%s' % pr_ref,
make=False,
)
env.run_crons()
assert prod.read_tree(prod.commit(prc_id.head)) == {
'f': 'c',
'g': 'a',
'h': 'xxx',
}
assert prc_id.state == 'opened', "state should be open still"
assert ('#%d' % pra_id.number) in prc_id.message
# check that merging the fixed PR fixes the flow and restarts a forward
# port process
with prod:
prod.post_status(prc.head, 'success', 'legal/cla')
prod.post_status(prc.head, 'success', 'ci/runbot')
prc.post_comment('hansen r+', config['role_reviewer']['token'])
env.run_crons()
assert prc_id.staging_id
with prod:
prod.post_status('staging.c', 'success', 'legal/cla')
prod.post_status('staging.c', 'success', 'ci/runbot')
env.run_crons()
*_, prd_id = env['runbot_merge.pull_requests'].search([], order='number')
assert ('#%d' % pra_id.number) in prd_id.message, \
"check that source / PR A is referenced by resume PR"
assert ('#%d' % prc_id.number) in prd_id.message, \
"check that parent / PR C is referenced by resume PR"
assert prd_id.parent_id == prc_id
assert prd_id.source_id == pra_id
assert re.match(
REF_PATTERN.format(target='d', source='conflicting'),
prd_id.refname
)
assert prod.read_tree(prod.commit(prd_id.head)) == {
'f': 'c',
'g': 'a',
'h': 'xxx',
'i': 'a',
}
def test_conflict_deleted(env, config, make_repo):
prod, other = make_basic(env, config, make_repo)
# remove f from b
with prod:
prod.make_commits(
'b', Commit('33', tree={'g': 'c'}, reset=True),
ref='heads/b'
)
# generate a conflict: update f in a
with prod:
[p_0] = prod.make_commits(
'a', Commit('p_0', tree={'f': 'xxx'}),
ref='heads/conflicting'
)
pr = prod.make_pr(target='a', head='conflicting')
prod.post_status(p_0, 'success', 'legal/cla')
prod.post_status(p_0, 'success', 'ci/runbot')
pr.post_comment('hansen r+', config['role_reviewer']['token'])
env.run_crons()
with prod:
prod.post_status('staging.a', 'success', 'legal/cla')
prod.post_status('staging.a', 'success', 'ci/runbot')
env.run_crons()
# wait a bit for PR webhook... ?
time.sleep(5)
env.run_crons()
# should have created a new PR
pr0, pr1 = env['runbot_merge.pull_requests'].search([], order='number')
# but it should not have a parent
assert not pr1.parent_id
assert pr1.source_id == pr0
assert prod.read_tree(prod.commit('b')) == {
'g': 'c',
}
assert pr1.state == 'opened'
# NOTE: no actual conflict markers because pr1 essentially adds f de-novo
assert prod.read_tree(prod.commit(pr1.head)) == {
'f': 'xxx',
'g': 'c',
}
# check that CI passing does not create more PRs
with prod:
validate_all([prod], [pr1.head])
env.run_crons()
time.sleep(5)
env.run_crons()
assert pr0 | pr1 == env['runbot_merge.pull_requests'].search([], order='number'),\
"CI passing should not have resumed the FP process on a conflicting / draft PR"
# fix the PR, should behave as if this were a normal PR
get_pr = prod.get_pr(pr1.number)
pr_repo, pr_ref = get_pr.branch
with pr_repo:
pr_repo.make_commits(
# if just given a branch name, goes and gets it from pr_repo whose
# "b" was cloned before that branch got rolled back
prod.commit('b').id,
Commit('f should indeed be removed', tree={'g': 'c'}, reset=True),
ref='heads/%s' % pr_ref,
make=False,
)
env.run_crons()
assert prod.read_tree(prod.commit(pr1.head)) == {
'g': 'c',
}
assert pr1.state == 'opened', "state should be open still"
def test_empty(env, config, make_repo, users):
""" Cherrypick of an already cherrypicked (or separately implemented)
commit -> create draft PR.

View File

@ -52,11 +52,7 @@ class re_matches:
return '~' + self._r.pattern + '~'
def seen(env, pr, users):
pr_id = env['runbot_merge.pull_requests'].search([
('repository.name', '=', pr.repo.name),
('number', '=', pr.number)
])
return users['user'], f'[Pull request status dashboard]({pr_id.url}).'
return users['user'], f'[Pull request status dashboard]({to_pr(env, pr).url}).'
def make_basic(env, config, make_repo, *, reponame='proj', project_name='myproject'):
""" Creates a basic repo with 3 forking branches