[MERGE] bzr-dir phase 2

[John Arbash Meinel]

Robert Collins wrote:
> On Sun, 2006-02-12 at 12:54 -0600, John A Meinel wrote:
> 
>> I'm starting to go through it, so here goes...
>>
>> What is the use case for get_branch_transport() based on branch_format?
> 
> As the docstring says, this provides the branch transport for use in
> that bzrdir. Different bzr dirs may be compatible with different branch
> formats, passing the branch format in during initialization allows the
> bzrdir to check for compatability - and we use that to ensure that
> format-string-less branch formats (that is branch data format 4, the
> current format) do not get used with meta-dir environments and vice
> verca.
> 
>> The parameter for get_workingtree_transport() is labeled 'branch_format'
>> not 'workingtree_format'.
> 
> Doh. Fixed.
> 
>> Should __init__() always be the first entry in a class?
> 
> I've been putting methods in alphabetical order ignoring __'s. But I can
> change happily.

In my head __init__ should be next to class Foo(), because that is how
you instantiate a class. Other people can comment. (I always put my
constructor as the first thing after typedefs and nested classes in C++).

> 
>> You almost use epydoc format for the doc strings, but it would be:
>>
>> :param _format: stuff
>>
>> rather than just
>> _format: stuff
> 
> I'll tweak the ones I spot, but I'm not too concerned unless you are.
> 

It isn't a blocker, but if we use consistent documentation, then we can
use 'epydoc' to generate the low-level documentation. And it might make
it possible for the introspective functions to do more.

>> 'BzrDir.open()' says "Open an existing *branch*, rooted at 'base'.
> 
> Fixed.
> 
>> We probably shouldn't be raising NotBranchError from
>> BzrDir.open_containing(). We should raise NotBzrError, or somesuch.
> 
> I thought it was best for code compatability, but I'm happy to change
> it.

open_containing() can return a BzrDir that only has a checkout, right? I
guess we could make NotBzrError a child of NotBranchError, and include
documentation that this is likely to change in the future.

> 
>> Why does 'sprout()' always create a standalone branch?
> 
> At this point in time I dont have any [implementable] use cases for
> making a new line of development but not making a working tree.
> Repositories are not implemented, and the UI has no concept of 'branch
> --no-working-tree' or some such.

Right, but the api shouldn't be that sprout() creates a standalone
should it? If format 4,5,6 all only support that, I'm okay with it. But
we want an api that doesn't changed when we get repositories.

> 
>> Why does get_repository_transport() create the 'repository' directory?
> 
> Same reason get_branch_transport creates the 'branch' directory - the
> bzrdir is setting up the environment for the repository format to use.
> It seemed cleaner to me to have the repository just say 'give me a ready
> to use transport' than to have every repository format need to fiddle
> with mkdir etc. Particularly as the repository is path agnostic - it
> does not know where it is based except via having a transport connection
> to it.

I agree with you. Its just that 'get_*' doesn't seem like something that
creates it. Just something that returns what is already there.

In my head, get_* doesn't usually have side effects.

> 
>> You have 'BzrDirFormat5' having format 4 branches, and format 6
>> repositories (always). Shouldn't that be format 5 repositories? (I think
>> this is just a documentation bug.)
> 
> Fixed.
> 
>> 10k lines is a lot of code to review. But your results do speak for
>> themselves. (I didn't get through everything, but I tried).
> 
> Thanks!
> 
> Rob

By the way, I would like to see this land soon. We have a whole lot of
major updates that are piling in, and they need to land, and give us a
bunch of time to stabilize things before the next release. (Right now
win32 support is toasted for all number of reasons.) And all of these
major api changes will conflict against other people's minor
improvements. My plugins started doing repo = getattr(branch,
'repository', branch) for compatibility. Unfortunately, Repository isn't
100% identical to branch. (I think _get_inventory_weave =>
get_inventory_weave).

I would like to avoid layer after layer of compatibility for my plugins.
Preferring to only have the compatability for 1 or 2 major versions, and
then expire the old ones.

John
=:->

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 249 bytes
Desc: OpenPGP digital signature
Url : https://lists.ubuntu.com/archives/bazaar/attachments/20060212/4e7ad4ac/attachment.pgp