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

[Kostia Balytskyi]

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

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

[Ryan McElroy]

On 3/29/17 2:18 PM, Kostia Balytskyi wrote:

# HG changeset patch
# User Kostia Balytskyi 
# 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