[RFC][Merge] repository support

[John A Meinel]

Robert Collins wrote:
> On Wed, 2006-02-15 at 11:12 -0600, John A Meinel wrote:
...

>> Wouldn't this be cleaner as (paraphrased):
>>
>> if local_repo:
>>   if force_new_repo:
>>     result_repo = None
>>   else:
>>     try:
>>       result_repo = result.find_repository()
>>     except
>>       result_repo = None
>>
>>   if not result_repo:
>>     result_repo.fetch(...)
>>   else:
>>     local_repo.clone(result, revision_id=revision_id, basis=basis_repo)
>>
>> It just seems less nested, with less redundancy.
> 
> Uhm, your paraphrased code seems buggy :O.
> 
> You would introduce a new repository even if the source bzrdir did not
> have one and force_new_repo was off.
> 

...

> I much prefer guaranteed apis where 'repo = find_repository()' always
> worked if it returns rather than throwing. Its difficult for developers
> if they have to remember that some things signal errors by returning
> None, some by returning -X, some by throwing exceptions. 

I understand that. But I also find that having try/except everywhere is
pretty ugly. And means that the 'exceptional' condition is actually
quite expected, and thus not exceptional.

But, this isn't the only place that we do it, and I'll accept that it is
okay.

> 
>> You have this same pattern later where you have:
>>
>> if force:
>>   do X
>> else:
>>   try:
>>     find
>>     do Y
>>   except
>>     do X
> 
> One complicating issue is that for clone() and sprout() we delegate the
> creation of the target repository to the repository itself, which is
> important because we don't know the right format to use and the
> repository does. sprout and clone also have different rules about when
> to make a new repository - because of their different contracts.
> 
....

>>> -    def create_repository(base):
>>> +    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'.



...

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

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


> 
>> can you name this "open_containing_from_transport"? Otherwise it sounds
>> like you are returning a transport not a branch (or bzr dir).
> 
> Ok.
> 

...

> I've factored out the two cases where it does follow that pattern into
> such a method.
> ...

Good.

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

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/20060215/4b5e8c8b/attachment.pgp