Paul Burba wrote:
>
> This is the first of several (probably 5 - 10) patch submissions that will
> allow subversion to run on the IBM iSeries under OS400 V5R4. I'll try to
> keep each patch relatively small and focused on one problem.
Excellent, we appreciate the effort to make the patches focused.
Sorry, I've written rather a lot of criticism below about what is really a
rather straightforward change.
> [[[
OK, here we want to insert a description of the change and its purpose,
something like:
OS400/EBCDIC port: convert command-line arguments from EBCDIC to UTF-8.
This is one of several patches to allow Subversion to run on the IBM iSeries
under OS400 V5R4.
> * subversion/svn/main.c:
> * subversion/svnadmin/main.c
> * subversion/svndumpfilter/main.c
> * subversion/svnlook/main.c
> * subversion/svnserve/main.c
> * subversion/svnsync/main.c
> * subversion/svnversion/main.c
> (SVN_UTF_ETOU_XLATE_HANDLE): Xlate key for ebcdic (CCSID 0) to utf-8
> conversions.
> (main): Convert incoming argv strings to utf-8. Note that
> svn_utf_cstring_to_utf8_ex() is used instead of
> svn_utf_cstring_to_utf8
> because V5R4 thinks native strings are in utf-8.
That note addresses an implementation detail so would be better in the code
than in the log message.
> ]]]
>
> Index: subversion/svn/main.c
> ===================================================================
> --- subversion/svn/main.c (revision 18362)
> +++ subversion/svn/main.c (working copy)
> @@ -48,6 +48,9 @@
>
> #include "svn_private_config.h"
>
> +#if AS400_UTF8
Do we already have the "configure" magic that sets this macro to true or false
on every system? If not (if it is set for AS400_UTF8 systems and unset
elsewhere) then the directive should be "ifdef". Or is it true on some
systems, false on others, and unset on others, in which case a two-level test
might be needed? Basically I don't want us using "#if" to test something that
may not be set, even though it is legal in C.
> +#define SVN_UTF_ETOU_XLATE_HANDLE "svn-utf-etou-xlate-handle"
> +#endif
> /*** Option Processing ***/
>
> @@ -780,6 +783,11 @@
> svn_cl__cmd_baton_t command_baton;
> svn_auth_baton_t *ab;
> svn_config_t *cfg;
> +#if AS400_UTF8
> + /* Even when compiled with UTF support in V5R4, argv is still
> + * encoded in ebcdic, so we'll need to convert it to utf-8. */
> + const char ** argv_utf8;
> +#endif
>
> /* Initialize the app. */
> if (svn_cmdline_init ("svn", stderr) != EXIT_SUCCESS)
> @@ -831,7 +839,20 @@
> }
>
> /* Else, parse options. */
> +#if !AS400_UTF8
> apr_getopt_init (&os, pool, argc, argv);
> +#else
> + /* Convert argv to utf-8 for call to apr_getopt_init(). */
> + argv_utf8 = apr_palloc (pool, sizeof(char *) * argc);
> + for (i = 0; i < argc; i++)
> + {
> + /* Use "0" for default job ccsid. */
I'm not sure if that comment adds anything: if "default job ccsid" is just a
description of the argument to the function then the code already says that. A
comment saying why would be of more use (e.g. "The [default] job ccsid isn't
used, it just has to look like a number").
> + if (svn_utf_cstring_to_utf8_ex (&argv_utf8[i], argv[i], "0",
> + SVN_UTF_ETOU_XLATE_HANDLE, pool))
> + return EXIT_FAILURE;
> + }
> + apr_getopt_init (&os, pool, argc, argv_utf8);
> +#endif
I see that the whole change is repeated verbatim in every "main.c" source file.
I would very much like to see both the total amount of duplication and the
number of system-specific blocks added to each file kept as small as possible.
Therefore I hope we can put this whole change inside a helper function and
reduce it to a single call, such as:
#if AS400_UTF8
SVN_INT_ERR (svn_cmdline_args_from_ebcdic (&argv, argv, pool));
#endif
to change argv before using it.
Or, even better, don't we need to convert the arguments to UTF-8 on every
system? In that case,
SVN_INT_ERR (svn_cmdline_args_to_utf8 (&argv, argv, pool));
unconditionally on every system, and remove the later native-to-UTF-8
processing that we presently do in dribs and drabs. I've sent a separate email
about that, as people interested in that change might well not be reading this
thread.
If that thread doesn't go anywhere quickly, and you want to get this change in
anyway, please could you write it as an svn_cmdline_args_from_ebcdic()
function, in subversion/libsvn_subr/cmdline.c
(subversion/include/svn_cmdline.h). Thanks. At least, that way, it we later
do it that way on all systems, the change to callers will only be a rename of
the function and a removal of the "#if"s.
Actually, that makes me wonder: if your change converts the arguments to UTF-8,
do the "native-to-utf8" routines that are called later on know that they don't
need to do anything? I suppose they do know this, because your compiler
creates an environment in which "native" equals "UTF-8".
- Julian
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Fri Feb 10 00:51:11 2006