Patchwork D1678: revset: pass pre-optimized tree in revset.matchany()

login
register
mail settings
Submitter phabricator
Date Dec. 13, 2017, 1:55 a.m.
Message ID <differential-rev-PHID-DREV-ci4qw5dzl6zvfa4y5477-req@phab.mercurial-scm.org>
Download mbox | patch
Permalink /patch/26260/
State Superseded
Headers show

Comments

phabricator - Dec. 13, 2017, 1:55 a.m.
pulkit created this revision.
Herald added a subscriber: mercurial-devel.
Herald added a reviewer: hg-reviewers.

REVISION SUMMARY
  This is a part of series where we move logic around so that we can get the tree
  at a higher level function such as repo.anyrevs(). This will help us in reading
  the tree and doing certain operations like updating visibility exceptions on
  base of that.

REPOSITORY
  rHG Mercurial

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

AFFECTED FILES
  mercurial/localrepo.py
  mercurial/revset.py

CHANGE DETAILS




To: pulkit, #hg-reviewers
Cc: mercurial-devel
phabricator - Dec. 13, 2017, 1:57 a.m.
pulkit added a subscriber: yuja.
pulkit added a comment.


  @yuja am I headed in the right direction? (I am not sure about whether the API's I changed are used by extensions or not)

REPOSITORY
  rHG Mercurial

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

To: pulkit, #hg-reviewers
Cc: yuja, mercurial-devel
phabricator - Dec. 13, 2017, 1:24 p.m.
yuja added a comment.


  In https://phab.mercurial-scm.org/D1678#28686, @pulkit wrote:
  
  > @yuja am I headed in the right direction? (I am not sure about whether the API's I changed are used by extensions or not)
  
  
  Sort of, but I was thinking of a simpler one. Just parse specs twice, which isn't
  costly operation.
  
    def unhidehashlikerevs(repo, specs, allowhidden|warnhidden):
        if not repo.filtername:
            return repo
        syms = set()
        for spec in specs:
            try:
                tree = revsetlang.parse(spec)
            except ParseError:
                continue  # will be reported later by scmutil.revrange()
            syms.update(revsetlang.hashlikesymbols(tree))
        if not syms:
            return repo
        pinned = perhaps_need_to_convert_to_nodes_or_revs?(syms)
        # filtered repo with the given extra pinned revisions; no idea if this is an ideal API
        return repo.filtered(repo.filtername, pinned)
  
  This allows us to get rid of an intermediate state where a repo is flagged as
  "visible-<x>", but "<x>" isn't added yet.
  
  We can apply this function at dispatch:
  
    def _dispatch(req):
        ...
                if repo:
                    ui = repo.ui
                    if options['hidden']:
                        repo = repo.unfiltered()
                    else:
                        # perhaps we'll need a registrar flag to get arguments taking revspecs
                        repo = scmutil.unhidehashlikerevs(repo, cmdoptions.get('rev', []))
  
  or simply at each command:
  
    def diff(...):
        ...
        repo = scmutil.unhidehashlikerevs(repo, changesets, 'allowhidden')
        revs = scmutil.revrange(repo, changesets)

REPOSITORY
  rHG Mercurial

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

To: pulkit, #hg-reviewers
Cc: yuja, mercurial-devel
phabricator - Dec. 14, 2017, 11:57 p.m.
pulkit added a comment.


  In https://phab.mercurial-scm.org/D1678#28729, @yuja wrote:
  
  > In https://phab.mercurial-scm.org/D1678#28686, @pulkit wrote:
  >
  > > @yuja am I headed in the right direction? (I am not sure about whether the API's I changed are used by extensions or not)
  >
  >
  > Sort of, but I was thinking of a simpler one. Just parse specs twice, which isn't
  >  costly operation.
  >
  >   def unhidehashlikerevs(repo, specs, allowhidden|warnhidden):
  >       if not repo.filtername:
  >           return repo
  >       syms = set()
  >       for spec in specs:
  >           try:
  >               tree = revsetlang.parse(spec)
  >           except ParseError:
  >               continue  # will be reported later by scmutil.revrange()
  >           syms.update(revsetlang.hashlikesymbols(tree))
  >       if not syms:
  >           return repo
  >       pinned = perhaps_need_to_convert_to_nodes_or_revs?(syms)
  >       # filtered repo with the given extra pinned revisions; no idea if this is an ideal API
  >       return repo.filtered(repo.filtername, pinned)
  >
  >
  > This allows us to get rid of an intermediate state where a repo is flagged as
  >  "visible-<x>", but "<x>" isn't added yet.
  >
  > We can apply this function at dispatch:
  >
  >   def _dispatch(req):
  >       ...
  >               if repo:
  >                   ui = repo.ui
  >                   if options['hidden']:
  >                       repo = repo.unfiltered()
  >                   else:
  >                       # perhaps we'll need a registrar flag to get arguments taking revspecs
  >                       repo = scmutil.unhidehashlikerevs(repo, cmdoptions.get('rev', []))
  >
  >
  > or simply at each command:
  >
  >   def diff(...):
  >       ...
  >       repo = scmutil.unhidehashlikerevs(repo, changesets, 'allowhidden')
  >       revs = scmutil.revrange(repo, changesets)
  >
  
  
  I like your idea. Thanks for it. After some hacking, I think we should call `scmutil.unhidehashlikerevs()` from each command level because at dispatch level we are not sure what `revs` are, some commands don't need `--rev` to take a rev like `hg export`. Since we are calling them at each command level, it will be better to call unhidehashlikerevs() in `scmutil.revrange|revsingle`. We will still need to store the filtername in dispatch and then later on decide on basis of that whether we want to uhide things.

REPOSITORY
  rHG Mercurial

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

To: pulkit, #hg-reviewers
Cc: yuja, mercurial-devel
phabricator - Dec. 15, 2017, 1 p.m.
yuja added a comment.


  > Since we are calling them at each command level, it will be better to call unhidehashlikerevs() in `scmutil.revrange|revsingle`.
  
  I thought about that, but `scmutil.rev*()` would have to return new filtered `repo`
  object, which didn't seem nice. And mutating a given `repo` argument has the
  original problem, when to discard unhidden revisions. So my conclusion was a
  separate function.
  
  > We will still need to store the filtername in dispatch and then later on
  >  decide on basis of that whether we want to uhide things.
  
  Doesn't each command know whether it should unhide or not?

REPOSITORY
  rHG Mercurial

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

To: pulkit, #hg-reviewers
Cc: yuja, mercurial-devel
phabricator - Dec. 19, 2017, 12:15 p.m.
pulkit abandoned this revision.
pulkit added a comment.


  In https://phab.mercurial-scm.org/D1678#29063, @yuja wrote:
  
  > > Since we are calling them at each command level, it will be better to call unhidehashlikerevs() in `scmutil.revrange|revsingle`.
  >
  > I thought about that, but `scmutil.rev*()` would have to return new filtered `repo`
  >  object, which didn't seem nice. And mutating a given `repo` argument has the
  >  original problem, when to discard unhidden revisions. So my conclusion was a
  >  separate function.
  >
  > > We will still need to store the filtername in dispatch and then later on
  > >  decide on basis of that whether we want to uhide things.
  >
  > Doesn't each command know whether it should unhide or not?
  
  
  Thanks for all the ideas, I have send the new series as https://phab.mercurial-scm.org/D1730 - https://phab.mercurial-scm.org/D1735 as they are entirely different changesets.

REPOSITORY
  rHG Mercurial

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

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

Patch

diff --git a/mercurial/revset.py b/mercurial/revset.py
--- a/mercurial/revset.py
+++ b/mercurial/revset.py
@@ -2163,26 +2163,25 @@ 
 
 def match(ui, spec, repo=None):
     """Create a matcher for a single revision spec"""
-    return matchany(ui, [spec], repo=repo)
+
+    if not specs:
+        return emptymatcher
+    if not all(specs):
+        raise error.ParseError(_("empty query"))
+
+    tree = buildtree([spec], repo)
+    return matchany(ui, tree, repo=repo)
 
 def emptymatcher(repo, subset=None):
     """ Matcher for empty specs """
     return baseset()
 
-def matchany(ui, specs, repo=None, localalias=None):
-    """Create a matcher that will include any revisions matching one of the
-    given specs
+def matchany(ui, tree, repo=None, localalias=None):
+    """Create a matcher for the tree
 
     If localalias is not None, it is a dict {name: definitionstring}. It takes
     precedence over [revsetalias] config section.
     """
-    if not specs:
-        return emptymatcher
-    if not all(specs):
-        raise error.ParseError(_("empty query"))
-
-    tree = buildtree(specs, repo)
-
     aliases = []
     warn = None
     if ui:
diff --git a/mercurial/localrepo.py b/mercurial/localrepo.py
--- a/mercurial/localrepo.py
+++ b/mercurial/localrepo.py
@@ -804,11 +804,19 @@ 
         definitions overriding user aliases, set ``localalias`` to
         ``{name: definitionstring}``.
         '''
+
+        if not specs:
+            return revset.emptymatcher(self)
+        if not all(specs):
+            raise error.ParseError(_("empty query"))
+
         if user:
-            m = revset.matchany(self.ui, specs, repo=self,
+            tree = revset.buildtree(specs, self)
+            m = revset.matchany(self.ui, tree, repo=self,
                                 localalias=localalias)
         else:
-            m = revset.matchany(None, specs, localalias=localalias)
+            tree = revset.buildtree(specs)
+            m = revset.matchany(None, tree, localalias=localalias)
         return m(self)
 
     def url(self):