{"thread":{"id":"55845","subject":"[PATCH] parallel-checkout: use grep -c to count workers in tests","startedAt":"2021-06-05T12:27:54Z","lastAt":"2021-06-06T01:41:44Z","messageCount":14,"participants":["René Scharfe","Matheus Tavares Bernardino","SZEDER Gábor","Ævar Arnfjörð Bjarmason","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"426486","messageId":"eb4bcd4c-e6d2-cbeb-8951-cf22b9d3d5fe@web.de","threadId":"55845","inReplyTo":null,"subject":"[PATCH] parallel-checkout: use grep -c to count workers in tests","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2021-06-05T12:27:47Z","receivedAt":"2021-06-05T12:27:54Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"The parallel checkout tests fail when run with /bin/dash on MacOS 11.4,\nreporting the following error:\n\n   ./t2080-parallel-checkout-basics.sh: 33: local: 0: bad variable name\n\nThis seems to be caused by a bug in dash, which doesn't like the pipe\ninto wc for some reason.  We can work around it and make the test\nslightly shorter and faster by having grep do the counting, though, so\nlet's do that.\n\nSigned-off-by: René Scharfe <l.s.r@web.de>\n---\nReduced test case for underlying dash issue:\n\n   $ dash -c 'foo () { local bar=$(echo baz | wc); }; foo'\n   dash: 1: local: 1: bad variable name\n\n\nThe pipe is not even required to trigger the issue:\n\n   $ dash -c 'foo () { local bar=$(wc /dev/null); }; foo'\n   dash: 1: local: 0: bad variable name\n\nTurning wc into a function calms the shell:\n\n   $ dash -c 'foo () { local bar=$(echo baz | wc); }; wc () { :; }; foo'\n\nSetting a global variable also works fine:\n\n   $ dash -c 'foo () { bar=$(echo baz | wc); }; foo'\n\n t/lib-parallel-checkout.sh | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/t/lib-parallel-checkout.sh b/t/lib-parallel-checkout.sh\nindex 21f5759732..145276eb4c 100644\n--- a/t/lib-parallel-checkout.sh\n+++ b/t/lib-parallel-checkout.sh\n@@ -27,7 +27,7 @@ test_checkout_workers () {\n \trm -f \"$trace_file\" &&\n \tGIT_TRACE2=\"$(pwd)/$trace_file\" \"$@\" 2>&8 &&\n\n-\tlocal workers=$(grep \"child_start\\[..*\\] git checkout--worker\" \"$trace_file\" | wc -l) &&\n+\tlocal workers=$(grep -c \"child_start\\[..*\\] git checkout--worker\" <\"$trace_file\") &&\n \ttest $workers -eq $expected_workers &&\n \trm \"$trace_file\"\n } 8>&2 2>&4\n--\n2.31.1\n"},{"id":"426490","messageId":"CAHd-oW7f0rfRaTyvJJMt7uNiLS4F9c=D3oKYc5v+unzxPPxjCw@mail.gmail.com","threadId":"55845","inReplyTo":"eb4bcd4c-e6d2-cbeb-8951-cf22b9d3d5fe@web.de","subject":"Re: [PATCH] parallel-checkout: use grep -c to count workers in tests","fromName":"Matheus Tavares Bernardino","fromEmail":"matheus.bernardino@usp.br","sentAt":"2021-06-05T14:31:58Z","receivedAt":"2021-06-05T14:32:13Z","isPatch":true,"sender":{"key":"matheus.tavb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12701583?v=4"},"body":"On Sat, Jun 5, 2021 at 9:27 AM René Scharfe <l.s.r@web.de> wrote:\n>\n> The parallel checkout tests fail when run with /bin/dash on MacOS 11.4,\n> reporting the following error:\n>\n>    ./t2080-parallel-checkout-basics.sh: 33: local: 0: bad variable name\n\nThat's interesting. It seems that dash is trying to use wc's output\n(in this case \"0\") as a local variable name which, of course, is not\nvalid.\n\nThis reply [1] to this bug report [2] mentions that dash expands a\nlocal assignment like the following:\n\n    x=\"1 2 3\"\n    local y=$x ---expands-to---> local y=1 2 3\n\nSo, in this case, dash thinks we are trying to declare three local\nvariables, y, 2, and 3, which is an error. In bash, the above commands\nwould result in $y getting the value \"1 2 3\", even though we didn't\nquote $x in the assignment. (BTW, the reply mentions that quoting the\nright side of the assignment should make this work in dash as well.)\n\nI wonder if that's what's happening here. Maybe \"wc -l\" is outputting\na space before the number, and that makes dash parse this line as\nsomething like `local workers=\"\" 0=\"\" `? If that's really the case (I\ncan't confirm because the bug seems to have been fixed in the dash\nversion I have), maybe we could mention that in the commit message.\n\n[1]: https://bugs.launchpad.net/ubuntu/+source/dash/+bug/139097/comments/6\n[2]: https://bugs.launchpad.net/ubuntu/+source/dash/+bug/139097\n\n> This seems to be caused by a bug in dash, which doesn't like the pipe\n> into wc for some reason.  We can work around it and make the test\n> slightly shorter and faster by having grep do the counting, though, so\n> let's do that.\n>\n> Signed-off-by: René Scharfe <l.s.r@web.de>\n> ---\n> Reduced test case for underlying dash issue:\n>\n>    $ dash -c 'foo () { local bar=$(echo baz | wc); }; foo'\n>    dash: 1: local: 1: bad variable name\n>\n>\n> The pipe is not even required to trigger the issue:\n>\n>    $ dash -c 'foo () { local bar=$(wc /dev/null); }; foo'\n>    dash: 1: local: 0: bad variable name\n>\n> Turning wc into a function calms the shell:\n>\n>    $ dash -c 'foo () { local bar=$(echo baz | wc); }; wc () { :; }; foo'\n>\n> Setting a global variable also works fine:\n>\n>    $ dash -c 'foo () { bar=$(echo baz | wc); }; foo'\n>\n>  t/lib-parallel-checkout.sh | 2 +-\n>  1 file changed, 1 insertion(+), 1 deletion(-)\n>\n> diff --git a/t/lib-parallel-checkout.sh b/t/lib-parallel-checkout.sh\n> index 21f5759732..145276eb4c 100644\n> --- a/t/lib-parallel-checkout.sh\n> +++ b/t/lib-parallel-checkout.sh\n> @@ -27,7 +27,7 @@ test_checkout_workers () {\n>         rm -f \"$trace_file\" &&\n>         GIT_TRACE2=\"$(pwd)/$trace_file\" \"$@\" 2>&8 &&\n>\n> -       local workers=$(grep \"child_start\\[..*\\] git checkout--worker\" \"$trace_file\" | wc -l) &&\n> +       local workers=$(grep -c \"child_start\\[..*\\] git checkout--worker\" <\"$trace_file\") &&\n\nNice, and the result is both cleaner and more efficient :) Just one\nminor nit: I think we could drop the redirection as grep can take the\nfile name as an argument.\n\n>         test $workers -eq $expected_workers &&\n>         rm \"$trace_file\"\n>  } 8>&2 2>&4\n> --\n> 2.31.1\n"},{"id":"426493","messageId":"a946daaa-6aa8-8527-5b55-ccf86de2a9d7@web.de","threadId":"55845","inReplyTo":"CAHd-oW7f0rfRaTyvJJMt7uNiLS4F9c=D3oKYc5v+unzxPPxjCw@mail.gmail.com","subject":"Re: [PATCH] parallel-checkout: use grep -c to count workers in tests","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2021-06-05T15:20:58Z","receivedAt":"2021-06-05T15:21:06Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 05.06.21 um 16:31 schrieb Matheus Tavares Bernardino:\n> On Sat, Jun 5, 2021 at 9:27 AM René Scharfe <l.s.r@web.de> wrote:\n>>\n>> The parallel checkout tests fail when run with /bin/dash on MacOS 11.4,\n>> reporting the following error:\n>>\n>>    ./t2080-parallel-checkout-basics.sh: 33: local: 0: bad variable name\n>\n> That's interesting. It seems that dash is trying to use wc's output\n> (in this case \"0\") as a local variable name which, of course, is not\n> valid.\n>\n> This reply [1] to this bug report [2] mentions that dash expands a\n> local assignment like the following:\n>\n>     x=\"1 2 3\"\n>     local y=$x ---expands-to---> local y=1 2 3\n>\n> So, in this case, dash thinks we are trying to declare three local\n> variables, y, 2, and 3, which is an error. In bash, the above commands\n> would result in $y getting the value \"1 2 3\", even though we didn't\n> quote $x in the assignment. (BTW, the reply mentions that quoting the\n> right side of the assignment should make this work in dash as well.)\n>\n> I wonder if that's what's happening here. Maybe \"wc -l\" is outputting\n> a space before the number, and that makes dash parse this line as\n> something like `local workers=\"\" 0=\"\" `? If that's really the case (I\n> can't confirm because the bug seems to have been fixed in the dash\n> version I have), maybe we could mention that in the commit message.\n>\n> [1]: https://bugs.launchpad.net/ubuntu/+source/dash/+bug/139097/comments/6\n> [2]: https://bugs.launchpad.net/ubuntu/+source/dash/+bug/139097\n\nAh, indeed:\n\n   $ dash -c 'foo () { local bar=$(echo \"1\"); }; foo'\n\n   $ dash -c 'foo () { local bar=$(echo \" 1\"); }; foo'\n   dash: 1: local: 1: bad variable name\n\n   $ wc -l </dev/null | tr ' ' s\n   sssssss0\n\n>>  t/lib-parallel-checkout.sh | 2 +-\n>>  1 file changed, 1 insertion(+), 1 deletion(-)\n>>\n>> diff --git a/t/lib-parallel-checkout.sh b/t/lib-parallel-checkout.sh\n>> index 21f5759732..145276eb4c 100644\n>> --- a/t/lib-parallel-checkout.sh\n>> +++ b/t/lib-parallel-checkout.sh\n>> @@ -27,7 +27,7 @@ test_checkout_workers () {\n>>         rm -f \"$trace_file\" &&\n>>         GIT_TRACE2=\"$(pwd)/$trace_file\" \"$@\" 2>&8 &&\n>>\n>> -       local workers=$(grep \"child_start\\[..*\\] git checkout--worker\" \"$trace_file\" | wc -l) &&\n>> +       local workers=$(grep -c \"child_start\\[..*\\] git checkout--worker\" <\"$trace_file\") &&\n>\n> Nice, and the result is both cleaner and more efficient :) Just one\n> minor nit: I think we could drop the redirection as grep can take the\n> file name as an argument.\n\nI'm not sure if there's a grep out there that prints the filename before\nthe count even if it deals with a single file.  git grep does that, at\nleast.  POSIX[3] implies the lack of filename prefix for the single file\ncase, but I don't know if we can rely on that everywhere.\n\n[3] https://pubs.opengroup.org/onlinepubs/9699919799/utilities/grep.html\n\n>\n>>         test $workers -eq $expected_workers &&\n>>         rm \"$trace_file\"\n>>  } 8>&2 2>&4\n>> --\n>> 2.31.1\n"},{"id":"426494","messageId":"eb8e938f-1308-6200-5484-e009f6e4c625@web.de","threadId":"55845","inReplyTo":"a946daaa-6aa8-8527-5b55-ccf86de2a9d7@web.de","subject":"Re: [PATCH] parallel-checkout: use grep -c to count workers in tests","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2021-06-05T15:30:06Z","receivedAt":"2021-06-05T15:30:15Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 05.06.21 um 17:20 schrieb René Scharfe:\n> Am 05.06.21 um 16:31 schrieb Matheus Tavares Bernardino:\n>> On Sat, Jun 5, 2021 at 9:27 AM René Scharfe <l.s.r@web.de> wrote:\n>> Just one\n>> minor nit: I think we could drop the redirection as grep can take the\n>> file name as an argument.\n>\n> I'm not sure if there's a grep out there that prints the filename before\n> the count even if it deals with a single file.  git grep does that, at\n> least.  POSIX[3] implies the lack of filename prefix for the single file\n> case, but I don't know if we can rely on that everywhere.\n>\n> [3] https://pubs.opengroup.org/onlinepubs/9699919799/utilities/grep.html\n\nExcept we got plenty of \"grep -c\" calls with a single filename and no\nredirection in t/ already.  Adding one more should be fine.\n\nRené\n"},{"id":"426499","messageId":"472c1411-fcf8-862b-cef9-52c2c994914b@web.de","threadId":"55845","inReplyTo":"eb4bcd4c-e6d2-cbeb-8951-cf22b9d3d5fe@web.de","subject":"[PATCH v2] parallel-checkout: avoid dash local bug in tests","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2021-06-05T18:11:24Z","receivedAt":"2021-06-05T18:11:34Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"The parallel checkout tests fail when run with /bin/dash on MacOS 11.4,\nreporting the following error:\n\n   ./t2080-parallel-checkout-basics.sh: 33: local: 0: bad variable name\n\nThat's because wc's output contains leading spaces and this version of\ndash erroneously expands the variable declaration as \"local workers= 0\",\ni.e. it tries to set the \"workers\" variable to the empty string and also\ndeclare a variable named \"0\", which not a valid name.  This is a known\ndash bug (https://bugs.launchpad.net/ubuntu/+source/dash/+bug/139097).\n\nWork around it by passing the command output directly to test instead of\nstoring it in a variable first.  While at it, let grep count the number\nof lines instead of piping its output to wc, which is a bit shorter and\nmore efficient.\n\nHelped-by: Matheus Tavares Bernardino <matheus.bernardino@usp.br>\nSigned-off-by: René Scharfe <l.s.r@web.de>\n---\nChanges since v1:\n- Explain the root cause.\n- Get rid of the local variable \"workers\".\n- Adjust title accordingly.\n- Still use grep -c, though.\n- Remove input redirection.\n\n t/lib-parallel-checkout.sh | 3 +--\n 1 file changed, 1 insertion(+), 2 deletions(-)\n\ndiff --git a/t/lib-parallel-checkout.sh b/t/lib-parallel-checkout.sh\nindex 21f5759732..66350d5207 100644\n--- a/t/lib-parallel-checkout.sh\n+++ b/t/lib-parallel-checkout.sh\n@@ -27,8 +27,7 @@ test_checkout_workers () {\n \trm -f \"$trace_file\" &&\n \tGIT_TRACE2=\"$(pwd)/$trace_file\" \"$@\" 2>&8 &&\n\n-\tlocal workers=$(grep \"child_start\\[..*\\] git checkout--worker\" \"$trace_file\" | wc -l) &&\n-\ttest $workers -eq $expected_workers &&\n+\ttest $(grep -c \"child_start\\[..*\\] git checkout--worker\" \"$trace_file\") -eq $expected_workers &&\n \trm \"$trace_file\"\n } 8>&2 2>&4\n\n--\n2.31.1\n\n"},{"id":"426500","messageId":"20210605190910.GJ2947267@szeder.dev","threadId":"55845","inReplyTo":"472c1411-fcf8-862b-cef9-52c2c994914b@web.de","subject":"Re: [PATCH v2] parallel-checkout: avoid dash local bug in tests","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2021-06-05T19:09:10Z","receivedAt":"2021-06-05T19:10:53Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Sat, Jun 05, 2021 at 08:11:24PM +0200, René Scharfe wrote:\n> The parallel checkout tests fail when run with /bin/dash on MacOS 11.4,\n> reporting the following error:\n> \n>    ./t2080-parallel-checkout-basics.sh: 33: local: 0: bad variable name\n> \n> That's because wc's output contains leading spaces and this version of\n> dash erroneously expands the variable declaration as \"local workers= 0\",\n> i.e. it tries to set the \"workers\" variable to the empty string and also\n> declare a variable named \"0\", which not a valid name.  This is a known\n> dash bug (https://bugs.launchpad.net/ubuntu/+source/dash/+bug/139097).\n\nPerhaps a more accurate wording for this bug would be:\n\n  ... and even fairly recent versions of dash erroneously perform\n  field splitting on the expansion of the command substitution before\n  assigning it to a local variable.\n\nI think the relevant part of POSIX is section 2.9.1 Simple Commands:\n\n  4. Each variable assignment shall be expanded for tilde expansion,\n     parameter expansion, command substitution, arithmetic expansion,\n     and quote removal prior to assigning the value.\n\n  https://pubs.opengroup.org/onlinepubs/9699919799/utilities/V3_chap02.html#tag_18_09_01\n\nNote that it didn't mention field splitting; though POSIX doesn't\nspecifies local variables in the first place, so...\n\nAnyway, this bug has been fixed in v0.5.11 (2020-06-01).\nThis is an old bug, it was already present in v0.5.5 (2009-01-13); I\ndidn't check earlier versions.\n\n> Work around it by passing the command output directly to test instead of\n> storing it in a variable first.  While at it, let grep count the number\n> of lines instead of piping its output to wc, which is a bit shorter and\n> more efficient.\n\nA more debug-friendly alternative would be to save 'grep's output to a\ntemporary file and use 'test_line_count = $expected_workers'.\n\n> Helped-by: Matheus Tavares Bernardino <matheus.bernardino@usp.br>\n> Signed-off-by: René Scharfe <l.s.r@web.de>\n> ---\n> Changes since v1:\n> - Explain the root cause.\n> - Get rid of the local variable \"workers\".\n> - Adjust title accordingly.\n> - Still use grep -c, though.\n> - Remove input redirection.\n> \n>  t/lib-parallel-checkout.sh | 3 +--\n>  1 file changed, 1 insertion(+), 2 deletions(-)\n> \n> diff --git a/t/lib-parallel-checkout.sh b/t/lib-parallel-checkout.sh\n> index 21f5759732..66350d5207 100644\n> --- a/t/lib-parallel-checkout.sh\n> +++ b/t/lib-parallel-checkout.sh\n> @@ -27,8 +27,7 @@ test_checkout_workers () {\n>  \trm -f \"$trace_file\" &&\n>  \tGIT_TRACE2=\"$(pwd)/$trace_file\" \"$@\" 2>&8 &&\n> \n> -\tlocal workers=$(grep \"child_start\\[..*\\] git checkout--worker\" \"$trace_file\" | wc -l) &&\n> -\ttest $workers -eq $expected_workers &&\n> +\ttest $(grep -c \"child_start\\[..*\\] git checkout--worker\" \"$trace_file\") -eq $expected_workers &&\n>  \trm \"$trace_file\"\n>  } 8>&2 2>&4\n> \n> --\n> 2.31.1\n> \n"},{"id":"426501","messageId":"87fsxw5bav.fsf@evledraar.gmail.com","threadId":"55845","inReplyTo":"472c1411-fcf8-862b-cef9-52c2c994914b@web.de","subject":"Re: [PATCH v2] parallel-checkout: avoid dash local bug in tests","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-06-05T19:56:05Z","receivedAt":"2021-06-05T20:07:11Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Sat, Jun 05 2021, René Scharfe wrote:\n\n> The parallel checkout tests fail when run with /bin/dash on MacOS 11.4,\n> reporting the following error:\n>\n>    ./t2080-parallel-checkout-basics.sh: 33: local: 0: bad variable name\n>\n> That's because wc's output contains leading spaces and this version of\n> dash erroneously expands the variable declaration as \"local workers= 0\",\n> i.e. it tries to set the \"workers\" variable to the empty string and also\n> declare a variable named \"0\", which not a valid name.  This is a known\n> dash bug (https://bugs.launchpad.net/ubuntu/+source/dash/+bug/139097).\n>\n> Work around it by passing the command output directly to test instead of\n> storing it in a variable first.  While at it, let grep count the number\n> of lines instead of piping its output to wc, which is a bit shorter and\n> more efficient.\n>\n> Helped-by: Matheus Tavares Bernardino <matheus.bernardino@usp.br>\n> Signed-off-by: René Scharfe <l.s.r@web.de>\n> ---\n> Changes since v1:\n> - Explain the root cause.\n> - Get rid of the local variable \"workers\".\n> - Adjust title accordingly.\n> - Still use grep -c, though.\n> - Remove input redirection.\n>\n>  t/lib-parallel-checkout.sh | 3 +--\n>  1 file changed, 1 insertion(+), 2 deletions(-)\n>\n> diff --git a/t/lib-parallel-checkout.sh b/t/lib-parallel-checkout.sh\n> index 21f5759732..66350d5207 100644\n> --- a/t/lib-parallel-checkout.sh\n> +++ b/t/lib-parallel-checkout.sh\n> @@ -27,8 +27,7 @@ test_checkout_workers () {\n>  \trm -f \"$trace_file\" &&\n>  \tGIT_TRACE2=\"$(pwd)/$trace_file\" \"$@\" 2>&8 &&\n>\n> -\tlocal workers=$(grep \"child_start\\[..*\\] git checkout--worker\" \"$trace_file\" | wc -l) &&\n> -\ttest $workers -eq $expected_workers &&\n> +\ttest $(grep -c \"child_start\\[..*\\] git checkout--worker\" \"$trace_file\") -eq $expected_workers &&\n>  \trm \"$trace_file\"\n>  } 8>&2 2>&4\n\nI'd find this thing much clearer if the v2 just narrowly focused on\navoiding the \"local\", and thus demonstrated the non-portable shell\nissue, and perhaps with something like:\n\t\n\tdiff --git a/t/check-non-portable-shell.pl b/t/check-non-portable-shell.pl\n\tindex fd3303552be..aad6f3e2bf1 100755\n\t--- a/t/check-non-portable-shell.pl\n\t+++ b/t/check-non-portable-shell.pl\n\t@@ -48,6 +48,7 @@ sub err {\n\t \t/\\bexport\\s+[A-Za-z0-9_]*=/ and err '\"export FOO=bar\" is not portable (use FOO=bar && export FOO)';\n\t \t/^\\s*([A-Z0-9_]+=(\\w*|([\"']).*?\\3)\\s+)+(\\w+)/ and exists($func{$4}) and\n\t \t\terr '\"FOO=bar shell_func\" assignment extends beyond \"shell_func\"';\n\t+\t/\\blocal\\b \\S+=\\$\\(.*?\\|\\s*\\bwc -l\\)/ and err 'whitespace handling in local=$(... | wc -l) differs in some dash versions';\n\t \t$line = '';\n\t \t# this resets our $. for each file\n\t \tclose ARGV if eof;\n\n\nThe let's do grep -c while we're at it part of this IMO just adds\nconfusion while skimming future portability issues with --grep=dash or\n--grep=POSIX in the future, and looking at the history in v1 it's just\nthere because in v1 the root cause wasn't fully understood.\n\nIf we're doing a general cleanup of that pattern it would seem to be\nbetter to search-replace this with the rest of them in another commit:\n\n    $ git grep '\\$\\(grep.*\\| wc -l' -- t | wc -l\n    27\n"},{"id":"426505","messageId":"CAHd-oW6_29y90Ui-2mKjoyjZS0n9tyBY2a5ON8shEyddM0t8pQ@mail.gmail.com","threadId":"55845","inReplyTo":"87fsxw5bav.fsf@evledraar.gmail.com","subject":"Re: [PATCH v2] parallel-checkout: avoid dash local bug in tests","fromName":"Matheus Tavares Bernardino","fromEmail":"matheus.bernardino@usp.br","sentAt":"2021-06-05T22:17:52Z","receivedAt":"2021-06-05T22:19:07Z","isPatch":true,"sender":{"key":"matheus.tavb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12701583?v=4"},"body":"On Sat, Jun 5, 2021 at 5:03 PM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:\n>\n>\n> On Sat, Jun 05 2021, René Scharfe wrote:\n>\n> > The parallel checkout tests fail when run with /bin/dash on MacOS 11.4,\n> > reporting the following error:\n> >\n> >    ./t2080-parallel-checkout-basics.sh: 33: local: 0: bad variable name\n> >\n> > That's because wc's output contains leading spaces and this version of\n> > dash erroneously expands the variable declaration as \"local workers= 0\",\n> > i.e. it tries to set the \"workers\" variable to the empty string and also\n> > declare a variable named \"0\", which not a valid name.  This is a known\n> > dash bug (https://bugs.launchpad.net/ubuntu/+source/dash/+bug/139097).\n> >\n> > Work around it by passing the command output directly to test instead of\n> > storing it in a variable first.  While at it, let grep count the number\n> > of lines instead of piping its output to wc, which is a bit shorter and\n> > more efficient.\n> >\n> > Helped-by: Matheus Tavares Bernardino <matheus.bernardino@usp.br>\n> > Signed-off-by: René Scharfe <l.s.r@web.de>\n> > ---\n> > Changes since v1:\n> > - Explain the root cause.\n> > - Get rid of the local variable \"workers\".\n> > - Adjust title accordingly.\n> > - Still use grep -c, though.\n> > - Remove input redirection.\n> >\n> >  t/lib-parallel-checkout.sh | 3 +--\n> >  1 file changed, 1 insertion(+), 2 deletions(-)\n> >\n> > diff --git a/t/lib-parallel-checkout.sh b/t/lib-parallel-checkout.sh\n> > index 21f5759732..66350d5207 100644\n> > --- a/t/lib-parallel-checkout.sh\n> > +++ b/t/lib-parallel-checkout.sh\n> > @@ -27,8 +27,7 @@ test_checkout_workers () {\n> >       rm -f \"$trace_file\" &&\n> >       GIT_TRACE2=\"$(pwd)/$trace_file\" \"$@\" 2>&8 &&\n> >\n> > -     local workers=$(grep \"child_start\\[..*\\] git checkout--worker\" \"$trace_file\" | wc -l) &&\n> > -     test $workers -eq $expected_workers &&\n> > +     test $(grep -c \"child_start\\[..*\\] git checkout--worker\" \"$trace_file\") -eq $expected_workers &&\n> >       rm \"$trace_file\"\n> >  } 8>&2 2>&4\n>\n> I'd find this thing much clearer if the v2 just narrowly focused on\n> avoiding the \"local\", and thus demonstrated the non-portable shell\n> issue,\n\nI don't have any strong preference, but if we are leaving the \"grep |\nwc -l\" -> \"grep -c\" conversion to a followup patch, perhaps the\nsimplest change focusing on the dash issue would be to quote the\nright-hand side of the \"local\" assignment:\n\n-     local workers=$(grep \"child_start\\[..*\\] git checkout--worker\"\n\"$trace_file\" | wc -l) &&\n+     local workers=\"$(grep \"child_start\\[..*\\] git checkout--worker\"\n\"$trace_file\" | wc -l)\" &&\n\n(René, could you confirm if this works to make the test pass on dash?)\n\nAlternatively, we could use `test_line_count` as SZEDER Gábor\nsuggested in a parallel reply.\n"},{"id":"426506","messageId":"CAHd-oW4zZmk-_h7Ah0W5434v3bKJkoTfRBZB79Et0XW+T=PfTQ@mail.gmail.com","threadId":"55845","inReplyTo":"CAHd-oW6_29y90Ui-2mKjoyjZS0n9tyBY2a5ON8shEyddM0t8pQ@mail.gmail.com","subject":"Re: [PATCH v2] parallel-checkout: avoid dash local bug in tests","fromName":"Matheus Tavares Bernardino","fromEmail":"matheus.bernardino@usp.br","sentAt":"2021-06-05T22:21:48Z","receivedAt":"2021-06-05T22:23:10Z","isPatch":true,"sender":{"key":"matheus.tavb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12701583?v=4"},"body":"On Sat, Jun 5, 2021 at 7:17 PM Matheus Tavares Bernardino\n<matheus.bernardino@usp.br> wrote:\n>\n> I don't have any strong preference, but if we are leaving the \"grep |\n> wc -l\" -> \"grep -c\" conversion to a followup patch, perhaps the\n> simplest change focusing on the dash issue would be to quote the\n> right-hand side of the \"local\" assignment:\n>\n> -     local workers=$(grep \"child_start\\[..*\\] git checkout--worker\"\n> \"$trace_file\" | wc -l) &&\n> +     local workers=\"$(grep \"child_start\\[..*\\] git checkout--worker\"\n> \"$trace_file\" | wc -l)\" &&\n\nOops, sorry for the odd line breaking. I forgot that Gmail's web\nclient would break these lines.\n"},{"id":"426513","messageId":"5ed77a50-3b34-4f3d-8348-a11d3930872b@web.de","threadId":"55845","inReplyTo":"87fsxw5bav.fsf@evledraar.gmail.com","subject":"Re: [PATCH v2] parallel-checkout: avoid dash local bug in tests","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2021-06-06T01:01:38Z","receivedAt":"2021-06-06T01:01:47Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 05.06.21 um 21:56 schrieb Ævar Arnfjörð Bjarmason:\n>\n> On Sat, Jun 05 2021, René Scharfe wrote:\n>\n>> The parallel checkout tests fail when run with /bin/dash on MacOS 11.4,\n>> reporting the following error:\n>>\n>>    ./t2080-parallel-checkout-basics.sh: 33: local: 0: bad variable name\n>>\n>> That's because wc's output contains leading spaces and this version of\n>> dash erroneously expands the variable declaration as \"local workers= 0\",\n>> i.e. it tries to set the \"workers\" variable to the empty string and also\n>> declare a variable named \"0\", which not a valid name.  This is a known\n>> dash bug (https://bugs.launchpad.net/ubuntu/+source/dash/+bug/139097).\n>>\n>> Work around it by passing the command output directly to test instead of\n>> storing it in a variable first.  While at it, let grep count the number\n>> of lines instead of piping its output to wc, which is a bit shorter and\n>> more efficient.\n>>\n>> Helped-by: Matheus Tavares Bernardino <matheus.bernardino@usp.br>\n>> Signed-off-by: René Scharfe <l.s.r@web.de>\n>> ---\n>> Changes since v1:\n>> - Explain the root cause.\n>> - Get rid of the local variable \"workers\".\n>> - Adjust title accordingly.\n>> - Still use grep -c, though.\n>> - Remove input redirection.\n>>\n>>  t/lib-parallel-checkout.sh | 3 +--\n>>  1 file changed, 1 insertion(+), 2 deletions(-)\n>>\n>> diff --git a/t/lib-parallel-checkout.sh b/t/lib-parallel-checkout.sh\n>> index 21f5759732..66350d5207 100644\n>> --- a/t/lib-parallel-checkout.sh\n>> +++ b/t/lib-parallel-checkout.sh\n>> @@ -27,8 +27,7 @@ test_checkout_workers () {\n>>  \trm -f \"$trace_file\" &&\n>>  \tGIT_TRACE2=\"$(pwd)/$trace_file\" \"$@\" 2>&8 &&\n>>\n>> -\tlocal workers=$(grep \"child_start\\[..*\\] git checkout--worker\" \"$trace_file\" | wc -l) &&\n>> -\ttest $workers -eq $expected_workers &&\n>> +\ttest $(grep -c \"child_start\\[..*\\] git checkout--worker\" \"$trace_file\") -eq $expected_workers &&\n>>  \trm \"$trace_file\"\n>>  } 8>&2 2>&4\n>\n> I'd find this thing much clearer if the v2 just narrowly focused on\n> avoiding the \"local\", and thus demonstrated the non-portable shell\n> issue,\n\nI was not aiming for a minimal fix and I don't think the patch above is\ntoo complex, but you're right that at this point in the release cycle a\nduct-tape-style patch would be better.\n\n> and perhaps with something like:\n>\n> \tdiff --git a/t/check-non-portable-shell.pl b/t/check-non-portable-shell.pl\n> \tindex fd3303552be..aad6f3e2bf1 100755\n> \t--- a/t/check-non-portable-shell.pl\n> \t+++ b/t/check-non-portable-shell.pl\n> \t@@ -48,6 +48,7 @@ sub err {\n> \t \t/\\bexport\\s+[A-Za-z0-9_]*=/ and err '\"export FOO=bar\" is not portable (use FOO=bar && export FOO)';\n> \t \t/^\\s*([A-Z0-9_]+=(\\w*|([\"']).*?\\3)\\s+)+(\\w+)/ and exists($func{$4}) and\n> \t \t\terr '\"FOO=bar shell_func\" assignment extends beyond \"shell_func\"';\n> \t+\t/\\blocal\\b \\S+=\\$\\(.*?\\|\\s*\\bwc -l\\)/ and err 'whitespace handling in local=$(... | wc -l) differs in some dash versions';\n\nAny command can output whitespace, it's not limited to wc -l.  So a\nbetter rule might be to always quote command substitutions in local\nvariable declarations (local foo=\"$(...)\").  Or to disallow assignments\nwith local altogether, like we already do for export, but that might be\na bit much.\n\n> \t \t$line = '';\n> \t \t# this resets our $. for each file\n> \t \tclose ARGV if eof;\n>\n>\n> The let's do grep -c while we're at it part of this IMO just adds\n> confusion while skimming future portability issues with --grep=dash or\n> --grep=POSIX in the future, and looking at the history in v1 it's just\n> there because in v1 the root cause wasn't fully understood.\n\nTrue, I was still gripping the \"use grep -c instead of grep | wc -l\"\nhammer.  I better rewatch https://www.youtube.com/watch?v=Yv4tI6939q0\n\n>\n> If we're doing a general cleanup of that pattern it would seem to be\n> better to search-replace this with the rest of them in another commit:\n>\n>     $ git grep '\\$\\(grep.*\\| wc -l' -- t | wc -l\n>     27\n>\n"},{"id":"426514","messageId":"6a111a2e-ed65-9c22-1f0d-cdd54b144692@web.de","threadId":"55845","inReplyTo":"20210605190910.GJ2947267@szeder.dev","subject":"Re: [PATCH v2] parallel-checkout: avoid dash local bug in tests","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2021-06-06T01:01:45Z","receivedAt":"2021-06-06T01:01:54Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 05.06.21 um 21:09 schrieb SZEDER Gábor:\n> On Sat, Jun 05, 2021 at 08:11:24PM +0200, René Scharfe wrote:\n>> The parallel checkout tests fail when run with /bin/dash on MacOS 11.4,\n>> reporting the following error:\n>>\n>>    ./t2080-parallel-checkout-basics.sh: 33: local: 0: bad variable name\n>>\n>> That's because wc's output contains leading spaces and this version of\n>> dash erroneously expands the variable declaration as \"local workers= 0\",\n>> i.e. it tries to set the \"workers\" variable to the empty string and also\n>> declare a variable named \"0\", which not a valid name.  This is a known\n>> dash bug (https://bugs.launchpad.net/ubuntu/+source/dash/+bug/139097).\n>\n> Perhaps a more accurate wording for this bug would be:\n>\n>   ... and even fairly recent versions of dash erroneously perform\n>   field splitting on the expansion of the command substitution before\n>   assigning it to a local variable.\n\nOK.\n\n>\n> I think the relevant part of POSIX is section 2.9.1 Simple Commands:\n>\n>   4. Each variable assignment shall be expanded for tilde expansion,\n>      parameter expansion, command substitution, arithmetic expansion,\n>      and quote removal prior to assigning the value.\n>\n>   https://pubs.opengroup.org/onlinepubs/9699919799/utilities/V3_chap02.html#tag_18_09_01\n>\n> Note that it didn't mention field splitting; though POSIX doesn't\n> specifies local variables in the first place, so...\n>\n> Anyway, this bug has been fixed in v0.5.11 (2020-06-01).\n> This is an old bug, it was already present in v0.5.5 (2009-01-13); I\n> didn't check earlier versions.\n>\n>> Work around it by passing the command output directly to test instead of\n>> storing it in a variable first.  While at it, let grep count the number\n>> of lines instead of piping its output to wc, which is a bit shorter and\n>> more efficient.\n>\n> A more debug-friendly alternative would be to save 'grep's output to a\n> temporary file and use 'test_line_count = $expected_workers'.\n\nYes, but that would cement the use of grep and wc -l and I still can't\nlet go of the idea that grep -c would be slightly quicker.  And it would\nadd file I/O.\n\nSomething like this would be more efficient in the expected case:\n\n\tif test $expected_count -ne $(grep -c -e \"$pattern\" \"$file\")\n\tthen\n\t\techo \"Expected $expected_count lines matching $patter, but got:\"\n\t\tgrep -e \"$pattern\" \"$file\"\n\t\treturn 1\n\tfi\n\nI have no performance numbers to show, just the vague feeling that the\ntest suite takes way too long already.\n\nAnyway, for now a minimal fix should do.  Debug features can be added to\nthis case and several similar ones later.\n\n>\n>> Helped-by: Matheus Tavares Bernardino <matheus.bernardino@usp.br>\n>> Signed-off-by: René Scharfe <l.s.r@web.de>\n>> ---\n>> Changes since v1:\n>> - Explain the root cause.\n>> - Get rid of the local variable \"workers\".\n>> - Adjust title accordingly.\n>> - Still use grep -c, though.\n>> - Remove input redirection.\n>>\n>>  t/lib-parallel-checkout.sh | 3 +--\n>>  1 file changed, 1 insertion(+), 2 deletions(-)\n>>\n>> diff --git a/t/lib-parallel-checkout.sh b/t/lib-parallel-checkout.sh\n>> index 21f5759732..66350d5207 100644\n>> --- a/t/lib-parallel-checkout.sh\n>> +++ b/t/lib-parallel-checkout.sh\n>> @@ -27,8 +27,7 @@ test_checkout_workers () {\n>>  \trm -f \"$trace_file\" &&\n>>  \tGIT_TRACE2=\"$(pwd)/$trace_file\" \"$@\" 2>&8 &&\n>>\n>> -\tlocal workers=$(grep \"child_start\\[..*\\] git checkout--worker\" \"$trace_file\" | wc -l) &&\n>> -\ttest $workers -eq $expected_workers &&\n>> +\ttest $(grep -c \"child_start\\[..*\\] git checkout--worker\" \"$trace_file\") -eq $expected_workers &&\n>>  \trm \"$trace_file\"\n>>  } 8>&2 2>&4\n>>\n>> --\n>> 2.31.1\n>>\n"},{"id":"426515","messageId":"3b71b62d-b299-aac8-f796-ee7a5d7f87b0@web.de","threadId":"55845","inReplyTo":"472c1411-fcf8-862b-cef9-52c2c994914b@web.de","subject":"[PATCH v3] parallel-checkout: avoid dash local bug in tests","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2021-06-06T01:01:57Z","receivedAt":"2021-06-06T01:02:04Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Dash bug https://bugs.launchpad.net/ubuntu/+source/dash/+bug/139097\nlets the shell erroneously perform field splitting on the expansion of a\ncommand substitution during declaration of a local variable.  It causes\nthe parallel-checkout tests to fail e.g. when running them with\n/bin/dash on MacOS 11.4, where they error out like this:\n\n   ./t2080-parallel-checkout-basics.sh: 33: local: 0: bad variable name\n\nThat's because the output of wc -l contains leading spaces and the\nreturned number of lines is treated as another variable to declare, i.e.\nas in \"local workers= 0\".\n\nWork around it by enclosing the command substitution in quotes.\n\nHelped-by: Matheus Tavares Bernardino <matheus.bernardino@usp.br>\nHelped-by: SZEDER Gábor <szeder.dev@gmail.com>\nHelped-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\nSigned-off-by: René Scharfe <l.s.r@web.de>\n---\nChanges since v2:\n- Use minimal fix.\n- New commit message.\n\n t/lib-parallel-checkout.sh | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/t/lib-parallel-checkout.sh b/t/lib-parallel-checkout.sh\nindex 21f5759732..83b279a846 100644\n--- a/t/lib-parallel-checkout.sh\n+++ b/t/lib-parallel-checkout.sh\n@@ -27,7 +27,7 @@ test_checkout_workers () {\n \trm -f \"$trace_file\" &&\n \tGIT_TRACE2=\"$(pwd)/$trace_file\" \"$@\" 2>&8 &&\n\n-\tlocal workers=$(grep \"child_start\\[..*\\] git checkout--worker\" \"$trace_file\" | wc -l) &&\n+\tlocal workers=\"$(grep \"child_start\\[..*\\] git checkout--worker\" \"$trace_file\" | wc -l)\" &&\n \ttest $workers -eq $expected_workers &&\n \trm \"$trace_file\"\n } 8>&2 2>&4\n--\n2.31.1\n"},{"id":"426516","messageId":"xmqqa6o3233q.fsf@gitster.g","threadId":"55845","inReplyTo":"5ed77a50-3b34-4f3d-8348-a11d3930872b@web.de","subject":"Re: [PATCH v2] parallel-checkout: avoid dash local bug in tests","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-06-06T01:28:41Z","receivedAt":"2021-06-06T01:28:45Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"René Scharfe <l.s.r@web.de> writes:\n\n> Am 05.06.21 um 21:56 schrieb Ævar Arnfjörð Bjarmason:\n>>\n>> On Sat, Jun 05 2021, René Scharfe wrote:\n>>\n>>> -\tlocal workers=$(grep \"child_start\\[..*\\] git checkout--worker\" \"$trace_file\" | wc -l) &&\n>>> -\ttest $workers -eq $expected_workers &&\n>>> +\ttest $(grep -c \"child_start\\[..*\\] git checkout--worker\" \"$trace_file\") -eq $expected_workers &&\n>>>  \trm \"$trace_file\"\n>>>  } 8>&2 2>&4\n>>\n>> I'd find this thing much clearer if the v2 just narrowly focused on\n>> avoiding the \"local\", and thus demonstrated the non-portable shell\n>> issue,\n>\n> I was not aiming for a minimal fix and I don't think the patch above is\n> too complex, but you're right that at this point in the release cycle a\n> duct-tape-style patch would be better.\n\nSorry for being late to the party but I tend to disagree.  \"grep -c\"\nis used elsewhere in the tests, we know it is safe.\n\nIOW, the above change is quite straight-forward.  It is much more\nobvious and close to \"duct-tape-style\" fix, than some altermatives\nlike enclosing the whole command substitution in a pair of\ndouble-quotes and rely on $workers always getting used without\ndouble-quotes around it.  To me, that looks more subtle and brittle.\n\n>> and perhaps with something like:\n>>\n>> \tdiff --git a/t/check-non-portable-shell.pl b/t/check-non-portable-shell.pl\n>> \tindex fd3303552be..aad6f3e2bf1 100755\n>> \t--- a/t/check-non-portable-shell.pl\n>> \t+++ b/t/check-non-portable-shell.pl\n>> \t@@ -48,6 +48,7 @@ sub err {\n>> \t \t/\\bexport\\s+[A-Za-z0-9_]*=/ and err '\"export FOO=bar\" is not portable (use FOO=bar && export FOO)';\n>> \t \t/^\\s*([A-Z0-9_]+=(\\w*|([\"']).*?\\3)\\s+)+(\\w+)/ and exists($func{$4}) and\n>> \t \t\terr '\"FOO=bar shell_func\" assignment extends beyond \"shell_func\"';\n>> \t+\t/\\blocal\\b \\S+=\\$\\(.*?\\|\\s*\\bwc -l\\)/ and err 'whitespace handling in local=$(... | wc -l) differs in some dash versions';\n>\n> Any command can output whitespace, it's not limited to wc -l.  So a\n> better rule might be to always quote command substitutions in local\n> variable declarations (local foo=\"$(...)\").  Or to disallow assignments\n> with local altogether, like we already do for export, but that might be\n> a bit much.\n\nI actually do not mind \"no assignments with local decl\" that much.\nI agree that is outside the scope of this particular fix.\n"},{"id":"426517","messageId":"xmqq5yyr22i2.fsf@gitster.g","threadId":"55845","inReplyTo":"3b71b62d-b299-aac8-f796-ee7a5d7f87b0@web.de","subject":"Re: [PATCH v3] parallel-checkout: avoid dash local bug in tests","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-06-06T01:41:41Z","receivedAt":"2021-06-06T01:41:44Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"René Scharfe <l.s.r@web.de> writes:\n\n> Dash bug https://bugs.launchpad.net/ubuntu/+source/dash/+bug/139097\n> lets the shell erroneously perform field splitting on the expansion of a\n> command substitution during declaration of a local variable.  It causes\n> the parallel-checkout tests to fail e.g. when running them with\n> /bin/dash on MacOS 11.4, where they error out like this:\n>\n>    ./t2080-parallel-checkout-basics.sh: 33: local: 0: bad variable name\n>\n> That's because the output of wc -l contains leading spaces and the\n> returned number of lines is treated as another variable to declare, i.e.\n> as in \"local workers= 0\".\n>\n> Work around it by enclosing the command substitution in quotes.\n>\n> Helped-by: Matheus Tavares Bernardino <matheus.bernardino@usp.br>\n> Helped-by: SZEDER Gábor <szeder.dev@gmail.com>\n> Helped-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n> Signed-off-by: René Scharfe <l.s.r@web.de>\n> ---\n> Changes since v2:\n> - Use minimal fix.\n> - New commit message.\n\nThanks.  As I said, I do not necessarily think this is conceptually\n\"minimal\", but we use workers only once and without surrounding dq\nso it is easy to see that this also is correct.\n\nWill queue.\n\n>\n>  t/lib-parallel-checkout.sh | 2 +-\n>  1 file changed, 1 insertion(+), 1 deletion(-)\n>\n> diff --git a/t/lib-parallel-checkout.sh b/t/lib-parallel-checkout.sh\n> index 21f5759732..83b279a846 100644\n> --- a/t/lib-parallel-checkout.sh\n> +++ b/t/lib-parallel-checkout.sh\n> @@ -27,7 +27,7 @@ test_checkout_workers () {\n>  \trm -f \"$trace_file\" &&\n>  \tGIT_TRACE2=\"$(pwd)/$trace_file\" \"$@\" 2>&8 &&\n>\n> -\tlocal workers=$(grep \"child_start\\[..*\\] git checkout--worker\" \"$trace_file\" | wc -l) &&\n> +\tlocal workers=\"$(grep \"child_start\\[..*\\] git checkout--worker\" \"$trace_file\" | wc -l)\" &&\n>  \ttest $workers -eq $expected_workers &&\n>  \trm \"$trace_file\"\n>  } 8>&2 2>&4\n> --\n> 2.31.1\n"}]}