[PATCH 7 of 7 shelve-ext v5] shelve: add some forwards compatibility to shelve operations

Ryan McElroy rm at fb.com
Thu Mar 30 05:31:28 EDT 2017


On 3/29/17 2:18 PM, Kostia Balytskyi wrote:
> # HG changeset patch
> # User Kostia Balytskyi <ikostia at fb.com>
> # Date 1490790691 25200
> #      Wed Mar 29 05:31:31 2017 -0700
> # Node ID 0953ca0a71a0e1f05078b0f3c255459e1ba56a4e
> # Parent  9ce0a366a6316d1cffc2a875e7dc6b4a3227c851
> shelve: add some forwards compatibility to shelve operations
>
> This patch handles cases where someone enabled an obs-based
> shelve in their repo, then disabled it while having made some
> shelves. Approach of this patch is described below:
> - assume this is what .hg/shelved looks like:
>    - sh1.patch (created 1s ago)
>    - sh1.oshelve (created 1s ago)
Would it make sense to have a "compatibility mode" where we also create 
the bundle, so that unshelving is actually backwards-compatible (if this 
option is enabled)?

>    - sh2.patch (created 2s ago)
>    - sh2.hg (created 2s ago)
> - when obs-based shelve is enabled, 'hg shelve --list' shows both
>    sh1 and s2 and is able to unshelve both. 'hg unshelbe' (without --name)

s/unshelbe/unshelve

>    will unshelve sh1.
> - when obs-based shelve is disabled, 'hg shelve --list' only shows
>    sh2, prints a warning that it found an obs-based shelve and is
>    only able to unshelve a traditional shelve. 'hg unshelve'
>    (without --name) will unshelve sh2.
>
> diff --git a/hgext/shelve.py b/hgext/shelve.py
> --- a/hgext/shelve.py
> +++ b/hgext/shelve.py
> @@ -66,9 +66,15 @@ testedwith = 'ships-with-hg-core'
>   
>   backupdir = 'shelve-backup'
>   shelvedir = 'shelved'
> -shelvefileextensions = ['hg', 'patch', 'oshelve']
>   # universal extension is present in all types of shelves
>   patchextension = 'patch'
> +# extension used for bundle-based traditional shelves
> +traditionalextension = 'hg'
> +# extension used for obs-based shelves
> +obsshelveextension = 'oshelve'
> +shelvefileextensions = [traditionalextension,
> +                        patchextension,
> +                        obsshelveextension]
>   
>   # we never need the user, so we use a
>   # generic user for all shelve operations
> @@ -530,7 +536,12 @@ def deletecmd(ui, repo, pats):
>               raise error.Abort(_("shelved change '%s' not found") % name)
>   
>   def listshelves(repo):
> -    """return all shelves in repo as list of (time, filename)"""
> +    """return a tuple of (allshelves, obsshelvespresent)
> +    where
> +    'allshelves' is a list of (time, filename) for shelves available
> +                in current repo configuration
> +    'obsshelvespresent' is a bool indicating whether repo has
> +                        any obs-based shelves"""
>       try:
>           names = repo.vfs.readdir(shelvedir)
>       except OSError as err:
> @@ -538,13 +549,22 @@ def listshelves(repo):
>               raise
>           return []
>       info = []
> +    obsshelvedisabled = not isobsshelve(repo, repo.ui)
> +    obsshelvespresent = False
>       for (name, _type) in names:
>           pfx, sfx = name.rsplit('.', 1)
> +        if sfx == obsshelveextension:
> +            obsshelvespresent = True
>           if not pfx or sfx != patchextension:
>               continue
> +        traditionalfpath = repo.vfs.join(shelvedir,
> +                                         pfx + '.' + traditionalextension)
> +        if obsshelvedisabled and not repo.vfs.exists(traditionalfpath):
> +            # this is not a traditional shelve
> +            continue
>           st = shelvedfile(repo, name).stat()
>           info.append((st.st_mtime, shelvedfile(repo, pfx).filename()))
> -    return sorted(info, reverse=True)
> +    return sorted(info, reverse=True), obsshelvespresent
>   
>   def listcmd(ui, repo, pats, opts):
>       """subcommand that displays the list of shelves"""
> @@ -554,7 +574,13 @@ def listcmd(ui, repo, pats, opts):
>           width = ui.termwidth()
>       namelabel = 'shelve.newest'
>       ui.pager('shelve')
> -    for mtime, name in listshelves(repo):
> +    availableshelves, obsshelvespresent = listshelves(repo)
> +    if (obsshelvespresent and not isobsshelve(repo, repo.ui) and
> +        ui.configbool('shelve', 'showobswarning', True)):
New configs need to be documented.

> +        ui.warn(_('warning: obsolescense-based shelve is disabled, but '
> +                  'obs-based shelve files are in the repo\n'
> +                  '(hint: set experimental.obsshelve=on in your hgrc)\n'))
> +    for mtime, name in availableshelves :
>           sname = util.split(name)[1]
>           if pats and sname not in pats:
>               continue
> @@ -952,7 +978,7 @@ def _dounshelve(ui, repo, *shelved, **op
>       elif len(shelved) > 1:
>           raise error.Abort(_('can only unshelve one change at a time'))
>       elif not shelved:
> -        shelved = listshelves(repo)
> +        shelved, __ = listshelves(repo)
>           if not shelved:
>               raise error.Abort(_('no shelved changes to apply!'))
>           basename = util.split(shelved[0][1])[1]
>

I like the idea of warning users in this case -- thanks for thinking 
about this case. I think that a backwards-compat mode might be a nice 
option to allow safely testing this feature before diving in with both 
feet. I know that would be a bit slower because of the additional 
bundle, but if its configurable people can choose whether to pay that cost.

Overall, this series direction looks good to me but there are enough 
nitpicks and important questions I'm going to mark as "changes 
requested" in patchwork.


More information about the Mercurial-devel mailing list