From: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
To: Jacob Abel <jacobabel@nullpo.dev>
Cc: git@vger.kernel.org, "Eric Sunshine" <sunshine@sunshineco.com>,
"Junio C Hamano" <gitster@pobox.com>,
"Phillip Wood" <phillip.wood123@gmail.com>,
"Rubén Justo" <rjusto@gmail.com>, "Taylor Blau" <me@ttaylorr.com>,
rsbecker@nexbridge.com
Subject: Re: [PATCH v6 0/4] worktree: Support `--orphan` when creating new worktrees
Date: Thu, 29 Dec 2022 11:42:49 +0100 [thread overview]
Message-ID: <221229.86v8lutr6g.gmgdl@evledraar.gmail.com> (raw)
In-Reply-To: <20221229063823.ij3jjuaar2fsayju@phi>
On Thu, Dec 29 2022, Jacob Abel wrote:
> 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.
next prev parent reply other threads:[~2022-12-29 10:55 UTC|newest]
Thread overview: 129+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-11-04 1:02 [PATCH 0/4] worktree: Support `--orphan` when creating new worktrees Jacob Abel
2022-11-04 1:03 ` [PATCH 1/4] worktree add: Include -B in usage docs Jacob Abel
2022-11-04 3:05 ` Eric Sunshine
2022-11-04 4:24 ` Jacob Abel
2022-11-04 1:03 ` [PATCH 2/4] builtin/worktree.c: Update checkout_worktree() to use git-worktree Jacob Abel
2022-11-04 1:32 ` Ævar Arnfjörð Bjarmason
2022-11-04 3:58 ` Jacob Abel
2022-11-04 20:45 ` Taylor Blau
2022-11-04 1:03 ` [PATCH 3/4] worktree add: add --orphan flag Jacob Abel
2022-11-04 1:33 ` Ævar Arnfjörð Bjarmason
2022-11-04 4:11 ` Jacob Abel
2022-11-04 5:03 ` Eric Sunshine
2022-11-04 16:41 ` Jacob Abel
2022-11-10 4:13 ` Eric Sunshine
2022-11-10 21:21 ` Jacob Abel
2022-11-04 1:03 ` [PATCH 4/4] worktree add: Add unit tests for --orphan Jacob Abel
2022-11-04 1:37 ` Ævar Arnfjörð Bjarmason
2022-11-04 4:17 ` Jacob Abel
2022-11-04 4:33 ` [PATCH 0/4] worktree: Support `--orphan` when creating new worktrees Eric Sunshine
2022-11-04 4:47 ` Jacob Abel
2022-11-04 4:50 ` Jacob Abel
2022-11-04 21:34 ` [PATCH v2 0/2] " Jacob Abel
2022-11-04 21:34 ` [PATCH v2 1/2] worktree add: Include -B in usage docs Jacob Abel
2022-11-04 21:34 ` [PATCH v2 2/2] worktree add: add --orphan flag Jacob Abel
2022-11-10 23:32 ` [PATCH v3 0/2] worktree: Support `--orphan` when creating new worktrees Jacob Abel
2022-11-10 23:32 ` [PATCH v3 1/2] worktree add: Include -B in usage docs Jacob Abel
2022-11-10 23:32 ` [PATCH v3 2/2] worktree add: add --orphan flag Jacob Abel
2022-11-15 21:08 ` Ævar Arnfjörð Bjarmason
2022-11-15 21:29 ` Eric Sunshine
2022-11-15 22:35 ` Ævar Arnfjörð Bjarmason
2022-11-16 0:19 ` Eric Sunshine
2022-11-19 3:13 ` Jacob Abel
2022-11-19 3:09 ` Jacob Abel
2022-11-19 11:50 ` Ævar Arnfjörð Bjarmason
2022-11-19 1:44 ` Jacob Abel
2022-11-22 6:00 ` Eric Sunshine
2022-11-22 23:09 ` Jacob Abel
2022-11-15 22:09 ` Ævar Arnfjörð Bjarmason
2022-11-19 2:57 ` Jacob Abel
2022-11-19 11:50 ` Ævar Arnfjörð Bjarmason
2022-11-16 0:39 ` [PATCH v3 0/2] worktree: Support `--orphan` when creating new worktrees Eric Sunshine
2022-11-17 10:00 ` Ævar Arnfjörð Bjarmason
2022-11-19 3:47 ` Jacob Abel
2022-11-19 11:48 ` Ævar Arnfjörð Bjarmason
2022-11-22 5:16 ` Eric Sunshine
2022-11-22 23:26 ` Jacob Abel
2022-11-22 23:55 ` Ævar Arnfjörð Bjarmason
2022-11-23 2:47 ` Jacob Abel
2022-11-23 2:43 ` Rubén Justo
2022-11-23 5:37 ` Jacob Abel
2022-11-23 7:35 ` Rubén Justo
2022-11-22 14:45 ` Phillip Wood
2022-11-23 4:21 ` Jacob Abel
2022-12-12 1:42 ` [PATCH v4 0/3] " Jacob Abel
2022-12-12 1:42 ` [PATCH v4 1/3] worktree add: Include -B in usage docs Jacob Abel
2022-12-12 1:42 ` [PATCH v4 2/3] worktree add: add --orphan flag Jacob Abel
2022-12-12 8:11 ` Ævar Arnfjörð Bjarmason
2022-12-12 14:55 ` Jacob Abel
2022-12-12 18:14 ` Ævar Arnfjörð Bjarmason
2022-12-12 22:39 ` Jacob Abel
2022-12-12 1:43 ` [PATCH v4 3/3] worktree add: Add hint to use --orphan when bad ref Jacob Abel
2022-12-12 8:35 ` Ævar Arnfjörð Bjarmason
2022-12-12 14:59 ` Jacob Abel
2022-12-12 18:16 ` Ævar Arnfjörð Bjarmason
2022-12-12 18:35 ` Eric Sunshine
2022-12-12 22:36 ` Jacob Abel
2022-12-12 22:38 ` Jacob Abel
2022-12-20 2:37 ` [PATCH v5 0/4] worktree: Support `--orphan` when creating new worktrees Jacob Abel
2022-12-20 2:37 ` [PATCH v5 1/4] worktree add: Include -B in usage docs Jacob Abel
2022-12-20 3:42 ` Junio C Hamano
2022-12-20 23:24 ` Jacob Abel
2022-12-20 2:37 ` [PATCH v5 2/4] worktree add: refactor opt exclusion tests Jacob Abel
2022-12-20 4:00 ` Junio C Hamano
2022-12-20 23:29 ` Jacob Abel
2022-12-20 2:38 ` [PATCH v5 3/4] worktree add: add --orphan flag Jacob Abel
2022-12-20 4:19 ` Junio C Hamano
2022-12-21 0:17 ` Jacob Abel
2022-12-20 2:38 ` [PATCH v5 4/4] worktree add: Add hint to use --orphan when bad ref Jacob Abel
2022-12-20 6:18 ` Junio C Hamano
2022-12-21 0:42 ` Jacob Abel
2022-12-28 6:16 ` [PATCH v6 0/4] worktree: Support `--orphan` when creating new worktrees Jacob Abel
2022-12-28 6:16 ` [PATCH v6 1/4] worktree add: include -B in usage docs Jacob Abel
2022-12-28 6:16 ` [PATCH v6 2/4] worktree add: refactor opt exclusion tests Jacob Abel
2022-12-28 12:54 ` Junio C Hamano
2022-12-29 6:51 ` Jacob Abel
2022-12-29 10:07 ` Junio C Hamano
2022-12-29 20:48 ` Jacob Abel
2023-01-06 6:31 ` Jacob Abel
2023-01-06 12:34 ` Junio C Hamano
2023-01-07 4:45 ` Jacob Abel
2022-12-28 6:17 ` [PATCH v6 3/4] worktree add: add --orphan flag Jacob Abel
2022-12-28 6:17 ` [PATCH v6 4/4] worktree add: add hint to direct users towards --orphan Jacob Abel
2023-01-06 14:19 ` Phillip Wood
2022-12-28 8:01 ` [PATCH v6 0/4] worktree: Support `--orphan` when creating new worktrees Ævar Arnfjörð Bjarmason
2022-12-29 6:38 ` Jacob Abel
2022-12-29 10:42 ` Ævar Arnfjörð Bjarmason [this message]
2022-12-29 21:22 ` Jacob Abel
2023-01-07 4:58 ` [PATCH v7 " Jacob Abel
2023-01-07 4:59 ` [PATCH v7 1/4] worktree add: include -B in usage docs Jacob Abel
2023-01-07 4:59 ` [PATCH v7 2/4] worktree add: refactor opt exclusion tests Jacob Abel
2023-01-08 7:13 ` Junio C Hamano
2023-01-08 15:08 ` Jacob Abel
2023-01-07 4:59 ` [PATCH v7 3/4] worktree add: add --orphan flag Jacob Abel
2023-01-07 4:59 ` [PATCH v7 4/4] worktree add: add hint to direct users towards --orphan Jacob Abel
2023-01-09 12:26 ` [PATCH v7 0/4] worktree: Support `--orphan` when creating new worktrees Ævar Arnfjörð Bjarmason
2023-01-09 17:11 ` Jacob Abel
2023-01-09 17:21 ` Ævar Arnfjörð Bjarmason
2023-01-09 17:26 ` Jacob Abel
2023-01-09 17:32 ` [PATCH v8 " Jacob Abel
2023-01-09 17:32 ` [PATCH v8 1/4] worktree add: include -B in usage docs Jacob Abel
2023-01-09 17:33 ` [PATCH v8 2/4] worktree add: refactor opt exclusion tests Jacob Abel
2023-01-09 17:33 ` [PATCH v8 3/4] worktree add: add --orphan flag Jacob Abel
2023-01-13 10:20 ` Phillip Wood
2023-01-13 17:32 ` Junio C Hamano
2023-01-14 22:47 ` Jacob Abel
2023-01-15 3:09 ` Junio C Hamano
2023-01-15 3:41 ` rsbecker
2023-01-15 3:49 ` Junio C Hamano
2023-01-18 22:46 ` 'Jacob Abel'
2023-01-18 22:18 ` Jacob Abel
2023-01-19 15:32 ` Ævar Arnfjörð Bjarmason
2023-01-19 16:32 ` Junio C Hamano
2023-01-16 10:47 ` Phillip Wood
2023-01-18 22:40 ` Jacob Abel
2023-01-19 16:18 ` Phillip Wood
2023-01-19 22:20 ` Jacob Abel
2023-01-09 17:33 ` [PATCH v8 4/4] worktree add: add hint to direct users towards --orphan Jacob Abel
2023-01-09 19:20 ` [PATCH v8 0/4] worktree: Support `--orphan` when creating new worktrees Ævar Arnfjörð Bjarmason
2023-01-13 17:34 ` Junio C Hamano
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=221229.86v8lutr6g.gmgdl@evledraar.gmail.com \
--to=avarab@gmail.com \
--cc=git@vger.kernel.org \
--cc=gitster@pobox.com \
--cc=jacobabel@nullpo.dev \
--cc=me@ttaylorr.com \
--cc=phillip.wood123@gmail.com \
--cc=rjusto@gmail.com \
--cc=rsbecker@nexbridge.com \
--cc=sunshine@sunshineco.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).