[PATCH 3 of 3 V5] revset: skip legacy lookup for revspec wrapped in 'revset(…)'

[Yuya Nishihara]

On Mon, 16 Apr 2018 19:49:43 +0200, Feld Boris wrote:
> On 16/04/2018 14:24, Yuya Nishihara wrote:
> > On Mon, 16 Apr 2018 08:58:31 +0200, Boris Feld wrote:
> >> # HG changeset patch
> >> # User Boris Feld <boris.feld at octobus.net>
> >> # Date 1523369212 -7200
> >> #      Tue Apr 10 16:06:52 2018 +0200
> >> # Node ID 109ca88347d7b531d4b48370efcbc4d6e850cf92
> >> # Parent  f363552ced37ae028bbcf2cba1f02ac623385f54
> >> # EXP-Topic noname
> >> # Available At https://bitbucket.org/octobus/mercurial-devel/
> >> #              hg pull https://bitbucket.org/octobus/mercurial-devel/ -r 109ca88347d7
> >> revset: skip legacy lookup for revspec wrapped in 'revset(…)'
> >> @@ -2191,7 +2194,12 @@ def matchany(ui, specs, lookup=None, loc
> >>           raise error.ParseError(_("empty query"))
> >>       parsedspecs = []
> >>       for s in specs:
> >> -        parsedspecs.append(revsetlang.parse(s, lookup))
> >> +        lookupthis = lookup
> >> +        stripped = s.strip()
> >> +        if (stripped.startswith(prefixrevset)
> >> +                and stripped.endswith(postfixrevset)):
> >> +            lookupthis = None
> >> +        parsedspecs.append(revsetlang.parse(s, lookupthis))
> > Is it okay to move this hack to revsetlang._parsewith?
> >
> > @@ -482,6 +485,8 @@ def _parsewith(spec, lookup=None, symini
> >         ...
> >       ParseError: ('invalid token', 4)
> >       """
> > +    if spec.startswith('revset(') and spec.endswith(')'):
> > +        lookup = None
> >       p = parser.parser(elements)
> >       tree, pos = p.parse(tokenize(spec, lookup=lookup,
> >                                    syminitletters=syminitletters))
> >
> > I don't think revset.match*() is the right place to do parsing stuff, and
> > we'll need a tokenizer to make it more correctly handle variants such as
> > ' revset ( ... )' or '... and revset(...)'.
> You're are right, moving it lower in the stack makes sense. Would it be 
> possible to implement it even lower in revsetlang.tokenize?

Maybe no. We'll need the tokenize() function to handle e.g. 'revset ( ... )'.

  if firsttwotokens(tokenize(s)) == ['revset', '(']:
      return tokenize(s)
  # otherwise, wrap lookup() to count parens and conditionally disable lookup?

> We tried preparing a V6 to move the code but we didn't find the queued 
> version of the first changeset. Were you waiting for us to move the code 
> yourself?

I've already fixed it locally, so will push soon.