From: Junio C Hamano <gitster@pobox.com>
Date: Thu, 16 Jun 2022 21:18:14 GMT
Subject: Re: [PATCH v2 3/5] tests: only automatically unset matching values from test_config
Message-ID: <xmqq35g4s3xl.fsf@gitster.g>
In-Reply-To: <20220616205456.19081-4-jacob.e.keller@intel.com>

Jacob Keller <jacob.keller@gmail.com> writes:

> +	# Only enable --fixed-value if we have two parameters
> +	if test $# < 2
> +	then
> +		fixedvalue=
> +	fi

Two comments:

 * Does "<" do what you expect to do?  Did you mean "-lt"?

 * Using "bug in the test script: $*" and diagnosing missing
   parameters, instead of silently ignoring the option the developer
   wrote, would be more preferrable.

> +	git ${config_dir:+-C "$config_dir"} config ${global:+--global} ${fixedvalue:+--fixed-value} --unset-all "$1" "$2"
>  	config_status=$?
>  	case "$config_status" in
>  	5) # ok, nothing to unset
> @@ -575,7 +586,7 @@ test_config () {
>  		esac
>  		shift
>  	done
> -	test_when_finished "test_unconfig ${config_dir:+-C '$config_dir'} ${global:+--global} '$1'" &&
> +	test_when_finished "test_unconfig ${config_dir:+-C '$config_dir'} --fixed-value ${global:+--global} '$1' '$2'" &&

Why are $1 and $2 enclosed in a pair of single quotes?  Is the
assumption that they do not contain a single quote themselves?

I guess that is true also for config_dir and shares the same
problem, so you are not introducing a new problem.

>  	git ${config_dir:+-C "$config_dir"} config ${global:+--global} "$1" "$2"
>  }