Patchwork [STABLE,V2] revset: evaluate sub expressions correctly (issue3775)

login
register
mail settings
Submitter Katsunori FUJIWARA
Date Jan. 23, 2013, 2:20 p.m.
Message ID <d84ac6f848203629caa3.1358950843@juju>
Download mbox | patch
Permalink /patch/718/
State Accepted
Commit 692cbda1eb50fe30c70792cb1e9380b28769467c
Headers show

Comments

Katsunori FUJIWARA - Jan. 23, 2013, 2:20 p.m.
# HG changeset patch
# User FUJIWARA Katsunori <foozy@lares.dti.ne.jp>
# Date 1358949175 -32400
# Branch stable
# Node ID d84ac6f848203629caa30003ff2b5c3028a05670
# Parent  a2e9fe93d9eaba4ad2727d6bcde3afe3985ac00b
revset: evaluate sub expressions correctly (issue3775)

Before this patch, sub expression may return unexpected result, if it
is joined with another expression by "or":

  - "^"/parentspec():
    "R or R^1" is not equal to "R^1 or R". the former returns only "R".

  - "~"/ancestorspec():
    "R or R~1" is not equal to "R~1 or R". the former returns only "R".

  - ":"/rangeset():
    "10 or (10 or 15):" is not equal to "(10 or 15): or 10". the
    former returns only 10 and 15 or grater (11 to 14 are not
    included).

In "or"-ed expression "A or B", the "subset" passed to evaluation of
"B" doesn't contain revisions gotten from evaluation of "A", for
efficiency.

In the other hand, "stringset()" fails to look corresponding revision
for specified string/symbol up, if "subset" doesn't contain that
revision.

So, predicates looking revisions up indirectly should evaluate sub
expressions of themselves not with passed "subset" but with "entire
revisions in the repository", to prevent "stringset()" from unexpected
failing to look symbols in them up.

But predicates in above example don't so. For example, in the case of
"R or R^1":

  1. "R^1" is evaluated with "subset" containing revisions other than
     "R", because "R" is already gotten by the former of "or"-ed
     expressions

  2. "parentspec()" evaluates "R" of "R^1" with such "subset"

  3. "stringset()" fails to look "R" up, because "R" is not contained
     in "subset"

  4. so, evaluation of "R^1" returns no revision

This patch evaluates sub expressions for predicates above with "entire
revisions in the repository".
Matt Mackall - Jan. 23, 2013, 11:41 p.m.
On Wed, 2013-01-23 at 23:20 +0900, FUJIWARA Katsunori wrote:
> # HG changeset patch
> # User FUJIWARA Katsunori <foozy@lares.dti.ne.jp>
> # Date 1358949175 -32400
> # Branch stable
> # Node ID d84ac6f848203629caa30003ff2b5c3028a05670
> # Parent  a2e9fe93d9eaba4ad2727d6bcde3afe3985ac00b
> revset: evaluate sub expressions correctly (issue3775)

Ok, I think I'm convinced this is correct. My understanding of the issue
here is:

a) the optimizer tries to arrange that given (x or y), we arrange for
the more expensive clause to be on the rigt
b) orset tries to keep the expensive clause from doing extra work by
filtering out the known-good revisions
c) rangeset and a few others were using subset to filter their _input
domain_ rather than their _output range_

(In the future, we're going to want to encode which predicates have a
direct relation between their range and domain to allow us to build
iterators.)

This is queued for stable, thanks.

Patch

diff --git a/mercurial/revset.py b/mercurial/revset.py
--- a/mercurial/revset.py
+++ b/mercurial/revset.py
@@ -223,13 +223,9 @@ 
     return stringset(repo, subset, x)
 
 def rangeset(repo, subset, x, y):
-    m = getset(repo, subset, x)
-    if not m:
-        m = getset(repo, list(repo), x)
-
-    n = getset(repo, subset, y)
-    if not n:
-        n = getset(repo, list(repo), y)
+    cl = repo.changelog
+    m = getset(repo, cl, x)
+    n = getset(repo, cl, y)
 
     if not m or not n:
         return []
@@ -326,7 +322,7 @@ 
         raise error.ParseError(_("~ expects a number"))
     ps = set()
     cl = repo.changelog
-    for r in getset(repo, subset, x):
+    for r in getset(repo, cl, x):
         for i in range(n):
             r = cl.parentrevs(r)[0]
         ps.add(r)
@@ -1139,7 +1135,7 @@ 
         raise error.ParseError(_("^ expects a number 0, 1, or 2"))
     ps = set()
     cl = repo.changelog
-    for r in getset(repo, subset, x):
+    for r in getset(repo, cl, x):
         if n == 0:
             ps.add(r)
         elif n == 1:
diff --git a/tests/test-revset.t b/tests/test-revset.t
--- a/tests/test-revset.t
+++ b/tests/test-revset.t
@@ -758,6 +758,37 @@ 
   $ hg log -r "${ISSUE3669_TIP}^" --template '{rev}\n'
   8
 
+test or-ed indirect predicates (issue3775)
+
+  $ log '6 or 6^1' | sort
+  5
+  6
+  $ log '6^1 or 6' | sort
+  5
+  6
+  $ log '4 or 4~1' | sort
+  2
+  4
+  $ log '4~1 or 4' | sort
+  2
+  4
+  $ log '(0 or 2):(4 or 6) or 0 or 6' | sort
+  0
+  1
+  2
+  3
+  4
+  5
+  6
+  $ log '0 or 6 or (0 or 2):(4 or 6)' | sort
+  0
+  1
+  2
+  3
+  4
+  5
+  6
+
 tests for 'remote()' predicate:
 #.  (csets in remote) (id)            (remote)
 1.  less than local   current branch  "default"