[MERGE] Reduce round trips pushing new branches substantially.

[Robert Collins]

On Mon, 2009-04-27 at 12:03 +1000, Andrew Bennetts wrote:
> bb:tweak
> 
> Comments and questions inline.


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

huh? This code handles that precise combination:
We get a JailBreak because we're trying to stack. We're in the server,
and thus not actually ever able to _do_ stacking, so we stop. The client
configures their proxy RemoteRepository as stacked, and its all good.

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

They aren't stackable.

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

Sure.

> > === 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?

That was from the prior patch that you already approved :P. However  I
think its obsoleted now as I've moved that back closer to the UI anyway.
Its probably one of the tests I need to cleanup.

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

I'll look closer at this.

> > +    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?

And that they were made. Sure, I'll add some verbiage.

> [...]
> > +        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”.

Will move, it got left behind in a rearrangement.

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

Not quite; I did consider it but really wanted to close off this
micro-cycle.

-Rob
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 197 bytes
Desc: This is a digitally signed message part
Url : https://lists.ubuntu.com/archives/bazaar/attachments/20090427/bb46c871/attachment.pgp