[PATCH 8 of 8] revset: introduce an API that avoids `formatspec` input serialization

[Yuya Nishihara]

On Sun, 13 Jan 2019 08:40:41 +0100, Boris FELD wrote:
> On 12/01/2019 06:00, Yuya Nishihara wrote:
> > On Fri, 11 Jan 2019 12:29:10 +0100, Boris Feld wrote:
> >> # HG changeset patch
> >> # User Boris Feld <boris.feld at octobus.net>
> >> # Date 1546605681 -3600
> >> #      Fri Jan 04 13:41:21 2019 +0100
> >> # Node ID 73926c4ab24d6c01723ed050601b134bdc89562f
> >> # Parent  4a56fbdacff33c3985bbb84f2e19ddfbd48ed4fa
> >> # EXP-Topic revs-efficiency
> >> # Available At https://bitbucket.org/octobus/mercurial-devel/
> >> #              hg pull https://bitbucket.org/octobus/mercurial-devel/ -r 73926c4ab24d
> >> revset: introduce an API that avoids `formatspec` input serialization
> > The idea looks good.
> >
> >> +class _inputrules(parser.basealiasrules):
> >> +    """replace internal input reference by their actual value"""
> >> +
> >> +    @classmethod
> >> +    def _getalias(cls, inputs, tree):
> >> +        if not isinstance(tree, tuple):
> >> +            return None
> >> +        if tree[0] != 'func':
> >> +            return None
> >> +        if getsymbol(tree[1]) != _internal_input:
> >> +            return None
> >> +        idx = int(getsymbol(tree[2]))
> >> +        newtree = ('func',
> >> +                   ('symbol', internal_input_func),
> >> +                   ('smartset', inputs[idx])
> >> +        )
> >> +        return parser.alias(idx, None, None, newtree), None
> > Perhaps, this can be just ('smartset', ...) node like 'symbol'/'string'.
> > There's not point to convert it to a 'func' unless it can be expressed
> > as a revset string.
> >
> > And if we can probably ditch the specialized _inputrules. See below.
> 
> We need to combine the smartset with the existing subset. Doing it
> within a function seems simpler and less impactful.

Well, there's no practical difference between ('func', ..., ('smartset',))
and ('smartset',).

For ('func', ..., ('smartset',)), we'll need two functions:
symbols[internal_input_func] and methods['smartset'].
symbols[internal_input_func] needs to check the argument type, and
methods['smartset'] would raise ParseError.

For ('smartset',), methods['smartset'] knows 'x' is a smartset, so we can
just return 'x & subset' / 'subset & x'. See stringset() for example.

> >> +def formatspecargs(expr, *args):
> >> +    """same as formatspec, but preserve some expensive arguments"""
> >> +    parsed = _parseargs(expr, args)
> >> +    ret = []
> >> +    inputs = []
> >> +    for t, arg in parsed:
> >> +        if t is None:
> >> +            ret.append(arg)
> >> +        if t == 'baseset':
> >> +            key = '%s(%d)' % (_internal_input, len(inputs))
> >> +            inputs.append(smartset.baseset(arg))
> >> +            ret.append(key)
> > Maybe we can assign integer (e.g. '$0') for each parameter here, and we can
> > just use the _aliasrules.expand() to replace it. '$x' is invalid in user
> > expression, so it can be used as a reserved symbol.
> 
> Don't we use '$0' for user alias?

Yes, but that doesn't matter since 'expr' here isn't an alias expression.

> Also, I'm not sure what's the value of using alias replacement here. Can
> you detail your reasoning?

To get rid of the custom _inputrule class.

> The alias logic seems pretty tied to having string (that it parses) as
> replacement, that is why I used a dedicated object.

So here we have string to parse, b''.join(ret), and replacements, inputs.
If b''.join(ret) contained '$0' instead of 'internal(0)', and if inputs
were dict of {'$0': alias('$0', ..., ('smartset', inputs[0]))}, we can just
feed them to _aliasrules.

  aliases = build_dict_of_inputs(inputs)
  tree = _parsewith(b''.join(ret), syminitletters=_aliassyminitletters)
  tree = _aliasrules.expand(aliases, tree)

That's the idea.

> > If this function returned a parsed tree, we can hide all the implementation
> > details in it, and we can probably remove the inputs argument from
> > revset.matchany().
> 
> But revset/matchany does not take a parsed tree, does it?

A parsed tree can be directly passed in to revset.makematcher(). Since
repo.revs() is an internal API, there aren't much we have to copy from
matchany():

  tree = foldconcat(tree)
  tree = analyze(tree)
  tree = optimize(tree)
  return tree