Patchwork [5,of,5] revset: remove hacky sorting in fullreposet.__and__

login
register
mail settings
Submitter via Mercurial-devel
Date June 24, 2016, 10:10 p.m.
Message ID <abcf4b35dbdcee9a721b.1466806217@martinvonz.mtv.corp.google.com>
Download mbox | patch
Permalink /patch/15611/
State Deferred
Headers show

Comments

via Mercurial-devel - June 24, 2016, 10:10 p.m.
# HG changeset patch
# User Martin von Zweigbergk <martinvonz@google.com>
# Date 1466804411 25200
#      Fri Jun 24 14:40:11 2016 -0700
# Node ID abcf4b35dbdcee9a721b49ae2bb815a313baec57
# Parent  87950985ede58fc03fb4a207cd45f197f1690cb8
revset: remove hacky sorting in fullreposet.__and__

We don't seem to have any more functions depending on the sorting done
in fullreposte.__and__, so let's remove it and open up for fixing
other bugs caused by it. Specifically, it prevents rangeset() from
doing "subset & r" as it should.
Yuya Nishihara - June 25, 2016, 5:09 a.m.
On Fri, 24 Jun 2016 15:10:17 -0700, Martin von Zweigbergk via Mercurial-devel wrote:
> # HG changeset patch
> # User Martin von Zweigbergk <martinvonz@google.com>
> # Date 1466804411 25200
> #      Fri Jun 24 14:40:11 2016 -0700
> # Node ID abcf4b35dbdcee9a721b49ae2bb815a313baec57
> # Parent  87950985ede58fc03fb4a207cd45f197f1690cb8
> revset: remove hacky sorting in fullreposet.__and__
> 
> We don't seem to have any more functions depending on the sorting done
> in fullreposte.__and__, so let's remove it and open up for fixing
> other bugs caused by it.

_descendants() seems to do 'subset &' to constrain the order or something.

https://selenic.com/repo/hg/file/fbe380dc227a/mercurial/revset.py#l807

> Specifically, it prevents rangeset() from
> doing "subset & r" as it should.
> 
> diff -r 87950985ede5 -r abcf4b35dbdc mercurial/revset.py
> --- a/mercurial/revset.py	Thu Jun 23 12:37:09 2016 -0700
> +++ b/mercurial/revset.py	Fri Jun 24 14:40:11 2016 -0700
> @@ -3619,18 +3619,8 @@
>              # object.
>              other = baseset(other - self._hiddenrevs)
>  
> -        # XXX As fullreposet is also used as bootstrap, this is wrong.
> -        #
> -        # With a giveme312() revset returning [3,1,2], this makes
> -        #   'hg log -r "giveme312()"' -> 1, 2, 3 (wrong)
> -        # We cannot just drop it because other usage still need to sort it:
> -        #   'hg log -r "all() and giveme312()"' -> 1, 2, 3 (right)
> -        #
> -        # There is also some faulty revset implementations that rely on it
> -        # (eg: children as of its state in e8075329c5fb)
> -        #
> -        # When we fix the two points above we can move this into the if clause
> -        other.sort(reverse=self.isdescending())
> +        # As fullreposet is used as bootstrap, ignore its ordering and take
> +        # that of other.

Well, I think I proposed this idea before, but now I'm skeptical about it.

First, it forces us to sort the "other" set in ascending order unless it has
its own order. That seems too picky to be right and we would need some amount
of tests to cover such cases.

Second, it prevents optimize() changing the evaluation order. Since filtering
conditions of revset functions do not depend on their inputs, we can reorder
nested expressions as if we were handling unordered set and pick order from
the outermost expression. For example, given 'A & B', 'b(order_by_A(a()))'
can be reordered as 'order_by_A(a(b()))'. IMHO, this is simpler and more
efficient than checking if optimize() can apply permutation.

Patch

diff -r 87950985ede5 -r abcf4b35dbdc mercurial/revset.py
--- a/mercurial/revset.py	Thu Jun 23 12:37:09 2016 -0700
+++ b/mercurial/revset.py	Fri Jun 24 14:40:11 2016 -0700
@@ -3619,18 +3619,8 @@ 
             # object.
             other = baseset(other - self._hiddenrevs)
 
-        # XXX As fullreposet is also used as bootstrap, this is wrong.
-        #
-        # With a giveme312() revset returning [3,1,2], this makes
-        #   'hg log -r "giveme312()"' -> 1, 2, 3 (wrong)
-        # We cannot just drop it because other usage still need to sort it:
-        #   'hg log -r "all() and giveme312()"' -> 1, 2, 3 (right)
-        #
-        # There is also some faulty revset implementations that rely on it
-        # (eg: children as of its state in e8075329c5fb)
-        #
-        # When we fix the two points above we can move this into the if clause
-        other.sort(reverse=self.isdescending())
+        # As fullreposet is used as bootstrap, ignore its ordering and take
+        # that of other.
         return other
 
 def prettyformatset(revs):
diff -r 87950985ede5 -r abcf4b35dbdc tests/test-revset.t
--- a/tests/test-revset.t	Thu Jun 23 12:37:09 2016 -0700
+++ b/tests/test-revset.t	Fri Jun 24 14:40:11 2016 -0700
@@ -181,7 +181,7 @@ 
     ('symbol', '3')
     ('symbol', '6'))
   * set:
-  <baseset+ [3, 5, 6]>
+  <baseset [3, 5, 6]>
   3
   5
   6
@@ -1707,7 +1707,7 @@ 
       ('symbol', '4')))
   * set:
   <addset
-    <baseset- [1, 3, 5]>,
+    <baseset [5, 3, 1]>,
     <generatorset+>>
   5
   3
@@ -1730,7 +1730,7 @@ 
   * set:
   <addset+
     <generatorset+>,
-    <baseset- [1, 3, 5]>>
+    <baseset [5, 3, 1]>>
   0
   1
   2