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
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
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,
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
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
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
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
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()'
*
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
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
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
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
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?
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
34 matches
Mail list logo
