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