Patches item #1704547, was opened at 2007-04-20 14:48 Message generated for change (Comment added) made by jorend You can respond by visiting: https://sourceforge.net/tracker/?func=detail&atid=305470&aid=1704547&group_id=5470
Please note that this message will contain a full copy of the comment thread, including the initial issue submission, for this request, not just the latest update. Category: Core (C code) Group: Python 2.6 Status: Open Resolution: None Priority: 5 Private: No Submitted By: Raghuram Devarakonda (draghuram) Assigned to: Martin v. Löwis (loewis) Summary: Use MoveFileEx() to implement os.rename() on windows Initial Comment: This patch is to fix bug 1693753. Basically, os.rename(src, dst) fails on windows if dst already exists. This is because MoveFile() is used to implement rename. This patch replaces this API with MoveFileEx() and uses the flag MOVEFILE_REPLACE_EXISTING causing the API to replace the dst if it already exists. This brings the behaviour in line with unix. Note that, I also use the flag MOVEFILE_COPY_ALLOWED which is required if dst is on a different volume. However, moving to a different volume may not be atomic operation (I am not sure about this) as the msdn doc says that move in this case is simulated with CopyFile() and DeleteFile(). The patch also includes a test case and doc update. Please let me know if the location of test case and name of test function are ok. Also, MoveFileEx() is only available on NT+ windows but it should be ok as support for Win9x, WinME is removed in 2.6. ---------------------------------------------------------------------- Comment By: Jason Orendorff (jorend) Date: 2007-04-26 09:25 Message: Logged In: YES user_id=18139 Originator: NO loewis: I wish I could say no. I would prefer the new behavior. But the existing behavior is documented and has been there for a long time. People are surely relying on it in some Windows-only program somewhere. And man, when their code breaks, it'll break by unrecoverably clobbering files. :( Still -1, sorry. ---------------------------------------------------------------------- Comment By: Martin v. Löwis (loewis) Date: 2007-04-26 04:06 Message: Logged In: YES user_id=21627 Originator: NO AFAICT, MOVEFILE_COPY_ALLOWED is not needed to fix 1693753. If the objective of this patch is to establish consistency across platforms, then this should *not* be added, since os.rename won't rename across volumes on Unix, either. So I'm also -1 on the patch in this form. jorend: would you still object if it only did MOVEFILE_REPLACE_EXISTING? ---------------------------------------------------------------------- Comment By: Raghuram Devarakonda (draghuram) Date: 2007-04-20 16:56 Message: Logged In: YES user_id=984087 Originator: YES > This changes the documented behavior of a commonly used function. Right. If this change is considered too big for 2.6, may be it can be applied to 3.0? > FWIW: MoveFileEx() with MOVEFILE_COPY_ALLOWED isn't transactionally > isolated. Other processes can see the new file being created, and watch > its size increase, while the old one still exists. It isn't atomic, > either: in certain error cases, e.g. if the process's permission to write > the target file is suddenly revoked, it will fail after making changes to > the filesystem. True. But isn't this the case with MoveFile() too? I couldn't find any clear mention about transactional behaviour of either MoveFile() or MoveFileEx(). Same goes for atomicity. > Also-- it looks like the test leaves one of the temp files lying around! I can take care of that. While I think about it, it is perhaps not correct for this test function to be in Win32ErrorTests. ---------------------------------------------------------------------- Comment By: Jason Orendorff (jorend) Date: 2007-04-20 16:47 Message: Logged In: YES user_id=18139 Originator: NO -1. This changes the documented behavior of a commonly used function. FWIW: MoveFileEx() with MOVEFILE_COPY_ALLOWED isn't transactionally isolated. Other processes can see the new file being created, and watch its size increase, while the old one still exists. It isn't atomic, either: in certain error cases, e.g. if the process's permission to write the target file is suddenly revoked, it will fail after making changes to the filesystem. Also-- it looks like the test leaves one of the temp files lying around! ---------------------------------------------------------------------- You can respond by visiting: https://sourceforge.net/tracker/?func=detail&atid=305470&aid=1704547&group_id=5470 _______________________________________________ Patches mailing list Patches@python.org http://mail.python.org/mailman/listinfo/patches
