[MERGE REQUEST] Couple of small fixes

[Robert Collins]

-1 as it stands due to the breaking of opening-remote-branches
efficiently.

On Fri, 2005-12-09 at 02:21 +0100, Jelmer Vernooij wrote:
> 
> -- 
> 
> 
> 
> 
> 
> 
> 
> plain text
> document
> attachment
> (merge-request.diff)
> 
> === modified file 'bzrlib/branch.py'
> --- bzrlib/branch.py    
> +++ bzrlib/branch.py    
> @@ -92,6 +92,15 @@
>              self.unlock()
>      return decorated
>  
> +branch_types = []


If branch_types is not a public symbol, it should be _branch_types. (I
think it should not be public). 

> +
> +def register_branch_type(bt):


what is bt? Is it a class? If so, I think this would be clearer as
def register_branch_class(klass):
...

(two letter variable names are rarely nice)


> +    """Utility function to help register a branch type"""
> +
> +    branch_types.append(bt)
> +
> +    mutter('registered foreign branch type %s', bt.__name__)

using bt.__name__ here is somewhat naughty: why not just bt ? muttering
is a debug tool anyway, the default class formatter is probably most
useful.

>  ######################################################################
>  # branch objects
>  
> @@ -114,14 +123,27 @@
>          return BzrBranch(get_transport(base),
> relax_version_check=True)
>          
>      @staticmethod
> -    def open(base):
> -        """Open an existing branch, rooted at 'base' (url)"""
> -        t = get_transport(base)
> -        mutter("trying to open %r with transport %r", base, t)
> -        return BzrBranch(t)
> +    def open(base,allowed_types=None):

code formatting nit: this should be
+    def open(base, allowed_types=None):

> +        """Open an existing branch, rooted at 'base' (url)
> +        
> +        allowed_types
> +            Branch types to allow (defaults to all branch types
> available)
> +        """
> +        if not allowed_types:
> +            allowed_types = branch_types
> +
> +        for bt in allowed_types:
> +            assert issubclass(bt, Branch)

This assert is not useful IMO. Why should an implementation have to
subclass Branch ? Testing of contract conformance is best done via the
test suite, not by hoping people have not overriden the wrong things.

Also, rather than 'bt' I'd like to see something like 'implementation'
or 'branch_class' - more clear.

> +            try:
> +                return bt.open(base)
> +            except NotBranchError:
> +                pass
> +        
> +        raise NotBranchError(path=base)
 
>      @staticmethod
> -    def open_containing(url):
> +    def open_containing(url,allowed_types=None):

Same as above - I won't repeat this for other cases, but will check on a
refreshed patch ;).

>          """Open an existing branch which contains url.
>          
>          This probes for a branch at url, and searches upwards from
> there.
> @@ -129,24 +151,28 @@
>          Basically we keep looking up until we find the control
> directory or
>          run into the root.  If there isn't one, raises
> NotBranchError.
>          If there is one, it is returned, along with the unused
> portion of url.
> -        """
> -        t = get_transport(url)
> -        while True:
> +
> +        allowed_types
> +            Branch types to allow (defaults to all branch types
> available)
> +        """
> +
> +        if not allowed_types:
> +            allowed_types = branch_types
> +
> +        for bt in allowed_types:
> +            assert issubclass(bt, Branch)
> +

Also the same as above.

>              try:
> -                return BzrBranch(t), t.relpath(url)
> +                return bt.open_containing(url)
>              except NotBranchError:
>                  pass
> -            new_t = t.clone('..')
> -            if new_t.base == t.base:
> -                # reached the root, whatever that may be
> -                raise NotBranchError(path=url)
> -            t = new_t
> +        
> +        raise NotBranchError(path=url)

I'm concerned about the above changes. This makes opening of containing
branches -much- slower. I think that requiring transport be acceptable
to all Branches is a reasonable requirement. If someone has a
ForeignBranch on SVN at a http:// location, they should teach the Http
transport to understand that, or just use ignore the transport in their
branch.
 
I've skipped the rest of the diff, as it will need to change to 
fit with this.

Cheers,
Rob



-- 
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/20051215/924e8f71/attachment.pgp