[MERGE] StackableBranch

[Martin Pool]

On Fri, Jun 13, 2008 at 6:27 AM, John Arbash Meinel
<john at arbash-meinel.com> wrote:
> 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:

I implicitly merged this in bringing in the overall stacking branch,
without seeing these commits at the end of the thread about 'bzr send'
behaviour.  So I'm going to do them now.

> 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.

I agree, done.  For symmetry I also changed set_stacked_on.

> ...
> -            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.)

Reusing the graph from above causes test failures; it's not
immediately obvious to me why.  (Because it also looks into
self.source, or because it has some implicit state?)  I changed it to
self.target.get_parent_map and we can revisit it later.

> 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.

I think I already renamed that to test_sprout_stacked, and I added this test:



    def test_unstack_fetches(self):
        """Removing the stacked-on branch pulls across all data"""
        # We have a mainline
        trunk_tree = self.make_branch_and_tree('mainline')
        trunk_revid = trunk_tree.commit('revision on mainline')
        # and make branch from it which is stacked
        try:
            new_dir = trunk_tree.bzrdir.sprout('newbranch', stacked=True)
        except (errors.UnstackableBranchFormat,
            errors.UnstackableRepositoryFormat), e:
            raise TestNotApplicable(e)
        # stacked repository
        self.assertRevisionNotInRepository('newbranch', trunk_revid)
        # now when we unstack that should implicitly fetch, to make sure that
        # the branch will still work
        new_branch = new_dir.open_branch()
        new_branch.set_stacked_on_url(None)
        self.assertRevisionInRepository('newbranch', trunk_revid)
        # of course it's still in the mainline
        self.assertRevisionInRepository('mainline', trunk_revid)
        # and now we're no longer stacked
        self.assertIs(new_branch.get_stacked_on_url(), None)

-- 
Martin <http://launchpad.net/~mbp/>