[PATCH] Change error reporting for xlate problems

I just spent a very non-pleasant hour looking into why svn.apache.org's
sync fails with UTF-8 errors. Turns out that apr_xlate_open() was
returning APR_ENOTIMPL and libsvn_subr was kindly ignoring that error
without even a log message.

The error handling logic in question was added in r15379 <->
http://svn.apache.org/r855453. The log message of that revision
indicates the intent was to fix a memory leak; whether excluding EINVAL
and ENOTIMPL from the error handling block that follows was intentional
is unclear.

So, any objections to the following?

[[[
Follow-up to r15379(r855453): don't discard apr_xlate_open() failures.

The incumbent code just leads to "Unable to translate to/from UTF-8"
errors later on, which renders svnsync unable to sync log messages
containing non-ASCII (even though they can be committed) and fails our
test suite in three places:

    FAIL: prop_tests.py 22: test prop* handle invalid property names
    FAIL: svnsync_tests.py 28: copy and reencode non-UTF-8 svn:* props
    FAIL: svnsync_tests.py 29: copy UTF-8 svn:* props identically

* subversion/libsvn_subr/utf.c
  (xlate_alloc_handle): Have EINVAL and ENOTIMPL return values from
    apr_xlate_open() fall through to the normal error handling.

    Rejigger that handling so that apr_strerror(EINVAL) or
    apr_strerror(ENOTIMPL) are included in the error chain.
    (The incumbent code would report error code E70023, which is
    APR_ENOTIMPL, but mere mortals aren't supposed to know that.)
]]]

[[[
Index: subversion/libsvn_subr/utf.c
===================================================================
--- subversion/libsvn_subr/utf.c (revision 1501080)
+++ subversion/libsvn_subr/utf.c (working copy)
@@ -230,9 +230,12 @@ xlate_alloc_handle(xlate_handle_node_t **ret,
 
   if (APR_STATUS_IS_EINVAL(apr_err) || APR_STATUS_IS_ENOTIMPL(apr_err))
     handle = NULL;
- else if (apr_err != APR_SUCCESS)
+ if (apr_err != APR_SUCCESS)
     {
       const char *errstr;
+ svn_error_t *err;
+ char apr_strerr[512];
+
       /* Can't use svn_error_wrap_apr here because it calls functions in
          this file, leading to infinite recursion. */
       if (frompage == SVN_APR_LOCALE_CHARSET)
@@ -248,7 +251,13 @@ xlate_alloc_handle(xlate_handle_node_t **ret,
                               _("Can't create a character converter from "
                                 "'%s' to '%s'"), frompage, topage);
 
- return svn_error_create(apr_err, NULL, errstr);
+ /* Just put the error on the stack, since svn_error_create duplicates it
+ later. APR_STRERR will be in the local encoding, not in UTF-8, though.
+ */
+ svn_strerror(apr_err, apr_strerr, sizeof(apr_strerr));
+ return svn_error_create(apr_err,
+ svn_error_create(apr_err, NULL, apr_strerr),
+ errstr);
     }
 
   /* Allocate and initialize the node. */
]]]

The error upon ENOTIMPL looks like this:

    svn: E070023: Can't create a character converter from native encoding to 'UTF-8'
    svn: E070023: This function has not been implemented on this platform

It appears as soon as svn_utf_cstring_to_utf8() is called --- which is
normally during argv parsing. The error happens even if the argv
arguments are all ASCII, which effectively adds a new dependency on
apr_xlate_* even for people who use only ASCII. I assume this new
failure mode for ASCII-only users means we cannot backport this change.

---
Thoughts?