D451: revset: remove order information from tree

[martinvonz (Martin von Zweigbergk)]

martinvonz added inline comments.

INLINE COMMENTS

> revset.py:136
> +def flipandset(repo, subset, y, x, order):
> +# 'flipand(y, x)' is equivalent to 'and(x, y)', but faster when y is 
> small
> +if order == anyorder:

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.

REPOSITORY
  rHG Mercurial

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

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

D451: revset: remove order information from tree

[quark (Jun Wu)]

This revision was automatically updated to reflect the committed changes.
Closed by commit rHG1b28525e6698: revset: remove order information from tree 
(API) (authored by quark).

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST UPDATE
  https://phab.mercurial-scm.org/D451?vs=1389=1434

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

AFFECTED FILES
  mercurial/revset.py
  mercurial/revsetlang.py
  tests/test-revset.t

CHANGE DETAILS

diff --git a/tests/test-revset.t b/tests/test-revset.t
--- a/tests/test-revset.t
+++ b/tests/test-revset.t
@@ -166,8 +166,7 @@
 None)
   * optimized:
   (rangeall
-None
-define)
+None)
   * set:
   
   0
@@ -495,8 +494,7 @@
 ('symbol', 'foo')
 (func
   ('symbol', '_notpublic')
-  None
-  any))
+  None))
   hg: parse error: can't use a key-value pair in this context
   [255]
 
@@ -538,52 +536,43 @@
 (not
   (func
 ('symbol', 'public')
-None
-any)
-  define)
+None))
 ('symbol', 'generations')
-('symbol', '0')
-define)
+('symbol', '0'))
   * optimized:
   (relsubscript
 (func
   ('symbol', '_notpublic')
-  None
-  any)
+  None)
 ('symbol', 'generations')
-('symbol', '0')
-define)
+('symbol', '0'))
 
 resolution of subscript and relation-subscript ternary operators:
 
   $ hg debugrevspec -p analyzed 'tip[0]'
   * analyzed:
   (subscript
 ('symbol', 'tip')
-('symbol', '0')
-define)
+('symbol', '0'))
   hg: parse error: can't use a subscript in this context
   [255]
 
   $ hg debugrevspec -p analyzed 'tip#rel[0]'
   * analyzed:
   (relsubscript
 ('symbol', 'tip')
 ('symbol', 'rel')
-('symbol', '0')
-define)
+('symbol', '0'))
   hg: parse error: unknown identifier: rel
   [255]
 
   $ hg debugrevspec -p analyzed '(tip#rel)[0]'
   * analyzed:
   (subscript
 (relation
   ('symbol', 'tip')
-  ('symbol', 'rel')
-  define)
-('symbol', '0')
-define)
+  ('symbol', 'rel'))
+('symbol', '0'))
   hg: parse error: can't use a subscript in this context
   [255]
 
@@ -593,23 +582,19 @@
 (relsubscript
   ('symbol', 'tip')
   ('symbol', 'rel')
-  ('symbol', '0')
-  define)
-('symbol', '1')
-define)
+  ('symbol', '0'))
+('symbol', '1'))
   hg: parse error: can't use a subscript in this context
   [255]
 
   $ hg debugrevspec -p analyzed 'tip#rel0#rel1[1]'
   * analyzed:
   (relsubscript
 (relation
   ('symbol', 'tip')
-  ('symbol', 'rel0')
-  define)
+  ('symbol', 'rel0'))
 ('symbol', 'rel1')
-('symbol', '1')
-define)
+('symbol', '1'))
   hg: parse error: unknown identifier: rel1
   [255]
 
@@ -619,11 +604,9 @@
 (relsubscript
   ('symbol', 'tip')
   ('symbol', 'rel0')
-  ('symbol', '0')
-  define)
+  ('symbol', '0'))
 ('symbol', 'rel1')
-('symbol', '1')
-define)
+('symbol', '1'))
   hg: parse error: unknown identifier: rel1
   [255]
 
@@ -700,20 +683,15 @@
 (or
   (list
 ('symbol', '0')
-('symbol', '1'))
-  define)
+('symbol', '1')))
 (not
-  ('symbol', '1')
-  follow)
-define)
+  ('symbol', '1')))
   * optimized:
   (difference
 (func
   ('symbol', '_list')
-  ('string', '0\x001')
-  define)
-('symbol', '1')
-define)
+  ('string', '0\x001'))
+('symbol', '1'))
   0
 
   $ hg debugrevspec -p unknown '0'
@@ -733,18 +711,14 @@
   (and
 (func
   ('symbol', 'r3232')
-  None
-  define)
-('symbol', '2')
-define)
+  None)
+('symbol', '2'))
   * optimized:
-  (and
+  (flipand
 ('symbol', '2')
 (func
   ('symbol', 'r3232')
-  None
-  define)
-define)
+  None))
   * analyzed set:
   
   * optimized set:
@@ -776,8 +750,7 @@
 None)
   * analyzed:
   (rangeall
-None
-define)
+None)
   * set:
   
   0
@@ -793,8 +766,7 @@
   $ try -p analyzed ':1'
   * analyzed:
   (rangepre
-('symbol', '1')
-define)
+('symbol', '1'))
   * set:
   
   0
@@ -805,9 +777,7 @@
 (or
   (list
 ('symbol', '1')
-('symbol', '2'))
-  define)
-define)
+('symbol', '2'
   * set:
   
   0
@@ -818,9 +788,7 @@
   (rangepre
 (and
   ('symbol', '1')
-  ('symbol', '2')
-  define)
-define)
+  ('symbol', '2')))
   * set:
   
 
@@ -1643,11 +1611,8 @@
 (difference
   (range
 ('symbol', '8')
-('symbol', '9')
-define)
-  ('symbol', '8')
-  define)
-define)
+('symbol', '9'))
+  ('symbol', '8')))
   * set:
   
   8
@@ -1663,8 +1628,7 @@
 ('symbol', 'only')
 (list
   ('symbol', '9')
-  ('symbol', '5'))
-define)
+  ('symbol', '5')))
   * set:
   
   2
@@ -1999,18 +1963,13 @@
 (and
   (range
 ('symbol', '3')
-   

D451: revset: remove order information from tree

[yuja (Yuya Nishihara)]

yuja accepted this revision.
yuja added a comment.
This revision is now accepted and ready to land.


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

REPOSITORY
  rHG Mercurial

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

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

D451: revset: remove order information from tree

[quark (Jun Wu)]

quark updated this revision to Diff 1389.
quark edited the summary of this revision.

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST UPDATE
  https://phab.mercurial-scm.org/D451?vs=1347=1389

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

AFFECTED FILES
  mercurial/revset.py
  mercurial/revsetlang.py
  tests/test-revset.t

CHANGE DETAILS

diff --git a/tests/test-revset.t b/tests/test-revset.t
--- a/tests/test-revset.t
+++ b/tests/test-revset.t
@@ -166,8 +166,7 @@
 None)
   * optimized:
   (rangeall
-None
-define)
+None)
   * set:
   
   0
@@ -495,8 +494,7 @@
 ('symbol', 'foo')
 (func
   ('symbol', '_notpublic')
-  None
-  any))
+  None))
   hg: parse error: can't use a key-value pair in this context
   [255]
 
@@ -538,52 +536,43 @@
 (not
   (func
 ('symbol', 'public')
-None
-any)
-  define)
+None))
 ('symbol', 'generations')
-('symbol', '0')
-define)
+('symbol', '0'))
   * optimized:
   (relsubscript
 (func
   ('symbol', '_notpublic')
-  None
-  any)
+  None)
 ('symbol', 'generations')
-('symbol', '0')
-define)
+('symbol', '0'))
 
 resolution of subscript and relation-subscript ternary operators:
 
   $ hg debugrevspec -p analyzed 'tip[0]'
   * analyzed:
   (subscript
 ('symbol', 'tip')
-('symbol', '0')
-define)
+('symbol', '0'))
   hg: parse error: can't use a subscript in this context
   [255]
 
   $ hg debugrevspec -p analyzed 'tip#rel[0]'
   * analyzed:
   (relsubscript
 ('symbol', 'tip')
 ('symbol', 'rel')
-('symbol', '0')
-define)
+('symbol', '0'))
   hg: parse error: unknown identifier: rel
   [255]
 
   $ hg debugrevspec -p analyzed '(tip#rel)[0]'
   * analyzed:
   (subscript
 (relation
   ('symbol', 'tip')
-  ('symbol', 'rel')
-  define)
-('symbol', '0')
-define)
+  ('symbol', 'rel'))
+('symbol', '0'))
   hg: parse error: can't use a subscript in this context
   [255]
 
@@ -593,23 +582,19 @@
 (relsubscript
   ('symbol', 'tip')
   ('symbol', 'rel')
-  ('symbol', '0')
-  define)
-('symbol', '1')
-define)
+  ('symbol', '0'))
+('symbol', '1'))
   hg: parse error: can't use a subscript in this context
   [255]
 
   $ hg debugrevspec -p analyzed 'tip#rel0#rel1[1]'
   * analyzed:
   (relsubscript
 (relation
   ('symbol', 'tip')
-  ('symbol', 'rel0')
-  define)
+  ('symbol', 'rel0'))
 ('symbol', 'rel1')
-('symbol', '1')
-define)
+('symbol', '1'))
   hg: parse error: unknown identifier: rel1
   [255]
 
@@ -619,11 +604,9 @@
 (relsubscript
   ('symbol', 'tip')
   ('symbol', 'rel0')
-  ('symbol', '0')
-  define)
+  ('symbol', '0'))
 ('symbol', 'rel1')
-('symbol', '1')
-define)
+('symbol', '1'))
   hg: parse error: unknown identifier: rel1
   [255]
 
@@ -700,20 +683,15 @@
 (or
   (list
 ('symbol', '0')
-('symbol', '1'))
-  define)
+('symbol', '1')))
 (not
-  ('symbol', '1')
-  follow)
-define)
+  ('symbol', '1')))
   * optimized:
   (difference
 (func
   ('symbol', '_list')
-  ('string', '0\x001')
-  define)
-('symbol', '1')
-define)
+  ('string', '0\x001'))
+('symbol', '1'))
   0
 
   $ hg debugrevspec -p unknown '0'
@@ -733,18 +711,14 @@
   (and
 (func
   ('symbol', 'r3232')
-  None
-  define)
-('symbol', '2')
-define)
+  None)
+('symbol', '2'))
   * optimized:
-  (and
+  (flipand
 ('symbol', '2')
 (func
   ('symbol', 'r3232')
-  None
-  define)
-define)
+  None))
   * analyzed set:
   
   * optimized set:
@@ -776,8 +750,7 @@
 None)
   * analyzed:
   (rangeall
-None
-define)
+None)
   * set:
   
   0
@@ -793,8 +766,7 @@
   $ try -p analyzed ':1'
   * analyzed:
   (rangepre
-('symbol', '1')
-define)
+('symbol', '1'))
   * set:
   
   0
@@ -805,9 +777,7 @@
 (or
   (list
 ('symbol', '1')
-('symbol', '2'))
-  define)
-define)
+('symbol', '2'
   * set:
   
   0
@@ -818,9 +788,7 @@
   (rangepre
 (and
   ('symbol', '1')
-  ('symbol', '2')
-  define)
-define)
+  ('symbol', '2')))
   * set:
   
 
@@ -1643,11 +1611,8 @@
 (difference
   (range
 ('symbol', '8')
-('symbol', '9')
-define)
-  ('symbol', '8')
-  define)
-define)
+('symbol', '9'))
+  ('symbol', '8')))
   * set:
   
   8
@@ -1663,8 +1628,7 @@
 ('symbol', 'only')
 (list
   ('symbol', '9')
-  ('symbol', '5'))
-define)
+  ('symbol', '5')))
   * set:
   
   2
@@ -1999,18 +1963,13 @@
 (and
   (range
 ('symbol', '3')
-('symbol', '0')
-define)
+('symbol', '0'))
   (range
 ('symbol', 

D451: revset: remove order information from tree

[yuja (Yuya Nishihara)]

yuja added a comment.


  > 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 
https://phab.mercurial-scm.org/rHGc63cb2d10d6d5dc823853300f14fa9638bccfb68 then.

INLINE COMMENTS

> quark wrote in revset.py:892
> 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?

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

> quark wrote in revsetlang.py:435
> I thought since `_notpublic` is atomic and cannot be further split. 
> `preserveorder` does not matter here.

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

> quark wrote in revsetlang.py:459
> Not sure. I think it's similar to function argument (which we preserves order 
> by default in line 463).

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

REPOSITORY
  rHG Mercurial

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

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

D451: revset: remove order information from tree

[quark (Jun Wu)]

quark added a comment.


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

INLINE COMMENTS

> yuja wrote in revset.py:63
> `methods` is the table of operators, not functions, which wouldn't be modified
> by extensions. I'll drop this change.

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.

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

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?

> yuja wrote in revsetlang.py:354
> Split this back to unary/binary/ternary cases since I slightly prefer
> explicit handling of node tuples.

Ha, that was an older version of this patch.

> yuja wrote in revsetlang.py:435
> 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)

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

> yuja wrote in revsetlang.py:443
> The order matters. Try `(contains("glob:") & 2:0):1` for example.

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

> yuja wrote in revsetlang.py:459
> Perhaps this is `preserveorder` since `keyvalue` node isn't a
> standalone expression.

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

REPOSITORY
  rHG Mercurial

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

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

D451: revset: remove order information from tree

[yuja (Yuya Nishihara)]

yuja added a comment.


  > 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 
https://phab.mercurial-scm.org/rHGc63cb2d10d6d5dc823853300f14fa9638bccfb68 
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. :-)

INLINE COMMENTS

> revset.py:892
> +def _flipand(repo, subset, args, order):
> +"""Equivalent to ``y and x``, but faster when x is small"""
> +x, y = getargs(args, 2, 2, _("missing argument"))

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

REPOSITORY
  rHG Mercurial

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

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

D451: revset: remove order information from tree

[yuja (Yuya Nishihara)]

yuja added inline comments.

INLINE COMMENTS

> yuja wrote in revsetlang.py:435
> Perhaps this should keep the current `preserveorder` since
> `not public()` is fully replaced with `_notpublic()`.

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)

REPOSITORY
  rHG Mercurial

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

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

D451: revset: remove order information from tree

[yuja (Yuya Nishihara)]

yuja added a comment.


  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.

INLINE COMMENTS

> revset.py:63
> +# argument. Maintain compatibility to make developers life easier.
> +return methods[x[0]](repo, subset, *x[1:])
>  

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

> revset.py:141
>  def differenceset(repo, subset, x, y, order):
> -return getset(repo, subset, x) - getset(repo, subset, y)
> +return getset(repo, subset, x, order) - getset(repo, subset, y)
>  

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

> revset.py:161
>  def notset(repo, subset, x, order):
>  return subset - getset(repo, subset, x)
>  

This, too. added `anyorder` in flight.

> revset.py:892
> +def _flipand(repo, subset, args, order):
> +"""Equivalent to ``y and x``, but faster when x is small"""
> +x, y = getargs(args, 2, 2, _("missing argument"))

This should be undocumented. Changed to a comment.

> revsetlang.py:354
> +'dagrange', 'range', 'parent', 'ancestor', 'relation',
> +'subscript', 'relsubscript', 'list'}:
> +return (op,) + tuple(_analyze(y) for y in x[1:])

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

> revsetlang.py:435
> +newsym = ('func', ('symbol', '_notpublic'), None)
> +o = _optimize(newsym, not small, False)
>  return o[0], o[1]

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

> revsetlang.py:443
>  elif op in ('rangepre', 'rangepost', 'parentpost'):
> -o = _optimize(x[1], small)
> -order = x[2]
> -return o[0], (op, o[1], order)
> +o = _optimize(x[1], small, False)
> +return o[0], (op, o[1])

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

> revsetlang.py:459
>  elif op == 'keyvalue':
> -w, t = _optimize(x[2], small)
> +w, t = _optimize(x[2], small, True)
>  return w, (op, x[1], t)

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

REPOSITORY
  rHG Mercurial

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

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

D451: revset: remove order information from tree

[quark (Jun Wu)]

quark updated this revision to Diff 1317.

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST UPDATE
  https://phab.mercurial-scm.org/D451?vs=1108=1317

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

AFFECTED FILES
  mercurial/revset.py
  mercurial/revsetlang.py
  tests/test-revset.t

CHANGE DETAILS

diff --git a/tests/test-revset.t b/tests/test-revset.t
--- a/tests/test-revset.t
+++ b/tests/test-revset.t
@@ -166,8 +166,7 @@
 None)
   * optimized:
   (rangeall
-None
-define)
+None)
   * set:
   
   0
@@ -495,8 +494,7 @@
 ('symbol', 'foo')
 (func
   ('symbol', '_notpublic')
-  None
-  any))
+  None))
   hg: parse error: can't use a key-value pair in this context
   [255]
 
@@ -538,52 +536,43 @@
 (not
   (func
 ('symbol', 'public')
-None
-any)
-  define)
+None))
 ('symbol', 'generations')
-('symbol', '0')
-define)
+('symbol', '0'))
   * optimized:
   (relsubscript
 (func
   ('symbol', '_notpublic')
-  None
-  any)
+  None)
 ('symbol', 'generations')
-('symbol', '0')
-define)
+('symbol', '0'))
 
 resolution of subscript and relation-subscript ternary operators:
 
   $ hg debugrevspec -p analyzed 'tip[0]'
   * analyzed:
   (subscript
 ('symbol', 'tip')
-('symbol', '0')
-define)
+('symbol', '0'))
   hg: parse error: can't use a subscript in this context
   [255]
 
   $ hg debugrevspec -p analyzed 'tip#rel[0]'
   * analyzed:
   (relsubscript
 ('symbol', 'tip')
 ('symbol', 'rel')
-('symbol', '0')
-define)
+('symbol', '0'))
   hg: parse error: unknown identifier: rel
   [255]
 
   $ hg debugrevspec -p analyzed '(tip#rel)[0]'
   * analyzed:
   (subscript
 (relation
   ('symbol', 'tip')
-  ('symbol', 'rel')
-  define)
-('symbol', '0')
-define)
+  ('symbol', 'rel'))
+('symbol', '0'))
   hg: parse error: can't use a subscript in this context
   [255]
 
@@ -593,23 +582,19 @@
 (relsubscript
   ('symbol', 'tip')
   ('symbol', 'rel')
-  ('symbol', '0')
-  define)
-('symbol', '1')
-define)
+  ('symbol', '0'))
+('symbol', '1'))
   hg: parse error: can't use a subscript in this context
   [255]
 
   $ hg debugrevspec -p analyzed 'tip#rel0#rel1[1]'
   * analyzed:
   (relsubscript
 (relation
   ('symbol', 'tip')
-  ('symbol', 'rel0')
-  define)
+  ('symbol', 'rel0'))
 ('symbol', 'rel1')
-('symbol', '1')
-define)
+('symbol', '1'))
   hg: parse error: unknown identifier: rel1
   [255]
 
@@ -619,11 +604,9 @@
 (relsubscript
   ('symbol', 'tip')
   ('symbol', 'rel0')
-  ('symbol', '0')
-  define)
+  ('symbol', '0'))
 ('symbol', 'rel1')
-('symbol', '1')
-define)
+('symbol', '1'))
   hg: parse error: unknown identifier: rel1
   [255]
 
@@ -700,20 +683,15 @@
 (or
   (list
 ('symbol', '0')
-('symbol', '1'))
-  define)
+('symbol', '1')))
 (not
-  ('symbol', '1')
-  follow)
-define)
+  ('symbol', '1')))
   * optimized:
   (difference
 (func
   ('symbol', '_list')
-  ('string', '0\x001')
-  define)
-('symbol', '1')
-define)
+  ('string', '0\x001'))
+('symbol', '1'))
   0
 
   $ hg debugrevspec -p unknown '0'
@@ -733,18 +711,16 @@
   (and
 (func
   ('symbol', 'r3232')
-  None
-  define)
-('symbol', '2')
-define)
+  None)
+('symbol', '2'))
   * optimized:
-  (and
-('symbol', '2')
-(func
-  ('symbol', 'r3232')
-  None
-  define)
-define)
+  (func
+('symbol', '_flipand')
+(list
+  ('symbol', '2')
+  (func
+('symbol', 'r3232')
+None)))
   * analyzed set:
   
   * optimized set:
@@ -776,8 +752,7 @@
 None)
   * analyzed:
   (rangeall
-None
-define)
+None)
   * set:
   
   0
@@ -793,8 +768,7 @@
   $ try -p analyzed ':1'
   * analyzed:
   (rangepre
-('symbol', '1')
-define)
+('symbol', '1'))
   * set:
   
   0
@@ -805,9 +779,7 @@
 (or
   (list
 ('symbol', '1')
-('symbol', '2'))
-  define)
-define)
+('symbol', '2'
   * set:
   
   0
@@ -818,9 +790,7 @@
   (rangepre
 (and
   ('symbol', '1')
-  ('symbol', '2')
-  define)
-define)
+  ('symbol', '2')))
   * set:
   
 
@@ -1643,11 +1613,8 @@
 (difference
   (range
 ('symbol', '8')
-('symbol', '9')
-define)
-  ('symbol', '8')
-  define)
-define)
+('symbol', '9'))
+  ('symbol', '8')))
   * set:
   
   8
@@ -1663,8 +1630,7 @@
 ('symbol', 'only')
 (list
   ('symbol', '9')
-  ('symbol', '5'))
-define)
+  ('symbol', '5')))
   * set:
   
   2
@@ -1999,25 +1965,20 @@
 (and
   (range
 ('symbol', '3')
-('symbol', '0')
-

D451: revset: remove order information from tree

[quark (Jun Wu)]

quark added a comment.


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

REPOSITORY
  rHG Mercurial

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

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

D451: revset: remove order information from tree

[yuja (Yuya Nishihara)]

yuja added a comment.


  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.19 user 0.19 sys 0.00 (best of 51)
  >   mozilla-central % hg.new perfrevset 'sort(public() & 1:3)'
  >   ! wall 0.000188 comb 0.00 user 0.00 sys 0.00 (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.

REPOSITORY
  rHG Mercurial

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

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

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.19 user 0.19 sys 0.00 (best of 51)
mozilla-central % hg.new perfrevset 'sort(public() & 1:3)'
! wall 0.000188 comb 0.00 user 0.00 sys 0.00 (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 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
___
Mercurial-devel mailing list
Mercurial-devel@mercurial-scm.org
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel

D451: revset: remove order information from tree

[yuja (Yuya Nishihara)]

yuja added a comment.


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

REPOSITORY
  rHG Mercurial

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

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

D451: revset: remove order information from tree

[quark (Jun Wu)]

quark added a comment.


  In https://phab.mercurial-scm.org/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` be conservative? If it sees any
  predicate registered with the old API, Disable `anyorder` optimization and
  change it to `followorder` in runtime?
  
  > 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 have.
  
  > 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.
  
  Maybe more conservative? Disable "anyorder" optimization unless:
  
  1. A config flag was set
  2. There is no user of 3-tuple registrar predicate
  
  I really like optimizations.

REPOSITORY
  rHG Mercurial

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

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

D451: revset: remove order information from tree

[yuja (Yuya Nishihara)]

yuja added a comment.


  In https://phab.mercurial-scm.org/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.

REPOSITORY
  rHG Mercurial

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

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

D451: revset: remove order information from tree

[quark (Jun Wu)]

quark added inline comments.

INLINE COMMENTS

> quark wrote in revset.py:55
> 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.

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

REPOSITORY
  rHG Mercurial

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

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

D451: revset: remove order information from tree

[quark (Jun Wu)]

quark added inline comments.

INLINE COMMENTS

> martinvonz wrote in test-revset.t:2893
> Does that mean you'll remove the other.sort() in fullreposet.__and__?

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.

REPOSITORY
  rHG Mercurial

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

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

D451: revset: remove order information from tree

[martinvonz (Martin von Zweigbergk)]

martinvonz added a comment.


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

INLINE COMMENTS

> quark wrote in test-revset.t:2893
> This is caused by `fullreposet` having a default order. If we remove that, it 
> would be optimized to `` here.

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

REPOSITORY
  rHG Mercurial

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

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

D451: revset: remove order information from tree

[quark (Jun Wu)]

quark added a comment.


  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(...)"
  - pushed(): same as upstream()
  
  So there are only 2 users (upstream() and pushed()) which can benefit from 
"subset.filter(...)". All of the remaining cases could benefit from the simpler 
API and do not take the "subset" argument.

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

D451: revset: remove order information from tree

[quark (Jun Wu)]

quark added inline comments.

INLINE COMMENTS

> yuja wrote in revset.py:55
> Perhaps the default `order=defineorder` would be safer
> at this point.

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.

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

D451: revset: remove order information from tree

[quark (Jun Wu)]

quark added a comment.


  In https://phab.mercurial-scm.org/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 CODE  | ORDER=define | ORDER=follow |
  | INITSET=[0,1] | [0,1]| [0,1]|
  | INITSET=[1,0] | [1,0]| [1,0]|
  |
  
  
  
  | NEW CODE  | ORDER=define | ORDER=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. I think it's cleaner to 
migrate everything to "strong define" (for core revsets). Third party code 
needs change to follow that. If they do not need `subrepo` (ex. remotenames 
case), the new API could be easier to use.
  
  
  
  Unrelated to this series, I also 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`.

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

D451: revset: remove order information from tree

[yuja (Yuya Nishihara)]

yuja added a comment.


  In https://phab.mercurial-scm.org/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.

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

D451: revset: remove order information from tree

[yuja (Yuya Nishihara)]

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

D451: revset: remove order information from tree

[quark (Jun Wu)]

quark added inline comments.

INLINE COMMENTS

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

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

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

D451: revset: remove order information from tree

[quark (Jun Wu)]

quark added inline comments.

INLINE COMMENTS

> yuja wrote in revset.py:129
> `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.

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.

> yuja wrote in revset.py:893
> IIUC, `followorder` is correct because the ordering flags of
> `x and y` are flipped as if they were `y and x`.

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

> yuja wrote in revset.py:1830
> 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.

I can do that.

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

D451: revset: remove order information from tree

[quark (Jun Wu)]

quark added a comment.


  In https://phab.mercurial-scm.org/D451#7257, @yuja wrote:
  
  > 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.
  
  
  I think the current way is easier to hide bugs if more revset's `defineorder` 
is not the default (ascending). For example, I think `p1(X+Y)` could be 
naturally parsed as `p1(X) + p1(Y)` so it is not equivalent to `p1(Y+X)`. But 
the current code relies heavily on default ascending order and could not 
implement that easily. (This series does not change `p1` behavior, it's just a 
possibility)
  
  My understanding about `anyorder` is, we can random-shuffle `anyorder` sets 
without affecting correctness - I had tried that approach and it did reveal 
issues (like "contains" does not respect "defineorder"). But I found 
https://phab.mercurial-scm.org/D456 to be a stronger test about ordering issues.
  
  > So I'm against to bring more "any" ordering without noticeable
  >  performance win. Can you measure it?
  
  Probably hard to measure. I think with the new suggested code pattern (either 
taking `subset` and `order`, or take none of them), it's harder to implement 
wrong.

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

D451: revset: remove order information from tree

[yuja (Yuya Nishihara)]

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
___
Mercurial-devel mailing list
Mercurial-devel@mercurial-scm.org
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel

D451: revset: remove order information from tree

[quark (Jun Wu)]

quark updated this revision to Diff 1108.

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST UPDATE
  https://phab.mercurial-scm.org/D451?vs=1103=1108

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

AFFECTED FILES
  mercurial/revset.py
  mercurial/revsetlang.py
  tests/test-revset.t

CHANGE DETAILS

diff --git a/tests/test-revset.t b/tests/test-revset.t
--- a/tests/test-revset.t
+++ b/tests/test-revset.t
@@ -166,8 +166,7 @@
 None)
   * optimized:
   (rangeall
-None
-define)
+None)
   * set:
   
   0
@@ -495,8 +494,7 @@
 ('symbol', 'foo')
 (func
   ('symbol', '_notpublic')
-  None
-  any))
+  None))
   hg: parse error: can't use a key-value pair in this context
   [255]
 
@@ -538,52 +536,43 @@
 (not
   (func
 ('symbol', 'public')
-None
-any)
-  define)
+None))
 ('symbol', 'generations')
-('symbol', '0')
-define)
+('symbol', '0'))
   * optimized:
   (relsubscript
 (func
   ('symbol', '_notpublic')
-  None
-  any)
+  None)
 ('symbol', 'generations')
-('symbol', '0')
-define)
+('symbol', '0'))
 
 resolution of subscript and relation-subscript ternary operators:
 
   $ hg debugrevspec -p analyzed 'tip[0]'
   * analyzed:
   (subscript
 ('symbol', 'tip')
-('symbol', '0')
-define)
+('symbol', '0'))
   hg: parse error: can't use a subscript in this context
   [255]
 
   $ hg debugrevspec -p analyzed 'tip#rel[0]'
   * analyzed:
   (relsubscript
 ('symbol', 'tip')
 ('symbol', 'rel')
-('symbol', '0')
-define)
+('symbol', '0'))
   hg: parse error: unknown identifier: rel
   [255]
 
   $ hg debugrevspec -p analyzed '(tip#rel)[0]'
   * analyzed:
   (subscript
 (relation
   ('symbol', 'tip')
-  ('symbol', 'rel')
-  define)
-('symbol', '0')
-define)
+  ('symbol', 'rel'))
+('symbol', '0'))
   hg: parse error: can't use a subscript in this context
   [255]
 
@@ -593,23 +582,19 @@
 (relsubscript
   ('symbol', 'tip')
   ('symbol', 'rel')
-  ('symbol', '0')
-  define)
-('symbol', '1')
-define)
+  ('symbol', '0'))
+('symbol', '1'))
   hg: parse error: can't use a subscript in this context
   [255]
 
   $ hg debugrevspec -p analyzed 'tip#rel0#rel1[1]'
   * analyzed:
   (relsubscript
 (relation
   ('symbol', 'tip')
-  ('symbol', 'rel0')
-  define)
+  ('symbol', 'rel0'))
 ('symbol', 'rel1')
-('symbol', '1')
-define)
+('symbol', '1'))
   hg: parse error: unknown identifier: rel1
   [255]
 
@@ -619,11 +604,9 @@
 (relsubscript
   ('symbol', 'tip')
   ('symbol', 'rel0')
-  ('symbol', '0')
-  define)
+  ('symbol', '0'))
 ('symbol', 'rel1')
-('symbol', '1')
-define)
+('symbol', '1'))
   hg: parse error: unknown identifier: rel1
   [255]
 
@@ -700,20 +683,15 @@
 (or
   (list
 ('symbol', '0')
-('symbol', '1'))
-  define)
+('symbol', '1')))
 (not
-  ('symbol', '1')
-  follow)
-define)
+  ('symbol', '1')))
   * optimized:
   (difference
 (func
   ('symbol', '_list')
-  ('string', '0\x001')
-  define)
-('symbol', '1')
-define)
+  ('string', '0\x001'))
+('symbol', '1'))
   0
 
   $ hg debugrevspec -p unknown '0'
@@ -733,18 +711,16 @@
   (and
 (func
   ('symbol', 'r3232')
-  None
-  define)
-('symbol', '2')
-define)
+  None)
+('symbol', '2'))
   * optimized:
-  (and
-('symbol', '2')
-(func
-  ('symbol', 'r3232')
-  None
-  define)
-define)
+  (func
+('symbol', '_flipand')
+(list
+  ('symbol', '2')
+  (func
+('symbol', 'r3232')
+None)))
   * analyzed set:
   
   * optimized set:
@@ -776,8 +752,7 @@
 None)
   * analyzed:
   (rangeall
-None
-define)
+None)
   * set:
   
   0
@@ -793,8 +768,7 @@
   $ try -p analyzed ':1'
   * analyzed:
   (rangepre
-('symbol', '1')
-define)
+('symbol', '1'))
   * set:
   
   0
@@ -805,9 +779,7 @@
 (or
   (list
 ('symbol', '1')
-('symbol', '2'))
-  define)
-define)
+('symbol', '2'
   * set:
   
   0
@@ -818,9 +790,7 @@
   (rangepre
 (and
   ('symbol', '1')
-  ('symbol', '2')
-  define)
-define)
+  ('symbol', '2')))
   * set:
   
 
@@ -1643,11 +1613,8 @@
 (difference
   (range
 ('symbol', '8')
-('symbol', '9')
-define)
-  ('symbol', '8')
-  define)
-define)
+('symbol', '9'))
+  ('symbol', '8')))
   * set:
   
   8
@@ -1663,8 +1630,7 @@
 ('symbol', 'only')
 (list
   ('symbol', '9')
-  ('symbol', '5'))
-define)
+  ('symbol', '5')))
   * set:
   
   2
@@ -1999,18 +1965,13 @@
 (and
   (range
 ('symbol', '3')
-('symbol', '0')
-

D451: revset: remove order information from tree

[quark (Jun Wu)]

quark updated this revision to Diff 1103.

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST UPDATE
  https://phab.mercurial-scm.org/D451?vs=1100=1103

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

AFFECTED FILES
  mercurial/revset.py
  mercurial/revsetlang.py
  tests/test-revset.t

CHANGE DETAILS

diff --git a/tests/test-revset.t b/tests/test-revset.t
--- a/tests/test-revset.t
+++ b/tests/test-revset.t
@@ -166,8 +166,7 @@
 None)
   * optimized:
   (rangeall
-None
-define)
+None)
   * set:
   
   0
@@ -495,8 +494,7 @@
 ('symbol', 'foo')
 (func
   ('symbol', '_notpublic')
-  None
-  any))
+  None))
   hg: parse error: can't use a key-value pair in this context
   [255]
 
@@ -538,52 +536,43 @@
 (not
   (func
 ('symbol', 'public')
-None
-any)
-  define)
+None))
 ('symbol', 'generations')
-('symbol', '0')
-define)
+('symbol', '0'))
   * optimized:
   (relsubscript
 (func
   ('symbol', '_notpublic')
-  None
-  any)
+  None)
 ('symbol', 'generations')
-('symbol', '0')
-define)
+('symbol', '0'))
 
 resolution of subscript and relation-subscript ternary operators:
 
   $ hg debugrevspec -p analyzed 'tip[0]'
   * analyzed:
   (subscript
 ('symbol', 'tip')
-('symbol', '0')
-define)
+('symbol', '0'))
   hg: parse error: can't use a subscript in this context
   [255]
 
   $ hg debugrevspec -p analyzed 'tip#rel[0]'
   * analyzed:
   (relsubscript
 ('symbol', 'tip')
 ('symbol', 'rel')
-('symbol', '0')
-define)
+('symbol', '0'))
   hg: parse error: unknown identifier: rel
   [255]
 
   $ hg debugrevspec -p analyzed '(tip#rel)[0]'
   * analyzed:
   (subscript
 (relation
   ('symbol', 'tip')
-  ('symbol', 'rel')
-  define)
-('symbol', '0')
-define)
+  ('symbol', 'rel'))
+('symbol', '0'))
   hg: parse error: can't use a subscript in this context
   [255]
 
@@ -593,23 +582,19 @@
 (relsubscript
   ('symbol', 'tip')
   ('symbol', 'rel')
-  ('symbol', '0')
-  define)
-('symbol', '1')
-define)
+  ('symbol', '0'))
+('symbol', '1'))
   hg: parse error: can't use a subscript in this context
   [255]
 
   $ hg debugrevspec -p analyzed 'tip#rel0#rel1[1]'
   * analyzed:
   (relsubscript
 (relation
   ('symbol', 'tip')
-  ('symbol', 'rel0')
-  define)
+  ('symbol', 'rel0'))
 ('symbol', 'rel1')
-('symbol', '1')
-define)
+('symbol', '1'))
   hg: parse error: unknown identifier: rel1
   [255]
 
@@ -619,11 +604,9 @@
 (relsubscript
   ('symbol', 'tip')
   ('symbol', 'rel0')
-  ('symbol', '0')
-  define)
+  ('symbol', '0'))
 ('symbol', 'rel1')
-('symbol', '1')
-define)
+('symbol', '1'))
   hg: parse error: unknown identifier: rel1
   [255]
 
@@ -700,20 +683,15 @@
 (or
   (list
 ('symbol', '0')
-('symbol', '1'))
-  define)
+('symbol', '1')))
 (not
-  ('symbol', '1')
-  follow)
-define)
+  ('symbol', '1')))
   * optimized:
   (difference
 (func
   ('symbol', '_list')
-  ('string', '0\x001')
-  define)
-('symbol', '1')
-define)
+  ('string', '0\x001'))
+('symbol', '1'))
   0
 
   $ hg debugrevspec -p unknown '0'
@@ -733,18 +711,16 @@
   (and
 (func
   ('symbol', 'r3232')
-  None
-  define)
-('symbol', '2')
-define)
+  None)
+('symbol', '2'))
   * optimized:
-  (and
-('symbol', '2')
-(func
-  ('symbol', 'r3232')
-  None
-  define)
-define)
+  (func
+('symbol', '_flipand')
+(list
+  ('symbol', '2')
+  (func
+('symbol', 'r3232')
+None)))
   * analyzed set:
   
   * optimized set:
@@ -776,8 +752,7 @@
 None)
   * analyzed:
   (rangeall
-None
-define)
+None)
   * set:
   
   0
@@ -793,8 +768,7 @@
   $ try -p analyzed ':1'
   * analyzed:
   (rangepre
-('symbol', '1')
-define)
+('symbol', '1'))
   * set:
   
   0
@@ -805,9 +779,7 @@
 (or
   (list
 ('symbol', '1')
-('symbol', '2'))
-  define)
-define)
+('symbol', '2'
   * set:
   
   0
@@ -818,9 +790,7 @@
   (rangepre
 (and
   ('symbol', '1')
-  ('symbol', '2')
-  define)
-define)
+  ('symbol', '2')))
   * set:
   
 
@@ -1643,11 +1613,8 @@
 (difference
   (range
 ('symbol', '8')
-('symbol', '9')
-define)
-  ('symbol', '8')
-  define)
-define)
+('symbol', '9'))
+  ('symbol', '8')))
   * set:
   
   8
@@ -1663,8 +1630,7 @@
 ('symbol', 'only')
 (list
   ('symbol', '9')
-  ('symbol', '5'))
-define)
+  ('symbol', '5')))
   * set:
   
   2
@@ -1999,18 +1965,13 @@
 (and
   (range
 ('symbol', '3')
-('symbol', '0')
-

D451: revset: remove order information from tree

[quark (Jun Wu)]

quark updated this revision to Diff 1100.

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST UPDATE
  https://phab.mercurial-scm.org/D451?vs=1099=1100

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

AFFECTED FILES
  mercurial/revset.py
  mercurial/revsetlang.py
  tests/test-revset.t

CHANGE DETAILS

diff --git a/tests/test-revset.t b/tests/test-revset.t
--- a/tests/test-revset.t
+++ b/tests/test-revset.t
@@ -166,8 +166,7 @@
 None)
   * optimized:
   (rangeall
-None
-define)
+None)
   * set:
   
   0
@@ -495,8 +494,7 @@
 ('symbol', 'foo')
 (func
   ('symbol', '_notpublic')
-  None
-  any))
+  None))
   hg: parse error: can't use a key-value pair in this context
   [255]
 
@@ -538,52 +536,43 @@
 (not
   (func
 ('symbol', 'public')
-None
-any)
-  define)
+None))
 ('symbol', 'generations')
-('symbol', '0')
-define)
+('symbol', '0'))
   * optimized:
   (relsubscript
 (func
   ('symbol', '_notpublic')
-  None
-  any)
+  None)
 ('symbol', 'generations')
-('symbol', '0')
-define)
+('symbol', '0'))
 
 resolution of subscript and relation-subscript ternary operators:
 
   $ hg debugrevspec -p analyzed 'tip[0]'
   * analyzed:
   (subscript
 ('symbol', 'tip')
-('symbol', '0')
-define)
+('symbol', '0'))
   hg: parse error: can't use a subscript in this context
   [255]
 
   $ hg debugrevspec -p analyzed 'tip#rel[0]'
   * analyzed:
   (relsubscript
 ('symbol', 'tip')
 ('symbol', 'rel')
-('symbol', '0')
-define)
+('symbol', '0'))
   hg: parse error: unknown identifier: rel
   [255]
 
   $ hg debugrevspec -p analyzed '(tip#rel)[0]'
   * analyzed:
   (subscript
 (relation
   ('symbol', 'tip')
-  ('symbol', 'rel')
-  define)
-('symbol', '0')
-define)
+  ('symbol', 'rel'))
+('symbol', '0'))
   hg: parse error: can't use a subscript in this context
   [255]
 
@@ -593,23 +582,19 @@
 (relsubscript
   ('symbol', 'tip')
   ('symbol', 'rel')
-  ('symbol', '0')
-  define)
-('symbol', '1')
-define)
+  ('symbol', '0'))
+('symbol', '1'))
   hg: parse error: can't use a subscript in this context
   [255]
 
   $ hg debugrevspec -p analyzed 'tip#rel0#rel1[1]'
   * analyzed:
   (relsubscript
 (relation
   ('symbol', 'tip')
-  ('symbol', 'rel0')
-  define)
+  ('symbol', 'rel0'))
 ('symbol', 'rel1')
-('symbol', '1')
-define)
+('symbol', '1'))
   hg: parse error: unknown identifier: rel1
   [255]
 
@@ -619,11 +604,9 @@
 (relsubscript
   ('symbol', 'tip')
   ('symbol', 'rel0')
-  ('symbol', '0')
-  define)
+  ('symbol', '0'))
 ('symbol', 'rel1')
-('symbol', '1')
-define)
+('symbol', '1'))
   hg: parse error: unknown identifier: rel1
   [255]
 
@@ -700,20 +683,15 @@
 (or
   (list
 ('symbol', '0')
-('symbol', '1'))
-  define)
+('symbol', '1')))
 (not
-  ('symbol', '1')
-  follow)
-define)
+  ('symbol', '1')))
   * optimized:
   (difference
 (func
   ('symbol', '_list')
-  ('string', '0\x001')
-  define)
-('symbol', '1')
-define)
+  ('string', '0\x001'))
+('symbol', '1'))
   0
 
   $ hg debugrevspec -p unknown '0'
@@ -733,18 +711,16 @@
   (and
 (func
   ('symbol', 'r3232')
-  None
-  define)
-('symbol', '2')
-define)
+  None)
+('symbol', '2'))
   * optimized:
-  (and
-('symbol', '2')
-(func
-  ('symbol', 'r3232')
-  None
-  define)
-define)
+  (func
+('symbol', '_flipand')
+(list
+  ('symbol', '2')
+  (func
+('symbol', 'r3232')
+None)))
   * analyzed set:
   
   * optimized set:
@@ -776,8 +752,7 @@
 None)
   * analyzed:
   (rangeall
-None
-define)
+None)
   * set:
   
   0
@@ -793,8 +768,7 @@
   $ try -p analyzed ':1'
   * analyzed:
   (rangepre
-('symbol', '1')
-define)
+('symbol', '1'))
   * set:
   
   0
@@ -805,9 +779,7 @@
 (or
   (list
 ('symbol', '1')
-('symbol', '2'))
-  define)
-define)
+('symbol', '2'
   * set:
   
   0
@@ -818,9 +790,7 @@
   (rangepre
 (and
   ('symbol', '1')
-  ('symbol', '2')
-  define)
-define)
+  ('symbol', '2')))
   * set:
   
 
@@ -1643,11 +1613,8 @@
 (difference
   (range
 ('symbol', '8')
-('symbol', '9')
-define)
-  ('symbol', '8')
-  define)
-define)
+('symbol', '9'))
+  ('symbol', '8')))
   * set:
   
   8
@@ -1663,8 +1630,7 @@
 ('symbol', 'only')
 (list
   ('symbol', '9')
-  ('symbol', '5'))
-define)
+  ('symbol', '5')))
   * set:
   
   2
@@ -1999,18 +1965,13 @@
 (and
   (range
 ('symbol', '3')
-('symbol', '0')
-

D451: revset: remove order information from tree

[quark (Jun Wu)]

quark updated this revision to Diff 1099.

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST UPDATE
  https://phab.mercurial-scm.org/D451?vs=1098=1099

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

AFFECTED FILES
  mercurial/revset.py
  mercurial/revsetlang.py
  tests/test-revset.t

CHANGE DETAILS

diff --git a/tests/test-revset.t b/tests/test-revset.t
--- a/tests/test-revset.t
+++ b/tests/test-revset.t
@@ -166,8 +166,7 @@
 None)
   * optimized:
   (rangeall
-None
-define)
+None)
   * set:
   
   0
@@ -495,8 +494,7 @@
 ('symbol', 'foo')
 (func
   ('symbol', '_notpublic')
-  None
-  any))
+  None))
   hg: parse error: can't use a key-value pair in this context
   [255]
 
@@ -538,52 +536,43 @@
 (not
   (func
 ('symbol', 'public')
-None
-any)
-  define)
+None))
 ('symbol', 'generations')
-('symbol', '0')
-define)
+('symbol', '0'))
   * optimized:
   (relsubscript
 (func
   ('symbol', '_notpublic')
-  None
-  any)
+  None)
 ('symbol', 'generations')
-('symbol', '0')
-define)
+('symbol', '0'))
 
 resolution of subscript and relation-subscript ternary operators:
 
   $ hg debugrevspec -p analyzed 'tip[0]'
   * analyzed:
   (subscript
 ('symbol', 'tip')
-('symbol', '0')
-define)
+('symbol', '0'))
   hg: parse error: can't use a subscript in this context
   [255]
 
   $ hg debugrevspec -p analyzed 'tip#rel[0]'
   * analyzed:
   (relsubscript
 ('symbol', 'tip')
 ('symbol', 'rel')
-('symbol', '0')
-define)
+('symbol', '0'))
   hg: parse error: unknown identifier: rel
   [255]
 
   $ hg debugrevspec -p analyzed '(tip#rel)[0]'
   * analyzed:
   (subscript
 (relation
   ('symbol', 'tip')
-  ('symbol', 'rel')
-  define)
-('symbol', '0')
-define)
+  ('symbol', 'rel'))
+('symbol', '0'))
   hg: parse error: can't use a subscript in this context
   [255]
 
@@ -593,23 +582,19 @@
 (relsubscript
   ('symbol', 'tip')
   ('symbol', 'rel')
-  ('symbol', '0')
-  define)
-('symbol', '1')
-define)
+  ('symbol', '0'))
+('symbol', '1'))
   hg: parse error: can't use a subscript in this context
   [255]
 
   $ hg debugrevspec -p analyzed 'tip#rel0#rel1[1]'
   * analyzed:
   (relsubscript
 (relation
   ('symbol', 'tip')
-  ('symbol', 'rel0')
-  define)
+  ('symbol', 'rel0'))
 ('symbol', 'rel1')
-('symbol', '1')
-define)
+('symbol', '1'))
   hg: parse error: unknown identifier: rel1
   [255]
 
@@ -619,11 +604,9 @@
 (relsubscript
   ('symbol', 'tip')
   ('symbol', 'rel0')
-  ('symbol', '0')
-  define)
+  ('symbol', '0'))
 ('symbol', 'rel1')
-('symbol', '1')
-define)
+('symbol', '1'))
   hg: parse error: unknown identifier: rel1
   [255]
 
@@ -700,20 +683,15 @@
 (or
   (list
 ('symbol', '0')
-('symbol', '1'))
-  define)
+('symbol', '1')))
 (not
-  ('symbol', '1')
-  follow)
-define)
+  ('symbol', '1')))
   * optimized:
   (difference
 (func
   ('symbol', '_list')
-  ('string', '0\x001')
-  define)
-('symbol', '1')
-define)
+  ('string', '0\x001'))
+('symbol', '1'))
   0
 
   $ hg debugrevspec -p unknown '0'
@@ -733,18 +711,16 @@
   (and
 (func
   ('symbol', 'r3232')
-  None
-  define)
-('symbol', '2')
-define)
+  None)
+('symbol', '2'))
   * optimized:
-  (and
-('symbol', '2')
-(func
-  ('symbol', 'r3232')
-  None
-  define)
-define)
+  (func
+('symbol', '_flipand')
+(list
+  ('symbol', '2')
+  (func
+('symbol', 'r3232')
+None)))
   * analyzed set:
   
   * optimized set:
@@ -776,8 +752,7 @@
 None)
   * analyzed:
   (rangeall
-None
-define)
+None)
   * set:
   
   0
@@ -793,8 +768,7 @@
   $ try -p analyzed ':1'
   * analyzed:
   (rangepre
-('symbol', '1')
-define)
+('symbol', '1'))
   * set:
   
   0
@@ -805,9 +779,7 @@
 (or
   (list
 ('symbol', '1')
-('symbol', '2'))
-  define)
-define)
+('symbol', '2'
   * set:
   
   0
@@ -818,9 +790,7 @@
   (rangepre
 (and
   ('symbol', '1')
-  ('symbol', '2')
-  define)
-define)
+  ('symbol', '2')))
   * set:
   
 
@@ -1643,11 +1613,8 @@
 (difference
   (range
 ('symbol', '8')
-('symbol', '9')
-define)
-  ('symbol', '8')
-  define)
-define)
+('symbol', '9'))
+  ('symbol', '8')))
   * set:
   
   8
@@ -1663,8 +1630,7 @@
 ('symbol', 'only')
 (list
   ('symbol', '9')
-  ('symbol', '5'))
-define)
+  ('symbol', '5')))
   * set:
   
   2
@@ -1999,18 +1965,13 @@
 (and
   (range
 ('symbol', '3')
-('symbol', '0')
-

D451: revset: remove order information from tree

[yuja (Yuya Nishihara)]

yuja added a comment.


  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.

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

D451: revset: remove order information from tree

[quark (Jun Wu)]

quark added inline comments.

INLINE COMMENTS

> revset.py:58-61
> +# TODO migrate existing functions to accept "order", and maybe wrap
> +# registered functions automatically if they do not accept "order".
> +m = methods[x[0]]
> +return m(repo, subset, *x[1:], order=order)

TODO can be removed.

> revset.py:129
>includepath=True)
>  return subset & xs
>  

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.

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