Patchwork [1,of,2] context: fix repo[n] not to make invalid changectx if n is out of range

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

Comments

Yuya Nishihara - Feb. 2, 2015, 2:15 p.m.
# HG changeset patch
# User Yuya Nishihara <yuya@tcha.org>
# Date 1420883783 -32400
#      Sat Jan 10 18:56:23 2015 +0900
# Node ID 184aa519a7a72ab10361d91f66455e3cea8f4f50
# Parent  3667bc21b8773715d9472a3b4e034b77e62c6451
context: fix repo[n] not to make invalid changectx if n is out of range

This problem was spotted by ba89f7b542c9 (and eb763217152a), so it uses
fullreposet(repo) instead. This fix is necessary to rewrite "rev()" revset
without using fullreposet.
Pierre-Yves David - Feb. 2, 2015, 2:25 p.m.
On 02/02/2015 02:15 PM, Yuya Nishihara wrote:
> # HG changeset patch
> # User Yuya Nishihara <yuya@tcha.org>
> # Date 1420883783 -32400
> #      Sat Jan 10 18:56:23 2015 +0900
> # Node ID 184aa519a7a72ab10361d91f66455e3cea8f4f50
> # Parent  3667bc21b8773715d9472a3b4e034b77e62c6451
> context: fix repo[n] not to make invalid changectx if n is out of range
>
> This problem was spotted by ba89f7b542c9 (and eb763217152a), so it uses
> fullreposet(repo) instead. This fix is necessary to rewrite "rev()" revset
> without using fullreposet.
>
> diff --git a/mercurial/context.py b/mercurial/context.py
> --- a/mercurial/context.py
> +++ b/mercurial/context.py
> @@ -371,6 +371,8 @@ class changectx(basectx):
>
>           try:
>               if isinstance(changeid, int):
> +                if changeid < nullrev or changeid >= len(repo.changelog):

len(repo.changelog) can be more expensive than you expect this is has to 
take filtering into account (iirc), you probably want `changeid in 
repo.changelog`
Yuya Nishihara - Feb. 2, 2015, 2:48 p.m.
On Mon, 02 Feb 2015 14:25:54 +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 1420883783 -32400
> > #      Sat Jan 10 18:56:23 2015 +0900
> > # Node ID 184aa519a7a72ab10361d91f66455e3cea8f4f50
> > # Parent  3667bc21b8773715d9472a3b4e034b77e62c6451
> > context: fix repo[n] not to make invalid changectx if n is out of range
> >
> > This problem was spotted by ba89f7b542c9 (and eb763217152a), so it uses
> > fullreposet(repo) instead. This fix is necessary to rewrite "rev()" revset
> > without using fullreposet.
> >
> > diff --git a/mercurial/context.py b/mercurial/context.py
> > --- a/mercurial/context.py
> > +++ b/mercurial/context.py
> > @@ -371,6 +371,8 @@ class changectx(basectx):
> >
> >           try:
> >               if isinstance(changeid, int):
> > +                if changeid < nullrev or changeid >= len(repo.changelog):
> 
> len(repo.changelog) can be more expensive than you expect this is has to 
> take filtering into account (iirc),

I think it just returns revlog.__len__(), and changectx.__init__() does it
for string revision.

> you probably want `changeid in repo.changelog`

It will iterate all nodes in changelog, see eb763217152a.

Regards,
Pierre-Yves David - Feb. 2, 2015, 2:51 p.m.
On 02/02/2015 02:48 PM, Yuya Nishihara wrote:
> On Mon, 02 Feb 2015 14:25:54 +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 1420883783 -32400
>>> #      Sat Jan 10 18:56:23 2015 +0900
>>> # Node ID 184aa519a7a72ab10361d91f66455e3cea8f4f50
>>> # Parent  3667bc21b8773715d9472a3b4e034b77e62c6451
>>> context: fix repo[n] not to make invalid changectx if n is out of range
>>>
>>> This problem was spotted by ba89f7b542c9 (and eb763217152a), so it uses
>>> fullreposet(repo) instead. This fix is necessary to rewrite "rev()" revset
>>> without using fullreposet.
>>>
>>> diff --git a/mercurial/context.py b/mercurial/context.py
>>> --- a/mercurial/context.py
>>> +++ b/mercurial/context.py
>>> @@ -371,6 +371,8 @@ class changectx(basectx):
>>>
>>>            try:
>>>                if isinstance(changeid, int):
>>> +                if changeid < nullrev or changeid >= len(repo.changelog):
>>
>> len(repo.changelog) can be more expensive than you expect this is has to
>> take filtering into account (iirc),
>
> I think it just returns revlog.__len__(), and changectx.__init__() does it
> for string revision.
>
>> you probably want `changeid in repo.changelog`
>
> It will iterate all nodes in changelog, see eb763217152a.

Can't we fix changelog.__contains__ to be non-stupid and fast instead?
Yuya Nishihara - Feb. 2, 2015, 3:29 p.m.
On Mon, 02 Feb 2015 14:51:19 +0000, Pierre-Yves David wrote:
> On 02/02/2015 02:48 PM, Yuya Nishihara wrote:
> > On Mon, 02 Feb 2015 14:25:54 +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 1420883783 -32400
> >>> #      Sat Jan 10 18:56:23 2015 +0900
> >>> # Node ID 184aa519a7a72ab10361d91f66455e3cea8f4f50
> >>> # Parent  3667bc21b8773715d9472a3b4e034b77e62c6451
> >>> context: fix repo[n] not to make invalid changectx if n is out of range
> >>>
> >>> This problem was spotted by ba89f7b542c9 (and eb763217152a), so it uses
> >>> fullreposet(repo) instead. This fix is necessary to rewrite "rev()" revset
> >>> without using fullreposet.
> >>>
> >>> diff --git a/mercurial/context.py b/mercurial/context.py
> >>> --- a/mercurial/context.py
> >>> +++ b/mercurial/context.py
> >>> @@ -371,6 +371,8 @@ class changectx(basectx):
> >>>
> >>>            try:
> >>>                if isinstance(changeid, int):
> >>> +                if changeid < nullrev or changeid >= len(repo.changelog):
> >>
> >> len(repo.changelog) can be more expensive than you expect this is has to
> >> take filtering into account (iirc),
> >
> > I think it just returns revlog.__len__(), and changectx.__init__() does it
> > for string revision.
> >
> >> you probably want `changeid in repo.changelog`
> >
> > It will iterate all nodes in changelog, see eb763217152a.
> 
> Can't we fix changelog.__contains__ to be non-stupid and fast instead?

It'll be possible. I'll try.

Regards,

Patch

diff --git a/mercurial/context.py b/mercurial/context.py
--- a/mercurial/context.py
+++ b/mercurial/context.py
@@ -371,6 +371,8 @@  class changectx(basectx):
 
         try:
             if isinstance(changeid, int):
+                if changeid < nullrev or changeid >= len(repo.changelog):
+                    raise IndexError
                 self._node = repo.changelog.node(changeid)
                 self._rev = changeid
                 return