{"thread":{"id":"36435","subject":"[PATCH] blame: add correct paddings in time_buf for align","startedAt":"2014-04-18T08:44:25Z","lastAt":"2014-04-22T14:39:10Z","messageCount":20,"participants":["Jiang Xin","Duy Nguyen","Junio C Hamano","Eric Sunshine","David Kastrup"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"239062","messageId":"43a3735a164c923acc8e6e2681dfbd727cda63de.1397810231.git.worldhello.net@gmail.com","threadId":"36435","inReplyTo":null,"subject":"[PATCH] blame: add correct paddings in time_buf for align","fromName":"Jiang Xin","fromEmail":"worldhello.net@gmail.com","sentAt":"2014-04-18T08:44:25Z","receivedAt":"2014-04-18T08:44:25Z","isPatch":true,"sender":{"key":"worldhello.net@gmail.com","avatar":"https://avatars.githubusercontent.com/u/183860?v=4"},"body":"When show blame information with relative time, the UTF-8 characters in\n`time_str` will break the alignment of columns after the date field.\nThis is because the `time_buf` padding with spaces should have a\nconstant display width, not a fixed strlen size. So we should call\nutf8_strwidth() instead of strlen() for calibration.\n\nSigned-off-by: Jiang Xin \n---\n\nBefore applying this patch:\n\n 5817da01 builtin-blame.c (Pierre Habouzit 6 年前 21) #include \"parse-options.h\"\n ffaf9cc0 builtin-blame.c (Geoffrey Thomas 5 年前 22) #include \"utf8.h\"\n 3b8a12e8 builtin/blame.c (Axel Bonnet 3 年 10 个月之前 23) #include \"userdiff.h\"\n 25ed3412 builtin/blame.c (Bo Yang 1 年 1 个月之前 24) #include \"line-range.h\"\n 58dbfa2e builtin/blame.c (Eric Sunshine 9 个月之前 25) #include \"line-log.h\"\n cee7f245 builtin-pickaxe.c (Junio C Hamano 8 年前 26) \n\nAfter applying this patch:\n\n 5817da01 builtin-blame.c (Pierre Habouzit 6 年前 21) #include \"parse-options.h\"\n ffaf9cc0 builtin-blame.c (Geoffrey Thomas 5 年前 22) #include \"utf8.h\"\n 3b8a12e8 builtin/blame.c (Axel Bonnet 3 年 10 个月之前 23) #include \"userdiff.h\"\n 25ed3412 builtin/blame.c (Bo Yang 1 年 1 个月之前 24) #include \"line-range.h\"\n 58dbfa2e builtin/blame.c (Eric Sunshine 9 个月之前 25) #include \"line-log.h\"\n cee7f245 builtin-pickaxe.c (Junio C Hamano 8 年前 26) \n\n builtin/blame.c | 9 ++++++++-\n 1 file changed, 8 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/blame.c b/builtin/blame.c\nindex 88cb799..c8f6647 100644\n--- a/builtin/blame.c\n+++ b/builtin/blame.c\n@@ -1564,12 +1564,19 @@ static const char *format_time(unsigned long time, const char *tz_str,\n \telse {\n \t\tconst char *time_str;\n \t\tint time_len;\n+\t\tint time_col;\n \t\tint tz;\n \t\ttz = atoi(tz_str);\n \t\ttime_str = show_date(time, tz, blame_date_mode);\n \t\ttime_len = strlen(time_str);\n \t\tmemcpy(time_buf, time_str, time_len);\n-\t\tmemset(time_buf + time_len, ' ', blame_date_width - time_len);\n+\t\t/*\n+\t\t * Add space paddings to time_buf to display a fixed width\n+\t\t * string, and use time_col for display width calibration.\n+\t\t */\n+\t\ttime_col = utf8_strwidth(time_str);\n+\t\tmemset(time_buf + time_len, ' ', blame_date_width - time_col);\n+\t\t*(time_buf + time_len + blame_date_width - time_col) = 0;\n \t}\n \treturn time_buf;\n }\n-- \n1.9.2.474.g17b2a16\n"},{"id":"239070","messageId":"CACsJy8BTBwqFZUU3i3cuv40B6AHw5SRY9DZN2PoKL4XzgEL2eA@mail.gmail.com","threadId":"36435","inReplyTo":"43a3735a164c923acc8e6e2681dfbd727cda63de.1397810231.git.worldhello.net@gmail.com","subject":"Re: [PATCH] blame: add correct paddings in time_buf for align","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2014-04-18T14:42:22Z","receivedAt":"2014-04-18T14:42:22Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Fri, Apr 18, 2014 at 3:44 PM, Jiang Xin wrote:\n> When show blame information with relative time, the UTF-8 characters in\n> `time_str` will break the alignment of columns after the date field.\n> This is because the `time_buf` padding with spaces should have a\n> constant display width, not a fixed strlen size. So we should call\n> utf8_strwidth() instead of strlen() for calibration.\n>\n> Signed-off-by: Jiang Xin \n> ---\n>\n> Before applying this patch:\n>\n> 5817da01 builtin-blame.c (Pierre Habouzit 6 年前 21) #include \"parse-options.h\"\n> ffaf9cc0 builtin-blame.c (Geoffrey Thomas 5 年前 22) #include \"utf8.h\"\n> 3b8a12e8 builtin/blame.c (Axel Bonnet 3 年 10 个月之前 23) #include \"userdiff.h\"\n> 25ed3412 builtin/blame.c (Bo Yang 1 年 1 个月之前 24) #include \"line-range.h\"\n> 58dbfa2e builtin/blame.c (Eric Sunshine 9 个月之前 25) #include \"line-log.h\"\n> cee7f245 builtin-pickaxe.c (Junio C Hamano 8 年前 26)\n>\n> After applying this patch:\n>\n> 5817da01 builtin-blame.c (Pierre Habouzit 6 年前 21) #include \"parse-options.h\"\n> ffaf9cc0 builtin-blame.c (Geoffrey Thomas 5 年前 22) #include \"utf8.h\"\n> 3b8a12e8 builtin/blame.c (Axel Bonnet 3 年 10 个月之前 23) #include \"userdiff.h\"\n> 25ed3412 builtin/blame.c (Bo Yang 1 年 1 个月之前 24) #include \"line-range.h\"\n> 58dbfa2e builtin/blame.c (Eric Sunshine 9 个月之前 25) #include \"line-log.h\"\n> cee7f245 builtin-pickaxe.c (Junio C Hamano 8 年前 26)\n\n\nThe numbers 21..26 still do not look aligned, both in gmail raw\nmessage view and gmane.\n\n>\n> builtin/blame.c | 9 ++++++++-\n> 1 file changed, 8 insertions(+), 1 deletion(-)\n>\n> diff --git a/builtin/blame.c b/builtin/blame.c\n> index 88cb799..c8f6647 100644\n> --- a/builtin/blame.c\n> +++ b/builtin/blame.c\n> @@ -1564,12 +1564,19 @@ static const char *format_time(unsigned long time, const char *tz_str,\n> else {\n> const char *time_str;\n> int time_len;\n> + int time_col;\n> int tz;\n> tz = atoi(tz_str);\n> time_str = show_date(time, tz, blame_date_mode);\n> time_len = strlen(time_str);\n> memcpy(time_buf, time_str, time_len);\n> - memset(time_buf + time_len, ' ', blame_date_width - time_len);\n> + /*\n> + * Add space paddings to time_buf to display a fixed width\n> + * string, and use time_col for display width calibration.\n> + */\n> + time_col = utf8_strwidth(time_str);\n> + memset(time_buf + time_len, ' ', blame_date_width - time_col);\n> + *(time_buf + time_len + blame_date_width - time_col) = 0;\n\nAnd you may want to turn time_buf[128] to strbuf as well while you're at it.\n\n> }\n> return time_buf;\n> }\n> --\n> 1.9.2.474.g17b2a16\n>\n> --\n> To unsubscribe from this list: send the line \"unsubscribe git\" in\n> the body of a message to majordomo@vger.kernel.org\n> More majordomo info at http://vger.kernel.org/majordomo-info.html\n\n\n\n-- \nDuy\n"},{"id":"239073","messageId":"xmqqd2ged0qg.fsf@gitster.dls.corp.google.com","threadId":"36435","inReplyTo":"CACsJy8BTBwqFZUU3i3cuv40B6AHw5SRY9DZN2PoKL4XzgEL2eA@mail.gmail.com","subject":"Re: [PATCH] blame: add correct paddings in time_buf for align","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-04-18T17:08:23Z","receivedAt":"2014-04-18T17:08:23Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Duy Nguyen writes:\n\n> On Fri, Apr 18, 2014 at 3:44 PM, Jiang Xin wrote:\n>> When show blame information with relative time, the UTF-8 characters in\n>> `time_str` will break the alignment of columns after the date field.\n>> This is because the `time_buf` padding with spaces should have a\n>> constant display width, not a fixed strlen size. So we should call\n>> utf8_strwidth() instead of strlen() for calibration.\n>>\n>> Signed-off-by: Jiang Xin \n>> ---\n>>\n>> Before applying this patch:\n>>\n>> 5817da01 builtin-blame.c (Pierre Habouzit 6 年前 21) #include \"parse-options.h\"\n>> ffaf9cc0 builtin-blame.c (Geoffrey Thomas 5 年前 22) #include \"utf8.h\"\n>> 3b8a12e8 builtin/blame.c (Axel Bonnet 3 年 10 个月之前 23) #include \"userdiff.h\"\n>> 25ed3412 builtin/blame.c (Bo Yang 1 年 1 个月之前 24) #include \"line-range.h\"\n>> 58dbfa2e builtin/blame.c (Eric Sunshine 9 个月之前 25) #include \"line-log.h\"\n>> cee7f245 builtin-pickaxe.c (Junio C Hamano 8 年前 26)\n>>\n>> After applying this patch:\n>>\n>> 5817da01 builtin-blame.c (Pierre Habouzit 6 年前 21) #include \"parse-options.h\"\n>> ffaf9cc0 builtin-blame.c (Geoffrey Thomas 5 年前 22) #include \"utf8.h\"\n>> 3b8a12e8 builtin/blame.c (Axel Bonnet 3 年 10 个月之前 23) #include \"userdiff.h\"\n>> 25ed3412 builtin/blame.c (Bo Yang 1 年 1 个月之前 24) #include \"line-range.h\"\n>> 58dbfa2e builtin/blame.c (Eric Sunshine 9 个月之前 25) #include \"line-log.h\"\n>> cee7f245 builtin-pickaxe.c (Junio C Hamano 8 年前 26)\n>\n>\n> The numbers 21..26 still do not look aligned, both in gmail raw\n> message view and gmane.\n\nInteresting. In my GNUS/Emacs on a fixed-column terminal where an\nCJK occupies two display columns, they do look aligned, but in my\nChrome showing news.gmane.org/gmane.comp.version-control.git/, they\ndo look jagged. When these lines shown in the browser get copied\nand pasted to gedit, they still look jagged, but after saving it to\na file and catting it to the same fixed-column terminal, they are\nperfectly aligned.\n\nFont issues, I guess?\n\n>> + /*\n>> + * Add space paddings to time_buf to display a fixed width\n>> + * string, and use time_col for display width calibration.\n>> + */\n>> + time_col = utf8_strwidth(time_str);\n>> + memset(time_buf + time_len, ' ', blame_date_width - time_col);\n>> + *(time_buf + time_len + blame_date_width - time_col) = 0;\n>\n> And you may want to turn time_buf[128] to strbuf as well while you're at it.\n\nGood eyes.\n"},{"id":"239089","messageId":"CACsJy8AS6O2vNQ7FW0ur=49TSfCRourF53YvEpofMpC7ocMDDg@mail.gmail.com","threadId":"36435","inReplyTo":"xmqqd2ged0qg.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] blame: add correct paddings in time_buf for align","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2014-04-19T00:20:36Z","receivedAt":"2014-04-19T00:20:36Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Sat, Apr 19, 2014 at 12:08 AM, Junio C Hamano wrote:\n> Duy Nguyen writes:\n>\n>> On Fri, Apr 18, 2014 at 3:44 PM, Jiang Xin wrote:\n>>> When show blame information with relative time, the UTF-8 characters in\n>>> `time_str` will break the alignment of columns after the date field.\n>>> This is because the `time_buf` padding with spaces should have a\n>>> constant display width, not a fixed strlen size. So we should call\n>>> utf8_strwidth() instead of strlen() for calibration.\n>>>\n>>> Signed-off-by: Jiang Xin \n>>> ---\n>>>\n>>> Before applying this patch:\n>>>\n>>> 5817da01 builtin-blame.c (Pierre Habouzit 6 年前 21) #include \"parse-options.h\"\n>>> ffaf9cc0 builtin-blame.c (Geoffrey Thomas 5 年前 22) #include \"utf8.h\"\n>>> 3b8a12e8 builtin/blame.c (Axel Bonnet 3 年 10 个月之前 23) #include \"userdiff.h\"\n>>> 25ed3412 builtin/blame.c (Bo Yang 1 年 1 个月之前 24) #include \"line-range.h\"\n>>> 58dbfa2e builtin/blame.c (Eric Sunshine 9 个月之前 25) #include \"line-log.h\"\n>>> cee7f245 builtin-pickaxe.c (Junio C Hamano 8 年前 26)\n>>>\n>>> After applying this patch:\n>>>\n>>> 5817da01 builtin-blame.c (Pierre Habouzit 6 年前 21) #include \"parse-options.h\"\n>>> ffaf9cc0 builtin-blame.c (Geoffrey Thomas 5 年前 22) #include \"utf8.h\"\n>>> 3b8a12e8 builtin/blame.c (Axel Bonnet 3 年 10 个月之前 23) #include \"userdiff.h\"\n>>> 25ed3412 builtin/blame.c (Bo Yang 1 年 1 个月之前 24) #include \"line-range.h\"\n>>> 58dbfa2e builtin/blame.c (Eric Sunshine 9 个月之前 25) #include \"line-log.h\"\n>>> cee7f245 builtin-pickaxe.c (Junio C Hamano 8 年前 26)\n>>\n>>\n>> The numbers 21..26 still do not look aligned, both in gmail raw\n>> message view and gmane.\n>\n> Interesting. In my GNUS/Emacs on a fixed-column terminal where an\n> CJK occupies two display columns, they do look aligned, but in my\n> Chrome showing news.gmane.org/gmane.comp.version-control.git/, they\n> do look jagged. When these lines shown in the browser get copied\n> and pasted to gedit, they still look jagged, but after saving it to\n> a file and catting it to the same fixed-column terminal, they are\n> perfectly aligned.\n>\n> Font issues, I guess?\n\nEmacs/x11, gedit and xterm all show aligned listings. So yes probably\nfont issues. And because it works fine in text-based environment or\neditors. I think we're good.\n-- \nDuy\n"},{"id":"239148","messageId":"cover.1398010052.git.worldhello.net@gmail.com","threadId":"36435","inReplyTo":"CACsJy8BTBwqFZUU3i3cuv40B6AHw5SRY9DZN2PoKL4XzgEL2eA@mail.gmail.com","subject":"[PATCH v2 0/2] peroper align of datetime filed of git-blame","fromName":"Jiang Xin","fromEmail":"worldhello.net@gmail.com","sentAt":"2014-04-20T16:13:51Z","receivedAt":"2014-04-20T16:13:51Z","isPatch":true,"sender":{"key":"worldhello.net@gmail.com","avatar":"https://avatars.githubusercontent.com/u/183860?v=4"},"body":"When rewrite time_buf[128] to strbuf, find another bug from the original\nimplement. See detail commit log of 1/2.\n\nJiang Xin (2):\n bugfix: fix broken time_buf paddings for git-blame\n blame: use different blame_date_width for different locale\n\n builtin/blame.c | 29 +++++++++++++++++++++++------\n 1 file changed, 23 insertions(+), 6 deletions(-)\n\n-- \n2.0.0.rc0.3.g444188f.dirty\n"},{"id":"239149","messageId":"2742234ee9199b26e646f30b6ae20a1c9bfe48f7.1398010052.git.worldhello.net@gmail.com","threadId":"36435","inReplyTo":"cover.1398010052.git.worldhello.net@gmail.com","subject":"[PATCH v2 1/2] bugfix: fix broken time_buf paddings for git-blame","fromName":"Jiang Xin","fromEmail":"worldhello.net@gmail.com","sentAt":"2014-04-20T16:13:52Z","receivedAt":"2014-04-20T16:13:52Z","isPatch":true,"sender":{"key":"worldhello.net@gmail.com","avatar":"https://avatars.githubusercontent.com/u/183860?v=4"},"body":"When `git blame` shows date field in a fixed width (as long as\nblame_date_width characters), if time_str shorter than that, add spaces\nfor padding. But there are two bugs in the following codes:\n\n memcpy(time_buf, time_str, time_len);\n memset(time_buf + time_len, ' ', blame_date_width - time_len);\n\n1. The type of blame_date_width is size_t (unsigned int). If time_len\n is greater than blame_ate_width, \"blame_date_width - time_len\" will\n never be a negative number, but a really big positive number, and\n will cause memory overwrite.\n\n This bug can be triggered if either l10n message for function\n show_date_relative() in date.c is longer then 30 charcters, then\n `git blame --date relative` may fail.\n\n2. When show blame information with relative time, the UTF-8 characters\n in time_str will break the alignment of columns after the date field.\n This is because the time_buf padding with spaces should have a\n constant display width, not a fixed strlen size. So we should call\n utf8_strwidth() instead of strlen() for calibration.\n\nSigned-off-by: Jiang Xin \n---\n builtin/blame.c | 19 ++++++++++++++-----\n 1 file changed, 14 insertions(+), 5 deletions(-)\n\ndiff --git a/builtin/blame.c b/builtin/blame.c\nindex 88cb799..0a0a858 100644\n--- a/builtin/blame.c\n+++ b/builtin/blame.c\n@@ -1556,22 +1556,31 @@ static void assign_blame(struct scoreboard *sb, int opt)\n static const char *format_time(unsigned long time, const char *tz_str,\n \t\t\t int show_raw_time)\n {\n-\tstatic char time_buf[128];\n+\tstatic struct strbuf time_buf = STRBUF_INIT;\n \n+\tstrbuf_reset(&time_buf);\n \tif (show_raw_time) {\n-\t\tsnprintf(time_buf, sizeof(time_buf), \"%lu %s\", time, tz_str);\n+\t\tstrbuf_addf(&time_buf, \"%lu %s\", time, tz_str);\n \t}\n \telse {\n \t\tconst char *time_str;\n+\t\tsize_t time_width;\n \t\tint time_len;\n \t\tint tz;\n \t\ttz = atoi(tz_str);\n \t\ttime_str = show_date(time, tz, blame_date_mode);\n \t\ttime_len = strlen(time_str);\n-\t\tmemcpy(time_buf, time_str, time_len);\n-\t\tmemset(time_buf + time_len, ' ', blame_date_width - time_len);\n+\t\tstrbuf_add(&time_buf, time_str, time_len);\n+\t\t/*\n+\t\t * Add space paddings to time_buf to display a fixed width\n+\t\t * string, and use time_width for display width calibration.\n+\t\t */\n+\t\tfor (time_width = utf8_strwidth(time_str);\n+\t\t time_width < blame_date_width;\n+\t\t time_width++)\n+\t\t\tstrbuf_addch(&time_buf, ' ');\n \t}\n-\treturn time_buf;\n+\treturn time_buf.buf;\n }\n \n #define OUTPUT_ANNOTATE_COMPAT\t001\n-- \n2.0.0.rc0.3.g444188f.dirty\n"},{"id":"239150","messageId":"07e4dcdc98b5eb9c78b9ed53bf2adc3b33139b67.1398010052.git.worldhello.net@gmail.com","threadId":"36435","inReplyTo":"cover.1398010052.git.worldhello.net@gmail.com","subject":"[PATCH v2 2/2] blame: use different blame_date_width for different locale","fromName":"Jiang Xin","fromEmail":"worldhello.net@gmail.com","sentAt":"2014-04-20T16:13:53Z","receivedAt":"2014-04-20T16:13:53Z","isPatch":true,"sender":{"key":"worldhello.net@gmail.com","avatar":"https://avatars.githubusercontent.com/u/183860?v=4"},"body":"When show date in relative date format for `git blame`, the max display\nwidth of datetime is set as the length of the string \"Thu Oct 19\n16:00:04 2006 -0700\" (30 characters long). But actually the max width\nfor C locale is only 22 (the length of string \"x years, xx months ago\").\nAnd for other locale, it maybe smaller. E.g. For Chinese locale, only\nneeds 16-character width.\n\nSet blame_date_width as the display width of _(\"4 years, 11 months\nago\"), so that translators can make the choice.\n\nSigned-off-by: Jiang Xin \n---\n builtin/blame.c | 10 +++++++++-\n 1 file changed, 9 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/blame.c b/builtin/blame.c\nindex 0a0a858..9350ea3 100644\n--- a/builtin/blame.c\n+++ b/builtin/blame.c\n@@ -2340,7 +2340,15 @@ parse_done:\n \t\tblame_date_width = sizeof(\"2006-10-19\");\n \t\tbreak;\n \tcase DATE_RELATIVE:\n-\t\t/* \"normal\" is used as the fallback for \"relative\" */\n+\t\t/* TRANSLATORS: what we care about is not the content itself,\n+\t\t but the display width of this string. We use the width of\n+\t\t the string as the max width of the datetime in relative\n+\t\t format. For English and many other languages, \"4 years,\n+\t\t 11 months ago\" is the longest one among \"89 seconds ago\",\n+\t\t \"89 minites ago\", \"35 hours ago\", \"13 days ago\", \"10 weeks\n+\t\t ago\", \"in the future\" and many others. */\n+\t\tblame_date_width = utf8_strwidth(_(\"4 years, 11 months ago\")) + 1; /* add the null */\n+\t\tbreak;\n \tcase DATE_LOCAL:\n \tcase DATE_NORMAL:\n \t\tblame_date_width = sizeof(\"Thu Oct 19 16:00:04 2006 -0700\");\n-- \n2.0.0.rc0.3.g444188f.dirty\n"},{"id":"239176","messageId":"CAPig+cSf8RcUBdeACSLTTNcF7hqCXX-jjxx7RDZt52u3BKJxDA@mail.gmail.com","threadId":"36435","inReplyTo":"2742234ee9199b26e646f30b6ae20a1c9bfe48f7.1398010052.git.worldhello.net@gmail.com","subject":"Re: [PATCH v2 1/2] bugfix: fix broken time_buf paddings for git-blame","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2014-04-20T20:28:12Z","receivedAt":"2014-04-20T20:28:12Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sun, Apr 20, 2014 at 12:13 PM, Jiang Xin wrote:\n> When `git blame` shows date field in a fixed width (as long as\n\ns/fixed width/fixed-width/\n\ns/long/wide/ would read a bit better.\n\n> blame_date_width characters), if time_str shorter than that, add spaces\n\ns/shorter/is shorter/\ns/add/it adds/\n\n> for padding. But there are two bugs in the following codes:\n>\n> memcpy(time_buf, time_str, time_len);\n> memset(time_buf + time_len, ' ', blame_date_width - time_len);\n>\n> 1. The type of blame_date_width is size_t (unsigned int). If time_len\n\ns/(unsigned int)/, which is unsigned/\n\n> is greater than blame_ate_width, \"blame_date_width - time_len\" will\n\ns/_ate/_date/\n\n> never be a negative number, but a really big positive number, and\n> will cause memory overwrite.\n>\n> This bug can be triggered if either l10n message for function\n> show_date_relative() in date.c is longer then 30 charcters, then\n\ns/then 30/than 30/\ns/charcters/characters/\n\n> `git blame --date relative` may fail.\n>\n> 2. When show blame information with relative time, the UTF-8 characters\n> in time_str will break the alignment of columns after the date field.\n> This is because the time_buf padding with spaces should have a\n> constant display width, not a fixed strlen size. So we should call\n> utf8_strwidth() instead of strlen() for calibration.\n>\n> Signed-off-by: Jiang Xin \n> ---\n> builtin/blame.c | 19 ++++++++++++++-----\n> 1 file changed, 14 insertions(+), 5 deletions(-)\n>\n> diff --git a/builtin/blame.c b/builtin/blame.c\n> index 88cb799..0a0a858 100644\n> --- a/builtin/blame.c\n> +++ b/builtin/blame.c\n> @@ -1556,22 +1556,31 @@ static void assign_blame(struct scoreboard *sb, int opt)\n> static const char *format_time(unsigned long time, const char *tz_str,\n> int show_raw_time)\n> {\n> - static char time_buf[128];\n> + static struct strbuf time_buf = STRBUF_INIT;\n>\n> + strbuf_reset(&time_buf);\n> if (show_raw_time) {\n> - snprintf(time_buf, sizeof(time_buf), \"%lu %s\", time, tz_str);\n> + strbuf_addf(&time_buf, \"%lu %s\", time, tz_str);\n> }\n> else {\n> const char *time_str;\n> + size_t time_width;\n> int time_len;\n> int tz;\n> tz = atoi(tz_str);\n> time_str = show_date(time, tz, blame_date_mode);\n> time_len = strlen(time_str);\n> - memcpy(time_buf, time_str, time_len);\n> - memset(time_buf + time_len, ' ', blame_date_width - time_len);\n> + strbuf_add(&time_buf, time_str, time_len);\n> + /*\n> + * Add space paddings to time_buf to display a fixed width\n> + * string, and use time_width for display width calibration.\n> + */\n> + for (time_width = utf8_strwidth(time_str);\n> + time_width < blame_date_width;\n> + time_width++)\n> + strbuf_addch(&time_buf, ' ');\n> }\n> - return time_buf;\n> + return time_buf.buf;\n> }\n>\n> #define OUTPUT_ANNOTATE_COMPAT 001\n> --\n> 2.0.0.rc0.3.g444188f.dirty\n"},{"id":"239185","messageId":"7vbnvvllx4.fsf@alter.siamese.dyndns.org","threadId":"36435","inReplyTo":"07e4dcdc98b5eb9c78b9ed53bf2adc3b33139b67.1398010052.git.worldhello.net@gmail.com","subject":"Re: [PATCH v2 2/2] blame: use different blame_date_width for different locale","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-04-20T21:40:23Z","receivedAt":"2014-04-20T21:40:23Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jiang Xin writes:\n\n> When show date in relative date format for `git blame`, the max display\n> width of datetime is set as the length of the string \"Thu Oct 19\n> 16:00:04 2006 -0700\" (30 characters long). But actually the max width\n> for C locale is only 22 (the length of string \"x years, xx months ago\").\n> And for other locale, it maybe smaller. E.g. For Chinese locale, only\n> needs 16-character width.\n>\n> Set blame_date_width as the display width of _(\"4 years, 11 months\n> ago\"), so that translators can make the choice.\n>\n> Signed-off-by: Jiang Xin \n> ---\n> builtin/blame.c | 10 +++++++++-\n> 1 file changed, 9 insertions(+), 1 deletion(-)\n>\n> diff --git a/builtin/blame.c b/builtin/blame.c\n> index 0a0a858..9350ea3 100644\n> --- a/builtin/blame.c\n> +++ b/builtin/blame.c\n> @@ -2340,7 +2340,15 @@ parse_done:\n> \t\tblame_date_width = sizeof(\"2006-10-19\");\n> \t\tbreak;\n> \tcase DATE_RELATIVE:\n> -\t\t/* \"normal\" is used as the fallback for \"relative\" */\n> +\t\t/* TRANSLATORS: what we care about is not the content itself,\n> +\t\t but the display width of this string. We use the width of\n> +\t\t the string as the max width of the datetime in relative\n> +\t\t format. For English and many other languages, \"4 years,\n> +\t\t 11 months ago\" is the longest one among \"89 seconds ago\",\n> +\t\t \"89 minites ago\", \"35 hours ago\", \"13 days ago\", \"10 weeks\n> +\t\t ago\", \"in the future\" and many others. */\n> +\t\tblame_date_width = utf8_strwidth(_(\"4 years, 11 months ago\")) + 1; /* add the null */\n> +\t\tbreak;\n\n\nThis is not wrong per-se, but I am not sure if translators would\nunderstand that \"years and months ago\" may not be the longuest\nvariant for their language and they are asked to use \"89 seconds\nago\" if the translation of that is longer than the translation for\n\"4 years, 11 months ago\" in their language, with the given\nexplanation.\n\nHow about dropping the second sentence \"We use...\" and end the\ncomment with something like:\n\n\tago\", \"in the future\" and many others. It is possible that\n\tthe translation of \"89 seconds ago\" may be the longest\n\tpossible translation in your language (then by definition,\n\tit would be longer than the translation of this string in\n\tyour language), and in such a case, please write the\n\ttranslation of \"89 seconds ago\" here.\n\nActually, even though \"many others\" is correct, the possibilities\nare within a bounded set of template i18n formats, no? I wonder if\nit would be nicer to the translators if we did not use a single\n\"representative string\" here, but add a helper function to date.c\nthat goes something like:\n\n int date_relative_maxwidth(void)\n {\n struct strbuf buf = STRBUF_INIT;\n static int length;\n\n if (length)\n return length;\n\n strbuf_addf(&buf, _(\"in the future\"));\n length = (length < buf.len) ? buf.len : length;\n strbuf_addf(&buf, Q_(\"%lu second ago\", \"%lu seconds ago\", 89), 89);\n length = (length < buf.len) ? buf.len : length;\n ...\n strbuf_addf(&buf, Q_(\"%lu year ago\", \"%lu years ago\", 9999), 9999);\n length = (length < buf.len) ? buf.len : length;\n\n return length;\n }\n\nimmediately after the definition of show_date_relative() and used it\nin this codepath?\n\nOf course, given a date that is far enough into the future, the\nactuall length is unbounded, so this approach will not come up with\nthe absolute minimum width to align all lines (you would need to do\na two pass, measuring how wide all timestamps that would appear in\nthe output before producing the first line of the output---but I do\nnot think it is worth it, and as long as we do not overflow the\nbuffer, showing occasional misalignment for a line that came from\nyear 27342 is much better than a worse latency-to-first-output).\n\n> \tcase DATE_LOCAL:\n> \tcase DATE_NORMAL:\n> \t\tblame_date_width = sizeof(\"Thu Oct 19 16:00:04 2006 -0700\");\n"},{"id":"239203","messageId":"cover.1398059411.git.worldhello.net@gmail.com","threadId":"36435","inReplyTo":"7vbnvvllx4.fsf@alter.siamese.dyndns.org","subject":"[PATCH v3 0/2] peroper align of datetime filed of git-blame","fromName":"Jiang Xin","fromEmail":"worldhello.net@gmail.com","sentAt":"2014-04-21T06:02:02Z","receivedAt":"2014-04-21T06:02:02Z","isPatch":true,"sender":{"key":"worldhello.net@gmail.com","avatar":"https://avatars.githubusercontent.com/u/183860?v=4"},"body":"Changes since v2:\n\n * Fixed typos in commit log 1/2. Thanks, Eric.\n\n * With the help of Junio, write a helper to get suitable blame_date_width.\n\nJiang Xin (2):\n bugfix: fix broken time_buf paddings for git-blame\n blame: use a helper to get suitable blame_date_width\n\n builtin/blame.c | 85 +++++++++++++++++++++++++++++++++++++++++++++++++++------\n 1 file changed, 77 insertions(+), 8 deletions(-)\n\n-- \n1.9.2.476.ga74def0\n"},{"id":"239204","messageId":"e8340b76f5c2b676a9594018b3385ead451412fa.1398059411.git.worldhello.net@gmail.com","threadId":"36435","inReplyTo":"cover.1398059411.git.worldhello.net@gmail.com","subject":"[PATCH v3 1/2] bugfix: fix broken time_buf paddings for git-blame","fromName":"Jiang Xin","fromEmail":"worldhello.net@gmail.com","sentAt":"2014-04-21T06:02:03Z","receivedAt":"2014-04-21T06:02:03Z","isPatch":true,"sender":{"key":"worldhello.net@gmail.com","avatar":"https://avatars.githubusercontent.com/u/183860?v=4"},"body":"Command `git blame --date relative` aligns the date field with a\nfixed-width (defined by blame_date_width), and if time_str is shorter\nthan that, it adds spaces for padding. But there are two bugs in the\nfollowing codes:\n\n time_len = strlen(time_str);\n ...\n memset(time_buf + time_len, ' ', blame_date_width - time_len);\n\n1. The type of blame_date_width is size_t, which is unsigned. If\n time_len is greater than blame_date_width, the result of\n \"blame_date_width - time_len\" will never be a negative number, but a\n really big positive number, and will cause memory overwrite.\n\n This bug can be triggered if either l10n message for function\n show_date_relative() in date.c is longer than 30 characters, then\n `git blame --date relative` may exit abnormally.\n\n2. When show blame information with relative time, the UTF-8 characters\n in time_str will break the alignment of columns after the date field.\n This is because the time_buf padding with spaces should have a\n constant display width, not a fixed strlen size. So we should call\n utf8_strwidth() instead of strlen() for width calibration.\n\nHelped-by: Nguyễn Thái Ngọc Duy \nHelped-by: Eric Sunshine \nSigned-off-by: Jiang Xin \n---\n builtin/blame.c | 21 ++++++++++++++-------\n 1 file changed, 14 insertions(+), 7 deletions(-)\n\ndiff --git a/builtin/blame.c b/builtin/blame.c\nindex 88cb799..35e95db 100644\n--- a/builtin/blame.c\n+++ b/builtin/blame.c\n@@ -1556,22 +1556,29 @@ static void assign_blame(struct scoreboard *sb, int opt)\n static const char *format_time(unsigned long time, const char *tz_str,\n \t\t\t int show_raw_time)\n {\n-\tstatic char time_buf[128];\n+\tstatic struct strbuf time_buf = STRBUF_INIT;\n \n+\tstrbuf_reset(&time_buf);\n \tif (show_raw_time) {\n-\t\tsnprintf(time_buf, sizeof(time_buf), \"%lu %s\", time, tz_str);\n+\t\tstrbuf_addf(&time_buf, \"%lu %s\", time, tz_str);\n \t}\n \telse {\n \t\tconst char *time_str;\n-\t\tint time_len;\n+\t\tsize_t time_width;\n \t\tint tz;\n \t\ttz = atoi(tz_str);\n \t\ttime_str = show_date(time, tz, blame_date_mode);\n-\t\ttime_len = strlen(time_str);\n-\t\tmemcpy(time_buf, time_str, time_len);\n-\t\tmemset(time_buf + time_len, ' ', blame_date_width - time_len);\n+\t\tstrbuf_addstr(&time_buf, time_str);\n+\t\t/*\n+\t\t * Add space paddings to time_buf to display a fixed width\n+\t\t * string, and use time_width for display width calibration.\n+\t\t */\n+\t\tfor (time_width = utf8_strwidth(time_str);\n+\t\t time_width < blame_date_width;\n+\t\t time_width++)\n+\t\t\tstrbuf_addch(&time_buf, ' ');\n \t}\n-\treturn time_buf;\n+\treturn time_buf.buf;\n }\n \n #define OUTPUT_ANNOTATE_COMPAT\t001\n-- \n1.9.2.476.ga74def0\n"},{"id":"239205","messageId":"17454bdfbd4e0e1516a64f75deabddb427792e99.1398059411.git.worldhello.net@gmail.com","threadId":"36435","inReplyTo":"cover.1398059411.git.worldhello.net@gmail.com","subject":"[PATCH v3 2/2] blame: use a helper to get suitable blame_date_width","fromName":"Jiang Xin","fromEmail":"worldhello.net@gmail.com","sentAt":"2014-04-21T06:02:04Z","receivedAt":"2014-04-21T06:02:04Z","isPatch":true,"sender":{"key":"worldhello.net@gmail.com","avatar":"https://avatars.githubusercontent.com/u/183860?v=4"},"body":"When show date in relative date format for git-blame, the max display\nwidth of datetime is set as the length of the string \"Thu Oct 19\n16:00:04 2006 -0700\" (30 characters long). But actually the max width\nfor C locale is only 22 (the length of string \"x years, xx months ago\").\nAnd for other locale, it maybe smaller. E.g. For Chinese locale, only\nneeds a half (16-character width).\n\nAdd a helper function date_relative_maxwidth() to date.c, which returns\nthe suitable display width for the relative date field in different\nlocale.\n\nSuggested-by: Junio C Hamano \nSigned-off-by: Jiang Xin \n---\n builtin/blame.c | 64 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++-\n 1 file changed, 63 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/blame.c b/builtin/blame.c\nindex 35e95db..478f739 100644\n--- a/builtin/blame.c\n+++ b/builtin/blame.c\n@@ -2229,6 +2229,67 @@ static int blame_move_callback(const struct option *option, const char *arg, int\n \treturn 0;\n }\n \n+static int date_relative_maxwidth(void)\n+{\n+\tstruct strbuf buf = STRBUF_INIT, years_sb = STRBUF_INIT;\n+\tstatic int maxwidth = 0;\n+\tint width;\n+\n+\tif (maxwidth)\n+\t\treturn maxwidth;\n+\n+\tstrbuf_addf(&buf, _(\"in the future\"));\n+\tmaxwidth = utf8_strwidth(buf.buf);\n+\n+\tstrbuf_reset(&buf);\n+\tstrbuf_addf(&buf, Q_(\"%lu second ago\", \"%lu seconds ago\", 89L), 89L);\n+\twidth = utf8_strwidth(buf.buf);\n+\tmaxwidth = (maxwidth < width) ? width : maxwidth;\n+\n+\tstrbuf_reset(&buf);\n+\tstrbuf_addf(&buf, Q_(\"%lu minute ago\", \"%lu minutes ago\", 89L), 89L);\n+\twidth = utf8_strwidth(buf.buf);\n+\tmaxwidth = (maxwidth < width) ? width : maxwidth;\n+\n+\tstrbuf_reset(&buf);\n+\tstrbuf_addf(&buf, Q_(\"%lu hour ago\", \"%lu hours ago\", 35L), 35L);\n+\twidth = utf8_strwidth(buf.buf);\n+\tmaxwidth = (maxwidth < width) ? width : maxwidth;\n+\n+\tstrbuf_reset(&buf);\n+\tstrbuf_addf(&buf, Q_(\"%lu day ago\", \"%lu days ago\", 13L), 13L);\n+\twidth = utf8_strwidth(buf.buf);\n+\tmaxwidth = (maxwidth < width) ? width : maxwidth;\n+\n+\tstrbuf_reset(&buf);\n+\tstrbuf_addf(&buf, Q_(\"%lu week ago\", \"%lu weeks ago\", 10L), 10L);\n+\twidth = utf8_strwidth(buf.buf);\n+\tmaxwidth = (maxwidth < width) ? width : maxwidth;\n+\n+\tstrbuf_reset(&buf);\n+\tstrbuf_addf(&buf, Q_(\"%lu month ago\", \"%lu months ago\", 12L), 12L);\n+\twidth = utf8_strwidth(buf.buf);\n+\tmaxwidth = (maxwidth < width) ? width : maxwidth;\n+\n+\tstrbuf_addf(&years_sb, Q_(\"%lu year\", \"%lu years\", 4L), 4L);\n+\tstrbuf_reset(&buf);\n+\tstrbuf_addf(&buf,\n+\t\tQ_(\"%s, %lu month ago\", \"%s, %lu months ago\", 11L),\n+\t\tyears_sb.buf, 11L);\n+\twidth = utf8_strwidth(buf.buf);\n+\tmaxwidth = (maxwidth < width) ? width : maxwidth;\n+\n+\tstrbuf_reset(&buf);\n+\tstrbuf_addf(&buf, Q_(\"%lu year ago\", \"%lu years ago\", 9999L), 9999L);\n+\twidth = utf8_strwidth(buf.buf);\n+\tmaxwidth = (maxwidth < width) ? width : maxwidth;\n+\n+\tstrbuf_release(&years_sb);\n+\tstrbuf_release(&buf);\n+\n+\treturn maxwidth;\n+}\n+\n int cmd_blame(int argc, const char **argv, const char *prefix)\n {\n \tstruct rev_info revs;\n@@ -2338,7 +2399,8 @@ parse_done:\n \t\tblame_date_width = sizeof(\"2006-10-19\");\n \t\tbreak;\n \tcase DATE_RELATIVE:\n-\t\t/* \"normal\" is used as the fallback for \"relative\" */\n+\t\tblame_date_width = date_relative_maxwidth() + 1; /* add the null */\n+\t\tbreak;\n \tcase DATE_LOCAL:\n \tcase DATE_NORMAL:\n \t\tblame_date_width = sizeof(\"Thu Oct 19 16:00:04 2006 -0700\");\n-- \n1.9.2.476.ga74def0\n"},{"id":"239210","messageId":"xmqqfvl6a9ar.fsf@gitster.dls.corp.google.com","threadId":"36435","inReplyTo":"17454bdfbd4e0e1516a64f75deabddb427792e99.1398059411.git.worldhello.net@gmail.com","subject":"Re: [PATCH v3 2/2] blame: use a helper to get suitable blame_date_width","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-04-21T17:20:44Z","receivedAt":"2014-04-21T17:20:44Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jiang Xin writes:\n\n> When show date in relative date format for git-blame, the max display\n> width of datetime is set as the length of the string \"Thu Oct 19\n> 16:00:04 2006 -0700\" (30 characters long). But actually the max width\n> for C locale is only 22 (the length of string \"x years, xx months ago\").\n> And for other locale, it maybe smaller. E.g. For Chinese locale, only\n> needs a half (16-character width).\n>\n> Add a helper function date_relative_maxwidth() to date.c, which returns\n> the suitable display width for the relative date field in different\n> locale.\n>\n> Suggested-by: Junio C Hamano \n> Signed-off-by: Jiang Xin \n> ---\n> builtin/blame.c | 64 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++-\n> 1 file changed, 63 insertions(+), 1 deletion(-)\n\nThat does not seem to have the helper in date.c immediately next to\nthe definition of show_date_relative(), as the above log message\nclaims, does it?\n\nI was hoping that you would respond with a set of counter-arugments\nto shoot the suggestion down. Here are a few obvious ones:\n\n - By inlining the implementation of show_date_relative() in the new\n helper function, we would end up having to maintain two copies of\n every format strings.\n\n - Even if we don't inline the logic and duplicate the format\n strings, and call show_date_relative() with some predetermined\n offsets instead, e.g.\n\n\tstatic int date_relative_maxwidth(void)\n\t{\n struct timebuf = STRBUF_INIT;\n\t\tstruct timeval now;\n static int maxwidth = 0;\n\n gettimeofday(&now, NULL);\n\n /* in the future */\n show_date_relative(now->tv_sec + 100, 0, &now, &timebuf);\n maxwidth = maxwidth < timebuf.len ? timebuf.len : maxwidth;\n strbuf_reset(&timebuf);\n\n\t\t/* seconds ago */\n show_date_relative(now->tv_sec - 89, 0, &now, &timebuf);\n maxwidth = maxwidth < timebuf.len ? timebuf.len : maxwidth;\n strbuf_reset(&timebuf);\n\n ...\n\t}\n\n we still end up hardcoding the logic in the code.\n\n - There is no guarantee that these predetermined offsets would\n produce the longest possible timestamp for the target language\n with these gettext strings. In a language where the noun\n \"second\" takes a lot longer shape for singular than the plural\n (e.g. \"1 SECOND\" vs \"2 SEC\", perhaps), checking for 89 seconds\n ago may not produce the longest string for \"%lu seconds ago\".\n The approach, \"we compute everything for translators\", sounds\n nice in theory, but may not work well in practice and the worst\n part is that there is no way for translators to work it around,\n unlike your original patch.\n\nSo after sleeping on the idea overnight, I think I like the\nsimplicity of your original patch better. It just needs to be\nexplained well for translators to understand.\n\nSorry for making you go off exploring in a strange direction.\n\nWhen msgmerge is run to update XX.po with a new version of git.pot,\ndoes it destroy comments an earlier translator placed in XX.po, or\nare they kept intact? What I am wondering is if we can do something\nlike this:\n\n In code:\n\n blame_date_width = strtoul(_(\"22 (C)\"), NULL, 10) + 1; /* add the null */\n\n In git.pot:\n\n #. This string encodes the preferred maximum display width for a\n #. relative timestamp in \"git blame\" output. For C locale, \"4 years,\n #. 11 months ago\", which takes 22 places, is the longest among various\n #. forms of relative timestamps, but your languate may need more or\n #. fewer display columns. If \"zh_CN\" locale needs 14 display columns to\n #. hold any relative timestamp in the reasonably near past, for\n #. example, translate this string as \"14 (zh_CN)\".\n msgid \"22 (C)\"\n msgstr \"\"\n\n In de.po:\n #. This string encodes the preferred maximum display width for a\n #. relative timestamp in \"git blame\" output. For C locale, \"4 years,\n #. 11 months ago\", which takes 22 places, is the longest among various\n #. forms of relative timestamps, but your languate may need more or\n #. fewer display columns. If zh_CN locale needs 14 display columns to\n #. hold any relative timestamp in the reasonably near past, for\n #. example, translate this string as \"14 (zh_CN)\".\n\t#.\n\t#. In de locale, \"vor %lu Jahren, und %lu Monaten\" is the\n #. longest and fits within 28 display columns.\n msgid \"22 (C)\"\n msgstr \"28 (de)\"\n\nwhere the instructions for tranlators to various languages come from\ngit.pot and the translator to a specific language can describe which\nvariant in the language described in XX.po is the longest. For that\nto work well, the last two lines in the comment an earlier \"de\"\ntranslator added need to be preserved across msgmerge, though.\n"},{"id":"239219","messageId":"xmqq38h6a3sk.fsf@gitster.dls.corp.google.com","threadId":"36435","inReplyTo":"xmqqfvl6a9ar.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v3 2/2] blame: use a helper to get suitable blame_date_width","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-04-21T19:19:39Z","receivedAt":"2014-04-21T19:19:39Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> What I am wondering is if we can do something\n> like this:\n> ...\n\nNah, that is a lot more stupid than just doing\n\n> In code:\n>\n> blame_date_width = strtoul(_(\"4 years, 11 months ago\"), NULL, 10) + 1;\n>\n> In git.pot:\n>\n> #. This string is used to tell us the maximum display width for a\n> #. relative timestamp in \"git blame\" output. For C locale, \"4 years,\n> #. 11 months ago\", which takes 22 places, is the longest among various\n> #. forms of relative timestamps, but your language may need more or\n> #. fewer display columns.\n> msgid \"4 years, 11 months ago\"\n> msgstr \"\"\n>\n> In de.po:\n> #. This string is used to tell us the maximum display width for a\n> #. relative timestamp in \"git blame\" output. For C locale, \"4 years,\n> #. 11 months ago\", which takes 22 places, is the longest among various\n> #. forms of relative timestamps, but your language may need more or\n> #. fewer display columns.\n> msgid \"4 years, 11 months ago\"\n> msgstr \"\"vor 4 Jahren, und 11 Monaten\"\n\nwhich is essentially how your very original looked like (modulo the\ncomments). So let's not try to be clever or cute, and just have a\ngood instruction in the TRANSLATORS comments.\n\nSorry for flipping and flopping on this one.\n"},{"id":"239322","messageId":"87vbu1ptsx.fsf@fencepost.gnu.org","threadId":"36435","inReplyTo":"7vbnvvllx4.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v2 2/2] blame: use different blame_date_width for different locale","fromName":"David Kastrup","fromEmail":"dak@gnu.org","sentAt":"2014-04-22T10:01:02Z","receivedAt":"2014-04-22T10:01:02Z","isPatch":true,"sender":{"key":"dak@gnu.org","avatar":"https://avatars.githubusercontent.com/u/52141349?v=4"},"body":"Junio C Hamano writes:\n\n> This is not wrong per-se, but I am not sure if translators would\n> understand that \"years and months ago\" may not be the longuest\n> variant for their language and they are asked to use \"89 seconds\n> ago\" if the translation of that is longer than the translation for\n> \"4 years, 11 months ago\" in their language, with the given\n> explanation.\n\nWhat's with the 89? And the other semi-magic numbers? If we fear about\nnon-arabic number formatting, at least in French French the worst\noffender may be quatre-vingt-quatorze (\"four score and fourteen\") or\nquatre-vingt-dix-neuf (\"four score and nineteen\"), namely 94 or 99. But\nI think it's improbable to get worded formatting here anyway. Or are\nthose the largest values with their respective granularity?\n\n-- \nDavid Kastrup\n"},{"id":"239329","messageId":"CANYiYbE5zdOq6F4YwSUojOyp9eexuM8=dY_T4hyd4W+Dea+XUg@mail.gmail.com","threadId":"36435","inReplyTo":"87vbu1ptsx.fsf@fencepost.gnu.org","subject":"Re: [PATCH v2 2/2] blame: use different blame_date_width for different locale","fromName":"Jiang Xin","fromEmail":"worldhello.net@gmail.com","sentAt":"2014-04-22T12:16:02Z","receivedAt":"2014-04-22T12:16:02Z","isPatch":true,"sender":{"key":"worldhello.net@gmail.com","avatar":"https://avatars.githubusercontent.com/u/183860?v=4"},"body":"2014-04-22 18:01 GMT+08:00 David Kastrup :\n> Junio C Hamano writes:\n>\n>> This is not wrong per-se, but I am not sure if translators would\n>> understand that \"years and months ago\" may not be the longuest\n>> variant for their language and they are asked to use \"89 seconds\n>> ago\" if the translation of that is longer than the translation for\n>> \"4 years, 11 months ago\" in their language, with the given\n>> explanation.\n>\n> What's with the 89? And the other semi-magic numbers?\n\nNot something magic, just what show_date_relative() in date.c is implemented:\n\n 98 diff = now->tv_sec - time;\n 99 if (diff < 90) {\n 100 strbuf_addf(timebuf,\n 101 Q_(\"%lu second ago\", \"%lu seconds\nago\", diff), diff);\n 102 return;\n 103 }\n 104 /* Turn it into minutes */\n 105 diff = (diff + 30) / 60;\n 106 if (diff < 90) {\n 107 strbuf_addf(timebuf,\n 108 Q_(\"%lu minute ago\", \"%lu minutes\nago\", diff), diff);\n 109 return;\n 110 }\n\n\n-- \nJiang Xin\n"},{"id":"239331","messageId":"CANYiYbGjyEHD_VC4jWVt5dvb6x2aCwpydmvKO9kwh5OW26iAMg@mail.gmail.com","threadId":"36435","inReplyTo":"xmqq38h6a3sk.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v3 2/2] blame: use a helper to get suitable blame_date_width","fromName":"Jiang Xin","fromEmail":"worldhello.net@gmail.com","sentAt":"2014-04-22T12:25:36Z","receivedAt":"2014-04-22T12:25:36Z","isPatch":true,"sender":{"key":"worldhello.net@gmail.com","avatar":"https://avatars.githubusercontent.com/u/183860?v=4"},"body":"2014-04-22 3:19 GMT+08:00 Junio C Hamano :\n> Junio C Hamano writes:\n>\n>> What I am wondering is if we can do something\n>> like this:\n>> ...\n>\n> Nah, that is a lot more stupid than just doing\n>\n>> In code:\n>>\n>> blame_date_width = strtoul(_(\"4 years, 11 months ago\"), NULL, 10) + 1;\n>>\n>> In git.pot:\n>>\n>> #. This string is used to tell us the maximum display width for a\n>> #. relative timestamp in \"git blame\" output. For C locale, \"4 years,\n>> #. 11 months ago\", which takes 22 places, is the longest among various\n>> #. forms of relative timestamps, but your language may need more or\n>> #. fewer display columns.\n>> msgid \"4 years, 11 months ago\"\n>> msgstr \"\"\n>>\n>> In de.po:\n>> #. This string is used to tell us the maximum display width for a\n>> #. relative timestamp in \"git blame\" output. For C locale, \"4 years,\n>> #. 11 months ago\", which takes 22 places, is the longest among various\n>> #. forms of relative timestamps, but your language may need more or\n>> #. fewer display columns.\n>> msgid \"4 years, 11 months ago\"\n>> msgstr \"\"vor 4 Jahren, und 11 Monaten\"\n>\n> which is essentially how your very original looked like (modulo the\n> comments). So let's not try to be clever or cute, and just have a\n> good instruction in the TRANSLATORS comments.\n>\n> Sorry for flipping and flopping on this one.\n\nI will send patch v4 tonight, and I think all l10n team leaders should\npay attention\nto this thread:\n\n * http://thread.gmane.org/gmane.comp.version-control.git/246464\n\n-- \nJiang Xin\n"},{"id":"239336","messageId":"cover.1398177411.git.worldhello.net@gmail.com","threadId":"36435","inReplyTo":"xmqq38h6a3sk.fsf@gitster.dls.corp.google.com","subject":"[PATCH v4 0/2] peroper align of datetime filed of git-blame","fromName":"Jiang Xin","fromEmail":"worldhello.net@gmail.com","sentAt":"2014-04-22T14:39:08Z","receivedAt":"2014-04-22T14:39:08Z","isPatch":true,"sender":{"key":"worldhello.net@gmail.com","avatar":"https://avatars.githubusercontent.com/u/183860?v=4"},"body":"Changes since V3:\n\n * rollback patch 2/2 to v2, but with a nicer comment for translators.\n\nJiang Xin (2):\n bugfix: fix broken time_buf paddings for git-blame\n blame: dynamic blame_date_width for different locales\n\n builtin/blame.c | 30 ++++++++++++++++++++++--------\n 1 file changed, 22 insertions(+), 8 deletions(-)\n\n-- \n1.9.2.476.gff10cf3.dirty\n"},{"id":"239337","messageId":"e8340b76f5c2b676a9594018b3385ead451412fa.1398177411.git.worldhello.net@gmail.com","threadId":"36435","inReplyTo":"cover.1398177411.git.worldhello.net@gmail.com","subject":"[PATCH v4 1/2] bugfix: fix broken time_buf paddings for git-blame","fromName":"Jiang Xin","fromEmail":"worldhello.net@gmail.com","sentAt":"2014-04-22T14:39:09Z","receivedAt":"2014-04-22T14:39:09Z","isPatch":true,"sender":{"key":"worldhello.net@gmail.com","avatar":"https://avatars.githubusercontent.com/u/183860?v=4"},"body":"Command `git blame --date relative` aligns the date field with a\nfixed-width (defined by blame_date_width), and if time_str is shorter\nthan that, it adds spaces for padding. But there are two bugs in the\nfollowing codes:\n\n time_len = strlen(time_str);\n ...\n memset(time_buf + time_len, ' ', blame_date_width - time_len);\n\n1. The type of blame_date_width is size_t, which is unsigned. If\n time_len is greater than blame_date_width, the result of\n \"blame_date_width - time_len\" will never be a negative number, but a\n really big positive number, and will cause memory overwrite.\n\n This bug can be triggered if either l10n message for function\n show_date_relative() in date.c is longer than 30 characters, then\n `git blame --date relative` may exit abnormally.\n\n2. When show blame information with relative time, the UTF-8 characters\n in time_str will break the alignment of columns after the date field.\n This is because the time_buf padding with spaces should have a\n constant display width, not a fixed strlen size. So we should call\n utf8_strwidth() instead of strlen() for width calibration.\n\nHelped-by: Nguyễn Thái Ngọc Duy \nHelped-by: Eric Sunshine \nSigned-off-by: Jiang Xin \n---\n builtin/blame.c | 21 ++++++++++++++-------\n 1 file changed, 14 insertions(+), 7 deletions(-)\n\ndiff --git a/builtin/blame.c b/builtin/blame.c\nindex 88cb799..35e95db 100644\n--- a/builtin/blame.c\n+++ b/builtin/blame.c\n@@ -1556,22 +1556,29 @@ static void assign_blame(struct scoreboard *sb, int opt)\n static const char *format_time(unsigned long time, const char *tz_str,\n \t\t\t int show_raw_time)\n {\n-\tstatic char time_buf[128];\n+\tstatic struct strbuf time_buf = STRBUF_INIT;\n \n+\tstrbuf_reset(&time_buf);\n \tif (show_raw_time) {\n-\t\tsnprintf(time_buf, sizeof(time_buf), \"%lu %s\", time, tz_str);\n+\t\tstrbuf_addf(&time_buf, \"%lu %s\", time, tz_str);\n \t}\n \telse {\n \t\tconst char *time_str;\n-\t\tint time_len;\n+\t\tsize_t time_width;\n \t\tint tz;\n \t\ttz = atoi(tz_str);\n \t\ttime_str = show_date(time, tz, blame_date_mode);\n-\t\ttime_len = strlen(time_str);\n-\t\tmemcpy(time_buf, time_str, time_len);\n-\t\tmemset(time_buf + time_len, ' ', blame_date_width - time_len);\n+\t\tstrbuf_addstr(&time_buf, time_str);\n+\t\t/*\n+\t\t * Add space paddings to time_buf to display a fixed width\n+\t\t * string, and use time_width for display width calibration.\n+\t\t */\n+\t\tfor (time_width = utf8_strwidth(time_str);\n+\t\t time_width < blame_date_width;\n+\t\t time_width++)\n+\t\t\tstrbuf_addch(&time_buf, ' ');\n \t}\n-\treturn time_buf;\n+\treturn time_buf.buf;\n }\n \n #define OUTPUT_ANNOTATE_COMPAT\t001\n-- \n1.9.2.476.gff10cf3.dirty\n"},{"id":"239338","messageId":"2d4e186a742b03788a843b6afcd9fcd7575a5c37.1398177411.git.worldhello.net@gmail.com","threadId":"36435","inReplyTo":"cover.1398177411.git.worldhello.net@gmail.com","subject":"[PATCH v4 2/2] blame: dynamic blame_date_width for different locales","fromName":"Jiang Xin","fromEmail":"worldhello.net@gmail.com","sentAt":"2014-04-22T14:39:10Z","receivedAt":"2014-04-22T14:39:10Z","isPatch":true,"sender":{"key":"worldhello.net@gmail.com","avatar":"https://avatars.githubusercontent.com/u/183860?v=4"},"body":"When show date in relative date format for git-blame, the max display\nwidth of datetime is set as the length of the string \"Thu Oct 19\n16:00:04 2006 -0700\" (30 characters long). But actually the max width\nfor C locale is only 22 (the length of string \"x years, xx months ago\").\nAnd for other locale, it maybe smaller. E.g. For Chinese locale, only\nneeds a half (16-character width).\n\nSet blame_date_width as the display width of _(\"4 years, 11 months\nago\"), so that translators can make the choice.\n\nHelped-by: Junio C Hamano \nSigned-off-by: Jiang Xin \n---\n builtin/blame.c | 9 ++++++++-\n 1 file changed, 8 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/blame.c b/builtin/blame.c\nindex 35e95db..128fc64 100644\n--- a/builtin/blame.c\n+++ b/builtin/blame.c\n@@ -2338,7 +2338,14 @@ parse_done:\n \t\tblame_date_width = sizeof(\"2006-10-19\");\n \t\tbreak;\n \tcase DATE_RELATIVE:\n-\t\t/* \"normal\" is used as the fallback for \"relative\" */\n+\t\t/* TRANSLATORS: This string is used to tell us the maximum\n+\t\t display width for a relative timestamp in \"git blame\"\n+\t\t output. For C locale, \"4 years, 11 months ago\", which\n+\t\t takes 22 places, is the longest among various forms of\n+\t\t relative timestamps, but your language may need more or\n+\t\t fewer display columns. */\n+\t\tblame_date_width = utf8_strwidth(_(\"4 years, 11 months ago\")) + 1; /* add the null */\n+\t\tbreak;\n \tcase DATE_LOCAL:\n \tcase DATE_NORMAL:\n \t\tblame_date_width = sizeof(\"Thu Oct 19 16:00:04 2006 -0700\");\n-- \n1.9.2.476.gff10cf3.dirty\n"}]}