D458: revset: correct ordering issues caused by `return subset.filter(...)`

quark (Jun Wu) phabricator at mercurial-scm.org
Sun Aug 20 22:00:55 UTC 2017


quark created this revision.
Herald added a subscriber: mercurial-devel.
Herald added a reviewer: hg-reviewers.

REVISION SUMMARY
  A lot of revset functions use `return subset.filter(...)` which enforces
  `followorder`. This patch changes them to use `intersect` so order is
  handled correctly.

REPOSITORY
  rHG Mercurial

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

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

CHANGE DETAILS

diff --git a/tests/test-revset-ordering.t b/tests/test-revset-ordering.t
--- a/tests/test-revset-ordering.t
+++ b/tests/test-revset-ordering.t
@@ -49,54 +49,39 @@
   ordering: defineorder not respected
 
   $ check 'adds("*")'
-  ordering: defineorder not respected
   $ check 'all()'
   $ check 'ancestors(E)'
   $ check 'author(test)'
-  ordering: defineorder not respected
   $ check 'bisect(untested)'
   $ check 'bookmark()'
   $ check 'branch(default)'
-  ordering: defineorder not respected
   $ check 'branchpoint()'
-  ordering: defineorder not respected
   $ check 'children(A)'
   $ check 'closed()'
-  ordering: defineorder not respected
   $ check 'contains(A)'
-  ordering: defineorder not respected
   $ check 'date("0 0")'
-  ordering: defineorder not respected
   $ check 'desc("")'
-  ordering: defineorder not respected
   $ check 'descendants(A)'
   $ check 'destination()'
-  ordering: defineorder not respected
   $ check 'draft()'
   $ check 'file("*")'
-  ordering: defineorder not respected
   $ check 'filelog("A")'
   $ check 'follow()'
   $ check 'grep("A")'
-  ordering: defineorder not respected
   $ check 'head()'
   $ check 'heads(E+C)'
   ordering: followorder not respected
   $ check 'merge()'
-  ordering: defineorder not respected
   $ check 'modifies("A")'
-  ordering: defineorder not respected
   $ check 'named("tags")'
   $ check 'origin()'
   $ check 'outgoing()'
   $ check 'p1(B+E+D)'
   $ check 'p2(B+E+D)'
   $ check 'parents(E)'
   $ check 'public()'
-  ordering: defineorder not respected
   $ check 'reverse(B+E+D)'
   $ check 'removes("A")'
-  ordering: defineorder not respected
   $ check 'roots(B+E+D)'
   $ check 'secret()'
   $ check 'sort(all())'
diff --git a/mercurial/revset.py b/mercurial/revset.py
--- a/mercurial/revset.py
+++ b/mercurial/revset.py
@@ -57,7 +57,7 @@
         raise error.ParseError(_("missing argument"))
     return methods[x[0]](repo, subset, *x[1:], order=order)
 
-def intersect(subset, newset, order):
+def intersect(subset, newset, order, **kwargs):
     """intersect two revsets with order preserved as requested
 
     If order is "followorder", preserve subset's order. If order is
@@ -69,7 +69,17 @@
         def myrevset(repo, subset, x, order):
             newset = ...  # a smartset that implements "myrevset"
             return revset.intersect(subset, newset, order)
+
+    If newset is a lambda, it will be applied to subset. kwargs will be passed
+    to "subset.filter". If order is "defineorder", the result will be sorted.
     """
+    # similar to subset.filter(....), but takes order into consideration
+    if util.safehasattr(newset, '__call__'):
+        s = subset.filter(newset, **kwargs)
+        if order == defineorder:
+            s.sort()
+        return s
+
     # some old code does not use smartset, normalize to smartset
     if not util.safehasattr(newset, 'isascending'):
         newset = subset & newset
@@ -270,16 +280,16 @@
     return baseset([destutil.destmerge(repo, sourceset=sourceset)])
 
 @predicate('adds(pattern)', safe=True)
-def adds(repo, subset, x):
+def adds(repo, subset, x, order):
     """Changesets that add a file matching pattern.
 
     The pattern without explicit kind like ``glob:`` is expected to be
     relative to the current directory and match against a file or a
     directory.
     """
     # i18n: "adds" is a keyword
     pat = getstring(x, _("adds requires a pattern"))
-    return checkstatus(repo, subset, pat, 1)
+    return checkstatus(repo, subset, pat, 1, order)
 
 @predicate('ancestor(*changeset)', safe=True)
 def ancestor(repo, subset, x):
@@ -387,14 +397,14 @@
     return subset & ps
 
 @predicate('author(string)', safe=True)
-def author(repo, subset, x):
+def author(repo, subset, x, order):
     """Alias for ``user(string)``.
     """
     # i18n: "author" is a keyword
     n = getstring(x, _("author requires a string"))
     kind, pattern, matcher = _substringmatcher(n, casesensitive=False)
-    return subset.filter(lambda x: matcher(repo[x].user()),
-                         condrepr=('<user %r>', n))
+    return intersect(subset, lambda x: matcher(repo[x].user()), order,
+                     condrepr=('<user %r>', n))
 
 @predicate('bisect(string)', safe=True)
 def bisect(repo, x):
@@ -455,7 +465,7 @@
     return bms
 
 @predicate('branch(string or set)', safe=True)
-def branch(repo, subset, x):
+def branch(repo, subset, x, order):
     """
     All changesets belonging to the given branch or the branches of the given
     changesets.
@@ -481,22 +491,22 @@
             # note: falls through to the revspec case if no branch with
             # this name exists and pattern kind is not specified explicitly
             if pattern in repo.branchmap():
-                return subset.filter(lambda r: matcher(getbranch(r)),
-                                     condrepr=('<branch %r>', b))
+                return intersect(subset, lambda r: matcher(getbranch(r)),
+                                 order, condrepr=('<branch %r>', b))
             if b.startswith('literal:'):
                 raise error.RepoLookupError(_("branch '%s' does not exist")
                                             % pattern)
         else:
-            return subset.filter(lambda r: matcher(getbranch(r)),
-                                 condrepr=('<branch %r>', b))
+            return intersect(subset, lambda r: matcher(getbranch(r)), order,
+                             condrepr=('<branch %r>', b))
 
     s = getset(repo, fullreposet(repo), x)
     b = set()
     for r in s:
         b.add(getbranch(r))
     c = s.__contains__
-    return subset.filter(lambda r: c(r) or getbranch(r) in b,
-                         condrepr=lambda: '<branch %r>' % sorted(b))
+    return intersect(subset, lambda r: c(r) or getbranch(r) in b, order,
+                     condrepr=lambda: '<branch %r>' % sorted(b))
 
 @predicate('bumped()', safe=True)
 def bumped(repo, subset, x):
@@ -530,7 +540,7 @@
         raise error.Abort(_("no bundle provided - specify with -R"))
     return bundlerevs
 
-def checkstatus(repo, subset, pat, field):
+def checkstatus(repo, subset, pat, field, order):
     hasset = matchmod.patkind(pat) == 'set'
 
     mcache = [None]
@@ -560,7 +570,8 @@
                 if m(f):
                     return True
 
-    return subset.filter(matches, condrepr=('<status[%r] %r>', field, pat))
+    return intersect(subset, matches, order,
+                     condrepr=('<status[%r] %r>', field, pat))
 
 def _children(repo, subset, parentset):
     if not parentset:
@@ -588,16 +599,16 @@
     return intersect(subset, cs, order)
 
 @predicate('closed()', safe=True)
-def closed(repo, subset, x):
+def closed(repo, subset, x, order):
     """Changeset is closed.
     """
     # i18n: "closed" is a keyword
     getargs(x, 0, 0, _("closed takes no arguments"))
-    return subset.filter(lambda r: repo[r].closesbranch(),
-                         condrepr='<branch closed>')
+    return intersect(subset, lambda r: repo[r].closesbranch(), order,
+                     condrepr='<branch closed>')
 
 @predicate('contains(pattern)')
-def contains(repo, subset, x):
+def contains(repo, subset, x, order):
     """The revision's manifest contains a file matching pattern (but might not
     modify it). See :hg:`help patterns` for information about file patterns.
 
@@ -621,10 +632,10 @@
                     return True
         return False
 
-    return subset.filter(matches, condrepr=('<contains %r>', pat))
+    return intersect(subset, matches, order, condrepr=('<contains %r>', pat))
 
 @predicate('converted([id])', safe=True)
-def converted(repo, subset, x):
+def converted(repo, subset, x, order):
     """Changesets converted from the given identifier in the old repository if
     present, or all converted changesets if no identifier is specified.
     """
@@ -643,21 +654,21 @@
         source = repo[r].extra().get('convert_revision', None)
         return source is not None and (rev is None or source.startswith(rev))
 
-    return subset.filter(lambda r: _matchvalue(r),
-                         condrepr=('<converted %r>', rev))
+    return intersect(subset, lambda r: _matchvalue(r), order,
+                     condrepr=('<converted %r>', rev))
 
 @predicate('date(interval)', safe=True)
-def date(repo, subset, x):
+def date(repo, subset, x, order):
     """Changesets within the interval, see :hg:`help dates`.
     """
     # i18n: "date" is a keyword
     ds = getstring(x, _("date requires a string"))
     dm = util.matchdate(ds)
-    return subset.filter(lambda x: dm(repo[x].date()[0]),
-                         condrepr=('<date %r>', ds))
+    return intersect(subset, lambda x: dm(repo[x].date()[0]), order,
+                     condrepr=('<date %r>', ds))
 
 @predicate('desc(string)', safe=True)
-def desc(repo, subset, x):
+def desc(repo, subset, x, order):
     """Search commit message for string. The match is case-insensitive.
 
     Pattern matching is supported for `string`. See
@@ -668,8 +679,8 @@
 
     kind, pattern, matcher = _substringmatcher(ds, casesensitive=False)
 
-    return subset.filter(lambda r: matcher(repo[r].description()),
-                         condrepr=('<desc %r>', ds))
+    return intersect(subset, lambda r: matcher(repo[r].description()), order,
+                     condrepr=('<desc %r>', ds))
 
 def _descendants(repo, x, followfirst=False, startdepth=None, stopdepth=None):
     roots = getset(repo, fullreposet(repo), x)
@@ -714,7 +725,7 @@
     return _descendants(repo, x, followfirst=True)
 
 @predicate('destination([set])', safe=True)
-def destination(repo, subset, x):
+def destination(repo, subset, x, order):
     """Changesets that were created by a graft, transplant or rebase operation,
     with the given revisions specified as the source.  Omitting the optional set
     is the same as passing all().
@@ -755,8 +766,8 @@
             r = src
             src = _getrevsource(repo, r)
 
-    return subset.filter(dests.__contains__,
-                         condrepr=lambda: '<destination %r>' % sorted(dests))
+    return intersect(subset, dests.__contains__, order,
+                     condrepr=lambda: '<destination %r>' % sorted(dests))
 
 @predicate('divergent()', safe=True)
 def divergent(repo, subset, x):
@@ -787,7 +798,7 @@
     return extincts
 
 @predicate('extra(label, [value])', safe=True)
-def extra(repo, subset, x):
+def extra(repo, subset, x, order):
     """Changesets with the given label in the extra metadata, with the given
     optional value.
 
@@ -813,8 +824,8 @@
         extra = repo[r].extra()
         return label in extra and (value is None or matcher(extra[label]))
 
-    return subset.filter(lambda r: _matchvalue(r),
-                         condrepr=('<extra[%r] %r>', label, value))
+    return intersect(subset, lambda r: _matchvalue(r), order,
+                     condrepr=('<extra[%r] %r>', label, value))
 
 @predicate('filelog(pattern)', safe=True)
 def filelog(repo, x):
@@ -1017,7 +1028,7 @@
     return spanset(repo)  # drop "null" if any
 
 @predicate('grep(regex)')
-def grep(repo, subset, x):
+def grep(repo, subset, x, order):
     """Like ``keyword(string)`` but accepts a regex. Use ``grep(r'...')``
     to ensure special escape characters are handled correctly. Unlike
     ``keyword(string)``, the match is case-sensitive.
@@ -1035,10 +1046,10 @@
                 return True
         return False
 
-    return subset.filter(matches, condrepr=('<grep %r>', gr.pattern))
+    return intersect(subset, matches, order, condrepr=('<grep %r>', gr.pattern))
 
 @predicate('_matchfiles', safe=True)
-def _matchfiles(repo, subset, x):
+def _matchfiles(repo, subset, x, order):
     # _matchfiles takes a revset list of prefixed arguments:
     #
     #   [p:foo, i:bar, x:baz]
@@ -1096,13 +1107,13 @@
                 return True
         return False
 
-    return subset.filter(matches,
-                         condrepr=('<matchfiles patterns=%r, include=%r '
-                                   'exclude=%r, default=%r, rev=%r>',
-                                   pats, inc, exc, default, rev))
+    return intersect(subset, matches, order,
+                     condrepr=('<matchfiles patterns=%r, include=%r '
+                               'exclude=%r, default=%r, rev=%r>',
+                               pats, inc, exc, default, rev))
 
 @predicate('file(pattern)', safe=True)
-def hasfile(repo, subset, x):
+def hasfile(repo, subset, x, order):
     """Changesets affecting files matched by pattern.
 
     For a faster but less accurate result, consider using ``filelog()``
@@ -1112,7 +1123,7 @@
     """
     # i18n: "file" is a keyword
     pat = getstring(x, _("file requires a pattern"))
-    return _matchfiles(repo, subset, ('string', 'p:' + pat))
+    return _matchfiles(repo, subset, ('string', 'p:' + pat), order)
 
 @predicate('head()', safe=True)
 def head(repo, x):
@@ -1144,7 +1155,7 @@
     return hiddenrevs
 
 @predicate('keyword(string)', safe=True)
-def keyword(repo, subset, x):
+def keyword(repo, subset, x, order):
     """Search commit message, user name, and names of changed files for
     string. The match is case-insensitive.
 
@@ -1159,7 +1170,7 @@
         return any(kw in encoding.lower(t)
                    for t in c.files() + [c.user(), c.description()])
 
-    return subset.filter(matches, condrepr=('<keyword %r>', kw))
+    return intersect(subset, matches, order, condrepr=('<keyword %r>', kw))
 
 @predicate('limit(set[, n[, offset]])', safe=True, takeorder=True)
 def limit(repo, subset, x, order):
@@ -1215,17 +1226,17 @@
     return baseset(datarepr=('<max %r, %r>', subset, os))
 
 @predicate('merge()', safe=True)
-def merge(repo, subset, x):
+def merge(repo, subset, x, order):
     """Changeset is a merge changeset.
     """
     # i18n: "merge" is a keyword
     getargs(x, 0, 0, _("merge takes no arguments"))
     cl = repo.changelog
-    return subset.filter(lambda r: cl.parentrevs(r)[1] != -1,
-                         condrepr='<merge>')
+    return intersect(subset, lambda r: cl.parentrevs(r)[1] != -1, order,
+                     condrepr='<merge>')
 
 @predicate('branchpoint()', safe=True)
-def branchpoint(repo, subset, x):
+def branchpoint(repo, subset, x, order):
     """Changesets with more than one child.
     """
     # i18n: "branchpoint" is a keyword
@@ -1241,8 +1252,8 @@
         for p in cl.parentrevs(r):
             if p >= baserev:
                 parentscount[p - baserev] += 1
-    return subset.filter(lambda r: parentscount[r - baserev] > 1,
-                         condrepr='<branchpoint>')
+    return intersect(subset, lambda r: parentscount[r - baserev] > 1, order,
+                     condrepr='<branchpoint>')
 
 @predicate('min(set)', safe=True)
 def minrev(repo, subset, x):
@@ -1260,16 +1271,16 @@
     return baseset(datarepr=('<min %r, %r>', subset, os))
 
 @predicate('modifies(pattern)', safe=True)
-def modifies(repo, subset, x):
+def modifies(repo, subset, x, order):
     """Changesets modifying files matched by pattern.
 
     The pattern without explicit kind like ``glob:`` is expected to be
     relative to the current directory and match against a file or a
     directory.
     """
     # i18n: "modifies" is a keyword
     pat = getstring(x, _("modifies requires a pattern"))
-    return checkstatus(repo, subset, pat, 0)
+    return checkstatus(repo, subset, pat, 0, order)
 
 @predicate('named(namespace)')
 def named(repo, x):
@@ -1570,15 +1581,15 @@
     return _phase(repo, phases.draft, phases.secret)
 
 @predicate('public()', safe=True)
-def public(repo, subset, x):
+def public(repo, subset, x, order):
     """Changeset in public phase."""
     # i18n: "public" is a keyword
     getargs(x, 0, 0, _("public takes no arguments"))
     phase = repo._phasecache.phase
     target = phases.public
     condition = lambda r: phase(repo, r) == target
-    return subset.filter(condition, condrepr=('<phase %r>', target),
-                         cache=False)
+    return intersect(subset, condition, order,
+                     condrepr=('<phase %r>', target), cache=False)
 
 @predicate('remote([id [,path]])', safe=False)
 def remote(repo, subset, x):
@@ -1616,16 +1627,16 @@
     return baseset()
 
 @predicate('removes(pattern)', safe=True)
-def removes(repo, subset, x):
+def removes(repo, subset, x, order):
     """Changesets which remove files matching pattern.
 
     The pattern without explicit kind like ``glob:`` is expected to be
     relative to the current directory and match against a file or a
     directory.
     """
     # i18n: "removes" is a keyword
     pat = getstring(x, _("removes requires a pattern"))
-    return checkstatus(repo, subset, pat, 2)
+    return checkstatus(repo, subset, pat, 2, order)
 
 @predicate('rev(number)', safe=True)
 def rev(repo, x):
@@ -1644,7 +1655,7 @@
     return baseset([l])
 
 @predicate('matching(revision [, field])', safe=True)
-def matching(repo, subset, x):
+def matching(repo, subset, x, order):
     """Changesets in which a given set of fields match the set of fields in the
     selected revision or set.
 
@@ -1753,7 +1764,8 @@
                 return True
         return False
 
-    return subset.filter(matches, condrepr=('<matching%r %r>', fields, revs))
+    return intersect(subset, matches, order,
+                     condrepr=('<matching%r %r>', fields, revs))
 
 @predicate('reverse(set)', safe=True, takeorder=True)
 def reverse(repo, subset, x, order):
@@ -1878,7 +1890,7 @@
     return baseset([c.rev() for c in ctxs])
 
 @predicate('subrepo([pattern])')
-def subrepo(repo, subset, x):
+def subrepo(repo, subset, x, order):
     """Changesets that add, modify or remove the given subrepo.  If no subrepo
     pattern is named, any subrepo changes are returned.
     """
@@ -1919,7 +1931,7 @@
 
         return False
 
-    return subset.filter(matches, condrepr=('<subrepo %r>', pat))
+    return intersect(subset, matches, order, condrepr=('<subrepo %r>', pat))
 
 def _mapbynodefunc(repo, s, f):
     """(repo, smartset, [node] -> [node]) -> smartset
@@ -2005,13 +2017,13 @@
 
 
 @predicate('user(string)', safe=True)
-def user(repo, subset, x):
+def user(repo, subset, x, order):
     """User name contains string. The match is case-insensitive.
 
     Pattern matching is supported for `string`. See
     :hg:`help revisions.patterns`.
     """
-    return author(repo, subset, x)
+    return author(repo, subset, x, order)
 
 @predicate('wdir()', safe=True)
 def wdir(repo, subset, x):



To: quark, #hg-reviewers
Cc: mercurial-devel


More information about the Mercurial-devel mailing list