[PATCH 3 of 7 V4] revset: insert _unordered to fix ordering of nested 'or' and '_list*()' (BC)

[Yuya Nishihara]

On Tue, 2 Aug 2016 09:37:49 -0700, Martin von Zweigbergk wrote:
> It seems we agree how it should work from the user's point of view, but
> let's confirm with a few examples:
> 
> 1. heads()
> 2. 1+2+0
> 3. sort(1+2+0)
> 4. reverse(heads()) & sort(heads())
> 
> 1) will be in ascending order. 2) will be in the given order (1 2 0). 3)
> will be in ascending order. 4) will be in descending order

Exactly.

> Still from the user's point of view, it's the ascending "native order" of
> heads() that shows up in 1) and 4). If you replace heads() by 5::10, you'd
> get that predicate's order, and its reverse, of course.

I have slightly different view. When I use heads(), I don't care the order.
heads() (and most other predicates) are neutral for ordering, and default to
the natural order of the revset, which is ascending.

> I think we also agree that the subsets passed to the revset predicates are
> just there as an optimization. It would have been simpler to do that
> intersection outside the revset predicates, but putting it inside gives the
> predicate the opportunity to consider only a subset of history when
> producing it's own set (before intersecting subset with it). If we had not
> been concerned about performance, sort(1+2+0) would not have received a
> subset as input and we would simply not have intersected it with
> fullreposet. Then fullreposet.__and__() would not have been special, and it
> would not have to ignore its own order as it currently does.

I agree the subset argument is implementation detail for optimization.

I don't get fullreposet.__and__() thing. IMHO, currently it does nothing
special. It complies to the abstractsmartset interface, which is __and__()
is ordered by self, not by other.

> However, we do care about performance, and passing in the subset argument
> has been a fruitful way of dealing with it. Unfortunately, blindly
> intersecting with the subset removes order (as intersection should), so
> some revsets that need a particular order avoid intersecting with the
> subset. Those who care less about order (who want the usual ascending
> order) do intersect with the subset and rely on the odd trick in
> fullreposet.__and__ to sort its argument.

Maybe we could handle the order separately if we don't care about performance,
something like: (unordered) set operations + a dominant ordering expression.

> So, I wonder if the order argument you defined should be passed to all
> revset predicates, just like the subset is. I saw you did something like
> that just for rangeset(), but treating rangeset() differently seems wrong
> to me. If the goal is to improve BC for extensions, we can probably do that
> in a different way (e.g. in the decorator). I really don't know if this is
> the right solution. Hopefully you have more good ideas.

I thought about that before. I tend to agree with you in that treating some
functions differently is bad, but I do that way because of two reasons:

 a. most functions don't need "order" flag. I don't want to introduce new
    trick because of a few order-sensitive functions.
 b. "order" flag seems not easy to understand, extension authors would be
    confused about how to handle it.