From c93ce7c2c2f47145659e96957bb1df3ccc7fac72 Mon Sep 17 00:00:00 2001
From: Xavier Morel <xmo@odoo.com>
Date: Thu, 1 Oct 2020 11:49:47 +0200
Subject: [PATCH] [FIX] runbot_merge: blocked flag false positive

The "blocked" computation would not take branch targets in account, so
PRs with the same label targeting *different branches* (possible if
somewhat rare due to our naming conventions) could block one another,
despite really being unrelated.

Also fix up some messages:

* if a PR is blocked due to having no merge method, it should say
  that, not "has no merge" (no merge what?)
* format un-managed branches as `$repo:$branch` in logging messages,
  `$repo#$thing` is for issues / PRs and `$branch` alone can be very
  unhelpful

Closes #407
---
 runbot_merge/controllers/__init__.py |  2 +-
 runbot_merge/models/pull_requests.py |  6 ++--
 runbot_merge/tests/test_multirepo.py | 47 ++++++++++++++++++++++++++++
 3 files changed, 52 insertions(+), 3 deletions(-)

diff --git a/runbot_merge/controllers/__init__.py b/runbot_merge/controllers/__init__.py
index e41fc869..5d11313e 100644
--- a/runbot_merge/controllers/__init__.py
+++ b/runbot_merge/controllers/__init__.py
@@ -129,7 +129,7 @@ def handle_pr(env, event):
         return "Nothing to update ({})".format(event['changes'].keys())
 
     if not branch:
-        _logger.info("Ignoring PR for un-managed branch %s#%s", r, b)
+        _logger.info("Ignoring PR for un-managed branch %s:%s", r, b)
         return "Not set up to care about {}:{}".format(r, b)
 
     author_name = pr['user']['login']
diff --git a/runbot_merge/models/pull_requests.py b/runbot_merge/models/pull_requests.py
index aaf7e281..da1f23c7 100644
--- a/runbot_merge/models/pull_requests.py
+++ b/runbot_merge/models/pull_requests.py
@@ -288,7 +288,8 @@ All substitutions are tentatively applied sequentially to the input.
         issue, pr = gh.pr(number)
 
         if not self.project_id._has_branch(pr['base']['ref']):
-            _logger.info("Tasked with loading PR %d for un-managed branch %s, ignoring", pr['number'], pr['base']['ref'])
+            _logger.info("Tasked with loading PR %d for un-managed branch %s:%s, ignoring",
+                         pr['number'], self.name, pr['base']['ref'])
             self.env['runbot_merge.pull_requests.feedback'].create({
                 'repository': self.id,
                 'pull_request': number,
@@ -664,13 +665,14 @@ class PullRequests(models.Model):
             batch = pr
             if not re.search(r':patch-\d+', pr.label):
                 batch = self.search([
+                    ('target', '=', pr.target.id),
                     ('label', '=', pr.label),
                     ('state', 'not in', ('merged', 'closed')),
                 ])
 
             # check if PRs are configured (single commit or merge method set)
             if not (pr.squash or pr.merge_method):
-                pr.blocked = 'has no merge'
+                pr.blocked = 'has no merge method'
                 continue
             other_unset = next((p for p in batch if not (p.squash or p.merge_method)), None)
             if other_unset:
diff --git a/runbot_merge/tests/test_multirepo.py b/runbot_merge/tests/test_multirepo.py
index ae9856ab..a4f70614 100644
--- a/runbot_merge/tests/test_multirepo.py
+++ b/runbot_merge/tests/test_multirepo.py
@@ -6,6 +6,7 @@ When preparing a staging, we simply want to ensure branch-matched PRs
 are staged concurrently in all repos
 """
 import json
+import time
 
 import pytest
 import requests
@@ -153,6 +154,52 @@ def test_stage_match(env, project, repo_a, repo_b, config):
     assert 'Related: {}'.format(pr_b.display_name) in repo_a.commit('master').message
     assert 'Related: {}'.format(pr_a.display_name) in repo_b.commit('master').message
 
+def test_different_targets(env, project, repo_a, repo_b, config):
+    """ PRs with different targets should not be matched together
+    """
+    project.write({
+        'batch_limit': 1,
+        'branch_ids': [(0, 0, {'name': 'other'})]
+    })
+    with repo_a:
+        make_branch(repo_a, 'master', 'initial', {'master': 'a_0'})
+        make_branch(repo_a, 'other', 'initial', {'other': 'a_0'})
+        pr_a = make_pr(
+            repo_a, 'do-a-thing', [{'mater': 'a_1'}],
+            target='master',
+            user=config['role_user']['token'],
+            reviewer=config['role_reviewer']['token'],
+        )
+    with repo_b:
+        make_branch(repo_b, 'master', 'initial', {'master': 'b_0'})
+        make_branch(repo_b, 'other', 'initial', {'other': 'b_0'})
+        pr_b = make_pr(
+            repo_b, 'do-a-thing', [{'other': 'b_1'}],
+            target='other',
+            user=config['role_user']['token'],
+            reviewer=config['role_reviewer']['token'],
+            statuses=[],
+        )
+    time.sleep(5)
+    env.run_crons()
+
+    pr_a = to_pr(env, pr_a)
+    pr_b = to_pr(env, pr_b)
+    assert pr_a.state == 'ready'
+    assert not pr_a.blocked
+    assert pr_a.staging_id
+
+    assert pr_b.blocked
+    assert pr_b.state == 'approved'
+    assert not pr_b.staging_id
+
+    for r in [repo_a, repo_b]:
+        with r:
+            r.post_status('staging.master', 'success', 'legal/cla')
+            r.post_status('staging.master', 'success', 'ci/runbot')
+    env.run_crons()
+    assert pr_a.state == 'merged'
+
 def test_stage_different_statuses(env, project, repo_a, repo_b, config):
     project.batch_limit = 1