Re: git: problematic git status output with some translations (such as fr_FR)

[Junio C Hamano]

Jonathan Nieder jrnie...@gmail.com writes:

 @@ -292,6 +291,48 @@ static const char *wt_status_diff_status_string(int 
 status)
   }
  }
  
 +static int maxwidth(const char *(*label)(int), int minval, int maxval)
 +{
 + const char *s;
 + int result = 0, i;
 +
 + for (i = minval; i = maxval; i++) {
 + const char *s = label(i);
 + int len = s ? strlen(s) : 0;

Shouldn't this be a utf8_strwidth(), as the value is to count number
of display columns to be used by the leading label part?

 + if (len  result)
 + result = len;
 + }
 + return result;
 +}
 +
 +static void wt_status_print_unmerged_data(struct wt_status *s,
 +   struct string_list_item *it)
 +{
 + const char *c = color(WT_STATUS_UNMERGED, s);
 + struct wt_status_change_data *d = it-util;
 + struct strbuf onebuf = STRBUF_INIT;
 + static char *padding;
 + const char *one, *how;
 + int len;
 +
 + if (!padding) {
 + int width = maxwidth(wt_status_unmerged_status_string, 1, 7);
 + width += strlen( );
 + padding = xmallocz(width);
 + memset(padding, ' ', width);
 + }
 +
 + one = quote_path(it-string, s-prefix, onebuf);
 + status_printf(s, color(WT_STATUS_HEADER, s), \t);
 +
 + how = wt_status_unmerged_status_string(d-stagemask);
 + if (!how)
 + how = _(bug);

I'd rather see the callee do this _(bug) thing, not this
particular caller.

 + len = strlen(padding) - utf8_strwidth(how);
 + status_printf_more(s, c, %s%.*s%s\n, how, len, padding, one);
 + strbuf_release(onebuf);
 +}
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: git: problematic git status output with some translations (such as fr_FR)

[Duy Nguyen]

On Fri, Dec 20, 2013 at 3:50 AM, Jonathan Nieder jrnie...@gmail.com wrote:
 Junio C Hamano wrote:
 Jonathan Nieder jrnie...@gmail.com writes:

 This includes the colon in the translated string, to make it easier to
 remember to keep the non-breaking space before it.

 Hmph, recent 3651e45c (wt-status: take the alignment burden off
 translators, 2013-11-05) seems to have gone in the different
 direction when it updated similar code for the non-unmerged paths.

 Yes, if this seems to go in the right direction, I'd add a follow-up
 for that when rerolling.

Just checking. Did you reroll it or did my filters move some mails to
spam/trash folder again?
-- 
Duy
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: git: problematic git status output with some translations (such as fr_FR)

[Raphael Hertzog]

Hi,

On Fri, 20 Dec 2013, Duy Nguyen wrote:
 On Fri, Dec 20, 2013 at 3:50 AM, Jonathan Nieder jrnie...@gmail.com wrote:
  Junio C Hamano wrote:
  Jonathan Nieder jrnie...@gmail.com writes:
 
  This includes the colon in the translated string, to make it easier to
  remember to keep the non-breaking space before it.
 
  Hmph, recent 3651e45c (wt-status: take the alignment burden off
  translators, 2013-11-05) seems to have gone in the different
  direction when it updated similar code for the non-unmerged paths.
 
  Yes, if this seems to go in the right direction, I'd add a follow-up
  for that when rerolling.
 
 i'm fine either way.

In both cases, the alignment burden is taken off the translators. But I
agree that it's best to keep the colon in the translatable string so that
translators can add the space required by proper typography.

So I'd favor Jonathan's approach. I just tested his patch and it
solves the immediate problem for me. Without the patch I have this
(French translation):

| Chemins non fusionnés :
|   (utilisez git add fichier... pour marquer comme résolu)
| 
|   modifié des deux côtés :fichier

And with it:

| Chemins non fusionnés :
|   (utilisez git add fichier... pour marquer comme résolu)
| 
|   modifié des deux côtés : fichier

However it looks like the padding is calculated on bytes
and not on (wide) characters, so we have 3 extra spaces before
the filename (one for ô, é, and the non-breaking space).
This can be problematic for translations where all characters
require multiple bytes. :-)

The patch is also incomplete since it currently breaks
the test suite in multiple places (the unnecessary padding
before the filename goes away), for example:

--- expected2014-01-16 21:37:11.270535950 +
+++ actual  2014-01-16 21:37:11.270535950 +
@@ -5,7 +5,7 @@
 Unmerged paths:
   (use git add/rm file... as appropriate to mark resolution)
 
-   both added: conflict.txt
-   deleted by them:main.txt
+   both added:  conflict.txt
+   deleted by them: main.txt
 
 no changes added to commit (use git add and/or git commit -a)
not ok 11 - status when conflicts with add and rm advice (deleted by them)


That was my two euros to help get this fixed because it annoys me
quite often.

Cheers,

-- 
Raphaël Hertzog ◈ Debian Developer

Discover the Debian Administrator's Handbook:
→ http://debian-handbook.info/get/
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: git: problematic git status output with some translations (such as fr_FR)

[Jonathan Nieder]

Raphaël Hertzog wrote[1]:

 Here's an example of the problematic output:
[...]
 # modifié des deux côtés :debian/control

Thanks for the ping, and sorry to leave this hanging before.

[...]
 --- a/wt-status.c
 +++ b/wt-status.c
 @@ -255,7 +255,7 @@ static void wt_status_print_unmerged_data(struct 
 wt_status *s,
   case 6: how = _(both added:); break;
   case 7: how = _(both modified:); break;
   }
 - status_printf_more(s, c, %-20s%s\n, how, one);
 + status_printf_more(s, c, %-19s %s\n, how, one);

It looks like the original code is assuming that

 (1) the number of bytes written is the width of a string, so they can
 line up

 (2) the how string is always = 19 bytes

Both assumptions are problematic.

Perhaps the value '20' should be dynamic (e.g.,

min(20,
1 + max(utf8_strwidth(all 'how' strings
  in the current UI language)))

) to allow the text to always line up?  Ever since 3651e45c
(wt-status: take the alignment burden off translators, 2013-11-05),
wt_status_print_change_data() picks its width dynamically for the same
reason.  So, something like the following (untested)?

This includes the colon in the translated string, to make it easier to
remember to keep the non-breaking space before it.

Signed-off-by: Jonathan Nieder jrnie...@gmail.com

[1] http://bugs.debian.org/725777

diff --git i/wt-status.c w/wt-status.c
index 4e55810..7b0e5b8 100644
--- i/wt-status.c
+++ w/wt-status.c
@@ -245,27 +245,26 @@ static void wt_status_print_trailer(struct wt_status *s)
 
 #define quote_path quote_path_relative
 
-static void wt_status_print_unmerged_data(struct wt_status *s,
- struct string_list_item *it)
+static const char *wt_status_unmerged_status_string(int stagemask)
 {
-   const char *c = color(WT_STATUS_UNMERGED, s);
-   struct wt_status_change_data *d = it-util;
-   struct strbuf onebuf = STRBUF_INIT;
-   const char *one, *how = _(bug);
-
-   one = quote_path(it-string, s-prefix, onebuf);
-   status_printf(s, color(WT_STATUS_HEADER, s), \t);
-   switch (d-stagemask) {
-   case 1: how = _(both deleted:); break;
-   case 2: how = _(added by us:); break;
-   case 3: how = _(deleted by them:); break;
-   case 4: how = _(added by them:); break;
-   case 5: how = _(deleted by us:); break;
-   case 6: how = _(both added:); break;
-   case 7: how = _(both modified:); break;
+   switch (stagemask) {
+   case 1:
+   return _(both deleted:);
+   case 2:
+   return _(added by us:);
+   case 3:
+   return _(deleted by them:);
+   case 4:
+   return _(added by them:);
+   case 5:
+   return _(deleted by us:);
+   case 6:
+   return _(both added:);
+   case 7:
+   return _(both modified:);
+   default:
+   return NULL;
}
-   status_printf_more(s, c, %-20s%s\n, how, one);
-   strbuf_release(onebuf);
 }
 
 static const char *wt_status_diff_status_string(int status)
@@ -292,6 +291,48 @@ static const char *wt_status_diff_status_string(int status)
}
 }
 
+static int maxwidth(const char *(*label)(int), int minval, int maxval)
+{
+   const char *s;
+   int result = 0, i;
+
+   for (i = minval; i = maxval; i++) {
+   const char *s = label(i);
+   int len = s ? strlen(s) : 0;
+   if (len  result)
+   result = len;
+   }
+   return result;
+}
+
+static void wt_status_print_unmerged_data(struct wt_status *s,
+ struct string_list_item *it)
+{
+   const char *c = color(WT_STATUS_UNMERGED, s);
+   struct wt_status_change_data *d = it-util;
+   struct strbuf onebuf = STRBUF_INIT;
+   static char *padding;
+   const char *one, *how;
+   int len;
+
+   if (!padding) {
+   int width = maxwidth(wt_status_unmerged_status_string, 1, 7);
+   width += strlen( );
+   padding = xmallocz(width);
+   memset(padding, ' ', width);
+   }
+
+   one = quote_path(it-string, s-prefix, onebuf);
+   status_printf(s, color(WT_STATUS_HEADER, s), \t);
+
+   how = wt_status_unmerged_status_string(d-stagemask);
+   if (!how)
+   how = _(bug);
+   len = strlen(padding) - utf8_strwidth(how);
+   status_printf_more(s, c, %s%.*s%s\n, how, len, padding, one);
+   strbuf_release(onebuf);
+}
+
 static void wt_status_print_change_data(struct wt_status *s,
int change_type,
struct string_list_item *it)
@@ -309,14 +350,8 @@ static void wt_status_print_change_data(struct wt_status 
*s,
int len;
 
if (!padding) {
-   int width = 0;
-   /* If DIFF_STATUS_* uses outside this range, we're in 

Re: git: problematic git status output with some translations (such as fr_FR)

[Junio C Hamano]

Jonathan Nieder jrnie...@gmail.com writes:

 This includes the colon in the translated string, to make it easier to
 remember to keep the non-breaking space before it.

Hmph, recent 3651e45c (wt-status: take the alignment burden off
translators, 2013-11-05) seems to have gone in the different
direction when it updated similar code for the non-unmerged paths.

 Signed-off-by: Jonathan Nieder jrnie...@gmail.com

 [1] http://bugs.debian.org/725777

 diff --git i/wt-status.c w/wt-status.c
 index 4e55810..7b0e5b8 100644
 --- i/wt-status.c
 +++ w/wt-status.c
 @@ -245,27 +245,26 @@ static void wt_status_print_trailer(struct wt_status *s)
  
  #define quote_path quote_path_relative
  
 -static void wt_status_print_unmerged_data(struct wt_status *s,
 -   struct string_list_item *it)
 +static const char *wt_status_unmerged_status_string(int stagemask)
  {
 - const char *c = color(WT_STATUS_UNMERGED, s);
 - struct wt_status_change_data *d = it-util;
 - struct strbuf onebuf = STRBUF_INIT;
 - const char *one, *how = _(bug);
 -
 - one = quote_path(it-string, s-prefix, onebuf);
 - status_printf(s, color(WT_STATUS_HEADER, s), \t);
 - switch (d-stagemask) {
 - case 1: how = _(both deleted:); break;
 - case 2: how = _(added by us:); break;
 - case 3: how = _(deleted by them:); break;
 - case 4: how = _(added by them:); break;
 - case 5: how = _(deleted by us:); break;
 - case 6: how = _(both added:); break;
 - case 7: how = _(both modified:); break;
 + switch (stagemask) {
 + case 1:
 + return _(both deleted:);
 + case 2:
 + return _(added by us:);
 + case 3:
 + return _(deleted by them:);
 + case 4:
 + return _(added by them:);
 + case 5:
 + return _(deleted by us:);
 + case 6:
 + return _(both added:);
 + case 7:
 + return _(both modified:);
 + default:
 + return NULL;
   }
 - status_printf_more(s, c, %-20s%s\n, how, one);
 - strbuf_release(onebuf);
  }
  
  static const char *wt_status_diff_status_string(int status)
 @@ -292,6 +291,48 @@ static const char *wt_status_diff_status_string(int 
 status)
   }
  }
  
 +static int maxwidth(const char *(*label)(int), int minval, int maxval)
 +{
 + const char *s;
 + int result = 0, i;
 +
 + for (i = minval; i = maxval; i++) {
 + const char *s = label(i);
 + int len = s ? strlen(s) : 0;
 + if (len  result)
 + result = len;
 + }
 + return result;
 +}
 +
 +static void wt_status_print_unmerged_data(struct wt_status *s,
 +   struct string_list_item *it)
 +{
 + const char *c = color(WT_STATUS_UNMERGED, s);
 + struct wt_status_change_data *d = it-util;
 + struct strbuf onebuf = STRBUF_INIT;
 + static char *padding;
 + const char *one, *how;
 + int len;
 +
 + if (!padding) {
 + int width = maxwidth(wt_status_unmerged_status_string, 1, 7);
 + width += strlen( );
 + padding = xmallocz(width);
 + memset(padding, ' ', width);
 + }
 +
 + one = quote_path(it-string, s-prefix, onebuf);
 + status_printf(s, color(WT_STATUS_HEADER, s), \t);
 +
 + how = wt_status_unmerged_status_string(d-stagemask);
 + if (!how)
 + how = _(bug);
 + len = strlen(padding) - utf8_strwidth(how);
 + status_printf_more(s, c, %s%.*s%s\n, how, len, padding, one);
 + strbuf_release(onebuf);
 +}
 +
  static void wt_status_print_change_data(struct wt_status *s,
   int change_type,
   struct string_list_item *it)
 @@ -309,14 +350,8 @@ static void wt_status_print_change_data(struct wt_status 
 *s,
   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;
 - }
 + /* If DIFF_STATUS_* uses outside the range [A..Z], we're in 
 trouble */
 + int width = maxwidth(wt_status_diff_status_string, 'A', 'Z');
   width += 2; /* colon and a space */
   padding = xmallocz(width);
   memset(padding, ' ', width);
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: git: problematic git status output with some translations (such as fr_FR)

[Jonathan Nieder]

Junio C Hamano wrote:
 Jonathan Nieder jrnie...@gmail.com writes:

 This includes the colon in the translated string, to make it easier to
 remember to keep the non-breaking space before it.

 Hmph, recent 3651e45c (wt-status: take the alignment burden off
 translators, 2013-11-05) seems to have gone in the different
 direction when it updated similar code for the non-unmerged paths.

Yes, if this seems to go in the right direction, I'd add a follow-up
for that when rerolling.

Alternatively if there is some library function to append a colon to a
string in a locale-appropriate way, that could work, too.  Pointers
welcome.

Thanks,
Jonathan
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: git: problematic git status output with some translations (such as fr_FR)

[Duy Nguyen]

On Fri, Dec 20, 2013 at 3:50 AM, Jonathan Nieder jrnie...@gmail.com wrote:
 Junio C Hamano wrote:
 Jonathan Nieder jrnie...@gmail.com writes:

 This includes the colon in the translated string, to make it easier to
 remember to keep the non-breaking space before it.

 Hmph, recent 3651e45c (wt-status: take the alignment burden off
 translators, 2013-11-05) seems to have gone in the different
 direction when it updated similar code for the non-unmerged paths.

 Yes, if this seems to go in the right direction, I'd add a follow-up
 for that when rerolling.

i'm fine either way.
-- 
Duy
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html