You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Justin Erenkrantz <je...@apache.org> on 2002/07/28 00:49:50 UTC
[PATCH] Fix log output when iconv is not available
On platforms without iconv (*BSDs), 'svn log' exits with a fatal
error when a unconvertable log message is encountered. It has to
do with check_non_ascii returning SVN_ERR_UNSUPPORTED_FEATURE when
it encounters this case and log_message_receiver only checking for
APR_EINVAL.
So, there are two solutions:
1) Alter check_non_ascii to return APR_EINVAL.
2) Have all callers of the utf conversion functions check for
APR_EINVAL *and* SVN_ERR_UNSUPPORTED_FEATURE.
My vote is for #1. The crew on #svn seemed much more in favor of #2.
I believe it makes more sense to have a consistent API in this case.
But, I just want it to work. =) -- justin
* subversion/libsvn_subr/utf.c:
(check_non_ascii): Return APR_EINVAL instead of
SVN_ERR_UNSUPPORTED_FEATURE.
Index: ./subversion/libsvn_subr/utf.c
===================================================================
--- ./subversion/libsvn_subr/utf.c
+++ ./subversion/libsvn_subr/utf.c Sat Jul 27 17:31:09 2002
@@ -202,10 +202,9 @@
}
-/* Return SVN_ERR_UNSUPPORTED_FEATURE if the first LEN bytes of DATA
- contain anything other than seven-bit, non-control (except for
- whitespace) ascii characters, finding the error pool from POOL.
- Otherwise, return SVN_NO_ERROR. */
+/* Return APR_EINVAL if the first LEN bytes of DATA contain anything other
+ than seven-bit, non-control (except for whitespace) ascii characters,
+ finding the error pool from POOL. Otherwise, return SVN_NO_ERROR. */
static svn_error_t *
check_non_ascii (const char *data, apr_size_t len, apr_pool_t *pool)
{
@@ -231,7 +230,7 @@
= apr_pstrndup (pool, data_start, (data - data_start));
return svn_error_createf
- (SVN_ERR_UNSUPPORTED_FEATURE, 0, NULL, pool,
+ (APR_EINVAL, 0, NULL, pool,
"Safe data:\n"
"\"%s\"\n"
"... was followed by non-ascii byte %d.\n"
@@ -243,7 +242,7 @@
else
{
return svn_error_createf
- (SVN_ERR_UNSUPPORTED_FEATURE, 0, NULL, pool,
+ (APR_EINVAL, 0, NULL, pool,
"Non-ascii character (code %d) detected, "
"and unable to convert to UTF-8.\n",
*((unsigned char *) data));
==== Option #2 ====
* subversion/clients/cmdline/log-cmd
(log_message_receiver): Check for SVN_ERR_UNSUPPORTED_FEATURE
instead of APR_EINVAL.
Index: ./subversion/clients/cmdline/log-cmd.c
===================================================================
--- ./subversion/clients/cmdline/log-cmd.c
+++ ./subversion/clients/cmdline/log-cmd.c Sat Jul 27 15:07:05 2002
@@ -150,7 +150,8 @@
stuff can come later! */
err = svn_utf_cstring_from_utf8 (&author_native, author, pool);
- if (err && (APR_STATUS_IS_EINVAL (err->apr_err)))
+ if (err && (APR_STATUS_IS_EINVAL (err->apr_err) ||
+ err->apr_err == SVN_ERR_UNSUPPORTED_FEATURE))
{
SVN_ERR (svn_utf_cstring_from_utf8
@@ -163,7 +164,8 @@
return err;
err = svn_utf_cstring_from_utf8 (&date_native, date, pool);
- if (err && (APR_STATUS_IS_EINVAL (err->apr_err))) /* unlikely! */
+ if (err && (APR_STATUS_IS_EINVAL (err->apr_err) ||
+ err->apr_err == SVN_ERR_UNSUPPORTED_FEATURE)) /* unlikely! */
{
SVN_ERR (svn_utf_cstring_from_utf8
(&date_native,
@@ -175,7 +177,8 @@
return err;
err = svn_utf_cstring_from_utf8 (&msg_native, msg, pool);
- if (err && (APR_STATUS_IS_EINVAL (err->apr_err)))
+ if (err && (APR_STATUS_IS_EINVAL (err->apr_err) ||
+ err->apr_err == SVN_ERR_UNSUPPORTED_FEATURE))
{
SVN_ERR (svn_utf_cstring_from_utf8
(&msg_native,
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Fix log output when iconv is not available
Posted by Karl Fogel <kf...@newton.ch.collab.net>.
Justin Erenkrantz <je...@apache.org> writes:
> On platforms without iconv (*BSDs), 'svn log' exits with a fatal
> error when a unconvertable log message is encountered. It has to
> do with check_non_ascii returning SVN_ERR_UNSUPPORTED_FEATURE when
> it encounters this case and log_message_receiver only checking for
> APR_EINVAL.
>
> So, there are two solutions:
>
> 1) Alter check_non_ascii to return APR_EINVAL.
> 2) Have all callers of the utf conversion functions check for
> APR_EINVAL *and* SVN_ERR_UNSUPPORTED_FEATURE.
>
> My vote is for #1. The crew on #svn seemed much more in favor of #2.
> I believe it makes more sense to have a consistent API in this case.
> But, I just want it to work. =) -- justin
While I agree with the change to check_non_ascii(), it should be part
of an overall change that implements strategy #2. Let me explain:
Taking check_non_ascii() as an independent interface, it makes little
sense for it to return UNSUPPORTED_FEATURE on finding "unsafe" data.
APR_EINVAL is a much more appropriate error. So +1 on that part of
the change.
Nevertheless, callers of the public utf conversion functions *should*
be able to distinguish between these two cases:
1. "We tried to convert the data, but some of it is not
representable in your locale."
versus
2. "We don't do conversion at all, and make no claims about whether
the data is representable in your locale."
Therefore, the direct callers of check_non_ascii() should check its
return value, and convert the error to SVN_ERR_UNSUPPORTED_FEATURE as
appropriate, so their callers can make this distinction.
I certainly agree that either strategy would serve to address your
issue above, of course. I'm thinking about the longer term, and have
a hunch that we may later want/need this knowledge, so the API should
not toss the information.
Note that your point (2)
> 2) Have all callers of the utf conversion functions check for
> APR_EINVAL *and* SVN_ERR_UNSUPPORTED_FEATURE.
...is not as serious as it sounds. Most callers don't check for a
specific error at all, they just handle all errors the same way. The
big exception to this is log_message_receiver(), because it's for
human consumption (as Uli pointed out), and it will have to be
sensitive in several ways. Checking for two documented error codes
instead of one will be the least of our worries; I don't think it's a
big burden.
Which brings me to my next point:
I'm planning to resolve both your issue above and related issue #807
by incorporating your patch below, plus some code from Ulrich Drepper,
plus some changes of my own. That's a P1, so look for it in the next
few days.
If any objections to the above plan, please post! Thanks,
-K
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org