Re: [PATCH v6 0/4] worktree: Support `--orphan` when creating new worktrees

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Dec 28, 2022, 08:01 UTC
Message-ID: <221228.868risuf5x.gmgdl@evledraar.gmail.com>
In-Reply-To: <20221228061539.13740-1-jacobabel@nullpo.dev>

On Wed, Dec 28 2022, Jacob Abel wrote:

Show 51 quoted lines

>   * Added save_param_arr() to preserve "$@" from being reset by
>     test_expect_success() in test_wt_add_excl() (2/4).
> [...]
> 2:  3d8b26f9d6 ! 2:  c03c112f79 worktree add: refactor opt exclusion tests
>     @@ t/t2400-worktree-add.sh: test_expect_success '"add" no auto-vivify with --detach
>      -test_expect_success '"add" -b/-B mutually exclusive' '
>      -	test_must_fail git worktree add -b poodle -B poodle bamboo main
>      -'
>     --
>     ++# Saves parameter sequence/array as a string so they can be safely stored in a
>     ++# variable and restored with `eval "set -- $arr"`. Sourced from
>     ++# https://stackoverflow.com/a/27503158/15064705
>     ++save_param_arr () {
>     ++	local i
>     ++	for i;
>     ++	do
>     ++		# For each argument:
>     ++		# 1. Append "\n" after each entry
>     ++		# 2. Convert "'" into "'\''"
>     ++		# 3. Prepend "'" before each entry
>     ++		# 4. Append " \" after each entry
>     ++		printf "%s\\n" "$i" | sed "
>     ++			s/'/'\\\\''/g
>     ++			1s/^/'/
>     ++			\$s/\$/' \\\\/
>     ++		"
>     ++	done
>     ++	echo " "
>     ++}
>     +
> [...]
>       ## Documentation/config/advice.txt ##
>     @@ t/t2400-worktree-add.sh: test_expect_success '"add" worktree with orphan branch,
>       	test_cmp expect .git/worktrees/orphan-with-lock-reason/locked
>       '
>
>     -+test_wt_add_empty_repo_orphan_hint() {
>     ++test_wt_add_empty_repo_orphan_hint () {
>      +	local context="$1"
>      +	shift
>     -+	local opts="$@"
>     ++	local arr=$(save_param_arr "$@")
>      +	test_expect_success "'worktree add' show orphan hint in empty repo w/ $context" '
>     ++		eval "set -- $arr" &&
>      +		test_when_finished "rm -rf empty_repo" &&
>      +		GIT_DIR="empty_repo" git init --bare &&
>     -+		test_must_fail git -C empty_repo worktree add $opts foobar/ 2> actual &&
>     ++		test_must_fail git -C empty_repo worktree add "$@" foobar/ 2> actual &&
>      +		grep "hint: If you meant to create a worktree containing a new orphan branch" actual
>      +	'
>      +}

The rest of this looks good to me, but this bit looks like you went down the rabbit hole of responding to Junio's feedback in https://lore.kernel.org/git/xmqqsfhawwqf.fsf@gitster.g/

I think as we're not dealing with any quoted arguments here it's not
worth copy/pasting some code to do shell quoting from StackOverflow,
i.e. for this series this squashed at the tip passes all the tests:
	
	diff --git a/t/t2400-worktree-add.sh b/t/t2400-worktree-add.sh
	index f43de591173..e5b01583cf2 100755
	--- a/t/t2400-worktree-add.sh
	+++ b/t/t2400-worktree-add.sh
	@@ -298,33 +298,11 @@ test_expect_success '"add" no auto-vivify with --detach and <branch> omitted' '
	 	test_must_fail git -C mish/mash symbolic-ref HEAD
	 '
	 
	-# Saves parameter sequence/array as a string so they can be safely stored in a
	-# variable and restored with `eval "set -- $arr"`. Sourced from
	-# https://stackoverflow.com/a/27503158/15064705
	-save_param_arr () {
	-	local i
	-	for i;
	-	do
	-		# For each argument:
	-		# 1. Append "\n" after each entry
	-		# 2. Convert "'" into "'\''"
	-		# 3. Prepend "'" before each entry
	-		# 4. Append " \" after each entry
	-		printf "%s\\n" "$i" | sed "
	-			s/'/'\\\\''/g
	-			1s/^/'/
	-			\$s/\$/' \\\\/
	-		"
	-	done
	-	echo " "
	-}
	-
	 # Helper function to test mutually exclusive options.
	 test_wt_add_excl () {
	-	local arr=$(save_param_arr "$@")
	-	test_expect_success "'worktree add' with $* has mutually exclusive options" '
	-		eval "set -- $arr" &&
	-		test_must_fail git worktree add "$@"
	+	local args="$@" &&
	+	test_expect_success "'worktree add' with '$args' has mutually exclusive options" '
	+		test_must_fail git worktree add $args
	 	'
	 }
	 
	@@ -398,21 +376,20 @@ test_expect_success '"add" worktree with orphan branch, lock, and reason' '
	 '
	 
	 test_wt_add_empty_repo_orphan_hint () {
	-	local context="$1"
	-	shift
	-	local arr=$(save_param_arr "$@")
	+	local context="$1" &&
	+	shift &&
	+	local args="$@" &&
	 	test_expect_success "'worktree add' show orphan hint in empty repo w/ $context" '
	-		eval "set -- $arr" &&
	 		test_when_finished "rm -rf empty_repo" &&
	 		GIT_DIR="empty_repo" git init --bare &&
	-		test_must_fail git -C empty_repo worktree add "$@" foobar/ 2> actual &&
	+		test_must_fail git -C empty_repo worktree add $args foobar/ 2> actual &&
	 		grep "hint: If you meant to create a worktree containing a new orphan branch" actual
	 	'
	 }
	 
	-test_wt_add_empty_repo_orphan_hint 'DWIM'
	-test_wt_add_empty_repo_orphan_hint '-b' -b foobar_branch
	-test_wt_add_empty_repo_orphan_hint '-B' -B foobar_branch
	+test_wt_add_empty_repo_orphan_hint DWIM
	+test_wt_add_empty_repo_orphan_hint -b -b foobar_branch
	+test_wt_add_empty_repo_orphan_hint -B -B foobar_branch
	 
	 test_expect_success 'local clone from linked checkout' '
	 	git clone --local here here-clone &&

Note also that you lost the &&-chaining.

If we do want to be slightly paranoid about it, doesn't just creating a:

	local args_str="$*" &&

And then using that in the description argument to test_expect_success() also address Junio's feedback, without the need for this quoting helper?