> It's good to have this problem addressed, but I have some concerns with
> this patch. Maybe my concerns result from lack of understanding.
>
> I see that a slightly modified version of this patch has already been
> committed.
Right, but Karl and Philip Martin expressed some concerns too with questions
I was asking or the code itself, so it's never too late. And better late
then never :-)
Besides, I was quick at committing.
>
> Erik Huelsmann wrote:
> > Log:
> > [[[
> > Fix svnserve error message translation by postponing locale
> initialization.
>
> Fixes it? Do you mean it correctly translates all messages according to
> the
> locale of the display on which they are printed? Perhaps the log message
> could
> be clearer.
From this comment and some of the comments below (as well as Philip's
comments), I think I may need to draw up a document describing our use of
gettext. Especially in relation to server environments.
Anyway, no, it really fixes it: before, the server would translate all
errors which occurred to the locale which the server is running in (if the
LC_MESSAGES constant is not defined). This is because, as you are asking
below, on those systems you can turn *on* message translation (by
initialising LC_ALL), but have no way to turn off LC_MESSAGES. Turning off
LC_ALL is not an option (also a question below), because we want our
libraries to translate the (internally UTF8) non-ascii characters to their
locale equivalents - which is why we can't unset LC_CTYPE.
> > * subversion/include/svn_cmdline.h,
> > subversion/libsvn_subr/cmdline.c (svn_cmdline_init): Deprecated.
> > (svn_cmdline_init2): New. Derived from svn_cmdline_init, but takes
> > an extra 'server_mode' boolean parameter.
> [...]
> > * subversion/svnserve/main.c: Set LC_ALL locale before calling _() (and
> > thus gettext()) to ensure getting the correct translation. Also,
> > don't translate messages which may be sent to the client.
> I assume that the reason for not translating some messages is because in
> some of its operating modes there is no way (yet) for this server to know
the
> client's locale, and thus the messages should be sent in their original
> English form.
Yes. And, if we were using gettext to do the translation to the client
locale (if we were able to marshall it to the server), the server would be
required to have all locales installed, not only for svn, but also for other
dependencies. Next to that, we currently use a process-global setting, which
I presume won't be threadsafe. We need a threadsafe solution for svnserve to
operate correctly on Windows. And for the final blow: Peter Lundblad told me
yesterday that locale names aren't standardized...
> > ]]]
>
> > Index: subversion/include/svn_cmdline.h
> > ===================================================================
> > --- subversion/include/svn_cmdline.h (revision 13176)
> > +++ subversion/include/svn_cmdline.h (working copy)
> > @@ -37,14 +37,31 @@
> > #endif /* __cplusplus */
> >
> >
> > -/** Set up the locale for character conversion, and initialize APR.
> > +/**
> > + *
> > + * @since New in 1.2
> > + *
> > + * Set up the locale and initialize APR and gettext.
> > + * Only sets up LC_CTYPE when @a server_mode is true, LC_ALL otherwise.
>
> Why does a server process want to run with the LC_CTYPE of its
> environment, but not any other locale categories? I'm not saying it's
wrong,
> but I can't think of the reason.
On the server we don't use any other categories (LC_MESSAGES for
translations and LC_CTYPE for character conversion); the client also uses
LC_TIME, but the server doesn't.
> > Index: subversion/svnserve/main.c
> > ===================================================================
> > --- subversion/svnserve/main.c (revision 13176)
> > +++ subversion/svnserve/main.c (working copy)
> > @@ -258,6 +261,7 @@
> > err = check_lib_versions ();
> > if (err)
> > {
> > + setlocale (LC_ALL, "");
> > svn_handle_error (err, stderr, FALSE);
> > svn_error_clear (err);
> > svn_pool_destroy (pool);
> > @@ -328,16 +332,18 @@
> >
> > case 'R':
> > params.read_only = TRUE;
> > + /* Don't translate the warning below: we may stay in
> > + server mode (and thus don't want to translate messages) */
>
> That doesn't make sense. Surely all warnings that go to stderr/stdout
> should be translated according the locale of the local environment.
>
> Are you doing this just because, if you turn translation on, you don't
> have a way of turning it off again?
Yup. I'll extend the comment.
>
> > svn_error_clear
> > (svn_cmdline_fprintf
> > (stderr, pool,
> > - _("Warning: -R is deprecated.\n"
> > - "Anonymous access is now read-only by default.\n"
> > - " auth-access = read|write|none (default write)\n"
> > - "Forcing all access to read-only for now\n")));
> > + "Warning: -R is deprecated.\n"
> > + "Anonymous access is now read-only by default.\n"
> > + "Forcing all access to read-only for now\n"));
> > break;
> >
> > case 'T':
> > @@ -350,6 +356,7 @@
> >
> > if (params.tunnel_user && run_mode != run_mode_tunnel)
> > {
> > + setlocale (LC_ALL, "");
>
> So you explicitly turn translation on before printing _some_ error
> messages...
Correct.
> > svn_error_clear
> > (svn_cmdline_fprintf
> > (stderr, pool,
>
> (The message here has no replaceable parameters, so _fputs would do, but
> that's irrelevant.)
Will change, while in there though.
> > @@ -359,9 +366,10 @@
> >
> > if (run_mode == run_mode_none)
> > {
> > - svn_error_clear
> > - (svn_cmdline_fprintf
> > - (stderr, pool, _("You must specify one of -d, -i, -t or
> -X.\n")));
> > + setlocale (LC_ALL, "");
> > + svn_error_clear (svn_cmdline_fputs
> > + (_("You must specify one of -d, -i, -t or
> -X.\n"),
> > + stderr, pool));
>
> (Is there any particular reason why you changed this from _fprintf to
> _fputs,
> but didn't change the previous one?)
umm, no.
> > usage(argv[0], pool);
> > }
> >
> > @@ -386,10 +394,8 @@
> > #endif
> > if (status)
> > {
> > - svn_error_clear
> > - (svn_cmdline_fprintf
> > - (stderr, pool, _("Can't create server socket: %s\n"),
> > - apr_strerror(status, errbuf, sizeof(errbuf))));
> > + fprintf (stderr, "Can't create server socket: %s\n",
> > + apr_strerror(status, errbuf, sizeof(errbuf)));
>
> ... but why don't you want this message (or the next two) translated?
> These aren't messages that will be sent to the client, are they?
Ah, these and the next messages only occur when started daemon mode. So,
yes, I do want to convert them. About the fprintf() call: the command line
client uses the same idea; when there is no UTF8 conversion (because the
text is all ascii), then straight fprintf()s are used. But, given that we do
want them translated after all, the issue turns moot.
>
> Also, why don't you want them converted to the native character encoding?
>
> > exit(1);
> > }
> >
> > @@ -400,33 +406,21 @@
> > status = apr_sockaddr_info_get(&sa, host, APR_INET, port, 0, pool);
> > if (status)
> > {
> > - svn_error_clear
> > - (svn_cmdline_fprintf
> > - (stderr, pool, _("Can't get address info: %s\n"),
> > - apr_strerror(status, errbuf, sizeof(errbuf))));
> > + fprintf (stderr, "Can't get address info: %s\n",
> > + apr_strerror(status, errbuf, sizeof(errbuf)));
> > exit(1);
> > }
> >
> > status = apr_socket_bind(sock, sa);
> > if (status)
> > {
> > - svn_error_clear
> > - (svn_cmdline_fprintf
> > - (stderr, pool, _("Can't bind server socket: %s\n"),
> > - apr_strerror(status, errbuf, sizeof(errbuf))));
> > + fprintf (stderr, "Can't bind server socket: %s\n",
> > + apr_strerror(status, errbuf, sizeof(errbuf)));
>
>
> Wouldn't the best thing be to leave the server running in its own locale,
> like it was, and just avoid translating (i.e. calling gettext on) any
messages
> that are transmitted from server to client? (They can still be marked for
> translation, with N_(), and the translations can be used when the client's
> locale is known.)
Ah, but that's impossible. Some of the reasons were outlined above, but also
because all error strings are currently marked with _(). So translation
(currently) happens deep down in libsvn_*. Those messages will have been
translated way before we can touch them. OTOH, it's impossible to move
translation up to right before we print the messages, because we need to
translate them before filling in the parameters...
> Apologies if I've got completely the wrong idea about this.
Being incompletely informed is easy in this area: it took a whole lot of
discussion before we were where we are now. I'll use this mail to generate a
notes/ document and address the other issues in an upcomming commit.
Thanks for the review!
bye,
Erik.
--
Lassen Sie Ihren Gedanken freien Lauf... z.B. per FreeSMS
GMX bietet bis zu 100 FreeSMS/Monat: http://www.gmx.net/de/go/mail
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Mon Feb 28 21:46:24 2005