Re: [PATCH v1 1/6] t0027: Add tests for get_stream_filter()

[Junio C Hamano <gitster@pobox.com>]

tboegi@web.de writes:

> From: Torsten Bögershausen <tboegi@web.de>
>
> When a filter is configured, a different code-path is used in convert.c
> and entry.c via get_stream_filter(), but there are no test cases yet.
>
> Add tests for the filter API by configuring the ident filter.
> The result of the SHA1 conversion is not checked, this is already
> done in other TC.
>
> Add a parameter to checkout_files() in t0027.
> While changing the signature, add another parameter for the eol= attribute.
> This is currently unused, tests for e.g.
> "* text=auto eol=lf" will be added in a separate commit.
>
> Signed-off-by: Torsten Bögershausen <tboegi@web.de>
> ---
>  t/t0027-auto-crlf.sh | 262 ++++++++++++++++++++++++++++-----------------------
>  1 file changed, 146 insertions(+), 116 deletions(-)
>
> diff --git a/t/t0027-auto-crlf.sh b/t/t0027-auto-crlf.sh
> index 504e5a0..681f0c5 100755
> --- a/t/t0027-auto-crlf.sh
> +++ b/t/t0027-auto-crlf.sh
> @@ -21,32 +21,45 @@ compare_ws_file () {
>  	pfx=$1
>  	exp=$2.expect
>  	act=$pfx.actual.$3
> -	tr '\015\000' QN <"$2" >"$exp" &&
> -	tr '\015\000' QN <"$3" >"$act" &&
> +	tr '\015\000abcdef01234567890' QN00000000000000000 <"$2" >"$exp" &&
> +	tr '\015\000abcdef01234567890' QN00000000000000000 <"$3" >"$act" &&

'0' is listed twice on purpose?

>  	test_cmp $exp $act &&
>  	rm $exp $act

As we are not forcing all the future callers of this helper
functions to feed only non problematic pathnames, we should quote
these variable references, but this is not a new issue this patch
introduces.

>  }
>  
>  create_gitattributes () {
>  	attr=$1
> +	ident=$2
> +	case "$2" in
> +		"")
> +			>.gitattributes
> +			;;
> +		i)
> +			echo "* ident" >.gitattributes
> +			;;
> +		*)
> +			echo >&2 invalid ident: $2
> +			exit 1

This is overindented.  Case arms align with case/esac, and their
bodies indent one level down, i.e.

	case "$2" in
        "")
        	>.gitattributes
                ;;
	esac

By the way, I somehow find it hard to follow to assign magic
meanings "clear" and "prepare ident for everybody" to a short
and cryptic "" (empty string) and "i".  If you were taking advantage
of the fact that all existing uses of this helper function pass only
one argument to it and you will get $ident="" automatically from the
ones that you did not touch, that might be a reasonable way to reduce
patch noise, but because you are doing things like this

> -	create_gitattributes "$attr" &&
> +	create_gitattributes "$attr" "" &&

anyway, it may make sense to be slightly more verbose and readable.

> +	esac
> +
>  	case "$attr" in
>  		auto)
> -		echo "*.txt text=auto" >.gitattributes
> +		echo "*.txt text=auto" >>.gitattributes
>  		;;
>  		...
>  		;;
>  		*)
>  		echo >&2 invalid attribute: $attr

As you are touching this helper to emit more than one thing into the
file, I'd consider doing 

	{
                case "$2" in
                clear)	;;
                ident)	echo '* ident' ;;
                ...
                esac &&
                case "$1" in
                auto)	echo '*.txt auto' ;;
                ...
                esac
	} >.gitattributes

or even

	{
                while test "$#" != 1
                do
                        printf "%s\n" "$1"
                        shift
                done &&
                case "$1" in
                auto)	echo '*.txt auto' ;;
                ...
                esac &&
	} >.gitattributes

anticipating that we may want more things added to the output in the
future.  The latter form allows your callers to say things like

	create_gitattributes "auto" "* ident" "*.bin binary" 

without having to touch the helper function.