[RFC/PATCH] i18n of multi-line messages

[RFC/PATCH] i18n of multi-line messages

[Junio C Hamano]

Advice messages are by definition meant for human end-users, and prime
candidates for i18n/l10n. They tend to also be more verbose to be helpful,
and need to be longer than just one line.

Although we do not have parameterized multi-line advice messages yet, once
we do, we cannot emit such a message like this:

	advise(_("Please rename %s to something else"), gostak);
        advise(_("so that we can avoid distimming %s unnecessarily."), doshes);

because some translations may need to have the replacement of 'gostak' on
the second line (or 'doshes' on the first line). Some languages may even
need to use three lines in order to fit the same message within a
reasonable width.

Instead, it has to be a single advise() construct, like this:

	advise(_("Please rename %s to something else\n"
                 "so that we can avoid distimming %s unnecessarily."),
		gostak, doshes); 

Update the advise() function and its existing callers to

 - take a format string that can be multi-line and translatable as a
   whole;
 - use the string and the parameters to form a localized message; and
 - append each line in the result to localization of the "hint: " prefix.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 advice.c         |   23 ++++++++++++++++-------
 builtin/revert.c |    9 ++++-----
 2 files changed, 20 insertions(+), 12 deletions(-)

diff --git a/advice.c b/advice.c
index e02e632..fcdf66a 100644
--- a/advice.c
+++ b/advice.c
@@ -21,11 +21,21 @@ static struct {
 
 void advise(const char *advice, ...)
 {
+	struct strbuf buf = STRBUF_INIT;
 	va_list params;
+	const char *cp, *np;
 
 	va_start(params, advice);
-	vreportf("hint: ", advice, params);
+	strbuf_addf(&buf, advice, params);
 	va_end(params);
+
+	for (cp = buf.buf; *cp; cp = np) {
+		np = strchrnul(cp, '\n');
+		fprintf(stderr, "%s%.*s\n", _("hint: "), (int)(np - cp), cp);
+		if (*np)
+			np++;
+	}
+	strbuf_release(&buf);
 }
 
 int git_default_advice_config(const char *var, const char *value)
@@ -46,16 +56,15 @@ int git_default_advice_config(const char *var, const char *value)
 int error_resolve_conflict(const char *me)
 {
 	error("'%s' is not possible because you have unmerged files.", me);
-	if (advice_resolve_conflict) {
+	if (advice_resolve_conflict)
 		/*
 		 * Message used both when 'git commit' fails and when
 		 * other commands doing a merge do.
 		 */
-		advise("Fix them up in the work tree,");
-		advise("and then use 'git add/rm <file>' as");
-		advise("appropriate to mark resolution and make a commit,");
-		advise("or use 'git commit -a'.");
-	}
+		advise(_("Fix them up in the work tree,\n"
+			 "and then use 'git add/rm <file>' as\n"
+			 "appropriate to mark resolution and make a commit,\n"
+			 "or use 'git commit -a'."));
 	return -1;
 }
 
diff --git a/builtin/revert.c b/builtin/revert.c
index fce3f92..440d2be 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -343,11 +343,10 @@ static void print_advice(int show_hint)
 		return;
 	}
 
-	if (show_hint) {
-		advise("after resolving the conflicts, mark the corrected paths");
-		advise("with 'git add <paths>' or 'git rm <paths>'");
-		advise("and commit the result with 'git commit'");
-	}
+	if (show_hint)
+		advise(_("after resolving the conflicts, mark the corrected paths\n"
+			 "with 'git add <paths>' or 'git rm <paths>'\n"
+			 "and commit the result with 'git commit'"));
 }
 
 static void write_message(struct strbuf *msgbuf, const char *filename)

Re: [RFC/PATCH] i18n of multi-line messages

[Ævar Arnfjörð Bjarmason]

On Thu, Dec 22, 2011 at 00:55, Junio C Hamano <gitster@pobox.com> wrote:

Nice. Thanks for tackling this. I'll no doubt be submitting some of
these myself once we start getting translations.

This is not a regression, but note that something like this:

> +               fprintf(stderr, "%s%.*s\n", _("hint: "), (int)(np - cp), cp);

Isn't going to cut it for RTL languages like Hebrew and Arabic, since
for them the "hint: " would effectively be at the end of the line.

I think the easiest way to tackle that sort of thing is to just do:

    _("hint: %.*s\n")

And have a TRANSLATORS comment indicating that the format string
should be kept, but that translators can move around the "hint", GNU
gettext also has a msgcheck feature to check that format strings are
compatible in the translations.

Re: [RFC/PATCH] i18n of multi-line messages

[Junio C Hamano]

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

> On Thu, Dec 22, 2011 at 00:55, Junio C Hamano <gitster@pobox.com> wrote:
>
> Nice. Thanks for tackling this. I'll no doubt be submitting some of
> these myself once we start getting translations.
>
> This is not a regression, but note that something like this:
>
>> +               fprintf(stderr, "%s%.*s\n", _("hint: "), (int)(np - cp), cp);
>
> Isn't going to cut it for RTL languages like Hebrew and Arabic, since
> for them the "hint: " would effectively be at the end of the line.
>
> I think the easiest way to tackle that sort of thing is to just do:
>
>     _("hint: %.*s\n")
>
> And have a TRANSLATORS comment indicating that the format string
> should be kept, but that translators can move around the "hint", GNU
> gettext also has a msgcheck feature to check that format strings are
> compatible in the translations.

Good point. Thanks.

Re: [RFC/PATCH] i18n of multi-line messages

[Thomas Rast]

Junio C Hamano <gitster@pobox.com> writes:

> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
>
>> I think the easiest way to tackle that sort of thing is to just do:
>>
>>     _("hint: %.*s\n")
>>
>> And have a TRANSLATORS comment indicating that the format string
>> should be kept, but that translators can move around the "hint", GNU
>> gettext also has a msgcheck feature to check that format strings are
>> compatible in the translations.
>
> Good point. Thanks.

Note that your commit message in pu still says

     - append each line in the result to localization of the "hint: " prefix.

even though you now fixed that to be more general.


(It also has a very weird case of mixed indentation when I view it with
'git show':

        advise(_("Please rename %s to something else"), gostak);
            advise(_("so that we can avoid distimming %s unnecessarily."), doshes);

Apparently the first line is indented with a tab, and the second with 8
spaces.)

-- 
Thomas Rast
trast@{inf,student}.ethz.ch

Re: [RFC/PATCH] i18n of multi-line messages

[Johannes Sixt]

Am 12/22/2011 0:55, schrieb Junio C Hamano:
>  void advise(const char *advice, ...)
>  {
> +	struct strbuf buf = STRBUF_INIT;
>  	va_list params;
> +	const char *cp, *np;
>  
>  	va_start(params, advice);
> -	vreportf("hint: ", advice, params);
> +	strbuf_addf(&buf, advice, params);
>  	va_end(params);
> +
> +	for (cp = buf.buf; *cp; cp = np) {
> +		np = strchrnul(cp, '\n');
> +		fprintf(stderr, "%s%.*s\n", _("hint: "), (int)(np - cp), cp);
> +		if (*np)
> +			np++;
> +	}
> +	strbuf_release(&buf);
>  }

IMHO, this logic should be moved into vreportf(), and we get proper
prefixing of multi-line warning(), error(), and die() messages for free.

> +		advise(_("Fix them up in the work tree,\n"
> +			 "and then use 'git add/rm <file>' as\n"
> +			 "appropriate to mark resolution and make a commit,\n"
> +			 "or use 'git commit -a'."));

<rant>
Can people please pay attention how they break multi-line messages? In
this particular case, (1) even in a 80-columns terminal the lines are
spectacularly short, and (2) a break in the middle of a word group can
easily be avoided such that the result does not look ugly:

hint: Fix them up in the work tree, and then use 'git add/rm <file>'
hint: as appropriate to mark resolution and make a commit,
hint: or use 'git commit -a'.

And, no, "It would break the 80-column limit of source code" does not
count for user-visible messages.
</rant>

-- Hannes

Re: [RFC/PATCH] i18n of multi-line messages

[Junio C Hamano]

Johannes Sixt <j.sixt@viscovery.net> writes:

>
>> +		advise(_("Fix them up in the work tree,\n"
>> +			 "and then use 'git add/rm <file>' as\n"
>> +			 "appropriate to mark resolution and make a commit,\n"
>> +			 "or use 'git commit -a'."));
>
> <rant>
> Can people please pay attention how they break multi-line messages? In
> ...
> </rant>

No need for ranting; please just make it so. The above is literal
translation without changing the current output.

Re: [RFC/PATCH] i18n of multi-line messages

[Junio C Hamano]

Johannes Sixt <j.sixt@viscovery.net> writes:

> Am 12/22/2011 0:55, schrieb Junio C Hamano:
>>  void advise(const char *advice, ...)
>>  {
>> +	struct strbuf buf = STRBUF_INIT;
>>  	va_list params;
>> +	const char *cp, *np;
>>  
>>  	va_start(params, advice);
>> -	vreportf("hint: ", advice, params);
>> +	strbuf_addf(&buf, advice, params);
>>  	va_end(params);
>> +
>> +	for (cp = buf.buf; *cp; cp = np) {
>> +		np = strchrnul(cp, '\n');
>> +		fprintf(stderr, "%s%.*s\n", _("hint: "), (int)(np - cp), cp);
>> +		if (*np)
>> +			np++;
>> +	}
>> +	strbuf_release(&buf);
>>  }
>
> IMHO, this logic should be moved into vreportf(), and we get proper
> prefixing of multi-line warning(), error(), and die() messages for free.

Very very good point.

Re: [RFC/PATCH] i18n of multi-line messages

[Junio C Hamano]

Johannes Sixt <j.sixt@viscovery.net> writes:

> IMHO, this logic should be moved into vreportf(), and we get proper
> prefixing of multi-line warning(), error(), and die() messages for free.

I agree with this, so here is a rewrite to do so.

Two points to note.

 - Existing users of vreportf() and vwritef() are modified to pass the
   prefix like "error", "warning", etc. without colon+SP. The formatting
   convention regarding how the "prefix" is separated from the body of the
   message should also be locale specific.

 - I expect some tests will fail, as there would be existing users that
   pass multi-line strings to die(), error() and friends.

For the latter, I didn't bother to check, but I would not be surprised if
everybody thinks the updated output that repeats the same prefix line to
every line is easier to read and formatted more consistently. Updating the
tests to go with this change is left as an exercise for the reader.

-- >8 --
Advice messages are by definition meant for human end-users, and prime
candidates for i18n/l10n. They tend to also be more verbose to be helpful,
and need to be longer than just one line.

Although we do not have parameterized multi-line advice messages yet, once
we do, we cannot emit such a message like this:

	advise(_("Please rename %s to something else"), gostak);
        advise(_("so that we can avoid distimming %s unnecessarily."), doshes);

because some translations may need to have the replacement of 'gostak' on
the second line (or 'doshes' on the first line). Some languages may even
need to use three lines in order to fit the same message within a
reasonable width.

Instead, it has to be a single advise() construct, like this:

	advise(_("Please rename %s to something else\n"
                 "so that we can avoid distimming %s unnecessarily."),
		gostak, doshes);

Update the advise() function and its existing callers to

 - take a format string that can be multi-line and translatable as a
   whole;
 - use the string and the parameters to form a localized message; and
 - append each line in the result to localization of the "hint: " prefix.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 advice.c         |   13 +++++------
 builtin/revert.c |    9 +++----
 http-backend.c   |    2 +-
 run-command.c    |    2 +-
 usage.c          |   63 +++++++++++++++++++++++++++++++++++++++++------------
 5 files changed, 60 insertions(+), 29 deletions(-)

diff --git a/advice.c b/advice.c
index e02e632..93a03f5 100644
--- a/advice.c
+++ b/advice.c
@@ -24,7 +24,7 @@ void advise(const char *advice, ...)
 	va_list params;
 
 	va_start(params, advice);
-	vreportf("hint: ", advice, params);
+	vreportf("hint", advice, params);
 	va_end(params);
 }
 
@@ -46,16 +46,15 @@ int git_default_advice_config(const char *var, const char *value)
 int error_resolve_conflict(const char *me)
 {
 	error("'%s' is not possible because you have unmerged files.", me);
-	if (advice_resolve_conflict) {
+	if (advice_resolve_conflict)
 		/*
 		 * Message used both when 'git commit' fails and when
 		 * other commands doing a merge do.
 		 */
-		advise("Fix them up in the work tree,");
-		advise("and then use 'git add/rm <file>' as");
-		advise("appropriate to mark resolution and make a commit,");
-		advise("or use 'git commit -a'.");
-	}
+		advise(_("Fix them up in the work tree,\n"
+			 "and then use 'git add/rm <file>' as\n"
+			 "appropriate to mark resolution and make a commit,\n"
+			 "or use 'git commit -a'."));
 	return -1;
 }
 
diff --git a/builtin/revert.c b/builtin/revert.c
index 1ea525c..3ad14a1 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -332,11 +332,10 @@ static void print_advice(int show_hint)
 		return;
 	}
 
-	if (show_hint) {
-		advise("after resolving the conflicts, mark the corrected paths");
-		advise("with 'git add <paths>' or 'git rm <paths>'");
-		advise("and commit the result with 'git commit'");
-	}
+	if (show_hint)
+		advise(_("after resolving the conflicts, mark the corrected paths\n"
+			 "with 'git add <paths>' or 'git rm <paths>'\n"
+			 "and commit the result with 'git commit'"));
 }
 
 static void write_message(struct strbuf *msgbuf, const char *filename)
diff --git a/http-backend.c b/http-backend.c
index 59ad7da..d372252 100644
--- a/http-backend.c
+++ b/http-backend.c
@@ -490,7 +490,7 @@ static NORETURN void die_webcgi(const char *err, va_list params)
 		hdr_nocache();
 		end_headers();
 
-		vreportf("fatal: ", err, params);
+		vreportf("fatal", err, params);
 	}
 	exit(0); /* we successfully reported a failure ;-) */
 }
diff --git a/run-command.c b/run-command.c
index 1c51043..f7a7b5c 100644
--- a/run-command.c
+++ b/run-command.c
@@ -78,7 +78,7 @@ static void notify_parent(void)
 
 static NORETURN void die_child(const char *err, va_list params)
 {
-	vwritef(child_err, "fatal: ", err, params);
+	vwritef(child_err, "fatal", err, params);
 	exit(128);
 }
 
diff --git a/usage.c b/usage.c
index a2a6678..2d392a4 100644
--- a/usage.c
+++ b/usage.c
@@ -6,45 +6,78 @@
 #include "git-compat-util.h"
 #include "cache.h"
 
+typedef void (*emit_fn)(struct strbuf *, void *);
+
+static void v_format(const char *prefix, const char *fmt, va_list params,
+		     emit_fn emit, void *cb_data)
+{
+	struct strbuf buf = STRBUF_INIT;
+	struct strbuf line = STRBUF_INIT;
+	const char *cp, *np;
+
+	strbuf_vaddf(&buf, fmt, params);
+	for (cp = buf.buf; *cp; cp = np) {
+		np = strchrnul(cp, '\n');
+		/*
+		 * TRANSLATORS: the format is designed so that in RTL
+		 * languages you could reorder and put the "prefix" at
+		 * the end instead of the beginning of a line if you
+		 * wanted to.
+		 */
+		strbuf_addf(&line,
+			    _("%s: %.*s\n"),
+			    prefix,
+			    (int)(np - cp), cp);
+		emit(&line, cb_data);
+		strbuf_reset(&line);
+		if (*np)
+			np++;
+	}
+	strbuf_release(&buf);
+	strbuf_release(&line);
+}
+
+static void emit_report(struct strbuf *line, void *cb_data)
+{
+	fprintf(stderr, "%.*s", (int)line->len, line->buf);
+}
+
 void vreportf(const char *prefix, const char *err, va_list params)
 {
-	char msg[4096];
-	vsnprintf(msg, sizeof(msg), err, params);
-	fprintf(stderr, "%s%s\n", prefix, msg);
+	v_format(prefix, err, params, emit_report, NULL);
 }
 
-void vwritef(int fd, const char *prefix, const char *err, va_list params)
+static void emit_write(struct strbuf *line, void *cb_data)
 {
-	char msg[4096];
-	int len = vsnprintf(msg, sizeof(msg), err, params);
-	if (len > sizeof(msg))
-		len = sizeof(msg);
+	int *fd = cb_data;
+	write_in_full(*fd, line->buf, line->len);
+}
 
-	write_in_full(fd, prefix, strlen(prefix));
-	write_in_full(fd, msg, len);
-	write_in_full(fd, "\n", 1);
+void vwritef(int fd, const char *prefix, const char *err, va_list params)
+{
+	v_format(prefix, err, params, emit_write, &fd);
 }
 
 static NORETURN void usage_builtin(const char *err, va_list params)
 {
-	vreportf("usage: ", err, params);
+	vreportf("usage", err, params);
 	exit(129);
 }
 
 static NORETURN void die_builtin(const char *err, va_list params)
 {
-	vreportf("fatal: ", err, params);
+	vreportf("fatal", err, params);
 	exit(128);
 }
 
 static void error_builtin(const char *err, va_list params)
 {
-	vreportf("error: ", err, params);
+	vreportf("error", err, params);
 }
 
 static void warn_builtin(const char *warn, va_list params)
 {
-	vreportf("warning: ", warn, params);
+	vreportf("warning", warn, params);
 }
 
 /* If we are in a dlopen()ed .so write to a global variable would segfault
-- 
1.7.8.1.389.gc5932

Re: [RFC/PATCH] i18n of multi-line messages

[Johannes Sixt]

Am 12/22/2011 8:38, schrieb Junio C Hamano:
> +static void v_format(const char *prefix, const char *fmt, va_list params,
> +		     emit_fn emit, void *cb_data)
> +{
> +	struct strbuf buf = STRBUF_INIT;
> +	struct strbuf line = STRBUF_INIT;
> +	const char *cp, *np;
> +
> +	strbuf_vaddf(&buf, fmt, params);
...
>  void vreportf(const char *prefix, const char *err, va_list params)
>  {
> -	char msg[4096];
> -	vsnprintf(msg, sizeof(msg), err, params);
> -	fprintf(stderr, "%s%s\n", prefix, msg);
> +	v_format(prefix, err, params, emit_report, NULL);
>  }

Using strbuf (or xmalloc for that matter) from a function that can be
called from die() is a big no-no. You should keep the fixed-sized buffer.

-- Hannes

Re: [RFC/PATCH] i18n of multi-line messages

[Junio C Hamano]

Johannes Sixt <j.sixt@viscovery.net> writes:

> Am 12/22/2011 8:38, schrieb Junio C Hamano:
>> +static void v_format(const char *prefix, const char *fmt, va_list params,
>> +		     emit_fn emit, void *cb_data)
>> +{
>> +	struct strbuf buf = STRBUF_INIT;
>> +	struct strbuf line = STRBUF_INIT;
>> +	const char *cp, *np;
>> +
>> +	strbuf_vaddf(&buf, fmt, params);
> ...
>>  void vreportf(const char *prefix, const char *err, va_list params)
>>  {
>> -	char msg[4096];
>> -	vsnprintf(msg, sizeof(msg), err, params);
>> -	fprintf(stderr, "%s%s\n", prefix, msg);
>> +	v_format(prefix, err, params, emit_report, NULL);
>>  }
>
> Using strbuf (or xmalloc for that matter) from a function that can be
> called from die() is a big no-no. You should keep the fixed-sized buffer.

I _think_ I liked the simplicity of having the logic to

 - format localized message to a multi-line buffer; and
 - split the contents of that buffer into lines and add prefix in an
   i18n friendly way

in vreportf(), but there are many problems in this approach, it seems.  We
may need to limit the message length for error()/die() codepath, but we do
not want to be limited in others, definitely not from advise().

Also some calls to error() are meant to produce plumbing error message and
should never be localized. The approach to localize the prefix in vreportf()
will not work for this reason.

I think we should start from the original "advise-only" way. In the longer
term (if somebody cares about it deeply), things can be fixed up and the
mechanism can then be unified in the following order:

 (1) figure out a way to allow error() and die() tell if they are called
     to produce a plumbing message that should not be translated (multiple
     approaches are possible, ranging from adding error_plumb() function
     to marking the message format string specially);

 (2) update the existing error()/die() calls that are used to produce
     plumbing message and mark them as such, using the mechanism decided
     in (1);

 (3) Take the v_format/vreport code from my patch I am discarding with
     this message, enhance them to turn the "prefix" i18n part
     consitional, and use that to reimplement the mechanism (1).

But that is not for this year.

Re: [RFC/PATCH] i18n of multi-line messages

[Johannes Sixt]

Am 12/22/2011 19:08, schrieb Junio C Hamano:
> I _think_ I liked the simplicity of having the logic to
> 
>  - format localized message to a multi-line buffer; and
>  - split the contents of that buffer into lines and add prefix in an
>    i18n friendly way
> 
> in vreportf(), but there are many problems in this approach, it seems.  We
> may need to limit the message length for error()/die() codepath, but we do
> not want to be limited in others, definitely not from advise().
> 
> Also some calls to error() are meant to produce plumbing error message and
> should never be localized. The approach to localize the prefix in vreportf()
> will not work for this reason.

IMO the solution to not translate plumbing messages is to omit the
initialization of the gettext machinery.

Anyway, here is a patch that modifies vreportf() in an i18n friendly way
(I think). It is not necessarily meant for inclusion. Notably, the
changes to the test suite indicate one problem in a class of error()
messages: A list of file names is produced that are indented by a tab.
But with the "error: " prefix, the visible indentation shrinks to two
spaces (the minimum possible). We may want to do something about it.

--- >8 ---
From: Johannes Sixt <j6t@kdbg.org>
Subject: [PATCH] Put a prefix on all lines of multi-line hint, warning,
 error messages

Signed-off-by: Johannes Sixt <j6t@kdbg.org>
---
 advice.c                             |    2 +-
 git-compat-util.h                    |    2 +-
 http-backend.c                       |    2 +-
 run-command.c                        |    2 +-
 t/t1011-read-tree-sparse-checkout.sh |    6 ++--
 t/t1506-rev-parse-diagnosis.sh       |    2 +-
 t/t7607-merge-overwrite.sh           |    6 ++--
 t/t7609-merge-co-error-msgs.sh       |   40 +++++++++++++++++-----------------
 usage.c                              |   19 +++++++++++-----
 9 files changed, 44 insertions(+), 37 deletions(-)

diff --git a/advice.c b/advice.c
index e02e632..38b55b6 100644
--- a/advice.c
+++ b/advice.c
@@ -24,7 +24,7 @@ void advise(const char *advice, ...)
 	va_list params;
 
 	va_start(params, advice);
-	vreportf("hint: ", advice, params);
+	vreportf("hint: %s\n", advice, params);
 	va_end(params);
 }
 
diff --git a/git-compat-util.h b/git-compat-util.h
index 8f3972c..26b7d19 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -239,7 +239,7 @@ extern char *gitbasename(char *);
 #include "compat/bswap.h"
 
 /* General helper functions */
-extern void vreportf(const char *prefix, const char *err, va_list params);
+extern void vreportf(const char *line_fmt, const char *err, va_list params);
 extern void vwritef(int fd, const char *prefix, const char *err, va_list params);
 extern NORETURN void usage(const char *err);
 extern NORETURN void usagef(const char *err, ...) __attribute__((format (printf, 1, 2)));
diff --git a/http-backend.c b/http-backend.c
index 869d515..fb91742 100644
--- a/http-backend.c
+++ b/http-backend.c
@@ -490,7 +490,7 @@ static NORETURN void die_webcgi(const char *err, va_list params)
 		hdr_nocache();
 		end_headers();
 
-		vreportf("fatal: ", err, params);
+		vreportf("fatal: %s\n", err, params);
 	}
 	exit(0); /* we successfully reported a failure ;-) */
 }
diff --git a/run-command.c b/run-command.c
index 1c51043..664c215 100644
--- a/run-command.c
+++ b/run-command.c
@@ -466,7 +466,7 @@ static void *run_thread(void *data)
 
 static NORETURN void die_async(const char *err, va_list params)
 {
-	vreportf("fatal: ", err, params);
+	vreportf("fatal: %s\n", err, params);
 
 	if (!pthread_equal(main_thread, pthread_self())) {
 		struct async *async = pthread_getspecific(async_key);
diff --git a/t/t1011-read-tree-sparse-checkout.sh b/t/t1011-read-tree-sparse-checkout.sh
index 5c0053a..22b6a20 100755
--- a/t/t1011-read-tree-sparse-checkout.sh
+++ b/t/t1011-read-tree-sparse-checkout.sh
@@ -242,9 +242,9 @@ test_expect_success 'print errors when failed to update worktree' '
 	test_must_fail git checkout top 2>actual &&
 	cat >expected <<\EOF &&
 error: The following untracked working tree files would be overwritten by checkout:
-	sub/added
-	sub/addedtoo
-Please move or remove them before you can switch branches.
+error: 	sub/added
+error: 	sub/addedtoo
+error: Please move or remove them before you can switch branches.
 Aborting
 EOF
 	test_cmp expected actual
diff --git a/t/t1506-rev-parse-diagnosis.sh b/t/t1506-rev-parse-diagnosis.sh
index 0843a1c..292f2fb 100755
--- a/t/t1506-rev-parse-diagnosis.sh
+++ b/t/t1506-rev-parse-diagnosis.sh
@@ -11,7 +11,7 @@ test_did_you_mean ()
 	sq="'" &&
 	cat >expected <<-EOF &&
 	fatal: Path '$2$3' $4, but not ${5:-$sq$3$sq}.
-	Did you mean '$1:$2$3'${2:+ aka $sq$1:./$3$sq}?
+	fatal: Did you mean '$1:$2$3'${2:+ aka $sq$1:./$3$sq}?
 	EOF
 	test_cmp expected error
 }
diff --git a/t/t7607-merge-overwrite.sh b/t/t7607-merge-overwrite.sh
index aa74184..7d6498d 100755
--- a/t/t7607-merge-overwrite.sh
+++ b/t/t7607-merge-overwrite.sh
@@ -104,9 +104,9 @@ test_expect_success 'will not overwrite untracked subtree' '
 
 cat >expect <<\EOF
 error: The following untracked working tree files would be overwritten by merge:
-	sub
-	sub2
-Please move or remove them before you can merge.
+error: 	sub
+error: 	sub2
+error: Please move or remove them before you can merge.
 Aborting
 EOF
 
diff --git a/t/t7609-merge-co-error-msgs.sh b/t/t7609-merge-co-error-msgs.sh
index 0e4a682..653eb7e 100755
--- a/t/t7609-merge-co-error-msgs.sh
+++ b/t/t7609-merge-co-error-msgs.sh
@@ -27,11 +27,11 @@ test_expect_success 'setup' '
 
 cat >expect <<\EOF
 error: The following untracked working tree files would be overwritten by merge:
-	five
-	four
-	three
-	two
-Please move or remove them before you can merge.
+error: 	five
+error: 	four
+error: 	three
+error: 	two
+error: Please move or remove them before you can merge.
 Aborting
 EOF
 
@@ -50,13 +50,13 @@ test_expect_success 'untracked files overwritten by merge (fast and non-fast for
 
 cat >expect <<\EOF
 error: Your local changes to the following files would be overwritten by merge:
-	four
-	three
-	two
-Please, commit your changes or stash them before you can merge.
+error: 	four
+error: 	three
+error: 	two
+error: Please, commit your changes or stash them before you can merge.
 error: The following untracked working tree files would be overwritten by merge:
-	five
-Please move or remove them before you can merge.
+error: 	five
+error: Please move or remove them before you can merge.
 Aborting
 EOF
 
@@ -70,9 +70,9 @@ test_expect_success 'untracked files or local changes ovewritten by merge' '
 
 cat >expect <<\EOF
 error: Your local changes to the following files would be overwritten by checkout:
-	rep/one
-	rep/two
-Please, commit your changes or stash them before you can switch branches.
+error: 	rep/one
+error: 	rep/two
+error: Please, commit your changes or stash them before you can switch branches.
 Aborting
 EOF
 
@@ -92,9 +92,9 @@ test_expect_success 'cannot switch branches because of local changes' '
 
 cat >expect <<\EOF
 error: Your local changes to the following files would be overwritten by checkout:
-	rep/one
-	rep/two
-Please, commit your changes or stash them before you can switch branches.
+error: 	rep/one
+error: 	rep/two
+error: Please, commit your changes or stash them before you can switch branches.
 Aborting
 EOF
 
@@ -106,9 +106,9 @@ test_expect_success 'not uptodate file porcelain checkout error' '
 
 cat >expect <<\EOF
 error: Updating the following directories would lose untracked files in it:
-	rep
-	rep2
-
+error: 	rep
+error: 	rep2
+error: 
 Aborting
 EOF
 
diff --git a/usage.c b/usage.c
index a2a6678..1b55afd 100644
--- a/usage.c
+++ b/usage.c
@@ -6,11 +6,18 @@
 #include "git-compat-util.h"
 #include "cache.h"
 
-void vreportf(const char *prefix, const char *err, va_list params)
+void vreportf(const char *line_fmt, const char *err, va_list params)
 {
 	char msg[4096];
+	char *line, *end;
+
 	vsnprintf(msg, sizeof(msg), err, params);
-	fprintf(stderr, "%s%s\n", prefix, msg);
+
+	for (line = msg; (end = strchr(line, '\n')); line = end) {
+		*end++ = 0;
+		fprintf(stderr, line_fmt, line);
+	}
+	fprintf(stderr, line_fmt, line);
 }
 
 void vwritef(int fd, const char *prefix, const char *err, va_list params)
@@ -27,24 +34,24 @@ void vwritef(int fd, const char *prefix, const char *err, va_list params)
 
 static NORETURN void usage_builtin(const char *err, va_list params)
 {
-	vreportf("usage: ", err, params);
+	vreportf("usage: %s\n", err, params);
 	exit(129);
 }
 
 static NORETURN void die_builtin(const char *err, va_list params)
 {
-	vreportf("fatal: ", err, params);
+	vreportf("fatal: %s\n", err, params);
 	exit(128);
 }
 
 static void error_builtin(const char *err, va_list params)
 {
-	vreportf("error: ", err, params);
+	vreportf("error: %s\n", err, params);
 }
 
 static void warn_builtin(const char *warn, va_list params)
 {
-	vreportf("warning: ", warn, params);
+	vreportf("warning: %s\n", warn, params);
 }
 
 /* If we are in a dlopen()ed .so write to a global variable would segfault
-- 
1.7.8.rc0.126.gd15506

Re: [RFC/PATCH] i18n of multi-line messages

[Junio C Hamano]

Johannes Sixt <j.sixt@viscovery.net> writes:

> IMO the solution to not translate plumbing messages is to omit the
> initialization of the gettext machinery.

That's clever, and might be a good approach. I didn't think things through
nor looked at the existing codepaths where we do that (and I won't be
looking at them over the holiday weekend).

> Anyway, here is a patch that modifies vreportf() in an i18n friendly way
> (I think). It is not necessarily meant for inclusion.

The test-part of the patch seems to match more or less what I tentatively
queued after I sent the "convert at vreportf() level" patch and then
discarded. You seem to have missed vwritef(), by the way.

Re: [RFC/PATCH] i18n of multi-line messages

[Chris Packham]

Hi Junio,

On 12/22/2011 08:38 PM, Junio C Hamano wrote:
> +	for (cp = buf.buf; *cp; cp = np) {
> +		np = strchrnul(cp, '\n');
> +		/*
> +		 * TRANSLATORS: the format is designed so that in RTL
> +		 * languages you could reorder and put the "prefix" at
> +		 * the end instead of the beginning of a line if you
> +		 * wanted to.
> +		 */
> +		strbuf_addf(&line,
> +			    _("%s: %.*s\n"),
> +			    prefix,
> +			    (int)(np - cp), cp);
> +		emit(&line, cb_data);
> +		strbuf_reset(&line);
> +		if (*np)
> +			np++;
> +	}

Forgive my ignorance if I've missed something, but how is this going to
work for RTL languages? Translators can change the format string but
they can't change the order of parameters passed to strbuf_addf.

Re: [RFC/PATCH] i18n of multi-line messages

[Andreas Schwab]

Chris Packham <judge.packham@gmail.com> writes:

> Hi Junio,
>
> On 12/22/2011 08:38 PM, Junio C Hamano wrote:
>> +	for (cp = buf.buf; *cp; cp = np) {
>> +		np = strchrnul(cp, '\n');
>> +		/*
>> +		 * TRANSLATORS: the format is designed so that in RTL
>> +		 * languages you could reorder and put the "prefix" at
>> +		 * the end instead of the beginning of a line if you
>> +		 * wanted to.
>> +		 */
>> +		strbuf_addf(&line,
>> +			    _("%s: %.*s\n"),
>> +			    prefix,
>> +			    (int)(np - cp), cp);
>> +		emit(&line, cb_data);
>> +		strbuf_reset(&line);
>> +		if (*np)
>> +			np++;
>> +	}
>
> Forgive my ignorance if I've missed something, but how is this going to
> work for RTL languages? Translators can change the format string but
> they can't change the order of parameters passed to strbuf_addf.

Translations can select the parameters to use with the n$ specification,
eg. "%3$.*2$s: %$1s\n"

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: [RFC/PATCH] i18n of multi-line messages

[Ævar Arnfjörð Bjarmason]

On Thu, Dec 22, 2011 at 08:38, Junio C Hamano <gitster@pobox.com> wrote:

[Re-formatted for clarity]

> -       vreportf("hint: ", advice, params);
> +       vreportf("hint", advice, params);
> -               vreportf("fatal: ", err, params);
> +               vreportf("fatal", err, params);
> -       vwritef(child_err, "fatal: ", err, params);
> +       vwritef(child_err, "fatal", err, params);
> -       vreportf("usage: ", err, params);
> +       vreportf("usage", err, params);
> -       vreportf("fatal: ", err, params);
> +       vreportf("fatal", err, params);
> -       vreportf("error: ", err, params);
> +       vreportf("error", err, params);
> -       vreportf("warning: ", warn, params);
> +       vreportf("warning", warn, params);

If we do it like this these would have to have something like:

    vreportf(N_("warning"), warn, params);

Followed by...:

> +       strbuf_vaddf(&buf, fmt, params);
> +       for (cp = buf.buf; *cp; cp = np) {
> +               np = strchrnul(cp, '\n');
> +               /*
> +                * TRANSLATORS: the format is designed so that in RTL
> +                * languages you could reorder and put the "prefix" at
> +                * the end instead of the beginning of a line if you
> +                * wanted to.
> +                */
> +               strbuf_addf(&line,
> +                           _("%s: %.*s\n"),
> +                           prefix,

Changing this to _(prefix).

> +                           (int)(np - cp), cp);
> +               emit(&line, cb_data);
> +               strbuf_reset(&line);
> +               if (*np)
> +                       np++;
> +       }
> +       strbuf_release(&buf);
> +       strbuf_release(&line);

But ideally to make things clear to the translators it's better to
give them something like this to translate:

    error: %s
    message

Instead of just, as two separate things:

    error
    message

Because:

 1. We might use "error", "warning", "usage" etc. somewhere else, and
    unless we start using the msgctxt feature of gettext we can't
    distinguish between these.

 2. If you present them as two separate things the translator is
    likely to get the case wrong (e.g. translate "error" in the
    nominative case instead of say accusative).

But it's not a big deal, the patch looks good to me as-is with those
N_() and _() changes. Just something to keep in mind.

We can always fixed issues like the one I'm raising later as they crop up.