Patchwork [2,of,2] revset: have rev() validate input by repo.__contains__()

login
register
mail settings
Submitter Yuya Nishihara
Date Feb. 2, 2015, 2:15 p.m.
Message ID <d247b985ebc0acdb837b.1422886540@mimosa>
Download mbox | patch
Permalink /patch/7593/
State Accepted
Headers show

Comments

Yuya Nishihara - Feb. 2, 2015, 2:15 p.m.
# HG changeset patch
# User Yuya Nishihara <yuya@tcha.org>
# Date 1420884012 -32400
#      Sat Jan 10 19:00:12 2015 +0900
# Node ID d247b985ebc0acdb837bc89288c8569151446620
# Parent  184aa519a7a72ab10361d91f66455e3cea8f4f50
revset: have rev() validate input by repo.__contains__()

fullreposet.__contains__() will be rewritten in order to support "null"
revision, so "rev()" can't rely on it.

"l in repo" is slightly slower than "l in fullreposet(repo)", but I think
the difference is acceptable.

revisions:
0) 0c4419faacbc "l not in fullreposet(repo) and l != node.nullrev"
1) this patch "l not in repo"

revset #0: rev(210000)
0) wall 0.000055 comb 0.000000 user 0.000000 sys 0.000000 (best of 43173)
1) wall 0.000067 comb 0.000000 user 0.000000 sys 0.000000 (best of 40090)
Pierre-Yves David - Feb. 2, 2015, 2:23 p.m.
On 02/02/2015 02:15 PM, Yuya Nishihara wrote:
> # HG changeset patch
> # User Yuya Nishihara <yuya@tcha.org>
> # Date 1420884012 -32400
> #      Sat Jan 10 19:00:12 2015 +0900
> # Node ID d247b985ebc0acdb837bc89288c8569151446620
> # Parent  184aa519a7a72ab10361d91f66455e3cea8f4f50
> revset: have rev() validate input by repo.__contains__()
>
> fullreposet.__contains__() will be rewritten in order to support "null"
> revision, so "rev()" can't rely on it.

Note that this change broke some of the Mercurial extension (because 
::nullrev stopped being empty). So we have to be careful (even if this 
change makes sense).

> "l in repo" is slightly slower than "l in fullreposet(repo)", but I think
> the difference is acceptable.

If l in an integer, why not `if l in repo.changelog?` It should be as 
fast as the fullreposet
Yuya Nishihara - Feb. 2, 2015, 3:29 p.m.
On Mon, 02 Feb 2015 14:23:42 +0000, Pierre-Yves David wrote:
> On 02/02/2015 02:15 PM, Yuya Nishihara wrote:
> > # HG changeset patch
> > # User Yuya Nishihara <yuya@tcha.org>
> > # Date 1420884012 -32400
> > #      Sat Jan 10 19:00:12 2015 +0900
> > # Node ID d247b985ebc0acdb837bc89288c8569151446620
> > # Parent  184aa519a7a72ab10361d91f66455e3cea8f4f50
> > revset: have rev() validate input by repo.__contains__()
> >
> > fullreposet.__contains__() will be rewritten in order to support "null"
> > revision, so "rev()" can't rely on it.
> 
> Note that this change broke some of the Mercurial extension (because
> ::nullrev stopped being empty). So we have to be careful (even if this
> change makes sense).

"::null" should work as long as the repository has no hidden revision, i.e.
len(subset) == len(repo).

http://selenic.com/repo/hg/file/3667bc21b877/mercurial/revset.py#l326
Matt Mackall - Feb. 2, 2015, 10:07 p.m.
On Mon, 2015-02-02 at 14:23 +0000, Pierre-Yves David wrote:
> 
> On 02/02/2015 02:15 PM, Yuya Nishihara wrote:
> > # HG changeset patch
> > # User Yuya Nishihara <yuya@tcha.org>
> > # Date 1420884012 -32400
> > #      Sat Jan 10 19:00:12 2015 +0900
> > # Node ID d247b985ebc0acdb837bc89288c8569151446620
> > # Parent  184aa519a7a72ab10361d91f66455e3cea8f4f50
> > revset: have rev() validate input by repo.__contains__()
> >
> > fullreposet.__contains__() will be rewritten in order to support "null"
> > revision, so "rev()" can't rely on it.
> 
> Note that this change broke some of the Mercurial extension (because 
> ::nullrev stopped being empty). So we have to be careful (even if this 
> change makes sense).

Sometimes I think there might be two Pierre-Yves Davids out there.
There's the one the who makes so many breaking internals changes that he
often can't even keep Mercurial compatible with his own extension
week-to-week.. and then there's some other guy who wrote the paragraph
above.

That other guy should probably see our documented policy before doing
too many more reviews:

http://mercurial.selenic.com/wiki/MercurialApi
Pierre-Yves David - Feb. 2, 2015, 10:12 p.m.
On 02/02/2015 10:07 PM, Matt Mackall wrote:
> On Mon, 2015-02-02 at 14:23 +0000, Pierre-Yves David wrote:
>>
>> On 02/02/2015 02:15 PM, Yuya Nishihara wrote:
>>> # HG changeset patch
>>> # User Yuya Nishihara <yuya@tcha.org>
>>> # Date 1420884012 -32400
>>> #      Sat Jan 10 19:00:12 2015 +0900
>>> # Node ID d247b985ebc0acdb837bc89288c8569151446620
>>> # Parent  184aa519a7a72ab10361d91f66455e3cea8f4f50
>>> revset: have rev() validate input by repo.__contains__()
>>>
>>> fullreposet.__contains__() will be rewritten in order to support "null"
>>> revision, so "rev()" can't rely on it.
>>
>> Note that this change broke some of the Mercurial extension (because
>> ::nullrev stopped being empty). So we have to be careful (even if this
>> change makes sense).
>
> Sometimes I think there might be two Pierre-Yves Davids out there.
> There's the one the who makes so many breaking internals changes that he
> often can't even keep Mercurial compatible with his own extension
> week-to-week.. and then there's some other guy who wrote the paragraph
> above.
>
> That other guy should probably see our documented policy before doing
> too many more reviews:
>
> http://mercurial.selenic.com/wiki/MercurialApi

My point is the message above was: the previous "nullrev" in revset 
related changes broke stuff (actually it broke stuff in core, releaved 
by evolve tests). So I expect more funny breakage from this change and 
we have to be "careful" about finding them (no idea how).
Pierre-Yves David - Feb. 2, 2015, 10:17 p.m.
On 02/02/2015 03:29 PM, Yuya Nishihara wrote:
> On Mon, 02 Feb 2015 14:23:42 +0000, Pierre-Yves David wrote:
>> On 02/02/2015 02:15 PM, Yuya Nishihara wrote:
>>> # HG changeset patch
>>> # User Yuya Nishihara <yuya@tcha.org>
>>> # Date 1420884012 -32400
>>> #      Sat Jan 10 19:00:12 2015 +0900
>>> # Node ID d247b985ebc0acdb837bc89288c8569151446620
>>> # Parent  184aa519a7a72ab10361d91f66455e3cea8f4f50
>>> revset: have rev() validate input by repo.__contains__()
>>>
>>> fullreposet.__contains__() will be rewritten in order to support "null"
>>> revision, so "rev()" can't rely on it.
>>
>> Note that this change broke some of the Mercurial extension (because
>> ::nullrev stopped being empty). So we have to be careful (even if this
>> change makes sense).
>
> "::null" should work as long as the repository has no hidden revision, i.e.
> len(subset) == len(repo).

The specific instance of the breakage was code using "null" as a special 
value and relying on revset being empty in that case. Not expecting a -1 
value. There may be other related bug lurking in internal code using revset.

http://42.netv6.net/evolve-main/rev/9e3f332f7630
Yuya Nishihara - Feb. 3, 2015, 2:43 p.m.
On Mon, 02 Feb 2015 22:17:21 +0000, Pierre-Yves David wrote:
> On 02/02/2015 03:29 PM, Yuya Nishihara wrote:
> > On Mon, 02 Feb 2015 14:23:42 +0000, Pierre-Yves David wrote:
> >> On 02/02/2015 02:15 PM, Yuya Nishihara wrote:
> >>> # HG changeset patch
> >>> # User Yuya Nishihara <yuya@tcha.org>
> >>> # Date 1420884012 -32400
> >>> #      Sat Jan 10 19:00:12 2015 +0900
> >>> # Node ID d247b985ebc0acdb837bc89288c8569151446620
> >>> # Parent  184aa519a7a72ab10361d91f66455e3cea8f4f50
> >>> revset: have rev() validate input by repo.__contains__()
> >>>
> >>> fullreposet.__contains__() will be rewritten in order to support "null"
> >>> revision, so "rev()" can't rely on it.
> >>
> >> Note that this change broke some of the Mercurial extension (because
> >> ::nullrev stopped being empty). So we have to be careful (even if this
> >> change makes sense).
> >
> > "::null" should work as long as the repository has no hidden revision, i.e.
> > len(subset) == len(repo).
> 
> The specific instance of the breakage was code using "null" as a special
> value and relying on revset being empty in that case. Not expecting a -1
> value. There may be other related bug lurking in internal code using revset.
> 
> http://42.netv6.net/evolve-main/rev/9e3f332f7630

I see. You're referring to "ancestors(null)" and future null-rev patches
might possibly expose similar problems.

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

I have to be careful about "%n" and "%ln".  Other format string won't point
null revision by accident.  "%d" can't handle negative number correctly, and
"rev(-1)" is new.

Regards,

Patch

diff --git a/mercurial/revset.py b/mercurial/revset.py
--- a/mercurial/revset.py
+++ b/mercurial/revset.py
@@ -1547,7 +1547,7 @@  def rev(repo, subset, x):
     except (TypeError, ValueError):
         # i18n: "rev" is a keyword
         raise error.ParseError(_("rev expects a number"))
-    if l not in fullreposet(repo) and l != node.nullrev:
+    if l not in repo:
         return baseset()
     return subset & baseset([l])