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

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.

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

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