Merge lp:~mbp/bzr/491763-transform-rename-failed into lp:bzr

[Martin Pool]

(mbp) better message when rename fails inside TreeTransform

Here's another attempt that changes transform more specifically. It would still be useful to do this in LocalTransport.rename, and there were already todo comments to that effect.

[Robert Collins (lifeless)]

Looks pretty good to me. Martin[gz] ?

[Martin Packman (gz)]

See the mailing list for general comments, specifics on this change below.

+ _fmt = "Failed to rename %(from_path)s to %(to_path)s: %(why)s"

As mentioned on list, `from_path` and `to_path` can be unicode and `why` can be a non-ascii bytestring. With BzrError subclasses we get "Unprintable exception..." rather than a total failure at least, but that's untested.

             osutils.rename(from_, to)
- except OSError, e:
+ except (IOError, OSError), e:

Presume this is because osutils.rename is fancy_rename on windows, that has code dealing with IOError inside? It's not clear to me it'll actually be raised here, as it's just using the standard os.rename and os.unlink functions.

+ raise errors.TransformRenameFailed(from_, to, str(e), e.errno)

When wrapping exceptions, passing the actual object is better than just the stringified version. Can safely format an exception object for output, can't say that for arbitrary bytestrings.

+ raise errors.TransformRenameFailed(to, from_, str(e), e.errno) <EOL>

Trailing whitespace.

This version is less fragile than the last one, but I don't really like changing the exception type unless you're sure those two except clauses in bzrlib.transform is the only code that cares.

review: Abstain

[Vincent Ladeuil (vila)]

bzr.dev: http://babune.ladeuil.net:24842/job/debug-windows/15/ not happy
this proposal: http://babune.ladeuil.net:24842/job/debug-windows/16/ happy

Martin [gz] and I would have preferred an automated vote for this, but hey, that's a huge progress :)

[Vincent Ladeuil (vila)]

Remember you've seen it here first !

One day, babune may vote Approve :)

review: Approve

[Robert Collins (lifeless)]

submitted to PQM by hand.

[lp:~mbp/bzr/491763-transform-rename-failed]

5198. By Martin Pool on 2010-05-13

Remove obsolete error

[Martin Pool (mbp)]

sent to pqm by email

=== modified file 'bzrlib/errors.py'
--- bzrlib/errors.py	2010-05-05 14:11:13 +0000
+++ bzrlib/errors.py	2010-05-13 16:12:40 +0000
@@ -1923,6 +1923,17 @@
 class CantMoveRoot(BzrError):
 
     _fmt = "Moving the root directory is not supported at this time"
+    
+    
+class TransformRenameFailed(BzrError):
+
+    _fmt = "Failed to rename %(from_path)s to %(to_path)s: %(why)s"
+    
+    def __init__(self, from_path, to_path, why, errno):
+        self.from_path = from_path
+        self.to_path = to_path
+        self.why = why
+        self.errno = errno
 
 
 class BzrMoveFailedError(BzrError):

=== modified file 'bzrlib/osutils.py'
--- bzrlib/osutils.py	2010-05-12 08:34:47 +0000
+++ bzrlib/osutils.py	2010-05-13 16:12:40 +0000
@@ -361,14 +361,6 @@
     return _win32_fixdrive(tempfile.mkdtemp(*args, **kwargs).replace('\\', '/'))
 
 
-def _add_rename_error_details(e, old, new):
-    new_e = OSError(e.errno, "failed to rename %s to %s: %s"
-        % (old, new, e.strerror))
-    new_e.filename = old
-    new_e.to_filename = new
-    return new_e
-
-
 def _win32_rename(old, new):
     """We expect to be able to atomically replace 'new' with old.
 
@@ -376,7 +368,7 @@
     and then deleted.
     """
     try:
-        fancy_rename(old, new, rename_func=_wrapped_rename, unlink_func=os.unlink)
+        fancy_rename(old, new, rename_func=os.rename, unlink_func=os.unlink)
     except OSError, e:
         if e.errno in (errno.EPERM, errno.EACCES, errno.EBUSY, errno.EINVAL):
             # If we try to rename a non-existant file onto cwd, we get
@@ -387,16 +379,6 @@
         raise
 
 
-def _wrapped_rename(old, new):
-    """Rename a file or directory"""
-    try:
-        os.rename(old, new)
-    except (IOError, OSError), e:
-        # this is eventually called by all rename-like functions, so should 
-        # catch all of them
-        raise _add_rename_error_details(e, old, new)
-
-
 def _mac_getcwd():
     return unicodedata.normalize('NFC', os.getcwdu())
 
@@ -407,8 +389,8 @@
 realpath = _posix_realpath
 pathjoin = os.path.join
 normpath = os.path.normpath
-rename = _wrapped_rename # overridden below on win32
 getcwd = os.getcwdu
+rename = os.rename
 dirname = os.path.dirname
 basename = os.path.basename
 split = os.path.split

=== modified file 'bzrlib/tests/test_errors.py'
--- bzrlib/tests/test_errors.py	2010-04-22 18:36:13 +0000
+++ bzrlib/tests/test_errors.py	2010-05-13 16:12:40 +0000
@@ -713,3 +713,9 @@
         e = errors.FileTimestampUnavailable("/path/foo")
         self.assertEquals("The filestamp for /path/foo is not available.",
             str(e))
+            
+    def test_transform_rename_failed(self):
+        e = errors.TransformRenameFailed(u"from", u"to", "readonly file", 2)
+        self.assertEquals(
+            u"Failed to rename from to to: readonly file",
+            str(e))

=== modified file 'bzrlib/tests/test_osutils.py'
--- bzrlib/tests/test_osutils.py	2010-05-05 14:11:13 +0000
+++ bzrlib/tests/test_osutils.py	2010-05-13 16:12:40 +0000
@@ -184,13 +184,6 @@
         shape = sorted(os.listdir('.'))
         self.assertEquals(['A', 'B'], shape)
 
-    def test_rename_error(self):
-        # We wrap os.rename to make it give an error including the filenames
-        # https://bugs.launchpad.net/bzr/+bug/491763
-        err = self.assertRaises(OSError, osutils.rename,
-            'nonexistent', 'target')
-        self.assertContainsString(str(err), 'nonexistent')
-
 
 class TestRandChars(tests.TestCase):
 

=== modified file 'bzrlib/tests/test_transform.py'
--- bzrlib/tests/test_transform.py	2010-05-10 16:25:47 +0000
+++ bzrlib/tests/test_transform.py	2010-05-13 16:12:40 +0000
@@ -852,6 +852,32 @@
         rename.set_executability(True, myfile)
         rename.apply()
 
+    def test_rename_fails(self):
+        # see https://bugs.launchpad.net/bzr/+bug/491763
+        create, root_id = self.get_transform()
+        first_dir = create.new_directory('first-dir', root_id, 'first-id')
+        myfile = create.new_file('myfile', root_id, 'myfile-text',
+                                 'myfile-id')
+        create.apply()
+        # make the file and directory readonly in the hope this will prevent
+        # renames
+        osutils.make_readonly(self.wt.abspath('first-dir'))
+        osutils.make_readonly(self.wt.abspath('myfile'))
+        # now transform to rename
+        rename_transform, root_id = self.get_transform()
+        file_trans_id = rename_transform.trans_id_file_id('myfile-id')
+        dir_id = rename_transform.trans_id_file_id('first-id')
+        rename_transform.adjust_path('newname', dir_id, file_trans_id)
+        e = self.assertRaises(errors.TransformRenameFailed,
+            rename_transform.apply)
+        # Looks like: 
+        # "Failed to rename .../work/.bzr/checkout/limbo/new-1
+        # to .../first-dir/newname: [Errno 13] Permission denied"
+        # so the first filename is not visible in it; we expect a strerror but
+        # it may vary per OS and language so it's not checked here
+        self.assertContainsRe(str(e),
+            "Failed to rename .*first-dir.newname:")
+
     def test_set_executability_order(self):
         """Ensure that executability behaves the same, no matter what order.
 

=== modified file 'bzrlib/transform.py'
--- bzrlib/transform.py	2010-05-11 08:44:59 +0000
+++ bzrlib/transform.py	2010-05-13 16:12:40 +0000
@@ -1657,7 +1657,7 @@
                       or trans_id in self._new_parent):
                     try:
                         mover.rename(full_path, self._limbo_name(trans_id))
-                    except OSError, e:
+                    except errors.TransformRenameFailed, e:
                         if e.errno != errno.ENOENT:
                             raise
                     else:
@@ -1688,7 +1688,7 @@
                 if trans_id in self._needs_rename:
                     try:
                         mover.rename(self._limbo_name(trans_id), full_path)
-                    except OSError, e:
+                    except errors.TransformRenameFailed, e:
                         # We may be renaming a dangling inventory id
                         if e.errno != errno.ENOENT:
                             raise
@@ -2926,10 +2926,12 @@
         """Rename a file from one path to another."""
         try:
             osutils.rename(from_, to)
-        except OSError, e:
+        except (IOError, OSError), e:
             if e.errno in (errno.EEXIST, errno.ENOTEMPTY):
                 raise errors.FileExists(to, str(e))
-            raise
+            # normal OSError doesn't include filenames so it's hard to see where
+            # the problem is, see https://bugs.launchpad.net/bzr/+bug/491763
+            raise errors.TransformRenameFailed(from_, to, str(e), e.errno)
         self.past_renames.append((from_, to))
 
     def pre_delete(self, from_, to):
@@ -2945,7 +2947,10 @@
     def rollback(self):
         """Reverse all renames that have been performed"""
         for from_, to in reversed(self.past_renames):
-            osutils.rename(to, from_)
+            try:
+                osutils.rename(to, from_)
+            except (OSError, IOError), e:
+                raise errors.TransformRenameFailed(to, from_, str(e), e.errno)                
         # after rollback, don't reuse _FileMover
         past_renames = None
         pending_deletions = None

=== modified file 'bzrlib/transport/local.py'
--- bzrlib/transport/local.py	2010-04-28 07:53:34 +0000
+++ bzrlib/transport/local.py	2010-05-13 16:12:40 +0000
@@ -403,14 +403,11 @@
         try:
             # *don't* call bzrlib.osutils.rename, because we want to
             # detect conflicting names on rename, and osutils.rename tries to
-            # mask cross-platform differences there; however we do update the
-            # exception to include the filenames
+            # mask cross-platform differences there
             os.rename(path_from, path_to)
         except (IOError, OSError),e:
             # TODO: What about path_to?
-            self._translate_error(
-                osutils._add_rename_error_details(e, path_from, path_to),
-                path_from)
+            self._translate_error(e, path_from)
 
     def move(self, rel_from, rel_to):
         """Move the item at rel_from to the location at rel_to"""