This is an archive of the discontinued Mercurial Phabricator instance.

Differential D3024

scmutil: add method for looking up a context given a revision symbol
ClosedPublic

Authored by martinvonz on Apr 2 2018, 7:20 PM.

Details

Reviewers
yuja
Group Reviewers
hg-reviewers
Commits
rHG0194dac77c93: scmutil: add method for looking up a context given a revision symbol
Summary

changectx's constructor currently supports a mix if inputs:

  • integer revnums
  • binary nodeids
  • '.', 'tip', 'null'
  • stringified revnums
  • namespaced identifiers (e.g. bookmarks and tags)
  • hex nodeids
  • partial hex nodeids

The first two are always internal [1]. The other five can be specified
by the user. The third type ('.', 'tip', 'null') often comes from
either the user or internal callers. We probably have some internal
callers that pass hex nodeids too, perhaps even partial ones
(histedit?). There are only a few callers that pass user-supplied
strings: revsets.stringset, peer.lookup, webutil.changeidctx, and
maybe one or two more.

Supporting this mix of things in the constructor is convenient, but a
bit strange, IMO. For example, if repo[node] is given a node that's
not in the repo, it will first check if it's bookmark etc before
raising an exception. Of course, the risk of it being a bookmark is
extremely small, but it just feels ugly.

Also, a problem with having this code in the constructor (whether it
supports a mix of types or not) is that it's harder to override (I'd
like to override it, and that's how this series started).

This patch starts moving out the handling of user-supplied strings by
introducing scmutil.revsymbol(). So far, that just checks that the
input is indeed a string, and then delegates to repo[symbol]. The
patch also calls it from revsets.stringset to prove that it works.

[1] Well, you probably can enter a 20-byte binary nodeid on the

command line, but I don't think we should care to preserve
support for that.

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 2 2018, 7:20 PM
yuja added a comment.Apr 3 2018, 8:30 AM

Seems fine.

What the final state of repo[x], x in repo, and repo.lookup(x) will be?

martinvonz added a comment.Apr 3 2018, 11:31 AM
In D3024#48946, @yuja wrote:

Seems fine.
What the final state of repo[x], x in repo, and repo.lookup(x) will be?

repo.lookup(x) will simply delegate to scmutil.revsymbol(x). IIUC, repo.lookup() is there partly to serve the "lookup" wire protocol command, so it makes sense to me to have it call scmutil.revsymbol(). Btw, perhaps the reason one can't do repo['.^'] is because repo[x] was backing the wire protocol command, and we didn't want to expose that functionality. If we remove support for looking up branches etc from repo[x], then perhaps we can add it back to a new method, if we think that's useful enough. Since that new method won't be backing the wire protocol command, I think we might as well make it support revsets so repo['.^'] would work. Note that there's already a repo.changectx() that seems to have very few users and perhaps we could repurpose it.

repo[x] and x in repo won't change (but changectx.__init__ will be much smaller), at least not to start with. Once changectx.__init__ only accepts a nodeid or a revnum, then perhaps we should move that into repo[x], but that's far down the road. I'm a little hesitant to drop support for repo['.'] and tell people to use repo.lookup('.'), even though that seems like the right thing to do.

In hindsight, I'm actually a little surprised we didn't have a function like revsymbol() before. It seems like an obvious thing to have, but I guess for historical reasons it ended up being inside changectx's constructor.

yuja accepted this revision as: yuja.Apr 3 2018, 11:59 AM

I'm not pretty sure about the details (such as repo[revset] vs repo[rev_or_node]),
but the direction sounds generally good to me.

Queued, thanks!

Closed by commit rHG0194dac77c93: scmutil: add method for looking up a context given a revision symbol (authored by martinvonz). · Explain WhyApr 3 2018, 12:00 PM
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

PathPackages
Mmercurial/revset.py (2 lines)
Mmercurial/scmutil.py (13 lines)

Diff 7537

mercurial/revset.py

def _sortedb(xs): def _sortedb(xs):
return sorted(util.rapply(pycompat.maybebytestr, xs)) return sorted(util.rapply(pycompat.maybebytestr, xs))
# operator methods # operator methods
def stringset(repo, subset, x, order): def stringset(repo, subset, x, order):
if not x: if not x:
raise error.ParseError(_("empty string is not a valid revision")) raise error.ParseError(_("empty string is not a valid revision"))
x = scmutil.intrev(repo[x]) x = scmutil.intrev(scmutil.revsymbol(repo, x))
if (x in subset if (x in subset
or x == node.nullrev and isinstance(subset, fullreposet)): or x == node.nullrev and isinstance(subset, fullreposet)):
return baseset([x]) return baseset([x])
return baseset() return baseset()
def rangeset(repo, subset, x, y, order): def rangeset(repo, subset, x, y, order):
m = getset(repo, fullreposet(repo), x) m = getset(repo, fullreposet(repo), x)
n = getset(repo, fullreposet(repo), y) n = getset(repo, fullreposet(repo), y)

mercurial/scmutil.py

def formatrevnode(ui, rev, node): def formatrevnode(ui, rev, node):
"""Format given revision and node depending on the current verbosity""" """Format given revision and node depending on the current verbosity"""
if ui.debugflag: if ui.debugflag:
hexfunc = hex hexfunc = hex
else: else:
hexfunc = short hexfunc = short
return '%d:%s' % (rev, hexfunc(node)) return '%d:%s' % (rev, hexfunc(node))
def revsymbol(repo, symbol):
"""Returns a context given a single revision symbol (as string).
This is similar to revsingle(), but accepts only a single revision symbol,
i.e. things like ".", "tip", "1234", "deadbeef", "my-bookmark" work, but
not "max(public())".
"""
if not isinstance(symbol, bytes):
msg = ("symbol (%s of type %s) was not a string, did you mean "
"repo[symbol]?" % (symbol, type(symbol)))
raise error.ProgrammingError(msg)
return repo[symbol]
def revsingle(repo, revspec, default='.', localalias=None): def revsingle(repo, revspec, default='.', localalias=None):
if not revspec and revspec != 0: if not revspec and revspec != 0:
return repo[default] return repo[default]
l = revrange(repo, [revspec], localalias=localalias) l = revrange(repo, [revspec], localalias=localalias)
if not l: if not l:
raise error.Abort(_('empty revision set')) raise error.Abort(_('empty revision set'))
return repo[l.last()] return repo[l.last()]