diff --git a/forwardport/models/forwardport.py b/forwardport/models/forwardport.py index be81981d..8caaca33 100644 --- a/forwardport/models/forwardport.py +++ b/forwardport/models/forwardport.py @@ -79,6 +79,15 @@ class BatchQueue(models.Model, Queue): ) batch.active = False + +CONFLICT_TEMPLATE = "WARNING: the latest change ({previous.head}) triggered " \ + "a conflict when updating the next forward-port " \ + "({next.display_name}), and has been ignored.\n\n" \ + "You will need to update this pull request differently, " \ + "or fix the issue by hand on {next.display_name}." +CHILD_CONFLICT = "WARNING: the update of {previous.display_name} to " \ + "{previous.head} has caused a conflict in this pull request, " \ + "data may have been lost." class UpdateQueue(models.Model, Queue): _name = 'forwardport.updates' _description = 'if a forward-port PR gets updated & has followups (cherrypick succeeded) the followups need to be updated as well' @@ -89,6 +98,7 @@ class UpdateQueue(models.Model, Queue): new_root = fields.Many2one('runbot_merge.pull_requests') def _process_item(self): + Feedback = self.env['runbot_merge.pull_requests.feedback'] previous = self.new_root with ExitStack() as s: for child in self.new_root._iter_descendants(): @@ -100,7 +110,7 @@ class UpdateQueue(models.Model, Queue): self.new_root.display_name ) if child.state in ('closed', 'merged'): - self.env['runbot_merge.pull_requests.feedback'].create({ + Feedback.create({ 'repository': child.repository.id, 'pull_request': child.number, 'message': "Ancestor PR %s has been updated but this PR" @@ -108,14 +118,31 @@ class UpdateQueue(models.Model, Queue): "\n\n" "You may want or need to manually update any" " followup PR." % ( - self.new_root.display_name, - child.state, - ) + self.new_root.display_name, + child.state, + ) }) return # QUESTION: update PR to draft if there are conflicts? - _, working_copy = previous._create_fp_branch( + conflicts, working_copy = previous._create_fp_branch( child.target, child.refname, s) + if conflicts: + _, out, err = conflicts + Feedback.create({ + 'repository': previous.repository.id, + 'pull_request': previous.number, + 'message': CONFLICT_TEMPLATE.format( + previous=previous, + next=child + ) + }) + Feedback.create({ + 'repository': child.repository.id, + 'pull_request': child.number, + 'message': CHILD_CONFLICT.format(previous=previous, next=child)\ + + (f'\n\nstdout:\n```\n{out.strip()}\n```' if out.strip() else '') + + (f'\n\nstderr:\n```\n{err.strip()}\n```' if err.strip() else '') + }) new_head = working_copy.stdout().rev_parse(child.refname).stdout.decode().strip() commits_count = int(working_copy.stdout().rev_list( diff --git a/forwardport/tests/test_updates.py b/forwardport/tests/test_updates.py index bea30fa8..5c5f60d9 100644 --- a/forwardport/tests/test_updates.py +++ b/forwardport/tests/test_updates.py @@ -2,11 +2,13 @@ Test cases for updating PRs during after the forward-porting process after the initial merge has succeeded (and forward-porting has started) """ +import re import sys import pytest -from utils import seen, re_matches, Commit, make_basic +from utils import seen, re_matches, Commit, make_basic, to_pr + def test_update_pr(env, config, make_repo, users): """ Even for successful cherrypicks, it's possible that e.g. CI doesn't @@ -119,57 +121,6 @@ More info at https://github.com/odoo/odoo/wiki/Mergebot#forward-port 'x': '5' }, "the followup FP should also have the update" - with pr_repo: - pr_repo.make_commits( - pr1_id.target.name, - Commit('fire!', tree={'h': '0'}), - ref='heads/%s' % pr_ref, - ) - env.run_crons() - # since there are PRs, this is going to update pr2 as broken - assert prod.read_tree(prod.commit(pr1_id.head)) == { - 'f': 'c', - 'g': 'b', - 'h': '0' - } - assert prod.read_tree(prod.commit(pr2_id.head)) == { - 'f': 'c', - 'g': 'a', - 'h': re_matches(r'''<<<\x3c<<< HEAD -a -======= -0 ->>>\x3e>>> [0-9a-f]{7,}.* -'''), - } - [project] = env['runbot_merge.project'].search([]) - pr2 = prod.get_pr(pr2_id.number) - # fail pr2 then fwbot r+ to check that we get a warning - with prod: - prod.post_status(pr2_id.head, 'failure', 'ci/runbot') - env.run_crons() # parse commit statuses - with prod: - pr2.post_comment(project.fp_github_name + ' r+', config['role_reviewer']['token']) - env.run_crons() # send feedback - - assert pr2.comments == [ - seen(env, pr2, users), - (users['user'], """Ping @{}, @{} -This PR targets c and is the last of the forward-port chain containing: -* {} - -To merge the full chain, say -> @{} r+ - -More info at https://github.com/odoo/odoo/wiki/Mergebot#forward-port -""".format(users['user'], users['reviewer'], pr1_id.display_name, project.fp_github_name)), - (users['user'], 'Ping @{}, @{}\n\nci/runbot failed on this forward-port PR'.format( - users['user'], users['reviewer'] - )), - (users['reviewer'], project.fp_github_name + ' r+'), - (users['user'], '@{}, you may want to rebuild or fix this PR as it has failed CI.'.format(users['reviewer'])), - ] - def test_update_merged(env, make_repo, config, users): """ Strange things happen when an FP gets closed / merged but then its parent is modified and the forwardport tries to update the (now merged) @@ -365,3 +316,92 @@ def test_duplicate_fw(env, make_repo, setreviewers, config, users): env.run_crons() # env.run_crons() assert PRs.search([], order='number') == pr_ids + +def test_subsequent_conflict(env, make_repo, config, users): + """ Test for updating an fw PR in the case where it produces a conflict in + the followup. Cf #467. + """ + repo, fork = make_basic(env, config, make_repo) + + # create a PR in branch A which adds a new file + with repo: + repo.make_commits('a', Commit('newfile', tree={'x': '0'}), ref='heads/pr1') + pr_1 = repo.make_pr(target='a', head='pr1') + repo.post_status('pr1', 'success', 'legal/cla') + repo.post_status('pr1', 'success', 'ci/runbot') + pr_1.post_comment('hansen r+', config['role_reviewer']['token']) + env.run_crons() + with repo: + repo.post_status('staging.a', 'success', 'legal/cla') + repo.post_status('staging.a', 'success', 'ci/runbot') + env.run_crons() + pr1_id = to_pr(env, pr_1) + assert pr1_id.state == 'merged' + + pr2_id = env['runbot_merge.pull_requests'].search([('source_id', '=', pr1_id.id)]) + assert pr2_id + with repo: + repo.post_status(pr2_id.head, 'success', 'legal/cla') + repo.post_status(pr2_id.head, 'success', 'ci/runbot') + env.run_crons() + + pr3_id = env['runbot_merge.pull_requests'].search([('parent_id', '=', pr2_id.id)]) + assert pr3_id + assert repo.read_tree(repo.commit(pr3_id.head)) == { + 'f': 'c', + 'g': 'a', + 'h': 'a', + 'x': '0', + } + + # update pr2: add a file "h" + pr2 = repo.get_pr(pr2_id.number) + t = {**repo.read_tree(repo.commit(pr2_id.head)), 'h': 'conflict!'} + with fork: + fork.make_commits(pr2_id.target.name, Commit('newfiles', tree=t), ref=pr2.ref, make=False) + env.run_crons() + + assert repo.read_tree(repo.commit(pr3_id.head)) == { + 'f': 'c', + 'g': 'a', + 'h': re_matches(r'''<<<\x3c<<< HEAD +a +======= +conflict! +>>>\x3e>>> [0-9a-f]{7,}.* +'''), + 'x': '0', + } + # skip comments: + # 1. link to mergebot status page + # 2. "forward port chain" bit + # 3. updated / modified & got detached + assert pr2.comments[3:] == [ + (users['user'], f"WARNING: the latest change ({pr2_id.head}) triggered " + f"a conflict when updating the next forward-port " + f"({pr3_id.display_name}), and has been ignored.\n\n" + f"You will need to update this pull request " + f"differently, or fix the issue by hand on " + f"{pr3_id.display_name}.") + ] + # skip comments: + # 1. link to status page + # 2. forward-port chain thing + assert repo.get_pr(pr3_id.number).comments[2:] == [ + (users['user'], re_matches(f'''\ +WARNING: the update of {pr2_id.display_name} to {pr2_id.head} has caused a \ +conflict in this pull request, data may have been lost. + +stdout: +``` +CONFLICT \(add/add\): Merge conflict in h +Auto-merging h +``` + +stderr: +``` +\\d{{2}}:\\d{{2}}:\\d{{2}}.\\d+ .* {pr2_id.head} +error: could not apply [0-9a-f]+\\.\\.\\. newfiles +hint: after resolving the conflicts, mark the corrected paths +.*''', re.DOTALL)) + ]