launchpad-reviewers team mailing list archive

[William Grant <me@xxxxxxxxxxxxxxxxxx>]

Review: Needs Fixing code

I'm not sure about the target.getDefaultGitRepository/setDefaultGitRepository design. They're yet more domain-specific methods on already huge Registry classes, which just isn't maintainable long-term.

During Disclosure we experimented with SharingService to stop the bloat, which ended up hideous more because of the different artifact types than because of a fundamental issue with the service concept.

Diff comments:

> === modified file 'lib/lp/code/configure.zcml'
> --- lib/lp/code/configure.zcml	2015-02-19 23:57:34 +0000
> +++ lib/lp/code/configure.zcml	2015-02-20 16:35:44 +0000
> @@ -1,4 +1,4 @@
> -<!-- Copyright 2009-2013 Canonical Ltd.  This software is licensed under the
> +<!-- Copyright 2009-2015 Canonical Ltd.  This software is licensed under the
>       GNU Affero General Public License version 3 (see the file LICENSE).
>  -->
>  
> @@ -858,6 +858,13 @@
>      <allow interface="lp.code.interfaces.gitnamespace.IGitNamespaceSet" />
>    </securedutility>
>  
> +  <!-- Default Git repositories -->
> +
> +  <adapter factory="lp.code.model.defaultgit.ProjectDefaultGitRepository" />
> +  <adapter factory="lp.code.model.defaultgit.PackageDefaultGitRepository" />
> +  <adapter factory="lp.code.model.defaultgit.OwnerProjectDefaultGitRepository" />
> +  <adapter factory="lp.code.model.defaultgit.OwnerPackageDefaultGitRepository" />
> +
>    <lp:help-folder folder="help" name="+help-code" />
>  
>    <!-- Diffs -->
> 
> === modified file 'lib/lp/code/errors.py'
> --- lib/lp/code/errors.py	2015-02-11 17:10:09 +0000
> +++ lib/lp/code/errors.py	2015-02-20 16:35:44 +0000
> @@ -1,4 +1,4 @@
> -# Copyright 2009-2013 Canonical Ltd.  This software is licensed under the
> +# Copyright 2009-2015 Canonical Ltd.  This software is licensed under the
>  # GNU Affero General Public License version 3 (see the file LICENSE).
>  
>  """Errors used in the lp/code modules."""
> @@ -22,6 +22,7 @@
>      'CannotDeleteBranch',
>      'CannotUpgradeBranch',
>      'CannotUpgradeNonHosted',
> +    'CannotHaveDefaultGitRepository',

Please sort.

>      'CannotHaveLinkedBranch',
>      'CodeImportAlreadyRequested',
>      'CodeImportAlreadyRunning',
> @@ -37,8 +38,10 @@
>      'GitRepositoryExists',
>      'GitTargetError',
>      'InvalidBranchMergeProposal',
> +    'InvalidGitRepositoryException',
>      'InvalidMergeQueueConfig',
>      'InvalidNamespace',
> +    'NoDefaultGitRepository',
>      'NoLinkedBranch',
>      'NoSuchBranch',
>      'NoSuchGitRepository',
> @@ -375,6 +378,41 @@
>      """Raised when there is an error determining a Git repository target."""
>  
>  
> +class InvalidGitRepositoryException(Exception):
> +    """Base exception for an error resolving a Git repository for a component.
> +
> +    Subclasses should set _msg_template to match their required display
> +    message.
> +    """
> +
> +    _msg_template = "Invalid Git repository for: %s"
> +
> +    def __init__(self, component):
> +        self.component = component
> +        # It's expected that components have a name attribute,
> +        # so let's assume they will and deal with any error if it occurs.
> +        try:
> +            component_name = component.name
> +        except AttributeError:
> +            component_name = str(component)
> +        # The display_message contains something readable for the user.
> +        self.display_message = self._msg_template % component_name
> +        Exception.__init__(self, self._msg_template % (repr(component),))
> +
> +
> +class CannotHaveDefaultGitRepository(InvalidGitRepositoryException):
> +    """Raised when we try to get the default Git repository for a thing that
> +    can't."""
> +
> +    _msg_template = "%s cannot have default Git repositories."""
> +
> +
> +class NoDefaultGitRepository(InvalidGitRepositoryException):
> +    """Raised when there's no default Git repository for a thing."""
> +
> +    _msg_template = "%s has no default Git repository."
> +
> +
>  class NoSuchGitRepository(NameLookupFailed):
>      """Raised when we try to load a Git repository that does not exist."""
>  
> 
> === added file 'lib/lp/code/interfaces/defaultgit.py'
> --- lib/lp/code/interfaces/defaultgit.py	1970-01-01 00:00:00 +0000
> +++ lib/lp/code/interfaces/defaultgit.py	2015-02-20 16:35:44 +0000
> @@ -0,0 +1,60 @@
> +# Copyright 2015 Canonical Ltd.  This software is licensed under the
> +# GNU Affero General Public License version 3 (see the file LICENSE).
> +
> +"""Interface for objects that have a default Git repository.
> +
> +A default Git repository is a repository that's somehow officially related
> +to an object.  It might be a project, a distribution source package, or a
> +combination of one of those with an owner to represent that owner's default
> +repository for that target.
> +"""
> +
> +__metaclass__ = type
> +__all__ = [
> +    'get_default_git_repository',
> +    'ICanHasDefaultGitRepository',
> +    ]
> +
> +from zope.interface import (
> +    Attribute,
> +    Interface,
> +    )
> +
> +from lp.code.errors import (
> +    CannotHaveDefaultGitRepository,
> +    NoDefaultGitRepository,
> +    )
> +
> +
> +class ICanHasDefaultGitRepository(Interface):
> +    """Something that has a default Git repository."""
> +
> +    context = Attribute("The object that can have a default Git repository.")
> +    repository = Attribute("The default Git repository.")
> +    path = Attribute(
> +        "The path for the default Git repository. "
> +        "Note that this will be set even if there is no default repository.")
> +
> +    def setRepository(repository):
> +        """Set the default repository.
> +
> +        :param repository: An `IGitRepository`.  After calling this,
> +            `ICanHasDefaultGitRepository.repository` will be `repository`.
> +        """
> +
> +
> +def get_default_git_repository(provided):
> +    """Get the `ICanHasDefaultGitRepository` for 'provided', whatever that is.
> +
> +    :raise CannotHaveDefaultGitRepository: If 'provided' can never have a
> +        default Git repository.
> +    :raise NoDefaultGitRepository: If 'provided' could have a default Git
> +        repository, but doesn't.
> +    :return: The `ICanHasDefaultGitRepository` object.
> +    """
> +    has_default_repository = ICanHasDefaultGitRepository(provided, None)
> +    if has_default_repository is None:
> +        raise CannotHaveDefaultGitRepository(provided)
> +    if has_default_repository.repository is None:
> +        raise NoDefaultGitRepository(provided)
> +    return has_default_repository
> 
> === modified file 'lib/lp/code/interfaces/gitnamespace.py'
> --- lib/lp/code/interfaces/gitnamespace.py	2015-02-13 18:34:45 +0000
> +++ lib/lp/code/interfaces/gitnamespace.py	2015-02-20 16:35:44 +0000
> @@ -76,6 +76,14 @@
>          :return: `IGitRepository` if found, otherwise 'default'.
>          """
>  
> +    def getDefault():
> +        """Find the default repository for this namespace.
> +
> +        This returns owner-target defaults, e.g. `lp:~owner/project`.  For
> +        target defaults such as `lp:project`, use
> +        `IGitRepositorySet.getDefaultForTarget` instead.
> +        """
> +
>      def __eq__(other):
>          """Is this namespace the same as another namespace?"""
>  
> 
> === modified file 'lib/lp/code/interfaces/gitrepository.py'
> --- lib/lp/code/interfaces/gitrepository.py	2015-02-19 23:57:34 +0000
> +++ lib/lp/code/interfaces/gitrepository.py	2015-02-20 16:35:44 +0000
> @@ -16,6 +16,7 @@
>  import re
>  
>  from lazr.restful.fields import Reference
> +from zope.component import getUtility
>  from zope.interface import (
>      Attribute,
>      Interface,
> @@ -32,7 +33,16 @@
>  from lp import _
>  from lp.app.enums import InformationType
>  from lp.app.validators import LaunchpadValidationError
> +from lp.code.interfaces.defaultgit import ICanHasDefaultGitRepository
>  from lp.code.interfaces.hasgitrepositories import IHasGitRepositories
> +from lp.registry.interfaces.distributionsourcepackage import (
> +    IDistributionSourcePackage,
> +    )
> +from lp.registry.interfaces.persondistributionsourcepackage import (
> +    IPersonDistributionSourcePackageFactory,
> +    )
> +from lp.registry.interfaces.personproduct import IPersonProductFactory
> +from lp.registry.interfaces.product import IProduct
>  from lp.registry.interfaces.role import IPersonRoles
>  from lp.services.fields import (
>      PersonChoice,
> @@ -349,9 +359,23 @@
>  
>      def getRepositoryDefaults(self):
>          """See `IGitRepository`."""
> -        # XXX cjwatson 2015-02-06: This will return shortcut defaults once
> -        # they're implemented.
> -        return []
> +        defaults = []
> +        if self.target_default:
> +            defaults.append(ICanHasDefaultGitRepository(self.target))
> +        if self.owner_default:
> +            if IProduct.providedBy(self.target):
> +                factory = getUtility(IPersonProductFactory)
> +                default = factory.create(self.owner, self.target)
> +            elif IDistributionSourcePackage.providedBy(self.target):
> +                factory = getUtility(IPersonDistributionSourcePackageFactory)
> +                default = factory.create(self.owner, self.target)
> +            else:
> +                # Also enforced by database constraint.
> +                raise AssertionError(
> +                    "Only projects or packages can have owner-target default "
> +                    "repositories.")
> +            defaults.append(ICanHasDefaultGitRepository(default))
> +        return sorted(defaults)
>  
>      def getRepositoryIdentities(self):
>          """See `IGitRepository`."""
> 
> === added file 'lib/lp/code/model/defaultgit.py'
> --- lib/lp/code/model/defaultgit.py	1970-01-01 00:00:00 +0000
> +++ lib/lp/code/model/defaultgit.py	2015-02-20 16:35:44 +0000
> @@ -0,0 +1,249 @@
> +# Copyright 2009-2015 Canonical Ltd.  This software is licensed under the
> +# GNU Affero General Public License version 3 (see the file LICENSE).
> +
> +"""Implementation of `ICanHasDefaultGitRepository`."""
> +
> +__metaclass__ = type
> +# Don't export anything -- anything you need from this module you can get by
> +# adapting another object.
> +__all__ = []
> +
> +from lazr.enum import (
> +    EnumeratedType,
> +    Item,
> +    )
> +from zope.component import adapts
> +from zope.interface import implements
> +
> +from lp.code.interfaces.defaultgit import ICanHasDefaultGitRepository
> +from lp.registry.interfaces.distributionsourcepackage import (
> +    IDistributionSourcePackage,
> +    )
> +from lp.registry.interfaces.persondistributionsourcepackage import (
> +    IPersonDistributionSourcePackage,
> +    )
> +from lp.registry.interfaces.personproduct import IPersonProduct
> +from lp.registry.interfaces.product import IProduct
> +
> +
> +class DefaultGitRepositoryOrder(EnumeratedType):
> +    """An enum used only for ordering."""
> +
> +    PROJECT = Item("Project shortcut")
> +    DISTRIBUTION_SOURCE_PACKAGE = Item("Distribution source package shortcut")
> +    OWNER_PROJECT = Item("Owner's default for a project")
> +    OWNER_DISTRIBUTION_SOURCE_PACKAGE = Item(
> +        "Owner's default for a distribution source package")

Do we need this? Unlike a Branch, a GitRepository cannot be a default for something other than its own target or owner-target, so the only ordering required is that target come before owner-target.

> +
> +
> +class BaseDefaultGitRepository:
> +    """Provides the common sorting algorithm."""
> +
> +    def __cmp__(self, other):
> +        if not ICanHasDefaultGitRepository.providedBy(other):
> +            raise AssertionError("Can't compare with: %r" % other)
> +        return cmp(self.sort_order, other.sort_order)
> +
> +    def __eq__(self, other):
> +        return (
> +            isinstance(other, self.__class__) and
> +            self.context == other.context)
> +
> +    def __ne__(self, other):
> +        return not self == other
> +
> +
> +class ProjectDefaultGitRepository(BaseDefaultGitRepository):
> +    """Implement a default Git repository for a project."""
> +
> +    adapts(IProduct)
> +    implements(ICanHasDefaultGitRepository)
> +
> +    sort_order = DefaultGitRepositoryOrder.PROJECT
> +
> +    def __init__(self, project):
> +        self.context = project
> +
> +    @property
> +    def project(self):
> +        return self.context
> +
> +    def __cmp__(self, other):
> +        result = super(ProjectDefaultGitRepository, self).__cmp__(other)
> +        if result != 0:
> +            return result
> +        else:
> +            return cmp(self.project.name, other.project.name)

This seems overcomplicated. I'd just do this:

@property
def sort_order(self):
    return (0, project.name)

(0 for target defaults, 1 for owner-target.)

> +
> +    @property
> +    def repository(self):
> +        """See `ICanHasDefaultGitRepository`."""
> +        return self.context.getDefaultGitRepository()
> +
> +    def setRepository(self, repository):
> +        """See `ICanHasDefaultGitRepository`."""
> +        self.context.setDefaultGitRepository(repository)

Did you consider not having those internal Git-specific methods on the Registry objects? All these classes could use the same method on another object that just took a target and an optional owner.

This would probably turn all these variants into tiny classes that define a sort_key and a path.

> +
> +    @property
> +    def path(self):
> +        """See `ICanHasDefaultGitRepository`."""
> +        return self.project.name
> +
> +
> +class PackageDefaultGitRepository(BaseDefaultGitRepository):
> +    """Implement a default Git repository for a distribution source package."""
> +
> +    adapts(IDistributionSourcePackage)
> +    implements(ICanHasDefaultGitRepository)
> +
> +    sort_order = DefaultGitRepositoryOrder.DISTRIBUTION_SOURCE_PACKAGE
> +
> +    def __init__(self, distro_source_package):
> +        self.context = distro_source_package
> +
> +    @property
> +    def distro_source_package(self):
> +        return self.context
> +
> +    @property
> +    def distribution(self):
> +        return self.context.distribution
> +
> +    @property
> +    def sourcepackagename(self):
> +        return self.context.sourcepackagename

I'm not sure these internal, two-use properties are worth the dozen lines.

> +
> +    def __cmp__(self, other):
> +        result = super(PackageDefaultGitRepository, self).__cmp__(other)
> +        if result != 0:
> +            return result
> +        else:
> +            my_names = (self.distribution.name, self.sourcepackagename.name)
> +            other_names = (
> +                other.distribution.name, other.sourcepackagename.name)
> +            return cmp(my_names, other_names)
> +
> +    @property
> +    def repository(self):
> +        """See `ICanHasDefaultGitRepository`."""
> +        return self.context.getDefaultGitRepository()
> +
> +    def setRepository(self, repository):
> +        """See `ICanHasDefaultGitRepository`."""
> +        self.context.setDefaultGitRepository(repository)
> +
> +    @property
> +    def path(self):
> +        """See `ICanHasDefaultGitRepository`."""
> +        return "%s/+source/%s" % (
> +            self.distribution.name, self.sourcepackagename.name)
> +
> +
> +class OwnerProjectDefaultGitRepository(BaseDefaultGitRepository):
> +    """Implement an owner's default Git repository for a project."""
> +
> +    adapts(IPersonProduct)
> +    implements(ICanHasDefaultGitRepository)
> +
> +    sort_order = DefaultGitRepositoryOrder.OWNER_PROJECT
> +
> +    def __init__(self, person_project):
> +        self.context = person_project
> +
> +    @property
> +    def person_project(self):
> +        return self.context
> +
> +    @property
> +    def person(self):
> +        return self.context.person
> +
> +    @property
> +    def project(self):
> +        return self.context.product
> +
> +    def __cmp__(self, other):
> +        result = super(OwnerProjectDefaultGitRepository, self).__cmp__(other)
> +        if result != 0:
> +            return result
> +        else:
> +            my_names = (self.person.name, self.project.name)
> +            other_names = (other.person.name, other.project.name)
> +            return cmp(my_names, other_names)
> +
> +    @property
> +    def repository(self):
> +        """See `ICanHasDefaultGitRepository`."""
> +        return self.context.getDefaultGitRepository(self.project)
> +
> +    def setRepository(self, repository):
> +        """See `ICanHasDefaultGitRepository`."""
> +        self.context.setDefaultGitRepository(self.project, repository)
> +
> +    @property
> +    def path(self):
> +        """See `ICanHasDefaultGitRepository`."""
> +        return "~%s/%s" % (self.person.name, self.project.name)
> +
> +
> +class OwnerPackageDefaultGitRepository(BaseDefaultGitRepository):
> +    """Implement an owner's default Git repository for a distribution source
> +    package."""
> +
> +    adapts(IPersonDistributionSourcePackage)
> +    implements(ICanHasDefaultGitRepository)
> +
> +    sort_order = DefaultGitRepositoryOrder.OWNER_DISTRIBUTION_SOURCE_PACKAGE
> +
> +    def __init__(self, person_distro_source_package):
> +        self.context = person_distro_source_package
> +
> +    @property
> +    def person_distro_source_package(self):
> +        return self.context
> +
> +    @property
> +    def person(self):
> +        return self.context.person
> +
> +    @property
> +    def distro_source_package(self):
> +        return self.context.distro_source_package
> +
> +    @property
> +    def distribution(self):
> +        return self.distro_source_package.distribution
> +
> +    @property
> +    def sourcepackagename(self):
> +        return self.distro_source_package.sourcepackagename
> +
> +    def __cmp__(self, other):
> +        result = super(OwnerPackageDefaultGitRepository, self).__cmp__(other)
> +        if result != 0:
> +            return result
> +        else:
> +            my_names = (
> +                self.person.name, self.distribution.name,
> +                self.sourcepackagename.name)
> +            other_names = (
> +                other.person.name, other.distribution.name,
> +                other.sourcepackagename.name)
> +            return cmp(my_names, other_names)
> +
> +    @property
> +    def repository(self):
> +        """See `ICanHasDefaultGitRepository`."""
> +        return self.context.getDefaultGitRepository(self.distro_source_package)
> +
> +    def setRepository(self, repository):
> +        """See `ICanHasDefaultGitRepository`."""
> +        self.context.setDefaultGitRepository(
> +            self.distro_source_package, repository)
> +
> +    @property
> +    def path(self):
> +        """See `ICanHasDefaultGitRepository`."""
> +        return "~%s/%s/+source/%s" % (
> +            self.person.name, self.distribution.name,
> +            self.sourcepackagename.name)
> 
> === modified file 'lib/lp/code/model/gitnamespace.py'
> --- lib/lp/code/model/gitnamespace.py	2015-02-13 18:34:45 +0000
> +++ lib/lp/code/model/gitnamespace.py	2015-02-20 16:35:44 +0000
> @@ -185,6 +185,12 @@
>              match = default
>          return match
>  
> +    def getDefault(self):
> +        """See `IGitNamespace`."""
> +        return IStore(GitRepository).find(
> +            GitRepository, self._getRepositoriesClause(),
> +            GitRepository.owner_default == True).one()
> +
>      def getAllowedInformationTypes(self, who=None):
>          """See `IGitNamespace`."""
>          raise NotImplementedError
> 
> === modified file 'lib/lp/code/model/tests/test_gitrepository.py'
> --- lib/lp/code/model/tests/test_gitrepository.py	2015-02-20 00:56:57 +0000
> +++ lib/lp/code/model/tests/test_gitrepository.py	2015-02-20 16:35:44 +0000
> @@ -24,12 +24,17 @@
>      GitRepositoryCreatorNotOwner,
>      GitTargetError,
>      )
> +from lp.code.interfaces.defaultgit import ICanHasDefaultGitRepository
>  from lp.code.interfaces.gitnamespace import (
>      IGitNamespacePolicy,
>      IGitNamespaceSet,
>      )
>  from lp.code.interfaces.gitrepository import IGitRepository
>  from lp.registry.enums import BranchSharingPolicy
> +from lp.registry.interfaces.persondistributionsourcepackage import (
> +    IPersonDistributionSourcePackageFactory,
> +    )
> +from lp.registry.interfaces.personproduct import IPersonProductFactory
>  from lp.services.database.constants import UTC_NOW
>  from lp.services.webapp.authorization import check_permission
>  from lp.testing import (
> @@ -111,6 +116,51 @@
>          repository = self.factory.makeGitRepository()
>          self.assertGitIdentity(repository, repository.unique_name)
>  
> +    def test_git_identity_default_for_project(self):
> +        # If a repository is the default for a project, then its Git
> +        # identity is the project name.
> +        project = self.factory.makeProduct()
> +        repository = self.factory.makeGitRepository(
> +            owner=project.owner, target=project)
> +        with person_logged_in(project.owner):
> +            project.setDefaultGitRepository(repository)
> +        self.assertGitIdentity(repository, project.name)
> +
> +    def test_git_identity_default_for_package(self):
> +        # If a repository is the default for a package, then its Git
> +        # identity uses the path to that package.
> +        dsp = self.factory.makeDistributionSourcePackage()
> +        repository = self.factory.makeGitRepository(target=dsp)
> +        with admin_logged_in():
> +            dsp.setDefaultGitRepository(repository)
> +        self.assertGitIdentity(
> +            repository,
> +            "%s/+source/%s" % (
> +                dsp.distribution.name, dsp.sourcepackagename.name))
> +
> +    def test_git_identity_owner_default_for_project(self):
> +        # If a repository is a person's default for a project, then its Git
> +        # identity is a combination of the person and project names.
> +        project = self.factory.makeProduct()
> +        repository = self.factory.makeGitRepository(target=project)
> +        with person_logged_in(repository.owner):
> +            repository.owner.setDefaultGitRepository(project, repository)
> +        self.assertGitIdentity(
> +            repository, "~%s/%s" % (repository.owner.name, project.name))
> +
> +    def test_git_identity_owner_default_for_package(self):
> +        # If a repository is a person's default for a package, then its Git
> +        # identity is a combination of the person name and the package path.
> +        dsp = self.factory.makeDistributionSourcePackage()
> +        repository = self.factory.makeGitRepository(target=dsp)
> +        with person_logged_in(repository.owner):
> +            repository.owner.setDefaultGitRepository(dsp, repository)
> +        self.assertGitIdentity(
> +            repository,
> +            "~%s/%s/+source/%s" % (
> +                repository.owner.name, dsp.distribution.name,
> +                dsp.sourcepackagename.name))
> +
>      def test_identities_no_defaults(self):
>          # If there are no defaults, the only repository identity is the
>          # unique name.
> @@ -119,8 +169,52 @@
>              [(repository.unique_name, repository)],
>              repository.getRepositoryIdentities())
>  
> -    # XXX cjwatson 2015-02-12: This will need to be expanded once support
> -    # for default repositories is in place.
> +    def test_default_for_project(self):
> +        # If a repository is the default for a project, then that is the
> +        # preferred identity.  Target defaults are preferred over
> +        # owner-target defaults.
> +        eric = self.factory.makePerson(name="eric")
> +        fooix = self.factory.makeProduct(name="fooix", owner=eric)
> +        repository = self.factory.makeGitRepository(
> +            owner=eric, target=fooix, name=u"fooix-repo")
> +        with person_logged_in(fooix.owner):
> +            repository.owner.setDefaultGitRepository(fooix, repository)
> +            fooix.setDefaultGitRepository(repository)
> +        eric_fooix = getUtility(IPersonProductFactory).create(eric, fooix)
> +        self.assertEqual(
> +            [ICanHasDefaultGitRepository(target)
> +             for target in (fooix, eric_fooix)],
> +            repository.getRepositoryDefaults())
> +        self.assertEqual(
> +            [("fooix", fooix), ("~eric/fooix", eric_fooix),
> +             ("~eric/fooix/+git/fooix-repo", repository)],
> +            repository.getRepositoryIdentities())
> +
> +    def test_default_for_package(self):
> +        # If a repository is the default for a package, then that is the
> +        # preferred identity.  Target defaults are preferred over
> +        # owner-target defaults.
> +        mint = self.factory.makeDistribution(name="mint")
> +        eric = self.factory.makePerson(name="eric")
> +        mint_choc = self.factory.makeDistributionSourcePackage(
> +            distribution=mint, sourcepackagename="choc")
> +        repository = self.factory.makeGitRepository(
> +            owner=eric, target=mint_choc, name=u"choc-repo")
> +        dsp = repository.target
> +        with admin_logged_in():
> +            repository.owner.setDefaultGitRepository(dsp, repository)
> +            dsp.setDefaultGitRepository(repository)
> +        eric_dsp = getUtility(IPersonDistributionSourcePackageFactory).create(
> +            eric, dsp)
> +        self.assertEqual(
> +            [ICanHasDefaultGitRepository(target)
> +             for target in (dsp, eric_dsp)],
> +            repository.getRepositoryDefaults())
> +        self.assertEqual(
> +            [("mint/+source/choc", dsp),
> +             ("~eric/mint/+source/choc", eric_dsp),
> +             ("~eric/mint/+source/choc/+git/choc-repo", repository)],
> +            repository.getRepositoryIdentities())
>  
>  
>  class TestGitRepositoryDateLastModified(TestCaseWithFactory):
> 
> === modified file 'lib/lp/registry/configure.zcml'
> --- lib/lp/registry/configure.zcml	2015-02-13 15:54:36 +0000
> +++ lib/lp/registry/configure.zcml	2015-02-20 16:35:44 +0000
> @@ -516,6 +516,7 @@
>                  findRelatedArchivePublications
>                  findRelatedArchives
>                  getBranches
> +                getDefaultGitRepository
>                  getMergeProposals
>                  getReleasesAndPublishingHistory
>                  getUsedBugTagsWithOpenCounts
> @@ -528,11 +529,17 @@
>                  personHasDriverRights
>                  publishing_history
>                  releases
> +                setDefaultGitRepository

This lets anyone call setDefaultGitRepository, so we're clearly not using the security proxies to protect it. This goes back to my previous comment about perhaps moving these methods out of Registry. We only need to keep domain-specific methods on the Registry objects for API purposes.

>                  sourcepackagename
>                  subscribers
>                  summary
>                  title
>                  upstream_product"/>
> +        <require
> +            permission="launchpad.Edit"
> +            set_attributes="
> +                setDefaultGitRepository
> +                " />

This shouldn't exist, since no code should ever write to a method slot.

>  
>          <!-- IStructuralSubscriptionTarget -->
>  
> 
> === modified file 'lib/lp/registry/interfaces/distributionsourcepackage.py'
> --- lib/lp/registry/interfaces/distributionsourcepackage.py	2015-02-06 15:29:45 +0000
> +++ lib/lp/registry/interfaces/distributionsourcepackage.py	2015-02-20 16:35:44 +0000
> @@ -1,4 +1,4 @@
> -# Copyright 2009-2012 Canonical Ltd.  This software is licensed under the
> +# Copyright 2009-2015 Canonical Ltd.  This software is licensed under the
>  # GNU Affero General Public License version 3 (see the file LICENSE).
>  
>  """Source package in Distribution interfaces."""
> @@ -175,6 +175,18 @@
>          on how this criteria will be centrally encoded.
>          """)
>  
> +    def getDefaultGitRepository():
> +        """Get the default Git repository for this package.
> +
> +        :return: An `IGitRepository`, or None.
> +        """
> +
> +    def setDefaultGitRepository(git_repository):
> +        """Set the default Git repository for this package.
> +
> +        :param git_repository: An `IGitRepository`, or None to unset.
> +        """
> +
>      def __eq__(other):
>          """IDistributionSourcePackage comparison method.
>  
> 
> === modified file 'lib/lp/registry/interfaces/person.py'
> --- lib/lp/registry/interfaces/person.py	2015-02-09 16:49:01 +0000
> +++ lib/lp/registry/interfaces/person.py	2015-02-20 16:35:44 +0000
> @@ -1,4 +1,4 @@
> -# Copyright 2009-2014 Canonical Ltd.  This software is licensed under the
> +# Copyright 2009-2015 Canonical Ltd.  This software is licensed under the
>  # GNU Affero General Public License version 3 (see the file LICENSE).
>  
>  """Person interfaces."""
> @@ -1581,6 +1581,13 @@
>          If no orderby is provided, Person.sortingColumns is used.
>          """
>  
> +    def getDefaultGitRepository(target):
> +        """Get this person's default Git repository for this target.
> +
> +        :param target: An `IHasGitRepositories`.
> +        :return: An `IGitRepository`, or None.
> +        """
> +
>  
>  class IPersonEditRestricted(Interface):
>      """IPerson attributes that require launchpad.Edit permission."""
> @@ -1779,6 +1786,13 @@
>          :return: a PPA `IArchive` record.
>          """
>  
> +    def setDefaultGitRepository(target, git_repository):
> +        """Set this person's default Git repository for this target.
> +
> +        :param target: An `IHasGitRepositories`.
> +        :param git_repository: An `IGitRepository`, or None to unset.
> +        """
> +
>  
>  class IPersonSpecialRestricted(Interface):
>      """IPerson methods that require launchpad.Special permission to use."""
> 
> === modified file 'lib/lp/registry/interfaces/product.py'
> --- lib/lp/registry/interfaces/product.py	2015-02-09 16:49:01 +0000
> +++ lib/lp/registry/interfaces/product.py	2015-02-20 16:35:44 +0000
> @@ -1,4 +1,4 @@
> -# Copyright 2009-2013 Canonical Ltd.  This software is licensed under the
> +# Copyright 2009-2015 Canonical Ltd.  This software is licensed under the
>  # GNU Affero General Public License version 3 (see the file LICENSE).
>  
>  """Interfaces including and related to IProduct."""
> @@ -837,6 +837,12 @@
>          The number of milestones returned per series is limited.
>          """
>  
> +    def getDefaultGitRepository():
> +        """Get the default Git repository for this project.
> +
> +        :return: An `IGitRepository`, or None.
> +        """
> +
>  
>  class IProductEditRestricted(IOfficialBugTagTargetRestricted):
>      """`IProduct` properties which require launchpad.Edit permission."""
> @@ -884,6 +890,12 @@
>          permitted.
>          """
>  
> +    def setDefaultGitRepository(git_repository):
> +        """Set the default Git repository for this project.
> +
> +        :param git_repository: An `IGitRepository`, or None to unset.
> +        """
> +
>  
>  class IProduct(
>      IBugTarget, IHasBugSupervisor, IHasDrivers, IProductEditRestricted,
> 
> === modified file 'lib/lp/registry/model/distributionsourcepackage.py'
> --- lib/lp/registry/model/distributionsourcepackage.py	2015-02-13 17:16:22 +0000
> +++ lib/lp/registry/model/distributionsourcepackage.py	2015-02-20 16:35:44 +0000
> @@ -1,4 +1,4 @@
> -# Copyright 2009-2012 Canonical Ltd.  This software is licensed under the
> +# Copyright 2009-2015 Canonical Ltd.  This software is licensed under the
>  # GNU Affero General Public License version 3 (see the file LICENSE).
>  
>  """Classes to represent source packages in a distribution."""
> @@ -32,6 +32,7 @@
>      Unicode,
>      )
>  import transaction
> +from zope.component import getUtility
>  from zope.interface import implements
>  
>  from lp.bugs.interfaces.bugsummary import IBugSummaryDimension
> @@ -39,6 +40,8 @@
>  from lp.bugs.model.structuralsubscription import (
>      StructuralSubscriptionTargetMixin,
>      )
> +from lp.code.errors import GitTargetError
> +from lp.code.interfaces.gitrepository import IGitRepositorySet
>  from lp.code.model.hasbranches import (
>      HasBranchesMixin,
>      HasMergeProposalsMixin,
> @@ -448,6 +451,23 @@
>          return [dspr for (dspr, pubs) in
>                  self.getReleasesAndPublishingHistory()]
>  
> +    def getDefaultGitRepository(self):
> +        """See `IDistributionSourcePackage`."""
> +        return getUtility(IGitRepositorySet).getDefaultRepository(self)
> +
> +    def setDefaultGitRepository(self, git_repository):
> +        """See `IDistributionSourcePackage`."""
> +        if git_repository is not None:
> +            if git_repository.target != self:
> +                raise GitTargetError(
> +                    "Cannot set default Git repository to one attached to "
> +                    "another target.")
> +            git_repository.setTargetDefault(True)
> +        else:
> +            previous = self.getDefaultGitRepository()
> +            if previous is not None:
> +                previous.setTargetDefault(False)
> +
>      def __eq__(self, other):
>          """See `IDistributionSourcePackage`."""
>          return (
> 
> === modified file 'lib/lp/registry/model/person.py'
> --- lib/lp/registry/model/person.py	2015-02-06 15:29:45 +0000
> +++ lib/lp/registry/model/person.py	2015-02-20 16:35:44 +0000
> @@ -1,4 +1,4 @@
> -# Copyright 2009-2014 Canonical Ltd.  This software is licensed under the
> +# Copyright 2009-2015 Canonical Ltd.  This software is licensed under the
>  # GNU Affero General Public License version 3 (see the file LICENSE).
>  
>  """Implementation classes for a Person."""
> @@ -140,7 +140,9 @@
>      )
>  from lp.bugs.model.bugtarget import HasBugsBase
>  from lp.bugs.model.structuralsubscription import StructuralSubscription
> +from lp.code.errors import GitTargetError
>  from lp.code.interfaces.branchcollection import IAllBranches
> +from lp.code.interfaces.gitrepository import IGitRepositorySet
>  from lp.code.model.hasbranches import (
>      HasBranchesMixin,
>      HasMergeProposalsMixin,
> @@ -3249,6 +3251,27 @@
>          grants = [(policy, self, self)]
>          getUtility(IAccessPolicyGrantSource).grant(grants)
>  
> +    def getDefaultGitRepository(self, target):
> +        """See `IPerson`."""
> +        return getUtility(IGitRepositorySet).getDefaultRepository(target, self)
> +
> +    def setDefaultGitRepository(self, target, git_repository):
> +        """See `IPerson`."""
> +        if IPerson.providedBy(target):
> +            raise GitTargetError(
> +                "Cannot set a person's default Git repository for a person, "
> +                "only for a project or a package.")
> +        if git_repository is not None:
> +            if git_repository.target != target:
> +                raise GitTargetError(
> +                    "Cannot set default Git repository to one attached to "
> +                    "another target.")
> +            git_repository.setOwnerDefault(True)
> +        else:
> +            previous = self.getDefaultGitRepository(target)
> +            if previous is not None:
> +                previous.setOwnerDefault(False)
> +
>  
>  class PersonSet:
>      """The set of persons."""
> 
> === modified file 'lib/lp/registry/model/persondistributionsourcepackage.py'
> --- lib/lp/registry/model/persondistributionsourcepackage.py	2015-02-06 13:37:58 +0000
> +++ lib/lp/registry/model/persondistributionsourcepackage.py	2015-02-20 16:35:44 +0000
> @@ -37,3 +37,12 @@
>      def displayname(self):
>          return '%s in %s' % (
>              self.person.displayname, self.distro_source_package.displayname)
> +
> +    def __eq__(self, other):
> +        return (
> +            IPersonDistributionSourcePackage.providedBy(other) and
> +            self.person.id == other.person.id and
> +            self.distro_source_package == other.distro_source_package)
> +
> +    def __ne__(self, other):
> +        return not self == other
> 
> === modified file 'lib/lp/registry/model/personproduct.py'
> --- lib/lp/registry/model/personproduct.py	2011-12-19 23:38:16 +0000
> +++ lib/lp/registry/model/personproduct.py	2015-02-20 16:35:44 +0000
> @@ -38,3 +38,12 @@
>      def displayname(self):
>          return '%s in %s' % (
>              self.person.displayname, self.product.displayname)
> +
> +    def __eq__(self, other):
> +        return (
> +            IPersonProduct.providedBy(other) and
> +            self.person.id == other.person.id and
> +            self.product.id == other.product.id)
> +
> +    def __ne__(self, other):
> +        return not self == other
> 
> === modified file 'lib/lp/registry/model/product.py'
> --- lib/lp/registry/model/product.py	2015-02-13 17:16:22 +0000
> +++ lib/lp/registry/model/product.py	2015-02-20 16:35:44 +0000
> @@ -1,4 +1,4 @@
> -# Copyright 2009-2013 Canonical Ltd.  This software is licensed under the
> +# Copyright 2009-2015 Canonical Ltd.  This software is licensed under the
>  # GNU Affero General Public License version 3 (see the file LICENSE).
>  
>  """Database classes including and related to Product."""
> @@ -116,7 +116,9 @@
>      StructuralSubscriptionTargetMixin,
>      )
>  from lp.code.enums import BranchType
> +from lp.code.errors import GitTargetError
>  from lp.code.interfaces.branch import DEFAULT_BRANCH_STATUS_IN_LISTING
> +from lp.code.interfaces.gitrepository import IGitRepositorySet
>  from lp.code.model.branch import Branch
>  from lp.code.model.branchnamespace import BRANCH_POLICY_ALLOWED_TYPES
>  from lp.code.model.hasbranches import (
> @@ -1569,6 +1571,23 @@
>              return True
>          return False
>  
> +    def getDefaultGitRepository(self):
> +        """See `IProductView`."""
> +        return getUtility(IGitRepositorySet).getDefaultRepository(self)
> +
> +    def setDefaultGitRepository(self, git_repository):
> +        """See `IProductEditRestricted`."""
> +        if git_repository is not None:
> +            if git_repository.target != self:
> +                raise GitTargetError(
> +                    "Cannot set default Git repository to one attached to "
> +                    "another target.")
> +            git_repository.setTargetDefault(True)
> +        else:
> +            previous = self.getDefaultGitRepository()
> +            if previous is not None:
> +                previous.setTargetDefault(False)

These are suspiciously identical to the DSP implementations.

> +
>  
>  def get_precached_products(products, need_licences=False,
>                             need_projectgroups=False, need_series=False,
> 
> === modified file 'lib/lp/registry/tests/test_distributionsourcepackage.py'
> --- lib/lp/registry/tests/test_distributionsourcepackage.py	2014-04-24 06:45:51 +0000
> +++ lib/lp/registry/tests/test_distributionsourcepackage.py	2015-02-20 16:35:44 +0000
> @@ -1,4 +1,4 @@
> -# Copyright 2009-2011 Canonical Ltd.  This software is licensed under the
> +# Copyright 2009-2015 Canonical Ltd.  This software is licensed under the
>  # GNU Affero General Public License version 3 (see the file LICENSE).
>  
>  """Tests for DistributionSourcePackage."""
> @@ -11,6 +11,7 @@
>  from zope.component import getUtility
>  from zope.security.proxy import removeSecurityProxy
>  
> +from lp.code.errors import GitTargetError
>  from lp.registry.interfaces.distribution import IDistributionSet
>  from lp.registry.model.distributionsourcepackage import (
>      DistributionSourcePackage,
> @@ -22,6 +23,7 @@
>  from lp.soyuz.enums import PackagePublishingStatus
>  from lp.soyuz.tests.test_publishing import SoyuzTestPublisher
>  from lp.testing import (
> +    person_logged_in,
>      StormStatementRecorder,
>      TestCaseWithFactory,
>      )
> @@ -165,6 +167,36 @@
>          driver = distribution.drivers[0]
>          self.assertTrue(dsp.personHasDriverRights(driver))
>  
> +    def test_default_git_repository_round_trip(self):
> +        # A default Git repository set using setDefaultGitRepository can be
> +        # retrieved using getDefaultGitRepository.
> +        dsp = self.factory.makeDistributionSourcePackage()
> +        self.assertIsNone(dsp.getDefaultGitRepository())
> +        repository = self.factory.makeGitRepository(target=dsp)
> +        with person_logged_in(dsp.distribution.owner):
> +            dsp.setDefaultGitRepository(repository)
> +        self.assertEqual(repository, dsp.getDefaultGitRepository())
> +
> +    def test_setDefaultGitRepository_None(self):
> +        # setDefaultGitRepository(None) clears the default.
> +        dsp = self.factory.makeDistributionSourcePackage()
> +        repository = self.factory.makeGitRepository(target=dsp)
> +        with person_logged_in(dsp.distribution.owner):
> +            dsp.setDefaultGitRepository(repository)
> +            dsp.setDefaultGitRepository(None)
> +        self.assertIsNone(dsp.getDefaultGitRepository())
> +
> +    def test_setDefaultGitRepository_different_target(self):
> +        # setDefaultGitRepository refuses if the repository is attached to a
> +        # different target.
> +        dsp = self.factory.makeDistributionSourcePackage()
> +        other_dsp = self.factory.makeDistributionSourcePackage(
> +            distribution=dsp.distribution)
> +        repository = self.factory.makeGitRepository(target=other_dsp)
> +        with person_logged_in(dsp.distribution.owner):
> +            self.assertRaises(
> +                GitTargetError, dsp.setDefaultGitRepository, repository)
> +
>  
>  class TestDistributionSourcePackageFindRelatedArchives(TestCaseWithFactory):
>  
> 
> === modified file 'lib/lp/registry/tests/test_person.py'
> --- lib/lp/registry/tests/test_person.py	2015-01-07 00:35:41 +0000
> +++ lib/lp/registry/tests/test_person.py	2015-02-20 16:35:44 +0000
> @@ -1,4 +1,4 @@
> -# Copyright 2009-2013 Canonical Ltd.  This software is licensed under the
> +# Copyright 2009-2015 Canonical Ltd.  This software is licensed under the
>  # GNU Affero General Public License version 3 (see the file LICENSE).
>  
>  __metaclass__ = type
> @@ -39,6 +39,7 @@
>      IllegalRelatedBugTasksParams,
>      )
>  from lp.bugs.model.bug import Bug
> +from lp.code.errors import GitTargetError
>  from lp.registry.enums import (
>      PersonVisibility,
>      TeamMembershipPolicy,
> @@ -774,6 +775,51 @@
>          self.assertContentEqual(
>              [], getUtility(IAccessPolicySource).findByTeam([team]))
>  
> +    def test_default_git_repository_round_trip(self):
> +        # A default Git repository set using setDefaultGitRepository can be
> +        # retrieved using getDefaultGitRepository.
> +        person = self.factory.makePerson()
> +        project = self.factory.makeProduct()
> +        self.assertIsNone(person.getDefaultGitRepository(project))
> +        repository = self.factory.makeGitRepository(
> +            owner=person, target=project)
> +        with person_logged_in(person):
> +            person.setDefaultGitRepository(project, repository)
> +        self.assertEqual(repository, person.getDefaultGitRepository(project))
> +
> +    def test_setDefaultGitRepository_None(self):
> +        # setDefaultGitRepository(target, None) clears the default.
> +        person = self.factory.makePerson()
> +        project = self.factory.makeProduct()
> +        repository = self.factory.makeGitRepository(
> +            owner=person, target=project)
> +        with person_logged_in(person):
> +            person.setDefaultGitRepository(project, repository)
> +            person.setDefaultGitRepository(project, None)
> +        self.assertIsNone(person.getDefaultGitRepository(project))
> +
> +    def test_setDefaultGitRepository_refuses_person(self):
> +        # setDefaultGitRepository refuses if the target is a person.
> +        person = self.factory.makePerson()
> +        repository = self.factory.makeGitRepository(owner=person)
> +        with person_logged_in(person):
> +            self.assertRaises(
> +                GitTargetError, person.setDefaultGitRepository, person,
> +                repository)
> +
> +    def test_setDefaultGitRepository_different_target(self):
> +        # setDefaultGitRepository refuses if the repository is attached to a
> +        # different target.
> +        person = self.factory.makePerson()
> +        project = self.factory.makeProduct()
> +        other_project = self.factory.makeProduct()
> +        repository = self.factory.makeGitRepository(
> +            owner=person, target=other_project)
> +        with person_logged_in(person):
> +            self.assertRaises(
> +                GitTargetError, person.setDefaultGitRepository, project,
> +                repository)
> +
>  
>  class TestPersonStates(TestCaseWithFactory):
>  
> 
> === modified file 'lib/lp/registry/tests/test_product.py'
> --- lib/lp/registry/tests/test_product.py	2015-02-19 19:23:23 +0000
> +++ lib/lp/registry/tests/test_product.py	2015-02-20 16:35:44 +0000
> @@ -1,4 +1,4 @@
> -# Copyright 2009-2013 Canonical Ltd.  This software is licensed under the
> +# Copyright 2009-2015 Canonical Ltd.  This software is licensed under the
>  # GNU Affero General Public License version 3 (see the file LICENSE).
>  
>  __metaclass__ = type
> @@ -56,6 +56,7 @@
>  from lp.bugs.interfaces.bugsummary import IBugSummaryDimension
>  from lp.bugs.interfaces.bugsupervisor import IHasBugSupervisor
>  from lp.bugs.interfaces.bugtarget import BUG_POLICY_ALLOWED_TYPES
> +from lp.code.errors import GitTargetError
>  from lp.code.model.branchnamespace import BRANCH_POLICY_ALLOWED_TYPES
>  from lp.registry.enums import (
>      BranchSharingPolicy,
> @@ -1227,6 +1228,35 @@
>          self.assertIsNot(None, getUtility(IAccessPolicySource).find(
>              [(product, InformationType.EMBARGOED)]).one())
>  
> +    def test_default_git_repository_round_trip(self):
> +        # A default Git repository set using setDefaultGitRepository can be
> +        # retrieved using getDefaultGitRepository.
> +        product = self.factory.makeProduct()
> +        self.assertIsNone(product.getDefaultGitRepository())
> +        repository = self.factory.makeGitRepository(target=product)
> +        with person_logged_in(product.owner):
> +            product.setDefaultGitRepository(repository)
> +        self.assertEqual(repository, product.getDefaultGitRepository())
> +
> +    def test_setDefaultGitRepository_None(self):
> +        # setDefaultGitRepository(None) clears the default.
> +        product = self.factory.makeProduct()
> +        repository = self.factory.makeGitRepository(target=product)
> +        with person_logged_in(product.owner):
> +            product.setDefaultGitRepository(repository)
> +            product.setDefaultGitRepository(None)
> +        self.assertIsNone(product.getDefaultGitRepository())
> +
> +    def test_setDefaultGitRepository_different_target(self):
> +        # setDefaultGitRepository refuses if the repository is attached to a
> +        # different target.
> +        product = self.factory.makeProduct()
> +        other_product = self.factory.makeProduct()
> +        repository = self.factory.makeGitRepository(target=other_product)
> +        with person_logged_in(product.owner):
> +            self.assertRaises(
> +                GitTargetError, product.setDefaultGitRepository, repository)
> +
>  
>  class TestProductBugInformationTypes(TestCaseWithFactory):
>  
> 


-- 
https://code.launchpad.net/~cjwatson/launchpad/git-defaults/+merge/250474
Your team Launchpad code reviewers is subscribed to branch lp:launchpad.