| 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
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
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
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
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).
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
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])