Launchpad itself

Merge lp://staging/~jml/launchpad/ubuntu-package-bug-345737 into lp://staging/launchpad

ubuntu-package-bug-345737
Merge into devel

Proposed by Jonathan Lange on 2009-07-16

Status:

Merged

Merged at revision:

not available

Proposed branch:

lp://staging/~jml/launchpad/ubuntu-package-bug-345737

Merge into:

lp://staging/launchpad

Diff against target:

None lines

To merge this branch:

bzr merge lp://staging/~jml/launchpad/ubuntu-package-bug-345737

High

Fix Released

Link a bug report

Reviewer	Review Type	Date Requested	Status
Tim Penhey (community)		2009-07-16	Approve on 2009-07-19
Review via email: mp+8868@code.staging.launchpad.net

Revision history for this message

Jonathan Lange (jml) wrote on 2009-07-16:

This branch makes lp:ubuntu/foo resolve to the branch associated with the development version of the 'foo' package on Ubuntu, defined as the branch linked to the Release pocket of that package of the current series of that distribution.

To do this, I've registered a new adapter from DistributionSourcePackage to ICanHasLinkedBranch. While I was there, I also added a bzr_identity attribute to ICanHasLinkedBranch, as well as a setBranch method. Neither of these are deeply associated with the change, but they are both things that are related & I've wanted to do for a while.

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

I had to change the linked branch traversal code to pass segments into the traversable adapters. This was so I could raise context-aware errors in the case of 'ubuntu/doesntexist' and 'ubuntu/doesntexist/openssh'.

I've also increased the interlinking of distro-related registry objects. In particular, SourcePackage now links to DistributionSourcePackage (the latter being conceptually a parent of the former), and DistributionSourcePackage links to the development source package.

There are a lot of pyflakes & pep8 cleanups along the way, inflating the size of the diff.

I've also based this branch on lp:~jml/launchpad/remove-getitem. This was necessary in order to get the adapt() method behaving correctly with DistributionSourcePackage.

Revision history for this message

Tim Penhey (thumper) wrote on 2009-07-17:

Download full text (16.6 KiB)

> 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`."""
> +
...

> 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

review: Needs Information

Revision history for this message

Jonathan Lange (jml) wrote on 2009-07-17:

Download full text (19.2 KiB)

On Fri, Jul 17, 2009 at 11:00 AM, Tim Penhey<email address hidden> wrote:
> Review: Needs Information
>> 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.
>

Yeah, just for the release pocket.

>> === 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?

No queries, it's a virtual object.

> Adaption -> other queries?
>

No queries in adaptation either.

However, getting the bzr_identity from the adapted object requires
loading the currentseries from the DistroSeries table. You probably
want to have a look at Distribution.currentseries anyway to resolve
some of your other questions.

If possible, I'd rather land this branch now with some performance
issues than in 2 months time.

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

Fair enough, although in some ways I'd prefer keeping the current
behaviour and picking a different name. It seems wrong to imbue this
code with knowledge of the name of the server.

Oh, I see later on you suggest the names 'bzr_path' and
'bzr_identity_path' for a property with the current behaviour. I like
that.

Changed.

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

Why do you say that?

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

On Fri, Jul 17, 2009 at 11:00 AM, Tim Penhey<tim@penhey.net> wrote:
> Review: Needs Information
>> 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.
>

Yeah, just for the release pocket.

>> === 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?

No queries, it's a virtual object.

> Adaption -> other queries?
>

No queries in adaptation either.

If possible, I'd rather land this branch now with some performance
issues than in 2 months time.

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

Fair enough, although in some ways I'd prefer keeping the current
behaviour and picking a different name. It seems wrong to imbue this
code with knowledge of the name of the server.

Oh, I see later on you suggest the names 'bzr_path' and
'bzr_identity_path' for a property with the current behaviour. I like
that.

Changed.

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

Why do you say that?

>> @@ -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)
>

Your wrapping here is very weird.

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

Good point. Changed.

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

Because all of the other adapters ought to do it.

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

Done.

>> +
>> +    @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?
>

Well, what this really means is that I'm missing a test. If
development_package is None, we can't set the branch, but there's no
defined behaviour for what *does* happen.

I've added a test to make sure this raises NoSuchDistroSeries -- I
couldn't think of a better exception -- and have changed the code
accordingly.

>
>> +        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 :)
>

Well, only because I've changed _all_ of the tests here to use
ICanHasLinkedBranch.

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

As a general principle, I think we should do as little special-casing
of +junk as possible. It always comes back to bite us.

I personally think that we can do without this special case at all.
However, if pressed, I'd favour simply preventing junk branches from
being linked. I think it's for another branch though.

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

Doesn't work. Internally, the link is made by code that looks like:
  getUtility(ISeriesSourcePackageBranchSet).new(branch, pocket,
distroseries, sourcepackagename)

getUtility guarantees that the object is secured, and 'new' requires
lp.Edit permissions.

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

Ditto.

>> +        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?
>

Yes. I'll rename it to bzr_path. See earlier comment.

>> === 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?
>

When there's no current series for the distribution. Updated to say so.

>> === 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?
>

Yes.

For evidence of this, see the implementation of Distribution.currentseries.

>> +        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?"
>

See above.

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

Thanks.

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

Cool. I hope they're addressed here.

jml

Revision history for this message

Tim Penhey (thumper) wrote on 2009-07-19:

On Fri, 17 Jul 2009 20:03:07 Jonathan Lange wrote:
> If possible, I'd rather land this branch now with some performance
> issues than in 2 months time.

I agree.

> > Also, it shouldn't really return anything if there is not a branch set.
>
> Why do you say that?

OK, I guess not. As long as the documentation says that the bzr_path is the
path of the branch if there was a branch set.

> > ICanHasLinkedBranch(self.product.development_focus).setBranch(branch)
>
> Your wrapping here is very weird.

I blame my mail client.

> > 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?
>
> As a general principle, I think we should do as little special-casing
> of +junk as possible. It always comes back to bite us.
>
> I personally think that we can do without this special case at all.
> However, if pressed, I'd favour simply preventing junk branches from
> being linked. I think it's for another branch though.

Definitely another branch.

> > 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.
>
> Doesn't work. Internally, the link is made by code that looks like:
> getUtility(ISeriesSourcePackageBranchSet).new(branch, pocket,
> distroseries, sourcepackagename)
>
> getUtility guarantees that the object is secured, and 'new' requires
> lp.Edit permissions.

Aah, ok. I wish we had a nicer way to test this without the logins, but not
for this branch.

> > The main questions I have resolve around the currentseries for a
> > distribution, and the actual content of ICanHasLinkedBranch.bzr_identity.
>
> Cool. I hope they're addressed here.

Yes. I wish that distribution always had one series by default like a
Product, but I think you've done as well as you can given that "some"
distributions may well not have set any series.

review approve

On Fri, 17 Jul 2009 20:03:07 Jonathan Lange wrote:
> If possible, I'd rather land this branch now with some performance
> issues than in 2 months time.

I agree.

> > Also, it shouldn't really return anything if there is not a branch set.
>
> Why do you say that?

OK, I guess not.  As long as the documentation says that the bzr_path is the 
path of the branch if there was a branch set.

> > ICanHasLinkedBranch(self.product.development_focus).setBranch(branch)
>
> Your wrapping here is very weird.

I blame my mail client.

> > 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?
>
> As a general principle, I think we should do as little special-casing
> of +junk as possible. It always comes back to bite us.
>
> I personally think that we can do without this special case at all.
> However, if pressed, I'd favour simply preventing junk branches from
> being linked. I think it's for another branch though.

Definitely another branch.

> > 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.
>
> Doesn't work. Internally, the link is made by code that looks like:
>   getUtility(ISeriesSourcePackageBranchSet).new(branch, pocket,
> distroseries, sourcepackagename)
>
> getUtility guarantees that the object is secured, and 'new' requires
> lp.Edit permissions.

Aah, ok.  I wish we had a nicer way to test this without the logins, but not 
for this branch.

> > The main questions I have resolve around the currentseries for a
> > distribution, and the actual content of ICanHasLinkedBranch.bzr_identity.
>
> Cool. I hope they're addressed here.

Yes.  I wish that distribution always had one series by default like a 
Product, but I think you've done as well as you can given that "some" 
distributions may well not have set any series.

review approve

review: Approve

Revision history for this message

Jonathan Lange (jml) wrote on 2009-07-19:

On Mon, Jul 20, 2009 at 8:48 AM, Tim Penhey<email address hidden> wrote:
> Review: Approve
> On Fri, 17 Jul 2009 20:03:07 Jonathan Lange wrote:
>> If possible, I'd rather land this branch now with some performance
>> issues than in 2 months time.
>
> I agree.
>
>> > Also, it shouldn't really return anything if there is not a branch set.
>>
>> Why do you say that?
>
> OK, I guess not. As long as the documentation says that the bzr_path is the
> path of the branch if there was a branch set.
>

Done.

Landing now.

jml

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk

Subscribers

People subscribed via source and target branches

to all changes:

Barki Mustapha

Celso Providelo

Christian Reis

Christy Awad

Colin Watson

Harpianto,ANDI

James Troup

John A Meinel

Jonathan Lange

Kevin bush

Launchpad code reviewers

Launchpad code reviewers from Canonical

Matthew Tanner

Maximiliano Bertacchini

Oguz Ersoz

Simon Brakhane

Ubuntu-BR DevOps

William Grant

alhawiti

api.ng

pedro cavazos

todaioan

wenjingwen

to status/vote changes:

Tzaddi

Tzaddi Belding

Launchpad itself

Merge lp://staging/~jml/launchpad/ubuntu-package-bug-345737 into lp://staging/launchpad

Commit message

Description of the change

Preview Diff

Subscribers

 === modified file 'lib/lp/code/configure.zcml'
 --- lib/lp/code/configure.zcml	2009-07-15 01:45:35 +0000
 +++ lib/lp/code/configure.zcml	2009-07-16 04:47:24 +0000
@@ -801,6 +801,7 @@
    <adapter factory="lp.code.model.linkedbranch.ProductSeriesLinkedBranch" />
    <adapter factory="lp.code.model.linkedbranch.ProductLinkedBranch" />
    <adapter factory="lp.code.model.linkedbranch.PackageLinkedBranch" />
++  <adapter factory="lp.code.model.linkedbranch.DistributionPackageLinkedBranch" />
    <lp:help-folder
        folder="help" type="canonical.launchpad.layers.CodeLayer" />
 === 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
@@ -64,6 +64,7 @@
+     )
  from lp.code.interfaces.branchlookup import IBranchLookup
  from lp.code.interfaces.branchtarget import IHasBranchTarget
++from lp.code.interfaces.linkedbranch import ICanHasLinkedBranch
  from canonical.launchpad.interfaces.launchpad import ILaunchpadCelebrities
  from lp.registry.interfaces.role import IHasOwner
  from lp.registry.interfaces.person import IPerson
@@ -81,6 +82,7 @@
  class BranchCreationException(Exception):
      """Base class for branch creation exceptions."""
++
  class BranchExists(BranchCreationException):
      """Raised when creating a branch that already exists."""
@@ -738,8 +740,7 @@
                  'development focus, then the result should be lp:product.  '
                  'If the branch is related to a series, then '
                  'lp:product/series.  Otherwise the result is '
--                'lp:~user/product/branch-name.'
--                )))
++                'lp:~user/product/branch-name.')))
      def addToLaunchBag(launchbag):
          """Add information about this branch to `launchbag'.
@@ -1083,8 +1084,6 @@
      last_scanned_id = Attribute("The revision id of the tip revision.")
--
--
  class IBranchCloud(Interface):
      """A utility to generate data for branch clouds.
@@ -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
          suite_sourcepackages = branch.associatedSuiteSourcePackages()
          # Take the first link if there is one.
          if len(suite_sourcepackages) > 0:
 === modified file 'lib/lp/code/interfaces/branchlookup.py'
 --- lib/lp/code/interfaces/branchlookup.py	2009-05-15 03:16:18 +0000
 +++ lib/lp/code/interfaces/branchlookup.py	2009-07-16 05:00:18 +0000
@@ -16,8 +16,14 @@
  class ILinkedBranchTraversable(Interface):
      """A thing that can be traversed to find a thing linked to a branch."""
--    def traverse(self, name):
--        """Return the object beneath this one that matches 'name'."""
++    def traverse(self, name, segments):
++        """Return the object beneath this one that matches 'name'.
++
++        :param name: The name of the object being traversed to.
++        :param segments: Remaining path segments.
++        :return: An `ILinkedBranchTraversable` object if traversing should
++            continue, an `ICanHasLinkedBranch` object otherwise.
++        """
  class ILinkedBranchTraverser(Interface):
@@ -40,7 +46,7 @@
          :return: One of
              * `IProduct`
              * `IProductSeries`
--            * (ISourcePackage, PackagePublishingPocket)
++            * `ISuiteSourcePackage`
              * `IDistributionSourcePackage`
          """
 === modified file 'lib/lp/code/interfaces/linkedbranch.py'
 --- lib/lp/code/interfaces/linkedbranch.py	2009-05-24 18:09:51 +0000
 +++ lib/lp/code/interfaces/linkedbranch.py	2009-07-16 01:58:59 +0000
@@ -23,6 +23,16 @@
      """Something that has a linked branch."""
      branch = Attribute("The linked branch.")
++    bzr_identity = Attribute('The Bazaar branch path for the linked branch.')
++
++    def setBranch(branch, registrant=None):
++        """Set the linked branch.
++
++        :param branch: An `IBranch`. After calling this,
++            `ICanHasLinkedBranch.branch` will be 'branch'.
++        :param registrant: The `IPerson` linking the branch. Not used by all
++            implementations.
++        """
  class CannotHaveLinkedBranch(Exception):
 === modified file 'lib/lp/code/model/branchlookup.py'
 --- lib/lp/code/model/branchlookup.py	2009-05-15 03:16:18 +0000
 +++ lib/lp/code/model/branchlookup.py	2009-07-16 05:39:51 +0000
@@ -29,7 +29,7 @@
  from lp.code.interfaces.linkedbranch import get_linked_branch, NoLinkedBranch
  from lp.registry.interfaces.distribution import IDistribution
  from lp.registry.interfaces.distroseries import (
--    IDistroSeries, IDistroSeriesSet)
++    IDistroSeries, IDistroSeriesSet, NoSuchDistroSeries)
  from lp.registry.interfaces.pillar import IPillarNameSet
  from lp.registry.interfaces.product import (
      InvalidProductName, IProduct, NoSuchProduct)
@@ -65,7 +65,7 @@
      implements(ILinkedBranchTraversable)
--    def traverse(self, name):
++    def traverse(self, name, segments):
          """See `ITraversable`.
          :raise NoSuchProduct: If 'name' doesn't match an existing pillar.
@@ -102,7 +102,7 @@
      adapts(IProduct)
      implements(ILinkedBranchTraversable)
--    def traverse(self, name):
++    def traverse(self, name, segments):
          """See `ITraversable`.
          :raises NoSuchProductSeries: if 'name' doesn't match an existing
@@ -124,12 +124,18 @@
      adapts(IDistribution)
      implements(ILinkedBranchTraversable)
--    def traverse(self, name):
++    def traverse(self, name, segments):
          """See `ITraversable`."""
--        # XXX: JonathanLange 2009-03-20 spec=package-branches bug=345737: This
--        # could also try to find a package and then return a reference to its
--        # development focus.
--        return getUtility(IDistroSeriesSet).fromSuite(self.context, name)
++        try:
++            return getUtility(IDistroSeriesSet).fromSuite(self.context, name)
++        except NoSuchDistroSeries:
++            sourcepackage = self.context.getSourcePackage(name)
++            if sourcepackage is None:
++                if segments:
++                    raise
++                else:
++                    raise NoSuchSourcePackageName(name)
++            return sourcepackage
  class DistroSeriesTraversable:
@@ -145,7 +151,7 @@
          self.distroseries = distroseries
          self.pocket = pocket
--    def traverse(self, name):
++    def traverse(self, name, segments):
          """See `ITraversable`."""
          sourcepackage = self.distroseries.getSourcePackage(name)
          if sourcepackage is None:
@@ -170,7 +176,7 @@
          traversable = RootTraversable()
          while segments:
              name = segments.pop(0)
--            context = traversable.traverse(name)
++            context = traversable.traverse(name, segments)
              traversable = adapt(context, ILinkedBranchTraversable)
              if traversable is None:
                  break
@@ -332,7 +338,7 @@
              namespace_set = getUtility(IBranchNamespaceSet)
              segments = iter(path.lstrip('~').split('/'))
              branch = namespace_set.traverse(segments)
--            suffix =  '/'.join(segments)
++            suffix = '/'.join(segments)
              if not check_permission('launchpad.View', branch):
                  raise NoSuchBranch(path)
              if suffix == '':
 === 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
@@ -11,9 +11,12 @@
  from zope.interface import implements
  from lp.code.interfaces.linkedbranch import ICanHasLinkedBranch
++from lp.registry.interfaces.distributionsourcepackage import (
++    IDistributionSourcePackage)
  from lp.registry.interfaces.product import IProduct
  from lp.registry.interfaces.productseries import IProductSeries
  from lp.registry.interfaces.suitesourcepackage import ISuiteSourcePackage
++from lp.soyuz.interfaces.publishing import PackagePublishingPocket
  class ProductSeriesLinkedBranch:
@@ -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])
++
++    def setBranch(self, branch, registrant=None):
++        """See `ICanHasLinkedBranch`."""
++        self.product_series.branch = branch
++
  class ProductLinkedBranch:
      """Implement a linked branch for a product."""
@@ -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)
++
  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
++
++    @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)
++        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
@@ -20,49 +20,49 @@
  from canonical.config import config
  from canonical.database.constants import UTC_NOW
  from canonical.launchpad import _
--from lp.code.model.branch import (
--    ClearDependentBranch, ClearOfficialPackageBranch, ClearSeriesBranch,
--    DeleteCodeImport, DeletionCallable, DeletionOperation,
--    update_trigger_modified_fields)
--from lp.code.model.branchjob import (
--    BranchDiffJob, BranchJob, BranchJobType, ReclaimBranchSpaceJob)
--from lp.code.model.branchmergeproposal import (
--    BranchMergeProposal)
--from lp.bugs.model.bugbranch import BugBranch
--from lp.code.model.codeimport import CodeImport, CodeImportSet
--from lp.code.model.codereviewcomment import CodeReviewComment
--from lp.registry.model.product import ProductSet
--from lp.blueprints.model.specificationbranch import (
--    SpecificationBranch)
--from lp.registry.model.sourcepackage import SourcePackage
  from canonical.launchpad.ftests import (
      ANONYMOUS, login, login_person, logout, syncUpdate)
--from lp.bugs.interfaces.bug import CreateBugParams, IBugSet
++from canonical.launchpad.interfaces.launchpad import ILaunchpadCelebrities
++from canonical.launchpad.webapp.interfaces import IOpenLaunchBag
++from canonical.testing import DatabaseFunctionalLayer, LaunchpadZopelessLayer
++
  from lp.blueprints.interfaces.specification import (
      ISpecificationSet, SpecificationDefinitionStatus)
++from lp.blueprints.model.specificationbranch import (
++    SpecificationBranch)
++from lp.bugs.interfaces.bug import CreateBugParams, IBugSet
++from lp.bugs.model.bugbranch import BugBranch
  from lp.code.bzr import BranchFormat, RepositoryFormat
  from lp.code.enums import (
      BranchLifecycleStatus, BranchSubscriptionNotificationLevel, BranchType,
      BranchVisibilityRule, CodeReviewNotificationLevel)
  from lp.code.interfaces.branch import (
      BranchCannotBePrivate, BranchCannotBePublic,
--    CannotDeleteBranch)
++    CannotDeleteBranch, DEFAULT_BRANCH_STATUS_IN_LISTING)
++from lp.code.interfaces.branchlookup import IBranchLookup
++from lp.code.interfaces.branchnamespace import IBranchNamespaceSet
  from lp.code.interfaces.branchmergeproposal import InvalidBranchMergeProposal
++from lp.code.interfaces.linkedbranch import ICanHasLinkedBranch
  from lp.code.interfaces.seriessourcepackagebranch import (
      IFindOfficialBranchLinks)
++from lp.code.model.branch import (
++    ClearDependentBranch, ClearOfficialPackageBranch, ClearSeriesBranch,
++    DeleteCodeImport, DeletionCallable, DeletionOperation,
++    update_trigger_modified_fields)
++from lp.code.model.branchjob import (
++    BranchDiffJob, BranchJob, BranchJobType, ReclaimBranchSpaceJob)
++from lp.code.model.branchmergeproposal import (
++    BranchMergeProposal)
++from lp.code.model.codeimport import CodeImport, CodeImportSet
++from lp.code.model.codereviewcomment import CodeReviewComment
  from lp.registry.interfaces.person import IPersonSet
  from lp.registry.interfaces.product import IProductSet
--from lp.code.interfaces.branch import DEFAULT_BRANCH_STATUS_IN_LISTING
--from lp.code.interfaces.branchlookup import IBranchLookup
--from lp.code.interfaces.branchnamespace import IBranchNamespaceSet
--from canonical.launchpad.interfaces.launchpad import ILaunchpadCelebrities
++from lp.registry.model.product import ProductSet
++from lp.registry.model.sourcepackage import SourcePackage
  from lp.soyuz.interfaces.publishing import PackagePublishingPocket
  from lp.testing import (
      run_with_login, TestCase, TestCaseWithFactory, time_counter)
  from lp.testing.factory import LaunchpadObjectFactory
--from canonical.launchpad.webapp.interfaces import IOpenLaunchBag
--
--from canonical.testing import DatabaseFunctionalLayer, LaunchpadZopelessLayer
  class TestCodeImport(TestCase):
@@ -94,7 +94,7 @@
      def _makeRevision(self, revno):
          # Make a revision and add it to the branch.
          rev = self.factory.makeRevision()
--        br = self.branch.createBranchRevision(revno, rev)
++        self.branch.createBranchRevision(revno, rev)
          return rev
      def testGetBySequenceNumber(self):
@@ -117,7 +117,7 @@
          self.assertEqual(1, branch_revision.sequence)
      def testNonExistant(self):
--        rev1 = self._makeRevision(1)
++        self._makeRevision(1)
          self.assertTrue(self.branch.getBranchRevision(sequence=2) is None)
          rev2 = self.factory.makeRevision()
          self.assertTrue(self.branch.getBranchRevision(revision=rev2) is None)
@@ -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)
      def test_linked_to_product_series(self):
          # If a branch is the development focus branch for a product series,
@@ -428,8 +429,9 @@
          branch = self.factory.makeProductBranch()
          product = branch.product
          series = self.factory.makeProductSeries(product=product)
--        series.branch = branch
--        self.assertBzrIdentity(branch, '%s/%s' % (product.name, series.name))
++        linked_branch = ICanHasLinkedBranch(series)
++        linked_branch.setBranch(branch)
++        self.assertBzrIdentity(branch, linked_branch.bzr_identity)
      def test_private_linked_to_product(self):
          # If a branch is private, then the bzr identity is the unique name,
@@ -439,8 +441,8 @@
          owner = removeSecurityProxy(branch).owner
          login_person(owner)
          self.addCleanup(logout)
--        product = branch.product
--        removeSecurityProxy(product).development_focus.branch = branch
++        product = removeSecurityProxy(branch.product)
++        ICanHasLinkedBranch(product).setBranch(branch)
          self.assertBzrIdentity(branch, branch.unique_name)
      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)
          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)
--        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)
++        self.assertBzrIdentity(branch, linked_branch.bzr_identity)
  class TestBranchDeletion(TestCaseWithFactory):
@@ -506,7 +529,7 @@
      def test_stackedBranchDisablesDeletion(self):
          # A branch that is stacked upon cannot be deleted.
--        branch = self.factory.makeAnyBranch(stacked_on=self.branch)
++        self.factory.makeAnyBranch(stacked_on=self.branch)
          self.assertFalse(self.branch.canBeDeleted())
      def test_subscriptionDoesntDisableDeletion(self):
@@ -679,8 +702,7 @@
               ' proposal.')),
              merge_proposal2:
              ('delete', _('This branch is the source branch of this merge'
--             ' proposal.'))
--             },
++             ' proposal.'))},
                           self.branch.deletionRequirements())
          self.assertEqual({
              merge_proposal1:
@@ -688,8 +710,7 @@
               ' proposal.')),
              merge_proposal2:
              ('delete', _('This branch is the target branch of this merge'
--             ' proposal.'))
--            },
++             ' proposal.'))},
              merge_proposal1.target_branch.deletionRequirements())
          self.assertEqual({
              merge_proposal1:
@@ -697,8 +718,7 @@
               ' proposal.')),
              merge_proposal2:
              ('alter', _('This branch is the dependent branch of this merge'
--             ' proposal.'))
--            },
++             ' proposal.'))},
              merge_proposal1.dependent_branch.deletionRequirements())
      def test_deleteMergeProposalSource(self):
@@ -722,7 +742,6 @@
      def test_deleteMergeProposalDependent(self):
          """break_links enables deleting merge proposal dependant branches."""
          merge_proposal1, merge_proposal2 = self.makeMergeProposals()
--        merge_proposal1_id = merge_proposal1.id
          merge_proposal1.dependent_branch.destroySelf(break_references=True)
          self.assertEqual(None, merge_proposal1.dependent_branch)
@@ -755,7 +774,6 @@
      def test_branchWithBugDeletion(self):
          """break_links allows deleting a branch with a bug."""
          bug1 = self.factory.makeBug()
--        bug2 = self.factory.makeBug()
          bug1.linkBranch(self.branch, self.branch.owner)
          bug_branch1 = bug1.linked_branches[0]
          bug_branch1_id = bug_branch1.id
@@ -951,7 +969,7 @@
          # some_branch.getStackedBranchesWithIncompleteMirrors does not include
          # stacked branches that haven't been mirrored at all.
          branch = self.factory.makeAnyBranch()
--        stacked_a = self.factory.makeAnyBranch(stacked_on=branch)
++        self.factory.makeAnyBranch(stacked_on=branch)
          self.assertEqual(
              set(), set(branch.getStackedBranchesWithIncompleteMirrors()))
@@ -1097,8 +1115,7 @@
          branch pair, then another landing target specifying the same pair
          raises.
          """
--        proposal = self.source.addLandingTarget(
--            self.user, self.target, self.dependent)
++        self.source.addLandingTarget(self.user, self.target, self.dependent)
          self.assertRaises(
              InvalidBranchMergeProposal, self.source.addLandingTarget,
@@ -1113,8 +1130,7 @@
              self.user, self.target, self.dependent)
          proposal.rejectBranch(self.user, 'some_revision')
          syncUpdate(proposal)
--        new_proposal = self.source.addLandingTarget(
--            self.user, self.target, self.dependent)
++        self.source.addLandingTarget(self.user, self.target, self.dependent)
      def test_attributeAssignment(self):
          """Smoke test to make sure the assignments are there."""
@@ -1614,7 +1630,6 @@
      def test_spec_unlink(self):
          # Branches can be unlinked from the spec as well.
--        user = getUtility(IPersonSet).getByEmail('test@canonical.com')
          branch = self.factory.makeAnyBranch()
          spec = self.factory.makeSpecification()
          branch.linkSpecification(spec, self.user)
 === 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
@@ -18,7 +18,7 @@
  from lp.code.interfaces.branchnamespace import (
      get_branch_namespace, InvalidNamespace)
  from lp.code.interfaces.linkedbranch import (
--    CannotHaveLinkedBranch, NoLinkedBranch)
++    CannotHaveLinkedBranch, ICanHasLinkedBranch, NoLinkedBranch)
  from lp.registry.interfaces.distroseries import NoSuchDistroSeries
  from canonical.launchpad.interfaces.launchpad import ILaunchpadCelebrities
  from lp.registry.interfaces.person import NoSuchPerson
@@ -230,7 +230,7 @@
          This is because Launchpad doesn't currently support ftp.
          """
--        branch = self.makeProductBranch()
++        self.makeProductBranch()
          branch_set = getUtility(IBranchLookup)
          branch2 = branch_set.getByUrl('ftp://bazaar.launchpad.dev/~aa/b/c')
          self.assertIs(None, branch2)
@@ -311,7 +311,7 @@
          # a non-existent series.
          self.assertRaises(
              NoSuchProduct, self.traverser.traverse, 'bb/dd')
--        product = self.factory.makeProduct(name='bb')
++        self.factory.makeProduct(name='bb')
          self.assertRaises(
              NoSuchProductSeries, self.traverser.traverse, 'bb/dd')
@@ -345,6 +345,13 @@
          ssp = package.getSuiteSourcePackage(PackagePublishingPocket.RELEASE)
          self.assertTraverses(package.path, ssp)
++    def test_distribution_source_package(self):
++        # `traverse` resolves 'distro/package' to the distribution source
++        # package.
++        dsp = self.factory.makeDistributionSourcePackage()
++        path = '%s/%s' % (dsp.distribution.name, dsp.sourcepackagename.name)
++        self.assertTraverses(path, dsp)
++
      def test_traverse_source_package_pocket(self):
          # `traverse` resolves 'distro/series-pocket/package' to the official
          # branch for 'pocket' on that package.
@@ -366,7 +373,7 @@
      def test_no_such_distro_series(self):
          # `traverse` raises `NoSuchDistroSeries` if the distro series doesn't
          # exist.
--        distro = self.factory.makeDistribution(name='distro')
++        self.factory.makeDistribution(name='distro')
          self.assertRaises(
              NoSuchDistroSeries, self.traverser.traverse,
              'distro/series/package')
@@ -380,6 +387,14 @@
          self.assertRaises(
              NoSuchSourcePackageName, self.traverser.traverse, path)
++    def test_no_such_distribution_sourcepackage(self):
++        # `traverse` raises `NoSuchSourcePackageName` if the package in
++        # distro/package doesn't exist.
++        distribution = self.factory.makeDistribution()
++        path = '%s/doesntexist' % distribution.name
++        self.assertRaises(
++            NoSuchSourcePackageName, self.traverser.traverse, path)
++
  class TestGetByLPPath(TestCaseWithFactory):
      """Ensure URLs are correctly expanded."""
@@ -397,10 +412,10 @@
          # components are found.
          self.assertRaises(
              NoSuchPerson, self.branch_lookup.getByLPPath, '~aa/bb/c')
--        owner = self.factory.makePerson(name='aa')
++        self.factory.makePerson(name='aa')
          self.assertRaises(
              NoSuchProduct, self.branch_lookup.getByLPPath, '~aa/bb/c')
--        product = self.factory.makeProduct(name='bb')
++        self.factory.makeProduct(name='bb')
          self.assertRaises(
              NoSuchBranch, self.branch_lookup.getByLPPath, '~aa/bb/c')
@@ -434,7 +449,7 @@
          # doesn't match an existing branch.
          self.assertRaises(
              NoSuchPerson, self.branch_lookup.getByLPPath, '~aa/+junk/c')
--        owner = self.factory.makePerson(name='aa')
++        self.factory.makePerson(name='aa')
          self.assertRaises(
              NoSuchBranch, self.branch_lookup.getByLPPath, '~aa/+junk/c')
@@ -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)
++
++
++class TestProductLinkedBranch(TestCaseWithFactory):
++
++    layer = DatabaseFunctionalLayer
++
++    def test_branch(self):
          # The linked branch of a product is the linked branch of its
          # development focus product series.
          branch = self.factory.makeProductBranch()
@@ -38,22 +60,130 @@
          removeSecurityProxy(product).development_focus.branch = branch
          self.assertEqual(branch, ICanHasLinkedBranch(product).branch)
--    def test_suitesourcepackage(self):
++    def test_setBranch(self):
++        # setBranch sets the linked branch of the development focus product
++        # series.
++        branch = self.factory.makeProductBranch()
++        product = removeSecurityProxy(branch.product)
++        ICanHasLinkedBranch(product).setBranch(branch)
++        self.assertEqual(branch, product.development_focus.branch)
++
++    def test_bzr_identity(self):
++        # The bzr_identity of a product linked branch is the product name.
++        product = self.factory.makeProduct()
++        self.assertEqual(
++            product.name, ICanHasLinkedBranch(product).bzr_identity)
++
++
++class TestSuiteSourcePackageLinkedBranch(TestCaseWithFactory):
++
++    layer = DatabaseFunctionalLayer
++
++    def test_branch(self):
          # The linked branch of a suite source package is the official branch
          # for the pocket of that source package.
          branch = self.factory.makeAnyBranch()
++        suite_sourcepackage = self.factory.makeSuiteSourcePackage()
++        ubuntu_branches = getUtility(ILaunchpadCelebrities).ubuntu_branches
++        registrant = ubuntu_branches.teamowner
++        run_with_login(
++            registrant,
++            suite_sourcepackage.sourcepackage.setBranch,
++            suite_sourcepackage.pocket, branch, registrant)
++        self.assertEqual(
++            branch, ICanHasLinkedBranch(suite_sourcepackage).branch)
++
++    def test_setBranch(self):
++        # setBranch sets the official branch for the appropriate pocket of the
++        # source package.
++        branch = self.factory.makeAnyBranch()
++        suite_sourcepackage = self.factory.makeSuiteSourcePackage()
++        ubuntu_branches = getUtility(ILaunchpadCelebrities).ubuntu_branches
++        registrant = ubuntu_branches.teamowner
++        run_with_login(
++            registrant,
++            ICanHasLinkedBranch(suite_sourcepackage).setBranch,
++            branch, registrant)
++        self.assertEqual(
++            branch,
++            suite_sourcepackage.sourcepackage.getBranch(
++                suite_sourcepackage.pocket))
++
++    def test_bzr_identity(self):
++        # The bzr_identity of a suite source package linked branch is the path
++        # of that suite source package.
++        suite_sourcepackage = self.factory.makeSuiteSourcePackage()
++        self.assertEqual(
++            suite_sourcepackage.path,
++            ICanHasLinkedBranch(suite_sourcepackage).bzr_identity)
++
++
++class TestDistributionSourcePackageLinkedBranch(TestCaseWithFactory):
++
++    layer = DatabaseFunctionalLayer
++
++    def test_branch(self):
++        # The linked branch of a distribution source package is the official
++        # branch for the release pocket of the development focus series for
++        # that package. Phew.
++        branch = self.factory.makeAnyBranch()
          sourcepackage = self.factory.makeSourcePackage()
++        dev_sourcepackage = sourcepackage.development_version
          pocket = PackagePublishingPocket.RELEASE
++
          ubuntu_branches = getUtility(ILaunchpadCelebrities).ubuntu_branches
          registrant = ubuntu_branches.teamowner
          run_with_login(
              ubuntu_branches.teamowner,
--            sourcepackage.setBranch, pocket, branch, registrant)
--        suite_sourcepackage = sourcepackage.getSuiteSourcePackage(pocket)
--        self.assertEqual(
--            branch, ICanHasLinkedBranch(suite_sourcepackage).branch)
--
--    def test_project(self):
++            dev_sourcepackage.setBranch, pocket, branch, registrant)
++
++        distribution_sourcepackage = sourcepackage.distribution_sourcepackage
++        self.assertEqual(
++            branch, ICanHasLinkedBranch(distribution_sourcepackage).branch)
++
++    def test_branch_when_no_series(self):
++        # Our data model allows distributions that have no series. The linked
++        # branch for a package in such a distribution is always None.
++        distro_package = self.factory.makeDistributionSourcePackage()
++        self.assertIs(None, ICanHasLinkedBranch(distro_package).branch)
++
++    def test_setBranch(self):
++        # Setting the linked branch for a distribution source package links
++        # the branch to the release pocket of the development focus series for
++        # that package.
++        branch = self.factory.makeAnyBranch()
++        sourcepackage = self.factory.makeSourcePackage()
++        distribution_sourcepackage = sourcepackage.distribution_sourcepackage
++
++        ubuntu_branches = getUtility(ILaunchpadCelebrities).ubuntu_branches
++        registrant = ubuntu_branches.teamowner
++        run_with_login(
++            registrant,
++            ICanHasLinkedBranch(distribution_sourcepackage).setBranch,
++            branch, registrant)
++
++        dev_sourcepackage = sourcepackage.development_version
++        pocket = PackagePublishingPocket.RELEASE
++        self.assertEqual(branch, dev_sourcepackage.getBranch(pocket))
++
++    def test_bzr_identity(self):
++        # The bzr_identity of a distribution source package linked branch is
++        # distro/package.
++        distribution_sourcepackage = (
++            self.factory.makeDistributionSourcePackage())
++        self.assertEqual(
++            '%s/%s' % (
++                distribution_sourcepackage.distribution.name,
++                distribution_sourcepackage.sourcepackagename.name),
++            ICanHasLinkedBranch(distribution_sourcepackage).bzr_identity)
++
++
++class TestProjectLinkedBranch(TestCaseWithFactory):
++
++    layer = DatabaseFunctionalLayer
++
++    def test_cannot_have_linked_branch(self):
++        # Projects cannot have linked branches.
          project = self.factory.makeProject()
          self.assertRaises(
              CannotHaveLinkedBranch, get_linked_branch, project)
 === modified file 'lib/lp/registry/browser/distributionsourcepackage.py'
 --- lib/lp/registry/browser/distributionsourcepackage.py	2009-07-10 12:34:49 +0000
 +++ lib/lp/registry/browser/distributionsourcepackage.py	2009-07-16 03:53:20 +0000
@@ -31,9 +31,8 @@
  from lp.answers.browser.questiontarget import (
          QuestionTargetFacetMixin, QuestionTargetTraversalMixin)
  from canonical.launchpad.webapp import (
--    ApplicationMenu, GetitemNavigation, LaunchpadEditFormView,
--    LaunchpadFormView, Link, StandardLaunchpadFacets, action, canonical_url,
--    redirection)
++    ApplicationMenu, LaunchpadEditFormView, LaunchpadFormView, Link,
++    Navigation, StandardLaunchpadFacets, action, canonical_url, redirection)
  from canonical.launchpad.webapp.menu import enabled_with_permission
  from canonical.launchpad.webapp.breadcrumb import BreadcrumbBuilder
@@ -88,13 +87,16 @@
          return Link('+filebug', text, icon='bug')
--class DistributionSourcePackageNavigation(GetitemNavigation,
++class DistributionSourcePackageNavigation(Navigation,
      BugTargetTraversalMixin, QuestionTargetTraversalMixin):
      usedfor = IDistributionSourcePackage
      redirection("+editbugcontact", "+subscribe")
++    def traverse(self, name):
++        return self.context.getVersion(name)
++
  class DecoratedDistributionSourcePackageRelease:
      """A decorated DistributionSourcePackageRelease.
 === modified file 'lib/lp/registry/configure.zcml'
 --- lib/lp/registry/configure.zcml	2009-07-13 14:13:07 +0000
 +++ lib/lp/registry/configure.zcml	2009-07-16 04:47:24 +0000
@@ -357,6 +357,7 @@
          <allow
              attributes="
                  distribution
++                development_version
                  sourcepackagename
                  name
                  displayname
 === 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.')
++
      def getReleasesAndPublishingHistory():
          """Return a list of all releases of this source package in this
          distribution and their correspodning publishing history.
@@ -85,9 +89,6 @@
          "Return a list of CURRENT publishing records for this source "
          "package in this distribution.")
--    def __getitem__(version):
--        """Should map to getVersion."""
--
      def getVersion(version):
          """Return the a DistributionSourcePackageRelease with the given
          version, or None if there has never been a release with that
 === modified file 'lib/lp/registry/interfaces/sourcepackage.py'
 --- lib/lp/registry/interfaces/sourcepackage.py	2009-06-12 16:36:02 +0000
 +++ lib/lp/registry/interfaces/sourcepackage.py	2009-07-15 07:25:04 +0000
@@ -125,6 +125,9 @@
      development_version = Attribute(
          "This package on the distro's current series.")
++    distribution_sourcepackage = Attribute(
++        "The IDistributionSourcePackage for this source package.")
++
      def __getitem__(version):
          """Return the source package release with the given version in this
          distro series, or None."""
 === 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 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
@@ -124,9 +132,6 @@
          _get_bug_reporting_guidelines,
          _set_bug_reporting_guidelines)
--    def __getitem__(self, version):
--        return self.getVersion(version)
--
      @property
      def latest_overall_publication(self):
          """See `IDistributionSourcePackage`."""
 === modified file 'lib/lp/registry/model/sourcepackage.py'
 --- lib/lp/registry/model/sourcepackage.py	2009-07-11 09:33:12 +0000
 +++ lib/lp/registry/model/sourcepackage.py	2009-07-15 07:25:04 +0000
@@ -424,6 +424,11 @@
              self.sourcepackagename, self.distribution.currentseries)
      @property
++    def distribution_sourcepackage(self):
++        """See `ISourcePackage`."""
++        return self.distribution.getSourcePackage(self.sourcepackagename)
++
++    @property
      def bug_reporting_guidelines(self):
          """See `IBugTarget`."""
          return self.distribution.bug_reporting_guidelines
 === 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.
++        self.assertIs(None, currentseries)
++        self.assertEqual(None, dsp.development_version)
++
++
  def test_suite():
      return unittest.TestLoader().loadTestsFromName(__name__)
 === modified file 'lib/lp/registry/tests/test_sourcepackage.py'
 --- lib/lp/registry/tests/test_sourcepackage.py	2009-06-02 13:52:48 +0000
 +++ lib/lp/registry/tests/test_sourcepackage.py	2009-07-15 07:25:04 +0000
@@ -154,6 +154,17 @@
          self.assertEqual(
              dev_sourcepackage, dev_sourcepackage.development_version)
++    def test_distribution_sourcepackage(self):
++        # ISourcePackage.distribution_sourcepackage is the distribution source
++        # package for the ISourcePackage.
++        sourcepackage = self.factory.makeSourcePackage()
++        distribution = sourcepackage.distribution
++        distribution_sourcepackage = distribution.getSourcePackage(
++            sourcepackage.sourcepackagename)
++        self.assertEqual(
++            distribution_sourcepackage,
++            sourcepackage.distribution_sourcepackage)
++
  class TestSourcePackageSecurity(TestCaseWithFactory):
      """Tests for source package branch linking security."""
 === modified file 'lib/lp/soyuz/doc/buildd-queuebuilder-lookup.txt'
 --- lib/lp/soyuz/doc/buildd-queuebuilder-lookup.txt	2009-04-29 20:27:44 +0000
 +++ lib/lp/soyuz/doc/buildd-queuebuilder-lookup.txt	2009-07-16 03:31:45 +0000
@@ -31,7 +31,7 @@
      >>> partner_archive = getUtility(IArchiveSet).getByDistroPurpose(
      ...     distribution=ubuntu, purpose=ArchivePurpose.PARTNER)
--    >>> dspr = ubuntu.getSourcePackage('mozilla-firefox')['0.9']
++    >>> dspr = ubuntu.getSourcePackage('mozilla-firefox').getVersion('0.9')
      >>> the_spr = dspr.sourcepackagerelease
      >>> initial_candidates = []
 === modified file 'lib/lp/soyuz/doc/distroseriesqueue.txt'
 --- lib/lp/soyuz/doc/distroseriesqueue.txt	2009-07-03 09:49:13 +0000
 +++ lib/lp/soyuz/doc/distroseriesqueue.txt	2009-07-16 03:31:45 +0000
@@ -712,7 +712,8 @@
  have in the sampledata.
    >>> [binary_queue] = queue_items
--  >>> non_matching_pmount = ubuntu.getSourcePackage('pmount')['0.1-2']
++  >>> pmount = ubuntu.getSourcePackage('pmount')
++  >>> non_matching_pmount = pmount.getVersion('0.1-2')
    >>> unused = binary_queue.addSource(
    ...    non_matching_pmount.sourcepackagerelease)
@@ -732,7 +733,7 @@
    ...     PackagePublishingPocket.RELEASE,
    ...     'pmount_0.1-1_source.changes', 'some content',
    ...     breezy_autotest.main_archive)
--  >>> matching_pmount = ubuntu.getSourcePackage('pmount')['0.1-1']
++  >>> matching_pmount = pmount.getVersion('0.1-1')
    >>> unused = candidate_queue.addSource(
    ...     matching_pmount.sourcepackagerelease)
@@ -1087,7 +1088,7 @@
  are. Contents can be attached to it.
     # Retrieve a SourcePackageRelease from the sampledata.
--   >>> a_source_package = ubuntu.getSourcePackage('pmount')['0.1-2']
++   >>> a_source_package = pmount.getVersion('0.1-2')
     >>> a_source_release = a_source_package.sourcepackagerelease
     >>> unused = delayed_copy.addSource(a_source_release)
 === modified file 'lib/lp/soyuz/doc/package-diff.txt'
 --- lib/lp/soyuz/doc/package-diff.txt	2009-05-06 20:53:05 +0000
 +++ lib/lp/soyuz/doc/package-diff.txt	2009-07-16 03:31:45 +0000
@@ -18,8 +18,8 @@
      >>> ubuntu = getUtility(IDistributionSet).getByName('ubuntu')
      >>> pmount = ubuntu.getSourcePackage('pmount')
--    >>> pmount_from = pmount['0.1-1'].sourcepackagerelease
--    >>> pmount_to = pmount['0.1-2'].sourcepackagerelease
++    >>> pmount_from = pmount.getVersion('0.1-1').sourcepackagerelease
++    >>> pmount_to = pmount.getVersion('0.1-2').sourcepackagerelease
  A packageDiff can be created from the two packages by calling
  requestDiffTo(). It takes two arguments: the user requesting the