{"thread":{"id":"15988","subject":"[RFC PATCH] commit: Warn about encodings unsupported by iconv.","startedAt":"2008-10-20T21:25:18Z","lastAt":"2008-10-21T10:43:25Z","messageCount":4,"participants":["Alexander Gavrilov","Junio C Hamano","Alex Riesen"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"93558","messageId":"1224537918-14024-1-git-send-email-angavrilov@gmail.com","threadId":"15988","inReplyTo":null,"subject":"[RFC PATCH] commit: Warn about encodings unsupported by iconv.","fromName":"Alexander Gavrilov","fromEmail":"angavrilov@gmail.com","sentAt":"2008-10-20T21:25:18Z","receivedAt":"2008-10-20T21:25:18Z","isPatch":true,"sender":{"key":"angavrilov@gmail.com","avatar":"https://avatars.githubusercontent.com/u/42666?v=4"},"body":"Currently git-commit and git-commit-tree silently allow\nusage of encodings that are unknown to iconv. This may\nconfuse the user, who then won't be able to use automatic\nencoding conversion in git-log and friends without any\nimmediately obvious reason. Note that the difference\nbetween a supported and an unsupported name may be as\nsmall as CP1251 vs CP-1251, or Shift-JIS vs ShiftJIS.\n\nThis commit adds a check for such cases, which produces\na warning similar to the one issued when a message claims\nto be utf-8, but actually is not.\n\nSigned-off-by: Alexander Gavrilov <angavrilov@gmail.com>\n---\n\n\tI wonder how common such situation may actually be, and \n\twhether gitk & git-gui (or core git itself) should explicitly\n\tprovide some way to deal with it in old commits. I personally\n\thit it during experimenting, and wrongly concluded that gitk\n\tdoes not support using multiple encodings in one repository.\n\t\n\tCurrent gitk implementation generally allows working around\n\tit by setting i18n.commitencoding to a valid name of the\n\tencoding used in the mislabeled commits. However, the\n\tdownside is that if the selected encoding cannot represent\n\tsome characters of an otherwise completely valid commit,\n\tthey come out as garbage. Always using --encoding=utf-8\n\tfrom GUI and relying on conversion done by git-log should\n\tfix this case, but it breaks the workaround.\n\t\n\t-- Alexander\n\n builtin-commit-tree.c |   25 +++++++++++++++++++++++--\n 1 files changed, 23 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin-commit-tree.c b/builtin-commit-tree.c\nindex 0453425..7f325a3 100644\n--- a/builtin-commit-tree.c\n+++ b/builtin-commit-tree.c\n@@ -45,6 +45,28 @@ static const char commit_utf8_warn[] =\n \"You may want to amend it after fixing the message, or set the config\\n\"\n \"variable i18n.commitencoding to the encoding your project uses.\\n\";\n \n+static const char commit_bad_encoding_warn[] =\n+\"Warning: commit encoding '%s' is not supported.\\n\"\n+\"You may want to change the value of the i18n.commitencoding config\\n\"\n+\"variable, and redo the commit. Use 'iconv --list' to see the list of\\n\"\n+\"available encoding names.\\n\";\n+\n+static void verify_commit_encoding(const char *text, const char *encoding)\n+{\n+\tif (is_encoding_utf8(encoding)) {\n+\t\tif (!is_utf8(text))\n+\t\t\tfprintf(stderr, commit_utf8_warn);\n+\t}\n+#ifndef NO_ICONV\n+\telse {\n+\t\tchar *conv = reencode_string(\"\", \"utf-8\", encoding);\n+\t\tif (!conv)\n+\t\t\tfprintf(stderr, commit_bad_encoding_warn, encoding);\n+\t\tfree(conv);\n+\t}\n+#endif\n+}\n+\n int commit_tree(const char *msg, unsigned char *tree,\n \t\tstruct commit_list *parents, unsigned char *ret,\n \t\tconst char *author)\n@@ -87,8 +109,7 @@ int commit_tree(const char *msg, unsigned char *tree,\n \tstrbuf_addstr(&buffer, msg);\n \n \t/* And check the encoding */\n-\tif (encoding_is_utf8 && !is_utf8(buffer.buf))\n-\t\tfprintf(stderr, commit_utf8_warn);\n+\tverify_commit_encoding(buffer.buf, git_commit_encoding);\n \n \tresult = write_sha1_file(buffer.buf, buffer.len, commit_type, ret);\n \tstrbuf_release(&buffer);\n-- \n1.6.0.20.g6148bc\n"},{"id":"93570","messageId":"7vmygy233r.fsf@gitster.siamese.dyndns.org","threadId":"15988","inReplyTo":"1224537918-14024-1-git-send-email-angavrilov@gmail.com","subject":"Re: [RFC PATCH] commit: Warn about encodings unsupported by iconv.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-10-21T00:39:36Z","receivedAt":"2008-10-21T00:39:36Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Alexander Gavrilov <angavrilov@gmail.com> writes:\n\n> diff --git a/builtin-commit-tree.c b/builtin-commit-tree.c\n> index 0453425..7f325a3 100644\n> --- a/builtin-commit-tree.c\n> +++ b/builtin-commit-tree.c\n> @@ -45,6 +45,28 @@ static const char commit_utf8_warn[] =\n>  \"You may want to amend it after fixing the message, or set the config\\n\"\n>  \"variable i18n.commitencoding to the encoding your project uses.\\n\";\n>  \n> +static const char commit_bad_encoding_warn[] =\n> +\"Warning: commit encoding '%s' is not supported.\\n\"\n> +\"You may want to change the value of the i18n.commitencoding config\\n\"\n> +\"variable, and redo the commit. Use 'iconv --list' to see the list of\\n\"\n> +\"available encoding names.\\n\";\n> +\n> +static void verify_commit_encoding(const char *text, const char *encoding)\n> +{\n> +\tif (is_encoding_utf8(encoding)) {\n> +\t\tif (!is_utf8(text))\n> +\t\t\tfprintf(stderr, commit_utf8_warn);\n> +\t}\n> +#ifndef NO_ICONV\n> +\telse {\n> +\t\tchar *conv = reencode_string(\"\", \"utf-8\", encoding);\n> +\t\tif (!conv)\n> +\t\t\tfprintf(stderr, commit_bad_encoding_warn, encoding);\n> +\t\tfree(conv);\n> +\t}\n> +#endif\n> +}\n> +\n\nI think the issue you are attempting to tackle is worth addressing, but I\nam not sure if this is the best approach.\n\nCommit_tree() does not re-encode the payload.\n\nIt just marks it with the encoding header.  Wouldn't that mean that it\nshould be possible for you to create a commit with its message encoded in\nKOI-8, and mark the resulting commit as encoded as such, on a host that is\nincapable of actually transcoding from KOI-8 to utf-8?  IOW, your being\nable to encode from i18n.commitencoding to utf-8 does not have much to do\nwith the validity of the configuration variable.\n\nIt would clarify the issues to think about what this new code would do on\na host without iconv, if you do not have the above #ifndef/#endif pair.\nThe replacement reencode_string() implementation always returns NULL, so\nthe code will always warn.\n\nI am guessing that the reason you added #ifndef/#endif is because what the\nwarning message says is incorrect.\n\n * \"is not supported\" is not correct.  \"is not supported HERE\" may be.\n\n * \"is not supported\" (nor \"is not supported HERE\") does not matter.  It\n   is log-reading side that does the re-encoding, not the commit\n   generating side.\n\n * what you would really want to say is \"might be incorrectly spelled\",\n   but your problem is that you do not have a direct way to check that.\n\nAnother reason of your \"#ifndef/#endif\" would be that there is no way to\nsquelch the warning message after seeing it on a NO_ICONV platform.\n\nBut that suggests that the \"#ifndef/#endif\" is not a good way to squelch\nthe message.  What would you do, after seeing the warning message and\nexamining the situation, you know KOI-8 is a valid encoding name, your\neditor is recording what you type in the commit log message in KOI-8, you\nknow you set i18n.commitencoding to KOI-8, and you know somehow your\nsystem is incapable of reencode_string(\"\", \"utf-8\" \"KOI-8\")?  There is no\nway to squelch the message.\n\nSo perhaps you would need some \"state\" variable that says \"I know this\ni18n.commitencoding configuration is valid\" if you go this route?  But the\nreason for \"I know\" would be either (1) because we earlier tried\nreencode_string() and it resulted in an Ok return, or (2) because the user\nverified that the configuration is valid, even though on this particular\nsystem it cannot be encoded to utf-8.  IOW, the latter one would be\n\"because the user tells us so\" --- which would be the same as trusting\ni18n.commitencoding from the beginning.  I dunno.\n\nI actually have an alternative approach to suggest.\n\nHow about adding a new i18n.commit-reencode-logmessage option (boolean),\nand when it is set, we actually re-encode from i18n.commitencoding to\n\"utf-8\" before creating the commit object (and obviously we do not store\n\"encoding\" header in the resulting commit)?  When the conversion fails, we\nknow it failed, so the warning you added does make sense in that context.\n"},{"id":"93578","messageId":"20081021061754.GA4656@blimp.localdomain","threadId":"15988","inReplyTo":"7vmygy233r.fsf@gitster.siamese.dyndns.org","subject":"Re: [RFC PATCH] commit: Warn about encodings unsupported by iconv.","fromName":"Alex Riesen","fromEmail":"raa.lkml@gmail.com","sentAt":"2008-10-21T06:17:54Z","receivedAt":"2008-10-21T06:17:54Z","isPatch":true,"sender":{"key":"raa.lkml@gmail.com","avatar":"https://avatars.githubusercontent.com/u/324101?v=4"},"body":"Junio C Hamano, Tue, Oct 21, 2008 02:39:36 +0200:\n> I actually have an alternative approach to suggest.\n> \n> How about adding a new i18n.commit-reencode-logmessage option (boolean),\n> and when it is set, we actually re-encode from i18n.commitencoding to\n> \"utf-8\" before creating the commit object (and obviously we do not store\n> \"encoding\" header in the resulting commit)?  When the conversion fails, we\n> know it failed, so the warning you added does make sense in that context.\n\nMaybe make the option a string, and allow to choose the target\nencoding (not only utf8, but anything user wishes)?\n"},{"id":"93590","messageId":"bb6f213e0810210343l3fd36056m3d087fef4b134f14@mail.gmail.com","threadId":"15988","inReplyTo":"7vmygy233r.fsf@gitster.siamese.dyndns.org","subject":"Re: [RFC PATCH] commit: Warn about encodings unsupported by iconv.","fromName":"Alexander Gavrilov","fromEmail":"angavrilov@gmail.com","sentAt":"2008-10-21T10:43:25Z","receivedAt":"2008-10-21T10:43:25Z","isPatch":true,"sender":{"key":"angavrilov@gmail.com","avatar":"https://avatars.githubusercontent.com/u/42666?v=4"},"body":"On Tue, Oct 21, 2008 at 4:39 AM, Junio C Hamano <gitster@pobox.com> wrote:\n> Alexander Gavrilov <angavrilov@gmail.com> writes:\n>\n>> diff --git a/builtin-commit-tree.c b/builtin-commit-tree.c\n>> index 0453425..7f325a3 100644\n>> --- a/builtin-commit-tree.c\n>> +++ b/builtin-commit-tree.c\n>> @@ -45,6 +45,28 @@ static const char commit_utf8_warn[] =\n>>  \"You may want to amend it after fixing the message, or set the config\\n\"\n>>  \"variable i18n.commitencoding to the encoding your project uses.\\n\";\n>>\n>> +static const char commit_bad_encoding_warn[] =\n>> +\"Warning: commit encoding '%s' is not supported.\\n\"\n>> +\"You may want to change the value of the i18n.commitencoding config\\n\"\n>> +\"variable, and redo the commit. Use 'iconv --list' to see the list of\\n\"\n>> +\"available encoding names.\\n\";\n>> +\n>> +static void verify_commit_encoding(const char *text, const char *encoding)\n>> +{\n>> +     if (is_encoding_utf8(encoding)) {\n>> +             if (!is_utf8(text))\n>> +                     fprintf(stderr, commit_utf8_warn);\n>> +     }\n>> +#ifndef NO_ICONV\n>> +     else {\n>> +             char *conv = reencode_string(\"\", \"utf-8\", encoding);\n>> +             if (!conv)\n>> +                     fprintf(stderr, commit_bad_encoding_warn, encoding);\n>> +             free(conv);\n>> +     }\n>> +#endif\n>> +}\n>> +\n\n> It just marks it with the encoding header.  Wouldn't that mean that it\n> should be possible for you to create a commit with its message encoded in\n> KOI-8, and mark the resulting commit as encoded as such, on a host that is\n> incapable of actually transcoding from KOI-8 to utf-8?  IOW, your being\n> able to encode from i18n.commitencoding to utf-8 does not have much to do\n> with the validity of the configuration variable.\n\nIt may be possible to check for reencode_string(\"\", encoding,\nencoding). IConv should be able to do the identity conversion for any\nvalid encoding. I checked with the iconv command-line executable on my\nsystem, and it correctly errors out on invalid names, and agrees to do\nthe conversion for valid ones.\n\n> It would clarify the issues to think about what this new code would do on\n> a host without iconv, if you do not have the above #ifndef/#endif pair.\n> The replacement reencode_string() implementation always returns NULL, so\n> the code will always warn.\n>\n> I am guessing that the reason you added #ifndef/#endif is because what the\n> warning message says is incorrect.\n>\n>  * \"is not supported\" is not correct.  \"is not supported HERE\" may be.\n>\n>  * \"is not supported\" (nor \"is not supported HERE\") does not matter.  It\n>   is log-reading side that does the re-encoding, not the commit\n>   generating side.\n\nThe trouble is, by the time it gets to the \"log-reading\" side, it may\nbe too late to do anything, because the commits have already been\ncreated, hashed and pushed out somewhere. And the originating side\nwon't notice because they would actually expect to get the messages\nwithout conversion from their git-log. It may be amended somewhat if\ngitk insists on receiving all data from git-log in utf-8 unless\nexplicitly told otherwise; on the other hand it may make people think\nthat the problem is in gitk...\n\n>  * what you would really want to say is \"might be incorrectly spelled\",\n>   but your problem is that you do not have a direct way to check that.\n>\n> Another reason of your \"#ifndef/#endif\" would be that there is no way to\n> squelch the warning message after seeing it on a NO_ICONV platform.\n>\n> But that suggests that the \"#ifndef/#endif\" is not a good way to squelch\n> the message.  What would you do, after seeing the warning message and\n> examining the situation, you know KOI-8 is a valid encoding name, your\n> editor is recording what you type in the commit log message in KOI-8, you\n> know you set i18n.commitencoding to KOI-8, and you know somehow your\n> system is incapable of reencode_string(\"\", \"utf-8\" \"KOI-8\")?  There is no\n> way to squelch the message.\n\nA bit of a problem is that unless git implements its own encoding\nalias table, the only authority on valid encoding names is iconv. So\nchecking would still involve running reencode_string, or looking at\niconv --list, possibly on another machine.\n\n> So perhaps you would need some \"state\" variable that says \"I know this\n> i18n.commitencoding configuration is valid\" if you go this route?  But the\n> reason for \"I know\" would be either (1) because we earlier tried\n> reencode_string() and it resulted in an Ok return, or (2) because the user\n> verified that the configuration is valid, even though on this particular\n> system it cannot be encoded to utf-8.  IOW, the latter one would be\n> \"because the user tells us so\" --- which would be the same as trusting\n> i18n.commitencoding from the beginning.  I dunno.\n\nMaybe if the option is called something like i18n.brokenIconv, people\nwould understand that it should not be enabled in normal\ncircumstances? It can also be used to tell GUI tools that git-log\ncannot be trusted to do the conversion properly -- perhaps being\nenabled system-wide by make install if NO_ICONV is set.\n\n> I actually have an alternative approach to suggest.\n>\n> How about adding a new i18n.commit-reencode-logmessage option (boolean),\n> and when it is set, we actually re-encode from i18n.commitencoding to\n> \"utf-8\" before creating the commit object (and obviously we do not store\n> \"encoding\" header in the resulting commit)?  When the conversion fails, we\n> know it failed, so the warning you added does make sense in that context.\n\nThis option changes the way data is actually processed before storing,\nso it probably cannot be turned on by default -- while the check is\nnearly useless _unless_ it is turned on by default.\n\nAlexander\n"}]}