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

[Martin Pool]

On 30 April 2010 08:19, Martin (gzlist) <gzlist at googlemail.com> wrote:
> 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.

Thanks again for the careful review.  Feel free to just post "I think
there may be problems please wait" onto a proposal if you have not yet
had time to detail them.

> 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.

That would be an option.  It seems a bit unclean to put a tuple of
strings where there was previously a plain string, though it will
muddle through.  Perhaps this is why the core python code doesn't do
it.

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

I don't think we're losing very much information about the stack
because we raise it from so near by, though I suppose we might in the
case that os.rename has itself been monkey patched to point elsewhere.
 You might be right that we lose other data.

>
> 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.

This is going to be hard where ever we tackle this, because the ideal
message  will include both the OS error message and the filenames.  So
I think we do need to get the strerror into unicode, but perhaps we
can't assume that the exception object attribute will be directly
convertable.  (Do we need decode_strerror(e.strerror)?)

>
> 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.

True.  So we should either put both into .filename, or leave that off
altogether.

> 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.

That grep result is a wee bit misleading because most of them are
doing something other than wrapping os.rename: either renaming some
other object like a tag, or doing rename over a different transport.
There are basically three things that go down to os.rename:
treetransform and localtransport, and perhaps also direct workingtree
rename.  These layers have no obligation to propagate up OSError or
IOError therefore they avoid the issue of fitting the problem into a
single .filename attribute.

Some of the other issues still exist though:

* we'd want them to show the underlying strerror combined with the
filenames; if it's a byte string not in the default encoding this will
be difficult; perhaps we need an explicit decode_strerror helper
around access to e.strerror?

* we will lose the original exception object and its traceback; I
think normally this will just terminate in the native os.rename
therefore not be enormously interesting.

-- 
Martin <http://launchpad.net/~mbp/>