> bazaar_identity has been changed to return 'lp:ubuntu/package' for source
> package branches associated to the development focus package.

My first thought on reading this was "for just the release pocket?", but I
guess we'll see.

> === modified file 'lib/lp/code/interfaces/branch.py'
> --- lib/lp/code/interfaces/branch.py	2009-07-15 01:45:35 +0000
> +++ lib/lp/code/interfaces/branch.py	2009-07-16 04:47:24 +0000
> @@ -1131,6 +1130,10 @@
>              'series': use_series.name}
>  
>      if branch.sourcepackage is not None:
> +        distro_package = branch.sourcepackage.distribution_sourcepackage
> +        linked_branch = ICanHasLinkedBranch(distro_package)
> +        if linked_branch.branch == branch:
> +            return lp_prefix + linked_branch.bzr_identity

What object traversal is really going on here?  And how will it impact getting
bazaar_identities for branch listings?

branch.sourcepackage -> No other queries
.distribution_sourcepackage -> Other queries?
Adaption -> other queries?

> === modified file 'lib/lp/code/model/linkedbranch.py'
> --- lib/lp/code/model/linkedbranch.py	2009-05-15 03:16:18 +0000
> +++ lib/lp/code/model/linkedbranch.py	2009-07-16 05:38:41 +0000
> @@ -30,6 +33,16 @@
>          """See `ICanHasLinkedBranch`."""
>          return self.product_series.branch
>  
> +    @property
> +    def bzr_identity(self):
> +        """See `ICanHasLinkedBranch`."""
> +        return '/'.join(
> +            [self.product_series.product.name, self.product_series.name])

I think that the bzr_identity here (and in following adapters) should really
have the lp: prefix too (from the config variable).  Otherwise bzr_identity
functions in different places return different types of strings.

Also, it shouldn't really return anything if there is not a branch set.

> @@ -45,6 +58,15 @@
>          """See `ICanHasLinkedBranch`."""
>          return ICanHasLinkedBranch(self.product.development_focus).branch
>  
> +    @property
> +    def bzr_identity(self):
> +        """See `ICanHasLinkedBranch`."""
> +        return self.product.name
> +
> +    def setBranch(self, branch, registrant=None):
> +        """See `ICanHasLinkedBranch`."""
> +        
ICanHasLinkedBranch(self.product.development_focus).setBranch(branch)

I think you should also pass the registrant through.  If we start to record
it, it'll save changing this later.

> +
>  
>  class PackageLinkedBranch:
>      """Implement a linked branch for a source package pocket."""
> @@ -61,3 +83,50 @@
>          package = self.suite_sourcepackage.sourcepackage
>          pocket = self.suite_sourcepackage.pocket
>          return package.getBranch(pocket)
> +
> +    @property
> +    def bzr_identity(self):
> +        """See `ICanHasLinkedBranch`."""
> +        return self.suite_sourcepackage.path
> +
> +    def setBranch(self, branch, registrant):
> +        """See `ICanHasLinkedBranch`."""
> +        package = self.suite_sourcepackage.sourcepackage
> +        pocket = self.suite_sourcepackage.pocket
> +        package.setBranch(pocket, branch, registrant)
> +
> +
> +class DistributionPackageLinkedBranch:
> +    """Implement a linked branch for an `IDistributionSourcePackage`."""
> +
> +    adapts(IDistributionSourcePackage)
> +    implements(ICanHasLinkedBranch)
> +
> +    def __init__(self, distribution_sourcepackage):
> +        self._distribution_sourcepackage = distribution_sourcepackage

Why use a leading underscore here?  None of the other adpaters do this.

If you think strongly that we should be using "private" variable, then go for
consistence and change the other adapters too.

> +
> +    @property
> +    def branch(self):
> +        """See `ICanHasLinkedBranch`."""
> +        development_package = (
> +            self._distribution_sourcepackage.development_version)
> +        if development_package is None:
> +            return None
> +        suite_sourcepackage = development_package.getSuiteSourcePackage(
> +            PackagePublishingPocket.RELEASE)
> +        return ICanHasLinkedBranch(suite_sourcepackage).branch
> +
> +    @property
> +    def bzr_identity(self):
> +        """See `ICanHasLinkedBranch`."""
> +        return '/'.join(
> +            [self._distribution_sourcepackage.distribution.name,
> +             self._distribution_sourcepackage.sourcepackagename.name])
> +
> +    def setBranch(self, branch, registrant):
> +        """See `ICanHasLinkedBranch`."""
> +        development_package = (
> +            self._distribution_sourcepackage.development_version)

In the branch property you check to see if the developmen_package is None, but
here you don't.  I'm assuming that either you want to check here, or not check
above.  Which is it?


> +        suite_sourcepackage = development_package.getSuiteSourcePackage(
> +            PackagePublishingPocket.RELEASE)
> +        ICanHasLinkedBranch(suite_sourcepackage).setBranch(branch, 
registrant)
> 
> === modified file 'lib/lp/code/model/tests/test_branch.py'
> --- lib/lp/code/model/tests/test_branch.py	2009-07-08 04:57:51 +0000
> +++ lib/lp/code/model/tests/test_branch.py	2009-07-16 03:22:24 +0000
> @@ -418,9 +418,10 @@
>          # If a branch is the development focus branch for a product, then 
it's
>          # bzr identity is lp:product.
>          branch = self.factory.makeProductBranch()
> -        product = branch.product
> -        removeSecurityProxy(product).development_focus.branch = branch
> -        self.assertBzrIdentity(branch, product.name)
> +        product = removeSecurityProxy(branch.product)
> +        linked_branch = ICanHasLinkedBranch(product)
> +        linked_branch.setBranch(branch)
> +        self.assertBzrIdentity(branch, linked_branch.bzr_identity)

Changing the bzr_identity of the linked_branch to be complete should just mean
changing the assertBzrIdentity :)

>      def test_linked_to_series_and_dev_focus(self):
> @@ -448,33 +450,54 @@
>          # branch for a series, the bzr identity will be the storter of the 
two
>          # URLs.
>          branch = self.factory.makeProductBranch()
> -        product = branch.product
> -        removeSecurityProxy(product).development_focus.branch = branch
> -        series = self.factory.makeProductSeries(product=product)
> -        series.branch = branch
> -        self.assertBzrIdentity(branch, product.name)
> +        series = self.factory.makeProductSeries(product=branch.product)
> +        product_link = ICanHasLinkedBranch(
> +            removeSecurityProxy(branch.product))
> +        series_link = ICanHasLinkedBranch(series)
> +        product_link.setBranch(branch)
> +        series_link.setBranch(branch)
> +        self.assertBzrIdentity(branch, product_link.bzr_identity)
>  
>      def test_junk_branch_always_unique_name(self):
>          # For junk branches, the bzr identity is always based on the unique
>          # name of the branch, even if it's linked to a product, product 
series
>          # or whatever.
>          branch = self.factory.makePersonalBranch()
> -        product = self.factory.makeProduct()
> -        removeSecurityProxy(product).development_focus.branch = branch
> +        product = removeSecurityProxy(self.factory.makeProduct())
> +        ICanHasLinkedBranch(product).setBranch(branch)

This one has always bothered me.  We should not allow junk branches to be
linked rather than just saying that their links don't show it.  What do you
think?

>          self.assertBzrIdentity(branch, branch.unique_name)
>  
> -    def test_linked_to_package_release(self):
> -        # If a branch is linked to the release pocket of a package, then 
the
> +    def test_linked_to_package(self):
> +        # If a branch is linked to a pocket of a package, then the
>          # bzr identity is the path to that package.
>          branch = self.factory.makePackageBranch()
> +        # Have to pick something that's not RELEASE in order to guarantee 
that
> +        # it's not the dev focus source package.
> +        pocket = PackagePublishingPocket.BACKPORTS
> +        linked_branch = ICanHasLinkedBranch(
> +            branch.sourcepackage.getSuiteSourcePackage(pocket))
>          registrant = getUtility(
>              ILaunchpadCelebrities).ubuntu_branches.teamowner
>          login_person(registrant)

To save with the login, logout dance, how about we just remove the security
proxy as with the other tests, but still pass the registrant through.

> -        branch.sourcepackage.setBranch(
> -            PackagePublishingPocket.RELEASE, branch, registrant)
> +        linked_branch.setBranch(branch, registrant)
>          logout()
>          login(ANONYMOUS)
> -        self.assertBzrIdentity(branch, branch.sourcepackage.path)
> +        self.assertBzrIdentity(branch, linked_branch.bzr_identity)
> +
> +    def test_linked_to_dev_package(self):
> +        # If a branch is linked to the development focus version of a 
package
> +        # then the bzr identity is distro/package.
> +        sourcepackage = self.factory.makeSourcePackage()
> +        distro_package = sourcepackage.distribution_sourcepackage
> +        branch = self.factory.makePackageBranch(
> +            sourcepackage=distro_package.development_version)
> +        linked_branch = ICanHasLinkedBranch(distro_package)
> +        registrant = getUtility(
> +            ILaunchpadCelebrities).ubuntu_branches.teamowner
> +        run_with_login(
> +            registrant,
> +            linked_branch.setBranch, branch, registrant)

Again, here, lets not complicate the test by worrying about who is doing the
linking.  We aren't testing the security policies here, so let's just remove
the security proxy.

> +        self.assertBzrIdentity(branch, linked_branch.bzr_identity)
> === modified file 'lib/lp/code/model/tests/test_branchlookup.py'
> --- lib/lp/code/model/tests/test_branchlookup.py	2009-06-02 13:52:48 +0000
> +++ lib/lp/code/model/tests/test_branchlookup.py	2009-07-16 05:52:44 +0000
> @@ -455,6 +470,24 @@
>              (branch, 'foo/bar/baz'),
>              self.branch_lookup.getByLPPath(path))
>  
> +    def test_resolve_distro_package_branch(self):
> +        # getByLPPath returns the branch associated with the distribution
> +        # source package referred to by the path.
> +        sourcepackage = self.factory.makeSourcePackage()
> +        branch = 
self.factory.makePackageBranch(sourcepackage=sourcepackage)
> +        distro_package = sourcepackage.distribution_sourcepackage
> +        ubuntu_branches = getUtility(ILaunchpadCelebrities).ubuntu_branches
> +        registrant = ubuntu_branches.teamowner
> +        run_with_login(
> +            registrant,
> +            ICanHasLinkedBranch(distro_package).setBranch, branch, 
registrant)
> +        self.assertEqual(
> +            (branch, None),
> +            self.branch_lookup.getByLPPath(
> +                '%s/%s' % (
> +                    distro_package.distribution.name,
> +                    distro_package.sourcepackagename.name)))
> +
>      def test_no_product_series_branch(self):
>          # getByLPPath raises `NoLinkedBranch` if there's no branch 
registered
>          # linked to the requested series.
> @@ -501,6 +534,12 @@
>              self.branch_lookup.getByLPPath, distribution.name)
>          self.assertEqual(distribution, exception.component)
>  
> +    def test_distribution_with_no_series(self):
> +        distro_package = self.factory.makeDistributionSourcePackage()
> +        path = ICanHasLinkedBranch(distro_package).bzr_identity
> +        self.assertRaises(
> +            NoLinkedBranch, self.branch_lookup.getByLPPath, path)
> +
>      def test_project_linked_branch(self):
>          # Projects cannot have linked branches, so `getByLPPath` raises a
>          # `CannotHaveLinkedBranch` error if we try to get the linked branch
> 
> === modified file 'lib/lp/code/model/tests/test_linkedbranch.py'
> --- lib/lp/code/model/tests/test_linkedbranch.py	2009-05-24 18:09:51 +0000
> +++ lib/lp/code/model/tests/test_linkedbranch.py	2009-07-16 05:38:41 +0000
> @@ -18,11 +18,11 @@
>  from lp.testing import run_with_login, TestCaseWithFactory
>  
>  
> -class TestLinkedBranch(TestCaseWithFactory):
> +class TestProductSeriesLinkedBranch(TestCaseWithFactory):
>  
>      layer = DatabaseFunctionalLayer
>  
> -    def test_product_series(self):
> +    def test_branch(self):
>          # The linked branch of a product series is its branch attribute.
>          product_series = self.factory.makeProductSeries()
>          product_series.branch = self.factory.makeProductBranch(
> @@ -30,7 +30,29 @@
>          self.assertEqual(
>              product_series.branch, 
ICanHasLinkedBranch(product_series).branch)
>  
> -    def test_product(self):
> +    def test_setBranch(self):
> +        # setBranch sets the linked branch of the product series.
> +        product_series = self.factory.makeProductSeries()
> +        branch = self.factory.makeProductBranch(
> +            product=product_series.product)
> +        ICanHasLinkedBranch(product_series).setBranch(branch)
> +        self.assertEqual(branch, product_series.branch)
> +
> +    def test_bzr_identity(self):
> +        # The bzr_identity of a product series linked branch is
> +        # product/product_series.
> +        product_series = self.factory.makeProductSeries()
> +        bzr_identity = '%s/%s' % (
> +            product_series.product.name, product_series.name)
> +        self.assertEqual(
> +            bzr_identity, ICanHasLinkedBranch(product_series).bzr_identity)

If you are against changing the implementation of bzr_identity for
ICanHasLinkedBranch then perhaps change it to bzr_identity_path or bzr_path?

> === modified file 'lib/lp/registry/interfaces/distributionsourcepackage.py'
> --- lib/lp/registry/interfaces/distributionsourcepackage.py	2009-07-10 
15:38:46 +0000
> +++ lib/lp/registry/interfaces/distributionsourcepackage.py	2009-07-16 
04:48:10 +0000
> @@ -68,6 +68,10 @@
>          "The list of all releases of this source package "
>          "in this distribution.")
>  
> +    development_version = Attribute(
> +        'The development version of this source package. None if there is 
no '
> +        'such package.')

When exactly would None occur?

> === modified file 'lib/lp/registry/model/distributionsourcepackage.py'
> --- lib/lp/registry/model/distributionsourcepackage.py	2009-07-10 15:38:46 
+0000
> +++ lib/lp/registry/model/distributionsourcepackage.py	2009-07-16 04:48:10 
+0000
> @@ -89,6 +89,14 @@
>              self.sourcepackagename.name, self.distribution.displayname)
>  
>      @property
> +    def development_version(self):
> +        """See `IDistributionSourcePackage`."""
> +        series = self.distribution.currentseries

If a distribution has at least one series, is currentseries always set?

> +        if series is None:
> +            return None
> +        return series.getSourcePackage(self.sourcepackagename)
> +
> +    @property
>      def _self_in_database(self):
>          """Return the equivalent database-backed record of self."""
>          # XXX: allenap 2008-11-13 bug=297736: This is a temporary
> === modified file 'lib/lp/registry/tests/test_distributionsourcepackage.py'
> --- lib/lp/registry/tests/test_distributionsourcepackage.py	2009-07-10 
15:38:46 +0000
> +++ lib/lp/registry/tests/test_distributionsourcepackage.py	2009-07-15 
07:55:56 +0000
> @@ -109,5 +109,25 @@
>  
>          self.assertEqual(related_archive_names, ['gedit-nightly'])
>  
> +    def test_development_version(self):
> +        # IDistributionSourcePackage.development_version is the 
ISourcePackage
> +        # for the current series of the distribution.
> +        dsp = self.factory.makeDistributionSourcePackage()
> +        series = 
self.factory.makeDistroRelease(distribution=dsp.distribution)
> +        self.assertEqual(series, dsp.distribution.currentseries)
> +        development_version = 
dsp.distribution.currentseries.getSourcePackage(
> +            dsp.sourcepackagename)
> +        self.assertEqual(development_version, dsp.development_version)
> +
> +    def test_development_version_no_current_series(self):
> +        # IDistributionSourcePackage.development_version is the 
ISourcePackage
> +        # for the current series of the distribution.
> +        dsp = self.factory.makeDistributionSourcePackage()
> +        currentseries = dsp.distribution.currentseries
> +        # The current series is None by default.

The interesting question becomes "is it ever None if there are any distro 
series?"

> +        self.assertIs(None, currentseries)
> +        self.assertEqual(None, dsp.development_version)
> +
> +
>  def test_suite():
>      return unittest.TestLoader().loadTestsFromName(__name__)
> 


On the whole, a nice branch to review.

The main questions I have resolve around the currentseries for a distribution,
and the actual content of ICanHasLinkedBranch.bzr_identity.

  review needs-info