Belated review of lp:~mbp/bzr/491763-transform-rename-failed

[Martin (gzlist)]

Wanted to comment on the transform rename issue[bug491763] proposed
fix[merge] aimed at making errors more comprehensible. However, it's
been committed before I finished working out the differences in error
handling across Python versions.

Focusing just on the reported bug, and unpacking the function
calls[unpacked_change] the change basically raises a new exception
with the message set as the old message with the two filenames, and
with `filename` and `to_filename` attributes added. If the aim is to
include the from and to filenames, a simpler
option[alternative_change] is to set the `filename` attribute and
reraise.

There are a number of problems with the committed code. Raising a
different exception loses the original stack, and the original
exception type which may be significant if it's an IOError or a
WindowsError (with additional members).

The new message formatting mixes filenames (perhaps unicode) and the
original strerror (perhaps a non-ascii bytestring). There's all kinds
of fallout here:

* StandardError types in Python prior to 2.6 do not have a __unicode__
method which breaks[unicode_error_issue] exception printing,
including[unicode_error_issue_trace] through bzrlib.trace.
* Localised errors[pt_os_rename] are raised with Python 2.5 and later
on non-english windows, this breaks[pt_osutils_rename] before the new
exception is even raised.

I thought problems with nice localised error messages were windows
specific[bug273978][bug507535], but apparently you can get them on
Ubuntu too these days[bug567584].

Alexander has another issue where the message of the original OSError
is stripped entirely (instead of being Russian in CP1251), but I've
not been able to track down the cause of that in the Python source, so
I don't know what versions it affects.

Finally, the new exception includes both filenames in the exception
string, and sets the filename attribute to the renamed-from name. This
is duplicative, and can lead to incorrect[confusing_error_nix]
messages.

There are lots of os.rename wrappers and other rename functions
already[grep_rename_functions], changing those seems preferable to
adding another layer. I strongly suggest using an alternative approach
instead, not trying to fix the issues I've mentioned one at a time.

Martin


[bug491763]: https://bugs.launchpad.net/bzr/+bug/491763/

[merge]: https://code.launchpad.net/~mbp/bzr/491763-transform-rename-failed/+merge/24293

[unpacked_change]:
     def rename(self, from_, to):
-        """Rename a file from one path to another.  Functions like os.rename"""
+        """Rename a file from one path to another."""
         try:
-            os.rename(from_, to)
+            try:
+                os.rename(from_, to)
+            except (IOError, OSError), e:
+                # this is eventually called by all rename-like functions, so
+                # should catch all of them
+                new_e = OSError(e.errno, "failed to rename %s to %s: %s"
+                    % (from_, to, e.strerror))
+                new_e.filename = from_
+                new_e.to_filename = to
+                raise new_e
         except OSError, e:
             if e.errno in (errno.EEXIST, errno.ENOTEMPTY):
                 raise errors.FileExists(to, str(e))
             raise
         self.past_renames.append((from_, to))

[alternative_change]:
     def rename(self, from_, to):
-        """Rename a file from one path to another.  Functions like os.rename"""
+        """Rename a file from one path to another."""
         try:
             os.rename(from_, to)
         except OSError, e:
             if e.errno in (errno.EEXIST, errno.ENOTEMPTY):
                 raise errors.FileExists(to, str(e))
+            e.filename = from_, to
             raise
         self.past_renames.append((from_, to))

[unicode_error_issue]:
>>> raise OSError(u"Σ")
Traceback (most recent call last):
  File "<stdin>", line 1, in ?
OSError>>>

[unicode_error_issue_trace]:
>>> import sys
>>> from bzrlib import trace, osutils
>>> try:
...   osutils.rename("nonexistant", u"Σ")
... except:
...   trace.report_exception(sys.exc_info(), sys.stderr)
...
Traceback (most recent call last):
  File "<stdin>", line 4, in <module>
  File "bzrlib\trace.py", line 498, in report_exception
    report_user_error(exc_info, err_file)
  File "bzrlib\trace.py", line 523, in report_user_error
    err_file.write("bzr: ERROR: %s\n" % (exc_info[1],))
UnicodeEncodeError: 'ascii' codec can't encode character u'\u03a3' in
position 54: ordinal not in range(128)

[pt_os_rename]:
>>> os.rename("nothere", u"Σ")
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
WindowsError: [Error 2] O sistema não conseguiu localizar o ficheiro
especificado.

[pt_osutils_rename]:
>>> osutils.rename("nothere", u"Σ")
Traceback (most recent call last):
  File "<stdin>", line 1, in ?
  File "bzrlib\osutils.py", line 380, in _win32_rename
    fancy_rename(old, new, rename_func=_wrapped_rename, unlink_func=os.unlink)
  File "bzrlib\osutils.py", line 236, in fancy_rename
    rename_func(new, tmp_name)
  File "bzrlib\osutils.py", line 398, in _wrapped_rename
    raise _add_rename_error_details(e, old, new)
  File "bzrlib\osutils.py", line 366, in _add_rename_error_details
    new_e = OSError(e.errno, "failed to rename %s to %s: %s"
UnicodeDecodeError: 'ascii' codec can't decode byte 0xe3 in position
11: ordinal not in range(128)

[bug273978]: https://bugs.launchpad.net/bzr/+bug/273978/

[bug507535]: https://bugs.launchpad.net/bzr/+bug/507535/

[bug567584]: https://bugs.launchpad.net/bzr/+bug/567584/

[confusing_error_nix]:
>>> from bzrlib import osutils
>>> import os
>>> file("/tmp/a","w").close()
>>> os.mkdir("/tmp/b")
>>> osutils.rename("/tmp/a", "/tmp/b")
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "bzrlib/osutils.py", line 398, in _wrapped_rename
    raise _add_rename_error_details(e, old, new)
OSError: [Errno 21] failed to rename /tmp/a to /tmp/b: Is a directory: '/tmp/a'

[grep_rename_functions]:
C:\bzr\bzr\dev>grep -r --include=*.py "def rename" bzrlib
bzrlib/bundle/bundle_data.py:        def renamed(kind, extra, lines):
bzrlib/commit.py:    def renamed(self, change, old_path, new_path):
bzrlib/commit.py:    def renamed(self, change, old_path, new_path):
bzrlib/inventory.py:    def rename(self, file_id, new_parent_id, new_name):
bzrlib/memorytree.py:    def rename_one(self, from_rel, to_rel):
bzrlib/tag.py:    def rename_revisions(self, rename_map):
bzrlib/tag.py:    def rename_revisions(self, rename_map):
bzrlib/tests/per_intertree/test_compare.py:    def renamed(self,
from_tree, to_tree, file_id, content_changed):
bzrlib/tests/per_repository/test_commit_builder.py:        def rename():
bzrlib/tests/per_repository/test_commit_builder.py:        def rename():
bzrlib/tests/stub_sftp.py:    def rename(self, oldpath, newpath):
bzrlib/tests/test_commit.py:    def renamed(self, change, old_path, new_path):
bzrlib/tests/test_transform.py:        def rename(self, source, target):
bzrlib/transform.py:    def rename(self, from_, to):
bzrlib/transport/brokenrename.py:    def rename(self, rel_from, rel_to):
bzrlib/transport/decorator.py:    def rename(self, rel_from, rel_to):
bzrlib/transport/fakenfs.py:    def rename(self, rel_from, rel_to):
bzrlib/transport/ftp/__init__.py:    def rename(self, rel_from, rel_to):
bzrlib/transport/local.py:    def rename(self, rel_from, rel_to):
bzrlib/transport/memory.py:    def rename(self, rel_from, rel_to):
bzrlib/transport/pathfilter.py:    def rename(self, rel_from, rel_to):
bzrlib/transport/remote.py:    def rename(self, rel_from, rel_to):
bzrlib/transport/sftp.py:    def rename(self, rel_from, rel_to):
bzrlib/transport/trace.py:    def rename(self, rel_from, rel_to):
bzrlib/transport/__init__.py:    def rename(self, rel_from, rel_to):
bzrlib/util/configobj/configobj.py:    def rename(self, oldkey, newkey):
bzrlib/workingtree.py:    def rename_one(self, from_rel, to_rel, after=False):
bzrlib/workingtree_4.py:    def rename_one(self, from_rel, to_rel, after=False):