threads / patch / 55845

patchparallel-checkout: use grep -c to count workers in tests

Subject: [PATCH] parallel-checkout: use grep -c to count workers in tests

## tl;dr

14 messages between Jun 5, 2021 and Jun 6, 2021. Diffs are folded; open one to read it.

replies: 13people: 5as markdown or json

René Scharfe· Jun 5, 2021, 12:27 UTC · lore

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(-)
Show changes to t/lib-parallel-checkout.sh +1 −2
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
--
2.31.1
Matheus Tavares Bernardino· Jun 5, 2021, 14:31 UTC · re: René Scharfe · lore

Re: [PATCH] parallel-checkout: use grep -c to count workers in tests

On Sat, Jun 5, 2021 at 9:27 AM René Scharfe <l.s.r@web.de> wrote:
Show 5 quoted lines
>
> 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

Show 39 quoted lines
> 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.

Show 5 quoted lines
>         test $workers -eq $expected_workers &&
>         rm "$trace_file"
>  } 8>&2 2>&4
> --
> 2.31.1
René Scharfe· Jun 5, 2021, 15:20 UTC · re: Matheus Tavares Bernardino · lore

Re: [PATCH] parallel-checkout: use grep -c to count workers in tests

Am 05.06.21 um 16:31 schrieb Matheus Tavares Bernardino:
Show 31 quoted lines
> 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
Show 17 quoted lines
>>  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
Show 6 quoted lines
>
>>         test $workers -eq $expected_workers &&
>>         rm "$trace_file"
>>  } 8>&2 2>&4
>> --
>> 2.31.1
René Scharfe· Jun 5, 2021, 15:30 UTC · re: René Scharfe · lore

Re: [PATCH] parallel-checkout: use grep -c to count workers in tests

Am 05.06.21 um 17:20 schrieb René Scharfe:
Show 12 quoted lines
> 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é
René Scharfe· Jun 5, 2021, 18:11 UTC · re: René Scharfe · lore

[PATCH v2] parallel-checkout: avoid dash local bug in tests

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 <matheus.bernardino@usp.br>
Signed-off-by: René Scharfe <l.s.r@web.de>
---
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(-)
Show changes to t/lib-parallel-checkout.sh +1 −3
diff --git a/t/lib-parallel-checkout.sh b/t/lib-parallel-checkout.sh
index 21f5759732..66350d5207 100644
--- a/t/lib-parallel-checkout.sh
+++ b/t/lib-parallel-checkout.sh
@@ -27,8 +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) &&
-	test $workers -eq $expected_workers &&
+	test $(grep -c "child_start\[..*\] git checkout--worker" "$trace_file") -eq $expected_workers &&
 	rm "$trace_file"
 } 8>&2 2>&4

--
2.31.1
SZEDER Gábor· Jun 5, 2021, 19:09 UTC · re: René Scharfe · lore

Re: [PATCH v2] parallel-checkout: avoid dash local bug in tests

On Sat, Jun 05, 2021 at 08:11:24PM +0200, René Scharfe wrote:
Show 10 quoted lines
> 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'.

Show 30 quoted lines
> Helped-by: Matheus Tavares Bernardino <matheus.bernardino@usp.br>
> Signed-off-by: René Scharfe <l.s.r@web.de>
> ---
> 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(-)
> 
> diff --git a/t/lib-parallel-checkout.sh b/t/lib-parallel-checkout.sh
> index 21f5759732..66350d5207 100644
> --- a/t/lib-parallel-checkout.sh
> +++ b/t/lib-parallel-checkout.sh
> @@ -27,8 +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) &&
> -	test $workers -eq $expected_workers &&
> +	test $(grep -c "child_start\[..*\] git checkout--worker" "$trace_file") -eq $expected_workers &&
>  	rm "$trace_file"
>  } 8>&2 2>&4
> 
> --
> 2.31.1
>
René Scharfe· Jun 6, 2021, 01:01 UTC · re: SZEDER Gábor · lore

Re: [PATCH v2] parallel-checkout: avoid dash local bug in tests

Am 05.06.21 um 21:09 schrieb SZEDER Gábor:
Show 17 quoted lines
> 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.
OK.
Show 23 quoted lines
>
> 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'.

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.

Show 31 quoted lines
>
>> Helped-by: Matheus Tavares Bernardino <matheus.bernardino@usp.br>
>> Signed-off-by: René Scharfe <l.s.r@web.de>
>> ---
>> 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(-)
>>
>> diff --git a/t/lib-parallel-checkout.sh b/t/lib-parallel-checkout.sh
>> index 21f5759732..66350d5207 100644
>> --- a/t/lib-parallel-checkout.sh
>> +++ b/t/lib-parallel-checkout.sh
>> @@ -27,8 +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) &&
>> -	test $workers -eq $expected_workers &&
>> +	test $(grep -c "child_start\[..*\] git checkout--worker" "$trace_file") -eq $expected_workers &&
>>  	rm "$trace_file"
>>  } 8>&2 2>&4
>>
>> --
>> 2.31.1
>>
Ævar Arnfjörð Bjarmason· Jun 5, 2021, 19:56 UTC · re: René Scharfe · lore

Re: [PATCH v2] parallel-checkout: avoid dash local bug in tests

On Sat, Jun 05 2021, René Scharfe wrote:
Show 42 quoted lines
> 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 <matheus.bernardino@usp.br>
> Signed-off-by: René Scharfe <l.s.r@web.de>
> ---
> 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(-)
>
> diff --git a/t/lib-parallel-checkout.sh b/t/lib-parallel-checkout.sh
> index 21f5759732..66350d5207 100644
> --- a/t/lib-parallel-checkout.sh
> +++ b/t/lib-parallel-checkout.sh
> @@ -27,8 +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) &&
> -	test $workers -eq $expected_workers &&
> +	test $(grep -c "child_start\[..*\] git checkout--worker" "$trace_file") -eq $expected_workers &&
>  	rm "$trace_file"
>  } 8>&2 2>&4
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
Matheus Tavares Bernardino· Jun 5, 2021, 22:17 UTC · re: Ævar Arnfjörð Bjarmason · lore

Re: [PATCH v2] parallel-checkout: avoid dash local bug in tests

On Sat, Jun 5, 2021 at 5:03 PM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:
Show 50 quoted lines
>
>
> On Sat, Jun 05 2021, 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).
> >
> > 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 <matheus.bernardino@usp.br>
> > Signed-off-by: René Scharfe <l.s.r@web.de>
> > ---
> > 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(-)
> >
> > diff --git a/t/lib-parallel-checkout.sh b/t/lib-parallel-checkout.sh
> > index 21f5759732..66350d5207 100644
> > --- a/t/lib-parallel-checkout.sh
> > +++ b/t/lib-parallel-checkout.sh
> > @@ -27,8 +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) &&
> > -     test $workers -eq $expected_workers &&
> > +     test $(grep -c "child_start\[..*\] git checkout--worker" "$trace_file") -eq $expected_workers &&
> >       rm "$trace_file"
> >  } 8>&2 2>&4
>
> 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.

Matheus Tavares Bernardino· Jun 5, 2021, 22:21 UTC · re: Matheus Tavares Bernardino · lore

Re: [PATCH v2] parallel-checkout: avoid dash local bug in tests

On Sat, Jun 5, 2021 at 7:17 PM Matheus Tavares Bernardino <matheus.bernardino@usp.br> wrote:

Show 10 quoted lines
>
> 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.

René Scharfe· Jun 6, 2021, 01:01 UTC · re: Ævar Arnfjörð Bjarmason · lore

Re: [PATCH v2] parallel-checkout: avoid dash local bug in tests

Am 05.06.21 um 21:56 schrieb Ævar Arnfjörð Bjarmason:
Show 49 quoted lines
>
> On Sat, Jun 05 2021, 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).
>>
>> 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 <matheus.bernardino@usp.br>
>> Signed-off-by: René Scharfe <l.s.r@web.de>
>> ---
>> 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(-)
>>
>> diff --git a/t/lib-parallel-checkout.sh b/t/lib-parallel-checkout.sh
>> index 21f5759732..66350d5207 100644
>> --- a/t/lib-parallel-checkout.sh
>> +++ b/t/lib-parallel-checkout.sh
>> @@ -27,8 +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) &&
>> -	test $workers -eq $expected_workers &&
>> +	test $(grep -c "child_start\[..*\] git checkout--worker" "$trace_file") -eq $expected_workers &&
>>  	rm "$trace_file"
>>  } 8>&2 2>&4
>
> 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.

Show 11 quoted lines
> 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.

Show 9 quoted lines
> 	 	$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.

True, I was still gripping the "use grep -c instead of grep | wc -l" hammer. I better rewatch https://www.youtube.com/watch?v=Yv4tI6939q0

Show 7 quoted lines
>
> 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
>
Junio C Hamano· Jun 6, 2021, 01:28 UTC · re: René Scharfe · lore

Re: [PATCH v2] parallel-checkout: avoid dash local bug in tests

René Scharfe <l.s.r@web.de> writes:
Show 17 quoted lines
> Am 05.06.21 um 21:56 schrieb Ævar Arnfjörð Bjarmason:
>>
>> On Sat, Jun 05 2021, René Scharfe wrote:
>>
>>> -	local workers=$(grep "child_start\[..*\] git checkout--worker" "$trace_file" | wc -l) &&
>>> -	test $workers -eq $expected_workers &&
>>> +	test $(grep -c "child_start\[..*\] git checkout--worker" "$trace_file") -eq $expected_workers &&
>>>  	rm "$trace_file"
>>>  } 8>&2 2>&4
>>
>> 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.

Sorry for being late to the party but I tend to disagree. "grep -c" is used elsewhere in the tests, we know it is safe.

IOW, the above change is quite straight-forward. It is much more obvious and close to "duct-tape-style" fix, than some altermatives like enclosing the whole command substitution in a pair of double-quotes and rely on $workers always getting used without double-quotes around it. To me, that looks more subtle and brittle.

Show 17 quoted lines
>> 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.

I actually do not mind "no assignments with local decl" that much. I agree that is outside the scope of this particular fix.

René Scharfe· Jun 6, 2021, 01:01 UTC · re: René Scharfe · lore

[PATCH v3] parallel-checkout: avoid dash local bug in tests

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 <matheus.bernardino@usp.br>
Helped-by: SZEDER Gábor <szeder.dev@gmail.com>
Helped-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Signed-off-by: René Scharfe <l.s.r@web.de>
---
Changes since v2:
- Use minimal fix.
- New commit message.
 t/lib-parallel-checkout.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
Show changes to t/lib-parallel-checkout.sh +1 −2
diff --git a/t/lib-parallel-checkout.sh b/t/lib-parallel-checkout.sh
index 21f5759732..83b279a846 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 "child_start\[..*\] git checkout--worker" "$trace_file" | wc -l)" &&
 	test $workers -eq $expected_workers &&
 	rm "$trace_file"
 } 8>&2 2>&4
--
2.31.1
Junio C Hamano· Jun 6, 2021, 01:41 UTC · re: René Scharfe · lore

Re: [PATCH v3] parallel-checkout: avoid dash local bug in tests

René Scharfe <l.s.r@web.de> writes:
Show 22 quoted lines
> 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 <matheus.bernardino@usp.br>
> Helped-by: SZEDER Gábor <szeder.dev@gmail.com>
> Helped-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> Signed-off-by: René Scharfe <l.s.r@web.de>
> ---
> Changes since v2:
> - Use minimal fix.
> - New commit message.

Thanks. As I said, I do not necessarily think this is conceptually "minimal", but we use workers only once and without surrounding dq so it is easy to see that this also is correct.

Will queue.
Show 19 quoted lines
>
>  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..83b279a846 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 "child_start\[..*\] git checkout--worker" "$trace_file" | wc -l)" &&
>  	test $workers -eq $expected_workers &&
>  	rm "$trace_file"
>  } 8>&2 2>&4
> --
> 2.31.1

← back to recent threads