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

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Dec 29, 2022, 10:42 UTC
Message-ID: <221229.86v8lutr6g.gmgdl@evledraar.gmail.com>
In-Reply-To: <20221229063823.ij3jjuaar2fsayju@phi>

On Thu, Dec 29 2022, Jacob Abel wrote:

Show 67 quoted lines

> On 22/12/28 09:01AM, Ævar Arnfjörð Bjarmason wrote:
>>
>> On Wed, Dec 28 2022, Jacob Abel wrote:
>> [...]
>> 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:
>
> Understood.
> [...]
>>
>> 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?
>
> Below is what I have come up with while still not needing the
> quoting helper. Could this work as an alternative?
>
> It doesn't handle quotes properly without a bit of help from the
> test author but it can handle them as long as you double escape the string.
>
> The diff also includes slight tweaks to the tests themselves to better verify
> the behavior.
>
> Note: The two extra tests added in the diff wouldn't be in the next revision but they
> are there to demonstrate that things work as expected with this change.
>
> [...]
>  # Helper function to test mutually exclusive options.
> +#
> +# Note: Any arguments that contain spaces must be double and single quoted, ex:
> +# test_wt_add_excl -b poodle --detach bamboo --lock --reason "'the reason'" main
>  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 "$@"
> -	'
> +	test_expect_success "'worktree add' with $* has mutually exclusive options" "
> +		test_must_fail git worktree add $* 2>actual &&
> +		grep -P 'fatal:( options)? .* cannot be used together' actual
> +	"
>  }
>
> +test_wt_add_excl -b poodle --detach bamboo --lock --reason "'the reason'" main
>  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
> @@ -397,19 +379,22 @@ test_expect_success '"add" worktree with orphan branch, lock, and reason' '
>  	test_cmp expect .git/worktrees/orphan-with-lock-reason/locked
>  '
>
> +# Note: Any arguments (except the first argument) that contain spaces must be
> +# double and single quoted, ex:
> +# test_wt_add_empty_repo_orphan_hint 'the context' --lock --reason "'the reason'"
> [...]
> +test_wt_add_empty_repo_orphan_hint 'the context' --lock --reason "'the reason'"
>  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

I haven't even tested this, but I'm still confused here, isn't this just a solution in seach of a problem?

To answer your question above: Yes, you can come up with shellscript code that handles this sort of quoting problem, but it's generally a Bad Idea and should be avoided.

*If* a function needs to take arguments that are quoted it's much better to "unpack" those arguments in full, and then pass them on, see e.g. how the "test_commit" helper in test-lib-functions.sh does it.

But in this case there was no need whatsoever for doing any of this, as none of the tests wanted to pass such arguments, they didn't need to be quoted at all.

But now for this reply you've come up with one such test, hence the "solution in search of a problem?" above.

I.e. is this a useful test, or just an excercise to stress generic quote handling we don't really need?

I originally suggested creating these trivial helpers in an earlier round because it avoided the copy/pasting of a series of tests, I think the v5 you had (https://lore.kernel.org/git/20221212014003.20290-3-jacobabel@nullpo.dev/) struck the right balance there, although as Junio noted it might need the tweaking for $@ v.s. $*.

But once we have to handle quoted arguments the better solution is to just ... not use that helper. I.e. there's no reason you can't just do:

	test_wt_add_excl -b poodle -B poodle bamboo main
	test_wt_add_excl -b poodle --orphan poodle bamboo
	[...]

Then:

	test_expect_success 'worktree add with quoted --reason arguments and --orphan' '
		test_must_fail git worktree add  --orphan poodle --detach bamboo --reason "'\''blah blah'\''"
	'

You don't need to make test_wt_add_excl() handle that case.