Fix dependency cycle false positive

This corrects a false-positive in the dependency cycle detection,
but only for the new URL-style depends-on headers.  It does not
do so for the legacy gerrit headers.

We used a single history list to store all the changes we enqueued
ahead of a given change, but this meant that if there was more
than one path to a change, we would see it in the history on the
second traversal.  Instead, when traversing the tree, use copies
of the history list at each stage so that it can be rewound when
going back up the tree.  The second path to a change will not
trip the cycle detection, and will proceed on to the point where
it notices the change is already in the queue and return harmlessly.

Also, check whether the exact change is in the history, not just
the number, since numbers are no longer unique with multiple sources.

Also, fix a bug in the test_crd_cycle test which was causing the
test to always pass since the changes were never enqueued due to
missing approval requirements.

Change-Id: I3241f90a1d7469d433cfa176e719322203d4d089
Story: 2001427
Task: 6133

4 files changed, 67 insertions, 4 deletions

diff --git a/tests/unit/test_gerrit_crd.py b/tests/unit/test_gerrit_crd.py
index 732bc3d60..a8924b902 100644
--- a/tests/unit/test_gerrit_crd.py
+++ b/tests/unit/test_gerrit_crd.py
@@ -90,6 +90,40 @@ class TestGerritCRD(ZuulTestCase):
             'project-merge', 'org/project1').changes
         self.assertEqual(changes, '2,1 1,1')
 
+    def test_crd_gate_triangle(self):
+        A = self.fake_gerrit.addFakeChange('org/project1', 'master', 'A')
+        B = self.fake_gerrit.addFakeChange('org/project2', 'master', 'B')
+        C = self.fake_gerrit.addFakeChange('org/project2', 'master', 'C')
+        A.addApproval('Code-Review', 2)
+        B.addApproval('Code-Review', 2)
+        C.addApproval('Code-Review', 2)
+        A.addApproval('Approved', 1)
+        B.addApproval('Approved', 1)
+
+        # C-->B
+        #  \ /
+        #   v
+        #   A
+
+        # C Depends-On: A
+        C.data['commitMessage'] = '%s\n\nDepends-On: %s\n' % (
+            C.subject, A.data['url'])
+        # B Depends-On: A
+        B.data['commitMessage'] = '%s\n\nDepends-On: %s\n' % (
+            B.subject, A.data['url'])
+        # C git-depends on B
+        C.setDependsOn(B, 1)
+        self.fake_gerrit.addEvent(C.addApproval('Approved', 1))
+        self.waitUntilSettled()
+
+        self.assertEqual(A.reported, 2)
+        self.assertEqual(B.reported, 2)
+        self.assertEqual(C.reported, 2)
+        self.assertEqual(A.data['status'], 'MERGED')
+        self.assertEqual(B.data['status'], 'MERGED')
+        self.assertEqual(C.data['status'], 'MERGED')
+        self.assertEqual(self.history[-1].changes, '1,1 2,1 3,1')
+
     def test_crd_branch(self):
         "Test cross-repo dependencies in multiple branches"
 
@@ -257,6 +291,7 @@ class TestGerritCRD(ZuulTestCase):
         B = self.fake_gerrit.addFakeChange('org/project2', 'master', 'B')
         A.addApproval('Code-Review', 2)
         B.addApproval('Code-Review', 2)
+        B.addApproval('Approved', 1)
 
         # A -> B -> A (via commit-depends)
 
@@ -522,6 +557,30 @@ class TestGerritCRD(ZuulTestCase):
         for job in self.history:
             self.assertEqual(len(job.changes.split()), 1)
 
+    def test_crd_check_triangle(self):
+        A = self.fake_gerrit.addFakeChange('org/project1', 'master', 'A')
+        B = self.fake_gerrit.addFakeChange('org/project2', 'master', 'B')
+        C = self.fake_gerrit.addFakeChange('org/project2', 'master', 'C')
+
+        # C-->B
+        #  \ /
+        #   v
+        #   A
+
+        # C Depends-On: A
+        C.data['commitMessage'] = '%s\n\nDepends-On: %s\n' % (
+            C.subject, A.data['url'])
+        # B Depends-On: A
+        B.data['commitMessage'] = '%s\n\nDepends-On: %s\n' % (
+            B.subject, A.data['url'])
+        # C git-depends on B
+        C.setDependsOn(B, 1)
+        self.fake_gerrit.addEvent(C.getPatchsetCreatedEvent(1))
+        self.waitUntilSettled()
+
+        self.assertEqual(C.reported, 1)
+        self.assertEqual(self.history[0].changes, '1,1 2,1 3,1')
+
     @simple_layout('layouts/three-projects.yaml')
     def test_crd_check_transitive(self):
         "Test transitive cross-repo dependencies"
diff --git a/tests/unit/test_gerrit_legacy_crd.py b/tests/unit/test_gerrit_legacy_crd.py
index c711e4d95..90c93ecae 100644
--- a/tests/unit/test_gerrit_legacy_crd.py
+++ b/tests/unit/test_gerrit_legacy_crd.py
@@ -260,6 +260,7 @@ class TestGerritLegacyCRD(ZuulTestCase):
         B = self.fake_gerrit.addFakeChange('org/project2', 'master', 'B')
         A.addApproval('Code-Review', 2)
         B.addApproval('Code-Review', 2)
+        B.addApproval('Approved', 1)
 
         # A -> B -> A (via commit-depends)
 
diff --git a/zuul/manager/dependent.py b/zuul/manager/dependent.py
index 20b376d6a..5ad761196 100644
--- a/zuul/manager/dependent.py
+++ b/zuul/manager/dependent.py
@@ -141,13 +141,16 @@ class DependentPipelineManager(PipelineManager):
 
     def enqueueChangesAhead(self, change, quiet, ignore_requirements,
                             change_queue, history=None):
-        if history and change.number in history:
+        if history and change in history:
             # detected dependency cycle
             self.log.warn("Dependency cycle detected")
             return False
         if hasattr(change, 'number'):
             history = history or []
-            history.append(change.number)
+            history = history + [change]
+        else:
+            # Don't enqueue dependencies ahead of a non-change ref.
+            return True
 
         ret = self.checkForChangesNeededBy(change, change_queue)
         if ret in [True, False]:
diff --git a/zuul/manager/independent.py b/zuul/manager/independent.py
index 0c2baf010..9da40d582 100644
--- a/zuul/manager/independent.py
+++ b/zuul/manager/independent.py
@@ -34,13 +34,13 @@ class IndependentPipelineManager(PipelineManager):
 
     def enqueueChangesAhead(self, change, quiet, ignore_requirements,
                             change_queue, history=None):
-        if history and change.number in history:
+        if history and change in history:
             # detected dependency cycle
             self.log.warn("Dependency cycle detected")
             return False
         if hasattr(change, 'number'):
             history = history or []
-            history.append(change.number)
+            history = history + [change]
         else:
             # Don't enqueue dependencies ahead of a non-change ref.
             return True