Fix implied branch matchers and override-checkout

When specifying job.override-checkout, we apply job variants from
that match the specified branch.  The mechanism we use to do that
is to create a synthetic Ref object to pass to the branch matcher
instead of the real branch of the Change (since the real branch
is likely different -- that's why override-checkout was specified).
However, branch matching behavior has gottes slightly more
sophisticated and Ref objects don't match in the same way that
Change objects do.

In particular, implied branch matchers match Ref subclasses that
have a branch attribute iff the match is exact.

This means that if a user constructed two branches:

* testbranch
* testbranch2

and used override-checkout to favor a job definition from testbranch2,
the implied branch matcher for the variant in testbranch would match
since the matcher behaved as if it were matching a Ref not a Change
or Branch.

To correct this, we update the simulated change object used in the
override-checkout variant matching routine to be a Branch (which
unsurprisingly has a branch attribute) instead of a Ref.

The test test_implied_branch_matcher_similar_override_checkout is added
to cover this test case.  Additionally, the test
test_implied_branch_matcher_similar is added for good measure (it tests
implied branch matchers in the same way but without specifying
override-checkout), though its behavior is already correct.

A release note is included since this may have an effect on job behavior.

Change-Id: I1104eaf02f752e8a73e9b04939f03a4888763b27

3 files changed, 169 insertions, 4 deletions

diff --git a/releasenotes/notes/override-checkout-implied-branches-29fc5b4e6226739b.yaml b/releasenotes/notes/override-checkout-implied-branches-29fc5b4e6226739b.yaml
new file mode 100644
index 000000000..df79162a1
--- /dev/null
+++ b/releasenotes/notes/override-checkout-implied-branches-29fc5b4e6226739b.yaml
@@ -0,0 +1,13 @@
+---
+fixes:
+  - |
+    The use of implied branch matchers in jobs where
+    `override-checkout` was specified could cause some jobs to include
+    unintended variants.  Specifically: job variants with implied
+    branch matchers on branches which are substring matches of a
+    branch specified in the `override-checkout` job attribute may have
+    been used when not intended.
+
+    This has been corrected so that the same job variant matching
+    process happens whether the change's branch or the branch
+    specified by override-checkout is used.
diff --git a/tests/unit/test_v3.py b/tests/unit/test_v3.py
index 225c6d355..674310a0c 100644
--- a/tests/unit/test_v3.py
+++ b/tests/unit/test_v3.py
@@ -923,6 +923,159 @@ class TestBranchMismatch(ZuulTestCase):
         self.assertIn('nodeset "bar" was not found', A.messages[0],
                       "A should have a syntax error reported")
 
+    def _updateConfig(self, config, branch):
+        file_dict = {'zuul.yaml': config}
+        C = self.fake_gerrit.addFakeChange('org/project1', branch, 'C',
+                                           files=file_dict)
+        C.setMerged()
+        self.fake_gerrit.addEvent(C.getChangeMergedEvent())
+        self.waitUntilSettled()
+
+    def test_implied_branch_matcher_similar(self):
+        # Test that we perform a full-text match with implied branch
+        # matchers.
+        self.create_branch('org/project1', 'testbranch')
+        self.create_branch('org/project1', 'testbranch2')
+        self.fake_gerrit.addEvent(
+            self.fake_gerrit.getFakeBranchCreatedEvent(
+                'org/project1', 'testbranch'))
+        self.fake_gerrit.addEvent(
+            self.fake_gerrit.getFakeBranchCreatedEvent(
+                'org/project1', 'testbranch2'))
+
+        config = textwrap.dedent(
+            """
+            - job:
+                name: testjob
+                vars:
+                  this_branch: testbranch
+                  testbranch: true
+            - project:
+                check: {jobs: [testjob]}
+            """)
+        self._updateConfig(config, 'testbranch')
+        config = textwrap.dedent(
+            """
+            - job:
+                name: testjob
+                vars:
+                  this_branch: testbranch2
+                  testbranch2: true
+            - project:
+                check: {jobs: [testjob]}
+            """)
+        self._updateConfig(config, 'testbranch2')
+
+        self.executor_server.hold_jobs_in_build = True
+        A = self.fake_gerrit.addFakeChange(
+            'org/project1', 'testbranch', 'A')
+        self.fake_gerrit.addEvent(A.getPatchsetCreatedEvent(1))
+        self.waitUntilSettled()
+
+        self.assertEqual(
+            {'testbranch': True, 'this_branch': 'testbranch'},
+            self.builds[0].job.combined_variables)
+
+        self.executor_server.release()
+        self.waitUntilSettled()
+
+        B = self.fake_gerrit.addFakeChange(
+            'org/project1', 'testbranch2', 'B')
+        self.fake_gerrit.addEvent(B.getPatchsetCreatedEvent(1))
+        self.waitUntilSettled()
+
+        # The two jobs should have distinct variables.  Notably,
+        # testbranch2 should not pick up vars from testbranch.
+        self.assertEqual(
+            {'testbranch2': True, 'this_branch': 'testbranch2'},
+            self.builds[0].job.combined_variables)
+
+        self.executor_server.hold_jobs_in_build = False
+        self.executor_server.release()
+        self.waitUntilSettled()
+
+        self.assertHistory([
+            dict(name='testjob', result='SUCCESS', changes='3,1'),
+            dict(name='testjob', result='SUCCESS', changes='4,1'),
+        ], ordered=False)
+
+    def test_implied_branch_matcher_similar_override_checkout(self):
+        # Overriding a checkout has some branch matching implications.
+        # Make sure that we are performing a full-text match on
+        # branches when we override a checkout.
+        self.create_branch('org/project1', 'testbranch')
+        self.create_branch('org/project1', 'testbranch2')
+        self.fake_gerrit.addEvent(
+            self.fake_gerrit.getFakeBranchCreatedEvent(
+                'org/project1', 'testbranch'))
+        self.fake_gerrit.addEvent(
+            self.fake_gerrit.getFakeBranchCreatedEvent(
+                'org/project1', 'testbranch2'))
+        config = textwrap.dedent(
+            """
+            - job:
+                name: testjob
+                vars:
+                  this_branch: testbranch
+                  testbranch: true
+            - project:
+                check: {jobs: [testjob]}
+            """)
+        self._updateConfig(config, 'testbranch')
+        config = textwrap.dedent(
+            """
+            - job:
+                name: testjob
+                vars:
+                  this_branch: testbranch2
+                  testbranch2: true
+            - project:
+                check: {jobs: [testjob]}
+            """)
+        self._updateConfig(config, 'testbranch2')
+
+        self.executor_server.hold_jobs_in_build = True
+        config = textwrap.dedent(
+            """
+            - job:
+                name: project-test1
+            - job:
+                name: testjob-testbranch
+                parent: testjob
+                override-checkout: testbranch
+            - job:
+                name: testjob-testbranch2
+                parent: testjob
+                override-checkout: testbranch2
+            - project:
+                check: {jobs: [testjob-testbranch, testjob-testbranch2]}
+            """)
+        file_dict = {'zuul.yaml': config}
+        A = self.fake_gerrit.addFakeChange(
+            'org/project1', 'master', 'A', files=file_dict)
+        self.fake_gerrit.addEvent(A.getPatchsetCreatedEvent(1))
+        self.waitUntilSettled()
+
+        self.assertEqual(
+            {'testbranch': True, 'this_branch': 'testbranch'},
+            self.builds[0].job.combined_variables)
+
+        # The two jobs should have distinct variables (notably, the
+        # variant on testbranch2 should not pick up vars from
+        # testbranch.
+        self.assertEqual(
+            {'testbranch2': True, 'this_branch': 'testbranch2'},
+            self.builds[1].job.combined_variables)
+
+        self.executor_server.hold_jobs_in_build = False
+        self.executor_server.release()
+        self.waitUntilSettled()
+
+        self.assertHistory([
+            dict(name='testjob-testbranch', result='SUCCESS', changes='3,1'),
+            dict(name='testjob-testbranch2', result='SUCCESS', changes='3,1'),
+        ], ordered=False)
+
 
 class TestBranchRef(ZuulTestCase):
     tenant_config_file = 'config/branch-ref/main.yaml'
diff --git a/zuul/model.py b/zuul/model.py
index b1fc88fb0..e053d51d8 100644
--- a/zuul/model.py
+++ b/zuul/model.py
@@ -3135,10 +3135,9 @@ class Job(ConfigObject):
             branch_change = change
         else:
             # If an override branch is supplied, create a very basic
-            # change (a Ref) and set its branch to the override
-            # branch.
-            branch_change = Ref(change.project)
-            branch_change.ref = override_branch
+            # change and set its branch to the override branch.
+            branch_change = Branch(change.project)
+            branch_change.branch = override_branch
 
         if self.branch_matcher and not self.branch_matcher.matches(
                 branch_change):