Re: [PATCH v6] ls-files: Add eol diagnostics

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Dec 7, 2015, 05:28 UTC
Message-ID: <CAPig+cR3PLvMbKTUUdrD3hVrr5RWEmd0CxE6iyYhddqBYUQ57Q@mail.gmail.com>
In-Reply-To: <56649CD7.9000902@web.de>

On Sun, Dec 6, 2015 at 3:38 PM, Torsten Bögershausen <tboegi@web.de> wrote:

Show 14 quoted lines

> When working in a cross-platform environment, a user wants to
> check if text files are stored normalized in the repository and if
> .gitattributes are set appropriately.
> [...]
> Signed-off-by: Torsten Bögershausen <tboegi@web.de>
> ---
> diff --git a/convert.c b/convert.c
> @@ -95,6 +100,63 @@ static int is_binary(unsigned long size, struct text_stat *stats)
> +const char *get_wt_convert_stats_ascii(const char *path)
> +{
> +       const char *ret;
> +       struct strbuf sb = STRBUF_INIT;
> +       if (strbuf_read_file(&sb, path, 0) < 0)
> +               return "";

Given the implementation of strbuf_read_file(), some memory may have been allocated to the strbuf even if it returns a failure, therefore this may be leaking.

> +       ret = gather_convert_stats_ascii(sb.buf, sb.len);
> +       strbuf_release(&sb);
> +       return ret;
> +}

You may, therefore, want to restructure the code like this to avoid the leak:

    const char *ret = "";
    struct strbuf sb = STRBUF_INIT;
    if (strbuf_read_file(...) >= 0)
        ret = gather_convert_stats_ascii(...);
    strbuf_release(&sb);
    return ret;

Show 6 quoted lines

> diff --git a/t/t0027-auto-crlf.sh b/t/t0027-auto-crlf.sh
> @@ -213,7 +238,20 @@ checkout_files () {
>                         git -c core.eol=$eol checkout $src$f.txt
>                 fi
>         done
> -

Was dropping this blank line intentional?

Show 13 quoted lines

> +       test_expect_success "ls-files --eol $lfname ${pfx}LF.txt" "
> +               cat >e <<-EOF &&
> +               i/text-crlf w/$(stats_ascii $crlfname) ${src}CRLF.txt
> +               i/text-crlf-lf w/$(stats_ascii $lfmixcrlf) ${src}CRLF_mix_LF.txt
> +               i/text-lf w/$(stats_ascii $lfname) ${src}LF.txt
> +               i/binary w/$(stats_ascii $lfmixcr) ${src}LF_mix_CR.txt
> +               i/binary w/$(stats_ascii $crlfnul) ${src}CRLF_nul.txt
> +               i/binary w/$(stats_ascii $crlfnul) ${src}LF_nul.txt
> +               EOF
> +               sort <e >expect &&
> +               git ls-files --eol $src* | sed -e 's!attr/[=a-z-]*!!g' -e 's/  */ /g' | sort >actual &&
> +               test_cmp expect actual &&
> +               rm e expect actual

If it is indeed important to cleanup these files so that they don't muck up following tests, then you'd be better off using:

    test_when_finished "rm -f e expect actual"

at the top of the test to ensure that the cleanup happens regardless of the whether the test succeeds or fails. On the other hand, if these files won't muck up subsequent tests, then it may be fine to skip cleanup altogether.

Show 9 quoted lines

> +       "
>         test_expect_success "checkout core.eol=$eol core.autocrlf=$crlf gitattributes=$attr file=LF" "
>                 compare_ws_file $pfx $lfname    ${src}LF.txt
>         "
> @@ -231,6 +269,37 @@ checkout_files () {
> +# Test control characters
> +# NUL SOH CR EOF==^Z
> +test_expect_success 'ls-files --eol -o Text/Binary' '
> +       test_when_finished "rm e expect actual TeBi_*" &&

It's probably not a big deal, but "rm -f" perhaps?

Show 24 quoted lines

> +       STRT=AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA &&
> +       STR=$STRT$STRT$STRT$STRT &&
> +       printf "${STR}BBB\001" >TeBi_127_S &&
> +       printf "${STR}BBBB\001">TeBi_128_S &&
> +       printf "${STR}BBB\032" >TeBi_127_E &&
> +       printf "\032${STR}BBB" >TeBi_E_127 &&
> +       printf "${STR}BBBB\000">TeBi_128_N &&
> +       printf "${STR}BBB\012">TeBi_128_L &&
> +       printf "${STR}BBB\015">TeBi_127_C &&
> +       printf "${STR}BB\015\012" >TeBi_126_CL &&
> +       printf "${STR}BB\015\012\015" >TeBi_126_CLC &&
> +       cat >e <<-\EOF &&
> +       i/ w/binary TeBi_127_S
> +       i/ w/text-no-eol TeBi_128_S
> +       i/ w/text-no-eol TeBi_127_E
> +       i/ w/binary TeBi_E_127
> +       i/ w/binary TeBi_128_N
> +       i/ w/text-lf TeBi_128_L
> +       i/ w/binary TeBi_127_C
> +       i/ w/text-crlf TeBi_126_CL
> +       i/ w/binary TeBi_126_CLC
> +       EOF
> +       sort <e >expect &&
> +       git ls-files --eol -o | egrep "TeBi_" | sed -e 's!attr/[=a-z-]*!!g' -e "s/  */ /g" | sort >actual &&

Okay. "egrep" is used about as frequently in test scripts as "grep -E".

Show 6 quoted lines

> +       test_cmp expect actual
> +'
> @@ -480,4 +549,20 @@ checkout_files    native  true  "lf"      LF    CRLF  CRLF_mix_LF  LF_mix_CR
> +# Should be the last test case
> +test_expect_success 'ls-files --eol -d' "
> +       rm  crlf_false_attr__CRLF.txt crlf_false_attr__CRLF_mix_LF.txt crlf_false_attr__LF.txt .gitattributes &&

Did you mean to wrap this in test_when_finished()?

If not, is it cleaning up gunk left by some earlier test(s)? If so, what happens if one of those tests failed and didn't create one of these files? In that case, this 'rm' would fail, causing this entire test to fail. Better would be to use "rm -f".

Show 12 quoted lines

> +       cat >expect <<-\EOF &&
> +       i/text-crlf w/ crlf_false_attr__CRLF.txt
> +       i/text-crlf-lf w/ crlf_false_attr__CRLF_mix_LF.txt
> +       i/text-lf w/ .gitattributes
> +       i/text-lf w/ crlf_false_attr__LF.txt
> +       EOF
> +       git ls-files --eol -d | sed -e 's!attr/[=a-z-]*!!g' -e 's/  */ /g' | sort >actual &&
> +       test_cmp expect actual &&
> +       rm expect actual
> +"
> +
> +

Style: Drop the extra blank line.

>  test_done
> --
> 2.6.2.403.gd7a84e3