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

This is an archive of the discontinued Mercurial Phabricator instance.

localrepo: use revsymbol() in lookup()
ClosedPublic

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

Details

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

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

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.

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
This revision was automatically updated to reflect the committed changes.