Patchwork [2,of,3] extensions: register functions always at loading extension (issue5601)

login
register
mail settings
Submitter Katsunori FUJIWARA
Date June 23, 2017, 5:48 p.m.
Message ID <bd02107d5a20adff17f7.1498240136@speaknoevil>
Download mbox | patch
Permalink /patch/21634/
State Accepted
Headers show

Comments

Katsunori FUJIWARA - June 23, 2017, 5:48 p.m.
# HG changeset patch
# User FUJIWARA Katsunori <foozy@lares.dti.ne.jp>
# Date 1498239560 -32400
#      Sat Jun 24 02:39:20 2017 +0900
# Node ID bd02107d5a20adff17f744a422b2530284215a7a
# Parent  5808466886a165460b52de226ab5d3c4c1379692
extensions: register functions always at loading extension (issue5601)

Before this patch, functions defined in extensions are registered via
extra loaders only in _dispatch(). Therefore, loading extensions in
other code paths like below omits registration of functions.

  - WSGI service
  - operation across repositories (e.g. subrepo)
  - test-duplicateoptions.py, using extensions.loadall() directly

To register functions always at loading new extension, this patch
moves implementation for extra loading from dispatch._dispatch() to
extensions.loadall().

AFAIK, only commands module causes cyclic dependency between
extensions module, but this patch imports all related modules just
before extra loading in loadall(), in order to centralize them.

This patch makes extensions.py depend on many other modules, even
though extensions.py itself doesn't. It should be avoided if possible,
but I don't have any better idea. Some other places like below aren't
reasonable for extra loading, IMHO.

  - specific function in newly added module:
    existing callers of extensions.loadall() should invoke it, too

  - hg.repository() or so:
    no-repo commands aren't covered by this.

BTW, this patch removes _loaded.add(name) on relocation, because
dispatch._loaded is used only for extraloaders (for similar reason,
"exts" variable is removed, too).
Yuya Nishihara - June 24, 2017, 3:10 a.m.
On Sat, 24 Jun 2017 02:48:56 +0900, FUJIWARA Katsunori wrote:
> # HG changeset patch
> # User FUJIWARA Katsunori <foozy@lares.dti.ne.jp>
> # Date 1498239560 -32400
> #      Sat Jun 24 02:39:20 2017 +0900
> # Node ID bd02107d5a20adff17f744a422b2530284215a7a
> # Parent  5808466886a165460b52de226ab5d3c4c1379692
> extensions: register functions always at loading extension (issue5601)

This makes me feel a bit awkward, but I couldn't come up with a better idea.
So I'm going to queue this in a few days.

> To register functions always at loading new extension, this patch
> moves implementation for extra loading from dispatch._dispatch() to
> extensions.loadall().
> 
> AFAIK, only commands module causes cyclic dependency between
> extensions module, but this patch imports all related modules just
> before extra loading in loadall(), in order to centralize them.

For "commands -> extensions.extensions() -> commands.loadcmdtable",
maybe we could split loader part from extensions.py. For example,

  extensions.loadall(ui, extraloaders)

But still we'll have to resolve "localrepo -> extensions.loadall()" cycle.
Perhaps, localrepository._loadextensions() shouldn't touch the global tables
because the repo may be a peer repository (and the extensions for the main
repo should be loaded beforehand.) However, hgweb appears not loading
extensions explicitly, which is probably a bug.
Matt Harbison - June 26, 2017, 12:24 a.m.
On Fri, 23 Jun 2017 13:48:56 -0400, FUJIWARA Katsunori  
<foozy@lares.dti.ne.jp> wrote:

> # HG changeset patch
> # User FUJIWARA Katsunori <foozy@lares.dti.ne.jp>
> # Date 1498239560 -32400
> #      Sat Jun 24 02:39:20 2017 +0900
> # Node ID bd02107d5a20adff17f744a422b2530284215a7a
> # Parent  5808466886a165460b52de226ab5d3c4c1379692
> extensions: register functions always at loading extension (issue5601)

...

> @@ -118,6 +129,14 @@ Check hgweb's load order:
>    4) foo reposetup
>    4) bar reposetup
> +(check that revset predicate foo() and bar() are available)
> +
> +  $ REQUEST_METHOD='GET' PATH_INFO='/shortlog' SCRIPT_NAME='' \
> +  >     QUERY_STRING='rev=foo() and bar()' \
> +  >     SERVER_PORT='80' SERVER_NAME='localhost' python hgweb.cgi \
> +  >     | grep '<a href="/rev/[0-9a-z]*">'
> +     <a href="/rev/c24b9ac61126">add file</a>
> +

This grep is failing on Windows, because a 400 page is being generated.   
The content is:

   Status: 400 no such method: C:\r

So it's almost certainly more MSYS path mangling, and not serious.  I  
can't figure out why the very similar looking query above it works, and  
you're probably more familiar than I am with this, so can you take a look  
when you get a chance?  Thanks.

>    $ echo 'foo = !' >> $HGRCPATH
>    $ echo 'bar = !' >> $HGRCPATH
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Katsunori FUJIWARA - June 26, 2017, 8:21 a.m.
At Sun, 25 Jun 2017 20:24:47 -0400,
Matt Harbison wrote:
> 
> On Fri, 23 Jun 2017 13:48:56 -0400, FUJIWARA Katsunori  
> <foozy@lares.dti.ne.jp> wrote:
> 
> > # HG changeset patch
> > # User FUJIWARA Katsunori <foozy@lares.dti.ne.jp>
> > # Date 1498239560 -32400
> > #      Sat Jun 24 02:39:20 2017 +0900
> > # Node ID bd02107d5a20adff17f744a422b2530284215a7a
> > # Parent  5808466886a165460b52de226ab5d3c4c1379692
> > extensions: register functions always at loading extension (issue5601)
> 
> ...
> 
> > @@ -118,6 +129,14 @@ Check hgweb's load order:
> >    4) foo reposetup
> >    4) bar reposetup
> > +(check that revset predicate foo() and bar() are available)
> > +
> > +  $ REQUEST_METHOD='GET' PATH_INFO='/shortlog' SCRIPT_NAME='' \
> > +  >     QUERY_STRING='rev=foo() and bar()' \
> > +  >     SERVER_PORT='80' SERVER_NAME='localhost' python hgweb.cgi \
> > +  >     | grep '<a href="/rev/[0-9a-z]*">'
> > +     <a href="/rev/c24b9ac61126">add file</a>
> > +
> 
> This grep is failing on Windows, because a 400 page is being generated.   
> The content is:
> 
>    Status: 400 no such method: C:\r
> 
> So it's almost certainly more MSYS path mangling, and not serious.  I  
> can't figure out why the very similar looking query above it works, and  
> you're probably more familiar than I am with this, so can you take a look  
> when you get a chance?  Thanks.

Ah, thanks for good catching!

I overlooked that other wsgicgi/CGI tests are guarded like as below:

  - omit testing by "#if no-msys"/"#require no-msys"

    - test-clone-cgi.t
    - test-hgweb-commands.t
    - test-newcgi.t
    - test-newercgi.t
    - test-oldcgi.t
    - test-push-cgi.t

  - branch at PATH_INFO setup by "#if msys/#else/#endif"

    - test-mq.t

  - MSYS seems to omit translation for PATH_INFO='/'

    - test-largefiles.t
    - (previous) test-extension.t

I'll post followup patch.

> >    $ echo 'foo = !' >> $HGRCPATH
> >    $ echo 'bar = !' >> $HGRCPATH
> > _______________________________________________
> > Mercurial-devel mailing list
> > Mercurial-devel@mercurial-scm.org
> > https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
>

Patch

diff --git a/mercurial/dispatch.py b/mercurial/dispatch.py
--- a/mercurial/dispatch.py
+++ b/mercurial/dispatch.py
@@ -30,17 +30,12 @@  from . import (
     error,
     extensions,
     fancyopts,
-    fileset,
     help,
     hg,
     hook,
     profiling,
     pycompat,
-    revset,
     scmutil,
-    templatefilters,
-    templatekw,
-    templater,
     ui as uimod,
     util,
 )
@@ -727,22 +722,6 @@  def _checkshellalias(lui, ui, args):
 
 _loaded = set()
 
-# list of (objname, loadermod, loadername) tuple:
-# - objname is the name of an object in extension module, from which
-#   extra information is loaded
-# - loadermod is the module where loader is placed
-# - loadername is the name of the function, which takes (ui, extensionname,
-#   extraobj) arguments
-extraloaders = [
-    ('cmdtable', commands, 'loadcmdtable'),
-    ('colortable', color, 'loadcolortable'),
-    ('filesetpredicate', fileset, 'loadpredicate'),
-    ('revsetpredicate', revset, 'loadpredicate'),
-    ('templatefilter', templatefilters, 'loadfilter'),
-    ('templatefunc', templater, 'loadfunction'),
-    ('templatekeyword', templatekw, 'loadkeyword'),
-]
-
 def _dispatch(req):
     args = req.args
     ui = req.ui
@@ -770,19 +749,11 @@  def _dispatch(req):
         # reposetup. Programs like TortoiseHg will call _dispatch several
         # times so we keep track of configured extensions in _loaded.
         extensions.loadall(lui)
-        exts = [ext for ext in extensions.extensions() if ext[0] not in _loaded]
         # Propagate any changes to lui.__class__ by extensions
         ui.__class__ = lui.__class__
 
         # (uisetup and extsetup are handled in extensions.loadall)
 
-        for name, module in exts:
-            for objname, loadermod, loadername in extraloaders:
-                extraobj = getattr(module, objname, None)
-                if extraobj is not None:
-                    getattr(loadermod, loadername)(ui, name, extraobj)
-            _loaded.add(name)
-
         # (reposetup is handled in hg.repository)
 
         addaliases(lui, commands.table)
diff --git a/mercurial/extensions.py b/mercurial/extensions.py
--- a/mercurial/extensions.py
+++ b/mercurial/extensions.py
@@ -243,6 +243,43 @@  def loadall(ui, whitelist=None):
     # entries could result in double execution. See issue4646.
     _aftercallbacks.clear()
 
+    # delay importing avoids cyclic dependency (especially commands)
+    from . import (
+        color,
+        commands,
+        fileset,
+        revset,
+        templatefilters,
+        templatekw,
+        templater,
+    )
+
+    # list of (objname, loadermod, loadername) tuple:
+    # - objname is the name of an object in extension module,
+    #   from which extra information is loaded
+    # - loadermod is the module where loader is placed
+    # - loadername is the name of the function,
+    #   which takes (ui, extensionname, extraobj) arguments
+    extraloaders = [
+        ('cmdtable', commands, 'loadcmdtable'),
+        ('colortable', color, 'loadcolortable'),
+        ('filesetpredicate', fileset, 'loadpredicate'),
+        ('revsetpredicate', revset, 'loadpredicate'),
+        ('templatefilter', templatefilters, 'loadfilter'),
+        ('templatefunc', templater, 'loadfunction'),
+        ('templatekeyword', templatekw, 'loadkeyword'),
+    ]
+
+    for name in _order[newindex:]:
+        module = _extensions[name]
+        if not module:
+            continue # loading this module failed
+
+        for objname, loadermod, loadername in extraloaders:
+            extraobj = getattr(module, objname, None)
+            if extraobj is not None:
+                getattr(loadermod, loadername)(ui, name, extraobj)
+
 def afterloaded(extension, callback):
     '''Run the specified function after a named extension is loaded.
 
diff --git a/tests/test-extension.t b/tests/test-extension.t
--- a/tests/test-extension.t
+++ b/tests/test-extension.t
@@ -77,15 +77,25 @@  Check that extensions are loaded in phas
   >     print "3) %s extsetup" % name
   > def reposetup(ui, repo):
   >    print "4) %s reposetup" % name
+  > 
+  > # custom predicate to check registration of functions at loading
+  > from mercurial import (
+  >     registrar,
+  >     smartset,
+  > )
+  > revsetpredicate = registrar.revsetpredicate()
+  > @revsetpredicate(name, safe=True) # safe=True for query via hgweb
+  > def custompredicate(repo, subset, x):
+  >     return smartset.baseset([r for r in subset if r in {0}])
   > EOF
 
   $ cp foo.py bar.py
   $ echo 'foo = foo.py' >> $HGRCPATH
   $ echo 'bar = bar.py' >> $HGRCPATH
 
-Command with no output, we just want to see the extensions loaded:
-
-  $ hg paths
+Check normal command's load order of extensions and registration of functions
+
+  $ hg log -r "foo() and bar()" -q
   1) foo imported
   1) bar imported
   2) foo uisetup
@@ -94,8 +104,9 @@  Command with no output, we just want to 
   3) bar extsetup
   4) foo reposetup
   4) bar reposetup
-
-Check hgweb's load order:
+  0:c24b9ac61126
+
+Check hgweb's load order of extensions and registration of functions
 
   $ cat > hgweb.cgi <<EOF
   > #!$PYTHON
@@ -118,6 +129,14 @@  Check hgweb's load order:
   4) foo reposetup
   4) bar reposetup
 
+(check that revset predicate foo() and bar() are available)
+
+  $ REQUEST_METHOD='GET' PATH_INFO='/shortlog' SCRIPT_NAME='' \
+  >     QUERY_STRING='rev=foo() and bar()' \
+  >     SERVER_PORT='80' SERVER_NAME='localhost' python hgweb.cgi \
+  >     | grep '<a href="/rev/[0-9a-z]*">'
+     <a href="/rev/c24b9ac61126">add file</a>
+
   $ echo 'foo = !' >> $HGRCPATH
   $ echo 'bar = !' >> $HGRCPATH