[IMP] forwardport: on conflict note previous FP can be r+'d

Closes #294

forwardport/models/project.py

@@ -36,6 +36,8 @@ from odoo.tools.appdirs import user_cache_dir
 from odoo.addons.runbot_merge import utils
 from odoo.addons.runbot_merge.models.pull_requests import RPLUS
 
+FOOTER = '\nMore info at https://github.com/odoo/odoo/wiki/Mergebot#forward-port\n'
+
 DEFAULT_DELTA = dateutil.relativedelta.relativedelta(days=3)
 
 _logger = logging.getLogger('odoo.addons.forwardport')
@@ -638,6 +640,18 @@ class PullRequests(models.Model):
                 # copy all delegates of source to new
                 'delegates': [(6, False, source.delegates.ids)]
             })
+            if has_conflicts and pr.parent_id and pr.state not in ('merged', 'closed'):
+                message = source._pingline() + """
+The next pull request (%s) is in conflict. You can merge the chain up to here by saying
+> @%s r+
+%s
+""" % (new_pr.display_name, pr.repository.project_id.fp_github_name, FOOTER)
+                self.env['runbot_merge.pull_requests.feedback'].create({
+                    'repository': pr.repository.id,
+                    'pull_request': pr.number,
+                    'message': message,
+                    'token_field': 'fp_github_token',
+                })
             # not great but we probably want to avoid the risk of the webhook
             # creating the PR from under us. There's still a "hole" between
             # the POST being executed on gh and the commit but...
@@ -647,7 +661,7 @@ class PullRequests(models.Model):
             source = pr.source_id or pr
             (h, out, err) = conflicts.get(pr) or (None, None, None)
 
-            footer = '\nMore info at https://github.com/odoo/odoo/wiki/Mergebot#forward-port\n'
+            footer = FOOTER
             if has_conflicts and not h:
                 footer = '\n**WARNING** at least one co-dependent PR (%s) ' \
                          'did not properly forward-port, you will need to ' \

forwardport/tests/test_simple.py

@@ -370,7 +370,7 @@ 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-and-Forwardbot#forward-port
+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']
@@ -471,117 +471,144 @@ More info at https://github.com/odoo/odoo/wiki/Mergebot#forward-port
 You may want or need to manually update any followup PR.""" % pr1.display_name)
     ]
 
-def test_conflict(env, config, make_repo):
+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)
-    # reset b to b~1 (g=a) parent so there's no b -> c conflict
+    # create a d branch
     with prod:
-        prod.update_ref('heads/b', prod.commit('b').parents[0], force=True)
+        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 g file in a PR to a
+    # generate a conflict: create a h file in a PR to a
     with prod:
         [p_0] = prod.make_commits(
-            'a', Commit('p_0', tree={'g': 'xxx'}),
+            '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()
-    # wait a bit for PR webhook... ?
+    pra_id, prb_id = env['runbot_merge.pull_requests'].search([], order='number')
-    time.sleep(5)
+    # 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
-    pr0, pr1 = env['runbot_merge.pull_requests'].search([], order='number')
     # but it should not have a parent, and there should be conflict markers
-    assert not pr1.parent_id
+    assert not prc_id.parent_id
-    assert pr1.source_id == pr0
+    assert prc_id.source_id == pra_id
-    assert prod.read_tree(prod.commit('b')) == {
+    assert prc_id.state == 'opened'
-        'f': 'c',
+
-        'g': 'a',
-    }
-    assert pr1.state == 'opened'
     p = prod.commit(p_0)
-    c = prod.commit(pr1.head)
+    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': re_matches(r'''<<<\x3c<<< HEAD
+        'g': 'a',
+        'h': re_matches(r'''<<<\x3c<<< HEAD
 a
 =======
 xxx
 >>>\x3e>>> [0-9a-f]{7,}(...)? temp
 '''),
     }
+    assert prod.get_pr(prb_id.number).comments == [
+        (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], [pr1.head])
+        validate_all([prod], [prc_id.head])
     env.run_crons()
     time.sleep(5)
     env.run_crons()
-    assert pr0 | pr1 == env['runbot_merge.pull_requests'].search([], order='number'),\
+    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
-    get_pr = prod.get_pr(pr1.number)
+    prc = prod.get_pr(prc_id.number)
-    pr_repo, pr_ref = get_pr.branch
+    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
-            prod.commit('b').id,
+            'c',
-            Commit('g should indeed b xxx', tree={'g': 'xxx'}),
+            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(pr1.head)) == {
+    assert prod.read_tree(prod.commit(prc_id.head)) == {
         'f': 'c',
-        'g': 'xxx',
+        'g': 'a',
+        'h': 'xxx',
     }
-    assert pr1.state == 'opened', "state should be open still"
+    assert prc_id.state == 'opened', "state should be open still"
-    assert ('#%d' % pr.number) in pr1.message
+    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(pr1.head, 'success', 'legal/cla')
+        prod.post_status(prc.head, 'success', 'legal/cla')
-        prod.post_status(pr1.head, 'success', 'ci/runbot')
+        prod.post_status(prc.head, 'success', 'ci/runbot')
-        get_pr.post_comment('hansen r+', config['role_reviewer']['token'])
+        prc.post_comment('hansen r+', config['role_reviewer']['token'])
     env.run_crons()
 
-    assert pr1.staging_id
+    assert prc_id.staging_id
     with prod:
-        prod.post_status('staging.b', 'success', 'legal/cla')
+        prod.post_status('staging.c', 'success', 'legal/cla')
-        prod.post_status('staging.b', 'success', 'ci/runbot')
+        prod.post_status('staging.c', 'success', 'ci/runbot')
     env.run_crons()
 
-    *_, pr2 = env['runbot_merge.pull_requests'].search([], order='number')
+    *_, prd_id = env['runbot_merge.pull_requests'].search([], order='number')
-    assert ('#%d' % pr.number) in pr2.message, \
+    assert ('#%d' % pra_id.number) in prd_id.message, \
-        "check that source / pr0 is referenced by resume PR"
+        "check that source / PR A is referenced by resume PR"
-    assert ('#%d' % pr1.number) in pr2.message, \
+    assert ('#%d' % prc_id.number) in prd_id.message, \
-        "check that parent / pr1 is referenced by resume PR"
+        "check that parent / PR C is referenced by resume PR"
-    assert pr2.parent_id == pr1
+    assert prd_id.parent_id == prc_id
-    assert pr2.source_id == pr0
+    assert prd_id.source_id == pra_id
     assert re.match(
-        REF_PATTERN.format(target='c', source='conflicting'),
+        REF_PATTERN.format(target='d', source='conflicting'),
-        pr2.refname
+        prd_id.refname
     )
-    assert prod.read_tree(prod.commit(pr2.head)) == {
+    assert prod.read_tree(prod.commit(prd_id.head)) == {
         'f': 'c',
-        'g': 'xxx',
+        'g': 'a',
-        'h': 'a',
+        'h': 'xxx',
+        'i': 'a',
     }
 
 def test_conflict_deleted(env, config, make_repo):

forwardport/tests/utils.py

@@ -39,11 +39,11 @@ class re_matches:
 def make_basic(env, config, make_repo, *, reponame='proj', project_name='myproject'):
     """ Creates a basic repo with 3 forking branches
 
-    0 -- 1 -- 2 -- 3 -- 4  : a
+    f = 0 -- 1 -- 2 -- 3 -- 4  : a
-              |
-              `-- 11 -- 22 : b
                   |
-                  `-- 111  : c
+    g =           `-- 11 -- 22 : b
+                     |
+    h =               `-- 111  : c
     each branch just adds and modifies a file (resp. f, g and h) through the
     contents sequence a b c d e
     """
@@ -56,9 +56,9 @@ def make_basic(env, config, make_repo, *, reponame='proj', project_name='myproje
             'github_prefix': 'hansen',
             'fp_github_token': config['github']['token'],
             'branch_ids': [
-                (0, 0, {'name': 'a', 'fp_sequence': 2, 'fp_target': True}),
+                (0, 0, {'name': 'a', 'fp_sequence': 10, 'fp_target': True}),
-                (0, 0, {'name': 'b', 'fp_sequence': 1, 'fp_target': True}),
+                (0, 0, {'name': 'b', 'fp_sequence': 8, 'fp_target': True}),
-                (0, 0, {'name': 'c', 'fp_sequence': 0, 'fp_target': True}),
+                (0, 0, {'name': 'c', 'fp_sequence': 6, 'fp_target': True}),
             ],
         })
 

runbot_merge/models/pull_requests.py

@@ -2,6 +2,7 @@ import ast
 import base64
 import collections
 import datetime
+import functools
 import io
 import itertools
 import json
@@ -102,10 +103,18 @@ class Project(models.Model):
             if not gh:
                 gh = ghs[repo] = repo.github()
 
-            remove = set().union(*remove)
+            # fold all grouped PRs'
-            add = set().union(*add)
+            tags_remove, tags_add = set(), set()
+            for minus, plus in zip(remove, add):
+                tags_remove.update(minus)
+                # need to remove minuses from to_add in case we get e.g.
+                # -foo +bar; -bar +baz, if we don't remove the minus, we'll end
+                # up with -foo +bar +baz instead of -foo +baz
+                tags_add.difference_update(minus)
+                tags_add.update(plus)
+
             try:
-                gh.change_tags(pr, remove, add)
+                gh.change_tags(pr, tags_remove, tags_add)
             except Exception:
                 _logger.exception(
                     "Error while trying to change the tags of %s#%s from %s to %s",
@@ -1347,11 +1356,11 @@ class Tagging(models.Model):
     tags_add = fields.Char(required=True, defualt='[]')
 
     def create(self, values):
-        values.pop('state_from', None)
+        before = str(values)
-        state_to = values.pop('state_to', None)
+        if values.pop('state_from', None):
-        if state_to:
             values['tags_remove'] = ALL_TAGS
-            values['tags_add'] = _TAGS[state_to]
+        if 'state_to' in values:
+            values['tags_add'] = _TAGS[values.pop('state_to')]
         if not isinstance(values.get('tags_remove', ''), str):
             values['tags_remove'] = json.dumps(list(values['tags_remove']))
         if not isinstance(values.get('tags_add', ''), str):