Re: [PATCH] test-lib: allow short options to be stacked

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 21, 2020, 04:53 UTC
Message-ID: <xmqqsgi29um6.fsf@gitster.c.googlers.com>
In-Reply-To: <48c28683412e3e0803d4c7189a6d66daddcdc580.1584759277.git.matheus.bernardino@usp.br>

Matheus Tavares <matheus.bernardino@usp.br> writes:

> +parse_option () {
> +	local opt="$@"

I do not think there is any context in which var="$@" makes sense in shell script (var="$*" is understandable, though).

Did you mean opt=$1 here?

> +# Parse options while taking care to leave $@ intact, so we will still
> +# have all the original command line options when executing the test
> +# script again for '--tee' and '--verbose-log' below.

The phrase "below" no longer makes much sense after moving lines around, does it?

Show 17 quoted lines

> +store_arg_to=
> +prev_opt=
> +for opt
> +do
> +	if test -n "$store_arg_to"
> +	then
> +		eval $store_arg_to=\$opt
> +		store_arg_to=
> +		prev_opt=
> +		continue
> +	fi
> +
> +	case "$opt" in
> +	--*)
> +		parse_option "$opt" ;;
> +	-?*)
> +		# stacked short options must be fed separately to parse_option

Are you calling concatenated short options, e.g. "-abc", as "stacked"? It sounds like a very unusual phrasing, at least to me.

Show 11 quoted lines

> +		for c in $(echo "${opt#-}" | sed 's/./& /g')
> +		do
> +			parse_option "-$c"
> +		done
> +		;;
> +	*)
> +		echo "error: unknown test option '$opt'" >&2; exit 1 ;;
> +	esac
>  
>  	prev_opt=$opt
>  done

I am personally not very enthused (the line counts vs benefit does not feel so great), but as long as it works correctly and maintainable, I won't complain too much.