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

Johannes Sixt [off-list ref] 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 <redacted>
---
 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

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

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.

On Thu, Dec 22, 2011 at 08:38, Junio C Hamano [off-list ref] 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.