| Message ID | 20221228061539.13740-3-jacobabel@nullpo.dev (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | worktree: Support `--orphan` when creating new worktrees | expand |
Jacob Abel <jacobabel@nullpo.dev> writes: > +# 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 Please do not copy from source with unknown licensing terms. Isn't it sufficient to stringify "$*" and let it later split at $IFS boundary for the particular purpose of this test anyway?
On 22/12/28 09:54PM, Junio C Hamano wrote: > Jacob Abel <jacobabel@nullpo.dev> writes: > > > +# 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 > > Please do not copy from source with unknown licensing terms. Understood. I have removed it. > > Isn't it sufficient to stringify "$*" and let it later split at $IFS > boundary for the particular purpose of this test anyway? Yes for these particular tests that should be acceptable. I tried putting an alternative together that still provides an avenue for handling quoted arguments without the helper [1]. Would this work? 1. https://lore.kernel.org/git/20221229063823.ij3jjuaar2fsayju@phi/
Jacob Abel <jacobabel@nullpo.dev> writes: > Yes for these particular tests that should be acceptable. I tried putting an > alternative together that still provides an avenue for handling quoted > arguments without the helper [1]. Would this work? > > 1. https://lore.kernel.org/git/20221229063823.ij3jjuaar2fsayju@phi/ Do we even need to handle an argument with $IFS whitespaces in it in these tests? If not, using "$@" where we pass the args to the commands we run, and using "$*" where we merely use it for human readable messages, would be sufficient.
On 22/12/29 07:07PM, Junio C Hamano wrote: > Jacob Abel <jacobabel@nullpo.dev> writes: > > > Yes for these particular tests that should be acceptable. I tried putting an > > alternative together that still provides an avenue for handling quoted > > arguments without the helper [1]. Would this work? > > > > 1. https://lore.kernel.org/git/20221229063823.ij3jjuaar2fsayju@phi/ > > Do we even need to handle an argument with $IFS whitespaces in it in > these tests? No. I was trying to make the change because you had suggested a change in the last revision [2]. > > If not, using "$@" where we pass the args to the commands we run, > and using "$*" where we merely use it for human readable messages, > would be sufficient. > We can't do this because it causes all the existing tests using these functions to fail. --- The diff I showed in [1] passes all the tests and those two tests with quoted strings were only temporarily added to illustrate that this works. They would be removed from the code prior to the v7 patch. --- From that diff, we can change $* to $@ (or any quoted variation of $@) such as below: test_wt_add_excl () { test_expect_success "'worktree add' with $* has mutually exclusive options" " - test_must_fail git worktree add $* 2>actual && + test_must_fail git worktree add \"$@\" 2>actual && grep -P 'fatal:( options)? .* cannot be used together' actual " } however this results in the tests failing with: error: bug in the test script: not 2 or 3 parameters to test-expect-success --- If we change back to single quotes and use "$@": test_wt_add_excl () { - 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_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 + ' } then the tests fail because test_expect_success changes $@ to its own arguments and the line: test_must_fail git worktree add "$@" 2>actual && expands into: test_must_fail git worktree add ' test_must_fail git worktree add "$@" 2>actual && grep -P "fatal:( options)? .* cannot be used together" actual ' --- Alternatively we could do: test_wt_add_excl () { - test_expect_success "'worktree add' with $* has mutually exclusive options" " - test_must_fail git worktree add $* 2>actual && + local opts="$@" && + test_expect_success "'worktree add' with '$opts' has mutually exclusive options" ' + test_must_fail git worktree add $opts 2>actual && grep -P 'fatal:( options)? .* cannot be used together' actual - " + ' } which would also work however this would just be reverting to the v5 revision [3] where you left your orignal review [2]. This would be essentually the same change that Ævar recommended [4]. --- So from my understanding of the situation, the only two options that pass all the existing tests are either: A: Use the diff in [1] without the two quote example tests included. B: Revert the changes to how this was done in v5 [3]. Both of these options work with me however option A will allow test authors to easily escape quotes if new tests needed to be added at some point in the future. 1. https://lore.kernel.org/git/20221229063823.ij3jjuaar2fsayju@phi/ 2. https://lore.kernel.org/git/xmqqsfhawwqf.fsf@gitster.g/ 3. https://lore.kernel.org/git/20221220023637.29042-3-jacobabel@nullpo.dev/ 4. https://lore.kernel.org/git/221228.868risuf5x.gmgdl@evledraar.gmail.com/
On 22/12/29 03:49PM, Jacob Abel wrote: > > [...] > > So from my understanding of the situation, the only two options that pass all > the existing tests are either: > > A: Use the diff in [1] without the two quote example tests included. > > B: Revert the changes to how this was done in v5 [3]. > > Both of these options work with me however option A will allow test authors to > easily escape quotes if new tests needed to be added at some point in the future. > > 1. https://lore.kernel.org/git/20221229063823.ij3jjuaar2fsayju@phi/ > 2. https://lore.kernel.org/git/xmqqsfhawwqf.fsf@gitster.g/ > 3. https://lore.kernel.org/git/20221220023637.29042-3-jacobabel@nullpo.dev/ > 4. https://lore.kernel.org/git/221228.868risuf5x.gmgdl@evledraar.gmail.com/ Sorry to poke this but I wanted to confirm which path I should proceed with. Both options are functionally complete and it'd just be a matter of choosing which version to push out for the revision.
Jacob Abel <jacobabel@nullpo.dev> writes: > On 22/12/29 03:49PM, Jacob Abel wrote: >> >> [...] >> >> So from my understanding of the situation, the only two options that pass all >> the existing tests are either: >> >> A: Use the diff in [1] without the two quote example tests included. >> >> B: Revert the changes to how this was done in v5 [3]. > ... > Sorry to poke this but I wanted to confirm which path I should proceed with. > Both options are functionally complete and it'd just be a matter of choosing > which version to push out for the revision. I think B. with "$@" -> "$*" (because you only want a flattened stringified version of the arguments in $opt to insert into the test name string) would be the more sensible avenue. Let's not over-engineer the tests---it is not the point of these new tests to ensure that "git worktree add" can take arguments that require to be quoted on the command line. Thanks.
On 23/01/06 09:34PM, Junio C Hamano wrote: > Jacob Abel <jacobabel@nullpo.dev> writes: > > > On 22/12/29 03:49PM, Jacob Abel wrote: > >> > >> [...] > >> > >> So from my understanding of the situation, the only two options that pass all > >> the existing tests are either: > >> > >> A: Use the diff in [1] without the two quote example tests included. > >> > >> B: Revert the changes to how this was done in v5 [3]. > > ... > > Sorry to poke this but I wanted to confirm which path I should proceed with. > > Both options are functionally complete and it'd just be a matter of choosing > > which version to push out for the revision. > > I think B. with "$@" -> "$*" (because you only want a flattened > stringified version of the arguments in $opt to insert into the > test name string) would be the more sensible avenue. Let's not > over-engineer the tests---it is not the point of these new tests to > ensure that "git worktree add" can take arguments that require to be > quoted on the command line. > > Thanks. Perfect, Thank you. The revision should be out shortly.
diff --git a/t/t2400-worktree-add.sh b/t/t2400-worktree-add.sh index d587e0b20d..bfbf13d6a4 100755 --- a/t/t2400-worktree-add.sh +++ b/t/t2400-worktree-add.sh @@ -298,17 +298,39 @@ test_expect_success '"add" no auto-vivify with --detach and <branch> omitted' ' test_must_fail git -C mish/mash symbolic-ref HEAD ' -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 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 '"add" -B/--detach mutually exclusive' ' - test_must_fail git worktree add -B poodle --detach bamboo 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 test_expect_success '"add -B" fails if the branch is checked out' ' git rev-parse newmain >before &&
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 | 40 +++++++++++++++++++++++++++++++--------- 1 file changed, 31 insertions(+), 9 deletions(-) -- 2.38.2