You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by "Peter N. Lundblad" <pe...@famlundblad.se> on 2005/07/17 21:40:54 UTC

Re: svn commit: r15343 - in trunk/subversion: libsvn_subr

On Fri, 15 Jul 2005 kou@tigris.org wrote:

> Extract NLS initialization code from svn_cmdline_init.
>
> * subversion/libsvn_subr/nls.c
>   (svn_nls_environment_init): Initialize NLS environment.
>   (svn_nls_init): Initialize NLS environment and set up
>   text domain.
>
These are new functions. YOu should say so in the log.

> Added: trunk/subversion/include/svn_nls.h
> Url: http://svn.collab.net/viewcvs/svn/trunk/subversion/include/svn_nls.h?rev=15343
> ==============================================================================
> --- (empty file)
> +++ trunk/subversion/include/svn_nls.h	Fri Jul 15 00:06:55 2005
> @@ -0,0 +1,57 @@
> +/**
> + * @copyright
> + * ====================================================================
> + * Copyright (c) 2000-2005 CollabNet.  All rights reserved.
> + *
> + * This software is licensed as described in the file COPYING, which
> + * you should have received as part of this distribution.  The terms
> + * are also available at http://subversion.tigris.org/license-1.html.
> + * If newer versions of this license are posted there, you may use a
> + * newer version instead, at your option.
> + *
> + * This software consists of voluntary contributions made by many
> + * individuals.  For exact contribution history, see the revision
> + * history and logs, available at http://subversion.tigris.org/.
> + * ====================================================================
> + * @endcopyright
> + *
> + * @file svn_nls.h
> + * @brief Support functions for NLS programs
> + */
> +
> +
> +
> +#ifndef SVN_NLS_H
> +#define SVN_NLS_H
> +
> +#ifdef __cplusplus
> +extern "C" {
> +#endif /* __cplusplus */
> +
> +/** Set up the NLS environment.
> + * Return the error @c APR_EINVAL or APR_INCOMPLETE if an

                                        ^ Missing @c.

> + * error occurs.
> + *
> + * @note This function is for bindings. You should usually
> + *       use @c svn_cmdline_init() instead of calling this

If I understand corrrectly those who has delved into the doxygen docs, you
shouldn't need @c here, since the parens will automatically create a link.

> + *       function directly. This function should be called
> + *       after initializing APR.

Missing @since New in 1.3.

> + */
> +svn_error_t *svn_nls_environment_init (void);
> +
> +/** Set up the NLS which includes NLS environment set up.

This docstring doesn't make the difference between this and the above
clear to me.

> + * Return the error @c APR_EINVAL or APR_INCOMPLETE if an

Missing @c (as above).

> + * error occurs.
> + *
> + * @note This function is for bindings. You should usually
> + *       use @c svn_cmdline_init() instead of calling this

Same about @c here too.

> + *       function directly. This function should be called
> + *       after initializing APR.

Missing @since.

> + */
> +svn_error_t *svn_nls_init (void);
> +

OK. So why do we need both these functions? Becuase the last one also
calls textdomain, which sets the default text domain to subversion. Does
this one-liner really deserve a public API? AFAICT we're relying on the
default text domain in two places in cmdline/cmdline (and I didn't find
any place in the other cmdline programs we have). Can we change those two
places from
gettext(...)
to
dgettext(PACKGE_NAME, ...)
we can stop using plain gettext and get rid of that textdomain() call.



 > +#ifdef __cplusplus
> +}
> +#endif /* __cplusplus */
> +
> +#endif /* SVN_NLS_H */
>
> Modified: trunk/subversion/libsvn_subr/cmdline.c
> Url: http://svn.collab.net/viewcvs/svn/trunk/subversion/libsvn_subr/cmdline.c?rev=15343&p1=trunk/subversion/libsvn_subr/cmdline.c&p2=trunk/subversion/libsvn_subr/cmdline.c&r1=15342&r2=15343
> ==============================================================================
> --- trunk/subversion/libsvn_subr/cmdline.c	(original)
> +++ trunk/subversion/libsvn_subr/cmdline.c	Fri Jul 15 00:06:55 2005
> @@ -161,69 +162,19 @@
>       up by APR at exit time. */
>    pool = svn_pool_create (NULL);
>    svn_utf_initialize (pool);
> -
> -#ifdef ENABLE_NLS
> -#ifdef WIN32
> +
>    {
> -    WCHAR ucs2_path[MAX_PATH];
> -    char* utf8_path;
> -    const char* internal_path;
> -    apr_pool_t* pool;
> -    apr_status_t apr_err;
> -    apr_size_t inwords, outbytes, outlength;
> -
> -    apr_pool_create (&pool, 0);
> -    /* get exe name - our locale info will be in '../share/locale' */
> -    inwords = GetModuleFileNameW (0, ucs2_path,
> -                                  sizeof (ucs2_path) / sizeof(ucs2_path[0]));
> -    if (! inwords)
> -      {
> -        /* We must be on a Win9x machine, so attempt to get an ANSI path,
> -           and convert it to Unicode. */
> -        CHAR ansi_path[MAX_PATH];
> -
> -        if (! GetModuleFileNameA (0, ansi_path, sizeof (ansi_path)))
> -          goto utf8_error;
> -
> -        inwords =
> -          MultiByteToWideChar (CP_ACP, 0, ansi_path, -1, ucs2_path,
> -                               sizeof (ucs2_path) / sizeof (ucs2_path[0]));
> -        if (! inwords)
> -          goto utf8_error;
> -      }
> -
> -    outbytes = outlength = 3 * (inwords + 1);
> -    utf8_path = apr_palloc (pool, outlength);
> -    apr_err = apr_conv_ucs2_to_utf8 (ucs2_path, &inwords,
> -                                     utf8_path, &outbytes);
> -    if (!apr_err && (inwords > 0 || outbytes == 0))
> -      apr_err = APR_INCOMPLETE;
> -    if (apr_err)
> +    svn_error_t *error = svn_nls_init();

We use err instead of error if not everywhere, at least very often.

> +    if (error)
>        {
> -       utf8_error:
>          if (error_stream)
> -          fprintf (error_stream, "Can't convert module path to UTF-8");
> +          fprintf(error_stream, "%s", error->message);

Space before paren.

> +

error->message is not guaranteed to be non-NULL.

> +        svn_error_clear(error);

Space before paren.

>          return EXIT_FAILURE;
>        }
> -
> -    utf8_path[outlength - outbytes] = '\0';
> -    internal_path = svn_path_internal_style (utf8_path, pool);
> -    /* get base path name */
> -    internal_path = svn_path_dirname (internal_path, pool);
> -    internal_path = svn_path_join (internal_path, SVN_LOCALE_RELATIVE_PATH,
> -                                   pool);
> -    bindtextdomain (PACKAGE_NAME, internal_path);
> -    apr_pool_destroy (pool);
>    }
> -#else
> -  bindtextdomain(PACKAGE_NAME, SVN_LOCALE_DIR);
> -#ifdef HAVE_BIND_TEXTDOMAIN_CODESET
> -  bind_textdomain_codeset(PACKAGE_NAME, "UTF-8");
> -#endif
> -#endif
> -  textdomain(PACKAGE_NAME);
> -#endif
> -
> +
>    return EXIT_SUCCESS;
>  }
>
>
> Added: trunk/subversion/libsvn_subr/nls.c
> Url: http://svn.collab.net/viewcvs/svn/trunk/subversion/libsvn_subr/nls.c?rev=15343
> ==============================================================================
> --- (empty file)
> +++ trunk/subversion/libsvn_subr/nls.c	Fri Jul 15 00:06:55 2005
> @@ -0,0 +1,129 @@
Regards,
//Peter

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org

Re: svn commit: r15343 - in trunk/subversion: libsvn_subr

Posted by "Peter N. Lundblad" <pe...@famlundblad.se>.
On Thu, 21 Jul 2005, Kouhei Sutou wrote:

> In <Pi...@localhost.localdomain>
> > > * subversion/libsvn_subr/cmdline.c(svn_cmdline_init),
> > >   subversion/libsvn_subr/nls.c(svn_nls_environment_init):
> >   ^ Missing *.
>
> I didn't add '*' because HACKING says that:
>
>   As an exception to the above, if you make exactly the same change in
>   several files, list all the changed files in one entry. For example:
>
>      * subversion/libsvn_ra_pigeons/twirl.c,
>        subversion/libsvn_ra_pigeons/roost.c:
>        Include svn_private_config.h.
>
> Is it OK?
>
>
Woops! You're correct. Sorry.

Regards,
//Peter

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org

Re: svn commit: r15343 - in trunk/subversion: libsvn_subr

Posted by Kouhei Sutou <ko...@cozmixng.org>.
Hi,

In <Pi...@localhost.localdomain>
  "Re: svn commit: r15343 - in trunk/subversion: libsvn_subr" on Wed, 20 Jul 2005 22:33:52 +0200 (CEST),
  "Peter N. Lundblad" <pe...@famlundblad.se> wrote:

> +1. Some log message nits below.

I committed in r15374.

> > * subversion/include/svn_nls.h:
> >   - Add '@since New in 1.3.'
> >   - Fix markups.
> >
> Say what function docs you fixed. I'd be lazier than you and just say
> "Docstring fixes" or something because we don't need that level of detail,
> but thats a matter of taste and I leave it up to you.

I used "Docstring fixes".

> > * subversion/libsvn_subr/cmdline.c(svn_cmdline_init),
> >   subversion/libsvn_subr/nls.c(svn_nls_environment_init):
>   ^ Missing *.

I didn't add '*' because HACKING says that:

  As an exception to the above, if you make exactly the same change in
  several files, list all the changed files in one entry. For example:

     * subversion/libsvn_ra_pigeons/twirl.c,
       subversion/libsvn_ra_pigeons/roost.c:
       Include svn_private_config.h.  

Is it OK?


Cheers,
--
kou

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org

Re: svn commit: r15343 - in trunk/subversion: libsvn_subr

Posted by "Peter N. Lundblad" <pe...@famlundblad.se>.
On Wed, 20 Jul 2005, Kouhei Sutou wrote:

> In <20...@cozmixng.org>
>   "Re: svn commit: r15343 - in trunk/subversion: libsvn_subr" on Tue, 19 Jul 2005 21:51:56 +0900 (JST),
>   Kouhei Sutou <ko...@cozmixng.org> wrote:
>
> > > >-    if (error == SVN_NO_ERROR)
> > > >+    if (err == SVN_NO_ERROR)
> > > >
> > > >
> > > SVN_NO_ERROR is NULL by definition. we usually test this as "if (!err)"
> >
> > Could you fix this problem and commit the patch if you
> > approve the patch?
>
> I re-made a patch. What about the new patch?
>

+1. Some log message nits below.

> [[[
> Fix documentation and coding style.
>
Add "Follow-up to r15343".

> * subversion/include/svn_nls.h:
>   - Add '@since New in 1.3.'
>   - Fix markups.
>
Say what function docs you fixed. I'd be lazier than you and just say
"Docstring fixes" or something because we don't need that level of detail,
but thats a matter of taste and I leave it up to you.


> * subversion/libsvn_subr/cmdline.c(svn_cmdline_init),

                                    ^ Missing space.
>   subversion/libsvn_subr/nls.c(svn_nls_environment_init):
  ^ Missing *.

>   - Use err instead of error as svn_error_t * variable name.
>   - Fix coding style.
>
> * subversion/libsvn_subr/nls.c(svn_nls_init): Fix coding style.
> ]]]
>
Else, it's fine.

Regards,
//Peter

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org

Re: svn commit: r15343 - in trunk/subversion: libsvn_subr

Posted by Kouhei Sutou <ko...@cozmixng.org>.
Hi,

In <20...@cozmixng.org>
  "Re: svn commit: r15343 - in trunk/subversion: libsvn_subr" on Tue, 19 Jul 2005 21:51:56 +0900 (JST),
  Kouhei Sutou <ko...@cozmixng.org> wrote:

> > >-    if (error == SVN_NO_ERROR)
> > >+    if (err == SVN_NO_ERROR)
> > >  
> > >
> > SVN_NO_ERROR is NULL by definition. we usually test this as "if (!err)"
> 
> Could you fix this problem and commit the patch if you
> approve the patch?

I re-made a patch. What about the new patch?

[[[
Fix documentation and coding style.

* subversion/include/svn_nls.h:
  - Add '@since New in 1.3.'
  - Fix markups.

* subversion/libsvn_subr/cmdline.c(svn_cmdline_init),
  subversion/libsvn_subr/nls.c(svn_nls_environment_init):
  - Use err instead of error as svn_error_t * variable name.
  - Fix coding style.

* subversion/libsvn_subr/nls.c(svn_nls_init): Fix coding style.
]]]


Thanks,
--
kou

Re: svn commit: r15343 - in trunk/subversion: libsvn_subr

Posted by Kouhei Sutou <ko...@cozmixng.org>.
Hi,

In <42...@xbc.nu>
  "Re: svn commit: r15343 - in trunk/subversion: libsvn_subr" on Tue, 19 Jul 2005 10:21:40 +0200,
  Branko Čibej <br...@xbc.nu> wrote:

> >What about the new patch?
> >  
> Looks O.K., except for one nit:
> 
> >-    if (error == SVN_NO_ERROR)
> >+    if (err == SVN_NO_ERROR)
> >  
> >
> SVN_NO_ERROR is NULL by definition. we usually test this as "if (!err)"

I see.
Could you fix this problem and commit the patch if you
approve the patch?


Thanks,
--
kou

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org


Re: svn commit: r15343 - in trunk/subversion: libsvn_subr

Posted by Branko Čibej <br...@xbc.nu>.
Kouhei Sutou wrote:

>Hi,
>
>In <42...@xbc.nu>
>  "Re: svn commit: r15343 - in trunk/subversion: libsvn_subr" on Mon, 18 Jul 2005 12:01:58 +0200,
>  Branko Čibej <br...@xbc.nu> wrote:
>
>  
>
>>>@@ -87,10 +87,10 @@
>>>          apr_err = APR_INCOMPLETE;
>>>        if (apr_err)
>>>          {
>>>-            error = svn_error_create(apr_err, NULL,
>>>-                                     _("Can't convert module path "
>>>-                                       "to UTF-8 from UCS-2: '%s'"),
>>>-                                     ucs2_path);
>>>+            err = svn_error_create(apr_err, NULL,
>>>+                                   _("Can't convert module path "
>>>+                                     "to UTF-8 from UCS-2: '%s'"),
>>>+                                   ucs2_path);
>>> 
>>>
>>>      
>>>
>>This should be svn_error_createf.
>>Another thing: this file uses the space-before-paren-in-function-call 
>>style, which you don't follow consistently. Please fix this.
>>    
>>
>
>I see.
>What about the new patch?
>  
>
Looks O.K., except for one nit:

>-    if (error == SVN_NO_ERROR)
>+    if (err == SVN_NO_ERROR)
>  
>
SVN_NO_ERROR is NULL by definition. we usually test this as "if (!err)"

-- Brane


---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org

Re: svn commit: r15343 - in trunk/subversion: libsvn_subr

Posted by Kouhei Sutou <ko...@cozmixng.org>.
Hi,

In <42...@xbc.nu>
  "Re: svn commit: r15343 - in trunk/subversion: libsvn_subr" on Mon, 18 Jul 2005 12:01:58 +0200,
  Branko Čibej <br...@xbc.nu> wrote:

> >@@ -87,10 +87,10 @@
> >           apr_err = APR_INCOMPLETE;
> >         if (apr_err)
> >           {
> >-            error = svn_error_create(apr_err, NULL,
> >-                                     _("Can't convert module path "
> >-                                       "to UTF-8 from UCS-2: '%s'"),
> >-                                     ucs2_path);
> >+            err = svn_error_create(apr_err, NULL,
> >+                                   _("Can't convert module path "
> >+                                     "to UTF-8 from UCS-2: '%s'"),
> >+                                   ucs2_path);
> >  
> >
> This should be svn_error_createf.
> Another thing: this file uses the space-before-paren-in-function-call 
> style, which you don't follow consistently. Please fix this.

I see.
What about the new patch?


Thanks,
--
kou

Re: svn commit: r15343 - in trunk/subversion: libsvn_subr

Posted by Branko Čibej <br...@xbc.nu>.
Kouhei Sutou wrote:

>@@ -87,10 +87,10 @@
>           apr_err = APR_INCOMPLETE;
>         if (apr_err)
>           {
>-            error = svn_error_create(apr_err, NULL,
>-                                     _("Can't convert module path "
>-                                       "to UTF-8 from UCS-2: '%s'"),
>-                                     ucs2_path);
>+            err = svn_error_create(apr_err, NULL,
>+                                   _("Can't convert module path "
>+                                     "to UTF-8 from UCS-2: '%s'"),
>+                                   ucs2_path);
>  
>
This should be svn_error_createf.
Another thing: this file uses the space-before-paren-in-function-call 
style, which you don't follow consistently. Please fix this.

-- Brane


---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org

Re: svn commit: r15343 - in trunk/subversion: libsvn_subr

Posted by Kouhei Sutou <ko...@cozmixng.org>.
Hi,

Peter, very thanks for your feedback.

In <Pi...@localhost.localdomain>
  "Re: svn commit: r15343 - in trunk/subversion: libsvn_subr" on Sun, 17 Jul 2005 23:40:54 +0200 (CEST),
  "Peter N. Lundblad" <pe...@famlundblad.se> wrote:

> > Extract NLS initialization code from svn_cmdline_init.
> >
> > * subversion/libsvn_subr/nls.c
> >   (svn_nls_environment_init): Initialize NLS environment.
> >   (svn_nls_init): Initialize NLS environment and set up
> >   text domain.
> >
> These are new functions. YOu should say so in the log.

Is the following new log message OK?

[[[
Extract NLS initialization code from svn_cmdline_init.

* subversion/libsvn_subr/nls.c: New file.
  (svn_nls_environment_init): New function.
  Initialize NLS environment.
  (svn_nls_init): New function.
  Initialize NLS environment and set up text domain.

* subversion/include/svn_nls.h: New file.

...
]]]


> > Added: trunk/subversion/include/svn_nls.h
...

I attached the patch applied your many comments. Could you
check this?

> > +svn_error_t *svn_nls_environment_init (void);
...
> > +svn_error_t *svn_nls_init (void);
>
> OK. So why do we need both these functions? Becuase the last one also
> calls textdomain, which sets the default text domain to subversion. Does
> this one-liner really deserve a public API? AFAICT we're relying on the
> default text domain in two places in cmdline/cmdline (and I didn't find
> any place in the other cmdline programs we have). Can we change those two
> places from
> gettext(...)
> to
> dgettext(PACKGE_NAME, ...)
> we can stop using plain gettext and get rid of that textdomain() call.

It seems that getting rid of textdomain() is good idea.

I think the changes for doing this is just only replace
gettext(...) by _(...). Because _(...) macro is defined as
dgettext(PACKAGE_NAME, ...) in
subversion/svn_private_config.h{w,.in}.


Cheers,
--
kou