[MERGE] Reduce round trips pushing new branches substantially.

[Andrew Bennetts]

Robert Collins wrote:
> 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.

There's “trying to stack” and there's “must stack”.  You're handling the
“trying to stack” case just fine.  I'm talking about the “must stack” case,
i.e. self._require_stacking=True.  If some caller (perhaps a plugin, perhaps
a later change bzrlib) does specify require_stacking=True and encounter a
JailBreak, then I think silently returning here would be an error.  I don't
think any such callers exist currently, but that doesn't mean there's no bug
here.

So, I expect that if you change the code to be (omitting comments):

    except errors.JailBreak:
        if self._require_stacking:
            raise
        return

Then our existing tests will still pass because the server doesn't pass
require_stacking=True to the repository acquisition policy, but it fixes the
bug I'm talking about.

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

Ugh, that's right.  How about a module-global function in bzrdir.py?  e.g.

    def get_default_stacking_format(rich_root):
        if rich_root:
            repo_format = pack_repo.RepositoryFormatKnitPack6RichRoot()
        else:
            repo_format = pack_repo.RepositoryFormatKnitPack6()
	branch_format = branch.BzrBranchFormat7()
        return branch_format, repo_format

And then update BzrDirMetaFormat1.require_stacking to use it too — it has
the same basic logic as yours, but uses RepositoryFormatKnitPack5* not 6.
It seems wrong to let these places get out of sync.

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

Hmm, I remember being ok with the code change, I must have missed the test
that cemented it though!

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

Cool.

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

Fair enough.

-Andrew.