Don't remove dependent version when downgrading to a version.

Adjusted the version traversal on downgrade
such that we can downgrade to a version that is a dependency for
a version in a different branch, *without* needing to remove that
dependent version as well.  Previously, the target version would be
seen as a "merge point" for it's normal up-revision as well as the
dependency.  This integrates with the changes for :ticket:`377`
and :ticket:`378` to improve treatment of branches with dependencies
overall.

Change-Id: Ica0732f6419f68ab85650170839ac8000ba3bbfb
Fixes: #379

5 files changed, 144 insertions, 32 deletions

diff --git a/alembic/script/base.py b/alembic/script/base.py
index 98c5311..a9b6e7d 100644
--- a/alembic/script/base.py
+++ b/alembic/script/base.py
@@ -329,7 +329,7 @@ class ScriptDirectory(object):
                 ancestor="Destination %(end)s is not a valid downgrade "
                 "target from current head(s)", end=destination):
             revs = self.revision_map.iterate_revisions(
-                current_rev, destination)
+                current_rev, destination, select_for_downgrade=True)
             return [
                 migration.MigrationStep.downgrade_from_script(
                     self.revision_map, script)
diff --git a/alembic/script/revision.py b/alembic/script/revision.py
index 6feba77..5284da9 100644
--- a/alembic/script/revision.py
+++ b/alembic/script/revision.py
@@ -197,10 +197,9 @@ class RevisionMap(object):
 
     def _add_depends_on(self, revision, map_):
         if revision.dependencies:
-            revision._resolved_dependencies = tuple(
-                map_[dep].revision for dep
-                in util.to_tuple(revision.dependencies)
-            )
+            deps = [map_[dep] for dep in util.to_tuple(revision.dependencies)]
+            revision._resolved_dependencies = tuple([d.revision for d in deps])
+
 
     def add_revision(self, revision, _replace=False):
         """add a single revision to an existing map.
@@ -528,7 +527,7 @@ class RevisionMap(object):
 
     def iterate_revisions(
             self, upper, lower, implicit_base=False, inclusive=False,
-            assert_relative_length=True):
+            assert_relative_length=True, select_for_downgrade=False):
         """Iterate through script revisions, starting at the given
         upper revision identifier and ending at the lower.
 
@@ -556,15 +555,25 @@ class RevisionMap(object):
             return relative_lower
 
         return self._iterate_revisions(
-            upper, lower, inclusive=inclusive, implicit_base=implicit_base)
+            upper, lower, inclusive=inclusive, implicit_base=implicit_base,
+            select_for_downgrade=select_for_downgrade)
 
     def _get_descendant_nodes(
-            self, targets, map_=None, check=False, include_dependencies=True):
+            self, targets, map_=None, check=False,
+            omit_immediate_dependencies=False, include_dependencies=True):
 
-        if include_dependencies:
-            fn = lambda rev: rev._all_nextrev
+        if omit_immediate_dependencies:
+            def fn(rev):
+                if rev not in targets:
+                    return rev._all_nextrev
+                else:
+                    return rev.nextrev
+        elif include_dependencies:
+            def fn(rev):
+                return rev._all_nextrev
         else:
-            fn = lambda rev: rev.nextrev
+            def fn(rev):
+                return rev.nextrev
 
         return self._iterate_related_revisions(
             fn, targets, map_=map_, check=check
@@ -574,9 +583,11 @@ class RevisionMap(object):
             self, targets, map_=None, check=False, include_dependencies=True):
 
         if include_dependencies:
-            fn = lambda rev: rev._all_down_revisions
+            def fn(rev):
+                return rev._all_down_revisions
         else:
-            fn = lambda rev: rev._versioned_down_revisions
+            def fn(rev):
+                return rev._versioned_down_revisions
 
         return self._iterate_related_revisions(
             fn, targets, map_=map_, check=check
@@ -605,19 +616,28 @@ class RevisionMap(object):
                 todo.extend(
                     map_[rev_id] for rev_id in fn(rev))
                 yield rev
-            if check and per_target.intersection(targets).difference([target]):
-                raise RevisionError(
-                    "Requested revision %s overlaps with "
-                    "other requested revisions" % target.revision)
+            if check:
+                overlaps = per_target.intersection(targets).\
+                    difference([target])
+                if overlaps:
+                    raise RevisionError(
+                        "Requested revision %s overlaps with "
+                        "other requested revisions %s" % (
+                            target.revision,
+                            ", ".join(r.revision for r in overlaps)
+                        )
+                    )
 
     def _iterate_revisions(
-            self, upper, lower, inclusive=True, implicit_base=False):
+            self, upper, lower, inclusive=True, implicit_base=False,
+            select_for_downgrade=False):
         """iterate revisions from upper to lower.
 
         The traversal is depth-first within branches, and breadth-first
         across branches as a whole.
 
         """
+
         requested_lowers = self.get_revisions(lower)
 
         # some complexity to accommodate an iteration where some
@@ -668,7 +688,12 @@ class RevisionMap(object):
         total_space = set(
             rev.revision for rev in upper_ancestors).intersection(
             rev.revision for rev
-            in self._get_descendant_nodes(lowers, check=True)
+            in self._get_descendant_nodes(
+                lowers, check=True,
+                omit_immediate_dependencies=(
+                    select_for_downgrade and requested_lowers
+                )
+            )
         )
 
         if not total_space:
@@ -688,7 +713,9 @@ class RevisionMap(object):
         #assert not branch_todo.intersection(uppers)
 
         todo = collections.deque(
-            r for r in uppers if r.revision in total_space)
+            r for r in uppers
+            if r.revision in total_space
+        )
 
         # iterate for total_space being emptied out
         total_space_modified = True
diff --git a/docs/build/changelog.rst b/docs/build/changelog.rst
index 0470ab3..ab7b3c3 100644
--- a/docs/build/changelog.rst
+++ b/docs/build/changelog.rst
@@ -8,6 +8,19 @@ Changelog
 
     .. change::
       :tags: bug, versioning
+      :tickets: 379
+
+      Adjusted the version traversal on downgrade
+      such that we can downgrade to a version that is a dependency for
+      a version in a different branch, *without* needing to remove that
+      dependent version as well.  Previously, the target version would be
+      seen as a "merge point" for it's normal up-revision as well as the
+      dependency.  This integrates with the changes for :ticket:`377`
+      and :ticket:`378` to improve treatment of branches with dependencies
+      overall.
+
+    .. change::
+      :tags: bug, versioning
       :tickets: 377
 
       Fixed bug where a downgrade to a version that is also a dependency
diff --git a/tests/test_revision.py b/tests/test_revision.py
index 498bea8..4238c0a 100644
--- a/tests/test_revision.py
+++ b/tests/test_revision.py
@@ -126,7 +126,7 @@ class APITest(TestBase):
 class DownIterateTest(TestBase):
     def _assert_iteration(
             self, upper, lower, assertion, inclusive=True, map_=None,
-            implicit_base=False):
+            implicit_base=False, select_for_downgrade=False):
         if map_ is None:
             map_ = self.map
         eq_(
@@ -134,7 +134,8 @@ class DownIterateTest(TestBase):
                 rev.revision for rev in
                 map_.iterate_revisions(
                     upper, lower,
-                    inclusive=inclusive, implicit_base=implicit_base
+                    inclusive=inclusive, implicit_base=implicit_base,
+                    select_for_downgrade=select_for_downgrade
                 )
             ],
             assertion
@@ -788,6 +789,23 @@ class MultipleBaseTest(DownIterateTest):
 
 class MultipleBaseCrossDependencyTestOne(DownIterateTest):
     def setUp(self):
+        """
+
+        base1 -----> a1a  -> b1a
+              +----> a1b  -> b1b
+                              |
+                  +-----------+
+                  |
+                  v
+        base3 -> a3 -> b3
+                  ^
+                  |
+                  +-----------+
+                              |
+        base2 -> a2 -> b2 -> c2 -> d2
+
+
+        """
         self.map = RevisionMap(
             lambda: [
                 Revision('base1', (), branch_labels='b_1'),
@@ -822,28 +840,66 @@ class MultipleBaseCrossDependencyTestOne(DownIterateTest):
             ]
         )
 
-    def test_we_need_head2(self):
+    def test_heads_to_base_downgrade(self):
+        self._assert_iteration(
+            "heads", "base",
+            [
+
+                'b1a', 'a1a', 'b1b', 'a1b', 'd2', 'c2', 'b2', 'a2', 'base2',
+                'b3', 'a3', 'base3',
+                'base1'
+            ],
+            select_for_downgrade=True
+        )
+
+    def test_we_need_head2_upgrade(self):
         # the 2 branch relies on the 3 branch
         self._assert_iteration(
             "b_2@head", "base",
             ['d2', 'c2', 'b2', 'a2', 'base2', 'a3', 'base3']
         )
 
-    def test_we_need_head3(self):
+    def test_we_need_head2_downgrade(self):
+        # the 2 branch relies on the 3 branch, but
+        # on the downgrade side, don't need to touch the 3 branch
+        self._assert_iteration(
+            "b_2@head", "b_2@base",
+            ['d2', 'c2', 'b2', 'a2', 'base2'],
+            select_for_downgrade=True
+        )
+
+    def test_we_need_head3_upgrade(self):
         # the 3 branch can be upgraded alone.
         self._assert_iteration(
             "b_3@head", "base",
             ['b3', 'a3', 'base3']
         )
 
-    def test_we_need_head1(self):
+    def test_we_need_head3_downgrade(self):
+        # the 3 branch can be upgraded alone.
+        self._assert_iteration(
+            "b_3@head", "base",
+            ['b3', 'a3', 'base3'],
+            select_for_downgrade=True
+        )
+
+    def test_we_need_head1_upgrade(self):
         # the 1 branch relies on the 3 branch
         self._assert_iteration(
             "b1b@head", "base",
             ['b1b', 'a1b', 'base1', 'a3', 'base3']
         )
 
-    def test_we_need_base2(self):
+    def test_we_need_head1_downgrade(self):
+        # going down we don't need a3-> base3, as long
+        # as we are limiting the base target
+        self._assert_iteration(
+            "b1b@head", "b1b@base",
+            ['b1b', 'a1b', 'base1'],
+            select_for_downgrade=True
+        )
+
+    def test_we_need_base2_upgrade(self):
         # consider a downgrade to b_2@base - we
         # want to run through all the "2"s alone, and we're done.
         self._assert_iteration(
@@ -851,14 +907,30 @@ class MultipleBaseCrossDependencyTestOne(DownIterateTest):
             ['d2', 'c2', 'b2', 'a2', 'base2']
         )
 
-    def test_we_need_base3(self):
+    def test_we_need_base2_downgrade(self):
+        # consider a downgrade to b_2@base - we
+        # want to run through all the "2"s alone, and we're done.
+        self._assert_iteration(
+            "heads", "b_2@base",
+            ['d2', 'c2', 'b2', 'a2', 'base2'],
+            select_for_downgrade=True
+        )
+
+    def test_we_need_base3_upgrade(self):
+        self._assert_iteration(
+            "heads", "b_3@base",
+            ['b1b', 'd2', 'c2', 'b3', 'a3', 'base3']
+        )
+
+    def test_we_need_base3_downgrade(self):
         # consider a downgrade to b_3@base - due to the a3 dependency, we
         # need to downgrade everything dependent on a3
         # as well, which means b1b and c2.  Then we can downgrade
         # the 3s.
         self._assert_iteration(
             "heads", "b_3@base",
-            ['b1b', 'd2', 'c2', 'b3', 'a3', 'base3']
+            ['b1b', 'd2', 'c2', 'b3', 'a3', 'base3'],
+            select_for_downgrade=True
         )
 
 
diff --git a/tests/test_version_traversal.py b/tests/test_version_traversal.py
index a8417ac..08c737f 100644
--- a/tests/test_version_traversal.py
+++ b/tests/test_version_traversal.py
@@ -696,10 +696,10 @@ class DependsOnBranchTestTwo(MigrationTest):
 
         self._assert_downgrade(
             self.b2.revision, heads,
-            [self.down_(self.bmerge), self.down_(self.d1)],
+            [self.down_(self.bmerge)],
             set([
                 self.amerge.revision,
-                self.b1.revision, self.cmerge.revision, self.b2.revision])
+                self.b1.revision, self.cmerge.revision, self.d1.revision])
         )
 
         # start with those heads..
@@ -773,8 +773,8 @@ class DependsOnBranchTestThree(MigrationTest):
         # to remove half of a merge point.
         self._assert_downgrade(
             'b1', ['a3', 'b2'],
-            [self.down_(self.a3), self.down_(self.b2)],
-            set(['a2', 'b1'])
+            [self.down_(self.b2)],
+            set(['a3'])  # we have b1 also, which is implied by a3
         )