patchblame: 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.

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(-)

On Fri, Apr 18, 2014 at 3:44 PM, Jiang Xin <worldhello.net@gmail.com> wrote:

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

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

Duy Nguyen <pclouds@gmail.com> writes:

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?

Good eyes.

On Sat, Apr 19, 2014 at 12:08 AM, Junio C Hamano <gitster@pobox.com> wrote:

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.

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(-)

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(-)

On Sun, Apr 20, 2014 at 12:13 PM, Jiang Xin <worldhello.net@gmail.com> wrote:

s/fixed width/fixed-width/

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

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

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

s/_ate/_date/

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

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(-)

Jiang Xin <worldhello.net@gmail.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.

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).

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(-)

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(-)

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(-)

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

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.

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

Nah, that is a lot more stupid than just doing

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.

2014-04-22 3:19 GMT+08:00 Junio C Hamano <gitster@pobox.com>:

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

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(-)

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(-)

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(-)

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

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?

2014-04-22 18:01 GMT+08:00 David Kastrup <dak@gnu.org>:

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         }