diff --git a/conftest.py b/conftest.py index 678deec3..ed8526d3 100644 --- a/conftest.py +++ b/conftest.py @@ -577,12 +577,12 @@ class Repo: if force and r.status_code == 422: self.update_ref(name, commit, force=force) return - assert 200 <= r.status_code < 300, r.json() + assert r.ok, r.text def update_ref(self, name, commit, force=False): assert self.hook r = self._session.patch('https://api.github.com/repos/{}/git/refs/{}'.format(self.name, name), json={'sha': commit, 'force': force}) - assert 200 <= r.status_code < 300, r.json() + assert r.ok, r.text def protect(self, branch): assert self.hook @@ -638,7 +638,7 @@ class Repo: ], 'base_tree': tree }) - assert 200 <= r.status_code < 300, r.json() + assert r.ok, r.text tree = r.json()['sha'] data = { @@ -652,7 +652,7 @@ class Repo: data['committer'] = commit.committer r = self._session.post('https://api.github.com/repos/{}/git/commits'.format(self.name), json=data) - assert 200 <= r.status_code < 300, r.json() + assert r.ok, r.text hashes.append(r.json()['sha']) parents = [hashes[-1]] diff --git a/forwardport/tests/test_simple.py b/forwardport/tests/test_simple.py index 3377bfa4..818f176b 100644 --- a/forwardport/tests/test_simple.py +++ b/forwardport/tests/test_simple.py @@ -474,11 +474,10 @@ def test_access_rights(env, config, make_repo, users, author, reviewer, delegate 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),\ + original_signoff = signoff(config['role_user'], st.message) + forward_signoff = signoff(config['role_' + reviewer], st.message) + assert st.message.index(original_signoff) <= st.message.index(forward_signoff),\ "Check that FP approver is after original PR approver as that's " \ "the review path for the PR" else: diff --git a/runbot_merge/controllers/__init__.py b/runbot_merge/controllers/__init__.py index 70765449..c91afcb2 100644 --- a/runbot_merge/controllers/__init__.py +++ b/runbot_merge/controllers/__init__.py @@ -237,30 +237,23 @@ def handle_status(env, event): 'status on %(sha)s %(context)s:%(state)s (%(target_url)s) [%(description)r]', event ) - Commits = env['runbot_merge.commit'] - env.cr.execute('SELECT id FROM runbot_merge_commit WHERE sha=%s FOR UPDATE', [event['sha']]) - c = Commits.browse(env.cr.fetchone()) - if c: - old = json.loads(c.statuses) - new = { - **old, - event['context']: { - 'state': event['state'], - 'target_url': event['target_url'], - 'description': event['description'] - } + status_value = json.dumps({ + event['context']: { + 'state': event['state'], + 'target_url': event['target_url'], + 'description': event['description'] } - if new != old: # don't update the commit if nothing's changed (e.g dupe status) - c.statuses = json.dumps(new) - else: - Commits.create({ - 'sha': event['sha'], - 'statuses': json.dumps({event['context']: { - 'state': event['state'], - 'target_url': event['target_url'], - 'description': event['description'] - }}) - }) + }) + # create status, or merge update into commit *unless* the update is already + # part of the status (dupe status) + env.cr.execute(""" + INSERT INTO runbot_merge_commit AS c (sha, to_check, statuses) + VALUES (%s, true, %s) + ON CONFLICT (sha) DO UPDATE + SET to_check = true, + statuses = c.statuses::jsonb || EXCLUDED.statuses::jsonb + WHERE NOT c.statuses::jsonb @> EXCLUDED.statuses::jsonb + """, [event['sha'], status_value]) return 'ok' diff --git a/runbot_merge/github.py b/runbot_merge/github.py index dca0e50f..89a873e4 100644 --- a/runbot_merge/github.py +++ b/runbot_merge/github.py @@ -8,6 +8,7 @@ import pathlib import pprint import textwrap import unicodedata +from datetime import datetime, timezone import requests import werkzeug.urls @@ -312,7 +313,10 @@ class GH(object): 'tree': c['new_tree'], 'parents': [prev], 'author': c['commit']['author'], - 'committer': c['commit']['committer'], + 'committer': dict( + c['commit']['committer'], + date=datetime.utcnow().strftime("%Y-%m-%dT%H:%M:%SZ") + ), }, check={409: MergeError}).json() logger.debug('copied %s to %s (parent: %s)', c['sha'], copy['sha'], prev) prev = mapping[c['sha']] = copy['sha'] diff --git a/runbot_merge/models/pull_requests.py b/runbot_merge/models/pull_requests.py index 84233d0c..19d0f2b0 100644 --- a/runbot_merge/models/pull_requests.py +++ b/runbot_merge/models/pull_requests.py @@ -518,10 +518,11 @@ class Branch(models.Model): Batch = self.env['runbot_merge.batch'] staged = Batch + original_heads = {} meta = {repo: {} for repo in self.project_id.repo_ids.having_branch(self)} for repo, it in meta.items(): gh = it['gh'] = repo.github() - it['head'] = gh.head(self.name) + it['head'] = original_heads[repo] = gh.head(self.name) # create tmp staging branch gh.set_ref('tmp.{}'.format(self.name), it['head']) @@ -567,23 +568,30 @@ class Branch(models.Model): for repo, h in heads.items() if not repo.endswith('^') ) - dummy_head = it['gh']('post', 'git/commits', json={ - 'message': '''force rebuild + dummy_head = {'sha': it['head']} + if it['head'] == original_heads[repo]: + # if the repo has not been updated by the staging, create a + # dummy commit to force rebuild + dummy_head = it['gh']('post', 'git/commits', json={ + 'message': '''force rebuild uniquifier: %s For-Commit-Id: %s %s''' % (r, it['head'], trailer), - 'tree': tree['sha'], - 'parents': [it['head']], - }).json() + 'tree': tree['sha'], + 'parents': [it['head']], + }).json() - # $repo is the head to check, $repo^ is the head to merge + # $repo is the head to check, $repo^ is the head to merge (they + # might be the same) heads[repo.name + '^'] = it['head'] heads[repo.name] = dummy_head['sha'] - self.env['runbot_merge.commit'].create({ - 'sha': dummy_head['sha'], - 'statuses': '{}', - }) + self.env.cr.execute( + "INSERT INTO runbot_merge_commit (sha, to_check, statuses) " + "VALUES (%s, true, '{}') " + "ON CONFLICT (sha) DO UPDATE SET to_check=true", + [dummy_head['sha']] + ) # create actual staging object st = self.env['runbot_merge.stagings'].create({ diff --git a/runbot_merge/tests/test_basic.py b/runbot_merge/tests/test_basic.py index 17b011ba..08730262 100644 --- a/runbot_merge/tests/test_basic.py +++ b/runbot_merge/tests/test_basic.py @@ -98,8 +98,6 @@ def test_trivial_flow(env, repo, page, users, config): assert s[1].get('class') == 'bg-success' assert s[1][0].text.strip() == '{}: ci/runbot'.format(repo.name) - assert re.match('^force rebuild', staging_head.message) - assert st.state == 'success' assert pr_id.state == 'merged' assert pr_page(page, pr).cssselect('.alert-success') @@ -846,18 +844,17 @@ def test_forward_port(env, repo, config): pr.post_comment('hansen r+ merge', config['role_reviewer']['token']) env.run_crons() - st = repo.commit('heads/staging.master') - assert st.message.startswith('force rebuild') + st = repo.commit('staging.master') with repo: repo.post_status(st.id, 'success', 'legal/cla') repo.post_status(st.id, 'success', 'ci/runbot') env.run_crons() - h = repo.commit('heads/master') - assert set(st.parents) == {h.id} + h = repo.commit('master') + assert st.id == h.id assert set(h.parents) == {m, pr.head} - commits = {c['sha'] for c in repo.log('heads/master')} + commits = {c['sha'] for c in repo.log('master')} assert len(commits) == 112 @pytest.mark.skip("Needs to find a way to make set_ref fail on *second* call.") @@ -1231,13 +1228,10 @@ class TestMergeMethod: staging = repo.commit('heads/staging.master') assert not repo.is_ancestor(prx.head, of=staging.id),\ "the pr head should not be an ancestor of the staging branch in a squash merge" - assert re.match('^force rebuild', staging.message) assert repo.read_tree(staging) == { 'm': 'c1', 'm2': 'm2', }, "the tree should still be correctly merged" - [actual_sha] = staging.parents - actual = repo.commit(actual_sha) - assert actual.parents == [m2],\ + assert staging.parents == [m2],\ "dummy commit aside, the previous master's tip should be the sole parent of the staging commit" with repo: @@ -1251,8 +1245,8 @@ class TestMergeMethod: assert pr.state == 'merged' assert prx.state == 'closed' assert json.loads(pr.commits_map) == { - c1: actual_sha, - '': actual_sha, + c1: staging.id, + '': staging.id, }, "for a squash, the one PR commit should be mapped to the one rebased commit" def test_pr_update_to_many_commits(self, repo, env): @@ -1443,8 +1437,7 @@ class TestMergeMethod: f'title\n\nbody\n\ncloses {pr_id.display_name}\n\nSigned-off-by: {reviewer}', frozenset([nm2, nb1]) ) - expected = (re_matches('^force rebuild'), frozenset([merge_head])) - assert staging == expected + assert staging == merge_head with repo: repo.post_status('heads/staging.master', 'success', 'legal/cla') @@ -1497,13 +1490,20 @@ class TestMergeMethod: +------+ +--^---+ """ with repo: - m0 = repo.make_commit(None, 'M0', None, tree={'m': '0'}) - m1 = repo.make_commit(m0, 'M1', None, tree={'m': '1'}) - m2 = repo.make_commit(m1, 'M2', None, tree={'m': '2'}) - repo.make_ref('heads/master', m2) + _, m1, m2 = repo.make_commits( + None, + Commit('M0', tree={'m': '0'}), + Commit('M1', tree={'m': '1'}), + Commit('M2', tree={'m': '2'}), + ref='heads/master' + ) + + b0, b1 = repo.make_commits( + m1, + Commit('B0', tree={'b': '0'}, author={'name': 'Maarten Tromp', 'email': 'm.tromp@example.nl', 'date': '1651-03-30T12:00:00Z'}), + Commit('B1', tree={'b': '1'}, author={'name': 'Rein Huydecoper', 'email': 'r.huydecoper@example.nl', 'date': '1986-04-17T12:00:00Z'}), + ) - b0 = repo.make_commit(m1, 'B0', None, tree={'m': '1', 'b': '0'}) - b1 = repo.make_commit(b0, 'B1', None, tree={'m': '1', 'b': '1'}) prx = repo.make_pr(title='title', body='body', target='master', head=b1) repo.post_status(prx.head, 'success', 'legal/cla') repo.post_status(prx.head, 'success', 'ci/runbot') @@ -1518,8 +1518,7 @@ class TestMergeMethod: reviewer = get_partner(env, users["reviewer"]).formatted_email nb1 = node(f'B1\n\ncloses {pr_id.display_name}\n\nSigned-off-by: {reviewer}', node(part_of('B0', pr_id), nm2)) - expected = node(re_matches('^force rebuild'), nb1) - assert staging == expected + assert staging == nb1 with repo: repo.post_status('heads/staging.master', 'success', 'legal/cla') @@ -1547,6 +1546,23 @@ class TestMergeMethod: b0: m0.id, # first PR's commit } assert m0.parents == [m2], "can't hurt to check the parent of our root commit" + assert m0.author['date'] != m0.committer['date'], "commit date should have been rewritten" + assert m1.author['date'] != m1.committer['date'], "commit date should have been rewritten" + + utcday = datetime.datetime.utcnow().date() + def parse(dt): + return datetime.datetime.strptime(dt, "%Y-%m-%dT%H:%M:%SZ") + + # FIXME: actual commit creation could run before the date rollover and + # local datetime.utcnow() after + assert parse(m0.committer['date']).date() == utcday + # FIXME: git date storage is unreliable and non-portable outside of an + # unsigned 31b epoch range so the m0 event may get flung in the + # future (compared to the literal datum), this test unexpectedly + # becoming true if run on the exact wrong day + assert parse(m0.author['date']).date() != utcday + assert parse(m1.committer['date']).date() == utcday + assert parse(m0.author['date']).date() != utcday @pytest.mark.skip(reason="what do if the PR contains merge commits???") def test_pr_contains_merges(self, repo, env): @@ -2397,11 +2413,7 @@ class TestBatching(object): p1, node(part_of('commit_PR2_01', pr2), node(part_of('commit_PR2_00', pr2), p1)) ) - expected = (re_matches('^force rebuild'), frozenset([p2])) - import pprint - pprint.pprint(staging) - pprint.pprint(expected) - assert staging == expected + assert staging == p2 def test_staging_batch_norebase(self, env, repo, users, config): """ If multiple PRs are ready for the same target at the same point, @@ -2426,7 +2438,7 @@ class TestBatching(object): assert pr2.staging_id assert pr1.staging_id == pr2.staging_id - log = list(repo.log('heads/staging.master')) + log = list(repo.log('staging.master')) staging = log_to_node(log) reviewer = get_partner(env, users["reviewer"]).formatted_email @@ -2441,8 +2453,7 @@ class TestBatching(object): p1, node('commit_PR2_01', node('commit_PR2_00', node('initial'))) ) - expected = (re_matches('^force rebuild'), frozenset([p2])) - assert staging == expected + assert staging == p2 def test_staging_batch_squash(self, env, repo, users, config): """ If multiple PRs are ready for the same target at the same point, @@ -2467,11 +2478,9 @@ class TestBatching(object): staging = log_to_node(log) reviewer = get_partner(env, users["reviewer"]).formatted_email - expected = node( - re_matches('^force rebuild'), - node('commit_PR2_00\n\ncloses {}#{}\n\nSigned-off-by: {}'.format(repo.name, pr2.number, reviewer), - node('commit_PR1_00\n\ncloses {}#{}\n\nSigned-off-by: {}'.format(repo.name, pr1.number, reviewer), - node('initial')))) + expected = node('commit_PR2_00\n\ncloses {}#{}\n\nSigned-off-by: {}'.format(repo.name, pr2.number, reviewer), + node('commit_PR1_00\n\ncloses {}#{}\n\nSigned-off-by: {}'.format(repo.name, pr1.number, reviewer), + node('initial'))) assert staging == expected def test_batching_pressing(self, env, repo, config): diff --git a/runbot_merge/tests/test_multirepo.py b/runbot_merge/tests/test_multirepo.py index 99d3f48b..ebc90c61 100644 --- a/runbot_merge/tests/test_multirepo.py +++ b/runbot_merge/tests/test_multirepo.py @@ -108,10 +108,15 @@ def test_stage_one(env, project, repo_a, repo_b, config): ) env.run_crons() - assert to_pr(env, pr_a).state == 'ready' - assert to_pr(env, pr_a).staging_id - assert to_pr(env, pr_b).state == 'ready' - assert not to_pr(env, pr_b).staging_id + pra_id = to_pr(env, pr_a) + assert pra_id.state == 'ready' + assert pra_id.staging_id + assert repo_a.commit('staging.master').message.startswith('commit_A_00') + assert repo_b.commit('staging.master').message.startswith('force rebuild') + + prb_id = to_pr(env, pr_b) + assert prb_id.state == 'ready' + assert not prb_id.staging_id get_related_pr_labels = XPath('.//*[normalize-space(text()) = "Linked pull requests"]//a/text()') def test_stage_match(env, project, repo_a, repo_b, config, page): @@ -364,15 +369,16 @@ def test_sub_match(env, project, repo_a, repo_b, repo_c, config): "branch-matched PRs should be part of the same staging" st = pr_b.staging_id - b_staging = repo_b.commit('heads/staging.master') - c_staging = repo_c.commit('heads/staging.master') + a_staging = repo_a.commit('staging.master') + b_staging = repo_b.commit('staging.master') + c_staging = repo_c.commit('staging.master') assert json.loads(st.heads) == { - repo_a.name: repo_a.commit('heads/staging.master').id, - repo_a.name + '^': repo_a.commit('heads/master').id, + repo_a.name: a_staging.id, + repo_a.name + '^': a_staging.parents[0], repo_b.name: b_staging.id, - repo_b.name + '^': b_staging.parents[0], + repo_b.name + '^': b_staging.id, repo_c.name: c_staging.id, - repo_c.name + '^': c_staging.parents[0], + repo_c.name + '^': c_staging.id, } def test_merge_fail(env, project, repo_a, repo_b, users, config): @@ -432,7 +438,6 @@ def test_merge_fail(env, project, repo_a, repo_b, users, config): c['commit']['message'] for c in repo_a.log('heads/staging.master') ] == [ - re_matches('^force rebuild'), """commit_do-b-thing_00 closes %s