[PATCH 2 of 6] graphlog: turn getlogrevs() into a generator

[Patrick Mézard]

Le 23/05/12 21:44, Matt Mackall a écrit :
> On Wed, 2012-05-23 at 09:44 +0200, Patrick Mézard wrote:
>> Le 22/05/12 21:37, Matt Mackall a écrit :
>>> On Tue, 2012-05-08 at 22:58 +0200, Patrick Mezard wrote:
>>>> # HG changeset patch
>>>> # User Patrick Mezard <patrick at mezard.eu>
>>>> # Date 1336509824 -7200
>>>> # Node ID 53db775e252c0f4f7add32b62e08096814d3ab7e
>>>> # Parent  d30440da7b38c671266ae1734db3921475a63caa
>>>> graphlog: turn getlogrevs() into a generator
>>>
>>> Queued these first two for default, thanks. Still mulling the rest, I'm
>>> afraid.
>>
>> By the way, I am not really attached to the precompute thing. This is
>> just the simplest/less invasive trick I found to "solve" the issue at
>> hand. The fundamental issues are:
>> - There is no execution context provided to revset functions, which makes caching and rewriting harder to implement.
>> - Being made of tuples, it is pretty hard to annotate the AST.
> 
> Yeah, I think we'll need to do something here. But I think I'd rather
> start down the path of implementing an incremental revset framework and
> see where that gets us. It will almost certainly need a caching scheme
> but it probably won't look much like this one.

Revisiting this topic I noticed you actually pushed this patch:

  graphlog: turn getlogrevs() into a generator
  http://selenic.com/hg/rev/058e14da7044

Does it mean you agree with turning revset.match() output into a stateful filtering function which could be called several times rather than into a generator?

I said that:
"""
I feel that calling the matcher multiple times is correct, at least with
the revsets involved in getlogrevs() because they are:
- stateless (no limit())
- respecting f(a|b) = f(a) | f(b), though I have no valid argument about
  that.
"""
but stateful functions can easily be accomodated by:
- making the stateful (sic), which they were already, being closures
- a clearer protocol to initiate and terminate the filtering process.

Assuming revset.getmatcher() returns such a filtering function, it would be used like:

m = revset.getmatcher(expr)
m.reset()  # implicit, could be useful to reuse costly matcher against new revision sequences
for revs in iterrevranges():
    filtered = m(revs)
    ... do something with filtered ...
filtered = m.close()
... do something with filtered ...

The close() part was missing from my implementation but is necessary for revset expression which need to see the whole input set before filtering. The two reset() and close() methods should be supported by all revset functions and symbols and called recursively on the tree.

What do you think of this model? Did you have something really different in mind?

--
Patrick Mézard