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

[Yuya Nishihara]

On Wed, 3 Aug 2016 14:24:28 -0700, Martin von Zweigbergk wrote:
> 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?

No difference. I just wanted to tell that I took many revset predicates as
set operations.

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

Yeah, mutating the other set seems messy, but that is another issue we have
in the current revset code. Given that fullreposet is a spanset, it's valid
to return the other set in self's order.

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

IMO, that was a possible workaround for the ordering issue. We could do that
if a) fullreposet were used only for bootstrapping and if b) all revset
predicates had stable order. Since (a) is wrong because of optimize(), I
don't think it's good idea to make fullreposet.__and__() disagree with its
superclass.

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

That's okay, but anyway we would have to pick one order from them.

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

I'm sure it won't be simple, but it appears not that complicated. I'll try.