Patchwork D6487: states: created new class to handle unified states(gsoc-19)

login
register
mail settings
Submitter phabricator
Date June 7, 2019, 7:42 a.m.
Message ID <differential-rev-PHID-DREV-qfm5nv6rdjsryivbuz4v-req@phab.mercurial-scm.org>
Download mbox | patch
Permalink /patch/40340/
State Superseded
Headers show

Comments

phabricator - June 7, 2019, 7:42 a.m.
taapas1128 created this revision.
Herald added a subscriber: mercurial-devel.
Herald added a reviewer: hg-reviewers.

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

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D6487

AFFECTED FILES
  mercurial/statecheck.py

CHANGE DETAILS




To: taapas1128, #hg-reviewers
Cc: mercurial-devel
phabricator - June 7, 2019, 11:40 a.m.
pulkit added a comment.


  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.

INLINE COMMENTS

> statecheck.py:10
> +
> +class statecheck(object):
> +    """a utility class that will to deal with multistep operations

Nice, looks good generally. Let's add another function to class `isunfinished()` which will tell whether this operation is unfinished or not.

> statecheck.py:22
> +        """cmdname is the name the command
> +
> +        fname is the file name in which data should be stored in .hg directory.

nit: unrequired new lines between in help

> statecheck.py:48
> +                    'To abort:                      hg bisect --reset\n')
> +
> +        elif self._cmdname == 'update':

nit: remove new lines between if and else if and else

> statecheck.py:64
> +        if self._cmdname == 'update':
> +            hint = _('last update was interrupted')
> +        else:

nit: return `_('') ..` instead of creating a temp variable.

> statecheck.py:69
> +
> +    def _mergepredicate(self, repo):
> +        """determines is a merge is in progress or not"""

once we have a `isunfinished` function, this can be part of that under `if cmd == 'merge'`

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D6487

To: taapas1128, #hg-reviewers
Cc: pulkit, mercurial-devel
phabricator - June 7, 2019, 11:45 a.m.
pulkit added inline comments.

INLINE COMMENTS

> statecheck.py:40
> +        self._allowcommit = allowcommit
> +        self._stopflag = stopflag
> +

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.

> statecheck.py:42
> +
> +    def _hintmessage(self):
> +        """returns the hint message corresponding to the command"""

nit: `def hint(..)`

> statecheck.py:61
> +
> +    def _statusmessage(self):
> +        """returns the status message corresponding to the command"""

nit: `def msg`

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D6487

To: taapas1128, #hg-reviewers
Cc: pulkit, mercurial-devel
phabricator - June 7, 2019, 12:58 p.m.
martinvonz added a comment.


  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.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D6487

To: taapas1128, #hg-reviewers
Cc: martinvonz, pulkit, mercurial-devel
phabricator - June 7, 2019, 1:49 p.m.
taapas1128 marked 3 inline comments as done and an inline comment as not done.
taapas1128 added a comment.


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

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D6487

To: taapas1128, #hg-reviewers
Cc: martinvonz, pulkit, mercurial-devel
phabricator - June 7, 2019, 2:55 p.m.
martinvonz requested changes to this revision.
martinvonz added a comment.
This revision now requires changes to proceed.


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

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D6487

To: taapas1128, #hg-reviewers, martinvonz
Cc: martinvonz, pulkit, mercurial-devel
phabricator - June 7, 2019, 4:11 p.m.
taapas1128 requested review of this revision.
taapas1128 added a comment.


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

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D6487

To: taapas1128, #hg-reviewers, martinvonz
Cc: martinvonz, pulkit, mercurial-devel
phabricator - June 7, 2019, 4:31 p.m.
martinvonz requested changes to this revision.
martinvonz added a comment.
This revision now requires changes to proceed.


  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?

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D6487

To: taapas1128, #hg-reviewers, martinvonz
Cc: martinvonz, pulkit, mercurial-devel
phabricator - June 7, 2019, 4:33 p.m.
martinvonz added a comment.


  In https://phab.mercurial-scm.org/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?

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D6487

To: taapas1128, #hg-reviewers, martinvonz
Cc: martinvonz, pulkit, mercurial-devel
phabricator - June 7, 2019, 7:52 p.m.
taapas1128 requested review of this revision.
taapas1128 added a comment.


  https://phab.mercurial-scm.org/D6487 : A new `statecheck` class is created with its utility functions.
  https://phab.mercurial-scm.org/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
  https://phab.mercurial-scm.org/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
  https://phab.mercurial-scm.org/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.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D6487

To: taapas1128, #hg-reviewers, martinvonz
Cc: martinvonz, pulkit, mercurial-devel
phabricator - June 7, 2019, 7:53 p.m.
taapas1128 added a comment.


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

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D6487

To: taapas1128, #hg-reviewers, martinvonz
Cc: martinvonz, pulkit, mercurial-devel
phabricator - June 8, 2019, 2:33 p.m.
martinvonz requested changes to this revision.
martinvonz added a comment.
This revision now requires changes to proceed.


  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.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D6487

To: taapas1128, #hg-reviewers, martinvonz
Cc: martinvonz, pulkit, mercurial-devel
phabricator - June 8, 2019, 3:25 p.m.
taapas1128 requested review of this revision.
taapas1128 added a comment.


  @martinvonz check it out I haveve done that.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D6487

To: taapas1128, #hg-reviewers, martinvonz
Cc: martinvonz, pulkit, mercurial-devel
phabricator - June 8, 2019, 3:29 p.m.
martinvonz requested changes to this revision.
martinvonz added a comment.
This revision now requires changes to proceed.


  In https://phab.mercurial-scm.org/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?

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D6487

To: taapas1128, #hg-reviewers, martinvonz
Cc: martinvonz, pulkit, mercurial-devel
phabricator - June 8, 2019, 3:41 p.m.
taapas1128 added a comment.


  @martinvonz Is it alright now?

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D6487

To: taapas1128, #hg-reviewers, martinvonz
Cc: martinvonz, pulkit, mercurial-devel
phabricator - June 8, 2019, 3:52 p.m.
taapas1128 added a comment.


  Regarding the message change that was previously in https://phab.mercurial-scm.org/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 ?

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D6487

To: taapas1128, #hg-reviewers, martinvonz
Cc: martinvonz, pulkit, mercurial-devel
phabricator - June 8, 2019, 4:03 p.m.
martinvonz requested changes to this revision.
martinvonz added a comment.
This revision now requires changes to proceed.


  In https://phab.mercurial-scm.org/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.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D6487

To: taapas1128, #hg-reviewers, martinvonz
Cc: martinvonz, pulkit, mercurial-devel
phabricator - June 8, 2019, 4:21 p.m.
taapas1128 added a comment.


  So should I squash the https://phab.mercurial-scm.org/D6487 and https://phab.mercurial-scm.org/D6488 too? That would make this patch really long as the very first patch that I sent?

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D6487

To: taapas1128, #hg-reviewers, martinvonz
Cc: martinvonz, pulkit, mercurial-devel
phabricator - June 8, 2019, 4:34 p.m.
taapas1128 added a comment.


  @martinvonz: Have a look now.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D6487

To: taapas1128, #hg-reviewers, martinvonz, durin42
Cc: mjpieters, Kwan, martinvonz, pulkit, mercurial-devel
phabricator - June 8, 2019, 4:47 p.m.
martinvonz added a comment.


  In https://phab.mercurial-scm.org/D6487#94391, @taapas1128 wrote:
  
  > So should I squash the https://phab.mercurial-scm.org/D6487 and https://phab.mercurial-scm.org/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 :)

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D6487

To: taapas1128, #hg-reviewers, martinvonz, durin42
Cc: mjpieters, Kwan, martinvonz, pulkit, mercurial-devel
phabricator - June 8, 2019, 5:01 p.m.
taapas1128 added a comment.


  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? :(

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D6487

To: taapas1128, #hg-reviewers, martinvonz, durin42
Cc: mjpieters, Kwan, martinvonz, pulkit, mercurial-devel
phabricator - June 8, 2019, 9:41 p.m.
martinvonz added a comment.


  In https://phab.mercurial-scm.org/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.

INLINE COMMENTS

> cmdutil.py:3301
> -# note: bisect is intentionally excluded
> -# (state file, clearable, allowcommit, error, hint)
> -unfinishedstates = [

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.

> state.py:104
> +        fname is the file name in which data should be stored in .hg directory.
> +        It is None for merge command.
> +        clearable boolean determines whether or not interrupted states can be

Why?

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D6487

To: taapas1128, #hg-reviewers, martinvonz, durin42
Cc: mjpieters, Kwan, martinvonz, pulkit, mercurial-devel
phabricator - June 8, 2019, 10:10 p.m.
martinvonz added a comment.


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

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D6487

To: taapas1128, #hg-reviewers, martinvonz, durin42
Cc: mjpieters, Kwan, martinvonz, pulkit, mercurial-devel

Patch

diff --git a/mercurial/statecheck.py b/mercurial/statecheck.py
new file mode 100644
--- /dev/null
+++ b/mercurial/statecheck.py
@@ -0,0 +1,71 @@ 
+from __future__ import absolute_import
+
+from .i18n import _
+
+from . import (
+    error,
+    util,
+)
+
+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.
+
+        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 _hintmessage(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 _statusmessage(self):
+        """returns the status message corresponding to the command"""
+        if self._cmdname == 'update':
+            hint = _('last update was interrupted')
+        else:
+            hint = _('%s in progress') % (self._cmdname)
+        return hint
+
+    def _mergepredicate(self, repo):
+        """determines is a merge is in progress or not"""
+        return len(repo[None].parents()) > 1