[MERGE] Reduce round trips pushing new branches substantially.

[Andrew Bennetts]

bb:tweak

Comments and questions inline.

Robert Collins wrote:
[...]
> === modified file 'bzrlib/bzrdir.py'
[...]
> +class BzrDirFormatAllInOne(BzrDirFormat):
> +    """Common class for formats before meta-dirs."""
> +
> +    def initialize_on_transport_ex(self, transport, use_existing_dir=False,
> +        create_prefix=False, force_new_repo=False, stacked_on=None,
> +        stack_on_pwd=None, repo_format_name=None, make_working_trees=None,
> +        shared_repo=False):
> +        """See BzrDirFormat.initialize_on_transport_ex."""
> +        require_stacking = (stacked_on is not None)
> +        # Format 5 cannot stack, but we've been asked do - actually init

"do" should be "to" in that comment.

> @@ -3166,8 +3428,13 @@
>          stack_on = self._get_full_stack_on()
>          if stack_on is None:
>              return
> -        stacked_dir = BzrDir.open(stack_on,
> -                                  possible_transports=possible_transports)
> +        try:
> +            stacked_dir = BzrDir.open(stack_on,
> +                                      possible_transports=possible_transports)
> +        except errors.JailBreak:
> +            # We keep the stacking details, but we are in the server code so
> +            # actually stacking is not needed.
> +            return

Hmm.  Shouldn't this check self._require_stacking, and raise if we get a
JailBreak and thus can't stack?  I wouldn't be surprised if that combination
of conditions (JailBreak in server + require_stacking==True) doesn't occur
in practice, but this looks like a latent bug.

[...]
>                      target_dir = BzrDir.open(stack_on,
>                          possible_transports=[self._bzrdir.root_transport])
>                  except errors.NotBranchError:
>                      # Nothing there, don't change formats
>                      pass
> +                except errors.JailBreak:
> +                    # stack_on is inaccessible, JFDI.
> +                    if format.repository_format.rich_root_data:
> +                        repo_format = pack_repo.RepositoryFormatKnitPack6RichRoot()
> +                    else:
> +                        repo_format = pack_repo.RepositoryFormatKnitPack6()

How many places do we hard-code the current default repository formats?  It
seems likely that we next update the default format we'll miss places like
this... why not use 'default'/'default-rich-root' from the format_registry?

[...]
> +                if branch_format and repo_format:
> +                    # Does support stacking, use its format.

Perhaps update that comment to say “Is accessible and does support stacking,
use its format”?  Doesn't matter a great deal...

[...]
> === modified file 'bzrlib/smart/request.py'
> --- bzrlib/smart/request.py	2009-04-15 02:07:35 +0000
> +++ bzrlib/smart/request.py	2009-04-24 05:08:51 +0000
> @@ -69,7 +69,7 @@
>              continue
>          else:
>              return
> -    raise errors.BzrError('jail break: %r' % (abspath,))
> +    raise errors.JailBreak(abspath)

Thanks for adding that exception.  Could you update the two assertRaises in
bzrlib/tests/test_smart_request.py to expect JailBreak rather than BzrError?

> === modified file 'bzrlib/tests/branch_implementations/test_create_clone.py'
[...]
>  class TestCreateClone(TestCaseWithBranch):
>  
> +    def test_create_clone_on_transport_missing_parent_dir(self):
> +        tree = self.make_branch_and_tree('source')
> +        tree.commit('a commit')
> +        source = tree.branch
> +        target_transport = self.get_transport('subdir').clone('target')
> +        self.assertRaises(errors.BzrCommandError,
> +            tree.branch.create_clone_on_transport, target_transport)

Do we really want a unit test ensuring that non-UI code raises
BzrCommandError?

> +    def test_create_clone_on_transport_use_existing_dir_false(self):
> +        tree = self.make_branch_and_tree('source')
> +        tree.commit('a commit')
> +        source = tree.branch
> +        target_transport = self.get_transport('target')
> +        target_transport.create_prefix()
> +        self.assertRaises(errors.BzrCommandError,
> +            tree.branch.create_clone_on_transport, target_transport)
> +        self.assertFalse(target_transport.has(".bzr"))

Ditto.

> === modified file 'bzrlib/tests/bzrdir_implementations/test_bzrdir.py'
[...]
> +    def test_format_initialize_on_transport_ex_use_existing_dir_False(self):
> +        if not self.bzrdir_format.is_supported():
> +            # Not initializable - not a failure either.
> +            return
> +        t = self.get_transport('dir')
> +        t.ensure_base()
> +        self.assertRaises(errors.FileExists, self.assertInitializeEx, t,
> +            use_existing_dir=False)

It's a bit odd to use assertInitializeEx here rather than calling
initialize_on_transport_ex directly, given that you don't expect any of the
assert* calls inside assertInitializeEx to be used.

> +    def test_format_initialize_on_transport_ex_repo_fmt_name_followed(self):
> +        t = self.get_transport('dir')
> +        # 1.6 is likely to neve be default

“never”

> +    def assertInitializeEx(self, t, need_meta=False, **kwargs):
> +        """Execute initialize_on_transport_ex and check it succeeded correctly.
> +
> +        :param t: The transport to initialize on.
> +        :param **kwargs: Additional arguments to pass to
> +            initialize_on_transport_ex.
> +        :return: the resulting repo, control dir tuple.
> +        """

It would be good to have the docstring explain what it means by “it
succeeded correctly.”  It's not a short method or totally straightforward
method so it's hard to see exactly what results/behaviours it is meant to
verify at a glance.  Reading the code it seems this method is essentially
about asserting that the format and class of the resulting bzrdir (both on
disk and as a python object) matches self.bzrdir_format?

[...]
> +        if isinstance(expected_format, bzrdir.RemoteBzrDirFormat):
> +            self.assertIsInstance(control, RemoteBzrDir)
> +        else:
> +            if need_meta and isinstance(expected_format, (bzrdir.BzrDirFormat5,
> +                bzrdir.BzrDirFormat6)):
> +                # Pre-metadir formats change when we are making something that
> +                # needs a metaformat, because clone is used for push.
> +                expected_format = bzrdir.BzrDirMetaFormat1()
> +            self.assertEqual(control._format.network_name(),
> +                expected_format.network_name())
> +            # Current RemoteBzrDirFormat's do not reliably get network_name
> +            # set, so only check in the off-case.

I don't understand this comment.  What's the off-case?  Oh, this else clause
is... it's a bit odd to have this comment more than half-way through the
else clause then!  I think perhaps this comment about unreliable
network_names would make more sense in the “on-case”.

> === modified file 'bzrlib/tests/test_smart.py'
> --- bzrlib/tests/test_smart.py	2009-04-15 03:45:24 +0000
> +++ bzrlib/tests/test_smart.py	2009-04-24 05:08:51 +0000
> @@ -350,6 +350,46 @@
>              request.execute, 'subdir')
>  
>  
> +class TestSmartServerRequestBzrDirInitializeEx(tests.TestCaseWithMemoryTransport):
> +    """Basic tests for BzrDir.initialize_ex in the smart server.
> +
> +    The main unit tests in test_bzrdir exercise the API coprehensively.

“comprehensively”

[...]
> +        # no branch, tree or repository is expected with the current
> +        # default formart.

“format”

I get the sense you were commenting in hurry :)

> === modified file 'bzrlib/transport/__init__.py'
[...]
> +    def create_prefix(self):
> +        """Create all the directorie sleading down to self.base."""

“directories”

Does this method let you replace the code with the XXX in
initialize_on_transport_ex in bzrdir.py?  Hmm, I guess the use of
do_catching_redirections makes that awkward.

-Andrew.