{"thread":{"id":"29234","subject":"[RFC/PATCH] i18n of multi-line messages","startedAt":"2011-12-21T23:55:26Z","lastAt":"2011-12-23T20:54:51Z","messageCount":15,"participants":["Junio C Hamano","Ævar Arnfjörð Bjarmason","Johannes Sixt","Chris Packham","Andreas Schwab","Thomas Rast"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"181602","messageId":"7vr4zxeaz5.fsf@alter.siamese.dyndns.org","threadId":"29234","inReplyTo":null,"subject":"[RFC/PATCH] i18n of multi-line messages","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-12-21T23:55:26Z","receivedAt":"2011-12-21T23:55:26Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Advice messages are by definition meant for human end-users, and prime\ncandidates for i18n/l10n. They tend to also be more verbose to be helpful,\nand need to be longer than just one line.\n\nAlthough we do not have parameterized multi-line advice messages yet, once\nwe do, we cannot emit such a message like this:\n\n\tadvise(_(\"Please rename %s to something else\"), gostak);\n advise(_(\"so that we can avoid distimming %s unnecessarily.\"), doshes);\n\nbecause some translations may need to have the replacement of 'gostak' on\nthe second line (or 'doshes' on the first line). Some languages may even\nneed to use three lines in order to fit the same message within a\nreasonable width.\n\nInstead, it has to be a single advise() construct, like this:\n\n\tadvise(_(\"Please rename %s to something else\\n\"\n \"so that we can avoid distimming %s unnecessarily.\"),\n\t\tgostak, doshes); \n\nUpdate the advise() function and its existing callers to\n\n - take a format string that can be multi-line and translatable as a\n whole;\n - use the string and the parameters to form a localized message; and\n - append each line in the result to localization of the \"hint: \" prefix.\n\nSigned-off-by: Junio C Hamano \n---\n advice.c | 23 ++++++++++++++++-------\n builtin/revert.c | 9 ++++-----\n 2 files changed, 20 insertions(+), 12 deletions(-)\n\ndiff --git a/advice.c b/advice.c\nindex e02e632..fcdf66a 100644\n--- a/advice.c\n+++ b/advice.c\n@@ -21,11 +21,21 @@ static struct {\n \n void advise(const char *advice, ...)\n {\n+\tstruct strbuf buf = STRBUF_INIT;\n \tva_list params;\n+\tconst char *cp, *np;\n \n \tva_start(params, advice);\n-\tvreportf(\"hint: \", advice, params);\n+\tstrbuf_addf(&buf, advice, params);\n \tva_end(params);\n+\n+\tfor (cp = buf.buf; *cp; cp = np) {\n+\t\tnp = strchrnul(cp, '\\n');\n+\t\tfprintf(stderr, \"%s%.*s\\n\", _(\"hint: \"), (int)(np - cp), cp);\n+\t\tif (*np)\n+\t\t\tnp++;\n+\t}\n+\tstrbuf_release(&buf);\n }\n \n int git_default_advice_config(const char *var, const char *value)\n@@ -46,16 +56,15 @@ int git_default_advice_config(const char *var, const char *value)\n int error_resolve_conflict(const char *me)\n {\n \terror(\"'%s' is not possible because you have unmerged files.\", me);\n-\tif (advice_resolve_conflict) {\n+\tif (advice_resolve_conflict)\n \t\t/*\n \t\t * Message used both when 'git commit' fails and when\n \t\t * other commands doing a merge do.\n \t\t */\n-\t\tadvise(\"Fix them up in the work tree,\");\n-\t\tadvise(\"and then use 'git add/rm ' as\");\n-\t\tadvise(\"appropriate to mark resolution and make a commit,\");\n-\t\tadvise(\"or use 'git commit -a'.\");\n-\t}\n+\t\tadvise(_(\"Fix them up in the work tree,\\n\"\n+\t\t\t \"and then use 'git add/rm ' as\\n\"\n+\t\t\t \"appropriate to mark resolution and make a commit,\\n\"\n+\t\t\t \"or use 'git commit -a'.\"));\n \treturn -1;\n }\n \ndiff --git a/builtin/revert.c b/builtin/revert.c\nindex fce3f92..440d2be 100644\n--- a/builtin/revert.c\n+++ b/builtin/revert.c\n@@ -343,11 +343,10 @@ static void print_advice(int show_hint)\n \t\treturn;\n \t}\n \n-\tif (show_hint) {\n-\t\tadvise(\"after resolving the conflicts, mark the corrected paths\");\n-\t\tadvise(\"with 'git add ' or 'git rm '\");\n-\t\tadvise(\"and commit the result with 'git commit'\");\n-\t}\n+\tif (show_hint)\n+\t\tadvise(_(\"after resolving the conflicts, mark the corrected paths\\n\"\n+\t\t\t \"with 'git add ' or 'git rm '\\n\"\n+\t\t\t \"and commit the result with 'git commit'\"));\n }\n \n static void write_message(struct strbuf *msgbuf, const char *filename)\n"},{"id":"181605","messageId":"CACBZZX7bgjFz7mvTySPKhR24coqOeVVy8+CsKHVj8Q3LF_-5ww@mail.gmail.com","threadId":"29234","inReplyTo":"7vr4zxeaz5.fsf@alter.siamese.dyndns.org","subject":"Re: [RFC/PATCH] i18n of multi-line messages","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2011-12-22T00:14:20Z","receivedAt":"2011-12-22T00:14:20Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"On Thu, Dec 22, 2011 at 00:55, Junio C Hamano wrote:\n\nNice. Thanks for tackling this. I'll no doubt be submitting some of\nthese myself once we start getting translations.\n\nThis is not a regression, but note that something like this:\n\n> +               fprintf(stderr, \"%s%.*s\\n\", _(\"hint: \"), (int)(np - cp), cp);\n\nIsn't going to cut it for RTL languages like Hebrew and Arabic, since\nfor them the \"hint: \" would effectively be at the end of the line.\n\nI think the easiest way to tackle that sort of thing is to just do:\n\n _(\"hint: %.*s\\n\")\n\nAnd have a TRANSLATORS comment indicating that the format string\nshould be kept, but that translators can move around the \"hint\", GNU\ngettext also has a msgcheck feature to check that format strings are\ncompatible in the translations.\n"},{"id":"181606","messageId":"7vmxale9so.fsf@alter.siamese.dyndns.org","threadId":"29234","inReplyTo":"CACBZZX7bgjFz7mvTySPKhR24coqOeVVy8+CsKHVj8Q3LF_-5ww@mail.gmail.com","subject":"Re: [RFC/PATCH] i18n of multi-line messages","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-12-22T00:20:55Z","receivedAt":"2011-12-22T00:20:55Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n> On Thu, Dec 22, 2011 at 00:55, Junio C Hamano wrote:\n>\n> Nice. Thanks for tackling this. I'll no doubt be submitting some of\n> these myself once we start getting translations.\n>\n> This is not a regression, but note that something like this:\n>\n>> +               fprintf(stderr, \"%s%.*s\\n\", _(\"hint: \"), (int)(np - cp), cp);\n>\n> Isn't going to cut it for RTL languages like Hebrew and Arabic, since\n> for them the \"hint: \" would effectively be at the end of the line.\n>\n> I think the easiest way to tackle that sort of thing is to just do:\n>\n> _(\"hint: %.*s\\n\")\n>\n> And have a TRANSLATORS comment indicating that the format string\n> should be kept, but that translators can move around the \"hint\", GNU\n> gettext also has a msgcheck feature to check that format strings are\n> compatible in the translations.\n\nGood point. Thanks.\n"},{"id":"181608","messageId":"4EF2D436.3080303@viscovery.net","threadId":"29234","inReplyTo":"7vr4zxeaz5.fsf@alter.siamese.dyndns.org","subject":"Re: [RFC/PATCH] i18n of multi-line messages","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2011-12-22T06:54:46Z","receivedAt":"2011-12-22T06:54:46Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 12/22/2011 0:55, schrieb Junio C Hamano:\n> void advise(const char *advice, ...)\n> {\n> +\tstruct strbuf buf = STRBUF_INIT;\n> \tva_list params;\n> +\tconst char *cp, *np;\n> \n> \tva_start(params, advice);\n> -\tvreportf(\"hint: \", advice, params);\n> +\tstrbuf_addf(&buf, advice, params);\n> \tva_end(params);\n> +\n> +\tfor (cp = buf.buf; *cp; cp = np) {\n> +\t\tnp = strchrnul(cp, '\\n');\n> +\t\tfprintf(stderr, \"%s%.*s\\n\", _(\"hint: \"), (int)(np - cp), cp);\n> +\t\tif (*np)\n> +\t\t\tnp++;\n> +\t}\n> +\tstrbuf_release(&buf);\n> }\n\nIMHO, this logic should be moved into vreportf(), and we get proper\nprefixing of multi-line warning(), error(), and die() messages for free.\n\n> +\t\tadvise(_(\"Fix them up in the work tree,\\n\"\n> +\t\t\t \"and then use 'git add/rm ' as\\n\"\n> +\t\t\t \"appropriate to mark resolution and make a commit,\\n\"\n> +\t\t\t \"or use 'git commit -a'.\"));\n\n\nCan people please pay attention how they break multi-line messages? In\nthis particular case, (1) even in a 80-columns terminal the lines are\nspectacularly short, and (2) a break in the middle of a word group can\neasily be avoided such that the result does not look ugly:\n\nhint: Fix them up in the work tree, and then use 'git add/rm '\nhint: as appropriate to mark resolution and make a commit,\nhint: or use 'git commit -a'.\n\nAnd, no, \"It would break the 80-column limit of source code\" does not\ncount for user-visible messages.\n\n\n-- Hannes\n"},{"id":"181609","messageId":"7vipl9drba.fsf@alter.siamese.dyndns.org","threadId":"29234","inReplyTo":"4EF2D436.3080303@viscovery.net","subject":"Re: [RFC/PATCH] i18n of multi-line messages","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-12-22T07:00:09Z","receivedAt":"2011-12-22T07:00:09Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Sixt writes:\n\n>\n>> +\t\tadvise(_(\"Fix them up in the work tree,\\n\"\n>> +\t\t\t \"and then use 'git add/rm ' as\\n\"\n>> +\t\t\t \"appropriate to mark resolution and make a commit,\\n\"\n>> +\t\t\t \"or use 'git commit -a'.\"));\n>\n> \n> Can people please pay attention how they break multi-line messages? In\n> ...\n> \n\nNo need for ranting; please just make it so. The above is literal\ntranslation without changing the current output.\n"},{"id":"181610","messageId":"7vehvxdrac.fsf@alter.siamese.dyndns.org","threadId":"29234","inReplyTo":"4EF2D436.3080303@viscovery.net","subject":"Re: [RFC/PATCH] i18n of multi-line messages","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-12-22T07:00:43Z","receivedAt":"2011-12-22T07:00:43Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Sixt writes:\n\n> Am 12/22/2011 0:55, schrieb Junio C Hamano:\n>> void advise(const char *advice, ...)\n>> {\n>> +\tstruct strbuf buf = STRBUF_INIT;\n>> \tva_list params;\n>> +\tconst char *cp, *np;\n>> \n>> \tva_start(params, advice);\n>> -\tvreportf(\"hint: \", advice, params);\n>> +\tstrbuf_addf(&buf, advice, params);\n>> \tva_end(params);\n>> +\n>> +\tfor (cp = buf.buf; *cp; cp = np) {\n>> +\t\tnp = strchrnul(cp, '\\n');\n>> +\t\tfprintf(stderr, \"%s%.*s\\n\", _(\"hint: \"), (int)(np - cp), cp);\n>> +\t\tif (*np)\n>> +\t\t\tnp++;\n>> +\t}\n>> +\tstrbuf_release(&buf);\n>> }\n>\n> IMHO, this logic should be moved into vreportf(), and we get proper\n> prefixing of multi-line warning(), error(), and die() messages for free.\n\nVery very good point.\n"},{"id":"181611","messageId":"7vaa6ldpk6.fsf@alter.siamese.dyndns.org","threadId":"29234","inReplyTo":"4EF2D436.3080303@viscovery.net","subject":"Re: [RFC/PATCH] i18n of multi-line messages","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-12-22T07:38:01Z","receivedAt":"2011-12-22T07:38:01Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Sixt writes:\n\n> IMHO, this logic should be moved into vreportf(), and we get proper\n> prefixing of multi-line warning(), error(), and die() messages for free.\n\nI agree with this, so here is a rewrite to do so.\n\nTwo points to note.\n\n - Existing users of vreportf() and vwritef() are modified to pass the\n prefix like \"error\", \"warning\", etc. without colon+SP. The formatting\n convention regarding how the \"prefix\" is separated from the body of the\n message should also be locale specific.\n\n - I expect some tests will fail, as there would be existing users that\n pass multi-line strings to die(), error() and friends.\n\nFor the latter, I didn't bother to check, but I would not be surprised if\neverybody thinks the updated output that repeats the same prefix line to\nevery line is easier to read and formatted more consistently. Updating the\ntests to go with this change is left as an exercise for the reader.\n\n-- >8 --\nAdvice messages are by definition meant for human end-users, and prime\ncandidates for i18n/l10n. They tend to also be more verbose to be helpful,\nand need to be longer than just one line.\n\nAlthough we do not have parameterized multi-line advice messages yet, once\nwe do, we cannot emit such a message like this:\n\n\tadvise(_(\"Please rename %s to something else\"), gostak);\n advise(_(\"so that we can avoid distimming %s unnecessarily.\"), doshes);\n\nbecause some translations may need to have the replacement of 'gostak' on\nthe second line (or 'doshes' on the first line). Some languages may even\nneed to use three lines in order to fit the same message within a\nreasonable width.\n\nInstead, it has to be a single advise() construct, like this:\n\n\tadvise(_(\"Please rename %s to something else\\n\"\n \"so that we can avoid distimming %s unnecessarily.\"),\n\t\tgostak, doshes);\n\nUpdate the advise() function and its existing callers to\n\n - take a format string that can be multi-line and translatable as a\n whole;\n - use the string and the parameters to form a localized message; and\n - append each line in the result to localization of the \"hint: \" prefix.\n\nSigned-off-by: Junio C Hamano \n---\n advice.c | 13 +++++------\n builtin/revert.c | 9 +++----\n http-backend.c | 2 +-\n run-command.c | 2 +-\n usage.c | 63 +++++++++++++++++++++++++++++++++++++++++------------\n 5 files changed, 60 insertions(+), 29 deletions(-)\n\ndiff --git a/advice.c b/advice.c\nindex e02e632..93a03f5 100644\n--- a/advice.c\n+++ b/advice.c\n@@ -24,7 +24,7 @@ void advise(const char *advice, ...)\n \tva_list params;\n \n \tva_start(params, advice);\n-\tvreportf(\"hint: \", advice, params);\n+\tvreportf(\"hint\", advice, params);\n \tva_end(params);\n }\n \n@@ -46,16 +46,15 @@ int git_default_advice_config(const char *var, const char *value)\n int error_resolve_conflict(const char *me)\n {\n \terror(\"'%s' is not possible because you have unmerged files.\", me);\n-\tif (advice_resolve_conflict) {\n+\tif (advice_resolve_conflict)\n \t\t/*\n \t\t * Message used both when 'git commit' fails and when\n \t\t * other commands doing a merge do.\n \t\t */\n-\t\tadvise(\"Fix them up in the work tree,\");\n-\t\tadvise(\"and then use 'git add/rm ' as\");\n-\t\tadvise(\"appropriate to mark resolution and make a commit,\");\n-\t\tadvise(\"or use 'git commit -a'.\");\n-\t}\n+\t\tadvise(_(\"Fix them up in the work tree,\\n\"\n+\t\t\t \"and then use 'git add/rm ' as\\n\"\n+\t\t\t \"appropriate to mark resolution and make a commit,\\n\"\n+\t\t\t \"or use 'git commit -a'.\"));\n \treturn -1;\n }\n \ndiff --git a/builtin/revert.c b/builtin/revert.c\nindex 1ea525c..3ad14a1 100644\n--- a/builtin/revert.c\n+++ b/builtin/revert.c\n@@ -332,11 +332,10 @@ static void print_advice(int show_hint)\n \t\treturn;\n \t}\n \n-\tif (show_hint) {\n-\t\tadvise(\"after resolving the conflicts, mark the corrected paths\");\n-\t\tadvise(\"with 'git add ' or 'git rm '\");\n-\t\tadvise(\"and commit the result with 'git commit'\");\n-\t}\n+\tif (show_hint)\n+\t\tadvise(_(\"after resolving the conflicts, mark the corrected paths\\n\"\n+\t\t\t \"with 'git add ' or 'git rm '\\n\"\n+\t\t\t \"and commit the result with 'git commit'\"));\n }\n \n static void write_message(struct strbuf *msgbuf, const char *filename)\ndiff --git a/http-backend.c b/http-backend.c\nindex 59ad7da..d372252 100644\n--- a/http-backend.c\n+++ b/http-backend.c\n@@ -490,7 +490,7 @@ static NORETURN void die_webcgi(const char *err, va_list params)\n \t\thdr_nocache();\n \t\tend_headers();\n \n-\t\tvreportf(\"fatal: \", err, params);\n+\t\tvreportf(\"fatal\", err, params);\n \t}\n \texit(0); /* we successfully reported a failure ;-) */\n }\ndiff --git a/run-command.c b/run-command.c\nindex 1c51043..f7a7b5c 100644\n--- a/run-command.c\n+++ b/run-command.c\n@@ -78,7 +78,7 @@ static void notify_parent(void)\n \n static NORETURN void die_child(const char *err, va_list params)\n {\n-\tvwritef(child_err, \"fatal: \", err, params);\n+\tvwritef(child_err, \"fatal\", err, params);\n \texit(128);\n }\n \ndiff --git a/usage.c b/usage.c\nindex a2a6678..2d392a4 100644\n--- a/usage.c\n+++ b/usage.c\n@@ -6,45 +6,78 @@\n #include \"git-compat-util.h\"\n #include \"cache.h\"\n \n+typedef void (*emit_fn)(struct strbuf *, void *);\n+\n+static void v_format(const char *prefix, const char *fmt, va_list params,\n+\t\t emit_fn emit, void *cb_data)\n+{\n+\tstruct strbuf buf = STRBUF_INIT;\n+\tstruct strbuf line = STRBUF_INIT;\n+\tconst char *cp, *np;\n+\n+\tstrbuf_vaddf(&buf, fmt, params);\n+\tfor (cp = buf.buf; *cp; cp = np) {\n+\t\tnp = strchrnul(cp, '\\n');\n+\t\t/*\n+\t\t * TRANSLATORS: the format is designed so that in RTL\n+\t\t * languages you could reorder and put the \"prefix\" at\n+\t\t * the end instead of the beginning of a line if you\n+\t\t * wanted to.\n+\t\t */\n+\t\tstrbuf_addf(&line,\n+\t\t\t _(\"%s: %.*s\\n\"),\n+\t\t\t prefix,\n+\t\t\t (int)(np - cp), cp);\n+\t\temit(&line, cb_data);\n+\t\tstrbuf_reset(&line);\n+\t\tif (*np)\n+\t\t\tnp++;\n+\t}\n+\tstrbuf_release(&buf);\n+\tstrbuf_release(&line);\n+}\n+\n+static void emit_report(struct strbuf *line, void *cb_data)\n+{\n+\tfprintf(stderr, \"%.*s\", (int)line->len, line->buf);\n+}\n+\n void vreportf(const char *prefix, const char *err, va_list params)\n {\n-\tchar msg[4096];\n-\tvsnprintf(msg, sizeof(msg), err, params);\n-\tfprintf(stderr, \"%s%s\\n\", prefix, msg);\n+\tv_format(prefix, err, params, emit_report, NULL);\n }\n \n-void vwritef(int fd, const char *prefix, const char *err, va_list params)\n+static void emit_write(struct strbuf *line, void *cb_data)\n {\n-\tchar msg[4096];\n-\tint len = vsnprintf(msg, sizeof(msg), err, params);\n-\tif (len > sizeof(msg))\n-\t\tlen = sizeof(msg);\n+\tint *fd = cb_data;\n+\twrite_in_full(*fd, line->buf, line->len);\n+}\n \n-\twrite_in_full(fd, prefix, strlen(prefix));\n-\twrite_in_full(fd, msg, len);\n-\twrite_in_full(fd, \"\\n\", 1);\n+void vwritef(int fd, const char *prefix, const char *err, va_list params)\n+{\n+\tv_format(prefix, err, params, emit_write, &fd);\n }\n \n static NORETURN void usage_builtin(const char *err, va_list params)\n {\n-\tvreportf(\"usage: \", err, params);\n+\tvreportf(\"usage\", err, params);\n \texit(129);\n }\n \n static NORETURN void die_builtin(const char *err, va_list params)\n {\n-\tvreportf(\"fatal: \", err, params);\n+\tvreportf(\"fatal\", err, params);\n \texit(128);\n }\n \n static void error_builtin(const char *err, va_list params)\n {\n-\tvreportf(\"error: \", err, params);\n+\tvreportf(\"error\", err, params);\n }\n \n static void warn_builtin(const char *warn, va_list params)\n {\n-\tvreportf(\"warning: \", warn, params);\n+\tvreportf(\"warning\", warn, params);\n }\n \n /* If we are in a dlopen()ed .so write to a global variable would segfault\n-- \n1.7.8.1.389.gc5932\n"},{"id":"181612","messageId":"4EF2E824.7020509@viscovery.net","threadId":"29234","inReplyTo":"7vaa6ldpk6.fsf@alter.siamese.dyndns.org","subject":"Re: [RFC/PATCH] i18n of multi-line messages","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2011-12-22T08:19:48Z","receivedAt":"2011-12-22T08:19:48Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 12/22/2011 8:38, schrieb Junio C Hamano:\n> +static void v_format(const char *prefix, const char *fmt, va_list params,\n> +\t\t emit_fn emit, void *cb_data)\n> +{\n> +\tstruct strbuf buf = STRBUF_INIT;\n> +\tstruct strbuf line = STRBUF_INIT;\n> +\tconst char *cp, *np;\n> +\n> +\tstrbuf_vaddf(&buf, fmt, params);\n...\n> void vreportf(const char *prefix, const char *err, va_list params)\n> {\n> -\tchar msg[4096];\n> -\tvsnprintf(msg, sizeof(msg), err, params);\n> -\tfprintf(stderr, \"%s%s\\n\", prefix, msg);\n> +\tv_format(prefix, err, params, emit_report, NULL);\n> }\n\nUsing strbuf (or xmalloc for that matter) from a function that can be\ncalled from die() is a big no-no. You should keep the fixed-sized buffer.\n\n-- Hannes\n"},{"id":"181616","messageId":"4EF30907.8070101@gmail.com","threadId":"29234","inReplyTo":"7vaa6ldpk6.fsf@alter.siamese.dyndns.org","subject":"Re: [RFC/PATCH] i18n of multi-line messages","fromName":"Chris Packham","fromEmail":"judge.packham@gmail.com","sentAt":"2011-12-22T10:40:07Z","receivedAt":"2011-12-22T10:40:07Z","isPatch":true,"sender":{"key":"judge.packham@gmail.com","avatar":"https://avatars.githubusercontent.com/u/155667?v=4"},"body":"Hi Junio,\n\nOn 12/22/2011 08:38 PM, Junio C Hamano wrote:\n> +\tfor (cp = buf.buf; *cp; cp = np) {\n> +\t\tnp = strchrnul(cp, '\\n');\n> +\t\t/*\n> +\t\t * TRANSLATORS: the format is designed so that in RTL\n> +\t\t * languages you could reorder and put the \"prefix\" at\n> +\t\t * the end instead of the beginning of a line if you\n> +\t\t * wanted to.\n> +\t\t */\n> +\t\tstrbuf_addf(&line,\n> +\t\t\t _(\"%s: %.*s\\n\"),\n> +\t\t\t prefix,\n> +\t\t\t (int)(np - cp), cp);\n> +\t\temit(&line, cb_data);\n> +\t\tstrbuf_reset(&line);\n> +\t\tif (*np)\n> +\t\t\tnp++;\n> +\t}\n\nForgive my ignorance if I've missed something, but how is this going to\nwork for RTL languages? Translators can change the format string but\nthey can't change the order of parameters passed to strbuf_addf.\n"},{"id":"181617","messageId":"m2r4zw4y6j.fsf@igel.home","threadId":"29234","inReplyTo":"4EF30907.8070101@gmail.com","subject":"Re: [RFC/PATCH] i18n of multi-line messages","fromName":"Andreas Schwab","fromEmail":"schwab@linux-m68k.org","sentAt":"2011-12-22T11:56:36Z","receivedAt":"2011-12-22T11:56:36Z","isPatch":true,"sender":{"key":"schwab@linux-m68k.org","avatar":"https://avatars.githubusercontent.com/u/2175493?v=4"},"body":"Chris Packham writes:\n\n> Hi Junio,\n>\n> On 12/22/2011 08:38 PM, Junio C Hamano wrote:\n>> +\tfor (cp = buf.buf; *cp; cp = np) {\n>> +\t\tnp = strchrnul(cp, '\\n');\n>> +\t\t/*\n>> +\t\t * TRANSLATORS: the format is designed so that in RTL\n>> +\t\t * languages you could reorder and put the \"prefix\" at\n>> +\t\t * the end instead of the beginning of a line if you\n>> +\t\t * wanted to.\n>> +\t\t */\n>> +\t\tstrbuf_addf(&line,\n>> +\t\t\t _(\"%s: %.*s\\n\"),\n>> +\t\t\t prefix,\n>> +\t\t\t (int)(np - cp), cp);\n>> +\t\temit(&line, cb_data);\n>> +\t\tstrbuf_reset(&line);\n>> +\t\tif (*np)\n>> +\t\t\tnp++;\n>> +\t}\n>\n> Forgive my ignorance if I've missed something, but how is this going to\n> work for RTL languages? Translators can change the format string but\n> they can't change the order of parameters passed to strbuf_addf.\n\nTranslations can select the parameters to use with the n$ specification,\neg. \"%3$.*2$s: %$1s\\n\"\n\nAndreas.\n\n-- \nAndreas Schwab, schwab@linux-m68k.org\nGPG Key fingerprint = 58CA 54C7 6D53 942B 1756 01D3 44D5 214B 8276 4ED5\n\"And now for something completely different.\"\n"},{"id":"181619","messageId":"871urwsff6.fsf@thomas.inf.ethz.ch","threadId":"29234","inReplyTo":"7vmxale9so.fsf@alter.siamese.dyndns.org","subject":"Re: [RFC/PATCH] i18n of multi-line messages","fromName":"Thomas Rast","fromEmail":"trast@student.ethz.ch","sentAt":"2011-12-22T17:07:57Z","receivedAt":"2011-12-22T17:07:57Z","isPatch":true,"sender":{"key":"tr@thomasrast.ch","avatar":"https://avatars.githubusercontent.com/u/153510?v=4"},"body":"Junio C Hamano writes:\n\n> Ævar Arnfjörð Bjarmason writes:\n>\n>> I think the easiest way to tackle that sort of thing is to just do:\n>>\n>> _(\"hint: %.*s\\n\")\n>>\n>> And have a TRANSLATORS comment indicating that the format string\n>> should be kept, but that translators can move around the \"hint\", GNU\n>> gettext also has a msgcheck feature to check that format strings are\n>> compatible in the translations.\n>\n> Good point. Thanks.\n\nNote that your commit message in pu still says\n\n - append each line in the result to localization of the \"hint: \" prefix.\n\neven though you now fixed that to be more general.\n\n\n(It also has a very weird case of mixed indentation when I view it with\n'git show':\n\n advise(_(\"Please rename %s to something else\"), gostak);\n advise(_(\"so that we can avoid distimming %s unnecessarily.\"), doshes);\n\nApparently the first line is indented with a tab, and the second with 8\nspaces.)\n\n-- \nThomas Rast\ntrast@{inf,student}.ethz.ch\n"},{"id":"181622","messageId":"7v39cceay0.fsf@alter.siamese.dyndns.org","threadId":"29234","inReplyTo":"4EF2E824.7020509@viscovery.net","subject":"Re: [RFC/PATCH] i18n of multi-line messages","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-12-22T18:08:23Z","receivedAt":"2011-12-22T18:08:23Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Sixt writes:\n\n> Am 12/22/2011 8:38, schrieb Junio C Hamano:\n>> +static void v_format(const char *prefix, const char *fmt, va_list params,\n>> +\t\t emit_fn emit, void *cb_data)\n>> +{\n>> +\tstruct strbuf buf = STRBUF_INIT;\n>> +\tstruct strbuf line = STRBUF_INIT;\n>> +\tconst char *cp, *np;\n>> +\n>> +\tstrbuf_vaddf(&buf, fmt, params);\n> ...\n>> void vreportf(const char *prefix, const char *err, va_list params)\n>> {\n>> -\tchar msg[4096];\n>> -\tvsnprintf(msg, sizeof(msg), err, params);\n>> -\tfprintf(stderr, \"%s%s\\n\", prefix, msg);\n>> +\tv_format(prefix, err, params, emit_report, NULL);\n>> }\n>\n> Using strbuf (or xmalloc for that matter) from a function that can be\n> called from die() is a big no-no. You should keep the fixed-sized buffer.\n\nI _think_ I liked the simplicity of having the logic to\n\n - format localized message to a multi-line buffer; and\n - split the contents of that buffer into lines and add prefix in an\n i18n friendly way\n\nin vreportf(), but there are many problems in this approach, it seems. We\nmay need to limit the message length for error()/die() codepath, but we do\nnot want to be limited in others, definitely not from advise().\n\nAlso some calls to error() are meant to produce plumbing error message and\nshould never be localized. The approach to localize the prefix in vreportf()\nwill not work for this reason.\n\nI think we should start from the original \"advise-only\" way. In the longer\nterm (if somebody cares about it deeply), things can be fixed up and the\nmechanism can then be unified in the following order:\n\n (1) figure out a way to allow error() and die() tell if they are called\n to produce a plumbing message that should not be translated (multiple\n approaches are possible, ranging from adding error_plumb() function\n to marking the message format string specially);\n\n (2) update the existing error()/die() calls that are used to produce\n plumbing message and mark them as such, using the mechanism decided\n in (1);\n\n (3) Take the v_format/vreport code from my patch I am discarding with\n this message, enhance them to turn the \"prefix\" i18n part\n consitional, and use that to reimplement the mechanism (1).\n\nBut that is not for this year.\n"},{"id":"181627","messageId":"CACBZZX5Seh__OmAxGZUcSE6wwxyWa6HYTo_5m=y8SKnE4LoB=A@mail.gmail.com","threadId":"29234","inReplyTo":"7vaa6ldpk6.fsf@alter.siamese.dyndns.org","subject":"Re: [RFC/PATCH] i18n of multi-line messages","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2011-12-22T21:44:35Z","receivedAt":"2011-12-22T21:44:35Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"On Thu, Dec 22, 2011 at 08:38, Junio C Hamano wrote:\n\n[Re-formatted for clarity]\n\n> -       vreportf(\"hint: \", advice, params);\n> +       vreportf(\"hint\", advice, params);\n> -               vreportf(\"fatal: \", err, params);\n> +               vreportf(\"fatal\", err, params);\n> -       vwritef(child_err, \"fatal: \", err, params);\n> +       vwritef(child_err, \"fatal\", err, params);\n> -       vreportf(\"usage: \", err, params);\n> +       vreportf(\"usage\", err, params);\n> -       vreportf(\"fatal: \", err, params);\n> +       vreportf(\"fatal\", err, params);\n> -       vreportf(\"error: \", err, params);\n> +       vreportf(\"error\", err, params);\n> -       vreportf(\"warning: \", warn, params);\n> +       vreportf(\"warning\", warn, params);\n\nIf we do it like this these would have to have something like:\n\n vreportf(N_(\"warning\"), warn, params);\n\nFollowed by...:\n\n> +       strbuf_vaddf(&buf, fmt, params);\n> +       for (cp = buf.buf; *cp; cp = np) {\n> +               np = strchrnul(cp, '\\n');\n> +               /*\n> +                * TRANSLATORS: the format is designed so that in RTL\n> +                * languages you could reorder and put the \"prefix\" at\n> +                * the end instead of the beginning of a line if you\n> +                * wanted to.\n> +                */\n> +               strbuf_addf(&line,\n> +                           _(\"%s: %.*s\\n\"),\n> +                           prefix,\n\nChanging this to _(prefix).\n\n> +                           (int)(np - cp), cp);\n> +               emit(&line, cb_data);\n> +               strbuf_reset(&line);\n> +               if (*np)\n> +                       np++;\n> +       }\n> +       strbuf_release(&buf);\n> +       strbuf_release(&line);\n\nBut ideally to make things clear to the translators it's better to\ngive them something like this to translate:\n\n error: %s\n message\n\nInstead of just, as two separate things:\n\n error\n message\n\nBecause:\n\n 1. We might use \"error\", \"warning\", \"usage\" etc. somewhere else, and\n unless we start using the msgctxt feature of gettext we can't\n distinguish between these.\n\n 2. If you present them as two separate things the translator is\n likely to get the case wrong (e.g. translate \"error\" in the\n nominative case instead of say accusative).\n\nBut it's not a big deal, the patch looks good to me as-is with those\nN_() and _() changes. Just something to keep in mind.\n\nWe can always fixed issues like the one I'm raising later as they crop up.\n"},{"id":"181631","messageId":"4EF422D3.2050802@viscovery.net","threadId":"29234","inReplyTo":"7v39cceay0.fsf@alter.siamese.dyndns.org","subject":"Re: [RFC/PATCH] i18n of multi-line messages","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2011-12-23T06:42:27Z","receivedAt":"2011-12-23T06:42:27Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 12/22/2011 19:08, schrieb Junio C Hamano:\n> I _think_ I liked the simplicity of having the logic to\n> \n> - format localized message to a multi-line buffer; and\n> - split the contents of that buffer into lines and add prefix in an\n> i18n friendly way\n> \n> in vreportf(), but there are many problems in this approach, it seems. We\n> may need to limit the message length for error()/die() codepath, but we do\n> not want to be limited in others, definitely not from advise().\n> \n> Also some calls to error() are meant to produce plumbing error message and\n> should never be localized. The approach to localize the prefix in vreportf()\n> will not work for this reason.\n\nIMO the solution to not translate plumbing messages is to omit the\ninitialization of the gettext machinery.\n\nAnyway, here is a patch that modifies vreportf() in an i18n friendly way\n(I think). It is not necessarily meant for inclusion. Notably, the\nchanges to the test suite indicate one problem in a class of error()\nmessages: A list of file names is produced that are indented by a tab.\nBut with the \"error: \" prefix, the visible indentation shrinks to two\nspaces (the minimum possible). We may want to do something about it.\n\n--- >8 ---\nFrom: Johannes Sixt \nSubject: [PATCH] Put a prefix on all lines of multi-line hint, warning,\n error messages\n\nSigned-off-by: Johannes Sixt \n---\n advice.c | 2 +-\n git-compat-util.h | 2 +-\n http-backend.c | 2 +-\n run-command.c | 2 +-\n t/t1011-read-tree-sparse-checkout.sh | 6 ++--\n t/t1506-rev-parse-diagnosis.sh | 2 +-\n t/t7607-merge-overwrite.sh | 6 ++--\n t/t7609-merge-co-error-msgs.sh | 40 +++++++++++++++++-----------------\n usage.c | 19 +++++++++++-----\n 9 files changed, 44 insertions(+), 37 deletions(-)\n\ndiff --git a/advice.c b/advice.c\nindex e02e632..38b55b6 100644\n--- a/advice.c\n+++ b/advice.c\n@@ -24,7 +24,7 @@ void advise(const char *advice, ...)\n \tva_list params;\n \n \tva_start(params, advice);\n-\tvreportf(\"hint: \", advice, params);\n+\tvreportf(\"hint: %s\\n\", advice, params);\n \tva_end(params);\n }\n \ndiff --git a/git-compat-util.h b/git-compat-util.h\nindex 8f3972c..26b7d19 100644\n--- a/git-compat-util.h\n+++ b/git-compat-util.h\n@@ -239,7 +239,7 @@ extern char *gitbasename(char *);\n #include \"compat/bswap.h\"\n \n /* General helper functions */\n-extern void vreportf(const char *prefix, const char *err, va_list params);\n+extern void vreportf(const char *line_fmt, const char *err, va_list params);\n extern void vwritef(int fd, const char *prefix, const char *err, va_list params);\n extern NORETURN void usage(const char *err);\n extern NORETURN void usagef(const char *err, ...) __attribute__((format (printf, 1, 2)));\ndiff --git a/http-backend.c b/http-backend.c\nindex 869d515..fb91742 100644\n--- a/http-backend.c\n+++ b/http-backend.c\n@@ -490,7 +490,7 @@ static NORETURN void die_webcgi(const char *err, va_list params)\n \t\thdr_nocache();\n \t\tend_headers();\n \n-\t\tvreportf(\"fatal: \", err, params);\n+\t\tvreportf(\"fatal: %s\\n\", err, params);\n \t}\n \texit(0); /* we successfully reported a failure ;-) */\n }\ndiff --git a/run-command.c b/run-command.c\nindex 1c51043..664c215 100644\n--- a/run-command.c\n+++ b/run-command.c\n@@ -466,7 +466,7 @@ static void *run_thread(void *data)\n \n static NORETURN void die_async(const char *err, va_list params)\n {\n-\tvreportf(\"fatal: \", err, params);\n+\tvreportf(\"fatal: %s\\n\", err, params);\n \n \tif (!pthread_equal(main_thread, pthread_self())) {\n \t\tstruct async *async = pthread_getspecific(async_key);\ndiff --git a/t/t1011-read-tree-sparse-checkout.sh b/t/t1011-read-tree-sparse-checkout.sh\nindex 5c0053a..22b6a20 100755\n--- a/t/t1011-read-tree-sparse-checkout.sh\n+++ b/t/t1011-read-tree-sparse-checkout.sh\n@@ -242,9 +242,9 @@ test_expect_success 'print errors when failed to update worktree' '\n \ttest_must_fail git checkout top 2>actual &&\n \tcat >expected <<\\EOF &&\n error: The following untracked working tree files would be overwritten by checkout:\n-\tsub/added\n-\tsub/addedtoo\n-Please move or remove them before you can switch branches.\n+error: \tsub/added\n+error: \tsub/addedtoo\n+error: Please move or remove them before you can switch branches.\n Aborting\n EOF\n \ttest_cmp expected actual\ndiff --git a/t/t1506-rev-parse-diagnosis.sh b/t/t1506-rev-parse-diagnosis.sh\nindex 0843a1c..292f2fb 100755\n--- a/t/t1506-rev-parse-diagnosis.sh\n+++ b/t/t1506-rev-parse-diagnosis.sh\n@@ -11,7 +11,7 @@ test_did_you_mean ()\n \tsq=\"'\" &&\n \tcat >expected <<-EOF &&\n \tfatal: Path '$2$3' $4, but not ${5:-$sq$3$sq}.\n-\tDid you mean '$1:$2$3'${2:+ aka $sq$1:./$3$sq}?\n+\tfatal: Did you mean '$1:$2$3'${2:+ aka $sq$1:./$3$sq}?\n \tEOF\n \ttest_cmp expected error\n }\ndiff --git a/t/t7607-merge-overwrite.sh b/t/t7607-merge-overwrite.sh\nindex aa74184..7d6498d 100755\n--- a/t/t7607-merge-overwrite.sh\n+++ b/t/t7607-merge-overwrite.sh\n@@ -104,9 +104,9 @@ test_expect_success 'will not overwrite untracked subtree' '\n \n cat >expect <<\\EOF\n error: The following untracked working tree files would be overwritten by merge:\n-\tsub\n-\tsub2\n-Please move or remove them before you can merge.\n+error: \tsub\n+error: \tsub2\n+error: Please move or remove them before you can merge.\n Aborting\n EOF\n \ndiff --git a/t/t7609-merge-co-error-msgs.sh b/t/t7609-merge-co-error-msgs.sh\nindex 0e4a682..653eb7e 100755\n--- a/t/t7609-merge-co-error-msgs.sh\n+++ b/t/t7609-merge-co-error-msgs.sh\n@@ -27,11 +27,11 @@ test_expect_success 'setup' '\n \n cat >expect <<\\EOF\n error: The following untracked working tree files would be overwritten by merge:\n-\tfive\n-\tfour\n-\tthree\n-\ttwo\n-Please move or remove them before you can merge.\n+error: \tfive\n+error: \tfour\n+error: \tthree\n+error: \ttwo\n+error: Please move or remove them before you can merge.\n Aborting\n EOF\n \n@@ -50,13 +50,13 @@ test_expect_success 'untracked files overwritten by merge (fast and non-fast for\n \n cat >expect <<\\EOF\n error: Your local changes to the following files would be overwritten by merge:\n-\tfour\n-\tthree\n-\ttwo\n-Please, commit your changes or stash them before you can merge.\n+error: \tfour\n+error: \tthree\n+error: \ttwo\n+error: Please, commit your changes or stash them before you can merge.\n error: The following untracked working tree files would be overwritten by merge:\n-\tfive\n-Please move or remove them before you can merge.\n+error: \tfive\n+error: Please move or remove them before you can merge.\n Aborting\n EOF\n \n@@ -70,9 +70,9 @@ test_expect_success 'untracked files or local changes ovewritten by merge' '\n \n cat >expect <<\\EOF\n error: Your local changes to the following files would be overwritten by checkout:\n-\trep/one\n-\trep/two\n-Please, commit your changes or stash them before you can switch branches.\n+error: \trep/one\n+error: \trep/two\n+error: Please, commit your changes or stash them before you can switch branches.\n Aborting\n EOF\n \n@@ -92,9 +92,9 @@ test_expect_success 'cannot switch branches because of local changes' '\n \n cat >expect <<\\EOF\n error: Your local changes to the following files would be overwritten by checkout:\n-\trep/one\n-\trep/two\n-Please, commit your changes or stash them before you can switch branches.\n+error: \trep/one\n+error: \trep/two\n+error: Please, commit your changes or stash them before you can switch branches.\n Aborting\n EOF\n \n@@ -106,9 +106,9 @@ test_expect_success 'not uptodate file porcelain checkout error' '\n \n cat >expect <<\\EOF\n error: Updating the following directories would lose untracked files in it:\n-\trep\n-\trep2\n-\n+error: \trep\n+error: \trep2\n+error: \n Aborting\n EOF\n \ndiff --git a/usage.c b/usage.c\nindex a2a6678..1b55afd 100644\n--- a/usage.c\n+++ b/usage.c\n@@ -6,11 +6,18 @@\n #include \"git-compat-util.h\"\n #include \"cache.h\"\n \n-void vreportf(const char *prefix, const char *err, va_list params)\n+void vreportf(const char *line_fmt, const char *err, va_list params)\n {\n \tchar msg[4096];\n+\tchar *line, *end;\n+\n \tvsnprintf(msg, sizeof(msg), err, params);\n-\tfprintf(stderr, \"%s%s\\n\", prefix, msg);\n+\n+\tfor (line = msg; (end = strchr(line, '\\n')); line = end) {\n+\t\t*end++ = 0;\n+\t\tfprintf(stderr, line_fmt, line);\n+\t}\n+\tfprintf(stderr, line_fmt, line);\n }\n \n void vwritef(int fd, const char *prefix, const char *err, va_list params)\n@@ -27,24 +34,24 @@ void vwritef(int fd, const char *prefix, const char *err, va_list params)\n \n static NORETURN void usage_builtin(const char *err, va_list params)\n {\n-\tvreportf(\"usage: \", err, params);\n+\tvreportf(\"usage: %s\\n\", err, params);\n \texit(129);\n }\n \n static NORETURN void die_builtin(const char *err, va_list params)\n {\n-\tvreportf(\"fatal: \", err, params);\n+\tvreportf(\"fatal: %s\\n\", err, params);\n \texit(128);\n }\n \n static void error_builtin(const char *err, va_list params)\n {\n-\tvreportf(\"error: \", err, params);\n+\tvreportf(\"error: %s\\n\", err, params);\n }\n \n static void warn_builtin(const char *warn, va_list params)\n {\n-\tvreportf(\"warning: \", warn, params);\n+\tvreportf(\"warning: %s\\n\", warn, params);\n }\n \n /* If we are in a dlopen()ed .so write to a global variable would segfault\n-- \n1.7.8.rc0.126.gd15506\n"},{"id":"181650","messageId":"7v62h7c8kk.fsf@alter.siamese.dyndns.org","threadId":"29234","inReplyTo":"4EF422D3.2050802@viscovery.net","subject":"Re: [RFC/PATCH] i18n of multi-line messages","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-12-23T20:54:51Z","receivedAt":"2011-12-23T20:54:51Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Sixt writes:\n\n> IMO the solution to not translate plumbing messages is to omit the\n> initialization of the gettext machinery.\n\nThat's clever, and might be a good approach. I didn't think things through\nnor looked at the existing codepaths where we do that (and I won't be\nlooking at them over the holiday weekend).\n\n> Anyway, here is a patch that modifies vreportf() in an i18n friendly way\n> (I think). It is not necessarily meant for inclusion.\n\nThe test-part of the patch seems to match more or less what I tentatively\nqueued after I sent the \"convert at vreportf() level\" patch and then\ndiscarded. You seem to have missed vwritef(), by the way.\n"}]}