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

[John Arbash Meinel]

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.


> ------------------------------------------------------------------------
> 
> # Bazaar revision bundle v0.9
> #
> # message:
> #   Some miscellaneous new APIs, tests and other changes from the hpss branch.
> # committer: Andrew Bennetts <andrew.bennetts at canonical.com>
> # date: Fri 2007-04-13 17:18:57.884000063 +1000
> 
> === modified file NEWS
> --- NEWS
> +++ NEWS
> @@ -42,9 +42,12 @@
>        to be in bzrlib/transport/smart.py.  (Andrew Bennetts)
>   
>      * The ``lock_write`` method of ``LockableFiles``, ``Repository`` and
> -     ``Branch`` now accept a ``token`` keyword argument, so that separate
> -     instances of those objects can share a lock if it has the right token.
> -     (Andrew Bennetts, Robert Collins)
> +      ``Branch`` now accept a ``token`` keyword argument, so that separate
> +      instances of those objects can share a lock if it has the right token.
> +      (Andrew Bennetts, Robert Collins)
> +
> +    * New method 'get_branch_reference' on 'BzrDir' allows the detection of
> +      branch references - which the smart server component needs.
>  
>    BUGFIXES:
>  
> 
> === 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.
> +
> +        format probing must have been completed before calling
> +        this method - it is assumed that the format of the branch
> +        in a_bzrdir is correct.
> +
> +        :param a_bzrdir: The bzrdir to get the branch data from.
> +        :return: None if the branch is not a reference branch.
> +        """
> +        return None
> +


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:

def get_reference_target(self, a_bzrdir):
   """If this format is a reference to a branch, return the target url.

   Format probing must have already been done before calling this
   method. It is assumed that this is the correct format for the branch
   in 'a_bzrdir'.

   :param a_bzrdir: The bzrdir where this format resides.
   :return: The URL to the referenced branch or None if this is not a
       branch reference.
   """
   return None


>      def get_format_string(self):
>          """Return the ASCII format string that identifies this format."""
>          raise NotImplementedError(self.get_format_string)
> @@ -1125,6 +1137,11 @@
>          """See BranchFormat.get_format_description()."""
>          return "Checkout reference format 1"
>          
> +    def get_reference(self, a_bzrdir):
> +        """See BranchFormat.get_reference()."""
> +        transport = a_bzrdir.get_branch_transport(None)
> +        return transport.get('location').read()
> +
>      def initialize(self, a_bzrdir, target_branch=None):
>          """Create a branch of this format in a_bzrdir."""
>          if target_branch is None:
> @@ -1745,6 +1762,19 @@
>          return "Experimental branch format"

...
>      @classmethod
> +    def get_reference(cls, a_bzrdir):
> +        """Get the target reference of the branch in a_bzrdir.
> +
> +        format probing must have been completed before calling
> +        this method - it is assumed that the format of the branch
> +        in a_bzrdir is correct.
> +
> +        :param a_bzrdir: The bzrdir to get the branch data from.
> +        :return: None if the branch is not a reference branch.
> +        """
> +        return None
> +

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


> +    @classmethod
>      def _initialize_control_files(cls, a_bzrdir, utf8_files, lock_filename,
>              lock_class):
>          branch_transport = a_bzrdir.get_branch_transport(cls)
> 
> === 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.


>      def get_branch_transport(self, branch_format):
>          """Get the transport for use by branch format in this BzrDir.
>  


...



...

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


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

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.

John
=:->