Patchwork D6566: abort: first prototype of hg abort

login
register
mail settings
Submitter phabricator
Date June 23, 2019, 5:43 p.m.
Message ID <differential-rev-PHID-DREV-yfmsboaj4zaxtbv6aoym-req@mercurial-scm.org>
Download mbox | patch
Permalink /patch/40652/
State Superseded
Headers show

Comments

phabricator - June 23, 2019, 5:43 p.m.
taapas1128 created this revision.
Herald added a subscriber: mercurial-devel.
Herald added a reviewer: hg-reviewers.

REVISION SUMMARY
  This adds the first prototype for `hg abort`. It includes the
  logic of `hg abort` and adds a way to register abort funtions
  for various unfinished operations.
  
  In the subsequent patches support will added for `graft`, `rebase`,
  `histedit`, `shelve` and `merge`.

REPOSITORY
  rHG Mercurial

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

AFFECTED FILES
  mercurial/commands.py
  mercurial/state.py
  tests/test-help.t

CHANGE DETAILS




To: taapas1128, #hg-reviewers
Cc: mercurial-devel
phabricator - June 23, 2019, 9:29 p.m.
martinvonz added a comment.


  Need to set the parent of this patch

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D6566/new/

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

To: taapas1128, #hg-reviewers
Cc: martinvonz, mercurial-devel
phabricator - June 24, 2019, 9:47 a.m.
taapas1128 added a comment.


  @martinvonz I wanted this as a seperate stack for now so that the previous stack can be committed first. Do you want me to update its parents and add it to the previous stack?

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D6566/new/

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

To: taapas1128, #hg-reviewers
Cc: martinvonz, mercurial-devel
phabricator - June 30, 2019, 7:08 p.m.
pulkit added inline comments.

INLINE COMMENTS

> commands.py:147
> +        if state.isunfinished(repo):
> +            abortstate = state
> +            break

`return state.abortfunc(ui, repo, **opts)` instead of storing and calling that later.

> commands.py:152
> +    if abortstate._abortfunc:
> +        return abortstate._abortfunc(ui, repo)
> +    raise error.Abort((_("%s does not support 'hg abort'") %

we should pass opts to abortfunc too. I can think of `--no-backup` as a flag which tells not to store a backup when aborting.

A nice addition will be `--dry-run` flag which tells which operation will be aborted. Also, maybe add a message saying `aborting <command>`.

> state.py:102
>                   reportonly=False, continueflag=False, stopflag=False ,
> -                 cmdmsg="", cmdhint="", statushint=""):
> +                 cmdmsg="", cmdhint="", statushint="", abortfunc=None):
>          """opname is the name the command or operation

the new argument needs documentation.

> state.py:136
>          self._continueflag = continueflag
> +        self._abortfunc = abortfunc
>  

since we will be using this argument outside of this class, let's not prepend `_` to the name.

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D6566/new/

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

To: taapas1128, #hg-reviewers
Cc: pulkit, martinvonz, mercurial-devel
phabricator - July 4, 2019, 3 p.m.
pulkit added inline comments.

INLINE COMMENTS

> commands.py:166
> +
> +    if nobackup:
> +        arglist = inspect.getargspec(abortstate.abortfunc)[0]

Arguments for abort can increase in future. Rather pass the opts directly to abort functions for now and make related functions look for flag which they need.

I agree it's not nice, but seems like a good tradeoff to start and have better code.

> commands.py:173
> +    if dryrun:
> +        raise error.Abort(_('aborting %s') % (abortstate._opname))
> +

should be `ui.status()` instead of `error.Abort()`

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D6566/new/

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

To: taapas1128, #hg-reviewers
Cc: pulkit, martinvonz, mercurial-devel
phabricator - July 5, 2019, 9:49 p.m.
pulkit added inline comments.

INLINE COMMENTS

> commands.py:135
>  
> +@command('abort',
> +    [('', 'no-backup', False, _('do not save backup copies of files')),

Can you mark this command as experimental for now, we will be close to release soon. Marking this as experimental will help us take more time to smoothen things out.

> commands.py:136
> +@command('abort',
> +    [('', 'no-backup', False, _('do not save backup copies of files')),
> +    ] + dryrunopts, helpcategory=command.CATEGORY_CHANGE_MANAGEMENT,

This help should be `'do not save backup bundle'` as similar to strip command.

> commands.py:142
> +
> +    Aborts an multistep operation like graft, histedit, rebase, merge,
> +    and unshelve if they are in an unfinished state.

s/an/a

> commands.py:154
> +    abortstate = None
> +    dryrun = opts.get('dry_run')
> +    for state in statemod._unfinishedstates:

need to prepend `r''` in front of dry_run for py3 compat.

> commands.py:165
> +    if dryrun:
> +        ui.status(_('aborting %s\n') % (abortstate._opname))
> +    return abortstate.abortfunc(ui, repo, **opts)

we should return here so that `abortstate.abortfunc()` is not called below in case of dry-run.

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D6566/new/

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

To: taapas1128, #hg-reviewers
Cc: pulkit, martinvonz, mercurial-devel
phabricator - July 9, 2019, 10:25 a.m.
pulkit added a comment.


  > hg abort currently supports --dry-run/-n flag only.
  > It is used to dry run hg abort
  
  We should specify what does dry run does i.e. it prints the operation which will be aborted if `hg abort` is run.

INLINE COMMENTS

> commands.py:139
> +def abort(ui, repo, **opts):
> +    """abort an unfinished operation(EXPERIMENTAL)
> +

You need a space between operation and (EXPERIMENTAL)

> commands.py:153
> +    if not abortstate.abortfunc:
> +        raise error.Abort((_("%s does not support 'hg abort'") %
> +                            (abortstate._opname)), hint=abortstate.hint())

`%s is in progress but does not support 'hg abort'`.

> commands.py:158
> +        return
> +    return abortstate.abortfunc(ui, repo, **opts)
> +

Now that we don't have backup flag, and --dry-run is tackled before calling abortfunc, we can prevent passing `**opts`.

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D6566/new/

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

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

Patch

diff --git a/tests/test-help.t b/tests/test-help.t
--- a/tests/test-help.t
+++ b/tests/test-help.t
@@ -5,6 +5,7 @@ 
   
   basic commands:
   
+   abort         abort an unfinished operation
    add           add the specified files on the next commit
    annotate      show changeset information by line for each file
    clone         make a copy of an existing repository
@@ -26,6 +27,7 @@ 
   (use 'hg help' for the full list of commands or 'hg -v' for details)
 
   $ hg -q
+   abort         abort an unfinished operation
    add           add the specified files on the next commit
    annotate      show changeset information by line for each file
    clone         make a copy of an existing repository
@@ -73,6 +75,7 @@ 
   
   Change manipulation:
   
+   abort         abort an unfinished operation
    backout       reverse effect of earlier changeset
    graft         copy changes from other branches onto the current branch
    merge         merge another revision into working directory
@@ -199,6 +202,7 @@ 
   
   Change manipulation:
   
+   abort         abort an unfinished operation
    backout       reverse effect of earlier changeset
    graft         copy changes from other branches onto the current branch
    merge         merge another revision into working directory
@@ -399,6 +403,7 @@ 
   
   basic commands:
   
+   abort         abort an unfinished operation
    add           add the specified files on the next commit
    annotate, blame
                  show changeset information by line for each file
@@ -2350,6 +2355,13 @@ 
   <tr><td colspan="2"><h2><a name="main" href="#main">Main Commands</a></h2></td></tr>
   
   <tr><td>
+  <a href="/help/abort">
+  abort
+  </a>
+  </td><td>
+  abort an unfinished operation
+  </td></tr>
+  <tr><td>
   <a href="/help/add">
   add
   </a>
diff --git a/mercurial/state.py b/mercurial/state.py
--- a/mercurial/state.py
+++ b/mercurial/state.py
@@ -99,7 +99,7 @@ 
 
     def __init__(self, opname, fname, clearable=False, allowcommit=False,
                  reportonly=False, continueflag=False, stopflag=False ,
-                 cmdmsg="", cmdhint="", statushint=""):
+                 cmdmsg="", cmdhint="", statushint="", abortfunc=None):
         """opname is the name the command or operation
         fname is the file name in which data should be stored in .hg directory.
         It is None for merge command.
@@ -135,6 +135,7 @@ 
         self._stopflag = stopflag
         self._reportonly = reportonly
         self._continueflag = continueflag
+        self._abortfunc = abortfunc
 
     def statusmsg(self):
         """returns the hint message corresponding to the command for
diff --git a/mercurial/commands.py b/mercurial/commands.py
--- a/mercurial/commands.py
+++ b/mercurial/commands.py
@@ -131,6 +131,28 @@ 
 
 # Commands start here, listed alphabetically
 
+@command('abort', [], helpcategory=command.CATEGORY_CHANGE_MANAGEMENT,
+        helpbasic=True)
+def abort(ui, repo, **opts):
+    """abort an unfinished operation
+
+    This function is the first prototype for hg abort command.
+    It first checks the state and checks which command is active
+    then calls then returns the respective logic for aborting the
+    command.
+    """
+    abortstate = None
+    for state in statemod._unfinishedstates:
+        if state.isunfinished(repo):
+            abortstate = state
+            break
+    if not abortstate:
+        raise error.Abort(_('no operation in progress'))
+    if abortstate._abortfunc:
+        return abortstate._abortfunc(ui, repo)
+    raise error.Abort((_("%s does not support 'hg abort'") %
+                        (abortstate._opname)), hint=abortstate.hint())
+
 @command('add',
     walkopts + subrepoopts + dryrunopts,
     _('[OPTION]... [FILE]...'),