[MERGE] Add Branch.get_reference and BzrDir.get_branch_reference, plus other miscellany

[Robert Collins]

On Fri, 2007-04-13 at 18:36 -0500, John Arbash Meinel wrote:
> Andrew Bennetts wrote:
> > Sorry for the slightly unfocused bundle, but it's fairly short.
> > 
> > This is yet more stuff from the hpss branch.
> > 
> > -Andrew.
> > 
> > 
> 
> A little more discussion about what this was trying to do would have
> helped. One thing that might have saved me is realizing that
> 'get_reference' was part of BranchFormat, not Branch.

Yes, part of the confusion comes from BranchExperimental which is a
Branch implementation that is its own format. Martin put this into the
code base as an experimental format to see how removing the separate
format classes might look. I think its presence at the moment is more
confusing than anything, as it is not homogenous with the other branch
types, nor does it offer any new features.

> > === modified file bzrlib/branch.py
> > --- bzrlib/branch.py
> > +++ bzrlib/branch.py
> > @@ -813,6 +813,18 @@
> >          """Return the current default format."""
> >          return klass._default_format
> >  
> > +    def get_reference(self, a_bzrdir):
> > +        """Get the target reference of the branch in a_bzrdir.
...
> 
> It took me a long time to understand this, until I merged the patch and
> saw that this was part of "BranchFormat" and not "Branch". Further, it
> is only implemented for "BranchReferenceFormat". I think it might be
> clearer as:

Agreed.

...          
> > +    def get_reference(self, a_bzrdir):
> > +        """See BranchFormat.get_reference()."""
> > +        transport = a_bzrdir.get_branch_transport(None)
> > +        return transport.get('location').read()
> >
> ^- Why is this defined 2 times? The first time it is a standard member
> of BranchFormat, and then as a class member of BranchExperimentalFormat.
> 
> It seems like it should be a classmethod of BranchFormat, and then it
> doesn't need to be redefined.
> 
> Either that, or it should always be a standard method (not class, and
> not static).

This is due to BranchExperimental being its own format (there is no
BranchExperimentalFormat class). One can view it as how transitional
code might look. regular methods on instances for normal Formats become
class methods on classes for Branches whose class is their format.

 
> > === modified file bzrlib/bzrdir.py
> > --- bzrlib/bzrdir.py
> > +++ bzrlib/bzrdir.py
> > @@ -420,6 +420,15 @@
> >                  raise errors.NoRepositoryPresent(self)
> >          raise errors.NoRepositoryPresent(self)
> >  
> > +    def get_branch_reference(self):
> > +        """Return the referenced URL for the branch in this bzrdir.
> > +
> > +        :raises NotBranchError: If there is no Branch.
> > +        :return: The URL the branch in this bzrdir references if it is a
> > +            reference branch, or None for regular branches.
> > +        """
> > +        return None
> > +
> 
> ^ I think:
> 
> If the branch in this bzrdir is a branch reference, return the target
> URL, else return None for regular branches.

Agreed.



> v- This doesn't seem to be "BoundSFTPBranch" anymore. Maybe
> "TestBoundNonLocalBranch" ?
> 
> > +class BoundSFTPBranch(TestCaseWithTransport):
> > +
> > +    def setUp(self):
> > +        TestCaseWithTransport.setUp(self)
> > +        self.vfs_transport_factory = MemoryServer
> > +        if self.transport_server is LocalURLServer:
> > +            self.transport_server = None

Agreed.

> I see tests for 'get_branch_reference' but I don't see tests for
> "find_branch_format".

Thats an

> You seem to have added 3 api functions
> 
> BranchFormat.get_reference
> BzrDir.get_branch_reference
> BzrDir.find_branch_format
> 
> They should all be mentioned in NEWS (if you are going to mention one of
> them). And they should all have tests.

I agree with this as well.

(BTW, while annotate blames me, I'm pretty sure this was jointly done
pair programming with Andrew; we happened to be on my machine :)).

-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/20070416/0ce6cfcc/attachment.pgp