Patchwork D6579: abort: added support for unshelve

login
register
mail settings
Submitter phabricator
Date June 26, 2019, 4:46 p.m.
Message ID <differential-rev-PHID-DREV-xokjqpothdrbcfa3ifvf-req@mercurial-scm.org>
Download mbox | patch
Permalink /patch/40682/
State Superseded
Headers show

Comments

phabricator - June 26, 2019, 4:46 p.m.
taapas1128 created this revision.
Herald added a subscriber: mercurial-devel.
Herald added a reviewer: hg-reviewers.

REVISION SUMMARY
  This patch adds the support for shelve in `hg abort` plan.
  `unshelveabort()` has been modified for independent calls.
  
  Results are shown as tests.

REPOSITORY
  rHG Mercurial

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

AFFECTED FILES
  hgext/shelve.py
  tests/test-abort.t

CHANGE DETAILS




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


  This patch needs to be rebased on tip of hg-committed as shelve is in core now.

REPOSITORY
  rHG Mercurial

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

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

To: taapas1128, #hg-reviewers
Cc: pulkit, mercurial-devel
phabricator - July 4, 2019, 2:59 p.m.
taapas1128 added a comment.


  I have rebased it.

REPOSITORY
  rHG Mercurial

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

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

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

INLINE COMMENTS

> shelve.py:630
> +    if not state:
> +        try:
> +            state = shelvedstate.load(repo)

Let's take this code out into a new function and reuse that function at both the places instead of duplicating code.

REPOSITORY
  rHG Mercurial

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

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

To: taapas1128, #hg-reviewers
Cc: pulkit, mercurial-devel
phabricator - July 8, 2019, 5:09 p.m.
mharbison72 added inline comments.

INLINE COMMENTS

> test-shelve2.t:734
> +  abort: merge does not support 'hg abort' (stripbased !)
> +  (use 'hg commit' or 'hg merge --abort') (stripbased !)
>    [255]

I think that you can eliminate the duplication with this:

  abort: no unshelve in progress (abortflag !)
  abort: merge does not support 'hg abort' (no-abortflag !)
  (use 'hg commit' or 'hg merge --abort') (no-abortflag !)

REPOSITORY
  rHG Mercurial

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

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

To: taapas1128, #hg-reviewers
Cc: mharbison72, pulkit, mercurial-devel
phabricator - July 8, 2019, 5:27 p.m.
mharbison72 added inline comments.

INLINE COMMENTS

> test-shelve2.t:1
> -#testcases stripbased phasebased
> +#testcases stripbased phasebased abortflag abortcommand
>  

Shouldn't these be 2 lines, to maximize the test coverage?  Presumably `--abort` and `hg abort` can work with either phase or strip based shelving.  See test-narrow.t, or some of the lfs tests.

  #testcases stripbased phasebased
  #testcases abortflag abortcommand

REPOSITORY
  rHG Mercurial

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

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

To: taapas1128, #hg-reviewers
Cc: mharbison72, pulkit, mercurial-devel
phabricator - July 8, 2019, 6:22 p.m.
taapas1128 added a comment.


  @mharbison72 Thanks for the suggestions. I have amended the patch accordingly.

REPOSITORY
  rHG Mercurial

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

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

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

INLINE COMMENTS

> shelve.py:628
>  
> -def unshelveabort(ui, repo, state, opts):
> +def _loadshelvedstate(ui, repo, continuef=False, abortf=False, **opts):
> +    try:

continuef and abortf can be read from opts, no need to pass them separately.

Also, `opts` should be pass as a dictionary here, no need to pass that as kwargs.

> shelve.py:648
> +                    'commit\n')
> +            ui.warn(msg)
> +            shelvedstate.clear(repo)

I think we can convert this `ui.warn()` to `error.Abort()` and hence get rid of `sys.exit()`. Send a standalone patch before this one which make this an error instead of warn.

> shelve.py:656
> +    if not state:
> +        statetuple = _loadshelvedstate(ui, repo, abortf=True)
> +        state, opts = statetuple

nit: `state, opts = _loadshelvedstate(ui, repo, {'abort'=True})`

> shelve.py:658
> +        state, opts = statetuple
> +    with repo.wlock(), repo.lock():
>          try:

let's take `wlock()` only again when if we are processing `hg abort`. We can define this function as `unshelveabort(ui, repo, state, abortcommand=False)` and pass state as None, and abortcommand as True. When abortcommand is True, reassign state to new one and take wlock. Also make sure to release the wlock later in case of abortcommand.

> shelve.py:896
>  
> -        try:
> -            state = shelvedstate.load(repo)
> -            if opts.get('keep') is None:
> -                opts['keep'] = state.keep
> -        except IOError as err:
> -            if err.errno != errno.ENOENT:
> -                raise
> -            cmdutil.wrongtooltocontinue(repo, _('unshelve'))
> -        except error.CorruptedState as err:
> -            ui.debug(pycompat.bytestr(err) + '\n')
> -            if continuef:
> -                msg = _('corrupted shelved state file')
> -                hint = _('please run hg unshelve --abort to abort unshelve '
> -                         'operation')
> -                raise error.Abort(msg, hint=hint)
> -            elif abortf:
> -                msg = _('could not read shelved state file, your working copy '
> -                        'may be in an unexpected state\nplease update to some '
> -                        'commit\n')
> -                ui.warn(msg)
> -                shelvedstate.clear(repo)
> -            return
> +        statetuple = _loadshelvedstate(ui, repo, continuef=continuef,
> +                                       abortf=abortf, **opts)

same as a comment above related to directly assigning it to `state, opts`

REPOSITORY
  rHG Mercurial

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

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

To: taapas1128, #hg-reviewers
Cc: mharbison72, pulkit, mercurial-devel
phabricator - July 10, 2019, 6:54 p.m.
pulkit added a comment.


  The commit message is outdated.

REPOSITORY
  rHG Mercurial

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

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

To: taapas1128, #hg-reviewers
Cc: mharbison72, pulkit, mercurial-devel
phabricator - July 10, 2019, 7:02 p.m.
taapas1128 added a comment.


  updated that.

REPOSITORY
  rHG Mercurial

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

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

To: taapas1128, #hg-reviewers
Cc: mharbison72, pulkit, mercurial-devel
phabricator - July 10, 2019, 7:06 p.m.
pulkit added inline comments.

INLINE COMMENTS

> shelve.py:648
> +                                'please update to some commit\n'))
> +    return (state, opts)
> +

we can prevent returning opts here. One user does not need it and other one is passing by reference.

REPOSITORY
  rHG Mercurial

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

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

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

Patch

diff --git a/tests/test-abort.t b/tests/test-abort.t
--- a/tests/test-abort.t
+++ b/tests/test-abort.t
@@ -1,9 +1,8 @@ 
-####TEST `hg abort` operation rebase
-
   $ cat >> $HGRCPATH <<EOF
   > [extensions]
   > rebase=
-  > 
+  > shelve=
+  > mq =
   > [phases]
   > publish=False
   > 
@@ -11,7 +10,7 @@ 
   > tglog = log -G --template "{rev}:{phase} '{desc}' {branches}\n"
   > EOF
 
-
+####TEST `hg abort` operation rebase
   $ hg init a
   $ cd a
 
@@ -597,8 +596,78 @@ 
   cannot clean up public changesets 6ec71c037d94
   graft aborted
   working directory is now at 6ec71c037d94
-
-
-
-
-
+  $ cd ..
+
+###TEST `hg abort` operation unshelve
+
+Prepare unshelve with a corrupted shelvedstate
+  $ hg init r1 && cd r1
+  $ echo text1 > file && hg add file
+  $ hg shelve
+  shelved as default
+  0 files updated, 0 files merged, 1 files removed, 0 files unresolved
+  $ echo text2 > file && hg ci -Am text1
+  adding file
+  $ hg unshelve
+  unshelving change 'default'
+  rebasing shelved changes
+  merging file
+  warning: conflicts while merging file! (edit, then use 'hg resolve --mark')
+  unresolved conflicts (see 'hg resolve', then 'hg unshelve --continue')
+  [1]
+  $ echo somethingsomething > .hg/shelvedstate
+
+Unshelve abort works with a corrupted shelvedstate
+  $ hg abort
+  could not read shelved state file, your working copy may be in an unexpected state
+  please update to some commit
+
+Unshelve abort fails with appropriate message if there's no unshelve in
+progress
+  $ hg abort
+  abort: merge does not support 'hg abort'
+  (use 'hg commit' or 'hg merge --abort')
+  [255]
+  $ cd ..
+
+Abort due to pending changes
+  $ hg init onlypendingchanges
+  $ cd onlypendingchanges
+  $ touch a
+  $ hg ci -Aqm a
+  $ echo f > f
+  $ hg add f
+  $ hg shelve
+  shelved as default
+  0 files updated, 0 files merged, 1 files removed, 0 files unresolved
+  $ echo g > f
+  $ echo 1 > a
+  $ hg unshelve
+  unshelving change 'default'
+  temporarily committing pending changes (restore with 'hg unshelve --abort')
+  rebasing shelved changes
+  merging f
+  warning: conflicts while merging f! (edit, then use 'hg resolve --mark')
+  unresolved conflicts (see 'hg resolve', then 'hg unshelve --continue')
+  [1]
+  $ hg st
+  M f
+  ? f.orig
+  $ cat f
+  <<<<<<< shelve:       f0be9a229575 - shelve: pending changes temporary commit
+  g
+  =======
+  f
+  >>>>>>> working-copy: 7d4133053e12 - shelve: changes to: a
+  $ cat f.orig
+  g
+  $ hg ci a -m 'intermediate other change'
+  abort: unshelve already in progress
+  (use 'hg unshelve --continue' or 'hg unshelve --abort')
+  [255]
+  $ hg abort
+  unshelve of 'default' aborted
+  $ cd ..
+
+
+
diff --git a/hgext/shelve.py b/hgext/shelve.py
--- a/hgext/shelve.py
+++ b/hgext/shelve.py
@@ -645,9 +645,28 @@ 
         raise error.Abort(_('working directory parents do not match unshelve '
                            'state'))
 
-def unshelveabort(ui, repo, state, opts):
-    """subcommand that abort an in-progress unshelve"""
-    with repo.lock():
+def unshelveabort(ui, repo, state=None):
+    """subcommand that abort an in-progress unshelve
+       flag abort determines abort is called as a flag
+       or 'hg abort'
+    """
+    if not state:
+        try:
+            state = shelvedstate.load(repo)
+        except IOError as err:
+            if err.errno != errno.ENOENT:
+                raise
+            cmdutil.wrongtooltocontinue(repo, _('unshelve'))
+        except error.CorruptedState as err:
+            ui.debug(pycompat.bytestr(err) + '\n')
+            msg = _('could not read shelved state file, your working copy '
+                'may be in an unexpected state\nplease update to some '
+                'commit\n')
+            ui.warn(msg)
+            shelvedstate.clear(repo)
+            return
+
+    with repo.wlock(), repo.lock():
         try:
             checkparents(repo, state)
 
@@ -972,7 +991,7 @@ 
             return
 
         if abortf:
-            return unshelveabort(ui, repo, state, opts)
+            return unshelveabort(ui, repo, state)
         elif continuef:
             return unshelvecontinue(ui, repo, state, opts)
     elif len(shelved) > 1:
@@ -1140,6 +1159,6 @@ 
 def extsetup(ui):
     statemod.addunfinished(
         'unshelve', fname=shelvedstate._filename, continueflag=True,
-        cmdmsg=_('unshelve already in progress')
+        cmdmsg=_('unshelve already in progress'), abortfunc=unshelveabort
     )