On Sat, Feb 24, 2018 at 11:28 AM, <[email protected]> wrote:
> UTF supports lossless conversion round tripping and conversions between
> UTF and other encodings are mostly round trip safe as Unicode aims to be
> a superset of all other character encodings. However, certain encodings
> (e.g. SHIFT-JIS) are known to have round trip issues [1].
>
> Add 'core.checkRoundtripEncoding', which contains a comma separated
> list of encodings, to define for what encodings Git should check the
> conversion round trip if they are used in the 'working-tree-encoding'
> attribute.
>
> Set SHIFT-JIS as default value for 'core.checkRoundtripEncoding'.
>
> [1]
> https://support.microsoft.com/en-us/help/170559/prb-conversion-problem-between-shift-jis-and-unicode
>
> Signed-off-by: Lars Schneider <[email protected]>
> ---
> diff --git a/convert.c b/convert.c
> @@ -289,6 +289,39 @@ static void trace_encoding(const char *context, const
> char *path,
> +static int check_roundtrip(const char* enc_name)
> +{
> + /*
> + * check_roundtrip_encoding contains a string of space and/or
> + * comma separated encodings (eg. "UTF-16, ASCII, CP1125").
> + * Search for the given encoding in that string.
> + */
> + const char *found = strcasestr(check_roundtrip_encoding, enc_name);
> + const char *next = found + strlen(enc_name);
Is this pointer arithmetic undefined behavior (according to the C
standard) if NULL is returned by strcasestr()? If so, how comfortable
are we with this? Perhaps if you add an 'if' into the mix, you can
avoid it:
if (found) {
const char *next = found + strlen(enc_name);
return ...long complicated expression...;
}
return false;
> + int len = strlen(check_roundtrip_encoding);
> + return (found && (
> + /*
> + * check that the found encoding is at the
> + * beginning of check_roundtrip_encoding or
> + * that it is prefixed with a space or comma
> + */
> + found == check_roundtrip_encoding || (
> + found > check_roundtrip_encoding &&
> + (*(found-1) == ' ' || *(found-1) == ',')
Although the 'found > check_roundtrip_encoding' expression is
effectively dead code in this case, it does document that the
programmer didn't just blindly use '*(found-1)' without taking
boundary conditions into consideration.
(It's dead code because, at this point, we know that strcasestr()
didn't return NULL and we know that 'found' doesn't equal
'check_roundtrip_encoding', therefore it _must_ be greater than
'check_roundtrip_encoding'.)
Just an observation; not a request to remove the dead code since it
has some documentation value.
> + )
> + ) && (
> + /*
> + * check that the found encoding is at the
> + * end of check_roundtrip_encoding or
> + * that it is suffixed with a space or comma
> + */
> + next == check_roundtrip_encoding + len || (
> + next < check_roundtrip_encoding + len &&
> + (*next == ' ' || *next == ',')
> + )
> + ));
> +}
> @@ -366,6 +399,47 @@ static int encode_to_git(const char *path, const char
> *src, size_t src_len,
> + if ((conv_flags & CONV_WRITE_OBJECT) && check_roundtrip(enc->name)) {
> + re_src = reencode_string_len(dst, dst_len,
> + enc->name, default_encoding,
> + &re_src_len);
> +
> + trace_printf("Checking roundtrip encoding for %s...\n",
> enc->name);
> + trace_encoding("reencoded source", path, enc->name,
> + re_src, re_src_len);
> +
> + if (!re_src || src_len != re_src_len ||
> + memcmp(src, re_src, src_len)) {
> + const char* msg = _("encoding '%s' from %s to %s and "
> + "back is not the same");
> + die(msg, path, enc->name, default_encoding);
Last two arguments need to be swapped.
> + }
> +
> + free(re_src);
> + }
> +
> strbuf_attach(buf, dst, dst_len, dst_len + 1);
> return 1;
> }
> diff --git a/t/t0028-working-tree-encoding.sh
> b/t/t0028-working-tree-encoding.sh
> @@ -225,4 +225,45 @@ test_expect_success 'error if encoding garbage is
> already in Git' '
> +test_expect_success '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 &&
> + echo "*.shift text working-tree-encoding=SHIFT-JIS" >>.gitattributes
> &&
> +
> + # SHIFT-JIS encoded files are round-trip checked by default...
> + GIT_TRACE=1 git add .gitattributes roundtrip.shift 2>&1 >/dev/null |
> + grep "Checking roundtrip encoding for SHIFT-JIS" &&
Why redirect to /dev/null? If something does go wrong somewhere, the
more output available for debugging the problem, the better, so
throwing it away unnecessarily seems contraindicated.
> + git reset &&
> +
> + # ... unless we overwrite the Git config!
> + test_config core.checkRoundtripEncoding "garbage" &&
> + ! GIT_TRACE=1 git add .gitattributes roundtrip.shift 2>&1 >/dev/null |
> + grep "Checking roundtrip encoding for SHIFT-JIS" &&
> + test_unconfig core.checkRoundtripEncoding &&
The "unconfig" won't take place if the test fails. Instead of
test_config/test_unconfig, you could use '-c' to set the config
transiently for the git-add operation:
! GIT_TRACE=1 git -c core.checkRoundtripEncoding=garbage add ...
> + git reset &&
> +
> + # UTF-16 encoded files should not be round-trip checked by default...
> + ! GIT_TRACE=1 git add roundtrip.utf16 2>&1 >/dev/null |
> + grep "Checking roundtrip encoding for UTF-16" &&
> + git reset &&
> +
> + # ... unless we tell Git to check it!
> + test_config_global core.checkRoundtripEncoding "UTF-16, UTF-32" &&
> + GIT_TRACE=1 git add roundtrip.utf16 2>&1 >/dev/null |
> + grep "Checking roundtrip encoding for UTF-16" &&
> + git reset &&
> +
> + # ... unless we tell Git to check it!
> + # (here we also check that the casing of the encoding is irrelevant)
> + test_config_global core.checkRoundtripEncoding "UTF-32, utf-16" &&
> + GIT_TRACE=1 git add roundtrip.utf16 2>&1 >/dev/null |
> + grep "Checking roundtrip encoding for UTF-16" &&
> + git reset &&
> +
> + # cleanup
> + rm roundtrip.shift roundtrip.utf16 &&
> + git reset --hard HEAD
> +'
Same comment as for patch 5/7: This cleanup won't happen if the test
fails. Instead, use test_when_finished() earlier in the test:
test_expect_success 'check roundtrip encoding' '
test_when_finished "rm -f roundtrip.shift roundtrip.utf16; git
reset --hard HEAD" &&
...
