Patchwork [2,of,2,v2] acl: support revsets [RFC]

# HG changeset patch
# User timeless <timeless@mozdev.org>
# Date 1462327099 0
#      Wed May 04 01:58:19 2016 +0000
# Node ID d4a9c04f6c504f24b7c8e228d5bff61118760573
# Parent  c5ad786b58c50a6b751217f88ca1626a19c3ff30
# Available At bb://timeless/mercurial-crew
#              hg pull bb://timeless/mercurial-crew -r d4a9c04f6c50
acl: support revsets [RFC]

This is an attempt to support revsets.
It works.
I can update the documentation if we want to go this route,
and add direct samples to the tests for it.

This change makes branch acl items into revset: items, which means
there is automatic testing of the revset: path.

----
An unfortunate side-effect of this change is that I've lost
the reporting indicating why a commit was rejected.

It's possible to re-engineer things so that such reporting could be
done.
----

Afaict, it does not help w/ bookmarks at all,
as bookmarks aren't applied until after the hook runs.
And I don't think one can roll back a bookmark by that point,
as I think the previous information would be lost.

On Thu, May 05, 2016 at 08:45:55AM +0000, timeless wrote:
> # HG changeset patch
> # User timeless <timeless@mozdev.org>
> # Date 1462327099 0
> #      Wed May 04 01:58:19 2016 +0000
> # Node ID d4a9c04f6c504f24b7c8e228d5bff61118760573
> # Parent  c5ad786b58c50a6b751217f88ca1626a19c3ff30
> # Available At bb://timeless/mercurial-crew
> #              hg pull bb://timeless/mercurial-crew -r d4a9c04f6c50
> acl: support revsets [RFC]
>
> This is an attempt to support revsets.
> It works.
> I can update the documentation if we want to go this route,
> and add direct samples to the tests for it.
>
> This change makes branch acl items into revset: items, which means
> there is automatic testing of the revset: path.
>
> ----
> An unfortunate side-effect of this change is that I've lost
> the reporting indicating why a commit was rejected.
>
> It's possible to re-engineer things so that such reporting could be
> done.
> ----
>
> Afaict, it does not help w/ bookmarks at all,
> as bookmarks aren't applied until after the hook runs.
> And I don't think one can roll back a bookmark by that point,
> as I think the previous information would be lost.

Hm, you might well be right. Sounds like maybe we should consider both
patches? Being able to use revsets here seems about right to me feel-wise.

>
> diff -r c5ad786b58c5 -r d4a9c04f6c50 hgext/acl.py
> --- a/hgext/acl.py	Thu Mar 03 23:29:26 2016 +0000
> +++ b/hgext/acl.py	Wed May 04 01:58:19 2016 +0000
> @@ -216,11 +216,12 @@
>  from __future__ import absolute_import
>
>  import getpass
> +import os
>
>  from mercurial.i18n import _
>  from mercurial import (
>      error,
> -    match,
> +    scmutil,
>      util,
>  )
>
> @@ -277,30 +278,47 @@
>          ui.debug('acl: %s not enabled\n' % key)
>          return None
>
> -    pats = [pat for pat, users in ui.configitems(key)
> +    pats = [pat.lstrip() for pat, users in ui.configitems(key)
>              if _usermatch(ui, user, users)]
>      ui.debug('acl: %s enabled, %d entries for user %s\n' %
>               (key, len(pats), user))
>
> +    # Arg-based ACL
> +    if not repo:
> +        if not pats:
> +            return util.never
> +        # If there's an asterisk (meaning "any"), always return True;
> +        # Otherwise, test if b is in pats
> +        if '*' in pats:
> +            return util.always
> +        return lambda b: b in pats
> +
>      # Branch-based ACL
> -    if not repo:
> -        if pats:
> -            # If there's an asterisk (meaning "any branch"), always return True;
> -            # Otherwise, test if b is in pats
> -            if '*' in pats:
> -                return util.always
> -            return lambda b: b in pats
> +    if key.endswith("branches"):
> +        # If there's an asterisk (meaning "any branch"), always return True;
> +        # Otherwise, test if b is in pats
> +        pats = [('revset:branch("%s")' % branch if branch != '*'
> +                                              else 'revset:all()')
> +                for branch in pats]
> +    # Convert Path based ACL to revsets
> +    revsets = [(pat[7:] if pat.startswith('revset:') else 'file("%s")' % pat)
> +            for pat in pats]
> +
> +    if not revsets:
>          return util.never
>
> -    # Path-based ACL
> -    if pats:
> -        return match.match(repo.root, '', pats)
> -    return util.never
> +    # Revset-based ACL
> +    rule = ' + '.join(revsets)
> +    # it might be nice to expose the rule to the client, but
> +    # this API isn't particularly friendly to that.
> +    criteria = '(%s) & (%s)' % (rule, '%s')
> +    return lambda b: scmutil.revrange(repo, [criteria % b])
>
>  def hook(ui, repo, hooktype, node=None, source=None, **kwargs):
>      if hooktype not in ['pretxnchangegroup', 'pretxncommit', 'prepushkey']:
>          raise error.Abort(_('config error - hook type "%s" cannot stop '
> -                           'incoming changesets, commits, nor bookmarks') % hooktype)
> +                            'incoming changesets, commits, nor bookmarks')
> +                          % hooktype)
>      if (hooktype == 'pretxnchangegroup' and
>          source not in ui.config('acl', 'sources', 'serve').split()):
>          ui.debug('acl: changes have source "%s" - skipping\n' % source)
> @@ -347,30 +365,39 @@
>          ui.readconfig(cfg, sections=['acl.groups', 'acl.allow.branches',
>              'acl.deny.branches', 'acl.allow', 'acl.deny'])
>
> -    allowbranches = buildmatch(ui, None, user, 'acl.allow.branches')
> -    denybranches = buildmatch(ui, None, user, 'acl.deny.branches')
> +    allowbranches = buildmatch(ui, repo, user, 'acl.allow.branches')
> +    denybranches = buildmatch(ui, repo, user, 'acl.deny.branches')
>      allow = buildmatch(ui, repo, user, 'acl.allow')
>      deny = buildmatch(ui, repo, user, 'acl.deny')
>
> -    for rev in xrange(repo[node], len(repo)):
> -        ctx = repo[rev]
> -        branch = ctx.branch()
> -        if denybranches and denybranches(branch):
> -            raise error.Abort(_('acl: user "%s" denied on branch "%s"'
> +    def checkbranches(user, ctx):
> +        if denybranches and denybranches(ctx):
> +            raise error.Abort(_('acl: user "%s" denied'
>                                 ' (changeset "%s")')
> -                               % (user, branch, ctx))
> -        if allowbranches and not allowbranches(branch):
> -            raise error.Abort(_('acl: user "%s" not allowed on branch "%s"'
> +                               % (user, ctx))
> +        if allowbranches and not allowbranches(ctx):
> +            raise error.Abort(_('acl: user "%s" not allowed'
>                                 ' (changeset "%s")')
> -                               % (user, branch, ctx))
> -        ui.debug('acl: branch access granted: "%s" on branch "%s"\n'
> -        % (ctx, branch))
> +                               % (user, ctx))
>
> -        for f in ctx.files():
> -            if deny and deny(f):
> -                raise error.Abort(_('acl: user "%s" denied on "%s"'
> -                ' (changeset "%s")') % (user, f, ctx))
> -            if allow and not allow(f):
> -                raise error.Abort(_('acl: user "%s" not allowed on "%s"'
> -                ' (changeset "%s")') % (user, f, ctx))
> -        ui.debug('acl: path access granted: "%s"\n' % ctx)
> +    def checkctx(user, ctx):
> +        if deny and deny(ctx):
> +            raise error.Abort(_('acl: user "%s" denied'
> +                ' (changeset "%s")') % (user, ctx))
> +        if allow and not allow(ctx):
> +            raise error.Abort(_('acl: user "%s" not allowed'
> +                ' (changeset "%s")') % (user, ctx))
> +    cwd = os.getcwd()
> +    os.chdir(os.path.dirname(repo.path))
> +    try:
> +        for rev in xrange(repo[node], len(repo)):
> +            ctx = repo[rev]
> +            branch = ctx.branch()
> +            checkbranches(user, ctx)
> +            ui.debug('acl: branch access granted: "%s" on branch "%s"\n'
> +            % (ctx, branch))
> +
> +            checkctx(user, ctx)
> +            ui.debug('acl: path access granted: "%s"\n' % ctx)
> +    finally:
> +        os.chdir(cwd)
> diff -r c5ad786b58c5 -r d4a9c04f6c50 tests/test-acl.t
> --- a/tests/test-acl.t	Thu Mar 03 23:29:26 2016 +0000
> +++ b/tests/test-acl.t	Wed May 04 01:58:19 2016 +0000
> @@ -340,12 +340,12 @@
>    acl: acl.allow enabled, 0 entries for user fred
>    acl: acl.deny not enabled
>    acl: branch access granted: "ef1ea85a6374" on branch "default"
> -  error: pretxnchangegroup.acl hook failed: acl: user "fred" not allowed on "foo/file.txt" (changeset "ef1ea85a6374")
> +  error: pretxnchangegroup.acl hook failed: acl: user "fred" not allowed (changeset "ef1ea85a6374")
>    bundle2-input-part: total payload size 1606
>    bundle2-input-bundle: 3 parts total
>    transaction abort!
>    rollback completed
> -  abort: acl: user "fred" not allowed on "foo/file.txt" (changeset "ef1ea85a6374")
> +  abort: acl: user "fred" not allowed (changeset "ef1ea85a6374")
>    no rollback information available
>    0:6675d58eff77
>
> @@ -410,12 +410,12 @@
>    acl: branch access granted: "f9cafe1212c8" on branch "default"
>    acl: path access granted: "f9cafe1212c8"
>    acl: branch access granted: "911600dab2ae" on branch "default"
> -  error: pretxnchangegroup.acl hook failed: acl: user "fred" not allowed on "quux/file.py" (changeset "911600dab2ae")
> +  error: pretxnchangegroup.acl hook failed: acl: user "fred" not allowed (changeset "911600dab2ae")
>    bundle2-input-part: total payload size 1606
>    bundle2-input-bundle: 3 parts total
>    transaction abort!
>    rollback completed
> -  abort: acl: user "fred" not allowed on "quux/file.py" (changeset "911600dab2ae")
> +  abort: acl: user "fred" not allowed (changeset "911600dab2ae")
>    no rollback information available
>    0:6675d58eff77
>
> @@ -477,12 +477,12 @@
>    acl: acl.allow enabled, 0 entries for user barney
>    acl: acl.deny enabled, 0 entries for user barney
>    acl: branch access granted: "ef1ea85a6374" on branch "default"
> -  error: pretxnchangegroup.acl hook failed: acl: user "barney" not allowed on "foo/file.txt" (changeset "ef1ea85a6374")
> +  error: pretxnchangegroup.acl hook failed: acl: user "barney" not allowed (changeset "ef1ea85a6374")
>    bundle2-input-part: total payload size 1606
>    bundle2-input-bundle: 3 parts total
>    transaction abort!
>    rollback completed
> -  abort: acl: user "barney" not allowed on "foo/file.txt" (changeset "ef1ea85a6374")
> +  abort: acl: user "barney" not allowed (changeset "ef1ea85a6374")
>    no rollback information available
>    0:6675d58eff77
>
> @@ -549,12 +549,12 @@
>    acl: branch access granted: "f9cafe1212c8" on branch "default"
>    acl: path access granted: "f9cafe1212c8"
>    acl: branch access granted: "911600dab2ae" on branch "default"
> -  error: pretxnchangegroup.acl hook failed: acl: user "fred" not allowed on "quux/file.py" (changeset "911600dab2ae")
> +  error: pretxnchangegroup.acl hook failed: acl: user "fred" not allowed (changeset "911600dab2ae")
>    bundle2-input-part: total payload size 1606
>    bundle2-input-bundle: 3 parts total
>    transaction abort!
>    rollback completed
> -  abort: acl: user "fred" not allowed on "quux/file.py" (changeset "911600dab2ae")
> +  abort: acl: user "fred" not allowed (changeset "911600dab2ae")
>    no rollback information available
>    0:6675d58eff77
>
> @@ -620,12 +620,12 @@
>    acl: branch access granted: "ef1ea85a6374" on branch "default"
>    acl: path access granted: "ef1ea85a6374"
>    acl: branch access granted: "f9cafe1212c8" on branch "default"
> -  error: pretxnchangegroup.acl hook failed: acl: user "fred" denied on "foo/Bar/file.txt" (changeset "f9cafe1212c8")
> +  error: pretxnchangegroup.acl hook failed: acl: user "fred" denied (changeset "f9cafe1212c8")
>    bundle2-input-part: total payload size 1606
>    bundle2-input-bundle: 3 parts total
>    transaction abort!
>    rollback completed
> -  abort: acl: user "fred" denied on "foo/Bar/file.txt" (changeset "f9cafe1212c8")
> +  abort: acl: user "fred" denied (changeset "f9cafe1212c8")
>    no rollback information available
>    0:6675d58eff77
>
> @@ -688,12 +688,12 @@
>    acl: acl.allow enabled, 0 entries for user barney
>    acl: acl.deny enabled, 0 entries for user barney
>    acl: branch access granted: "ef1ea85a6374" on branch "default"
> -  error: pretxnchangegroup.acl hook failed: acl: user "barney" not allowed on "foo/file.txt" (changeset "ef1ea85a6374")
> +  error: pretxnchangegroup.acl hook failed: acl: user "barney" not allowed (changeset "ef1ea85a6374")
>    bundle2-input-part: total payload size 1606
>    bundle2-input-bundle: 3 parts total
>    transaction abort!
>    rollback completed
> -  abort: acl: user "barney" not allowed on "foo/file.txt" (changeset "ef1ea85a6374")
> +  abort: acl: user "barney" not allowed (changeset "ef1ea85a6374")
>    no rollback information available
>    0:6675d58eff77
>
> @@ -754,8 +754,8 @@
>    acl: acl.allow enabled, 1 entries for user fred
>    acl: acl.deny enabled, 2 entries for user fred
>    acl: branch access granted: "ef1ea85a6374" on branch "default"
> +  invalid branchheads cache (served): tip differs
>    acl: path access granted: "ef1ea85a6374"
> -  invalid branchheads cache (served): tip differs
>    bundle2-input-part: total payload size 537
>    bundle2-input-part: "pushkey" (params: 4 mandatory) supported
>    calling hook prepushkey.acl: hgext.acl.hook
> @@ -841,8 +841,8 @@
>    acl: acl.allow enabled, 1 entries for user fred
>    acl: acl.deny enabled, 2 entries for user fred
>    acl: branch access granted: "ef1ea85a6374" on branch "default"
> +  invalid branchheads cache (served): tip differs
>    acl: path access granted: "ef1ea85a6374"
> -  invalid branchheads cache (served): tip differs
>    bundle2-input-part: total payload size 537
>    bundle2-input-part: "pushkey" (params: 4 mandatory) supported
>    calling hook prepushkey.acl: hgext.acl.hook
> @@ -1020,12 +1020,12 @@
>    acl: branch access granted: "f9cafe1212c8" on branch "default"
>    acl: path access granted: "f9cafe1212c8"
>    acl: branch access granted: "911600dab2ae" on branch "default"
> -  error: pretxnchangegroup.acl hook failed: acl: user "wilma" not allowed on "quux/file.py" (changeset "911600dab2ae")
> +  error: pretxnchangegroup.acl hook failed: acl: user "wilma" not allowed (changeset "911600dab2ae")
>    bundle2-input-part: total payload size 1606
>    bundle2-input-bundle: 3 parts total
>    transaction abort!
>    rollback completed
> -  abort: acl: user "wilma" not allowed on "quux/file.py" (changeset "911600dab2ae")
> +  abort: acl: user "wilma" not allowed (changeset "911600dab2ae")
>    no rollback information available
>    0:6675d58eff77
>
> @@ -1173,12 +1173,12 @@
>    acl: branch access granted: "f9cafe1212c8" on branch "default"
>    acl: path access granted: "f9cafe1212c8"
>    acl: branch access granted: "911600dab2ae" on branch "default"
> -  error: pretxnchangegroup.acl hook failed: acl: user "betty" not allowed on "quux/file.py" (changeset "911600dab2ae")
> +  error: pretxnchangegroup.acl hook failed: acl: user "betty" not allowed (changeset "911600dab2ae")
>    bundle2-input-part: total payload size 1606
>    bundle2-input-bundle: 3 parts total
>    transaction abort!
>    rollback completed
> -  abort: acl: user "betty" not allowed on "quux/file.py" (changeset "911600dab2ae")
> +  abort: acl: user "betty" not allowed (changeset "911600dab2ae")
>    no rollback information available
>    0:6675d58eff77
>
> @@ -1430,12 +1430,12 @@
>    acl: branch access granted: "ef1ea85a6374" on branch "default"
>    acl: path access granted: "ef1ea85a6374"
>    acl: branch access granted: "f9cafe1212c8" on branch "default"
> -  error: pretxnchangegroup.acl hook failed: acl: user "fred" denied on "foo/Bar/file.txt" (changeset "f9cafe1212c8")
> +  error: pretxnchangegroup.acl hook failed: acl: user "fred" denied (changeset "f9cafe1212c8")
>    bundle2-input-part: total payload size 1606
>    bundle2-input-bundle: 3 parts total
>    transaction abort!
>    rollback completed
> -  abort: acl: user "fred" denied on "foo/Bar/file.txt" (changeset "f9cafe1212c8")
> +  abort: acl: user "fred" denied (changeset "f9cafe1212c8")
>    no rollback information available
>    0:6675d58eff77
>
> @@ -1595,12 +1595,12 @@
>    acl: branch access granted: "ef1ea85a6374" on branch "default"
>    acl: path access granted: "ef1ea85a6374"
>    acl: branch access granted: "f9cafe1212c8" on branch "default"
> -  error: pretxnchangegroup.acl hook failed: acl: user "fred" denied on "foo/Bar/file.txt" (changeset "f9cafe1212c8")
> +  error: pretxnchangegroup.acl hook failed: acl: user "fred" denied (changeset "f9cafe1212c8")
>    bundle2-input-part: total payload size 1606
>    bundle2-input-bundle: 3 parts total
>    transaction abort!
>    rollback completed
> -  abort: acl: user "fred" denied on "foo/Bar/file.txt" (changeset "f9cafe1212c8")
> +  abort: acl: user "fred" denied (changeset "f9cafe1212c8")
>    no rollback information available
>    0:6675d58eff77
>
> @@ -1808,12 +1808,12 @@
>    acl: path access granted: "f9cafe1212c8"
>    acl: branch access granted: "911600dab2ae" on branch "default"
>    acl: path access granted: "911600dab2ae"
> -  error: pretxnchangegroup.acl hook failed: acl: user "astro" denied on branch "foobar" (changeset "e8fc755d4d82")
> +  error: pretxnchangegroup.acl hook failed: acl: user "astro" denied (changeset "e8fc755d4d82")
>    bundle2-input-part: total payload size 2101
>    bundle2-input-bundle: 4 parts total
>    transaction abort!
>    rollback completed
> -  abort: acl: user "astro" denied on branch "foobar" (changeset "e8fc755d4d82")
> +  abort: acl: user "astro" denied (changeset "e8fc755d4d82")
>    no rollback information available
>    2:fb35475503ef
>
> @@ -1840,6 +1840,7 @@
>    listing keys for "phases"
>    checking for updated bookmarks
>    listing keys for "bookmarks"
> +  invalid branchheads cache (served): tip differs
>    listing keys for "bookmarks"
>    4 changesets found
>    list of changesets:
> @@ -1877,12 +1878,12 @@
>    acl: acl.deny.branches not enabled
>    acl: acl.allow not enabled
>    acl: acl.deny not enabled
> -  error: pretxnchangegroup.acl hook failed: acl: user "astro" not allowed on branch "default" (changeset "ef1ea85a6374")
> +  error: pretxnchangegroup.acl hook failed: acl: user "astro" not allowed (changeset "ef1ea85a6374")
>    bundle2-input-part: total payload size 2101
>    bundle2-input-bundle: 4 parts total
>    transaction abort!
>    rollback completed
> -  abort: acl: user "astro" not allowed on branch "default" (changeset "ef1ea85a6374")
> +  abort: acl: user "astro" not allowed (changeset "ef1ea85a6374")
>    no rollback information available
>    2:fb35475503ef
>
> @@ -1948,12 +1949,12 @@
>    acl: acl.deny.branches not enabled
>    acl: acl.allow not enabled
>    acl: acl.deny not enabled
> -  error: pretxnchangegroup.acl hook failed: acl: user "astro" not allowed on branch "default" (changeset "ef1ea85a6374")
> +  error: pretxnchangegroup.acl hook failed: acl: user "astro" not allowed (changeset "ef1ea85a6374")
>    bundle2-input-part: total payload size 2101
>    bundle2-input-bundle: 4 parts total
>    transaction abort!
>    rollback completed
> -  abort: acl: user "astro" not allowed on branch "default" (changeset "ef1ea85a6374")
> +  abort: acl: user "astro" not allowed (changeset "ef1ea85a6374")
>    no rollback information available
>    2:fb35475503ef
>
> @@ -2208,12 +2209,12 @@
>    acl: acl.deny.branches enabled, 1 entries for user george
>    acl: acl.allow not enabled
>    acl: acl.deny not enabled
> -  error: pretxnchangegroup.acl hook failed: acl: user "george" denied on branch "default" (changeset "ef1ea85a6374")
> +  error: pretxnchangegroup.acl hook failed: acl: user "george" denied (changeset "ef1ea85a6374")
>    bundle2-input-part: total payload size 2101
>    bundle2-input-bundle: 4 parts total
>    transaction abort!
>    rollback completed
> -  abort: acl: user "george" denied on branch "default" (changeset "ef1ea85a6374")
> +  abort: acl: user "george" denied (changeset "ef1ea85a6374")
>    no rollback information available
>    2:fb35475503ef
>
> @@ -2241,6 +2242,7 @@
>    listing keys for "phases"
>    checking for updated bookmarks
>    listing keys for "bookmarks"
> +  invalid branchheads cache (served): tip differs
>    listing keys for "bookmarks"
>    4 changesets found
>    list of changesets:
> @@ -2369,12 +2371,12 @@
>    acl: acl.deny.branches enabled, 1 entries for user george
>    acl: acl.allow not enabled
>    acl: acl.deny not enabled
> -  error: pretxnchangegroup.acl hook failed: acl: user "george" denied on branch "default" (changeset "ef1ea85a6374")
> +  error: pretxnchangegroup.acl hook failed: acl: user "george" denied (changeset "ef1ea85a6374")
>    bundle2-input-part: total payload size 2101
>    bundle2-input-bundle: 4 parts total
>    transaction abort!
>    rollback completed
> -  abort: acl: user "george" denied on branch "default" (changeset "ef1ea85a6374")
> +  abort: acl: user "george" denied (changeset "ef1ea85a6374")
>    no rollback information available
>    2:fb35475503ef
>
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel

On 05/11/2016 03:39 AM, Augie Fackler wrote:
> On Thu, May 05, 2016 at 08:45:55AM +0000, timeless wrote:
>> # HG changeset patch
>> # User timeless <timeless@mozdev.org>
>> # Date 1462327099 0
>> #      Wed May 04 01:58:19 2016 +0000
>> # Node ID d4a9c04f6c504f24b7c8e228d5bff61118760573
>> # Parent  c5ad786b58c50a6b751217f88ca1626a19c3ff30
>> # Available At bb://timeless/mercurial-crew
>> #              hg pull bb://timeless/mercurial-crew -r d4a9c04f6c50
>> acl: support revsets [RFC]
>>
>> This is an attempt to support revsets.
>> It works.
>> I can update the documentation if we want to go this route,
>> and add direct samples to the tests for it.
>>
>> This change makes branch acl items into revset: items, which means
>> there is automatic testing of the revset: path.
>>
>> ----
>> An unfortunate side-effect of this change is that I've lost
>> the reporting indicating why a commit was rejected.
>>
>> It's possible to re-engineer things so that such reporting could be
>> done.

We can probably have a config to report a spefic message for a rule, 
don't we?

>> ----
>>
>> Afaict, it does not help w/ bookmarks at all,
>> as bookmarks aren't applied until after the hook runs.
>> And I don't think one can roll back a bookmark by that point,
>> as I think the previous information would be lost.
> Hm, you might well be right. Sounds like maybe we should consider both
> patches? Being able to use revsets here seems about right to me feel-wise.

This series creates the following though flow on my side:

1) Why do user need to setup hook by hand that seems like something acl 
should do by itself
2) Bookmark handling would be better/safer at the transaction level, but 
we don't have good hook data at that point
3) If we improve hooks used, user will have to adjust config to take 
advantage of it (because of (1))

This create a tickling temptation to rework acl to register hooks and 
improve transaction to track bookmark event before moving forward here.

>
>> diff -r c5ad786b58c5 -r d4a9c04f6c50 hgext/acl.py
>> --- a/hgext/acl.py	Thu Mar 03 23:29:26 2016 +0000
>> +++ b/hgext/acl.py	Wed May 04 01:58:19 2016 +0000
>> @@ -216,11 +216,12 @@
>>   from __future__ import absolute_import
>>
>>   import getpass
>> +import os
>>
>>   from mercurial.i18n import _
>>   from mercurial import (
>>       error,
>> -    match,
>> +    scmutil,
>>       util,
>>   )
>>
>> @@ -277,30 +278,47 @@
>>           ui.debug('acl: %s not enabled\n' % key)
>>           return None
>>
>> -    pats = [pat for pat, users in ui.configitems(key)
>> +    pats = [pat.lstrip() for pat, users in ui.configitems(key)
>>               if _usermatch(ui, user, users)]
>>       ui.debug('acl: %s enabled, %d entries for user %s\n' %
>>                (key, len(pats), user))
>>
>> +    # Arg-based ACL
>> +    if not repo:
>> +        if not pats:
>> +            return util.never
>> +        # If there's an asterisk (meaning "any"), always return True;
>> +        # Otherwise, test if b is in pats
>> +        if '*' in pats:
>> +            return util.always
>> +        return lambda b: b in pats
>> +
>>       # Branch-based ACL
>> -    if not repo:
>> -        if pats:
>> -            # If there's an asterisk (meaning "any branch"), always return True;
>> -            # Otherwise, test if b is in pats
>> -            if '*' in pats:
>> -                return util.always
>> -            return lambda b: b in pats
>> +    if key.endswith("branches"):
>> +        # If there's an asterisk (meaning "any branch"), always return True;
>> +        # Otherwise, test if b is in pats
>> +        pats = [('revset:branch("%s")' % branch if branch != '*'
>> +                                              else 'revset:all()')
>> +                for branch in pats]
>> +    # Convert Path based ACL to revsets
>> +    revsets = [(pat[7:] if pat.startswith('revset:') else 'file("%s")' % pat)
>> +            for pat in pats]
>> +
>> +    if not revsets:
>>           return util.never
>>
>> -    # Path-based ACL
>> -    if pats:
>> -        return match.match(repo.root, '', pats)
>> -    return util.never
>> +    # Revset-based ACL
>> +    rule = ' + '.join(revsets)
>> +    # it might be nice to expose the rule to the client, but
>> +    # this API isn't particularly friendly to that.
>> +    criteria = '(%s) & (%s)' % (rule, '%s')
>> +    return lambda b: scmutil.revrange(repo, [criteria % b])
>>
>>   def hook(ui, repo, hooktype, node=None, source=None, **kwargs):
>>       if hooktype not in ['pretxnchangegroup', 'pretxncommit', 'prepushkey']:
>>           raise error.Abort(_('config error - hook type "%s" cannot stop '
>> -                           'incoming changesets, commits, nor bookmarks') % hooktype)
>> +                            'incoming changesets, commits, nor bookmarks')
>> +                          % hooktype)
>>       if (hooktype == 'pretxnchangegroup' and
>>           source not in ui.config('acl', 'sources', 'serve').split()):
>>           ui.debug('acl: changes have source "%s" - skipping\n' % source)
>> @@ -347,30 +365,39 @@
>>           ui.readconfig(cfg, sections=['acl.groups', 'acl.allow.branches',
>>               'acl.deny.branches', 'acl.allow', 'acl.deny'])
>>
>> -    allowbranches = buildmatch(ui, None, user, 'acl.allow.branches')
>> -    denybranches = buildmatch(ui, None, user, 'acl.deny.branches')
>> +    allowbranches = buildmatch(ui, repo, user, 'acl.allow.branches')
>> +    denybranches = buildmatch(ui, repo, user, 'acl.deny.branches')
>>       allow = buildmatch(ui, repo, user, 'acl.allow')
>>       deny = buildmatch(ui, repo, user, 'acl.deny')
>>
>> -    for rev in xrange(repo[node], len(repo)):
>> -        ctx = repo[rev]
>> -        branch = ctx.branch()
>> -        if denybranches and denybranches(branch):
>> -            raise error.Abort(_('acl: user "%s" denied on branch "%s"'
>> +    def checkbranches(user, ctx):
>> +        if denybranches and denybranches(ctx):
>> +            raise error.Abort(_('acl: user "%s" denied'
>>                                  ' (changeset "%s")')
>> -                               % (user, branch, ctx))
>> -        if allowbranches and not allowbranches(branch):
>> -            raise error.Abort(_('acl: user "%s" not allowed on branch "%s"'
>> +                               % (user, ctx))
>> +        if allowbranches and not allowbranches(ctx):
>> +            raise error.Abort(_('acl: user "%s" not allowed'
>>                                  ' (changeset "%s")')
>> -                               % (user, branch, ctx))
>> -        ui.debug('acl: branch access granted: "%s" on branch "%s"\n'
>> -        % (ctx, branch))
>> +                               % (user, ctx))
>>
>> -        for f in ctx.files():
>> -            if deny and deny(f):
>> -                raise error.Abort(_('acl: user "%s" denied on "%s"'
>> -                ' (changeset "%s")') % (user, f, ctx))
>> -            if allow and not allow(f):
>> -                raise error.Abort(_('acl: user "%s" not allowed on "%s"'
>> -                ' (changeset "%s")') % (user, f, ctx))
>> -        ui.debug('acl: path access granted: "%s"\n' % ctx)
>> +    def checkctx(user, ctx):
>> +        if deny and deny(ctx):
>> +            raise error.Abort(_('acl: user "%s" denied'
>> +                ' (changeset "%s")') % (user, ctx))
>> +        if allow and not allow(ctx):
>> +            raise error.Abort(_('acl: user "%s" not allowed'
>> +                ' (changeset "%s")') % (user, ctx))
>> +    cwd = os.getcwd()
>> +    os.chdir(os.path.dirname(repo.path))
>> +    try:
>> +        for rev in xrange(repo[node], len(repo)):
>> +            ctx = repo[rev]
>> +            branch = ctx.branch()
>> +            checkbranches(user, ctx)
>> +            ui.debug('acl: branch access granted: "%s" on branch "%s"\n'
>> +            % (ctx, branch))
>> +
>> +            checkctx(user, ctx)
>> +            ui.debug('acl: path access granted: "%s"\n' % ctx)
>> +    finally:
>> +        os.chdir(cwd)
>> diff -r c5ad786b58c5 -r d4a9c04f6c50 tests/test-acl.t
>> --- a/tests/test-acl.t	Thu Mar 03 23:29:26 2016 +0000
>> +++ b/tests/test-acl.t	Wed May 04 01:58:19 2016 +0000
>> @@ -340,12 +340,12 @@
>>     acl: acl.allow enabled, 0 entries for user fred
>>     acl: acl.deny not enabled
>>     acl: branch access granted: "ef1ea85a6374" on branch "default"
>> -  error: pretxnchangegroup.acl hook failed: acl: user "fred" not allowed on "foo/file.txt" (changeset "ef1ea85a6374")
>> +  error: pretxnchangegroup.acl hook failed: acl: user "fred" not allowed (changeset "ef1ea85a6374")
>>     bundle2-input-part: total payload size 1606
>>     bundle2-input-bundle: 3 parts total
>>     transaction abort!
>>     rollback completed
>> -  abort: acl: user "fred" not allowed on "foo/file.txt" (changeset "ef1ea85a6374")
>> +  abort: acl: user "fred" not allowed (changeset "ef1ea85a6374")
>>     no rollback information available
>>     0:6675d58eff77
>>
>> @@ -410,12 +410,12 @@
>>     acl: branch access granted: "f9cafe1212c8" on branch "default"
>>     acl: path access granted: "f9cafe1212c8"
>>     acl: branch access granted: "911600dab2ae" on branch "default"
>> -  error: pretxnchangegroup.acl hook failed: acl: user "fred" not allowed on "quux/file.py" (changeset "911600dab2ae")
>> +  error: pretxnchangegroup.acl hook failed: acl: user "fred" not allowed (changeset "911600dab2ae")
>>     bundle2-input-part: total payload size 1606
>>     bundle2-input-bundle: 3 parts total
>>     transaction abort!
>>     rollback completed
>> -  abort: acl: user "fred" not allowed on "quux/file.py" (changeset "911600dab2ae")
>> +  abort: acl: user "fred" not allowed (changeset "911600dab2ae")
>>     no rollback information available
>>     0:6675d58eff77
>>
>> @@ -477,12 +477,12 @@
>>     acl: acl.allow enabled, 0 entries for user barney
>>     acl: acl.deny enabled, 0 entries for user barney
>>     acl: branch access granted: "ef1ea85a6374" on branch "default"
>> -  error: pretxnchangegroup.acl hook failed: acl: user "barney" not allowed on "foo/file.txt" (changeset "ef1ea85a6374")
>> +  error: pretxnchangegroup.acl hook failed: acl: user "barney" not allowed (changeset "ef1ea85a6374")
>>     bundle2-input-part: total payload size 1606
>>     bundle2-input-bundle: 3 parts total
>>     transaction abort!
>>     rollback completed
>> -  abort: acl: user "barney" not allowed on "foo/file.txt" (changeset "ef1ea85a6374")
>> +  abort: acl: user "barney" not allowed (changeset "ef1ea85a6374")
>>     no rollback information available
>>     0:6675d58eff77
>>
>> @@ -549,12 +549,12 @@
>>     acl: branch access granted: "f9cafe1212c8" on branch "default"
>>     acl: path access granted: "f9cafe1212c8"
>>     acl: branch access granted: "911600dab2ae" on branch "default"
>> -  error: pretxnchangegroup.acl hook failed: acl: user "fred" not allowed on "quux/file.py" (changeset "911600dab2ae")
>> +  error: pretxnchangegroup.acl hook failed: acl: user "fred" not allowed (changeset "911600dab2ae")
>>     bundle2-input-part: total payload size 1606
>>     bundle2-input-bundle: 3 parts total
>>     transaction abort!
>>     rollback completed
>> -  abort: acl: user "fred" not allowed on "quux/file.py" (changeset "911600dab2ae")
>> +  abort: acl: user "fred" not allowed (changeset "911600dab2ae")
>>     no rollback information available
>>     0:6675d58eff77
>>
>> @@ -620,12 +620,12 @@
>>     acl: branch access granted: "ef1ea85a6374" on branch "default"
>>     acl: path access granted: "ef1ea85a6374"
>>     acl: branch access granted: "f9cafe1212c8" on branch "default"
>> -  error: pretxnchangegroup.acl hook failed: acl: user "fred" denied on "foo/Bar/file.txt" (changeset "f9cafe1212c8")
>> +  error: pretxnchangegroup.acl hook failed: acl: user "fred" denied (changeset "f9cafe1212c8")
>>     bundle2-input-part: total payload size 1606
>>     bundle2-input-bundle: 3 parts total
>>     transaction abort!
>>     rollback completed
>> -  abort: acl: user "fred" denied on "foo/Bar/file.txt" (changeset "f9cafe1212c8")
>> +  abort: acl: user "fred" denied (changeset "f9cafe1212c8")
>>     no rollback information available
>>     0:6675d58eff77
>>
>> @@ -688,12 +688,12 @@
>>     acl: acl.allow enabled, 0 entries for user barney
>>     acl: acl.deny enabled, 0 entries for user barney
>>     acl: branch access granted: "ef1ea85a6374" on branch "default"
>> -  error: pretxnchangegroup.acl hook failed: acl: user "barney" not allowed on "foo/file.txt" (changeset "ef1ea85a6374")
>> +  error: pretxnchangegroup.acl hook failed: acl: user "barney" not allowed (changeset "ef1ea85a6374")
>>     bundle2-input-part: total payload size 1606
>>     bundle2-input-bundle: 3 parts total
>>     transaction abort!
>>     rollback completed
>> -  abort: acl: user "barney" not allowed on "foo/file.txt" (changeset "ef1ea85a6374")
>> +  abort: acl: user "barney" not allowed (changeset "ef1ea85a6374")
>>     no rollback information available
>>     0:6675d58eff77
>>
>> @@ -754,8 +754,8 @@
>>     acl: acl.allow enabled, 1 entries for user fred
>>     acl: acl.deny enabled, 2 entries for user fred
>>     acl: branch access granted: "ef1ea85a6374" on branch "default"
>> +  invalid branchheads cache (served): tip differs
>>     acl: path access granted: "ef1ea85a6374"
>> -  invalid branchheads cache (served): tip differs
>>     bundle2-input-part: total payload size 537
>>     bundle2-input-part: "pushkey" (params: 4 mandatory) supported
>>     calling hook prepushkey.acl: hgext.acl.hook
>> @@ -841,8 +841,8 @@
>>     acl: acl.allow enabled, 1 entries for user fred
>>     acl: acl.deny enabled, 2 entries for user fred
>>     acl: branch access granted: "ef1ea85a6374" on branch "default"
>> +  invalid branchheads cache (served): tip differs
>>     acl: path access granted: "ef1ea85a6374"
>> -  invalid branchheads cache (served): tip differs
>>     bundle2-input-part: total payload size 537
>>     bundle2-input-part: "pushkey" (params: 4 mandatory) supported
>>     calling hook prepushkey.acl: hgext.acl.hook
>> @@ -1020,12 +1020,12 @@
>>     acl: branch access granted: "f9cafe1212c8" on branch "default"
>>     acl: path access granted: "f9cafe1212c8"
>>     acl: branch access granted: "911600dab2ae" on branch "default"
>> -  error: pretxnchangegroup.acl hook failed: acl: user "wilma" not allowed on "quux/file.py" (changeset "911600dab2ae")
>> +  error: pretxnchangegroup.acl hook failed: acl: user "wilma" not allowed (changeset "911600dab2ae")
>>     bundle2-input-part: total payload size 1606
>>     bundle2-input-bundle: 3 parts total
>>     transaction abort!
>>     rollback completed
>> -  abort: acl: user "wilma" not allowed on "quux/file.py" (changeset "911600dab2ae")
>> +  abort: acl: user "wilma" not allowed (changeset "911600dab2ae")
>>     no rollback information available
>>     0:6675d58eff77
>>
>> @@ -1173,12 +1173,12 @@
>>     acl: branch access granted: "f9cafe1212c8" on branch "default"
>>     acl: path access granted: "f9cafe1212c8"
>>     acl: branch access granted: "911600dab2ae" on branch "default"
>> -  error: pretxnchangegroup.acl hook failed: acl: user "betty" not allowed on "quux/file.py" (changeset "911600dab2ae")
>> +  error: pretxnchangegroup.acl hook failed: acl: user "betty" not allowed (changeset "911600dab2ae")
>>     bundle2-input-part: total payload size 1606
>>     bundle2-input-bundle: 3 parts total
>>     transaction abort!
>>     rollback completed
>> -  abort: acl: user "betty" not allowed on "quux/file.py" (changeset "911600dab2ae")
>> +  abort: acl: user "betty" not allowed (changeset "911600dab2ae")
>>     no rollback information available
>>     0:6675d58eff77
>>
>> @@ -1430,12 +1430,12 @@
>>     acl: branch access granted: "ef1ea85a6374" on branch "default"
>>     acl: path access granted: "ef1ea85a6374"
>>     acl: branch access granted: "f9cafe1212c8" on branch "default"
>> -  error: pretxnchangegroup.acl hook failed: acl: user "fred" denied on "foo/Bar/file.txt" (changeset "f9cafe1212c8")
>> +  error: pretxnchangegroup.acl hook failed: acl: user "fred" denied (changeset "f9cafe1212c8")
>>     bundle2-input-part: total payload size 1606
>>     bundle2-input-bundle: 3 parts total
>>     transaction abort!
>>     rollback completed
>> -  abort: acl: user "fred" denied on "foo/Bar/file.txt" (changeset "f9cafe1212c8")
>> +  abort: acl: user "fred" denied (changeset "f9cafe1212c8")
>>     no rollback information available
>>     0:6675d58eff77
>>
>> @@ -1595,12 +1595,12 @@
>>     acl: branch access granted: "ef1ea85a6374" on branch "default"
>>     acl: path access granted: "ef1ea85a6374"
>>     acl: branch access granted: "f9cafe1212c8" on branch "default"
>> -  error: pretxnchangegroup.acl hook failed: acl: user "fred" denied on "foo/Bar/file.txt" (changeset "f9cafe1212c8")
>> +  error: pretxnchangegroup.acl hook failed: acl: user "fred" denied (changeset "f9cafe1212c8")
>>     bundle2-input-part: total payload size 1606
>>     bundle2-input-bundle: 3 parts total
>>     transaction abort!
>>     rollback completed
>> -  abort: acl: user "fred" denied on "foo/Bar/file.txt" (changeset "f9cafe1212c8")
>> +  abort: acl: user "fred" denied (changeset "f9cafe1212c8")
>>     no rollback information available
>>     0:6675d58eff77
>>
>> @@ -1808,12 +1808,12 @@
>>     acl: path access granted: "f9cafe1212c8"
>>     acl: branch access granted: "911600dab2ae" on branch "default"
>>     acl: path access granted: "911600dab2ae"
>> -  error: pretxnchangegroup.acl hook failed: acl: user "astro" denied on branch "foobar" (changeset "e8fc755d4d82")
>> +  error: pretxnchangegroup.acl hook failed: acl: user "astro" denied (changeset "e8fc755d4d82")
>>     bundle2-input-part: total payload size 2101
>>     bundle2-input-bundle: 4 parts total
>>     transaction abort!
>>     rollback completed
>> -  abort: acl: user "astro" denied on branch "foobar" (changeset "e8fc755d4d82")
>> +  abort: acl: user "astro" denied (changeset "e8fc755d4d82")
>>     no rollback information available
>>     2:fb35475503ef
>>
>> @@ -1840,6 +1840,7 @@
>>     listing keys for "phases"
>>     checking for updated bookmarks
>>     listing keys for "bookmarks"
>> +  invalid branchheads cache (served): tip differs
>>     listing keys for "bookmarks"
>>     4 changesets found
>>     list of changesets:
>> @@ -1877,12 +1878,12 @@
>>     acl: acl.deny.branches not enabled
>>     acl: acl.allow not enabled
>>     acl: acl.deny not enabled
>> -  error: pretxnchangegroup.acl hook failed: acl: user "astro" not allowed on branch "default" (changeset "ef1ea85a6374")
>> +  error: pretxnchangegroup.acl hook failed: acl: user "astro" not allowed (changeset "ef1ea85a6374")
>>     bundle2-input-part: total payload size 2101
>>     bundle2-input-bundle: 4 parts total
>>     transaction abort!
>>     rollback completed
>> -  abort: acl: user "astro" not allowed on branch "default" (changeset "ef1ea85a6374")
>> +  abort: acl: user "astro" not allowed (changeset "ef1ea85a6374")
>>     no rollback information available
>>     2:fb35475503ef
>>
>> @@ -1948,12 +1949,12 @@
>>     acl: acl.deny.branches not enabled
>>     acl: acl.allow not enabled
>>     acl: acl.deny not enabled
>> -  error: pretxnchangegroup.acl hook failed: acl: user "astro" not allowed on branch "default" (changeset "ef1ea85a6374")
>> +  error: pretxnchangegroup.acl hook failed: acl: user "astro" not allowed (changeset "ef1ea85a6374")
>>     bundle2-input-part: total payload size 2101
>>     bundle2-input-bundle: 4 parts total
>>     transaction abort!
>>     rollback completed
>> -  abort: acl: user "astro" not allowed on branch "default" (changeset "ef1ea85a6374")
>> +  abort: acl: user "astro" not allowed (changeset "ef1ea85a6374")
>>     no rollback information available
>>     2:fb35475503ef
>>
>> @@ -2208,12 +2209,12 @@
>>     acl: acl.deny.branches enabled, 1 entries for user george
>>     acl: acl.allow not enabled
>>     acl: acl.deny not enabled
>> -  error: pretxnchangegroup.acl hook failed: acl: user "george" denied on branch "default" (changeset "ef1ea85a6374")
>> +  error: pretxnchangegroup.acl hook failed: acl: user "george" denied (changeset "ef1ea85a6374")
>>     bundle2-input-part: total payload size 2101
>>     bundle2-input-bundle: 4 parts total
>>     transaction abort!
>>     rollback completed
>> -  abort: acl: user "george" denied on branch "default" (changeset "ef1ea85a6374")
>> +  abort: acl: user "george" denied (changeset "ef1ea85a6374")
>>     no rollback information available
>>     2:fb35475503ef
>>
>> @@ -2241,6 +2242,7 @@
>>     listing keys for "phases"
>>     checking for updated bookmarks
>>     listing keys for "bookmarks"
>> +  invalid branchheads cache (served): tip differs
>>     listing keys for "bookmarks"
>>     4 changesets found
>>     list of changesets:
>> @@ -2369,12 +2371,12 @@
>>     acl: acl.deny.branches enabled, 1 entries for user george
>>     acl: acl.allow not enabled
>>     acl: acl.deny not enabled
>> -  error: pretxnchangegroup.acl hook failed: acl: user "george" denied on branch "default" (changeset "ef1ea85a6374")
>> +  error: pretxnchangegroup.acl hook failed: acl: user "george" denied (changeset "ef1ea85a6374")
>>     bundle2-input-part: total payload size 2101
>>     bundle2-input-bundle: 4 parts total
>>     transaction abort!
>>     rollback completed
>> -  abort: acl: user "george" denied on branch "default" (changeset "ef1ea85a6374")
>> +  abort: acl: user "george" denied (changeset "ef1ea85a6374")
>>     no rollback information available
>>     2:fb35475503ef
>>
>> _______________________________________________
>> Mercurial-devel mailing list
>> Mercurial-devel@mercurial-scm.org
>> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel

> On May 11, 2016, at 6:12 AM, Pierre-Yves David <pierre-yves.david@ens-lyon.org> wrote:
> 
>>> Afaict, it does not help w/ bookmarks at all,
>>> as bookmarks aren't applied until after the hook runs.
>>> And I don't think one can roll back a bookmark by that point,
>>> as I think the previous information would be lost.
>> Hm, you might well be right. Sounds like maybe we should consider both
>> patches? Being able to use revsets here seems about right to me feel-wise.
> 
> This series creates the following though flow on my side:
> 
> 1) Why do user need to setup hook by hand that seems like something acl should do by itself

I don’t know what this means.

> 2) Bookmark handling would be better/safer at the transaction level, but we don't have good hook data at that point

I think the problem is not the transaction-level business for bookmarks, but instead the fact that old clients send bookmarks in a separate transaction.

> 3) If we improve hooks used, user will have to adjust config to take advantage of it (because of (1))
> 
> This create a tickling temptation to rework acl to register hooks and improve transaction to track bookmark event before moving forward here.

I understand that temptation, but I think that’s out of scope for what this series is trying to accomplish (and not entirely feasible anyway with old clients potentially in play.)

On 05/11/2016 02:23 PM, Augie Fackler wrote:
>> On May 11, 2016, at 6:12 AM, Pierre-Yves David <pierre-yves.david@ens-lyon.org> wrote:
>>
>>>> Afaict, it does not help w/ bookmarks at all,
>>>> as bookmarks aren't applied until after the hook runs.
>>>> And I don't think one can roll back a bookmark by that point,
>>>> as I think the previous information would be lost.
>>> Hm, you might well be right. Sounds like maybe we should consider both
>>> patches? Being able to use revsets here seems about right to me feel-wise.
>> This series creates the following though flow on my side:
>>
>> 1) Why do user need to setup hook by hand that seems like something acl should do by itself
> I don’t know what this means.

If you look at patch 1 you can see that in addition to adding the acl 
extension, the config need to be updated:

    $ echo 'pretxnchangegroup.acl = python:hgext.acl.hook' >> $config
    $ echo 'prepushkey.acl = python:hgext.acl.hook' >> $config


>> 2) Bookmark handling would be better/safer at the transaction level, but we don't have good hook data at that point
> I think the problem is not the transaction-level business for bookmarks, but instead the fact that old clients send bookmarks in a separate transaction.

No, the problem is that we cannot implement that hook at the pretnxclose 
level, because the data of which hook is not available here. This is an 
issue that need to be tackled at some point anyway. But doing the a 
pretnxclose hook is the right point to handle acl request as this is 
when we'll be able to have a view of the full operation.

The fact old client use a different transaction is independant and 
"unsolvable" but have no effect on the approach used here.

>
>> 3) If we improve hooks used, user will have to adjust config to take advantage of it (because of (1))
>>
>> This create a tickling temptation to rework acl to register hooks and improve transaction to track bookmark event before moving forward here.
> I understand that temptation, but I think that’s out of scope for what this series is trying to accomplish (and not entirely feasible anyway with old clients potentially in play.)

But I'm not super eager to move forward on this bookmark things with 
such a brittle implementation, especially, fixes to issue5165 
<https://bz.mercurial-scm.org/show_bug.cgi?id=5165> might break the 
current approach.

as a remindier, changing the hooks used will not have any change on the 
challenge.

> An unfortunate side-effect of this change is that I've lost
> the reporting indicating why a commit was rejected.
>
> It's possible to re-engineer things so that such reporting could be
> done.

Pierre-Yves David wrote:
> We can probably have a config to report a [specific] message for a rule, don't
> we?

Even if it wasn't merging rules (and unrolling rules and running them
individually isn't a big deal), w/o some effort, you still lose
information.

The old rule could tell you which file in a commit violated a rule.
Unless someone writes a revset for each file individually, and unless
we have it reporting the revset rule item, it's unclear to me how well
a user will be able to understand the error.

worse, if the server is using custom revsets, it's possible that
reporting a revset to the user won't enable the user to run the revset
to get an answer. At the very least, the server would need to consider
spitting out a table of all transcluded revset aliases.

But the bigger part of "understanding a revset match"... really isn't
something that ACL should worry about. It's more something that
log/revset/something should offer to handle. If a user presents a
revset to hg, it should be possible for hg to explain why the revset
applies to a rev.

In log form, that could mean highlighting the matching parts (e.g.
colorizing the file()s that match and the desc() that matches,
possibly indicating ancestors that are connected or something). This
is well beyond what could possibly be considered by ACL, and isn't
something I'm up for doing.