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

Yuya Nishihara yuya at tcha.org
Mon Jun 27 11:52:57 EDT 2016


On Sun, 26 Jun 2016 08:00:14 -0700, Martin von Zweigbergk wrote:
> On Sun, Jun 26, 2016 at 1:33 AM, Yuya Nishihara <yuya at tcha.org> wrote:
> > On Sat, 25 Jun 2016 06:26:17 -0700, Martin von Zweigbergk wrote:  
> >> On Fri, Jun 24, 2016 at 10:58 PM, Yuya Nishihara <yuya at tcha.org> wrote:  
> >> > On Fri, 24 Jun 2016 10:27:03 -0700, Martin von Zweigbergk wrote:  
> >> >> On Thu, Jun 23, 2016 at 8:59 AM, Yuya Nishihara <yuya at tcha.org> wrote:  
> >> >> > # HG changeset patch
> >> >> > # User Yuya Nishihara <yuya at tcha.org>
> >> >> > # Date 1455629461 -32400
> >> >> > #      Tue Feb 16 22:31:01 2016 +0900
> >> >> > # Node ID bd5e886ceca4257dd843f98539a18ef7c31ec8e0
> >> >> > # Parent  0a1b1af8c9b8aaa0690de2fc0614b531dc03a5c2
> >> >> > revset: insert _unordered to fix ordering of nested 'or' and '_list*()' (BC)  
> >> >  
> >> >> Getting the order right is not an optimization, so I'd really like the
> >> >> order to be fixed before optimize(), and then optimize() can (as you
> >> >> do in later patches) remove some unnecessary ordering.  
> >> >
> >> > optimize() does a bunch of tree rewriting stuffs unrelated to the
> >> > optimization. Do you mean we should split optimize() into two more parts
> >> > so that we can compare the optimized result with the pure result?  
> >>
> >> It seems to me like that would make it clearer if optimize() only did
> >> optimization, and I had assumed that that's how it already was. Since
> >> that is far from the case, that would obviously be another patch if we
> >> even want to do that.  
> >
> > Sure. I like the idea of splitting optimize(). If it is simple enough, let's
> > try that as a separate series.
> >  
> >> > I'm okay to add unorderedor(), _unorderdlist*(), etc. and replace _unordered()
> >> > by them. That will avoid confusion.  
> >>
> >> Yes, I think that would make it much clearer that they were not
> >> following the subset ordering like the other functions do. Thanks!  
> >
> > will do in V5. BTW, it seems we have opposite definition for these terms.
> > My definition is:
> >
> >   ordered:   has special ordering like rangeset() and orset()
> >   unordered: has no special requirement for ordering, follows the subset
> >
> > From your comment, I should rename the current _list to _(un)?orderedlist
> > to mark it a function having special ordering requirement.  
> 
> My view is this:
> 
> * All predicates define some native order that's visible when used at
> the top level, or when used in unions. For most, it's simply ascending
> by rev number. E.g. rangeset, dagset, sorted have a different order.
> * "x & y" should preserve the order of x

Yeah, my view is different:

* Most predicates are like filters, which have no particular order, and they
  take the order specified by the input, which is ascending by rev number
  by default.
* A few predicates (e.g. range, sort, or) have order, which is applied only
  when the predicate is used at top level, "x" of "x & y".

> So when I used "ordered" or "{un,mis}ordered", I meant whether it
> preserves the order of the subset it's intersected with. I don't think
> we should have any functions that return an unspecified order.

I agree on this point. But I think the order should be specified by the subset
argument, so "subset & x" guarantees the result is stable.

> I believe that's what Pierre-Yves's recent commit for sorting sets in
> ascending order was about. Hopefully we don't have any cases left of
> unspecified order.

IIRC, 69c6e9623bdc was necessary to make repr(baseset(...)) stable.

> Do you agree? I got the feeling earlier that you did not think each
> function should necessarily have a native order, as long as the revset
> module as a whole made sure the result made sense.

If a native order means ascending by rev number, I don't think it's necessary.
I think the important property is that each function take the order specified
by the input set.


More information about the Mercurial-devel mailing list