{"thread":{"id":"47426","subject":"[PATCH v1] convert: add support for 'encoding' attribute","startedAt":"2017-12-11T15:50:57Z","lastAt":"2018-02-27T22:39:35Z","messageCount":36,"participants":["lars.schneider@autodesk.com","Eric Sunshine","Johannes Sixt","Lars Schneider","Junio C Hamano","Jeff King","Torsten Bögershausen","tboegi@web.de","Peter Krefting"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"334608","messageId":"20171211155023.1405-1-lars.schneider@autodesk.com","threadId":"47426","inReplyTo":null,"subject":"[PATCH v1] convert: add support for 'encoding' attribute","fromName":"","fromEmail":"lars.schneider@autodesk.com","sentAt":"2017-12-11T15:50:23Z","receivedAt":"2017-12-11T15:50:57Z","isPatch":true,"sender":{"key":"lars.schneider@autodesk.com","avatar":"https://gravatar.com/avatar/524188df3a38b02692619777bb90d9cc735db1ad75f5304e0bf09f8efd299775?d=mp&s=160"},"body":"From: Lars Schneider \n\nGit and its tools (e.g. git diff) expect all text files in UTF-8\nencoding. Git will happily accept content in all other encodings, too,\nbut it might not be able to process the text (e.g. viewing diffs or\nchanging line endings).\n\nAdd an attribute to tell Git what encoding the user has defined for a\ngiven file. If the content is added to the index, then Git converts the\ncontent to a canonical UTF-8 representation. On checkout Git will\nreverse the conversion.\n\nReviewed-by: Patrick Lühne \nSigned-off-by: Lars Schneider \n---\n\nHi,\n\nhere is a WIP patch to add text encoding support for files encoded with\nsomething other than UTF-8 [RFC].\n\nThe 'encoding' attribute is already used to view blobs in gitk. That\ncould be a problem as the content is stored in Git with the defined\nencoding. This patch would interpret the content as UTF-8 encoded and\nit would try to reencode it to the defined encoding on checkout.\nPlus, many repos define the attribute very broad (e.g. \"* encoding=cp1251\").\nThese folks would see errors like these with my patch:\n error: failed to encode 'foo.bar' from utf-8 to cp1251\n\nA quick search on GitHub reveals 2,722 repositories that use the\n'encoding' attribute [1]. Using the GitHub API [2], I identified the\nfollowing encodings in all publicly accessible repositories:\n\n ANSI <-- garbage (ignore by my implementation)\n cp1251\n cp866\n cp950\n iso8859-1\n koi8-r\n shiftjis <-- garbage (ignore by my implementation)\n UTF-8 <-- unnecessary (ignore by my implementation)\n utf8 <-- garbage (ignore by my implementation)\n\nTODOs:\n - The iconv docs mention that \"errno == EINVAL\" is harmless but\n we don't handle that case in utf8.c:reencode_string_iconv()\n - Git does not compile with NO_ICONV=1 right now because of\n compat/precompose_utf8.c. I will send a patch to fix that.\n\nQuestions:\n - Should I use warning() or error() in the patch?\n (currently I use the warning)\n - Do you agree with the approach in general?\n\nThanks,\nLars\n\n\n[RFC] http://public-inbox.org/git/BDB9B884-6D17-4BE3-A83C-F67E2AFA2B46@gmail.com\n [1] https://github.com/search?p=1&q=encoding+filename%3Agitattributes&type=Code&utf8=%E2%9C%93\n [2] curl --user larsxschneider:secret -H 'Accept: application/vnd.github.v3.text-match+json' 'https://api.github.com/search/code?q=encoding+in:file+filename:gitattributes' | jq -r '.items[].text_matches[].fragment' | sed 's/.*encoding=//' | sort | uniq\n [3] https://www.gnu.org/software/libc/manual/html_node/iconv-Examples.html#iconv-Examples\n\n\n\n\nNotes:\n Base Ref: master\n Web-Diff: https://github.com/larsxschneider/git/commit/afc9e88a4d\n Checkout: git fetch https://github.com/larsxschneider/git encoding-v1 && git checkout afc9e88a4d\n\n Documentation/gitattributes.txt | 27 ++++++\n convert.c | 192 +++++++++++++++++++++++++++++++++++++++-\n t/t0028-encoding.sh | 60 +++++++++++++\n 3 files changed, 278 insertions(+), 1 deletion(-)\n create mode 100755 t/t0028-encoding.sh\n\ndiff --git a/Documentation/gitattributes.txt b/Documentation/gitattributes.txt\nindex 30687de81a..84de2fa49c 100644\n--- a/Documentation/gitattributes.txt\n+++ b/Documentation/gitattributes.txt\n@@ -146,6 +146,33 @@ Unspecified::\n Any other value causes Git to act as if `text` has been left\n unspecified.\n\n+`encoding`\n+^^^^^^^^^^\n+\n+By default Git assumes UTF-8 encoding for text files. The `encoding`\n+attribute sets the encoding to be used in the working directory.\n+If the path is added to the index, then Git encodes the content to\n+UTF-8. On checkout the content is encoded back to the original\n+encoding. Consequently, you can use all built-in Git text processing\n+tools (e.g. git diff, line ending conversions, etc.) with your\n+non-UTF-8 encoded file.\n+\n+Please note that re-encoding content can cause errors and requires\n+resources. Use the `encoding` attribute only if you cannot store\n+a file in UTF-8 encoding and if you want Git to be able to process\n+the text.\n+\n+------------------------\n+*.txt\t\ttext encoding=UTF-16\n+------------------------\n+\n+All `iconv` encodings with a stable round-trip conversion to and from\n+UTF-8 are supported. You can see a full list with the following command:\n+\n+------------------------\n+iconv --list\n+------------------------\n+\n `eol`\n ^^^^^\n\ndiff --git a/convert.c b/convert.c\nindex 20d7ab67bd..ee19c17104 100644\n--- a/convert.c\n+++ b/convert.c\n@@ -7,6 +7,7 @@\n #include \"sigchain.h\"\n #include \"pkt-line.h\"\n #include \"sub-process.h\"\n+#include \"utf8.h\"\n\n /*\n * convert.c - convert a file when checking it out and checking it in.\n@@ -256,6 +257,149 @@ static int will_convert_lf_to_crlf(size_t len, struct text_stat *stats,\n\n }\n\n+#ifdef NO_ICONV\n+#ifndef _ICONV_T\n+/* The type is just a placeholder and not actually used. */\n+typedef void* iconv_t;\n+#endif\n+#endif\n+\n+static struct encoding {\n+\tconst char *name;\n+\ticonv_t to_git; /* conversion to Git's canonical encoding (UTF-8) */\n+\ticonv_t to_worktree; /* conversion to user-defined encoding */\n+\tstruct encoding *next;\n+} *encoding, **encoding_tail;\n+static const char *default_encoding = \"utf-8\";\n+static iconv_t invalid_conversion = (iconv_t)-1;\n+\n+static int encode_to_git(const char *path, const char *src, size_t src_len,\n+\t\t\t struct strbuf *buf, struct encoding *enc)\n+{\n+#ifndef NO_ICONV\n+\tchar *dst, *re_src;\n+\tint dst_len, re_src_len;\n+\n+\t/*\n+\t * No encoding is specified or there is nothing to encode.\n+\t * Tell the caller that the content was not modified.\n+\t */\n+\tif (!enc || (src && !src_len))\n+\t\treturn 0;\n+\n+\t/*\n+\t * Looks like we got called from \"would_convert_to_git()\".\n+\t * This means Git wants to know if it would encode (= modify!)\n+\t * the content. Let's answer with \"yes\", since an encoding was\n+\t * specified.\n+\t */\n+\tif (!buf && !src)\n+\t\treturn 1;\n+\n+\tif (enc->to_git == invalid_conversion) {\n+\t\tenc->to_git = iconv_open(default_encoding, encoding->name);\n+\t\tif (enc->to_git == invalid_conversion)\n+\t\t\twarning(_(\"unsupported encoding %s\"), encoding->name);\n+\t}\n+\n+\tif (enc->to_worktree == invalid_conversion)\n+\t\tenc->to_worktree = iconv_open(encoding->name, default_encoding);\n+\n+\t/*\n+\t * Do nothing if the encoding is not supported. This could happen in\n+\t * two cases:\n+\t * (1) The encoding is garbage. That is no problem as we would not\n+\t * encode the content to UTF-8 on \"add\" and we would not encode\n+\t * it back on \"checkout\".\n+\t * (2) Git users use different iconv versions that support different\n+\t * encodings. This could lead to problems, as the content might\n+\t * not be encoded on \"add\" but encoded back on \"checkout\" (or\n+\t * the other way around).\n+\t * We print a one-time warning to the user in both cases above.\n+\t */\n+\tif (enc->to_git == invalid_conversion || enc->to_worktree == invalid_conversion)\n+\t\treturn 0;\n+\n+\tdst = reencode_string_iconv(src, src_len, enc->to_git, &dst_len);\n+\tif (!dst)\n+\t\t/*\n+\t\t * We could add the blob \"as-is\" to Git. However, on checkout\n+\t\t * we would try to reencode to the original encoding. This\n+\t\t * would fail and we would leave the user with a messed-up\n+\t\t * working tree. Let's try to avoid this by screaming loud.\n+\t\t */\n+\t\tdie(_(\"failed to encode '%s' from %s to %s\"),\n+\t\t\tpath, enc->name, default_encoding);\n+\n+\t/*\n+\t * Encode dst back to ensure no information is lost. This wastes\n+\t * a few cycles as most conversions are round trip conversion\n+\t * safe. However, content that has an invalid encoding might not\n+\t * match its original byte sequence after the UTF-8 conversion\n+\t * round trip. Let's play safe here and check the round trip\n+\t * conversion.\n+\t */\n+\tre_src = reencode_string_iconv(dst, dst_len, enc->to_worktree, &re_src_len);\n+\tif (!re_src || strcmp(src, re_src)) {\n+\t\tdie(_(\"encoding '%s' from %s to %s and back is not the same\"),\n+\t\t\tpath, enc->name, default_encoding);\n+\t}\n+\tfree(re_src);\n+\n+\tstrbuf_attach(buf, dst, dst_len, dst_len + 1);\n+\treturn 1;\n+#else\n+\twarning(_(\"cannot encode '%s' from %s to %s because \"\n+\t\t\"your Git was not compiled with encoding support\"),\n+\t\tpath, enc->name, default_encoding);\n+\treturn 0;\n+#endif\n+}\n+\n+static int encode_to_worktree(const char *path, const char *src, size_t src_len,\n+\t\t\t struct strbuf *buf, struct encoding *enc)\n+{\n+#ifndef NO_ICONV\n+\tchar *dst;\n+\tint dst_len;\n+\n+\t/*\n+\t * No encoding is specified or there is nothing to encode.\n+\t * Tell the caller that the content was not modified.\n+\t */\n+\tif (!enc || (src && !src_len))\n+\t\treturn 0;\n+\n+\tif (enc->to_worktree == invalid_conversion) {\n+\t\tenc->to_worktree = iconv_open(encoding->name, default_encoding);\n+\t\tif (enc->to_worktree == invalid_conversion)\n+\t\t\twarning(\"unsupported encoding %s\", encoding->name);\n+\t}\n+\n+\t/*\n+\t * Do nothing if the encoding is not supported.\n+\t * See detailed explanation in encode_to_git().\n+\t */\n+\tif (enc->to_worktree == invalid_conversion)\n+\t\treturn 0;\n+\n+\tdst = reencode_string_iconv(src, src_len, enc->to_worktree, &dst_len);\n+\tif (!dst) {\n+\t\twarning(\"failed to encode '%s' from %s to %s\",\n+\t\t\tpath, default_encoding, encoding->name);\n+\t\treturn 0;\n+\t}\n+\n+\tstrbuf_attach(buf, dst, dst_len, dst_len + 1);\n+\treturn 1;\n+#else\n+\twarning(_(\"cannot encode '%s' from %s to %s because \"\n+\t\t\"your Git was not compiled with encoding support\"),\n+\t\tpath, default_encoding, encoding->name);\n+\treturn 0;\n+#endif\n+}\n+\n static int crlf_to_git(const struct index_state *istate,\n \t\t const char *path, const char *src, size_t len,\n \t\t struct strbuf *buf,\n@@ -969,6 +1113,32 @@ static int ident_to_worktree(const char *path, const char *src, size_t len,\n \treturn 1;\n }\n\n+static struct encoding *git_path_check_encoding(struct attr_check_item *check)\n+{\n+\tconst char *value = check->value;\n+\tstruct encoding *enc;\n+\n+\tif (ATTR_UNSET(value))\n+\t\treturn NULL;\n+\n+\tfor (enc = encoding; enc; enc = enc->next)\n+\t\tif (!strcmp(value, enc->name))\n+\t\t\treturn enc;\n+\n+\t/* Don't encode to the default encoding */\n+\tif (!strcasecmp(value, default_encoding))\n+\t\treturn NULL;\n+\n+\tenc = xcalloc(1, sizeof(struct convert_driver));\n+\tenc->name = xstrdup(value);\n+\tenc->to_git = invalid_conversion;\n+\tenc->to_worktree = invalid_conversion;\n+\t*encoding_tail = enc;\n+\tencoding_tail = &(enc->next);\n+\n+\treturn enc;\n+}\n+\n static enum crlf_action git_path_check_crlf(struct attr_check_item *check)\n {\n \tconst char *value = check->value;\n@@ -1024,6 +1194,7 @@ struct conv_attrs {\n \tenum crlf_action attr_action; /* What attr says */\n \tenum crlf_action crlf_action; /* When no attr is set, use core.autocrlf */\n \tint ident;\n+\tstruct encoding *encoding; /* Supported encoding or default encoding if NULL */\n };\n\n static void convert_attrs(struct conv_attrs *ca, const char *path)\n@@ -1032,8 +1203,9 @@ static void convert_attrs(struct conv_attrs *ca, const char *path)\n\n \tif (!check) {\n \t\tcheck = attr_check_initl(\"crlf\", \"ident\", \"filter\",\n-\t\t\t\t\t \"eol\", \"text\", NULL);\n+\t\t\t\t\t \"eol\", \"text\", \"encoding\", NULL);\n \t\tuser_convert_tail = &user_convert;\n+\t\tencoding_tail = &encoding;\n \t\tgit_config(read_convert_config, NULL);\n \t}\n\n@@ -1055,6 +1227,7 @@ static void convert_attrs(struct conv_attrs *ca, const char *path)\n \t\t\telse if (eol_attr == EOL_CRLF)\n \t\t\t\tca->crlf_action = CRLF_TEXT_CRLF;\n \t\t}\n+\t\tca->encoding = git_path_check_encoding(ccheck + 5);\n \t} else {\n \t\tca->drv = NULL;\n \t\tca->crlf_action = CRLF_UNDEFINED;\n@@ -1135,6 +1308,13 @@ int convert_to_git(const struct index_state *istate,\n \t\tsrc = dst->buf;\n \t\tlen = dst->len;\n \t}\n+\n+\tret |= encode_to_git(path, src, len, dst, ca.encoding);\n+\tif (ret && dst) {\n+\t\tsrc = dst->buf;\n+\t\tlen = dst->len;\n+\t}\n+\n \tif (checksafe != SAFE_CRLF_KEEP_CRLF) {\n \t\tret |= crlf_to_git(istate, path, src, len, dst, ca.crlf_action, checksafe);\n \t\tif (ret && dst) {\n@@ -1158,6 +1338,7 @@ void convert_to_git_filter_fd(const struct index_state *istate,\n \tif (!apply_filter(path, NULL, 0, fd, dst, ca.drv, CAP_CLEAN, NULL))\n \t\tdie(\"%s: clean filter '%s' failed\", path, ca.drv->name);\n\n+\tencode_to_git(path, dst->buf, dst->len, dst, ca.encoding);\n \tcrlf_to_git(istate, path, dst->buf, dst->len, dst, ca.crlf_action, checksafe);\n \tident_to_git(path, dst->buf, dst->len, dst, ca.ident);\n }\n@@ -1189,6 +1370,12 @@ static int convert_to_working_tree_internal(const char *path, const char *src,\n \t\t}\n \t}\n\n+\tret |= encode_to_worktree(path, src, len, dst, ca.encoding);\n+\tif (ret) {\n+\t\tsrc = dst->buf;\n+\t\tlen = dst->len;\n+\t}\n+\n \tret_filter = apply_filter(\n \t\tpath, src, len, -1, dst, ca.drv, CAP_SMUDGE, dco);\n \tif (!ret_filter && ca.drv && ca.drv->required)\n@@ -1655,6 +1842,9 @@ struct stream_filter *get_stream_filter(const char *path, const unsigned char *s\n \tif (ca.drv && (ca.drv->process || ca.drv->smudge || ca.drv->clean))\n \t\treturn NULL;\n\n+\tif (ca.encoding)\n+\t\treturn NULL;\n+\n \tif (ca.crlf_action == CRLF_AUTO || ca.crlf_action == CRLF_AUTO_CRLF)\n \t\treturn NULL;\n\ndiff --git a/t/t0028-encoding.sh b/t/t0028-encoding.sh\nnew file mode 100755\nindex 0000000000..24f24f9e0d\n--- /dev/null\n+++ b/t/t0028-encoding.sh\n@@ -0,0 +1,60 @@\n+#!/bin/sh\n+\n+test_description='encoding conversion via gitattributes'\n+\n+. ./test-lib.sh\n+\n+test_expect_success 'setup test repo' '\n+\n+\ttext=\"hallo there!\\ncan you read me?\" &&\n+\n+\techo \"*.utf16 text encoding=utf-16\" >.gitattributes &&\n+\tprintf \"$text\" >t.utf8.raw &&\n+\tprintf \"$text\" | iconv -f UTF-8 -t UTF-16 >t.utf16.raw &&\n+\tcp t.utf16.raw t.utf16 &&\n+\n+\tgit add .gitattributes t.utf16.raw t.utf16 &&\n+\tgit commit -m initial\n+'\n+\n+test_expect_success 'ensure UTF-8 is stored in Git' '\n+\tgit cat-file -p :t.utf16 >t.utf16.git &&\n+\ttest_cmp_bin t.utf8.raw t.utf16.git\n+'\n+\n+test_expect_success 're-encode to UTF-16 on checkout' '\n+\trm t.utf16 &&\n+\tgit checkout t.utf16 &&\n+\ttest_cmp_bin t.utf16.raw t.utf16\n+'\n+\n+test_expect_success 'warn if an unsupported encoding is used' '\n+\techo \"*.garbage text encoding=garbage\" >>.gitattributes &&\n+\tprintf \"garbage\" >t.garbage &&\n+\tgit add t.garbage 2>error.out &&\n+\ttest_i18ngrep \"warning: unsupported encoding\" error.out &&\n+\n+\t# cleanup\n+\tgit reset --hard HEAD\n+'\n+\n+test_expect_success 'fail if files with invalid encoding are added' '\n+\tprintf \"\\0\\0h\\0a\" >error.utf16 &&\n+\t# The test string encoding would fail\n+\t# test_must_fail iconv -f utf-16 -t utf-8 error.utf16 &&\n+\ttest_must_fail git add error.utf16\n+'\n+\n+# Some sequences might trigger errno == E2BIG in reencode_string_iconv, utf.8.\n+# This would cause no error on \"git add\" and, consequently, the Git internal\n+# UTF-8 encoded blob would contain garbage. Hence, the worktree file after a\n+# checkout would contain garbage, too. This garbage would not match the file\n+# that was initially added.\n+test_expect_success 'fail if encoding from X to UTF-8 and back to X is not the same' '\n+\tprintf \"\\xc3\\x28\" >error.utf16 &&\n+\t# The test string re-encoding would fail\n+\t# iconv -f utf-16 -t utf-8 error.utf16 | iconv -f utf-8 -t utf-16 &&\n+\ttest_must_fail git add error.utf16\n+'\n+\n+test_done\n\nbase-commit: 95ec6b1b3393eb6e26da40c565520a8db9796e9f\n--\n2.15.1\n\n"},{"id":"334616","messageId":"CAPig+cQ6VSXXSYJOiZeTqUpwijVhvvUYzXF8U3KCBsOQ91HPZQ@mail.gmail.com","threadId":"47426","inReplyTo":"20171211155023.1405-1-lars.schneider@autodesk.com","subject":"Re: [PATCH v1] convert: add support for 'encoding' attribute","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2017-12-11T18:39:27Z","receivedAt":"2017-12-11T18:39:36Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Mon, Dec 11, 2017 at 10:50 AM, wrote:\n> From: Lars Schneider \n>\n> Git and its tools (e.g. git diff) expect all text files in UTF-8\n> encoding. Git will happily accept content in all other encodings, too,\n> but it might not be able to process the text (e.g. viewing diffs or\n> changing line endings).\n>\n> Add an attribute to tell Git what encoding the user has defined for a\n> given file. If the content is added to the index, then Git converts the\n> content to a canonical UTF-8 representation. On checkout Git will\n> reverse the conversion.\n>\n> Reviewed-by: Patrick Lühne \n> Signed-off-by: Lars Schneider \n> ---\n> diff --git a/convert.c b/convert.c\n> @@ -256,6 +257,149 @@ static int will_convert_lf_to_crlf(size_t len, struct text_stat *stats,\n> +static int encode_to_git(const char *path, const char *src, size_t src_len,\n> + struct strbuf *buf, struct encoding *enc)\n> +{\n> +#ifndef NO_ICONV\n> + char *dst, *re_src;\n> + int dst_len, re_src_len;\n> +\n> + /*\n> + * No encoding is specified or there is nothing to encode.\n> + * Tell the caller that the content was not modified.\n> + */\n> + if (!enc || (src && !src_len))\n> + return 0;\n> +\n> + /*\n> + * Looks like we got called from \"would_convert_to_git()\".\n> + * This means Git wants to know if it would encode (= modify!)\n> + * the content. Let's answer with \"yes\", since an encoding was\n> + * specified.\n> + */\n> + if (!buf && !src)\n> + return 1;\n> +\n> + if (enc->to_git == invalid_conversion) {\n> + enc->to_git = iconv_open(default_encoding, encoding->name);\n> + if (enc->to_git == invalid_conversion)\n> + warning(_(\"unsupported encoding %s\"), encoding->name);\n> + }\n> +\n> + if (enc->to_worktree == invalid_conversion)\n> + enc->to_worktree = iconv_open(encoding->name, default_encoding);\n\nDo you need to be calling iconv_close() somewhere on the result of the\niconv_open() calls? [Answering myself after reading the rest of the\npatch: You're caching these opened 'iconv' descriptors, so you don't\nplan on closing them.]\n\n> + [...]\n> + /*\n> + * Encode dst back to ensure no information is lost. This wastes\n> + * a few cycles as most conversions are round trip conversion\n> + * safe. However, content that has an invalid encoding might not\n> + * match its original byte sequence after the UTF-8 conversion\n> + * round trip. Let's play safe here and check the round trip\n> + * conversion.\n> + */\n> + re_src = reencode_string_iconv(dst, dst_len, enc->to_worktree, &re_src_len);\n> + if (!re_src || strcmp(src, re_src)) {\n\nYou're using strcmp() as opposed to memcmp() because you expect\n're_src' will unconditionally be UTF-8-encoded, right?\n\n> + die(_(\"encoding '%s' from %s to %s and back is not the same\"),\n> + path, enc->name, default_encoding);\n> + }\n> + free(re_src);\n> +\n> + strbuf_attach(buf, dst, dst_len, dst_len + 1);\n> + return 1;\n> +#else\n> + warning(_(\"cannot encode '%s' from %s to %s because \"\n> + \"your Git was not compiled with encoding support\"),\n> + path, enc->name, default_encoding);\n> + return 0;\n> +#endif\n> +}\n"},{"id":"334630","messageId":"a07a0b5c-453a-f644-d5e5-7d185a943088@kdbg.org","threadId":"47426","inReplyTo":"20171211155023.1405-1-lars.schneider@autodesk.com","subject":"Re: [PATCH v1] convert: add support for 'encoding' attribute","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2017-12-11T20:47:24Z","receivedAt":"2017-12-11T20:47:32Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 11.12.2017 um 16:50 schrieb lars.schneider@autodesk.com:\n> From: Lars Schneider \n> \n> Git and its tools (e.g. git diff) expect all text files in UTF-8\n> encoding. Git will happily accept content in all other encodings, too,\n> but it might not be able to process the text (e.g. viewing diffs or\n> changing line endings).\n> \n> Add an attribute to tell Git what encoding the user has defined for a\n> given file. If the content is added to the index, then Git converts the\n> content to a canonical UTF-8 representation. On checkout Git will\n> reverse the conversion.\n> \n> Reviewed-by: Patrick Lühne \n> Signed-off-by: Lars Schneider \n> ---\n> \n> Hi,\n> \n> here is a WIP patch to add text encoding support for files encoded with\n> something other than UTF-8 [RFC].\n> \n> The 'encoding' attribute is already used to view blobs in gitk. That\n> could be a problem as the content is stored in Git with the defined\n> encoding. This patch would interpret the content as UTF-8 encoded and\n\nThis will be a major drawback for me because my code base stores text \nfiles that are not UTF-8 encoded. And I do use the existing 'encoding' \nattribute to view the text in git-gui and gitk. Repurposing this \nattribute name is not an option, IMO.\n\n> it would try to reencode it to the defined encoding on checkout > Plus, many repos define the attribute very broad (e.g. \"* \nencoding=cp1251\").\n> These folks would see errors like these with my patch:\n> error: failed to encode 'foo.bar' from utf-8 to cp1251\n\n-- Hannes\n"},{"id":"334648","messageId":"0BA27EC9-3836-407A-9A8A-52D06B8052AE@gmail.com","threadId":"47426","inReplyTo":"a07a0b5c-453a-f644-d5e5-7d185a943088@kdbg.org","subject":"Re: [PATCH v1] convert: add support for 'encoding' attribute","fromName":"Lars Schneider","fromEmail":"larsxschneider@gmail.com","sentAt":"2017-12-11T23:42:23Z","receivedAt":"2017-12-11T23:42:18Z","isPatch":true,"sender":{"key":"larsxschneider@gmail.com","avatar":"https://avatars.githubusercontent.com/u/477434?v=4"},"body":"\nOn 11 Dec 2017, at 21:47, Johannes Sixt wrote:\n\n> Am 11.12.2017 um 16:50 schrieb lars.schneider@autodesk.com:\n>> From: Lars Schneider \n>> Git and its tools (e.g. git diff) expect all text files in UTF-8\n>> encoding. Git will happily accept content in all other encodings, too,\n>> but it might not be able to process the text (e.g. viewing diffs or\n>> changing line endings).\n>> Add an attribute to tell Git what encoding the user has defined for a\n>> given file. If the content is added to the index, then Git converts the\n>> content to a canonical UTF-8 representation. On checkout Git will\n>> reverse the conversion.\n>> Reviewed-by: Patrick Lühne \n>> Signed-off-by: Lars Schneider \n>> ---\n>> Hi,\n>> here is a WIP patch to add text encoding support for files encoded with\n>> something other than UTF-8 [RFC].\n>> The 'encoding' attribute is already used to view blobs in gitk. That\n>> could be a problem as the content is stored in Git with the defined\n>> encoding. This patch would interpret the content as UTF-8 encoded and\n> \n> This will be a major drawback for me because my code base stores text files that are not UTF-8 encoded. And I do use the existing 'encoding' attribute to view the text in git-gui and gitk. Repurposing this attribute name is not an option, IMO.\n\nI understand your point of view and I kind of expected that that reply.\nThanks for the feedback!\n\nQuestion is: Given that \"encoding\" is not available, how could I name\n the attribute without confusing the user?\n\nI contemplated:\n - \"enc\" or \"encode\" because \"eol\" and \"ident\" use abbreviations, too\n (enc could be confused with encryption. plus, a user might ask\n what is the difference between \"enc\" and \"encoding\" attribute :-)\n - \"wte\", \"wtenc\", or \"worktree-encoding\" to emphasize that this is \n the encoding used in the worktree \n (I fear that users think that is git-worktree, the command, related)\n\nI think my favorite is \"worktree-encoding\".\nWhat do you think?\n\nThanks,\nLars \n\n\nBTW: I am curios, can you share what encoding you use?\nMy main use case is UTF-16 and I was surprised that I haven't\nfound a single public repo on github.com with \"encoding=utf-16\"\n\n"},{"id":"334650","messageId":"09713FC5-9D71-4B39-BFA3-0CB9C63321A2@gmail.com","threadId":"47426","inReplyTo":"CAPig+cQ6VSXXSYJOiZeTqUpwijVhvvUYzXF8U3KCBsOQ91HPZQ@mail.gmail.com","subject":"Re: [PATCH v1] convert: add support for 'encoding' attribute","fromName":"Lars Schneider","fromEmail":"larsxschneider@gmail.com","sentAt":"2017-12-11T23:47:59Z","receivedAt":"2017-12-11T23:47:58Z","isPatch":true,"sender":{"key":"larsxschneider@gmail.com","avatar":"https://avatars.githubusercontent.com/u/477434?v=4"},"body":"\nOn 11 Dec 2017, at 19:39, Eric Sunshine wrote:\n\n> On Mon, Dec 11, 2017 at 10:50 AM, wrote:\n>> From: Lars Schneider \n>> \n>> Git and its tools (e.g. git diff) expect all text files in UTF-8\n>> encoding. Git will happily accept content in all other encodings, too,\n>> but it might not be able to process the text (e.g. viewing diffs or\n>> changing line endings).\n>> \n>> Add an attribute to tell Git what encoding the user has defined for a\n>> given file. If the content is added to the index, then Git converts the\n>> content to a canonical UTF-8 representation. On checkout Git will\n>> reverse the conversion.\n>> \n>> Reviewed-by: Patrick Lühne \n>> Signed-off-by: Lars Schneider \n>> ---\n>> diff --git a/convert.c b/convert.c\n>> @@ -256,6 +257,149 @@ static int will_convert_lf_to_crlf(size_t len, struct text_stat *stats,\n>> +static int encode_to_git(const char *path, const char *src, size_t src_len,\n>> + struct strbuf *buf, struct encoding *enc)\n>> +{\n>> +#ifndef NO_ICONV\n>> + char *dst, *re_src;\n>> + int dst_len, re_src_len;\n>> +\n>> + /*\n>> + * No encoding is specified or there is nothing to encode.\n>> + * Tell the caller that the content was not modified.\n>> + */\n>> + if (!enc || (src && !src_len))\n>> + return 0;\n>> +\n>> + /*\n>> + * Looks like we got called from \"would_convert_to_git()\".\n>> + * This means Git wants to know if it would encode (= modify!)\n>> + * the content. Let's answer with \"yes\", since an encoding was\n>> + * specified.\n>> + */\n>> + if (!buf && !src)\n>> + return 1;\n>> +\n>> + if (enc->to_git == invalid_conversion) {\n>> + enc->to_git = iconv_open(default_encoding, encoding->name);\n>> + if (enc->to_git == invalid_conversion)\n>> + warning(_(\"unsupported encoding %s\"), encoding->name);\n>> + }\n>> +\n>> + if (enc->to_worktree == invalid_conversion)\n>> + enc->to_worktree = iconv_open(encoding->name, default_encoding);\n> \n> Do you need to be calling iconv_close() somewhere on the result of the\n> iconv_open() calls? [Answering myself after reading the rest of the\n> patch: You're caching these opened 'iconv' descriptors, so you don't\n> plan on closing them.]\n\nShould this information go into the commit message to avoid confusing\nfuture readers? I think, yes.\n\n\n>> + [...]\n>> + /*\n>> + * Encode dst back to ensure no information is lost. This wastes\n>> + * a few cycles as most conversions are round trip conversion\n>> + * safe. However, content that has an invalid encoding might not\n>> + * match its original byte sequence after the UTF-8 conversion\n>> + * round trip. Let's play safe here and check the round trip\n>> + * conversion.\n>> + */\n>> + re_src = reencode_string_iconv(dst, dst_len, enc->to_worktree, &re_src_len);\n>> + if (!re_src || strcmp(src, re_src)) {\n> \n> You're using strcmp() as opposed to memcmp() because you expect\n> 're_src' will unconditionally be UTF-8-encoded, right?\n\nYes. But since you mention it, I think it would be better to use\nmemcmp() here! Plus, it might be a tiny bit faster ;-)\n\nThanks,\nLars"},{"id":"334651","messageId":"CAPig+cRtsCeWZZcj-jRLsqwb5OSHLBQFyGCvpAXeHRmcbTtT+w@mail.gmail.com","threadId":"47426","inReplyTo":"09713FC5-9D71-4B39-BFA3-0CB9C63321A2@gmail.com","subject":"Re: [PATCH v1] convert: add support for 'encoding' attribute","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2017-12-11T23:58:49Z","receivedAt":"2017-12-11T23:58:55Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Mon, Dec 11, 2017 at 6:47 PM, Lars Schneider\n wrote:\n> On 11 Dec 2017, at 19:39, Eric Sunshine wrote:\n>> On Mon, Dec 11, 2017 at 10:50 AM, wrote:\n>>> From: Lars Schneider \n>>>\n>>> Git and its tools (e.g. git diff) expect all text files in UTF-8\n>>> encoding. Git will happily accept content in all other encodings, too,\n>>> but it might not be able to process the text (e.g. viewing diffs or\n>>> changing line endings).\n>>>\n>>> Add an attribute to tell Git what encoding the user has defined for a\n>>> given file. If the content is added to the index, then Git converts the\n>>> content to a canonical UTF-8 representation. On checkout Git will\n>>> reverse the conversion.\n>>>\n>>> Reviewed-by: Patrick Lühne \n>>> Signed-off-by: Lars Schneider \n>>> ---\n>>> +static int encode_to_git(const char *path, const char *src, size_t src_len,\n>>> + struct strbuf *buf, struct encoding *enc)\n>>> +{\n>>> + if (enc->to_git == invalid_conversion) {\n>>> + enc->to_git = iconv_open(default_encoding, encoding->name);\n>>> + if (enc->to_git == invalid_conversion)\n>>> + warning(_(\"unsupported encoding %s\"), encoding->name);\n>>> + }\n>>> +\n>>> + if (enc->to_worktree == invalid_conversion)\n>>> + enc->to_worktree = iconv_open(encoding->name, default_encoding);\n>>\n>> Do you need to be calling iconv_close() somewhere on the result of the\n>> iconv_open() calls? [Answering myself after reading the rest of the\n>> patch: You're caching these opened 'iconv' descriptors, so you don't\n>> plan on closing them.]\n>\n> Should this information go into the commit message to avoid confusing\n> future readers? I think, yes.\n\nMaybe. However, the code which does the actual caching is so distant\nfrom these iconv_open() invocations that it might be more helpful to\nhave an in-code comment here saying that the \"missing\" iconv_close()\ninvocations is intentional.\n"},{"id":"334659","messageId":"xmqqzi6o3go9.fsf@gitster.mtv.corp.google.com","threadId":"47426","inReplyTo":"0BA27EC9-3836-407A-9A8A-52D06B8052AE@gmail.com","subject":"Re: [PATCH v1] convert: add support for 'encoding' attribute","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-12-12T00:59:02Z","receivedAt":"2017-12-12T00:59:15Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Lars Schneider writes:\n\n> I contemplated:\n> - \"enc\" or \"encode\" because \"eol\" and \"ident\" use abbreviations, too\n> (enc could be confused with encryption. plus, a user might ask\n> what is the difference between \"enc\" and \"encoding\" attribute :-)\n> - \"wte\", \"wtenc\", or \"worktree-encoding\" to emphasize that this is \n> the encoding used in the worktree \n> (I fear that users think that is git-worktree, the command, related)\n\nIn the context of Git, the word \"worktree\" does have a specific\nmeaning that is different from working tree. \n\nStepping back a bit, what does this thing do you are introducing?\nAnd what does the other thing do that J6t is using, that would get\nconfused with this new one?\n\nWhat does the other one do? \"Declare that the contents of this path\nis in this encoding\"? As opposed to the new one, which tells Git to\n\"run iconv from and to this encoding when checking out and checking\nin\"?\n\nIf so, any phrase that depends heavily on the word \"encode\" would\nnot help differenciating the two uses. The phrase needs to be\nsomething that contrasts the new one, which actively modifies things\n(what is on the filesystem is not what is stored in the object\nstore), with the old one, which does not (passed as a declaration to\na viewer what encoding the contents already use and does not change\nanything).\n\nDo people who will use this feature familiar with the concept of\nsmudge/clean? If you want to avoid \"working-tree\" (or \"worktree\",\nwhich definitely you would want to avoid) because you fear confused\nusers, perhaps \"smudge-encoding\" would work (we declare that the\nresult of smudge operations are left in this encoding, so the\nopposite operation \"clean\" will do the reverse---and we say this\nwithout explicitly saying that the other end of the conversion is\nalways UTF-8)? Or \"checkout-encoding\" (the same explanation; we do\nnot say the opposite operation \"checkin/add\" will do the reverse).\n\nI personally do not think \"working-tree-encoding\" is too horrible,\nbut I do agree that some users may be confused. So I dunno.\n\n\n\n"},{"id":"334662","messageId":"01dfe614-e707-30f3-6347-bb9bd2e5541c@kdbg.org","threadId":"47426","inReplyTo":"0BA27EC9-3836-407A-9A8A-52D06B8052AE@gmail.com","subject":"Re: [PATCH v1] convert: add support for 'encoding' attribute","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2017-12-12T07:09:03Z","receivedAt":"2017-12-12T07:09:11Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 12.12.2017 um 00:42 schrieb Lars Schneider:\n> BTW: I am curios, can you share what encoding you use?\n> My main use case is UTF-16 and I was surprised that I haven't\n> found a single public repo on github.com with \"encoding=utf-16\"\n\nShift-JIS and CP1252. These are used for Windows resource files (*.rc).\n\n-- Hannes\n"},{"id":"334663","messageId":"48ebcb7a-2a7f-e790-2613-a1f3e3dc41f9@kdbg.org","threadId":"47426","inReplyTo":"xmqqzi6o3go9.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v1] convert: add support for 'encoding' attribute","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2017-12-12T07:15:08Z","receivedAt":"2017-12-12T07:15:15Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 12.12.2017 um 01:59 schrieb Junio C Hamano:\n> Stepping back a bit, what does this thing do you are introducing?\n> And what does the other thing do that J6t is using, that would get\n> confused with this new one?\n> \n> What does the other one do? \"Declare that the contents of this path\n> is in this encoding\"? As opposed to the new one, which tells Git to\n> \"run iconv from and to this encoding when checking out and checking\n> in\"?\n> \n> If so, any phrase that depends heavily on the word \"encode\" would\n> not help differenciating the two uses. The phrase needs to be\n> something that contrasts the new one, which actively modifies things\n> (what is on the filesystem is not what is stored in the object\n> store), with the old one, which does not (passed as a declaration to\n> a viewer what encoding the contents already use and does not change\n> anything).\n\nWell explained!\n\n> ... perhaps \"smudge-encoding\" would work (we declare that the\n> result of smudge operations are left in this encoding, so the\n> opposite operation \"clean\" will do the reverse---and we say this\n> without explicitly saying that the other end of the conversion is\n> always UTF-8)? Or \"checkout-encoding\" (the same explanation; we do\n> not say the opposite operation \"checkin/add\" will do the reverse).\n\nI would favor \"checkout-encoding\" over \"smudge-encoding\" only because \n\"checkout\" is better known than \"smudge\", I would think. I do not have \nbetter suggestions.\n\n-- Hannes\n"},{"id":"334672","messageId":"F571AAED-C325-4151-9566-6BA249C1B3D4@gmail.com","threadId":"47426","inReplyTo":"48ebcb7a-2a7f-e790-2613-a1f3e3dc41f9@kdbg.org","subject":"Re: [PATCH v1] convert: add support for 'encoding' attribute","fromName":"Lars Schneider","fromEmail":"larsxschneider@gmail.com","sentAt":"2017-12-12T10:55:53Z","receivedAt":"2017-12-12T10:56:01Z","isPatch":true,"sender":{"key":"larsxschneider@gmail.com","avatar":"https://avatars.githubusercontent.com/u/477434?v=4"},"body":"\n> On 12 Dec 2017, at 08:15, Johannes Sixt wrote:\n> \n> Am 12.12.2017 um 01:59 schrieb Junio C Hamano:\n>> Stepping back a bit, what does this thing do you are introducing?\n>> And what does the other thing do that J6t is using, that would get\n>> confused with this new one?\n>> What does the other one do? \"Declare that the contents of this path\n>> is in this encoding\"? As opposed to the new one, which tells Git to\n>> \"run iconv from and to this encoding when checking out and checking\n>> in\"?\n>> If so, any phrase that depends heavily on the word \"encode\" would\n>> not help differenciating the two uses. The phrase needs to be\n>> something that contrasts the new one, which actively modifies things\n>> (what is on the filesystem is not what is stored in the object\n>> store), with the old one, which does not (passed as a declaration to\n>> a viewer what encoding the contents already use and does not change\n>> anything).\n> \n> Well explained!\n> \n>> ... perhaps \"smudge-encoding\" would work (we declare that the\n>> result of smudge operations are left in this encoding, so the\n>> opposite operation \"clean\" will do the reverse---and we say this\n>> without explicitly saying that the other end of the conversion is\n>> always UTF-8)? Or \"checkout-encoding\" (the same explanation; we do\n>> not say the opposite operation \"checkin/add\" will do the reverse).\n> \n> I would favor \"checkout-encoding\" over \"smudge-encoding\" only because \"checkout\" is better known than \"smudge\", I would think. I do not have better suggestions.\n\nThanks for your thoughts! \"checkout-encoding\" would work for me.\nHowever, it might convey that Git \"does change the files of the\nuser in some way\" (which is true from Git's perspective but not\nfrom the user perspective).\n\nPatrick and I brainstormed a few more possible alternatives:\n\napply-encoding\nblob-encoding\ncheckout-encoding\ndiff-encoding\nencoding-hint\nexternal-encoding\nhandle-as\ninternal-encoding\nkeep-encoding\npresent-as\npreserve-encoding\nsource-encoding\ntext-conversion\ntext-encoding\ntreat-as \ntreat-encoding-as\n\nOur favorite is \"treat-encoding-as\". Do you consider this better\nor worse than \"checkout-encoding\"?\n\nThanks,\nLars\n\n\nPS: Naming things is hard ;-)"},{"id":"334673","messageId":"3FB29434-D2DE-4215-AD33-69D6BB7DF30A@gmail.com","threadId":"47426","inReplyTo":"CAPig+cRtsCeWZZcj-jRLsqwb5OSHLBQFyGCvpAXeHRmcbTtT+w@mail.gmail.com","subject":"Re: [PATCH v1] convert: add support for 'encoding' attribute","fromName":"Lars Schneider","fromEmail":"larsxschneider@gmail.com","sentAt":"2017-12-12T10:58:55Z","receivedAt":"2017-12-12T10:59:03Z","isPatch":true,"sender":{"key":"larsxschneider@gmail.com","avatar":"https://avatars.githubusercontent.com/u/477434?v=4"},"body":"\n> On 12 Dec 2017, at 00:58, Eric Sunshine wrote:\n> \n> On Mon, Dec 11, 2017 at 6:47 PM, Lars Schneider\n> wrote:\n>> On 11 Dec 2017, at 19:39, Eric Sunshine wrote:\n>>> On Mon, Dec 11, 2017 at 10:50 AM, wrote:\n>>>> From: Lars Schneider \n>>>> \n>>>> Git and its tools (e.g. git diff) expect all text files in UTF-8\n>>>> encoding. Git will happily accept content in all other encodings, too,\n>>>> but it might not be able to process the text (e.g. viewing diffs or\n>>>> changing line endings).\n>>>> \n>>>> Add an attribute to tell Git what encoding the user has defined for a\n>>>> given file. If the content is added to the index, then Git converts the\n>>>> content to a canonical UTF-8 representation. On checkout Git will\n>>>> reverse the conversion.\n>>>> \n>>>> Reviewed-by: Patrick Lühne \n>>>> Signed-off-by: Lars Schneider \n>>>> ---\n>>>> +static int encode_to_git(const char *path, const char *src, size_t src_len,\n>>>> + struct strbuf *buf, struct encoding *enc)\n>>>> +{\n>>>> + if (enc->to_git == invalid_conversion) {\n>>>> + enc->to_git = iconv_open(default_encoding, encoding->name);\n>>>> + if (enc->to_git == invalid_conversion)\n>>>> + warning(_(\"unsupported encoding %s\"), encoding->name);\n>>>> + }\n>>>> +\n>>>> + if (enc->to_worktree == invalid_conversion)\n>>>> + enc->to_worktree = iconv_open(encoding->name, default_encoding);\n>>> \n>>> Do you need to be calling iconv_close() somewhere on the result of the\n>>> iconv_open() calls? [Answering myself after reading the rest of the\n>>> patch: You're caching these opened 'iconv' descriptors, so you don't\n>>> plan on closing them.]\n>> \n>> Should this information go into the commit message to avoid confusing\n>> future readers? I think, yes.\n> \n> Maybe. However, the code which does the actual caching is so distant\n> from these iconv_open() invocations that it might be more helpful to\n> have an in-code comment here saying that the \"missing\" iconv_close()\n> invocations is intentional.\n\nAgreed. I will add that in v2.\n\nThanks,\nLars\n\n"},{"id":"334694","messageId":"xmqq1sjz3frf.fsf@gitster.mtv.corp.google.com","threadId":"47426","inReplyTo":"F571AAED-C325-4151-9566-6BA249C1B3D4@gmail.com","subject":"Re: [PATCH v1] convert: add support for 'encoding' attribute","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-12-12T19:31:00Z","receivedAt":"2017-12-12T19:31:09Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Lars Schneider writes:\n\n> Our favorite is \"treat-encoding-as\". Do you consider this better\n> or worse than \"checkout-encoding\"?\n\nI am afraid that \"treat as\" is not sufficiently specific and would\ninvite a misinterpretation, e.g. \"You record the bytes I throw at\nyou as-is in the object store, but treat them appropriately as\ncontents that are encoded in cp1252 when presenting\".\n\nwhat is missing is at what stage in the overall user experience does\nthat \"treating\" happens. That causes such a misinterpretation.\n\nSo from that point of view, \"checkout-\" or\" working-tree-\" would be\na better phrase to accompany \"encoding\" to clarify what this attr is\nfor than \"treat-as\".\n\nHaving said all that, this \"feature\" would need a moderate amount of\nclear description in the documentation, and between the perfect and\na suboptimal name, the amount of explanation required would not be\nall that different, I suspect. We need to say that those who wish\nto use this attribute are buying into recording their contents in\nUTF-8 and when the contents are externalized to the working tree,\nthey are converted to the encoding the value of this attribute\nspecifies and vice versa.\n\n"},{"id":"334766","messageId":"D2973852-BDF4-4251-8DE3-CE6AAD303A5F@gmail.com","threadId":"47426","inReplyTo":"xmqq1sjz3frf.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v1] convert: add support for 'encoding' attribute","fromName":"Lars Schneider","fromEmail":"larsxschneider@gmail.com","sentAt":"2017-12-13T17:57:55Z","receivedAt":"2017-12-13T17:58:03Z","isPatch":true,"sender":{"key":"larsxschneider@gmail.com","avatar":"https://avatars.githubusercontent.com/u/477434?v=4"},"body":"\n> On 12 Dec 2017, at 20:31, Junio C Hamano wrote:\n> \n> Lars Schneider writes:\n> \n>> Our favorite is \"treat-encoding-as\". Do you consider this better\n>> or worse than \"checkout-encoding\"?\n> \n> I am afraid that \"treat as\" is not sufficiently specific and would\n> invite a misinterpretation, e.g. \"You record the bytes I throw at\n> you as-is in the object store, but treat them appropriately as\n> contents that are encoded in cp1252 when presenting\".\n> \n> what is missing is at what stage in the overall user experience does\n> that \"treating\" happens. That causes such a misinterpretation.\n> \n> So from that point of view, \"checkout-\" or\" working-tree-\" would be\n> a better phrase to accompany \"encoding\" to clarify what this attr is\n> for than \"treat-as\".\n\nAlright. I'll go with \"checkout-encoding\" then.\n\n\n> Having said all that, this \"feature\" would need a moderate amount of\n> clear description in the documentation, and between the perfect and\n> a suboptimal name, the amount of explanation required would not be\n> all that different, I suspect. We need to say that those who wish\n> to use this attribute are buying into recording their contents in\n> UTF-8 and when the contents are externalized to the working tree,\n> they are converted to the encoding the value of this attribute\n> specifies and vice versa.\n\nAbsolutely! The docs will come in v2. I just wanted to send out a v1\nto make people aware of what I am working on.\n\nI think storing the encoding in gitattributes might not be the perfect\nsolution but probably a pragmatic one (because it works and the changes\nto Git are rather small). In a perfect world I think I would store\nthe encoding of a file in the tree object. I didn't pursue that solution\nas this would change the Git data model which would open a can of worms\nfor a problem that not that many people have (almost everyone is on\nUTF-8 anyways).\n\n- Lars\n"},{"id":"334767","messageId":"xmqqzi6mwlai.fsf@gitster.mtv.corp.google.com","threadId":"47426","inReplyTo":"D2973852-BDF4-4251-8DE3-CE6AAD303A5F@gmail.com","subject":"Re: [PATCH v1] convert: add support for 'encoding' attribute","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-12-13T18:11:01Z","receivedAt":"2017-12-13T18:11:14Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Lars Schneider writes:\n\n> ... In a perfect world I think I would store\n> the encoding of a file in the tree object. I didn't pursue that solution\n> as this would change the Git data model which would open a can of worms\n> for a problem that not that many people have (almost everyone is on\n> UTF-8 anyways).\n\nHaving that \"encoding\" trailt recorded in the tree that contains the\nblob would mean that the same blob can be recorded with one\n\"encoding\" trait in a tree, and in a different tree it can be\nrecorded with a different \"encoding\" trait. I doubt it really makes\nsense.\n\n"},{"id":"334783","messageId":"C062CB10-9885-46B4-9614-65A39BD5FBE8@gmail.com","threadId":"47426","inReplyTo":"xmqqzi6mwlai.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v1] convert: add support for 'encoding' attribute","fromName":"Lars Schneider","fromEmail":"larsxschneider@gmail.com","sentAt":"2017-12-13T23:02:32Z","receivedAt":"2017-12-13T23:02:41Z","isPatch":true,"sender":{"key":"larsxschneider@gmail.com","avatar":"https://avatars.githubusercontent.com/u/477434?v=4"},"body":"\n> On 13 Dec 2017, at 19:11, Junio C Hamano wrote:\n> \n> Lars Schneider writes:\n> \n>> ... In a perfect world I think I would store\n>> the encoding of a file in the tree object. I didn't pursue that solution\n>> as this would change the Git data model which would open a can of worms\n>> for a problem that not that many people have (almost everyone is on\n>> UTF-8 anyways).\n> \n> Having that \"encoding\" trailt recorded in the tree that contains the\n> blob would mean that the same blob can be recorded with one\n> \"encoding\" trait in a tree, and in a different tree it can be\n> recorded with a different \"encoding\" trait. I doubt it really makes\n> sense.\n\nThe \"blob object\" would store the text data encoded in a canonical \nUTF-8 form. The \"tree object\" would store the encoding. On checkin \nGit would convert the text from the stored encoding to UTF-8 and on \ncheckout it would do the reverse.\n\nThat way you could control the encoding for a text file specific\nfor each path similar to the \"mode bits\". That also means you could\nchange the encoding of a file while the blob content stays the same.\n\nChanging the encoding of a file with the .gitattributes approach\ncan be difficult if you have configured the attribute with a very \nbroad pattern (e.g. *.foo). You would either need to rename the\nfile or limit the scope of your pattern.\n\n- Lars\n"},{"id":"334857","messageId":"xmqq7etosymp.fsf@gitster.mtv.corp.google.com","threadId":"47426","inReplyTo":"C062CB10-9885-46B4-9614-65A39BD5FBE8@gmail.com","subject":"Re: [PATCH v1] convert: add support for 'encoding' attribute","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-12-14T23:01:02Z","receivedAt":"2017-12-14T23:01:09Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Lars Schneider writes:\n\n> That way you could control the encoding for a text file specific\n> for each path similar to the \"mode bits\". That also means you could\n> change the encoding of a file while the blob content stays the same.\n\nThat is exactly why I said that I doubt it makes sense.\n\nWhen you have the same blob object (that is in UTF-8) appear at two\nplaces in a tree (or two different subtrees inside a single tree)\nand the two tree entries that point at the blob tells contradicting\nstory, you would have a checkout of the same contents in two\ndifferent encodings. If you have the same blob object appear in two\nadjacent commits at the same path, with one commit's tree recording\nits encoding differently from the other's, you would switch\nencodings when you switch branches. I doubt these are \"features\",\nbut sources of confusion.\n\nBe it a feature, or a misfeature, it is shared with the existing\nsolution which is .gitattributes embedded in the tree, so you are\nnot making anything better or worse by moving the information to\ntree entry.\n\nWhat I actually expected to hear when somebody talks about \"ideal\"\n(which may not even be achievable given existing user base) was to\nmake blob object declare its desired external representation. That\nwould remove the two possible confusion cited above, because once\nyou have a blob, you have everything needed to externalize it.\n\nIn any case, I do not think we'd go there anyway, so ...\n"},{"id":"334869","messageId":"20171215095838.GA3567@sigill.intra.peff.net","threadId":"47426","inReplyTo":"20171211155023.1405-1-lars.schneider@autodesk.com","subject":"Re: [PATCH v1] convert: add support for 'encoding' attribute","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-12-15T09:58:38Z","receivedAt":"2017-12-15T09:58:51Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Dec 11, 2017 at 04:50:23PM +0100, lars.schneider@autodesk.com wrote:\n\n> From: Lars Schneider \n> \n> Git and its tools (e.g. git diff) expect all text files in UTF-8\n> encoding. Git will happily accept content in all other encodings, too,\n> but it might not be able to process the text (e.g. viewing diffs or\n> changing line endings).\n> \n> Add an attribute to tell Git what encoding the user has defined for a\n> given file. If the content is added to the index, then Git converts the\n> content to a canonical UTF-8 representation. On checkout Git will\n> reverse the conversion.\n\nThis made me wonder what happens when you have a file that _was_ utf16,\nand then you later convert it to utf8 and add a .gitattributes entry.\n\nI tried this with your patch:\n\n git init repo\n cd repo\n \n echo foo | iconv -t utf16 >file\n git add file\n git commit -m utf16\n \n echo 'file encoding=utf16' >.gitattributes\n touch file ;# make it stat-dirty\n git commit -m convert\n \n git checkout HEAD^\n\nThat works OK, because we try to read the attributes from the\ndestination tree for a checkout. If you do this:\n\n echo 'file encoding=utf16' >.git/info/attributes\n git checkout HEAD^\n\nthen we get:\n\n warning: failed to encode 'file' from utf-8 to utf16\n\nAt least it figured out that it couldn't convert the content. It's\nslightly troubling that it would try in the first place, though; are\nthere encoding pairs where we might accidentally generate nonsense?\n\nIt _should_ be uncommon, I think, to have a repo-level attribute set\nlike that. It's very common for us to use whatever happens to be in the\nchecked-out .gitattributes for some attributes (e.g., when doing a diff\nof an older commit), but I think for checkout-related ones it's not. So\nI think it may generally work in practice. And certainly the line-ending\ncode would share any similar problems, but at least there the result is\nless confusing than mojibake.\n\nPlaying around, I also managed to do this:\n\n echo 'file encoding=utf16' >.gitattributes\n echo foo >file\n\n # I did these with an old version of git that didn't\n # support the new attribute, so they blindly added the utf8 content.\n git add .\n git commit -m convert\n\n git.compile checkout HEAD^\n\nwhich yielded:\n\n fatal: encoding 'file' from utf16 to utf-8 and back is not the same\n\nNow obviously my situation is somewhat nonsense. I was trying to force\nthe in-repo representation to utf8, but ended up with a mismatched\nworking tree file. But what's somewhat troubling is that I couldn't\ncheckout _away_ from that state due to the die() in convert_to_git().\nWhich is in turn just there as part of refresh_index().\n\nAnd indeed, other commands hit the same problem:\n\n $ git.compile diff\n fatal: encoding 'file' from utf16 to utf-8 and back is not the same\n\n $ git.compile checkout -f\n fatal: encoding 'file' from utf16 to utf-8 and back is not the same\n\nIt may make sense to die() during \"git add .\" (since we're actually\nchanging the index entry, and we don't want to put nonsense into a\ntree). But I'm not sure it's the best thing for operations which just\nwant to read the content. For them, perhaps it would be more appropriate\nto issue a warning and return the untouched content.\n\n-Peff\n"},{"id":"334950","messageId":"20171217171404.GA18175@tor.lan","threadId":"47426","inReplyTo":"20171211155023.1405-1-lars.schneider@autodesk.com","subject":"Re: [PATCH v1] convert: add support for 'encoding' attribute","fromName":"Torsten Bögershausen","fromEmail":"tboegi@web.de","sentAt":"2017-12-17T17:14:04Z","receivedAt":"2017-12-17T17:14:29Z","isPatch":true,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"On Mon, Dec 11, 2017 at 04:50:23PM +0100, lars.schneider@autodesk.com wrote:\n> From: Lars Schneider \n> \n> Git and its tools (e.g. git diff) expect all text files in UTF-8\n> encoding. Git will happily accept content in all other encodings, too,\n> but it might not be able to process the text (e.g. viewing diffs or\n> changing line endings).\n> \n> Add an attribute to tell Git what encoding the user has defined for a\n> given file. If the content is added to the index, then Git converts the\n> content to a canonical UTF-8 representation. On checkout Git will\n> reverse the conversion.\n> \n> Reviewed-by: Patrick Lühne \n> Signed-off-by: Lars Schneider \n> ---\n> \n> Hi,\n> \n> here is a WIP patch to add text encoding support for files encoded with\n> something other than UTF-8 [RFC].\n> \n> The 'encoding' attribute is already used to view blobs in gitk. That\n> could be a problem as the content is stored in Git with the defined\n> encoding. This patch would interpret the content as UTF-8 encoded and\n> it would try to reencode it to the defined encoding on checkout.\n> Plus, many repos define the attribute very broad (e.g. \"* encoding=cp1251\").\n> These folks would see errors like these with my patch:\n> error: failed to encode 'foo.bar' from utf-8 to cp1251\n> \n> A quick search on GitHub reveals 2,722 repositories that use the\n> 'encoding' attribute [1]. Using the GitHub API [2], I identified the\n> following encodings in all publicly accessible repositories:\n> \n> ANSI <-- garbage (ignore by my implementation)\n> cp1251\n> cp866\n> cp950\n> iso8859-1\n> koi8-r\n> shiftjis <-- garbage (ignore by my implementation)\n> UTF-8 <-- unnecessary (ignore by my implementation)\n> utf8 <-- garbage (ignore by my implementation)\n> \n> TODOs:\n> - The iconv docs mention that \"errno == EINVAL\" is harmless but\n> we don't handle that case in utf8.c:reencode_string_iconv()\n> - Git does not compile with NO_ICONV=1 right now because of\n> compat/precompose_utf8.c. I will send a patch to fix that.\n\nMinor: Does Git not compile under MacOS when setting NO_ICONV=1 ?\nThis is possible not introduced by the patch. \n\n> \n> Questions:\n> - Should I use warning() or error() in the patch?\n> (currently I use the warning)\n\nErrors, see below.\n\n> - Do you agree with the approach in general?\n\nYes, some comments inline.\n\n> \n> Thanks,\n> Lars\n> \n> \n> [RFC] http://public-inbox.org/git/BDB9B884-6D17-4BE3-A83C-F67E2AFA2B46@gmail.com\n> [1] https://github.com/search?p=1&q=encoding+filename%3Agitattributes&type=Code&utf8=%E2%9C%93\n> [2] curl --user larsxschneider:secret -H 'Accept: application/vnd.github.v3.text-match+json' 'https://api.github.com/search/code?q=encoding+in:file+filename:gitattributes' | jq -r '.items[].text_matches[].fragment' | sed 's/.*encoding=//' | sort | uniq\n> [3] https://www.gnu.org/software/libc/manual/html_node/iconv-Examples.html#iconv-Examples\n> \n> \n> \n> \n> Notes:\n> Base Ref: master\n> Web-Diff: https://github.com/larsxschneider/git/commit/afc9e88a4d\n> Checkout: git fetch https://github.com/larsxschneider/git encoding-v1 && git checkout afc9e88a4d\n> \n> Documentation/gitattributes.txt | 27 ++++++\n> convert.c | 192 +++++++++++++++++++++++++++++++++++++++-\n> t/t0028-encoding.sh | 60 +++++++++++++\n> 3 files changed, 278 insertions(+), 1 deletion(-)\n> create mode 100755 t/t0028-encoding.sh\n> \n> diff --git a/Documentation/gitattributes.txt b/Documentation/gitattributes.txt\n> index 30687de81a..84de2fa49c 100644\n> --- a/Documentation/gitattributes.txt\n> +++ b/Documentation/gitattributes.txt\n> @@ -146,6 +146,33 @@ Unspecified::\n> Any other value causes Git to act as if `text` has been left\n> unspecified.\n> \n> +`encoding`\n> +^^^^^^^^^^\n> +\n> +By default Git assumes UTF-8 encoding for text files. The `encoding`\n\nThis is probably \"ASCII\" and it's supersets like ISO-8859-1 or UTF-8.\n\n> +attribute sets the encoding to be used in the working directory.\n> +If the path is added to the index, then Git encodes the content to\n> +UTF-8. On checkout the content is encoded back to the original\n> +encoding. Consequently, you can use all built-in Git text processing\n> +tools (e.g. git diff, line ending conversions, etc.) with your\n> +non-UTF-8 encoded file.\n> +\n> +Please note that re-encoding content can cause errors and requires\n> +resources. Use the `encoding` attribute only if you cannot store\n> +a file in UTF-8 encoding and if you want Git to be able to process\n> +the text.\n> +\n> +------------------------\n> +*.txt\t\ttext encoding=UTF-16\n> +------------------------\n\nI think that encoding (or worktree-encoding as said elsewhere) must imply text.\n(That is not done in convert.c, but assuming binary or even auto\ndoes not make much sense to me)\n\n\n> +\n> +All `iconv` encodings with a stable round-trip conversion to and from\n> +UTF-8 are supported. You can see a full list with the following command:\n> +\n> +------------------------\n> +iconv --list\n> +------------------------\n> +\n> `eol`\n> ^^^^^\n> \n> diff --git a/convert.c b/convert.c\n> index 20d7ab67bd..ee19c17104 100644\n> --- a/convert.c\n> +++ b/convert.c\n> @@ -7,6 +7,7 @@\n> #include \"sigchain.h\"\n> #include \"pkt-line.h\"\n> #include \"sub-process.h\"\n> +#include \"utf8.h\"\n> \n> /*\n> * convert.c - convert a file when checking it out and checking it in.\n> @@ -256,6 +257,149 @@ static int will_convert_lf_to_crlf(size_t len, struct text_stat *stats,\n> \n> }\n\nI would avoid to use these #ifdefs here.\nAll functions can be in strbuf.c (and may have #ifdefs there), but not here.\n\n> \n> +#ifdef NO_ICONV\n> +#ifndef _ICONV_T\n> +/* The type is just a placeholder and not actually used. */\n> +typedef void* iconv_t;\n> +#endif\n> +#endif\n> +\n> +static struct encoding {\n> +\tconst char *name;\n> +\ticonv_t to_git; /* conversion to Git's canonical encoding (UTF-8) */\n> +\ticonv_t to_worktree; /* conversion to user-defined encoding */\n> +\tstruct encoding *next;\n> +} *encoding, **encoding_tail;\n\nThis seems like an optimazation, assuning that iconv_open() eats a lot of ressources.\nI don't think this is the case. (Undere MacOS we run iconv_open() together with\nevery opendir(), and optimizing this out did not show any measurable improvements)\n\n> +static const char *default_encoding = \"utf-8\";\n\nThe most portable form is \"UTF-8\" (correct me if that is wrong)\n\n> +static iconv_t invalid_conversion = (iconv_t)-1;\n> +\n> +static int encode_to_git(const char *path, const char *src, size_t src_len,\n> +\t\t\t struct strbuf *buf, struct encoding *enc)\n> +{\n> +#ifndef NO_ICONV\n> +\tchar *dst, *re_src;\n> +\tint dst_len, re_src_len;\n> +\n> +\t/*\n> +\t * No encoding is specified or there is nothing to encode.\n> +\t * Tell the caller that the content was not modified.\n> +\t */\n> +\tif (!enc || (src && !src_len))\n> +\t\treturn 0;\n> +\n> +\t/*\n> +\t * Looks like we got called from \"would_convert_to_git()\".\n> +\t * This means Git wants to know if it would encode (= modify!)\n> +\t * the content. Let's answer with \"yes\", since an encoding was\n> +\t * specified.\n> +\t */\n> +\tif (!buf && !src)\n> +\t\treturn 1;\n> +\n> +\tif (enc->to_git == invalid_conversion) {\n> +\t\tenc->to_git = iconv_open(default_encoding, encoding->name);\n> +\t\tif (enc->to_git == invalid_conversion)\n> +\t\t\twarning(_(\"unsupported encoding %s\"), encoding->name);\n> +\t}\n\n \t/* There are 2 different types of reaction:\n\t Either users know what that a warning means: You asked for problems,\n\t \t and do the right thing. Other may may ignore the warning\n\t\t\t - in both cases an error is useful */\n\n> +\n> +\tif (enc->to_worktree == invalid_conversion)\n> +\t\tenc->to_worktree = iconv_open(encoding->name, default_encoding);\n> +\n> +\t/*\n> +\t * Do nothing if the encoding is not supported. This could happen in\n> +\t * two cases:\n> +\t * (1) The encoding is garbage. That is no problem as we would not\n> +\t * encode the content to UTF-8 on \"add\" and we would not encode\n> +\t * it back on \"checkout\".\n> +\t * (2) Git users use different iconv versions that support different\n> +\t * encodings. This could lead to problems, as the content might\n> +\t * not be encoded on \"add\" but encoded back on \"checkout\" (or\n> +\t * the other way around).\n> +\t * We print a one-time warning to the user in both cases above.\n> +\t */\n\nIsn't an error better than \"garbage in -> garbage out\" ?\n\n> +\tif (enc->to_git == invalid_conversion || enc->to_worktree == invalid_conversion)\n> +\t\treturn 0;\n> +\n> +\tdst = reencode_string_iconv(src, src_len, enc->to_git, &dst_len);\n\nI would suggest to write a function in strbuf and use it here:\n\nint strbuf_add_reencode((struct strbuf *sb, const void *data, size_t len,\n const char *from, const char *to)\n\n> +\tif (!dst)\n> +\t\t/*\n> +\t\t * We could add the blob \"as-is\" to Git. However, on checkout\n> +\t\t * we would try to reencode to the original encoding. This\n> +\t\t * would fail and we would leave the user with a messed-up\n> +\t\t * working tree. Let's try to avoid this by screaming loud.\n> +\t\t */\n> +\t\tdie(_(\"failed to encode '%s' from %s to %s\"),\n> +\t\t\tpath, enc->name, default_encoding);\n> +\n> +\t/*\n> +\t * Encode dst back to ensure no information is lost. This wastes\n\n\"wastes\" ? \"uses\" i would say.\n\n> +\t * a few cycles as most conversions are round trip conversion\n> +\t * safe. However, content that has an invalid encoding might not\n> +\t * match its original byte sequence after the UTF-8 conversion\n> +\t * round trip. Let's play safe here and check the round trip\n> +\t * conversion.\n> +\t */\n> +\tre_src = reencode_string_iconv(dst, dst_len, enc->to_worktree, &re_src_len);\n> +\tif (!re_src || strcmp(src, re_src)) {\n> +\t\tdie(_(\"encoding '%s' from %s to %s and back is not the same\"),\n> +\t\t\tpath, enc->name, default_encoding);\n> +\t}\n> +\tfree(re_src);\n> +\n> +\tstrbuf_attach(buf, dst, dst_len, dst_len + 1);\n> +\treturn 1;\n> +#else\n> +\twarning(_(\"cannot encode '%s' from %s to %s because \"\n> +\t\t\"your Git was not compiled with encoding support\"),\n> +\t\tpath, enc->name, default_encoding);\n> +\treturn 0;\n> +#endif\n> +}\n> +\n> +static int encode_to_worktree(const char *path, const char *src, size_t src_len,\n> +\t\t\t struct strbuf *buf, struct encoding *enc)\n\n\nSimilar here: Better to use a (new) function from strbuf.\n\n> +{\n> +#ifndef NO_ICONV\n> +\tchar *dst;\n> +\tint dst_len;\n> +\n> +\t/*\n> +\t * No encoding is specified or there is nothing to encode.\n> +\t * Tell the caller that the content was not modified.\n> +\t */\n> +\tif (!enc || (src && !src_len))\n> +\t\treturn 0;\n> +\n> +\tif (enc->to_worktree == invalid_conversion) {\n> +\t\tenc->to_worktree = iconv_open(encoding->name, default_encoding);\n> +\t\tif (enc->to_worktree == invalid_conversion)\n> +\t\t\twarning(\"unsupported encoding %s\", encoding->name);\n> +\t}\n> +\n> +\t/*\n> +\t * Do nothing if the encoding is not supported.\n> +\t * See detailed explanation in encode_to_git().\n> +\t */\n> +\tif (enc->to_worktree == invalid_conversion)\n> +\t\treturn 0;\n> +\n> +\tdst = reencode_string_iconv(src, src_len, enc->to_worktree, &dst_len);\n> +\tif (!dst) {\n> +\t\twarning(\"failed to encode '%s' from %s to %s\",\n> +\t\t\tpath, default_encoding, encoding->name);\n> +\t\treturn 0;\n> +\t}\n> +\n> +\tstrbuf_attach(buf, dst, dst_len, dst_len + 1);\n> +\treturn 1;\n> +#else\n> +\twarning(_(\"cannot encode '%s' from %s to %s because \"\n> +\t\t\"your Git was not compiled with encoding support\"),\n> +\t\tpath, default_encoding, encoding->name);\n> +\treturn 0;\n> +#endif\n> +}\n> +\n> static int crlf_to_git(const struct index_state *istate,\n> \t\t const char *path, const char *src, size_t len,\n> \t\t struct strbuf *buf,\n> @@ -969,6 +1113,32 @@ static int ident_to_worktree(const char *path, const char *src, size_t len,\n> \treturn 1;\n> }\n> \n> +static struct encoding *git_path_check_encoding(struct attr_check_item *check)\n> +{\n> +\tconst char *value = check->value;\n> +\tstruct encoding *enc;\n> +\n> +\tif (ATTR_UNSET(value))\n> +\t\treturn NULL;\n> +\n> +\tfor (enc = encoding; enc; enc = enc->next)\n> +\t\tif (!strcmp(value, enc->name))\n> +\t\t\treturn enc;\n> +\n> +\t/* Don't encode to the default encoding */\n> +\tif (!strcasecmp(value, default_encoding))\n> +\t\treturn NULL;\n> +\n> +\tenc = xcalloc(1, sizeof(struct convert_driver));\n> +\tenc->name = xstrdup(value);\n> +\tenc->to_git = invalid_conversion;\n> +\tenc->to_worktree = invalid_conversion;\n> +\t*encoding_tail = enc;\n> +\tencoding_tail = &(enc->next);\n> +\n> +\treturn enc;\n> +}\n> +\n> static enum crlf_action git_path_check_crlf(struct attr_check_item *check)\n> {\n> \tconst char *value = check->value;\n> @@ -1024,6 +1194,7 @@ struct conv_attrs {\n> \tenum crlf_action attr_action; /* What attr says */\n> \tenum crlf_action crlf_action; /* When no attr is set, use core.autocrlf */\n> \tint ident;\n> +\tstruct encoding *encoding; /* Supported encoding or default encoding if NULL */\n> };\n> \n> static void convert_attrs(struct conv_attrs *ca, const char *path)\n> @@ -1032,8 +1203,9 @@ static void convert_attrs(struct conv_attrs *ca, const char *path)\n> \n> \tif (!check) {\n> \t\tcheck = attr_check_initl(\"crlf\", \"ident\", \"filter\",\n> -\t\t\t\t\t \"eol\", \"text\", NULL);\n> +\t\t\t\t\t \"eol\", \"text\", \"encoding\", NULL);\n> \t\tuser_convert_tail = &user_convert;\n> +\t\tencoding_tail = &encoding;\n> \t\tgit_config(read_convert_config, NULL);\n> \t}\n\nAs written before, \"worktree-encoding\" should imply text.\n\n> \n> @@ -1055,6 +1227,7 @@ static void convert_attrs(struct conv_attrs *ca, const char *path)\n> \t\t\telse if (eol_attr == EOL_CRLF)\n> \t\t\t\tca->crlf_action = CRLF_TEXT_CRLF;\n> \t\t}\n> +\t\tca->encoding = git_path_check_encoding(ccheck + 5);\n> \t} else {\n> \t\tca->drv = NULL;\n> \t\tca->crlf_action = CRLF_UNDEFINED;\n> @@ -1135,6 +1308,13 @@ int convert_to_git(const struct index_state *istate,\n> \t\tsrc = dst->buf;\n> \t\tlen = dst->len;\n> \t}\n> +\n> +\tret |= encode_to_git(path, src, len, dst, ca.encoding);\n> +\tif (ret && dst) {\n> +\t\tsrc = dst->buf;\n> +\t\tlen = dst->len;\n> +\t}\n> +\n> \tif (checksafe != SAFE_CRLF_KEEP_CRLF) {\n> \t\tret |= crlf_to_git(istate, path, src, len, dst, ca.crlf_action, checksafe);\n> \t\tif (ret && dst) {\n> @@ -1158,6 +1338,7 @@ void convert_to_git_filter_fd(const struct index_state *istate,\n> \tif (!apply_filter(path, NULL, 0, fd, dst, ca.drv, CAP_CLEAN, NULL))\n> \t\tdie(\"%s: clean filter '%s' failed\", path, ca.drv->name);\n> \n> +\tencode_to_git(path, dst->buf, dst->len, dst, ca.encoding);\n> \tcrlf_to_git(istate, path, dst->buf, dst->len, dst, ca.crlf_action, checksafe);\n> \tident_to_git(path, dst->buf, dst->len, dst, ca.ident);\n> }\n> @@ -1189,6 +1370,12 @@ static int convert_to_working_tree_internal(const char *path, const char *src,\n> \t\t}\n> \t}\n> \n> +\tret |= encode_to_worktree(path, src, len, dst, ca.encoding);\n> +\tif (ret) {\n> +\t\tsrc = dst->buf;\n> +\t\tlen = dst->len;\n> +\t}\n> +\n> \tret_filter = apply_filter(\n> \t\tpath, src, len, -1, dst, ca.drv, CAP_SMUDGE, dco);\n> \tif (!ret_filter && ca.drv && ca.drv->required)\n> @@ -1655,6 +1842,9 @@ struct stream_filter *get_stream_filter(const char *path, const unsigned char *s\n> \tif (ca.drv && (ca.drv->process || ca.drv->smudge || ca.drv->clean))\n> \t\treturn NULL;\n> \n> +\tif (ca.encoding)\n> +\t\treturn NULL;\n> +\n> \tif (ca.crlf_action == CRLF_AUTO || ca.crlf_action == CRLF_AUTO_CRLF)\n> \t\treturn NULL;\n> \n\n> diff --git a/t/t0028-encoding.sh b/t/t0028-encoding.sh\n\n(I didn't review the test yet)\n\nAnother comment for a possible improvement:\n\"git diff\" could be told to write the worktree-encoding into the diff,\nand \"git apply\" can pick that up. \n"},{"id":"334961","messageId":"20171218101334.GA20685@tor.lan","threadId":"47426","inReplyTo":"a07a0b5c-453a-f644-d5e5-7d185a943088@kdbg.org","subject":"Re: [PATCH v1] convert: add support for 'encoding' attribute","fromName":"Torsten Bögershausen","fromEmail":"tboegi@web.de","sentAt":"2017-12-18T10:13:34Z","receivedAt":"2017-12-18T10:13:58Z","isPatch":true,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"On Mon, Dec 11, 2017 at 09:47:24PM +0100, Johannes Sixt wrote:\n> Am 11.12.2017 um 16:50 schrieb lars.schneider@autodesk.com:\n> >From: Lars Schneider \n> >\n> >Git and its tools (e.g. git diff) expect all text files in UTF-8\n> >encoding. Git will happily accept content in all other encodings, too,\n> >but it might not be able to process the text (e.g. viewing diffs or\n> >changing line endings).\n> >\n> >Add an attribute to tell Git what encoding the user has defined for a\n> >given file. If the content is added to the index, then Git converts the\n> >content to a canonical UTF-8 representation. On checkout Git will\n> >reverse the conversion.\n> >\n> >Reviewed-by: Patrick Lühne \n> >Signed-off-by: Lars Schneider \n> >---\n> >\n> >Hi,\n> >\n> >here is a WIP patch to add text encoding support for files encoded with\n> >something other than UTF-8 [RFC].\n> >\n> >The 'encoding' attribute is already used to view blobs in gitk. That\n> >could be a problem as the content is stored in Git with the defined\n> >encoding. This patch would interpret the content as UTF-8 encoded and\n> \n> This will be a major drawback for me because my code base stores text files\n> that are not UTF-8 encoded. And I do use the existing 'encoding' attribute\n> to view the text in git-gui and gitk. Repurposing this attribute name is not\n> an option, IMO.\n\nJust to confirm my missing knowledge here:\nDoes this mean, that git-gui and gitk can decode/reencode\nthe content of a file/blob, when the .gitattributes say so ?\n\nIf yes, would it make sense to enhance the \"git diff\" instead ?\n\"git diff --encoding\" will pick up the commited encoding from\n.attributes, convert it into UTF-8, and run the diff ?\nWe actually could enhance the \"git diff\" output with a single\nline saying\n\"Git index-encoding=cp1251\"\nor so, which can be picked up by \"git apply\".\n\nThe advantage would be that we could continue to commit in UTF-16\nas before, and avoid the glitches with .gitattributes, that Peff\npointed out.\n\nDoes this make sense ?\n\n> \n> >it would try to reencode it to the defined encoding on checkout > Plus,\n> >many repos define the attribute very broad (e.g. \"*\n> encoding=cp1251\").\n\nIs this a user mistake ?\n\n> >These folks would see errors like these with my patch:\n> > error: failed to encode 'foo.bar' from utf-8 to cp1251\n> \n> -- Hannes\n"},{"id":"334962","messageId":"D1E22F0E-EC10-4133-9177-AE20F398A8AC@gmail.com","threadId":"47426","inReplyTo":"20171215095838.GA3567@sigill.intra.peff.net","subject":"Re: [PATCH v1] convert: add support for 'encoding' attribute","fromName":"Lars Schneider","fromEmail":"larsxschneider@gmail.com","sentAt":"2017-12-18T10:54:32Z","receivedAt":"2017-12-18T10:54:40Z","isPatch":true,"sender":{"key":"larsxschneider@gmail.com","avatar":"https://avatars.githubusercontent.com/u/477434?v=4"},"body":"\n> On 15 Dec 2017, at 10:58, Jeff King wrote:\n> \n> On Mon, Dec 11, 2017 at 04:50:23PM +0100, lars.schneider@autodesk.com wrote:\n> \n>> From: Lars Schneider \n>> \n>> Git and its tools (e.g. git diff) expect all text files in UTF-8\n>> encoding. Git will happily accept content in all other encodings, too,\n>> but it might not be able to process the text (e.g. viewing diffs or\n>> changing line endings).\n>> \n>> Add an attribute to tell Git what encoding the user has defined for a\n>> given file. If the content is added to the index, then Git converts the\n>> content to a canonical UTF-8 representation. On checkout Git will\n>> reverse the conversion.\n> \n> This made me wonder what happens when you have a file that _was_ utf16,\n> and then you later convert it to utf8 and add a .gitattributes entry.\n> \n> I tried this with your patch:\n> \n> git init repo\n> cd repo\n> \n> echo foo | iconv -t utf16 >file\n> git add file\n> git commit -m utf16\n> \n> echo 'file encoding=utf16' >.gitattributes\n> touch file ;# make it stat-dirty\n> git commit -m convert\n> \n> git checkout HEAD^\n> \n> That works OK, because we try to read the attributes from the\n> destination tree for a checkout. If you do this:\n> \n> echo 'file encoding=utf16' >.git/info/attributes\n> git checkout HEAD^\n> \n> then we get:\n> \n> warning: failed to encode 'file' from utf-8 to utf16\n> \n> At least it figured out that it couldn't convert the content. It's\n> slightly troubling that it would try in the first place, though; are\n> there encoding pairs where we might accidentally generate nonsense?\n\nAt this point we interpret utf-16 content as utf-8 and try to convert\nit to utf-16. That of course fails because utf-16 content is no valid\nutf-8. How could we stop trying that? How could Git possibly know what\nkind of encoding is used (apart from our new hint in gitattributes)?\n\nI asked myself the question about nonsense encoding pairs, too. That\nwas the reason I added the \"X -> UTF-8 -> Y && X == Y\" check on Git\nadd. However, with \".git/info/attributes\" you could circumvent that\ncheck and probably generate nonsense.\n\n\n> It _should_ be uncommon, I think, to have a repo-level attribute set\n> like that. It's very common for us to use whatever happens to be in the\n> checked-out .gitattributes for some attributes (e.g., when doing a diff\n> of an older commit), but I think for checkout-related ones it's not. So\n> I think it may generally work in practice. And certainly the line-ending\n> code would share any similar problems, but at least there the result is\n> less confusing than mojibake.\n> \n> Playing around, I also managed to do this:\n> \n> echo 'file encoding=utf16' >.gitattributes\n> echo foo >file\n> \n> # I did these with an old version of git that didn't\n> # support the new attribute, so they blindly added the utf8 content.\n> git add .\n> git commit -m convert\n> \n> git.compile checkout HEAD^\n> \n> which yielded:\n> \n> fatal: encoding 'file' from utf16 to utf-8 and back is not the same\n> \n> Now obviously my situation is somewhat nonsense. I was trying to force\n> the in-repo representation to utf8, but ended up with a mismatched\n> working tree file. But what's somewhat troubling is that I couldn't\n> checkout _away_ from that state due to the die() in convert_to_git().\n> Which is in turn just there as part of refresh_index().\n> \n> And indeed, other commands hit the same problem:\n> \n> $ git.compile diff\n> fatal: encoding 'file' from utf16 to utf-8 and back is not the same\n> \n> $ git.compile checkout -f\n> fatal: encoding 'file' from utf16 to utf-8 and back is not the same\n> \n> It may make sense to die() during \"git add .\" (since we're actually\n> changing the index entry, and we don't want to put nonsense into a\n> tree). But I'm not sure it's the best thing for operations which just\n> want to read the content. For them, perhaps it would be more appropriate\n> to issue a warning and return the untouched content.\n\nAbsolutely! Thanks for spotting this. I will try to run die() only on\n\"git add\" in v2.\n\n- Lars"},{"id":"334964","messageId":"20171218125907.GA4665@sigill.intra.peff.net","threadId":"47426","inReplyTo":"D1E22F0E-EC10-4133-9177-AE20F398A8AC@gmail.com","subject":"Re: [PATCH v1] convert: add support for 'encoding' attribute","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-12-18T12:59:08Z","receivedAt":"2017-12-18T12:59:16Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Dec 18, 2017 at 11:54:32AM +0100, Lars Schneider wrote:\n\n> > warning: failed to encode 'file' from utf-8 to utf16\n> > \n> > At least it figured out that it couldn't convert the content. It's\n> > slightly troubling that it would try in the first place, though; are\n> > there encoding pairs where we might accidentally generate nonsense?\n> \n> At this point we interpret utf-16 content as utf-8 and try to convert\n> it to utf-16. That of course fails because utf-16 content is no valid\n> utf-8. How could we stop trying that? How could Git possibly know what\n> kind of encoding is used (apart from our new hint in gitattributes)?\n\nYeah, sorry if I wasn't clear: I don't really have an answer to those\nquestions either. So this is probably the best we can do. I was mostly\njust trying to think through the worst case, and what could go wrong.\n\n> > It may make sense to die() during \"git add .\" (since we're actually\n> > changing the index entry, and we don't want to put nonsense into a\n> > tree). But I'm not sure it's the best thing for operations which just\n> > want to read the content. For them, perhaps it would be more appropriate\n> > to issue a warning and return the untouched content.\n> \n> Absolutely! Thanks for spotting this. I will try to run die() only on\n> \"git add\" in v2.\n\nGreat, thanks!\n\n-Peff\n"},{"id":"334965","messageId":"20171218131249.GB4665@sigill.intra.peff.net","threadId":"47426","inReplyTo":"20171218101334.GA20685@tor.lan","subject":"Re: [PATCH v1] convert: add support for 'encoding' attribute","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-12-18T13:12:49Z","receivedAt":"2017-12-18T13:12:56Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Dec 18, 2017 at 11:13:34AM +0100, Torsten Bögershausen wrote:\n\n> Just to confirm my missing knowledge here:\n> Does this mean, that git-gui and gitk can decode/reencode\n> the content of a file/blob, when the .gitattributes say so ?\n\nThat's my impression, yes.\n\n> If yes, would it make sense to enhance the \"git diff\" instead ?\n> \"git diff --encoding\" will pick up the commited encoding from\n> .attributes, convert it into UTF-8, and run the diff ?\n\nYou can do something like this already:\n\n git config diff.utf16.textconv 'iconv -f utf16 -t utf8'\n echo file diff=utf16 >.gitattributes\n\nI have no objection to teaching it that \"encoding\" means roughly the\nsame thing, which would have two advantages:\n\n - we could do it in-process, which would be much faster\n\n - we could skip the extra config step, which is a blocker for\n server-side diffs on sites like GitHub\n\nI don't think you'd really need \"--encoding\". This could just be\ntriggered by the normal \"--textconv\" rules (i.e., just treat this \"as\nif\" the textconv above was configured when we see an encoding\nattribute).\n\n> We actually could enhance the \"git diff\" output with a single\n> line saying\n> \"Git index-encoding=cp1251\"\n> or so, which can be picked up by \"git apply\".\n\nThat extends it beyond what textconv can do (we assume that textconv\npatches are _just_ for human consumption, and can't be applied). And it\nwould mean you'd potentially want to trigger it in more places. On the\nother hand, I don't know that we're guaranteed that a round-trip\nbetween encodings will produce a byte-wise identical result. The nice\nthing about piggy-backing on textconv is that it's already dealt with\nthat problem.\n\n-Peff\n"},{"id":"334973","messageId":"xmqqzi6fykx1.fsf@gitster.mtv.corp.google.com","threadId":"47426","inReplyTo":"20171218101334.GA20685@tor.lan","subject":"Re: [PATCH v1] convert: add support for 'encoding' attribute","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-12-18T18:02:02Z","receivedAt":"2017-12-18T18:02:14Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Torsten Bögershausen writes:\n\n> Just to confirm my missing knowledge here:\n> Does this mean, that git-gui and gitk can decode/reencode\n> the content of a file/blob, when the .gitattributes say so ?\n\nThese programs, when told that a file is in an encoding, read bytes\nfrom that file and interpret that byte sequence in the given\nencoding, and blits the corresponding glyphs on the screen---giving\nUTF-8 out is not the primary feature of them, so I am not sure\ndecode/reencode is a good term to use here.\n"},{"id":"334995","messageId":"c86c6f68-e78c-a36c-80e1-5d63e8f9d4d9@kdbg.org","threadId":"47426","inReplyTo":"20171218101334.GA20685@tor.lan","subject":"Re: [PATCH v1] convert: add support for 'encoding' attribute","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2017-12-18T21:55:11Z","receivedAt":"2017-12-18T21:55:20Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 18.12.2017 um 11:13 schrieb Torsten Bögershausen:\n> Just to confirm my missing knowledge here:\n> Does this mean, that git-gui and gitk can decode/reencode\n> the content of a file/blob, when the .gitattributes say so ?\n\nNo. I think they parse the output of git-diff et.al., split it per file, \nand then consult .gitattributes to interpret the byte sequence according \nto the encoding.\n\n-- Hannes\n"},{"id":"335220","messageId":"20171223080854.GA30360@tor.lan","threadId":"47426","inReplyTo":"20171218131249.GB4665@sigill.intra.peff.net","subject":"Re: [PATCH v1] convert: add support for 'encoding' attribute","fromName":"Torsten Bögershausen","fromEmail":"tboegi@web.de","sentAt":"2017-12-23T08:08:54Z","receivedAt":"2017-12-23T08:09:20Z","isPatch":true,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"On Mon, Dec 18, 2017 at 08:12:49AM -0500, Jeff King wrote:\n> On Mon, Dec 18, 2017 at 11:13:34AM +0100, Torsten Bögershausen wrote:\n> \n> > Just to confirm my missing knowledge here:\n> > Does this mean, that git-gui and gitk can decode/reencode\n> > the content of a file/blob, when the .gitattributes say so ?\n> \n> That's my impression, yes.\n> \n> > If yes, would it make sense to enhance the \"git diff\" instead ?\n> > \"git diff --encoding\" will pick up the commited encoding from\n> > .attributes, convert it into UTF-8, and run the diff ?\n> \n> You can do something like this already:\n> \n> git config diff.utf16.textconv 'iconv -f utf16 -t utf8'\n> echo file diff=utf16 >.gitattributes\n> \n> I have no objection to teaching it that \"encoding\" means roughly the\n> same thing, which would have two advantages:\n> \n> - we could do it in-process, which would be much faster\n> \n> - we could skip the extra config step, which is a blocker for\n> server-side diffs on sites like GitHub\n> \n> I don't think you'd really need \"--encoding\". This could just be\n> triggered by the normal \"--textconv\" rules (i.e., just treat this \"as\n> if\" the textconv above was configured when we see an encoding\n> attribute).\n\nI can probably come up with a patch the next weeks or so.\n\n> \n> > We actually could enhance the \"git diff\" output with a single\n> > line saying\n> > \"Git index-encoding=cp1251\"\n> > or so, which can be picked up by \"git apply\".\n> \n> That extends it beyond what textconv can do (we assume that textconv\n> patches are _just_ for human consumption, and can't be applied). And it\n> would mean you'd potentially want to trigger it in more places. On the\n> other hand, I don't know that we're guaranteed that a round-trip\n> between encodings will produce a byte-wise identical result. The nice\n> thing about piggy-backing on textconv is that it's already dealt with\n> that problem.\n> \n> -Peff\n"},{"id":"335470","messageId":"BF5C58AC-C1E6-4AD5-A4F8-C4CC5C9108F7@gmail.com","threadId":"47426","inReplyTo":"20171217171404.GA18175@tor.lan","subject":"Re: [PATCH v1] convert: add support for 'encoding' attribute","fromName":"Lars Schneider","fromEmail":"larsxschneider@gmail.com","sentAt":"2017-12-28T16:14:25Z","receivedAt":"2017-12-28T16:14:34Z","isPatch":true,"sender":{"key":"larsxschneider@gmail.com","avatar":"https://avatars.githubusercontent.com/u/477434?v=4"},"body":"\n> On 17 Dec 2017, at 18:14, Torsten Bögershausen wrote:\n> \n> On Mon, Dec 11, 2017 at 04:50:23PM +0100, lars.schneider@autodesk.com wrote:\n>> From: Lars Schneider \n>> \n\n>> +`encoding`\n>> +^^^^^^^^^^\n>> +\n>> +By default Git assumes UTF-8 encoding for text files. The `encoding`\n> \n> This is probably \"ASCII\" and it's supersets like ISO-8859-1 or UTF-8.\n\nI am not sure I understand what you want to tell me here.\nDo you think UTF-8 encoding is not correct in the sentence above?\n\n> \n>> +attribute sets the encoding to be used in the working directory.\n>> +If the path is added to the index, then Git encodes the content to\n>> +UTF-8. On checkout the content is encoded back to the original\n>> +encoding. Consequently, you can use all built-in Git text processing\n>> +tools (e.g. git diff, line ending conversions, etc.) with your\n>> +non-UTF-8 encoded file.\n>> +\n>> +Please note that re-encoding content can cause errors and requires\n>> +resources. Use the `encoding` attribute only if you cannot store\n>> +a file in UTF-8 encoding and if you want Git to be able to process\n>> +the text.\n>> +\n>> +------------------------\n>> +*.txt\t\ttext encoding=UTF-16\n>> +------------------------\n> \n> I think that encoding (or worktree-encoding as said elsewhere) must imply text.\n> (That is not done in convert.c, but assuming binary or even auto\n> does not make much sense to me)\n\n\"text\" only means \"LF\". \"-text\" means CRLF which would be perfectly fine\nfor UTF-16. Therefore I don't think we need to imply text.\nDoes this make sense?\n\n> \n> \n>> +\n>> +All `iconv` encodings with a stable round-trip conversion to and from\n>> +UTF-8 are supported. You can see a full list with the following command:\n>> +\n>> +------------------------\n>> +iconv --list\n>> +------------------------\n>> +\n>> `eol`\n>> ^^^^^\n>> \n>> diff --git a/convert.c b/convert.c\n>> index 20d7ab67bd..ee19c17104 100644\n>> --- a/convert.c\n>> +++ b/convert.c\n>> @@ -7,6 +7,7 @@\n>> #include \"sigchain.h\"\n>> #include \"pkt-line.h\"\n>> #include \"sub-process.h\"\n>> +#include \"utf8.h\"\n>> \n>> /*\n>> * convert.c - convert a file when checking it out and checking it in.\n>> @@ -256,6 +257,149 @@ static int will_convert_lf_to_crlf(size_t len, struct text_stat *stats,\n>> \n>> }\n> \n> I would avoid to use these #ifdefs here.\n> All functions can be in strbuf.c (and may have #ifdefs there), but not here.\n\nI'll try that. But wouldn't it make more sense to move the functions to utf.c?\n\n> \n>> \n>> +#ifdef NO_ICONV\n>> +#ifndef _ICONV_T\n>> +/* The type is just a placeholder and not actually used. */\n>> +typedef void* iconv_t;\n>> +#endif\n>> +#endif\n>> +\n>> +static struct encoding {\n>> +\tconst char *name;\n>> +\ticonv_t to_git; /* conversion to Git's canonical encoding (UTF-8) */\n>> +\ticonv_t to_worktree; /* conversion to user-defined encoding */\n>> +\tstruct encoding *next;\n>> +} *encoding, **encoding_tail;\n> \n> This seems like an optimazation, assuning that iconv_open() eats a lot of ressources.\n> I don't think this is the case. (Undere MacOS we run iconv_open() together with\n> every opendir(), and optimizing this out did not show any measurable improvements)\n\nTrue, but then I would need to free() the memory in a lot of places.\nTherefore I thought it is easier to keep the object. OK for you?\n\n\n>> +static const char *default_encoding = \"utf-8\";\n> \n> The most portable form is \"UTF-8\" (correct me if that is wrong)\n\nIt shouldn't matter. But I've changed it to uppercase to be on the safe side.\n\n\n>> +static iconv_t invalid_conversion = (iconv_t)-1;\n>> +\n>> +static int encode_to_git(const char *path, const char *src, size_t src_len,\n>> +\t\t\t struct strbuf *buf, struct encoding *enc)\n>> +{\n>> +#ifndef NO_ICONV\n>> +\tchar *dst, *re_src;\n>> +\tint dst_len, re_src_len;\n>> +\n>> +\t/*\n>> +\t * No encoding is specified or there is nothing to encode.\n>> +\t * Tell the caller that the content was not modified.\n>> +\t */\n>> +\tif (!enc || (src && !src_len))\n>> +\t\treturn 0;\n>> +\n>> +\t/*\n>> +\t * Looks like we got called from \"would_convert_to_git()\".\n>> +\t * This means Git wants to know if it would encode (= modify!)\n>> +\t * the content. Let's answer with \"yes\", since an encoding was\n>> +\t * specified.\n>> +\t */\n>> +\tif (!buf && !src)\n>> +\t\treturn 1;\n>> +\n>> +\tif (enc->to_git == invalid_conversion) {\n>> +\t\tenc->to_git = iconv_open(default_encoding, encoding->name);\n>> +\t\tif (enc->to_git == invalid_conversion)\n>> +\t\t\twarning(_(\"unsupported encoding %s\"), encoding->name);\n>> +\t}\n> \n> \t/* There are 2 different types of reaction:\n> \t Either users know what that a warning means: You asked for problems,\n> \t \t and do the right thing. Other may may ignore the warning\n> \t\t\t - in both cases an error is useful */\n\nAgreed!\n\n\n>> +\tif (enc->to_worktree == invalid_conversion)\n>> +\t\tenc->to_worktree = iconv_open(encoding->name, default_encoding);\n>> +\n>> +\t/*\n>> +\t * Do nothing if the encoding is not supported. This could happen in\n>> +\t * two cases:\n>> +\t * (1) The encoding is garbage. That is no problem as we would not\n>> +\t * encode the content to UTF-8 on \"add\" and we would not encode\n>> +\t * it back on \"checkout\".\n>> +\t * (2) Git users use different iconv versions that support different\n>> +\t * encodings. This could lead to problems, as the content might\n>> +\t * not be encoded on \"add\" but encoded back on \"checkout\" (or\n>> +\t * the other way around).\n>> +\t * We print a one-time warning to the user in both cases above.\n>> +\t */\n> \n> Isn't an error better than \"garbage in -> garbage out\" ?\n\nAgreed. I changed the warning to an error.\n\n\n>> diff --git a/t/t0028-encoding.sh b/t/t0028-encoding.sh\n> \n> (I didn't review the test yet)\n> \n> Another comment for a possible improvement:\n> \"git diff\" could be told to write the worktree-encoding into the diff,\n> and \"git apply\" can pick that up. \n\nYes, we could do that. However, I would tackle that in a separate series.\n\n\nThanks,\nLars\n\n\n\n"},{"id":"335522","messageId":"20171229125954.GA9772@tor.lan","threadId":"47426","inReplyTo":"BF5C58AC-C1E6-4AD5-A4F8-C4CC5C9108F7@gmail.com","subject":"Re: [PATCH v1] convert: add support for 'encoding' attribute","fromName":"Torsten Bögershausen","fromEmail":"tboegi@web.de","sentAt":"2017-12-29T12:59:54Z","receivedAt":"2017-12-29T13:01:35Z","isPatch":true,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"On 2017-12-28 17:14, Lars Schneider wrote:> \n>> On 17 Dec 2017, at 18:14, Torsten Bögershausen wrote:\n>>\n>> On Mon, Dec 11, 2017 at 04:50:23PM +0100, lars.schneider@autodesk.com wrote:\n>>> From: Lars Schneider \n>>>\n> \n>>> +`encoding`\n>>> +^^^^^^^^^^\n>>> +\n>>> +By default Git assumes UTF-8 encoding for text files. The `encoding`\n>>\n>> This is probably \"ASCII\" and it's supersets like ISO-8859-1 or UTF-8.\n> \n> I am not sure I understand what you want to tell me here.\n> Do you think UTF-8 encoding is not correct in the sentence above?\n\nGit itself was designed to handle source code in ASCII.\nText files which are encoded in ISO-8859-1, UTF-8 or any\nsuper-set of ASCII are handled as well, and what exactly to do\nwith bytes >0x80 is outside the responsibility of Git.\nE.g. the interpretation and rendering on the screen may be\ndependent on the locale or being guessed.\nAll in all, saying that Git expects UTF-8 may be \"overdriven\".\nAny kind of file that uses an '\\n' as an end of line\nand has no NUL bytes '\\0' works as a text file in Git.\n(End of BlaBla ;-)\n\nWe can probably replace:\n\"By default Git assumes UTF-8 encoding for text files\"\n\nwith something like\n\"Git handles UTF-8 encoded files as text files, but UTF-16 encoded\n files are handled as binary files\"\n\n>>\n>>> +attribute sets the encoding to be used in the working directory.\n>>> +If the path is added to the index, then Git encodes the content to\n>>> +UTF-8. On checkout the content is encoded back to the original\n>>> +encoding. Consequently, you can use all built-in Git text processing\n>>> +tools (e.g. git diff, line ending conversions, etc.) with your\n>>> +non-UTF-8 encoded file.\n>>> +\n>>> +Please note that re-encoding content can cause errors and requires\n>>> +resources. Use the `encoding` attribute only if you cannot store\n>>> +a file in UTF-8 encoding and if you want Git to be able to process\n>>> +the text.\n>>> +\n>>> +------------------------\n>>> +*.txt\t\ttext encoding=UTF-16\n>>> +------------------------\n>>\n>> I think that encoding (or worktree-encoding as said elsewhere) must imply text.\n>> (That is not done in convert.c, but assuming binary or even auto\n>> does not make much sense to me)\n> \n> \"text\" only means \"LF\". \"-text\" means CRLF which would be perfectly fine\n> for UTF-16. Therefore I don't think we need to imply text.\n> Does this make sense?\nYes and now.\n\n\"text\" means convert CRLF at \"git add\" (or commit) into LF,\nAnd potentially LF into CRLF at checkout,\ndepending on the EOL attribute (if set), core.autocrlf and/or core.eol.\n\n\"-text\" means don't touch CRLF or LF at all. Files with CRLF are commited\nwith CRLF.\nRunning a Unix like \"diff\" tool will often show \"^M\" to indicate that there\nis a CR before the LF, which may be distracting or confusing.\n\nIf someone ever wants to handle the UTF-16 files on a Linux/Mac/Unix system,\nit makes sense to convert the line endings into LF before storing them\ninto the index (at least this is my experience).\n\n(Not specifying \"text\" or \"-text\" at all will trigger the auto-handling,\n which is not needed at all).\nSo if we want to be sure that line endings are CRLF in the worktree we\nshould ask the user to specify things like this:\n\n*.ext worktree-encoding=UTF-16 text eol=CRLF\n\nIt may be enough to give this example in the documentation.\nand I would rather be over-careful here, to avoid future problems.\n\n> \n>>\n>>\n>>> +\n>>> +All `iconv` encodings with a stable round-trip conversion to and from\n>>> +UTF-8 are supported. You can see a full list with the following command:\n>>> +\n>>> +------------------------\n>>> +iconv --list\n>>> +------------------------\n>>> +\n>>> `eol`\n>>> ^^^^^\n>>>\n>>> diff --git a/convert.c b/convert.c\n>>> index 20d7ab67bd..ee19c17104 100644\n>>> --- a/convert.c\n>>> +++ b/convert.c\n>>> @@ -7,6 +7,7 @@\n>>> #include \"sigchain.h\"\n>>> #include \"pkt-line.h\"\n>>> #include \"sub-process.h\"\n>>> +#include \"utf8.h\"\n>>>\n>>> /*\n>>> * convert.c - convert a file when checking it out and checking it in.\n>>> @@ -256,6 +257,149 @@ static int will_convert_lf_to_crlf(size_t len, struct text_stat *stats,\n>>>\n>>> }\n>>\n>> I would avoid to use these #ifdefs here.\n>> All functions can be in strbuf.c (and may have #ifdefs there), but not here.\n> \n> I'll try that. But wouldn't it make more sense to move the functions to utf.c?\n\nMay be.\nOriginally utf8.c was about encoding and all kind of UTF-8 related stuff.\nEspecially it didn't know anything about strbuf.\nI don't know why strbuf.h and other functions had been added here,\n\nI once moved them into strbuf.c without any problems, but never send out\na patch, because of possible merge conflicts in ongoing patches.\n\nIn any case, if it is about strbuf, I would try to put it into strbuf.c\n\n>>\n>>>\n>>> +#ifdef NO_ICONV\n>>> +#ifndef _ICONV_T\n>>> +/* The type is just a placeholder and not actually used. */\n>>> +typedef void* iconv_t;\n>>> +#endif\n>>> +#endif\n>>> +\n>>> +static struct encoding {\n>>> +\tconst char *name;\n>>> +\ticonv_t to_git; /* conversion to Git's canonical encoding (UTF-8) */\n>>> +\ticonv_t to_worktree; /* conversion to user-defined encoding */\n>>> +\tstruct encoding *next;\n>>> +} *encoding, **encoding_tail;\n>>\n>> This seems like an optimazation, assuning that iconv_open() eats a lot of ressources.\n>> I don't think this is the case. (Undere MacOS we run iconv_open() together with\n>> every opendir(), and optimizing this out did not show any measurable improvements)\n> \n> True, but then I would need to free() the memory in a lot of places.How many ?\n\n> Therefore I thought it is easier to keep the object. OK for you?\n\nMy first impression was that this optimization made the review hard to do.\nI would rather see a clean alloc/free open/close scheme in one patch.\nThis allows to follow the logic.\nAnd, if needed, an optimizing patch on top of that, saying now\nwe saved 30 lines of code.\n\n> \n> \n>>> +static const char *default_encoding = \"utf-8\";\n>>\n>> The most portable form is \"UTF-8\" (correct me if that is wrong)\n> \n> It shouldn't matter. But I've changed it to uppercase to be on the safe side.Good.\n\nFor the interested reader, see utf8.c:\nstatic const char *fallback_encoding(const char *name)\n{\n\t/*\n\t * Some platforms do not have the variously spelled variants of\n\t * UTF-8, so let's fall back to trying the most official\n\t * spelling. We do so only as a fallback in case the platform\n\t * does understand the user's spelling, but not our official\n\t * one.\n\t */\n\n \n> \n>>> +static iconv_t invalid_conversion = (iconv_t)-1;\n>>> +\n>>> +static int encode_to_git(const char *path, const char *src, size_t src_len,\n>>> +\t\t\t struct strbuf *buf, struct encoding *enc)\n>>> +{\n>>> +#ifndef NO_ICONV\n>>> +\tchar *dst, *re_src;\n>>> +\tint dst_len, re_src_len;\n>>> +\n>>> +\t/*\n>>> +\t * No encoding is specified or there is nothing to encode.\n>>> +\t * Tell the caller that the content was not modified.\n>>> +\t */\n>>> +\tif (!enc || (src && !src_len))\n>>> +\t\treturn 0;\n>>> +\n>>> +\t/*\n>>> +\t * Looks like we got called from \"would_convert_to_git()\".\n>>> +\t * This means Git wants to know if it would encode (= modify!)\n>>> +\t * the content. Let's answer with \"yes\", since an encoding was\n>>> +\t * specified.\n>>> +\t */\n>>> +\tif (!buf && !src)\n>>> +\t\treturn 1;\n>>> +\n>>> +\tif (enc->to_git == invalid_conversion) {\n>>> +\t\tenc->to_git = iconv_open(default_encoding, encoding->name);\n>>> +\t\tif (enc->to_git == invalid_conversion)\n>>> +\t\t\twarning(_(\"unsupported encoding %s\"), encoding->name);\n>>> +\t}\n>>\n>> \t/* There are 2 different types of reaction:\n>> \t Either users know what that a warning means: You asked for problems,\n>> \t \t and do the right thing. Other may may ignore the warning\n>> \t\t\t - in both cases an error is useful */\n> \n> Agreed!\n> \n> \n>>> +\tif (enc->to_worktree == invalid_conversion)\n>>> +\t\tenc->to_worktree = iconv_open(encoding->name, default_encoding);\n>>> +\n>>> +\t/*\n>>> +\t * Do nothing if the encoding is not supported. This could happen in\n>>> +\t * two cases:\n>>> +\t * (1) The encoding is garbage. That is no problem as we would not\n>>> +\t * encode the content to UTF-8 on \"add\" and we would not encode\n>>> +\t * it back on \"checkout\".\n>>> +\t * (2) Git users use different iconv versions that support different\n>>> +\t * encodings. This could lead to problems, as the content might\n>>> +\t * not be encoded on \"add\" but encoded back on \"checkout\" (or\n>>> +\t * the other way around).\n>>> +\t * We print a one-time warning to the user in both cases above.\n>>> +\t */\n>>\n>> Isn't an error better than \"garbage in -> garbage out\" ?\n> \n> Agreed. I changed the warning to an error.\n> \n> \n>>> diff --git a/t/t0028-encoding.sh b/t/t0028-encoding.sh\n>>\n>> (I didn't review the test yet)\n>>\n>> Another comment for a possible improvement:\n>> \"git diff\" could be told to write the worktree-encoding into the diff,\n>> and \"git apply\" can pick that up. \n> \n> Yes, we could do that. However, I would tackle that in a separate series.\n> \n> \n> Thanks,\n> Lars\n> \n\n\n"},{"id":"335524","messageId":"20171229132824.17551-1-tboegi@web.de","threadId":"47426","inReplyTo":"20171218131249.GB4665@sigill.intra.peff.net","subject":"[PATCH/RFC 0/2] git diff --UTF-8","fromName":"","fromEmail":"tboegi@web.de","sentAt":"2017-12-29T13:28:24Z","receivedAt":"2017-12-29T13:28:48Z","isPatch":true,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"From: Torsten Bögershausen \n\nRFC patch: convert files from e.g. UTF-16 into UTF-8 while running\n\"git diff\".\nThe diff must be called with \"git diff --UTF-8\" and the \"encoding\"\nattribute must be set for the file(s).\n\nThe commit messages may need some improvements, and a closer look\nat diff.c, how command line options are forwared, is appreciated.\n\nIt may even be possible to integrate t4066 somewhere...\n\nTorsten Bögershausen (2):\n convert_to_git(): checksafe becomes an integer\n git diff: Allow to reencode into UTF-8\n\n Documentation/diff-options.txt | 4 ++\n Documentation/gitattributes.txt | 9 +++++\n apply.c | 4 +-\n convert.c | 60 +++++++++++++++++++++++-----\n convert.h | 20 +++++-----\n diff.c | 40 +++++++++++++++++--\n diff.h | 1 +\n diffcore.h | 3 ++\n environment.c | 2 +-\n sha1_file.c | 6 +--\n t/t4066-diff-encoding.sh | 86 +++++++++++++++++++++++++++++++++++++++++\n 11 files changed, 205 insertions(+), 30 deletions(-)\n create mode 100755 t/t4066-diff-encoding.sh\n\n-- \n2.15.1.271.g1a4e40aa5d\n\n"},{"id":"335525","messageId":"20171229132829.17637-1-tboegi@web.de","threadId":"47426","inReplyTo":"20171218131249.GB4665@sigill.intra.peff.net","subject":"[PATCH/RFC 2/2] git diff: Allow to reencode into UTF-8","fromName":"","fromEmail":"tboegi@web.de","sentAt":"2017-12-29T13:28:29Z","receivedAt":"2017-12-29T13:28:50Z","isPatch":true,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"From: Torsten Bögershausen \n\nWhen blobs are encoded in UTF-16, `git diff` will treat them as binary.\nMake it possible to show a user readable diff encoded in UTF-8.\nThis allows to run git diff and feed the into a web sever.\n\nImprove Git to look at the \"encodig\" attribute and to reencode the\ncontent into UTF-8 before running the diff itself.\n\nSigned-off-by: Torsten Bögershausen \n---\n Documentation/diff-options.txt | 4 ++\n Documentation/gitattributes.txt | 9 +++++\n convert.c | 40 +++++++++++++++++++\n convert.h | 2 +\n diff.c | 38 ++++++++++++++++--\n diff.h | 1 +\n diffcore.h | 3 ++\n t/t4066-diff-encoding.sh | 86 +++++++++++++++++++++++++++++++++++++++++\n 8 files changed, 180 insertions(+), 3 deletions(-)\n create mode 100755 t/t4066-diff-encoding.sh\n\ndiff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt\nindex 9d1586b956..bf2f115f11 100644\n--- a/Documentation/diff-options.txt\n+++ b/Documentation/diff-options.txt\n@@ -629,6 +629,10 @@ endif::git-format-patch[]\n \tlinkgit:git-log[1], but not for linkgit:git-format-patch[1] or\n \tdiff plumbing commands.\n \n+--UTF-8::\n+\tGit converts the content into UTF-8 before running the diff when the\n+\t\"encoding\" attribute is defined. See linkgit:gitattributes[5]\n+\n --ignore-submodules[=]::\n \tIgnore changes to submodules in the diff generation. can be\n \teither \"none\", \"untracked\", \"dirty\" or \"all\", which is the default.\ndiff --git a/Documentation/gitattributes.txt b/Documentation/gitattributes.txt\nindex 30687de81a..753a7c39b7 100644\n--- a/Documentation/gitattributes.txt\n+++ b/Documentation/gitattributes.txt\n@@ -881,6 +881,15 @@ advantages to choosing this method:\n 3. Caching. Textconv caching can speed up repeated diffs, such as those\n you might trigger by running `git log -p`.\n \n+Running diff on UTF-16 encoded files\n+^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^\n+\n+Git can convert UTF-16 encoded into UTF-8 before they are feed\n+into the diff machinery: `diff --UTF-8 file.xxx`.\n+\n+------------------------\n+file.xxx encoding=UTF-16\n+------------------------\n \n Marking files as binary\n ^^^^^^^^^^^^^^^^^^^^^^^\ndiff --git a/convert.c b/convert.c\nindex 5efcc3b73b..45577ce504 100644\n--- a/convert.c\n+++ b/convert.c\n@@ -7,6 +7,7 @@\n #include \"sigchain.h\"\n #include \"pkt-line.h\"\n #include \"sub-process.h\"\n+#include \"utf8.h\"\n \n /*\n * convert.c - convert a file when checking it out and checking it in.\n@@ -734,6 +735,34 @@ static struct convert_driver {\n \tint required;\n } *user_convert, **user_convert_tail;\n \n+const char *get_encoding_attr(const char *path)\n+{\n+\tstatic struct attr_check *check;\n+\tif (!check)\n+\t\tcheck = attr_check_initl(\"encoding\", NULL);\n+\tif (!git_check_attr(path, check)) {\n+\t\tstruct attr_check_item *ccheck = check->items;\n+\t\tconst char *value;\n+\t\tvalue = ccheck->value;\n+\t\tif (ATTR_UNSET(value))\n+\t\t\treturn NULL;\n+\t\treturn value;\n+\t}\n+\treturn NULL;\n+}\n+\n+static int reencode_into_strbuf(const char *path, const char *src, size_t len,\n+\t\t\t\tstruct strbuf *dst, const char *encoding)\n+{\n+\tint outsz = 0;\n+\tchar *buf;\n+\tbuf = reencode_string_len(src, (int)len, \"UTF-8\", encoding, &outsz);\n+\tif (!buf)\n+\t\treturn 0;\n+\tstrbuf_attach(dst, buf, outsz, outsz);\n+\treturn SAFE_CRLF_REENCODE;\n+}\n+\n static int apply_filter(const char *path, const char *src, size_t len,\n \t\t\tint fd, struct strbuf *dst, struct convert_driver *drv,\n \t\t\tconst unsigned int wanted_capability,\n@@ -1136,6 +1165,17 @@ int convert_to_git(const struct index_state *istate,\n \n \tconvert_attrs(&ca, path);\n \n+\tif (checksafe & SAFE_CRLF_REENCODE) {\n+\t\tconst char *encoding = get_encoding_attr(path);\n+\t\tif (encoding) {\n+\t\t\tret |= reencode_into_strbuf(path, src, len, dst,\n+\t\t\t\t\t\t encoding);\n+\t\t\tif (ret && dst) {\n+\t\t\t\tsrc = dst->buf;\n+\t\t\t\tlen = dst->len;\n+\t\t\t}\n+\t\t}\n+\t}\n \tret |= apply_filter(path, src, len, -1, dst, ca.drv, CAP_CLEAN, NULL);\n \tif (!ret && ca.drv && ca.drv->required)\n \t\tdie(\"%s: clean filter '%s' failed\", path, ca.drv->name);\ndiff --git a/convert.h b/convert.h\nindex 532af00423..0b093715c9 100644\n--- a/convert.h\n+++ b/convert.h\n@@ -13,6 +13,7 @@ struct index_state;\n #define SAFE_CRLF_WARN (1<<1)\n #define SAFE_CRLF_RENORMALIZE (1<<2)\n #define SAFE_CRLF_KEEP_CRLF (1<<3)\n+#define SAFE_CRLF_REENCODE (1<<4)\n \n extern int safe_crlf;\n \n@@ -60,6 +61,7 @@ extern const char *get_cached_convert_stats_ascii(const struct index_state *ista\n \t\t\t\t\t\t const char *path);\n extern const char *get_wt_convert_stats_ascii(const char *path);\n extern const char *get_convert_attr_ascii(const char *path);\n+extern const char *get_encoding_attr(const char *path);\n \n /* returns 1 if *dst was used */\n extern int convert_to_git(const struct index_state *istate,\ndiff --git a/diff.c b/diff.c\nindex 5e3aaea6e0..07480a465c 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -3191,6 +3191,12 @@ static void builtin_diff(const char *name_a,\n \t\t\t\t\t header.buf, header.len, 0);\n \t\t\tstrbuf_reset(&header);\n \t\t}\n+\t\tif (one && one->reencoded_to_utf8)\n+\t\t strbuf_addf(&header, \"a is converted to UTF-8 from %s\\n\",\n+\t\t\t get_encoding_attr(one->path));\n+\t\tif (two && two->reencoded_to_utf8)\n+\t\t strbuf_addf(&header, \"b is converted to UTF-8 from %s\\n\",\n+\t\t\t get_encoding_attr(two->path));\n \n \t\tmf1.size = fill_textconv(textconv_one, one, &mf1.ptr);\n \t\tmf2.size = fill_textconv(textconv_two, two, &mf2.ptr);\n@@ -3520,6 +3526,7 @@ int diff_populate_filespec(struct diff_filespec *s, unsigned int flags)\n {\n \tint size_only = flags & CHECK_SIZE_ONLY;\n \tint err = 0;\n+\tint ret = 0;\n \t/*\n \t * demote FAIL to WARN to allow inspecting the situation\n \t * instead of refusing.\n@@ -3527,7 +3534,8 @@ int diff_populate_filespec(struct diff_filespec *s, unsigned int flags)\n \tint checksafe = (safe_crlf == SAFE_CRLF_FAIL\n \t\t\t\t ? SAFE_CRLF_WARN\n \t\t\t\t : safe_crlf);\n-\n+\tif (s->reencode_to_utf8)\n+\t\tchecksafe |= SAFE_CRLF_REENCODE;\n \tif (!DIFF_FILE_VALID(s))\n \t\tdie(\"internal error: asking to populate invalid file.\");\n \tif (S_ISDIR(s->mode))\n@@ -3603,17 +3611,22 @@ int diff_populate_filespec(struct diff_filespec *s, unsigned int flags)\n \t\t/*\n \t\t * Convert from working tree format to canonical git format\n \t\t */\n-\t\tif (convert_to_git(&the_index, s->path, s->data, s->size, &buf, checksafe)) {\n+\t\tret = convert_to_git(&the_index, s->path, s->data, s->size, &buf, checksafe);\n+\n+\t\tif (ret) {\n \t\t\tsize_t size = 0;\n \t\t\tmunmap(s->data, s->size);\n \t\t\ts->should_munmap = 0;\n \t\t\ts->data = strbuf_detach(&buf, &size);\n \t\t\ts->size = size;\n \t\t\ts->should_free = 1;\n+\t\t\tif (ret & SAFE_CRLF_REENCODE)\n+\t\t\t\t s->reencoded_to_utf8 = 1;\n \t\t}\n \t}\n \telse {\n \t\tenum object_type type;\n+\t\tconst char *encoding = NULL;\n \t\tif (size_only || (flags & CHECK_BINARY)) {\n \t\t\ttype = sha1_object_info(s->oid.hash, &s->size);\n \t\t\tif (type < 0)\n@@ -3629,6 +3642,20 @@ int diff_populate_filespec(struct diff_filespec *s, unsigned int flags)\n \t\ts->data = read_sha1_file(s->oid.hash, &type, &s->size);\n \t\tif (!s->data)\n \t\t\tdie(\"unable to read %s\", oid_to_hex(&s->oid));\n+\t\tif (s->reencode_to_utf8)\n+\t\t\tencoding = get_encoding_attr(s->path);\n+\t\tif (encoding) {\n+\t\t\tint outsz = 0;\n+\t\t\tchar *buf;\n+\t\t\tbuf = reencode_string_len(s->data, (int)s->size,\n+\t\t\t\t\t\t \"UTF-8\", encoding, &outsz);\n+\t\t\tif (buf) {\n+\t\t\t\tfree(s->data);\n+\t\t\t\ts->data = buf;\n+\t\t\t\ts->size = outsz;\n+\t\t\t\ts->reencoded_to_utf8 = 1;\n+\t\t\t}\n+\t\t}\n \t\ts->should_free = 1;\n \t}\n \treturn 0;\n@@ -4627,7 +4654,9 @@ int diff_opt_parse(struct diff_options *options,\n \t\tenable_patch_output(&options->output_format);\n \t\toptions->flags.binary = 1;\n \t}\n-\telse if (!strcmp(arg, \"--full-index\"))\n+\telse if (!strcmp(arg, \"--UTF-8\")) {\n+\t\toptions->flags.reencode_to_utf8 = 1;\n+\t} else if (!strcmp(arg, \"--full-index\"))\n \t\toptions->flags.full_index = 1;\n \telse if (!strcmp(arg, \"-a\") || !strcmp(arg, \"--text\"))\n \t\toptions->flags.text = 1;\n@@ -5695,6 +5724,8 @@ static int diff_filespec_is_identical(struct diff_filespec *one,\n \n static int diff_filespec_check_stat_unmatch(struct diff_filepair *p)\n {\n+\tp->one->reencode_to_utf8 = p->reencode_to_utf8;\n+\tp->two->reencode_to_utf8 = p->reencode_to_utf8;\n \tif (p->done_skip_stat_unmatch)\n \t\treturn p->skip_stat_unmatch_result;\n \n@@ -5735,6 +5766,7 @@ static void diffcore_skip_stat_unmatch(struct diff_options *diffopt)\n \tfor (i = 0; i < q->nr; i++) {\n \t\tstruct diff_filepair *p = q->queue[i];\n \n+\t\tp->reencode_to_utf8 = diffopt->flags.reencode_to_utf8;\n \t\tif (diff_filespec_check_stat_unmatch(p))\n \t\t\tdiff_q(&outq, p);\n \t\telse {\ndiff --git a/diff.h b/diff.h\nindex 7cf276f077..d2137bab58 100644\n--- a/diff.h\n+++ b/diff.h\n@@ -65,6 +65,7 @@ struct diff_flags {\n \tunsigned recursive:1;\n \tunsigned tree_in_recursive:1;\n \tunsigned binary:1;\n+\tunsigned reencode_to_utf8:1;\n \tunsigned text:1;\n \tunsigned full_index:1;\n \tunsigned silent_on_remove:1;\ndiff --git a/diffcore.h b/diffcore.h\nindex a30da161da..2e84730778 100644\n--- a/diffcore.h\n+++ b/diffcore.h\n@@ -47,6 +47,8 @@ struct diff_filespec {\n \tunsigned has_more_entries : 1; /* only appear in combined diff */\n \t/* data should be considered \"binary\"; -1 means \"don't know yet\" */\n \tsigned int is_binary : 2;\n+\tunsigned reencode_to_utf8 : 1;\n+\tunsigned reencoded_to_utf8 : 1;\n \tstruct userdiff_driver *driver;\n };\n \n@@ -72,6 +74,7 @@ struct diff_filepair {\n \tunsigned is_unmerged : 1;\n \tunsigned done_skip_stat_unmatch : 1;\n \tunsigned skip_stat_unmatch_result : 1;\n+\tunsigned reencode_to_utf8 : 1;\n };\n #define DIFF_PAIR_UNMERGED(p) ((p)->is_unmerged)\n \ndiff --git a/t/t4066-diff-encoding.sh b/t/t4066-diff-encoding.sh\nnew file mode 100755\nindex 0000000000..9b89253877\n--- /dev/null\n+++ b/t/t4066-diff-encoding.sh\n@@ -0,0 +1,86 @@\n+#!/bin/sh\n+\n+test_description='git diff with encoding attribute'\n+\n+. ./test-lib.sh\n+\n+printf '\\303\\244rger\\n\\303\\266se\\n\\303\\274bel\\n' |\n+\ticonv -f UTF-8 -t UTF-16 >UTF-16\n+printf '\\303\\266se\\n\\303\\274bel\\n\\303\\245gren\\n' |\n+\ticonv -f UTF-8 -t UTF-16 >file2\n+\n+test_expect_success 'setup' '\n+\tcp UTF-16 file &&\n+\tgit add file &&\n+\tgit commit -m \"add file in UTF-16\" &&\n+\ttest_tick &&\n+\techo \"file encoding=UTF-16\" >.gitattributes\n+'\n+\n+test_expect_success 'diff --UTF-8 against local change' '\n+\tcp file2 file &&\n+\ttest_tick &&\n+\tcat >expect <<-\\EOF &&\n+\tdiff --git a/file b/file\n+\tindex 26acf09..06d06e4 100644\n+\ta is converted to UTF-8 from UTF-16\n+\tb is converted to UTF-8 from UTF-16\n+\t--- a/file\n+\t+++ b/file\n+\t@@ -1,3 +1,3 @@\n+\t-ärger\n+\t öse\n+\t übel\n+\t+ågren\n+EOF\n+\tgit diff --UTF-8 file >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'diff against local change' '\n+\tcp file2 file &&\n+\ttest_tick &&\n+\tcat >expect <<-\\EOF &&\n+\tdiff --git a/file b/file\n+\tindex 26acf09..06d06e4 100644\n+\tBinary files a/file and b/file differ\n+EOF\n+\tgit diff file >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'commit local change' '\n+\tgit add file &&\n+\tgit commit -m \"add file V2 in UTF-16\" &&\n+\ttest_tick\n+'\n+\n+test_expect_success 'diff --UTF-8 HEAD against HEAD^' '\n+\tcat >expect <<-\\EOF &&\n+\tdiff --git a/file b/file\n+\tindex 26acf09..06d06e4 100644\n+\ta is converted to UTF-8 from UTF-16\n+\tb is converted to UTF-8 from UTF-16\n+\t--- a/file\n+\t+++ b/file\n+\t@@ -1,3 +1,3 @@\n+\t-ärger\n+\t öse\n+\t übel\n+\t+ågren\n+EOF\n+\tgit diff --UTF-8 HEAD^ HEAD -- file >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'diff HEAD against HEAD^' '\n+\tcat >expect <<-\\EOF &&\n+\tdiff --git a/file b/file\n+\tindex 26acf09..06d06e4 100644\n+\tBinary files a/file and b/file differ\n+EOF\n+\tgit diff HEAD^ HEAD -- file >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_done\n-- \n2.15.1.271.g1a4e40aa5d\n\n"},{"id":"335526","messageId":"20171229132828.17594-1-tboegi@web.de","threadId":"47426","inReplyTo":"20171218131249.GB4665@sigill.intra.peff.net","subject":"[PATCH/RFC 1/2] convert_to_git(): checksafe becomes an integer","fromName":"","fromEmail":"tboegi@web.de","sentAt":"2017-12-29T13:28:28Z","receivedAt":"2017-12-29T13:28:53Z","isPatch":true,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"From: Torsten Bögershausen \n\nWhen calling convert_to_git(), the checksafe parameter has been used to\ncheck if commit would give a non-roundtrip conversion of EOL.\n\nWhen checksafe was introduced, 3 values had been in use:\nSAFE_CRLF_FALSE: no warning\nSAFE_CRLF_FAIL: reject the commit if EOL do not roundtrip\nSAFE_CRLF_WARN: warn the user if EOL do not roundtrip\n\nToday a small flaw is found in the code base:\nAn integer with the value 0 is passed as the parameter checksafe\ninstead of the correct enum value SAFE_CRLF_FALSE.\n\nIn the next commit there is a need to turn checksafe into a bitmap, which\nallows to tell convert_to_git() to obey the encoding attribute or not.\n\nSigned-off-by: Torsten Bögershausen \n---\n apply.c | 4 ++--\n convert.c | 20 ++++++++++----------\n convert.h | 18 ++++++++----------\n diff.c | 4 ++--\n environment.c | 2 +-\n sha1_file.c | 6 +++---\n 6 files changed, 26 insertions(+), 28 deletions(-)\n\ndiff --git a/apply.c b/apply.c\nindex 321a9fa68d..a422516062 100644\n--- a/apply.c\n+++ b/apply.c\n@@ -2263,7 +2263,7 @@ static void show_stats(struct apply_state *state, struct patch *patch)\n static int read_old_data(struct stat *st, struct patch *patch,\n \t\t\t const char *path, struct strbuf *buf)\n {\n-\tenum safe_crlf safe_crlf = patch->crlf_in_old ?\n+\tint checksafe = patch->crlf_in_old ?\n \t\tSAFE_CRLF_KEEP_CRLF : SAFE_CRLF_RENORMALIZE;\n \tswitch (st->st_mode & S_IFMT) {\n \tcase S_IFLNK:\n@@ -2281,7 +2281,7 @@ static int read_old_data(struct stat *st, struct patch *patch,\n \t\t * should never look at the index when explicit crlf option\n \t\t * is given.\n \t\t */\n-\t\tconvert_to_git(NULL, path, buf->buf, buf->len, buf, safe_crlf);\n+\t\tconvert_to_git(NULL, path, buf->buf, buf->len, buf, checksafe);\n \t\treturn 0;\n \tdefault:\n \t\treturn -1;\ndiff --git a/convert.c b/convert.c\nindex 1a41a48e15..5efcc3b73b 100644\n--- a/convert.c\n+++ b/convert.c\n@@ -195,13 +195,13 @@ static enum eol output_eol(enum crlf_action crlf_action)\n \n static void check_safe_crlf(const char *path, enum crlf_action crlf_action,\n \t\t\t struct text_stat *old_stats, struct text_stat *new_stats,\n-\t\t\t enum safe_crlf checksafe)\n+\t\t\t int checksafe)\n {\n \tif (old_stats->crlf && !new_stats->crlf ) {\n \t\t/*\n \t\t * CRLFs would not be restored by checkout\n \t\t */\n-\t\tif (checksafe == SAFE_CRLF_WARN)\n+\t\tif (checksafe & SAFE_CRLF_WARN)\n \t\t\twarning(_(\"CRLF will be replaced by LF in %s.\\n\"\n \t\t\t\t \"The file will have its original line\"\n \t\t\t\t \" endings in your working directory.\"), path);\n@@ -211,7 +211,7 @@ static void check_safe_crlf(const char *path, enum crlf_action crlf_action,\n \t\t/*\n \t\t * CRLFs would be added by checkout\n \t\t */\n-\t\tif (checksafe == SAFE_CRLF_WARN)\n+\t\tif (checksafe & SAFE_CRLF_WARN)\n \t\t\twarning(_(\"LF will be replaced by CRLF in %s.\\n\"\n \t\t\t\t \"The file will have its original line\"\n \t\t\t\t \" endings in your working directory.\"), path);\n@@ -268,7 +268,7 @@ static int will_convert_lf_to_crlf(size_t len, struct text_stat *stats,\n static int crlf_to_git(const struct index_state *istate,\n \t\t const char *path, const char *src, size_t len,\n \t\t struct strbuf *buf,\n-\t\t enum crlf_action crlf_action, enum safe_crlf checksafe)\n+\t\t enum crlf_action crlf_action, int checksafe)\n {\n \tstruct text_stat stats;\n \tchar *dst;\n@@ -298,12 +298,12 @@ static int crlf_to_git(const struct index_state *istate,\n \t\t * unless we want to renormalize in a merge or\n \t\t * cherry-pick.\n \t\t */\n-\t\tif ((checksafe != SAFE_CRLF_RENORMALIZE) &&\n+\t\tif ((!(checksafe & SAFE_CRLF_RENORMALIZE)) &&\n \t\t has_crlf_in_index(istate, path))\n \t\t\tconvert_crlf_into_lf = 0;\n \t}\n-\tif ((checksafe == SAFE_CRLF_WARN ||\n-\t (checksafe == SAFE_CRLF_FAIL)) && len) {\n+\tif (((checksafe & SAFE_CRLF_WARN) ||\n+\t ((checksafe & SAFE_CRLF_FAIL) && len))) {\n \t\tstruct text_stat new_stats;\n \t\tmemcpy(&new_stats, &stats, sizeof(new_stats));\n \t\t/* simulate \"git add\" */\n@@ -1129,7 +1129,7 @@ const char *get_convert_attr_ascii(const char *path)\n \n int convert_to_git(const struct index_state *istate,\n \t\t const char *path, const char *src, size_t len,\n- struct strbuf *dst, enum safe_crlf checksafe)\n+\t\t struct strbuf *dst, int checksafe)\n {\n \tint ret = 0;\n \tstruct conv_attrs ca;\n@@ -1144,7 +1144,7 @@ int convert_to_git(const struct index_state *istate,\n \t\tsrc = dst->buf;\n \t\tlen = dst->len;\n \t}\n-\tif (checksafe != SAFE_CRLF_KEEP_CRLF) {\n+\tif (!(checksafe & SAFE_CRLF_KEEP_CRLF)) {\n \t\tret |= crlf_to_git(istate, path, src, len, dst, ca.crlf_action, checksafe);\n \t\tif (ret && dst) {\n \t\t\tsrc = dst->buf;\n@@ -1156,7 +1156,7 @@ int convert_to_git(const struct index_state *istate,\n \n void convert_to_git_filter_fd(const struct index_state *istate,\n \t\t\t const char *path, int fd, struct strbuf *dst,\n-\t\t\t enum safe_crlf checksafe)\n+\t\t\t int checksafe)\n {\n \tstruct conv_attrs ca;\n \tconvert_attrs(&ca, path);\ndiff --git a/convert.h b/convert.h\nindex 4f2da225a8..532af00423 100644\n--- a/convert.h\n+++ b/convert.h\n@@ -8,15 +8,13 @@\n \n struct index_state;\n \n-enum safe_crlf {\n-\tSAFE_CRLF_FALSE = 0,\n-\tSAFE_CRLF_FAIL = 1,\n-\tSAFE_CRLF_WARN = 2,\n-\tSAFE_CRLF_RENORMALIZE = 3,\n-\tSAFE_CRLF_KEEP_CRLF = 4\n-};\n+#define SAFE_CRLF_FALSE 0\n+#define SAFE_CRLF_FAIL (1<<0)\n+#define SAFE_CRLF_WARN (1<<1)\n+#define SAFE_CRLF_RENORMALIZE (1<<2)\n+#define SAFE_CRLF_KEEP_CRLF (1<<3)\n \n-extern enum safe_crlf safe_crlf;\n+extern int safe_crlf;\n \n enum auto_crlf {\n \tAUTO_CRLF_FALSE = 0,\n@@ -66,7 +64,7 @@ extern const char *get_convert_attr_ascii(const char *path);\n /* returns 1 if *dst was used */\n extern int convert_to_git(const struct index_state *istate,\n \t\t\t const char *path, const char *src, size_t len,\n-\t\t\t struct strbuf *dst, enum safe_crlf checksafe);\n+\t\t\t struct strbuf *dst, int checksafe);\n extern int convert_to_working_tree(const char *path, const char *src,\n \t\t\t\t size_t len, struct strbuf *dst);\n extern int async_convert_to_working_tree(const char *path, const char *src,\n@@ -85,7 +83,7 @@ static inline int would_convert_to_git(const struct index_state *istate,\n extern void convert_to_git_filter_fd(const struct index_state *istate,\n \t\t\t\t const char *path, int fd,\n \t\t\t\t struct strbuf *dst,\n-\t\t\t\t enum safe_crlf checksafe);\n+\t\t\t\t int checksafe);\n extern int would_convert_to_git_filter_fd(const char *path);\n \n /*****************************************************************\ndiff --git a/diff.c b/diff.c\nindex fb22b19f09..5e3aaea6e0 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -3524,7 +3524,7 @@ int diff_populate_filespec(struct diff_filespec *s, unsigned int flags)\n \t * demote FAIL to WARN to allow inspecting the situation\n \t * instead of refusing.\n \t */\n-\tenum safe_crlf crlf_warn = (safe_crlf == SAFE_CRLF_FAIL\n+\tint checksafe = (safe_crlf == SAFE_CRLF_FAIL\n \t\t\t\t ? SAFE_CRLF_WARN\n \t\t\t\t : safe_crlf);\n \n@@ -3603,7 +3603,7 @@ int diff_populate_filespec(struct diff_filespec *s, unsigned int flags)\n \t\t/*\n \t\t * Convert from working tree format to canonical git format\n \t\t */\n-\t\tif (convert_to_git(&the_index, s->path, s->data, s->size, &buf, crlf_warn)) {\n+\t\tif (convert_to_git(&the_index, s->path, s->data, s->size, &buf, checksafe)) {\n \t\t\tsize_t size = 0;\n \t\t\tmunmap(s->data, s->size);\n \t\t\ts->should_munmap = 0;\ndiff --git a/environment.c b/environment.c\nindex 63ac38a46f..10ee89a28d 100644\n--- a/environment.c\n+++ b/environment.c\n@@ -49,7 +49,7 @@ enum auto_crlf auto_crlf = AUTO_CRLF_FALSE;\n int check_replace_refs = 1;\n char *git_replace_ref_base;\n enum eol core_eol = EOL_UNSET;\n-enum safe_crlf safe_crlf = SAFE_CRLF_WARN;\n+int safe_crlf = SAFE_CRLF_WARN;\n unsigned whitespace_rule_cfg = WS_DEFAULT_RULE;\n enum branch_track git_branch_track = BRANCH_TRACK_REMOTE;\n enum rebase_setup_type autorebase = AUTOREBASE_NEVER;\ndiff --git a/sha1_file.c b/sha1_file.c\nindex 3da70ac650..78e002392e 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -133,7 +133,7 @@ static struct cached_object *find_cached_object(const unsigned char *sha1)\n }\n \n \n-static enum safe_crlf get_safe_crlf(unsigned flags)\n+static int get_checksafe(unsigned flags)\n {\n \tif (flags & HASH_RENORMALIZE)\n \t\treturn SAFE_CRLF_RENORMALIZE;\n@@ -1752,7 +1752,7 @@ static int index_mem(struct object_id *oid, void *buf, size_t size,\n \tif ((type == OBJ_BLOB) && path) {\n \t\tstruct strbuf nbuf = STRBUF_INIT;\n \t\tif (convert_to_git(&the_index, path, buf, size, &nbuf,\n-\t\t\t\t get_safe_crlf(flags))) {\n+\t\t\t\t get_checksafe(flags))) {\n \t\t\tbuf = strbuf_detach(&nbuf, &size);\n \t\t\tre_allocated = 1;\n \t\t}\n@@ -1786,7 +1786,7 @@ static int index_stream_convert_blob(struct object_id *oid, int fd,\n \tassert(would_convert_to_git_filter_fd(path));\n \n \tconvert_to_git_filter_fd(&the_index, path, fd, &sbuf,\n-\t\t\t\t get_safe_crlf(flags));\n+\t\t\t\t get_checksafe(flags));\n \n \tif (write_object)\n \t\tret = write_sha1_file(sbuf.buf, sbuf.len, typename(OBJ_BLOB),\n-- \n2.15.1.271.g1a4e40aa5d\n\n"},{"id":"335528","messageId":"2ADD036C-6714-4DE5-A290-4C76EFACE166@gmail.com","threadId":"47426","inReplyTo":"20171229125954.GA9772@tor.lan","subject":"Re: [PATCH v1] convert: add support for 'encoding' attribute","fromName":"Lars Schneider","fromEmail":"larsxschneider@gmail.com","sentAt":"2017-12-29T13:56:17Z","receivedAt":"2017-12-29T13:56:26Z","isPatch":true,"sender":{"key":"larsxschneider@gmail.com","avatar":"https://avatars.githubusercontent.com/u/477434?v=4"},"body":"\n> On 29 Dec 2017, at 13:59, Torsten Bögershausen wrote:\n> \n> On 2017-12-28 17:14, Lars Schneider wrote:> \n>>> On 17 Dec 2017, at 18:14, Torsten Bögershausen wrote:\n>>> \n>>> On Mon, Dec 11, 2017 at 04:50:23PM +0100, lars.schneider@autodesk.com wrote:\n>>>> From: Lars Schneider \n>>>> \n>> \n>>>> +`encoding`\n>>>> +^^^^^^^^^^\n>>>> +\n>>>> +By default Git assumes UTF-8 encoding for text files. The `encoding`\n>>> \n>>> This is probably \"ASCII\" and it's supersets like ISO-8859-1 or UTF-8.\n>> \n>> I am not sure I understand what you want to tell me here.\n>> Do you think UTF-8 encoding is not correct in the sentence above?\n> \n> Git itself was designed to handle source code in ASCII.\n> Text files which are encoded in ISO-8859-1, UTF-8 or any\n> super-set of ASCII are handled as well, and what exactly to do\n> with bytes >0x80 is outside the responsibility of Git.\n> E.g. the interpretation and rendering on the screen may be\n> dependent on the locale or being guessed.\n> All in all, saying that Git expects UTF-8 may be \"overdriven\".\n> Any kind of file that uses an '\\n' as an end of line\n> and has no NUL bytes '\\0' works as a text file in Git.\n> (End of BlaBla ;-)\n> \n> We can probably replace:\n> \"By default Git assumes UTF-8 encoding for text files\"\n> \n> with something like\n> \"Git handles UTF-8 encoded files as text files, but UTF-16 encoded\n> files are handled as binary files\"\n\nWell, UTF-32 are handled as binary file, too :-)\n\nHow about this?\n\n Git recognizes files encoded with ASCII or one of its supersets\n (e.g. UTF-8 or ISO-8859-1) as text files. All other...\n\n\n> \n>>> \n>>>> +attribute sets the encoding to be used in the working directory.\n>>>> +If the path is added to the index, then Git encodes the content to\n>>>> +UTF-8. On checkout the content is encoded back to the original\n>>>> +encoding. Consequently, you can use all built-in Git text processing\n>>>> +tools (e.g. git diff, line ending conversions, etc.) with your\n>>>> +non-UTF-8 encoded file.\n>>>> +\n>>>> +Please note that re-encoding content can cause errors and requires\n>>>> +resources. Use the `encoding` attribute only if you cannot store\n>>>> +a file in UTF-8 encoding and if you want Git to be able to process\n>>>> +the text.\n>>>> +\n>>>> +------------------------\n>>>> +*.txt\t\ttext encoding=UTF-16\n>>>> +------------------------\n>>> \n>>> I think that encoding (or worktree-encoding as said elsewhere) must imply text.\n>>> (That is not done in convert.c, but assuming binary or even auto\n>>> does not make much sense to me)\n>> \n>> \"text\" only means \"LF\". \"-text\" means CRLF which would be perfectly fine\n>> for UTF-16. Therefore I don't think we need to imply text.\n>> Does this make sense?\n> Yes and now.\n> \n> \"text\" means convert CRLF at \"git add\" (or commit) into LF,\n> And potentially LF into CRLF at checkout,\n> depending on the EOL attribute (if set), core.autocrlf and/or core.eol.\n> \n> \"-text\" means don't touch CRLF or LF at all. Files with CRLF are commited\n> with CRLF.\n> Running a Unix like \"diff\" tool will often show \"^M\" to indicate that there\n> is a CR before the LF, which may be distracting or confusing.\n> \n> If someone ever wants to handle the UTF-16 files on a Linux/Mac/Unix system,\n> it makes sense to convert the line endings into LF before storing them\n> into the index (at least this is my experience).\n> \n> (Not specifying \"text\" or \"-text\" at all will trigger the auto-handling,\n> which is not needed at all).\n> So if we want to be sure that line endings are CRLF in the worktree we\n> should ask the user to specify things like this:\n> \n> *.ext worktree-encoding=UTF-16 text eol=CRLF\n> \n> It may be enough to give this example in the documentation.\n> and I would rather be over-careful here, to avoid future problems.\n\nAgreed. I'll add that example!\n\n\n>>>> +\n>>>> +All `iconv` encodings with a stable round-trip conversion to and from\n>>>> +UTF-8 are supported. You can see a full list with the following command:\n>>>> +\n>>>> +------------------------\n>>>> +iconv --list\n>>>> +------------------------\n>>>> +\n>>>> `eol`\n>>>> ^^^^^\n>>>> \n>>>> diff --git a/convert.c b/convert.c\n>>>> index 20d7ab67bd..ee19c17104 100644\n>>>> --- a/convert.c\n>>>> +++ b/convert.c\n>>>> @@ -7,6 +7,7 @@\n>>>> #include \"sigchain.h\"\n>>>> #include \"pkt-line.h\"\n>>>> #include \"sub-process.h\"\n>>>> +#include \"utf8.h\"\n>>>> \n>>>> /*\n>>>> * convert.c - convert a file when checking it out and checking it in.\n>>>> @@ -256,6 +257,149 @@ static int will_convert_lf_to_crlf(size_t len, struct text_stat *stats,\n>>>> \n>>>> }\n>>> \n>>> I would avoid to use these #ifdefs here.\n>>> All functions can be in strbuf.c (and may have #ifdefs there), but not here.\n>> \n>> I'll try that. But wouldn't it make more sense to move the functions to utf.c?\n> \n> May be.\n> Originally utf8.c was about encoding and all kind of UTF-8 related stuff.\n> Especially it didn't know anything about strbuf.\n> I don't know why strbuf.h and other functions had been added here,\n> \n> I once moved them into strbuf.c without any problems, but never send out\n> a patch, because of possible merge conflicts in ongoing patches.\n> \n> In any case, if it is about strbuf, I would try to put it into strbuf.c\n\nI think I simplified the code. I reused \"reencode_string_len\".\nI added just two BOM check functions to utf8.c ... I'll send out a new series\nin a bit.\n\n- Lars\n\n"},{"id":"335769","messageId":"xmqq7esyiwju.fsf@gitster.mtv.corp.google.com","threadId":"47426","inReplyTo":"20171229125954.GA9772@tor.lan","subject":"Re: [PATCH v1] convert: add support for 'encoding' attribute","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-01-03T19:15:01Z","receivedAt":"2018-01-03T19:15:12Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Torsten Bögershausen writes:\n\n> May be.\n> Originally utf8.c was about encoding and all kind of UTF-8 related stuff.\n> Especially it didn't know anything about strbuf.\n> I don't know why strbuf.h and other functions had been added here,\n>\n> I once moved them into strbuf.c without any problems, but never send out\n> a patch, because of possible merge conflicts in ongoing patches.\n>\n> In any case, if it is about strbuf, I would try to put it into strbuf.c\n\nPlease don't.\n\nA code that happens to use strbuf as a container and about\nmanipulating the contents is quite different from a code about\nstrbuf. The latter is to enhance and extend how the strbuf as a\ncontainer behaves. An operation about character encoding for a\nstring that happens to be stored in strbuf is more about the\nencoding, and much much less about strbuf.\n\nconvert.c is about massaging contents coming from the outside world\ninto a shape stored in Git and the other way around, and there are\nmultiple ways the contents are massaged. EOL convention may be\nadjusted, characters may be reencoded, end-user defined conversion\nmay be applied. Some of these operations may use helpers specific\nfor the task from other more library-ish files, like checking if a\nstring looks like encoded in UTF-8 from utf8.[ch].\n\n"},{"id":"335775","messageId":"31DC76F1-7D11-4EDE-96C4-97F3AE50EDE9@gmail.com","threadId":"47426","inReplyTo":"xmqq7esyiwju.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v1] convert: add support for 'encoding' attribute","fromName":"Lars Schneider","fromEmail":"larsxschneider@gmail.com","sentAt":"2018-01-03T20:45:13Z","receivedAt":"2018-01-03T20:45:23Z","isPatch":true,"sender":{"key":"larsxschneider@gmail.com","avatar":"https://avatars.githubusercontent.com/u/477434?v=4"},"body":"\nOn 03 Jan 2018, at 20:15, Junio C Hamano wrote:\n\n> Torsten Bögershausen writes:\n> \n>> May be.\n>> Originally utf8.c was about encoding and all kind of UTF-8 related stuff.\n>> Especially it didn't know anything about strbuf.\n>> I don't know why strbuf.h and other functions had been added here,\n>> \n>> I once moved them into strbuf.c without any problems, but never send out\n>> a patch, because of possible merge conflicts in ongoing patches.\n>> \n>> In any case, if it is about strbuf, I would try to put it into strbuf.c\n> \n> Please don't.\n> \n> A code that happens to use strbuf as a container and about\n> manipulating the contents is quite different from a code about\n> strbuf. The latter is to enhance and extend how the strbuf as a\n> container behaves. An operation about character encoding for a\n> string that happens to be stored in strbuf is more about the\n> encoding, and much much less about strbuf.\n> \n> convert.c is about massaging contents coming from the outside world\n> into a shape stored in Git and the other way around, and there are\n> multiple ways the contents are massaged. EOL convention may be\n> adjusted, characters may be reencoded, end-user defined conversion\n> may be applied. Some of these operations may use helpers specific\n> for the task from other more library-ish files, like checking if a\n> string looks like encoded in UTF-8 from utf8.[ch].\n\nAgreed. I did that in v2. See these patches:\n\nhttps://public-inbox.org/git/20171229152222.39680-3-lars.schneider@autodesk.com/\nhttps://public-inbox.org/git/20171229152222.39680-4-lars.schneider@autodesk.com/\n\n- Lars"},{"id":"340311","messageId":"20180226172706.7822-1-tboegi@web.de","threadId":"47426","inReplyTo":"20171218131249.GB4665@sigill.intra.peff.net","subject":"[PATCH/RFC 1/1] Auto diff of UTF-16 files in UTF-8","fromName":"","fromEmail":"tboegi@web.de","sentAt":"2018-02-26T17:27:06Z","receivedAt":"2018-02-26T17:27:28Z","isPatch":true,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"From: Torsten Bögershausen \n\nWhen an UTF-16 file is commited and later changed, `git diff` shows\n\"Binary files XX and YY differ\".\n\nWhen the user wants a diff in UTF-8, a textconv needs to be specified\nin .gitattributes and the textconv must be configured.\n\nA more user-friendly diff can be produced for UTF-16 if\n- the user did not use `git diff --binary`\n- the blob is identified as binary\n- the blob has an UTF-16 BOM\n- the blob can be converted into UTF-8\n\nEnhance the diff machinery to auto-detect UTF-16 blobs and show them\nas UTF-8, unless the user specifies `git diff --binary` which creates\na binary diff.\n\nSigned-off-by: Torsten Bögershausen \n---\n diff.c | 43 ++++++++++++++++++++-\n diffcore.h | 3 ++\n t/t4066-diff-encoding.sh | 98 ++++++++++++++++++++++++++++++++++++++++++++++++\n utf8.h | 11 ++++++\n 4 files changed, 153 insertions(+), 2 deletions(-)\n create mode 100755 t/t4066-diff-encoding.sh\n\ndiff --git a/diff.c b/diff.c\nindex fb22b19f09..51831ee94d 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -3192,6 +3192,10 @@ static void builtin_diff(const char *name_a,\n \t\t\tstrbuf_reset(&header);\n \t\t}\n \n+\t\tif (one && one->reencoded_from_utf16)\n+\t\t\tstrbuf_addf(&header, \"a is converted to UTF-8 from UTF-16\\n\");\n+\t\tif (two && two->reencoded_from_utf16)\n+\t\t\tstrbuf_addf(&header, \"b is converted to UTF-8 from UTF-16\\n\");\n \t\tmf1.size = fill_textconv(textconv_one, one, &mf1.ptr);\n \t\tmf2.size = fill_textconv(textconv_two, two, &mf2.ptr);\n \n@@ -3611,8 +3615,25 @@ int diff_populate_filespec(struct diff_filespec *s, unsigned int flags)\n \t\t\ts->size = size;\n \t\t\ts->should_free = 1;\n \t\t}\n-\t}\n-\telse {\n+\t\tif (!s->binary && buffer_is_binary(s->data, s->size) &&\n+\t\t buffer_has_utf16_bom(s->data, s->size)) {\n+\t\t\tint outsz = 0;\n+\t\t\tchar *outbuf;\n+\t\t\toutbuf = reencode_string_len(s->data, (int)s->size,\n+\t\t\t\t\t\t \"UTF-8\", \"UTF-16\", &outsz);\n+\t\t\tif (outbuf) {\n+\t\t\t\tif (s->should_free)\n+\t\t\t\t\tfree(s->data);\n+\t\t\t\tif (s->should_munmap)\n+\t\t\t\t\tmunmap(s->data, s->size);\n+\t\t\t\ts->should_munmap = 0;\n+\t\t\t\ts->data = outbuf;\n+\t\t\t\ts->size = outsz;\n+\t\t\t\ts->reencoded_from_utf16 = 1;\n+\t\t\t\ts->should_free = 1;\n+\t\t\t}\n+\t\t}\n+\t} else {\n \t\tenum object_type type;\n \t\tif (size_only || (flags & CHECK_BINARY)) {\n \t\t\ttype = sha1_object_info(s->oid.hash, &s->size);\n@@ -3629,6 +3650,19 @@ int diff_populate_filespec(struct diff_filespec *s, unsigned int flags)\n \t\ts->data = read_sha1_file(s->oid.hash, &type, &s->size);\n \t\tif (!s->data)\n \t\t\tdie(\"unable to read %s\", oid_to_hex(&s->oid));\n+\t\tif (!s->binary && buffer_is_binary(s->data, s->size) &&\n+\t\t buffer_has_utf16_bom(s->data, s->size)) {\n+\t\t\tint outsz = 0;\n+\t\t\tchar *buf;\n+\t\t\tbuf = reencode_string_len(s->data, (int)s->size,\n+\t\t\t\t\t\t \"UTF-8\", \"UTF-16\", &outsz);\n+\t\t\tif (buf) {\n+\t\t\t\tfree(s->data);\n+\t\t\t\ts->data = buf;\n+\t\t\t\ts->size = outsz;\n+\t\t\t\ts->reencoded_from_utf16 = 1;\n+\t\t\t}\n+\t\t}\n \t\ts->should_free = 1;\n \t}\n \treturn 0;\n@@ -5695,6 +5729,10 @@ static int diff_filespec_is_identical(struct diff_filespec *one,\n \n static int diff_filespec_check_stat_unmatch(struct diff_filepair *p)\n {\n+\tif (p->binary) {\n+\t\tp->one->binary = 1;\n+\t\tp->two->binary = 1;\n+\t}\n \tif (p->done_skip_stat_unmatch)\n \t\treturn p->skip_stat_unmatch_result;\n \n@@ -5735,6 +5773,7 @@ static void diffcore_skip_stat_unmatch(struct diff_options *diffopt)\n \tfor (i = 0; i < q->nr; i++) {\n \t\tstruct diff_filepair *p = q->queue[i];\n \n+\t\tp->binary = diffopt->flags.binary;\n \t\tif (diff_filespec_check_stat_unmatch(p))\n \t\t\tdiff_q(&outq, p);\n \t\telse {\ndiff --git a/diffcore.h b/diffcore.h\nindex a30da161da..3cd97bb93b 100644\n--- a/diffcore.h\n+++ b/diffcore.h\n@@ -47,6 +47,8 @@ struct diff_filespec {\n \tunsigned has_more_entries : 1; /* only appear in combined diff */\n \t/* data should be considered \"binary\"; -1 means \"don't know yet\" */\n \tsigned int is_binary : 2;\n+\tunsigned binary : 1;\n+\tunsigned reencoded_from_utf16 : 1;\n \tstruct userdiff_driver *driver;\n };\n \n@@ -72,6 +74,7 @@ struct diff_filepair {\n \tunsigned is_unmerged : 1;\n \tunsigned done_skip_stat_unmatch : 1;\n \tunsigned skip_stat_unmatch_result : 1;\n+\tunsigned binary : 1;\n };\n #define DIFF_PAIR_UNMERGED(p) ((p)->is_unmerged)\n \ndiff --git a/t/t4066-diff-encoding.sh b/t/t4066-diff-encoding.sh\nnew file mode 100755\nindex 0000000000..9bb3c70ada\n--- /dev/null\n+++ b/t/t4066-diff-encoding.sh\n@@ -0,0 +1,98 @@\n+#!/bin/sh\n+\n+test_description='git diff with encoding attribute'\n+\n+. ./test-lib.sh\n+\n+printf '\\303\\244rger\\n\\303\\266se\\n\\303\\274bel\\n' |\n+\ticonv -f UTF-8 -t UTF-16 >UTF-16\n+printf '\\303\\266se\\n\\303\\274bel\\n\\303\\245st\\n' |\n+\ticonv -f UTF-8 -t UTF-16 >file2\n+\n+test_expect_success 'setup' '\n+\tcp UTF-16 file &&\n+\tgit add file &&\n+\tgit commit -m \"add file in UTF-16\" &&\n+\ttest_tick &&\n+\techo \"file encoding=UTF-16\" >.gitattributes\n+'\n+\n+test_expect_success 'diff against local change' '\n+\tcp file2 file &&\n+\ttest_tick &&\n+\tcat >expect <<-\\EOF &&\n+\tdiff --git a/file b/file\n+\tindex 26acf09..e98d27a 100644\n+\ta is converted to UTF-8 from UTF-16\n+\tb is converted to UTF-8 from UTF-16\n+\t--- a/file\n+\t+++ b/file\n+\t@@ -1,3 +1,3 @@\n+\t-ärger\n+\t öse\n+\t übel\n+\t+åst\n+EOF\n+\tgit diff file >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'diff --binary against local change' '\n+\tcp file2 file &&\n+\ttest_tick &&\n+\tcat >expect <<-\\EOF &&\n+\tdiff --git a/file b/file\n+\tindex 26acf09b0aad19fb22566956d1a39cb4e2a3b420..e98d27acfb90cfcfc84fcc5173baa4aa7828290f 100644\n+\tGIT binary patch\n+\tliteral 28\n+\tecmezW?;ArgLn;Fo!ykquAe{qbJq3!C0BHb{ln3Pi\n+\n+\tliteral 32\n+\ticmezW?+HT@Lpnn$kmO?c#!w7oaWVX1NCMJ1Ko$VA_z0~4\n+\n+EOF\n+\tgit diff --binary file >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'commit local change' '\n+\tgit add file &&\n+\tgit commit -m \"add file V2 in UTF-16\" &&\n+\ttest_tick\n+'\n+\n+test_expect_success 'diff HEAD against HEAD^' '\n+\tcat >expect <<-\\EOF &&\n+\tdiff --git a/file b/file\n+\tindex 26acf09..e98d27a 100644\n+\ta is converted to UTF-8 from UTF-16\n+\tb is converted to UTF-8 from UTF-16\n+\t--- a/file\n+\t+++ b/file\n+\t@@ -1,3 +1,3 @@\n+\t-ärger\n+\t öse\n+\t übel\n+\t+åst\n+EOF\n+\tgit diff HEAD^ HEAD -- file >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'diff --binary HEAD against HEAD^' '\n+\tcat >expect <<-\\EOF &&\n+\tdiff --git a/file b/file\n+\tindex 26acf09b0aad19fb22566956d1a39cb4e2a3b420..e98d27acfb90cfcfc84fcc5173baa4aa7828290f 100644\n+\tGIT binary patch\n+\tliteral 28\n+\tecmezW?;ArgLn;Fo!ykquAe{qbJq3!C0BHb{ln3Pi\n+\n+\tliteral 32\n+\ticmezW?+HT@Lpnn$kmO?c#!w7oaWVX1NCMJ1Ko$VA_z0~4\n+\t\n+EOF\n+\tgit diff --binary HEAD^ HEAD -- file >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_done\ndiff --git a/utf8.h b/utf8.h\nindex 6bbcf31a83..a2184d0300 100644\n--- a/utf8.h\n+++ b/utf8.h\n@@ -16,6 +16,17 @@ int utf8_fprintf(FILE *, const char *, ...);\n extern const char utf8_bom[];\n extern int skip_utf8_bom(char **, size_t);\n \n+static inline int buffer_has_utf16_bom(const void *buf, size_t len) {\n+ const unsigned char *text = (unsigned char *)buf;\n+ if (!text || len < 2)\n+ return 0;\n+ if (text[0] == 0xff && text[1] == 0xfe)\n+ return 1;\n+ if (text[0] == 0xfe && text[1] == 0xff)\n+ return 1;\n+ return 0;\n+}\n+\n void strbuf_add_wrapped_text(struct strbuf *buf,\n \t\tconst char *text, int indent, int indent2, int width);\n void strbuf_add_wrapped_bytes(struct strbuf *buf, const char *data, int len,\n-- \n2.16.1.194.gb2e45c695d\n\n"},{"id":"340318","messageId":"alpine.DEB.2.00.1802261939390.15209@ds9.cixit.se","threadId":"47426","inReplyTo":"20180226172706.7822-1-tboegi@web.de","subject":"Re: [PATCH/RFC 1/1] Auto diff of UTF-16 files in UTF-8","fromName":"Peter Krefting","fromEmail":"peter@softwolves.pp.se","sentAt":"2018-02-26T18:43:08Z","receivedAt":"2018-02-26T18:51:24Z","isPatch":true,"sender":{"key":"peter@softwolves.pp.se","avatar":"https://avatars.githubusercontent.com/u/990764?v=4"},"body":"tboegi@web.de:\n\n> +static inline int buffer_has_utf16_bom(const void *buf, size_t len) {\n> + const unsigned char *text = (unsigned char *)buf;\n> + if (!text || len < 2)\n> + return 0;\n> + if (text[0] == 0xff && text[1] == 0xfe)\n> + return 1;\n> + if (text[0] == 0xfe && text[1] == 0xff)\n> + return 1;\n> + return 0;\n> +}\n\nThis would also match a UTF-32LE BOM, not that I think anyone would \nactually use that for text files, but it might be worth adding a test \nfor that, just in case?\n\n if (text[0] == 0xff && text[1] == 0xfe)\n return len < 4 || (text[2] != 0 && text[3] != 0);\n\n-- \n\\\\// Peter - http://www.softwolves.pp.se/\n"},{"id":"340478","messageId":"20180227223929.GA3484@sigill.intra.peff.net","threadId":"47426","inReplyTo":"20180226172706.7822-1-tboegi@web.de","subject":"Re: [PATCH/RFC 1/1] Auto diff of UTF-16 files in UTF-8","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-02-27T22:39:29Z","receivedAt":"2018-02-27T22:39:35Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Feb 26, 2018 at 06:27:06PM +0100, tboegi@web.de wrote:\n\n> @@ -3611,8 +3615,25 @@ int diff_populate_filespec(struct diff_filespec *s, unsigned int flags)\n> \t\t\ts->size = size;\n> \t\t\ts->should_free = 1;\n> \t\t}\n> -\t}\n> -\telse {\n> +\t\tif (!s->binary && buffer_is_binary(s->data, s->size) &&\n> +\t\t buffer_has_utf16_bom(s->data, s->size)) {\n> +\t\t\tint outsz = 0;\n> +\t\t\tchar *outbuf;\n> +\t\t\toutbuf = reencode_string_len(s->data, (int)s->size,\n> +\t\t\t\t\t\t \"UTF-8\", \"UTF-16\", &outsz);\n> +\t\t\tif (outbuf) {\n> +\t\t\t\tif (s->should_free)\n> +\t\t\t\t\tfree(s->data);\n> +\t\t\t\tif (s->should_munmap)\n> +\t\t\t\t\tmunmap(s->data, s->size);\n> +\t\t\t\ts->should_munmap = 0;\n> +\t\t\t\ts->data = outbuf;\n> +\t\t\t\ts->size = outsz;\n> +\t\t\t\ts->reencoded_from_utf16 = 1;\n> +\t\t\t\ts->should_free = 1;\n> +\t\t\t}\n> +\t\t}\n> +\t} else {\n\nI don't think it makes sense to do the conversion deep inside\ndiff_populate_filespec(), because it will kick in much more than you'd\nwant (e.g., \"format-patch | am\" would stop working with this patch, I\nthink).\n\nI think you'd want to hook this in at the same level as fill_textconv().\nIn fact, one way to do it would be to have the get_textconv() stack just\nfill in a special driver that does the auto-detection. This is similar\nto my earlier patch, but it avoids overriding the user-facing config for\nnon-textconv things (and naturally any actual configured textconv filter\nwould override the auto-detection).\n\n-Peff\n"}]}