diff --git a/runbot_merge/github.py b/runbot_merge/github.py index 4b17fa9b..0f53a286 100644 --- a/runbot_merge/github.py +++ b/runbot_merge/github.py @@ -172,7 +172,7 @@ class GH(object): """ Rebase pr's commits on top of dest, updates dest unless ``reset`` is set. - Returns the hash of the rebased head. + Returns the hash of the rebased head and a map of all PR commits (to the PR they were rebased to) """ logger = _logger.getChild('rebase') original_head = self.head(dest) @@ -189,6 +189,7 @@ class GH(object): c['new_tree'] = self.merge(c['sha'], dest, tmp_msg)['tree']['sha'] prev = original_head + mapping = {} for c in commits: copy = self('post', 'git/commits', json={ 'message': c['commit']['message'], @@ -198,7 +199,7 @@ class GH(object): 'committer': c['commit']['committer'], }, check={409: exceptions.MergeError}).json() logger.debug('copied %s to %s (parent: %s)', c['sha'], copy['sha'], prev) - prev = copy['sha'] + prev = mapping[c['sha']] = copy['sha'] if reset: self.set_ref(dest, original_head) @@ -209,7 +210,7 @@ class GH(object): self._repo, pr, dest, reset, len(commits), prev) # prev is updated after each copy so it's the rebased PR head - return prev + return prev, mapping # fetch various bits of issues / prs to load them def pr(self, number): diff --git a/runbot_merge/models/pull_requests.py b/runbot_merge/models/pull_requests.py index ddbc2b8e..710a682e 100644 --- a/runbot_merge/models/pull_requests.py +++ b/runbot_merge/models/pull_requests.py @@ -494,6 +494,7 @@ class PullRequests(models.Model): batch_id = fields.Many2one('runbot_merge.batch',compute='_compute_active_batch', store=True) batch_ids = fields.Many2many('runbot_merge.batch') staging_id = fields.Many2one(related='batch_id.staging_id', store=True) + commits_map = fields.Char(help="JSON-encoded mapping of PR commits to actually integrated commits. The integration head (either a merge commit or the PR's topmost) is mapped from the 'empty' pr commit (the key is an empty string, because you can't put a null key in json maps).", default='{}') link_warned = fields.Boolean( default=False, help="Whether we've already warned that this (ready)" @@ -961,12 +962,16 @@ class PullRequests(models.Model): # on top of target msg = self._build_merge_message(commits[-1]['commit']['message']) commits[-1]['commit']['message'] = msg - return gh.rebase(self.number, target, commits=commits) + head, mapping = gh.rebase(self.number, target, commits=commits) + self.commits_map = json.dumps({**mapping, '': head}) + return head def _stage_rebase_merge(self, gh, target, commits): msg = self._build_merge_message(self.message) - h = gh.rebase(self.number, target, reset=True, commits=commits) - return gh.merge(h, target, msg)['sha'] + h, mapping = gh.rebase(self.number, target, reset=True, commits=commits) + merge_head = gh.merge(h, target, msg)['sha'] + self.commits_map = json.dumps({**mapping, '': merge_head}) + return merge_head def _stage_merge(self, gh, target, commits): pr_head = commits[-1] # oldest to newest @@ -981,6 +986,7 @@ class PullRequests(models.Model): if len(merge) == 1: [base_commit] = merge + commits_map = {c['sha']: c['sha'] for c in commits} if base_commit: # replicate pr_head with base_commit replaced by # the current head @@ -996,11 +1002,18 @@ class PullRequests(models.Model): 'parents': new_parents, }).json() gh.set_ref(target, copy['sha']) + # merge commit *and old PR head* map to the pr head replica + commits_map[''] = commits_map[pr_head['sha']] = copy['sha'] + self.commits_map = json.dumps(commits_map) return copy['sha'] else: # otherwise do a regular merge msg = self._build_merge_message(self.message) - return gh.merge(self.head, target, msg)['sha'] + merge_head = gh.merge(self.head, target, msg)['sha'] + # and the merge commit is the normal merge head + commits_map[''] = merge_head + self.commits_map = json.dumps(commits_map) + return merge_head # state changes on reviews RPLUS = { @@ -1363,7 +1376,7 @@ class Stagings(models.Model): self.env['runbot_merge.pull_requests.feedback'].create({ 'repository': pr.repository.id, 'pull_request': pr.number, - 'message': "Merged, thanks!", + 'message': "Merged at %s, thanks!" % json.loads(pr.commits_map)[''], 'close': True, }) finally: diff --git a/runbot_merge/tests/test_basic.py b/runbot_merge/tests/test_basic.py index 04b560d1..b3efd92a 100644 --- a/runbot_merge/tests/test_basic.py +++ b/runbot_merge/tests/test_basic.py @@ -975,11 +975,16 @@ class TestMergeMethod: repo.post_status(staging.id, 'success', 'legal/cla') repo.post_status(staging.id, 'success', 'ci/runbot') run_crons(env) - assert env['runbot_merge.pull_requests'].search([ + pr = env['runbot_merge.pull_requests'].search([ ('repository.name', '=', repo.name), ('number', '=', prx.number) - ]).state == 'merged' + ]) + assert pr.state == 'merged' assert prx.state == 'closed' + assert json.loads(pr.commits_map) == { + c1: actual_sha, + '': actual_sha, + }, "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): """ @@ -1157,16 +1162,26 @@ class TestMergeMethod: repo.post_status('heads/staging.master', 'success', 'ci/runbot') run_crons(env) - assert env['runbot_merge.pull_requests'].search([ + pr = env['runbot_merge.pull_requests'].search([ ('repository.name', '=', repo.name), ('number', '=', prx.number), - ]).state == 'merged' + ]) + assert pr.state == 'merged' # check that the dummy commit is not in the final master master = log_to_node(repo.log('heads/master')) assert master == merge_head - final_tree = repo.read_tree(repo.commit('heads/master')) + head = repo.commit('heads/master') + final_tree = repo.read_tree(head) assert final_tree == {'m': b'2', 'b': b'1'}, "sanity check of final tree" + r1 = repo.commit(head.parents[1]) + r0 = repo.commit(r1.parents[0]) + assert json.loads(pr.commits_map) == { + b0: r0.id, + b1: r1.id, + '': head.id, + } + assert r0.parents == [m2] def test_pr_rebase_ff(self, repo, env, users): """ test result on rebase-merge @@ -1220,17 +1235,28 @@ class TestMergeMethod: repo.post_status('heads/staging.master', 'success', 'ci/runbot') run_crons(env) - assert env['runbot_merge.pull_requests'].search([ + pr = env['runbot_merge.pull_requests'].search([ ('repository.name', '=', repo.name), ('number', '=', prx.number), - ]).state == 'merged' + ]) + assert pr.state == 'merged' # check that the dummy commit is not in the final master master = log_to_node(repo.log('heads/master')) assert master == nb1 - final_tree = repo.read_tree(repo.commit('heads/master')) + head = repo.commit('heads/master') + final_tree = repo.read_tree(head) assert final_tree == {'m': b'2', 'b': b'1'}, "sanity check of final tree" + m1 = head + m0 = repo.commit(m1.parents[0]) + assert json.loads(pr.commits_map) == { + '': m1.id, # merge commit + b1: m1.id, # second PR's commit + b0: m0.id, # first PR's commit + } + assert m0.parents == [m2], "can't hurt to check the parent of our root commit" + @pytest.mark.skip(reason="what do if the PR contains merge commits???") def test_pr_contains_merges(self, repo, env): pass @@ -1272,6 +1298,14 @@ class TestMergeMethod: expected = node('gibberish\n\nblahblah\n\ncloses {}#{}' '\n\nSigned-off-by: {}'.format(repo.name, prx.number, reviewer), m, c0) assert log_to_node(repo.log('heads/master')), expected + pr = env['runbot_merge.pull_requests'].search([ + ('repository.name', '=', repo.name), + ('number', '=', prx.number), + ]) + assert json.loads(pr.commits_map) == { + prx.head: prx.head, + '': master.id + } def test_unrebase_emptymessage(self, repo, env, users): """ When merging between master branches (e.g. forward port), the PR