| Message ID | 20221228061539.13740-1-jacobabel@nullpo.dev (mailing list archive) |
|---|---|
| Headers | show |
| Series | worktree: Support `--orphan` when creating new worktrees | expand |
On Wed, Dec 28 2022, Jacob Abel wrote: > * 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?
On 22/12/28 09:01AM, Ævar Arnfjörð Bjarmason wrote: > > On Wed, Dec 28 2022, Jacob Abel wrote: > > > * 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: Understood. > > 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. Ah yes. Apologies. Fixed. > > 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. diff --git a/t/t2400-worktree-add.sh b/t/t2400-worktree-add.sh index f43de59117..1d5843c956 100755 --- a/t/t2400-worktree-add.sh +++ b/t/t2400-worktree-add.sh @@ -298,36 +298,18 @@ 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. +# +# 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 () { - local context="$1" - shift - 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 "$@" foobar/ 2> actual && - grep "hint: If you meant to create a worktree containing a new orphan branch" actual - ' + local context="$1" && + shift && + test_expect_success "'worktree add' show orphan hint in empty repo w/ $context" " + 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 && + ! grep 'error: unknown switch' actual && + grep 'hint: If you meant to create a worktree containing a new orphan branch' actual + " } +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 -- 2.38.2 And the results of running `cd t/ && sh ./t2400-worktree-add.sh -x -r 1,32,49`: Initialized empty Git repository in /path/to/git/repo/t/trash directory.t2400-worktree-add/.git/ expecting success of 2400.1 'setup': test_commit init ++ test_commit init ++ local notick= ++ local echo=echo ++ local append= ++ local author= ++ local signoff= ++ local indir= ++ local tag=light ++ test 1 '!=' 0 ++ case "$1" in ++ break ++ indir= ++ local file=init.t ++ test -n '' ++ echo init ++ git add -- init.t ++ test -z '' ++ test_tick ++ test -z '' ++ test_tick=1112911993 ++ GIT_COMMITTER_DATE='1112911993 -0700' ++ GIT_AUTHOR_DATE='1112911993 -0700' ++ export GIT_COMMITTER_DATE GIT_AUTHOR_DATE ++ git commit -m init [main (root-commit) 2519212] init Author: A U Thor <author@example.com> 1 file changed, 1 insertion(+) create mode 100644 init.t ++ case "$tag" in ++ git tag init ok 1 - setup ok 2 # skip "add" an existing worktree (--run) ok 3 # skip "add" an existing empty worktree (--run) ok 4 # skip "add" using shorthand - fails when no previous branch (--run) ok 5 # skip "add" using - shorthand (--run) ok 6 # skip "add" refuses to checkout locked branch (--run) ok 7 # skip checking out paths not complaining about linked checkouts (--run) ok 8 # skip "add" worktree (--run) ok 9 # skip "add" worktree with lock (--run) ok 10 # skip "add" worktree with lock and reason (--run) ok 11 # skip "add" worktree with reason but no lock (--run) ok 12 # skip "add" worktree from a subdir (--run) ok 13 # skip "add" from a linked checkout (--run) ok 14 # skip "add" worktree creating new branch (--run) ok 15 # skip die the same branch is already checked out (--run) ok 16 # skip die the same branch is already checked out (symlink) (--run) ok 17 # skip not die the same branch is already checked out (--run) ok 18 # skip not die on re-checking out current branch (--run) ok 19 # skip "add" from a bare repo (--run) ok 20 # skip checkout from a bare repo without "add" (--run) ok 21 # skip "add" default branch of a bare repo (--run) ok 22 # skip "add" to bare repo with worktree config (--run) ok 23 # skip checkout with grafts (--run) ok 24 # skip "add" from relative HEAD (--run) ok 25 # skip "add -b" with <branch> omitted (--run) ok 26 # skip "add --detach" with <branch> omitted (--run) ok 27 # skip "add" with <branch> omitted (--run) ok 28 # skip "add" checks out existing branch of dwimd name (--run) ok 29 # skip "add <path>" dwim fails with checked out branch (--run) ok 30 # skip "add --force" with existing dwimd name doesnt die (--run) ok 31 # skip "add" no auto-vivify with --detach and <branch> omitted (--run) expecting success of 2400.32 ''worktree add' with -b poodle --detach bamboo --lock --reason 'the reason' main has mutually exclusive options': test_must_fail git worktree add -b poodle --detach bamboo --lock --reason 'the reason' main 2>actual && grep -P 'fatal:( options)? .* cannot be used together' actual ++ test_must_fail git worktree add -b poodle --detach bamboo --lock --reason 'the reason' main ++ case "$1" in ++ _test_ok= ++ test_must_fail_acceptable git worktree add -b poodle --detach bamboo --lock --reason 'the reason' main ++ test git = env ++ case "$1" in ++ return 0 ++ git worktree add -b poodle --detach bamboo --lock --reason 'the reason' main ++ exit_code=128 ++ test 128 -eq 0 ++ test_match_signal 13 128 ++ test 128 = 141 ++ test 128 = 269 ++ return 1 ++ test 128 -gt 129 ++ test 128 -eq 127 ++ test 128 -eq 126 ++ return 0 ++ grep -P 'fatal:( options)? .* cannot be used together' actual fatal: options '-b', '-B', and '--detach' cannot be used together ok 32 - 'worktree add' with -b poodle --detach bamboo --lock --reason 'the reason' main has mutually exclusive options ok 33 # skip 'worktree add' with -b poodle -B poodle bamboo main has mutually exclusive options (--run) ok 34 # skip 'worktree add' with -b poodle --detach bamboo main has mutually exclusive options (--run) ok 35 # skip 'worktree add' with -B poodle --detach bamboo main has mutually exclusive options (--run) ok 36 # skip 'worktree add' with -B poodle --orphan poodle bamboo has mutually exclusive options (--run) ok 37 # skip 'worktree add' with -b poodle --orphan poodle bamboo has mutually exclusive options (--run) ok 38 # skip 'worktree add' with --orphan poodle --detach bamboo has mutually exclusive options (--run) ok 39 # skip 'worktree add' with --orphan poodle --no-checkout bamboo has mutually exclusive options (--run) ok 40 # skip 'worktree add' with --orphan poodle bamboo main has mutually exclusive options (--run) ok 41 # skip "add -B" fails if the branch is checked out (--run) ok 42 # skip add -B (--run) ok 43 # skip add --quiet (--run) ok 44 # skip "add --orphan" (--run) ok 45 # skip "add --orphan" fails if the branch already exists (--run) ok 46 # skip "add --orphan" with empty repository (--run) ok 47 # skip "add" worktree with orphan branch and lock (--run) ok 48 # skip "add" worktree with orphan branch, lock, and reason (--run) expecting success of 2400.49 ''worktree add' show orphan hint in empty repo w/ the context': test_when_finished 'rm -rf empty_repo' && GIT_DIR='empty_repo' git init --bare && test_must_fail git -C empty_repo worktree add --lock --reason 'the reason' foobar/ 2>actual && ! grep 'error: unknown switch' actual && grep 'hint: If you meant to create a worktree containing a new orphan branch' actual ++ test_when_finished 'rm -rf empty_repo' ++ test 0 = 0 ++ test_cleanup='{ rm -rf empty_repo } && (exit "$eval_ret"); eval_ret=$?; :' ++ GIT_DIR=empty_repo ++ git init --bare Initialized empty Git repository in /path/to/git/repo/t/trash directory.t2400-worktree-add/empty_repo/ ++ test_must_fail git -C empty_repo worktree add --lock --reason 'the reason' foobar/ ++ case "$1" in ++ _test_ok= ++ test_must_fail_acceptable git -C empty_repo worktree add --lock --reason 'the reason' foobar/ ++ test git = env ++ case "$1" in ++ return 0 ++ git -C empty_repo worktree add --lock --reason 'the reason' foobar/ ++ exit_code=128 ++ test 128 -eq 0 ++ test_match_signal 13 128 ++ test 128 = 141 ++ test 128 = 269 ++ return 1 ++ test 128 -gt 129 ++ test 128 -eq 127 ++ test 128 -eq 126 ++ return 0 ++ grep 'error: unknown switch' actual ++ grep 'hint: If you meant to create a worktree containing a new orphan branch' actual hint: If you meant to create a worktree containing a new orphan branch ++ rm -rf empty_repo ++ exit 0 ++ eval_ret=0 ++ : ok 49 - 'worktree add' show orphan hint in empty repo w/ the context ok 50 # skip 'worktree add' show orphan hint in empty repo w/ DWIM (--run) ok 51 # skip 'worktree add' show orphan hint in empty repo w/ -b (--run) ok 52 # skip 'worktree add' show orphan hint in empty repo w/ -B (--run) ok 53 # skip local clone from linked checkout (--run) ok 54 # skip local clone --shared from linked checkout (--run) ok 55 # skip "add" worktree with --no-checkout (--run) ok 56 # skip "add" worktree with --checkout (--run) ok 57 # skip put a worktree under rebase (--run) ok 58 # skip add a worktree, checking out a rebased branch (--run) ok 59 # skip checking out a rebased branch from another worktree (--run) ok 60 # skip not allow to delete a branch under rebase (--run) ok 61 # skip rename a branch under rebase not allowed (--run) ok 62 # skip check out from current worktree branch ok (--run) ok 63 # skip checkout a branch under bisect (--run) ok 64 # skip rename a branch under bisect not allowed (--run) ok 65 # skip --track sets up tracking (--run) ok 66 # skip "add" <path> <remote/branch> w/ no HEAD (--run) ok 67 # skip --no-track avoids setting up tracking (--run) ok 68 # skip "add" <path> <non-existent-branch> fails (--run) ok 69 # skip "add" <path> <branch> dwims (--run) ok 70 # skip "add" <path> <branch> dwims with checkout.defaultRemote (--run) ok 71 # skip git worktree add does not match remote (--run) ok 72 # skip git worktree add --guess-remote sets up tracking (--run) ok 73 # skip git worktree add with worktree.guessRemote sets up tracking (--run) ok 74 # skip git worktree --no-guess-remote option overrides config (--run) ok 75 # skip "add" invokes post-checkout hook (branch) (--run) ok 76 # skip "add" invokes post-checkout hook (detached) (--run) ok 77 # skip "add --no-checkout" suppresses post-checkout hook (--run) ok 78 # skip "add" in other worktree invokes post-checkout hook (--run) ok 79 # skip "add" in bare repo invokes post-checkout hook (--run) ok 80 # skip "add" an existing but missing worktree (--run) ok 81 # skip "add" an existing locked but missing worktree (--run) ok 82 # skip "add" not tripped up by magic worktree matching" (--run) ok 83 # skip sanitize generated worktree name (--run) ok 84 # skip "add" should not fail because of another bad worktree (--run) ok 85 # skip "add" with uninitialized submodule, with submodule.recurse unset (--run) ok 86 # skip "add" with uninitialized submodule, with submodule.recurse set (--run) ok 87 # skip "add" with initialized submodule, with submodule.recurse unset (--run) ok 88 # skip "add" with initialized submodule, with submodule.recurse set (--run) # passed all 88 test(s) 1..88
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.
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/
While working with the worktree based git workflow, I realised that setting up a new git repository required switching between the traditional and worktree based workflows. Searching online I found a SO answer [1] which seemed to support this and which indicated that adding support for this should not be technically difficult. This patchset has four parts: * adding `-B` to the usage docs (noticed during dev and it seemed too small to justify a separate submission) * adding a helper fn to simplify testing for mutual exclusion of options in `t/t2400-worktree-add.sh` * adding orphan branch functionality (as is present in `git-switch`) to `git-worktree-add` * adding an advise for using --orphan when `git worktree add` fails due to a bad ref. Changes from v5: * Touched up commit title and message for 1/4 [2]. * Changed `[-b | -B]` to `(-b | -B)` in 1/4 [2]. * Add whitespace to both sides of () for test_wt_add_excl() in t2400 (2/4) [3]. * Changed test_wt_add_excl() to use `$*` and `$@` instead of `$opts` (2/4) [3]. * Added save_param_arr() to preserve "$@" from being reset by test_expect_success() in test_wt_add_excl() (2/4). * Reordered Signed-off-by lines in 3/4 [4]. * Cleaned up commit message in 3/4 to better illustrate the change [4]. * Cleaned up commit message in 4/4 to better illustrate the change. * Cleaned up commit title in 4/4 [5]. * Changed test_wt_add_empty_repo_orphan_hint() to use `$@` instead of `$opts` (4/4) [5]. 1. https://stackoverflow.com/a/68717229/15064705/ 2. https://lore.kernel.org/git/xmqqo7ryyc4f.fsf@gitster.g/ 3. https://lore.kernel.org/git/xmqqsfhawwqf.fsf@gitster.g/ 4. https://lore.kernel.org/git/xmqqlen2wvtn.fsf@gitster.g/ 5. https://lore.kernel.org/git/xmqqfsdawqbw.fsf@gitster.g/ Jacob Abel (4): worktree add: include -B in usage docs worktree add: refactor opt exclusion tests worktree add: add --orphan flag worktree add: add hint to direct users towards --orphan Documentation/config/advice.txt | 4 ++ Documentation/git-worktree.txt | 17 ++++- advice.c | 1 + advice.h | 1 + builtin/worktree.c | 65 +++++++++++++++++-- t/t2400-worktree-add.sh | 110 +++++++++++++++++++++++++++++--- 6 files changed, 181 insertions(+), 17 deletions(-) Range-diff against v5: 1: 05371640ad ! 1: a9ef3eca7b worktree add: Include -B in usage docs @@ Metadata Author: Jacob Abel <jacobabel@nullpo.dev> ## Commit message ## - worktree add: Include -B in usage docs + worktree add: include -B in usage docs - While -B behavior is already documented, it was not included in the - usage docs for either the man page or the help text. This change fixes - that and brings the usage docs in line with how the flags are documented - in other commands such as git checkout. + Document `-B` next to where `-b` is already documented to bring the + usage docs in line with other commands such as git checkout. Signed-off-by: Jacob Abel <jacobabel@nullpo.dev> @@ Documentation/git-worktree.txt: SYNOPSIS [verse] 'git worktree add' [-f] [--detach] [--checkout] [--lock [--reason <string>]] - [-b <new-branch>] <path> [<commit-ish>] -+ [[-b | -B] <new-branch>] <path> [<commit-ish>] ++ [(-b | -B) <new-branch>] <path> [<commit-ish>] 'git worktree list' [-v | --porcelain [-z]] 'git worktree lock' [--reason <string>] <worktree> 'git worktree move' <worktree> <new-path> @@ builtin/worktree.c #define BUILTIN_WORKTREE_ADD_USAGE \ N_("git worktree add [-f] [--detach] [--checkout] [--lock [--reason <string>]]\n" \ - " [-b <new-branch>] <path> [<commit-ish>]") -+ " [[-b | -B] <new-branch>] <path> [<commit-ish>]") ++ " [(-b | -B) <new-branch>] <path> [<commit-ish>]") #define BUILTIN_WORKTREE_LIST_USAGE \ N_("git worktree list [-v | --porcelain [-z]]") #define BUILTIN_WORKTREE_LOCK_USAGE \ 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 " " ++} + -test_expect_success '"add" -b/--detach mutually exclusive' ' - test_must_fail git worktree add -b poodle --detach bamboo main -' +# Helper function to test mutually exclusive options. -+test_wt_add_excl() { -+ local opts="$@" && -+ test_expect_success "'worktree add' with '$opts' has mutually exclusive options" ' -+ test_must_fail git worktree add $opts ++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 "$@" + ' +} 3: ccae9cec2e ! 3: 9b93e2493a worktree add: add --orphan flag @@ Commit message workflow. Current Behavior: - - % git init --bare foo.git - Initialized empty Git repository in /path/to/foo.git/ + % git -C foo.git --no-pager branch -l + + main % git -C foo.git worktree add main/ Preparing worktree (new branch 'main') + HEAD is now at 6c93a75 a commit + % + + % git init bar.git + Initialized empty Git repository in /path/to/bar.git/ + % git -C bar.git --no-pager branch -l + + % git -C bar.git worktree add main/ + Preparing worktree (new branch 'main') fatal: not a valid object name: 'HEAD' % New Behavior: - % git init --bare foo.git - Initialized empty Git repository in /path/to/foo.git/ - % git -C foo.git worktree add --orphan main main/ + % git -C foo.git --no-pager branch -l + + main + % git -C foo.git worktree add main/ + Preparing worktree (new branch 'main') + HEAD is now at 6c93a75 a commit + % + + % git init --bare bar.git + Initialized empty Git repository in /path/to/bar.git/ + % git -C bar.git --no-pager branch -l + + % git -C bar.git worktree add main/ + Preparing worktree (new branch 'main') + fatal: invalid reference: HEAD + % git -C bar.git worktree add --orphan main main/ Preparing worktree (new branch 'main') % - Signed-off-by: Jacob Abel <jacobabel@nullpo.dev> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> + Signed-off-by: Jacob Abel <jacobabel@nullpo.dev> ## Documentation/git-worktree.txt ## @@ Documentation/git-worktree.txt: SYNOPSIS [verse] 'git worktree add' [-f] [--detach] [--checkout] [--lock [--reason <string>]] - [[-b | -B] <new-branch>] <path> [<commit-ish>] + [(-b | -B) <new-branch>] <path> [<commit-ish>] +'git worktree add' [-f] [--lock [--reason <string>]] + --orphan <new-branch> <path> 'git worktree list' [-v | --porcelain [-z]] @@ builtin/worktree.c #define BUILTIN_WORKTREE_ADD_USAGE \ N_("git worktree add [-f] [--detach] [--checkout] [--lock [--reason <string>]]\n" \ -- " [[-b | -B] <new-branch>] <path> [<commit-ish>]") -+ " [[-b | -B] <new-branch>] <path> [<commit-ish>]"), \ +- " [(-b | -B) <new-branch>] <path> [<commit-ish>]") ++ " [(-b | -B) <new-branch>] <path> [<commit-ish>]"), \ + N_("git worktree add [-f] [--lock [--reason <string>]]\n" \ + " --orphan <new-branch> <path>") + @@ builtin/worktree.c: static int add(int ac, const char **av, const char *prefix) strvec_push(&cp.args, "branch"); ## t/t2400-worktree-add.sh ## -@@ t/t2400-worktree-add.sh: test_wt_add_excl() { +@@ t/t2400-worktree-add.sh: test_wt_add_excl () { 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 4: df4c1fa469 ! 4: 737fca6986 worktree add: Add hint to use --orphan when bad ref @@ Metadata Author: Jacob Abel <jacobabel@nullpo.dev> ## Commit message ## - worktree add: Add hint to use --orphan when bad ref + worktree add: add hint to direct users towards --orphan Adds a new advice/hint in `git worktree add` for when the user tries to create a new worktree from a reference that doesn't exist. + Current Behavior: + + % git init --bare foo.git + Initialized empty Git repository in /path/to/foo.git/ + % git -C foo.git worktree add main/ + Preparing worktree (new branch 'main') + fatal: invalid reference: HEAD + % + + New Behavior: + + % git init --bare foo.git + Initialized empty Git repository in /path/to/foo.git/ + % git -C foo.git worktree add main/ + Preparing worktree (new branch 'main') + hint: If you meant to create a worktree containing a new orphan branch + hint: (branch with no commits) for this repository, you can do so + hint: using the --orphan option: + hint: + hint: git worktree add --orphan main ./main + hint: + hint: Disable this message with "git config advice.worktreeAddOrphan false" + fatal: invalid reference: HEAD + % + Signed-off-by: Jacob Abel <jacobabel@nullpo.dev> ## 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 + ' +} -- 2.38.2