Page MenuHomePhabricator

revset: add expect to check the size of a set
ClosedPublic

Authored by navaneeth.suresh on Feb 3 2019, 9:28 AM.

Details

Summary

expectsize(<set>, <int>) revset fails if <set> is not exactly <int> elements.
expectsize(<set>, <min:max>) revset fails if <set> is not exactly between
<min> and <max> inclusive.
one(<set>) alias for expectsize(<set>, 1).

This then allows an alias for hg next to be update -r one(children(.))
with sane failure behavior, and also makes some other scripting tasks
a little less difficult.

(Summary from WeShouldDoThat)

Diff Detail

Repository
rHG Mercurial
Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

This then allows an alias for hg next to be update -r one(children(.))
with sane failure behavior, and also makes some other scripting tasks
a little less difficult.

Just for record, hg next has a nice prompt if there are multiple childrens. So the expect or one revset might not be very useful there.

mercurial/revset.py
838

it will be good if we add the length of revset in the error message here.

839

why are we special casing 1 here?

1444

I am not sure why we need this. @durin42 might have thoughts on this.

navaneeth.suresh marked 2 inline comments as done.Feb 4 2019, 1:07 AM

@pulkit Thanks for the quick review! I've updated the revision with the suggested changes. @durin42 It would be great if you let us know your opinion on one(<set>) as an alias for expect(<set>, 1). I did it as it was present in WeShouldDoThat page.

yuja added a subscriber: yuja.Feb 4 2019, 7:20 AM

+@predicate('expect(set[, size[, min, max]])', safe=True, takeorder=True)

First, I think the word expect is too general. Perhaps, this should be called
expectsize() or expectlen().

It's also unclear what's the difference between size and min/max.
Instead of these parameters, maybe we can add a size parameter that takes
a number or a range min:max. See also the following patch.

https://www.mercurial-scm.org/pipermail/mercurial-devel/2019-February/127916.html

+ if len(rev) != n:
+ raise error.Abort(_('revset is not of expected size'))

Better to raise RepoLookupError so the error can be caught by present(...).

+ return rev

You need to filter rev by subset. Since we'll probably want to get an ordered
result from expect(set), we'll have to conditionalize the filtering direction:

if order == followorder:
    return subset & rev
else:
    return rev & subset

You can try out some combinations of expect(5:0) & 1:10 and
10:1 & expect(0:5).

First, I think the word expect is too general. Perhaps, this should be called
expectsize() or expectlen().

I can make it expectsize().

It's also unclear what's the difference between size and min/max.
Instead of these parameters, maybe we can add a size parameter that takes
a number or a range min:max. See also the following patch.
https://www.mercurial-scm.org/pipermail/mercurial-devel/2019-February/127916.html

Unfortunately, I can't see this in hg-stable which I was working.

+ if len(rev) != n:
+ raise error.Abort(_('revset is not of expected size'))

Better to raise RepoLookupError so the error can be caught by present(...).

For sure. I could do that.

+ return rev

You need to filter rev by subset. Since we'll probably want to get an ordered
result from expect(set), we'll have to conditionalize the filtering direction:

if order == followorder:
    return subset & rev
else:
    return rev & subset

You can try out some combinations of expect(5:0) & 1:10 and
10:1 & expect(0:5).

I got into many errors while using this. I might be not understanding this correctly. Could you please elaborate?

yuja added a comment.Feb 5 2019, 5:44 PM
> You can try out some combinations of `expect(5:0) & 1:10` and
>  `10:1 & expect(0:5)`.
I got into many errors while using this. I might be not understanding this correctly. Could you please elaborate?

Can you share your failed attempt?

Maybe you can get how revset works by testing expression with
hg debugrevspec -v.

navaneeth.suresh edited the summary of this revision. (Show Details)Feb 6 2019, 11:15 AM
navaneeth.suresh updated this revision to Diff 13851.

Can you share your failed attempt?
Maybe you can get how revset works by testing expression with
hg debugrevspec -v.

I somehow managed to meet your requirements in the current revision @yuja. Please let me know if I missed out something there.

yuja added a comment.Feb 7 2019, 8:18 AM

+ $ hg log -r 'expectsize(0:2, 3)'
+ changeset: 0:2785f51eece5
+ branch: a
+ user: test
+ date: Thu Jan 01 00:00:00 1970 +0000
+ summary: 0

Nit: the test outputs look unnecessarily verbose. Use log (not hg log)
instead.

+@predicate('expectsize(set[, size])', safe=True, takeorder=True)
+def expectrevsetsize(repo, subset, x, order, n=None):
+ """Abort if the revset doesn't expect given size"""
+ args = getargsdict(x, 'expect', 'set size')
+ size = args.get('size', n)
+ if size is not None:
+ try:
+ # size is given as integer range on expectsize(<set>, <intrange>)

The helper function has been queued. Can you rewrite this to use the
getintrange helper?

+ if len(rev) not in range(size[0], size[1]+1):

range() builds a list of integers on Python 2, which isn't what we want.
Python does support not (x <= y <= z) syntax, so you can just compare
integer bounds.

+@predicate('one(set)', safe=True, takeorder=True)
+def one(repo, subset, x, order):
+ """An alias for expect(<set>, 1)"""
+ return expectrevsetsize(repo, subset, x, order, n=1)

Can you remove one() from this patch?

I don't follow the original proposal, but I guess one() would be meant to
a user-defined alias (i.e. revsetalias.one(x) = expectsize(x, 1)).

@yuja I've updated the revision with the suggested changes.

yuja added a comment.Feb 9 2019, 8:27 PM

+@predicate('expectsize(set[, size])', safe=True, takeorder=True)
+def expectrevsetsize(repo, subset, x, order):
+ """Abort if the revset doesn't expect given size"""
+ args = getargsdict(x, 'expect', 'set size')
+ size = args.get('size')
+ if size is not None:
+ minsize, maxsize = getintrange(size,
+ _('expectsize requires a size range'
+ ' or a positive integer'),
+ _('size range bounds must be integers'))

Maybe needs to specify the default min/max values to e.g. possible min/max
values or None.

+ if minsize != maxsize:
+ size = (minsize, maxsize)
+ else:
+ size = minsize
+ if size is None or 'set' not in args:
+ raise error.ParseError(_('invalid set of arguments'))
+ rev = getset(repo, fullreposet(repo), args['set'], order=order)
+ if isinstance(size, tuple):
+ if len(rev) < minsize or len(rev) > maxsize:
+ raise error.RepoLookupError(
+ _('revset size mismatch.'
+ ' expected between %d and %d, got %d') % (minsize,
+ maxsize,
+ len(rev)))
+ if isinstance(size, int):
+ if len(rev) != size:
+ raise error.RepoLookupError(
+ _('revset size mismatch.'
+ ' expected %d, got %d') % (size, len(rev)))

There's no point to duplicate these "if"s because both minsize/maxsize should
be set. We could switch the error messages by minsize == maxsize, but which
doesn't mean we need different size types depending on minsize/maxsize values.

+ # filter rev by subset. since we'll probably want to get an ordered
+ # result from expectsize(<set>), we'll have to conditionalize the
+ # filtering direction

Nit: this comment seems unnecessary since it just rephrase the code.

+ if order == followorder:
+ return subset & rev
+ else:
+ return rev & subset

yuja added a comment.Feb 10 2019, 9:16 PM

Getting close.

nit-pick is resolved. getintrange() will throw a ParseError on setting size to one of min:, :max or :. In ideal case, on calling from expectsize() it shouldn't fail.

yuja added a comment.Feb 11 2019, 8:43 AM
nit-pick is resolved. `getintrange()` will throw a `ParseError` on setting size to one of `min:`, `:max` or `:`. In ideal case, on calling from `expectsize()` it shouldn't fail.

So you need to pass in the default min/max values to getintrange().
The default min can be just 0. The default max can be len(repo) + 1
(i.e. # of null:tip) for example, or None and test maxsize is not None
explicitly.

yuja added a comment.Feb 12 2019, 7:49 AM

+@predicate('expectsize(set[, size])', safe=True, takeorder=True)
+def expectrevsetsize(repo, subset, x, order):
+ """Abort if the revset doesn't expect given size"""
+ args = getargsdict(x, 'expect', 'set size')

Fixed function name and queued, thanks.

This revision was automatically updated to reflect the committed changes.
pulkit added inline comments.Feb 12 2019, 8:53 AM
mercurial/revset.py
820

Can you improve this documentation as a follow-up? We should mentioned about specifying ranges, and also if the set has the given size, that set is returned.

mercurial/revset.py
820

Doing that right away!