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