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
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,
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