# [PATCH 1/2] log.c: fix translation markings 6 messages from 2015-01-06 to 2015-01-06. Participants: Kyle J. McKay, Ramsay Jones, Andreas Schwab, Junio C Hamano. Thread: https://gitlist.dev/t/38290 ## Kyle J. McKay, 2015-01-06 10:34 Subject: [PATCH 1/2] log.c: fix translation markings Message-ID: URL: https://gitlist.dev/e/ecb18f9d6ac56da0a61c3b98f8f2236%4074d39fa044aa309eaea14b9f57fe79c ``` 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 --- 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 [] [] [[--] ...]\n") - N_(" or: git show [options] ..."), + N_("git log [] [] [[--] ...]"), + N_("git show [options] ..."), NULL }; -- 2.1.4 ``` ## Kyle J. McKay, 2015-01-06 10:34 Subject: [PATCH 2/2] gettext.h: add parentheses around N_ expansion Message-ID: URL: https://gitlist.dev/e/f67db70c63f8dcbfc58d7ffda86ff10%4074d39fa044aa309eaea14b9f57fe79c In-Reply-To: ``` 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 --- 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 ``` ## Ramsay Jones, 2015-01-06 13:24 Subject: Re: [PATCH 2/2] gettext.h: add parentheses around N_ expansion Message-ID: <54ABE205.70801@ramsay1.demon.co.uk> URL: https://gitlist.dev/e/54ABE205.70801%40ramsay1.demon.co.uk In-Reply-To: ``` 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 > --- > 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 ``` ## Kyle J. McKay, 2015-01-06 14:38 Subject: Re: [PATCH 2/2] gettext.h: add parentheses around N_ expansion Message-ID: <694DC462-40E3-4081-8616-64483FCD0D77@gmail.com> URL: https://gitlist.dev/e/694DC462-40E3-4081-8616-64483FCD0D77%40gmail.com In-Reply-To: <54ABE205.70801@ramsay1.demon.co.uk> ``` 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 >> --- >> 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 ``` ## Andreas Schwab, 2015-01-06 15:16 Subject: Re: [PATCH 2/2] gettext.h: add parentheses around N_ expansion Message-ID: <87tx04rlbw.fsf@igel.home> URL: https://gitlist.dev/e/87tx04rlbw.fsf%40igel.home In-Reply-To: <694DC462-40E3-4081-8616-64483FCD0D77@gmail.com> ``` "Kyle J. McKay" 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." ``` ## Junio C Hamano, 2015-01-06 19:29 Subject: Re: [PATCH 1/2] log.c: fix translation markings Message-ID: URL: https://gitlist.dev/e/xmqqzj9voght.fsf%40gitster.dls.corp.google.com In-Reply-To: ``` "Kyle J. McKay" 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 > --- > 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 [] [] [[--] ...]\n") > - N_(" or: git show [options] ..."), > + N_("git log [] [] [[--] ...]"), > + N_("git show [options] ..."), > NULL > }; ```