Patchwork [3,of,5] revset: drop redundant check for unknown alias arguments

login
register
mail settings
Submitter Yuya Nishihara
Date March 29, 2016, 3:27 p.m.
Message ID <e89acb348d532903e002.1459265268@mimosa>
Download mbox | patch
Permalink /patch/14141/
State Accepted
Headers show

Comments

Yuya Nishihara - March 29, 2016, 3:27 p.m.
# HG changeset patch
# User Yuya Nishihara <yuya@tcha.org>
# Date 1455449228 -32400
#      Sun Feb 14 20:27:08 2016 +0900
# Node ID e89acb348d532903e002bf834609472f9a50bdfb
# Parent  fa7880cac50272f805ae1a233a477b0a9f56a585
revset: drop redundant check for unknown alias arguments

Since _parsealiasdefn() rejects unknown alias arguments, _checkaliasarg() is
unnecessary. New test is added to make sure unknown '$n' symbols are rejected.
Pierre-Yves David - March 30, 2016, 12:36 a.m.
On 03/29/2016 08:27 AM, Yuya Nishihara wrote:
> # HG changeset patch
> # User Yuya Nishihara <yuya@tcha.org>
> # Date 1455449228 -32400
> #      Sun Feb 14 20:27:08 2016 +0900
> # Node ID e89acb348d532903e002bf834609472f9a50bdfb
> # Parent  fa7880cac50272f805ae1a233a477b0a9f56a585
> revset: drop redundant check for unknown alias arguments
>
> Since _parsealiasdefn() rejects unknown alias arguments, _checkaliasarg() is
> unnecessary. New test is added to make sure unknown '$n' symbols are rejected.
>
> diff --git a/mercurial/revset.py b/mercurial/revset.py
> --- a/mercurial/revset.py
> +++ b/mercurial/revset.py
> @@ -2267,17 +2267,6 @@ def _getaliasarg(tree):
>           return tree[1]
>       return None
>
> -def _checkaliasarg(tree, known=None):
> -    """Check tree contains no _aliasarg construct or only ones which
> -    value is in known. Used to avoid alias placeholders injection.
> -    """
> -    if isinstance(tree, tuple):
> -        arg = _getaliasarg(tree)
> -        if arg is not None and (not known or arg not in known):
> -            raise error.UnknownIdentifier('_aliasarg', [])
> -        for t in tree:
> -            _checkaliasarg(t, known)
> -
>   # the set of valid characters for the initial letter of symbols in
>   # alias declarations and definitions
>   _aliassyminitletters = set(c for c in [chr(i) for i in xrange(256)]
> @@ -2443,8 +2432,6 @@ class revsetalias(object):
>
>           try:
>               self.replacement = _parsealiasdefn(value, self.args)
> -            # Check for placeholder injection
> -            _checkaliasarg(self.replacement, self.args)
>           except error.ParseError as inst:
>               self.error = _('failed to parse the definition of revset alias'
>                              ' "%s": %s') % (self.name, parseerrordetail(inst))
> diff --git a/tests/test-revset.t b/tests/test-revset.t
> --- a/tests/test-revset.t
> +++ b/tests/test-revset.t
> @@ -1820,6 +1820,15 @@ but 'all()' should never be substituded
>       <spanset+ 0:9>>
>     0
>
> +test unknown reference:
> +
> +  $ try "unknownref(0)" --config 'revsetalias.unknownref($1)=$1:$2'
> +  (func
> +    ('symbol', 'unknownref')
> +    ('symbol', '0'))
> +  abort: failed to parse the definition of revset alias "unknownref": '$' not for alias arguments
> +  [255]

This abort is complaining about something without naming it. Could we 
get updated to: '$' not for alias arguments "$2"

This patches seems to just show the issue (by adding a test, great!), 
not actually introducing it so I've taken the series, can you follow up 
on this ?

Cheers,
Yuya Nishihara - March 30, 2016, 2:16 p.m.
On Tue, 29 Mar 2016 17:36:37 -0700, Pierre-Yves David wrote:
> On 03/29/2016 08:27 AM, Yuya Nishihara wrote:
> > +  $ try "unknownref(0)" --config 'revsetalias.unknownref($1)=$1:$2'
> > +  (func
> > +    ('symbol', 'unknownref')
> > +    ('symbol', '0'))
> > +  abort: failed to parse the definition of revset alias "unknownref": '$' not for alias arguments
> > +  [255]  
> 
> This abort is complaining about something without naming it. Could we
> get updated to: '$' not for alias arguments "$2"

Ah, good point. Also this message seems a bit long. I'll revisit it later as
I'm sloppy to resolve conflicts that would be introduced by the message change.

Patch

diff --git a/mercurial/revset.py b/mercurial/revset.py
--- a/mercurial/revset.py
+++ b/mercurial/revset.py
@@ -2267,17 +2267,6 @@  def _getaliasarg(tree):
         return tree[1]
     return None
 
-def _checkaliasarg(tree, known=None):
-    """Check tree contains no _aliasarg construct or only ones which
-    value is in known. Used to avoid alias placeholders injection.
-    """
-    if isinstance(tree, tuple):
-        arg = _getaliasarg(tree)
-        if arg is not None and (not known or arg not in known):
-            raise error.UnknownIdentifier('_aliasarg', [])
-        for t in tree:
-            _checkaliasarg(t, known)
-
 # the set of valid characters for the initial letter of symbols in
 # alias declarations and definitions
 _aliassyminitletters = set(c for c in [chr(i) for i in xrange(256)]
@@ -2443,8 +2432,6 @@  class revsetalias(object):
 
         try:
             self.replacement = _parsealiasdefn(value, self.args)
-            # Check for placeholder injection
-            _checkaliasarg(self.replacement, self.args)
         except error.ParseError as inst:
             self.error = _('failed to parse the definition of revset alias'
                            ' "%s": %s') % (self.name, parseerrordetail(inst))
diff --git a/tests/test-revset.t b/tests/test-revset.t
--- a/tests/test-revset.t
+++ b/tests/test-revset.t
@@ -1820,6 +1820,15 @@  but 'all()' should never be substituded 
     <spanset+ 0:9>>
   0
 
+test unknown reference:
+
+  $ try "unknownref(0)" --config 'revsetalias.unknownref($1)=$1:$2'
+  (func
+    ('symbol', 'unknownref')
+    ('symbol', '0'))
+  abort: failed to parse the definition of revset alias "unknownref": '$' not for alias arguments
+  [255]
+
   $ hg debugrevspec --debug --config revsetalias.anotherbadone='branch(' "tip"
   ('symbol', 'tip')
   warning: failed to parse the definition of revset alias "anotherbadone": at 7: not a prefix: end