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

From: Junio C Hamano <gitster@pobox.com>
Date: Feb 2, 2016, 21:18 UTC
Message-ID: <xmqqk2mmretb.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <1454431988-1941-1-git-send-email-tboegi@web.de>

tboegi@web.de writes:

Show 31 quoted lines

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

Show 15 quoted lines

>  }
>  
>  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.

Show 11 quoted lines

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