Patchwork D3055: localrepo: use revsymbol() in lookup()

login
register
mail settings
Submitter phabricator
Date April 3, 2018, 10:09 p.m.
Message ID <differential-rev-PHID-DREV-w73jlsay3ylvc4cujk65-req@phab.mercurial-scm.org>
Download mbox | patch
Permalink /patch/30218/
State Superseded
Headers show

Comments

phabricator - April 3, 2018, 10:09 p.m.
martinvonz created this revision.
Herald added a subscriber: mercurial-devel.
Herald added a reviewer: hg-reviewers.

REVISION SUMMARY
  lookup() seems to be about looking up a revision based on a symbol
  that may come from the user (via the wire protocol), so revsymbol() is
  appropriate here.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D3055

AFFECTED FILES
  mercurial/localrepo.py

CHANGE DETAILS




To: martinvonz, #hg-reviewers
Cc: mercurial-devel
phabricator - April 3, 2018, 11:22 p.m.
indygreg added a comment.


  `scmutil.revsymbol()` just calls `repo[x]`. So what is your intention here?
  
  FWIW, we must preserve `repo.lookup('0')` resolving to revision 0 because `share.poolnaming=identity` relies on it.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D3055

To: martinvonz, #hg-reviewers
Cc: indygreg, mercurial-devel
phabricator - April 3, 2018, 11:33 p.m.
martinvonz added a comment.


  In https://phab.mercurial-scm.org/D3055#49290, @indygreg wrote:
  
  > `scmutil.revsymbol()` just calls `repo[x]`. So what is your intention here?
  
  
  See https://phab.mercurial-scm.org/D3024, including the discussion with Yuya there. In short, revsymbol will gradually take over responsibility from changectx.__init__ for interpreting strings.
  
  > FWIW, we must preserve `repo.lookup('0')` resolving to revision 0 because `share.poolnaming=identity` relies on it.
  
  That will continue to work, since '0' is a string.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D3055

To: martinvonz, #hg-reviewers
Cc: indygreg, mercurial-devel
phabricator - April 3, 2018, 11:34 p.m.
indygreg added a comment.


  Also, `lookup()` is part of the peer interface (i.e. how remote consumers are supposed to interact with the repo instance). They are supposed to call `repo.peer()` to get a `localpeer()` instance that exposes just the peer interface.
  
  It /should/ be acceptable to remove all the peer interface members from `localrepository` and require everyone go through `peer()`. This includes `known()`, `pushkey()`, `branchmap()`, etc. That would mean `localpeer` would have to access private attributes of `localrepository`. That should be fine: the classes are in the same module. It would make monkeypatching from extensions a little bit more complicated. But it would still be possible.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D3055

To: martinvonz, #hg-reviewers
Cc: indygreg, mercurial-devel

Patch

diff --git a/mercurial/localrepo.py b/mercurial/localrepo.py
--- a/mercurial/localrepo.py
+++ b/mercurial/localrepo.py
@@ -1021,7 +1021,7 @@ 
                 pass
 
     def lookup(self, key):
-        return self[key].node()
+        return scmutil.revsymbol(self, key).node()
 
     def lookupbranch(self, key, remote=None):
         repo = remote or self