[MERGE] StackableBranch

[John Arbash Meinel]

John Arbash Meinel has voted tweak.
Status is now: Conditionally approved
Comment:
+    def get_stacked_on(self):
+        raise errors.UnstackableBranchFormat(self._format, self.base)

^- I would say that this should be:

get_stacked_on_url() to make it clear that you aren't getting a Branch 
object here.

In other places we did plain "get_parent", or get_bound_location versus 
get_master_branch. We could do "get_stacked_on_location()" to be more 
consistent, but that is getting rather long.

...
-            revision_ids = self.source.all_revision_ids()
+            source_revision_ids = 
frozenset(self.source.all_revision_ids())
+            revision_ids = source_revision_ids - \
+ 
frozenset(self.target.get_parent_map(source_revision_ids))
              revision_keys = [(revid,) for revid in revision_ids]
              index = 
self.target._pack_collection.revision_index.combined_index
              present_revision_ids = set(item[1][0] for item in
@@ -2779,18 +2781,33 @@
          if not find_ghosts and revision_id is not None:
              return self._walk_to_common_revisions([revision_id])
          elif revision_id is not None:
-            source_ids = self.source.get_ancestry(revision_id)
-            if source_ids[0] is not None:
-                raise AssertionError()
-            source_ids.pop(0)
+            # Find ghosts: search for revisions pointing from one 
repository to
+            # the other, and vice versa, anywhere in the history of 
revision_id.
+            graph = self.target.get_graph(other_repository=self.source)
+            searcher = 
graph._make_breadth_first_searcher([revision_id])
+            found_ids = set()
+            while True:
+                try:
+                    next_revs, ghosts = searcher.next_with_ghosts()
+                except StopIteration:
+                    break
+                if revision_id in ghosts:
+                    raise errors.NoSuchRevision(self.source, 
revision_id)
+                found_ids.update(next_revs)
+                found_ids.update(ghosts)
+            found_ids = frozenset(found_ids)
+            # Double query here: should be able to avoid this by 
changing the
+            # graph api further.
+            result_set = found_ids - frozenset(
+                self.target.get_graph().get_parent_map(found_ids))

^- I don't think you want to get_graph() here. Why not just use the 
'graph' object that you just opened earlier? Or why not just use 
'self.target.get_parent_map()' directly. (I believe using 'graph' is the 
slighlty better option.)


...

This tests is at least mis-named:
+    def test_set_stacked_on_fetches(self):
+        # We have a mainline
+        trunk_tree = self.make_branch_and_tree('mainline')
+        trunk_revid = trunk_tree.commit('mainline')
+        # and make branch from it which is stacked
+        try:
+            new_dir = trunk_tree.bzrdir.sprout('newbranch', 
stacked=True)
+        except (errors.UnstackableBranchFormat,
+            errors.UnstackableRepositoryFormat):
+            # not a testable combination.
+            return
+        new_tree = new_dir.open_workingtree()
+        new_tree.commit('something local')

I think you want a test that does 'set_stacked_on(None)' and ensure that 
any revisions have been fetched over. But 'set_stacked_on' is not called 
here. I think you probably want to keep this test, but under a different 
name.


For details, see: 
http://bundlebuggy.aaronbentley.com/request/%3C484E2C4E.90803%40canonical.com%3E