This is an archive of the discontinued Mercurial Phabricator instance.

Differential D1143

revset: update visibility exceptions with hidden commits from the tree
AbandonedPublic

Authored by pulkit on Oct 17 2017, 8:19 AM.

Details

Reviewers
indygreg
yuja
Group Reviewers
hg-reviewers
Summary

This patch adds functionality to update the visibility exceptions 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.

Diff Detail

Repository
rHG Mercurial
Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

pulkit created this revision.Oct 17 2017, 8:19 AM
dlax added a subscriber: dlax.Oct 17 2017, 3:09 PM
indygreg requested changes to this revision.Oct 18 2017, 12:31 PM
indygreg added a subscriber: indygreg.

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.

mercurial/revset.py
2201

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.

2278

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.

2283

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

2286–2287

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

2288–2291

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?

This revision now requires changes to proceed.Oct 18 2017, 12:31 PM
pulkit edited the summary of this revision. (Show Details)Nov 2 2017, 2:05 PM
pulkit retitled this revision from revset: update repo.pinnedrevs with hidden commits from the tree to revset: update visibility exceptions with hidden commits from the tree.
pulkit updated this revision to Diff 3205.
pulkit planned changes to this revision.Nov 22 2017, 8:24 PM
pulkit updated this revision to Diff 4097.Dec 4 2017, 9:17 AM
yuja requested changes to this revision.Dec 6 2017, 9:15 AM
yuja added a subscriber: yuja.

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()
mercurial/revset.py
2241

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

2248

Please move this to revsetlang module.

2258

No need to test the length.

2260

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

2266

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

This revision now requires changes to proceed.Dec 6 2017, 9:15 AM
pulkit abandoned this revision.Feb 14 2018, 11:26 AM

Revision Contents
Changeset List

PathPackages
Mmercurial/revset.py (74 lines)

Diff 4097

mercurial/revset.py

error, error,
hbisect, hbisect,
match as matchmod, match as matchmod,
node, node,
obsolete as obsmod, obsolete as obsmod,
obsutil, obsutil,
pathutil, pathutil,
phases, phases,
pycompat,
registrar, registrar,
repoview, repoview,
revsetlang, revsetlang,
scmutil, scmutil,
smartset, smartset,
util, util,
) )
if localalias: if localalias:
aliases.extend(localalias.items()) aliases.extend(localalias.items())
if aliases: if aliases:
tree = revsetlang.expandaliases(tree, aliases, warn=warn) tree = revsetlang.expandaliases(tree, aliases, warn=warn)
tree = revsetlang.foldconcat(tree) tree = revsetlang.foldconcat(tree)
tree = revsetlang.analyze(tree) tree = revsetlang.analyze(tree)
tree = revsetlang.optimize(tree) tree = revsetlang.optimize(tree)
posttreebuilthook(tree, repo) posttreebuilthook(tree, repo)
# add revs to visibility exceptions if hashes of hidden revs is passed and
# accessing hidden commmits is allowed
if repo and repo.filtername in ('visible-allowhidden',
indygregUnsubmitted
Not Done

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.

indygreg: Nit: this should be a tuple, not a list. Tuples are immutable and I believe Python will compile…
'visible-warnhidden'):
_updateexceptions(tree, repo)
return makematcher(tree) return makematcher(tree)
def makematcher(tree): def makematcher(tree):
"""Create a matcher from an evaluatable tree""" """Create a matcher from an evaluatable tree"""
def mfunc(repo, subset=None, order=None): def mfunc(repo, subset=None, order=None):
if order is None: if order is None:
if subset is None: if subset is None:
order = defineorder # 'x' order = defineorder # 'x'
if func._safe: if func._safe:
safesymbols.add(name) safesymbols.add(name)
# load built-in predicates explicitly to setup safesymbols # load built-in predicates explicitly to setup safesymbols
loadpredicate(None, None, predicate) loadpredicate(None, None, predicate)
# tell hggettext to extract docstrings from these functions: # tell hggettext to extract docstrings from these functions:
i18nfunctions = symbols.values() 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:
yujaUnsubmitted
Not Done

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

yuja: Perhaps this can be moved to `_updateexceptions()` phase, so that we can simply collect hash…
# It's a rev number
return False
except ValueError:
pass
return hashre.match(symbol)
def gethashsymbols(tree, maxrev):
yujaUnsubmitted
Not Done

Please move this to revsetlang module.

yuja: Please move this to `revsetlang` module.
"""
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":
yujaUnsubmitted
Not Done

No need to test the length.

yuja: No need to test the length.
results.append(tree[1])
elif tree[0] == "func" and tree[1] == _listtuple:
yujaUnsubmitted
Not Done

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

yuja: We can get rid of it by processing a pre-optimized tree.
# 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
yujaUnsubmitted
Not Done

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

yuja: Seems tricky. Instead, test the _ishashsymbol() before adding to the results list?
return results
return [s for s in results if _ishashsymbol(s, maxrev)]
def _updateexceptions(tree, repo):
"""
extracts the symbols that looks like hashes and add them to
visibility exceptions set for accessing hidden hashes
"""
hiddenset = set()
unfi = repo.unfiltered()
unficl = unfi.changelog
indygregUnsubmitted
Not Done

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.

indygreg: I'm not sure if the number of items in `symbols` is expected to be large, but if it is…
cl = repo.changelog
symbols = gethashsymbols(tree, len(unficl))
pmatch = unficl._partialmatch
for revnode in symbols:
try:
indygregUnsubmitted
Not Done

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

indygreg: `repo.changelog` in a loop is bad for perf. Please alias the changelog to a local variable.
revnode = pmatch(revnode)
except error.LookupError:
revnode = None
if revnode is not None:
indygregUnsubmitted
Not Done

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

indygreg: Nit: these 2 lines can be combined to avoid extra indentation.
rev = unficl.rev(revnode)
if rev not in cl:
hiddenset.add(rev)
indygregUnsubmitted
Not Done

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?

indygreg: Nit: I think it is better to have the list of hidden changesets after the message, not in the…
if hiddenset:
if repo.filtername == 'visible-warnhidden':
hsets = _(",").join([pycompat.bytestr(unfi[l]) for l in hiddenset])
repo.ui.warn(_("warning: accessing hidden changesets for write "
"operation: %s\n") % hsets)
repo.addvisibilityexceptions(hiddenset)
repo.invalidatevolatilesets()