| Message ID | c2c2747ff57f68ccad8b509af037e1fc4a524fa1.1712235356.git.ps@pks.im (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | t: exercise Git/JGit reftable compatibility | expand |
On Fri, Apr 5, 2024 at 3:51 AM Patrick Steinhardt <ps@pks.im> wrote: > In `test_expect_perms()` we assign the output of a command to a variable > declared via `local`. To assert that the command is actually successful > we also chain it with `&&`. This construct is seemingly not portable and > may fail with "local: 1: bad variable name". > > Split up the variable declaration and assignment to fix this. Under what configuration, circumstances, conditions did you encounter this problem? I ask because this isn't the only such case in the test suite, as shown by: git grep -nP 'local ..*=\$\(..*&&' -- t What makes this case distinct from the others? Including such information would improve the commit message and help future readers. > Signed-off-by: Patrick Steinhardt <ps@pks.im> > --- > diff --git a/t/t0610-reftable-basics.sh b/t/t0610-reftable-basics.sh > @@ -80,8 +80,9 @@ test_expect_success 'init: reinitializing reftable with files backend fails' ' > test_expect_perms () { > local perms="$1" > local file="$2" > - local actual=$(ls -l "$file") && > + local actual > > + actual=$(ls -l "$file") &&
On Fri, Apr 05, 2024 at 05:14:34AM -0400, Eric Sunshine wrote: > On Fri, Apr 5, 2024 at 3:51 AM Patrick Steinhardt <ps@pks.im> wrote: > > In `test_expect_perms()` we assign the output of a command to a variable > > declared via `local`. To assert that the command is actually successful > > we also chain it with `&&`. This construct is seemingly not portable and > > may fail with "local: 1: bad variable name". > > > > Split up the variable declaration and assignment to fix this. > > Under what configuration, circumstances, conditions did you encounter > this problem? I ask because this isn't the only such case in the test > suite, as shown by: > > git grep -nP 'local ..*=\$\(..*&&' -- t > > What makes this case distinct from the others? Including such > information would improve the commit message and help future readers. I only ever saw this in some subset of GitHub CI jobs. See e.g. [1], where the logs [2] show the following error: ``` ok 6 - init: reinitializing reftable with files backend fails + test_when_finished rm -rf repo + test 0 = 0 + test_cleanup={ rm -rf repo } && (exit "$eval_ret"); eval_ret=$?; : + umask 002 + git init --shared=true repo Initialized empty shared Git repository in /home/runner/work/git/git/t/trash directory.t0610-reftable-basics/repo/.git/ + git -C repo config core.sharedrepository + test 1 = 1 + test_expect_perms -rw-rw-r-- repo/.git/reftable/tables.list + local perms=-rw-rw-r-- + local file=repo/.git/reftable/tables.list + ls -l repo/.git/reftable/tables.list + local actual=-rw-rw-r-- 1 runner docker 43 Apr 4 11:55 repo/.git/reftable/tables.list t0610-reftable-basics.sh: 83: local: 1: bad variable name + die + code=2 + test_atexit_handler + test : != : + return 0 + test -n + echo FATAL: Unexpected exit with code 2 FATAL: Unexpected exit with code 2 ``` I was a bit surprised by this, too, and cannot reproduce it with any of my systems. But I would bet this is dependent on the actual version of your shell because it only happened with the Ubuntu 20.04 and 16.04 jobs. I didn't dig much deeper though as the fix is easy enough, even though it is surprising. [1]: https://github.com/git/git/actions/runs/8554157462/job/23438877643 [2]: https://github.com/git/git/actions/runs/8554157462/artifacts/1384620962 Patrick > > Signed-off-by: Patrick Steinhardt <ps@pks.im> > > --- > > diff --git a/t/t0610-reftable-basics.sh b/t/t0610-reftable-basics.sh > > @@ -80,8 +80,9 @@ test_expect_success 'init: reinitializing reftable with files backend fails' ' > > test_expect_perms () { > > local perms="$1" > > local file="$2" > > - local actual=$(ls -l "$file") && > > + local actual > > > > + actual=$(ls -l "$file") &&
Patrick Steinhardt <ps@pks.im> writes: > test_expect_perms () { > local perms="$1" > local file="$2" > - local actual=$(ls -l "$file") && > + local actual > > + actual=$(ls -l "$file") && Isn't this the same as what ebee5580 (parallel-checkout: avoid dash local bug in tests, 2021-06-06) fixed? The rule for variable assignment is mishandled when local is involved by some shells. perms=$1 file=$2 actual=$(ls -l "$file") would be perfectly fine, and should be fine with "local" prefixed on these lines, but the last one with local without "" qround $(...) incorrectly makes the substitution subject to field splitting. I think the right fix should look rather like this, instead. t/t0610-reftable-basics.sh | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git i/t/t0610-reftable-basics.sh w/t/t0610-reftable-basics.sh index 686781192e..894896933e 100755 --- i/t/t0610-reftable-basics.sh +++ w/t/t0610-reftable-basics.sh @@ -81,9 +81,9 @@ test_expect_success 'init: reinitializing reftable with files backend fails' ' ' test_expect_perms () { - local perms="$1" - local file="$2" - local actual=$(ls -l "$file") && + local perms="$1" && + local file="$2" && + local actual="$(ls -l "$file")" && case "$actual" in $perms*)
On Fri, Apr 05, 2024 at 09:02:47AM -0700, Junio C Hamano wrote: > Patrick Steinhardt <ps@pks.im> writes: > > > test_expect_perms () { > > local perms="$1" > > local file="$2" > > - local actual=$(ls -l "$file") && > > + local actual > > > > + actual=$(ls -l "$file") && > > Isn't this the same as what ebee5580 (parallel-checkout: avoid dash > local bug in tests, 2021-06-06) fixed? > > The rule for variable assignment is mishandled when local is > involved by some shells. > > perms=$1 > file=$2 > actual=$(ls -l "$file") > > would be perfectly fine, and should be fine with "local" prefixed on > these lines, but the last one with local without "" qround $(...) > incorrectly makes the substitution subject to field splitting. > > I think the right fix should look rather like this, instead. Ah, interesting, thanks for the pointer! I'll send v2 of this series early next week. Patrick > t/t0610-reftable-basics.sh | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git i/t/t0610-reftable-basics.sh w/t/t0610-reftable-basics.sh > index 686781192e..894896933e 100755 > --- i/t/t0610-reftable-basics.sh > +++ w/t/t0610-reftable-basics.sh > @@ -81,9 +81,9 @@ test_expect_success 'init: reinitializing reftable with files backend fails' ' > ' > > test_expect_perms () { > - local perms="$1" > - local file="$2" > - local actual=$(ls -l "$file") && > + local perms="$1" && > + local file="$2" && > + local actual="$(ls -l "$file")" && > > case "$actual" in > $perms*) >
diff --git a/t/t0610-reftable-basics.sh b/t/t0610-reftable-basics.sh index aa9282007c..3b1aa99e7c 100755 --- a/t/t0610-reftable-basics.sh +++ b/t/t0610-reftable-basics.sh @@ -80,8 +80,9 @@ test_expect_success 'init: reinitializing reftable with files backend fails' ' test_expect_perms () { local perms="$1" local file="$2" - local actual=$(ls -l "$file") && + local actual + actual=$(ls -l "$file") && case "$actual" in $perms*) : happy
In `test_expect_perms()` we assign the output of a command to a variable declared via `local`. To assert that the command is actually successful we also chain it with `&&`. This construct is seemingly not portable and may fail with "local: 1: bad variable name". Split up the variable declaration and assignment to fix this. Signed-off-by: Patrick Steinhardt <ps@pks.im> --- t/t0610-reftable-basics.sh | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)