This is an archive of the discontinued Mercurial Phabricator instance.

revset: remove order information from tree
ClosedPublic

Authored by quark on Aug 19 2017, 1:42 PM.

Details

Summary

Keeping order in tree makes AST operation harder. And there could be
invalid cases if trees could be generated and compounded freely, like:

SetA(order=define) & SetB(order=define)
                                ^^^^^^ couldn't be satisfied

This patch changes the code to calculate order on the fly, during tree
traversal. Optimization of reordering and arguments is preserved by
introducing a new internal operation flipand.

Diff Detail

Repository
rHG Mercurial
Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

quark created this revision.Aug 19 2017, 1:42 PM
quark updated this revision to Diff 1091.Aug 19 2017, 2:01 PM
quark added a subscriber: yuja.EditedAug 19 2017, 2:17 PM

@yuja Let me know if this can simplify matchtree, buildtree implementation.

I also wonder if it makes sense to move (part of, mostly weight related) _optimize to runtime (getset), since the revset functions could have more information. For example, if sort gets rev as sort key, it could use getset(order=any) instead of getset(order=define). Some functions like ancestors(revs) also do not care about the order of revs, and we are being conservative - using define for all function arguments now.

tests/test-revset.t
2507

The new code is less efficient here. I guess we it might be solvable by having a _reverseand operator that _optimize may use.

quark updated this revision to Diff 1092.Aug 19 2017, 2:19 PM
quark edited the summary of this revision. (Show Details)Aug 19 2017, 2:30 PM
quark updated this revision to Diff 1093.Aug 19 2017, 2:52 PM
quark updated this revision to Diff 1094.Aug 19 2017, 2:58 PM
quark edited the summary of this revision. (Show Details)Aug 19 2017, 4:05 PM
quark updated this revision to Diff 1095.
quark added inline comments.Aug 19 2017, 4:06 PM
tests/test-revset.t
2893

This is caused by fullreposet having a default order. If we remove that, it would be optimized to <baseset [1, 3, 5]> here.

quark marked an inline comment as done.Aug 19 2017, 4:09 PM
quark updated this revision to Diff 1096.Aug 19 2017, 4:50 PM
quark updated this revision to Diff 1097.Aug 19 2017, 5:03 PM
quark retitled this revision from [RFC] revset: remove order information from tree to revset: remove order information from tree.Aug 19 2017, 5:17 PM
quark updated this revision to Diff 1098.
quark added inline comments.Aug 19 2017, 10:58 PM
mercurial/revset.py
59–66

TODO can be removed.

133

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.

yuja added a comment.Aug 19 2017, 11:05 PM

Clever. I haven't looked this carefully, but the general direction seems fine.

@yuja Let me know if this can simplify matchtree, buildtree implementation.

Actually matchtree can ignore extra elements in a node tuple, so the existence
of order flag is acceptable, though this will slightly simplify the match function.

I also wonder if it makes sense to move (part of, mostly weight related) _optimize to runtime (getset), since the revset functions could have more information. For example, if sort gets rev as sort key, it could use getset(order=any) instead of getset(order=define). Some functions like ancestors(revs) also do not care about the order of revs, and we are being conservative - using define for all function arguments now.

That could be, perhaps.

quark updated this revision to Diff 1099.Aug 20 2017, 12:12 AM
quark marked an inline comment as done.Aug 20 2017, 12:13 AM
quark updated this revision to Diff 1103.Aug 20 2017, 4:43 AM
quark updated this revision to Diff 1108.Aug 20 2017, 3:58 PM
yuja added a comment.Aug 21 2017, 11:28 AM

(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?

mercurial/revset.py
133

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.

900

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

1836

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.

quark added a comment.EditedAug 21 2017, 12:36 PM

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).

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 (D453). If people write code using the new API, it's much harder to have ordering issues.

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.

So I'm against to bring more "any" ordering without noticeable
performance win. Can you measure it?

Probably hard to measure. Since it's in theory useful for performance, I'm leaning towards keeping it. Test can random-shuffle anyorder sets and expect the result to still be correct (I didn't include that test change since D456 has better coverage with less code).


I'll wait for some comments before updating the series. The planned changes are:

  • move micro optimizations like anyorder in sort to a single patch.
  • move D456 to maybe the first patch in the series so we can track ordering correctness changes closely.
quark added inline comments.Aug 21 2017, 1:46 PM
mercurial/revset.py
133

For subset & xs to be correct, subset needs to be in ascending order. That is true currently. But it is not very obvious why subset is in ascending order here (or, the question is, who is responsible to sort it?).

I think it's simpler to not depend on it and make every revset respect defineorder explicitly. That also allows us to remove some unnecessary sorting.

900

In this case, y is expected to completely redefine the order. So y's subset's order does not matter.

1836

I can do that.

quark added inline comments.Aug 21 2017, 1:48 PM
mercurial/revset.py
900

By "y's subset", I mean "getset(repo, subset, x, xorder)".

yuja added a comment.Aug 22 2017, 10:23 AM
In 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 (D453). If people write code using the new API, it's much harder to have ordering issues.
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.

mercurial/revset.py
56

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

900

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.)

yuja added a comment.Aug 22 2017, 10:29 AM
In D451#7257, @yuja wrote:

So, anyorder for not x would be my mistake because x and not y could be
theoretically flipped.

FWIW, this should be okay since not x follows the order of the input subset.
The order of the x doesn't matter.

quark added a comment.EditedAug 22 2017, 12:12 PM
In D451#7456, @yuja wrote:

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.

I see. The old code allows "weak define" that "define" becomes "follow". There is no way to tell if a revset is "strong define" or "weak define" from the help text. So it could be confusing sometimes.

list(revset.match(None, '_flipand(1:0, _flipand(1:0, 0::1))', order=ORDER)(repo, revset.baseset(INITSET)))
OLD CODEORDER=defineORDER=follow
INITSET=[0,1][0,1][0,1]
INITSET=[1,0][1,0][1,0]
NEW CODEORDER=defineORDER=follow
INITSET=[0,1][0,1][0,1]
INITSET=[1,0][0,1][1,0]

Since set.sort() is lazy and optimized to a no-op. Migrating everything to "strong define" does not seem to hurt performance in the default use-case. So I'd like to do that for core revsets.

but the situation for 3rd-party extensions would be worse

I think not all 3rd-party extensions need subset. subset is only required if they need filter the subset. There are many cases where just defining a small set is enough (ex. remotenames()). I'd like to show another "define a small set" example: d6708c20, where it's easier to make mistake using the old API (well, it's the reviewer of that change to blame).

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.

For the old code, all predicates must not change order if it's not supposed to define. I now see that's why anyorder is less useful in the old code - not seems to be the only valid case. The new code would allow anyorder to be used in wider cases.


Not directly related to this series, I think some revsets might want a non-ascending "define" order. For example, it seems more natural if p1(A+B) could be equivalent to p1(A)+p1(B). Same applies to p2, parents, children and maybe roots, heads.

quark added inline comments.Aug 22 2017, 12:19 PM
mercurial/revset.py
56

Agree anyorder could be a surprise. I was trying to optimize aggressively. Maybe followorder is a better default since subset & x is the old default.

quark added a comment.EditedAug 22 2017, 2:15 PM

Interestingly, I checked some well known 3rd-party extensions:

hgsubversion:

  • svnrev() uses x for x in myset if x in subset therefore enforces "defineorder" and wrong.
  • fromsvn() is also wrong similarly.

hg-git:

  • fromgit(), gitnode(): happened to be "followorder" since it uses x for x in subset if ...

mutable-history:

  • troubled(), suspended(), precursors(), ...: all of them use subset & ... , therefore enforces "followorder"

remotenames:

  • remotenames(): uses subset & ... pattern
  • upstream(): uses smartset.filteredset(subset, ...), which could be changed to subset.filter(...). But it's literally subset & tipancestors and does not have to use subset.filter.
  • pushed(): same as upstream()

All of them could just remove subset argument without hurting performance. They may not be able to do that because they need to support older Mercurial. But I think that could be seen as an indication that new code probably does not need the subset argument.

I had also wanted to remove the need to pass subset, so I'd be happy so that change.

tests/test-revset.t
2893

Does that mean you'll remove the other.sort() in fullreposet.and?

quark added inline comments.Aug 22 2017, 2:28 PM
tests/test-revset.t
2893

In another way. With the new code (anyorder gets aggressively used), fullrepo & xs would be optimized to xs & fullrepo and the latter does not have the sort.

quark added inline comments.Aug 22 2017, 4:58 PM
mercurial/revset.py
56

Actually, defineorder as default also makes sense and seems to be better. I'll use it.

yuja added a comment.Aug 23 2017, 9:11 AM
In D451#7499, @quark wrote:

hgsubversion:

  • svnrev() uses x for x in myset if x in subset therefore enforces "defineorder" and wrong.
  • fromsvn() is also wrong similarly.

Yes, they are wrong (and I've pointed out that before in hgsubversion thread.)
They could benefit from new subset-less API.

hg-git:

  • fromgit(), gitnode(): happened to be "followorder" since it uses x for x in subset if ...

mutable-history:

  • troubled(), suspended(), precursors(), ...: all of them use subset & ... , therefore enforces "followorder"

remotenames:

  • remotenames(): uses subset & ... pattern
  • upstream(): uses smartset.filteredset(subset, ...), which could be changed to subset.filter(...). But it's literally subset & tipancestors and does not have to use subset.filter.
  • pushed(): same as upstream()

All of them could just remove subset argument without hurting performance. They may not be able to do that because they need to support older Mercurial. But I think that could be seen as an indication that new code probably does not need the subset argument.

The point is these implementations would become invalid if subset were
optimized to "any" order. That's breaking change, but which is likely to be
not covered by tests.

As I said, most revset predicates have no explicit order. I introduced the
order flag in order to work around a few exceptions, which are x:y,
x + y, sort() and reverse(), IIRC. A strong "define" is exceptional.

So, is it really make sense to revamp the revset ordering rules? I don't
think so. I generally like this series, but -1 for bringing "any" order
everywhere.

quark added a comment.EditedAug 23 2017, 11:41 AM
In D451#7698, @yuja wrote:

The point is these implementations would become invalid if subset were
optimized to "any" order. That's breaking change, but which is likely to be
not covered by tests.

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?

That seems to address the concern about correctness of legacy code.

As I said, most revset predicates have no explicit order. I introduced the
order flag in order to work around a few exceptions, which are x:y,
x + y, sort() and reverse(), IIRC. A strong "define" is exceptional.

I feel it inconsistent if x:y has a strong order but x::y does not.
I also want to change p1, etc's define order as mentioned above.

So, is it really make sense to revamp the revset ordering rules? I don't
think so. I generally like this series, but -1 for bringing "any" order
everywhere.

I think in general, the new code is more explicit and better testable.
I'd like to move forward and not get blocked by legacy code.

I understand "anyorder" in the old code is almost a mistake. But with
the new code, and suppose no legacy code is used, "anyoder" is safe.
It seems to be simple (only used in a few places), less error-prone
(core hg has and encourages strong defineorder), and do have value
for optimization. I'd like to keep it. I can gate it by a config
option but I don't think that's necessary if we make registrar handle
it automatically.

yuja added a comment.Aug 24 2017, 11:05 AM

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. How fast is the anyorder set in real-world
queries? Is that worth introducing more "if"s and corresponding tests?

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.

As I said, most revset predicates have no explicit order. I introduced the
order flag in order to work around a few exceptions, which are x:y,
x + y, sort() and reverse(), IIRC. A strong "define" is exceptional.

I feel it inconsistent if x:y has a strong order but x::y does not.

It would be surprising if x::parent suddenly starts listing parent's children
in reverse order.

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?

So, is it really make sense to revamp the revset ordering rules? I don't
think so. I generally like this series, but -1 for bringing "any" order
everywhere.

I think in general, the new code is more explicit and better testable.
I'd like to move forward and not get blocked by legacy code.
I understand "anyorder" in the old code is almost a mistake. But with
the new code, and suppose no legacy code is used, "anyoder" is safe.
It seems to be simple (only used in a few places), less error-prone
(core hg has and encourages strong defineorder), and do have value
for optimization. I'd like to keep it. I can gate it by a config
option but I don't think that's necessary if we make registrar handle
it automatically.

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 current rule is IMHO, simpler. Almost all revsets are in the same
order unless they are explicitly sorted or concatenated.

quark added a comment.EditedAug 24 2017, 12:01 PM
In 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.

I just got a new idea: Wrap those 3-argument predicate in a function that
does an extra sort if it's defineorder [1]. That seems to solve things
cleanly.

How fast is the anyorder set in real-world
queries? Is that worth introducing more "if"s and corresponding tests?

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)

So I think anyorder definitely has value (not that much to a revset expert though).

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.

I hope the word "hard" here could suggest that the new code does make things simpler at least in this case.

It would be surprising if x::parent suddenly starts listing parent's children
in reverse order.
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?

We can say revsets taking N revs returning O(N) revs would maintain
the order, and not for other revsets. That'd be clearly defined.
But I don't feel strong. We can keep the existing behavior here.

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 enforce-define [1] idea seems to address this well)

The current rule is IMHO, simpler. Almost all revsets are in the same
order unless they are explicitly sorted or concatenated.

Maybe. But if you insist defineorder does not always need to "define" order.
I hope I can rename it to maybedefineorder, which will make it less confusing
for developers. That said, I still prefer strong "define"s. It seems Martin
also likes it. I think it's simpler because both developers and end-users
won't need to worry about the "weak define" concept.

yuja added a comment.Aug 25 2017, 11:34 AM

First, I'm tired of discussing this. Perhaps, you would be the same (guessing
from the initial reply.) I don't think this should be a blocker of your previous
series which optimizes draft() ... something.

If you want to move things forward, please split non-controversial parts,
which I think are:

  • remove "order" from parsed tree, use _flipand() instead
  • make subset argument optional

Alternatively, I could send my build/matchtree patch without respecting to
this series to unblock your original patch.

I just got a new idea: Wrap those 3-argument predicate in a function that
does an extra sort if it's defineorder [1]. That seems to solve things
cleanly.

I don't think it's clean, but yeah doable. If I had to take this series, that would
be the safest workaround for old 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)

So I think anyorder definitely has value (not that much to a revset expert though).

In this example, public() & 1:3 could be fully optimized to anyorder
without the help of _flipand(). It's obvious that the order of public() & 1:3
doesn't matter.

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.

I hope the word "hard" here could suggest that the new code does make things simpler at least in this case.

I think it's "simpler" at the cost of bringing the "order" concept everywhere.

We can say revsets taking N revs returning O(N) revs would maintain
the order, and not for other revsets. That'd be clearly defined.
But I don't feel strong. We can keep the existing behavior here.

Yep. I don't want to think about the complexity to determine if a revset
predicate may define its own order.

Maybe. But if you insist defineorder does not always need to "define" order.
I hope I can rename it to maybedefineorder, which will make it less confusing
for developers.

Seems good.

That said, I still prefer strong "define"s. It seems Martin
also likes it.

I guess he liked it since the new registrar would get rid of the subset
argument. So do I in that regard.

I think it's simpler because both developers and end-users
won't need to worry about the "weak define" concept.

Users and (most) developers don't have to think about it in either case.
Almost all revsets should be in revision order.

quark added a comment.EditedAug 25 2017, 12:47 PM

Don't worry about that optimization patch. I'll refactor this series and use maybedefine. Thanks!

yuja added a comment.Aug 27 2017, 8:32 AM

It seems _optimize() has more bugs, so I decided to not queue this.
I have partially updated patch, but how can I collaborate with you?

FWIW, I think _optimize() could use defineorder/anyorder constants
in place of True/False for readablity.

mercurial/revset.py
64

methods is the table of operators, not functions, which wouldn't be modified
by extensions. I'll drop this change.

143

right-hand side was originally anyorder, so updated in flight.

163

This, too. added anyorder in flight.

894

This should be undocumented. Changed to a comment.

mercurial/revsetlang.py
392

Split this back to unary/binary/ternary cases since I slightly prefer
explicit handling of node tuples.

474

Perhaps this should keep the current preserveorder since
not public() is fully replaced with _notpublic().

482

The order matters. Try (contains("glob:") & 2:0):1 for example.

497

Perhaps this is preserveorder since keyvalue node isn't a
standalone expression.

yuja added inline comments.Aug 27 2017, 8:42 AM
mercurial/revsetlang.py
474

This is an existing bug, btw.

$ hg debugrevspec -p analyzed -p optimized 'not public()'
* analyzed:
(not
  (func
    ('symbol', 'public')
    None
    any)
  define)
* optimized:
(func
  ('symbol', '_notpublic')
  None
  any)
yuja added a comment.Aug 27 2017, 10:48 AM

It seems _optimize() has more bugs

Perhaps we can eliminate preserveorder flag from _optimize() at all.
flipand can always be inserted since and(x, y) is exactly the same as
flipand(x, y) if order != defineorder. For or, we could

  • backout c63cb2d10d6d assuming the optimization wouldn't be that useful
  • or add an internal method to select define/anyorder tree at runtime, e.g. (switch-by-order (or original-tree) (or sorted-tree))

One of the drawback of the current patch is we have to resolve the order
constraint twice, at parsing phase and evaluation phase. That's probably why
I decided to embed the flag to parsed tree, though I don't remember it. :-)

mercurial/revset.py
894

Nit: this could be an internal method (= operator) like difference
since we don't need to embed it in revset expression.

quark added a comment.Aug 27 2017, 1:26 PM

I have partially updated patch, but how can I collaborate with you?

You can phabsend . which will update this patch (assuming Differential Revision: .../D451 line exists), and I can fix the rest of things.

Perhaps we can eliminate preserveorder flag from _optimize() at all.

I like that simplicity. I don't think or optimization is that important (so there was no _flipor).

If we can attach extra hints (not affecting correctness if get lost) to tree nodes (ex. not tuple, but a TreeNode object with a estimated_weight property), the optimization about and and or could be moved to runtime and _flipand becomes unnecessary.

mercurial/revset.py
64

hgsubversion replaces stringset to support names like r123. We did similar things to support D123. I guess the most correct way would be using repo.names somehow.

I don't feel strong. Dropping this makes core code cleaner. I can fix existing extensions we use.

894

Since we had _notpublic, I felt the practice here was to not "pollute" the "methods" table. i.e. "methods" can only contain names outputted directly from the parser.

So I still slightly prefer not using an operator. What do you think?

mercurial/revsetlang.py
392

Ha, that was an older version of this patch.

474

I thought since _notpublic is atomic and cannot be further split. preserveorder does not matter here.

482

Good catch. I realized this when working on revset.py but forgot to update it here.

497

Not sure. I think it's similar to function argument (which we preserves order by default in line 463).

yuja updated this revision to Diff 1347.Aug 28 2017, 9:00 AM
yuja added a comment.Aug 28 2017, 9:16 AM

You can phabsend . which will update this patch

Done.

Perhaps we can eliminate preserveorder flag from _optimize() at all.

I like that simplicity. I don't think or optimization is that important (so there was no _flipor).

Let's revert c63cb2d10d6d then.

mercurial/revset.py
894

I don't have strong preference as we already have pseudo operator difference.

mercurial/revsetlang.py
474

It's just for logical correctness. Since not public() is replaced with
_notpublic(), the ordering constraint should be derived from
not public(), not from public().

497

preserveorder is switched to True at func node, and its
child list just passes around it. The same rule should apply to
keyvalue.

Anyway, we should get rid of it from _optimize().

quark edited the summary of this revision. (Show Details)Aug 29 2017, 10:02 AM
quark updated this revision to Diff 1389.
yuja accepted this revision.Aug 30 2017, 9:21 AM

Flagged as (API) and queued the series, thanks!

This revision is now accepted and ready to land.Aug 30 2017, 9:21 AM
This revision was automatically updated to reflect the committed changes.
martinvonz added inline comments.Aug 30 2017, 1:10 PM
mercurial/revset.py
143

I think using the regular (x,y) order would be clearer. You'd need to rename it then. Maybe something like:

# 'smallyand(x, y)' is equivalent to 'and(x, y)', but faster when y is small

Feel free to ignore.

quark added inline comments.Aug 30 2017, 3:46 PM
mercurial/revset.py
143

I came up with a similar idea yesterday but didn't write a patch. I think maintaining AST node order could be less confusing for developers. So I'll send a patch.

Maybe this could be an additional option of the "andset".