Patchwork D1143: revset: update repo.pinnedrevs with hidden commits from the tree

login
register
mail settings
Submitter phabricator
Date Oct. 17, 2017, 12:19 p.m.
Message ID <differential-rev-PHID-DREV-za42bemhkn4nkxktkg72-req@phab.mercurial-scm.org>
Download mbox | patch
Permalink /patch/25092/
State Superseded
Headers show

Comments

phabricator - Oct. 17, 2017, 12:19 p.m.
pulkit created this revision.
Herald added a subscriber: mercurial-devel.
Herald added a reviewer: hg-reviewers.

REVISION SUMMARY
  This patch adds functionality to update the pinnedrevs set with the hidden
  commits whose hashes are passed if the command allow accessing them and showing
  warning depending on repo.filtername which is set by the type of command.
  
  The logic to check whether a symbol is hash and logic related to showing
  warning to user is imported from directacess extension from fb-hgext.

REPOSITORY
  rHG Mercurial

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

AFFECTED FILES
  mercurial/revset.py

CHANGE DETAILS




To: pulkit, #hg-reviewers
Cc: mercurial-devel
phabricator - Oct. 18, 2017, 4:32 p.m.
indygreg requested changes to this revision.
indygreg added a comment.
This revision now requires changes to proceed.


  I'm not going to queue the remainder of this series because I'm unsure what the decisions around this feature are. I will add some nits on the code though.

INLINE COMMENTS

> revset.py:2207
> +    # accessing hidden commmits is allowed
> +    if repo and repo.filtername in ['visible-hidden', 'visible-warnhidden']:
> +        _updatepinnedrevs(tree, repo)

Nit: this should be a tuple, not a list. Tuples are immutable and I believe Python will compile it once instead of allocating a new list object every invocation.

> revset.py:2283
> +        try:
> +            revnode = cl._partialmatch(revnode)
> +        except error.LookupError:

I'm not sure if the number of items in `symbols` is expected to be large, but if it is, aliasing `cl._partialmatch` to avoid an attribute lookup is a justifiable micro-optimization.

> revset.py:2288
> +            rev = cl.rev(revnode)
> +            if rev not in repo.changelog:
> +                hiddenset.add(rev)

`repo.changelog` in a loop is bad for perf. Please alias the changelog to a local variable.

> revset.py:2291-2292
> +
> +    if hiddenset:
> +        if repo.filtername == 'visible-warnhidden':
> +            repo.ui.warn(_("warning: accessing hidden changesets %s "

Nit: these 2 lines can be combined to avoid extra indentation.

> revset.py:2293-2296
> +            repo.ui.warn(_("warning: accessing hidden changesets %s "
> +                           "for write operation\n") %
> +                         (",".join([str(repo.unfiltered()[l])
> +                          for l in hiddenset])))

Nit: I think it is better to have the list of hidden changesets after the message, not in the middle of it.

Nit: I /think/ "," should be localized.

Nit: str should probably be replaced with something from pycompat for Python 3 compatibility?

REPOSITORY
  rHG Mercurial

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

To: pulkit, #hg-reviewers, indygreg
Cc: indygreg, dlax, mercurial-devel
phabricator - Dec. 6, 2017, 2:16 p.m.
yuja requested changes to this revision.
yuja added a comment.
This revision now requires changes to proceed.


  Can't we apply `_updateexceptions()` out of the `revset.match*()`?
  It's horrible that a revset query does mutate the repo.
  
    # somewhere near scmutil.revrange()
    tree = revsetlang.parse(spec)
    symbols = revsetlang.hashlikesymbols(tree)
    ... continue _updateexceptions()

INLINE COMMENTS

> revset.py:2241
> +        n = int(symbol)
> +        if n <= maxrev:
> +            # It's a rev number

Perhaps this can be moved to `_updateexceptions()` phase, so
that we can simply collect hash-like strings without the knowledge
of the tip revision.

> revset.py:2248
> +
> +def gethashsymbols(tree, maxrev):
> +    """

Please move this to `revsetlang` module.

> revset.py:2258
> +    results = []
> +    if len(tree) in (2, 3) and tree[0] == "symbol":
> +        results.append(tree[1])

No need to test the length.

> revset.py:2260
> +        results.append(tree[1])
> +    elif tree[0] == "func" and tree[1] == _listtuple:
> +        # the optimiser will group sequence of hash request

We can get rid of it by processing a pre-optimized tree.

> revset.py:2266
> +            results += gethashsymbols(subtree, maxrev)
> +        # return directly, we don't need to filter symbols again
> +        return results

Seems tricky. Instead, test the _ishashsymbol() before adding
to the results list?

REPOSITORY
  rHG Mercurial

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

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

Patch

diff --git a/mercurial/revset.py b/mercurial/revset.py
--- a/mercurial/revset.py
+++ b/mercurial/revset.py
@@ -2202,6 +2202,10 @@ 
     tree = revsetlang.analyze(tree)
     tree = revsetlang.optimize(tree)
     posttreebuilthook(tree, repo)
+    # add commits to pinned revs if hashes of hidden revs is passed and
+    # accessing hidden commmits is allowed
+    if repo and repo.filtername in ['visible-hidden', 'visible-warnhidden']:
+        _updatepinnedrevs(tree, repo)
     return makematcher(tree)
 
 def makematcher(tree):
@@ -2230,3 +2234,66 @@ 
 
 # tell hggettext to extract docstrings from these functions:
 i18nfunctions = symbols.values()
+
+hashre = util.re.compile('[0-9a-fA-F]{1,40}')
+_listtuple = ('symbol', '_list')
+
+def _ishashsymbol(symbol, maxrev):
+    """ returns true if symbol looks like a hash """
+
+    try:
+        n = int(symbol)
+        if n <= maxrev:
+            # It's a rev number
+            return False
+    except ValueError:
+        pass
+    return hashre.match(symbol)
+
+def gethashsymbols(tree, maxrev):
+    """ returns the list of symbols of the tree that look like hashes
+    for example for the revset 3::abe3ff it will return ('abe3ff') """
+
+    if not tree:
+        return []
+
+    results = []
+    if len(tree) in (2, 3) and tree[0] == "symbol":
+        results.append(tree[1])
+    elif tree[0] == "func" and tree[1] == _listtuple:
+        # the optimiser will group sequence of hash request
+        results += tree[2][1].split('\0')
+    elif len(tree) >= 2:
+        for subtree in tree[1:]:
+            results += gethashsymbols(subtree, maxrev)
+        # return directly, we don't need to filter symbols again
+        return results
+    return [s for s in results if _ishashsymbol(s, maxrev)]
+
+def _updatepinnedrevs(tree, repo):
+    """ extracts the symbols that looks like hashes and add them to
+    repo._pinnedrevs for accessing hidden hashes
+    """
+
+    hiddenset = set()
+    cl = repo.unfiltered().changelog
+    symbols = gethashsymbols(tree, len(cl))
+    for revnode in symbols:
+        try:
+            revnode = cl._partialmatch(revnode)
+        except error.LookupError:
+            revnode = None
+        if revnode is not None:
+            rev = cl.rev(revnode)
+            if rev not in repo.changelog:
+                hiddenset.add(rev)
+
+    if hiddenset:
+        if repo.filtername == 'visible-warnhidden':
+            repo.ui.warn(_("warning: accessing hidden changesets %s "
+                           "for write operation\n") %
+                         (",".join([str(repo.unfiltered()[l])
+                          for l in hiddenset])))
+
+        repo.pinnedrevs.update(hiddenset)
+        repo.invalidatevolatilesets()