Johannes Schindelin [off-list ref] writes:
On Fri, 30 Jan 2009, Geoffrey Thomas wrote:
quoted
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?
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.
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:
Johannes Schindelin [off-list ref] writes:
quoted
On Fri, 30 Jan 2009, Geoffrey Thomas wrote:
quoted
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?
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.
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.
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.
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.
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 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).
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
On Sun, Feb 01, 2009 at 10:48:51PM -0800, Junio C Hamano wrote:
quoted
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.
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).
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
Johannes Schindelin [off-list ref] writes:
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.
Ahh, you are absolutely right here, and use of utf8_width without checking
is actively breaking things.
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.
Yeah, that makes sense.
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.
Thanks for checking --- I suspected something like that would be there
somewhere.