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

[Augie Fackler]

On Mon, Mar 30, 2015 at 1:54 PM, FUJIWARA Katsunori
<foozy at lares.dti.ne.jp> wrote:
> At Mon, 30 Mar 2015 10:28:59 -0400,
> Augie Fackler wrote:
>>
>> On Mon, Mar 30, 2015 at 01:45:45PM +0900, Yuya Nishihara wrote:
>> > On Mon, 30 Mar 2015 03:34:26 +0900, FUJIWARA Katsunori wrote:

>> > > +    def findbyrevnum(self, revnum, abort=False):

>> >
>> > I think findbyrevnum() can be moved to scmutil to avoid adding new methods
>> > to localrepository.
>
> Using something in context.py from scmutil.py causes importing
> problem, because context.py already imports scmutil.py.
>
> Would you mean that implementation of "findbyrevnum()" itself should
> be placed not in context.py but in scmutil.py ? or "import inside
> function" like below or so ?
>
>     def findbyrevnum(...):
>         import context
>         return context.findbyrevnum(...)

Let's definitely avoid the "import inside function" - those still
scream of layering violations to me.

>
>
>> Yeah, I'm not sure this utility method belongs on localrepo.
>
> I choose adding them to localrepository, because they can replace many
> existing "x in repo" or so easily, without worrying about import
> problem :-)

Ah, I didn't quite understand the use case. That seems somewhat reasonable.

>
>
>> I'm also not terribly convinced by its interface, though I haven't
>> looked at callers yet. (It returns None if a revision isn't found,
>> unless it's hidden, unless you specify a flag to the function to ask
>> it to abort instead. Could it just return none if something wasn't
>> found and be done?)
>
> If findbyrevnum() always returns None at failure of looking up, some
> code paths should:
>
>   - examine whether root cause of None is "unknown" or "invisible", and
>   - raise appropriate exception (RepoLookupError or FilteredRepoLookupError)

I'd much rather see some consistent behavior rather than this deeply
magical-feeling API. I'd actually be happier with two functions:

findbyrevnum -> always raises an exception if something is missing
containsrevnum -> always returns a boolean

(That said, API design is hard, so I'll not push /too/ hard on this,
but cleaning up the API of localrepo is a thing we should strive for,
rather than letting it be a dumping ground.)

>
> Revset predicate "rev()" (in patch #3) and "id()" (in patch #9) are
> typical cases to have to raise such exception.
>
> If caller of findbyrevnum() should do so, (1) it causes duplication of
> similar code or (2) some additional utilities are needed.
>
>
> BTW, at first, I think also adding both "checking-existence" and
> "aborting" (like "dirty" and "bailifchanged" in subrepo, I did !), but
> this approach seems to be complicated in this case.
>
>
>>
>> > _______________________________________________
>> > Mercurial-devel mailing list
>> > Mercurial-devel at selenic.com
>> > http://selenic.com/mailman/listinfo/mercurial-devel
>>
>
> ----------------------------------------------------------------------
> [FUJIWARA Katsunori]                             foozy at lares.dti.ne.jp