This is an archive of the discontinued Mercurial Phabricator instance.

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

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

Download Raw Diff

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

Herald added a reviewer: hg-reviewers. · View Herald TranscriptOct 17 2017, 8:19 AM

Herald added a subscriber: mercurial-devel. · View Herald Transcript

pulkit added a child revision: D1144: directaccess: add tests for read only-commands.Oct 17 2017, 8:19 AM

dlax added a subscriber: dlax.Oct 17 2017, 3:09 PM

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
2207	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.
2283	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.
2288	`repo.changelog` in a loop is bad for perf. Please alias the changelog to a local variable.
2291–2292	Nit: these 2 lines can be combined to avoid extra indentation.
2293–2296	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 edited parent revisions, added: D1288: dispatch: filter the repository depending on func.cmdtype; removed: D1142: dispatch: filter the repository depending on func.cmdtype.Nov 2 2017, 2:12 PM

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

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
2246	Perhaps this can be moved to `_updateexceptions()` phase, so that we can simply collect hash-like strings without the knowledge of the tip revision.
2253	Please move this to `revsetlang` module.
2263	No need to test the length.
2265	We can get rid of it by processing a pre-optimized tree.
2271	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

			Path	Packages
M			mercurial/revset.py (67 lines)

Status	Author	Revision
Abandoned	pulkit	D1286 repoview: consider visibility exceptions in filterrevs()
Abandoned	pulkit	D1144 directaccess: add tests for read only-commands
Abandoned	pulkit	D1143 revset: update visibility exceptions with hidden commits from the tree
Abandoned	pulkit	D1288 dispatch: filter the repository depending on func.cmdtype
Abandoned	pulkit	D1287 repoview: add two new filternames to be used for accessing hidden commits
Abandoned	pulkit	D1583 repoview: consider visibilityexceptions while considering filterrevs
Abandoned	pulkit	D1492 repoview: take out the logic of computehidden to a new function
Abandoned	pulkit	D1285 repoview: add a new attribute _visibilityexceptions and related API

Diff 2905

mercurial/revset.py

	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 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']:
				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…
				_updatepinnedrevs(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":
				results.append(tree[1])
				elif tree[0] == "func" and tree[1] == _listtuple:
				yujaUnsubmitted Not Done No need to test the length. yuja: No need to test the length.
				# the optimiser will group sequence of hash request
				results += tree[2][1].split('\0')
				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.
				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)]
				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?

				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)
				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…
				except error.LookupError:
				revnode = None
				if revnode is not None:
				rev = cl.rev(revnode)
				if rev not in repo.changelog:
				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.
				hiddenset.add(rev)

				if hiddenset:
				if repo.filtername == 'visible-warnhidden':
				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.
				repo.ui.warn(_("warning: accessing hidden changesets %s "
				"for write operation\n") %
				(",".join([str(repo.unfiltered()[l])
				for l in hiddenset])))
				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…

				repo.pinnedrevs.update(hiddenset)
				repo.invalidatevolatilesets()

Diff	ID	Description	Created	Lint	Unit
Base		Base
Diff 1	2905		Oct 17 2017, 8:19 AM	★	★
Diff 2	3205		Nov 2 2017, 2:05 PM	★	★
Diff 3	4097		Dec 4 2017, 9:17 AM	★	★