Re: [PATCH v8 7/7] convert: add round trip check based on 'core.checkRoundtripEncoding'

[Eric Sunshine <sunshine@sunshineco.com>]

On Sat, Feb 24, 2018 at 11:28 AM,  <lars.schneider@autodesk.com> 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 <larsxschneider@gmail.com>
> ---
> 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" &&
        ...