D451: revset: remove order information from tree

[quark (Jun Wu)]

quark added a comment.


  In https://phab.mercurial-scm.org/D451#8029, @yuja wrote:
  
  > > How about making `registrar.revsetpredicate` conservative? If it sees any
  > >  predicate registered with the old API, Disable `anyorder` optimization and
  > >  change it to `followorder` in runtime?
  >
  > Sounds unnecessarily complicated.
  
  
  It could be a `maybeanyorder`. I think it's just a few lines of change.
  
  > How fast is the `anyorder` set in real-world
  >  queries? Is that worth introducing more "if"s and corresponding tests?
  
  It's easy to construct revsets that are slow for the current code:
  
    mozilla-central % hg.old perfrevset 'sort(public() & 1:3)'
    ! wall 0.193297 comb 0.190000 user 0.190000 sys 0.000000 (best of 51)
    mozilla-central % hg.new perfrevset 'sort(public() & 1:3)'
    ! wall 0.000188 comb 0.000000 user 0.000000 sys 0.000000 (best of 13504)
  
  Sure, revset experts can optimize it manually. But I'd like to make it work
  automatically without requiring expert knowledge.
  
  > We can apply `anyorder` optimization to inner queries even if we decided to not
  >  optimize `_flipand()`. For example, think `ancestors(complex_query)`, where
  >  `complex_query` can be computed in `anyorder` constraint because we can be
  >  sure the order of head revisions doesn't matter. OTOH, `_flipand(x, y)` is hard
  >  because `x` is passed to arbitrary sub-expression `y`.
  
  For `ancestors` (and many other predicates), the new code has that optimization already.
  For `_flipand`, the new code also optimizes it cleanly.
  
  > It would be surprising if `x::parent` suddenly starts listing `parent`'s children
  >  in reverse order.
  
  I didn't mean to do that. (I didn't even know `5:0` is non-empty before)
  
  >> I also want to change `p1`, etc's define order as mentioned above.
  > 
  > I don't like this idea because it would make things more inconsistent.
  >  For instance, `branch(a + b)` could be `branch(a) + branch(b)`, and `ancestors(a:b)`
  >  could be `ancestors(a) + ... + ancestors(b)`, but is that really what we want?
  
  The line could be whether a revset taking N revs returning O(N) revs.
  So `ancestors` or `branch` won't change.
  
  > I doubt if it is really error-prone. We'll have to make sure that a revset
  >  function returns a set in the right "defined" order (or an unordered set
  >  which will be sorted implicitly by `intersect()`.)
  
  The new code introduces `revspec --check-order` and if all third party
  extensions use that, it will be correct. Since I'd expect new code to
  mostly not having the `subset` argument, they will be correct.
  
  > The current rule is IMHO, simpler. Almost all revsets are in the same
  >  order unless they are explicitly sorted or concatenated.
  
  My opinion could be subjective. But I hope I can quote Martin's words on IRC:
  
  > Aug 22 13:20:46 <martinvonz>	junw: that makes sense. i found the code easier to understand after your patches. i think the problem was largely that i didn't (and still don't) understand what the current idea about ordering is

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D451

To: quark, #hg-reviewers
Cc: martinvonz, yuja, mercurial-devel