[Python-ideas] Implementation of shutil.move

[Steven D'Aprano]

Masklinn wrote:
> On 13 août 2011, at 19:08, MRAB <python at mrabarnett.plus.com> wrote:
>> On 13/08/2011 15:59, David Townshend wrote:
>>> There was a suggestion on the issue tracker that it might be better to
>>> add an optional argument to shutil.move rather than create a new
>>> function. Does anyone have any comments or suggestions about this?
>>>
>> [For reference, it's issue 12741 ("Add function similar to shutil.move
>> that does not overwrite".)]
>>
>> My preference is for adding an optional argument because the difference
>> in behaviour seems too small to justify a different function.
> Likewise, and even more so because the current behaviour is platform-dependent and dome platforms already behave as proposed, so the proposal really unifies the behaviour across platforms. 


I dispute this. I don't think the suggested function will work as 
described, at least not as given in the bug tracker, and certainly not 
in a platform-independent way.

I don't think you can make it completely platform independent, because 
the underlying file system operations are too different. E.g. file 
renames are atomic on POSIX systems, but not on Windows. Glossing over 
these real differences will merely give people a false sense of 
security. It is important for people to understand the limitations of 
what is possible on their system, and not be given wrong ideas.

(That's not to say that the proposed function is useless. It may be the 
closest thing that some users will get to the desired behaviour.)

For documentation purposes alone, having two separate functions is valuable.

The suggested implementation on the tracker

http://bugs.python.org/issue12741

wraps the existing shutil.move function. The most natural implementation 
is a separate function that calls the first function. To have one 
function implement both behaviours requires an unnatural implementation, 
such as:

def move(src, dest, flag=False):
     if flag:
         try:
             fd = os.open(dst, os.O_EXCL | os.O_CREAT)
         except OSError:
             raise Error('Destination exists')
         try:
             move(src, dst, False)  # recursive call
         finally:
             os.close(fd)
     else:
          # Insert current implementation here...

Slightly less ugly might be to rename the current function move to 
_move, then have this:

def move(src, dest, flag=False):
     if flag:
         try:
             fd = os.open(dst, os.O_EXCL | os.O_CREAT)
         except OSError:
             raise Error('Destination exists')
         try:
             _move(src, dst)
         finally:
             os.close(fd)
     else:
          _move(src, dst)


Either case is a prime example of why Guido's rule of thumb that 
functions shouldn't take arguments which select between two (or more) 
alternative behaviours is a good design principle. Better to keep two 
separate functions.


-1 on a flag argument to shutil.move.

+1 on a "safe" version of shutil.move.

+0 on the given implementation. I'm not convinced that the proposed 
function actually is any safer.

On the tracker, the OP says:

"If this fails, then it means that the file exists, but if it passes, 
then dst is locked so no other process can create it."

But that's not what actually happens. On my Linux box, I can do this in 
one terminal:

 >>> dst = 'spam'
 >>> fd = os.open(dst, os.O_EXCL | os.O_CREAT)
 >>> os.path.exists(dst)
True
 >>> open(dst).read()  # Confirm file is empty.
''


I then switch to another terminal, and do this:

[steve at sylar ~]$ cat spam  # File exists, and is empty.
[steve at sylar ~]$ echo "Writing to a locked file" > spam
[steve at sylar ~]$


And back to the first one:

 >>> open(dst).read()
'Writing to a locked file\n'

So despite the allegedly exclusive lock, another process was able to 
overwrite the file after the lock was taken.




-- 
Steven