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

[John Arbash Meinel]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Aaron Bentley wrote:
> Hi all,
> 
> This patch fixes the FTP transport so that it does not require atomic
> rename for FTP servers.  It uses osutils.fancy_rename to implement this.
> 
> It turns out that fancy_rename does not fail safe-- if it is impossible
> to delete the original file, it would leave the filesystem in an
> inconsistent state.  So this patch fixes fancy_rename so that it
> restores the original state on failure.
> 
> Aaron


...

=== modified file bzrlib/osutils.py
- --- bzrlib/osutils.py
+++ bzrlib/osutils.py
@@ -224,7 +224,14 @@
             # If the file used to exist, rename it back into place
             # otherwise just delete it from the tmp location
             if success:
- -                unlink_func(tmp_name)
+                try:
+                    unlink_func(tmp_name)
+                except:
+                    # If the unlink fails, then restore the old state,
as if
+                    # an atomic rename failed
+                    rename_func(new, old)
+                    rename_func(tmp_name, new)
+                    raise
             else:
                 rename_func(tmp_name, new)


I'm not sure... By this time we have already successfully moved the new
file into place, so the "atomic" portion of it has happened. We haven't
cleaned up a temp file, but that has been moved out of the way, so it
won't conflict. So it seems artificial to fail the rest.

Specifically, the steps are:


1) create new temp file (a.new.tmp)
2) rename existing file to temp file (a => a.old.tmp)
3) rename temp file to target (a.new.tmp => a)
4) delete old file (del a.old.tmp)

If step (1) fails: abort with nothing to do
If step (2) fails: abort and delete a.new.tmp
If step (3) fails: abort, move a.old.tmp => a, delete a.new.tmp
If step (4) fails:
  I would say "let the user know, but everything else completed".

  At the very least I wouldn't:
    a => a.new.tmp
    a.old.tmp => a
  without "del a.new.tmp"

In the original way, you may end up with an "a.old.tmp" file lying
around. In the new way, you end up with an "a.new.tmp" file lying around.

I would say that "you've succeeded" because the new file has taken the
place of the old file. Not "you've failed" because a cleanup step failed.



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.


John
=:->

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.5 (Cygwin)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org

iD8DBQFGI58jJdeBCYSNAAMRAvRCAJ46CnMjoGVAjRbWs4k32i/cVW6C/wCg0RYx
8hmiFTA4u+EDI4N/lhuHYJw=
=rwlc
-----END PGP SIGNATURE-----