( )⚙ D8714 state: support validated declaration of nested unfinished ops

This is an archive of the discontinued Mercurial Phabricator instance.

Differential D8714

state: support validated declaration of nested unfinished ops
ClosedPublic

Authored by dploch on Jul 8 2020, 6:50 PM.

Details

Reviewers
pulkit
Group Reviewers
hg-reviewers
Commits
rHG5322e738be0f: state: support validated declaration of nested unfinished ops
Summary

This enables extensions to define commands that delgate to rebase, evolve, etc. one or more times to also have their own unfinished states for the full sequence of operations without monkey-patching _unfinishedstates.

Diff Detail

Repository
rHG Mercurial
Branch
default
Lint
No Linters Available
Unit
No Unit Test Coverage

Event Timeline

dploch created this revision.Jul 8 2020, 6:50 PM
mjacob added a subscriber: mjacob.Jul 8 2020, 8:28 PM

Can you give an example of an extension that would benefit from this? It’s not immediately obvious to me, and ideally patches should be written such that it’s obvious for most people. :)

Can you add a test case? That would probably also help understanding the use case.

dploch added a comment.Jul 10 2020, 9:19 PM
In D8714#129998, @mjacob wrote:

Can you give an example of an extension that would benefit from this? It’s not immediately obvious to me, and ideally patches should be written such that it’s obvious for most people. :)

To give a concrete example, we have an internal command which does something similar to chainify. It generates N individual commits with different parents (based on input flags), then rebases then sequentially into a single chain in a specific order; it currently does some hacky wrapping of methods in this file to make it work. We have need of another command that does something similar with multiple evolve instructions; rather than repeat the hacks, I thought it would be good to make the support cleaner by implementing it upstream.

Can you add a test case? That would probably also help understanding the use case.

I added a test file with a simple-ish example extension that hopefully clarifies the intent. I revised the changes a bit to make the intended usage in extensions cleaner/more intuitive (I probably should have done this in the first place since there were also two minor bugs in the initial draft.)

dploch updated this revision to Diff 21904.Jul 14 2020, 4:38 PM
pulkit added a subscriber: pulkit.Jul 17 2020, 1:13 PM

Following test fails with this patch:

--- /tests/test-check-code.t
+++ /tests/test-check-code.t.err
@@ -33,6 +33,16 @@
   Skipping i18n/polib.py it has no-che?k-code (glob)
   Skipping mercurial/statprof.py it has no-che?k-code (glob)
   Skipping tests/badserverext.py it has no-che?k-code (glob)
+  tests/test-state-extension.t:79:
+   >   $ hg co -q .^
+   warning: ^ must be quoted
+  tests/test-state-extension.t:82:
+   >   $ hg co -q .^
+   warning: ^ must be quoted
+  tests/test-state-extension.t:85:
+   >   $ hg co -q .^
+   warning: ^ must be quoted
+  [1]
 
 @commands in debugcommands.py should be in alphabetical order.
 

ERROR: test-check-code.t output changed
!
--- /tests/test-check-pylint.t
+++ /tests/test-check-pylint.t.err
@@ -20,3 +20,9 @@
   ------------------------------------ (?)
   Your code has been rated at 10.00/10 (?)
    (?)
+  ************* Module mercurial.state
+  W:200, 0: Dangerous default value [] as argument (dangerous-default-value)
+  
+  ------------------------------------
+  Your code has been rated at 10.00/10
+  

ERROR: test-check-pylint.t output changed
!
dploch updated this revision to Diff 21962.Jul 17 2020, 8:04 PM
dploch added a comment.Jul 17 2020, 8:05 PM
In D8714#130965, @pulkit wrote:

Following test fails with this patch:

--- /tests/test-check-code.t
+++ /tests/test-check-code.t.err
@@ -33,6 +33,16 @@
   Skipping i18n/polib.py it has no-che?k-code (glob)
   Skipping mercurial/statprof.py it has no-che?k-code (glob)
   Skipping tests/badserverext.py it has no-che?k-code (glob)
+  tests/test-state-extension.t:79:
+   >   $ hg co -q .^
+   warning: ^ must be quoted
+  tests/test-state-extension.t:82:
+   >   $ hg co -q .^
+   warning: ^ must be quoted
+  tests/test-state-extension.t:85:
+   >   $ hg co -q .^
+   warning: ^ must be quoted
+  [1]
 @commands in debugcommands.py should be in alphabetical order.
ERROR: test-check-code.t output changed
!
--- /tests/test-check-pylint.t
+++ /tests/test-check-pylint.t.err
@@ -20,3 +20,9 @@
   ------------------------------------ (?)
   Your code has been rated at 10.00/10 (?)
    (?)
+  ************* Module mercurial.state
+  W:200, 0: Dangerous default value [] as argument (dangerous-default-value)
+  
+  ------------------------------------
+  Your code has been rated at 10.00/10
+  
ERROR: test-check-pylint.t output changed
!

Fixed.

pulkit accepted this revision.Jul 20 2020, 8:13 AM

This looks quite interesting.

This revision is now accepted and ready to land.Jul 20 2020, 8:13 AM

Revision Contents
Changeset List

PathPackages
Mmercurial/state.py (93 lines)
AMtests/test-state-extension.t (136 lines)

Diff 21962

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
import contextlib
from .i18n import _ from .i18n import _
from . import ( from . import (
error, error,
pycompat, pycompat,
util, util,
) )
from .utils import cborutil from .utils import cborutil
self, self,
opname, opname,
fname, fname,
clearable, clearable,
allowcommit, allowcommit,
reportonly, reportonly,
continueflag, continueflag,
stopflag, stopflag,
childopnames,
cmdmsg, cmdmsg,
cmdhint, cmdhint,
statushint, statushint,
abortfunc, abortfunc,
continuefunc, continuefunc,
): ):
self._opname = opname self._opname = opname
self._fname = fname self._fname = fname
self._clearable = clearable self._clearable = clearable
self._allowcommit = allowcommit self._allowcommit = allowcommit
self._reportonly = reportonly self._reportonly = reportonly
self._continueflag = continueflag self._continueflag = continueflag
self._stopflag = stopflag self._stopflag = stopflag
self._childopnames = childopnames
self._delegating = False
self._cmdmsg = cmdmsg self._cmdmsg = cmdmsg
self._cmdhint = cmdhint self._cmdhint = cmdhint
self._statushint = statushint self._statushint = statushint
self.abortfunc = abortfunc self.abortfunc = abortfunc
self.continuefunc = continuefunc self.continuefunc = continuefunc
def statusmsg(self): def statusmsg(self):
"""returns the hint message corresponding to the command for """returns the hint message corresponding to the command for
return _(b'hg %s --continue') % (self._opname) return _(b'hg %s --continue') % (self._opname)
def isunfinished(self, repo): def isunfinished(self, repo):
"""determines whether a multi-step operation is in progress """determines whether a multi-step operation is in progress
or not or not
""" """
if self._opname == b'merge': if self._opname == b'merge':
return len(repo[None].parents()) > 1 return len(repo[None].parents()) > 1
elif self._delegating:
return False
else: else:
return repo.vfs.exists(self._fname) return repo.vfs.exists(self._fname)
# A list of statecheck objects for multistep operations like graft. # A list of statecheck objects for multistep operations like graft.
_unfinishedstates = [] _unfinishedstates = []
_unfinishedstatesbyname = {}
def addunfinished( def addunfinished(
opname, opname,
fname, fname,
clearable=False, clearable=False,
allowcommit=False, allowcommit=False,
reportonly=False, reportonly=False,
continueflag=False, continueflag=False,
stopflag=False, stopflag=False,
childopnames=None,
cmdmsg=b"", cmdmsg=b"",
cmdhint=b"", cmdhint=b"",
statushint=b"", statushint=b"",
abortfunc=None, abortfunc=None,
continuefunc=None, continuefunc=None,
): ):
"""this registers a new command or operation to unfinishedstates """this registers a new command or operation to unfinishedstates
opname is the name the command or operation opname is the name the command or operation
fname is the file name in which data should be stored in .hg directory. fname is the file name in which data should be stored in .hg directory.
It is None for merge command. It is None for merge command.
clearable boolean determines whether or not interrupted states can be clearable boolean determines whether or not interrupted states can be
cleared by running `hg update -C .` which in turn deletes the cleared by running `hg update -C .` which in turn deletes the
state file. state file.
allowcommit boolean decides whether commit is allowed during interrupted allowcommit boolean decides whether commit is allowed during interrupted
state or not. state or not.
reportonly flag is used for operations like bisect where we just reportonly flag is used for operations like bisect where we just
need to detect the operation using 'hg status --verbose' need to detect the operation using 'hg status --verbose'
continueflag is a boolean determines whether or not a command supports continueflag is a boolean determines whether or not a command supports
`--continue` option or not. `--continue` option or not.
stopflag is a boolean that determines whether or not a command supports stopflag is a boolean that determines whether or not a command supports
--stop flag --stop flag
childopnames is a list of other opnames this op uses as sub-steps of its
own execution. They must already be added.
cmdmsg is used to pass a different status message in case standard cmdmsg is used to pass a different status message in case standard
message of the format "abort: cmdname in progress" is not desired. message of the format "abort: cmdname in progress" is not desired.
cmdhint is used to pass a different hint message in case standard cmdhint is used to pass a different hint message in case standard
message of the format "To continue: hg cmdname --continue message of the format "To continue: hg cmdname --continue
To abort: hg cmdname --abort" is not desired. To abort: hg cmdname --abort" is not desired.
statushint is used to pass a different status message in case standard statushint is used to pass a different status message in case standard
message of the format ('To continue: hg cmdname --continue' message of the format ('To continue: hg cmdname --continue'
'To abort: hg cmdname --abort') is not desired 'To abort: hg cmdname --abort') is not desired
abortfunc stores the function required to abort an unfinished state. abortfunc stores the function required to abort an unfinished state.
continuefunc stores the function required to finish an interrupted continuefunc stores the function required to finish an interrupted
operation. operation.
""" """
childopnames = childopnames or []
statecheckobj = _statecheck( statecheckobj = _statecheck(
opname, opname,
fname, fname,
clearable, clearable,
allowcommit, allowcommit,
reportonly, reportonly,
continueflag, continueflag,
stopflag, stopflag,
childopnames,
cmdmsg, cmdmsg,
cmdhint, cmdhint,
statushint, statushint,
abortfunc, abortfunc,
continuefunc, continuefunc,
) )
if opname == b'merge': if opname == b'merge':
_unfinishedstates.append(statecheckobj) _unfinishedstates.append(statecheckobj)
else: else:
# This check enforces that for any op 'foo' which depends on op 'bar',
# 'foo' comes before 'bar' in _unfinishedstates. This ensures that
# getrepostate() always returns the most specific applicable answer.
for childopname in childopnames:
if childopname not in _unfinishedstatesbyname:
raise error.ProgrammingError(
_(b'op %s depends on unknown op %s') % (opname, childopname)
)
_unfinishedstates.insert(0, statecheckobj) _unfinishedstates.insert(0, statecheckobj)
if opname in _unfinishedstatesbyname:
raise error.ProgrammingError(_(b'op %s registered twice') % opname)
_unfinishedstatesbyname[opname] = statecheckobj
def _getparentandchild(opname, childopname):
p = _unfinishedstatesbyname.get(opname, None)
if not p:
raise error.ProgrammingError(_(b'unknown op %s') % opname)
if childopname not in p._childopnames:
raise error.ProgrammingError(
_(b'op %s does not delegate to %s') % (opname, childopname)
)
c = _unfinishedstatesbyname[childopname]
return p, c
@contextlib.contextmanager
def delegating(repo, opname, childopname):
"""context wrapper for delegations from opname to childopname.
requires that childopname was specified when opname was registered.
Usage:
def my_command_foo_that_uses_rebase(...):
...
with state.delegating(repo, 'foo', 'rebase'):
_run_rebase(...)
...
"""
p, c = _getparentandchild(opname, childopname)
if p._delegating:
raise error.ProgrammingError(
_(b'cannot delegate from op %s recursively') % opname
)
p._delegating = True
try:
yield
except error.ConflictResolutionRequired as e:
# Rewrite conflict resolution advice for the parent opname.
if e.opname == childopname:
raise error.ConflictResolutionRequired(opname)
raise e
finally:
p._delegating = False
def ischildunfinished(repo, opname, childopname):
"""Returns true if both opname and childopname are unfinished."""
p, c = _getparentandchild(opname, childopname)
return (p._delegating or p.isunfinished(repo)) and c.isunfinished(repo)
def continuechild(ui, repo, opname, childopname):
"""Checks that childopname is in progress, and continues it."""
p, c = _getparentandchild(opname, childopname)
if not ischildunfinished(repo, opname, childopname):
raise error.ProgrammingError(
_(b'child op %s of parent %s is not unfinished')
% (childopname, opname)
)
if not c.continuefunc:
raise error.ProgrammingError(
_(b'op %s has no continue function') % childopname
)
return c.continuefunc(ui, repo)
addunfinished( addunfinished(
b'update', b'update',
fname=b'updatestate', fname=b'updatestate',
clearable=True, clearable=True,
cmdmsg=_(b'last update was interrupted'), cmdmsg=_(b'last update was interrupted'),
cmdhint=_(b"use 'hg update' to get a consistent checkout"), cmdhint=_(b"use 'hg update' to get a consistent checkout"),
statushint=_(b"To continue: hg update ."), statushint=_(b"To continue: hg update ."),

tests/test-state-extension.t

  • This file was added.
Test extension of unfinished states support.
$ mkdir chainify
$ cd chainify
$ cat >> chainify.py <<EOF
> from mercurial import cmdutil, error, extensions, exthelper, node, scmutil, state
> from hgext import rebase
>
> eh = exthelper.exthelper()
>
> extsetup = eh.finalextsetup
> cmdtable = eh.cmdtable
>
> # Rebase calls addunfinished in uisetup, so we have to call it in extsetup.
> # Ideally there'd by an 'extensions.afteruisetup()' just like
> # 'extensions.afterloaded()' to allow nesting multiple commands.
> @eh.extsetup
> def _extsetup(ui):
> state.addunfinished(
> b'chainify',
> b'chainify.state',
> continueflag=True,
> childopnames=[b'rebase'])
>
> def _node(repo, arg):
> return node.hex(scmutil.revsingle(repo, arg).node())
>
> @eh.command(
> b'chainify',
> [(b'r', b'revs', [], b'revs to chain', b'REV'),
> (b'', b'continue', False, b'continue op')],
> b'chainify [-r REV] +',
> inferrepo=True)
> def chainify(ui, repo, **opts):
> """Rebases r1, r2, r3, etc. into a chain."""
> with repo.wlock(), repo.lock():
> cmdstate = state.cmdstate(repo, b'chainify.state')
> if opts['continue']:
> if not cmdstate.exists():
> raise error.Abort(b'no chainify in progress')
> else:
> cmdutil.checkunfinished(repo)
> data = {
> b'tip': _node(repo, opts['revs'][0]),
> b'revs': b','.join(_node(repo, r) for r in opts['revs'][1:]),
> }
> cmdstate.save(1, data)
>
> data = cmdstate.read()
> while data[b'revs']:
> tip = data[b'tip']
> revs = data[b'revs'].split(b',')
> with state.delegating(repo, b'chainify', b'rebase'):
> ui.status(b'rebasing %s onto %s\n' % (revs[0][:12], tip[:12]))
> if state.ischildunfinished(repo, b'chainify', b'rebase'):
> rc = state.continuechild(ui, repo, b'chainify', b'rebase')
> else:
> rc = rebase.rebase(ui, repo, rev=[revs[0]], dest=tip)
> if rc and rc != 0:
> raise error.Abort(b'rebase failed (rc: %d)' % rc)
> data[b'tip'] = _node(repo, b'tip')
> data[b'revs'] = b','.join(revs[1:])
> cmdstate.save(1, data)
> cmdstate.delete()
> ui.status(b'done chainifying\n')
> EOF
$ chainifypath=`pwd`/chainify.py
$ echo '[extensions]' >> $HGRCPATH
$ echo "chainify = $chainifypath" >> $HGRCPATH
$ echo "rebase =" >> $HGRCPATH
$ cd $TESTTMP
$ hg init a
$ cd a
$ echo base > base.txt
$ hg commit -Aqm 'base commit'
$ echo foo > file1
$ hg commit -Aqm 'add file'
$ hg co -q ".^"
$ echo bar > file2
$ hg commit -Aqm 'add other file'
$ hg co -q ".^"
$ echo foo2 > file1
$ hg commit -Aqm 'add conflicting file'
$ hg co -q ".^"
$ hg log --graph --template '{rev} {files}'
o 3 file1
|
| o 2 file2
|/
| o 1 file1
|/
@ 0 base.txt
$ hg chainify -r 8430cfdf77c2 -r f8596309dff8 -r a858b338b3e9
rebasing f8596309dff8 onto 8430cfdf77c2
rebasing 2:f8596309dff8 "add other file"
saved backup bundle to $TESTTMP/* (glob)
rebasing a858b338b3e9 onto 83c722183a8e
rebasing 2:a858b338b3e9 "add conflicting file"
merging file1
warning: conflicts while merging file1! (edit, then use 'hg resolve --mark')
unresolved conflicts (see 'hg resolve', then 'hg chainify --continue')
[1]
$ hg status --config commands.status.verbose=True
M file1
? file1.orig
# The repository is in an unfinished *chainify* state.
# Unresolved merge conflicts:
#
# file1
#
# To mark files as resolved: hg resolve --mark FILE
# To continue: hg chainify --continue
# To abort: hg chainify --abort
$ echo foo3 > file1
$ hg resolve --mark file1
(no more unresolved files)
continue: hg chainify --continue
$ hg chainify --continue
rebasing a858b338b3e9 onto 83c722183a8e
rebasing 2:a858b338b3e9 "add conflicting file"
saved backup bundle to $TESTTMP/* (glob)
done chainifying
$ hg log --graph --template '{rev} {files}'
o 3 file1
|
o 2 file2
|
o 1 file1
|
@ 0 base.txt