[PATCH] shelve: always backup shelves instead of deleting them
durham at fb.com
Fri Jun 26 12:42:26 CDT 2015
On 6/26/15, 9:05 AM, "Colin Chan" <colinchan at 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 at ens-lyon.org>
>>>> 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
>>>>> saves backpus into .ht/strip-backup. The goal is to prevent data loss
>>>>> when using unshelve (e.g.,
>>>>> at the
>>>>> expense of using more disk space over time.
>>>> This changes seems strange to me, We should never delete a shelve
>>>> the whole unshelving process is done. It seems to be the current
>>>> 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
>> 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
>> 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.
More information about the Mercurial-devel