Re: [PATCH v7] ls-files: Add eol diagnostics
- From
Eric Sunshine <sunshine@sunshineco.com>
- Date
- Dec 7, 2015, 20:21 UTC
- Message-ID
- <CAPig+cQayKit8JzYtYJrLE1EFiNAOqpnC+GDm7MpiSO8zHJySw@mail.gmail.com>
- In-Reply-To
- <5665BD39.1040403@web.de>
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...
Show 15 quoted lines
> 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.
Show 15 quoted lines
> + 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'
Show 18 quoted lines
> + 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.
Show 15 quoted lines
> + 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
Show 17 quoted lines
> + 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.
Show 9 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
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.
Show 5 quoted lines
> +" > + > test_done > -- > 2.6.2.403.gd7a84e3