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

[Patrick Steinhardt <ps@pks.im>]

[-- Attachment #1: Type: text/plain, Size: 3237 bytes --]

On Fri, Apr 05, 2024 at 05:08:59PM -0700, Junio C Hamano wrote:
> 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

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]