[MERGE] Stacking policy

[Ian Clatworthy]

Aaron Bentley wrote:
> Ian Clatworthy wrote:
>> Aaron Bentley wrote:
>>> This patch implements policy for stacking branches.

Well done on this. A bit more feedback below for your consideration.

bb:tweak

>> Will everything Just Work if a user has
>> both an old and a new version of bzr installed on their computer?
>> If the user creates a branch stacked on another using the newer
>> version, will running the old version on that branch work?
> 
> No.  The old version of bzr won't support stacking.
> 
>> If not, will it fail gracefully?
> 
> Yes.  They'll get an error that the branch or repo is in an unsupported
> format, because old branch/repo formats don't support stacking.

Ah good. Sounds like everyone is happy about not needing a new
format here then.

>>> +        source_branch = bzrlib.branch.Branch.open(
>>> +            self.get_vfs_only_url('source'))
>>>          self.assertEqual(target_repo._format, source_branch.repository._format)
>> I'm yet to appreciate why this change is needed? Is there a short explanation?
> 
> - From what I can tell, this is a test of whether the on-disk format of
> the source equals the target.  It was failing because source was
> RemoteRepositoryFormat, and target was the on-disk format.

OK.

>>> +class TestTransportConfig(TestCaseWithBzrDir):
>>> +
>>> +    def setUp(self):
>>> +        TestCaseWithBzrDir.setUp(self)

I included this method in my previous email but I forgot to mention why.
As all it does in call the matching method in the superclass, it's
redundant isn't it?

> +        try:
> +            local_branch = self.open_branch()
> +        except errors.NotBranchError:
> +            local_branch = None
> +        else:
> +            # enable fallbacks when branch is not a branch reference
> +            if local_branch.repository.has_same_location(local_repo):
> +                local_repo = local_branch.repository
> +            if preserve_stacking:
> +                try:
> +                    stack_on = local_branch.get_stacked_on()
> +                except (errors.UnstackableBranchFormat,
> +                        errors.UnstackableRepositoryFormat,
> +                        errors.NotStacked):
> +                    pass
> +
>          if local_repo:
>              # may need to copy content in
>              repository_policy = result.determine_repository_policy(
> -                force_new_repo)
> +                force_new_repo, stack_on)
>              make_working_trees = local_repo.make_working_trees()
>              result_repo = repository_policy.acquire_repository(
>                  make_working_trees, local_repo.is_shared())
>              result_repo.fetch(local_repo, revision_id=revision_id)
> +        else:
> +            result_repo = None
>          # 1 if there is a branch present
>          #   make sure its content is available in the target repository
>          #   clone it.

This particular change looks all good to me. Immediately after this, the
(old) code goes on to open the branch and set local_branch again. As you've
already done that now, I think the try/catch/else I'm talking about can
simply become something like:

  if local_branch is not None:
    ...

> @@ -973,6 +1024,7 @@
>          target_transport.ensure_base()
>          cloning_format = self.cloning_metadir()
>          result = cloning_format.initialize_on_transport(target_transport)
> +        repository_policy = None

It doesn't do any harm but I think that setting repository_policy to None is
not needed here? It looks like all code paths go through the bit that set
it later on.

Ian C.