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

[Martin von Zweigbergk]

On Wed, Aug 3, 2016 at 9:05 AM, Yuya Nishihara <yuya at tcha.org> wrote:
> 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.

How is the "neutral, but default to ascending" order that you think
they should have different from plain "ascending" that I think they
should have? Any observable difference?

>
>> 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.

It does something very special, IMO:
"other.sort(reverse=self.isdescending())". I think sorting the other
set is wrong. If I'm reading the comment in the function right ("XXX
As fullreposet is also used as bootstrap, this is wrong."),
Pierre-Yves (I assume, but I haven't checked) also thinks it's wrong
for it to sort the other set.

>
>> 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.

IMO, if we did not care about performance, each set would define
*some* order. For most it would be ascending order.

>
>> 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.

Perhaps we could make the @predicate function handle some of it? Maybe
@predicate('...', definesorder=True) or so that makes the revset
engine pass an order argument to it and to honor the order it returns
(maybe it was more complicated, I don't remember). It turns out that
most predicates don't care much about the subset either, so we could
even change the predicate to make the subset argument optional.