{"thread":{"id":"47921","subject":"[PATCH v8 0/7] convert: add support for different encodings","startedAt":"2018-02-24T16:29:42Z","lastAt":"2018-03-04T19:58:33Z","messageCount":20,"participants":["lars.schneider@autodesk.com","Eric Sunshine","Lars Schneider"],"isPatch":true,"patchVersion":8,"patchTotal":7},"messages":[{"id":"340175","messageId":"20180224162801.98860-1-lars.schneider@autodesk.com","threadId":"47921","inReplyTo":null,"subject":"[PATCH v8 0/7] convert: add support for different encodings","fromName":"","fromEmail":"lars.schneider@autodesk.com","sentAt":"2018-02-24T16:27:54Z","receivedAt":"2018-02-24T16:29:42Z","isPatch":true,"sender":{"key":"lars.schneider@autodesk.com","avatar":"https://gravatar.com/avatar/524188df3a38b02692619777bb90d9cc735db1ad75f5304e0bf09f8efd299775?d=mp&s=160"},"body":"From: Lars Schneider \n\nHi,\n\nPatches 1-4, 6 are preparation and helper functions.\nPatch 5,7 are the actual change.\n\nThis series depends on Torsten's 8462ff43e4 (convert_to_git():\nsafe_crlf/checksafe becomes int conv_flags, 2018-01-13) which is\nalready in master.\n\nChanges since v7:\n\n* make it clearer in the documentation that Git stores content \"as-is\"\n by default. Content is only stored in UTF-8 if w-t-e is used (Junio)\n* add test case for $GIT_DIR/info/attributes support (Junio)\n\nThanks,\nLars\n\n RFC: https://public-inbox.org/git/BDB9B884-6D17-4BE3-A83C-F67E2AFA2B46@gmail.com/\n v1: https://public-inbox.org/git/20171211155023.1405-1-lars.schneider@autodesk.com/\n v2: https://public-inbox.org/git/20171229152222.39680-1-lars.schneider@autodesk.com/\n v3: https://public-inbox.org/git/20180106004808.77513-1-lars.schneider@autodesk.com/\n v4: https://public-inbox.org/git/20180120152418.52859-1-lars.schneider@autodesk.com/\n v5: https://public-inbox.org/git/20180129201855.9182-1-tboegi@web.de/\n v6: https://public-inbox.org/git/20180209132830.55385-1-lars.schneider@autodesk.com/\n v7: https://public-inbox.org/git/20180215152711.158-1-lars.schneider@autodesk.com/\n\n\nBase Ref:\nWeb-Diff: https://github.com/larsxschneider/git/commit/2758a2da29\nCheckout: git fetch https://github.com/larsxschneider/git encoding-v8 && git checkout 2758a2da29\n\n\n### Interdiff (v7..v8):\n\ndiff --git a/Documentation/gitattributes.txt b/Documentation/gitattributes.txt\nindex 10cb37795d..11315054f4 100644\n--- a/Documentation/gitattributes.txt\n+++ b/Documentation/gitattributes.txt\n@@ -275,11 +275,11 @@ few exceptions. Even though...\n `working-tree-encoding`\n ^^^^^^^^^^^^^^^^^^^^^^^\n\n-Git recognizes files encoded with ASCII or one of its supersets (e.g.\n-UTF-8 or ISO-8859-1) as text files. All other encodings are usually\n-interpreted as binary and consequently built-in Git text processing\n-tools (e.g. 'git diff') as well as most Git web front ends do not\n-visualize the content.\n+Git recognizes files encoded in ASCII or one of its supersets (e.g.\n+UTF-8, ISO-8859-1, ...) as text files. Files encoded in certain other\n+encodings (e.g. UTF-16) are interpreted as binary and consequently\n+built-in Git text processing tools (e.g. 'git diff') as well as most Git\n+web front ends do not visualize the contents of these files by default.\n\n In these cases you can tell Git the encoding of a file in the working\n directory with the `working-tree-encoding` attribute. If a file with this\n@@ -291,12 +291,24 @@ the content is reencoded back to the specified encoding.\n Please note that using the `working-tree-encoding` attribute may have a\n number of pitfalls:\n\n-- Third party Git implementations that do not support the\n- `working-tree-encoding` attribute will checkout the respective files\n- UTF-8 encoded and not in the expected encoding. Consequently, these\n- files will appear different which typically causes trouble. This is\n- in particular the case for older Git versions and alternative Git\n- implementations such as JGit or libgit2 (as of February 2018).\n+- Alternative Git implementations (e.g. JGit or libgit2) and older Git\n+ versions (as of March 2018) do not support the `working-tree-encoding`\n+ attribute. If you decide to use the `working-tree-encoding` attribute\n+ in your repository, then it is strongly recommended to ensure that all\n+ clients working with the repository support it.\n+\n+ If you declare `*.proj` files as UTF-16 and you add `foo.proj` with an\n+ `working-tree-encoding` enabled Git client, then `foo.proj` will be\n+ stored as UTF-8 internally. A client without `working-tree-encoding`\n+ support will checkout `foo.proj` as UTF-8 encoded file. This will\n+ typically cause trouble for the users of this file.\n+\n+ If a Git client, that does not support the `working-tree-encoding`\n+ attribute, adds a new file `bar.proj`, then `bar.proj` will be\n+ stored \"as-is\" internally (in this example probably as UTF-16).\n+ A client with `working-tree-encoding` support will interpret the\n+ internal contents as UTF-8 and try to convert it to UTF-16 on checkout.\n+ That operation will fail and cause an error.\n\n - Reencoding content to non-UTF encodings can cause errors as the\n conversion might not be UTF-8 round trip safe. If you suspect your\ndiff --git a/t/t0028-working-tree-encoding.sh b/t/t0028-working-tree-encoding.sh\nindex e4717402a5..e34c21eb29 100755\n--- a/t/t0028-working-tree-encoding.sh\n+++ b/t/t0028-working-tree-encoding.sh\n@@ -13,8 +13,11 @@ test_expect_success 'setup test repo' '\n \techo \"*.utf16 text working-tree-encoding=utf-16\" >.gitattributes &&\n \tprintf \"$text\" >test.utf8.raw &&\n \tprintf \"$text\" | iconv -f UTF-8 -t UTF-16 >test.utf16.raw &&\n+\tprintf \"$text\" | iconv -f UTF-8 -t UTF-32 >test.utf32.raw &&\n \tcp test.utf16.raw test.utf16 &&\n+\tcp test.utf32.raw test.utf32 &&\n\n+\t# Add only UTF-16 file, we will add the UTF-32 file later\n \tgit add .gitattributes test.utf16 &&\n \tgit commit -m initial\n '\n@@ -24,7 +27,7 @@ test_expect_success 'ensure UTF-8 is stored in Git' '\n \ttest_cmp_bin test.utf8.raw test.utf16.git &&\n\n \t# cleanup\n-\trm test.utf8.raw test.utf16.git\n+\trm test.utf16.git\n '\n\n test_expect_success 're-encode to UTF-16 on checkout' '\n@@ -36,6 +39,19 @@ test_expect_success 're-encode to UTF-16 on checkout' '\n \trm test.utf16.raw\n '\n\n+test_expect_success 'check $GIT_DIR/info/attributes support' '\n+\techo \"*.utf32 text working-tree-encoding=utf-32\" >.git/info/attributes &&\n+\n+\tgit add test.utf32 &&\n+\n+\tgit cat-file -p :test.utf32 >test.utf32.git &&\n+\ttest_cmp_bin test.utf8.raw test.utf32.git &&\n+\n+\t# cleanup\n+\tgit reset --hard HEAD &&\n+\trm test.utf8.raw test.utf32.raw test.utf32.git\n+'\n+\n test_expect_success 'check prohibited UTF BOM' '\n \tprintf \"\\0a\\0b\\0c\" >nobom.utf16be.raw &&\n \tprintf \"a\\0b\\0c\\0\" >nobom.utf16le.raw &&\n\n\n### Patches\n\nLars Schneider (7):\n strbuf: remove unnecessary NUL assignment in xstrdup_tolower()\n strbuf: add xstrdup_toupper()\n utf8: add function to detect prohibited UTF-16/32 BOM\n utf8: add function to detect a missing UTF-16/32 BOM\n convert: add 'working-tree-encoding' attribute\n convert: add tracing for 'working-tree-encoding' attribute\n convert: add round trip check based on 'core.checkRoundtripEncoding'\n\n Documentation/config.txt | 6 +\n Documentation/gitattributes.txt | 86 +++++++++++++\n config.c | 5 +\n convert.c | 256 ++++++++++++++++++++++++++++++++++++-\n convert.h | 2 +\n environment.c | 1 +\n sha1_file.c | 2 +-\n strbuf.c | 13 +-\n strbuf.h | 1 +\n t/t0028-working-tree-encoding.sh | 269 +++++++++++++++++++++++++++++++++++++++\n utf8.c | 37 ++++++\n utf8.h | 25 ++++\n 12 files changed, 700 insertions(+), 3 deletions(-)\n create mode 100755 t/t0028-working-tree-encoding.sh\n\n\nbase-commit: 8a2f0888555ce46ac87452b194dec5cb66fb1417\n--\n2.16.1\n\n"},{"id":"340176","messageId":"20180224162801.98860-2-lars.schneider@autodesk.com","threadId":"47921","inReplyTo":"20180224162801.98860-1-lars.schneider@autodesk.com","subject":"[PATCH v8 1/7] strbuf: remove unnecessary NUL assignment in xstrdup_tolower()","fromName":"","fromEmail":"lars.schneider@autodesk.com","sentAt":"2018-02-24T16:27:55Z","receivedAt":"2018-02-24T16:29:45Z","isPatch":true,"sender":{"key":"lars.schneider@autodesk.com","avatar":"https://gravatar.com/avatar/524188df3a38b02692619777bb90d9cc735db1ad75f5304e0bf09f8efd299775?d=mp&s=160"},"body":"From: Lars Schneider \n\nSince 3733e69464 (use xmallocz to avoid size arithmetic, 2016-02-22) we\nallocate the buffer for the lower case string with xmallocz(). This\nalready ensures a NUL at the end of the allocated buffer.\n\nRemove the unnecessary assignment.\n\nSigned-off-by: Lars Schneider \n---\n strbuf.c | 1 -\n 1 file changed, 1 deletion(-)\n\ndiff --git a/strbuf.c b/strbuf.c\nindex 1df674e919..55b7daeb35 100644\n--- a/strbuf.c\n+++ b/strbuf.c\n@@ -781,7 +781,6 @@ char *xstrdup_tolower(const char *string)\n \tresult = xmallocz(len);\n \tfor (i = 0; i < len; i++)\n \t\tresult[i] = tolower(string[i]);\n-\tresult[i] = '\\0';\n \treturn result;\n }\n \n-- \n2.16.1\n\n"},{"id":"340177","messageId":"20180224162801.98860-3-lars.schneider@autodesk.com","threadId":"47921","inReplyTo":"20180224162801.98860-1-lars.schneider@autodesk.com","subject":"[PATCH v8 2/7] strbuf: add xstrdup_toupper()","fromName":"","fromEmail":"lars.schneider@autodesk.com","sentAt":"2018-02-24T16:27:56Z","receivedAt":"2018-02-24T16:29:48Z","isPatch":true,"sender":{"key":"lars.schneider@autodesk.com","avatar":"https://gravatar.com/avatar/524188df3a38b02692619777bb90d9cc735db1ad75f5304e0bf09f8efd299775?d=mp&s=160"},"body":"From: Lars Schneider \n\nCreate a copy of an existing string and make all characters upper case.\nSimilar xstrdup_tolower().\n\nThis function is used in a subsequent commit.\n\nSigned-off-by: Lars Schneider \n---\n strbuf.c | 12 ++++++++++++\n strbuf.h | 1 +\n 2 files changed, 13 insertions(+)\n\ndiff --git a/strbuf.c b/strbuf.c\nindex 55b7daeb35..b635f0bdc4 100644\n--- a/strbuf.c\n+++ b/strbuf.c\n@@ -784,6 +784,18 @@ char *xstrdup_tolower(const char *string)\n \treturn result;\n }\n \n+char *xstrdup_toupper(const char *string)\n+{\n+\tchar *result;\n+\tsize_t len, i;\n+\n+\tlen = strlen(string);\n+\tresult = xmallocz(len);\n+\tfor (i = 0; i < len; i++)\n+\t\tresult[i] = toupper(string[i]);\n+\treturn result;\n+}\n+\n char *xstrvfmt(const char *fmt, va_list ap)\n {\n \tstruct strbuf buf = STRBUF_INIT;\ndiff --git a/strbuf.h b/strbuf.h\nindex 14c8c10d66..df7ced53ed 100644\n--- a/strbuf.h\n+++ b/strbuf.h\n@@ -607,6 +607,7 @@ __attribute__((format (printf,2,3)))\n extern int fprintf_ln(FILE *fp, const char *fmt, ...);\n \n char *xstrdup_tolower(const char *);\n+char *xstrdup_toupper(const char *);\n \n /**\n * Create a newly allocated string using printf format. You can do this easily\n-- \n2.16.1\n\n"},{"id":"340178","messageId":"20180224162801.98860-4-lars.schneider@autodesk.com","threadId":"47921","inReplyTo":"20180224162801.98860-1-lars.schneider@autodesk.com","subject":"[PATCH v8 3/7] utf8: add function to detect prohibited UTF-16/32 BOM","fromName":"","fromEmail":"lars.schneider@autodesk.com","sentAt":"2018-02-24T16:27:57Z","receivedAt":"2018-02-24T16:29:50Z","isPatch":true,"sender":{"key":"lars.schneider@autodesk.com","avatar":"https://gravatar.com/avatar/524188df3a38b02692619777bb90d9cc735db1ad75f5304e0bf09f8efd299775?d=mp&s=160"},"body":"From: Lars Schneider \n\nWhenever a data stream is declared to be UTF-16BE, UTF-16LE, UTF-32BE\nor UTF-32LE a BOM must not be used [1]. The function returns true if\nthis is the case.\n\nThis function is used in a subsequent commit.\n\n[1] http://unicode.org/faq/utf_bom.html#bom10\n\nSigned-off-by: Lars Schneider \n---\n utf8.c | 24 ++++++++++++++++++++++++\n utf8.h | 9 +++++++++\n 2 files changed, 33 insertions(+)\n\ndiff --git a/utf8.c b/utf8.c\nindex 2c27ce0137..914881cd1f 100644\n--- a/utf8.c\n+++ b/utf8.c\n@@ -538,6 +538,30 @@ char *reencode_string_len(const char *in, int insz,\n }\n #endif\n \n+static int has_bom_prefix(const char *data, size_t len,\n+\t\t\t const char *bom, size_t bom_len)\n+{\n+\treturn (len >= bom_len) && !memcmp(data, bom, bom_len);\n+}\n+\n+static const char utf16_be_bom[] = {0xFE, 0xFF};\n+static const char utf16_le_bom[] = {0xFF, 0xFE};\n+static const char utf32_be_bom[] = {0x00, 0x00, 0xFE, 0xFF};\n+static const char utf32_le_bom[] = {0xFF, 0xFE, 0x00, 0x00};\n+\n+int has_prohibited_utf_bom(const char *enc, const char *data, size_t len)\n+{\n+\treturn (\n+\t (!strcmp(enc, \"UTF-16BE\") || !strcmp(enc, \"UTF-16LE\")) &&\n+\t (has_bom_prefix(data, len, utf16_be_bom, sizeof(utf16_be_bom)) ||\n+\t has_bom_prefix(data, len, utf16_le_bom, sizeof(utf16_le_bom)))\n+\t) || (\n+\t (!strcmp(enc, \"UTF-32BE\") || !strcmp(enc, \"UTF-32LE\")) &&\n+\t (has_bom_prefix(data, len, utf32_be_bom, sizeof(utf32_be_bom)) ||\n+\t has_bom_prefix(data, len, utf32_le_bom, sizeof(utf32_le_bom)))\n+\t);\n+}\n+\n /*\n * Returns first character length in bytes for multi-byte `text` according to\n * `encoding`.\ndiff --git a/utf8.h b/utf8.h\nindex 6bbcf31a83..4711429af9 100644\n--- a/utf8.h\n+++ b/utf8.h\n@@ -70,4 +70,13 @@ typedef enum {\n void strbuf_utf8_align(struct strbuf *buf, align_type position, unsigned int width,\n \t\t const char *s);\n \n+/*\n+ * Whenever a data stream is declared to be UTF-16BE, UTF-16LE, UTF-32BE\n+ * or UTF-32LE a BOM must not be used [1]. The function returns true if\n+ * this is the case.\n+ *\n+ * [1] http://unicode.org/faq/utf_bom.html#bom10\n+ */\n+int has_prohibited_utf_bom(const char *enc, const char *data, size_t len);\n+\n #endif\n-- \n2.16.1\n\n"},{"id":"340179","messageId":"20180224162801.98860-6-lars.schneider@autodesk.com","threadId":"47921","inReplyTo":"20180224162801.98860-1-lars.schneider@autodesk.com","subject":"[PATCH v8 5/7] convert: add 'working-tree-encoding' attribute","fromName":"","fromEmail":"lars.schneider@autodesk.com","sentAt":"2018-02-24T16:27:59Z","receivedAt":"2018-02-24T16:29:53Z","isPatch":true,"sender":{"key":"lars.schneider@autodesk.com","avatar":"https://gravatar.com/avatar/524188df3a38b02692619777bb90d9cc735db1ad75f5304e0bf09f8efd299775?d=mp&s=160"},"body":"From: Lars Schneider \n\nGit recognizes files encoded with ASCII or one of its supersets (e.g.\nUTF-8 or ISO-8859-1) as text files. All other encodings are usually\ninterpreted as binary and consequently built-in Git text processing\ntools (e.g. 'git diff') as well as most Git web front ends do not\nvisualize the content.\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\nSigned-off-by: Lars Schneider \n---\n Documentation/gitattributes.txt | 78 ++++++++++++++\n convert.c | 157 ++++++++++++++++++++++++++-\n convert.h | 1 +\n sha1_file.c | 2 +-\n t/t0028-working-tree-encoding.sh | 226 +++++++++++++++++++++++++++++++++++++++\n 5 files changed, 462 insertions(+), 2 deletions(-)\n create mode 100755 t/t0028-working-tree-encoding.sh\n\ndiff --git a/Documentation/gitattributes.txt b/Documentation/gitattributes.txt\nindex 30687de81a..eddeaee1f7 100644\n--- a/Documentation/gitattributes.txt\n+++ b/Documentation/gitattributes.txt\n@@ -272,6 +272,84 @@ few exceptions. Even though...\n catch potential problems early, safety triggers.\n \n \n+`working-tree-encoding`\n+^^^^^^^^^^^^^^^^^^^^^^^\n+\n+Git recognizes files encoded in ASCII or one of its supersets (e.g.\n+UTF-8, ISO-8859-1, ...) as text files. Files encoded in certain other\n+encodings (e.g. UTF-16) are interpreted as binary and consequently\n+built-in Git text processing tools (e.g. 'git diff') as well as most Git\n+web front ends do not visualize the contents of these files by default.\n+\n+In these cases you can tell Git the encoding of a file in the working\n+directory with the `working-tree-encoding` attribute. If a file with this\n+attribute is added to Git, then Git reencodes the content from the\n+specified encoding to UTF-8. Finally, Git stores the UTF-8 encoded\n+content in its internal data structure (called \"the index\"). On checkout\n+the content is reencoded back to the specified encoding.\n+\n+Please note that using the `working-tree-encoding` attribute may have a\n+number of pitfalls:\n+\n+- Alternative Git implementations (e.g. JGit or libgit2) and older Git\n+ versions (as of March 2018) do not support the `working-tree-encoding`\n+ attribute. If you decide to use the `working-tree-encoding` attribute\n+ in your repository, then it is strongly recommended to ensure that all\n+ clients working with the repository support it.\n+\n+ If you declare `*.proj` files as UTF-16 and you add `foo.proj` with an\n+ `working-tree-encoding` enabled Git client, then `foo.proj` will be\n+ stored as UTF-8 internally. A client without `working-tree-encoding`\n+ support will checkout `foo.proj` as UTF-8 encoded file. This will\n+ typically cause trouble for the users of this file.\n+\n+ If a Git client, that does not support the `working-tree-encoding`\n+ attribute, adds a new file `bar.proj`, then `bar.proj` will be\n+ stored \"as-is\" internally (in this example probably as UTF-16).\n+ A client with `working-tree-encoding` support will interpret the\n+ internal contents as UTF-8 and try to convert it to UTF-16 on checkout.\n+ That operation will fail and cause an error.\n+\n+- Reencoding content requires resources that might slow down certain\n+ Git operations (e.g 'git checkout' or 'git add').\n+\n+Use the `working-tree-encoding` attribute only if you cannot store a file\n+in UTF-8 encoding and if you want Git to be able to process the content\n+as text.\n+\n+As an example, use the following attributes if your '*.proj' files are\n+UTF-16 encoded with byte order mark (BOM) and you want Git to perform\n+automatic line ending conversion based on your platform.\n+\n+------------------------\n+*.proj\t\ttext working-tree-encoding=UTF-16\n+------------------------\n+\n+Use the following attributes if your '*.proj' files are UTF-16 little\n+endian encoded without BOM and you want Git to use Windows line endings\n+in the working directory. Please note, it is highly recommended to\n+explicitly define the line endings with `eol` if the `working-tree-encoding`\n+attribute is used to avoid ambiguity.\n+\n+------------------------\n+*.proj \t\ttext working-tree-encoding=UTF-16LE eol=CRLF\n+------------------------\n+\n+You can get a list of all available encodings on your platform with the\n+following command:\n+\n+------------------------\n+iconv --list\n+------------------------\n+\n+If you do not know the encoding of a file, then you can use the `file`\n+command to guess the encoding:\n+\n+------------------------\n+file foo.proj\n+------------------------\n+\n+\n `ident`\n ^^^^^^^\n \ndiff --git a/convert.c b/convert.c\nindex b976eb968c..d20c341b6d 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@@ -265,6 +266,110 @@ static int will_convert_lf_to_crlf(size_t len, struct text_stat *stats,\n \n }\n \n+static struct encoding {\n+\tconst char *name;\n+\tstruct encoding *next;\n+} *encoding, **encoding_tail;\n+static const char *default_encoding = \"UTF-8\";\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, int conv_flags)\n+{\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+\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 (has_prohibited_utf_bom(enc->name, src, src_len)) {\n+\t\tconst char *error_msg = _(\n+\t\t\t\"BOM is prohibited in '%s' if encoded as %s\");\n+\t\t/*\n+\t\t * This advise is shown for UTF-??BE and UTF-??LE encodings.\n+\t\t * We truncate the encoding name to 6 chars with %.6s to cut\n+\t\t * off the last two \"byte order\" characters.\n+\t\t */\n+\t\tconst char *advise_msg = _(\n+\t\t\t\"The file '%s' contains a byte order mark (BOM). \"\n+\t\t\t\"Please use %.6s as working-tree-encoding.\");\n+\t\tadvise(advise_msg, path, enc->name);\n+\t\tif (conv_flags & CONV_WRITE_OBJECT)\n+\t\t\tdie(error_msg, path, enc->name);\n+\t\telse\n+\t\t\terror(error_msg, path, enc->name);\n+\n+\t} else if (is_missing_required_utf_bom(enc->name, src, src_len)) {\n+\t\tconst char *error_msg = _(\n+\t\t\t\"BOM is required in '%s' if encoded as %s\");\n+\t\tconst char *advise_msg = _(\n+\t\t\t\"The file '%s' is missing a byte order mark (BOM). \"\n+\t\t\t\"Please use %sBE or %sLE (depending on the byte order) \"\n+\t\t\t\"as working-tree-encoding.\");\n+\t\tadvise(advise_msg, path, enc->name, enc->name);\n+\t\tif (conv_flags & CONV_WRITE_OBJECT)\n+\t\t\tdie(error_msg, path, enc->name);\n+\t\telse\n+\t\t\terror(error_msg, path, enc->name);\n+\t}\n+\n+\tdst = reencode_string_len(src, src_len, default_encoding, enc->name,\n+\t\t\t\t &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\tconst char* msg = _(\"failed to encode '%s' from %s to %s\");\n+\t\tif (conv_flags & CONV_WRITE_OBJECT)\n+\t\t\tdie(msg, path, enc->name, default_encoding);\n+\t\telse\n+\t\t\terror(msg, path, enc->name, default_encoding);\n+\t}\n+\n+\tstrbuf_attach(buf, dst, dst_len, dst_len + 1);\n+\treturn 1;\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+\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+\tdst = reencode_string_len(src, src_len, enc->name, default_encoding,\n+\t\t\t\t &dst_len);\n+\tif (!dst) {\n+\t\terror(\"failed to encode '%s' from %s to %s\",\n+\t\t\tpath, enc->name, default_encoding);\n+\t\treturn 0;\n+\t}\n+\n+\tstrbuf_attach(buf, dst, dst_len, dst_len + 1);\n+\treturn 1;\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@@ -978,6 +1083,35 @@ 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_TRUE(value) || ATTR_FALSE(value) || ATTR_UNSET(value) ||\n+\t !strlen(value))\n+\t\treturn NULL;\n+\n+\tfor (enc = encoding; enc; enc = enc->next)\n+\t\tif (!strcasecmp(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(*enc));\n+\t/*\n+\t * Ensure encoding names are always upper case (e.g. UTF-8) to\n+\t * simplify subsequent string comparisons.\n+\t */\n+\tenc->name = xstrdup_toupper(value);\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@@ -1033,6 +1167,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 *working_tree_encoding; /* Supported encoding or default encoding if NULL */\n };\n \n static void convert_attrs(struct conv_attrs *ca, const char *path)\n@@ -1041,8 +1176,10 @@ 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\", \"working-tree-encoding\",\n+\t\t\t\t\t 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@@ -1064,6 +1201,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->working_tree_encoding = git_path_check_encoding(ccheck + 5);\n \t} else {\n \t\tca->drv = NULL;\n \t\tca->crlf_action = CRLF_UNDEFINED;\n@@ -1144,6 +1282,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.working_tree_encoding, conv_flags);\n+\tif (ret && dst) {\n+\t\tsrc = dst->buf;\n+\t\tlen = dst->len;\n+\t}\n+\n \tif (!(conv_flags & CONV_EOL_KEEP_CRLF)) {\n \t\tret |= crlf_to_git(istate, path, src, len, dst, ca.crlf_action, conv_flags);\n \t\tif (ret && dst) {\n@@ -1167,6 +1312,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.working_tree_encoding, conv_flags);\n \tcrlf_to_git(istate, path, dst->buf, dst->len, dst, ca.crlf_action, conv_flags);\n \tident_to_git(path, dst->buf, dst->len, dst, ca.ident);\n }\n@@ -1198,6 +1344,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.working_tree_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@@ -1664,6 +1816,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.working_tree_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/convert.h b/convert.h\nindex 65ab3e5167..1d9539ed0b 100644\n--- a/convert.h\n+++ b/convert.h\n@@ -12,6 +12,7 @@ struct index_state;\n #define CONV_EOL_RNDTRP_WARN (1<<1) /* Warn if CRLF to LF to CRLF is different */\n #define CONV_EOL_RENORMALIZE (1<<2) /* Convert CRLF to LF */\n #define CONV_EOL_KEEP_CRLF (1<<3) /* Keep CRLF line endings as is */\n+#define CONV_WRITE_OBJECT (1<<4) /* Content is written to the index */\n \n extern int global_conv_flags_eol;\n \ndiff --git a/sha1_file.c b/sha1_file.c\nindex 6bc7c6ada9..e2f319d677 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -138,7 +138,7 @@ static int get_conv_flags(unsigned flags)\n \tif (flags & HASH_RENORMALIZE)\n \t\treturn CONV_EOL_RENORMALIZE;\n \telse if (flags & HASH_WRITE_OBJECT)\n-\t return global_conv_flags_eol;\n+\t\treturn global_conv_flags_eol | CONV_WRITE_OBJECT;\n \telse\n \t\treturn 0;\n }\ndiff --git a/t/t0028-working-tree-encoding.sh b/t/t0028-working-tree-encoding.sh\nnew file mode 100755\nindex 0000000000..a55bbcb6d3\n--- /dev/null\n+++ b/t/t0028-working-tree-encoding.sh\n@@ -0,0 +1,226 @@\n+#!/bin/sh\n+\n+test_description='working-tree-encoding conversion via gitattributes'\n+\n+. ./test-lib.sh\n+\n+test_expect_success 'setup test repo' '\n+\tgit config core.eol lf &&\n+\n+\ttext=\"hallo there!\\ncan you read me?\" &&\n+\techo \"*.utf16 text working-tree-encoding=utf-16\" >.gitattributes &&\n+\tprintf \"$text\" >test.utf8.raw &&\n+\tprintf \"$text\" | iconv -f UTF-8 -t UTF-16 >test.utf16.raw &&\n+\tprintf \"$text\" | iconv -f UTF-8 -t UTF-32 >test.utf32.raw &&\n+\tcp test.utf16.raw test.utf16 &&\n+\tcp test.utf32.raw test.utf32 &&\n+\n+\t# Add only UTF-16 file, we will add the UTF-32 file later\n+\tgit add .gitattributes test.utf16 &&\n+\tgit commit -m initial\n+'\n+\n+test_expect_success 'ensure UTF-8 is stored in Git' '\n+\tgit cat-file -p :test.utf16 >test.utf16.git &&\n+\ttest_cmp_bin test.utf8.raw test.utf16.git &&\n+\n+\t# cleanup\n+\trm test.utf16.git\n+'\n+\n+test_expect_success 're-encode to UTF-16 on checkout' '\n+\trm test.utf16 &&\n+\tgit checkout test.utf16 &&\n+\ttest_cmp_bin test.utf16.raw test.utf16 &&\n+\n+\t# cleanup\n+\trm test.utf16.raw\n+'\n+\n+test_expect_success 'check $GIT_DIR/info/attributes support' '\n+\techo \"*.utf32 text working-tree-encoding=utf-32\" >.git/info/attributes &&\n+\n+\tgit add test.utf32 &&\n+\n+\tgit cat-file -p :test.utf32 >test.utf32.git &&\n+\ttest_cmp_bin test.utf8.raw test.utf32.git &&\n+\n+\t# cleanup\n+\tgit reset --hard HEAD &&\n+\trm test.utf8.raw test.utf32.raw test.utf32.git\n+'\n+\n+test_expect_success 'check prohibited UTF BOM' '\n+\tprintf \"\\0a\\0b\\0c\" >nobom.utf16be.raw &&\n+\tprintf \"a\\0b\\0c\\0\" >nobom.utf16le.raw &&\n+\tprintf \"\\376\\777\\0a\\0b\\0c\" >bebom.utf16be.raw &&\n+\tprintf \"\\777\\376a\\0b\\0c\\0\" >lebom.utf16le.raw &&\n+\n+\tprintf \"\\0\\0\\0a\\0\\0\\0b\\0\\0\\0c\" >nobom.utf32be.raw &&\n+\tprintf \"a\\0\\0\\0b\\0\\0\\0c\\0\\0\\0\" >nobom.utf32le.raw &&\n+\tprintf \"\\0\\0\\376\\777\\0\\0\\0a\\0\\0\\0b\\0\\0\\0c\" >bebom.utf32be.raw &&\n+\tprintf \"\\777\\376\\0\\0a\\0\\0\\0b\\0\\0\\0c\\0\\0\\0\" >lebom.utf32le.raw &&\n+\n+\techo \"*.utf16be text working-tree-encoding=utf-16be\" >>.gitattributes &&\n+\techo \"*.utf16le text working-tree-encoding=utf-16le\" >>.gitattributes &&\n+\techo \"*.utf32be text working-tree-encoding=utf-32be\" >>.gitattributes &&\n+\techo \"*.utf32le text working-tree-encoding=utf-32le\" >>.gitattributes &&\n+\n+\t# Here we add a UTF-16 files with BOM (big-endian and little-endian)\n+\t# but we tell Git to treat it as UTF-16BE/UTF-16LE. In these cases\n+\t# the BOM is prohibited.\n+\tcp bebom.utf16be.raw bebom.utf16be &&\n+\ttest_must_fail git add bebom.utf16be 2>err.out &&\n+\ttest_i18ngrep \"fatal: BOM is prohibited .* UTF-16BE\" err.out &&\n+\n+\tcp lebom.utf16le.raw lebom.utf16be &&\n+\ttest_must_fail git add lebom.utf16be 2>err.out &&\n+\ttest_i18ngrep \"fatal: BOM is prohibited .* UTF-16BE\" err.out &&\n+\n+\tcp bebom.utf16be.raw bebom.utf16le &&\n+\ttest_must_fail git add bebom.utf16le 2>err.out &&\n+\ttest_i18ngrep \"fatal: BOM is prohibited .* UTF-16LE\" err.out &&\n+\n+\tcp lebom.utf16le.raw lebom.utf16le &&\n+\ttest_must_fail git add lebom.utf16le 2>err.out &&\n+\ttest_i18ngrep \"fatal: BOM is prohibited .* UTF-16LE\" err.out &&\n+\n+\t# ... and the same for UTF-32\n+\tcp bebom.utf32be.raw bebom.utf32be &&\n+\ttest_must_fail git add bebom.utf32be 2>err.out &&\n+\ttest_i18ngrep \"fatal: BOM is prohibited .* UTF-32BE\" err.out &&\n+\n+\tcp lebom.utf32le.raw lebom.utf32be &&\n+\ttest_must_fail git add lebom.utf32be 2>err.out &&\n+\ttest_i18ngrep \"fatal: BOM is prohibited .* UTF-32BE\" err.out &&\n+\n+\tcp bebom.utf32be.raw bebom.utf32le &&\n+\ttest_must_fail git add bebom.utf32le 2>err.out &&\n+\ttest_i18ngrep \"fatal: BOM is prohibited .* UTF-32LE\" err.out &&\n+\n+\tcp lebom.utf32le.raw lebom.utf32le &&\n+\ttest_must_fail git add lebom.utf32le 2>err.out &&\n+\ttest_i18ngrep \"fatal: BOM is prohibited .* UTF-32LE\" err.out &&\n+\n+\t# cleanup\n+\tgit reset --hard HEAD\n+'\n+\n+test_expect_success 'check required UTF BOM' '\n+\techo \"*.utf32 text working-tree-encoding=utf-32\" >>.gitattributes &&\n+\n+\tcp nobom.utf16be.raw nobom.utf16 &&\n+\ttest_must_fail git add nobom.utf16 2>err.out &&\n+\ttest_i18ngrep \"fatal: BOM is required .* UTF-16\" err.out &&\n+\n+\tcp nobom.utf16le.raw nobom.utf16 &&\n+\ttest_must_fail git add nobom.utf16 2>err.out &&\n+\ttest_i18ngrep \"fatal: BOM is required .* UTF-16\" err.out &&\n+\n+\tcp nobom.utf32be.raw nobom.utf32 &&\n+\ttest_must_fail git add nobom.utf32 2>err.out &&\n+\ttest_i18ngrep \"fatal: BOM is required .* UTF-32\" err.out &&\n+\n+\tcp nobom.utf32le.raw nobom.utf32 &&\n+\ttest_must_fail git add nobom.utf32 2>err.out &&\n+\ttest_i18ngrep \"fatal: BOM is required .* UTF-32\" err.out &&\n+\n+\t# cleanup\n+\trm nobom.utf16 nobom.utf32 &&\n+\tgit reset --hard HEAD\n+'\n+\n+test_expect_success 'eol conversion for UTF-16 encoded files on checkout' '\n+\tprintf \"one\\ntwo\\nthree\\n\" >lf.utf8.raw &&\n+\tprintf \"one\\r\\ntwo\\r\\nthree\\r\\n\" >crlf.utf8.raw &&\n+\n+\tcat lf.utf8.raw | iconv -f UTF-8 -t UTF-16 >lf.utf16.raw &&\n+\tcat crlf.utf8.raw | iconv -f UTF-8 -t UTF-16 >crlf.utf16.raw &&\n+\tcp crlf.utf16.raw eol.utf16 &&\n+\n+\tcat >expectIndexLF <<-\\EOF &&\n+\t\ti/lf w/-text attr/text \teol.utf16\n+\tEOF\n+\n+\tgit add eol.utf16 &&\n+\tgit commit -m eol &&\n+\n+\t# UTF-16 with CRLF (Windows line endings)\n+\trm eol.utf16 &&\n+\tgit -c core.eol=crlf checkout eol.utf16 &&\n+\ttest_cmp_bin crlf.utf16.raw eol.utf16 &&\n+\n+\t# Although the file has CRLF in the working tree, ensure LF in the index\n+\tgit ls-files --eol eol.utf16 >actual &&\n+\ttest_cmp expectIndexLF actual &&\n+\n+\t# UTF-16 with LF (Unix line endings)\n+\trm eol.utf16 &&\n+\tgit -c core.eol=lf checkout eol.utf16 &&\n+\ttest_cmp_bin lf.utf16.raw eol.utf16 &&\n+\n+\t# The file LF in the working tree, ensure LF in the index\n+\tgit ls-files --eol eol.utf16 >actual &&\n+\ttest_cmp expectIndexLF actual&&\n+\n+\trm crlf.utf16.raw crlf.utf8.raw lf.utf16.raw lf.utf8.raw &&\n+\n+\t# cleanup\n+\tgit reset --hard HEAD^\n+'\n+\n+test_expect_success 'check unsupported encodings' '\n+\n+\techo \"*.nothing text working-tree-encoding=\" >>.gitattributes &&\n+\tprintf \"nothing\" >t.nothing &&\n+\tgit add t.nothing &&\n+\n+\techo \"*.garbage text working-tree-encoding=garbage\" >>.gitattributes &&\n+\tprintf \"garbage\" >t.garbage &&\n+\ttest_must_fail git add t.garbage 2>err.out &&\n+\ttest_i18ngrep \"fatal: failed to encode\" err.out &&\n+\n+\t# cleanup\n+\trm err.out &&\n+\tgit reset --hard HEAD\n+'\n+\n+test_expect_success 'error if encoding round trip is not the same during refresh' '\n+\tBEFORE_STATE=$(git rev-parse HEAD) &&\n+\n+\t# Skip the UTF-16 filter for the added file\n+\t# This simulates a Git version that has no working tree encoding support\n+\techo \"hallo\" >nonsense.utf16 &&\n+\tTEST_HASH=$(git hash-object --no-filters -w nonsense.utf16) &&\n+\tgit update-index --add --cacheinfo 100644 $TEST_HASH nonsense.utf16 &&\n+\tCOMMIT=$(git commit-tree -p $(git rev-parse HEAD) -m \"plain commit\" $(git write-tree)) &&\n+\tgit update-ref refs/heads/master $COMMIT &&\n+\n+\ttest_must_fail git checkout HEAD^ 2>err.out &&\n+\ttest_i18ngrep \"error: .* overwritten by checkout:\" err.out &&\n+\n+\t# cleanup\n+\trm err.out &&\n+\tgit reset --hard $BEFORE_STATE\n+'\n+\n+test_expect_success 'error if encoding garbage is already in Git' '\n+\tBEFORE_STATE=$(git rev-parse HEAD) &&\n+\n+\t# Skip the UTF-16 filter for the added file\n+\t# This simulates a Git version that has no checkoutEncoding support\n+\tcp nobom.utf16be.raw nonsense.utf16 &&\n+\tTEST_HASH=$(git hash-object --no-filters -w nonsense.utf16) &&\n+\tgit update-index --add --cacheinfo 100644 $TEST_HASH nonsense.utf16 &&\n+\tCOMMIT=$(git commit-tree -p $(git rev-parse HEAD) -m \"plain commit\" $(git write-tree)) &&\n+\tgit update-ref refs/heads/master $COMMIT &&\n+\n+\tgit diff 2>err.out &&\n+\ttest_i18ngrep \"error: BOM is required\" err.out &&\n+\n+\t# cleanup\n+\trm err.out &&\n+\tgit reset --hard $BEFORE_STATE\n+'\n+\n+test_done\n-- \n2.16.1\n\n"},{"id":"340180","messageId":"20180224162801.98860-7-lars.schneider@autodesk.com","threadId":"47921","inReplyTo":"20180224162801.98860-1-lars.schneider@autodesk.com","subject":"[PATCH v8 6/7] convert: add tracing for 'working-tree-encoding' attribute","fromName":"","fromEmail":"lars.schneider@autodesk.com","sentAt":"2018-02-24T16:28:00Z","receivedAt":"2018-02-24T16:29:55Z","isPatch":true,"sender":{"key":"lars.schneider@autodesk.com","avatar":"https://gravatar.com/avatar/524188df3a38b02692619777bb90d9cc735db1ad75f5304e0bf09f8efd299775?d=mp&s=160"},"body":"From: Lars Schneider \n\nAdd the GIT_TRACE_WORKING_TREE_ENCODING environment variable to enable\ntracing for content that is reencoded with the 'working-tree-encoding'\nattribute. This is useful to debug encoding issues.\n\nSigned-off-by: Lars Schneider \n---\n convert.c | 25 +++++++++++++++++++++++++\n t/t0028-working-tree-encoding.sh | 2 ++\n 2 files changed, 27 insertions(+)\n\ndiff --git a/convert.c b/convert.c\nindex d20c341b6d..c4e2fd5fa5 100644\n--- a/convert.c\n+++ b/convert.c\n@@ -266,6 +266,29 @@ static int will_convert_lf_to_crlf(size_t len, struct text_stat *stats,\n \n }\n \n+static void trace_encoding(const char *context, const char *path,\n+\t\t\t const char *encoding, const char *buf, size_t len)\n+{\n+\tstatic struct trace_key coe = TRACE_KEY_INIT(WORKING_TREE_ENCODING);\n+\tstruct strbuf trace = STRBUF_INIT;\n+\tint i;\n+\n+\tstrbuf_addf(&trace, \"%s (%s, considered %s):\\n\", context, path, encoding);\n+\tfor (i = 0; i < len && buf; ++i) {\n+\t\tstrbuf_addf(\n+\t\t\t&trace,\"| \\e[2m%2i:\\e[0m %2x \\e[2m%c\\e[0m%c\",\n+\t\t\ti,\n+\t\t\t(unsigned char) buf[i],\n+\t\t\t(buf[i] > 32 && buf[i] < 127 ? buf[i] : ' '),\n+\t\t\t((i+1) % 8 && (i+1) < len ? ' ' : '\\n')\n+\t\t);\n+\t}\n+\tstrbuf_addchars(&trace, '\\n', 1);\n+\n+\ttrace_strbuf(&coe, &trace);\n+\tstrbuf_release(&trace);\n+}\n+\n static struct encoding {\n \tconst char *name;\n \tstruct encoding *next;\n@@ -325,6 +348,7 @@ static int encode_to_git(const char *path, const char *src, size_t src_len,\n \t\t\terror(error_msg, path, enc->name);\n \t}\n \n+\ttrace_encoding(\"source\", path, enc->name, src, src_len);\n \tdst = reencode_string_len(src, src_len, default_encoding, enc->name,\n \t\t\t\t &dst_len);\n \tif (!dst) {\n@@ -340,6 +364,7 @@ static int encode_to_git(const char *path, const char *src, size_t src_len,\n \t\telse\n \t\t\terror(msg, path, enc->name, default_encoding);\n \t}\n+\ttrace_encoding(\"destination\", path, default_encoding, dst, dst_len);\n \n \tstrbuf_attach(buf, dst, dst_len, dst_len + 1);\n \treturn 1;\ndiff --git a/t/t0028-working-tree-encoding.sh b/t/t0028-working-tree-encoding.sh\nindex a55bbcb6d3..8666669b2d 100755\n--- a/t/t0028-working-tree-encoding.sh\n+++ b/t/t0028-working-tree-encoding.sh\n@@ -4,6 +4,8 @@ test_description='working-tree-encoding conversion via gitattributes'\n \n . ./test-lib.sh\n \n+GIT_TRACE_WORKING_TREE_ENCODING=1 && export GIT_TRACE_WORKING_TREE_ENCODING\n+\n test_expect_success 'setup test repo' '\n \tgit config core.eol lf &&\n \n-- \n2.16.1\n\n"},{"id":"340181","messageId":"20180224162801.98860-5-lars.schneider@autodesk.com","threadId":"47921","inReplyTo":"20180224162801.98860-1-lars.schneider@autodesk.com","subject":"[PATCH v8 4/7] utf8: add function to detect a missing UTF-16/32 BOM","fromName":"","fromEmail":"lars.schneider@autodesk.com","sentAt":"2018-02-24T16:27:58Z","receivedAt":"2018-02-24T16:30:02Z","isPatch":true,"sender":{"key":"lars.schneider@autodesk.com","avatar":"https://gravatar.com/avatar/524188df3a38b02692619777bb90d9cc735db1ad75f5304e0bf09f8efd299775?d=mp&s=160"},"body":"From: Lars Schneider \n\nIf the endianness is not defined in the encoding name, then let's\nbe strict and require a BOM to avoid any encoding confusion. The\nis_missing_required_utf_bom() function returns true if a required BOM\nis missing.\n\nThe Unicode standard instructs to assume big-endian if there in no BOM\nfor UTF-16/32 [1][2]. However, the W3C/WHATWG encoding standard used\nin HTML5 recommends to assume little-endian to \"deal with deployed\ncontent\" [3]. Strictly requiring a BOM seems to be the safest option\nfor content in Git.\n\nThis function is used in a subsequent commit.\n\n[1] http://unicode.org/faq/utf_bom.html#gen6\n[2] http://www.unicode.org/versions/Unicode10.0.0/ch03.pdf\n Section 3.10, D98, page 132\n[3] https://encoding.spec.whatwg.org/#utf-16le\n\nSigned-off-by: Lars Schneider \n---\n utf8.c | 13 +++++++++++++\n utf8.h | 16 ++++++++++++++++\n 2 files changed, 29 insertions(+)\n\ndiff --git a/utf8.c b/utf8.c\nindex 914881cd1f..5113d26e56 100644\n--- a/utf8.c\n+++ b/utf8.c\n@@ -562,6 +562,19 @@ int has_prohibited_utf_bom(const char *enc, const char *data, size_t len)\n \t);\n }\n \n+int is_missing_required_utf_bom(const char *enc, const char *data, size_t len)\n+{\n+\treturn (\n+\t !strcmp(enc, \"UTF-16\") &&\n+\t !(has_bom_prefix(data, len, utf16_be_bom, sizeof(utf16_be_bom)) ||\n+\t has_bom_prefix(data, len, utf16_le_bom, sizeof(utf16_le_bom)))\n+\t) || (\n+\t !strcmp(enc, \"UTF-32\") &&\n+\t !(has_bom_prefix(data, len, utf32_be_bom, sizeof(utf32_be_bom)) ||\n+\t has_bom_prefix(data, len, utf32_le_bom, sizeof(utf32_le_bom)))\n+\t);\n+}\n+\n /*\n * Returns first character length in bytes for multi-byte `text` according to\n * `encoding`.\ndiff --git a/utf8.h b/utf8.h\nindex 4711429af9..62f86fba64 100644\n--- a/utf8.h\n+++ b/utf8.h\n@@ -79,4 +79,20 @@ void strbuf_utf8_align(struct strbuf *buf, align_type position, unsigned int wid\n */\n int has_prohibited_utf_bom(const char *enc, const char *data, size_t len);\n \n+/*\n+ * If the endianness is not defined in the encoding name, then we\n+ * require a BOM. The function returns true if a required BOM is missing.\n+ *\n+ * The Unicode standard instructs to assume big-endian if there\n+ * in no BOM for UTF-16/32 [1][2]. However, the W3C/WHATWG\n+ * encoding standard used in HTML5 recommends to assume\n+ * little-endian to \"deal with deployed content\" [3].\n+ *\n+ * [1] http://unicode.org/faq/utf_bom.html#gen6\n+ * [2] http://www.unicode.org/versions/Unicode10.0.0/ch03.pdf\n+ * Section 3.10, D98, page 132\n+ * [3] https://encoding.spec.whatwg.org/#utf-16le\n+ */\n+int is_missing_required_utf_bom(const char *enc, const char *data, size_t len);\n+\n #endif\n-- \n2.16.1\n\n"},{"id":"340182","messageId":"20180224162801.98860-8-lars.schneider@autodesk.com","threadId":"47921","inReplyTo":"20180224162801.98860-1-lars.schneider@autodesk.com","subject":"[PATCH v8 7/7] convert: add round trip check based on 'core.checkRoundtripEncoding'","fromName":"","fromEmail":"lars.schneider@autodesk.com","sentAt":"2018-02-24T16:28:01Z","receivedAt":"2018-02-24T16:30:05Z","isPatch":true,"sender":{"key":"lars.schneider@autodesk.com","avatar":"https://gravatar.com/avatar/524188df3a38b02692619777bb90d9cc735db1ad75f5304e0bf09f8efd299775?d=mp&s=160"},"body":"From: Lars Schneider \n\nUTF supports lossless conversion round tripping and conversions between\nUTF and other encodings are mostly round trip safe as Unicode aims to be\na superset of all other character encodings. However, certain encodings\n(e.g. SHIFT-JIS) are known to have round trip issues [1].\n\nAdd 'core.checkRoundtripEncoding', which contains a comma separated\nlist of encodings, to define for what encodings Git should check the\nconversion round trip if they are used in the 'working-tree-encoding'\nattribute.\n\nSet SHIFT-JIS as default value for 'core.checkRoundtripEncoding'.\n\n[1] https://support.microsoft.com/en-us/help/170559/prb-conversion-problem-between-shift-jis-and-unicode\n\nSigned-off-by: Lars Schneider \n---\n Documentation/config.txt | 6 ++++\n Documentation/gitattributes.txt | 8 +++++\n config.c | 5 +++\n convert.c | 74 ++++++++++++++++++++++++++++++++++++++++\n convert.h | 1 +\n environment.c | 1 +\n t/t0028-working-tree-encoding.sh | 41 ++++++++++++++++++++++\n 7 files changed, 136 insertions(+)\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex 0e25b2c92b..d7a56054a5 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -530,6 +530,12 @@ core.autocrlf::\n \tThis variable can be set to 'input',\n \tin which case no output conversion is performed.\n \n+core.checkRoundtripEncoding::\n+\tA comma separated list of encodings that Git performs UTF-8 round\n+\ttrip checks on if they are used in an `working-tree-encoding`\n+\tattribute (see linkgit:gitattributes[5]). The default value is\n+\t`SHIFT-JIS`.\n+\n core.symlinks::\n \tIf false, symbolic links are checked out as small plain files that\n \tcontain the link text. linkgit:git-update-index[1] and\ndiff --git a/Documentation/gitattributes.txt b/Documentation/gitattributes.txt\nindex eddeaee1f7..11315054f4 100644\n--- a/Documentation/gitattributes.txt\n+++ b/Documentation/gitattributes.txt\n@@ -310,6 +310,14 @@ number of pitfalls:\n internal contents as UTF-8 and try to convert it to UTF-16 on checkout.\n That operation will fail and cause an error.\n \n+- Reencoding content to non-UTF encodings can cause errors as the\n+ conversion might not be UTF-8 round trip safe. If you suspect your\n+ encoding to not be round trip safe, then add it to\n+ `core.checkRoundtripEncoding` to make Git check the round trip\n+ encoding (see linkgit:git-config[1]). SHIFT-JIS (Japanese character\n+ set) is known to have round trip issues with UTF-8 and is checked by\n+ default.\n+\n - Reencoding content requires resources that might slow down certain\n Git operations (e.g 'git checkout' or 'git add').\n \ndiff --git a/config.c b/config.c\nindex 1f003fbb90..d0ada9fcd4 100644\n--- a/config.c\n+++ b/config.c\n@@ -1172,6 +1172,11 @@ static int git_default_core_config(const char *var, const char *value)\n \t\treturn 0;\n \t}\n \n+\tif (!strcmp(var, \"core.checkroundtripencoding\")) {\n+\t\tcheck_roundtrip_encoding = xstrdup(value);\n+\t\treturn 0;\n+\t}\n+\n \tif (!strcmp(var, \"core.notesref\")) {\n \t\tnotes_ref_name = xstrdup(value);\n \t\treturn 0;\ndiff --git a/convert.c b/convert.c\nindex c4e2fd5fa5..398cd9cf7b 100644\n--- a/convert.c\n+++ b/convert.c\n@@ -289,6 +289,39 @@ static void trace_encoding(const char *context, const char *path,\n \tstrbuf_release(&trace);\n }\n \n+static int check_roundtrip(const char* enc_name)\n+{\n+\t/*\n+\t * check_roundtrip_encoding contains a string of space and/or\n+\t * comma separated encodings (eg. \"UTF-16, ASCII, CP1125\").\n+\t * Search for the given encoding in that string.\n+\t */\n+\tconst char *found = strcasestr(check_roundtrip_encoding, enc_name);\n+\tconst char *next = found + strlen(enc_name);\n+\tint len = strlen(check_roundtrip_encoding);\n+\treturn (found && (\n+\t\t\t/*\n+\t\t\t * check that the found encoding is at the\n+\t\t\t * beginning of check_roundtrip_encoding or\n+\t\t\t * that it is prefixed with a space or comma\n+\t\t\t */\n+\t\t\tfound == check_roundtrip_encoding || (\n+\t\t\t\tfound > check_roundtrip_encoding &&\n+\t\t\t\t(*(found-1) == ' ' || *(found-1) == ',')\n+\t\t\t)\n+\t\t) && (\n+\t\t\t/*\n+\t\t\t * check that the found encoding is at the\n+\t\t\t * end of check_roundtrip_encoding or\n+\t\t\t * that it is suffixed with a space or comma\n+\t\t\t */\n+\t\t\tnext == check_roundtrip_encoding + len || (\n+\t\t\t\tnext < check_roundtrip_encoding + len &&\n+\t\t\t\t(*next == ' ' || *next == ',')\n+\t\t\t)\n+\t\t));\n+}\n+\n static struct encoding {\n \tconst char *name;\n \tstruct encoding *next;\n@@ -366,6 +399,47 @@ static int encode_to_git(const char *path, const char *src, size_t src_len,\n \t}\n \ttrace_encoding(\"destination\", path, default_encoding, dst, dst_len);\n \n+\t/*\n+\t * UTF supports lossless conversion round tripping [1] and conversions\n+\t * between UTF and other encodings are mostly round trip safe as\n+\t * Unicode aims to be a superset of all other character encodings.\n+\t * However, certain encodings (e.g. SHIFT-JIS) are known to have round\n+\t * trip issues [2]. Check the round trip conversion for all encodings\n+\t * listed in core.checkRoundtripEncoding.\n+\t *\n+\t * The round trip check is only performed if content is written to Git.\n+\t * This ensures that no information is lost during conversion to/from\n+\t * the internal UTF-8 representation.\n+\t *\n+\t * Please note, the code below is not tested because I was not able to\n+\t * generate a faulty round trip without an iconv error. Iconv errors\n+\t * are already caught above.\n+\t *\n+\t * [1] http://unicode.org/faq/utf_bom.html#gen2\n+\t * [2] https://support.microsoft.com/en-us/help/170559/prb-conversion-problem-between-shift-jis-and-unicode\n+\t */\n+\tif ((conv_flags & CONV_WRITE_OBJECT) && check_roundtrip(enc->name)) {\n+\t\tchar *re_src;\n+\t\tint re_src_len;\n+\n+\t\tre_src = reencode_string_len(dst, dst_len,\n+\t\t\t\t\t enc->name, default_encoding,\n+\t\t\t\t\t &re_src_len);\n+\n+\t\ttrace_printf(\"Checking roundtrip encoding for %s...\\n\", enc->name);\n+\t\ttrace_encoding(\"reencoded source\", path, enc->name,\n+\t\t\t re_src, re_src_len);\n+\n+\t\tif (!re_src || src_len != re_src_len ||\n+\t\t memcmp(src, re_src, src_len)) {\n+\t\t\tconst char* msg = _(\"encoding '%s' from %s to %s and \"\n+\t\t\t\t\t \"back is not the same\");\n+\t\t\tdie(msg, path, enc->name, default_encoding);\n+\t\t}\n+\n+\t\tfree(re_src);\n+\t}\n+\n \tstrbuf_attach(buf, dst, dst_len, dst_len + 1);\n \treturn 1;\n }\ndiff --git a/convert.h b/convert.h\nindex 1d9539ed0b..765abfbd60 100644\n--- a/convert.h\n+++ b/convert.h\n@@ -56,6 +56,7 @@ struct delayed_checkout {\n };\n \n extern enum eol core_eol;\n+extern char *check_roundtrip_encoding;\n extern const char *get_cached_convert_stats_ascii(const struct index_state *istate,\n \t\t\t\t\t\t const char *path);\n extern const char *get_wt_convert_stats_ascii(const char *path);\ndiff --git a/environment.c b/environment.c\nindex 10a32c20ac..5bae9131ad 100644\n--- a/environment.c\n+++ b/environment.c\n@@ -50,6 +50,7 @@ int check_replace_refs = 1;\n char *git_replace_ref_base;\n enum eol core_eol = EOL_UNSET;\n int global_conv_flags_eol = CONV_EOL_RNDTRP_WARN;\n+char *check_roundtrip_encoding = \"SHIFT-JIS\";\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/t/t0028-working-tree-encoding.sh b/t/t0028-working-tree-encoding.sh\nindex 8666669b2d..e34c21eb29 100755\n--- a/t/t0028-working-tree-encoding.sh\n+++ b/t/t0028-working-tree-encoding.sh\n@@ -225,4 +225,45 @@ test_expect_success 'error if encoding garbage is already in Git' '\n \tgit reset --hard $BEFORE_STATE\n '\n \n+test_expect_success 'check roundtrip encoding' '\n+\ttext=\"hallo there!\\nroundtrip test here!\" &&\n+\tprintf \"$text\" | iconv -f UTF-8 -t SHIFT-JIS >roundtrip.shift &&\n+\tprintf \"$text\" | iconv -f UTF-8 -t UTF-16 >roundtrip.utf16 &&\n+\techo \"*.shift text working-tree-encoding=SHIFT-JIS\" >>.gitattributes &&\n+\n+\t# SHIFT-JIS encoded files are round-trip checked by default...\n+\tGIT_TRACE=1 git add .gitattributes roundtrip.shift 2>&1 >/dev/null |\n+\t\tgrep \"Checking roundtrip encoding for SHIFT-JIS\" &&\n+\tgit reset &&\n+\n+\t# ... unless we overwrite the Git config!\n+\ttest_config core.checkRoundtripEncoding \"garbage\" &&\n+\t! GIT_TRACE=1 git add .gitattributes roundtrip.shift 2>&1 >/dev/null |\n+\t\tgrep \"Checking roundtrip encoding for SHIFT-JIS\" &&\n+\ttest_unconfig core.checkRoundtripEncoding &&\n+\tgit reset &&\n+\n+\t# UTF-16 encoded files should not be round-trip checked by default...\n+\t! GIT_TRACE=1 git add roundtrip.utf16 2>&1 >/dev/null |\n+\t\tgrep \"Checking roundtrip encoding for UTF-16\" &&\n+\tgit reset &&\n+\n+\t# ... unless we tell Git to check it!\n+\ttest_config_global core.checkRoundtripEncoding \"UTF-16, UTF-32\" &&\n+\tGIT_TRACE=1 git add roundtrip.utf16 2>&1 >/dev/null |\n+\t\tgrep \"Checking roundtrip encoding for UTF-16\" &&\n+\tgit reset &&\n+\n+\t# ... unless we tell Git to check it!\n+\t# (here we also check that the casing of the encoding is irrelevant)\n+\ttest_config_global core.checkRoundtripEncoding \"UTF-32, utf-16\" &&\n+\tGIT_TRACE=1 git add roundtrip.utf16 2>&1 >/dev/null |\n+\t\tgrep \"Checking roundtrip encoding for UTF-16\" &&\n+\tgit reset &&\n+\n+\t# cleanup\n+\trm roundtrip.shift roundtrip.utf16 &&\n+\tgit reset --hard HEAD\n+'\n+\n test_done\n-- \n2.16.1\n\n"},{"id":"340193","messageId":"CAPig+cQdoicxSMLRbdCfFGXyXLirbRLVJi2QahCSe1ZesVOKBg@mail.gmail.com","threadId":"47921","inReplyTo":"20180224162801.98860-4-lars.schneider@autodesk.com","subject":"Re: [PATCH v8 3/7] utf8: add function to detect prohibited UTF-16/32 BOM","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2018-02-25T03:41:17Z","receivedAt":"2018-02-25T03:41:23Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sat, Feb 24, 2018 at 11:27 AM, wrote:\n> Whenever a data stream is declared to be UTF-16BE, UTF-16LE, UTF-32BE\n> or UTF-32LE a BOM must not be used [1]. The function returns true if\n> this is the case.\n>\n> [1] http://unicode.org/faq/utf_bom.html#bom10\n>\n> Signed-off-by: Lars Schneider \n> ---\n> diff --git a/utf8.c b/utf8.c\n> @@ -538,6 +538,30 @@ char *reencode_string_len(const char *in, int insz,\n> +int has_prohibited_utf_bom(const char *enc, const char *data, size_t len)\n> +{\n> + return (\n> + (!strcmp(enc, \"UTF-16BE\") || !strcmp(enc, \"UTF-16LE\")) &&\n> + (has_bom_prefix(data, len, utf16_be_bom, sizeof(utf16_be_bom)) ||\n> + has_bom_prefix(data, len, utf16_le_bom, sizeof(utf16_le_bom)))\n> + ) || (\n> + (!strcmp(enc, \"UTF-32BE\") || !strcmp(enc, \"UTF-32LE\")) &&\n> + (has_bom_prefix(data, len, utf32_be_bom, sizeof(utf32_be_bom)) ||\n> + has_bom_prefix(data, len, utf32_le_bom, sizeof(utf32_le_bom)))\n> + );\n> +}\n\nIs this interpretation correct? When I read [1], I interpret it as\nsaying that no BOM _of any sort_ should be present when the encoding\nis declared as one of UTF-16BE, UTF-16LE, UTF-32BE, or UTF-32LE. This\ncode, on the other hand, only checks for BOMs corresponding to the\ndeclared size (16 or 32 bits).\n\nI suppose the intention of [1] is to detect a mismatch between the\ndeclared encoding and how the stream is actually encoded. The check\nimplemented here will fail to detect a mismatch between, say, declared\nencoding UTF-16BE and actual encoding UTF-32BE.\n"},{"id":"340195","messageId":"CAPig+cSX9uLWi++3pOis+iPGqEbrAL5px70CB-aUjDVGYZqweA@mail.gmail.com","threadId":"47921","inReplyTo":"20180224162801.98860-5-lars.schneider@autodesk.com","subject":"Re: [PATCH v8 4/7] utf8: add function to detect a missing UTF-16/32 BOM","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2018-02-25T03:52:24Z","receivedAt":"2018-02-25T03:52:30Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sat, Feb 24, 2018 at 11:27 AM, wrote:\n> If the endianness is not defined in the encoding name, then let's\n> be strict and require a BOM to avoid any encoding confusion. The\n> is_missing_required_utf_bom() function returns true if a required BOM\n> is missing.\n>\n> The Unicode standard instructs to assume big-endian if there in no BOM\n> for UTF-16/32 [1][2]. However, the W3C/WHATWG encoding standard used\n> in HTML5 recommends to assume little-endian to \"deal with deployed\n> content\" [3]. Strictly requiring a BOM seems to be the safest option\n> for content in Git.\n>\n> Signed-off-by: Lars Schneider \n> ---\n> diff --git a/utf8.h b/utf8.h\n> @@ -79,4 +79,20 @@ void strbuf_utf8_align(struct strbuf *buf, align_type position, unsigned int wid\n> +/*\n> + * If the endianness is not defined in the encoding name, then we\n> + * require a BOM. The function returns true if a required BOM is missing.\n> + *\n> + * The Unicode standard instructs to assume big-endian if there\n> + * in no BOM for UTF-16/32 [1][2]. However, the W3C/WHATWG\n> + * encoding standard used in HTML5 recommends to assume\n> + * little-endian to \"deal with deployed content\" [3].\n\nPerhaps you could tack on to the comment here the final bit of\nexplanation from the commit message which ties these conflicting\nrecommendations together. In particular:\n\n Therefore, strictly requiring a BOM seems to be the\n safest option for content in Git.\n\n> + */\n> +int is_missing_required_utf_bom(const char *enc, const char *data, size_t len);\n"},{"id":"340196","messageId":"CAPig+cRtp5g0iDd3pHybZXd_VkGG2-a4CdCZdiqN55Cr1T5Zyg@mail.gmail.com","threadId":"47921","inReplyTo":"20180224162801.98860-6-lars.schneider@autodesk.com","subject":"Re: [PATCH v8 5/7] convert: add 'working-tree-encoding' attribute","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2018-02-25T07:15:30Z","receivedAt":"2018-02-25T07:15:39Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sat, Feb 24, 2018 at 11:27 AM, wrote:\n> Git recognizes files encoded with ASCII or one of its supersets (e.g.\n> UTF-8 or ISO-8859-1) as text files. All other encodings are usually\n> interpreted as binary and consequently built-in Git text processing\n> tools (e.g. 'git diff') as well as most Git web front ends do not\n> visualize the content.\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> Signed-off-by: Lars Schneider \n> ---\n> diff --git a/Documentation/gitattributes.txt b/Documentation/gitattributes.txt\n> @@ -272,6 +272,84 @@ few exceptions. Even though...\n> +Please note that using the `working-tree-encoding` attribute may have a\n> +number of pitfalls:\n> +\n> +- Alternative Git implementations (e.g. JGit or libgit2) and older Git\n> + versions (as of March 2018) do not support the `working-tree-encoding`\n> + attribute. If you decide to use the `working-tree-encoding` attribute\n> + in your repository, then it is strongly recommended to ensure that all\n> + clients working with the repository support it.\n> +\n> + If you declare `*.proj` files as UTF-16 and you add `foo.proj` with an\n> + `working-tree-encoding` enabled Git client, then `foo.proj` will be\n> + stored as UTF-8 internally. A client without `working-tree-encoding`\n> + support will checkout `foo.proj` as UTF-8 encoded file. This will\n> + typically cause trouble for the users of this file.\n\nThe above paragraph is giving an example of the scenario described in\nthe paragraph above it. It might, therefore, be clearer to start this\nparagraph with \"For example,...\". Also, as an aid to non-Windows\ndevelopers, it might be helpful to introduce \".proj\" files as\n\"Microsoft Visual Studio `*.proj` files...\".\n\n> diff --git a/convert.c b/convert.c\n> @@ -265,6 +266,110 @@ static int will_convert_lf_to_crlf(size_t len, struct text_stat *stats,\n> +static struct encoding {\n> + const char *name;\n> + struct encoding *next;\n> +} *encoding, **encoding_tail;\n\nWhy does this struct need to be a linked-list since it contains only a\nname? In fact, why do the struct and linked list exist at all? Back in\nv1 when the struct held more members, it made sense to place them in a\ncollection so they could be re-used, but today it just seems an\nunnecessary artifact which buys you nothing.\n\nIs the plan that some future patch series will add members to the\nstruct? If not, then it seems that it should be retired altogether.\n\n> +static const char *default_encoding = \"UTF-8\";\n> +\n> +static int encode_to_git(const char *path, const char *src, size_t src_len,\n> + struct strbuf *buf, struct encoding *enc, int conv_flags)\n> +{\n> + [...]\n> + if (has_prohibited_utf_bom(enc->name, src, src_len)) {\n> + const char *error_msg = _(\n> + \"BOM is prohibited in '%s' if encoded as %s\");\n> + /*\n> + * This advise is shown for UTF-??BE and UTF-??LE encodings.\n\ns/advise/advice/\n\n> + * We truncate the encoding name to 6 chars with %.6s to cut\n> + * off the last two \"byte order\" characters.\n> + */\n> + const char *advise_msg = _(\n> + \"The file '%s' contains a byte order mark (BOM). \"\n> + \"Please use %.6s as working-tree-encoding.\");\n> + advise(advise_msg, path, enc->name);\n> + if (conv_flags & CONV_WRITE_OBJECT)\n> + die(error_msg, path, enc->name);\n> + else\n> + error(error_msg, path, enc->name);\n\nWhat is the intended behavior in the non-die() case? As implemented,\nthis is still going to attempt the conversion even though it threw an\nerror. Should it be returning 0 here instead? Same question for the\ncouple additional cases below.\n\n> +\n> + } else if (is_missing_required_utf_bom(enc->name, src, src_len)) {\n> + const char *error_msg = _(\n> + \"BOM is required in '%s' if encoded as %s\");\n> + const char *advise_msg = _(\n> + \"The file '%s' is missing a byte order mark (BOM). \"\n> + \"Please use %sBE or %sLE (depending on the byte order) \"\n> + \"as working-tree-encoding.\");\n> + advise(advise_msg, path, enc->name, enc->name);\n> + if (conv_flags & CONV_WRITE_OBJECT)\n> + die(error_msg, path, enc->name);\n> + else\n> + error(error_msg, path, enc->name);\n> + }\n\nFor a function which presumably is agnostic about the working-tree\nencoding (supporting whatever iconv does), it nevertheless has\nunusually intimate knowledge about UTF BOMs, in general, and the\ninternals of has_prohibited_utf_bom() and\nis_missing_required_utf_bom(), in particular. It might be cleaner, and\nwould simplify this function, if all this UTF BOM-specific gunk was\nmoved elsewhere and generalized into a \"validate conversion\" function.\n\nAs this series is already at v8, perhaps such cleanup could be done as\na follow-on series (by someone) rather than re-rolling.\n\n> + dst = reencode_string_len(src, src_len, default_encoding, enc->name,\n> + &dst_len);\n> + if (!dst) {\n> + /*\n> + * We could add the blob \"as-is\" to Git. However, on checkout\n> + * we would try to reencode to the original encoding. This\n> + * would fail and we would leave the user with a messed-up\n> + * working tree. Let's try to avoid this by screaming loud.\n> + */\n> + const char* msg = _(\"failed to encode '%s' from %s to %s\");\n> + if (conv_flags & CONV_WRITE_OBJECT)\n> + die(msg, path, enc->name, default_encoding);\n> + else\n> + error(msg, path, enc->name, default_encoding);\n> + }\n> +\n> + strbuf_attach(buf, dst, dst_len, dst_len + 1);\n> + return 1;\n> +}\n> +\n> +static int encode_to_worktree(const char *path, const char *src, size_t src_len,\n> + struct strbuf *buf, struct encoding *enc)\n> +{\n> + [...]\n> + dst = reencode_string_len(src, src_len, enc->name, default_encoding,\n> + &dst_len);\n> + if (!dst) {\n> + error(\"failed to encode '%s' from %s to %s\",\n> + path, enc->name, default_encoding);\n\nThe last two arguments need to be swapped.\n\n> + return 0;\n> + }\n> +\n> + strbuf_attach(buf, dst, dst_len, dst_len + 1);\n> + return 1;\n> +}\n> @@ -978,6 +1083,35 @@ static int ident_to_worktree(const char *path, const char *src, size_t len,\n> +static struct encoding *git_path_check_encoding(struct attr_check_item *check)\n> +{\n> + if (ATTR_TRUE(value) || ATTR_FALSE(value) || ATTR_UNSET(value) ||\n> + !strlen(value))\n> + return NULL;\n> +\n> + for (enc = encoding; enc; enc = enc->next)\n> + if (!strcasecmp(value, enc->name))\n> + return enc;\n> +\n> + /* Don't encode to the default encoding */\n> + if (!strcasecmp(value, default_encoding))\n> + return NULL;\n\nYou could handle this easy-to-detect case before attempting the more\nexpensive loop just above. (Not necessarily worth a re-roll.)\n\n> + enc = xcalloc(1, sizeof(*enc));\n> + /*\n> + * Ensure encoding names are always upper case (e.g. UTF-8) to\n> + * simplify subsequent string comparisons.\n> + */\n> + enc->name = xstrdup_toupper(value);\n> + *encoding_tail = enc;\n> + encoding_tail = &(enc->next);\n> +\n> + return enc;\n> +}\n> diff --git a/t/t0028-working-tree-encoding.sh b/t/t0028-working-tree-encoding.sh\n> @@ -0,0 +1,226 @@\n> +test_expect_success 'ensure UTF-8 is stored in Git' '\n> + git cat-file -p :test.utf16 >test.utf16.git &&\n> + test_cmp_bin test.utf8.raw test.utf16.git &&\n> +\n> + # cleanup\n> + rm test.utf16.git\n> +'\n\nThis cleanup won't take place if test_cmp_bin() fails. Instead,\ncleanups are typically handled by test_when_finished() to ensure that\nthe cleanup action occurs even if the test fails.\n\n test_expect_success 'ensure UTF-8 is stored in Git' '\n test_when_finished \"rm -f test.utf16.git\" &&\n ...\n\nSame comment for remaining tests.\n\n> +test_expect_success 'check prohibited UTF BOM' '\n> + printf \"\\0a\\0b\\0c\" >nobom.utf16be.raw &&\n> + printf \"a\\0b\\0c\\0\" >nobom.utf16le.raw &&\n> + printf \"\\376\\777\\0a\\0b\\0c\" >bebom.utf16be.raw &&\n> + printf \"\\777\\376a\\0b\\0c\\0\" >lebom.utf16le.raw &&\n> +\n> + printf \"\\0\\0\\0a\\0\\0\\0b\\0\\0\\0c\" >nobom.utf32be.raw &&\n> + printf \"a\\0\\0\\0b\\0\\0\\0c\\0\\0\\0\" >nobom.utf32le.raw &&\n> + printf \"\\0\\0\\376\\777\\0\\0\\0a\\0\\0\\0b\\0\\0\\0c\" >bebom.utf32be.raw &&\n> + printf \"\\777\\376\\0\\0a\\0\\0\\0b\\0\\0\\0c\\0\\0\\0\" >lebom.utf32le.raw &&\n\nThese resources seem to be used by multiple tests. Perhaps create them\nin a distinct \"setup\" test instead of bundling them in this test?\n\n> + echo \"*.utf16be text working-tree-encoding=utf-16be\" >>.gitattributes &&\n> + echo \"*.utf16le text working-tree-encoding=utf-16le\" >>.gitattributes &&\n> + echo \"*.utf32be text working-tree-encoding=utf-32be\" >>.gitattributes &&\n> + echo \"*.utf32le text working-tree-encoding=utf-32le\" >>.gitattributes &&\n> +\n> + # Here we add a UTF-16 files with BOM (big-endian and little-endian)\n> + # but we tell Git to treat it as UTF-16BE/UTF-16LE. In these cases\n> + # the BOM is prohibited.\n> + cp bebom.utf16be.raw bebom.utf16be &&\n> + test_must_fail git add bebom.utf16be 2>err.out &&\n> + test_i18ngrep \"fatal: BOM is prohibited .* UTF-16BE\" err.out &&\n> +\n> + cp lebom.utf16le.raw lebom.utf16be &&\n> + test_must_fail git add lebom.utf16be 2>err.out &&\n> + test_i18ngrep \"fatal: BOM is prohibited .* UTF-16BE\" err.out &&\n> +\n> + cp bebom.utf16be.raw bebom.utf16le &&\n> + test_must_fail git add bebom.utf16le 2>err.out &&\n> + test_i18ngrep \"fatal: BOM is prohibited .* UTF-16LE\" err.out &&\n> +\n> + cp lebom.utf16le.raw lebom.utf16le &&\n> + test_must_fail git add lebom.utf16le 2>err.out &&\n> + test_i18ngrep \"fatal: BOM is prohibited .* UTF-16LE\" err.out &&\n> +\n> + # ... and the same for UTF-32\n> + cp bebom.utf32be.raw bebom.utf32be &&\n> + test_must_fail git add bebom.utf32be 2>err.out &&\n> + test_i18ngrep \"fatal: BOM is prohibited .* UTF-32BE\" err.out &&\n> +\n> + cp lebom.utf32le.raw lebom.utf32be &&\n> + test_must_fail git add lebom.utf32be 2>err.out &&\n> + test_i18ngrep \"fatal: BOM is prohibited .* UTF-32BE\" err.out &&\n> +\n> + cp bebom.utf32be.raw bebom.utf32le &&\n> + test_must_fail git add bebom.utf32le 2>err.out &&\n> + test_i18ngrep \"fatal: BOM is prohibited .* UTF-32LE\" err.out &&\n> +\n> + cp lebom.utf32le.raw lebom.utf32le &&\n> + test_must_fail git add lebom.utf32le 2>err.out &&\n> + test_i18ngrep \"fatal: BOM is prohibited .* UTF-32LE\" err.out &&\n> +\n> + # cleanup\n> + git reset --hard HEAD\n> +'\n\nClumping all these checks into the same test makes it difficult to\ndetermine which one failed (if one does fail). Also, the amount of\ncopy/paste code is easy to get wrong and a bit onerous to review.\nAutomating via for-loops would address these concerns. For instance,\nto check all combinations (not just 8 combinations tested above):\n\n for i in 16be 16le 32be 32le\n do\n for j in 16be 16le 32be 32le\n do\n test_expect_success \"check prohibited UTF BOM utf$i/utf$j\" '\n test_when_finished \"git reset --hard HEAD\" &&\n cp bebom.utf$i.raw bebom.utf$j &&\n test_must_fail git add bebom.utf$j 2>err.out &&\n J=$(echo $j | tr bel BEL) &&\n test_i18ngrep \"fatal: BOM is prohibited .* UTF-$J\" err.out\n '\n done\n done\n\nAlternately, the test could be moved to a function and called for each\ncombination:\n\n check_prohibited_bom () {\n i=$1\n j=$2\n test_expect_success \"check prohibited UTF BOM utf$i/utf$j\" '\n ...\n '\n }\n check_prohibited_bom 16be 16be\n check_prohibited_bom 16be 16le\n ...\n\nSame comment regarding copy/paste in tests below.\n\nProbably not worth a re-roll.\n\n> +test_expect_success 'check required UTF BOM' '\n> + echo \"*.utf32 text working-tree-encoding=utf-32\" >>.gitattributes &&\n> +\n> + cp nobom.utf16be.raw nobom.utf16 &&\n> + test_must_fail git add nobom.utf16 2>err.out &&\n> + test_i18ngrep \"fatal: BOM is required .* UTF-16\" err.out &&\n> +\n> + cp nobom.utf16le.raw nobom.utf16 &&\n> + test_must_fail git add nobom.utf16 2>err.out &&\n> + test_i18ngrep \"fatal: BOM is required .* UTF-16\" err.out &&\n> +\n> + cp nobom.utf32be.raw nobom.utf32 &&\n> + test_must_fail git add nobom.utf32 2>err.out &&\n> + test_i18ngrep \"fatal: BOM is required .* UTF-32\" err.out &&\n> +\n> + cp nobom.utf32le.raw nobom.utf32 &&\n> + test_must_fail git add nobom.utf32 2>err.out &&\n> + test_i18ngrep \"fatal: BOM is required .* UTF-32\" err.out &&\n> +\n> + # cleanup\n> + rm nobom.utf16 nobom.utf32 &&\n> + git reset --hard HEAD\n> +'\n"},{"id":"340205","messageId":"392C5E11-A3CA-4D65-B9D4-9EE30258C83A@gmail.com","threadId":"47921","inReplyTo":"CAPig+cQdoicxSMLRbdCfFGXyXLirbRLVJi2QahCSe1ZesVOKBg@mail.gmail.com","subject":"Re: [PATCH v8 3/7] utf8: add function to detect prohibited UTF-16/32 BOM","fromName":"Lars Schneider","fromEmail":"larsxschneider@gmail.com","sentAt":"2018-02-25T11:35:35Z","receivedAt":"2018-02-25T11:35:45Z","isPatch":true,"sender":{"key":"larsxschneider@gmail.com","avatar":"https://avatars.githubusercontent.com/u/477434?v=4"},"body":"\n> On 25 Feb 2018, at 04:41, Eric Sunshine wrote:\n> \n> On Sat, Feb 24, 2018 at 11:27 AM, wrote:\n>> Whenever a data stream is declared to be UTF-16BE, UTF-16LE, UTF-32BE\n>> or UTF-32LE a BOM must not be used [1]. The function returns true if\n>> this is the case.\n>> \n>> [1] http://unicode.org/faq/utf_bom.html#bom10\n>> \n>> Signed-off-by: Lars Schneider \n>> ---\n>> diff --git a/utf8.c b/utf8.c\n>> @@ -538,6 +538,30 @@ char *reencode_string_len(const char *in, int insz,\n>> +int has_prohibited_utf_bom(const char *enc, const char *data, size_t len)\n>> +{\n>> + return (\n>> + (!strcmp(enc, \"UTF-16BE\") || !strcmp(enc, \"UTF-16LE\")) &&\n>> + (has_bom_prefix(data, len, utf16_be_bom, sizeof(utf16_be_bom)) ||\n>> + has_bom_prefix(data, len, utf16_le_bom, sizeof(utf16_le_bom)))\n>> + ) || (\n>> + (!strcmp(enc, \"UTF-32BE\") || !strcmp(enc, \"UTF-32LE\")) &&\n>> + (has_bom_prefix(data, len, utf32_be_bom, sizeof(utf32_be_bom)) ||\n>> + has_bom_prefix(data, len, utf32_le_bom, sizeof(utf32_le_bom)))\n>> + );\n>> +}\n> \n> Is this interpretation correct? When I read [1], I interpret it as\n> saying that no BOM _of any sort_ should be present when the encoding\n> is declared as one of UTF-16BE, UTF-16LE, UTF-32BE, or UTF-32LE.\n\nCorrect!\n\n> This\n> code, on the other hand, only checks for BOMs corresponding to the\n> declared size (16 or 32 bits).\n\nHmm. Interesting thought. You are saying that my code won't complain if\na document declared as UTF-16LE has a UTF32-LE BOM, correct? I would say\nthis is correct behavior in context of this function. This function assumes\nthat the document is proper UTF-16/UTF-16BE/UTF-16LE but it is wrongly\ndeclared with respect to its BOM in the .gitattributes. Would this\ncomment make it more clear to you?\n\n\t/*\n\t * If a data stream is declared as UTF-16BE or UTF-16LE, then a UTF-16\n\t * BOM must not be used [1]. The same applies for the UTF-32 equivalents.\n\t * The function returns true if this rule is violated.\n\t *\n\t * [1] http://unicode.org/faq/utf_bom.html#bom10\n\t */\n\n\nI think what you are referring to is a different class of error and\nwould therefore warrant its own checker function. Would you agree?\n\n\n> I suppose the intention of [1] is to detect a mismatch between the\n> declared encoding and how the stream is actually encoded. The check\n> implemented here will fail to detect a mismatch between, say, declared\n> encoding UTF-16BE and actual encoding UTF-32BE.\n\nAs stated above the intention is to detect wrong BOMs! I think we cannot \ndetect the \"declared as UTF-16BE but actually UTF-32BE\" error.\n\nConsider this:\n\nprintf \"test\" | iconv -f UTF-8 -t UTF-32BE | iconv -f UTF-16BE -t UTF-8 | od -c\n0000000 \\0 t \\0 e \\0 s \\0 t\n0000010\n\nIn the first step we \"encode\" the string to UTF-32BE and then we \"decode\" it as\nUTF-16BE. The result is valid although not correct. Does this make sense?\n\nThanks,\nLars\n\n\n"},{"id":"340206","messageId":"8EE59DC3-69E0-412B-AC50-5D348D6D5BE0@gmail.com","threadId":"47921","inReplyTo":"CAPig+cSX9uLWi++3pOis+iPGqEbrAL5px70CB-aUjDVGYZqweA@mail.gmail.com","subject":"Re: [PATCH v8 4/7] utf8: add function to detect a missing UTF-16/32 BOM","fromName":"Lars Schneider","fromEmail":"larsxschneider@gmail.com","sentAt":"2018-02-25T11:41:34Z","receivedAt":"2018-02-25T11:41:43Z","isPatch":true,"sender":{"key":"larsxschneider@gmail.com","avatar":"https://avatars.githubusercontent.com/u/477434?v=4"},"body":"\n> On 25 Feb 2018, at 04:52, Eric Sunshine wrote:\n> \n> On Sat, Feb 24, 2018 at 11:27 AM, wrote:\n>> If the endianness is not defined in the encoding name, then let's\n>> be strict and require a BOM to avoid any encoding confusion. The\n>> is_missing_required_utf_bom() function returns true if a required BOM\n>> is missing.\n>> \n>> The Unicode standard instructs to assume big-endian if there in no BOM\n>> for UTF-16/32 [1][2]. However, the W3C/WHATWG encoding standard used\n>> in HTML5 recommends to assume little-endian to \"deal with deployed\n>> content\" [3]. Strictly requiring a BOM seems to be the safest option\n>> for content in Git.\n>> \n>> Signed-off-by: Lars Schneider \n>> ---\n>> diff --git a/utf8.h b/utf8.h\n>> @@ -79,4 +79,20 @@ void strbuf_utf8_align(struct strbuf *buf, align_type position, unsigned int wid\n>> +/*\n>> + * If the endianness is not defined in the encoding name, then we\n>> + * require a BOM. The function returns true if a required BOM is missing.\n>> + *\n>> + * The Unicode standard instructs to assume big-endian if there\n>> + * in no BOM for UTF-16/32 [1][2]. However, the W3C/WHATWG\n>> + * encoding standard used in HTML5 recommends to assume\n>> + * little-endian to \"deal with deployed content\" [3].\n> \n> Perhaps you could tack on to the comment here the final bit of\n> explanation from the commit message which ties these conflicting\n> recommendations together. In particular:\n> \n> Therefore, strictly requiring a BOM seems to be the\n> safest option for content in Git.\n\nAgreed. I'll change it.\n\nThanks,\nLars"},{"id":"340227","messageId":"CAPig+cR81J3fTOtrgAumAs=RC5hqYFfSmeb-ru-Yf_ahFuBiew@mail.gmail.com","threadId":"47921","inReplyTo":"20180224162801.98860-8-lars.schneider@autodesk.com","subject":"Re: [PATCH v8 7/7] convert: add round trip check based on 'core.checkRoundtripEncoding'","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2018-02-25T19:50:56Z","receivedAt":"2018-02-25T19:51:03Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sat, Feb 24, 2018 at 11:28 AM, wrote:\n> UTF supports lossless conversion round tripping and conversions between\n> UTF and other encodings are mostly round trip safe as Unicode aims to be\n> a superset of all other character encodings. However, certain encodings\n> (e.g. SHIFT-JIS) are known to have round trip issues [1].\n>\n> Add 'core.checkRoundtripEncoding', which contains a comma separated\n> list of encodings, to define for what encodings Git should check the\n> conversion round trip if they are used in the 'working-tree-encoding'\n> attribute.\n>\n> Set SHIFT-JIS as default value for 'core.checkRoundtripEncoding'.\n>\n> [1] https://support.microsoft.com/en-us/help/170559/prb-conversion-problem-between-shift-jis-and-unicode\n>\n> Signed-off-by: Lars Schneider \n> ---\n> diff --git a/convert.c b/convert.c\n> @@ -289,6 +289,39 @@ static void trace_encoding(const char *context, const char *path,\n> +static int check_roundtrip(const char* enc_name)\n> +{\n> + /*\n> + * check_roundtrip_encoding contains a string of space and/or\n> + * comma separated encodings (eg. \"UTF-16, ASCII, CP1125\").\n> + * Search for the given encoding in that string.\n> + */\n> + const char *found = strcasestr(check_roundtrip_encoding, enc_name);\n> + const char *next = found + strlen(enc_name);\n\nIs this pointer arithmetic undefined behavior (according to the C\nstandard) if NULL is returned by strcasestr()? If so, how comfortable\nare we with this? Perhaps if you add an 'if' into the mix, you can\navoid it:\n\n if (found) {\n const char *next = found + strlen(enc_name);\n return ...long complicated expression...;\n }\n return false;\n\n> + int len = strlen(check_roundtrip_encoding);\n> + return (found && (\n> + /*\n> + * check that the found encoding is at the\n> + * beginning of check_roundtrip_encoding or\n> + * that it is prefixed with a space or comma\n> + */\n> + found == check_roundtrip_encoding || (\n> + found > check_roundtrip_encoding &&\n> + (*(found-1) == ' ' || *(found-1) == ',')\n\nAlthough the 'found > check_roundtrip_encoding' expression is\neffectively dead code in this case, it does document that the\nprogrammer didn't just blindly use '*(found-1)' without taking\nboundary conditions into consideration.\n\n(It's dead code because, at this point, we know that strcasestr()\ndidn't return NULL and we know that 'found' doesn't equal\n'check_roundtrip_encoding', therefore it _must_ be greater than\n'check_roundtrip_encoding'.)\n\nJust an observation; not a request to remove the dead code since it\nhas some documentation value.\n\n> + )\n> + ) && (\n> + /*\n> + * check that the found encoding is at the\n> + * end of check_roundtrip_encoding or\n> + * that it is suffixed with a space or comma\n> + */\n> + next == check_roundtrip_encoding + len || (\n> + next < check_roundtrip_encoding + len &&\n> + (*next == ' ' || *next == ',')\n> + )\n> + ));\n> +}\n> @@ -366,6 +399,47 @@ static int encode_to_git(const char *path, const char *src, size_t src_len,\n> + if ((conv_flags & CONV_WRITE_OBJECT) && check_roundtrip(enc->name)) {\n> + re_src = reencode_string_len(dst, dst_len,\n> + enc->name, default_encoding,\n> + &re_src_len);\n> +\n> + trace_printf(\"Checking roundtrip encoding for %s...\\n\", enc->name);\n> + trace_encoding(\"reencoded source\", path, enc->name,\n> + re_src, re_src_len);\n> +\n> + if (!re_src || src_len != re_src_len ||\n> + memcmp(src, re_src, src_len)) {\n> + const char* msg = _(\"encoding '%s' from %s to %s and \"\n> + \"back is not the same\");\n> + die(msg, path, enc->name, default_encoding);\n\nLast two arguments need to be swapped.\n\n> + }\n> +\n> + free(re_src);\n> + }\n> +\n> strbuf_attach(buf, dst, dst_len, dst_len + 1);\n> return 1;\n> }\n> diff --git a/t/t0028-working-tree-encoding.sh b/t/t0028-working-tree-encoding.sh\n> @@ -225,4 +225,45 @@ test_expect_success 'error if encoding garbage is already in Git' '\n> +test_expect_success 'check roundtrip encoding' '\n> + text=\"hallo there!\\nroundtrip test here!\" &&\n> + printf \"$text\" | iconv -f UTF-8 -t SHIFT-JIS >roundtrip.shift &&\n> + printf \"$text\" | iconv -f UTF-8 -t UTF-16 >roundtrip.utf16 &&\n> + echo \"*.shift text working-tree-encoding=SHIFT-JIS\" >>.gitattributes &&\n> +\n> + # SHIFT-JIS encoded files are round-trip checked by default...\n> + GIT_TRACE=1 git add .gitattributes roundtrip.shift 2>&1 >/dev/null |\n> + grep \"Checking roundtrip encoding for SHIFT-JIS\" &&\n\nWhy redirect to /dev/null? If something does go wrong somewhere, the\nmore output available for debugging the problem, the better, so\nthrowing it away unnecessarily seems contraindicated.\n\n> + git reset &&\n> +\n> + # ... unless we overwrite the Git config!\n> + test_config core.checkRoundtripEncoding \"garbage\" &&\n> + ! GIT_TRACE=1 git add .gitattributes roundtrip.shift 2>&1 >/dev/null |\n> + grep \"Checking roundtrip encoding for SHIFT-JIS\" &&\n> + test_unconfig core.checkRoundtripEncoding &&\n\nThe \"unconfig\" won't take place if the test fails. Instead of\ntest_config/test_unconfig, you could use '-c' to set the config\ntransiently for the git-add operation:\n\n ! GIT_TRACE=1 git -c core.checkRoundtripEncoding=garbage add ...\n\n> + git reset &&\n> +\n> + # UTF-16 encoded files should not be round-trip checked by default...\n> + ! GIT_TRACE=1 git add roundtrip.utf16 2>&1 >/dev/null |\n> + grep \"Checking roundtrip encoding for UTF-16\" &&\n> + git reset &&\n> +\n> + # ... unless we tell Git to check it!\n> + test_config_global core.checkRoundtripEncoding \"UTF-16, UTF-32\" &&\n> + GIT_TRACE=1 git add roundtrip.utf16 2>&1 >/dev/null |\n> + grep \"Checking roundtrip encoding for UTF-16\" &&\n> + git reset &&\n> +\n> + # ... unless we tell Git to check it!\n> + # (here we also check that the casing of the encoding is irrelevant)\n> + test_config_global core.checkRoundtripEncoding \"UTF-32, utf-16\" &&\n> + GIT_TRACE=1 git add roundtrip.utf16 2>&1 >/dev/null |\n> + grep \"Checking roundtrip encoding for UTF-16\" &&\n> + git reset &&\n> +\n> + # cleanup\n> + rm roundtrip.shift roundtrip.utf16 &&\n> + git reset --hard HEAD\n> +'\n\nSame comment as for patch 5/7: This cleanup won't happen if the test\nfails. Instead, use test_when_finished() earlier in the test:\n\n test_expect_success 'check roundtrip encoding' '\n test_when_finished \"rm -f roundtrip.shift roundtrip.utf16; git\nreset --hard HEAD\" &&\n ...\n"},{"id":"340375","messageId":"CAPig+cSh6YVK_KbinEv+1fwU1Wo4BErfYBZ76KKdhu9dM1D5Dg@mail.gmail.com","threadId":"47921","inReplyTo":"392C5E11-A3CA-4D65-B9D4-9EE30258C83A@gmail.com","subject":"Re: [PATCH v8 3/7] utf8: add function to detect prohibited UTF-16/32 BOM","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2018-02-27T05:17:44Z","receivedAt":"2018-02-27T05:17:50Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sun, Feb 25, 2018 at 6:35 AM, Lars Schneider\n wrote:\n>> On 25 Feb 2018, at 04:41, Eric Sunshine wrote:\n>> Is this interpretation correct? When I read [1], I interpret it as\n>> saying that no BOM _of any sort_ should be present when the encoding\n>> is declared as one of UTF-16BE, UTF-16LE, UTF-32BE, or UTF-32LE.\n>\n> Correct!\n>\n>> This code, on the other hand, only checks for BOMs corresponding\n>> to the declared size (16 or 32 bits).\n>\n> Hmm. Interesting thought. You are saying that my code won't complain if\n> a document declared as UTF-16LE has a UTF32-LE BOM, correct?\n\nWell, not specifically that case since UTF-16LE BOM is a subset of UTF32-LE BOM.\n\nMy observation was more general in that [1] seems to say that there\nshould be _no_ BOM whatsoever if one of UTF-16BE, UTF-16LE, UTF-32BE,\nor UTF-32LE is declared.\n\n> I would say\n> this is correct behavior in context of this function. This function assumes\n> that the document is proper UTF-16/UTF-16BE/UTF-16LE but it is wrongly\n> declared with respect to its BOM in the .gitattributes. Would this\n> comment make it more clear to you?\n> /*\n> * If a data stream is declared as UTF-16BE or UTF-16LE, then a UTF-16\n> * BOM must not be used [1]. The same applies for the UTF-32 equivalents.\n> * The function returns true if this rule is violated.\n> *\n> * [1] http://unicode.org/faq/utf_bom.html#bom10\n> */\n> I think what you are referring to is a different class of error and\n> would therefore warrant its own checker function. Would you agree?\n\nI don't understand to what different class of error you refer. The\nFAQ[1] seems pretty clear to me in that if one of those declarations\nis used explicitly, then there should be _no_ BOM, period. It doesn't\nsay anything about allowing a BOM for a differently-sized encoding (16\nvs 32).\n\nIf I squint very hard, I _guess_ I can see how you interpret [1] with\nthe more narrow meaning of the restriction applying only to a BOM of\nthe same size as the declared encoding, though reading it that way\ndoesn't come easily to me.\n\n>> I suppose the intention of [1] is to detect a mismatch between the\n>> declared encoding and how the stream is actually encoded. The check\n>> implemented here will fail to detect a mismatch between, say, declared\n>> encoding UTF-16BE and actual encoding UTF-32BE.\n>\n> As stated above the intention is to detect wrong BOMs! I think we cannot\n> detect the \"declared as UTF-16BE but actually UTF-32BE\" error.\n>\n> Consider this:\n>\n> printf \"test\" | iconv -f UTF-8 -t UTF-32BE | iconv -f UTF-16BE -t UTF-8 | od -c\n> 0000000 \\0 t \\0 e \\0 s \\0 t\n> 0000010\n>\n> In the first step we \"encode\" the string to UTF-32BE and then we \"decode\" it as\n> UTF-16BE. The result is valid although not correct. Does this make sense?\n\nI'm probably being dense, but I don't understand what this is trying\nto illustrate in relation to has_prohibited_utf_bom().\n"},{"id":"340410","messageId":"3A3B1BD6-642C-467F-B80C-82853410C235@gmail.com","threadId":"47921","inReplyTo":"CAPig+cRtp5g0iDd3pHybZXd_VkGG2-a4CdCZdiqN55Cr1T5Zyg@mail.gmail.com","subject":"Re: [PATCH v8 5/7] convert: add 'working-tree-encoding' attribute","fromName":"Lars Schneider","fromEmail":"larsxschneider@gmail.com","sentAt":"2018-02-27T11:16:56Z","receivedAt":"2018-02-27T11:58:56Z","isPatch":true,"sender":{"key":"larsxschneider@gmail.com","avatar":"https://avatars.githubusercontent.com/u/477434?v=4"},"body":"\n> On 25 Feb 2018, at 08:15, Eric Sunshine wrote:\n> \n> On Sat, Feb 24, 2018 at 11:27 AM, wrote:\n>> Git recognizes files encoded with ASCII or one of its supersets (e.g.\n>> UTF-8 or ISO-8859-1) as text files. All other encodings are usually\n>> interpreted as binary and consequently built-in Git text processing\n>> tools (e.g. 'git diff') as well as most Git web front ends do not\n>> visualize the content.\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>> Signed-off-by: Lars Schneider \n>> ---\n>> diff --git a/Documentation/gitattributes.txt b/Documentation/gitattributes.txt\n>> @@ -272,6 +272,84 @@ few exceptions. Even though...\n>> +Please note that using the `working-tree-encoding` attribute may have a\n>> +number of pitfalls:\n>> +\n>> +- Alternative Git implementations (e.g. JGit or libgit2) and older Git\n>> + versions (as of March 2018) do not support the `working-tree-encoding`\n>> + attribute. If you decide to use the `working-tree-encoding` attribute\n>> + in your repository, then it is strongly recommended to ensure that all\n>> + clients working with the repository support it.\n>> +\n>> + If you declare `*.proj` files as UTF-16 and you add `foo.proj` with an\n>> + `working-tree-encoding` enabled Git client, then `foo.proj` will be\n>> + stored as UTF-8 internally. A client without `working-tree-encoding`\n>> + support will checkout `foo.proj` as UTF-8 encoded file. This will\n>> + typically cause trouble for the users of this file.\n> \n> The above paragraph is giving an example of the scenario described in\n> the paragraph above it. It might, therefore, be clearer to start this\n> paragraph with \"For example,...\". Also, as an aid to non-Windows\n> developers, it might be helpful to introduce \".proj\" files as\n> \"Microsoft Visual Studio `*.proj` files...\".\n\nAgreed. How about this?\n\n For example, Microsoft Visual Studio resources files (`*.rc`) or\n PowerShell script files (`*.ps1`) are sometimes encoded in UTF-16. \n If you declare `*.ps1` as files as UTF-16 and you add `foo.ps1` with \n a `working-tree-encoding` enabled Git client, then `foo.ps1` will be\n stored as UTF-8 internally. A client without `working-tree-encoding`\n support will checkout `foo.ps1` as UTF-8 encoded file. This will\n typically cause trouble for the users of this file.\n\n\n>> diff --git a/convert.c b/convert.c\n>> @@ -265,6 +266,110 @@ static int will_convert_lf_to_crlf(size_t len, struct text_stat *stats,\n>> +static struct encoding {\n>> + const char *name;\n>> + struct encoding *next;\n>> +} *encoding, **encoding_tail;\n> \n> Why does this struct need to be a linked-list since it contains only a\n> name? In fact, why do the struct and linked list exist at all? Back in\n> v1 when the struct held more members, it made sense to place them in a\n> collection so they could be re-used, but today it just seems an\n> unnecessary artifact which buys you nothing.\n> \n> Is the plan that some future patch series will add members to the\n> struct? If not, then it seems that it should be retired altogether.\n\nAbsolutely! Thank you!\n\n\n>> +static const char *default_encoding = \"UTF-8\";\n>> +\n>> +static int encode_to_git(const char *path, const char *src, size_t src_len,\n>> + struct strbuf *buf, struct encoding *enc, int conv_flags)\n>> +{\n>> + [...]\n>> + if (has_prohibited_utf_bom(enc->name, src, src_len)) {\n>> + const char *error_msg = _(\n>> + \"BOM is prohibited in '%s' if encoded as %s\");\n>> + /*\n>> + * This advise is shown for UTF-??BE and UTF-??LE encodings.\n> \n> s/advise/advice/\n\nAgreed!\n\n\n>> + * We truncate the encoding name to 6 chars with %.6s to cut\n>> + * off the last two \"byte order\" characters.\n>> + */\n>> + const char *advise_msg = _(\n>> + \"The file '%s' contains a byte order mark (BOM). \"\n>> + \"Please use %.6s as working-tree-encoding.\");\n>> + advise(advise_msg, path, enc->name);\n>> + if (conv_flags & CONV_WRITE_OBJECT)\n>> + die(error_msg, path, enc->name);\n>> + else\n>> + error(error_msg, path, enc->name);\n> \n> What is the intended behavior in the non-die() case? As implemented,\n> this is still going to attempt the conversion even though it threw an\n> error. Should it be returning 0 here instead? Same question for the\n> couple additional cases below.\n\nGood question. My intention was to print an error and then let iconv try\nthe conversion anyways. I agree that this is bogus. Let's return in case\nof an error right away.\n\n\n>> +\n>> + } else if (is_missing_required_utf_bom(enc->name, src, src_len)) {\n>> + const char *error_msg = _(\n>> + \"BOM is required in '%s' if encoded as %s\");\n>> + const char *advise_msg = _(\n>> + \"The file '%s' is missing a byte order mark (BOM). \"\n>> + \"Please use %sBE or %sLE (depending on the byte order) \"\n>> + \"as working-tree-encoding.\");\n>> + advise(advise_msg, path, enc->name, enc->name);\n>> + if (conv_flags & CONV_WRITE_OBJECT)\n>> + die(error_msg, path, enc->name);\n>> + else\n>> + error(error_msg, path, enc->name);\n>> + }\n> \n> For a function which presumably is agnostic about the working-tree\n> encoding (supporting whatever iconv does), it nevertheless has\n> unusually intimate knowledge about UTF BOMs, in general, and the\n> internals of has_prohibited_utf_bom() and\n> is_missing_required_utf_bom(), in particular. It might be cleaner, and\n> would simplify this function, if all this UTF BOM-specific gunk was\n> moved elsewhere and generalized into a \"validate conversion\" function.\n\nAgreed! How about this?\n\n\t/*\n\t * If we convert to an UTF encoding, then check for common BOM errors.\n\t */\n\tif (!memcmp(\"UTF-\", enc, 4))\n\t\tif (has_utf_bom_error(path, enc, src, src_len, die_on_error))\n\t\t\treturn 0;\n\n\n>> + dst = reencode_string_len(src, src_len, default_encoding, enc->name,\n>> + &dst_len);\n>> + if (!dst) {\n>> + /*\n>> + * We could add the blob \"as-is\" to Git. However, on checkout\n>> + * we would try to reencode to the original encoding. This\n>> + * would fail and we would leave the user with a messed-up\n>> + * working tree. Let's try to avoid this by screaming loud.\n>> + */\n>> + const char* msg = _(\"failed to encode '%s' from %s to %s\");\n>> + if (conv_flags & CONV_WRITE_OBJECT)\n>> + die(msg, path, enc->name, default_encoding);\n>> + else\n>> + error(msg, path, enc->name, default_encoding);\n>> + }\n>> +\n>> + strbuf_attach(buf, dst, dst_len, dst_len + 1);\n>> + return 1;\n>> +}\n>> +\n>> +static int encode_to_worktree(const char *path, const char *src, size_t src_len,\n>> + struct strbuf *buf, struct encoding *enc)\n>> +{\n>> + [...]\n>> + dst = reencode_string_len(src, src_len, enc->name, default_encoding,\n>> + &dst_len);\n>> + if (!dst) {\n>> + error(\"failed to encode '%s' from %s to %s\",\n>> + path, enc->name, default_encoding);\n> \n> The last two arguments need to be swapped.\n\nNice catch!\n\n\n>> + return 0;\n>> + }\n>> +\n>> + strbuf_attach(buf, dst, dst_len, dst_len + 1);\n>> + return 1;\n>> +}\n>> @@ -978,6 +1083,35 @@ static int ident_to_worktree(const char *path, const char *src, size_t len,\n>> +static struct encoding *git_path_check_encoding(struct attr_check_item *check)\n>> +{\n>> + if (ATTR_TRUE(value) || ATTR_FALSE(value) || ATTR_UNSET(value) ||\n>> + !strlen(value))\n>> + return NULL;\n>> +\n>> + for (enc = encoding; enc; enc = enc->next)\n>> + if (!strcasecmp(value, enc->name))\n>> + return enc;\n>> +\n>> + /* Don't encode to the default encoding */\n>> + if (!strcasecmp(value, default_encoding))\n>> + return NULL;\n> \n> You could handle this easy-to-detect case before attempting the more\n> expensive loop just above. (Not necessarily worth a re-roll.)\n\nTrue, but I delete the loop anyways when I removed the \"encoding \nlinked list\" as suggested by your comment above.\n\n\n>> + enc = xcalloc(1, sizeof(*enc));\n>> + /*\n>> + * Ensure encoding names are always upper case (e.g. UTF-8) to\n>> + * simplify subsequent string comparisons.\n>> + */\n>> + enc->name = xstrdup_toupper(value);\n>> + *encoding_tail = enc;\n>> + encoding_tail = &(enc->next);\n>> +\n>> + return enc;\n>> +}\n>> diff --git a/t/t0028-working-tree-encoding.sh b/t/t0028-working-tree-encoding.sh\n>> @@ -0,0 +1,226 @@\n>> +test_expect_success 'ensure UTF-8 is stored in Git' '\n>> + git cat-file -p :test.utf16 >test.utf16.git &&\n>> + test_cmp_bin test.utf8.raw test.utf16.git &&\n>> +\n>> + # cleanup\n>> + rm test.utf16.git\n>> +'\n> \n> This cleanup won't take place if test_cmp_bin() fails. Instead,\n> cleanups are typically handled by test_when_finished() to ensure that\n> the cleanup action occurs even if the test fails.\n> \n> test_expect_success 'ensure UTF-8 is stored in Git' '\n> test_when_finished \"rm -f test.utf16.git\" &&\n> ...\n> \n> Same comment for remaining tests.\n\nAgreed! That's the proper way to cleanup the tests. I'll fix my tests.\n\n\n>> +test_expect_success 'check prohibited UTF BOM' '\n>> + printf \"\\0a\\0b\\0c\" >nobom.utf16be.raw &&\n>> + printf \"a\\0b\\0c\\0\" >nobom.utf16le.raw &&\n>> + printf \"\\376\\777\\0a\\0b\\0c\" >bebom.utf16be.raw &&\n>> + printf \"\\777\\376a\\0b\\0c\\0\" >lebom.utf16le.raw &&\n>> +\n>> + printf \"\\0\\0\\0a\\0\\0\\0b\\0\\0\\0c\" >nobom.utf32be.raw &&\n>> + printf \"a\\0\\0\\0b\\0\\0\\0c\\0\\0\\0\" >nobom.utf32le.raw &&\n>> + printf \"\\0\\0\\376\\777\\0\\0\\0a\\0\\0\\0b\\0\\0\\0c\" >bebom.utf32be.raw &&\n>> + printf \"\\777\\376\\0\\0a\\0\\0\\0b\\0\\0\\0c\\0\\0\\0\" >lebom.utf32le.raw &&\n> \n> These resources seem to be used by multiple tests. Perhaps create them\n> in a distinct \"setup\" test instead of bundling them in this test?\n\nGood idea!\n\n\n>> + echo \"*.utf16be text working-tree-encoding=utf-16be\" >>.gitattributes &&\n>> + echo \"*.utf16le text working-tree-encoding=utf-16le\" >>.gitattributes &&\n>> + echo \"*.utf32be text working-tree-encoding=utf-32be\" >>.gitattributes &&\n>> + echo \"*.utf32le text working-tree-encoding=utf-32le\" >>.gitattributes &&\n>> +\n>> + # Here we add a UTF-16 files with BOM (big-endian and little-endian)\n>> + # but we tell Git to treat it as UTF-16BE/UTF-16LE. In these cases\n>> + # the BOM is prohibited.\n>> + cp bebom.utf16be.raw bebom.utf16be &&\n>> + test_must_fail git add bebom.utf16be 2>err.out &&\n>> + test_i18ngrep \"fatal: BOM is prohibited .* UTF-16BE\" err.out &&\n>> +\n>> + cp lebom.utf16le.raw lebom.utf16be &&\n>> + test_must_fail git add lebom.utf16be 2>err.out &&\n>> + test_i18ngrep \"fatal: BOM is prohibited .* UTF-16BE\" err.out &&\n>> +\n>> + cp bebom.utf16be.raw bebom.utf16le &&\n>> + test_must_fail git add bebom.utf16le 2>err.out &&\n>> + test_i18ngrep \"fatal: BOM is prohibited .* UTF-16LE\" err.out &&\n>> +\n>> + cp lebom.utf16le.raw lebom.utf16le &&\n>> + test_must_fail git add lebom.utf16le 2>err.out &&\n>> + test_i18ngrep \"fatal: BOM is prohibited .* UTF-16LE\" err.out &&\n>> +\n>> + # ... and the same for UTF-32\n>> + cp bebom.utf32be.raw bebom.utf32be &&\n>> + test_must_fail git add bebom.utf32be 2>err.out &&\n>> + test_i18ngrep \"fatal: BOM is prohibited .* UTF-32BE\" err.out &&\n>> +\n>> + cp lebom.utf32le.raw lebom.utf32be &&\n>> + test_must_fail git add lebom.utf32be 2>err.out &&\n>> + test_i18ngrep \"fatal: BOM is prohibited .* UTF-32BE\" err.out &&\n>> +\n>> + cp bebom.utf32be.raw bebom.utf32le &&\n>> + test_must_fail git add bebom.utf32le 2>err.out &&\n>> + test_i18ngrep \"fatal: BOM is prohibited .* UTF-32LE\" err.out &&\n>> +\n>> + cp lebom.utf32le.raw lebom.utf32le &&\n>> + test_must_fail git add lebom.utf32le 2>err.out &&\n>> + test_i18ngrep \"fatal: BOM is prohibited .* UTF-32LE\" err.out &&\n>> +\n>> + # cleanup\n>> + git reset --hard HEAD\n>> +'\n> \n> Clumping all these checks into the same test makes it difficult to\n> determine which one failed (if one does fail). Also, the amount of\n> copy/paste code is easy to get wrong and a bit onerous to review.\n> Automating via for-loops would address these concerns. For instance,\n> to check all combinations (not just 8 combinations tested above):\n> \n> for i in 16be 16le 32be 32le\n> do\n> for j in 16be 16le 32be 32le\n> do\n> test_expect_success \"check prohibited UTF BOM utf$i/utf$j\" '\n> test_when_finished \"git reset --hard HEAD\" &&\n> cp bebom.utf$i.raw bebom.utf$j &&\n> test_must_fail git add bebom.utf$j 2>err.out &&\n> J=$(echo $j | tr bel BEL) &&\n> test_i18ngrep \"fatal: BOM is prohibited .* UTF-$J\" err.out\n> '\n> done\n> done\n> \n> Alternately, the test could be moved to a function and called for each\n> combination:\n> \n> check_prohibited_bom () {\n> i=$1\n> j=$2\n> test_expect_success \"check prohibited UTF BOM utf$i/utf$j\" '\n> ...\n> '\n> }\n> check_prohibited_bom 16be 16be\n> check_prohibited_bom 16be 16le\n> ...\n> \n> Same comment regarding copy/paste in tests below.\n\nIn general I share your concern about code duplication.\nHowever, I don't want be \"too clever\" in testing code\nas it becomes hard to read it. Therefore, I implemented\na loop over \"16 32\" which is a good compromise I think.\n\n \nThanks you very much for this thorough review,\nLars"},{"id":"340619","messageId":"CAPig+cQ1CNS0GbUWfeMBc+Q+fHjN3+t7c0RMQ9fWLrcZHBBJxw@mail.gmail.com","threadId":"47921","inReplyTo":"3A3B1BD6-642C-467F-B80C-82853410C235@gmail.com","subject":"Re: [PATCH v8 5/7] convert: add 'working-tree-encoding' attribute","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2018-02-28T21:20:49Z","receivedAt":"2018-02-28T21:20:56Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, Feb 27, 2018 at 6:16 AM, Lars Schneider\n wrote:\n>> On 25 Feb 2018, at 08:15, Eric Sunshine wrote:\n>> On Sat, Feb 24, 2018 at 11:27 AM, wrote:\n>> The above paragraph is giving an example of the scenario described in\n>> the paragraph above it. It might, therefore, be clearer to start this\n>> paragraph with \"For example,...\". Also, as an aid to non-Windows\n>> developers, it might be helpful to introduce \".proj\" files as\n>> \"Microsoft Visual Studio `*.proj` files...\".\n>\n> Agreed. How about this?\n>\n> For example, Microsoft Visual Studio resources files (`*.rc`) or\n> PowerShell script files (`*.ps1`) are sometimes encoded in UTF-16.\n> If you declare `*.ps1` as files as UTF-16 and you add `foo.ps1` with\n> a `working-tree-encoding` enabled Git client, then `foo.ps1` will be\n> stored as UTF-8 internally. A client without `working-tree-encoding`\n> support will checkout `foo.ps1` as UTF-8 encoded file. This will\n> typically cause trouble for the users of this file.\n\nSince \"*.proj\" is mentioned a number of times in paragraphs below this\none, perhaps just stick with it instead of introducing \"*.rc\" and\n\"*.ps1\", which don't necessarily add value. Alternately, if you use\n.rc and .ps1, then change the remaining .proj references to follow\nsuit.\n\n>>> diff --git a/convert.c b/convert.c\n>>> + } else if (is_missing_required_utf_bom(enc->name, src, src_len)) {\n>>> + const char *error_msg = _(\n>>> + \"BOM is required in '%s' if encoded as %s\");\n>>> + const char *advise_msg = _(\n>>> + \"The file '%s' is missing a byte order mark (BOM). \"\n>>> + \"Please use %sBE or %sLE (depending on the byte order) \"\n>>> + \"as working-tree-encoding.\");\n>>> + advise(advise_msg, path, enc->name, enc->name);\n>>> + if (conv_flags & CONV_WRITE_OBJECT)\n>>> + die(error_msg, path, enc->name);\n>>> + else\n>>> + error(error_msg, path, enc->name);\n>>> + }\n>>\n>> For a function which presumably is agnostic about the working-tree\n>> encoding (supporting whatever iconv does), it nevertheless has\n>> unusually intimate knowledge about UTF BOMs, in general, and the\n>> internals of has_prohibited_utf_bom() and\n>> is_missing_required_utf_bom(), in particular. It might be cleaner, and\n>> would simplify this function, if all this UTF BOM-specific gunk was\n>> moved elsewhere and generalized into a \"validate conversion\" function.\n>\n> Agreed! How about this?\n>\n> /*\n> * If we convert to an UTF encoding, then check for common BOM errors.\n> */\n> if (!memcmp(\"UTF-\", enc, 4))\n> if (has_utf_bom_error(path, enc, src, src_len, die_on_error))\n> return 0;\n\nBetter. The comment merely repeats the code, which is clear in its\nintent, thus doesn't really add value (but that's a minor point).\n\nI'd probably generalize it further to a \"validate conversion\" function\n(which itself would have this UTF-specific knowledge), but that's a\nmatter of taste and can be done later when/if other types of\nvalidation requirements arise.\n"},{"id":"340623","messageId":"DF6F3855-EFE7-4C13-AA53-819AAE0DEF7A@gmail.com","threadId":"47921","inReplyTo":"CAPig+cSh6YVK_KbinEv+1fwU1Wo4BErfYBZ76KKdhu9dM1D5Dg@mail.gmail.com","subject":"Re: [PATCH v8 3/7] utf8: add function to detect prohibited UTF-16/32 BOM","fromName":"Lars Schneider","fromEmail":"larsxschneider@gmail.com","sentAt":"2018-02-28T21:34:17Z","receivedAt":"2018-02-28T21:34:28Z","isPatch":true,"sender":{"key":"larsxschneider@gmail.com","avatar":"https://avatars.githubusercontent.com/u/477434?v=4"},"body":"\n> On 27 Feb 2018, at 06:17, Eric Sunshine wrote:\n> \n> On Sun, Feb 25, 2018 at 6:35 AM, Lars Schneider\n> wrote:\n>>> On 25 Feb 2018, at 04:41, Eric Sunshine wrote:\n>>> Is this interpretation correct? When I read [1], I interpret it as\n>>> saying that no BOM _of any sort_ should be present when the encoding\n>>> is declared as one of UTF-16BE, UTF-16LE, UTF-32BE, or UTF-32LE.\n>> \n>> Correct!\n>> \n>>> This code, on the other hand, only checks for BOMs corresponding\n>>> to the declared size (16 or 32 bits).\n>> \n>> Hmm. Interesting thought. You are saying that my code won't complain if\n>> a document declared as UTF-16LE has a UTF32-LE BOM, correct?\n> \n> Well, not specifically that case since UTF-16LE BOM is a subset of UTF32-LE BOM.\n\nCorrect - bad example on my part!\n\n\n> My observation was more general in that [1] seems to say that there\n> should be _no_ BOM whatsoever if one of UTF-16BE, UTF-16LE, UTF-32BE,\n> or UTF-32LE is declared.\n\nYou are saying that a document declared as UTF-16LE must not start \nwith 0000feff (UTF-32BE BOM)? I interpreted that situation as a \"feff\"\nin the middle of a file and therefore the BOM should be treated as\nZWNBSP as explained here: http://unicode.org/faq/utf_bom.html#bom6\n\nPlus, if \"_no_ BOM whatsoever\" is allowed then wouldn't we need to check\nfor UTF-1, UTF-7, and UTF-8 BOM's too?\n\nI dunno.\n\n\n>> I would say\n>> this is correct behavior in context of this function. This function assumes\n>> that the document is proper UTF-16/UTF-16BE/UTF-16LE but it is wrongly\n>> declared with respect to its BOM in the .gitattributes. Would this\n>> comment make it more clear to you?\n>> /*\n>> * If a data stream is declared as UTF-16BE or UTF-16LE, then a UTF-16\n>> * BOM must not be used [1]. The same applies for the UTF-32 equivalents.\n>> * The function returns true if this rule is violated.\n>> *\n>> * [1] http://unicode.org/faq/utf_bom.html#bom10\n>> */\n>> I think what you are referring to is a different class of error and\n>> would therefore warrant its own checker function. Would you agree?\n> \n> I don't understand to what different class of error you refer. The\n> FAQ[1] seems pretty clear to me in that if one of those declarations\n> is used explicitly, then there should be _no_ BOM, period. It doesn't\n> say anything about allowing a BOM for a differently-sized encoding (16\n> vs 32).\n> \n> If I squint very hard, I _guess_ I can see how you interpret [1] with\n> the more narrow meaning of the restriction applying only to a BOM of\n> the same size as the declared encoding, though reading it that way\n> doesn't come easily to me.\n\nFor me it is somewhat the other way around :-)\nSince I am not sure what is right, I decided to ask the Internet:\nhttps://stackoverflow.com/questions/49038872/is-a-utf-32be-bom-valid-in-an-utf-16le-declared-data-stream\n\nLet's see if someone has a good answer.\n\n- Lars\n\n"},{"id":"340982","messageId":"5955B68D-B6E3-43C0-8C70-47E37F1204E1@gmail.com","threadId":"47921","inReplyTo":"CAPig+cR81J3fTOtrgAumAs=RC5hqYFfSmeb-ru-Yf_ahFuBiew@mail.gmail.com","subject":"Re: [PATCH v8 7/7] convert: add round trip check based on 'core.checkRoundtripEncoding'","fromName":"Lars Schneider","fromEmail":"larsxschneider@gmail.com","sentAt":"2018-03-04T19:08:11Z","receivedAt":"2018-03-04T19:08:23Z","isPatch":true,"sender":{"key":"larsxschneider@gmail.com","avatar":"https://avatars.githubusercontent.com/u/477434?v=4"},"body":"\n> On 25 Feb 2018, at 20:50, Eric Sunshine wrote:\n> \n> On Sat, Feb 24, 2018 at 11:28 AM, wrote:\n>> UTF supports lossless conversion round tripping and conversions between\n>> UTF and other encodings are mostly round trip safe as Unicode aims to be\n>> a superset of all other character encodings. However, certain encodings\n>> (e.g. SHIFT-JIS) are known to have round trip issues [1].\n>> \n>> Add 'core.checkRoundtripEncoding', which contains a comma separated\n>> list of encodings, to define for what encodings Git should check the\n>> conversion round trip if they are used in the 'working-tree-encoding'\n>> attribute.\n>> \n>> Set SHIFT-JIS as default value for 'core.checkRoundtripEncoding'.\n>> \n>> [1] https://support.microsoft.com/en-us/help/170559/prb-conversion-problem-between-shift-jis-and-unicode\n>> \n>> Signed-off-by: Lars Schneider \n>> ---\n>> diff --git a/convert.c b/convert.c\n>> @@ -289,6 +289,39 @@ static void trace_encoding(const char *context, const char *path,\n>> +static int check_roundtrip(const char* enc_name)\n>> +{\n>> + /*\n>> + * check_roundtrip_encoding contains a string of space and/or\n>> + * comma separated encodings (eg. \"UTF-16, ASCII, CP1125\").\n>> + * Search for the given encoding in that string.\n>> + */\n>> + const char *found = strcasestr(check_roundtrip_encoding, enc_name);\n>> + const char *next = found + strlen(enc_name);\n> \n> Is this pointer arithmetic undefined behavior (according to the C\n> standard) if NULL is returned by strcasestr()? If so, how comfortable\n> are we with this? Perhaps if you add an 'if' into the mix, you can\n> avoid it:\n> \n> if (found) {\n> const char *next = found + strlen(enc_name);\n> return ...long complicated expression...;\n> }\n> return false;\n\nOK. I've fixed it this way:\n\n\tif (!found)\n\t\treturn 0;\n\n\n[...]\n>> +\n>> + if (!re_src || src_len != re_src_len ||\n>> + memcmp(src, re_src, src_len)) {\n>> + const char* msg = _(\"encoding '%s' from %s to %s and \"\n>> + \"back is not the same\");\n>> + die(msg, path, enc->name, default_encoding);\n> \n> Last two arguments need to be swapped.\n\nHm. Are you sure? I think it is correct as it is. We are in encode_to_git()\nhere and that means we encode *to* \"default encoding\", no?\n\n\n>> + }\n>> +\n>> + free(re_src);\n>> + }\n>> +\n>> strbuf_attach(buf, dst, dst_len, dst_len + 1);\n>> return 1;\n>> }\n>> diff --git a/t/t0028-working-tree-encoding.sh b/t/t0028-working-tree-encoding.sh\n>> @@ -225,4 +225,45 @@ test_expect_success 'error if encoding garbage is already in Git' '\n>> +test_expect_success 'check roundtrip encoding' '\n>> + text=\"hallo there!\\nroundtrip test here!\" &&\n>> + printf \"$text\" | iconv -f UTF-8 -t SHIFT-JIS >roundtrip.shift &&\n>> + printf \"$text\" | iconv -f UTF-8 -t UTF-16 >roundtrip.utf16 &&\n>> + echo \"*.shift text working-tree-encoding=SHIFT-JIS\" >>.gitattributes &&\n>> +\n>> + # SHIFT-JIS encoded files are round-trip checked by default...\n>> + GIT_TRACE=1 git add .gitattributes roundtrip.shift 2>&1 >/dev/null |\n>> + grep \"Checking roundtrip encoding for SHIFT-JIS\" &&\n> \n> Why redirect to /dev/null? If something does go wrong somewhere, the\n> more output available for debugging the problem, the better, so\n> throwing it away unnecessarily seems contraindicated.\n\nOK!\n\n\n>> + git reset &&\n>> +\n>> + # ... unless we overwrite the Git config!\n>> + test_config core.checkRoundtripEncoding \"garbage\" &&\n>> + ! GIT_TRACE=1 git add .gitattributes roundtrip.shift 2>&1 >/dev/null |\n>> + grep \"Checking roundtrip encoding for SHIFT-JIS\" &&\n>> + test_unconfig core.checkRoundtripEncoding &&\n> \n> The \"unconfig\" won't take place if the test fails. Instead of\n> test_config/test_unconfig, you could use '-c' to set the config\n> transiently for the git-add operation:\n> \n> ! GIT_TRACE=1 git -c core.checkRoundtripEncoding=garbage add ...\n\nAgreed. Although test_config (in t/test-lib-functions.sh) automatically \nunsets itself after the test is over. \n\n\n>> + git reset &&\n>> +\n>> + # UTF-16 encoded files should not be round-trip checked by default...\n>> + ! GIT_TRACE=1 git add roundtrip.utf16 2>&1 >/dev/null |\n>> + grep \"Checking roundtrip encoding for UTF-16\" &&\n>> + git reset &&\n>> +\n>> + # ... unless we tell Git to check it!\n>> + test_config_global core.checkRoundtripEncoding \"UTF-16, UTF-32\" &&\n>> + GIT_TRACE=1 git add roundtrip.utf16 2>&1 >/dev/null |\n>> + grep \"Checking roundtrip encoding for UTF-16\" &&\n>> + git reset &&\n>> +\n>> + # ... unless we tell Git to check it!\n>> + # (here we also check that the casing of the encoding is irrelevant)\n>> + test_config_global core.checkRoundtripEncoding \"UTF-32, utf-16\" &&\n>> + GIT_TRACE=1 git add roundtrip.utf16 2>&1 >/dev/null |\n>> + grep \"Checking roundtrip encoding for UTF-16\" &&\n>> + git reset &&\n>> +\n>> + # cleanup\n>> + rm roundtrip.shift roundtrip.utf16 &&\n>> + git reset --hard HEAD\n>> +'\n> \n> Same comment as for patch 5/7: This cleanup won't happen if the test\n> fails. Instead, use test_when_finished() earlier in the test:\n> \n> test_expect_success 'check roundtrip encoding' '\n> test_when_finished \"rm -f roundtrip.shift roundtrip.utf16; git\n> reset --hard HEAD\" &&\n> ...\n\nAgreed!\n\nThanks you,\nLars\n\n"},{"id":"340983","messageId":"CAPig+cR1zEn0ktDtQnUt2272gZ04S-Ssg7Vzja6QZejTLN_fbQ@mail.gmail.com","threadId":"47921","inReplyTo":"5955B68D-B6E3-43C0-8C70-47E37F1204E1@gmail.com","subject":"Re: [PATCH v8 7/7] convert: add round trip check based on 'core.checkRoundtripEncoding'","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2018-03-04T19:58:26Z","receivedAt":"2018-03-04T19:58:33Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sun, Mar 4, 2018 at 2:08 PM, Lars Schneider wrote:\n>> On 25 Feb 2018, at 20:50, Eric Sunshine wrote:\n>> On Sat, Feb 24, 2018 at 11:28 AM, wrote:\n>>> + if (!re_src || src_len != re_src_len ||\n>>> + memcmp(src, re_src, src_len)) {\n>>> + const char* msg = _(\"encoding '%s' from %s to %s and \"\n>>> + \"back is not the same\");\n>>> + die(msg, path, enc->name, default_encoding);\n>>\n>> Last two arguments need to be swapped.\n>\n> Hm. Are you sure? I think it is correct as it is. We are in encode_to_git()\n> here and that means we encode *to* \"default encoding\", no?\n\nOkay. I guess I was just looking at the most recent\nreencode_string_len() -- and maybe overlooked the \"and back\" -- and\nwas thinking that this error message applied directly to it, but I see\nyour point about the error saying something about encode_to_git()\noverall, in which case I agree with you.\n\n>>> + test_config core.checkRoundtripEncoding \"garbage\" &&\n>>> + ! GIT_TRACE=1 git add .gitattributes roundtrip.shift 2>&1 >/dev/null |\n>>> + grep \"Checking roundtrip encoding for SHIFT-JIS\" &&\n>>> + test_unconfig core.checkRoundtripEncoding &&\n>>\n>> The \"unconfig\" won't take place if the test fails. Instead of\n>> test_config/test_unconfig, you could use '-c' to set the config\n>> transiently for the git-add operation:\n>>\n>> ! GIT_TRACE=1 git -c core.checkRoundtripEncoding=garbage add ...\n>\n> Agreed. Although test_config (in t/test-lib-functions.sh) automatically\n> unsets itself after the test is over.\n\nYep, so you could get by with that alone. The test_unconfig() simply\nisn't needed.\n"}]}