Patchwork [10,of,10,shelve-ext] shelve: add logic to preserve active bookmarks

login
register
mail settings
Submitter Kostia Balytskyi
Date Nov. 29, 2016, 3:23 p.m.
Message ID <5c55e23c067998adf36b.1480432984@dev1902.lla1.facebook.com>
Download mbox | patch
Permalink /patch/17792/
State Accepted
Headers show

Comments

Kostia Balytskyi - Nov. 29, 2016, 3:23 p.m.
# HG changeset patch
# User Kostia Balytskyi <ikostia@fb.com>
# Date 1480431519 28800
#      Tue Nov 29 06:58:39 2016 -0800
# Node ID 5c55e23c067998adf36b9a2c6eb028ba1a7fc643
# Parent  533d99eca3bf11c4aac869674e0abb16b74ed670
shelve: add logic to preserve active bookmarks

This adds an explicit active-bookmark-handling logic
to *both* traditional and obs-based shelve. Although it
is possible to only add it to obs-based, I think it would
be ugly and I see no harm in explicitly handling bookmarks
in addition to reliance on trasnactions.
Jun Wu - Nov. 29, 2016, 4:36 p.m.
General direction looks good to me. I have a quick scan and commented on
some nits.

It seems the unshelve process is a different from the traditional one so
a lot of methods were changed to accept new "obsshelve" (bool), "tr",
"shfile" parameters. In this case, it may be cleaner to have two different
"unshelver" classes that implement different approaches and keep internal
states like "shfile" etc.

That said, the above is only a suggestion, not a request-change. We can
always do refactoring later.

Excerpts from Kostia Balytskyi's message of 2016-11-29 07:23:04 -0800:
> # HG changeset patch
> # User Kostia Balytskyi <ikostia@fb.com>
> # Date 1480431519 28800
> #      Tue Nov 29 06:58:39 2016 -0800
> # Node ID 5c55e23c067998adf36b9a2c6eb028ba1a7fc643
> # Parent  533d99eca3bf11c4aac869674e0abb16b74ed670
> shelve: add logic to preserve active bookmarks
> 
> This adds an explicit active-bookmark-handling logic
> to *both* traditional and obs-based shelve. Although it
> is possible to only add it to obs-based, I think it would
> be ugly and I see no harm in explicitly handling bookmarks
> in addition to reliance on trasnactions.
> 
> diff --git a/hgext/shelve.py b/hgext/shelve.py
> --- a/hgext/shelve.py
> +++ b/hgext/shelve.py
> @@ -30,6 +30,7 @@ import time
>  
>  from mercurial.i18n import _
>  from mercurial import (
> +    bookmarks,
>      bundle2,
>      bundlerepo,
>      changegroup,
> @@ -200,6 +201,8 @@ class shelvedstate(object):
>      _nokeep = 'nokeep'
>      _obsbased = 'obsbased'
>      _traditional = 'traditional'
> +    # colon is essential to differentiate from a real bookmark name
> +    _noactivebook = ':no-active-bookmark'
>  
>      def __init__(self, ui, repo):
>          self.ui = ui
> @@ -222,6 +225,7 @@ class shelvedstate(object):
>              branchtorestore = fp.readline().strip()
>              keep = fp.readline().strip() == cls._keep
>              obsshelve = fp.readline().strip() == cls._obsbased
> +            activebook = fp.readline().strip()
>          except (ValueError, TypeError) as err:
>              raise error.CorruptedState(str(err))
>          finally:
> @@ -237,6 +241,9 @@ class shelvedstate(object):
>              obj.branchtorestore = branchtorestore
>              obj.keep = keep
>              obj.obsshelve = obsshelve
> +            obj.activebookmark = ''
> +            if activebook != cls._noactivebook:
> +                obj.activebookmark = activebook
>          except error.RepoLookupError as err:
>              raise error.CorruptedState(str(err))
>  
> @@ -244,7 +251,7 @@ class shelvedstate(object):
>  
>      @classmethod
>      def save(cls, repo, name, originalwctx, pendingctx, nodestoprune,
> -             branchtorestore, keep=False, obsshelve=False):
> +             branchtorestore, keep=False, obsshelve=False, activebook=''):
>          fp = repo.vfs(cls._filename, 'wb')
>          fp.write('%i\n' % cls._version)
>          fp.write('%s\n' % name)
> @@ -257,6 +264,7 @@ class shelvedstate(object):
>          fp.write('%s\n' % branchtorestore)
>          fp.write('%s\n' % (cls._keep if keep else cls._nokeep))
>          fp.write('%s\n' % (cls._obsbased if obsshelve else cls._traditional))
> +        fp.write('%s\n' % (activebook or cls._noactivebook))
>          fp.close()
>  
>      @classmethod
> @@ -295,6 +303,16 @@ def cleanupoldbackups(repo):
>                  if err.errno != errno.ENOENT:
>                      raise
>  
> +def _backupactivebookmark(repo):
> +    activebookmark = repo._activebookmark
> +    if activebookmark:
> +        bookmarks.deactivate(repo)
> +    return activebookmark
> +
> +def _restoreactivebookmark(repo, mark):
> +    if mark:
> +        bookmarks.activate(repo, mark)
> +
>  def _aborttransaction(repo):
>      '''Abort current transaction for shelve/unshelve, but keep dirstate
>      '''
> @@ -410,7 +428,7 @@ def _includeunknownfiles(repo, pats, opt
>          extra['shelve_unknown'] = '\0'.join(s.unknown)
>          repo[None].add(s.unknown)
>  
> -def _finishshelve(ui, repo, tr):
> +def _finishshelve(ui, repo, tr, activebookmark):
>      if isobsshelve(repo, ui):
>          tr.close()
>          tr.release()
> @@ -433,7 +451,7 @@ def _docreatecmd(ui, repo, pats, opts):
>      if not opts.get('message'):
>          opts['message'] = desc
>  
> -    lock = tr = None
> +    lock = tr = activebookmark = None
>      try:
>          lock = repo.lock()
>  
> @@ -449,6 +467,7 @@ def _docreatecmd(ui, repo, pats, opts):
>                            not opts.get('addremove', False))
>  
>          name = getshelvename(repo, parent, opts)
> +        activebookmark = _backupactivebookmark(repo)
>          extra = {}
>          if includeunknown:
>              _includeunknownfiles(repo, pats, opts, extra)
> @@ -463,7 +482,8 @@ def _docreatecmd(ui, repo, pats, opts):
>              node = cmdutil.commit(ui, repo, commitfunc, pats, opts)
>          else:
>              node = cmdutil.dorecord(ui, repo, commitfunc, None,
> -                                    False, cmdutil.recordfilter, *pats, **opts)
> +                                    False, cmdutil.recordfilter, *pats,
> +                                    **opts)
>          if not node:
>              _nothingtoshelvemessaging(ui, repo, pats, opts)
>              return 1
> @@ -477,8 +497,9 @@ def _docreatecmd(ui, repo, pats, opts):
>          if origbranch != repo['.'].branch() and not _isbareshelve(pats, opts):
>              repo.dirstate.setbranch(origbranch)
>  
> -        _finishshelve(ui, repo, tr)
> +        _finishshelve(ui, repo, tr, activebookmark)
>      finally:
> +        _restoreactivebookmark(repo, activebookmark)
>          lockmod.release(tr, lock)
>  
>  def _isbareshelve(pats, opts):
> @@ -701,6 +722,7 @@ def unshelvecontinue(ui, repo, state, op
>          restorebranch(ui, repo, state.branchtorestore)
>  
>          state.prunenodes()
> +        _restoreactivebookmark(repo, state.activebookmark)
>          shelvedstate.clear(repo)
>          unshelvecleanup(ui, repo, state.name, opts)
>          ui.status(_("unshelve of '%s' complete\n") % state.name)
> @@ -741,7 +763,8 @@ def _unshelverestorecommit(ui, repo, bas
>      return repo, shelvectx
>  
>  def _rebaserestoredcommit(ui, repo, opts, tr, oldtiprev, basename, pctx,
> -                          tmpwctx, shelvectx, branchtorestore, obsshelve):
> +                          tmpwctx, shelvectx, branchtorestore, obsshelve,
> +                          activebookmark):
>      """Rebase restored commit from its original location to a destination"""
>      # If the shelve is not immediately on top of the commit
>      # we'll be merging with, rebase it to be on top.
> @@ -778,7 +801,8 @@ def _rebaserestoredcommit(ui, repo, opts
>          nodestoprune = [repo.changelog.node(rev)
>                          for rev in xrange(oldtiprev, len(repo))]
>          shelvedstate.save(repo, basename, pctx, tmpwctx, nodestoprune,
> -                          branchtorestore, opts.get('keep'), obsshelve)
> +                          branchtorestore, opts.get('keep'), obsshelve,
> +                          activebookmark)
>  
>          util.rename(repo.join('rebasestate'),
>                      repo.join('unshelverebasestate'))
> @@ -805,7 +829,8 @@ def _forgetunknownfiles(repo, shelvectx,
>      toforget = (addedafter & shelveunknown) - addedbefore
>      repo[None].forget(toforget)
>  
> -def _finishunshelve(repo, oldtiprev, tr, obsshelve):
> +def _finishunshelve(repo, oldtiprev, tr, obsshelve, activebookmark):
> +    _restoreactivebookmark(repo, activebookmark)
>      if obsshelve:
>          tr.close()
>          return
> @@ -954,6 +979,7 @@ def _dounshelve(ui, repo, *shelved, **op
>          # and shelvectx is the unshelved changes. Then we merge it all down
>          # to the original pctx.
>  
> +        activebookmark = _backupactivebookmark(repo)
>          tmpwctx, addedbefore = _commitworkingcopychanges(ui, repo, opts,
>                                                           tmpwctx)
>  
> @@ -970,7 +996,7 @@ def _dounshelve(ui, repo, *shelved, **op
>              shelvectx = _rebaserestoredcommit(ui, repo, opts, tr, oldtiprev,
>                                                basename, pctx, tmpwctx,
>                                                shelvectx, branchtorestore,
> -                                              obsshelve)
> +                                              obsshelve, activebookmark)
>              mergefiles(ui, repo, pctx, shelvectx)
>          restorebranch(ui, repo, branchtorestore)
>          _forgetunknownfiles(repo, shelvectx, addedbefore)
> @@ -979,7 +1005,7 @@ def _dounshelve(ui, repo, *shelved, **op
>              _obsoleteredundantnodes(repo, tr, pctx, shelvectx, tmpwctx)
>  
>          shelvedstate.clear(repo)
> -        _finishunshelve(repo, oldtiprev, tr, obsshelve)
> +        _finishunshelve(repo, oldtiprev, tr, obsshelve, activebookmark)
>          unshelvecleanup(ui, repo, basename, opts)
>      finally:
>          if tr:

Patch

diff --git a/hgext/shelve.py b/hgext/shelve.py
--- a/hgext/shelve.py
+++ b/hgext/shelve.py
@@ -30,6 +30,7 @@  import time
 
 from mercurial.i18n import _
 from mercurial import (
+    bookmarks,
     bundle2,
     bundlerepo,
     changegroup,
@@ -200,6 +201,8 @@  class shelvedstate(object):
     _nokeep = 'nokeep'
     _obsbased = 'obsbased'
     _traditional = 'traditional'
+    # colon is essential to differentiate from a real bookmark name
+    _noactivebook = ':no-active-bookmark'
 
     def __init__(self, ui, repo):
         self.ui = ui
@@ -222,6 +225,7 @@  class shelvedstate(object):
             branchtorestore = fp.readline().strip()
             keep = fp.readline().strip() == cls._keep
             obsshelve = fp.readline().strip() == cls._obsbased
+            activebook = fp.readline().strip()
         except (ValueError, TypeError) as err:
             raise error.CorruptedState(str(err))
         finally:
@@ -237,6 +241,9 @@  class shelvedstate(object):
             obj.branchtorestore = branchtorestore
             obj.keep = keep
             obj.obsshelve = obsshelve
+            obj.activebookmark = ''
+            if activebook != cls._noactivebook:
+                obj.activebookmark = activebook
         except error.RepoLookupError as err:
             raise error.CorruptedState(str(err))
 
@@ -244,7 +251,7 @@  class shelvedstate(object):
 
     @classmethod
     def save(cls, repo, name, originalwctx, pendingctx, nodestoprune,
-             branchtorestore, keep=False, obsshelve=False):
+             branchtorestore, keep=False, obsshelve=False, activebook=''):
         fp = repo.vfs(cls._filename, 'wb')
         fp.write('%i\n' % cls._version)
         fp.write('%s\n' % name)
@@ -257,6 +264,7 @@  class shelvedstate(object):
         fp.write('%s\n' % branchtorestore)
         fp.write('%s\n' % (cls._keep if keep else cls._nokeep))
         fp.write('%s\n' % (cls._obsbased if obsshelve else cls._traditional))
+        fp.write('%s\n' % (activebook or cls._noactivebook))
         fp.close()
 
     @classmethod
@@ -295,6 +303,16 @@  def cleanupoldbackups(repo):
                 if err.errno != errno.ENOENT:
                     raise
 
+def _backupactivebookmark(repo):
+    activebookmark = repo._activebookmark
+    if activebookmark:
+        bookmarks.deactivate(repo)
+    return activebookmark
+
+def _restoreactivebookmark(repo, mark):
+    if mark:
+        bookmarks.activate(repo, mark)
+
 def _aborttransaction(repo):
     '''Abort current transaction for shelve/unshelve, but keep dirstate
     '''
@@ -410,7 +428,7 @@  def _includeunknownfiles(repo, pats, opt
         extra['shelve_unknown'] = '\0'.join(s.unknown)
         repo[None].add(s.unknown)
 
-def _finishshelve(ui, repo, tr):
+def _finishshelve(ui, repo, tr, activebookmark):
     if isobsshelve(repo, ui):
         tr.close()
         tr.release()
@@ -433,7 +451,7 @@  def _docreatecmd(ui, repo, pats, opts):
     if not opts.get('message'):
         opts['message'] = desc
 
-    lock = tr = None
+    lock = tr = activebookmark = None
     try:
         lock = repo.lock()
 
@@ -449,6 +467,7 @@  def _docreatecmd(ui, repo, pats, opts):
                           not opts.get('addremove', False))
 
         name = getshelvename(repo, parent, opts)
+        activebookmark = _backupactivebookmark(repo)
         extra = {}
         if includeunknown:
             _includeunknownfiles(repo, pats, opts, extra)
@@ -463,7 +482,8 @@  def _docreatecmd(ui, repo, pats, opts):
             node = cmdutil.commit(ui, repo, commitfunc, pats, opts)
         else:
             node = cmdutil.dorecord(ui, repo, commitfunc, None,
-                                    False, cmdutil.recordfilter, *pats, **opts)
+                                    False, cmdutil.recordfilter, *pats,
+                                    **opts)
         if not node:
             _nothingtoshelvemessaging(ui, repo, pats, opts)
             return 1
@@ -477,8 +497,9 @@  def _docreatecmd(ui, repo, pats, opts):
         if origbranch != repo['.'].branch() and not _isbareshelve(pats, opts):
             repo.dirstate.setbranch(origbranch)
 
-        _finishshelve(ui, repo, tr)
+        _finishshelve(ui, repo, tr, activebookmark)
     finally:
+        _restoreactivebookmark(repo, activebookmark)
         lockmod.release(tr, lock)
 
 def _isbareshelve(pats, opts):
@@ -701,6 +722,7 @@  def unshelvecontinue(ui, repo, state, op
         restorebranch(ui, repo, state.branchtorestore)
 
         state.prunenodes()
+        _restoreactivebookmark(repo, state.activebookmark)
         shelvedstate.clear(repo)
         unshelvecleanup(ui, repo, state.name, opts)
         ui.status(_("unshelve of '%s' complete\n") % state.name)
@@ -741,7 +763,8 @@  def _unshelverestorecommit(ui, repo, bas
     return repo, shelvectx
 
 def _rebaserestoredcommit(ui, repo, opts, tr, oldtiprev, basename, pctx,
-                          tmpwctx, shelvectx, branchtorestore, obsshelve):
+                          tmpwctx, shelvectx, branchtorestore, obsshelve,
+                          activebookmark):
     """Rebase restored commit from its original location to a destination"""
     # If the shelve is not immediately on top of the commit
     # we'll be merging with, rebase it to be on top.
@@ -778,7 +801,8 @@  def _rebaserestoredcommit(ui, repo, opts
         nodestoprune = [repo.changelog.node(rev)
                         for rev in xrange(oldtiprev, len(repo))]
         shelvedstate.save(repo, basename, pctx, tmpwctx, nodestoprune,
-                          branchtorestore, opts.get('keep'), obsshelve)
+                          branchtorestore, opts.get('keep'), obsshelve,
+                          activebookmark)
 
         util.rename(repo.join('rebasestate'),
                     repo.join('unshelverebasestate'))
@@ -805,7 +829,8 @@  def _forgetunknownfiles(repo, shelvectx,
     toforget = (addedafter & shelveunknown) - addedbefore
     repo[None].forget(toforget)
 
-def _finishunshelve(repo, oldtiprev, tr, obsshelve):
+def _finishunshelve(repo, oldtiprev, tr, obsshelve, activebookmark):
+    _restoreactivebookmark(repo, activebookmark)
     if obsshelve:
         tr.close()
         return
@@ -954,6 +979,7 @@  def _dounshelve(ui, repo, *shelved, **op
         # and shelvectx is the unshelved changes. Then we merge it all down
         # to the original pctx.
 
+        activebookmark = _backupactivebookmark(repo)
         tmpwctx, addedbefore = _commitworkingcopychanges(ui, repo, opts,
                                                          tmpwctx)
 
@@ -970,7 +996,7 @@  def _dounshelve(ui, repo, *shelved, **op
             shelvectx = _rebaserestoredcommit(ui, repo, opts, tr, oldtiprev,
                                               basename, pctx, tmpwctx,
                                               shelvectx, branchtorestore,
-                                              obsshelve)
+                                              obsshelve, activebookmark)
             mergefiles(ui, repo, pctx, shelvectx)
         restorebranch(ui, repo, branchtorestore)
         _forgetunknownfiles(repo, shelvectx, addedbefore)
@@ -979,7 +1005,7 @@  def _dounshelve(ui, repo, *shelved, **op
             _obsoleteredundantnodes(repo, tr, pctx, shelvectx, tmpwctx)
 
         shelvedstate.clear(repo)
-        _finishunshelve(repo, oldtiprev, tr, obsshelve)
+        _finishunshelve(repo, oldtiprev, tr, obsshelve, activebookmark)
         unshelvecleanup(ui, repo, basename, opts)
     finally:
         if tr: