This is an archive of the discontinued Mercurial Phabricator instance.

Differential D3669

cmdutil: have statefile names in STATES instead of functions
Changes PlannedPublic

Authored by pulkit on May 29 2018, 6:22 PM.

Download Raw Diff

Details

Reviewers

None

Group Reviewers

hg-reviewers

Summary

This will help us in unifying cmdutil.STATES and cmdutil.unfinishedstates as the
checking logic for STATES is moved to _getrepostate() and STATES now have just
filenames which is similar to unfinishedstates.

Diff Detail

Repository

rHG Mercurial

Lint

Lint Skipped

Unit

Unit Tests Skipped

Event Timeline

pulkit created this revision.May 29 2018, 6:22 PM

Herald added a reviewer: hg-reviewers. · View Herald TranscriptMay 29 2018, 6:22 PM

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

pulkit added a child revision: D3670: cmdutil: don't return statedetectionpredicate function from _getrepostate().May 29 2018, 6:22 PM

martinvonz added a subscriber: martinvonz.May 29 2018, 6:59 PM

martinvonz added inline comments.

mercurial/cmdutil.py
649–650	The point of the STATES dict (as opposed to putting all the code here) seems to be to make it generic and extensible. Handling "merge" differently here seems like a step backwards. How about instead making unfinishedstates more generic by supporting a predicate there instead of the file? We could still allow files as well for BC for a little while.

pulkit added inline comments.Jun 1 2018, 6:50 AM

mercurial/cmdutil.py
3210	How about instead making unfinishedstates more generic by supporting a predicate there instead of the file? We could still allow files as well for BC for a little while. @martinvonz : because of above util.unlink() we have to keep the filenames in unfinishedstates. :(

martinvonz added inline comments.Jun 1 2018, 1:07 PM

mercurial/cmdutil.py
3210	We don't have to. We could add a `clearstatefn` function in addition to the `statedetectionpredicate`. Perhaps the `if not clearable and ...` stuff could move into the function in that case

pulkit added inline comments.Jun 3 2018, 8:08 AM

mercurial/cmdutil.py
3210	Don't you think it's getting a bit complicated?

martinvonz added inline comments.Jun 11 2018, 3:55 PM

mercurial/cmdutil.py
3210	It's a bit more complicated, I agree, but I think it's better than the alternative. If it removes the need for clearable, it's not that much more complicated. An alternative would be to create an object with [is]dirty(), allowcommit(), and clear() methods and have a function that creates such an object for file-based states (all but .hg/merge?). That would reduce the duplication of fileexistspredicate(<name>) and clearfunction(<name>). PS. Sorry about the delay. I've been away.

pulkit added inline comments.Jul 1 2018, 5:27 PM

mercurial/cmdutil.py
3210	(Sorry, I missed this one too. I have turned off personal emails from phabricator and use mercurial-devel to keep track of people replying to me.) The object sounds good API. I will try that.

I won't have time before the upcoming freeze to get this done.

Revision Contents
Changeset List

			Path	Packages
M			mercurial/cmdutil.py (21 lines)

Commit	Parents	Author	Summary	Date
		Pulkit Goyal		May 29 2018, 6:08 PM

	Status	Author	Revision
	Changes Planned	pulkit	D3670 cmdutil: don't return statedetectionpredicate function from _getrepostate()
	Changes Planned	pulkit	D3669 cmdutil: have statefile names in STATES instead of functions

Diff 8924

mercurial/cmdutil.py

	def fileexistspredicate(filename):			def fileexistspredicate(filename):
	return lambda repo: repo.vfs.exists(filename)			return lambda repo: repo.vfs.exists(filename)

	def _mergepredicate(repo):			def _mergepredicate(repo):
	return len(repo[None].parents()) > 1			return len(repo[None].parents()) > 1

	STATES = (			STATES = (
	# (state, predicate to detect states, helpful message function)			# (state, predicate to detect states, helpful message function)
	('histedit', fileexistspredicate('histedit-state'), _histeditmsg),			('histedit', 'histedit-state', _histeditmsg),
	('bisect', fileexistspredicate('bisect.state'), _bisectmsg),			('bisect', 'bisect.state', _bisectmsg),
	('graft', fileexistspredicate('graftstate'), _graftmsg),			('graft', 'graftstate', _graftmsg),
	('unshelve', fileexistspredicate('unshelverebasestate'), _unshelvemsg),			('unshelve', 'unshelverebasestate', _unshelvemsg),
	('rebase', fileexistspredicate('rebasestate'), _rebasemsg),			('rebase', 'rebasestate', _rebasemsg),
	# The merge state is part of a list that will be iterated over.			# The merge state is part of a list that will be iterated over.
	# They need to be last because some of the other unfinished states may also			# They need to be last because some of the other unfinished states may also
	# be in a merge or update state (eg. rebase, histedit, graft, etc).			# be in a merge or update state (eg. rebase, histedit, graft, etc).
	# We want those to have priority.			# We want those to have priority.
	('merge', _mergepredicate, _mergemsg),			('merge', None, _mergemsg),
	)			)

	def _getrepostate(repo):			def _getrepostate(repo):
	# experimental config: commands.status.skipstates			# experimental config: commands.status.skipstates
	skip = set(repo.ui.configlist('commands', 'status.skipstates'))			skip = set(repo.ui.configlist('commands', 'status.skipstates'))
	for state, statedetectionpredicate, msgfn in STATES:			for state, filename, msgfn in STATES:
	if state in skip:			if state in skip:
	continue			continue
	if statedetectionpredicate(repo):			detected = False
				if state == 'merge':
				detected = _mergepredicate(repo)
				martinvonzUnsubmitted Not Done The point of the STATES dict (as opposed to putting all the code here) seems to be to make it generic and extensible. Handling "merge" differently here seems like a step backwards. How about instead making unfinishedstates more generic by supporting a predicate there instead of the file? We could still allow files as well for BC for a little while. martinvonz: The point of the STATES dict (as opposed to putting all the code here) seems to be to make it…
				else:
				detected = repo.vfs.exists(filename)
				if detected:
	return (state, msgfn)			return (state, msgfn)

	def morestatus(repo, fm):			def morestatus(repo, fm):
	statetuple = _getrepostate(repo)			statetuple = _getrepostate(repo)
	label = 'status.morestatus'			label = 'status.morestatus'
	if statetuple:			if statetuple:
	fm.startitem()			fm.startitem()
	state, helpfulmsg = statetuple			state, helpfulmsg = statetuple
	'''Check for unfinished operations (as above), and clear the ones			'''Check for unfinished operations (as above), and clear the ones
	that are clearable.			that are clearable.
	'''			'''
	for f, clearable, allowcommit, msg, hint in unfinishedstates:			for f, clearable, allowcommit, msg, hint in unfinishedstates:
	if not clearable and repo.vfs.exists(f):			if not clearable and repo.vfs.exists(f):
	raise error.Abort(msg, hint=hint)			raise error.Abort(msg, hint=hint)
	for f, clearable, allowcommit, msg, hint in unfinishedstates:			for f, clearable, allowcommit, msg, hint in unfinishedstates:
	if clearable and repo.vfs.exists(f):			if clearable and repo.vfs.exists(f):
	util.unlink(repo.vfs.join(f))			util.unlink(repo.vfs.join(f))
				pulkitAuthorUnsubmitted Not Done How about instead making unfinishedstates more generic by supporting a predicate there instead of the file? We could still allow files as well for BC for a little while. @martinvonz : because of above util.unlink() we have to keep the filenames in unfinishedstates. :( pulkit: >How about instead making unfinishedstates more generic by supporting a predicate there >…
				martinvonzUnsubmitted Not Done We don't have to. We could add a `clearstatefn` function in addition to the `statedetectionpredicate`. Perhaps the `if not clearable and ...` stuff could move into the function in that case martinvonz: We don't have to. We could add a `clearstatefn` function in addition to the…
				pulkitAuthorUnsubmitted Not Done Don't you think it's getting a bit complicated? pulkit: Don't you think it's getting a bit complicated?
				martinvonzUnsubmitted Not Done It's a bit more complicated, I agree, but I think it's better than the alternative. If it removes the need for clearable, it's not that much more complicated. An alternative would be to create an object with [is]dirty(), allowcommit(), and clear() methods and have a function that creates such an object for file-based states (all but .hg/merge?). That would reduce the duplication of fileexistspredicate(<name>) and clearfunction(<name>). PS. Sorry about the delay. I've been away. martinvonz: It's a bit more complicated, I agree, but I think it's better than the alternative. If it…
				pulkitAuthorUnsubmitted Not Done (Sorry, I missed this one too. I have turned off personal emails from phabricator and use mercurial-devel to keep track of people replying to me.) The object sounds good API. I will try that. pulkit: (Sorry, I missed this one too. I have turned off personal emails from phabricator and use…

	afterresolvedstates = [			afterresolvedstates = [
	('graftstate',			('graftstate',
	_('hg graft --continue')),			_('hg graft --continue')),
	]			]

	def howtocontinue(repo):			def howtocontinue(repo):
	'''Check for an unfinished operation and return the command to finish			'''Check for an unfinished operation and return the command to finish

Diff	ID	Base	Description	Created	Lint	Unit
Base			Base
Diff 1	8924			May 29 2018, 6:22 PM	★	★