blame: add correct paddings in time_buf for align
PATCH 1/1 blame: add correct paddings in time_buf for align
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.
---
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(-)
builtin/blame.c +8 -1
@@ -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; }
Signature · 1 line
1.9.2.474.g17b2a16
Quoted 18 lines · Jiang Xin
When show blame information with relative time, the UTF-8 characters in
time_strwill break the alignment of columns after the date field. This is because thetime_bufpadding with spaces should have a constant display width, not a fixed strlen size. So we should callutf8_strwidth()instead ofstrlen()for calibration.Signed-off-by Jiang Xin <[email protected]>---
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)
Quoted 8 lines
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.
Quoted 16 lines
builtin/blame.c | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-)builtin/blame.c +1
-0@@ -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);
builtin/blame.c static const char *format_time(unsigned long time, const char *tz_str, · line 1579
Quoted 10 lines
builtin/blame.c +7
-1time_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.
Quoted 10 lines
builtin/blame.c +0
-1}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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Signature · 1 line
Duy
v2 cover peroper align of datetime filed of git-blame
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(-)
Signature · 1 line
2.0.0.rc0.3.g444188f.dirty
PATCH v2 1/2 bugfix: fix broken time_buf paddings for git-blame
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.
--- builtin/blame.c | 19 ++++++++++++++----- 1 file changed, 14 insertions(+), 5 deletions(-)
builtin/blame.c +14 -5
@@ -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
Signature · 1 line
2.0.0.rc0.3.g444188f.dirty
Re: [PATCH v2 1/2] bugfix: fix broken time_buf paddings for git-blame
Quoted 1 line · Jiang Xin
When
git blameshows date field in a fixed width (as long as
s/fixed width/fixed-width/
s/long/wide/ would read a bit better.
Quoted 1 line
blame_date_widthcharacters), iftime_strshorter than that, add spaces
s/shorter/is shorter/
s/add/it adds/
Quoted 6 lines
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_widthissize_t(unsigned int). Iftime_len
s/(unsigned int)/, which is unsigned/
Quoted 1 line
is greater than
blame_ate_width, "blame_date_width-time_len" will
s/_ate/_date/
Quoted 5 lines
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()indate.cis longer then 30 charcters, then
s/then 30/than 30/
s/charcters/characters/
Quoted 56 lines
git blame --date relativemay fail.2. When show blame information with relative time, the UTF-8 characters in
time_strwill break the alignment of columns after the date field. This is because thetime_bufpadding with spaces should have a constant display width, not a fixed strlen size. So we should callutf8_strwidth()instead ofstrlen()for calibration.Signed-off-by Jiang Xin <[email protected]>--- builtin/blame.c | 19 ++++++++++++++----- 1 file changed, 14 insertions(+), 5 deletions(-)builtin/blame.c +14
-6@@ -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
In reply to Jiang XinWhen 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.
--- builtin/blame.c | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-)
builtin/blame.c +9 -1
@@ -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" );
Signature · 1 line
2.0.0.rc0.3.g444188f.dirty
Re: [PATCH v2 2/2] blame: use different blame_date_width for different locale
Quoted 23 lines · 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_widthas the display width of_("4 years, 11 months ago"), so that translators can make the choice.Signed-off-by Jiang Xin <[email protected]>--- builtin/blame.c | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-)builtin/blame.c +0
-0@@ -2340,7 +2340,15 @@ parse_done: blame_date_width =sizeof ("2006-10-19" );break ;case DATE_RELATIVE :
builtin/blame.c parse_done: · line 2351
Quoted 10 lines
builtin/blame.c +9
-1-/* "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).
Quoted 3 lines
builtin/blame.c +0
-0case DATE_LOCAL :case DATE_NORMAL : blame_date_width =sizeof ("Thu Oct 19 16:00:04 2006 -0700" );
v3 cover peroper align of datetime filed of git-blame
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(-)
Signature · 1 line
1.9.2.476.ga74def0
PATCH v3 1/2 bugfix: fix broken time_buf paddings for git-blame
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.
--- builtin/blame.c | 21 ++++++++++++++------- 1 file changed, 14 insertions(+), 7 deletions(-)
builtin/blame.c +14 -7
@@ -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
Signature · 1 line
1.9.2.476.ga74def0
PATCH v3 2/2 blame: use a helper to get suitable blame_date_width
In reply to Jiang XinWhen 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.
--- builtin/blame.c | 64 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 63 insertions(+), 1 deletion(-)
builtin/blame.c +63 -1
@@ -2229,6 +2229,67 @@ static int blame_move_callback(const struct option *option, const char *arg, intreturn 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" );
Signature · 1 line
1.9.2.476.ga74def0
Re: [PATCH v3 2/2] blame: use a helper to get suitable blame_date_width
Quoted 11 lines · 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()todate.c, which returns the suitable display width for the relative date field in different locale.
Quoted 5 lines
Suggested-by Junio C Hamano <[email protected]>Signed-off-by Jiang Xin <[email protected]>--- 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
Quoted 3 lines · Junio C Hamano
What I am wondering is if we can do something
like this:
...
Nah, that is a lot more stupid than just doing
Quoted 14 lines
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 ""
Quoted 8 lines
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
2014-04-22 3:19 GMT+08:00 Junio C Hamano <[email protected]>:
Quoted 31 lines
Quoted 3 lines · Junio C Hamano
What I am wondering is if we can do something
like this:
...Nah, that is a lot more stupid than just doing
Quoted 14 lines
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 ""Quoted 8 lines
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"
Quoted 5 lines
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
Signature · 1 line
Jiang Xin
v4 cover peroper align of datetime filed of git-blame
In reply to Junio C HamanoChanges 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(-)
Signature · 1 line
1.9.2.476.gff10cf3.dirty
PATCH v4 1/2 bugfix: fix broken time_buf paddings for git-blame
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.
--- builtin/blame.c | 21 ++++++++++++++------- 1 file changed, 14 insertions(+), 7 deletions(-)
builtin/blame.c +14 -7
@@ -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
Signature · 1 line
1.9.2.476.gff10cf3.dirty
PATCH v4 2/2 blame: dynamic blame_date_width for different locales
In reply to Jiang XinWhen 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.
--- builtin/blame.c | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-)
builtin/blame.c +8 -1
@@ -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" );
Signature · 1 line
1.9.2.476.gff10cf3.dirty
Re: [PATCH v2 2/2] blame: use different blame_date_width for different locale
In reply to Junio C HamanoQuoted 6 lines · Junio C Hamano
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?
Signature · 1 line
David Kastrup
Re: [PATCH v2 2/2] blame: use different blame_date_width for different locale
2014-04-22 18:01 GMT+08:00 David Kastrup <[email protected]>:
Quoted 10 lines
Quoted 6 lines · Junio C Hamano
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 }Signature · 1 line
Jiang Xin
Quoted 29 lines · Duy Nguyen
Quoted 18 lines · Jiang Xin
When show blame information with relative time, the UTF-8 characters in
time_strwill break the alignment of columns after the date field. This is because thetime_bufpadding with spaces should have a constant display width, not a fixed strlen size. So we should callutf8_strwidth()instead ofstrlen()for calibration.Signed-off-by Jiang Xin <[email protected]>---
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)Quoted 8 lines
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)
Quoted 2 lines
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?
Quoted 9 lines
builtin/blame.c static const char *format_time(unsigned long time, const char *tz_str, · line 1579
Quoted 7 lines
builtin/blame.c +7
-0+/* +* 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.
Quoted 42 lines · Junio C Hamano
Quoted 29 lines · Duy Nguyen
Quoted 18 lines · Jiang Xin
When show blame information with relative time, the UTF-8 characters in
time_strwill break the alignment of columns after the date field. This is because thetime_bufpadding with spaces should have a constant display width, not a fixed strlen size. So we should callutf8_strwidth()instead ofstrlen()for calibration.Signed-off-by Jiang Xin <[email protected]>---
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)Quoted 8 lines
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)Quoted 2 lines
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.
Quoted 1 line
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.
Signature · 1 line
Duy