From: René Scharfe <hidden> Date: 2021-06-05 12:27:54
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 <redacted>
---
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(-)
On Sat, Jun 5, 2021 at 9:27 AM René Scharfe [off-list ref] 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
quoted hunk
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 <redacted>
---
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(-)
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.
From: René Scharfe <hidden> Date: 2021-06-05 15:21:06
Am 05.06.21 um 16:31 schrieb Matheus Tavares Bernardino:
On Sat, Jun 5, 2021 at 9:27 AM René Scharfe [off-list ref] wrote:
quoted
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
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
From: René Scharfe <hidden> Date: 2021-06-05 15:30:15
Am 05.06.21 um 17:20 schrieb René Scharfe:
Am 05.06.21 um 16:31 schrieb Matheus Tavares Bernardino:
quoted
On Sat, Jun 5, 2021 at 9:27 AM René Scharfe [off-list ref] 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é
From: René Scharfe <hidden> Date: 2021-06-05 18:11:34
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 because wc's output contains leading spaces and this version of
dash erroneously expands the variable declaration as "local workers= 0",
i.e. it tries to set the "workers" variable to the empty string and also
declare a variable named "0", which not a valid name. This is a known
dash bug (https://bugs.launchpad.net/ubuntu/+source/dash/+bug/139097).
Work around it by passing the command output directly to test instead of
storing it in a variable first. While at it, let grep count the number
of lines instead of piping its output to wc, which is a bit shorter and
more efficient.
Helped-by: Matheus Tavares Bernardino [off-list ref]
Signed-off-by: René Scharfe <redacted>
---
Changes since v1:
- Explain the root cause.
- Get rid of the local variable "workers".
- Adjust title accordingly.
- Still use grep -c, though.
- Remove input redirection.
t/lib-parallel-checkout.sh | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
From: SZEDER Gábor <hidden> Date: 2021-06-05 19:10:53
On Sat, Jun 05, 2021 at 08:11:24PM +0200, René Scharfe 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 because wc's output contains leading spaces and this version of
dash erroneously expands the variable declaration as "local workers= 0",
i.e. it tries to set the "workers" variable to the empty string and also
declare a variable named "0", which not a valid name. This is a known
dash bug (https://bugs.launchpad.net/ubuntu/+source/dash/+bug/139097).
Perhaps a more accurate wording for this bug would be:
... and even fairly recent versions of dash erroneously perform
field splitting on the expansion of the command substitution before
assigning it to a local variable.
I think the relevant part of POSIX is section 2.9.1 Simple Commands:
4. Each variable assignment shall be expanded for tilde expansion,
parameter expansion, command substitution, arithmetic expansion,
and quote removal prior to assigning the value.
https://pubs.opengroup.org/onlinepubs/9699919799/utilities/V3_chap02.html#tag_18_09_01
Note that it didn't mention field splitting; though POSIX doesn't
specifies local variables in the first place, so...
Anyway, this bug has been fixed in v0.5.11 (2020-06-01).
This is an old bug, it was already present in v0.5.5 (2009-01-13); I
didn't check earlier versions.
Work around it by passing the command output directly to test instead of
storing it in a variable first. While at it, let grep count the number
of lines instead of piping its output to wc, which is a bit shorter and
more efficient.
A more debug-friendly alternative would be to save 'grep's output to a
temporary file and use 'test_line_count = $expected_workers'.
quoted hunk
Helped-by: Matheus Tavares Bernardino [off-list ref]
Signed-off-by: René Scharfe <redacted>
---
Changes since v1:
- Explain the root cause.
- Get rid of the local variable "workers".
- Adjust title accordingly.
- Still use grep -c, though.
- Remove input redirection.
t/lib-parallel-checkout.sh | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
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 because wc's output contains leading spaces and this version of
dash erroneously expands the variable declaration as "local workers= 0",
i.e. it tries to set the "workers" variable to the empty string and also
declare a variable named "0", which not a valid name. This is a known
dash bug (https://bugs.launchpad.net/ubuntu/+source/dash/+bug/139097).
Work around it by passing the command output directly to test instead of
storing it in a variable first. While at it, let grep count the number
of lines instead of piping its output to wc, which is a bit shorter and
more efficient.
Helped-by: Matheus Tavares Bernardino [off-list ref]
Signed-off-by: René Scharfe <redacted>
---
Changes since v1:
- Explain the root cause.
- Get rid of the local variable "workers".
- Adjust title accordingly.
- Still use grep -c, though.
- Remove input redirection.
t/lib-parallel-checkout.sh | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
I'd find this thing much clearer if the v2 just narrowly focused on
avoiding the "local", and thus demonstrated the non-portable shell
issue, and perhaps with something like:
diff --git a/t/check-non-portable-shell.pl b/t/check-non-portable-shell.pl
index fd3303552be..aad6f3e2bf1 100755
--- a/t/check-non-portable-shell.pl
+++ b/t/check-non-portable-shell.pl
@@ -48,6 +48,7 @@ sub err {
/\bexport\s+[A-Za-z0-9_]*=/ and err '"export FOO=bar" is not portable (use FOO=bar && export FOO)';
/^\s*([A-Z0-9_]+=(\w*|(["']).*?\3)\s+)+(\w+)/ and exists($func{$4}) and
err '"FOO=bar shell_func" assignment extends beyond "shell_func"';
+ /\blocal\b \S+=\$\(.*?\|\s*\bwc -l\)/ and err 'whitespace handling in local=$(... | wc -l) differs in some dash versions';
$line = '';
# this resets our $. for each file
close ARGV if eof;
The let's do grep -c while we're at it part of this IMO just adds
confusion while skimming future portability issues with --grep=dash or
--grep=POSIX in the future, and looking at the history in v1 it's just
there because in v1 the root cause wasn't fully understood.
If we're doing a general cleanup of that pattern it would seem to be
better to search-replace this with the rest of them in another commit:
$ git grep '\$\(grep.*\| wc -l' -- t | wc -l
27
On Sat, Jun 5, 2021 at 5:03 PM Ævar Arnfjörð Bjarmason [off-list ref] wrote:
On Sat, Jun 05 2021, René Scharfe wrote:
quoted
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 because wc's output contains leading spaces and this version of
dash erroneously expands the variable declaration as "local workers= 0",
i.e. it tries to set the "workers" variable to the empty string and also
declare a variable named "0", which not a valid name. This is a known
dash bug (https://bugs.launchpad.net/ubuntu/+source/dash/+bug/139097).
Work around it by passing the command output directly to test instead of
storing it in a variable first. While at it, let grep count the number
of lines instead of piping its output to wc, which is a bit shorter and
more efficient.
Helped-by: Matheus Tavares Bernardino [off-list ref]
Signed-off-by: René Scharfe <redacted>
---
Changes since v1:
- Explain the root cause.
- Get rid of the local variable "workers".
- Adjust title accordingly.
- Still use grep -c, though.
- Remove input redirection.
t/lib-parallel-checkout.sh | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
I'd find this thing much clearer if the v2 just narrowly focused on
avoiding the "local", and thus demonstrated the non-portable shell
issue,
I don't have any strong preference, but if we are leaving the "grep |
wc -l" -> "grep -c" conversion to a followup patch, perhaps the
simplest change focusing on the dash issue would be to quote the
right-hand side of the "local" assignment:
- local workers=$(grep "child_start\[..*\] git checkout--worker"
"$trace_file" | wc -l) &&
+ local workers="$(grep "child_start\[..*\] git checkout--worker"
"$trace_file" | wc -l)" &&
(René, could you confirm if this works to make the test pass on dash?)
Alternatively, we could use `test_line_count` as SZEDER Gábor
suggested in a parallel reply.
On Sat, Jun 5, 2021 at 7:17 PM Matheus Tavares Bernardino
[off-list ref] wrote:
I don't have any strong preference, but if we are leaving the "grep |
wc -l" -> "grep -c" conversion to a followup patch, perhaps the
simplest change focusing on the dash issue would be to quote the
right-hand side of the "local" assignment:
- local workers=$(grep "child_start\[..*\] git checkout--worker"
"$trace_file" | wc -l) &&
+ local workers="$(grep "child_start\[..*\] git checkout--worker"
"$trace_file" | wc -l)" &&
Oops, sorry for the odd line breaking. I forgot that Gmail's web
client would break these lines.
From: René Scharfe <hidden> Date: 2021-06-06 01:01:47
Am 05.06.21 um 21:56 schrieb Ævar Arnfjörð Bjarmason:
On Sat, Jun 05 2021, René Scharfe wrote:
quoted
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 because wc's output contains leading spaces and this version of
dash erroneously expands the variable declaration as "local workers= 0",
i.e. it tries to set the "workers" variable to the empty string and also
declare a variable named "0", which not a valid name. This is a known
dash bug (https://bugs.launchpad.net/ubuntu/+source/dash/+bug/139097).
Work around it by passing the command output directly to test instead of
storing it in a variable first. While at it, let grep count the number
of lines instead of piping its output to wc, which is a bit shorter and
more efficient.
Helped-by: Matheus Tavares Bernardino [off-list ref]
Signed-off-by: René Scharfe <redacted>
---
Changes since v1:
- Explain the root cause.
- Get rid of the local variable "workers".
- Adjust title accordingly.
- Still use grep -c, though.
- Remove input redirection.
t/lib-parallel-checkout.sh | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
I'd find this thing much clearer if the v2 just narrowly focused on
avoiding the "local", and thus demonstrated the non-portable shell
issue,
I was not aiming for a minimal fix and I don't think the patch above is
too complex, but you're right that at this point in the release cycle a
duct-tape-style patch would be better.
and perhaps with something like:
diff --git a/t/check-non-portable-shell.pl b/t/check-non-portable-shell.pl
index fd3303552be..aad6f3e2bf1 100755
--- a/t/check-non-portable-shell.pl
+++ b/t/check-non-portable-shell.pl
@@ -48,6 +48,7 @@ sub err {
/\bexport\s+[A-Za-z0-9_]*=/ and err '"export FOO=bar" is not portable (use FOO=bar && export FOO)';
/^\s*([A-Z0-9_]+=(\w*|(["']).*?\3)\s+)+(\w+)/ and exists($func{$4}) and
err '"FOO=bar shell_func" assignment extends beyond "shell_func"';
+ /\blocal\b \S+=\$\(.*?\|\s*\bwc -l\)/ and err 'whitespace handling in local=$(... | wc -l) differs in some dash versions';
Any command can output whitespace, it's not limited to wc -l. So a
better rule might be to always quote command substitutions in local
variable declarations (local foo="$(...)"). Or to disallow assignments
with local altogether, like we already do for export, but that might be
a bit much.
$line = '';
# this resets our $. for each file
close ARGV if eof;
The let's do grep -c while we're at it part of this IMO just adds
confusion while skimming future portability issues with --grep=dash or
--grep=POSIX in the future, and looking at the history in v1 it's just
there because in v1 the root cause wasn't fully understood.
If we're doing a general cleanup of that pattern it would seem to be
better to search-replace this with the rest of them in another commit:
$ git grep '\$\(grep.*\| wc -l' -- t | wc -l
27
From: René Scharfe <hidden> Date: 2021-06-06 01:01:54
Am 05.06.21 um 21:09 schrieb SZEDER Gábor:
On Sat, Jun 05, 2021 at 08:11:24PM +0200, René Scharfe wrote:
quoted
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 because wc's output contains leading spaces and this version of
dash erroneously expands the variable declaration as "local workers= 0",
i.e. it tries to set the "workers" variable to the empty string and also
declare a variable named "0", which not a valid name. This is a known
dash bug (https://bugs.launchpad.net/ubuntu/+source/dash/+bug/139097).
Perhaps a more accurate wording for this bug would be:
... and even fairly recent versions of dash erroneously perform
field splitting on the expansion of the command substitution before
assigning it to a local variable.
OK.
I think the relevant part of POSIX is section 2.9.1 Simple Commands:
4. Each variable assignment shall be expanded for tilde expansion,
parameter expansion, command substitution, arithmetic expansion,
and quote removal prior to assigning the value.
https://pubs.opengroup.org/onlinepubs/9699919799/utilities/V3_chap02.html#tag_18_09_01
Note that it didn't mention field splitting; though POSIX doesn't
specifies local variables in the first place, so...
Anyway, this bug has been fixed in v0.5.11 (2020-06-01).
This is an old bug, it was already present in v0.5.5 (2009-01-13); I
didn't check earlier versions.
quoted
Work around it by passing the command output directly to test instead of
storing it in a variable first. While at it, let grep count the number
of lines instead of piping its output to wc, which is a bit shorter and
more efficient.
A more debug-friendly alternative would be to save 'grep's output to a
temporary file and use 'test_line_count = $expected_workers'.
Yes, but that would cement the use of grep and wc -l and I still can't
let go of the idea that grep -c would be slightly quicker. And it would
add file I/O.
Something like this would be more efficient in the expected case:
if test $expected_count -ne $(grep -c -e "$pattern" "$file")
then
echo "Expected $expected_count lines matching $patter, but got:"
grep -e "$pattern" "$file"
return 1
fi
I have no performance numbers to show, just the vague feeling that the
test suite takes way too long already.
Anyway, for now a minimal fix should do. Debug features can be added to
this case and several similar ones later.
quoted
Helped-by: Matheus Tavares Bernardino [off-list ref]
Signed-off-by: René Scharfe <redacted>
---
Changes since v1:
- Explain the root cause.
- Get rid of the local variable "workers".
- Adjust title accordingly.
- Still use grep -c, though.
- Remove input redirection.
t/lib-parallel-checkout.sh | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
From: René Scharfe <hidden> Date: 2021-06-06 01:02:04
Dash bug https://bugs.launchpad.net/ubuntu/+source/dash/+bug/139097
lets the shell erroneously perform field splitting on the expansion of a
command substitution during declaration of a local variable. It causes
the parallel-checkout tests to fail e.g. when running them with
/bin/dash on MacOS 11.4, where they error out like this:
./t2080-parallel-checkout-basics.sh: 33: local: 0: bad variable name
That's because the output of wc -l contains leading spaces and the
returned number of lines is treated as another variable to declare, i.e.
as in "local workers= 0".
Work around it by enclosing the command substitution in quotes.
Helped-by: Matheus Tavares Bernardino [off-list ref]
Helped-by: SZEDER Gábor [off-list ref]
Helped-by: Ævar Arnfjörð Bjarmason [off-list ref]
Signed-off-by: René Scharfe <redacted>
---
Changes since v2:
- Use minimal fix.
- New commit message.
t/lib-parallel-checkout.sh | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)