Patchwork [6,of,7,shelve-ext,v5] shelve: add obs-based unshelve functionality

# HG changeset patch
# User Kostia Balytskyi <ikostia@fb.com>
# Date 1490790691 25200
#      Wed Mar 29 05:31:31 2017 -0700
# Node ID 9ce0a366a6316d1cffc2a875e7dc6b4a3227c851
# Parent  743fea249a852994c5bd3e47cdfb617719f19a0a
shelve: add obs-based unshelve functionality

Obsolescense-based unshelve works as follows:
1. Instead of stripping temporary nodes, markers are created to
obsolete them.
2. Restoring commit is just finding it in an unfiltered repo.
3. '--keep' is only passed to rebase on traditional unshelves
(and thus traditional rebases), becuase we want markers to be
created fro obsolete-based rebases.
4. 'hg unshelve' uses unfiltered repo to perform rebases
because we want rebase to be able to create markers between original
and new commits. 'rebaseskipobsolete' is disabled to make rebase not
skip the commit altogether.
5. As per sprint discussions, hiding commits with obsmarkers is
not ideal. This is okay for now, as long as obsshelve is an
experimental feature. In future, once a better approach to
hiding things is implemented, we can change the way obsshelve
works (and also change its name to not refer to obsolescense).

Tests for obs-shelve are coming in a separate series.

On 3/29/17 2:18 PM, Kostia Balytskyi wrote:
> # HG changeset patch
> # User Kostia Balytskyi <ikostia@fb.com>
> # Date 1490790691 25200
> #      Wed Mar 29 05:31:31 2017 -0700
> # Node ID 9ce0a366a6316d1cffc2a875e7dc6b4a3227c851
> # Parent  743fea249a852994c5bd3e47cdfb617719f19a0a
> shelve: add obs-based unshelve functionality
>
> Obsolescense-based unshelve works as follows:
> 1. Instead of stripping temporary nodes, markers are created to
> obsolete them.
> 2. Restoring commit is just finding it in an unfiltered repo.
> 3. '--keep' is only passed to rebase on traditional unshelves
> (and thus traditional rebases), becuase we want markers to be
> created fro obsolete-based rebases.
s/fro/for

> 4. 'hg unshelve' uses unfiltered repo to perform rebases
> because we want rebase to be able to create markers between original
> and new commits. 'rebaseskipobsolete' is disabled to make rebase not
> skip the commit altogether.
> 5. As per sprint discussions, hiding commits with obsmarkers is
> not ideal. This is okay for now, as long as obsshelve is an
> experimental feature. In future, once a better approach to
s/In future/In the future

> hiding things is implemented, we can change the way obsshelve
> works (and also change its name to not refer to obsolescense).
This is a great comment.

>
> Tests for obs-shelve are coming in a separate series.
Why?

>
> diff --git a/hgext/shelve.py b/hgext/shelve.py
> --- a/hgext/shelve.py
> +++ b/hgext/shelve.py
> @@ -25,6 +25,7 @@ from __future__ import absolute_import
>   import collections
>   import errno
>   import itertools
> +import time
>   
>   from mercurial.i18n import _
>   from mercurial import (
> @@ -254,8 +255,13 @@ class shelvedstate(object):
>   
>       def prunenodes(self, ui, repo):
>           """Cleanup temporary nodes from the repo"""
> -        repair.strip(ui, repo, self.nodestoprune, backup=False,
> -                     topic='shelve')
> +        if self.obsshelve:
> +            unfi = repo.unfiltered()
> +            relations = [(unfi[n], ()) for n in self.nodestoprune]
> +            obsolete.createmarkers(repo, relations)
> +        else:
> +            repair.strip(ui, repo, self.nodestoprune, backup=False,
> +                         topic='shelve')
>   
>   def cleanupoldbackups(repo):
>       vfs = vfsmod.vfs(repo.vfs.join(backupdir))
> @@ -675,9 +681,14 @@ def unshelvecontinue(ui, repo, state, op
>   
>           repo.vfs.rename('unshelverebasestate', 'rebasestate')
>           try:
> -            rebase.rebase(ui, repo, **{
> -                'continue' : True
> -            })
> +            # if shelve is obs-based, we want rebase to be able
> +            # to create markers to already-obsoleted commits
> +            _repo = repo.unfiltered() if state.obsshelve else repo
> +            with ui.configoverride({('experimental', 'rebaseskipobsolete'):
> +                                    'off'}, 'unshelve'):
> +                rebase.rebase(ui, _repo, **{
> +                    'continue' : True,
> +                    })
>           except Exception:
>               repo.vfs.rename('rebasestate', 'unshelverebasestate')
>               raise
> @@ -717,31 +728,59 @@ def _commitworkingcopychanges(ui, repo,
>       with ui.configoverride({('ui', 'quiet'): True}):
>           node = cmdutil.commit(ui, repo, commitfunc, [], tempopts)
>       tmpwctx = repo[node]
> +    ui.debug("temporary working copy commit: %s:%s\n" %
> +             (tmpwctx.rev(), nodemod.short(node)))
>       return tmpwctx, addedbefore
>   
> -def _unshelverestorecommit(ui, repo, basename):
> +def _unshelverestorecommit(ui, repo, basename, obsshelve):
>       """Recreate commit in the repository during the unshelve"""
>       with ui.configoverride({('ui', 'quiet'): True}):
> -        shelvedfile(repo, basename, 'hg').applybundle()
> -        shelvectx = repo['tip']
> +        if obsshelve:
> +            md = shelvedfile(repo, basename, 'oshelve').readobsshelveinfo()
> +            shelvenode = nodemod.bin(md['node'])
> +            repo = repo.unfiltered()
> +            try:
> +                shelvectx = repo[shelvenode]
> +            except error.RepoLookupError:
> +                m = _("shelved node %s not found in repo")
> +                raise error.Abort(m % md['node'])
> +        else:
> +            shelvedfile(repo, basename, 'hg').applybundle()
> +            shelvectx = repo['tip']
>       return repo, shelvectx
>   
>   def _rebaserestoredcommit(ui, repo, opts, tr, oldtiprev, basename, pctx,
>                             tmpwctx, shelvectx, branchtorestore,
> -                          activebookmark):
> +                          activebookmark, obsshelve):
>       """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.
>       if tmpwctx.node() == shelvectx.parents()[0].node():
> +        # shelvectx is immediately on top of the tmpwctx
>           return shelvectx
>   
> +    # we need a new commit extra every time we perform a rebase to ensure
> +    # that "nothing to rebase" does not happen with obs-based shelve
> +    # "nothing to rebase" means that tip does not point to a "successor"
> +    # commit after a rebase and we have no way to learn which commit
> +    # should be a "shelvectx". this is a dirty hack until we implement
> +    # some way to learn the results of rebase operation, other than
> +    # text output and return code
> +    def extrafn(ctx, extra):
> +        extra['unshelve_time'] = str(time.time())
> +
>       ui.status(_('rebasing shelved changes\n'))
>       try:
> +        # we only want keep to be true if shelve is traditional, since
> +        # for obs-based shelve, rebase will also be obs-based and
> +        # markers created help us track the relationship between shelvectx
> +        # and its new version
>           rebase.rebase(ui, repo, **{
>               'rev': [shelvectx.rev()],
>               'dest': str(tmpwctx.rev()),
> -            'keep': True,
> +            'keep': not obsshelve,
>               'tool': opts.get('tool', ''),
> +            'extrafn': extrafn if obsshelve else None
>           })
>       except error.InterventionRequired:
>           tr.close()
> @@ -749,7 +788,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'), activebookmark)
> +                          branchtorestore, opts.get('keep'),
> +                          activebookmark, obsshelve)
>   
>           repo.vfs.rename('rebasestate', 'unshelverebasestate')
>           raise error.InterventionRequired(
> @@ -775,8 +815,11 @@ def _forgetunknownfiles(repo, shelvectx,
>       toforget = (addedafter & shelveunknown) - addedbefore
>       repo[None].forget(toforget)
>   
> -def _finishunshelve(repo, oldtiprev, tr, activebookmark):
> +def _finishunshelve(repo, oldtiprev, tr, activebookmark, obsshelve):
>       _restoreactivebookmark(repo, activebookmark)
> +    if obsshelve:
> +        tr.close()
> +        return
>       # The transaction aborting will strip all the commits for us,
>       # but it doesn't update the inmemory structures, so addchangegroup
>       # hooks still fire and try to operate on the missing commits.
> @@ -795,6 +838,20 @@ def _checkunshelveuntrackedproblems(ui,
>           hint = _("run hg status to see which files are missing")
>           raise error.Abort(m, hint=hint)
>   
> +def _obsoleteredundantnodes(repo, tr, pctx, shelvectx, tmpwctx):
Maybe call this pruneredundantnodes?

Comment should clarify what is considered "redundant".
> +    # order is important in the list of [shelvectx, tmpwctx] below
> +    # some nodes may already be obsolete
> +    unfi = repo.unfiltered()
> +    nodestoobsolete = filter(lambda x: x != pctx, [shelvectx, tmpwctx])
> +    seen = set()
> +    relations = []
> +    for nto in nodestoobsolete:
> +        if nto in seen:
> +            continue
> +        seen.add(nto)
> +        relations.append((unfi[nto.rev()], ()))
> +    obsolete.createmarkers(unfi, relations)
> +
>   @command('unshelve',
>            [('a', 'abort', None,
>              _('abort an incomplete unshelve operation')),
> @@ -907,6 +964,14 @@ def _dounshelve(ui, repo, *shelved, **op
>           raise error.Abort(_("shelved change '%s' not found") % basename)
>   
>       lock = tr = None
> +    obsshelve = isobsshelve(repo, ui)
> +    obsshelvedfile = shelvedfile(repo, basename, 'oshelve')
> +    if obsshelve and not obsshelvedfile.exists():
> +        # although we can unshelve a obs-based shelve technically,
> +        # this particular shelve was created using a traditional way
> +        obsshelve = False
> +        ui.note(_("falling back to traditional unshelve since "
> +                  "shelve was traditional"))
>       try:
>           lock = repo.lock()
>           tr = repo.transaction('unshelve', report=lambda x: None)
> @@ -921,28 +986,31 @@ def _dounshelve(ui, repo, *shelved, **op
>           # to the original pctx.
>   
>           activebookmark = _backupactivebookmark(repo)
> -        overrides = {('ui', 'forcemerge'): opts.get('tool', '')}
> -        with ui.configoverride(overrides, 'unshelve'):
> -            tmpwctx, addedbefore = _commitworkingcopychanges(ui, repo, opts,
> -                                                             tmpwctx)
> -            repo, shelvectx = _unshelverestorecommit(ui, repo, basename,
> -                                                     oldquiet)
> -            _checkunshelveuntrackedproblems(ui, repo, shelvectx)
> -            branchtorestore = ''
> -            if shelvectx.branch() != shelvectx.p1().branch():
> -                branchtorestore = shelvectx.branch()
> +        tmpwctx, addedbefore = _commitworkingcopychanges(ui, repo, opts,
> +                                                         tmpwctx)
> +        repo, shelvectx = _unshelverestorecommit(ui, repo, basename, obsshelve)
> +        _checkunshelveuntrackedproblems(ui, repo, shelvectx)
> +        branchtorestore = ''
> +        if shelvectx.branch() != shelvectx.p1().branch():
> +            branchtorestore = shelvectx.branch()
>   
> +        rebaseconfigoverrides = {('ui', 'forcemerge'): opts.get('tool', ''),
> +                                 ('experimental', 'rebaseskipobsolete'): 'off'}
> +        with ui.configoverride(rebaseconfigoverrides, 'unshelve'):
>               shelvectx = _rebaserestoredcommit(ui, repo, opts, tr, oldtiprev,
>                                                 basename, pctx, tmpwctx,
>                                                 shelvectx, branchtorestore,
> -                                              activebookmark)
> +                                              activebookmark, obsshelve)
>               mergefiles(ui, repo, pctx, shelvectx)
>               restorebranch(ui, repo, branchtorestore)
>               _forgetunknownfiles(repo, shelvectx, addedbefore)
>   
> -            shelvedstate.clear(repo)
> -            _finishunshelve(repo, oldtiprev, tr, activebookmark)
> -            unshelvecleanup(ui, repo, basename, opts)
> +        if obsshelve:
> +            _obsoleteredundantnodes(repo, tr, pctx, shelvectx, tmpwctx)
> +
> +        shelvedstate.clear(repo)
> +        _finishunshelve(repo, oldtiprev, tr, activebookmark, obsshelve)
> +        unshelvecleanup(ui, repo, basename, opts)
>       finally:
>           if tr:
>               tr.release()

Again, I think that this would be clearer with more separated logic 
rather than interspersing the two. It also seems that there are a lot of 
not-obviously-related changes here, such as renaming overrides, which 
make it more taxing to review.

On 30/03/2017 10:24, Ryan McElroy wrote:
> On 3/29/17 2:18 PM, Kostia Balytskyi wrote:

>> # HG changeset patch

>> # User Kostia Balytskyi <ikostia@fb.com>

>> # Date 1490790691 25200

>> #      Wed Mar 29 05:31:31 2017 -0700

>> # Node ID 9ce0a366a6316d1cffc2a875e7dc6b4a3227c851

>> # Parent  743fea249a852994c5bd3e47cdfb617719f19a0a

>> shelve: add obs-based unshelve functionality

>>

>> Obsolescense-based unshelve works as follows:

>> 1. Instead of stripping temporary nodes, markers are created to

>> obsolete them.

>> 2. Restoring commit is just finding it in an unfiltered repo.

>> 3. '--keep' is only passed to rebase on traditional unshelves

>> (and thus traditional rebases), becuase we want markers to be

>> created fro obsolete-based rebases.

> s/fro/for

>

>> 4. 'hg unshelve' uses unfiltered repo to perform rebases

>> because we want rebase to be able to create markers between original

>> and new commits. 'rebaseskipobsolete' is disabled to make rebase not

>> skip the commit altogether.

>> 5. As per sprint discussions, hiding commits with obsmarkers is

>> not ideal. This is okay for now, as long as obsshelve is an

>> experimental feature. In future, once a better approach to

> s/In future/In the future

>

>> hiding things is implemented, we can change the way obsshelve

>> works (and also change its name to not refer to obsolescense).

> This is a great comment.

>

>>

>> Tests for obs-shelve are coming in a separate series.

> Why?

It is not yet clear what is the best way to send them. It can either be 
a single >1000 line
file or ~30 separate patches. In any case, I want to have series as 
short as possible.
>

>>

>> diff --git a/hgext/shelve.py b/hgext/shelve.py

>> --- a/hgext/shelve.py

>> +++ b/hgext/shelve.py

>> @@ -25,6 +25,7 @@ from __future__ import absolute_import

>>   import collections

>>   import errno

>>   import itertools

>> +import time

>>     from mercurial.i18n import _

>>   from mercurial import (

>> @@ -254,8 +255,13 @@ class shelvedstate(object):

>>         def prunenodes(self, ui, repo):

>>           """Cleanup temporary nodes from the repo"""

>> -        repair.strip(ui, repo, self.nodestoprune, backup=False,

>> -                     topic='shelve')

>> +        if self.obsshelve:

>> +            unfi = repo.unfiltered()

>> +            relations = [(unfi[n], ()) for n in self.nodestoprune]

>> +            obsolete.createmarkers(repo, relations)

>> +        else:

>> +            repair.strip(ui, repo, self.nodestoprune, backup=False,

>> +                         topic='shelve')

>>     def cleanupoldbackups(repo):

>>       vfs = vfsmod.vfs(repo.vfs.join(backupdir))

>> @@ -675,9 +681,14 @@ def unshelvecontinue(ui, repo, state, op

>>             repo.vfs.rename('unshelverebasestate', 'rebasestate')

>>           try:

>> -            rebase.rebase(ui, repo, **{

>> -                'continue' : True

>> -            })

>> +            # if shelve is obs-based, we want rebase to be able

>> +            # to create markers to already-obsoleted commits

>> +            _repo = repo.unfiltered() if state.obsshelve else repo

>> +            with ui.configoverride({('experimental', 

>> 'rebaseskipobsolete'):

>> +                                    'off'}, 'unshelve'):

>> +                rebase.rebase(ui, _repo, **{

>> +                    'continue' : True,

>> +                    })

>>           except Exception:

>>               repo.vfs.rename('rebasestate', 'unshelverebasestate')

>>               raise

>> @@ -717,31 +728,59 @@ def _commitworkingcopychanges(ui, repo,

>>       with ui.configoverride({('ui', 'quiet'): True}):

>>           node = cmdutil.commit(ui, repo, commitfunc, [], tempopts)

>>       tmpwctx = repo[node]

>> +    ui.debug("temporary working copy commit: %s:%s\n" %

>> +             (tmpwctx.rev(), nodemod.short(node)))

>>       return tmpwctx, addedbefore

>>   -def _unshelverestorecommit(ui, repo, basename):

>> +def _unshelverestorecommit(ui, repo, basename, obsshelve):

>>       """Recreate commit in the repository during the unshelve"""

>>       with ui.configoverride({('ui', 'quiet'): True}):

>> -        shelvedfile(repo, basename, 'hg').applybundle()

>> -        shelvectx = repo['tip']

>> +        if obsshelve:

>> +            md = shelvedfile(repo, basename, 

>> 'oshelve').readobsshelveinfo()

>> +            shelvenode = nodemod.bin(md['node'])

>> +            repo = repo.unfiltered()

>> +            try:

>> +                shelvectx = repo[shelvenode]

>> +            except error.RepoLookupError:

>> +                m = _("shelved node %s not found in repo")

>> +                raise error.Abort(m % md['node'])

>> +        else:

>> +            shelvedfile(repo, basename, 'hg').applybundle()

>> +            shelvectx = repo['tip']

>>       return repo, shelvectx

>>     def _rebaserestoredcommit(ui, repo, opts, tr, oldtiprev, 

>> basename, pctx,

>>                             tmpwctx, shelvectx, branchtorestore,

>> -                          activebookmark):

>> +                          activebookmark, obsshelve):

>>       """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.

>>       if tmpwctx.node() == shelvectx.parents()[0].node():

>> +        # shelvectx is immediately on top of the tmpwctx

>>           return shelvectx

>>   +    # we need a new commit extra every time we perform a rebase to 

>> ensure

>> +    # that "nothing to rebase" does not happen with obs-based shelve

>> +    # "nothing to rebase" means that tip does not point to a 

>> "successor"

>> +    # commit after a rebase and we have no way to learn which commit

>> +    # should be a "shelvectx". this is a dirty hack until we implement

>> +    # some way to learn the results of rebase operation, other than

>> +    # text output and return code

>> +    def extrafn(ctx, extra):

>> +        extra['unshelve_time'] = str(time.time())

>> +

>>       ui.status(_('rebasing shelved changes\n'))

>>       try:

>> +        # we only want keep to be true if shelve is traditional, since

>> +        # for obs-based shelve, rebase will also be obs-based and

>> +        # markers created help us track the relationship between 

>> shelvectx

>> +        # and its new version

>>           rebase.rebase(ui, repo, **{

>>               'rev': [shelvectx.rev()],

>>               'dest': str(tmpwctx.rev()),

>> -            'keep': True,

>> +            'keep': not obsshelve,

>>               'tool': opts.get('tool', ''),

>> +            'extrafn': extrafn if obsshelve else None

>>           })

>>       except error.InterventionRequired:

>>           tr.close()

>> @@ -749,7 +788,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'), 

>> activebookmark)

>> +                          branchtorestore, opts.get('keep'),

>> +                          activebookmark, obsshelve)

>>             repo.vfs.rename('rebasestate', 'unshelverebasestate')

>>           raise error.InterventionRequired(

>> @@ -775,8 +815,11 @@ def _forgetunknownfiles(repo, shelvectx,

>>       toforget = (addedafter & shelveunknown) - addedbefore

>>       repo[None].forget(toforget)

>>   -def _finishunshelve(repo, oldtiprev, tr, activebookmark):

>> +def _finishunshelve(repo, oldtiprev, tr, activebookmark, obsshelve):

>>       _restoreactivebookmark(repo, activebookmark)

>> +    if obsshelve:

>> +        tr.close()

>> +        return

>>       # The transaction aborting will strip all the commits for us,

>>       # but it doesn't update the inmemory structures, so addchangegroup

>>       # hooks still fire and try to operate on the missing commits.

>> @@ -795,6 +838,20 @@ def _checkunshelveuntrackedproblems(ui,

>>           hint = _("run hg status to see which files are missing")

>>           raise error.Abort(m, hint=hint)

>>   +def _obsoleteredundantnodes(repo, tr, pctx, shelvectx, tmpwctx):

> Maybe call this pruneredundantnodes?

>

> Comment should clarify what is considered "redundant".

>> +    # order is important in the list of [shelvectx, tmpwctx] below

>> +    # some nodes may already be obsolete

>> +    unfi = repo.unfiltered()

>> +    nodestoobsolete = filter(lambda x: x != pctx, [shelvectx, tmpwctx])

>> +    seen = set()

>> +    relations = []

>> +    for nto in nodestoobsolete:

>> +        if nto in seen:

>> +            continue

>> +        seen.add(nto)

>> +        relations.append((unfi[nto.rev()], ()))

>> +    obsolete.createmarkers(unfi, relations)

>> +

>>   @command('unshelve',

>>            [('a', 'abort', None,

>>              _('abort an incomplete unshelve operation')),

>> @@ -907,6 +964,14 @@ def _dounshelve(ui, repo, *shelved, **op

>>           raise error.Abort(_("shelved change '%s' not found") % 

>> basename)

>>         lock = tr = None

>> +    obsshelve = isobsshelve(repo, ui)

>> +    obsshelvedfile = shelvedfile(repo, basename, 'oshelve')

>> +    if obsshelve and not obsshelvedfile.exists():

>> +        # although we can unshelve a obs-based shelve technically,

>> +        # this particular shelve was created using a traditional way

>> +        obsshelve = False

>> +        ui.note(_("falling back to traditional unshelve since "

>> +                  "shelve was traditional"))

>>       try:

>>           lock = repo.lock()

>>           tr = repo.transaction('unshelve', report=lambda x: None)

>> @@ -921,28 +986,31 @@ def _dounshelve(ui, repo, *shelved, **op

>>           # to the original pctx.

>>             activebookmark = _backupactivebookmark(repo)

>> -        overrides = {('ui', 'forcemerge'): opts.get('tool', '')}

>> -        with ui.configoverride(overrides, 'unshelve'):

>> -            tmpwctx, addedbefore = _commitworkingcopychanges(ui, 

>> repo, opts,

>> - tmpwctx)

>> -            repo, shelvectx = _unshelverestorecommit(ui, repo, 

>> basename,

>> -                                                     oldquiet)

>> -            _checkunshelveuntrackedproblems(ui, repo, shelvectx)

>> -            branchtorestore = ''

>> -            if shelvectx.branch() != shelvectx.p1().branch():

>> -                branchtorestore = shelvectx.branch()

>> +        tmpwctx, addedbefore = _commitworkingcopychanges(ui, repo, 

>> opts,

>> + tmpwctx)

>> +        repo, shelvectx = _unshelverestorecommit(ui, repo, basename, 

>> obsshelve)

>> +        _checkunshelveuntrackedproblems(ui, repo, shelvectx)

>> +        branchtorestore = ''

>> +        if shelvectx.branch() != shelvectx.p1().branch():

>> +            branchtorestore = shelvectx.branch()

>>   +        rebaseconfigoverrides = {('ui', 'forcemerge'): 

>> opts.get('tool', ''),

>> +                                 ('experimental', 

>> 'rebaseskipobsolete'): 'off'}

>> +        with ui.configoverride(rebaseconfigoverrides, 'unshelve'):

>>               shelvectx = _rebaserestoredcommit(ui, repo, opts, tr, 

>> oldtiprev,

>>                                                 basename, pctx, tmpwctx,

>>                                                 shelvectx, 

>> branchtorestore,

>> -                                              activebookmark)

>> +                                              activebookmark, 

>> obsshelve)

>>               mergefiles(ui, repo, pctx, shelvectx)

>>               restorebranch(ui, repo, branchtorestore)

>>               _forgetunknownfiles(repo, shelvectx, addedbefore)

>>   -            shelvedstate.clear(repo)

>> -            _finishunshelve(repo, oldtiprev, tr, activebookmark)

>> -            unshelvecleanup(ui, repo, basename, opts)

>> +        if obsshelve:

>> +            _obsoleteredundantnodes(repo, tr, pctx, shelvectx, tmpwctx)

>> +

>> +        shelvedstate.clear(repo)

>> +        _finishunshelve(repo, oldtiprev, tr, activebookmark, obsshelve)

>> +        unshelvecleanup(ui, repo, basename, opts)

>>       finally:

>>           if tr:

>>               tr.release()

>

> Again, I think that this would be clearer with more separated logic 

> rather than interspersing the two. It also seems that there are a lot 

> of not-obviously-related changes here, such as renaming overrides, 

> which make it more taxing to review.

See my reply to the previous patch for the logic separation. I do not 
think there's a lot of obsolescense/traditional branching in this 
function. Sorry about the overrides renaming, but I feel like wrapping 
smaller pieces of code is good.
> _______________________________________________

> Mercurial-devel mailing list

> Mercurial-devel@mercurial-scm.org

> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel