( )⚙ 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
Lint Skipped
Unit
Unit Tests Skipped

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 7578

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