[PATCH 4 of 9] localrepo: add findbyrevnum for convenience

Matt Mackall mpm at selenic.com
Fri Apr 3 11:09:01 CDT 2015


On Sat, 2015-04-04 at 00:24 +0900, FUJIWARA Katsunori wrote:
> At Tue, 31 Mar 2015 12:59:48 -0500,
> Matt Mackall wrote:
> > 
> > On Tue, 2015-03-31 at 13:02 -0400, Augie Fackler wrote:
> > > On Tue, Mar 31, 2015 at 12:54 PM, Pierre-Yves David
> > > <pierre-yves.david at ens-lyon.org> wrote:
> > > >
> > > >
> > > > On 03/31/2015 02:55 AM, FUJIWARA Katsunori wrote:
> > > >>      - use lookup functions factored out to "repolookup.py", otherwise
> > > >
> > > >
> > > > I think that getting the lookup logic out of context.py would make sense.
> > > > I've never been very comfortable with having this critical logic in
> > > > context.py.
> > >
> > > Yeah, I think having lookup functions live outside context makes a
> > > bunch of sense.
> > 
> > The code is where it needs to be to make context creation as
> > light-weight as possible. We create so many contexts in inner loops and
> > such that sticking function calls in here matters. I'd rather have two
> > copies of this code than introduce overhead here.
> 
> OK, I'll post revised ones not introducing overhead into
> "changectx.__init__()".
> 
> BTW, please let me confirm some points about "int"/"long" handling in
> "changectx.__init__()".
> 
>   1. indexing problem can't be detected for "int" changeid
> 
>      Because "repo.changelog.index" has (or may have ?) one extra
>      entry:

It's always there. We used to have conditionals "if rev == -1:"
everywhere in the revlog index accessors. But since index[-1] looks at
the end of the list in Python, we just put a magic entry at the end for
nullrev and got rid of all the conditionals.

>      - "repo.changelog.node(REV)" overlooks indexing error if "REV ==
>        len(repo.changelog)", and
> 
>      - "repo.changelog.node(REV)" returns unexpected node for "REV < -1"
> 
>      According to them, "int" changeid for "changectx()" should be not
>      "indexing number" (sequential offsets from the tip if negative)
>      but "revision number".

Yep. The magic (and a bit regrettable) -n behavior should only be for
strings from the command line.

>      (Q. 1-1) do I understand correctly ?
> 
>      Then, there is no checking code in "changectx.__init__()":
> 
>             if isinstance(changeid, int):
>                 self._node = repo.changelog.node(changeid)
>                 self._rev = changeid
>                 return
> 
>      (Q. 1-2) Is it intentional (or kept for historical reason) ? or
>      should we add checking code like below for safety ?
> 
>             if isinstance(changeid, int):
>                 if changeid < nullrev or changeid >= len(repo.changelog):
>                     raise IndexError(changeid)

This one's hard. As this is maybe the most common way to initialize a
ctx, that extra overhead matters.

> 
> 
>   2. "long" changeid is treated as "indexing number"
> 
>      As described above, "int" changeid is treated as "revision
>      number".
> 
>      But on the other hand, "long" changeid is treated as "indexing
>      number", because it is converted into string.
> 
>      In addition to it, it may unexpectedly match against hash ID
>      partially.
> 
>      (Q. 2) Is it intentional (or kept for historical reason) ? or
>      should we treat "long" changeid as same as "int" one, too ?

I don't know of any reason for it being different from int.

-- 
Mathematics is the supreme nostalgia of our time.




More information about the Mercurial-devel mailing list