[PATCH 1/2] log.c: fix translation markings

[PATCH 1/2] log.c: fix translation markings

[Kyle J. McKay]

The parse_options API expects an array of alternative usage lines
to which it automatically ads the language-appropriate "or" when
displaying.  Each of these options is marked for translation with N_
and then later translated when gettext is called on each element
of the array.

Since the N_ macro just expands to its argument, if two N_-marked
strings appear next to each other without being separated by anything
else such as a comma, the preprocessor will join them into one string.

In that case two separate strings get marked for translation, but at
runtime they have been joined into a single string passed to gettext
which then fails to get translated because the combined string was
never marked for translation.

Fix this by properly separating the two N_ marked strings with
a comma and removing the embedded "\n" and "   or:" that are
properly supplied by the parse_options API.

Signed-off-by: Kyle J. McKay <mackyle@gmail.com>
---
 builtin/log.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/builtin/log.c b/builtin/log.c
index f2a9f015..923ffe72 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -38,8 +38,8 @@ static const char *fmt_patch_subject_prefix = "PATCH";
 static const char *fmt_pretty;
 
 static const char * const builtin_log_usage[] = {
-	N_("git log [<options>] [<revision range>] [[--] <path>...]\n")
-	N_("   or: git show [options] <object>..."),
+	N_("git log [<options>] [<revision range>] [[--] <path>...]"),
+	N_("git show [options] <object>..."),
 	NULL
 };
 
-- 
2.1.4

[PATCH 2/2] gettext.h: add parentheses around N_ expansion

[Kyle J. McKay]

The N_ macro is used to mark strings for translation without
actually translating them.  At runtime the string is expected
to be passed to the gettext API for translation.

If two N_ macro invocations appear next to each other with only
whitespace (or nothing at all) between them, the two separate
strings will be marked for translation, but the preprocessor
will then combine the strings into one and at runtime the
string passed to gettext will not match the strings that were
translated.

Avoid this by adding parentheses around the expansion of the
N_ macro so that instead of ending up with two adjacent strings
that are then combined by the preprocessor, two adjacent strings
surrounded by parentheses result instead which causes a compile
error so the mistake can be quickly found and corrected.

Signed-off-by: Kyle J. McKay <mackyle@gmail.com>
---
This patch is optional, but prevents the problem fixed by 1/2
from recurring.

 gettext.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/gettext.h b/gettext.h
index 7671d09d..d11a4139 100644
--- a/gettext.h
+++ b/gettext.h
@@ -63,6 +63,6 @@ const char *Q_(const char *msgid, const char *plu, unsigned long n)
 }
 
 /* Mark msgid for translation but do not translate it. */
-#define N_(msgid) msgid
+#define N_(msgid) (msgid)
 
 #endif
-- 
2.1.4

Re: [PATCH 2/2] gettext.h: add parentheses around N_ expansion

[Ramsay Jones]

On 06/01/15 10:34, Kyle J. McKay wrote:
> The N_ macro is used to mark strings for translation without
> actually translating them.  At runtime the string is expected
> to be passed to the gettext API for translation.
> 
> If two N_ macro invocations appear next to each other with only
> whitespace (or nothing at all) between them, the two separate
> strings will be marked for translation, but the preprocessor
> will then combine the strings into one and at runtime the
> string passed to gettext will not match the strings that were
> translated.
> 
> Avoid this by adding parentheses around the expansion of the
> N_ macro so that instead of ending up with two adjacent strings
> that are then combined by the preprocessor, two adjacent strings
> surrounded by parentheses result instead which causes a compile
> error so the mistake can be quickly found and corrected.
> 
> Signed-off-by: Kyle J. McKay <mackyle@gmail.com>
> ---
> This patch is optional, but prevents the problem fixed by 1/2
> from recurring.
> 
>  gettext.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/gettext.h b/gettext.h
> index 7671d09d..d11a4139 100644
> --- a/gettext.h
> +++ b/gettext.h
> @@ -63,6 +63,6 @@ const char *Q_(const char *msgid, const char *plu, unsigned long n)
>  }
>  
>  /* Mark msgid for translation but do not translate it. */
> -#define N_(msgid) msgid
> +#define N_(msgid) (msgid)
>  
>  #endif
> 

Hmm, see commit 642f85faa ("i18n: avoid parenthesized string as
array initializer", 07-04-2011), for a counter-point. :-P

ATB,
Ramsay Jones

Re: [PATCH 2/2] gettext.h: add parentheses around N_ expansion

[Kyle J. McKay]

On Jan 6, 2015, at 05:24, Ramsay Jones wrote:

> On 06/01/15 10:34, Kyle J. McKay wrote:
>> Avoid this by adding parentheses around the expansion of the
>> N_ macro so that instead of ending up with two adjacent strings
>> that are then combined by the preprocessor, two adjacent strings
>> surrounded by parentheses result instead which causes a compile
>> error so the mistake can be quickly found and corrected.
>>
>> Signed-off-by: Kyle J. McKay <mackyle@gmail.com>
>> ---
>> This patch is optional, but prevents the problem fixed by 1/2
>> from recurring.
>>
>> gettext.h | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/gettext.h b/gettext.h
>> index 7671d09d..d11a4139 100644
>> --- a/gettext.h
>> +++ b/gettext.h
>> @@ -63,6 +63,6 @@ const char *Q_(const char *msgid, const char  
>> *plu, unsigned long n)
>> }
>>
>> /* Mark msgid for translation but do not translate it. */
>> -#define N_(msgid) msgid
>> +#define N_(msgid) (msgid)
>>
>> #endif
>>
>
> Hmm, see commit 642f85faa ("i18n: avoid parenthesized string as
> array initializer", 07-04-2011), for a counter-point. :-P

Oh interesting.  So the bug bug fixed by 1/2 must have crept in after  
that change then.

Even clang -ansi -pedantic doesn't seem to complain about this.  And  
("a") is just as much a constant expression as "a".  Are you sure it's  
not just a tcc bug?

In any case, 642f85faa is talking about this:

   static const char ignore_error[] = ("something");

which is assigning a const char * to a const char [].

But builtin/log.c uses this:

   static const char * const builtin_log_usage[] = {("something",  
"else"};

Which is assigning a const char * to a const char * const.

Anyhow, if it breaks some existing compiler that works now, it  
shouldn't be picked up.  But it would sure be nice to have something  
to prevent a recurrence of the bug 1/2 fixes.  Perhaps the (msgid)  
version should be enabled when __GNUC__ is defined -- that should be  
compatible and enough folks use that it would catch such a problem  
early.

-Kyle

Re: [PATCH 2/2] gettext.h: add parentheses around N_ expansion

[Andreas Schwab]

"Kyle J. McKay" <mackyle@gmail.com> writes:

> Even clang -ansi -pedantic doesn't seem to complain about this.  And ("a")
> is just as much a constant expression as "a".  Are you sure it's not just
> a tcc bug?

The C standard asks for a string literal as the initializer, not an
expression.

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: [PATCH 1/2] log.c: fix translation markings

[Junio C Hamano]

"Kyle J. McKay" <mackyle@gmail.com> writes:

> Fix this by properly separating the two N_ marked strings with
> a comma ...

Sensible.

> ... and removing the embedded "\n" and "   or:" that are
> properly supplied by the parse_options API.

Wow.  I learn new things or reminded of old things every day.

Thanks.

>
> Signed-off-by: Kyle J. McKay <mackyle@gmail.com>
> ---
>  builtin/log.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/builtin/log.c b/builtin/log.c
> index f2a9f015..923ffe72 100644
> --- a/builtin/log.c
> +++ b/builtin/log.c
> @@ -38,8 +38,8 @@ static const char *fmt_patch_subject_prefix = "PATCH";
>  static const char *fmt_pretty;
>  
>  static const char * const builtin_log_usage[] = {
> -	N_("git log [<options>] [<revision range>] [[--] <path>...]\n")
> -	N_("   or: git show [options] <object>..."),
> +	N_("git log [<options>] [<revision range>] [[--] <path>...]"),
> +	N_("git show [options] <object>..."),
>  	NULL
>  };