[PATCH 2 of 2] util/rename: update to work better with virus scanners on Windows
Sune Foldager
cryo at cyanite.org
Tue Feb 17 08:35:25 UTC 2009
Matt Mackall wrote:
> You've really lost me here. Your code does:
> temp = tempfile.mktemp(dir=os.path.dirname(dst) or '.')
> os.rename(dst, temp)
> os.unlink(temp)
> os.rename(src, dst)
> My code does:
> temp = dst + "-force-rename"
> os.rename(dst, temp)
> os.unlink(temp)
> os.rename(src, dst)
>
> As far as I can see, the difference between my fix and yours (ignoring
> the extra bits you added) is I use a constant name and you use a random
> one.
Ah, apologies for that! I miss-read your patch (too early in the morning).
Yours is indeed simpler, and I also considered that solution myself. The
only problem would seem to be that a stale file lying around from a
previously failed operation would make the current and all future operations
fail as well. This is why I still think a somewhat random name (and the file
existence check mktemp does) is a good idea. Like this, any stale
"dirstate-force-rename" file could block all future repo operations.
>> If we could at least do it any other way than making pointless files,
even
>> if it's not the way I do it in my patch.
> You did read the comment for the existing code, right? We MUST make
> pointless files to work reliably, because Windows will allow us to
> successfully delete a file and STILL not allow us to rename to it.
Yes I did, but I was talking about an extra pointless file only created to
harvest its name. This is how the current code works because of its use of
mkstemp. This is not essential, and doesn't help secure anything either, as
there is a race condition between close/unlink and rename anyway, if one
wants to be paranoid. I read your patch as "create temp file. Close. Unlink.
Rename to its name + '-force-rename'". I am sorry for the confusion and the
critique addressing something you didn't propose. I am still a bit worried
about stale files with the admittedly very simple " + '-force-rename'"
solution.
--
Sune Foldager
More information about the Mercurial-devel
mailing list