[MERGE/RFC] Odd processing during BzrDir.sprout()

[Andrew Bennetts]

John Arbash Meinel wrote:
[...]
> The main reason for the RFC is that I'm wondering if a better fix would just be:
>         if recurse == 'down' and repository.supports_tree_reference():
> 
> so we just disable this extra lookup if we know in advance that we won't *do*
> anything with it.

I have no opinion on this particular issue.

> I came across this, while working on my sftp tests, because it would seem to
> finish the transfer. And then just sit around for a while, thinking, before it
> actually finished the branch.
> 
> Looking more closely, I think we need to address some stuff in BzrDir.sprout().
> 
> I see it doing:
>             source_branch = self.open_branch()
>             source_repository = source_branch.repository
> 
> But I never see it *locking* those objects. Which means it isn't caching any
> information between calls.

I agree.  And similarly, if it falls through to the "no branch so just open a
repo" case, it doesn't lock source_repository either.

This method could do with an overhaul.  The way it locks and unlocks things, if
it does at all, is pretty ad hoc.

> There is also something inherently wrong (IMO) about having "cmd_branch" do:
>   br_from.bzrdir.sprout(target_url...)
> 
> and then having sprout do:
>   br_from = self.open_branch()
> 
> and creating an entirely new Branch instance. (One that isn't locked, or
> sharing *any* state with the branch we just used to probe the ancestry,
> possibly resolve -r XXX information, etc.)

Ouch.  Yes.

All that said, I'm inclined to say you should land this patch.  It seems ok to
me, and your numbers say that it's a very big win.  It would be nice to do a
more comprehensive improvement, but we can do that later.

So,

bb:approve

-Andrew.