This is an archive of the discontinued Mercurial Phabricator instance.

states: created new class to handle multi-step unfinished states
AbandonedPublic

Authored by taapas1128 on Jun 7 2019, 3:42 AM.

Download Raw Diff

Details

Reviewers

martinvonz
durin42

Group Reviewers

hg-reviewers

Summary

For the purpose of handling states for various multistep operations like
hg graft,hg histedit,hg bisect et al a new class called statecheck
is created .This will help in having a unified approach towards these commands
and handle them with ease.

The class takes in 5 basic arguments which include the name of the command, the
name of the state file associated with it , clearable flag , allowcommit flag and
stopflag which determines whether the command supports --stop option or not.

It further contains a unified way off returning status and hint messages.
The functions of the class generate these messages based on the name of the
command and stopflag.

Merge has been handles seperately with the help of predicate instead of a
filename.

This imports unfinishedstates, checkunfinished(), and clearunfinished()
from cmdutil.py and makes it compatible with the new statecheck class.
This also adds support of the new class state.statecheck
throughout core and extensions. This also removes the
previous method cmdutil.unfinishedstates and its API.

The results are shown in tests.

Diff Detail

Repository

rHG Mercurial

Lint

Lint Skipped

Unit

Unit Tests Skipped

Event Timeline

taapas1128 created this revision.Jun 7 2019, 3:42 AM

Herald added a reviewer: hg-reviewers. · View Herald TranscriptJun 7 2019, 3:42 AM

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

taapas1128 added a child revision: D6488: statecheck: added support for unfinishedstates and utilities.Jun 7 2019, 3:43 AM

We already have mercurial/state.py and there is a cmdstate class. We can extend that class to have these variables, but for now let's move this class to that file instead of creating a new one.

mercurial/statecheck.py
10 ↗	(On Diff #15359)	Nice, looks good generally. Let's add another function to class `isunfinished()` which will tell whether this operation is unfinished or not.
22 ↗	(On Diff #15359)	nit: unrequired new lines between in help
48 ↗	(On Diff #15359)	nit: remove new lines between if and else if and else
64 ↗	(On Diff #15359)	nit: return `_('') ..` instead of creating a temp variable.
69 ↗	(On Diff #15359)	once we have a `isunfinished` function, this can be part of that under `if cmd == 'merge'`

pulkit added inline comments.Jun 7 2019, 7:45 AM

mercurial/statecheck.py
40 ↗	(On Diff #15359)	We generally prepend `_` to variable names if they are private to that class. That is, we don't want them to be accessed outside of the class. This is just a coding guidelines which is quite usually followed. It will be nice, if we don't add `_` before variables which can be used outside the class. Like `clearable` maybe.
42 ↗	(On Diff #15359)	nit: `def hint(..)`
61 ↗	(On Diff #15359)	nit: `def msg`

This class seems unused. The steps I proposed were intended to give a slow transition, while having the code used at every step. This patch is *harder* for me to review than the original large patch was, because now I have to go and look at the other patches to see if this class behaves correctly. Can you follow the steps I suggested instead? Otherwise I'd prefer to just resurrect the old patch even though it's much larger than I'd like.

taapas1128 marked 5 inline comments as done.Jun 7 2019, 9:42 AM

taapas1128 updated this revision to Diff 15362.

@martinvonz okay I will do that then send the whole code divided as patches.

taapas1128 retitled this revision from states: created new class to handle unified states(gsoc-19) to states: created new class to handle multi-step operation states.Jun 7 2019, 10:25 AM

taapas1128 updated this revision to Diff 15367.

I'll mark this as "Request Changes". That way it's clearer that it's not ready for review. When you're done, you can set the status back to "Ready for Review" (or whatever the status is called).

This revision now requires changes to proceed.Jun 7 2019, 10:55 AM

@martinvonz I have updated the stack(D6487, D6488, D6494) for review up to incorporation of the checkunfinisheds() and clearunfinished() in the new API. Tests are in 3rd commit D6494. have a look whenever you are free.

This still looks unused. Can you see if you can split up according to the steps I suggested, or tell me why that won't work or why you don't think it's a good idea?

This revision now requires changes to proceed.Jun 7 2019, 12:31 PM

In D6487#94272, @martinvonz wrote:

This still looks unused. Can you see if you can split up according to the steps I suggested, or tell me why that won't work or why you don't think it's a good idea?

Oh, and I'm on vacation for a week now. It feels like we should VC about this, but that's not possible. I think Pulkit understood what I suggested, so maybe he can help explain in more detail of necessary?

D6487 : A new statecheck class is created with its utility functions.
D6488 : A new list of statecheck objects is created and checkunfinished and clearunfinished are imported and modified in accordance to the class and the new list
D6494 : cmdutil.checkunfinished and cmdutil.clearunfinished is removed . Throughout core and extensions they are replaced by statemod.checkunfinished and statemod.clearunfinished. Also, the previous appends in extensions that were made to cmdutil.unfinishedstates are now made to statemod.unfinishedstates as statecheck objects. Tests results are stated
D6495 : Support for the functionalities is added to`cmdutil.STATES` is added to statemod.unfinishedstates. So cmdutil.STATES is completely removed . Tests results are stated.

@martinvonz @pulkit : Have a look now the stack is complete.

Can you squash the next two patches into this one? It's hard to review it in its current form, because you need to go back and forth between the first three patches to understand how the code was moved.

This revision now requires changes to proceed.Jun 8 2019, 10:33 AM

@martinvonz check it out I haveve done that.

In D6487#94341, @taapas1128 wrote:

@martinvonz check it out I haveve done that.

Not according to Phabricator. Maybe you need to update the state in Phabricator?

This revision now requires changes to proceed.Jun 8 2019, 11:29 AM

@martinvonz Is it alright now?

taapas1128 requested review of this revision.Jun 8 2019, 11:43 AM

Regarding the message change that was previously in D6494 :

Doing that would be kind of difficult because these changes are not due to refactoring but due to statecheck.hint() which is a unified function for generating hint messages. If I change the messages in accordance to cmdutil.unfinishedstates then this problem will be there in the in integration of STATES. If you still want it I can do that by removing statecheck.hint() completely and getting the messages from cmdutil.py. So should I do that ?

In D6487#94348, @taapas1128 wrote:

@martinvonz Is it alright now?

No, this patch still appears to add a new class without updating callers. I can't tell if the new code does the same thing as the old code without looking at later patches.

This revision now requires changes to proceed.Jun 8 2019, 12:03 PM

So should I squash the D6487 and D6488 too? That would make this patch really long as the very first patch that I sent?

taapas1128 retitled this revision from states: created new class to handle multi-step operation states to states: created new class to handle multi-step unfinished states.Jun 8 2019, 12:30 PM

taapas1128 edited the summary of this revision. (Show Details)

taapas1128 updated this revision to Diff 15390.

Herald added a reviewer: durin42. · View Herald TranscriptJun 8 2019, 12:30 PM

Herald added subscribers: Kwan, mjpieters. · View Herald Transcript

taapas1128 edited child revisions, added: D6495: statecheck: added functionality for cmdutil.STATES; removed: D6488: statecheck: added support for unfinishedstates and utilities.Jun 8 2019, 12:31 PM

@martinvonz: Have a look now.

In D6487#94391, @taapas1128 wrote:

So should I squash the D6487 and D6488 too?

Yes, please do.

That would make this patch really long as the very first patch that I sent?

Well, you could split them up as I suggested earlier :)

I have squashed them now . Splitting them into exactly those steps would not have been possible that is why is split them this way. Is this alright for your reviewing or should I try splitting them again? :(

taapas1128 abandoned this revision.Jun 8 2019, 5:00 PM

In D6487#94407, @taapas1128 wrote:

Splitting them into exactly those steps would not have been possible

Why not? The current form is better than before, but I would like to know why my suggestion is not possible. My suggested step 1 is surely possible.

mercurial/cmdutil.py
3301 ↗	(On Diff #15390)	Since bisect was intentionally excluded before this patch, it's probably best to add it in a separate patch. That way you get a place (i.e. the commit message) to explain why it was excluded before and why it should no longer be excluded.
mercurial/state.py
104	Why?

Please ignore my latest messages. I had not noticed yet that this was abandoned.

Revision Contents
Changeset List

			Path	Packages
M			mercurial/state.py (65 lines)

Commit	Parents	Author	Summary	Date
3e83cc0296e7	a97b12f726e4	Taapas Agrawal		Jun 7 2019, 9:59 AM

Status	Author	Revision
Abandoned	taapas1128	D6495 statecheck: added functionality for cmdutil.STATES
Abandoned	taapas1128	D6494 statecheck: added support for the statecheck.unfinishedstates
Abandoned	taapas1128	D6488 statecheck: added support for unfinishedstates and utilities
Abandoned	taapas1128	D6487 states: created new class to handle multi-step unfinished states

Diff 15367

mercurial/state.py

	.hg/ directory.			.hg/ directory.

	We store the data on disk in cbor, for which we use the CBOR format to encode			We store the data on disk in cbor, for which we use the CBOR format to encode
	the data.			the data.
	"""			"""

	from __future__ import absolute_import			from __future__ import absolute_import

				from .i18n import _

	from . import (			from . import (
	error,			error,
	util,			util,
	)			)
	from .utils import (			from .utils import (
	cborutil,			cborutil,
	)			)


	def delete(self):			def delete(self):
	"""drop the state file if exists"""			"""drop the state file if exists"""
	util.unlinkpath(self._repo.vfs.join(self.fname), ignoremissing=True)			util.unlinkpath(self._repo.vfs.join(self.fname), ignoremissing=True)

	def exists(self):			def exists(self):
	"""check whether the state file exists or not"""			"""check whether the state file exists or not"""
	return self._repo.vfs.exists(self.fname)			return self._repo.vfs.exists(self.fname)

				class statecheck(object):
				"""a utility class that will to deal with multistep operations
				like graft, histedit, bisect, update etc and check whether such commands
				are in an unfinished conditition of not and return appropriate message
				and hint.
				It also has the ability to register and determine the states of any new
				multistep operation or multistep command extension.
				"""

				def __init__(self, cmdname, fname, clearable=False, allowcommit=False,
				stopflag=False):
				"""cmdname is the name the command
				fname is the file name in which data should be stored in .hg directory.
				It is None for merge command.
				martinvonzUnsubmitted Not Done Why? martinvonz: Why?
				clearable boolean determines whether or not interrupted states can be
				cleared by running `hg update -C .`
				allowcommit boolean decides whether commit is allowed during interrupted
				state or not.
				stopflag is a boolean that determines whether or not a command supports
				--stop flag
				"""
				self.cmdname = cmdname
				self.fname = fname
				self.clearable = clearable
				self.allowcommit = allowcommit
				self.stopflag = stopflag

				def hint(self):
				"""returns the hint message corresponding to the command"""
				if self.cmdname == 'bisect':
				msg = _('To mark the changeset good: hg bisect --good\n'
				'To mark the changeset bad: hg bisect --bad\n'
				'To abort: hg bisect --reset\n')
				elif self.cmdname == 'update':
				msg = _("use 'hg update' to get a consistent checkout")
				else:
				msg = (_('To continue: hg %s --continue\n'
				'To abort: hg %s --abort') % (self.cmdname,
				self.cmdname))
				if self.stopflag:
				msg = msg + (_('\nTo stop: hg %s --stop') %
				(self.cmdname))
				return msg

				def msg(self):
				"""returns the status message corresponding to the command"""
				if self.cmdname == 'update':
				return _('last update was interrupted')
				else:
				return _('%s in progress') % (self.cmdname)

				def inunfinished(self, repo, mergecheck=False):
				"""determines whether a multi-step operation is in progress
				or not
				mergecheck flag helps in determining state specifically for merge
				"""
				if self.cmdname == 'merge' or mergecheck:
				return len(repo[None].parents()) > 1
				else:
				return repo.vfs.exists(self.fname)

Diff	ID	Description	Created	Lint	Unit
Base		Base
Diff 1	15359		Jun 7 2019, 3:42 AM	★	★
Diff 2	15362		Jun 7 2019, 9:41 AM	★	★
Diff 3	15367		Jun 7 2019, 10:25 AM	★	★
Diff 4	15390		Jun 8 2019, 12:30 PM	★	★