patchbuiltin-blame.c: Use utf8_strwidth for author's names

From: Geoffrey Thomas <geofft@mit.edu>
I'm about to use this pattern more than once, so make it a common function.

Signed-off-by: Geoffrey Thomas <geofft@mit.edu>
---
 utf8.c |   12 ++++++++++++
 utf8.h |    1 +
 2 files changed, 13 insertions(+), 0 deletions(-)

From: Geoffrey Thomas <geofft@mit.edu>
git blame misaligns output if a author's name has a differing display width and
strlen; for instance, an accented Latin letter that takes two bytes to encode
will cause the rest of the line to be shifted to the left by one. To fix this,
use utf8_strwidth instead of strlen (and compute the padding ourselves, since
printf doesn't know about UTF-8).

Signed-off-by: Geoffrey Thomas <geofft@mit.edu>
---
 builtin-blame.c |    7 ++++---
 1 files changed, 4 insertions(+), 3 deletions(-)

Hi,

On Fri, 30 Jan 2009, Geoffrey Thomas wrote:

Good point (even if your commit message has lines much longer than 72 chars, ASCII ones at that).

But how certain are you at that point that the authors are in UTF-8 format? IOW what encoding conversions were possibly performed up to that point?

You might want to make it easier on possible reviewers by putting that discussion into the commit message.

Ciao, Dscho

Oops; I'll fix that.

I don't believe there are any encoding conversions performed up to that point. IIRC git doesn't require any encoding but encourages UTF-8; if it's something obscure, I have no way of knowing how wide in screen columns the author field is because I likely don't have a library for it in git at all. I do have a utf8.c, though.

Currently, however, printf("%*.*s", width, width, author) is simply wrong, because printf only cares about bytes, not screen columns. Do you think I should fall back on the old behavior if i18n.commitencoding is set, or if at least one of the author names isn't parseable as UTF-8, or something? Or should I be doing this with iconv and assuming all commits are encoded in the current encoding specified via $LANG or $LC_whatever?

On Fri, Jan 30, 2009 at 05:22:07PM -0500, Geoffrey Thomas wrote:

Don't we pull the author from the commit message after it has been converted using reencode_commit_message (see get_commit_info)? That should be respecting the log output encoding.

It looks like we just throw away the information on what we encoded _to_ (i.e., the second parameter of reencode_commit_message). Probably we need to remember that and use a generic "what is the width of this string in this encoding" function.

-Peff

Hi,

On Fri, 30 Jan 2009, Geoffrey Thomas wrote:

I do not know what encoding the author is at that point, but if you cannot be sure that it is UTF-8, using utf8_strwidth() is just as wrong as the current code, IMHO.

Ciao, Dscho

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

That is true, but then we are not losing anything.

This codepath is not about the payload (the contents of the files) but the author name part of the commit log message, and UTF-8 would probably be the only sensible encoding to standardize on.

If your project uses UTF-8 for everybody, great, we will align them better than we did before. If not, sorry, you will get a different misaligned names.

That assumes utf8_width() does not barf when fed an invalid byte sequence, but I did not think it is that fragile (I didn't actually audit the codepath, though).

Hi,

On Sun, 1 Feb 2009, Junio C Hamano wrote:

Almost agree, except for shops where you have an enforced encoding that you cannot easily change.

And last time I checked, many more encodings used 1 character/byte (or for that matter, 1 column / byte) than not; utf8_width would be "more wrong" than strlen() here, because strlen() would "happen to work" here.

There _has_ to be a way to check if the current author string is encoded in UTF-8. All I am asking is that the original poster would put just a _little_ more effort into the issue and make the thing dependent on the knowledge -- as opposed to the assumption -- that the author is encoded in UTF-8.

That is the code that barfs in wcwidth:

        if (ch < 32 || (ch >= 0x7f && ch < 0xa0))
                return -1;

That is not a big problem, but Geoff's code does not handle that case correctly. For example, in Code-page 437, a name like "£ïñûç" would result in a negative width.

But hey, it is definitely not my itch, I will never suffer from the fallout of this patch, as I am safely within US-ASCII. I just thought I saw a potential problem and a possible way out. That's it.

Ciao, Dscho

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

Ahh, you are absolutely right here, and use of utf8_width without checking is actively breaking things.

Yeah, that makes sense.

Thanks for checking --- I suspected something like that would be there somewhere.

On Sun, Feb 01, 2009 at 10:48:51PM -0800, Junio C Hamano wrote:

We should be able to know the encoding (we call reencode_commit_message, but we don't bother to save the result). It should be trivial to do:

int strwidth(const char *s, const char *encoding)
{
  if (!strcmp(encoding, "utf-8"))
    return utf8_strwidth(s);
  /* ideally, else if (some_other_encoding_family) */
  else
    return strlen(s);
}

Then utf-8 is fixed, and other encodings keep identical behavior (and don't even waste cycles on utf-8 decoding). And it should be obvious to anyone who wants to add a width detector for their pet encoding where it should go.

-Peff

On Fri, Jan 30, 2009 at 04:41:28AM -0500, Geoffrey Thomas wrote:

I know next to nothing about our encoding functions, but this seems suspiciously similar to utf8_width in utf8.c. There is also a git_wcwidth, but I don't know how they relate.

-Peff

On Sat, 31 Jan 2009, Jeff King wrote:

git_wcwidth determines the screen columns of a single ucs_char_t. utf8_width returns the git_wcwidth of the first character in a string. utf8_strwidth (the function added by this patch) is a simple loop around utf8_width, because writing the loop every time would be silly.

On that note, there are probably more cases in the code that ought to use something like utf8_strwidth. I only noticed this one case because I'm working on a project with someone with an accented letter in his last name.

On Sat, Jan 31, 2009 at 03:51:48AM -0500, Geoffrey Thomas wrote:

Urgh. Sorry. If I had taken 3 seconds to actually _look_ at your patch, I would have seen that (instead I thought "don't we already have something that does this" and went straight to the existing code).

But no, it looks like we don't already have this, so your patch is fine. Sorry for the noise.

-Peff