Code review comment for lp://staging/~jml/launchpad/ubuntu-package-bug-345737

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