yuja added a comment.

  In https://phab.mercurial-scm.org/D451#7281, @quark wrote:
  
  > I think it's more correct if all core revsets support `defineorder` 
explicitly. The current code depends on `revset.makematcher` using ascending 
fullreposet as default to make `defineorder` implicitly functional. If the 
callsite passes an non-ascending (unordered, or descending) set to the returned 
matcher, the code would behave wrong (ex. `_flipand(1:0, _flipand(1:0, 0::1))` 
would be wrong if a descending fullreposet is passed).
  
  
  Perhaps `_flipand(1:0, _flipand(1:0, 0::1))` would return `[1, 0]` if the 
input set
  were reversed. IMHO, that's correct under the original design.
  
  > It seems to me that the reason why people can get ordering wrong is because 
of legacy APIs (`repo, subset, x`). I tried to address that by introducing 
`intersect(subset, xs, order)` and suggest `repo, x` API 
(https://phab.mercurial-scm.org/D453). If people write code using the new API, 
it's much harder to have ordering issues.
  > 
  > https://phab.mercurial-scm.org/D456 provides a good test coverage about 
core revset ordering issues. If we really want to address ordering issues of 
3rd party code, maybe we can deprecate `repo, subset, x` API and force people 
to either take `subset` and `order`, or none of them. Or require an explicit 
`supportorder` flag to be defined to mark it as order-safe.
  
  I agree new decorator API would be slightly better for trivial cases, but the 
situation
  for 3rd-party extensions would be worse. Before, `subset` was the canonical
  source of ordering. Since most revset predicates should not have their own 
order,
  they just needed to return a set in subset's order.
  
    return subset.filter(...)
  
  IIUC, this will be no longer be valid. All revset predicates will have to 
take care
  of `order` flag if they need a `subset` argument, just because few predicates
  want to enforce their order. This seems not a good balance.

INLINE COMMENTS

> revset.py:55
>  
> -def getset(repo, subset, x):
> +def getset(repo, subset, x, order=anyorder):
>      if not x:

Perhaps the default `order=defineorder` would be safer
at this point.

> quark wrote in revset.py:893
> In this case, `y` is expected to completely redefine the order. So `y`'s 
> `subset`'s order does not matter.

So in your proposed design, that's true. x's order doesn't matter.

I just meant, in the original design, `x` should follow the subset's order
because `y` could have no explicit ordering (so `y` follows `x`, which follows 
`subset`.)

REPOSITORY
  rHG Mercurial

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

To: quark, #hg-reviewers
Cc: yuja, mercurial-devel
_______________________________________________
Mercurial-devel mailing list
Mercurial-devel@mercurial-scm.org
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel

Reply via email to