[RFC][Merge] repository support

[Robert Collins]

On Wed, 2006-02-15 at 15:43 -0600, John A Meinel wrote:
> Robert Collins wrote:
> > On Wed, 2006-02-15 at 11:12 -0600, John A Meinel wrote:
> ...
> 
> >>> +    def create_branch_convenience(base, force_new_repo=False, force_new_tree=None):
> >>> +        """Create a new BzrDir, Branch and Repository at the url 'base'.
> >>> +
> >>> +        This is a convenience function - it will use an existing repository
> >>> +        if possible, can be told explicitly whether to create a working tree or
> >>> +        nor.
> >> not
> 
> 
> I don't know if you saw this one. You want to use the work 'not', you
> have the word 'nor'.

Thanks.

> 
> 
> ...
> 
> >> Isn't this incorrect? You have create, and then find, and then create
> >> again. Shouldn't it be:
> > 
> > BzrDir.create() creates the bzrdir itself. I think that lead to a
> > misread of the code :(. Its clearer now with the extracted
> > _find_or_create_repository.
> > 
> > ...
> 
> 
> I think I broke it up at an unfortunate location. You specifically have:
> 
>         bzrdir = BzrDir.create(base)
>         if force_new_repo:
>             bzrdir.create_repository()
>         try:
>             repo = bzrdir.find_repository()
>         except errors.NoRepositoryPresent:
>             bzrdir.create_repository()
>         return bzrdir.create_branch()
> 
> Now, unless I miss my guess that at least needs an 'else:' otherwise you
> are using 'find_repository()' to find the repo you just created, and if
> it didn't find it, you would be creating it again, and not returning a repo.

Right. Pseudo code:
if we always want a new repository:
  create one
if we can find a repository:
  use it
otherwise:
  make one.

find_repository() stops at the local repository if there is one.

> At the very least something is fishy there. It might be clearer after
> you switched to using find_or_create.

This code block is already switched to that as it happens.


> >> I didn't go through all of the tests very thoroughly. But generally
> >> things look good. My only comment is the repeating paradigm of
> >> find_or_create. (+1 from me)
> > 
> > 
> > Thanks
> > 
> > Are you happy with my replies/actions above? If so I'll merge it.
> > 
> > Cheers,
> > Rob
> 
> I think its okay. It would be nice to get a second (third) reviewer on
> major changes like this. But past experience has shown that it won't
> happen. 1k+ (and up to 10k, ouch!) is a lot of lines to review.
> 
> Its probably good enough to be merged.
> 
> But after all of these major changes, I would *really* like to see some
> thorough code cleanups, or reviews or something. We want to release an
> 0.8 soon, and we've got lots of code that needs some thorough shaking.

I'm happy to keep cycling around on this for $N reviewers, I've got
plenty to keep me occupied :).

I'm not quite sure whether to merge this at this point or not now :p.

Rob
(who has consolidated the lockable files instances in format 4/5/6 trees
and is currently tackling upgrade).


-- 
GPG key available at: <http://www.robertcollins.net/keys.txt>.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: This is a digitally signed message part
Url : https://lists.ubuntu.com/archives/bazaar/attachments/20060216/89f2a3ad/attachment.pgp