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