| Message ID | eb4bcd4c-e6d2-cbeb-8951-cf22b9d3d5fe@web.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | parallel-checkout: use grep -c to count workers in tests | expand |
On Sat, Jun 5, 2021 at 9:27 AM René Scharfe <l.s.r@web.de> wrote: > > The parallel checkout tests fail when run with /bin/dash on MacOS 11.4, > reporting the following error: > > ./t2080-parallel-checkout-basics.sh: 33: local: 0: bad variable name That's interesting. It seems that dash is trying to use wc's output (in this case "0") as a local variable name which, of course, is not valid. This reply [1] to this bug report [2] mentions that dash expands a local assignment like the following: x="1 2 3" local y=$x ---expands-to---> local y=1 2 3 So, in this case, dash thinks we are trying to declare three local variables, y, 2, and 3, which is an error. In bash, the above commands would result in $y getting the value "1 2 3", even though we didn't quote $x in the assignment. (BTW, the reply mentions that quoting the right side of the assignment should make this work in dash as well.) I wonder if that's what's happening here. Maybe "wc -l" is outputting a space before the number, and that makes dash parse this line as something like `local workers="" 0="" `? If that's really the case (I can't confirm because the bug seems to have been fixed in the dash version I have), maybe we could mention that in the commit message. [1]: https://bugs.launchpad.net/ubuntu/+source/dash/+bug/139097/comments/6 [2]: https://bugs.launchpad.net/ubuntu/+source/dash/+bug/139097 > This seems to be caused by a bug in dash, which doesn't like the pipe > into wc for some reason. We can work around it and make the test > slightly shorter and faster by having grep do the counting, though, so > let's do that. > > Signed-off-by: René Scharfe <l.s.r@web.de> > --- > Reduced test case for underlying dash issue: > > $ dash -c 'foo () { local bar=$(echo baz | wc); }; foo' > dash: 1: local: 1: bad variable name > > > The pipe is not even required to trigger the issue: > > $ dash -c 'foo () { local bar=$(wc /dev/null); }; foo' > dash: 1: local: 0: bad variable name > > Turning wc into a function calms the shell: > > $ dash -c 'foo () { local bar=$(echo baz | wc); }; wc () { :; }; foo' > > Setting a global variable also works fine: > > $ dash -c 'foo () { bar=$(echo baz | wc); }; foo' > > t/lib-parallel-checkout.sh | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/t/lib-parallel-checkout.sh b/t/lib-parallel-checkout.sh > index 21f5759732..145276eb4c 100644 > --- a/t/lib-parallel-checkout.sh > +++ b/t/lib-parallel-checkout.sh > @@ -27,7 +27,7 @@ test_checkout_workers () { > rm -f "$trace_file" && > GIT_TRACE2="$(pwd)/$trace_file" "$@" 2>&8 && > > - local workers=$(grep "child_start\[..*\] git checkout--worker" "$trace_file" | wc -l) && > + local workers=$(grep -c "child_start\[..*\] git checkout--worker" <"$trace_file") && Nice, and the result is both cleaner and more efficient :) Just one minor nit: I think we could drop the redirection as grep can take the file name as an argument. > test $workers -eq $expected_workers && > rm "$trace_file" > } 8>&2 2>&4 > -- > 2.31.1
Am 05.06.21 um 16:31 schrieb Matheus Tavares Bernardino: > On Sat, Jun 5, 2021 at 9:27 AM René Scharfe <l.s.r@web.de> wrote: >> >> The parallel checkout tests fail when run with /bin/dash on MacOS 11.4, >> reporting the following error: >> >> ./t2080-parallel-checkout-basics.sh: 33: local: 0: bad variable name > > That's interesting. It seems that dash is trying to use wc's output > (in this case "0") as a local variable name which, of course, is not > valid. > > This reply [1] to this bug report [2] mentions that dash expands a > local assignment like the following: > > x="1 2 3" > local y=$x ---expands-to---> local y=1 2 3 > > So, in this case, dash thinks we are trying to declare three local > variables, y, 2, and 3, which is an error. In bash, the above commands > would result in $y getting the value "1 2 3", even though we didn't > quote $x in the assignment. (BTW, the reply mentions that quoting the > right side of the assignment should make this work in dash as well.) > > I wonder if that's what's happening here. Maybe "wc -l" is outputting > a space before the number, and that makes dash parse this line as > something like `local workers="" 0="" `? If that's really the case (I > can't confirm because the bug seems to have been fixed in the dash > version I have), maybe we could mention that in the commit message. > > [1]: https://bugs.launchpad.net/ubuntu/+source/dash/+bug/139097/comments/6 > [2]: https://bugs.launchpad.net/ubuntu/+source/dash/+bug/139097 Ah, indeed: $ dash -c 'foo () { local bar=$(echo "1"); }; foo' $ dash -c 'foo () { local bar=$(echo " 1"); }; foo' dash: 1: local: 1: bad variable name $ wc -l </dev/null | tr ' ' s sssssss0 >> t/lib-parallel-checkout.sh | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/t/lib-parallel-checkout.sh b/t/lib-parallel-checkout.sh >> index 21f5759732..145276eb4c 100644 >> --- a/t/lib-parallel-checkout.sh >> +++ b/t/lib-parallel-checkout.sh >> @@ -27,7 +27,7 @@ test_checkout_workers () { >> rm -f "$trace_file" && >> GIT_TRACE2="$(pwd)/$trace_file" "$@" 2>&8 && >> >> - local workers=$(grep "child_start\[..*\] git checkout--worker" "$trace_file" | wc -l) && >> + local workers=$(grep -c "child_start\[..*\] git checkout--worker" <"$trace_file") && > > Nice, and the result is both cleaner and more efficient :) Just one > minor nit: I think we could drop the redirection as grep can take the > file name as an argument. I'm not sure if there's a grep out there that prints the filename before the count even if it deals with a single file. git grep does that, at least. POSIX[3] implies the lack of filename prefix for the single file case, but I don't know if we can rely on that everywhere. [3] https://pubs.opengroup.org/onlinepubs/9699919799/utilities/grep.html > >> test $workers -eq $expected_workers && >> rm "$trace_file" >> } 8>&2 2>&4 >> -- >> 2.31.1
Am 05.06.21 um 17:20 schrieb René Scharfe: > Am 05.06.21 um 16:31 schrieb Matheus Tavares Bernardino: >> On Sat, Jun 5, 2021 at 9:27 AM René Scharfe <l.s.r@web.de> wrote: >> Just one >> minor nit: I think we could drop the redirection as grep can take the >> file name as an argument. > > I'm not sure if there's a grep out there that prints the filename before > the count even if it deals with a single file. git grep does that, at > least. POSIX[3] implies the lack of filename prefix for the single file > case, but I don't know if we can rely on that everywhere. > > [3] https://pubs.opengroup.org/onlinepubs/9699919799/utilities/grep.html Except we got plenty of "grep -c" calls with a single filename and no redirection in t/ already. Adding one more should be fine. René
diff --git a/t/lib-parallel-checkout.sh b/t/lib-parallel-checkout.sh index 21f5759732..145276eb4c 100644 --- a/t/lib-parallel-checkout.sh +++ b/t/lib-parallel-checkout.sh @@ -27,7 +27,7 @@ test_checkout_workers () { rm -f "$trace_file" && GIT_TRACE2="$(pwd)/$trace_file" "$@" 2>&8 && - local workers=$(grep "child_start\[..*\] git checkout--worker" "$trace_file" | wc -l) && + local workers=$(grep -c "child_start\[..*\] git checkout--worker" <"$trace_file") && test $workers -eq $expected_workers && rm "$trace_file" } 8>&2 2>&4
The parallel checkout tests fail when run with /bin/dash on MacOS 11.4, reporting the following error: ./t2080-parallel-checkout-basics.sh: 33: local: 0: bad variable name This seems to be caused by a bug in dash, which doesn't like the pipe into wc for some reason. We can work around it and make the test slightly shorter and faster by having grep do the counting, though, so let's do that. Signed-off-by: René Scharfe <l.s.r@web.de> --- Reduced test case for underlying dash issue: $ dash -c 'foo () { local bar=$(echo baz | wc); }; foo' dash: 1: local: 1: bad variable name The pipe is not even required to trigger the issue: $ dash -c 'foo () { local bar=$(wc /dev/null); }; foo' dash: 1: local: 0: bad variable name Turning wc into a function calms the shell: $ dash -c 'foo () { local bar=$(echo baz | wc); }; wc () { :; }; foo' Setting a global variable also works fine: $ dash -c 'foo () { bar=$(echo baz | wc); }; foo' t/lib-parallel-checkout.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) -- 2.31.1