Improving %ld substiution in `repo.revs`

[Yuya Nishihara]

On Thu, 20 Dec 2018 10:27:32 +0100, Boris FELD wrote:
> On 17/12/2018 14:36, Yuya Nishihara wrote:
> > On Mon, 17 Dec 2018 12:38:17 +0000, Boris FELD wrote:
> >> It is possible to passvarious substitution when passing revset to
> >> `repo.revs` (eg: repo.revs("heads(%ld)", [4, 5, 6]). 
> >>
> >> The current implementation handle this in a quick and dirty way. First a
> >> new revset string is build:
> >>
> >>
> >>  * code: repo.revs("heads(%ld)", [4, 5, 6])
> >>  * transformed into (repo.revs("heads(_intlist(4, 5, 6))")
> >>  * then revset parser build a syntax tree
> >>  * _intlist turns its argument into a baseset
> >>
> >> This is clever and simple, it works well for a small number of revision.
> >> However, when the set of revision growth, this becomes ineffective. For
> >> example, discovery might call revset where %ld is replace by hundred of
> >> thousands of revisions. The cost of serializing and serializing them
> >> to/from string become significant.
> > To avoid formatting/parsing overheads, we could directly build a parse tree
> > and feed it to revset.makematcher(). It will be somewhat similar to alias
> > expansion.
> 
> The current API is very easy to use and I would rather keep it that way.
> Having developer choose between API simplicity and speed seems sub-optimal.
> 
> What does your proposal look like in practice API wise?

repo.revs("heads(%ld)", [4, 5, 6])
  rewrite "heads(%ld)" as e.g. "heads($0)" so that it can be parsed as alias
  tree = parse("heads($0)")
  replace the placeholder arguments with e.g. ('intlist', 4, 5, 6)
  analyze, optimize, etc. the tree
  return revset.makematcher(tree)

> > Unlike templater, revset has no context variable. So I had to hack the repo
> > instance in the example above, which isn't nice for your use case.
> Could we have this done "automatically" with repo.revs? where %ld turned
> into special local revsets?

If we want to support the % syntax, it's probably better to embed the
replacements in the parsed tree. The whole point of using an internal function
is that we don't have to touch the parsing stages.

> > This wouldn't be simple if we want to implement it cleanly. Maybe we can
> > embed a smartset in a parse tree, as say (smartset <smartset>) node, but
> > a smartset is mutable and it can be sorted/reordered while evaluating a
> > revset. That's probably okay in many cases, but can lead to surprising
> > results.
> Ha, good point, maybe we need some sort of light smartset copy.

Or making smartsets truly immutable by replacing e.g. sort() with sorted().