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.

-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!

> 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.

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?

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.

I really wish one can reply to comments like in email, by quoting part of original mail. I don't know if the new tracker allows such option. Coming to the point,

1) MOVEFILE_COPY_ALLOWED is not needed to fix 1693753 but is needed if old functionality needs to be maintained. The API MoveFile() does move files across file systems (as per msdn doc).

2) As per os.rename doc, renaming across file systems *may* fail on some unix flavors. That gave me the impression that it *may* succeed on some others. If we can confirm that it *always* fails on *all* flavors, we can update the doc and I can do away with MOVEFILE_COPY_ALLOWED.

3) The patch breaks exiting code behaviour as jorend noted but not patching for that reason alone does not seem right to me, particularly since the patch adds desirable functionality. I don't think coming up with a new function name is a good idea. Is it ok to change the behaviour  of rename() based on some setting in "os" module? If nothing else, the patch can be applied to 3.0 and 2.x users can be given warnings.

draghuram, Thanks, now I understand about MOVEFILE_COPY_ALLOWED.

I don't think 3.0 is the answer--it's not the "subtle breakage" release.

Eventually someone will do for the filesystem what the subprocess module did for processes:  implement a *real* compatibility layer that behaves the same across all supported platforms.  Until that sunny day, I think we have to live with the quirks.  :-\

FWIW, I googled "python os.rename windows" only to find a (very) old discussion on python-dev about exact same topic.

http://mail.python.org/pipermail/python-dev/2001-May/014972.html

The idea of having optional parameter to os.rename() is interesting. What do you think?

draghuram, I think this should be discussed on python-dev. I'd be in favor of implementing subtle breakage, but I have been criticized for that view, also. I agree with jorend that if there is reason to reject it in 2.6, there is also reason to reject it forever.

Ok. I will post to python-dev about this. BTW, have either of you found any reference to MOVEFILE_REPLACE_EXISTING being atomic operation (for intra-file system moving of course)? I couldn't find any direct reference.

Raghu.

Hi Martin,

Following the python-dev discussion about the topic, I would suggest that this patch be closed along with the original bug 1693753. There doesn't seem to be a way of implementing atomic rename() on windows. 

http://mail.python.org/pipermail/python-dev/2007-April/072887.html
http://mail.python.org/pipermail/python-dev/2007-May/072892.html

Thanks,
Raghu