Bazaar

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

  1. 491763-transform-rename-failed
  2. Merge into bzr.dev
Proposed by Martin Pool
Status: Merged
Approved by: Vincent Ladeuil
Approved revision: no longer in the source branch.
Merged at revision: 5228
Proposed branch: lp:~mbp/bzr/491763-transform-rename-failed
Merge into: lp:bzr
Diff against target: 215 lines (+57/-37)
7 files modified
bzrlib/errors.py (+11/-0)
bzrlib/osutils.py (+2/-20)
bzrlib/tests/test_errors.py (+6/-0)
bzrlib/tests/test_osutils.py (+0/-7)
bzrlib/tests/test_transform.py (+26/-0)
bzrlib/transform.py (+10/-5)
bzrlib/transport/local.py (+2/-5)
To merge this branch: bzr merge lp:~mbp/bzr/491763-transform-rename-failed
Reviewer Review Type Date Requested Status
Vincent Ladeuil Approve
Martin Packman (community) Abstain
Review via email: mp+24474@code.launchpad.net

Commit message

(mbp) better message when rename fails inside TreeTransform

Description of the change

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.

To post a comment you must log in.
Revision history for this message
Robert Collins (lifeless) wrote :

Looks pretty good to me. Martin[gz] ?

Revision history for this message
Martin Packman (gz) wrote :

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
Revision history for this message
Vincent Ladeuil (vila) wrote :

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 :)

Revision history for this message
Vincent Ladeuil (vila) wrote :

Remember you've seen it here first !

One day, babune may vote Approve :)

review: Approve
Revision history for this message
Robert Collins (lifeless) wrote :

submitted to PQM by hand.

Revision history for this message
Martin Pool (mbp) wrote :

sent to pqm by email

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'bzrlib/errors.py'
2--- bzrlib/errors.py 2010-05-05 14:11:13 +0000
3+++ bzrlib/errors.py 2010-05-13 16:12:40 +0000
4@@ -1923,6 +1923,17 @@
5 class CantMoveRoot(BzrError):
6
7 _fmt = "Moving the root directory is not supported at this time"
8+
9+
10+class TransformRenameFailed(BzrError):
11+
12+ _fmt = "Failed to rename %(from_path)s to %(to_path)s: %(why)s"
13+
14+ def __init__(self, from_path, to_path, why, errno):
15+ self.from_path = from_path
16+ self.to_path = to_path
17+ self.why = why
18+ self.errno = errno
19
20
21 class BzrMoveFailedError(BzrError):
22
23=== modified file 'bzrlib/osutils.py'
24--- bzrlib/osutils.py 2010-05-12 08:34:47 +0000
25+++ bzrlib/osutils.py 2010-05-13 16:12:40 +0000
26@@ -361,14 +361,6 @@
27 return _win32_fixdrive(tempfile.mkdtemp(*args, **kwargs).replace('\\', '/'))
28
29
30-def _add_rename_error_details(e, old, new):
31- new_e = OSError(e.errno, "failed to rename %s to %s: %s"
32- % (old, new, e.strerror))
33- new_e.filename = old
34- new_e.to_filename = new
35- return new_e
36-
37-
38 def _win32_rename(old, new):
39 """We expect to be able to atomically replace 'new' with old.
40
41@@ -376,7 +368,7 @@
42 and then deleted.
43 """
44 try:
45- fancy_rename(old, new, rename_func=_wrapped_rename, unlink_func=os.unlink)
46+ fancy_rename(old, new, rename_func=os.rename, unlink_func=os.unlink)
47 except OSError, e:
48 if e.errno in (errno.EPERM, errno.EACCES, errno.EBUSY, errno.EINVAL):
49 # If we try to rename a non-existant file onto cwd, we get
50@@ -387,16 +379,6 @@
51 raise
52
53
54-def _wrapped_rename(old, new):
55- """Rename a file or directory"""
56- try:
57- os.rename(old, new)
58- except (IOError, OSError), e:
59- # this is eventually called by all rename-like functions, so should
60- # catch all of them
61- raise _add_rename_error_details(e, old, new)
62-
63-
64 def _mac_getcwd():
65 return unicodedata.normalize('NFC', os.getcwdu())
66
67@@ -407,8 +389,8 @@
68 realpath = _posix_realpath
69 pathjoin = os.path.join
70 normpath = os.path.normpath
71-rename = _wrapped_rename # overridden below on win32
72 getcwd = os.getcwdu
73+rename = os.rename
74 dirname = os.path.dirname
75 basename = os.path.basename
76 split = os.path.split
77
78=== modified file 'bzrlib/tests/test_errors.py'
79--- bzrlib/tests/test_errors.py 2010-04-22 18:36:13 +0000
80+++ bzrlib/tests/test_errors.py 2010-05-13 16:12:40 +0000
81@@ -713,3 +713,9 @@
82 e = errors.FileTimestampUnavailable("/path/foo")
83 self.assertEquals("The filestamp for /path/foo is not available.",
84 str(e))
85+
86+ def test_transform_rename_failed(self):
87+ e = errors.TransformRenameFailed(u"from", u"to", "readonly file", 2)
88+ self.assertEquals(
89+ u"Failed to rename from to to: readonly file",
90+ str(e))
91
92=== modified file 'bzrlib/tests/test_osutils.py'
93--- bzrlib/tests/test_osutils.py 2010-05-05 14:11:13 +0000
94+++ bzrlib/tests/test_osutils.py 2010-05-13 16:12:40 +0000
95@@ -184,13 +184,6 @@
96 shape = sorted(os.listdir('.'))
97 self.assertEquals(['A', 'B'], shape)
98
99- def test_rename_error(self):
100- # We wrap os.rename to make it give an error including the filenames
101- # https://bugs.launchpad.net/bzr/+bug/491763
102- err = self.assertRaises(OSError, osutils.rename,
103- 'nonexistent', 'target')
104- self.assertContainsString(str(err), 'nonexistent')
105-
106
107 class TestRandChars(tests.TestCase):
108
109
110=== modified file 'bzrlib/tests/test_transform.py'
111--- bzrlib/tests/test_transform.py 2010-05-10 16:25:47 +0000
112+++ bzrlib/tests/test_transform.py 2010-05-13 16:12:40 +0000
113@@ -852,6 +852,32 @@
114 rename.set_executability(True, myfile)
115 rename.apply()
116
117+ def test_rename_fails(self):
118+ # see https://bugs.launchpad.net/bzr/+bug/491763
119+ create, root_id = self.get_transform()
120+ first_dir = create.new_directory('first-dir', root_id, 'first-id')
121+ myfile = create.new_file('myfile', root_id, 'myfile-text',
122+ 'myfile-id')
123+ create.apply()
124+ # make the file and directory readonly in the hope this will prevent
125+ # renames
126+ osutils.make_readonly(self.wt.abspath('first-dir'))
127+ osutils.make_readonly(self.wt.abspath('myfile'))
128+ # now transform to rename
129+ rename_transform, root_id = self.get_transform()
130+ file_trans_id = rename_transform.trans_id_file_id('myfile-id')
131+ dir_id = rename_transform.trans_id_file_id('first-id')
132+ rename_transform.adjust_path('newname', dir_id, file_trans_id)
133+ e = self.assertRaises(errors.TransformRenameFailed,
134+ rename_transform.apply)
135+ # Looks like:
136+ # "Failed to rename .../work/.bzr/checkout/limbo/new-1
137+ # to .../first-dir/newname: [Errno 13] Permission denied"
138+ # so the first filename is not visible in it; we expect a strerror but
139+ # it may vary per OS and language so it's not checked here
140+ self.assertContainsRe(str(e),
141+ "Failed to rename .*first-dir.newname:")
142+
143 def test_set_executability_order(self):
144 """Ensure that executability behaves the same, no matter what order.
145
146
147=== modified file 'bzrlib/transform.py'
148--- bzrlib/transform.py 2010-05-11 08:44:59 +0000
149+++ bzrlib/transform.py 2010-05-13 16:12:40 +0000
150@@ -1657,7 +1657,7 @@
151 or trans_id in self._new_parent):
152 try:
153 mover.rename(full_path, self._limbo_name(trans_id))
154- except OSError, e:
155+ except errors.TransformRenameFailed, e:
156 if e.errno != errno.ENOENT:
157 raise
158 else:
159@@ -1688,7 +1688,7 @@
160 if trans_id in self._needs_rename:
161 try:
162 mover.rename(self._limbo_name(trans_id), full_path)
163- except OSError, e:
164+ except errors.TransformRenameFailed, e:
165 # We may be renaming a dangling inventory id
166 if e.errno != errno.ENOENT:
167 raise
168@@ -2926,10 +2926,12 @@
169 """Rename a file from one path to another."""
170 try:
171 osutils.rename(from_, to)
172- except OSError, e:
173+ except (IOError, OSError), e:
174 if e.errno in (errno.EEXIST, errno.ENOTEMPTY):
175 raise errors.FileExists(to, str(e))
176- raise
177+ # normal OSError doesn't include filenames so it's hard to see where
178+ # the problem is, see https://bugs.launchpad.net/bzr/+bug/491763
179+ raise errors.TransformRenameFailed(from_, to, str(e), e.errno)
180 self.past_renames.append((from_, to))
181
182 def pre_delete(self, from_, to):
183@@ -2945,7 +2947,10 @@
184 def rollback(self):
185 """Reverse all renames that have been performed"""
186 for from_, to in reversed(self.past_renames):
187- osutils.rename(to, from_)
188+ try:
189+ osutils.rename(to, from_)
190+ except (OSError, IOError), e:
191+ raise errors.TransformRenameFailed(to, from_, str(e), e.errno)
192 # after rollback, don't reuse _FileMover
193 past_renames = None
194 pending_deletions = None
195
196=== modified file 'bzrlib/transport/local.py'
197--- bzrlib/transport/local.py 2010-04-28 07:53:34 +0000
198+++ bzrlib/transport/local.py 2010-05-13 16:12:40 +0000
199@@ -403,14 +403,11 @@
200 try:
201 # *don't* call bzrlib.osutils.rename, because we want to
202 # detect conflicting names on rename, and osutils.rename tries to
203- # mask cross-platform differences there; however we do update the
204- # exception to include the filenames
205+ # mask cross-platform differences there
206 os.rename(path_from, path_to)
207 except (IOError, OSError),e:
208 # TODO: What about path_to?
209- self._translate_error(
210- osutils._add_rename_error_details(e, path_from, path_to),
211- path_from)
212+ self._translate_error(e, path_from)
213
214 def move(self, rel_from, rel_to):
215 """Move the item at rel_from to the location at rel_to"""