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

[Eric Sunshine <sunshine@sunshineco.com>]

On Monday, December 7, 2015, Torsten Bögershausen <tboegi@web.de> wrote:
> 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.[...]

A few style comments this time around...

> Helped-By: Eric Sunshine <sunshine@sunshineco.com>
> Signed-off-by: Torsten Bögershausen <tboegi@web.de>
> ---
> diff --git a/builtin/ls-files.c b/builtin/ls-files.c
> @@ -47,6 +48,21 @@ static const char *tag_modified = "";
> +static void write_eolinfo(const struct cache_entry *ce, const char *path)
> +{
> +       struct stat st;
> +       const char *i_txt = "";
> +       const char *w_txt = "";
> +       if (!show_eol)
> +               return;
> +       if (ce && S_ISREG(ce->ce_mode))
> +               i_txt = get_cached_convert_stats_ascii(ce->name);
> +       if (!lstat(path, &st) && (S_ISREG(st.st_mode)))

Style: unnecessary parentheses around S_ISREG(), which is inconsistent
with S_ISREG() two lines above.

> +               w_txt = get_wt_convert_stats_ascii(path);
> +       printf("i/%-13s w/%-13s attr/%-9s ", i_txt, w_txt,
> +                                get_convert_attr_ascii(path));
> +}
> diff --git a/convert.c b/convert.c
> @@ -95,6 +100,62 @@ static int is_binary(unsigned long size, struct text_stat *stats)
> +static unsigned int gather_convert_stats(const char *data, unsigned long size)
> +{
> +       struct text_stat stats;
> +       if (!data || !size)
> +               return 0;
> +       gather_stats(data, size, &stats);
> +       if (is_binary(size, &stats) || stats.cr != stats.crlf)
> +               return CONVERT_STAT_BITS_BIN;
> +       else if (stats.crlf && (stats.crlf == stats.lf))

Style: unnecessary parentheses around 'foo == bar'

> +               return CONVERT_STAT_BITS_TXT_CRLF;
> +       else if (stats.crlf && stats.lf)
> +               return CONVERT_STAT_BITS_TXT_CRLF | CONVERT_STAT_BITS_TXT_LF;
> +       else if (stats.lf)
> +               return CONVERT_STAT_BITS_TXT_LF;
> +       else
> +               return 0;
> +}
> +
> +static const char *gather_convert_stats_ascii(const char *data, unsigned long size)
> +{
> +       unsigned int convert_stats = gather_convert_stats(data, size);
> +
> +       if (convert_stats & CONVERT_STAT_BITS_BIN)
> +               return "binary";
> +       switch (convert_stats) {
> +               case CONVERT_STAT_BITS_TXT_LF:
> +                       return("text-lf");

Style: space after "return".

Also, can we lose the unnecessary noisy parentheses? (I recall
mentioning this in my first review.)

Same for "return" statements below.

> +               case CONVERT_STAT_BITS_TXT_CRLF:
> +                       return("text-crlf");
> +               case CONVERT_STAT_BITS_TXT_LF | CONVERT_STAT_BITS_TXT_CRLF:
> +                       return("text-crlf-lf");
> +               default:
> +                       return ("text-no-eol");
> +       }
> +}
> diff --git a/t/t0027-auto-crlf.sh b/t/t0027-auto-crlf.sh
> @@ -214,6 +239,20 @@ checkout_files () {
>                 fi
>         done
>
> +       test_expect_success "ls-files --eol $lfname ${pfx}LF.txt" "
> +         test_when_finished 'rm e expect actual' &&

Style: test_when_finished is incorrectly indented with tab+spaces
rather than only tabs

> +               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
> +       "
> @@ -480,4 +550,20 @@ checkout_files    native  true  "lf"      LF    CRLF  CRLF_mix_LF  LF_mix_CR
>  checkout_files    native  false "crlf"    CRLF  CRLF  CRLF         CRLF_mix_CR  CRLF_nul
>  checkout_files    native  true  "crlf"    CRLF  CRLF  CRLF         CRLF_mix_CR  CRLF_nul
>
> +

Style: unnecessary blank line

> +# Should be the last test case: remove some files from the worktree
> +# run 'git ls-files -d'

This seems kind of fragile. Might it be possible to either recreate
those files when this test finishes or instead provide a function
which creates them on-demand for tests which need them? My concern is
that there is a good chance that someone later adding tests will
overlook this comment, especially since most people just plop new
tests at the bottom of the file.

> +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 &&

Style: too many spaces following 'rm'

Same issue raised in my previous review: If one or more of these files
did not get created,  for instance, because some earlier test failed,
then this 'rm' will fail, thus causing this entire test to fail
unnecessarily. Therefore, 'rm -f' would make more sense.

> +       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

Also, from the previous review: test_when_finished() would be a more
reliable way to clean up these files if they really ought to be
cleaned up.

> +"
> +
>  test_done
> --
> 2.6.2.403.gd7a84e3