D451: revset: remove order information from tree

yuja (Yuya Nishihara) phabricator at mercurial-scm.org
Mon Aug 21 11:28:35 EDT 2017


yuja added a comment.


  (just scanned the series; no careful review yet)
  
  Perhaps the name `defineorder` was misleading. As of lazyset was introduced,
  most revset predicates "follow"ed the order of the input `subset`, which was by
  default rev-ascending. This is also true now. And there are a few exceptions
  (e.g. `rangeset`), which MAY enforce their ordering scheme if the flag is `defineorder`.
  
  So, `anyorder` for `not x` would be my mistake because `x and not y` could be
  theoretically flipped.
  
  We could change this policy so all predicates must "define" their own ordering
  schemes (as you did, I think), but I guess that would lead to bugs that are hardly
  noticed. So I'm against to bring more "any" ordering without noticeable
  performance win. Can you measure it?

INLINE COMMENTS

> quark wrote in revset.py:129
> In theory this should be:
> 
>   if order == defineorder:
>       return xs & subset
>   else:
>       return subset & xs
> 
> But it's a bit tricky to find a counterexample. I'm still trying.

`subset & xs` should be correct since `dagrange` doesn't have
its own order unlike `rangeset`.

Most revset functions "follow" the default order even if they
are used where they may "define" order.

> revset.py:893
> +        # followorder for now to hide those issues.
> +        xorder = followorder
> +    else:

IIUC, `followorder` is correct because the ordering flags of
`x and y` are flipped as if they were `y and x`.

> revset.py:1830
> +
> +    revs = getset(repo, subset, s, sorder)
>  

Can you split this to new patch, and preferably include a micro benchmark?

Revset had historically lots of subtle ordering bugs, and I believe
there are still some. Fewer "if"s should be better in general.

REPOSITORY
  rHG Mercurial

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

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


More information about the Mercurial-devel mailing list