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

[Martin (gzlist)]

Thanks for going through this in detail. Obviously got what I was
trying to explain here, and the new branch with a BzrError-derived
exception avoids several of the problems with trying to use OSError.
I'm a little confused why it still does `"%s %s %s" % (maybe_unicode,
maybe_unicode, maybe_bytestring)` though, this is the main
only-works-on-the-English-developers'-machines problem I've been
whining about.

On 30/04/2010, Martin Pool <mbp at canonical.com> wrote:
>
> 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.

Yup, should have done that, just thought I had a bit longer to fuss
over the details.

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

It's a little cute, but it's minimally invasive and avoids changing
the type of the exception, which is much more likely to be relied on
by callers than a parameter that's often None.

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

Yeah, this is tricky (despite the 'easy' tag in some of the bugs), but
using the filename attribute largely avoids the problem as it keeps
the original (maybe unicode) names intact but formats them repr'd.
I've an idea of how to fix the general class of bugs, but don't have
an easy was of testing it, or of preventing regressions from devs who
have ascii-only strerror and mostly use ascii filenames.

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

Yup. One option I didn't bother mentioning was to try and put the
problem filename only in the attr. Would work for some cases like
ENOENT and EISDIR but is fiddly and not a general solution.

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

Hm, yeah, I guess my point here was there are a number of
circumstances where you might want a before-and-after record if
something goes wrong. Having comments in the tree like `# TODO: What
about path_to?` suggests other exception routes could do with being
able to record two related paths rather than just one.

Martin