{"thread":{"id":"27118","subject":"[PATCH 1/2] mailinfo.c: Allow convert_to_utf8() to specify both src/dst charset and do conversion alone","startedAt":"2011-04-16T20:49:31Z","lastAt":"2011-04-18T19:36:22Z","messageCount":2,"participants":["ZHANG, Le","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"165983","messageId":"1302986971-28080-1-git-send-email-r0bertz@gentoo.org","threadId":"27118","inReplyTo":null,"subject":"[PATCH 1/2] mailinfo.c: Allow convert_to_utf8() to specify both src/dst charset and do conversion alone","fromName":"ZHANG, Le","fromEmail":"r0bertz@gentoo.org","sentAt":"2011-04-16T20:49:31Z","receivedAt":"2011-04-16T20:49:31Z","isPatch":true,"sender":{"key":"r0bertz@gentoo.org","avatar":"https://avatars.githubusercontent.com/u/29025?v=4"},"body":"The convert_to_utf8() function actually converts to whatever charset\n\"metainfo_charset\" variable contains, which is not necessarily UTF-8.\nRename it to convert_to(), and give an extra parameter \"to_charset\" to\nspecify what charset to re-encode to. Also rename its \"charset\"\nparameter to \"from_charset\" to clarify which is which.\n\nAlso convert_to_utf8() tries to guess what the src charset is if it is\nnot specified. And the guess_charset() function does not do exactly what\nits name says. So make convert_to() only do the conversion. Make a new\nfunction guess_and_convert_to().\n\nSigned-off-by: ZHANG, Le \n---\n builtin/mailinfo.c | 44 ++++++++++++++++++++------------------------\n 1 files changed, 20 insertions(+), 24 deletions(-)\n\ndiff --git a/builtin/mailinfo.c b/builtin/mailinfo.c\nindex 71e6262..0f42ff1 100644\n--- a/builtin/mailinfo.c\n+++ b/builtin/mailinfo.c\n@@ -472,6 +472,20 @@ static struct strbuf *decode_b_segment(const struct strbuf *b_seg)\n \treturn out;\n }\n \n+static void convert_to(struct strbuf *line, const char *to_charset, const char *from_charset)\n+{\n+\tchar *out;\n+\n+\tif (!strcasecmp(to_charset, from_charset))\n+\t\treturn;\n+\n+\tout = reencode_string(line->buf, to_charset, from_charset);\n+\tif (!out)\n+\t\tdie(\"cannot convert from %s to %s\",\n+\t\t from_charset, to_charset);\n+\tstrbuf_attach(line, out, strlen(out), strlen(out));\n+}\n+\n /*\n * When there is no known charset, guess.\n *\n@@ -483,32 +497,14 @@ static struct strbuf *decode_b_segment(const struct strbuf *b_seg)\n * Otherwise, we default to assuming it is Latin1 for historical\n * reasons.\n */\n-static const char *guess_charset(const struct strbuf *line, const char *target_charset)\n+static void guess_and_convert_to(struct strbuf *line, const char *to_charset)\n {\n-\tif (is_encoding_utf8(target_charset)) {\n+\tif (is_encoding_utf8(to_charset)) {\n \t\tif (is_utf8(line->buf))\n-\t\t\treturn NULL;\n-\t}\n-\treturn \"ISO8859-1\";\n-}\n-\n-static void convert_to_utf8(struct strbuf *line, const char *charset)\n-{\n-\tchar *out;\n-\n-\tif (!charset || !*charset) {\n-\t\tcharset = guess_charset(line, metainfo_charset);\n-\t\tif (!charset)\n \t\t\treturn;\n \t}\n \n-\tif (!strcasecmp(metainfo_charset, charset))\n-\t\treturn;\n-\tout = reencode_string(line->buf, metainfo_charset, charset);\n-\tif (!out)\n-\t\tdie(\"cannot convert from %s to %s\",\n-\t\t charset, metainfo_charset);\n-\tstrbuf_attach(line, out, strlen(out), strlen(out));\n+ convert_to(line, to_charset, \"ISO8859-1\");\n }\n \n static int decode_header_bq(struct strbuf *it)\n@@ -577,7 +573,7 @@ static int decode_header_bq(struct strbuf *it)\n \t\t\tbreak;\n \t\t}\n \t\tif (metainfo_charset)\n-\t\t\tconvert_to_utf8(dec, charset_q.buf);\n+\t\t\tconvert_to(dec, metainfo_charset, charset_q.buf);\n \n \t\tstrbuf_addbuf(&outbuf, dec);\n \t\tstrbuf_release(dec);\n@@ -602,7 +598,7 @@ static void decode_header(struct strbuf *it)\n \t * This can be binary guck but there is no charset specified.\n \t */\n \tif (metainfo_charset)\n-\t\tconvert_to_utf8(it, \"\");\n+\t\tguess_and_convert_to(it, metainfo_charset);\n }\n \n static void decode_transfer_encoding(struct strbuf *line)\n@@ -796,7 +792,7 @@ static int handle_commit_msg(struct strbuf *line)\n \n \t/* normalize the log message to UTF-8. */\n \tif (metainfo_charset)\n-\t\tconvert_to_utf8(line, charset.buf);\n+\t\tconvert_to(line, metainfo_charset, charset.buf);\n \n \tif (use_scissors && is_scissors_line(line)) {\n \t\tint i;\n-- \n1.7.5.rc2.5.gb2ee76.dirty\n"},{"id":"166042","messageId":"7vd3kj8i89.fsf@alter.siamese.dyndns.org","threadId":"27118","inReplyTo":"1302986971-28080-1-git-send-email-r0bertz@gentoo.org","subject":"Re: [PATCH 1/2] mailinfo.c: Allow convert_to_utf8() to specify both src/dst charset and do conversion alone","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-04-18T19:36:22Z","receivedAt":"2011-04-18T19:36:22Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"ZHANG, Le\" writes:\n\n> And the guess_charset() function does not do exactly what\n> its name says.\n\nReally? See below.\n\n> @@ -483,32 +497,14 @@ static struct strbuf *decode_b_segment(const struct strbuf *b_seg)\n> * Otherwise, we default to assuming it is Latin1 for historical\n> * reasons.\n> */\n> -static const char *guess_charset(const struct strbuf *line, const char *target_charset)\n> +static void guess_and_convert_to(struct strbuf *line, const char *to_charset)\n> {\n> +\tif (is_encoding_utf8(to_charset)) {\n> \t\tif (is_utf8(line->buf))\n> \t\t\treturn;\n> \t}\n> \n> + convert_to(line, to_charset, \"ISO8859-1\");\n\nBroken indent.\n\nI have to wonder if this helper should be inlined into its single caller,\ni.e.\n\n \tif (metainfo_charset) {\n\t\tif (is_encoding_utf8(metainfo_charset) && is_utf8(it->buf))\n\t\t\t; /* nothing to be done */\n\t\telse\n\t\t\tconvert_to(it, metainfo_charset, \"ISO8859-1\");\n\t}\n\nThe decode_header() codepath is the only place that we have to handle a\npossible binary guck without an explicit \"this piece of text is in that\nencoding\" available in the message, and we guess by \"if the data looks\nlike utf-8, we treat it as such, otherwise we assume it is 8859-1 which\nwas historically popular\". All the other codepaths we should know what\nencoding the incoming data is in.\n\nHaving said all that, I do not think your patch is correct.\n\n - Does the code with your patch work correctly when the incoming data is\n pre-MIME and does not specify charset, in which case charset.buf may be\n an empty string?\n\n - When the incoming data does not say in what encoding it is in, our\n intended behaviour is to inspect each line, and if it looks like utf-8\n then assume it is in utf-8, otherwise assume it is in 8859-1. And then\n we convert it to whatever encoding the repository wants, often utf-8\n (coming from metainfo_charset, suitable for recording commit log\n messages).\n\n We might want to change this heuristic in the future, but I do not see\n a need for doing so right now (Cf. b59d398: Do a better job at guessing\n unknown character sets, 2007-07-17).\n\n I do think the guess_and_convert_to() does not implement that intended\n logic correctly. When we are _not_ encoding to UTF-8, we do not even\n bother to inspect the data to guess if it is UTF-8. Shouldn't it be\n more like (modulo \"NULL implies utf-8\"):\n\n\tif (is_utf8(it->buf))\n \tfrom_charset = \"utf-8\";\n\telse\n \tfrom_charset = \"ISO8859-1\";\n\tif (is_encoding_utf8(metainfo_charset) && !strcmp(from_charset, \"utf-8\"))\n \t; /* nothing to do */\n\telse if (strcasecmp(to_charset, from_charset))\n\t\tconvert_to(it, metainfo_charset, from_charset);\n\n - I don't think the commit message part is handled correctly anymore with\n your patch. When you want UTF-8 commit log message (metainfo_charset is\n set to utf-8), and when the incoming data does not have its charset\n specified, we should be doing the same \"guess line-by-line\" conversion.\n You seem to have lost that with this patch, which would be a grave\n regression.\n\nPlease apply the attached patch that adds a test at the end of t5100 and\nmake sure the test passes to prevent that regression from happening.\n\nThanks.\n\n"}]}