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

This is an archive of the discontinued Mercurial Phabricator instance.

Differential D3055

localrepo: use revsymbol() in lookup()
ClosedPublic

Authored by martinvonz on Apr 3 2018, 6:08 PM.

Details

Reviewers
indygreg
Group Reviewers
hg-reviewers
Commits
rHG0a1fb171dc1d: localrepo: use revsymbol() in lookup()
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.

Diff Detail

Repository
rHG Mercurial
Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

martinvonz created this revision.Apr 3 2018, 6:08 PM
indygreg added a subscriber: indygreg.Apr 3 2018, 7:22 PM

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.

martinvonz added a comment.EditedApr 3 2018, 7:33 PM
In D3055#49290, @indygreg wrote:

scmutil.revsymbol() just calls repo[x]. So what is your intention here?

See 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.

indygreg added a comment.Apr 3 2018, 7:34 PM

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.

indygreg accepted this revision.Apr 3 2018, 7:36 PM
This revision is now accepted and ready to land.Apr 3 2018, 7:36 PM
Closed by commit rHG0a1fb171dc1d: localrepo: use revsymbol() in lookup() (authored by martinvonz). · Explain WhyApr 3 2018, 7:36 PM
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

PathPackages
Mmercurial/localrepo.py (2 lines)

Diff 7586

mercurial/localrepo.py

return self.branchmap().branchtip(branch) return self.branchmap().branchtip(branch)
except KeyError: except KeyError:
if not ignoremissing: if not ignoremissing:
raise error.RepoLookupError(_("unknown branch '%s'") % branch) raise error.RepoLookupError(_("unknown branch '%s'") % branch)
else: else:
pass pass
def lookup(self, key): def lookup(self, key):
return self[key].node() return scmutil.revsymbol(self, key).node()
def lookupbranch(self, key, remote=None): def lookupbranch(self, key, remote=None):
repo = remote or self repo = remote or self
if key in repo.branchmap(): if key in repo.branchmap():
return key return key
repo = (remote and remote.local()) and remote or self repo = (remote and remote.local()) and remote or self
return repo[key].branch() return repo[key].branch()