[PATCH 2 of 2] Reorder rename operations to minimise risk of leaving repository in unknown state

[Laurens Holst]

Hi Sune,

Op 3-10-2009 8:05, Sune Foldager schreef:
> Laurens Holst wrote:
>>          temp = tempname(dst)
>>          os.rename(dst, temp)
>> +        os.rename(src, dst)
>>          os.unlink(temp)
>> -        os.rename(src, dst)
>>
>
> Since I sorta wrote the code you're changing above and since Windows
> integrity has been pretty sensitive to this code in the various versions
> we tried in the past, I am bit worried about just changing it but... did
> you see if this actually helps anything?

I re-tried a couple of times, and wasn’t able to reproduce the issue, I 
think it only happens when the timing is just right… so a bit hard to 
verify. However Steve Borho said yesterday he is going to do a test to 
reproduce this through two interactive Python sessions.

> If it does, I would analyse it as follows: os.rename creates, as far as
> the stupid virus scanner is concerned, a new file, which is then opened
> for scanning. The file 'src', having not been created right this
> instant, might have been scanned in that past and already closed now, so
> the rename to dst can go through. Then the unlink might fail, of course.
>
> ...but I don't see how this code in any way prevents unlink from
> failing, which would in any case prevent the transaction from
> completing. But I guess you're mainly concerned about a corrupt dirstate
> file?

Indeed.

I think the core problem here is that in Windows, there is simply not a 
concept of an atomic rename to an existing file. So there is a 
workaround in place, but that workaround only works if files were opened 
with the flag FILE_SHARE_DELETE, something that is not guaranteed, not 
even done by default. When it’s not set, I don’t think there’s a way to 
do an atomic rename, all you can do is to try to write the code so that 
it fails in the least amount of cases.

I think that the results of this improvement will be:

1. file A locked before rename - dirstate intact
2. file A locked after rename - dirstate intact
3. file B locked before rename - dirstate not intact

Versus right now:

1. file A locked before rename - dirstate intact
2. file A locked after rename - dirstate not intact
3. file B locked before rename - dirstate not intact

~Laurens

-- 
~~ Ushiko-san! Kimi wa doushite, Ushiko-san nan da!! ~~
Laurens Holst, developer, Utrecht, the Netherlands
Website: www.grauw.nl. Backbase employee; www.backbase.com


-------------- next part --------------
A non-text attachment was scrubbed...
Name: smime.p7s
Type: application/pkcs7-signature
Size: 3274 bytes
Desc: S/MIME Cryptographic Signature
Url : http://selenic.com/pipermail/mercurial-devel/attachments/20091003/fc85be10/attachment.bin