This is an archive of the discontinued Mercurial Phabricator instance.

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

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

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
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

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
884

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

885

why are we special casing 1 here?

1484

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
866

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
866

Doing that right away!