| Message ID | 20221220023637.29042-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: > 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 | 20 ++++++++++---------- > 1 file changed, 10 insertions(+), 10 deletions(-) OK. > +# Helper function to test mutually exclusive options. > +test_wt_add_excl() { Have SP on both sides of (), i.e. test_wt_add_excl () { > + local opts="$@" && > + test_expect_success "'worktree add' with '$opts' has mutually exclusive options" ' > + test_must_fail git worktree add $opts > + ' > +} In this particular case with the given arguments, it does not make a difference, but make it a habit to think twice if "$*" is what you want when you write "$@". "$@" does a lot more than just concatenate $1, $2, ... with SP in between, and the use of $opts in the message merely wants the "concatenate with SP" behaviour, which is what "$*" is for. I actually would write the above if I were doing this patch: test_wt_add_excl () { test_expect_success "'worktree add' with $* has mutually exclusive options" ' test_must_fail git worktree add "$@" ' } Notice the use of "$@" on the "git worktree add" invocation? This allows callers of test_wt_add_excl pass parameters with SP in it, thanks to the magic "$@". Again, as I said earlier, for these callers ... > +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 ... the distinction does not matter, but helper --lock --reason "my reason with multiple words" bamboo main must be written with "$@", like so: helper () { git worktree add "$@" } not helper () { # BAD local opt="$@" git worktree add $opt } $opt in this case is a SP-concatenated string opt="--lock --reason my reason with multiple words bamboo main" and passing it without quotes around it to "git worktree add" will give only "my" as the parameter to the option "--reason", with three extra words on the comand line.
On 22/12/20 01:00PM, Junio C Hamano wrote: > Jacob Abel <jacobabel@nullpo.dev> writes: > > > 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 | 20 ++++++++++---------- > > 1 file changed, 10 insertions(+), 10 deletions(-) > > OK. > > > +# Helper function to test mutually exclusive options. > > +test_wt_add_excl() { > > Have SP on both sides of (), i.e. > > test_wt_add_excl () { Done. > > > + local opts="$@" && > > + test_expect_success "'worktree add' with '$opts' has mutually exclusive options" ' > > + test_must_fail git worktree add $opts > > + ' > > +} > > In this particular case with the given arguments, it does not make a > difference, but make it a habit to think twice if "$*" is what you > want when you write "$@". "$@" does a lot more than just concatenate > $1, $2, ... with SP in between, and the use of $opts in the message > merely wants the "concatenate with SP" behaviour, which is what "$*" > is for. Understood. > > I actually would write the above if I were doing this patch: > > test_wt_add_excl () { > test_expect_success "'worktree add' with $* has mutually exclusive options" ' > test_must_fail git worktree add "$@" > ' > } > Changed. > Notice the use of "$@" on the "git worktree add" invocation? This > allows callers of test_wt_add_excl pass parameters with SP in it, > thanks to the magic "$@". Again, as I said earlier, for these > callers ... > > > +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 > > ... the distinction does not matter, but > > helper --lock --reason "my reason with multiple words" bamboo main > > must be written with "$@", like so: > > helper () { > git worktree add "$@" > } > > not > > helper () { # BAD > local opt="$@" > git worktree add $opt > } > > $opt in this case is a SP-concatenated string > > opt="--lock --reason my reason with multiple words bamboo main" > > and passing it without quotes around it to "git worktree add" > will give only "my" as the parameter to the option "--reason", > with three extra words on the comand line. This makes sense. I'm not the best with shell scripting so I appreciate the explanation.
diff --git a/t/t2400-worktree-add.sh b/t/t2400-worktree-add.sh index d587e0b20d..f05e2483c2 100755 --- a/t/t2400-worktree-add.sh +++ b/t/t2400-worktree-add.sh @@ -298,17 +298,17 @@ 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 -' - -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_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 | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) -- 2.38.2