( )⚙ D3047 push: avoid using repo.lookup() for converting to nodeid

This is an archive of the discontinued Mercurial Phabricator instance.

Differential D3047

push: avoid using repo.lookup() for converting to nodeid
ClosedPublic

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

Details

Reviewers
indygreg
Group Reviewers
hg-reviewers
Commits
rHG4d69668a65d8: push: avoid using repo.lookup() for converting to nodeid
Summary

repo.lookup(x) currently simply does repo[x].node(), which supports
various types of inputs. As I explained in 0194dac77c93 (scmutil: add
method for looking up a context given a revision symbol, 2018-04-02),
I'd like to split that up so we use the new scmutil.revsymbol() for
string inputs repo[x] for integer revnums and binary nodeids. Since
repo.lookup() seems to exist in order to serve peer.lookup(), I think
it should be calling revsymbol. However, we have several callers that
use repo.lookup() with something that's not a string, so we need to
remove those first. This patch starts doing that. Many more will
follow.

Diff Detail

Repository
rHG Mercurial
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

martinvonz created this revision.Apr 3 2018, 6:08 PM
indygreg accepted this revision.Apr 3 2018, 7:09 PM
This revision is now accepted and ready to land.Apr 3 2018, 7:09 PM
indygreg added a comment.Apr 3 2018, 7:11 PM

I'll likely take this series. But part of me wonders if we should be performing conversion on the changelog instead. I guess it depends on whether the string inputs should resolve names and other things not known to the changelog itself. For converting integer revision numbers to nodes, I would argue we should go through the changelog.

Anyway, we'll likely have a bikeshed about all of this once we establish a formal interface for the changelog. That's a bit of a ways off, however. These changes seem fine for today.

martinvonz added a comment.Apr 3 2018, 7:13 PM
In D3047#49274, @indygreg wrote:

I'll likely take this series. But part of me wonders if we should be performing conversion on the changelog instead. I guess it depends on whether the string inputs should resolve names and other things not known to the changelog itself. For converting integer revision numbers to nodes, I would argue we should go through the changelog.
Anyway, we'll likely have a bikeshed about all of this once we establish a formal interface for the changelog. That's a bit of a ways off, however. These changes seem fine for today.

Sure, we can talk about that later :) I think I'm at least moving us closer to that by moving out the handling of user-supplied strings.

Closed by commit rHG4d69668a65d8: push: avoid using repo.lookup() for converting to nodeid (authored by martinvonz). · Explain WhyApr 3 2018, 7:24 PM
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

PathPackages
Mmercurial/commands.py (2 lines)

Diff 7587

mercurial/commands.py

hint=_("see 'hg help config.paths'")) hint=_("see 'hg help config.paths'"))
dest = path.pushloc or path.loc dest = path.pushloc or path.loc
branches = (path.branch, opts.get('branch') or []) branches = (path.branch, opts.get('branch') or [])
ui.status(_('pushing to %s\n') % util.hidepassword(dest)) ui.status(_('pushing to %s\n') % util.hidepassword(dest))
revs, checkout = hg.addbranchrevs(repo, repo, branches, opts.get('rev')) revs, checkout = hg.addbranchrevs(repo, repo, branches, opts.get('rev'))
other = hg.peer(repo, opts, dest) other = hg.peer(repo, opts, dest)
if revs: if revs:
revs = [repo.lookup(r) for r in scmutil.revrange(repo, revs)] revs = [repo[r].node() for r in scmutil.revrange(repo, revs)]
if not revs: if not revs:
raise error.Abort(_("specified revisions evaluate to an empty set"), raise error.Abort(_("specified revisions evaluate to an empty set"),
hint=_("use different revision arguments")) hint=_("use different revision arguments"))
elif path.pushrev: elif path.pushrev:
# It doesn't make any sense to specify ancestor revisions. So limit # It doesn't make any sense to specify ancestor revisions. So limit
# to DAG heads to make discovery simpler. # to DAG heads to make discovery simpler.
expr = revsetlang.formatspec('heads(%r)', path.pushrev) expr = revsetlang.formatspec('heads(%r)', path.pushrev)
revs = scmutil.revrange(repo, [expr]) revs = scmutil.revrange(repo, [expr])