On Wed, 2010-05-12 at 19:07 -0400, Greg Ames wrote:
> This patch also produces readable error messages on z/OS by defining two new
> svn_cmdline functions which take native string arguments, then calling
> svn_cmdline_fprintf_asis() from print_error(), only in the path where I can
> see that native strings are being used.
Hi Greg.
I get why you need this patch (or some equivalent) for z/OS, but I still
don't quite understand whether or why it's also correct for a 'normal'
build of Subversion.
In an attempt both to understand and to minimize the patch: would you
get the same result by converting 'err->message' from native encoding to
UTF-8 and then printing it in the way that it currently is printed?
Like this:
[[[
Index: subversion/libsvn_subr/error.c
===================================================================
--- subversion/libsvn_subr/error.c (revision 944163)
+++ subversion/libsvn_subr/error.c (working copy)
@@ -413,15 +413,18 @@ print_error(svn_error_t *err, FILE *stre
/* Skip it. We already printed the file-line coordinates. */
}
/* Only print the same APR error string once. */
- else if (err->message)
- {
- svn_error_clear(svn_cmdline_fprintf(stream, err->pool, "%s%s\n",
- prefix, err->message));
- }
else
{
+ if (err->message)
+ {
+ /* Set ERR_STRING to a UTF-8 version of ERR->message. */
+ temp_err = svn_utf_cstring_to_utf8(&err_string, err->message,
+ err->pool);
+ if (temp_err)
+ { /* ... */ }
+ }
/* Is this a Subversion-specific error code? */
- if ((err->apr_err > APR_OS_START_USEERR)
+ else if ((err->apr_err > APR_OS_START_USEERR)
&& (err->apr_err <= APR_OS_START_CANONERR))
err_string = svn_strerror(err->apr_err, errbuf, sizeof(errbuf));
/* Otherwise, this must be an APR error code. */
]]]
It appears that Subversion currently assumes the message is already in
UTF-8. Is that not strictly true? Note that the messages nearly always
come from "gettext" (via the _() macro).
The bit that gets me is that if the current conversion (from UTF-8 to
native encoding) is correct, then replacing it with no conversion (or
native -> UTF-8 then UTF-8 to native) must be wrong.
I don't know; I'm asking if you can say something to shed light on it.
Also note that sometimes the message comes not from the client software
but from a network response sent by the server, so there is probably
another place where the encoding will need to be changed.
- Julian
> Greg
>
> Index: subversion/libsvn_subr/cmdline.c
> ===================================================================
> --- subversion/libsvn_subr/cmdline.c (revision 943729)
> +++ subversion/libsvn_subr/cmdline.c (working copy)
> @@ -316,6 +316,19 @@
> }
>
> svn_error_t *
> +svn_cmdline_fprintf_asis(FILE *stream, apr_pool_t *pool, const char *fmt,
> ...)
> +{
> + const char *message;
> + va_list ap;
> +
> + va_start(ap, fmt);
> + message = apr_pvsprintf(pool, fmt, ap);
> + va_end(ap);
> +
> + return svn_cmdline_fputs_asis(message, stream, pool);
> +}
> +
> +svn_error_t *
> svn_cmdline_fputs(const char *string, FILE* stream, apr_pool_t *pool)
> {
> svn_error_t *err;
> @@ -348,6 +361,28 @@
> }
>
> svn_error_t *
> +svn_cmdline_fputs_asis(const char *string, FILE* stream, apr_pool_t *pool)
> +{
> +
> + /* On POSIX systems, errno will be set on an error in fputs, but this
> might
> + not be the case on other platforms. We reset errno and only
> + use it if it was set by the below fputs call. Else, we just return
> + a generic error. */
> + errno = 0;
> +
> + if (fputs(string, stream) == EOF)
> + {
> + if (errno)
> + return svn_error_wrap_apr(errno, _("Write error"));
> + else
> + return svn_error_create
> + (SVN_ERR_IO_WRITE_ERROR, NULL, NULL);
> + }
> +
> + return SVN_NO_ERROR;
> +}
> +
> +svn_error_t *
> svn_cmdline_fflush(FILE *stream)
> {
> /* See comment in svn_cmdline_fputs about use of errno and stdio. */
> Index: subversion/libsvn_subr/error.c
> ===================================================================
> --- subversion/libsvn_subr/error.c (revision 943729)
> +++ subversion/libsvn_subr/error.c (working copy)
> @@ -415,8 +415,8 @@
> /* Only print the same APR error string once. */
> else if (err->message)
> {
> - svn_error_clear(svn_cmdline_fprintf(stream, err->pool, "%s%s\n",
> - prefix, err->message));
> + svn_error_clear(svn_cmdline_fprintf_asis(stream, err->pool, "%s%s\n",
> + prefix, err->message));
> }
> else
> {
> Index: subversion/include/svn_cmdline.h
> ===================================================================
> --- subversion/include/svn_cmdline.h (revision 943729)
> +++ subversion/include/svn_cmdline.h (working copy)
> @@ -128,6 +128,28 @@
> FILE *stream,
> apr_pool_t *pool);
>
> +/** Write to the stdio @a stream, using a printf-like format string @a fmt,
> + * passed through apr_pvsprintf(). All string arguments are in the native
> + * encoding; no codepage conversion is done. Use @a pool for
> + * temporary allocation.
> + *
> + * @since New in ??
> + */
> +svn_error_t *svn_cmdline_fprintf_asis(FILE *stream,
> + apr_pool_t *pool,
> + const char *fmt,
> + ...)
> + __attribute__((format(printf, 3, 4)));
> +
> +/** Output the @a string to the stdio @a stream without changing the
> encoding.
> + * Use @a pool for temporary allocation.
> + *
> + * @since New in ??
> + */
> +svn_error_t *svn_cmdline_fputs_asis(const char *string,
> + FILE *stream,
> + apr_pool_t *pool);
> +
> /** Flush output buffers of the stdio @a stream, returning an error if that
> * fails. This is just a wrapper for the standard fflush() function for
> * consistent error handling.
