Re: [PATCH] utf8: handle systems that don't write BOM for UTF-16

From: "Torsten Bögershausen" <tboegi@web.de>
To: "brian m. carlson" <sandals@crustytoothpaste.net>
Cc: git@vger.kernel.org, larsxschneider@gmail.com,
	Rich Felker <dalias@libc.org>, Junio C Hamano <gitster@pobox.com>,
	Kevin Daudt <me@ikke.info>
Subject: Re: [PATCH] utf8: handle systems that don't write BOM for UTF-16
Date: Sun, 10 Feb 2019 08:04:13 +0000	[thread overview]
Message-ID: <20190210080413.u56vr3fgoejjzjfm@tb-raspi4> (raw)
In-Reply-To: <20190209200802.277139-1-sandals@crustytoothpaste.net>

On Sat, Feb 09, 2019 at 08:08:01PM +0000, brian m. carlson wrote:
> When serializing UTF-16 (and UTF-32), there are three possible ways to
> write the stream. One can write the data with a BOM in either big-endian
> or little-endian format, or one can write the data without a BOM in
> big-endian format.
>
> Most systems' iconv implementations choose to write it with a BOM in
> some endianness, since this is the most foolproof, and it is resistant
> to misinterpretation on Windows, where UTF-16 and the little-endian
> serialization are very common. For compatibility with Windows and to
> avoid accidental misuse there, Git always wants to write UTF-16 with a
> BOM, and will refuse to read UTF-16 without it.
>
> However, musl's iconv implementation writes UTF-16 without a BOM,
> relying on the user to interpret it as big-endian. This causes t0028 and
> the related functionality to fail, since Git won't read the file without
> a BOM.
>
> Add a Makefile and #define knob, ICONV_NEEDS_BOM, that can be set if the
> iconv implementation has this behavior. When set, Git will write a BOM
> manually for UTF-16 and UTF-32 and then force the data to be written in
> UTF-16BE or UTF-32BE. We choose big-endian behavior here because the
> tests use the raw "UTF-16" encoding, which will be big-endian when the
> implementation requires this knob to be set.
>
> Update the tests to detect this case and write test data with an added
> BOM if necessary. Always write the BOM in the tests in big-endian
> format, since all iconv implementations that omit a BOM must use
> big-endian serialization according to the Unicode standard.
>
> Preserve the existing behavior for systems which do not have this knob
> enabled, since they may use optimized implementations, including
> defaulting to the native endianness, to gain improved performance, which
> can be significant with large checkouts.

Is the based on measurements on a real system ?

I think we agree that Git will write UTF-16 always as big endian with BOM,
following the tradition of iconv/libiconv.
If yes, we can reduce the lines of code/#idefs somewhat, have the knob always on,
and reduce the maintenance burden a little bit, giving a simpler patch.

What do you think ?

diff --git a/t/t0028-working-tree-encoding.sh b/t/t0028-working-tree-encoding.sh
index e58ecbfc44..ef19c98e67 100755
--- a/t/t0028-working-tree-encoding.sh
+++ b/t/t0028-working-tree-encoding.sh
@@ -13,7 +13,8 @@ test_expect_success 'setup test files' '
 	echo "*.utf16 text working-tree-encoding=utf-16" >.gitattributes &&
 	echo "*.utf16lebom text working-tree-encoding=UTF-16LE-BOM" >>.gitattributes &&
 	printf "$text" >test.utf8.raw &&
-	printf "$text" | iconv -f UTF-8 -t UTF-16 >test.utf16.raw &&
+	printf "\376\377"                         >test.utf16.raw &&
+	printf "$text" | iconv -f UTF-8 -t UTF-16BE >>test.utf16.raw &&
 	printf "$text" | iconv -f UTF-8 -t UTF-32 >test.utf32.raw &&
 	printf "\377\376"                         >test.utf16lebom.raw &&
 	printf "$text" | iconv -f UTF-8 -t UTF-32LE >>test.utf16lebom.raw &&
diff --git a/utf8.c b/utf8.c
index 83824dc2f4..d3731273be 100644
--- a/utf8.c
+++ b/utf8.c
@@ -564,7 +564,8 @@ char *reencode_string_len(const char *in, size_t insz,
 		bom_str = utf16_le_bom;
 		bom_len = sizeof(utf16_le_bom);
 		out_encoding = "UTF-16LE";
-	} else if (same_utf_encoding("UTF-16BE-BOM", out_encoding)) {
+	} else if (same_utf_encoding("UTF-16BE-BOM", out_encoding) ||
+		   same_utf_encoding("UTF-16", out_encoding)) {
 		bom_str = utf16_be_bom;
 		bom_len = sizeof(utf16_be_bom);
 		out_encoding = "UTF-16BE";


>
> Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
> ---
>  Makefile                         |  6 ++++++
>  t/t0028-working-tree-encoding.sh | 25 ++++++++++++++++++++++---
>  utf8.c                           | 10 ++++++++++
>  3 files changed, 38 insertions(+), 3 deletions(-)
>
> diff --git a/Makefile b/Makefile
> index 571160a2c4..b2a4765e5f 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -259,6 +259,9 @@ all::
>  # Define OLD_ICONV if your library has an old iconv(), where the second
>  # (input buffer pointer) parameter is declared with type (const char **).
>  #
> +# Define ICONV_NEEDS_BOM if your iconv implementation does not write a
> +# byte-order mark (BOM) when writing UTF-16 or UTF-32.
> +#
>  # Define NO_DEFLATE_BOUND if your zlib does not have deflateBound.
>  #
>  # Define NO_R_TO_GCC_LINKER if your gcc does not like "-R/path/lib"
> @@ -1415,6 +1418,9 @@ ifndef NO_ICONV
>  		EXTLIBS += $(ICONV_LINK) -liconv
>  	endif
>  endif
> +ifdef ICONV_NEEDS_BOM
> +	BASIC_CFLAGS += -DICONV_NEEDS_BOM
> +endif
>  ifdef NEEDS_LIBGEN
>  	EXTLIBS += -lgen
>  endif
> diff --git a/t/t0028-working-tree-encoding.sh b/t/t0028-working-tree-encoding.sh
> index e58ecbfc44..bfc4a9d4dd 100755
> --- a/t/t0028-working-tree-encoding.sh
> +++ b/t/t0028-working-tree-encoding.sh
> @@ -6,6 +6,25 @@ test_description='working-tree-encoding conversion via gitattributes'
>
>  GIT_TRACE_WORKING_TREE_ENCODING=1 && export GIT_TRACE_WORKING_TREE_ENCODING
>
> +test_lazy_prereq NO_UTF16_BOM '
> +	test $(printf abc | iconv -f UTF-8 -t UTF-16 | wc -c) = 6
> +'
> +
> +test_lazy_prereq NO_UTF32_BOM '
> +	test $(printf abc | iconv -f UTF-8 -t UTF-32 | wc -c) = 12
> +'
> +
> +write_utf16 () {
> +	test_have_prereq NO_UTF16_BOM && printf '\xfe\xff'
> +	iconv -f UTF-8 -t UTF-16
> +
> +}
> +
> +write_utf32 () {
> +	test_have_prereq NO_UTF32_BOM && printf '\x00\x00\xfe\xff'
> +	iconv -f UTF-8 -t UTF-32
> +}
> +
>  test_expect_success 'setup test files' '
>  	git config core.eol lf &&
>
> @@ -13,8 +32,8 @@ test_expect_success 'setup test files' '
>  	echo "*.utf16 text working-tree-encoding=utf-16" >.gitattributes &&
>  	echo "*.utf16lebom text working-tree-encoding=UTF-16LE-BOM" >>.gitattributes &&
>  	printf "$text" >test.utf8.raw &&
> -	printf "$text" | iconv -f UTF-8 -t UTF-16 >test.utf16.raw &&
> -	printf "$text" | iconv -f UTF-8 -t UTF-32 >test.utf32.raw &&
> +	printf "$text" | write_utf16 >test.utf16.raw &&
> +	printf "$text" | write_utf32 >test.utf32.raw &&
>  	printf "\377\376"                         >test.utf16lebom.raw &&
>  	printf "$text" | iconv -f UTF-8 -t UTF-32LE >>test.utf16lebom.raw &&
>
> @@ -223,7 +242,7 @@ test_expect_success ICONV_SHIFT_JIS 'check roundtrip encoding' '
>
>  	text="hallo there!\nroundtrip test here!" &&
>  	printf "$text" | iconv -f UTF-8 -t SHIFT-JIS >roundtrip.shift &&
> -	printf "$text" | iconv -f UTF-8 -t UTF-16 >roundtrip.utf16 &&
> +	printf "$text" | write_utf16 >roundtrip.utf16 &&
>  	echo "*.shift text working-tree-encoding=SHIFT-JIS" >>.gitattributes &&
>
>  	# SHIFT-JIS encoded files are round-trip checked by default...
> diff --git a/utf8.c b/utf8.c
> index 83824dc2f4..133199de0e 100644
> --- a/utf8.c
> +++ b/utf8.c
> @@ -568,6 +568,16 @@ char *reencode_string_len(const char *in, size_t insz,
>  		bom_str = utf16_be_bom;
>  		bom_len = sizeof(utf16_be_bom);
>  		out_encoding = "UTF-16BE";
> +#ifdef ICONV_NEEDS_BOM
> +	} else if (same_utf_encoding("UTF-16", out_encoding)) {
> +		bom_str = utf16_be_bom;
> +		bom_len = sizeof(utf16_be_bom);
> +		out_encoding = "UTF-16BE";
> +	} else if (same_utf_encoding("UTF-32", out_encoding)) {
> +		bom_str = utf32_be_bom;
> +		bom_len = sizeof(utf32_be_bom);
> +		out_encoding = "UTF-32BE";
> +#endif
>  	}
>
>  	conv = iconv_open(out_encoding, in_encoding);
