Patchwork [3,of,5] registrar: define revsetpredicate to decorate revset predicate

login
register
mail settings
Submitter Katsunori FUJIWARA
Date Jan. 5, 2016, 11:48 a.m.
Message ID <c6554052f10db3a9312b.1451994516@feefifofum>
Download mbox | patch
Permalink /patch/12528/
State Superseded
Commit ac11ba7c2e5606ac0c26ad13c8e4626743775777
Delegated to: Yuya Nishihara
Headers show

Comments

Katsunori FUJIWARA - Jan. 5, 2016, 11:48 a.m.
# HG changeset patch
# User FUJIWARA Katsunori <foozy@lares.dti.ne.jp>
# Date 1451994203 -32400
#      Tue Jan 05 20:43:23 2016 +0900
# Node ID c6554052f10db3a9312b17c658bf8c579198a5d2
# Parent  de07b46cc5de1baa86d40492941fd56c849311c3
registrar: define revsetpredicate to decorate revset predicate

'revsetpredicate' is used to replace 'revset.predicate' and
'revset.extpredicate' in subsequent patches.

This patch also adds 'loadpredicate()' to revset, because this
combination helps to figure out how the name of safe predicate is put
into 'safesymbols'.

This patch still uses 'safesymbols' set to examine whether the
predicate corresponded to the 'name' is safe from DoS attack or not,
because just setting 'func._safe' property needs changes below for
such examination.

  before:
      name in revset.safesymbols

  after:
      getattr(revset.symbols.get(name, None), '_safe', False)
Yuya Nishihara - Jan. 7, 2016, 3:01 p.m.
On Tue, 05 Jan 2016 20:48:36 +0900, FUJIWARA Katsunori wrote:
> # HG changeset patch
> # User FUJIWARA Katsunori <foozy@lares.dti.ne.jp>
> # Date 1451994203 -32400
> #      Tue Jan 05 20:43:23 2016 +0900
> # Node ID c6554052f10db3a9312b17c658bf8c579198a5d2
> # Parent  de07b46cc5de1baa86d40492941fd56c849311c3
> registrar: define revsetpredicate to decorate revset predicate
> 
> 'revsetpredicate' is used to replace 'revset.predicate' and
> 'revset.extpredicate' in subsequent patches.
> 
> This patch also adds 'loadpredicate()' to revset, because this
> combination helps to figure out how the name of safe predicate is put
> into 'safesymbols'.
> 
> This patch still uses 'safesymbols' set to examine whether the
> predicate corresponded to the 'name' is safe from DoS attack or not,
> because just setting 'func._safe' property needs changes below for
> such examination.
> 
>   before:
>       name in revset.safesymbols
> 
>   after:
>       getattr(revset.symbols.get(name, None), '_safe', False)
> 
> diff --git a/mercurial/registrar.py b/mercurial/registrar.py
> --- a/mercurial/registrar.py
> +++ b/mercurial/registrar.py
> @@ -208,3 +208,30 @@ class funcregistrarbase(object):
>          """Execute exra setup for registered function, if needed
>          """
>          pass
> +
> +class revsetpredicate(funcregistrarbase):
> +    """Decorator to register revset predicate
> +
> +    Usage::
> +
> +        revsetpredicate = registrar.revsetpredicate()
> +
> +        @revsetpredicate('mypredicate(arg1, arg2[, arg3])')
> +        def mypredicatefunc(repo, subset, x):
> +            '''Explanation of this revset predicate ....
> +            '''
> +            pass
> +
> +    The first string argument is used also in online help.
> +
> +    Optional argument 'safe' indicates whether a predicate is safe for
> +    DoS attack (False by default).
> +
> +    'revsetpredicate' instance in example above can be used to
> +    decorate multiple functions.
> +    """
> +    getname = funcregistrarbase.parsefuncdecl
> +    formatdoc = "``%s``\n    %s"
> +
> +    def extrasetup(self, name, func, safe=False):
> +        func._safe = safe
> diff --git a/mercurial/revset.py b/mercurial/revset.py
> --- a/mercurial/revset.py
> +++ b/mercurial/revset.py
> @@ -3667,5 +3667,16 @@ def prettyformatset(revs):
>          p = q
>      return '\n'.join('  ' * l + s for l, s in lines)
>  
> +def loadpredicate(ui, extname, predicateobj):
> +    """Load revset predicates from specified predicateobj
> +    """
> +    if not isinstance(predicateobj, registrar.revsetpredicate):
> +        # extension may not know about recent 'revsetpredicate' naming rule
> +        return

This looks like a temporary workaround, but the next patch doesn't remove it.

> +    for name, func in predicateobj.table.iteritems():
> +        symbols[name] = func
> +        if func._safe:
> +            safesymbols.add(name)
> +
>  # tell hggettext to extract docstrings from these functions:
>  i18nfunctions = symbols.values()
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@selenic.com
> https://selenic.com/mailman/listinfo/mercurial-devel
Katsunori FUJIWARA - Jan. 9, 2016, 10:41 a.m.
At Fri, 8 Jan 2016 00:01:57 +0900,
Yuya Nishihara wrote:
> 
> On Tue, 05 Jan 2016 20:48:36 +0900, FUJIWARA Katsunori wrote:
> > # HG changeset patch
> > # User FUJIWARA Katsunori <foozy@lares.dti.ne.jp>
> > # Date 1451994203 -32400
> > #      Tue Jan 05 20:43:23 2016 +0900
> > # Node ID c6554052f10db3a9312b17c658bf8c579198a5d2
> > # Parent  de07b46cc5de1baa86d40492941fd56c849311c3
> > registrar: define revsetpredicate to decorate revset predicate
> > 
> > 'revsetpredicate' is used to replace 'revset.predicate' and
> > 'revset.extpredicate' in subsequent patches.
> > 
> > This patch also adds 'loadpredicate()' to revset, because this
> > combination helps to figure out how the name of safe predicate is put
> > into 'safesymbols'.
> > 
> > This patch still uses 'safesymbols' set to examine whether the
> > predicate corresponded to the 'name' is safe from DoS attack or not,
> > because just setting 'func._safe' property needs changes below for
> > such examination.
> > 
> >   before:
> >       name in revset.safesymbols
> > 
> >   after:
> >       getattr(revset.symbols.get(name, None), '_safe', False)
> > 
> > diff --git a/mercurial/registrar.py b/mercurial/registrar.py
> > --- a/mercurial/registrar.py
> > +++ b/mercurial/registrar.py
> > @@ -208,3 +208,30 @@ class funcregistrarbase(object):
> >          """Execute exra setup for registered function, if needed
> >          """
> >          pass
> > +
> > +class revsetpredicate(funcregistrarbase):
> > +    """Decorator to register revset predicate
> > +
> > +    Usage::
> > +
> > +        revsetpredicate = registrar.revsetpredicate()
> > +
> > +        @revsetpredicate('mypredicate(arg1, arg2[, arg3])')
> > +        def mypredicatefunc(repo, subset, x):
> > +            '''Explanation of this revset predicate ....
> > +            '''
> > +            pass
> > +
> > +    The first string argument is used also in online help.
> > +
> > +    Optional argument 'safe' indicates whether a predicate is safe for
> > +    DoS attack (False by default).
> > +
> > +    'revsetpredicate' instance in example above can be used to
> > +    decorate multiple functions.
> > +    """
> > +    getname = funcregistrarbase.parsefuncdecl
> > +    formatdoc = "``%s``\n    %s"
> > +
> > +    def extrasetup(self, name, func, safe=False):
> > +        func._safe = safe
> > diff --git a/mercurial/revset.py b/mercurial/revset.py
> > --- a/mercurial/revset.py
> > +++ b/mercurial/revset.py
> > @@ -3667,5 +3667,16 @@ def prettyformatset(revs):
> >          p = q
> >      return '\n'.join('  ' * l + s for l, s in lines)
> >  
> > +def loadpredicate(ui, extname, predicateobj):
> > +    """Load revset predicates from specified predicateobj
> > +    """
> > +    if not isinstance(predicateobj, registrar.revsetpredicate):
> > +        # extension may not know about recent 'revsetpredicate' naming rule
> > +        return
> 
> This looks like a temporary workaround, but the next patch doesn't remove it.

No, this is a guard for (existing) 3rd party extension, which
accidentally defines 'revsetpredicate' symbol in it for another
purpose.

If this comment causes confusion, how about "3rd party extension may
define this for another purpose" or so ?


> > +    for name, func in predicateobj.table.iteritems():
> > +        symbols[name] = func
> > +        if func._safe:
> > +            safesymbols.add(name)
> > +
> >  # tell hggettext to extract docstrings from these functions:
> >  i18nfunctions = symbols.values()
> > _______________________________________________
> > Mercurial-devel mailing list
> > Mercurial-devel@selenic.com
> > https://selenic.com/mailman/listinfo/mercurial-devel
> 

----------------------------------------------------------------------
[FUJIWARA Katsunori]                             foozy@lares.dti.ne.jp
Yuya Nishihara - Jan. 9, 2016, 11:32 a.m.
On Sat, 09 Jan 2016 19:41:37 +0900, FUJIWARA Katsunori wrote:
> At Fri, 8 Jan 2016 00:01:57 +0900,
> Yuya Nishihara wrote:
> > On Tue, 05 Jan 2016 20:48:36 +0900, FUJIWARA Katsunori wrote:
> > > +def loadpredicate(ui, extname, predicateobj):
> > > +    """Load revset predicates from specified predicateobj
> > > +    """
> > > +    if not isinstance(predicateobj, registrar.revsetpredicate):
> > > +        # extension may not know about recent 'revsetpredicate' naming rule
> > > +        return
> > 
> > This looks like a temporary workaround, but the next patch doesn't remove it.
> 
> No, this is a guard for (existing) 3rd party extension, which
> accidentally defines 'revsetpredicate' symbol in it for another
> purpose.
> 
> If this comment causes confusion, how about "3rd party extension may
> define this for another purpose" or so ?

In that case, it's better to have some warning. Silent return is likely to
shadow bugs.

But I don't think it is necessary. We haven't care when we added 'testedwith',
'buglink' or 'minimumhgversion'.

Patch

diff --git a/mercurial/registrar.py b/mercurial/registrar.py
--- a/mercurial/registrar.py
+++ b/mercurial/registrar.py
@@ -208,3 +208,30 @@  class funcregistrarbase(object):
         """Execute exra setup for registered function, if needed
         """
         pass
+
+class revsetpredicate(funcregistrarbase):
+    """Decorator to register revset predicate
+
+    Usage::
+
+        revsetpredicate = registrar.revsetpredicate()
+
+        @revsetpredicate('mypredicate(arg1, arg2[, arg3])')
+        def mypredicatefunc(repo, subset, x):
+            '''Explanation of this revset predicate ....
+            '''
+            pass
+
+    The first string argument is used also in online help.
+
+    Optional argument 'safe' indicates whether a predicate is safe for
+    DoS attack (False by default).
+
+    'revsetpredicate' instance in example above can be used to
+    decorate multiple functions.
+    """
+    getname = funcregistrarbase.parsefuncdecl
+    formatdoc = "``%s``\n    %s"
+
+    def extrasetup(self, name, func, safe=False):
+        func._safe = safe
diff --git a/mercurial/revset.py b/mercurial/revset.py
--- a/mercurial/revset.py
+++ b/mercurial/revset.py
@@ -3667,5 +3667,16 @@  def prettyformatset(revs):
         p = q
     return '\n'.join('  ' * l + s for l, s in lines)
 
+def loadpredicate(ui, extname, predicateobj):
+    """Load revset predicates from specified predicateobj
+    """
+    if not isinstance(predicateobj, registrar.revsetpredicate):
+        # extension may not know about recent 'revsetpredicate' naming rule
+        return
+    for name, func in predicateobj.table.iteritems():
+        symbols[name] = func
+        if func._safe:
+            safesymbols.add(name)
+
 # tell hggettext to extract docstrings from these functions:
 i18nfunctions = symbols.values()