Patchwork D6501: state: created new class statecheck to handle unfinishedstates

login
register
mail settings
Submitter phabricator
Date June 8, 2019, 8:58 p.m.
Message ID <differential-rev-PHID-DREV-45oird5h3ybyzaqfpo7h-req@phab.mercurial-scm.org>
Download mbox | patch
Permalink /patch/40374/
State New
Headers show

Comments

phabricator - June 8, 2019, 8:58 p.m.
taapas1128 created this revision.
Herald added a reviewer: durin42.
Herald added subscribers: mercurial-devel, mjpieters.
Herald added a reviewer: martinvonz.
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 4 basic arguments which include the name of the command, the
  name of the state file associated with it , clearable flag , allowcommit flag.
  
  This also also adds the support of`checkunfinished()` and
  `clearunfinished()` to the class.
  
  Tests remain unchanged.

REPOSITORY
  rHG Mercurial

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

AFFECTED FILES
  hgext/histedit.py
  hgext/rebase.py
  hgext/shelve.py
  hgext/transplant.py
  mercurial/state.py

CHANGE DETAILS




To: taapas1128, durin42, martinvonz, #hg-reviewers
Cc: mjpieters, mercurial-devel
phabricator - June 8, 2019, 10:09 p.m.
martinvonz requested changes to this revision.
martinvonz added inline comments.
This revision now requires changes to proceed.

INLINE COMMENTS

> histedit.py:2291
>      cmdutil.summaryhooks.add('histedit', summaryhook)
> -    statemod.unfinishedstates.append(
> -        ['histedit-state', False, True, _('histedit in progress'),
> -         _("use 'hg histedit --continue' or 'hg histedit --abort'")])
> +    statemod.unfinishedstates.append(statemod.statecheck('histedit',
> +    'histedit-state', clearable=False, allowcommit=True))

We may eventually want to add a function for registering states and thereby be able to make unfinishedstates and statecheck private. This is good for now, though.

> state.py:103
> +        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? (I asked the same thing on a different patch, but I think that one may have been dropped. I'm on mobile and too lazy to check.)

> state.py:105
> +        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

It looks like the code actually just deletes the state file (fname). Should we say that here instead? (I'm not sure we should.)

> state.py:118
> +        """returns the hint message corresponding to the command"""
> +        if self.cmdname == 'graft':
> +            msg = _("use 'hg graft --continue' or 'hg graft --stop' to stop")

It seems unfortunate to have to write each message in this function. That's going to make it a little harder for extensions to add their commands to the list. Perhaps the hint should instead be passed in to the statecheck constructor (much like it was before this patch)? Same thing with msg() below.

> state.py:143
> +        """
> +        if self.cmdname == 'merge' or mergecheck:
> +            return len(repo[None].parents()) > 1

Should this be left for a later patch? I don't see "merge" getting registered anywhere yet.

REPOSITORY
  rHG Mercurial

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

To: taapas1128, durin42, martinvonz, #hg-reviewers
Cc: mjpieters, mercurial-devel
phabricator - June 9, 2019, 10:29 a.m.
taapas1128 marked an inline comment as done.
taapas1128 added inline comments.

INLINE COMMENTS

> martinvonz wrote in state.py:103
> Why? (I asked the same thing on a different patch, but I think that one may have been dropped. I'm on mobile and too lazy to check.)

actually I merge is not detected w.r.t statefile but with the help of a predicate but since it needs be part of the same class I named the statefile as None for it.

> martinvonz wrote in state.py:105
> It looks like the code actually just deletes the state file (fname). Should we say that here instead? (I'm not sure we should.)

telling that here would be fine I suppose so I should add it here.

> martinvonz wrote in state.py:118
> It seems unfortunate to have to write each message in this function. That's going to make it a little harder for extensions to add their commands to the list. Perhaps the hint should instead be passed in to the statecheck constructor (much like it was before this patch)? Same thing with msg() below.

I might have a better solution to it what if I leave this as it is and add two more variables to the constructor `specialmsg=None` and `specialhint=None`. In case the extension uses any special message or hint in place of standard ones these two values can be given accordingly and standard messages can be overidden.

> martinvonz wrote in state.py:143
> Should this be left for a later patch? I don't see "merge" getting registered anywhere yet.

yeah it should be part of the next patch but having it here will make `isfinished`complete

REPOSITORY
  rHG Mercurial

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

To: taapas1128, durin42, martinvonz, #hg-reviewers
Cc: mjpieters, mercurial-devel
phabricator - June 9, 2019, 11:11 a.m.
taapas1128 marked an inline comment as done.
taapas1128 added inline comments.

INLINE COMMENTS

> martinvonz wrote in state.py:118
> It seems unfortunate to have to write each message in this function. That's going to make it a little harder for extensions to add their commands to the list. Perhaps the hint should instead be passed in to the statecheck constructor (much like it was before this patch)? Same thing with msg() below.

I have implemented that have a look. For core commands I have retained the if cases for `hgext` I have replaced that

> martinvonz wrote in state.py:143
> Should this be left for a later patch? I don't see "merge" getting registered anywhere yet.

removed that

REPOSITORY
  rHG Mercurial

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

To: taapas1128, durin42, martinvonz, #hg-reviewers
Cc: mjpieters, mercurial-devel
phabricator - June 10, 2019, 1:28 p.m.
martinvonz added inline comments.

INLINE COMMENTS

> state.py:106
> +        clearable boolean determines whether or not interrupted states can be
> +        cleared by running `hg update -C` which in turn deletes the state file.
> +        allowcommit boolean decides whether commit is allowed during interrupted

Nit: The `.` got lost here. Without it, `hg update -C` might also update to another commit.

> state.py:147
> +
> +    def isunfinished(self, repo, mergecheck=False):
> +        """determines whether a multi-step operation is in progress or not"""

Nit: mergecheck seems unused and it seems no callers pass in a value for it, so it could probably be removed (at least for now).

> state.py:125
> +        if not self.specialhint:
> +            if self.cmdname == 'graft':
> +                hint = _("use 'hg graft --continue' or 'hg graft --stop' to stop")

Should move the graft and update cases to where they're registered (around line 152)

> state.py:101
> +    def __init__(self, cmdname, fname, clearable=False, allowcommit=False,
> +                 specialmsg=None, specialhint=None):
> +        """cmdname is the name the command

Not: we usually don't include "special" in the names of this type of argument. A default of None often implies that it's calculated in some way if it's not provided.

> state.py:140
> +            if self.cmdname == 'update':
> +                msg = _('last update was interrupted')
> +            else:

Nit: return here and in the case below for consistency (or assign to msg also on line 145) .

REPOSITORY
  rHG Mercurial

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

To: taapas1128, durin42, martinvonz, #hg-reviewers
Cc: mjpieters, mercurial-devel
phabricator - June 10, 2019, 1:38 p.m.
pulkit added inline comments.

INLINE COMMENTS

> state.py:133
> +            return hint
> +        else:
> +            return self.specialhint

nit: no need of this else

> state.py:151
> +
> +unfinishedstates=[]
> +unfinishedstates.append(statecheck('graft','graftstate', clearable=True,

nit: space before and after the operator

REPOSITORY
  rHG Mercurial

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

To: taapas1128, durin42, martinvonz, #hg-reviewers
Cc: pulkit, mjpieters, mercurial-devel
phabricator - June 10, 2019, 11:02 p.m.
pulkit added inline comments.

INLINE COMMENTS

> histedit.py:2291
>      cmdutil.summaryhooks.add('histedit', summaryhook)
> -    statemod.unfinishedstates.append(
> -        ['histedit-state', False, True, _('histedit in progress'),
> -         _("use 'hg histedit --continue' or 'hg histedit --abort'")])
> +    statemod.unfinishedstates.append(statemod.statecheck('histedit',
> +    'histedit-state', clearable=False, allowcommit=True))

the code around these statements doing `append` is looking a bit complex. for making it cleaner and more readable, let's do either one of thing:

1. initialize an object before and then append
2. improve formatting of this

This applies to all the `append` which are there in the patch.

> state.py:91
>  
> -# A list of state files kept by multistep operations like graft.
> -# Since graft cannot be aborted, it is considered 'clearable' by update.

this comment on top of unfinished state is lost. We should reword and keep it.

> state.py:92
> +class statecheck(object):
> +    """a utility class that will to deal with multistep operations
> +       like graft, histedit, bisect, update etc and check whether such commands

`that will to deal` -> `that deals`

> state.py:116
> +        """
> +        self.cmdname = cmdname
> +        self.fname = fname

prepend all the class names with an `_`, like `self._cmdname`, `self._fname` etc. only the variables

That's a code style to say they are internal to readers and class users.

> state.py:140
> +
> +unfinishedstates=[]
> +unfinishedstates.append(statecheck('graft', 'graftstate', clearable=True,

needs space between `=` operator. I guess this is caught by `test-check*`, make sure you run tests.

REPOSITORY
  rHG Mercurial

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

To: taapas1128, durin42, martinvonz, #hg-reviewers
Cc: pulkit, mjpieters, mercurial-devel
phabricator - June 11, 2019, 8:25 a.m.
taapas1128 added inline comments.

INLINE COMMENTS

> pulkit wrote in state.py:140
> needs space between `=` operator. I guess this is caught by `test-check*`, make sure you run tests.

oh I will correct that. Strange it wasn't caught in test-check-code.t. I always try to run that.

REPOSITORY
  rHG Mercurial

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

To: taapas1128, durin42, martinvonz, #hg-reviewers
Cc: pulkit, mjpieters, mercurial-devel
phabricator - June 11, 2019, 8:28 a.m.
taapas1128 added inline comments.

INLINE COMMENTS

> taapas1128 wrote in state.py:140
> oh I will correct that. Strange it wasn't caught in test-check-code.t. I always try to run that.

I tried again it was not detected. Should I file this as a bug ?

REPOSITORY
  rHG Mercurial

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

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

Patch

diff --git a/mercurial/state.py b/mercurial/state.py
--- a/mercurial/state.py
+++ b/mercurial/state.py
@@ -88,43 +88,96 @@ 
         """check whether the state file exists or not"""
         return self._repo.vfs.exists(self.fname)
 
-# A list of state files kept by multistep operations like graft.
-# Since graft cannot be aborted, it is considered 'clearable' by update.
-# note: bisect is intentionally excluded
-# (state file, clearable, allowcommit, error, hint)
-unfinishedstates = [
-    ('graftstate', True, False, _('graft in progress'),
-     _("use 'hg graft --continue' or 'hg graft --stop' to stop")),
-    ('updatestate', True, False, _('last update was interrupted'),
-     _("use 'hg update' to get a consistent checkout"))
-    ]
+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):
+        """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
+
+    def hint(self):
+        """returns the hint message corresponding to the command"""
+        if self.cmdname == 'graft':
+            msg = _("use 'hg graft --continue' or 'hg graft --stop' to stop")
+        elif self.cmdname == 'update':
+            msg = _("use 'hg update' to get a consistent checkout")
+        elif self.cmdname == 'transplant':
+            msg = _("use 'hg transplant --continue' or 'hg update' to abort")
+        else:
+            msg = (_("use 'hg %s --continue' or 'hg %s --abort'") %
+            (self.cmdname,self.cmdname))
+        return msg
+
+    def msg(self):
+        """returns the status message corresponding to the command"""
+        if self.cmdname == 'update':
+            return _('last update was interrupted')
+        elif self.cmdname == 'unshelve':
+            return _('unshelve already in progress')
+        else:
+            return _('%s in progress') % (self.cmdname)
+
+    def isunfinished(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)
+
+unfinishedstates=[]
+unfinishedstates.append(statecheck('graft','graftstate', clearable=True,
+                                    allowcommit=False))
+unfinishedstates.append(statecheck('update', 'updatestate', clearable=True,
+                                    allowcommit=False))
 
 def checkunfinished(repo, commit=False):
     '''Look for an unfinished multistep operation, like graft, and abort
     if found. It's probably good to check this right before
     bailifchanged().
     '''
     # Check for non-clearable states first, so things like rebase will take
     # precedence over update.
-    for f, clearable, allowcommit, msg, hint in unfinishedstates:
-        if clearable or (commit and allowcommit):
+    for state in unfinishedstates:
+        if state.clearable or (commit and state.allowcommit):
             continue
-        if repo.vfs.exists(f):
-            raise error.Abort(msg, hint=hint)
+        if state.isunfinished(repo):
+            raise error.Abort(state.msg(), hint=state.hint())
 
-    for f, clearable, allowcommit, msg, hint in unfinishedstates:
-        if not clearable or (commit and allowcommit):
+    for s in unfinishedstates:
+        if not s.clearable or (commit and s.allowcommit):
             continue
-        if repo.vfs.exists(f):
-            raise error.Abort(msg, hint=hint)
+        if s.isunfinished(repo):
+            raise error.Abort(s.msg(), hint=s.hint())
 
 def clearunfinished(repo):
     '''Check for unfinished operations (as above), and clear the ones
     that are clearable.
     '''
-    for f, clearable, allowcommit, msg, hint in unfinishedstates:
-        if not clearable and repo.vfs.exists(f):
-            raise error.Abort(msg, hint=hint)
-    for f, clearable, allowcommit, msg, hint in unfinishedstates:
-        if clearable and repo.vfs.exists(f):
-            util.unlink(repo.vfs.join(f))
+    for state in unfinishedstates:
+        if not state.clearable and state.isunfinished(repo):
+            raise error.Abort(state.msg(), hint=state.hint())
+
+    for s in unfinishedstates:
+        if s.clearable and s.isunfinished(repo):
+            util.unlink(repo.vfs.join(s.fname))
diff --git a/hgext/transplant.py b/hgext/transplant.py
--- a/hgext/transplant.py
+++ b/hgext/transplant.py
@@ -758,9 +758,8 @@ 
     return n and nodemod.hex(n) or ''
 
 def extsetup(ui):
-    statemod.unfinishedstates.append(
-        ['transplant/journal', True, False, _('transplant in progress'),
-         _("use 'hg transplant --continue' or 'hg update' to abort")])
+    statemod.unfinishedstates.append(statemod.statecheck('transplant',
+    'transplant/journal', clearable=True, allowcommit=False))
 
 # tell hggettext to extract docstrings from these functions:
 i18nfunctions = [revsettransplanted, kwtransplanted]
diff --git a/hgext/shelve.py b/hgext/shelve.py
--- a/hgext/shelve.py
+++ b/hgext/shelve.py
@@ -1140,10 +1140,8 @@ 
         return createcmd(ui, repo, pats, opts)
 
 def extsetup(ui):
-    statemod.unfinishedstates.append(
-        [shelvedstate._filename, False, False,
-         _('unshelve already in progress'),
-         _("use 'hg unshelve --continue' or 'hg unshelve --abort'")])
+    statemod.unfinishedstates.append(statemod.statecheck('unshelve',
+    shelvedstate._filename, clearable=False, allowcommit=False))
     cmdutil.afterresolvedstates.append(
         [shelvedstate._filename, _('hg unshelve --continue')])
 
diff --git a/hgext/rebase.py b/hgext/rebase.py
--- a/hgext/rebase.py
+++ b/hgext/rebase.py
@@ -1947,8 +1947,7 @@ 
     entry[1].append(('t', 'tool', '',
                      _("specify merge tool for rebase")))
     cmdutil.summaryhooks.add('rebase', summaryhook)
-    statemod.unfinishedstates.append(
-        ['rebasestate', False, False, _('rebase in progress'),
-         _("use 'hg rebase --continue' or 'hg rebase --abort'")])
+    statemod.unfinishedstates.append(statemod.statecheck('rebase',
+    'rebasestate', clearable=False, allowcommit=False))
     cmdutil.afterresolvedstates.append(
         ['rebasestate', _('hg rebase --continue')])
diff --git a/hgext/histedit.py b/hgext/histedit.py
--- a/hgext/histedit.py
+++ b/hgext/histedit.py
@@ -2288,8 +2288,7 @@ 
 
 def extsetup(ui):
     cmdutil.summaryhooks.add('histedit', summaryhook)
-    statemod.unfinishedstates.append(
-        ['histedit-state', False, True, _('histedit in progress'),
-         _("use 'hg histedit --continue' or 'hg histedit --abort'")])
+    statemod.unfinishedstates.append(statemod.statecheck('histedit',
+    'histedit-state', clearable=False, allowcommit=True))
     cmdutil.afterresolvedstates.append(
         ['histedit-state', _('hg histedit --continue')])