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

[Jacob Abel <jacobabel@nullpo.dev>]

On 22/12/29 11:42AM, Ævar Arnfjörð Bjarmason wrote:
>
> On Thu, Dec 29 2022, Jacob Abel wrote:
> >
> > [...]
> > 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.
> >
> > [...]
> > +test_wt_add_excl -b poodle --detach bamboo --lock --reason "'the reason'" main
> > [...]
> > +test_wt_add_empty_repo_orphan_hint 'the context' --lock --reason "'the reason'"
>
> 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?

The latter. I included a note in my reply that those two tests were included
solely to illustrate the behavior was as expected and would not be included in
the actual patch revision.

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

I generally agree. I was mostly just trying to address the spirit of the
suggested changes.

As I mentioned at the end of my most recent reply to Junio [1], I'm OK with
either the diff I proposed [2] (minus the extra tests) or just reverting the
changes entirely and going with what we did in the v5 patch.

At the end of the day this seems like it was a minor nit on some test code that
ended up leading down a rabbit hole due to the multiple layers of nesting. I
only proposed the diff [2] because it seemed to fully address to original
concern without doing anything particularly egregious (unlike v6 w/ the helper).

So while I prefer my diff, I'm not at all attached to either solution and will
implement whatever you and Junio decide for v7 with no complaints.

1. https://lore.kernel.org/git/20221229204841.ol3r6z2pfrodv7yx@phi/
2. https://lore.kernel.org/git/20221229063823.ij3jjuaar2fsayju@phi/