{"thread":{"id":"17452","subject":"[PATCH] builtin-blame.c: Use utf8_strwidth for author's names","startedAt":"2009-01-30T09:41:28Z","lastAt":"2009-02-03T04:30:30Z","messageCount":13,"participants":["Geoffrey Thomas","Johannes Schindelin","Jeff King","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"102558","messageId":"1233308489-2656-1-git-send-email-geofft@mit.edu","threadId":"17452","inReplyTo":null,"subject":"[PATCH] utf8: add utf8_strwidth()","fromName":"Geoffrey Thomas","fromEmail":"geofft@mit.edu","sentAt":"2009-01-30T09:41:28Z","receivedAt":"2009-01-30T09:41:28Z","isPatch":true,"sender":{"key":"geofft@mit.edu","avatar":"https://gravatar.com/avatar/97e2e046698e0410999c17f55c50454e4d6bd1e5091d11beb2ba2c4d8a022fca?d=mp&s=160"},"body":"From: Geoffrey Thomas <geofft@mit.edu>\nI'm about to use this pattern more than once, so make it a common function.\n\nSigned-off-by: Geoffrey Thomas <geofft@mit.edu>\n---\n utf8.c |   12 ++++++++++++\n utf8.h |    1 +\n 2 files changed, 13 insertions(+), 0 deletions(-)\n\ndiff --git a/utf8.c b/utf8.c\nindex dc37353..a2d888d 100644\n--- a/utf8.c\n+++ b/utf8.c\n@@ -246,6 +246,18 @@ int utf8_width(const char **start, size_t *remainder_p)\n \treturn git_wcwidth(ch);\n }\n \n+/*\n+ * Returns the total number of columns required by a null-terminated\n+ * string.\n+ */\n+size_t utf8_strwidth(const char *string)\n+{\n+\tsize_t width = 0;\n+\twhile (string && *string)\n+\t\twidth += utf8_width(&string, NULL);\n+\treturn width;\n+}\n+\n int is_utf8(const char *text)\n {\n \twhile (*text) {\ndiff --git a/utf8.h b/utf8.h\nindex 98cce1b..1ae3450 100644\n--- a/utf8.h\n+++ b/utf8.h\n@@ -5,6 +5,7 @@ typedef unsigned int ucs_char_t;  /* assuming 32bit int */\n \n ucs_char_t pick_one_utf8_char(const char **start, size_t *remainder_p);\n int utf8_width(const char **start, size_t *remainder_p);\n+size_t utf8_strwidth(const char *string);\n int is_utf8(const char *text);\n int is_encoding_utf8(const char *name);\n \n-- \n1.5.6.5\n"},{"id":"102557","messageId":"1233308489-2656-2-git-send-email-geofft@mit.edu","threadId":"17452","inReplyTo":"1233308489-2656-1-git-send-email-geofft@mit.edu","subject":"[PATCH] builtin-blame.c: Use utf8_strwidth for author's names","fromName":"Geoffrey Thomas","fromEmail":"geofft@mit.edu","sentAt":"2009-01-30T09:41:29Z","receivedAt":"2009-01-30T09:41:29Z","isPatch":true,"sender":{"key":"geofft@mit.edu","avatar":"https://gravatar.com/avatar/97e2e046698e0410999c17f55c50454e4d6bd1e5091d11beb2ba2c4d8a022fca?d=mp&s=160"},"body":"From: Geoffrey Thomas <geofft@mit.edu>\ngit blame misaligns output if a author's name has a differing display width and\nstrlen; for instance, an accented Latin letter that takes two bytes to encode\nwill cause the rest of the line to be shifted to the left by one. To fix this,\nuse utf8_strwidth instead of strlen (and compute the padding ourselves, since\nprintf doesn't know about UTF-8).\n\nSigned-off-by: Geoffrey Thomas <geofft@mit.edu>\n---\n builtin-blame.c |    7 ++++---\n 1 files changed, 4 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin-blame.c b/builtin-blame.c\nindex aae14ef..2941fc0 100644\n--- a/builtin-blame.c\n+++ b/builtin-blame.c\n@@ -19,6 +19,7 @@\n #include \"string-list.h\"\n #include \"mailmap.h\"\n #include \"parse-options.h\"\n+#include \"utf8.h\"\n \n static char blame_usage[] = \"git blame [options] [rev-opts] [rev] [--] file\";\n \n@@ -1619,9 +1620,9 @@ static void emit_other(struct scoreboard *sb, struct blame_entry *ent, int opt)\n \t\t\t\t       ent->s_lno + 1 + cnt);\n \n \t\t\tif (!(opt & OUTPUT_NO_AUTHOR))\n-\t\t\t\tprintf(\" (%-*.*s %10s\",\n-\t\t\t\t       longest_author, longest_author,\n+\t\t\t\tprintf(\" (%s%*s %10s\",\n \t\t\t\t       ci.author,\n+\t\t\t\t       longest_author - utf8_strwidth(ci.author), \"\",\n \t\t\t\t       format_time(ci.author_time,\n \t\t\t\t\t\t   ci.author_tz,\n \t\t\t\t\t\t   show_raw_time));\n@@ -1755,7 +1756,7 @@ static void find_alignment(struct scoreboard *sb, int *option)\n \t\tif (!(suspect->commit->object.flags & METAINFO_SHOWN)) {\n \t\t\tsuspect->commit->object.flags |= METAINFO_SHOWN;\n \t\t\tget_commit_info(suspect->commit, &ci, 1);\n-\t\t\tnum = strlen(ci.author);\n+\t\t\tnum = utf8_strwidth(ci.author);\n \t\t\tif (longest_author < num)\n \t\t\t\tlongest_author = num;\n \t\t}\n-- \n1.5.6.5\n"},{"id":"102626","messageId":"alpine.DEB.1.00.0901301811180.3586@pacific.mpi-cbg.de","threadId":"17452","inReplyTo":"1233308489-2656-2-git-send-email-geofft@mit.edu","subject":"Re: [PATCH] builtin-blame.c: Use utf8_strwidth for author's names","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2009-01-30T17:12:39Z","receivedAt":"2009-01-30T17:12:39Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Fri, 30 Jan 2009, Geoffrey Thomas wrote:\n\n> From: Geoffrey Thomas <geofft@mit.edu>\n> git blame misaligns output if a author's name has a differing display width and\n> strlen; for instance, an accented Latin letter that takes two bytes to encode\n> will cause the rest of the line to be shifted to the left by one. To fix this,\n> use utf8_strwidth instead of strlen (and compute the padding ourselves, since\n> printf doesn't know about UTF-8).\n\nGood point (even if your commit message has lines much longer than 72 \nchars, ASCII ones at that).\n\nBut how certain are you at that point that the authors are in UTF-8 \nformat?  IOW what encoding conversions were possibly performed up to that \npoint?\n\nYou might want to make it easier on possible reviewers by putting that \ndiscussion into the commit message.\n\nCiao,\nDscho\n"},{"id":"102648","messageId":"alpine.DEB.2.00.0901301710130.1984@vinegar-pot.mit.edu","threadId":"17452","inReplyTo":"alpine.DEB.1.00.0901301811180.3586@pacific.mpi-cbg.de","subject":"Re: [PATCH] builtin-blame.c: Use utf8_strwidth for author's names","fromName":"Geoffrey Thomas","fromEmail":"geofft@mit.edu","sentAt":"2009-01-30T22:22:07Z","receivedAt":"2009-01-30T22:22:07Z","isPatch":true,"sender":{"key":"geofft@mit.edu","avatar":"https://gravatar.com/avatar/97e2e046698e0410999c17f55c50454e4d6bd1e5091d11beb2ba2c4d8a022fca?d=mp&s=160"},"body":"> Good point (even if your commit message has lines much longer than 72\n> chars, ASCII ones at that).\n\nOops; I'll fix that.\n\n> But how certain are you at that point that the authors are in UTF-8\n> format?  IOW what encoding conversions were possibly performed up to that\n> point?\n\nI don't believe there are any encoding conversions performed up to that \npoint. IIRC git doesn't require any encoding but encourages UTF-8; if it's \nsomething obscure, I have no way of knowing how wide in screen columns the \nauthor field is because I likely don't have a library for it in git at \nall. I do have a utf8.c, though.\n\nCurrently, however, printf(\"%*.*s\", width, width, author) is simply wrong, \nbecause printf only cares about bytes, not screen columns. Do you think I \nshould fall back on the old behavior if i18n.commitencoding is set, or if \nat least one of the author names isn't parseable as UTF-8, or something? \nOr should I be doing this with iconv and assuming all commits are \nencoded in the current encoding specified via $LANG or $LC_whatever?\n\n-- \nGeoffrey Thomas\ngeofft@mit.edu\n"},{"id":"102683","messageId":"20090131071711.GA3710@coredump.intra.peff.net","threadId":"17452","inReplyTo":"1233308489-2656-1-git-send-email-geofft@mit.edu","subject":"Re: [PATCH] utf8: add utf8_strwidth()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2009-01-31T07:17:11Z","receivedAt":"2009-01-31T07:17:11Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jan 30, 2009 at 04:41:28AM -0500, Geoffrey Thomas wrote:\n\n> I'm about to use this pattern more than once, so make it a common function.\n\nI know next to nothing about our encoding functions, but this seems\nsuspiciously similar to utf8_width in utf8.c. There is also a\ngit_wcwidth, but I don't know how they relate.\n\n-Peff\n"},{"id":"102684","messageId":"20090131072425.GD3033@coredump.intra.peff.net","threadId":"17452","inReplyTo":"alpine.DEB.2.00.0901301710130.1984@vinegar-pot.mit.edu","subject":"Re: [PATCH] builtin-blame.c: Use utf8_strwidth for author's names","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2009-01-31T07:24:25Z","receivedAt":"2009-01-31T07:24:25Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jan 30, 2009 at 05:22:07PM -0500, Geoffrey Thomas wrote:\n\n> I don't believe there are any encoding conversions performed up to that  \n> point. IIRC git doesn't require any encoding but encourages UTF-8; if it's \n> something obscure, I have no way of knowing how wide in screen columns the \n> author field is because I likely don't have a library for it in git at  \n> all. I do have a utf8.c, though.\n\nDon't we pull the author from the commit message after it has been\nconverted using reencode_commit_message (see get_commit_info)? That\nshould be respecting the log output encoding.\n\nIt looks like we just throw away the information on what we encoded _to_\n(i.e., the second parameter of reencode_commit_message).  Probably we\nneed to remember that and use a generic \"what is the width of this\nstring in this encoding\" function.\n\n-Peff\n"},{"id":"102687","messageId":"alpine.DEB.2.00.0901310348000.3373@vinegar-pot.mit.edu","threadId":"17452","inReplyTo":"20090131071711.GA3710@coredump.intra.peff.net","subject":"Re: [PATCH] utf8: add utf8_strwidth()","fromName":"Geoffrey Thomas","fromEmail":"geofft@mit.edu","sentAt":"2009-01-31T08:51:48Z","receivedAt":"2009-01-31T08:51:48Z","isPatch":true,"sender":{"key":"geofft@mit.edu","avatar":"https://gravatar.com/avatar/97e2e046698e0410999c17f55c50454e4d6bd1e5091d11beb2ba2c4d8a022fca?d=mp&s=160"},"body":"On Sat, 31 Jan 2009, Jeff King wrote:\n> I know next to nothing about our encoding functions, but this seems\n> suspiciously similar to utf8_width in utf8.c. There is also a\n> git_wcwidth, but I don't know how they relate.\n\ngit_wcwidth determines the screen columns of a single ucs_char_t. \nutf8_width returns the git_wcwidth of the first character in a string. \nutf8_strwidth (the function added by this patch) is a simple loop around \nutf8_width, because writing the loop every time would be silly.\n\nOn that note, there are probably more cases in the code that ought to use \nsomething like utf8_strwidth. I only noticed this one case because I'm \nworking on a project with someone with an accented letter in his last \nname.\n\n-- \nGeoffrey Thomas\ngeofft@mit.edu\n"},{"id":"102688","messageId":"20090131085652.GA6484@coredump.intra.peff.net","threadId":"17452","inReplyTo":"alpine.DEB.2.00.0901310348000.3373@vinegar-pot.mit.edu","subject":"Re: [PATCH] utf8: add utf8_strwidth()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2009-01-31T08:56:52Z","receivedAt":"2009-01-31T08:56:52Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Jan 31, 2009 at 03:51:48AM -0500, Geoffrey Thomas wrote:\n\n> On Sat, 31 Jan 2009, Jeff King wrote:\n>> I know next to nothing about our encoding functions, but this seems\n>> suspiciously similar to utf8_width in utf8.c. There is also a\n>> git_wcwidth, but I don't know how they relate.\n>\n> git_wcwidth determines the screen columns of a single ucs_char_t.  \n> utf8_width returns the git_wcwidth of the first character in a string.  \n> utf8_strwidth (the function added by this patch) is a simple loop around  \n> utf8_width, because writing the loop every time would be silly.\n\nUrgh. Sorry. If I had taken 3 seconds to actually _look_ at your patch,\nI would have seen that (instead I thought \"don't we already have\nsomething that does this\" and went straight to the existing code).\n\nBut no, it looks like we don't already have this, so your patch is fine.\nSorry for the noise.\n\n-Peff\n"},{"id":"102788","messageId":"alpine.DEB.1.00.0902012333060.3586@pacific.mpi-cbg.de","threadId":"17452","inReplyTo":"alpine.DEB.2.00.0901301710130.1984@vinegar-pot.mit.edu","subject":"Re: [PATCH] builtin-blame.c: Use utf8_strwidth for author's names","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2009-02-01T22:34:21Z","receivedAt":"2009-02-01T22:34:21Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Fri, 30 Jan 2009, Geoffrey Thomas wrote:\n\n> Currently, however, printf(\"%*.*s\", width, width, author) is simply \n> wrong, because printf only cares about bytes, not screen columns. Do you \n> think I should fall back on the old behavior if i18n.commitencoding is \n> set, or if at least one of the author names isn't parseable as UTF-8, or \n> something? Or should I be doing this with iconv and assuming all commits \n> are encoded in the current encoding specified via $LANG or $LC_whatever?\n\nI do not know what encoding the author is at that point, but if you cannot \nbe sure that it is UTF-8, using utf8_strwidth() is just as wrong as the \ncurrent code, IMHO.\n\nCiao,\nDscho\n"},{"id":"102828","messageId":"7v8wopmizw.fsf@gitster.siamese.dyndns.org","threadId":"17452","inReplyTo":"alpine.DEB.1.00.0902012333060.3586@pacific.mpi-cbg.de","subject":"Re: [PATCH] builtin-blame.c: Use utf8_strwidth for author's names","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-02-02T06:48:51Z","receivedAt":"2009-02-02T06:48:51Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:\n\n> On Fri, 30 Jan 2009, Geoffrey Thomas wrote:\n>\n>> Currently, however, printf(\"%*.*s\", width, width, author) is simply \n>> wrong, because printf only cares about bytes, not screen columns. Do you \n>> think I should fall back on the old behavior if i18n.commitencoding is \n>> set, or if at least one of the author names isn't parseable as UTF-8, or \n>> something? Or should I be doing this with iconv and assuming all commits \n>> are encoded in the current encoding specified via $LANG or $LC_whatever?\n>\n> I do not know what encoding the author is at that point, but if you cannot \n> be sure that it is UTF-8, using utf8_strwidth() is just as wrong as the \n> current code, IMHO.\n\nThat is true, but then we are not losing anything.\n\nThis codepath is not about the payload (the contents of the files) but the\nauthor name part of the commit log message, and UTF-8 would probably be\nthe only sensible encoding to standardize on.\n\nIf your project uses UTF-8 for everybody, great, we will align them better\nthan we did before.  If not, sorry, you will get a different misaligned\nnames.\n\nThat assumes utf8_width() does not barf when fed an invalid byte sequence,\nbut I did not think it is that fragile (I didn't actually audit the\ncodepath, though).\n"},{"id":"102851","messageId":"alpine.DEB.1.00.0902021326420.3586@pacific.mpi-cbg.de","threadId":"17452","inReplyTo":"7v8wopmizw.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] builtin-blame.c: Use utf8_strwidth for author's names","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2009-02-02T12:40:11Z","receivedAt":"2009-02-02T12:40:11Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Sun, 1 Feb 2009, Junio C Hamano wrote:\n\n> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:\n> \n> > On Fri, 30 Jan 2009, Geoffrey Thomas wrote:\n> >\n> >> Currently, however, printf(\"%*.*s\", width, width, author) is simply \n> >> wrong, because printf only cares about bytes, not screen columns. Do \n> >> you think I should fall back on the old behavior if \n> >> i18n.commitencoding is set, or if at least one of the author names \n> >> isn't parseable as UTF-8, or something? Or should I be doing this \n> >> with iconv and assuming all commits are encoded in the current \n> >> encoding specified via $LANG or $LC_whatever?\n> >\n> > I do not know what encoding the author is at that point, but if you \n> > cannot be sure that it is UTF-8, using utf8_strwidth() is just as \n> > wrong as the current code, IMHO.\n> \n> That is true, but then we are not losing anything.\n> \n> This codepath is not about the payload (the contents of the files) but \n> the author name part of the commit log message, and UTF-8 would probably \n> be the only sensible encoding to standardize on.\n\nAlmost agree, except for shops where you have an enforced encoding that \nyou cannot easily change.\n\nAnd last time I checked, many more encodings used 1 character/byte (or for \nthat matter, 1 column / byte) than not; utf8_width would be \"more wrong\" \nthan strlen() here, because strlen() would \"happen to work\" here.\n\n> If your project uses UTF-8 for everybody, great, we will align them \n> better than we did before.  If not, sorry, you will get a different \n> misaligned names.\n\nThere _has_ to be a way to check if the current author string is encoded \nin UTF-8.  All I am asking is that the original poster would put just a \n_little_ more effort into the issue and make the thing dependent on the \nknowledge -- as opposed to the assumption -- that the author is encoded in \nUTF-8.\n\n> That assumes utf8_width() does not barf when fed an invalid byte \n> sequence, but I did not think it is that fragile (I didn't actually \n> audit the codepath, though).\n\nThat is the code that barfs in wcwidth:\n\n        if (ch < 32 || (ch >= 0x7f && ch < 0xa0))\n                return -1;\n\nThat is not a big problem, but Geoff's code does not handle that case \ncorrectly.  For example, in Code-page 437, a name like \"£ïñûç\" would \nresult in a negative width.\n\nBut hey, it is definitely not my itch, I will never suffer from the \nfallout of this patch, as I am safely within US-ASCII.  I just thought I \nsaw a potential problem and a possible way out.  That's it.\n\nCiao,\nDscho\n"},{"id":"102852","messageId":"20090202124139.GA8325@sigio.peff.net","threadId":"17452","inReplyTo":"7v8wopmizw.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] builtin-blame.c: Use utf8_strwidth for author's names","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2009-02-02T12:41:40Z","receivedAt":"2009-02-02T12:41:40Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Feb 01, 2009 at 10:48:51PM -0800, Junio C Hamano wrote:\n\n> > I do not know what encoding the author is at that point, but if you cannot \n> > be sure that it is UTF-8, using utf8_strwidth() is just as wrong as the \n> > current code, IMHO.\n> \n> That is true, but then we are not losing anything.\n> \n> This codepath is not about the payload (the contents of the files) but the\n> author name part of the commit log message, and UTF-8 would probably be\n> the only sensible encoding to standardize on.\n> \n> If your project uses UTF-8 for everybody, great, we will align them better\n> than we did before.  If not, sorry, you will get a different misaligned\n> names.\n> \n> That assumes utf8_width() does not barf when fed an invalid byte sequence,\n> but I did not think it is that fragile (I didn't actually audit the\n> codepath, though).\n\nWe should be able to know the encoding (we call reencode_commit_message,\nbut we don't bother to save the result). It should be trivial to do:\n\nint strwidth(const char *s, const char *encoding)\n{\n  if (!strcmp(encoding, \"utf-8\"))\n    return utf8_strwidth(s);\n  /* ideally, else if (some_other_encoding_family) */\n  else\n    return strlen(s);\n}\n\nThen utf-8 is fixed, and other encodings keep identical behavior (and\ndon't even waste cycles on utf-8 decoding). And it should be obvious to\nanyone who wants to add a width detector for their pet encoding where it\nshould go.\n\n-Peff\n"},{"id":"102914","messageId":"7vd4e0f8gp.fsf@gitster.siamese.dyndns.org","threadId":"17452","inReplyTo":"alpine.DEB.1.00.0902021326420.3586@pacific.mpi-cbg.de","subject":"Re: [PATCH] builtin-blame.c: Use utf8_strwidth for author's names","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-02-03T04:30:30Z","receivedAt":"2009-02-03T04:30:30Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:\n\n> And last time I checked, many more encodings used 1 character/byte (or for \n> that matter, 1 column / byte) than not; utf8_width would be \"more wrong\" \n> than strlen() here, because strlen() would \"happen to work\" here.\n\nAhh, you are absolutely right here, and use of utf8_width without checking\nis actively breaking things.\n\n> There _has_ to be a way to check if the current author string is encoded \n> in UTF-8.  All I am asking is that the original poster would put just a \n> _little_ more effort into the issue and make the thing dependent on the \n> knowledge -- as opposed to the assumption -- that the author is encoded in \n> UTF-8.\n\nYeah, that makes sense.\n\n> That is the code that barfs in wcwidth:\n>\n>         if (ch < 32 || (ch >= 0x7f && ch < 0xa0))\n>                 return -1;\n>\n> That is not a big problem, but Geoff's code does not handle that case \n> correctly.\n\nThanks for checking --- I suspected something like that would be there\nsomewhere.\n"}]}