Patchwork [4,of,5,V2] revset: use delayregistrar to register predicate in extension easily

login
register
mail settings
Submitter Katsunori FUJIWARA
Date Dec. 29, 2015, 3:08 p.m.
Message ID <0aa9cf12fc6ce052b0f2.1451401731@feefifofum>
Download mbox | patch
Permalink /patch/12401/
State Accepted
Headers show

Comments

Katsunori FUJIWARA - Dec. 29, 2015, 3:08 p.m.
# HG changeset patch
# User FUJIWARA Katsunori <foozy@lares.dti.ne.jp>
# Date 1451401110 -32400
#      Tue Dec 29 23:58:30 2015 +0900
# Node ID 0aa9cf12fc6ce052b0f2e91674d1c56fe6078939
# Parent  a1e4c18851155866ab75a35aa5bbae81945d7db5
revset: use delayregistrar to register predicate in extension easily

Previous patch introduced 'revset.predicate' decorator to register
revset predicate function easily.

But it shouldn't be used in extension directly, because it registers
specified function immediately. Registration itself can't be restored,
even if extension loading fails after that.

Therefore, registration should be delayed until 'uisetup()' or so.

This patch uses 'extpredicate' decorator derived from 'delayregistrar'
to register predicate in extension easily.

This patch also tests whether 'registrar.delayregistrar' avoids
function registration if 'setup()' isn't invoked on it, because
'extpredicate' is the first user of it.
Yuya Nishihara - Jan. 1, 2016, 8:53 a.m.
On Wed, 30 Dec 2015 00:08:51 +0900, FUJIWARA Katsunori wrote:
> # HG changeset patch
> # User FUJIWARA Katsunori <foozy@lares.dti.ne.jp>
> # Date 1451401110 -32400
> #      Tue Dec 29 23:58:30 2015 +0900
> # Node ID 0aa9cf12fc6ce052b0f2e91674d1c56fe6078939
> # Parent  a1e4c18851155866ab75a35aa5bbae81945d7db5
> revset: use delayregistrar to register predicate in extension easily
> 
> Previous patch introduced 'revset.predicate' decorator to register
> revset predicate function easily.
> 
> But it shouldn't be used in extension directly, because it registers
> specified function immediately. Registration itself can't be restored,
> even if extension loading fails after that.
> 
> Therefore, registration should be delayed until 'uisetup()' or so.
> 
> This patch uses 'extpredicate' decorator derived from 'delayregistrar'
> to register predicate in extension easily.
> 
> This patch also tests whether 'registrar.delayregistrar' avoids
> function registration if 'setup()' isn't invoked on it, because
> 'extpredicate' is the first user of it.

> --- a/hgext/mq.py
> +++ b/hgext/mq.py
> @@ -3558,9 +3558,11 @@ def summaryhook(ui, repo):
>          # i18n: column positioning for "hg summary"
>          ui.note(_("mq:     (empty queue)\n"))
>  
> +revsetpredicate = revset.extpredicate()
> +
> +@revsetpredicate('mq()')
>  def revsetmq(repo, subset, x):
> -    """``mq()``
> -    Changesets managed by MQ.
> +    """Changesets managed by MQ.
>      """
>      revset.getargs(x, 0, 0, _("mq takes no arguments"))
>      applied = set([repo[r.node].rev() for r in repo.mq.applied])
> @@ -3596,7 +3598,7 @@ def extsetup(ui):
>          if extmodule.__file__ != __file__:
>              dotable(getattr(extmodule, 'cmdtable', {}))
>  
> -    revset.symbols['mq'] = revsetmq
> +    revsetpredicate.setup()

"cmdtable" was wrong?

I like consistent extension APIs, so my idea was something like this:

1. define table of revset symbols per extension

  # creates new table and associated decorator
  revsetpredicate = registrar.revsetpredicate()
  # or
  revsetsymbols = {}
  revsetpredicate = registrar.revsetpredicate(revsetsymbols)

  # revsetpredicate['mq'] = revsetmq
  @revsetpredicate('mq()')
  def revsetmq(...):
      ...

2. load them by dispatch._dispatch()
Katsunori FUJIWARA - Jan. 1, 2016, 4:28 p.m.
At Fri, 1 Jan 2016 17:53:10 +0900,
Yuya Nishihara wrote:
> 
> On Wed, 30 Dec 2015 00:08:51 +0900, FUJIWARA Katsunori wrote:
> > # HG changeset patch
> > # User FUJIWARA Katsunori <foozy@lares.dti.ne.jp>
> > # Date 1451401110 -32400
> > #      Tue Dec 29 23:58:30 2015 +0900
> > # Node ID 0aa9cf12fc6ce052b0f2e91674d1c56fe6078939
> > # Parent  a1e4c18851155866ab75a35aa5bbae81945d7db5
> > revset: use delayregistrar to register predicate in extension easily
> > 
> > Previous patch introduced 'revset.predicate' decorator to register
> > revset predicate function easily.
> > 
> > But it shouldn't be used in extension directly, because it registers
> > specified function immediately. Registration itself can't be restored,
> > even if extension loading fails after that.
> > 
> > Therefore, registration should be delayed until 'uisetup()' or so.
> > 
> > This patch uses 'extpredicate' decorator derived from 'delayregistrar'
> > to register predicate in extension easily.
> > 
> > This patch also tests whether 'registrar.delayregistrar' avoids
> > function registration if 'setup()' isn't invoked on it, because
> > 'extpredicate' is the first user of it.
> 
> > --- a/hgext/mq.py
> > +++ b/hgext/mq.py
> > @@ -3558,9 +3558,11 @@ def summaryhook(ui, repo):
> >          # i18n: column positioning for "hg summary"
> >          ui.note(_("mq:     (empty queue)\n"))
> >  
> > +revsetpredicate = revset.extpredicate()
> > +
> > +@revsetpredicate('mq()')
> >  def revsetmq(repo, subset, x):
> > -    """``mq()``
> > -    Changesets managed by MQ.
> > +    """Changesets managed by MQ.
> >      """
> >      revset.getargs(x, 0, 0, _("mq takes no arguments"))
> >      applied = set([repo[r.node].rev() for r in repo.mq.applied])
> > @@ -3596,7 +3598,7 @@ def extsetup(ui):
> >          if extmodule.__file__ != __file__:
> >              dotable(getattr(extmodule, 'cmdtable', {}))
> >  
> > -    revset.symbols['mq'] = revsetmq
> > +    revsetpredicate.setup()

Augie, could you drop this series from clowncopter if it is possible ?
It will make re-work easy.


> "cmdtable" was wrong?
> 
> I like consistent extension APIs, so my idea was something like this:
> 
> 1. define table of revset symbols per extension
> 
>   # creates new table and associated decorator
>   revsetpredicate = registrar.revsetpredicate()

In this case, loading logic looks inside 'revsetpredicate' object to
get predicate functions to be loaded, doesn't it ? (= table-ness of
bridging object isn't important)

>   # or
>   revsetsymbols = {}
>   revsetpredicate = registrar.revsetpredicate(revsetsymbols)

(BTW, would you also mean that revset decorator specific code should
be placed in registrar.py by sample code above ? I think that it
shouldn't so, IMHO)


>   # revsetpredicate['mq'] = revsetmq
>   @revsetpredicate('mq()')
>   def revsetmq(...):
>       ...
> 
> 2. load them by dispatch._dispatch()

What about registration steps below, if explicit setup() on
revsetpredicate isn't suitable for consistent API ? This can prevents
dispatch (and registrar) from depending on revset, template XXXX, and
so on.

  dispatch._dispatch()
  - extensions.loadall()
    - at loading extension
      - instantiate revset.predicate or so as 'revsetpredicate' (*1)
        - at loading revset.py
          - register revset specific hook (*2) to registrar.py
      - revset.predicate()
        - decorator stores information internally
    - uisetup of extension
    - extsetup of extension
  - invoke hooks registered in registrar.py on each extensions
    - invoke hook (*2)
      - load predicates from 'revsetpredicate' attribute (*1) of extension


----------------------------------------------------------------------
[FUJIWARA Katsunori]                             foozy@lares.dti.ne.jp
Katsunori FUJIWARA - Jan. 2, 2016, 5:30 a.m.
At Sat, 02 Jan 2016 01:28:28 +0900,
FUJIWARA Katsunori wrote:
> 
> At Fri, 1 Jan 2016 17:53:10 +0900,
> Yuya Nishihara wrote:
> > 
> > On Wed, 30 Dec 2015 00:08:51 +0900, FUJIWARA Katsunori wrote:
> > > # HG changeset patch
> > > # User FUJIWARA Katsunori <foozy@lares.dti.ne.jp>
> > > # Date 1451401110 -32400
> > > #      Tue Dec 29 23:58:30 2015 +0900
> > > # Node ID 0aa9cf12fc6ce052b0f2e91674d1c56fe6078939
> > > # Parent  a1e4c18851155866ab75a35aa5bbae81945d7db5
> > > revset: use delayregistrar to register predicate in extension easily
> > > 
> > > Previous patch introduced 'revset.predicate' decorator to register
> > > revset predicate function easily.
> > > 
> > > But it shouldn't be used in extension directly, because it registers
> > > specified function immediately. Registration itself can't be restored,
> > > even if extension loading fails after that.
> > > 
> > > Therefore, registration should be delayed until 'uisetup()' or so.
> > > 
> > > This patch uses 'extpredicate' decorator derived from 'delayregistrar'
> > > to register predicate in extension easily.
> > > 
> > > This patch also tests whether 'registrar.delayregistrar' avoids
> > > function registration if 'setup()' isn't invoked on it, because
> > > 'extpredicate' is the first user of it.
> > 
> > > --- a/hgext/mq.py
> > > +++ b/hgext/mq.py
> > > @@ -3558,9 +3558,11 @@ def summaryhook(ui, repo):
> > >          # i18n: column positioning for "hg summary"
> > >          ui.note(_("mq:     (empty queue)\n"))
> > >  
> > > +revsetpredicate = revset.extpredicate()
> > > +
> > > +@revsetpredicate('mq()')
> > >  def revsetmq(repo, subset, x):
> > > -    """``mq()``
> > > -    Changesets managed by MQ.
> > > +    """Changesets managed by MQ.
> > >      """
> > >      revset.getargs(x, 0, 0, _("mq takes no arguments"))
> > >      applied = set([repo[r.node].rev() for r in repo.mq.applied])
> > > @@ -3596,7 +3598,7 @@ def extsetup(ui):
> > >          if extmodule.__file__ != __file__:
> > >              dotable(getattr(extmodule, 'cmdtable', {}))
> > >  
> > > -    revset.symbols['mq'] = revsetmq
> > > +    revsetpredicate.setup()
> 
> Augie, could you drop this series from clowncopter if it is possible ?
> It will make re-work easy.

Oh, this series was already imported into master repo. I'll make
changes on recent tip, if needed :-)

----------------------------------------------------------------------
[FUJIWARA Katsunori]                             foozy@lares.dti.ne.jp
Yuya Nishihara - Jan. 2, 2016, 8:56 a.m.
On Sat, 02 Jan 2016 01:28:28 +0900, FUJIWARA Katsunori wrote:
> At Fri, 1 Jan 2016 17:53:10 +0900,
> Yuya Nishihara wrote:
> > "cmdtable" was wrong?
> > 
> > I like consistent extension APIs, so my idea was something like this:
> > 
> > 1. define table of revset symbols per extension
> > 
> >   # creates new table and associated decorator
> >   revsetpredicate = registrar.revsetpredicate()
> 
> In this case, loading logic looks inside 'revsetpredicate' object to
> get predicate functions to be loaded, doesn't it ? (= table-ness of
> bridging object isn't important)

Right. The difference is who controls the loading logic.

  (a) @command          mercurial.dispatch loads cmdtable
  (b) @revsetpredicate  hgext.xxx updates revset.symbols

We haven't (a) for revsets and templates, and your patch follows it,
which is straightforward. I just wondered if (a) is preferable as an extension
API.

> >   revsetpredicate = registrar.revsetpredicate(revsetsymbols)
> 
> (BTW, would you also mean that revset decorator specific code should
> be placed in registrar.py by sample code above ? I think that it
> shouldn't so, IMHO)

Yes. If the registrar provides all decorators, an extension can avoid
importing templatekw or templatefilters just because it defines new template
keywords/filters. (though the revset module would be necessary to define new
workable revset predicates.)

> > 2. load them by dispatch._dispatch()
> 
> What about registration steps below, if explicit setup() on
> revsetpredicate isn't suitable for consistent API ? This can prevents
> dispatch (and registrar) from depending on revset, template XXXX, and
> so on.
>
>   dispatch._dispatch()
>   - extensions.loadall()
>     - at loading extension
>       - instantiate revset.predicate or so as 'revsetpredicate' (*1)
>         - at loading revset.py
>           - register revset specific hook (*2) to registrar.py
>       - revset.predicate()
>         - decorator stores information internally
>     - uisetup of extension
>     - extsetup of extension
>   - invoke hooks registered in registrar.py on each extensions
>     - invoke hook (*2)
>       - load predicates from 'revsetpredicate' attribute (*1) of extension

registrar shouldn't import revset, but I think it's fine for dispatch to
import revset. And import-time hook registration won't be necessary.

Regards,
Katsunori FUJIWARA - Jan. 3, 2016, 10:56 a.m.
At Sat, 2 Jan 2016 17:56:59 +0900,
Yuya Nishihara wrote:
> 
> On Sat, 02 Jan 2016 01:28:28 +0900, FUJIWARA Katsunori wrote:
> > At Fri, 1 Jan 2016 17:53:10 +0900,
> > Yuya Nishihara wrote:
> > > "cmdtable" was wrong?
> > > 
> > > I like consistent extension APIs, so my idea was something like this:
> > > 
> > > 1. define table of revset symbols per extension
> > > 
> > >   # creates new table and associated decorator
> > >   revsetpredicate = registrar.revsetpredicate()
> > 
> > In this case, loading logic looks inside 'revsetpredicate' object to
> > get predicate functions to be loaded, doesn't it ? (= table-ness of
> > bridging object isn't important)
> 
> Right. The difference is who controls the loading logic.
> 
>   (a) @command          mercurial.dispatch loads cmdtable
>   (b) @revsetpredicate  hgext.xxx updates revset.symbols
> 
> We haven't (a) for revsets and templates, and your patch follows it,
> which is straightforward. I just wondered if (a) is preferable as an extension
> API.

Thank you for explanation.

> > >   revsetpredicate = registrar.revsetpredicate(revsetsymbols)
> > 
> > (BTW, would you also mean that revset decorator specific code should
> > be placed in registrar.py by sample code above ? I think that it
> > shouldn't so, IMHO)
> 
> Yes. If the registrar provides all decorators, an extension can avoid
> importing templatekw or templatefilters just because it defines new template
> keywords/filters. (though the revset module would be necessary to define new
> workable revset predicates.)

Yeah, I though that placing decorator in each corresponded module is
reasonable, because defining new workable function requires importing
corresponded module in many cases, to use utilities in it: revset,
fileset, some template keyword listing elements or so, for example.

But it is obvious that module importing should be avoided if possible,
as you described.


> > > 2. load them by dispatch._dispatch()
> > 
> > What about registration steps below, if explicit setup() on
> > revsetpredicate isn't suitable for consistent API ? This can prevents
> > dispatch (and registrar) from depending on revset, template XXXX, and
> > so on.
> >
> >   dispatch._dispatch()
> >   - extensions.loadall()
> >     - at loading extension
> >       - instantiate revset.predicate or so as 'revsetpredicate' (*1)
> >         - at loading revset.py
> >           - register revset specific hook (*2) to registrar.py
> >       - revset.predicate()
> >         - decorator stores information internally
> >     - uisetup of extension
> >     - extsetup of extension
> >   - invoke hooks registered in registrar.py on each extensions
> >     - invoke hook (*2)
> >       - load predicates from 'revsetpredicate' attribute (*1) of extension
> 
> registrar shouldn't import revset, but I think it's fine for dispatch to
> import revset. And import-time hook registration won't be necessary.

OK, I'll send another series to revise this series in such manner.

> Regards,
> 

----------------------------------------------------------------------
[FUJIWARA Katsunori]                             foozy@lares.dti.ne.jp

Patch

diff --git a/hgext/largefiles/overrides.py b/hgext/largefiles/overrides.py
--- a/hgext/largefiles/overrides.py
+++ b/hgext/largefiles/overrides.py
@@ -812,9 +812,11 @@  def overridepull(orig, ui, repo, source=
         ui.status(_("%d largefiles cached\n") % numcached)
     return result
 
+revsetpredicate = revset.extpredicate()
+
+@revsetpredicate('pulled()')
 def pulledrevsetsymbol(repo, subset, x):
-    """``pulled()``
-    Changesets that just has been pulled.
+    """Changesets that just has been pulled.
 
     Only available with largefiles from pull --lfrev expressions.
 
diff --git a/hgext/largefiles/uisetup.py b/hgext/largefiles/uisetup.py
--- a/hgext/largefiles/uisetup.py
+++ b/hgext/largefiles/uisetup.py
@@ -9,7 +9,7 @@ 
 '''setup for largefiles extension: uisetup'''
 
 from mercurial import archival, cmdutil, commands, extensions, filemerge, hg, \
-    httppeer, merge, scmutil, sshpeer, wireproto, revset, subrepo, copies
+    httppeer, merge, scmutil, sshpeer, wireproto, subrepo, copies
 from mercurial.i18n import _
 from mercurial.hgweb import hgweb_mod, webcommands
 
@@ -83,7 +83,6 @@  def uisetup(ui):
                ('', 'lfrev', [],
                 _('download largefiles for these revisions'), _('REV'))]
     entry[1].extend(pullopt)
-    revset.symbols['pulled'] = overrides.pulledrevsetsymbol
 
     entry = extensions.wrapcommand(commands.table, 'clone',
                                    overrides.overrideclone)
@@ -170,3 +169,5 @@  def uisetup(ui):
         if name == 'transplant':
             extensions.wrapcommand(getattr(module, 'cmdtable'), 'transplant',
                 overrides.overridetransplant)
+
+    overrides.revsetpredicate.setup()
diff --git a/hgext/mq.py b/hgext/mq.py
--- a/hgext/mq.py
+++ b/hgext/mq.py
@@ -3558,9 +3558,11 @@  def summaryhook(ui, repo):
         # i18n: column positioning for "hg summary"
         ui.note(_("mq:     (empty queue)\n"))
 
+revsetpredicate = revset.extpredicate()
+
+@revsetpredicate('mq()')
 def revsetmq(repo, subset, x):
-    """``mq()``
-    Changesets managed by MQ.
+    """Changesets managed by MQ.
     """
     revset.getargs(x, 0, 0, _("mq takes no arguments"))
     applied = set([repo[r.node].rev() for r in repo.mq.applied])
@@ -3596,7 +3598,7 @@  def extsetup(ui):
         if extmodule.__file__ != __file__:
             dotable(getattr(extmodule, 'cmdtable', {}))
 
-    revset.symbols['mq'] = revsetmq
+    revsetpredicate.setup()
 
 colortable = {'qguard.negative': 'red',
               'qguard.positive': 'yellow',
diff --git a/hgext/rebase.py b/hgext/rebase.py
--- a/hgext/rebase.py
+++ b/hgext/rebase.py
@@ -64,6 +64,9 @@  def _destrebase(repo):
     branch = repo[None].branch()
     return repo[branch].rev()
 
+revsetpredicate = revset.extpredicate()
+
+@revsetpredicate('_destrebase')
 def _revsetdestrebase(repo, subset, x):
     # ``_rebasedefaultdest()``
 
@@ -1231,4 +1234,4 @@  def uisetup(ui):
          _("use 'hg rebase --continue' or 'hg rebase --abort'")])
     # ensure rebased rev are not hidden
     extensions.wrapfunction(repoview, '_getdynamicblockers', _rebasedvisible)
-    revset.symbols['_destrebase'] = _revsetdestrebase
+    revsetpredicate.setup()
diff --git a/hgext/transplant.py b/hgext/transplant.py
--- a/hgext/transplant.py
+++ b/hgext/transplant.py
@@ -693,9 +693,11 @@  def _dotransplant(ui, repo, *revs, **opt
         if cleanupfn:
             cleanupfn()
 
+revsetpredicate = revset.extpredicate()
+
+@revsetpredicate('transplanted([set])')
 def revsettransplanted(repo, subset, x):
-    """``transplanted([set])``
-    Transplanted changesets in set, or all transplanted changesets.
+    """Transplanted changesets in set, or all transplanted changesets.
     """
     if x:
         s = revset.getset(repo, subset, x)
@@ -711,7 +713,7 @@  def kwtransplanted(repo, ctx, **args):
     return n and revlog.hex(n) or ''
 
 def extsetup(ui):
-    revset.symbols['transplanted'] = revsettransplanted
+    revsetpredicate.setup()
     templatekw.keywords['transplanted'] = kwtransplanted
     cmdutil.unfinishedstates.append(
         ['series', True, False, _('transplant in progress'),
diff --git a/mercurial/revset.py b/mercurial/revset.py
--- a/mercurial/revset.py
+++ b/mercurial/revset.py
@@ -487,11 +487,36 @@  class predicate(registrar.funcregistrar)
 
     The first string argument of the constructor is used also in
     online help.
+
+    Use 'extpredicate' instead of this to register revset predicate in
+    extensions.
     """
     table = symbols
     formatdoc = "``%s``\n    %s"
     getname = registrar.funcregistrar.parsefuncdecl
 
+class extpredicate(registrar.delayregistrar):
+    """Decorator to register revset predicate in extensions
+
+    Usage::
+
+        revsetpredicate = revset.extpredicate()
+
+        @revsetpredicate('mypredicate(arg1, arg2[, arg3])')
+        def mypredicatefunc(repo, subset, x):
+            '''Explanation of this revset predicate ....
+            '''
+            pass
+
+        def uisetup(ui):
+            revsetpredicate.setup()
+
+    'revsetpredicate' instance above can be used to decorate multiple
+    functions, and 'setup()' on it registers all such functions at
+    once.
+    """
+    registrar = predicate
+
 @predicate('_destupdate')
 def _destupdate(repo, subset, x):
     # experimental revset for update destination
diff --git a/tests/test-revset.t b/tests/test-revset.t
--- a/tests/test-revset.t
+++ b/tests/test-revset.t
@@ -2189,3 +2189,43 @@  test error message of bad revset
   [255]
 
   $ cd ..
+
+Test registrar.delayregistrar via revset.extpredicate
+
+'extpredicate' decorator shouldn't register any functions until
+'setup()' on it.
+
+  $ cd repo
+
+  $ cat <<EOF > $TESTTMP/custompredicate.py
+  > from mercurial import revset
+  > 
+  > revsetpredicate = revset.extpredicate()
+  > 
+  > @revsetpredicate('custom1()')
+  > def custom1(repo, subset, x):
+  >     return revset.baseset([1])
+  > @revsetpredicate('custom2()')
+  > def custom2(repo, subset, x):
+  >     return revset.baseset([2])
+  > 
+  > def uisetup(ui):
+  >     if ui.configbool('custompredicate', 'enabled'):
+  >         revsetpredicate.setup()
+  > EOF
+  $ cat <<EOF > .hg/hgrc
+  > [extensions]
+  > custompredicate = $TESTTMP/custompredicate.py
+  > EOF
+
+  $ hg debugrevspec "custom1()"
+  hg: parse error: unknown identifier: custom1
+  [255]
+  $ hg debugrevspec "custom2()"
+  hg: parse error: unknown identifier: custom2
+  [255]
+  $ hg debugrevspec "custom1() or custom2()" --config custompredicate.enabled=true
+  1
+  2
+
+  $ cd ..