[PATCH 2 of 2] localrepo: eliminate os.unlink call in wwrite

Adrian Buehlmann adrian at cadifra.com
Sat Dec 4 09:26:55 CST 2010


On 2010-12-04 15:15, Mads Kiilerich wrote:
> Adrian Buehlmann wrote, On 12/04/2010 01:39 PM:
>> # HG changeset patch
>> # User Adrian Buehlmann<adrian at cadifra.com>
>> # Date 1291463926 -3600
>> # Node ID a68d37f33c943b32bae677c8ef117c4268d10f79
>> # Parent  d117e5fec89474724361f2618eddbae350ba29d1
>> localrepo: eliminate os.unlink call in wwrite
>>
>> The opener already unlinks the filename before 'w'riting, for both
>> the symlink and the normal file case.
>>
>> The effect of resetting the flags for normal files is now achieved
>> by setting the new resetflags parameter of the opener's __call__
>> function to True.
>>
>> Avoids tripping issue2524 on Windows (os.unlink followed by a write).
>>
>> diff --git a/mercurial/localrepo.py b/mercurial/localrepo.py
>> --- a/mercurial/localrepo.py
>> +++ b/mercurial/localrepo.py
>> @@ -629,14 +629,10 @@ class localrepository(repo.repository):
>>
>>       def wwrite(self, filename, data, flags):
>>           data = self._filter(self._decodefilterpats, filename, data)
>> -        try:
>> -            os.unlink(self.wjoin(filename))
>> -        except OSError:
>> -            pass
>>           if 'l' in flags:
>>               self.wopener.symlink(data, filename)
>>           else:
>> -            self.wopener(filename, 'w').write(data)
>> +            self.wopener(filename, 'w', resetflags=True).write(data)
>>               if 'x' in flags:
>>                   util.set_flags(self.wjoin(filename), False, True)
> 
> This will go in the opposite direction of what is requested in 
> http://mercurial.selenic.com/bts/issue2351 . That might be ok, but I 
> still think the "atomic write" patches on 
> http://selenic.com/pipermail/mercurial-devel/2010-July/thread.html#22772 
> would be nice. Could you perhaps test them on windows or give an updated 
> report of your current gut feeling?

No. I won't test them, sorry. That old thread is now completely moot.

util.opener has been refactored in 6ff784de7c3a (released with 1.7.1) by
myself after discussions with mpm on IRC to do an unlink on 'w'rite. Did
you miss that? *That* is the other direction. And the ship has already
sailed in that direction.

My gut feeling about the AV-scanner problems luckily has moved from guts
to brain recently, since I now understand what the problem is: os.unlink
on Windows.

See http://mercurial.selenic.com/wiki/UnlinkingFilesOnWindows for how it
works and http://mercurial.selenic.com/bts/issue2524 for what harm it
causes. (I've been throwing around these links quite a number of times
now on this list...)

All half-way decent AV-Scanners open files on Windows having set
FILE_SHARE_DELETE for dwShareMode on CreateFile (as does
mercurial.windows.posixfile, see osutil.c line 476).

http://msdn.microsoft.com/en-us/library/aa363858(VS.85).aspx

As described on UnlinkingFilesOnWindows, this blocks recreation of files
that have been sent into the dreaded "scheduled delete" state by the
sequence of posixfile(f) (what the scanner does) followed by
os.unlink(f) (what mercurial does).

> Note that another reason to unlink before writing is to break hardlinks

Nothing new to me. That's what the opener does and still does after
applying these patches here. I'm quite familiar with that, as
bf826c0b9537 demonstrates.

(Under what stone have you been, Mads? :).

> and getting rid of other meta data (ownership and extended attributes) 
> and avoid tripping on other processes reading the old file. At least on 
> "unix".


More information about the Mercurial-devel mailing list