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.f...@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. > >> +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? Also, I'm not sure what's the value of using alias replacement here. Can you detail your reasoning? The alias logic seems pretty tied to having string (that it parses) as replacement, that is why I used a dedicated object. > > raise ProgrammingError if t is not None nor 'baseset'. > >> + return (b''.join(ret), inputs) > 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? Overall. I feel like you have a design adjustment in mind, but I can't figure out the big picture. > _______________________________________________ > Mercurial-devel mailing list > Mercurial-devel@mercurial-scm.org > https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel _______________________________________________ Mercurial-devel mailing list Mercurial-devel@mercurial-scm.org https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
