[PATCH] blame: add correct paddings in time_buf for align

[PATCH] blame: add correct paddings in time_buf for align

[Jiang Xin]

When show blame information with relative time, the UTF-8 characters in
`time_str` will break the alignment of columns after the date field.
This is because the `time_buf` padding with spaces should have a
constant display width, not a fixed strlen size.  So we should call
utf8_strwidth() instead of strlen() for calibration.

Signed-off-by: Jiang Xin <worldhello.net@gmail.com>
---

Before applying this patch:

        5817da01 builtin-blame.c   (Pierre Habouzit             6 年前                         21) #include "parse-options.h"
        ffaf9cc0 builtin-blame.c   (Geoffrey Thomas             5 年前                         22) #include "utf8.h"
        3b8a12e8 builtin/blame.c   (Axel Bonnet                 3 年 10 个月之前            23) #include "userdiff.h"
        25ed3412 builtin/blame.c   (Bo Yang                     1 年 1 个月之前             24) #include "line-range.h"
        58dbfa2e builtin/blame.c   (Eric Sunshine               9 个月之前                   25) #include "line-log.h"
        cee7f245 builtin-pickaxe.c (Junio C Hamano              8 年前                         26) 

After applying this patch:

        5817da01 builtin-blame.c   (Pierre Habouzit             6 年前                           21) #include "parse-options.h"
        ffaf9cc0 builtin-blame.c   (Geoffrey Thomas             5 年前                           22) #include "utf8.h"
        3b8a12e8 builtin/blame.c   (Axel Bonnet                 3 年 10 个月之前                 23) #include "userdiff.h"
        25ed3412 builtin/blame.c   (Bo Yang                     1 年 1 个月之前                  24) #include "line-range.h"
        58dbfa2e builtin/blame.c   (Eric Sunshine               9 个月之前                       25) #include "line-log.h"
        cee7f245 builtin-pickaxe.c (Junio C Hamano              8 年前                           26) 

 builtin/blame.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/builtin/blame.c b/builtin/blame.c
index 88cb799..c8f6647 100644
--- a/builtin/blame.c
+++ b/builtin/blame.c
@@ -1564,12 +1564,19 @@ static const char *format_time(unsigned long time, const char *tz_str,
 	else {
 		const char *time_str;
 		int time_len;
+		int time_col;
 		int tz;
 		tz = atoi(tz_str);
 		time_str = show_date(time, tz, blame_date_mode);
 		time_len = strlen(time_str);
 		memcpy(time_buf, time_str, time_len);
-		memset(time_buf + time_len, ' ', blame_date_width - time_len);
+		/*
+		 * Add space paddings to time_buf to display a fixed width
+		 * string, and use time_col for display width calibration.
+		 */
+		time_col = utf8_strwidth(time_str);
+		memset(time_buf + time_len, ' ', blame_date_width - time_col);
+		*(time_buf + time_len + blame_date_width - time_col) = 0;
 	}
 	return time_buf;
 }
-- 
1.9.2.474.g17b2a16

Re: [PATCH] blame: add correct paddings in time_buf for align

[Duy Nguyen]

On Fri, Apr 18, 2014 at 3:44 PM, Jiang Xin <worldhello.net@gmail.com> wrote:
> When show blame information with relative time, the UTF-8 characters in
> `time_str` will break the alignment of columns after the date field.
> This is because the `time_buf` padding with spaces should have a
> constant display width, not a fixed strlen size.  So we should call
> utf8_strwidth() instead of strlen() for calibration.
>
> Signed-off-by: Jiang Xin <worldhello.net@gmail.com>
> ---
>
> Before applying this patch:
>
>         5817da01 builtin-blame.c   (Pierre Habouzit             6 年前                         21) #include "parse-options.h"
>         ffaf9cc0 builtin-blame.c   (Geoffrey Thomas             5 年前                         22) #include "utf8.h"
>         3b8a12e8 builtin/blame.c   (Axel Bonnet                 3 年 10 个月之前            23) #include "userdiff.h"
>         25ed3412 builtin/blame.c   (Bo Yang                     1 年 1 个月之前             24) #include "line-range.h"
>         58dbfa2e builtin/blame.c   (Eric Sunshine               9 个月之前                   25) #include "line-log.h"
>         cee7f245 builtin-pickaxe.c (Junio C Hamano              8 年前                         26)
>
> After applying this patch:
>
>         5817da01 builtin-blame.c   (Pierre Habouzit             6 年前                           21) #include "parse-options.h"
>         ffaf9cc0 builtin-blame.c   (Geoffrey Thomas             5 年前                           22) #include "utf8.h"
>         3b8a12e8 builtin/blame.c   (Axel Bonnet                 3 年 10 个月之前                 23) #include "userdiff.h"
>         25ed3412 builtin/blame.c   (Bo Yang                     1 年 1 个月之前                  24) #include "line-range.h"
>         58dbfa2e builtin/blame.c   (Eric Sunshine               9 个月之前                       25) #include "line-log.h"
>         cee7f245 builtin-pickaxe.c (Junio C Hamano              8 年前                           26)


The numbers 21..26 still do not look aligned, both in gmail raw
message view and gmane.

>
>  builtin/blame.c | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/builtin/blame.c b/builtin/blame.c
> index 88cb799..c8f6647 100644
> --- a/builtin/blame.c
> +++ b/builtin/blame.c
> @@ -1564,12 +1564,19 @@ static const char *format_time(unsigned long time, const char *tz_str,
>         else {
>                 const char *time_str;
>                 int time_len;
> +               int time_col;
>                 int tz;
>                 tz = atoi(tz_str);
>                 time_str = show_date(time, tz, blame_date_mode);
>                 time_len = strlen(time_str);
>                 memcpy(time_buf, time_str, time_len);
> -               memset(time_buf + time_len, ' ', blame_date_width - time_len);
> +               /*
> +                * Add space paddings to time_buf to display a fixed width
> +                * string, and use time_col for display width calibration.
> +                */
> +               time_col = utf8_strwidth(time_str);
> +               memset(time_buf + time_len, ' ', blame_date_width - time_col);
> +               *(time_buf + time_len + blame_date_width - time_col) = 0;

And you may want to turn time_buf[128] to strbuf as well while you're at it.

>         }
>         return time_buf;
>  }
> --
> 1.9.2.474.g17b2a16
>
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html



-- 
Duy

Re: [PATCH] blame: add correct paddings in time_buf for align

[Junio C Hamano]

Duy Nguyen <pclouds@gmail.com> writes:

> On Fri, Apr 18, 2014 at 3:44 PM, Jiang Xin <worldhello.net@gmail.com> wrote:
>> When show blame information with relative time, the UTF-8 characters in
>> `time_str` will break the alignment of columns after the date field.
>> This is because the `time_buf` padding with spaces should have a
>> constant display width, not a fixed strlen size.  So we should call
>> utf8_strwidth() instead of strlen() for calibration.
>>
>> Signed-off-by: Jiang Xin <worldhello.net@gmail.com>
>> ---
>>
>> Before applying this patch:
>>
>>         5817da01 builtin-blame.c   (Pierre Habouzit             6 年前                         21) #include "parse-options.h"
>>         ffaf9cc0 builtin-blame.c   (Geoffrey Thomas             5 年前                         22) #include "utf8.h"
>>         3b8a12e8 builtin/blame.c   (Axel Bonnet                 3 年 10 个月之前            23) #include "userdiff.h"
>>         25ed3412 builtin/blame.c   (Bo Yang                     1 年 1 个月之前             24) #include "line-range.h"
>>         58dbfa2e builtin/blame.c   (Eric Sunshine               9 个月之前                   25) #include "line-log.h"
>>         cee7f245 builtin-pickaxe.c (Junio C Hamano              8 年前                         26)
>>
>> After applying this patch:
>>
>>         5817da01 builtin-blame.c   (Pierre Habouzit             6 年前                           21) #include "parse-options.h"
>>         ffaf9cc0 builtin-blame.c   (Geoffrey Thomas             5 年前                           22) #include "utf8.h"
>>         3b8a12e8 builtin/blame.c   (Axel Bonnet                 3 年 10 个月之前                 23) #include "userdiff.h"
>>         25ed3412 builtin/blame.c   (Bo Yang                     1 年 1 个月之前                  24) #include "line-range.h"
>>         58dbfa2e builtin/blame.c   (Eric Sunshine               9 个月之前                       25) #include "line-log.h"
>>         cee7f245 builtin-pickaxe.c (Junio C Hamano              8 年前                           26)
>
>
> The numbers 21..26 still do not look aligned, both in gmail raw
> message view and gmane.

Interesting.  In my GNUS/Emacs on a fixed-column terminal where an
CJK occupies two display columns, they do look aligned, but in my
Chrome showing news.gmane.org/gmane.comp.version-control.git/, they
do look jagged.  When these lines shown in the browser get copied
and pasted to gedit, they still look jagged, but after saving it to
a file and catting it to the same fixed-column terminal, they are
perfectly aligned.

Font issues, I guess?

>> +               /*
>> +                * Add space paddings to time_buf to display a fixed width
>> +                * string, and use time_col for display width calibration.
>> +                */
>> +               time_col = utf8_strwidth(time_str);
>> +               memset(time_buf + time_len, ' ', blame_date_width - time_col);
>> +               *(time_buf + time_len + blame_date_width - time_col) = 0;
>
> And you may want to turn time_buf[128] to strbuf as well while you're at it.

Good eyes.

Re: [PATCH] blame: add correct paddings in time_buf for align

[Duy Nguyen]

On Sat, Apr 19, 2014 at 12:08 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Duy Nguyen <pclouds@gmail.com> writes:
>
>> On Fri, Apr 18, 2014 at 3:44 PM, Jiang Xin <worldhello.net@gmail.com> wrote:
>>> When show blame information with relative time, the UTF-8 characters in
>>> `time_str` will break the alignment of columns after the date field.
>>> This is because the `time_buf` padding with spaces should have a
>>> constant display width, not a fixed strlen size.  So we should call
>>> utf8_strwidth() instead of strlen() for calibration.
>>>
>>> Signed-off-by: Jiang Xin <worldhello.net@gmail.com>
>>> ---
>>>
>>> Before applying this patch:
>>>
>>>         5817da01 builtin-blame.c   (Pierre Habouzit             6 年前                         21) #include "parse-options.h"
>>>         ffaf9cc0 builtin-blame.c   (Geoffrey Thomas             5 年前                         22) #include "utf8.h"
>>>         3b8a12e8 builtin/blame.c   (Axel Bonnet                 3 年 10 个月之前            23) #include "userdiff.h"
>>>         25ed3412 builtin/blame.c   (Bo Yang                     1 年 1 个月之前             24) #include "line-range.h"
>>>         58dbfa2e builtin/blame.c   (Eric Sunshine               9 个月之前                   25) #include "line-log.h"
>>>         cee7f245 builtin-pickaxe.c (Junio C Hamano              8 年前                         26)
>>>
>>> After applying this patch:
>>>
>>>         5817da01 builtin-blame.c   (Pierre Habouzit             6 年前                           21) #include "parse-options.h"
>>>         ffaf9cc0 builtin-blame.c   (Geoffrey Thomas             5 年前                           22) #include "utf8.h"
>>>         3b8a12e8 builtin/blame.c   (Axel Bonnet                 3 年 10 个月之前                 23) #include "userdiff.h"
>>>         25ed3412 builtin/blame.c   (Bo Yang                     1 年 1 个月之前                  24) #include "line-range.h"
>>>         58dbfa2e builtin/blame.c   (Eric Sunshine               9 个月之前                       25) #include "line-log.h"
>>>         cee7f245 builtin-pickaxe.c (Junio C Hamano              8 年前                           26)
>>
>>
>> The numbers 21..26 still do not look aligned, both in gmail raw
>> message view and gmane.
>
> Interesting.  In my GNUS/Emacs on a fixed-column terminal where an
> CJK occupies two display columns, they do look aligned, but in my
> Chrome showing news.gmane.org/gmane.comp.version-control.git/, they
> do look jagged.  When these lines shown in the browser get copied
> and pasted to gedit, they still look jagged, but after saving it to
> a file and catting it to the same fixed-column terminal, they are
> perfectly aligned.
>
> Font issues, I guess?

Emacs/x11, gedit and xterm all show aligned listings. So yes probably
font issues. And because it works fine in text-based environment or
editors. I think we're good.
-- 
Duy

[PATCH v2 0/2] peroper align of datetime filed of git-blame

[Jiang Xin]

When rewrite time_buf[128] to strbuf, find another bug from the original
implement.  See detail commit log of 1/2.

Jiang Xin (2):
  bugfix: fix broken time_buf paddings for git-blame
  blame: use different blame_date_width for different locale

 builtin/blame.c | 29 +++++++++++++++++++++++------
 1 file changed, 23 insertions(+), 6 deletions(-)

-- 
2.0.0.rc0.3.g444188f.dirty

[PATCH v2 1/2] bugfix: fix broken time_buf paddings for git-blame

[Jiang Xin]

When `git blame` shows date field in a fixed width (as long as
blame_date_width characters), if time_str shorter than that, add spaces
for padding.  But there are two bugs in the following codes:

        memcpy(time_buf, time_str, time_len);
        memset(time_buf + time_len, ' ', blame_date_width - time_len);

1. The type of blame_date_width is size_t (unsigned int).  If time_len
   is greater than blame_ate_width, "blame_date_width - time_len" will
   never be a negative number, but a really big positive number, and
   will cause memory overwrite.

   This bug can be triggered if either l10n message for function
   show_date_relative() in date.c is longer then 30 charcters, then
   `git blame --date relative` may fail.

2. When show blame information with relative time, the UTF-8 characters
   in time_str will break the alignment of columns after the date field.
   This is because the time_buf padding with spaces should have a
   constant display width, not a fixed strlen size.  So we should call
   utf8_strwidth() instead of strlen() for calibration.

Signed-off-by: Jiang Xin <worldhello.net@gmail.com>
---
 builtin/blame.c | 19 ++++++++++++++-----
 1 file changed, 14 insertions(+), 5 deletions(-)

diff --git a/builtin/blame.c b/builtin/blame.c
index 88cb799..0a0a858 100644
--- a/builtin/blame.c
+++ b/builtin/blame.c
@@ -1556,22 +1556,31 @@ static void assign_blame(struct scoreboard *sb, int opt)
 static const char *format_time(unsigned long time, const char *tz_str,
 			       int show_raw_time)
 {
-	static char time_buf[128];
+	static struct strbuf time_buf = STRBUF_INIT;
 
+	strbuf_reset(&time_buf);
 	if (show_raw_time) {
-		snprintf(time_buf, sizeof(time_buf), "%lu %s", time, tz_str);
+		strbuf_addf(&time_buf, "%lu %s", time, tz_str);
 	}
 	else {
 		const char *time_str;
+		size_t time_width;
 		int time_len;
 		int tz;
 		tz = atoi(tz_str);
 		time_str = show_date(time, tz, blame_date_mode);
 		time_len = strlen(time_str);
-		memcpy(time_buf, time_str, time_len);
-		memset(time_buf + time_len, ' ', blame_date_width - time_len);
+		strbuf_add(&time_buf, time_str, time_len);
+		/*
+		 * Add space paddings to time_buf to display a fixed width
+		 * string, and use time_width for display width calibration.
+		 */
+		for (time_width = utf8_strwidth(time_str);
+		     time_width < blame_date_width;
+		     time_width++)
+			strbuf_addch(&time_buf, ' ');
 	}
-	return time_buf;
+	return time_buf.buf;
 }
 
 #define OUTPUT_ANNOTATE_COMPAT	001
-- 
2.0.0.rc0.3.g444188f.dirty

Re: [PATCH v2 1/2] bugfix: fix broken time_buf paddings for git-blame

[Eric Sunshine]

On Sun, Apr 20, 2014 at 12:13 PM, Jiang Xin <worldhello.net@gmail.com> wrote:
> When `git blame` shows date field in a fixed width (as long as

s/fixed width/fixed-width/

s/long/wide/ would read a bit better.

> blame_date_width characters), if time_str shorter than that, add spaces

s/shorter/is shorter/
s/add/it adds/

> for padding.  But there are two bugs in the following codes:
>
>         memcpy(time_buf, time_str, time_len);
>         memset(time_buf + time_len, ' ', blame_date_width - time_len);
>
> 1. The type of blame_date_width is size_t (unsigned int).  If time_len

s/(unsigned int)/, which is unsigned/

>    is greater than blame_ate_width, "blame_date_width - time_len" will

s/_ate/_date/

>    never be a negative number, but a really big positive number, and
>    will cause memory overwrite.
>
>    This bug can be triggered if either l10n message for function
>    show_date_relative() in date.c is longer then 30 charcters, then

s/then 30/than 30/
s/charcters/characters/

>    `git blame --date relative` may fail.
>
> 2. When show blame information with relative time, the UTF-8 characters
>    in time_str will break the alignment of columns after the date field.
>    This is because the time_buf padding with spaces should have a
>    constant display width, not a fixed strlen size.  So we should call
>    utf8_strwidth() instead of strlen() for calibration.
>
> Signed-off-by: Jiang Xin <worldhello.net@gmail.com>
> ---
>  builtin/blame.c | 19 ++++++++++++++-----
>  1 file changed, 14 insertions(+), 5 deletions(-)
>
> diff --git a/builtin/blame.c b/builtin/blame.c
> index 88cb799..0a0a858 100644
> --- a/builtin/blame.c
> +++ b/builtin/blame.c
> @@ -1556,22 +1556,31 @@ static void assign_blame(struct scoreboard *sb, int opt)
>  static const char *format_time(unsigned long time, const char *tz_str,
>                                int show_raw_time)
>  {
> -       static char time_buf[128];
> +       static struct strbuf time_buf = STRBUF_INIT;
>
> +       strbuf_reset(&time_buf);
>         if (show_raw_time) {
> -               snprintf(time_buf, sizeof(time_buf), "%lu %s", time, tz_str);
> +               strbuf_addf(&time_buf, "%lu %s", time, tz_str);
>         }
>         else {
>                 const char *time_str;
> +               size_t time_width;
>                 int time_len;
>                 int tz;
>                 tz = atoi(tz_str);
>                 time_str = show_date(time, tz, blame_date_mode);
>                 time_len = strlen(time_str);
> -               memcpy(time_buf, time_str, time_len);
> -               memset(time_buf + time_len, ' ', blame_date_width - time_len);
> +               strbuf_add(&time_buf, time_str, time_len);
> +               /*
> +                * Add space paddings to time_buf to display a fixed width
> +                * string, and use time_width for display width calibration.
> +                */
> +               for (time_width = utf8_strwidth(time_str);
> +                    time_width < blame_date_width;
> +                    time_width++)
> +                       strbuf_addch(&time_buf, ' ');
>         }
> -       return time_buf;
> +       return time_buf.buf;
>  }
>
>  #define OUTPUT_ANNOTATE_COMPAT 001
> --
> 2.0.0.rc0.3.g444188f.dirty

[PATCH v2 2/2] blame: use different blame_date_width for different locale

[Jiang Xin]

When show date in relative date format for `git blame`, the max display
width of datetime is set as the length of the string "Thu Oct 19
16:00:04 2006 -0700" (30 characters long).  But actually the max width
for C locale is only 22 (the length of string "x years, xx months ago").
And for other locale, it maybe smaller.  E.g. For Chinese locale, only
needs 16-character width.

Set blame_date_width as the display width of _("4 years, 11 months
ago"), so that translators can make the choice.

Signed-off-by: Jiang Xin <worldhello.net@gmail.com>
---
 builtin/blame.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/builtin/blame.c b/builtin/blame.c
index 0a0a858..9350ea3 100644
--- a/builtin/blame.c
+++ b/builtin/blame.c
@@ -2340,7 +2340,15 @@ parse_done:
 		blame_date_width = sizeof("2006-10-19");
 		break;
 	case DATE_RELATIVE:
-		/* "normal" is used as the fallback for "relative" */
+		/* TRANSLATORS: what we care about is not the content itself,
+		   but the display width of this string.  We use the width of
+		   the string as the max width of the datetime in relative
+		   format.  For English and many other languages, "4 years,
+		   11 months ago" is the longest one among "89 seconds ago",
+		   "89 minites ago", "35 hours ago", "13 days ago", "10 weeks
+		   ago", "in the future" and many others. */
+		blame_date_width = utf8_strwidth(_("4 years, 11 months ago")) + 1; /* add the null */
+		break;
 	case DATE_LOCAL:
 	case DATE_NORMAL:
 		blame_date_width = sizeof("Thu Oct 19 16:00:04 2006 -0700");
-- 
2.0.0.rc0.3.g444188f.dirty

Re: [PATCH v2 2/2] blame: use different blame_date_width for different locale

[Junio C Hamano]

Jiang Xin <worldhello.net@gmail.com> writes:

> When show date in relative date format for `git blame`, the max display
> width of datetime is set as the length of the string "Thu Oct 19
> 16:00:04 2006 -0700" (30 characters long).  But actually the max width
> for C locale is only 22 (the length of string "x years, xx months ago").
> And for other locale, it maybe smaller.  E.g. For Chinese locale, only
> needs 16-character width.
>
> Set blame_date_width as the display width of _("4 years, 11 months
> ago"), so that translators can make the choice.
>
> Signed-off-by: Jiang Xin <worldhello.net@gmail.com>
> ---
>  builtin/blame.c | 10 +++++++++-
>  1 file changed, 9 insertions(+), 1 deletion(-)
>
> diff --git a/builtin/blame.c b/builtin/blame.c
> index 0a0a858..9350ea3 100644
> --- a/builtin/blame.c
> +++ b/builtin/blame.c
> @@ -2340,7 +2340,15 @@ parse_done:
>  		blame_date_width = sizeof("2006-10-19");
>  		break;
>  	case DATE_RELATIVE:
> -		/* "normal" is used as the fallback for "relative" */
> +		/* TRANSLATORS: what we care about is not the content itself,
> +		   but the display width of this string.  We use the width of
> +		   the string as the max width of the datetime in relative
> +		   format.  For English and many other languages, "4 years,
> +		   11 months ago" is the longest one among "89 seconds ago",
> +		   "89 minites ago", "35 hours ago", "13 days ago", "10 weeks
> +		   ago", "in the future" and many others. */
> +		blame_date_width = utf8_strwidth(_("4 years, 11 months ago")) + 1; /* add the null */
> +		break;


This is not wrong per-se, but I am not sure if translators would
understand that "years and months ago" may not be the longuest
variant for their language and they are asked to use "89 seconds
ago" if the translation of that is longer than the translation for
"4 years, 11 months ago" in their language, with the given
explanation.

How about dropping the second sentence "We use..." and end the
comment with something like:

	ago", "in the future" and many others.  It is possible that
	the translation of "89 seconds ago" may be the longest
	possible translation in your language (then by definition,
	it would be longer than the translation of this string in
	your language), and in such a case, please write the
	translation of "89 seconds ago" here.

Actually, even though "many others" is correct, the possibilities
are within a bounded set of template i18n formats, no?  I wonder if
it would be nicer to the translators if we did not use a single
"representative string" here, but add a helper function to date.c
that goes something like:

    int date_relative_maxwidth(void)
    {
        struct strbuf buf = STRBUF_INIT;
        static int length;

        if (length)
            return length;

        strbuf_addf(&buf, _("in the future"));
        length = (length < buf.len) ? buf.len : length;
        strbuf_addf(&buf, Q_("%lu second ago", "%lu seconds ago", 89), 89);
        length = (length < buf.len) ? buf.len : length;
        ...
        strbuf_addf(&buf, Q_("%lu year ago", "%lu years ago", 9999), 9999);
        length = (length < buf.len) ? buf.len : length;

        return length;
    }

immediately after the definition of show_date_relative() and used it
in this codepath?

Of course, given a date that is far enough into the future, the
actuall length is unbounded, so this approach will not come up with
the absolute minimum width to align all lines (you would need to do
a two pass, measuring how wide all timestamps that would appear in
the output before producing the first line of the output---but I do
not think it is worth it, and as long as we do not overflow the
buffer, showing occasional misalignment for a line that came from
year 27342 is much better than a worse latency-to-first-output).

>  	case DATE_LOCAL:
>  	case DATE_NORMAL:
>  		blame_date_width = sizeof("Thu Oct 19 16:00:04 2006 -0700");

[PATCH v3 0/2] peroper align of datetime filed of git-blame

[Jiang Xin]

Changes since v2:

 * Fixed typos in commit log 1/2. Thanks, Eric.

 * With the help of Junio, write a helper to get suitable blame_date_width.

Jiang Xin (2):
  bugfix: fix broken time_buf paddings for git-blame
  blame: use a helper to get suitable blame_date_width

 builtin/blame.c | 85 +++++++++++++++++++++++++++++++++++++++++++++++++++------
 1 file changed, 77 insertions(+), 8 deletions(-)

-- 
1.9.2.476.ga74def0

[PATCH v3 1/2] bugfix: fix broken time_buf paddings for git-blame

[Jiang Xin]

Command `git blame --date relative` aligns the date field with a
fixed-width (defined by blame_date_width), and if time_str is shorter
than that, it adds spaces for padding.  But there are two bugs in the
following codes:

        time_len = strlen(time_str);
        ...
        memset(time_buf + time_len, ' ', blame_date_width - time_len);

1. The type of blame_date_width is size_t, which is unsigned.  If
   time_len is greater than blame_date_width, the result of
   "blame_date_width - time_len" will never be a negative number, but a
   really big positive number, and will cause memory overwrite.

   This bug can be triggered if either l10n message for function
   show_date_relative() in date.c is longer than 30 characters, then
   `git blame --date relative` may exit abnormally.

2. When show blame information with relative time, the UTF-8 characters
   in time_str will break the alignment of columns after the date field.
   This is because the time_buf padding with spaces should have a
   constant display width, not a fixed strlen size.  So we should call
   utf8_strwidth() instead of strlen() for width calibration.

Helped-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
Helped-by: Eric Sunshine <sunshine@sunshineco.com>
Signed-off-by: Jiang Xin <worldhello.net@gmail.com>
---
 builtin/blame.c | 21 ++++++++++++++-------
 1 file changed, 14 insertions(+), 7 deletions(-)

diff --git a/builtin/blame.c b/builtin/blame.c
index 88cb799..35e95db 100644
--- a/builtin/blame.c
+++ b/builtin/blame.c
@@ -1556,22 +1556,29 @@ static void assign_blame(struct scoreboard *sb, int opt)
 static const char *format_time(unsigned long time, const char *tz_str,
 			       int show_raw_time)
 {
-	static char time_buf[128];
+	static struct strbuf time_buf = STRBUF_INIT;
 
+	strbuf_reset(&time_buf);
 	if (show_raw_time) {
-		snprintf(time_buf, sizeof(time_buf), "%lu %s", time, tz_str);
+		strbuf_addf(&time_buf, "%lu %s", time, tz_str);
 	}
 	else {
 		const char *time_str;
-		int time_len;
+		size_t time_width;
 		int tz;
 		tz = atoi(tz_str);
 		time_str = show_date(time, tz, blame_date_mode);
-		time_len = strlen(time_str);
-		memcpy(time_buf, time_str, time_len);
-		memset(time_buf + time_len, ' ', blame_date_width - time_len);
+		strbuf_addstr(&time_buf, time_str);
+		/*
+		 * Add space paddings to time_buf to display a fixed width
+		 * string, and use time_width for display width calibration.
+		 */
+		for (time_width = utf8_strwidth(time_str);
+		     time_width < blame_date_width;
+		     time_width++)
+			strbuf_addch(&time_buf, ' ');
 	}
-	return time_buf;
+	return time_buf.buf;
 }
 
 #define OUTPUT_ANNOTATE_COMPAT	001
-- 
1.9.2.476.ga74def0

[PATCH v3 2/2] blame: use a helper to get suitable blame_date_width

[Jiang Xin]

When show date in relative date format for git-blame, the max display
width of datetime is set as the length of the string "Thu Oct 19
16:00:04 2006 -0700" (30 characters long).  But actually the max width
for C locale is only 22 (the length of string "x years, xx months ago").
And for other locale, it maybe smaller.  E.g. For Chinese locale, only
needs a half (16-character width).

Add a helper function date_relative_maxwidth() to date.c, which returns
the suitable display width for the relative date field in different
locale.

Suggested-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Jiang Xin <worldhello.net@gmail.com>
---
 builtin/blame.c | 64 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 63 insertions(+), 1 deletion(-)

diff --git a/builtin/blame.c b/builtin/blame.c
index 35e95db..478f739 100644
--- a/builtin/blame.c
+++ b/builtin/blame.c
@@ -2229,6 +2229,67 @@ static int blame_move_callback(const struct option *option, const char *arg, int
 	return 0;
 }
 
+static int date_relative_maxwidth(void)
+{
+	struct strbuf buf = STRBUF_INIT, years_sb = STRBUF_INIT;
+	static int maxwidth = 0;
+	int width;
+
+	if (maxwidth)
+		return maxwidth;
+
+	strbuf_addf(&buf, _("in the future"));
+	maxwidth = utf8_strwidth(buf.buf);
+
+	strbuf_reset(&buf);
+	strbuf_addf(&buf, Q_("%lu second ago", "%lu seconds ago", 89L), 89L);
+	width = utf8_strwidth(buf.buf);
+	maxwidth = (maxwidth < width) ? width : maxwidth;
+
+	strbuf_reset(&buf);
+	strbuf_addf(&buf, Q_("%lu minute ago", "%lu minutes ago", 89L), 89L);
+	width = utf8_strwidth(buf.buf);
+	maxwidth = (maxwidth < width) ? width : maxwidth;
+
+	strbuf_reset(&buf);
+	strbuf_addf(&buf, Q_("%lu hour ago", "%lu hours ago", 35L), 35L);
+	width = utf8_strwidth(buf.buf);
+	maxwidth = (maxwidth < width) ? width : maxwidth;
+
+	strbuf_reset(&buf);
+	strbuf_addf(&buf, Q_("%lu day ago", "%lu days ago", 13L), 13L);
+	width = utf8_strwidth(buf.buf);
+	maxwidth = (maxwidth < width) ? width : maxwidth;
+
+	strbuf_reset(&buf);
+	strbuf_addf(&buf, Q_("%lu week ago", "%lu weeks ago", 10L), 10L);
+	width = utf8_strwidth(buf.buf);
+	maxwidth = (maxwidth < width) ? width : maxwidth;
+
+	strbuf_reset(&buf);
+	strbuf_addf(&buf, Q_("%lu month ago", "%lu months ago", 12L), 12L);
+	width = utf8_strwidth(buf.buf);
+	maxwidth = (maxwidth < width) ? width : maxwidth;
+
+	strbuf_addf(&years_sb, Q_("%lu year", "%lu years", 4L), 4L);
+	strbuf_reset(&buf);
+	strbuf_addf(&buf,
+		Q_("%s, %lu month ago", "%s, %lu months ago", 11L),
+		years_sb.buf, 11L);
+	width = utf8_strwidth(buf.buf);
+	maxwidth = (maxwidth < width) ? width : maxwidth;
+
+	strbuf_reset(&buf);
+	strbuf_addf(&buf, Q_("%lu year ago", "%lu years ago", 9999L), 9999L);
+	width = utf8_strwidth(buf.buf);
+	maxwidth = (maxwidth < width) ? width : maxwidth;
+
+	strbuf_release(&years_sb);
+	strbuf_release(&buf);
+
+	return maxwidth;
+}
+
 int cmd_blame(int argc, const char **argv, const char *prefix)
 {
 	struct rev_info revs;
@@ -2338,7 +2399,8 @@ parse_done:
 		blame_date_width = sizeof("2006-10-19");
 		break;
 	case DATE_RELATIVE:
-		/* "normal" is used as the fallback for "relative" */
+		blame_date_width = date_relative_maxwidth() + 1; /* add the null */
+		break;
 	case DATE_LOCAL:
 	case DATE_NORMAL:
 		blame_date_width = sizeof("Thu Oct 19 16:00:04 2006 -0700");
-- 
1.9.2.476.ga74def0

Re: [PATCH v3 2/2] blame: use a helper to get suitable blame_date_width

[Junio C Hamano]

Jiang Xin <worldhello.net@gmail.com> writes:

> When show date in relative date format for git-blame, the max display
> width of datetime is set as the length of the string "Thu Oct 19
> 16:00:04 2006 -0700" (30 characters long).  But actually the max width
> for C locale is only 22 (the length of string "x years, xx months ago").
> And for other locale, it maybe smaller.  E.g. For Chinese locale, only
> needs a half (16-character width).
>
> Add a helper function date_relative_maxwidth() to date.c, which returns
> the suitable display width for the relative date field in different
> locale.
>
> Suggested-by: Junio C Hamano <gitster@pobox.com>
> Signed-off-by: Jiang Xin <worldhello.net@gmail.com>
> ---
>  builtin/blame.c | 64 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 63 insertions(+), 1 deletion(-)

That does not seem to have the helper in date.c immediately next to
the definition of show_date_relative(), as the above log message
claims, does it?

I was hoping that you would respond with a set of counter-arugments
to shoot the suggestion down.  Here are a few obvious ones:

 - By inlining the implementation of show_date_relative() in the new
   helper function, we would end up having to maintain two copies of
   every format strings.

 - Even if we don't inline the logic and duplicate the format
   strings, and call show_date_relative() with some predetermined
   offsets instead, e.g.

	static int date_relative_maxwidth(void)
	{
                struct timebuf = STRBUF_INIT;
		struct timeval now;
                static int maxwidth = 0;

                gettimeofday(&now, NULL);

                /* in the future */
                show_date_relative(now->tv_sec + 100, 0, &now, &timebuf);
                maxwidth = maxwidth < timebuf.len ? timebuf.len : maxwidth;
                strbuf_reset(&timebuf);

		/* seconds ago */
                show_date_relative(now->tv_sec - 89, 0, &now, &timebuf);
                maxwidth = maxwidth < timebuf.len ? timebuf.len : maxwidth;
                strbuf_reset(&timebuf);

                ...
	}

   we still end up hardcoding the logic in the code.

 - There is no guarantee that these predetermined offsets would
   produce the longest possible timestamp for the target language
   with these gettext strings.  In a language where the noun
   "second" takes a lot longer shape for singular than the plural
   (e.g. "1 SECOND" vs "2 SEC", perhaps), checking for 89 seconds
   ago may not produce the longest string for "%lu seconds ago".
   The approach, "we compute everything for translators", sounds
   nice in theory, but may not work well in practice and the worst
   part is that there is no way for translators to work it around,
   unlike your original patch.

So after sleeping on the idea overnight, I think I like the
simplicity of your original patch better.  It just needs to be
explained well for translators to understand.

Sorry for making you go off exploring in a strange direction.

When msgmerge is run to update XX.po with a new version of git.pot,
does it destroy comments an earlier translator placed in XX.po, or
are they kept intact?  What I am wondering is if we can do something
like this:

    In code:

        blame_date_width = strtoul(_("22 (C)"), NULL, 10) + 1; /* add the null */

    In git.pot:

        #. This string encodes the preferred maximum display width for a
        #. relative timestamp in "git blame" output.  For C locale, "4 years,
        #. 11 months ago", which takes 22 places, is the longest among various
        #. forms of relative timestamps, but your languate may need more or
        #. fewer display columns.  If "zh_CN" locale needs 14 display columns to
        #. hold any relative timestamp in the reasonably near past, for
        #. example, translate this string as "14 (zh_CN)".
        msgid "22 (C)"
        msgstr ""

    In de.po:
        #. This string encodes the preferred maximum display width for a
        #. relative timestamp in "git blame" output.  For C locale, "4 years,
        #. 11 months ago", which takes 22 places, is the longest among various
        #. forms of relative timestamps, but your languate may need more or
        #. fewer display columns.  If zh_CN locale needs 14 display columns to
        #. hold any relative timestamp in the reasonably near past, for
        #. example, translate this string as "14 (zh_CN)".
	#.
	#. In de locale, "vor %lu Jahren, und %lu Monaten" is the
        #. longest and fits within 28 display columns.
        msgid "22 (C)"
        msgstr "28 (de)"

where the instructions for tranlators to various languages come from
git.pot and the translator to a specific language can describe which
variant in the language described in XX.po is the longest.  For that
to work well, the last two lines in the comment an earlier "de"
translator added need to be preserved across msgmerge, though.

Re: [PATCH v3 2/2] blame: use a helper to get suitable blame_date_width

[Junio C Hamano]

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

> What I am wondering is if we can do something
> like this:
> ...

Nah, that is a lot more stupid than just doing

>     In code:
>
>         blame_date_width = strtoul(_("4 years, 11 months ago"), NULL, 10) + 1;
>
>     In git.pot:
>
>         #. This string is used to tell us the maximum display width for a
>         #. relative timestamp in "git blame" output.  For C locale, "4 years,
>         #. 11 months ago", which takes 22 places, is the longest among various
>         #. forms of relative timestamps, but your language may need more or
>         #. fewer display columns.
>         msgid "4 years, 11 months ago"
>         msgstr ""
>
>     In de.po:
>         #. This string is used to tell us the maximum display width for a
>         #. relative timestamp in "git blame" output.  For C locale, "4 years,
>         #. 11 months ago", which takes 22 places, is the longest among various
>         #. forms of relative timestamps, but your language may need more or
>         #. fewer display columns.
>         msgid "4 years, 11 months ago"
>         msgstr ""vor 4 Jahren, und 11 Monaten"

which is essentially how your very original looked like (modulo the
comments).  So let's not try to be clever or cute, and just have a
good instruction in the TRANSLATORS comments.

Sorry for flipping and flopping on this one.

Re: [PATCH v3 2/2] blame: use a helper to get suitable blame_date_width

[Jiang Xin]

2014-04-22 3:19 GMT+08:00 Junio C Hamano <gitster@pobox.com>:
> Junio C Hamano <gitster@pobox.com> writes:
>
>> What I am wondering is if we can do something
>> like this:
>> ...
>
> Nah, that is a lot more stupid than just doing
>
>>     In code:
>>
>>         blame_date_width = strtoul(_("4 years, 11 months ago"), NULL, 10) + 1;
>>
>>     In git.pot:
>>
>>         #. This string is used to tell us the maximum display width for a
>>         #. relative timestamp in "git blame" output.  For C locale, "4 years,
>>         #. 11 months ago", which takes 22 places, is the longest among various
>>         #. forms of relative timestamps, but your language may need more or
>>         #. fewer display columns.
>>         msgid "4 years, 11 months ago"
>>         msgstr ""
>>
>>     In de.po:
>>         #. This string is used to tell us the maximum display width for a
>>         #. relative timestamp in "git blame" output.  For C locale, "4 years,
>>         #. 11 months ago", which takes 22 places, is the longest among various
>>         #. forms of relative timestamps, but your language may need more or
>>         #. fewer display columns.
>>         msgid "4 years, 11 months ago"
>>         msgstr ""vor 4 Jahren, und 11 Monaten"
>
> which is essentially how your very original looked like (modulo the
> comments).  So let's not try to be clever or cute, and just have a
> good instruction in the TRANSLATORS comments.
>
> Sorry for flipping and flopping on this one.

I will send patch v4 tonight, and I think all l10n team leaders should
pay attention
to this thread:

 * http://thread.gmane.org/gmane.comp.version-control.git/246464

-- 
Jiang Xin

[PATCH v4 0/2] peroper align of datetime filed of git-blame

[Jiang Xin]

Changes since V3:

 * rollback patch 2/2 to v2, but with a nicer comment for translators.

Jiang Xin (2):
  bugfix: fix broken time_buf paddings for git-blame
  blame: dynamic blame_date_width for different locales

 builtin/blame.c | 30 ++++++++++++++++++++++--------
 1 file changed, 22 insertions(+), 8 deletions(-)

-- 
1.9.2.476.gff10cf3.dirty

[PATCH v4 1/2] bugfix: fix broken time_buf paddings for git-blame

[Jiang Xin]

Command `git blame --date relative` aligns the date field with a
fixed-width (defined by blame_date_width), and if time_str is shorter
than that, it adds spaces for padding.  But there are two bugs in the
following codes:

        time_len = strlen(time_str);
        ...
        memset(time_buf + time_len, ' ', blame_date_width - time_len);

1. The type of blame_date_width is size_t, which is unsigned.  If
   time_len is greater than blame_date_width, the result of
   "blame_date_width - time_len" will never be a negative number, but a
   really big positive number, and will cause memory overwrite.

   This bug can be triggered if either l10n message for function
   show_date_relative() in date.c is longer than 30 characters, then
   `git blame --date relative` may exit abnormally.

2. When show blame information with relative time, the UTF-8 characters
   in time_str will break the alignment of columns after the date field.
   This is because the time_buf padding with spaces should have a
   constant display width, not a fixed strlen size.  So we should call
   utf8_strwidth() instead of strlen() for width calibration.

Helped-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
Helped-by: Eric Sunshine <sunshine@sunshineco.com>
Signed-off-by: Jiang Xin <worldhello.net@gmail.com>
---
 builtin/blame.c | 21 ++++++++++++++-------
 1 file changed, 14 insertions(+), 7 deletions(-)

diff --git a/builtin/blame.c b/builtin/blame.c
index 88cb799..35e95db 100644
--- a/builtin/blame.c
+++ b/builtin/blame.c
@@ -1556,22 +1556,29 @@ static void assign_blame(struct scoreboard *sb, int opt)
 static const char *format_time(unsigned long time, const char *tz_str,
 			       int show_raw_time)
 {
-	static char time_buf[128];
+	static struct strbuf time_buf = STRBUF_INIT;
 
+	strbuf_reset(&time_buf);
 	if (show_raw_time) {
-		snprintf(time_buf, sizeof(time_buf), "%lu %s", time, tz_str);
+		strbuf_addf(&time_buf, "%lu %s", time, tz_str);
 	}
 	else {
 		const char *time_str;
-		int time_len;
+		size_t time_width;
 		int tz;
 		tz = atoi(tz_str);
 		time_str = show_date(time, tz, blame_date_mode);
-		time_len = strlen(time_str);
-		memcpy(time_buf, time_str, time_len);
-		memset(time_buf + time_len, ' ', blame_date_width - time_len);
+		strbuf_addstr(&time_buf, time_str);
+		/*
+		 * Add space paddings to time_buf to display a fixed width
+		 * string, and use time_width for display width calibration.
+		 */
+		for (time_width = utf8_strwidth(time_str);
+		     time_width < blame_date_width;
+		     time_width++)
+			strbuf_addch(&time_buf, ' ');
 	}
-	return time_buf;
+	return time_buf.buf;
 }
 
 #define OUTPUT_ANNOTATE_COMPAT	001
-- 
1.9.2.476.gff10cf3.dirty

[PATCH v4 2/2] blame: dynamic blame_date_width for different locales

[Jiang Xin]

When show date in relative date format for git-blame, the max display
width of datetime is set as the length of the string "Thu Oct 19
16:00:04 2006 -0700" (30 characters long).  But actually the max width
for C locale is only 22 (the length of string "x years, xx months ago").
And for other locale, it maybe smaller.  E.g. For Chinese locale, only
needs a half (16-character width).

Set blame_date_width as the display width of _("4 years, 11 months
ago"), so that translators can make the choice.

Helped-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Jiang Xin <worldhello.net@gmail.com>
---
 builtin/blame.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/builtin/blame.c b/builtin/blame.c
index 35e95db..128fc64 100644
--- a/builtin/blame.c
+++ b/builtin/blame.c
@@ -2338,7 +2338,14 @@ parse_done:
 		blame_date_width = sizeof("2006-10-19");
 		break;
 	case DATE_RELATIVE:
-		/* "normal" is used as the fallback for "relative" */
+		/* TRANSLATORS: This string is used to tell us the maximum
+		   display width for a relative timestamp in "git blame"
+		   output.  For C locale, "4 years, 11 months ago", which
+		   takes 22 places, is the longest among various forms of
+		   relative timestamps, but your language may need more or
+		   fewer display columns. */
+		blame_date_width = utf8_strwidth(_("4 years, 11 months ago")) + 1; /* add the null */
+		break;
 	case DATE_LOCAL:
 	case DATE_NORMAL:
 		blame_date_width = sizeof("Thu Oct 19 16:00:04 2006 -0700");
-- 
1.9.2.476.gff10cf3.dirty

Re: [PATCH v2 2/2] blame: use different blame_date_width for different locale

[David Kastrup]

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

> This is not wrong per-se, but I am not sure if translators would
> understand that "years and months ago" may not be the longuest
> variant for their language and they are asked to use "89 seconds
> ago" if the translation of that is longer than the translation for
> "4 years, 11 months ago" in their language, with the given
> explanation.

What's with the 89?  And the other semi-magic numbers?  If we fear about
non-arabic number formatting, at least in French French the worst
offender may be quatre-vingt-quatorze ("four score and fourteen") or
quatre-vingt-dix-neuf ("four score and nineteen"), namely 94 or 99.  But
I think it's improbable to get worded formatting here anyway.  Or are
those the largest values with their respective granularity?

-- 
David Kastrup

Re: [PATCH v2 2/2] blame: use different blame_date_width for different locale

[Jiang Xin]

2014-04-22 18:01 GMT+08:00 David Kastrup <dak@gnu.org>:
> Junio C Hamano <gitster@pobox.com> writes:
>
>> This is not wrong per-se, but I am not sure if translators would
>> understand that "years and months ago" may not be the longuest
>> variant for their language and they are asked to use "89 seconds
>> ago" if the translation of that is longer than the translation for
>> "4 years, 11 months ago" in their language, with the given
>> explanation.
>
> What's with the 89?  And the other semi-magic numbers?

Not something magic, just what show_date_relative() in date.c is implemented:

    98          diff = now->tv_sec - time;
    99          if (diff < 90) {
    100                 strbuf_addf(timebuf,
    101                          Q_("%lu second ago", "%lu seconds
ago", diff), diff);
    102                 return;
    103         }
    104         /* Turn it into minutes */
    105         diff = (diff + 30) / 60;
    106         if (diff < 90) {
    107                 strbuf_addf(timebuf,
    108                          Q_("%lu minute ago", "%lu minutes
ago", diff), diff);
    109                 return;
    110         }


-- 
Jiang Xin