launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Diff comments:

> 
> === modified file 'lib/lp/registry/personmerge.py'
> --- lib/lp/registry/personmerge.py	2015-09-16 13:30:33 +0000
> +++ lib/lp/registry/personmerge.py	2018-09-25 18:01:53 +0000
> @@ -141,6 +141,25 @@
>          ''' % vars())
>  
>  
> +def _mergeGitRuleGrant(cur, from_id, to_id):
> +    # Update only the GitRuleGrants that will not conflict.
> +    cur.execute('''
> +        UPDATE GitRuleGrant
> +        SET grantee=%(to_id)d
> +        WHERE
> +            grantee = %(from_id)d
> +            AND rule NOT IN (
> +                SELECT rule
> +                FROM GitRuleGrant
> +                WHERE grantee = %(to_id)d
> +                )
> +        ''' % vars())
> +    # and delete those left over.
> +    cur.execute('''
> +        DELETE FROM GitRuleGrant WHERE grantee = %(from_id)d
> +        ''' % vars())

It's a good question; but true deny grants are only really a thing for grantee_type == REPOSITORY_OWNER, and those grants don't participate in person merging.  In all other cases, there are no implicit permissions, so there's nothing to deny.

However, on a similar note, we normally resolve multiple matching grants (e.g. different grants to two teams both of which a given user is a member) by taking the union of the permissions they grant; so perhaps we should do the same here, and do something like:

    UPDATE GitRuleGrant
    SET
        can_create = can_create OR other.can_create,
        can_push = can_push OR other.can_push,
        can_force_push = can_force_push OR other.can_force_push
    FROM GitRuleGrant AS other
    WHERE
        GitRuleGrant.grantee = %(from_id)d
        AND other.grantee = %(to_id)d
        AND GitRuleGrant.rule = other.rule;

What do you think?  It seems to make conceptual sense to me.

> +
> +
>  def _mergeBranches(from_person, to_person):
>      # This shouldn't use removeSecurityProxy.
>      branches = getUtility(IBranchCollection).ownedBy(from_person)


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