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

[Anton Shestakov]

On Wed, 16 Jan 2019 23:18:38 +0900
Yuya Nishihara <yuya at tcha.org> wrote:

> On Wed, 16 Jan 2019 18:29:57 +0800, Anton Shestakov wrote:
> > # HG changeset patch
> > # User Anton Shestakov <av6 at 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!