Patchwork [V2] revset: support ranges in #generations relation

login
register
mail settings
Submitter Anton Shestakov
Date Jan. 16, 2019, 10:29 a.m.
Message ID <039a0d0d913afac1464b.1547634597@neuro>
Download mbox | patch
Permalink /patch/37787/
State Superseded
Headers show

Comments

Anton Shestakov - Jan. 16, 2019, 10:29 a.m.
# HG changeset patch
# User Anton Shestakov <av6@dwimlabs.net>
# Date 1547564229 -28800
#      Tue Jan 15 22:57:09 2019 +0800
# Node ID 039a0d0d913afac1464bfc1ea8a49b22e803b081
# Parent  8aca89a694d4bd7d25877b3652fb83e187ea1802
revset: support ranges in #generations relation
Yuya Nishihara - Jan. 16, 2019, 2:18 p.m.
On Wed, 16 Jan 2019 18:29:57 +0800, Anton Shestakov wrote:
> # HG changeset patch
> # User Anton Shestakov <av6@dwimlabs.net>
> # Date 1547564229 -28800
> #      Tue Jan 15 22:57:09 2019 +0800
> # Node ID 039a0d0d913afac1464bfc1ea8a49b22e803b081
> # Parent  8aca89a694d4bd7d25877b3652fb83e187ea1802
> revset: support ranges in #generations relation

The idea sounds good to me.

> +def generationsrel(repo, subset, x, rel, a, b, order):
> +    # TODO: rewrite tests, and drop startdepth argument from ancestors() and
> +    # descendants() predicates
> +    if a < 0 or a is None:

Need to test "is None" first. "None < 0" doesn't work on Py3.

> +        if b >= 0 or b is None:
> +            startdepth = 1

Maybe startdepth = 0?

> +        else:
> +            startdepth = -b + 1
> +        if a is None:
> +            stopdepth = None
> +        else:
> +            stopdepth = -a + 1
> +        aset = _ancestors(repo, subset, x, False, startdepth, stopdepth)
>      else:
> -        return _descendants(repo, subset, x, startdepth=n, stopdepth=n + 1)
> +        aset = baseset()
> +
> +    if b >= 0 or b is None:
> +        if a < 0 or a is None:
> +            startdepth = 0
> +        else:
> +            startdepth = a
> +        stopdepth = b
> +        dset = _descendants(repo, subset, x, False, startdepth, stopdepth)
> +    else:
> +        dset = baseset()

It's probably better to split the function of the range resolution, and add some
doctests. And it might be actually simpler to write down all conditions
({[:], [a:], [:b], [a:b]} x {a, b <= 0, a, b > 0, sgn(a) != sgn(b)}) separately.

> +    return aset + dset

Nit: "aset" (ancestors set) here is confusing since we have another "a".

>  def relsubscriptset(repo, subset, x, y, z, order):
>      # this is pretty basic implementation of 'x#y[z]' operator, still
>      # experimental so undocumented. see the wiki for further ideas.
>      # https://www.mercurial-scm.org/wiki/RevsetOperatorPlan
>      rel = getsymbol(y)
> -    n = getinteger(z, _("relation subscript must be an integer"))
> +    try:
> +        a, b = getrange(z, '')
> +    except error.ParseError:
> +        a = getinteger(z, _("relation subscript must be an integer"))
> +        b = a + 1

Maybe a = b. Unlike python range, a:b in revset is both inclusive.
Anton Shestakov - Jan. 17, 2019, 8:22 a.m.
On Wed, 16 Jan 2019 23:18:38 +0900
Yuya Nishihara <yuya@tcha.org> wrote:

> On Wed, 16 Jan 2019 18:29:57 +0800, Anton Shestakov wrote:
> > # HG changeset patch
> > # User Anton Shestakov <av6@dwimlabs.net>
> > # Date 1547564229 -28800
> > #      Tue Jan 15 22:57:09 2019 +0800
> > # Node ID 039a0d0d913afac1464bfc1ea8a49b22e803b081
> > # Parent  8aca89a694d4bd7d25877b3652fb83e187ea1802
> > revset: support ranges in #generations relation
> 
> The idea sounds good to me.
> 
> > +def generationsrel(repo, subset, x, rel, a, b, order):
> > +    # TODO: rewrite tests, and drop startdepth argument from ancestors() and
> > +    # descendants() predicates
> > +    if a < 0 or a is None:
> 
> Need to test "is None" first. "None < 0" doesn't work on Py3.

Right, fixed.

> > +        if b >= 0 or b is None:
> > +            startdepth = 1
> 
> Maybe startdepth = 0?

Given foo#g[-1:1], startdepth=0 everywhere would include foo in both
aset and dset, so I do startdepth=1 in one case.

> > +        else:
> > +            startdepth = -b + 1
> > +        if a is None:
> > +            stopdepth = None
> > +        else:
> > +            stopdepth = -a + 1
> > +        aset = _ancestors(repo, subset, x, False, startdepth, stopdepth)
> >      else:
> > -        return _descendants(repo, subset, x, startdepth=n, stopdepth=n + 1)
> > +        aset = baseset()
> > +
> > +    if b >= 0 or b is None:
> > +        if a < 0 or a is None:
> > +            startdepth = 0
> > +        else:
> > +            startdepth = a
> > +        stopdepth = b
> > +        dset = _descendants(repo, subset, x, False, startdepth, stopdepth)
> > +    else:
> > +        dset = baseset()
> 
> It's probably better to split the function of the range resolution, and add some
> doctests. And it might be actually simpler to write down all conditions
> ({[:], [a:], [:b], [a:b]} x {a, b <= 0, a, b > 0, sgn(a) != sgn(b)}) separately.

I've sent V3 and V4, they handle this feedback item somewhat
differently. Pick whatever you like best, and we'll go from there.

> > +    return aset + dset
> 
> Nit: "aset" (ancestors set) here is confusing since we have another "a".

That's handled in V4 (but not in V3). Naming in V3 in general is not
perfect.

> >  def relsubscriptset(repo, subset, x, y, z, order):
> >      # this is pretty basic implementation of 'x#y[z]' operator, still
> >      # experimental so undocumented. see the wiki for further ideas.
> >      # https://www.mercurial-scm.org/wiki/RevsetOperatorPlan
> >      rel = getsymbol(y)
> > -    n = getinteger(z, _("relation subscript must be an integer"))
> > +    try:
> > +        a, b = getrange(z, '')
> > +    except error.ParseError:
> > +        a = getinteger(z, _("relation subscript must be an integer"))
> > +        b = a + 1
> 
> Maybe a = b. Unlike python range, a:b in revset is both inclusive.

Yeah, I don't know what's best here: make them work as python ranges or
as revset ranges. I can sort of see arguments for both sides, but I've
went with revset-like behavior in V3/V4.

Thank you for the review!
Yuya Nishihara - Jan. 17, 2019, 10:59 a.m.
On Thu, 17 Jan 2019 16:22:05 +0800, Anton Shestakov wrote:
> On Wed, 16 Jan 2019 23:18:38 +0900
> Yuya Nishihara <yuya@tcha.org> wrote:
> 
> > On Wed, 16 Jan 2019 18:29:57 +0800, Anton Shestakov wrote:
> > > # HG changeset patch
> > > # User Anton Shestakov <av6@dwimlabs.net>
> > > # Date 1547564229 -28800
> > > #      Tue Jan 15 22:57:09 2019 +0800
> > > # Node ID 039a0d0d913afac1464bfc1ea8a49b22e803b081
> > > # Parent  8aca89a694d4bd7d25877b3652fb83e187ea1802
> > > revset: support ranges in #generations relation
> > 
> > The idea sounds good to me.
> > 
> > > +def generationsrel(repo, subset, x, rel, a, b, order):
> > > +    # TODO: rewrite tests, and drop startdepth argument from ancestors() and
> > > +    # descendants() predicates
> > > +    if a < 0 or a is None:
> > 
> > Need to test "is None" first. "None < 0" doesn't work on Py3.
> 
> Right, fixed.
> 
> > > +        if b >= 0 or b is None:
> > > +            startdepth = 1
> > 
> > Maybe startdepth = 0?
> 
> Given foo#g[-1:1], startdepth=0 everywhere would include foo in both
> aset and dset, so I do startdepth=1 in one case.

That doesn't matter since duplicated revisions will be filtered out by
smartset.addset.

Patch

diff --git a/mercurial/revset.py b/mercurial/revset.py
--- a/mercurial/revset.py
+++ b/mercurial/revset.py
@@ -225,24 +225,54 @@  def notset(repo, subset, x, order):
 def relationset(repo, subset, x, y, order):
     raise error.ParseError(_("can't use a relation in this context"))
 
-def generationsrel(repo, subset, x, rel, n, order):
-    # TODO: support range, rewrite tests, and drop startdepth argument
-    # from ancestors() and descendants() predicates
-    if n <= 0:
-        n = -n
-        return _ancestors(repo, subset, x, startdepth=n, stopdepth=n + 1)
+def generationsrel(repo, subset, x, rel, a, b, order):
+    # TODO: rewrite tests, and drop startdepth argument from ancestors() and
+    # descendants() predicates
+    if a < 0 or a is None:
+        if b >= 0 or b is None:
+            startdepth = 1
+        else:
+            startdepth = -b + 1
+        if a is None:
+            stopdepth = None
+        else:
+            stopdepth = -a + 1
+        aset = _ancestors(repo, subset, x, False, startdepth, stopdepth)
     else:
-        return _descendants(repo, subset, x, startdepth=n, stopdepth=n + 1)
+        aset = baseset()
+
+    if b >= 0 or b is None:
+        if a < 0 or a is None:
+            startdepth = 0
+        else:
+            startdepth = a
+        stopdepth = b
+        dset = _descendants(repo, subset, x, False, startdepth, stopdepth)
+    else:
+        dset = baseset()
+
+    return aset + dset
 
 def relsubscriptset(repo, subset, x, y, z, order):
     # this is pretty basic implementation of 'x#y[z]' operator, still
     # experimental so undocumented. see the wiki for further ideas.
     # https://www.mercurial-scm.org/wiki/RevsetOperatorPlan
     rel = getsymbol(y)
-    n = getinteger(z, _("relation subscript must be an integer"))
+    try:
+        a, b = getrange(z, '')
+    except error.ParseError:
+        a = getinteger(z, _("relation subscript must be an integer"))
+        b = a + 1
+    else:
+        def getbound(i):
+            if i is None:
+                return None
+            msg = _("relation subscript bounds must be integers")
+            return getinteger(i, msg)
+        a, b = [getbound(i) for i in (a, b)]
 
     if rel in subscriptrelations:
-        return subscriptrelations[rel](repo, subset, x, rel, n, order)
+        return subscriptrelations[rel](repo, subset, x, rel, a, b, order)
 
     relnames = [r for r in subscriptrelations.keys() if len(r) > 1]
     raise error.UnknownIdentifier(rel, relnames)
diff --git a/tests/test-revset.t b/tests/test-revset.t
--- a/tests/test-revset.t
+++ b/tests/test-revset.t
@@ -648,6 +648,9 @@  parse errors of relation, subscript and 
   $ hg debugrevspec '.#generations[1-2]'
   hg: parse error: relation subscript must be an integer
   [255]
+  $ hg debugrevspec '.#generations[foo:bar]'
+  hg: parse error: relation subscript bounds must be integers
+  [255]
 
 suggested relations
 
@@ -1274,6 +1277,29 @@  test ancestors/descendants relation subs
   $ log '.#g[(-1)]'
   8
 
+  $ log '6#generations[0:2]'
+  6
+  7
+  $ log '6#generations[-1:2]'
+  4
+  5
+  6
+  7
+  $ log '6#generations[0:]'
+  6
+  7
+  $ log '5#generations[:0]'
+  0
+  1
+  3
+  $ log '3#generations[:]'
+  0
+  1
+  3
+  5
+  6
+  7
+
   $ hg debugrevspec -p parsed 'roots(:)#g[2]'
   * parsed:
   (relsubscript