Patchwork shelve: always backup shelves instead of deleting them

login
register
mail settings
Submitter Colin Chan
Date June 24, 2015, 7:16 p.m.
Message ID <7a02a3b3941cb62203b6.1435173408@devvm264.prn1.facebook.com>
Download mbox | patch
Permalink /patch/9773/
State Superseded
Commit 8a6264a2ee60c615e78dd5ad0e414cec9ff8c5bc
Headers show

Comments

Colin Chan - June 24, 2015, 7:16 p.m.
# HG changeset patch
# User Colin Chan <colinchan@fb.com>
# Date 1434989752 25200
#      Mon Jun 22 09:15:52 2015 -0700
# Node ID 7a02a3b3941cb62203b6efc145e58c543e8b8909
# Parent  7fdd1782fc4ee9da87d8af13e806dc9055db2c38
shelve: always backup shelves instead of deleting them

Instead of being deleted, shelve files are now moved into the
.hg/shelve-backup directory. This is designed to be very similar to how strip
saves backpus into .ht/strip-backup. The goal is to prevent data loss especially
when using unshelve (e.g., http://bz.selenic.com/show_bug.cgi?id=4732) at the
expense of using more disk space over time.
Pierre-Yves David - June 24, 2015, 10:47 p.m.
On 06/24/2015 12:16 PM, Colin Chan wrote:
> # HG changeset patch
> # User Colin Chan <colinchan@fb.com>
> # Date 1434989752 25200
> #      Mon Jun 22 09:15:52 2015 -0700
> # Node ID 7a02a3b3941cb62203b6efc145e58c543e8b8909
> # Parent  7fdd1782fc4ee9da87d8af13e806dc9055db2c38
> shelve: always backup shelves instead of deleting them
>
> Instead of being deleted, shelve files are now moved into the
> .hg/shelve-backup directory. This is designed to be very similar to how strip
> saves backpus into .ht/strip-backup. The goal is to prevent data loss especially
> when using unshelve (e.g., http://bz.selenic.com/show_bug.cgi?id=4732) at the
> expense of using more disk space over time.

This changes seems strange to me, We should never delete a shelve until 
the whole unshelving process is done. It seems to be the current intend 
of the code, if it happen to currently not be the case, it should be 
easy to fix.

Especially, this change seems unrelated to issue4732 and I fail to see 
how it improve the situation three


>
> diff --git a/hgext/shelve.py b/hgext/shelve.py
> --- a/hgext/shelve.py
> +++ b/hgext/shelve.py
> @@ -22,6 +22,7 @@
>   """
>
>   import collections
> +import itertools
>   from mercurial.i18n import _
>   from mercurial.node import nullid, nullrev, bin, hex
>   from mercurial import changegroup, cmdutil, scmutil, phases, commands
> @@ -48,6 +49,7 @@
>           self.repo = repo
>           self.name = name
>           self.vfs = scmutil.vfs(repo.join('shelved'))
> +        self.backupvfs = scmutil.vfs(repo.join('shelve-backup'))
>           self.ui = self.repo.ui
>           if filetype:
>               self.fname = name + '.' + filetype
> @@ -60,8 +62,21 @@
>       def filename(self):
>           return self.vfs.join(self.fname)
>
> -    def unlink(self):
> -        util.unlink(self.filename())
> +    def backupfilename(self):

This seems to be internal method only, consider renaming them with a 
leading _ to highlight that.

> +        def gennames(base):
> +            yield base
> +            for i in itertools.count(1):
> +                yield '%s-%d' % (base, i)
> +
> +        name = self.backupvfs.join(self.fname)
> +        for n in gennames(name):
> +            if not self.backupvfs.exists(n):
> +                return n
> +
> +    def movetobackup(self):
> +        if not self.backupvfs.isdir():
> +            self.backupvfs.makedir()
> +        util.rename(self.filename(), self.backupfilename())
>
>       def stat(self):
>           return self.vfs.stat(self.fname)
> @@ -281,7 +296,7 @@
>           for (name, _type) in repo.vfs.readdir('shelved'):
>               suffix = name.rsplit('.', 1)[-1]
>               if suffix in ('hg', 'patch'):
> -                shelvedfile(repo, name).unlink()
> +                shelvedfile(repo, name).movetobackup()
>       finally:
>           lockmod.release(wlock)
>
> @@ -293,7 +308,7 @@
>       try:
>           for name in pats:
>               for suffix in 'hg patch'.split():
> -                shelvedfile(repo, name, suffix).unlink()
> +                shelvedfile(repo, name, suffix).movetobackup()
>       except OSError, err:
>           if err.errno != errno.ENOENT:
>               raise
> @@ -442,7 +457,7 @@
>       """remove related files after an unshelve"""
>       if not opts['keep']:
>           for filetype in 'hg patch'.split():
> -            shelvedfile(repo, name, filetype).unlink()
> +            shelvedfile(repo, name, filetype).movetobackup()
>
>   def unshelvecontinue(ui, repo, state, opts):
>       """subcommand to continue an in-progress unshelve"""
> @@ -505,18 +520,19 @@
>       restore. If none is given, the most recent shelved change is used.
>
>       If a shelved change is applied successfully, the bundle that
> -    contains the shelved changes is deleted afterwards.
> +    contains the shelved changes is moved to a backup location
> +    (.hg/shelve-backup).
>
>       Since you can restore a shelved change on top of an arbitrary
>       commit, it is possible that unshelving will result in a conflict
>       between your changes and the commits you are unshelving onto. If
>       this occurs, you must resolve the conflict, then use
>       ``--continue`` to complete the unshelve operation. (The bundle
> -    will not be deleted until you successfully complete the unshelve.)
> +    will not be moved until you successfully complete the unshelve.)
>
>       (Alternatively, you can use ``--abort`` to abandon an unshelve
>       that causes a conflict. This reverts the unshelved changes, and
> -    does not delete the bundle.)
> +    leaves the bundle in place.)
>       """
>       abortf = opts['abort']
>       continuef = opts['continue']
> diff --git a/tests/test-shelve.t b/tests/test-shelve.t
> --- a/tests/test-shelve.t
> +++ b/tests/test-shelve.t
> @@ -85,6 +85,12 @@
>     nothing changed
>     [1]
>
> +make sure shelve files were backed up
> +
> +  $ ls .hg/shelve-backup
> +  default.hg
> +  default.patch
> +
>   create an mq patch - shelving should work fine with a patch applied
>
>     $ echo n > n
> @@ -154,6 +160,14 @@
>     $ hg shelve -d default
>     $ hg qfinish -a -q
>
> +ensure shelve backups aren't overwritten
> +
> +  $ ls .hg/shelve-backup/
> +  default.hg
> +  default.hg-1
> +  default.patch
> +  default.patch-1
> +
>   local edits should not prevent a shelved change from applying
>
>     $ printf "z\na\n" > a/a
Durham Goode - June 24, 2015, 10:54 p.m.
On 6/24/15, 3:47 PM, "Pierre-Yves David" <pierre-yves.david@ens-lyon.org>
wrote:

>
>
>On 06/24/2015 12:16 PM, Colin Chan wrote:
>> # HG changeset patch
>> # User Colin Chan <colinchan@fb.com>
>> # Date 1434989752 25200
>> #      Mon Jun 22 09:15:52 2015 -0700
>> # Node ID 7a02a3b3941cb62203b6efc145e58c543e8b8909
>> # Parent  7fdd1782fc4ee9da87d8af13e806dc9055db2c38
>> shelve: always backup shelves instead of deleting them
>>
>> Instead of being deleted, shelve files are now moved into the
>> .hg/shelve-backup directory. This is designed to be very similar to how
>>strip
>> saves backpus into .ht/strip-backup. The goal is to prevent data loss
>>especially
>> when using unshelve (e.g., http://bz.selenic.com/show_bug.cgi?id=4732)
>>at the
>> expense of using more disk space over time.
>
>This changes seems strange to me, We should never delete a shelve until
>the whole unshelving process is done. It seems to be the current intend
>of the code, if it happen to currently not be the case, it should be
>easy to fix.

We have had users do dumb things, where they do an unshelve, then get
confused (or mercurial does something weird and unexpected) and they
unintentionally overwrite their working copy.  Shelve gives us an
opportunity to store a checkpoint of their work permanently, so I think we
should do this.

>
>Especially, this change seems unrelated to issue4732 and I fail to see
>how it improve the situation three

This is related because it would have prevented data loss.  It would also
prevent data loss in all the other bizarre shelve/unshelve bugs that
exists that maybe we don't know about.  Sure we could fix shelve, and we
should, but that's a lot more effort, and at least this patch will save
some user tears in the mean time.
Pierre-Yves David - June 26, 2015, 10:14 a.m.
On 06/24/2015 03:54 PM, Durham Goode wrote:
>
>
> On 6/24/15, 3:47 PM, "Pierre-Yves David" <pierre-yves.david@ens-lyon.org>
> wrote:
>
>>
>>
>> On 06/24/2015 12:16 PM, Colin Chan wrote:
>>> # HG changeset patch
>>> # User Colin Chan <colinchan@fb.com>
>>> # Date 1434989752 25200
>>> #      Mon Jun 22 09:15:52 2015 -0700
>>> # Node ID 7a02a3b3941cb62203b6efc145e58c543e8b8909
>>> # Parent  7fdd1782fc4ee9da87d8af13e806dc9055db2c38
>>> shelve: always backup shelves instead of deleting them
>>>
>>> Instead of being deleted, shelve files are now moved into the
>>> .hg/shelve-backup directory. This is designed to be very similar to how
>>> strip
>>> saves backpus into .ht/strip-backup. The goal is to prevent data loss
>>> especially
>>> when using unshelve (e.g., http://bz.selenic.com/show_bug.cgi?id=4732)
>>> at the
>>> expense of using more disk space over time.
>>
>> This changes seems strange to me, We should never delete a shelve until
>> the whole unshelving process is done. It seems to be the current intend
>> of the code, if it happen to currently not be the case, it should be
>> easy to fix.
>
> We have had users do dumb things, where they do an unshelve, then get
> confused (or mercurial does something weird and unexpected) and they
> unintentionally overwrite their working copy.  Shelve gives us an
> opportunity to store a checkpoint of their work permanently, so I think we
> should do this.

Shelve is expected to be a very light weight operation. The shelve 
bundle can contains a lot of heavy data (because it contains any draft 
changesets under the wdir when we shelve). I do not think we can 
reasonably backup all this by default in all cases.

We could maybe hide this under a config flags if you think this is 
necessary.

Also, having more details on the pattern that get user to loose changes 
would be good. We do not want bumb users to be exposed to such things.

>> Especially, this change seems unrelated to issue4732 and I fail to see
>> how it improve the situation three
>
> This is related because it would have prevented data loss.

No, it would not. The way I read issue4732 is:

If there is uncommited change during unshelve:

1) We open a transaction
2) we commit them
3) we apply the shelve bundle
4) we rebase the shelve bundle ontop of the local change
5) we update back the original place
6) we revert to the result of the rebase
7) we rollback the transaction

issue4732 is that if we abort anytime between 2 and 6, we rollback the 
temporary commit and do not restore the file content. Leading to data lose.

The local uncommitted change are not in the shelve data so backing it up 
will not restore it.

As this issue (4732) lead to unrecoverable data lose, I've upgraded it 
to critical and we should aim at getting it fixed quickly.
Colin Chan - June 26, 2015, 4:05 p.m.
On 06/26/2015 03:14 AM, Pierre-Yves David wrote:
>
>
> On 06/24/2015 03:54 PM, Durham Goode wrote:
>>
>>
>> On 6/24/15, 3:47 PM, "Pierre-Yves David" <pierre-yves.david@ens-lyon.org>
>> wrote:
>>
>>>
>>>
>>> On 06/24/2015 12:16 PM, Colin Chan wrote:
>>>> # HG changeset patch
>>>> # User Colin Chan <colinchan@fb.com>
>>>> # Date 1434989752 25200
>>>> #      Mon Jun 22 09:15:52 2015 -0700
>>>> # Node ID 7a02a3b3941cb62203b6efc145e58c543e8b8909
>>>> # Parent  7fdd1782fc4ee9da87d8af13e806dc9055db2c38
>>>> shelve: always backup shelves instead of deleting them
>>>>
>>>> Instead of being deleted, shelve files are now moved into the
>>>> .hg/shelve-backup directory. This is designed to be very similar to how
>>>> strip
>>>> saves backpus into .ht/strip-backup. The goal is to prevent data loss
>>>> especially
>>>> when using unshelve (e.g., http://bz.selenic.com/show_bug.cgi?id=4732)
>>>> at the
>>>> expense of using more disk space over time.
>>>
>>> This changes seems strange to me, We should never delete a shelve until
>>> the whole unshelving process is done. It seems to be the current intend
>>> of the code, if it happen to currently not be the case, it should be
>>> easy to fix.
>>
>> We have had users do dumb things, where they do an unshelve, then get
>> confused (or mercurial does something weird and unexpected) and they
>> unintentionally overwrite their working copy.  Shelve gives us an
>> opportunity to store a checkpoint of their work permanently, so I
>> think we
>> should do this.
>
> Shelve is expected to be a very light weight operation. The shelve
> bundle can contains a lot of heavy data (because it contains any draft
> changesets under the wdir when we shelve). I do not think we can
> reasonably backup all this by default in all cases.
>
> We could maybe hide this under a config flags if you think this is
> necessary.
>
> Also, having more details on the pattern that get user to loose changes
> would be good. We do not want bumb users to be exposed to such things.
>

One case where this can happen is if a user is unshelving something and 
accidentally removes their changes when resolving merge conflicts.  The 
unshelve still completes, but the merge conflicts were resolved 
incorrectly, and so the shelved changes are lost.  Backing up the shelve 
data gives the user the ability to try the unshelve again (after 
manually retrieving the data from the backup directory).

Adding a config flag to control whether backups are made sounds like a 
good compromise to me.  I will look into doing that.

>>> Especially, this change seems unrelated to issue4732 and I fail to see
>>> how it improve the situation three
>>
>> This is related because it would have prevented data loss.
>
> No, it would not. The way I read issue4732 is:
>
> If there is uncommited change during unshelve:
>
> 1) We open a transaction
> 2) we commit them
> 3) we apply the shelve bundle
> 4) we rebase the shelve bundle ontop of the local change
> 5) we update back the original place
> 6) we revert to the result of the rebase
> 7) we rollback the transaction
>
> issue4732 is that if we abort anytime between 2 and 6, we rollback the
> temporary commit and do not restore the file content. Leading to data lose.
>
> The local uncommitted change are not in the shelve data so backing it up
> will not restore it.
>
> As this issue (4732) lead to unrecoverable data lose, I've upgraded it
> to critical and we should aim at getting it fixed quickly.
>

You are right that the patch does not address this issue; that was a 
mistake.  The first time I read the issue I misunderstood it.
Durham Goode - June 26, 2015, 5:42 p.m.
On 6/26/15, 9:05 AM, "Colin Chan" <colinchan@fb.com> wrote:

>
>On 06/26/2015 03:14 AM, Pierre-Yves David wrote:
>>
>>
>> On 06/24/2015 03:54 PM, Durham Goode wrote:
>>>
>>>
>>> On 6/24/15, 3:47 PM, "Pierre-Yves David"
>>><pierre-yves.david@ens-lyon.org>
>>> wrote:
>>>
>>>>
>>>>
>>>> On 06/24/2015 12:16 PM, Colin Chan wrote:
>>>>> # HG changeset patch
>>>>> # User Colin Chan <colinchan@fb.com>
>>>>> # Date 1434989752 25200
>>>>> #      Mon Jun 22 09:15:52 2015 -0700
>>>>> # Node ID 7a02a3b3941cb62203b6efc145e58c543e8b8909
>>>>> # Parent  7fdd1782fc4ee9da87d8af13e806dc9055db2c38
>>>>> shelve: always backup shelves instead of deleting them
>>>>>
>>>>> Instead of being deleted, shelve files are now moved into the
>>>>> .hg/shelve-backup directory. This is designed to be very similar to
>>>>>how
>>>>> strip
>>>>> saves backpus into .ht/strip-backup. The goal is to prevent data loss
>>>>> especially
>>>>> when using unshelve (e.g.,
>>>>>http://bz.selenic.com/show_bug.cgi?id=4732)
>>>>> at the
>>>>> expense of using more disk space over time.
>>>>
>>>> This changes seems strange to me, We should never delete a shelve
>>>>until
>>>> the whole unshelving process is done. It seems to be the current
>>>>intend
>>>> of the code, if it happen to currently not be the case, it should be
>>>> easy to fix.
>>>
>>> We have had users do dumb things, where they do an unshelve, then get
>>> confused (or mercurial does something weird and unexpected) and they
>>> unintentionally overwrite their working copy.  Shelve gives us an
>>> opportunity to store a checkpoint of their work permanently, so I
>>> think we
>>> should do this.
>>
>> Shelve is expected to be a very light weight operation. The shelve
>> bundle can contains a lot of heavy data (because it contains any draft
>> changesets under the wdir when we shelve). I do not think we can
>> reasonably backup all this by default in all cases.

We already do store every draft commit anyone ever made, either in the
repo, or in the strip-backups. This shelve-backup seems like it would be
significantly smaller compared to that.

>>
>> We could maybe hide this under a config flags if you think this is
>> necessary.
>>
>> Also, having more details on the pattern that get user to loose changes
>> would be good. We do not want bumb users to be exposed to such things.
>>
>
>One case where this can happen is if a user is unshelving something and
>accidentally removes their changes when resolving merge conflicts.  The
>unshelve still completes, but the merge conflicts were resolved
>incorrectly, and so the shelved changes are lost.  Backing up the shelve
>data gives the user the ability to try the unshelve again (after
>manually retrieving the data from the backup directory).
>
>Adding a config flag to control whether backups are made sounds like a
>good compromise to me.  I will look into doing that.
>
>>>> Especially, this change seems unrelated to issue4732 and I fail to see
>>>> how it improve the situation three
>>>
>>> This is related because it would have prevented data loss.
>>
>> No, it would not. The way I read issue4732 is:
>>
>> If there is uncommited change during unshelve:
>>
>> 1) We open a transaction
>> 2) we commit them
>> 3) we apply the shelve bundle
>> 4) we rebase the shelve bundle ontop of the local change
>> 5) we update back the original place
>> 6) we revert to the result of the rebase
>> 7) we rollback the transaction
>>
>> issue4732 is that if we abort anytime between 2 and 6, we rollback the
>> temporary commit and do not restore the file content. Leading to data
>>lose.
>>
>> The local uncommitted change are not in the shelve data so backing it up
>> will not restore it.
>>
>> As this issue (4732) lead to unrecoverable data lose, I've upgraded it
>> to critical and we should aim at getting it fixed quickly.
>>
>
>You are right that the patch does not address this issue; that was a
>mistake.  The first time I read the issue I misunderstood it.

The user scenario which brought that issue to light was a person
accidentally doing unshelve twice, and ctrl+c'ing the second one, so they
lost the first unshelve's changes.  So it's semi related, but you're
right, not completely.
Matt Mackall - June 26, 2015, 11:53 p.m.
On Fri, 2015-06-26 at 17:42 +0000, Durham Goode wrote:
> 
> On 6/26/15, 9:05 AM, "Colin Chan" <colinchan@fb.com> wrote:
> 
> >
> >On 06/26/2015 03:14 AM, Pierre-Yves David wrote:
> >>
> >>
> >> On 06/24/2015 03:54 PM, Durham Goode wrote:
> >>>
> >>>
> >>> On 6/24/15, 3:47 PM, "Pierre-Yves David"
> >>><pierre-yves.david@ens-lyon.org>
> >>> wrote:
> >>>
> >>>>
> >>>>
> >>>> On 06/24/2015 12:16 PM, Colin Chan wrote:
> >>>>> # HG changeset patch
> >>>>> # User Colin Chan <colinchan@fb.com>
> >>>>> # Date 1434989752 25200
> >>>>> #      Mon Jun 22 09:15:52 2015 -0700
> >>>>> # Node ID 7a02a3b3941cb62203b6efc145e58c543e8b8909
> >>>>> # Parent  7fdd1782fc4ee9da87d8af13e806dc9055db2c38
> >>>>> shelve: always backup shelves instead of deleting them
> >>>>>
> >>>>> Instead of being deleted, shelve files are now moved into the
> >>>>> .hg/shelve-backup directory. This is designed to be very similar to
> >>>>>how
> >>>>> strip
> >>>>> saves backpus into .ht/strip-backup. The goal is to prevent data loss
> >>>>> especially
> >>>>> when using unshelve (e.g.,
> >>>>>http://bz.selenic.com/show_bug.cgi?id=4732)
> >>>>> at the
> >>>>> expense of using more disk space over time.
> >>>>
> >>>> This changes seems strange to me, We should never delete a shelve
> >>>>until
> >>>> the whole unshelving process is done. It seems to be the current
> >>>>intend
> >>>> of the code, if it happen to currently not be the case, it should be
> >>>> easy to fix.
> >>>
> >>> We have had users do dumb things, where they do an unshelve, then get
> >>> confused (or mercurial does something weird and unexpected) and they
> >>> unintentionally overwrite their working copy.  Shelve gives us an
> >>> opportunity to store a checkpoint of their work permanently, so I
> >>> think we
> >>> should do this.
> >>
> >> Shelve is expected to be a very light weight operation. The shelve
> >> bundle can contains a lot of heavy data (because it contains any draft
> >> changesets under the wdir when we shelve). I do not think we can
> >> reasonably backup all this by default in all cases.
> 
> We already do store every draft commit anyone ever made, either in the
> repo, or in the strip-backups. This shelve-backup seems like it would be
> significantly smaller compared to that.

That's not always true. We recently had a user complain that a single
shelve was over a gigabyte. That was because he had a local-only repo
that was all draft (because he never pushed it anywhere). He didn't
necessarily use any history editing.

If we had bundlepatches rather than our current trick, they could be a
lot lighter.
Durham Goode - June 27, 2015, 12:46 a.m.
On 6/26/15, 4:53 PM, "Matt Mackall" <mpm@selenic.com> wrote:

>On Fri, 2015-06-26 at 17:42 +0000, Durham Goode wrote:
>> 
>> On 6/26/15, 9:05 AM, "Colin Chan" <colinchan@fb.com> wrote:
>> 
>> >
>> >On 06/26/2015 03:14 AM, Pierre-Yves David wrote:
>> >>
>> >>
>> >> On 06/24/2015 03:54 PM, Durham Goode wrote:
>> >>>
>> >>>
>> >>> On 6/24/15, 3:47 PM, "Pierre-Yves David"
>> >>><pierre-yves.david@ens-lyon.org>
>> >>> wrote:
>> >>>
>> >>>>
>> >>>>
>> >>>> On 06/24/2015 12:16 PM, Colin Chan wrote:
>> >>>>> # HG changeset patch
>> >>>>> # User Colin Chan <colinchan@fb.com>
>> >>>>> # Date 1434989752 25200
>> >>>>> #      Mon Jun 22 09:15:52 2015 -0700
>> >>>>> # Node ID 7a02a3b3941cb62203b6efc145e58c543e8b8909
>> >>>>> # Parent  7fdd1782fc4ee9da87d8af13e806dc9055db2c38
>> >>>>> shelve: always backup shelves instead of deleting them
>> >>>>>
>> >>>>> Instead of being deleted, shelve files are now moved into the
>> >>>>> .hg/shelve-backup directory. This is designed to be very similar
>>to
>> >>>>>how
>> >>>>> strip
>> >>>>> saves backpus into .ht/strip-backup. The goal is to prevent data
>>loss
>> >>>>> especially
>> >>>>> when using unshelve (e.g.,
>> >>>>>http://bz.selenic.com/show_bug.cgi?id=4732)
>> >>>>> at the
>> >>>>> expense of using more disk space over time.
>> >>>>
>> >>>> This changes seems strange to me, We should never delete a shelve
>> >>>>until
>> >>>> the whole unshelving process is done. It seems to be the current
>> >>>>intend
>> >>>> of the code, if it happen to currently not be the case, it should
>>be
>> >>>> easy to fix.
>> >>>
>> >>> We have had users do dumb things, where they do an unshelve, then
>>get
>> >>> confused (or mercurial does something weird and unexpected) and they
>> >>> unintentionally overwrite their working copy.  Shelve gives us an
>> >>> opportunity to store a checkpoint of their work permanently, so I
>> >>> think we
>> >>> should do this.
>> >>
>> >> Shelve is expected to be a very light weight operation. The shelve
>> >> bundle can contains a lot of heavy data (because it contains any
>>draft
>> >> changesets under the wdir when we shelve). I do not think we can
>> >> reasonably backup all this by default in all cases.
>> 
>> We already do store every draft commit anyone ever made, either in the
>> repo, or in the strip-backups. This shelve-backup seems like it would be
>> significantly smaller compared to that.
>
>That's not always true. We recently had a user complain that a single
>shelve was over a gigabyte. That was because he had a local-only repo
>that was all draft (because he never pushed it anywhere). He didn't
>necessarily use any history editing.
>
>If we had bundlepatches rather than our current trick, they could be a
>lot lighter.

What if we only kept the backup shelves around for a limited time?  Like,
every time we do a shelve/unshelve we delete any shelve-backups older than
a month.  That provides the recoverability behavior, without the permanent
growth.
Matt Mackall - June 29, 2015, 10:41 p.m.
> What if we only kept the backup shelves around for a limited time?  Like,
> every time we do a shelve/unshelve we delete any shelve-backups older than
> a month.  That provides the recoverability behavior, without the permanent
> growth.

I think a last-N strategy is a better fit.

Patch

diff --git a/hgext/shelve.py b/hgext/shelve.py
--- a/hgext/shelve.py
+++ b/hgext/shelve.py
@@ -22,6 +22,7 @@ 
 """
 
 import collections
+import itertools
 from mercurial.i18n import _
 from mercurial.node import nullid, nullrev, bin, hex
 from mercurial import changegroup, cmdutil, scmutil, phases, commands
@@ -48,6 +49,7 @@ 
         self.repo = repo
         self.name = name
         self.vfs = scmutil.vfs(repo.join('shelved'))
+        self.backupvfs = scmutil.vfs(repo.join('shelve-backup'))
         self.ui = self.repo.ui
         if filetype:
             self.fname = name + '.' + filetype
@@ -60,8 +62,21 @@ 
     def filename(self):
         return self.vfs.join(self.fname)
 
-    def unlink(self):
-        util.unlink(self.filename())
+    def backupfilename(self):
+        def gennames(base):
+            yield base
+            for i in itertools.count(1):
+                yield '%s-%d' % (base, i)
+
+        name = self.backupvfs.join(self.fname)
+        for n in gennames(name):
+            if not self.backupvfs.exists(n):
+                return n
+
+    def movetobackup(self):
+        if not self.backupvfs.isdir():
+            self.backupvfs.makedir()
+        util.rename(self.filename(), self.backupfilename())
 
     def stat(self):
         return self.vfs.stat(self.fname)
@@ -281,7 +296,7 @@ 
         for (name, _type) in repo.vfs.readdir('shelved'):
             suffix = name.rsplit('.', 1)[-1]
             if suffix in ('hg', 'patch'):
-                shelvedfile(repo, name).unlink()
+                shelvedfile(repo, name).movetobackup()
     finally:
         lockmod.release(wlock)
 
@@ -293,7 +308,7 @@ 
     try:
         for name in pats:
             for suffix in 'hg patch'.split():
-                shelvedfile(repo, name, suffix).unlink()
+                shelvedfile(repo, name, suffix).movetobackup()
     except OSError, err:
         if err.errno != errno.ENOENT:
             raise
@@ -442,7 +457,7 @@ 
     """remove related files after an unshelve"""
     if not opts['keep']:
         for filetype in 'hg patch'.split():
-            shelvedfile(repo, name, filetype).unlink()
+            shelvedfile(repo, name, filetype).movetobackup()
 
 def unshelvecontinue(ui, repo, state, opts):
     """subcommand to continue an in-progress unshelve"""
@@ -505,18 +520,19 @@ 
     restore. If none is given, the most recent shelved change is used.
 
     If a shelved change is applied successfully, the bundle that
-    contains the shelved changes is deleted afterwards.
+    contains the shelved changes is moved to a backup location
+    (.hg/shelve-backup).
 
     Since you can restore a shelved change on top of an arbitrary
     commit, it is possible that unshelving will result in a conflict
     between your changes and the commits you are unshelving onto. If
     this occurs, you must resolve the conflict, then use
     ``--continue`` to complete the unshelve operation. (The bundle
-    will not be deleted until you successfully complete the unshelve.)
+    will not be moved until you successfully complete the unshelve.)
 
     (Alternatively, you can use ``--abort`` to abandon an unshelve
     that causes a conflict. This reverts the unshelved changes, and
-    does not delete the bundle.)
+    leaves the bundle in place.)
     """
     abortf = opts['abort']
     continuef = opts['continue']
diff --git a/tests/test-shelve.t b/tests/test-shelve.t
--- a/tests/test-shelve.t
+++ b/tests/test-shelve.t
@@ -85,6 +85,12 @@ 
   nothing changed
   [1]
 
+make sure shelve files were backed up
+
+  $ ls .hg/shelve-backup
+  default.hg
+  default.patch
+
 create an mq patch - shelving should work fine with a patch applied
 
   $ echo n > n
@@ -154,6 +160,14 @@ 
   $ hg shelve -d default
   $ hg qfinish -a -q
 
+ensure shelve backups aren't overwritten
+
+  $ ls .hg/shelve-backup/
+  default.hg
+  default.hg-1
+  default.patch
+  default.patch-1
+
 local edits should not prevent a shelved change from applying
 
   $ printf "z\na\n" > a/a