On 22/12/29 07:07PM, Junio C Hamano wrote: > Jacob Abel <jacobabel@xxxxxxxxxx> 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@xxxxxxxxxx/ 4. https://lore.kernel.org/git/221228.868risuf5x.gmgdl@xxxxxxxxxxxxxxxxxxx/
