Re: [PATCH v5 2/4] worktree add: refactor opt exclusion tests

From: Junio C Hamano <gitster@pobox.com>
Date: Dec 20, 2022, 04:00 UTC
Message-ID: <xmqqsfhawwqf.fsf@gitster.g>
In-Reply-To: <20221220023637.29042-3-jacobabel@nullpo.dev>

Jacob Abel <jacobabel@nullpo.dev> writes:

Show 7 quoted lines

> Pull duplicate test code into a function so that additional opt
> combinations can be tested succinctly.
>
> Signed-off-by: Jacob Abel <jacobabel@nullpo.dev>
> ---
>  t/t2400-worktree-add.sh | 20 ++++++++++----------
>  1 file changed, 10 insertions(+), 10 deletions(-)

OK.

> +# Helper function to test mutually exclusive options.
> +test_wt_add_excl() {

Have SP on both sides of (), i.e.

	test_wt_add_excl () {

Show 5 quoted lines

> +	local opts="$@" &&
> +	test_expect_success "'worktree add' with '$opts' has mutually exclusive options" '
> +		test_must_fail git worktree add $opts
> +	'
> +}

In this particular case with the given arguments, it does not make a difference, but make it a habit to think twice if "$*" is what you want when you write "$@". "$@" does a lot more than just concatenate $1, $2, ... with SP in between, and the use of $opts in the message merely wants the "concatenate with SP" behaviour, which is what "$*" is for.

I actually would write the above if I were doing this patch:

    test_wt_add_excl () {
	test_expect_success "'worktree add' with $* has mutually exclusive options" '
		test_must_fail git worktree add "$@"
	'
    }

Notice the use of "$@" on the "git worktree add" invocation? This allows callers of test_wt_add_excl pass parameters with SP in it, thanks to the magic "$@". Again, as I said earlier, for these callers ...

> +test_wt_add_excl -b poodle -B poodle bamboo main
> +test_wt_add_excl -b poodle --detach bamboo main
> +test_wt_add_excl -B poodle --detach bamboo main

... the distinction does not matter, but

    helper --lock --reason "my reason with multiple words" bamboo main

must be written with "$@", like so:

    helper () {
	git worktree add "$@"
    }

not

    helper () { # BAD
	local opt="$@"
	git worktree add $opt
    }

$opt in this case is a SP-concatenated string

	opt="--lock --reason my reason with multiple words bamboo main"

and passing it without quotes around it to "git worktree add" will give only "my" as the parameter to the option "--reason", with three extra words on the comand line.