git status: minor output format error

PATCH In review Wolfgang Rohdewald 4 participants 6 messages Jun 15, 2016

lore

Share & tools

lore

Wolfgang Rohdewald

author

2016-06-15 22:59 UTC #0 lore raw

git version 1.8.3.2

git status in German says (extract)

# geändert: kajongg.py
# gelöscht: playfield.py

as you can see, there is one space too much before playfield.py

LANG=C git status is correct:

#       modified:   kajongg.py
#       deleted:    playfield.py

so it seems the spacing between the columns expects "deleted" to have the same number of letters in all languages.

Signature · 1 line

Wolfgang

Nguyễn Thái Ngọc Duy

2016-06-15 22:59 UTC #1 lore raw

PATCH 1/1 wt-status: take the alignment burden off translators

It's not easy for translators to see spaces in these strings have to align, especially when there are no guarantees that these strings are grouped together in .po files.

Refactor the code and do the alignment automatically. Let the translator specify how big the alignment is, though, in case some languages need more than 12 columns for the preceding text.

Noticed-by Wolfgang Rohdewald <[email protected]>

Signed-off-by Nguyễn Thái Ngọc Duy <[email protected]>

---
Ralf and Quan, don't update your translations yet. If this change gets merged it'll simplify your task a bit.

 wt-status.c | 51 +++++++++++++++++++++++++++++++++++++++++++--------
 1 file changed, 43 insertions(+), 8 deletions(-)

wt-status.c +43 -8

index b4e44ba..f9715d3 100644
@@ -15,6 +15,7 @@
 #include "submodule.h"
 #include "column.h"
 #include "strbuf.h"
+#include "utf8.h"
 
 static char default_wt_status_colors[][COLOR_MAXLEN] = {
 	GIT_COLOR_NORMAL, /* WT_STATUS_HEADER */

@@ -276,6 +277,10 @@ static void wt_status_print_change_data(struct wt_status *s,
 	const char *one, *two;
 	struct strbuf onebuf = STRBUF_INIT, twobuf = STRBUF_INIT;
 	struct strbuf extra = STRBUF_INIT;
+	const char *what;
+	static const char *spaces = "                                ";
+	static int width = -1;
+	int len;
 
 	one_name = two_name = it->string;
 	switch (change_type) {

@@ -309,32 +314,62 @@ static void wt_status_print_change_data(struct wt_status *s,
 	status_printf(s, color(WT_STATUS_HEADER, s), "\t");
 	switch (status) {
 	case DIFF_STATUS_ADDED:
-		status_printf_more(s, c, _("new file:   %s"), one);
+		what = _("new file");
 		break;
 	case DIFF_STATUS_COPIED:
-		status_printf_more(s, c, _("copied:     %s -> %s"), one, two);
+		what = _("copied");
 		break;
 	case DIFF_STATUS_DELETED:
-		status_printf_more(s, c, _("deleted:    %s"), one);
+		what = _("deleted");
 		break;
 	case DIFF_STATUS_MODIFIED:
-		status_printf_more(s, c, _("modified:   %s"), one);
+		what = _("modified");
 		break;
 	case DIFF_STATUS_RENAMED:
-		status_printf_more(s, c, _("renamed:    %s -> %s"), one, two);
+		what = _("renamed");
 		break;
 	case DIFF_STATUS_TYPE_CHANGED:
-		status_printf_more(s, c, _("typechange: %s"), one);
+		what = _("typechange");
 		break;
 	case DIFF_STATUS_UNKNOWN:
-		status_printf_more(s, c, _("unknown:    %s"), one);
+		what = _("unknown");
 		break;
 	case DIFF_STATUS_UNMERGED:
-		status_printf_more(s, c, _("unmerged:   %s"), one);
+		what = _("unmerged");
 		break;
 	default:
 		die(_("bug: unhandled diff status %c"), status);
 	}
+	if (width == -1) {
+		/*
+		 * Translators: if you do translate this, replace it
+		 * with a decimal number of how many columns needed
+		 * to align file names in "git status":
+		 *
+		 * |<-columns->|
+		 *
+		 *   unmerged: foo.c
+		 *   copied:   foo.c -> bar.c
+		 *
+		 * The default value is 12. Normally you would not
+		 * need to translate this at all unless the translated
+		 * strings are longer than 12 columns and therefore
+		 * break alignment.
+		 */
+		width = atoi(_("<wt-status.c:width>"));
+		if (width <= 0 || width > 32)
+			width = 12;
+	}
+	if (width > utf8_strwidth(what) + 1)
+		len = width - utf8_strwidth(what) - 1;
+	else
+		len = 0;
+	if (status == DIFF_STATUS_COPIED || status == DIFF_STATUS_RENAMED)
+		status_printf_more(s, c, "%s:%.*s%s -> %s",
+				   what, len, spaces, one, two);
+	else
+		status_printf_more(s, c, "%s:%.*s%s",
+				   what, len, spaces, one);
 	if (extra.len) {
 		status_printf_more(s, color(WT_STATUS_HEADER, s), "%s", extra.buf);
 		strbuf_release(&extra);

Signature · 1 line

1.8.2.83.gc99314b

Nguyễn Thái Ngọc Duy

2016-06-15 22:59 UTC #2 lore raw

PATCH v2 1/1 wt-status: take the alignment burden off translators

It's not easy for translators to see spaces in these strings have to align, especially when there are no guarantees that these strings are grouped together in .po files. Refactor the code and do the alignment automatically.

Noticed-by Wolfgang Rohdewald <[email protected]>

Signed-off-by Nguyễn Thái Ngọc Duy <[email protected]>

---
v2 calculates maximum length automatically too so the translator does not have to test and specify that manually.

 wt-status.c | 80 ++++++++++++++++++++++++++++++++++++++++---------------------
 1 file changed, 53 insertions(+), 27 deletions(-)

wt-status.c +53 ~~-27~~

index b4e44ba..4625cdb 100644
@@ -15,6 +15,7 @@
 #include "submodule.h"
 #include "column.h"
 #include "strbuf.h"
+#include "utf8.h"
 
 static char default_wt_status_colors[][COLOR_MAXLEN] = {
 	GIT_COLOR_NORMAL, /* WT_STATUS_HEADER */

@@ -264,6 +265,30 @@ static void wt_status_print_unmerged_data(struct wt_status *s,
 	strbuf_release(&onebuf);
 }
 
+static const char *wt_status_diff_status_string(int status)
+{
+	switch (status) {
+	case DIFF_STATUS_ADDED:
+		return _("new file");
+	case DIFF_STATUS_COPIED:
+		return _("copied");
+	case DIFF_STATUS_DELETED:
+		return _("deleted");
+	case DIFF_STATUS_MODIFIED:
+		return _("modified");
+	case DIFF_STATUS_RENAMED:
+		return _("renamed");
+	case DIFF_STATUS_TYPE_CHANGED:
+		return _("typechange");
+	case DIFF_STATUS_UNKNOWN:
+		return _("unknown");
+	case DIFF_STATUS_UNMERGED:
+		return _("unmerged");
+	default:
+		return NULL;
+	}
+}
+
 static void wt_status_print_change_data(struct wt_status *s,
 					int change_type,
 					struct string_list_item *it)

@@ -276,6 +301,23 @@ static void wt_status_print_change_data(struct wt_status *s,
 	const char *one, *two;
 	struct strbuf onebuf = STRBUF_INIT, twobuf = STRBUF_INIT;
 	struct strbuf extra = STRBUF_INIT;
+	static char *padding;
+	const char *what;
+	int len;
+
+	if (!padding) {
+		int width = 0;
+		/* If DIFF_STATUS_* uses outside this range, we're in trouble */
+		for (status = 'A'; status <= 'Z'; status++) {
+			what = wt_status_diff_status_string(status);
+			len = what ? strlen(what) : 0;
+			if (len > width)
+				width = len;
+		}
+		width += 2;	/* colon and a space */
+		padding = xmallocz(width);
+		memset(padding, ' ', width);
+	}
 
 	one_name = two_name = it->string;
 	switch (change_type) {

@@ -307,34 +349,18 @@ static void wt_status_print_change_data(struct wt_status *s,
 	two = quote_path(two_name, s->prefix, &twobuf);
 
 	status_printf(s, color(WT_STATUS_HEADER, s), "\t");
-	switch (status) {
-	case DIFF_STATUS_ADDED:
-		status_printf_more(s, c, _("new file:   %s"), one);
-		break;
-	case DIFF_STATUS_COPIED:
-		status_printf_more(s, c, _("copied:     %s -> %s"), one, two);
-		break;
-	case DIFF_STATUS_DELETED:
-		status_printf_more(s, c, _("deleted:    %s"), one);
-		break;
-	case DIFF_STATUS_MODIFIED:
-		status_printf_more(s, c, _("modified:   %s"), one);
-		break;
-	case DIFF_STATUS_RENAMED:
-		status_printf_more(s, c, _("renamed:    %s -> %s"), one, two);
-		break;
-	case DIFF_STATUS_TYPE_CHANGED:
-		status_printf_more(s, c, _("typechange: %s"), one);
-		break;
-	case DIFF_STATUS_UNKNOWN:
-		status_printf_more(s, c, _("unknown:    %s"), one);
-		break;
-	case DIFF_STATUS_UNMERGED:
-		status_printf_more(s, c, _("unmerged:   %s"), one);
-		break;
-	default:
+	what = wt_status_diff_status_string(status);
+	if (!what)
 		die(_("bug: unhandled diff status %c"), status);
-	}
+	/* 1 for colon, which is not part of "what" */
+	len = strlen(padding) - (utf8_strwidth(what) + 1);
+	assert(len >= 0);
+	if (status == DIFF_STATUS_COPIED || status == DIFF_STATUS_RENAMED)
+		status_printf_more(s, c, "%s:%.*s%s -> %s",
+				   what, len, padding, one, two);
+	else
+		status_printf_more(s, c, "%s:%.*s%s",
+				   what, len, padding, one);
 	if (extra.len) {
 		status_printf_more(s, color(WT_STATUS_HEADER, s), "%s", extra.buf);
 		strbuf_release(&extra);

Signature · 1 line

1.8.2.82.gc24b958

Junio C Hamano

2016-06-15 22:59 UTC #3 lore raw

Re: [PATCH] wt-status: take the alignment burden off translators

In reply to Nguyễn Thái Ngọc Duy

wt-status.c static void wt_status_print_change_data(struct wt_status *s, · line 281

Quoted 1 line · Nguyễn Thái Ngọc Duy

wt-status.c +1 -0

+	static const char *spaces = "                                ";

I do not see anything that limits "len" used to show only show the prefix bytes in this array not to ask for more than you have spaces here. It won't overrun the end of this array, but I suspect your result will not align when that happens.

Quoted 50 lines

wt-status.c +17 -8

+	static int width = -1;
+	int len;
 
 	one_name = two_name = it->string;
 	switch (change_type) {

@@ -309,32 +314,62 @@ static void wt_status_print_change_data(struct wt_status *s,
 	status_printf(s, color(WT_STATUS_HEADER, s), "\t");
 	switch (status) {
 	case DIFF_STATUS_ADDED:
-		status_printf_more(s, c, _("new file:   %s"), one);
+		what = _("new file");
 		break;
 	case DIFF_STATUS_COPIED:
-		status_printf_more(s, c, _("copied:     %s -> %s"), one, two);
+		what = _("copied");
 		break;
 	case DIFF_STATUS_DELETED:
-		status_printf_more(s, c, _("deleted:    %s"), one);
+		what = _("deleted");
 		break;
 	case DIFF_STATUS_MODIFIED:
-		status_printf_more(s, c, _("modified:   %s"), one);
+		what = _("modified");
 		break;
 	case DIFF_STATUS_RENAMED:
-		status_printf_more(s, c, _("renamed:    %s -> %s"), one, two);
+		what = _("renamed");
 		break;
 	case DIFF_STATUS_TYPE_CHANGED:
-		status_printf_more(s, c, _("typechange: %s"), one);
+		what = _("typechange");
 		break;
 	case DIFF_STATUS_UNKNOWN:
-		status_printf_more(s, c, _("unknown:    %s"), one);
+		what = _("unknown");
 		break;
 	case DIFF_STATUS_UNMERGED:
-		status_printf_more(s, c, _("unmerged:   %s"), one);
+		what = _("unmerged");
 		break;
 	default:
 		die(_("bug: unhandled diff status %c"), status);
 	}
+	if (width == -1) {
+		/*
+		 * Translators: if you do translate this, replace it
+		 * with a decimal number of how many columns needed
+		 * to align file names in "git status":
+		 *
+		 * |<-columns->|

wt-status.c static void wt_status_print_change_data(struct wt_status *s, · line 359

Quoted 10 lines

wt-status.c +10 -0

+		 *
+		 *   unmerged: foo.c
+		 *   copied:   foo.c -> bar.c
+		 *
+		 * The default value is 12. Normally you would not
+		 * need to translate this at all unless the translated
+		 * strings are longer than 12 columns and therefore
+		 * break alignment.
+		 */
+		width = atoi(_("<wt-status.c:width>"));

Somewhat ugly. Wouldn't it work better to keep the "what" strings in a static array and compute "width" just once? Is the sparseness of DIFF_STATUS_* enums what makes such an implementation cumbersome?

I dunno.

wt-status.c static void wt_status_print_change_data(struct wt_status *s, · line 364

Quoted 5 lines

wt-status.c +5 -0

+		if (width <= 0 || width > 32)
+			width = 12;
+	}
+	if (width > utf8_strwidth(what) + 1)
+		len = width - utf8_strwidth(what) - 1;

Just a style, but this is far easier to read if you said:

	if (width > utf8_strwidth(what) + 1)
		len = width - (utf8_strwidth(what) + 1);

because you are essentially doing:

	if (A > B)
        	x = A - B;

Or even

	len = width - (utf8_strwidth(what) + 1);
        if (len < 0)
        	len = 0;

Quoted 11 lines

wt-status.c +8 -0

+	else
+		len = 0;
+	if (status == DIFF_STATUS_COPIED || status == DIFF_STATUS_RENAMED)
+		status_printf_more(s, c, "%s:%.*s%s -> %s",
+				   what, len, spaces, one, two);
+	else
+		status_printf_more(s, c, "%s:%.*s%s",
+				   what, len, spaces, one);
 	if (extra.len) {
 		status_printf_more(s, color(WT_STATUS_HEADER, s), "%s", extra.buf);
 		strbuf_release(&extra);

Duy Nguyen

2016-06-15 22:59 UTC #4 lore raw

In reply to Wolfgang Rohdewald

Quoted 7 lines · Wolfgang Rohdewald

git version 1.8.3.2
git status in German says (extract)
# geändert: kajongg.py
# gelöscht: playfield.py

Quoted 8 lines

as you can see, there is one space too much before playfield.py
LANG=C git status is correct:
#       modified:   kajongg.py
#       deleted:    playfield.py
so it seems the spacing between the columns expects "deleted" to have the same number of letters in all languages.

No, the translations control the number of columns in this case (although it's not very intuitive for translators). Something like this may fix it. I haven't tested because I don't have "de" locale installed.

po/de.po +1 -1

index 35a44b9..d1846d2 100644
@@ -1404,7 +1404,7 @@ msgstr "kopiert:     %s -> %s"
 #: wt-status.c:313
 #, c-format
 msgid "deleted:    %s"
-msgstr "gelöscht:    %s"
+msgstr "gelöscht:   %s"
 
 #: wt-status.c:316
 #, c-format

Signature · 1 line

Duy

Ralf Thielow

2016-06-15 22:59 UTC #5 lore raw

Quoted 28 lines · Duy Nguyen

Quoted 7 lines · Wolfgang Rohdewald
git version 1.8.3.2
git status in German says (extract)
# geändert: kajongg.py
# gelöscht: playfield.py
Quoted 8 lines
as you can see, there is one space too much before playfield.py
LANG=C git status is correct:
#       modified:   kajongg.py
#       deleted:    playfield.py
so it seems the spacing between the columns expects "deleted" to have the same number of letters in all languages.
No, the translations control the number of columns in this case (although it's not very intuitive for translators). Something like this may fix it. I haven't tested because I don't have "de" locale installed.
po/de.po +0 -0
index 35a44b9..d1846d2 100644

po/de.po msgstr "kopiert: %s -> %s" · line 1408

Quoted 6 lines

po/de.po +1 -1

@@ -1404,7 +1404,7 @@ msgstr "kopiert:     %s -> %s"
 #: wt-status.c:313
 #, c-format
 msgid "deleted:    %s"
-msgstr "gelöscht:    %s"
+msgstr "gelöscht:   %s"

The columns don't match in other related messages, either. Let's see what happens with [1].

[1]
http://article.gmane.org/gmane.comp.version-control.git/237283

Quoted 4 lines

po/de.po +0 -1

 #: wt-status.c:316
 #, c-format
--

Duy