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

From: René Scharfe <l.s.r@web.de>
Date: Jun 5, 2021, 15:20 UTC
Message-ID: <a946daaa-6aa8-8527-5b55-ccf86de2a9d7@web.de>
In-Reply-To: <CAHd-oW7f0rfRaTyvJJMt7uNiLS4F9c=D3oKYc5v+unzxPPxjCw@mail.gmail.com>

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