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

From: Matheus Tavares Bernardino <matheus.bernardino@usp.br>
Date: Jun 5, 2021, 14:31 UTC
Message-ID: <CAHd-oW7f0rfRaTyvJJMt7uNiLS4F9c=D3oKYc5v+unzxPPxjCw@mail.gmail.com>
In-Reply-To: <eb4bcd4c-e6d2-cbeb-8951-cf22b9d3d5fe@web.de>

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