[PATCH] shelve: always backup shelves instead of deleting them

[Colin Chan]

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 at ens-lyon.org>
>> wrote:
>>
>>>
>>>
>>> On 06/24/2015 12:16 PM, Colin Chan wrote:
>>>> # HG changeset patch
>>>> # User Colin Chan <colinchan at 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.