You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Greg Ames <am...@gmail.com> on 2010/05/12 23:07:31 UTC
[PATCH] readable error messages on non-ASCII systems, v2
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.
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.
RE: [PATCH] readable error messages on non-ASCII systems, v2
Posted by Bert Huijben <be...@qqmail.nl>.
> -----Original Message-----
> From: Julian Foad [mailto:julian.foad@wandisco.com]
> Sent: maandag 17 mei 2010 11:25
> To: Greg Ames
> Cc: Subversion Dev
> Subject: Re: [PATCH] readable error messages on non-ASCII systems, v2
>
> (Cc'ing the dev@ list.)
>
> On Fri, 2010-05-14, Greg Ames wrote:
> > Fri, May 14, 2010, Julian Foad wrote:
> > > 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?
> >
> > I had to adjust the patch to fit 1.5.6 (no SQLite here). Then
> > svn_utf_* reported that there were non-UTF-8 characters at the
> > beginning of the message. This time it was the "svn: " prefix passed
> > to svn_cmdline_fprintf a few lines down. I changed the patch to also
> > convert the prefix input variable to UTF-8 and pass that to
> > svn_cmdline_fprintf, and I see readable error messages again.
>
> Hi Greg. Great - it sounds like that gives you a less invasive solution
> to that particular bit. Let us know how you get on.
Googling for z/OS and Ascii shows that there are some z/OS C compilers that allow compiling programs using ASCII literals.
(E.g. http://www-01.ibm.com/software/awdtools/czos/features/czosv1r2.html: See 'ASCII Support')
Did you try using these compiler flags instead of trying to fix the source code?
I'm afraid that you will only find more and more places where we assume that our literals use ASCII, so if I would have to fix Subversion I would look into using some automated tool. (Another thing I thought about was using some kind of C preprocessor before compiling, which translates the strings to utf-8 using some escaping mechanism)
I think our output handling is mostly separated to allow converting from utf-8 to the native encoding, so maybe applying a few compiler flags can help you more than patching the sourcecode.
Bert
>
> - Julian
>
Re: [PATCH] readable error messages on non-ASCII systems, v2
Posted by Julian Foad <ju...@wandisco.com>.
(Cc'ing the dev@ list.)
On Fri, 2010-05-14, Greg Ames wrote:
> Fri, May 14, 2010, Julian Foad wrote:
> > 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?
>
> I had to adjust the patch to fit 1.5.6 (no SQLite here). Then
> svn_utf_* reported that there were non-UTF-8 characters at the
> beginning of the message. This time it was the "svn: " prefix passed
> to svn_cmdline_fprintf a few lines down. I changed the patch to also
> convert the prefix input variable to UTF-8 and pass that to
> svn_cmdline_fprintf, and I see readable error messages again.
Hi Greg. Great - it sounds like that gives you a less invasive solution
to that particular bit. Let us know how you get on.
- Julian
Re: [PATCH] readable error messages on non-ASCII systems, v2
Posted by Julian Foad <ju...@wandisco.com>.
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.