| Message ID | 20240406000902.3082301-4-gitster@pobox.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 341aad8d41ca8321d826e1ce012e4faf1a8be2a4 |
| Headers | show |
| Series | local VAR="VAL" | expand |
On Fri, Apr 05, 2024 at 05:08:59PM -0700, Junio C Hamano wrote: > Future-proof test scripts that do > > local VAR=VAL > > without quoting VAL (which is OK in POSIX but broken in some shells) > that is a positional parameter, e.g. $4. > > Signed-off-by: Junio C Hamano <gitster@pobox.com> > --- > t/lib-parallel-checkout.sh | 2 +- > t/t2400-worktree-add.sh | 2 +- > t/t4210-log-i18n.sh | 4 ++-- > t/test-lib-functions.sh | 2 +- > 4 files changed, 5 insertions(+), 5 deletions(-) > > diff --git a/t/lib-parallel-checkout.sh b/t/lib-parallel-checkout.sh > index acaee9cbb6..8324d6c96d 100644 > --- a/t/lib-parallel-checkout.sh > +++ b/t/lib-parallel-checkout.sh > @@ -20,7 +20,7 @@ test_checkout_workers () { > BUG "too few arguments to test_checkout_workers" > fi && > > - local expected_workers=$1 && > + local expected_workers="$1" && > shift && I was wondering a bit why this is a problem in t0610, but not over here. As far as I understand it these statements are fine in practice because the expanded values cannot be split, right? So if "$1" expanded to something with spaces in between things would start to break. In any case, changing all of these to be quoted feels like the right thing to do regardless of whether or not it happens to work with the current values of "$1". Otherwise it's simply a confusing failure waiting to happen. Patrick > local trace_file=trace-test-checkout-workers && > diff --git a/t/t2400-worktree-add.sh b/t/t2400-worktree-add.sh > index 051363acbb..5851e07290 100755 > --- a/t/t2400-worktree-add.sh > +++ b/t/t2400-worktree-add.sh > @@ -404,7 +404,7 @@ test_expect_success '"add" worktree with orphan branch, lock, and reason' ' > # Note: Quoted arguments containing spaces are not supported. > test_wt_add_orphan_hint () { > local context="$1" && > - local use_branch=$2 && > + local use_branch="$2" && > shift 2 && > local opts="$*" && > test_expect_success "'worktree add' show orphan hint in bad/orphan HEAD w/ $context" ' > diff --git a/t/t4210-log-i18n.sh b/t/t4210-log-i18n.sh > index d2dfcf164e..75216f19ce 100755 > --- a/t/t4210-log-i18n.sh > +++ b/t/t4210-log-i18n.sh > @@ -64,7 +64,7 @@ test_expect_success 'log --grep does not find non-reencoded values (latin1)' ' > ' > > triggers_undefined_behaviour () { > - local engine=$1 > + local engine="$1" > > case $engine in > fixed) > @@ -85,7 +85,7 @@ triggers_undefined_behaviour () { > } > > mismatched_git_log () { > - local pattern=$1 > + local pattern="$1" > > LC_ALL=$is_IS_locale git log --encoding=ISO-8859-1 --format=%s \ > --grep=$pattern > diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh > index 2f8868caa1..3204afbafb 100644 > --- a/t/test-lib-functions.sh > +++ b/t/test-lib-functions.sh > @@ -1689,7 +1689,7 @@ test_parse_ls_tree_oids () { > # Choose a port number based on the test script's number and store it in > # the given variable name, unless that variable already contains a number. > test_set_port () { > - local var=$1 port > + local var="$1" port > > if test $# -ne 1 || test -z "$var" > then > -- > 2.44.0-501-g19981daefd > >
Patrick Steinhardt <ps@pks.im> writes: > On Fri, Apr 05, 2024 at 05:08:59PM -0700, Junio C Hamano wrote: >> Future-proof test scripts that do >> >> local VAR=VAL >> >> without quoting VAL (which is OK in POSIX but broken in some shells) >> that is a positional parameter, e.g. $4. >> >> Signed-off-by: Junio C Hamano <gitster@pobox.com> >> --- >> t/lib-parallel-checkout.sh | 2 +- >> t/t2400-worktree-add.sh | 2 +- >> t/t4210-log-i18n.sh | 4 ++-- >> t/test-lib-functions.sh | 2 +- >> 4 files changed, 5 insertions(+), 5 deletions(-) >> >> diff --git a/t/lib-parallel-checkout.sh b/t/lib-parallel-checkout.sh >> index acaee9cbb6..8324d6c96d 100644 >> --- a/t/lib-parallel-checkout.sh >> +++ b/t/lib-parallel-checkout.sh >> @@ -20,7 +20,7 @@ test_checkout_workers () { >> BUG "too few arguments to test_checkout_workers" >> fi && >> >> - local expected_workers=$1 && >> + local expected_workers="$1" && >> shift && > > I was wondering a bit why this is a problem in t0610, but not over here. > As far as I understand it these statements are fine in practice because > the expanded values cannot be split, right? So if "$1" expanded to > something with spaces in between things would start to break. Correct. > In any case, changing all of these to be quoted feels like the right > thing to do regardless of whether or not it happens to work with the > current values of "$1". Otherwise it's simply a confusing failure > waiting to happen. Again, agreed. That is where my "Future-proof" comes from. The true objective of this change is so that the last patch does not have to learn too much exceptions ;-) As long as expected_workers is expected to be a number (an unbroken sequence of digits), even if we add more callers to this helper in the future, $1 we see here is expected to be $IFS safe. So in that sense my "future-proof" is a white lie.
diff --git a/t/lib-parallel-checkout.sh b/t/lib-parallel-checkout.sh index acaee9cbb6..8324d6c96d 100644 --- a/t/lib-parallel-checkout.sh +++ b/t/lib-parallel-checkout.sh @@ -20,7 +20,7 @@ test_checkout_workers () { BUG "too few arguments to test_checkout_workers" fi && - local expected_workers=$1 && + local expected_workers="$1" && shift && local trace_file=trace-test-checkout-workers && diff --git a/t/t2400-worktree-add.sh b/t/t2400-worktree-add.sh index 051363acbb..5851e07290 100755 --- a/t/t2400-worktree-add.sh +++ b/t/t2400-worktree-add.sh @@ -404,7 +404,7 @@ test_expect_success '"add" worktree with orphan branch, lock, and reason' ' # Note: Quoted arguments containing spaces are not supported. test_wt_add_orphan_hint () { local context="$1" && - local use_branch=$2 && + local use_branch="$2" && shift 2 && local opts="$*" && test_expect_success "'worktree add' show orphan hint in bad/orphan HEAD w/ $context" ' diff --git a/t/t4210-log-i18n.sh b/t/t4210-log-i18n.sh index d2dfcf164e..75216f19ce 100755 --- a/t/t4210-log-i18n.sh +++ b/t/t4210-log-i18n.sh @@ -64,7 +64,7 @@ test_expect_success 'log --grep does not find non-reencoded values (latin1)' ' ' triggers_undefined_behaviour () { - local engine=$1 + local engine="$1" case $engine in fixed) @@ -85,7 +85,7 @@ triggers_undefined_behaviour () { } mismatched_git_log () { - local pattern=$1 + local pattern="$1" LC_ALL=$is_IS_locale git log --encoding=ISO-8859-1 --format=%s \ --grep=$pattern diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh index 2f8868caa1..3204afbafb 100644 --- a/t/test-lib-functions.sh +++ b/t/test-lib-functions.sh @@ -1689,7 +1689,7 @@ test_parse_ls_tree_oids () { # Choose a port number based on the test script's number and store it in # the given variable name, unless that variable already contains a number. test_set_port () { - local var=$1 port + local var="$1" port if test $# -ne 1 || test -z "$var" then
Future-proof test scripts that do local VAR=VAL without quoting VAL (which is OK in POSIX but broken in some shells) that is a positional parameter, e.g. $4. Signed-off-by: Junio C Hamano <gitster@pobox.com> --- t/lib-parallel-checkout.sh | 2 +- t/t2400-worktree-add.sh | 2 +- t/t4210-log-i18n.sh | 4 ++-- t/test-lib-functions.sh | 2 +- 4 files changed, 5 insertions(+), 5 deletions(-)