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

login
register
mail settings
Submitter phabricator
Date April 2, 2018, 11:20 p.m.
Message ID <differential-rev-PHID-DREV-hkp3kbzrryty4k2u4hv4-req@phab.mercurial-scm.org>
Download mbox | patch
Permalink /patch/30149/
State Superseded
Headers show

Comments

phabricator - April 2, 2018, 11:20 p.m.
martinvonz created this revision.
Herald added a subscriber: mercurial-devel.
Herald added a reviewer: hg-reviewers.

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

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D3024

AFFECTED FILES
  mercurial/revset.py
  mercurial/scmutil.py

CHANGE DETAILS




To: martinvonz, #hg-reviewers
Cc: mercurial-devel
phabricator - April 3, 2018, 12:30 p.m.
yuja added a comment.


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

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D3024

To: martinvonz, #hg-reviewers, yuja
Cc: mercurial-devel
phabricator - April 3, 2018, 3:31 p.m.
martinvonz added a comment.


  In https://phab.mercurial-scm.org/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.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D3024

To: martinvonz, #hg-reviewers, yuja
Cc: mercurial-devel
phabricator - April 3, 2018, 3:59 p.m.
yuja accepted this revision as: yuja.
yuja added a comment.


  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!

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D3024

To: martinvonz, #hg-reviewers, yuja
Cc: mercurial-devel

Patch

diff --git a/mercurial/scmutil.py b/mercurial/scmutil.py
--- a/mercurial/scmutil.py
+++ b/mercurial/scmutil.py
@@ -433,6 +433,19 @@ 
         hexfunc = short
     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):
     if not revspec and revspec != 0:
         return repo[default]
diff --git a/mercurial/revset.py b/mercurial/revset.py
--- a/mercurial/revset.py
+++ b/mercurial/revset.py
@@ -118,7 +118,7 @@ 
 def stringset(repo, subset, x, order):
     if not x:
         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
         or x == node.nullrev and isinstance(subset, fullreposet)):
         return baseset([x])