This is an archive of the discontinued Mercurial Phabricator instance.

Differential D1678

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

Authored by pulkit on Dec 12 2017, 8:55 PM.

Download Raw Diff

Details

Reviewers

None

Group Reviewers

hg-reviewers

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.

Diff Detail

Repository

rHG Mercurial

Lint

Lint Skipped

Unit

Unit Tests Skipped

Event Timeline

pulkit created this revision.Dec 12 2017, 8:55 PM

Herald added a reviewer: hg-reviewers. · View Herald TranscriptDec 12 2017, 8:55 PM

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

@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)

In 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)

In D1678#28729, @yuja wrote:

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

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?

In 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 D1730 - D1735 as they are entirely different changesets.

Revision Contents
Changeset List

			Path	Packages
M			mercurial/localrepo.py (12 lines)
M			mercurial/revset.py (21 lines)

Status	Author	Revision
Abandoned	pulkit	D1678 revset: pass pre-optimized tree in revset.matchany()
Abandoned	pulkit	D1677 revset: extract the logic to build the tree in it's own function
Abandoned	pulkit	D1676 revset: take out logic generating matcher for empty spec in it's own fn

Diff 4414

mercurial/localrepo.py

	def anyrevs(self, specs, user=False, localalias=None):			def anyrevs(self, specs, user=False, localalias=None):
	'''Find revisions matching one of the given revsets.			'''Find revisions matching one of the given revsets.

	Revset aliases from the configuration are not expanded by default. To			Revset aliases from the configuration are not expanded by default. To
	expand user aliases, specify ``user=True``. To provide some local			expand user aliases, specify ``user=True``. To provide some local
	definitions overriding user aliases, set ``localalias`` to			definitions overriding user aliases, set ``localalias`` to
	``{name: definitionstring}``.			``{name: definitionstring}``.
	'''			'''

				if not specs:
				return revset.emptymatcher(self)
				if not all(specs):
				raise error.ParseError(_("empty query"))

	if user:			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)			localalias=localalias)
	else:			else:
	m = revset.matchany(None, specs, localalias=localalias)			tree = revset.buildtree(specs)
				m = revset.matchany(None, tree, localalias=localalias)
	return m(self)			return m(self)

	def url(self):			def url(self):
	return 'file:' + self.root			return 'file:' + self.root

	def hook(self, name, throw=False, **args):			def hook(self, name, throw=False, **args):
	"""Call a hook, passing this repo instance.			"""Call a hook, passing this repo instance.

mercurial/revset.py

	else:			else:
	tree = ('or',			tree = ('or',
	('list',) + tuple(revsetlang.parse(s, lookup) for s in specs))			('list',) + tuple(revsetlang.parse(s, lookup) for s in specs))

	return tree			return tree

	def match(ui, spec, repo=None):			def match(ui, spec, repo=None):
	"""Create a matcher for a single revision spec"""			"""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):			def emptymatcher(repo, subset=None):
	""" Matcher for empty specs """			""" Matcher for empty specs """
	return baseset()			return baseset()

	def matchany(ui, specs, repo=None, localalias=None):			def matchany(ui, tree, repo=None, localalias=None):
	"""Create a matcher that will include any revisions matching one of the			"""Create a matcher for the tree
	given specs

	If localalias is not None, it is a dict {name: definitionstring}. It takes			If localalias is not None, it is a dict {name: definitionstring}. It takes
	precedence over [revsetalias] config section.			precedence over [revsetalias] config section.
	"""			"""
	if not specs:
	return emptymatcher
	if not all(specs):
	raise error.ParseError(_("empty query"))

	tree = buildtree(specs, repo)

	aliases = []			aliases = []
	warn = None			warn = None
	if ui:			if ui:
	aliases.extend(ui.configitems('revsetalias'))			aliases.extend(ui.configitems('revsetalias'))
	warn = ui.warn			warn = ui.warn
	if localalias:			if localalias:
	aliases.extend(localalias.items())			aliases.extend(localalias.items())
	if aliases:			if aliases:

Diff	ID	Base	Description	Created	Lint	Unit
Base			Base
Diff 1	4414			Dec 12 2017, 8:55 PM	★	★