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

[Pierre-Yves David]

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.

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

-- 
Pierre-Yves David