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

[FUJIWARA Katsunori]

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:
> > > # 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".
> > >
> > > diff --git a/mercurial/localrepo.py b/mercurial/localrepo.py
> > > --- a/mercurial/localrepo.py
> > > +++ b/mercurial/localrepo.py
> > > @@ -1900,6 +1900,21 @@ class localrepository(object):
> > >              fp.close()
> > >          return self.pathto(fp.name[len(self.root) + 1:])
> > >
> > > +    def findbyrevnum(self, revnum, abort=False):
> > > +        """Find the revision by revision number in this repository
> > > +
> > > +        ``revnum`` should be ``int``. All negative values other than
> > > +        ``-1`` (as null revision) are treated as invalid revision number.
> > > +
> > > +        This returns ``(node, revnum)`` tuple, if the revision
> > > +        corresponded to the specified ``revnum`` is found. If that
> > > +        revision is hidden one, this raises FilteredRepoLookupError.
> > > +
> > > +        Other wise, this returns ``None`` (or raises RepoLookupError if
> > > +        ``abort``).
> > > +        """
> > > +        return context.findbyrevnum(self, revnum, abort=abort)
> >
> > 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(...)


> 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 :-)


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

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