[PATCH RFC] check-code: add rule for os.remove/unlink and friends

[Idan Kamara]

On Tue, Oct 9, 2012 at 11:05 AM, Adrian Buehlmann <adrian at cadifra.com>
wrote:
>
> On 2012-10-08 16:26, Idan Kamara wrote:
> > # HG changeset patch
> > # User Idan Kamara <idankk86 at gmail.com>
> > # Date 1349706107 -7200
> > # Branch stable
> > # Node ID 0561e2d7dd3770ddcd2a0f27e009b767ddbea5f7
> > # Parent  6cd5ee6df3c463d2025cf17158c4ad8ca484d00f
> > check-code: add rule for os.remove/unlink and friends
> >
> > I seem to remember util.unlink/unlinkpaths were born because of issues
> > we had on Windows. Should we enforce this on the entire codebase?
>
> [..]
> > If so, I'll send a patch before this one that fixes all the errors.
>
> I have been thinking about this a bit.
>
> While there might be some value in reviewing all the file accesses, I
> currently have the impression that, for example, boldly enforcing
> util.unlink everywhere could be problematic or at least a bit pointless
> at best for some cases.

OK, given your analysis below for the required effort, it's a lot more
than the sed I was willing to do ;)

>
> If you look at the implementation for Windows of util.unlink
>
>   http://selenic.com/repo/hg/file/3c775c5a6c03/mercurial/win32.py#l344
>
> you will notice that it does a lot of things (given how minor the task
> it has to do basically is), and assumes a couple of other things.
>
> The whole machinery revolves mostly among the problem of trying to make
> it possible to delete a file that has been opened for reading.
>
> (See also http://mercurial.selenic.com/wiki/UnlinkingFilesOnWindows )
>
> The assumptions util.unlink makes are probably the most problematic
> here: the file must have been opened with the Windows API flag
> FILE_SHARE_DELETE, which is the case if the file has been opened using
> Mercurial's posixfile. If it has been opened using Python's open(), all
> bets are off anyway. However, if Mercurial happens to *not* have opened
> the file while deleting it, it doesn't matter anyway: The file can be
> deleted with Python's plain simple os.unlink(), _provided_ no other
> Process has it open at that time. Virus scanners are the usual culprits
> of bad behaviors here. "Good" virus scanners at least do open the files
> with FILE_SHARE_DELETE. The worst ones lock the file into a memory region.
>
> So before (somewhat) "blindly" going through the Mercurial codebase and
> replacing every os.unlink() call with util.unlink, we would perhaps also
> have to review a bit how Mercurial itself opens the respective file.
>
> The most problematic pattern for Windows is deleting a file that has
> been opened for reading *and* then recreating it right after that under
> the same name again. Mercurial does that perfectly on Windows when
> dealing with files in the working dir and inside .hg. But I don't know
> if it's done that perfect for every single code snippet we have in
> Mercurial's code base.
>
> Sorry for the long post. Please blame Windows for these stupidities (not
> me, thanks :-).

Thanks for the explanation nonetheless!
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.mercurial-scm.org/pipermail/mercurial-devel/attachments/20121009/407bf541/attachment-0002.html>