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

[Pierre-Yves David]

On 03/31/2015 02:55 AM, FUJIWARA Katsunori wrote:
>
> At Mon, 30 Mar 2015 14:58:44 -0700,
> Pierre-Yves David wrote:
>>
>>
>>
>> On 03/29/2015 11:34 AM, FUJIWARA Katsunori wrote:
>>> # HG changeset patch
>>> # User FUJIWARA Katsunori <foozy at lares.dti.ne.jp>
>>> # Date 1427653752 -32400
>>> #      Mon Mar 30 03:29:12 2015 +0900
>>> # Node ID efa05b47238ba2a0bca69cd40b8e2e1aed3a2cf2
>>> # Parent  5f035c783b0443fabc15e773af176c7ef092c1de
>>> localrepo: add findbyrevnum for convenience
>>>
>>> This allows to use "context.findbyrevnum()" easily, without additional
>>> importing "context".
>>
>> You have awaken the localrepo bloat warden. How much caller do we have
>> for that? can it goes into scmutil as the other revnum related function?
>
> I'm also worry about bloating localrepo, but I had no other ideas
> where these functions are defined: scmutil.py doesn't seem good,
> because it is already imported by context.py
>
> But now, I hit on an idea below. What about this ?
>
>    - factor looking up logic in "changectx.__init__"  out to "repolookup.py" or so
>
>      "byrevnum()", "byhashid()" and so on are defined as public "lookup
>      logic".
>
>      These take "repo" and "changeid" argument, and return "(node,
>      revnum)" tuple for the revision found (or None at failure).
>
>    - define functions below in "repolookup.py":
>
>      - "ensure()" to find the target revision (and abort at failure)
>      - "contains()" to examine existence of the target revision
>
>      These take "repo", "changeid" and "lookup logic" arguments
>
>      For example, "hashid in repo" examination can be replaced by
>      "repolookup.contains(repo, hasid, repolookup.byhashid)".
>
>    - add "lookupfunc" optional argument to "changectx.__init__()"
>
>      - use only "lookupfunc" to initialize own fields, if it is specified
>
>        This can prevent "changectx.__init__()" from executing
>        unexpected (and useless) looking up. For example, below
>        instantiation aborts immediately at failure of looking up by
>        hashid.
>
>            ctx = context.changectx(repo, hashid, repolookup.byhashid)
>
>      - 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.

You should probably gather a few more opinion about that before diving 
into this path.

However, We have to be careful about potential performance overhead when 
it comes to changectx, impact could be significant. Please keep an eyes 
on the performance impact.


-- 
Pierre-Yves David