Re: [PATCH 3/6] t: local VAR="VAL" (quote positional parameters)

From: Patrick Steinhardt <ps@pks.im>
Date: Apr 8, 2024, 15:30 UTC
Message-ID: <ZhQNq4ITp68ikVVy@tanuki>
In-Reply-To: <20240406000902.3082301-4-gitster@pobox.com>

On Fri, Apr 05, 2024 at 05:08:59PM -0700, Junio C Hamano wrote:

Show 26 quoted lines

> Future-proof test scripts that do
> 
> 	local VAR=VAL
> 
> without quoting VAL (which is OK in POSIX but broken in some shells)
> that is a positional parameter, e.g. $4.
> 
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
>  t/lib-parallel-checkout.sh | 2 +-
>  t/t2400-worktree-add.sh    | 2 +-
>  t/t4210-log-i18n.sh        | 4 ++--
>  t/test-lib-functions.sh    | 2 +-
>  4 files changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/t/lib-parallel-checkout.sh b/t/lib-parallel-checkout.sh
> index acaee9cbb6..8324d6c96d 100644
> --- a/t/lib-parallel-checkout.sh
> +++ b/t/lib-parallel-checkout.sh
> @@ -20,7 +20,7 @@ test_checkout_workers () {
>  		BUG "too few arguments to test_checkout_workers"
>  	fi &&
>  
> -	local expected_workers=$1 &&
> +	local expected_workers="$1" &&
>  	shift &&

I was wondering a bit why this is a problem in t0610, but not over here. As far as I understand it these statements are fine in practice because the expanded values cannot be split, right? So if "$1" expanded to something with spaces in between things would start to break.

In any case, changing all of these to be quoted feels like the right thing to do regardless of whether or not it happens to work with the current values of "$1". Otherwise it's simply a confusing failure waiting to happen.

Patrick

Show 53 quoted lines

>  	local trace_file=trace-test-checkout-workers &&
> diff --git a/t/t2400-worktree-add.sh b/t/t2400-worktree-add.sh
> index 051363acbb..5851e07290 100755
> --- a/t/t2400-worktree-add.sh
> +++ b/t/t2400-worktree-add.sh
> @@ -404,7 +404,7 @@ test_expect_success '"add" worktree with orphan branch, lock, and reason' '
>  # Note: Quoted arguments containing spaces are not supported.
>  test_wt_add_orphan_hint () {
>  	local context="$1" &&
> -	local use_branch=$2 &&
> +	local use_branch="$2" &&
>  	shift 2 &&
>  	local opts="$*" &&
>  	test_expect_success "'worktree add' show orphan hint in bad/orphan HEAD w/ $context" '
> diff --git a/t/t4210-log-i18n.sh b/t/t4210-log-i18n.sh
> index d2dfcf164e..75216f19ce 100755
> --- a/t/t4210-log-i18n.sh
> +++ b/t/t4210-log-i18n.sh
> @@ -64,7 +64,7 @@ test_expect_success 'log --grep does not find non-reencoded values (latin1)' '
>  '
>  
>  triggers_undefined_behaviour () {
> -	local engine=$1
> +	local engine="$1"
>  
>  	case $engine in
>  	fixed)
> @@ -85,7 +85,7 @@ triggers_undefined_behaviour () {
>  }
>  
>  mismatched_git_log () {
> -	local pattern=$1
> +	local pattern="$1"
>  
>  	LC_ALL=$is_IS_locale git log --encoding=ISO-8859-1 --format=%s \
>  		--grep=$pattern
> diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
> index 2f8868caa1..3204afbafb 100644
> --- a/t/test-lib-functions.sh
> +++ b/t/test-lib-functions.sh
> @@ -1689,7 +1689,7 @@ test_parse_ls_tree_oids () {
>  # Choose a port number based on the test script's number and store it in
>  # the given variable name, unless that variable already contains a number.
>  test_set_port () {
> -	local var=$1 port
> +	local var="$1" port
>  
>  	if test $# -ne 1 || test -z "$var"
>  	then
> -- 
> 2.44.0-501-g19981daefd
> 
>