Fix change history cycle detection

We were short-circuiting loading some changes which appeared in
our dependent change query history because we were only looking
at change numbers, not change+patchset.  Record both in the history
to avoid this problem.

Note, this could likely be simplified further, but rather than doing
so now, I'll leave that for when we implement cross-source dependencies,
which is very likely to change this further.

This also corrects a defect in the fake gerrit change test fixture.

Change-Id: Ie1d9fe0dfd32032caf4ff8c90bded30d5e7bf886

tests/base.py

@@ -410,8 +410,8 @@ class FakeGerritChange(object):
         needed = other.data.get('neededBy', [])
         d = {'id': self.data['id'],
              'number': self.data['number'],
-             'ref': self.patchsets[patchset - 1]['ref'],
-             'revision': self.patchsets[patchset - 1]['revision']
+             'ref': self.patchsets[-1]['ref'],
+             'revision': self.patchsets[-1]['revision']
              }
         needed.append(d)
         other.data['neededBy'] = needed

tests/unit/test_scheduler.py

@@ -1178,6 +1178,34 @@ class TestScheduler(ZuulTestCase):
         repo.heads.master.commit = repo.commit('init')
         self.test_build_configuration()
 
+    def test_dependent_changes_rebase(self):
+        # Test that no errors occur when we walk a dependency tree
+        # with an unused leaf node due to a rebase.
+        # Start by constructing: C -> B -> A
+        A = self.fake_gerrit.addFakeChange('org/project', 'master', 'A')
+        B = self.fake_gerrit.addFakeChange('org/project', 'master', 'B')
+        B.setDependsOn(A, 1)
+
+        C = self.fake_gerrit.addFakeChange('org/project', 'master', 'C')
+        C.setDependsOn(B, 1)
+
+        # Then rebase to form: D -> C -> A
+        C.addPatchset()  # C,2
+        C.setDependsOn(A, 1)
+
+        D = self.fake_gerrit.addFakeChange('org/project', 'master', 'D')
+        D.setDependsOn(C, 2)
+
+        # Walk the entire tree
+        self.fake_gerrit.addEvent(A.getPatchsetCreatedEvent(1))
+        self.waitUntilSettled()
+        self.assertEqual(len(self.history), 3)
+
+        # Verify that walking just part of the tree still works
+        self.fake_gerrit.addEvent(D.getPatchsetCreatedEvent(1))
+        self.waitUntilSettled()
+        self.assertEqual(len(self.history), 6)
+
     def test_dependent_changes_dequeue(self):
         "Test that dependent patches are not needlessly tested"
 

zuul/driver/gerrit/gerritconnection.py

@@ -371,10 +371,14 @@ class GerritConnection(BaseConnection):
 
     def _updateChange(self, change, history=None):
 
-        # In case this change is already in the history we have a cyclic
-        # dependency and don't need to update ourselves again as this gets
-        # done in a previous frame of the call stack.
-        if history and change.number in history:
+        # In case this change is already in the history we have a
+        # cyclic dependency and don't need to update ourselves again
+        # as this gets done in a previous frame of the call stack.
+        # NOTE(jeblair): I don't think it's possible to hit this case
+        # anymore as all paths hit the change cache first.
+        if (history and change.number and change.patchset and
+            (change.number, change.patchset) in history):
+            self.log.debug("Change %s is in history" % (change,))
             return change
 
         self.log.info("Updating %s" % (change,))
@@ -420,7 +424,7 @@ class GerritConnection(BaseConnection):
             history = []
         else:
             history = history[:]
-        history.append(change.number)
+        history.append((change.number, change.patchset))
 
         needs_changes = []
         if 'dependsOn' in data:
@@ -465,7 +469,7 @@ class GerritConnection(BaseConnection):
             # reference the latest patchset of its Depends-On (this
             # change). In case the dep is already in history we already
             # refreshed this change so refresh is not needed in this case.
-            refresh = dep_num not in history
+            refresh = (dep_num, dep_ps) not in history
             dep = self._getChange(
                 dep_num, dep_ps, refresh=refresh, history=history)
             if (not dep.is_merged) and dep.is_current_patchset: