[MERGE] Support FTP servers with non-atomic rename (bug #89436)

[John Arbash Meinel]

Aaron Bentley wrote:
> John Arbash Meinel wrote:

...

>> So I'm +1 on the update to FTP. I'm at least -0 on the change to
>> fancy_rename. Because I think you are failing when you don't have to.
> 
> I changed fancy_rename so that the FTP tests would pass.  I cannot make
> the FTP change without either fixing fancy_rename or disabling
> test_rename_dir_nonempty, and the latter seems unwise.
> 
> Aaron

FTPTransport.rename() should not be using 'fancy_rename'.
FTPTransport.move() should.

I guess I was mistaken in which functions you were changing. I would not
use "fancy_rename" for directories, and I certainly would avoid it in
'def rename()'.

The specific docstrings are:

def rename(self, rel_from, rel_to):
    """Rename a file or directory.

    This *must* fail if the destination is a nonempty directory - it must
    not automatically remove it.  It should raise DirectoryNotEmpty, or
    some other PathError if the case can't be specifically detected.

    If the destination is an empty directory or a file this function may
    either fail or succeed, depending on the underlying transport.  It
    should not attempt to remove the destination if overwriting is not the
    native transport behaviour.  If at all possible the transport should
    ensure that the rename either completes or not, without leaving the
    destination deleted and the new file not moved in place.

    This is intended mainly for use in implementing LockDir.
    """



def move(self, rel_from, rel_to):
    """Move the item at rel_from to the location at rel_to.

    The destination is deleted if possible, even if it's a non-empty
    directory tree.

    If a transport can directly implement this it is suggested that
    it do so for efficiency.
    """


So "Transport.rename()" translates to os.rename() on all platforms.
(Since it is defined to fail for non-empty directories, and is likely to
fail for existing target files on some platforms.)

Transport.move() is defined to have POSIX file rename semantics. So it
will overwrite an existing file.


So your change should have been to FtpTransport.move()

which is what:
https://bugs.launchpad.net/bzr/+bug/89436

was actually talking about.


It is unfortunate that we have 2 similar functions (move() and rename())
that have slightly different behavior. But it is specifically because we
need "rename-over-the-top" for files (we call that move()) and we don't
want it for directories (we call that rename()).

Arguably they could be called:

 rename_with_overwrite()
and
 rename_no_replace()

Or something like that to make it more clear.

John
=:->