[PATCH 2 of 2] shelve: use backup functions instead of manually copying dirstate

[Mateusz Kwapich]

I don’t like the “crossing transaction scope boundary” too but the shelve extension was doing it anyway.
After this patch the code of aborttransaction is short and easy to understand. I definitely think that
shelve need more care in the future to do things in faster in less hacky way. 

On 5/25/16, 3:09 AM, "FUJIWARA Katsunori" <foozy at lares.dti.ne.jp> wrote:

>At Tue, 24 May 2016 14:15:52 -0700,
>Mateusz Kwapich wrote:
>> 
>> # HG changeset patch
>> # User Mateusz Kwapich <mitrandir at fb.com>
>> # Date 1464121793 25200
>> #      Tue May 24 13:29:53 2016 -0700
>> # Node ID 5f509e6681089b79eccf7890a4632ef45006eb6d
>> # Parent  0b235895f8304569f6b12722fe2c080a83261ce3
>> shelve: use backup functions instead of manually copying dirstate
>> 
>> This increases encapsulation of dirstate: the dirstate file is private
>> to the dirstate module and shouldn't be touched by extensions directly.
>
>Cleanup for this code path itself seems good (and thank you for taking
>care of my hacky work :-)).
>
>But I'm not sure whether this "crossing transaction scope boundary" of
>save/restore scope is reasonable.
>
>So, +0 for this patch (I don't' have any better idea)
>
>> diff --git a/hgext/shelve.py b/hgext/shelve.py
>> --- a/hgext/shelve.py
>> +++ b/hgext/shelve.py
>> @@ -226,28 +226,10 @@ def cleanupoldbackups(repo):
>>  def _aborttransaction(repo):
>>      '''Abort current transaction for shelve/unshelve, but keep dirstate
>>      '''
>> -    backupname = 'dirstate.shelve'
>> -    dirstatebackup = None
>> -    try:
>> -        # create backup of (un)shelved dirstate, because aborting transaction
>> -        # should restore dirstate to one at the beginning of the
>> -        # transaction, which doesn't include the result of (un)shelving
>> -        fp = repo.vfs.open(backupname, "w")
>> -        dirstatebackup = backupname
>> -        # clearing _dirty/_dirtypl of dirstate by _writedirstate below
>> -        # is unintentional. but it doesn't cause problem in this case,
>> -        # because no code path refers them until transaction is aborted.
>> -        repo.dirstate._writedirstate(fp) # write in-memory changes forcibly
>> -
>> -        tr = repo.currenttransaction()
>> -        tr.abort()
>> -
>> -        # restore to backuped dirstate
>> -        repo.vfs.rename(dirstatebackup, 'dirstate')
>> -        dirstatebackup = None
>> -    finally:
>> -        if dirstatebackup:
>> -            repo.vfs.unlink(dirstatebackup)
>> +    tr = repo.currenttransaction()
>> +    repo.dirstate.savebackup(tr, suffix='.shelve')
>> +    tr.abort()
>> +    repo.dirstate.restorebackup(None, suffix='.shelve')
>>  
>>  def createcmd(ui, repo, pats, opts):
>>      """subcommand that creates a new shelve"""
>> _______________________________________________
>> Mercurial-devel mailing list
>> Mercurial-devel at mercurial-scm.org
>> https://urldefense.proofpoint.com/v2/url?u=https-3A__www.mercurial-2Dscm.org_mailman_listinfo_mercurial-2Ddevel&d=CwIBAg&c=5VD0RTtNlTh3ycd41b3MUw&r=dK7q_6fOymlfdGMBe3wUaA&m=AHP8CvzUububpbZVXnRrrpatafr0vWBPL_2HFpU_4Ss&s=biEmGdF5Iso-R6aoPL_ybVUu0X3U-EbMV69Ujy1WJbU&e= 
>
>----------------------------------------------------------------------
>[FUJIWARA Katsunori]                             foozy at lares.dti.ne.jp