From: Junio C Hamano <gitster@pobox.com>
Date: Mon, 11 Jan 2016 17:48:45 GMT
Subject: Re: [PATCH v10] ls-files: add eol diagnostics
Message-ID: <xmqq8u3wxb0i.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <1452446203-20693-1-git-send-email-tboegi@web.de>

tboegi@web.de writes:

> Changes agains pu:
> convert.c: 
>  - Indent switch case
>  - Introduced convert_is_binary() 
>
> ls-files.c:
>  - Early out in write_eolinfo()
>  - Use TAB instead of multiple spaces when -z is used
>
> t0027:
>  - case-esac indentattion
>  - cat EOF | sort, remove "e"
>  - Replace " with '
> Documentation/git-ls-files.txt:
>  -  Try to make things clearer
>  - -z turns SPACES inot TAB

Thanks for a summary.

>  Documentation/git-ls-files.txt |  30 +++++++++++
>  builtin/ls-files.c             |  24 +++++++++
>  convert.c                      | 119 +++++++++++++++++++++++++++++++----------
>  convert.h                      |   3 ++
>  t/t0027-auto-crlf.sh           | 112 +++++++++++++++++++++++++++++++++-----
>  5 files changed, 248 insertions(+), 40 deletions(-)
>
> diff --git a/Documentation/git-ls-files.txt b/Documentation/git-ls-files.txt
> index e26f01f..03f4770 100644
> --- a/Documentation/git-ls-files.txt
> +++ b/Documentation/git-ls-files.txt
> @@ -12,6 +12,7 @@ SYNOPSIS
>  'git ls-files' [-z] [-t] [-v]
>  		(--[cached|deleted|others|ignored|stage|unmerged|killed|modified])*
>  		(-[c|d|o|i|s|u|k|m])*
> +		[--eol]
>  		[-x <pattern>|--exclude=<pattern>]
>  		[-X <file>|--exclude-from=<file>]
>  		[--exclude-per-directory=<file>]
> @@ -147,6 +148,23 @@ a space) at the start of each line:
>  	possible for manual inspection; the exact format may change at
>  	any time.
>  
> +--eol::
> +	Show <eolinfo> and <eolattr> of files.
> +	<eolinfo> is the file content identification used by Git when
> +	the "text" attribute is "auto" (or not set and core.autocrlf is not false).
> +	<eolinfo> is either "binary", "none", "lf", "crlf" or "mixed" or "".
> ++
> +"" means the file is not a regular file, it is not in the index or
> +not accessable in the working tree.
> ++
> +<eolattr> is the attribute that is used when checking out or committing,
> +it is either "", "-text", "text", "text=auto", "eol=lf", "eol=crlf".
> ++
> +Both the <eolinfo> in the index ("i/<eolinfo>")
> +and in the working tree ("w/<eolinfo>") are shown for regular files,
> +followed by the  ("attr/<eolattr>").
> +Whenever a file is not a regular file, both <eolinfo> and <eolattr> are "".

I think the last line is a subset of what you already said at the
beginning ("" means the file is not...).

> +		if (line_terminator == '\n')
> +			printf("i/%-6s w/%-6s attr/%-9s ", i_txt, w_txt, a_txt);

Can we do something better than these hard-coded constants?  Why
can't the "one HT between each" approach be used for both?

> +		else
> +			printf("i/%s\tw/%s\tattr/%s\t", i_txt, w_txt, a_txt);
> +	}

> +stats_ascii () {
> +	case "$1" in
> +	LF)
> +	echo lf
> +	;;

Indent the "do this thing" part one level down, i.e.

	case A in
        X)
        	do a thing
                ;;
	...
	esac

> @@ -214,6 +239,19 @@ checkout_files () {
>  		fi
>  	done
>  
> +	test_expect_success "ls-files --eol $lfname ${pfx}LF.txt" "
> +		test_when_finished 'rm expect actual' &&
> +		cat <<-EOF | sort >expect &&

Do you have to cat into a pipe?

		sort <<-EOF >expect &&

In general, "don't cat a single thing into a pipe".

> +		i/crlf w/$(stats_ascii $crlfname) ${src}CRLF.txt
> +		i/mixed w/$(stats_ascii $lfmixcrlf) ${src}CRLF_mix_LF.txt
> +		i/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
> +		git ls-files --eol $src* | sed -e 's!attr/[=a-z-]*!!g' -e 's/  */ /g' | sort >actual &&

Break the pipeline, like this:

		git ls-files --eol $src* |
		sed -e 's!attr/[=a-z-]*!!g' -e 's/  */ /g' |
		sort >actual &&

> +		test_cmp expect actual
> +	"

Also, does the test body need to be quoted with double quotes,
allowing the substitutions happen _before_ the test runs?  We try to
enclose the last parameter to test_expect_success in single quotes
when possible, as subsitututing the $variables while computing the
arguments to the test_expect_success function historically was a
source of an unpleasant-to-debug bug (e.g. when they made the
resulting eval'ed string become syntactically incorrect).