Patchwork [STABLE] revset: have rev() drop out-of-range or filtered rev explicitly (issue4396)

login
register
mail settings
Submitter Yuya Nishihara
Date Oct. 19, 2014, 8:48 a.m.
Message ID <76ab4107fb04497dcb9f.1413708482@mimosa>
Download mbox | patch
Permalink /patch/6413/
State Accepted
Commit ba89f7b542c90b79bc677650df461b2f1fa675a8
Headers show

Comments

Yuya Nishihara - Oct. 19, 2014, 8:48 a.m.
# HG changeset patch
# User Yuya Nishihara <yuya@tcha.org>
# Date 1413704913 -32400
#      Sun Oct 19 16:48:33 2014 +0900
# Branch stable
# Node ID 76ab4107fb04497dcb9fc6eb9f90272572105963
# Parent  a516f593498da57f5ec9c7967c967ee42626cd15
revset: have rev() drop out-of-range or filtered rev explicitly (issue4396)

The recent optimization of "and" operation relies on the assumption that
the rhs set does not contain invalid revisions.  So rev() has to remove
invalid revisions.

This is still faster than using `.filter(lambda r: r == l)`.

revset #0: rev(25)
0) wall 0.026341 comb 0.020000 user 0.020000 sys 0.000000 (best of 113)
1) wall 0.000038 comb 0.000000 user 0.000000 sys 0.000000 (best of 66567)
2) wall 0.000062 comb 0.000000 user 0.000000 sys 0.000000 (best of 43699)
(0: bbf4f3dfd700^, 1: 3.2-rc, 2: this patch)
Yuya Nishihara - Oct. 23, 2014, noon
On Mon, 20 Oct 2014 09:46:15 -0700, Pierre-Yves David wrote:
> On 10/20/2014 06:02 AM, Yuya Nishihara wrote:
> > On Mon, 20 Oct 2014 03:06:01 -0700, Pierre-Yves David wrote:
> >> I've changed it to::
> >>
> >>       if l not in repo.changelog:
> >>
> >> And pushed the result to the clowncopter.
> >
> > Isn't it slower than "l in repo" for large number?
> > revlog doesn't have  __contains__().
> >
> > revset #0: rev(210000)
> > 0) wall 0.026161 comb 0.030000 user 0.030000 sys 0.000000 (best of 107)
> > 1) wall 0.000045 comb 0.000000 user 0.000000 sys 0.000000 (best of 58973)
> > 2) wall 0.000063 comb 0.000000 user 0.000000 sys 0.000000 (best of 42704)
> > 3) wall 0.002724 comb 0.000000 user 0.000000 sys 0.000000 (best of 1060)
> 
> gasp! I did not realised that. I switched it to `l not in 
> fullreposet(repo)` in a hurry. We should probably fix changelog 
> containment in 3.3. Thanks alot for double checking this, should not 
> have tried to get thing "better" (but I happy to have found a bug).

Maybe new patch isn't pushed to clowncopter yet?
The slow version was queued:

http://selenic.com/repo/hg/rev/ba89f7b542c9

Regards,
Pierre-Yves David - Oct. 23, 2014, 12:34 p.m.
On 10/23/2014 05:00 AM, Yuya Nishihara wrote:
> On Mon, 20 Oct 2014 09:46:15 -0700, Pierre-Yves David wrote:
>> On 10/20/2014 06:02 AM, Yuya Nishihara wrote:
>>> On Mon, 20 Oct 2014 03:06:01 -0700, Pierre-Yves David wrote:
>>>> I've changed it to::
>>>>
>>>>        if l not in repo.changelog:
>>>>
>>>> And pushed the result to the clowncopter.
>>>
>>> Isn't it slower than "l in repo" for large number?
>>> revlog doesn't have  __contains__().
>>>
>>> revset #0: rev(210000)
>>> 0) wall 0.026161 comb 0.030000 user 0.030000 sys 0.000000 (best of 107)
>>> 1) wall 0.000045 comb 0.000000 user 0.000000 sys 0.000000 (best of 58973)
>>> 2) wall 0.000063 comb 0.000000 user 0.000000 sys 0.000000 (best of 42704)
>>> 3) wall 0.002724 comb 0.000000 user 0.000000 sys 0.000000 (best of 1060)
>>
>> gasp! I did not realised that. I switched it to `l not in
>> fullreposet(repo)` in a hurry. We should probably fix changelog
>> containment in 3.3. Thanks alot for double checking this, should not
>> have tried to get thing "better" (but I happy to have found a bug).
>
> Maybe new patch isn't pushed to clowncopter yet?
> The slow version was queued:

Something apparently went wrong durign my second push :-/ Can you issue 
a follow up?

Patch

diff --git a/mercurial/revset.py b/mercurial/revset.py
--- a/mercurial/revset.py
+++ b/mercurial/revset.py
@@ -1359,6 +1359,8 @@  def rev(repo, subset, x):
     except (TypeError, ValueError):
         # i18n: "rev" is a keyword
         raise error.ParseError(_("rev expects a number"))
+    if l < 0 or l >= len(repo) or l not in repo:
+        return baseset()
     return subset & baseset([l])
 
 def matching(repo, subset, x):
diff --git a/tests/test-obsolete.t b/tests/test-obsolete.t
--- a/tests/test-obsolete.t
+++ b/tests/test-obsolete.t
@@ -185,6 +185,8 @@  check that various commands work well wi
   abort: hidden revision '4'!
   (use --hidden to access hidden revisions)
   [255]
+  $ hg debugrevspec 'rev(6)'
+  $ hg debugrevspec 'rev(4)'
 
 Check that public changeset are not accounted as obsolete:
 
diff --git a/tests/test-revset.t b/tests/test-revset.t
--- a/tests/test-revset.t
+++ b/tests/test-revset.t
@@ -437,6 +437,18 @@  Test empty set input
   4
   8
   9
+
+Test explicit numeric revision
+  $ log 'rev(-1)'
+  $ log 'rev(0)'
+  0
+  $ log 'rev(9)'
+  9
+  $ log 'rev(10)'
+  $ log 'rev(tip)'
+  hg: parse error: rev expects a number
+  [255]
+
   $ log 'outgoing()'
   8
   9