[Patches] [ python-Patches-1704547 ] Use MoveFileEx() to implement os.rename() on windows

[SourceForge.net]

Patches item #1704547, was opened at 2007-04-20 14:48
Message generated for change (Comment added) made by draghuram
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: Raghuram Devarakonda (draghuram)
Date: 2007-04-26 10:27

Message:
Logged In: YES 
user_id=984087
Originator: YES


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.



----------------------------------------------------------------------

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