Code review comment for lp:~spiv/bzr/sprout-does-not-reopen-repo

Sorry about the delay on this. In general, I really like the idea of this patch.

I would at least mutter a warning about the double fallback, we don't have to display it on the screen. It would be nice to fix it, but you don't have to get everything right on the first attempt.

I really don't like the proliferation of "no, really, use this object" parameters. I think the problem is that BzrDir.sprout() wants to be the master, but it still defers some of the work to Branch/Repo/WorkingTree. I don't have a better answer, though. I know you mentioned this, too.

I'd personally really like to see a:
  X.open_write_locked(create=True)

Or something along those lines.

I think part of the confusion is also that if you got something which is a Branch, and you think there is a tree there, the standard use is:
 branch.bzrdir.open_workingtree()

However, that *reopens* the Branch and Repository.

We also have a lot of code that does stuff like:
  a) If there is a WT, do X
  b) If not, but there is a Branch, do Y
  c) If not, but there is a Repository, do Z

It would be nice to simplify a lot of that if we could. That is some of what "BzrDir.open_containing_tree_or_branch" is supposed to be. Though many times I want "open_containing_tree_or_direct_branch" (don't recurse for a branch, but you can recurse for a Tree).

This is potentially disruptive to bzr-svn, because it adds a new parameter that BzrDir will be passing down the stack. As such, adding a cleaner new api may be an easier way to get what we want. It would let us add a compatability shim for objects that don't implement the new interface, and could be cleaner overall.

Then again, the practical side of me says, you've done the work, it works, even if it isn't perfect, it is better than what we have.