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

login
register
mail settings
Submitter Kostia Balytskyi
Date March 29, 2017, 1:18 p.m.
Message ID <0953ca0a71a0e1f05078.1490793530@devvm1416.lla2.facebook.com>
Download mbox | patch
Permalink /patch/19817/
State Changes Requested
Headers show

Comments

Kostia Balytskyi - March 29, 2017, 1:18 p.m.
# HG changeset patch
# User Kostia Balytskyi <ikostia@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)
  - 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)
  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.
Ryan McElroy - March 30, 2017, 9:31 a.m.
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 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.
Kostia Balytskyi - March 30, 2017, 10:31 a.m.
On 30/03/2017 10:31, 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 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)?

We chatted in person and agreed on the following.
Despite my initial idea of making 'experimental.obsshelve' a clear line 
between Mercurial knowing about obs-shelves and not knowing about them, 
there seem to be no harm in letting list and unshelve know how to deal 
with obsshelves. This means that current patch can be dropped entirely 
and I will need to change a bit of unshelve's behavior. Effectively, it 
will work like this:
- I run 'hg shelve --config experimental.obsshelve=on --name myshelve', 
my changes are obs-shelved
- I run 'hg shelve --list --config experimental.obsshelve=off', 
'myshelve' is shown among other shelves
- I run 'hg unshelve --config experimental.obsshelve=off myshelve', and 
the operation is successful

I will modify shelve to behave like this in the next series unless 
someone objects bofore.
>

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

> _______________________________________________

> Mercurial-devel mailing list

> Mercurial-devel@mercurial-scm.org

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

Patch

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)):
+        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]