{"thread":{"id":"56632","subject":"[PATCH] p5311: handle spaces in wc(1) output","startedAt":"2021-10-02T20:33:34Z","lastAt":"2021-10-04T16:16:51Z","messageCount":5,"participants":["René Scharfe","Taylor Blau","Ævar Arnfjörð Bjarmason","Jeff King","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"437788","messageId":"a682e2c8-fecc-906e-0ff6-93de2b311d14@web.de","threadId":"56632","inReplyTo":null,"subject":"[PATCH] p5311: handle spaces in wc(1) output","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2021-10-02T20:33:18Z","receivedAt":"2021-10-02T20:33:34Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Some implementations of wc(1) align their output with leading spaces,\neven when just a single number is requested, e.g. with \"wc -c\". p5311\nruns all tests successfully on such a platform, but fails to aggregate\ntheir results and reports:\n\n # passed all 33 test(s)\n 1..33\n bad input line: 57144\n\nUse the helper function test_file_size to get the number without any\nspaces in a portable way to avoid the issue.\n\nSigned-off-by: René Scharfe \n---\n t/perf/README | 2 +-\n t/perf/p5311-pack-bitmaps-fetch.sh | 2 +-\n 2 files changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/t/perf/README b/t/perf/README\nindex fb9127a66f..802402d738 100644\n--- a/t/perf/README\n+++ b/t/perf/README\n@@ -190,7 +190,7 @@ shown in the aggregated output. For example:\n \t'\n\n \ttest_size 'output size'\n-\t\twc -c Some implementations of wc(1) align their output with leading spaces,\n> even when just a single number is requested, e.g. with \"wc -c\". p5311\n> runs all tests successfully on such a platform, but fails to aggregate\n> their results and reports:\n\nThis makes sense, and makes me think that wc's platform-specific\nimplementations are too tricky to use when we are being picky about\nleading spaces.\n\nIn other words, I think that your fix is absolutely correct, but I\nwonder if test_size should be friendlier in what it accepts, and to\nchomp off any leading space. So perhaps something like the below would\nwork without any modification to p5311.\n\n--- 8< ---\n\nSubject: [PATCH] t/perf/aggregate.perl: tolerate leading spaces\n\nWhen using `test_size` with `wc -c`, users on certain platforms can run\ninto issues when `wc` emits leading space characters in its output,\nwhich confuses get_times.\n\nCallers could switch to use test_file_size instead of `wc -c` (the\nformer never prints leading space characters, so will always work with\ntest_size regardless of platform), but this is an easy enough spot to\nmiss that we should teach get_times to be more tolerant of the input it\naccepts.\n\nTeach get_times to do just that by stripping any leading space\ncharacters.\n\nSigned-off-by: Taylor Blau \n---\n t/perf/aggregate.perl | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/t/perf/aggregate.perl b/t/perf/aggregate.perl\nindex 82c0df4553..575d2000cc 100755\n--- a/t/perf/aggregate.perl\n+++ b/t/perf/aggregate.perl\n@@ -17,8 +17,8 @@ sub get_times {\n \t\tmy $rt = ((defined $1 ? $1 : 0.0)*60+$2)*60+$3;\n \t\treturn ($rt, $4, $5);\n \t# size\n-\t} elsif ($line =~ /^\\d+$/) {\n-\t\treturn $&;\n+\t} elsif ($line =~ /^\\s*(\\d+)$/) {\n+\t\treturn $1;\n \t} else {\n \t\tdie \"bad input line: $line\";\n \t}\n--\n2.33.0.96.g73915697e6\n"},{"id":"437801","messageId":"87wnmuo7ii.fsf@evledraar.gmail.com","threadId":"56632","inReplyTo":"YVk8SeuDIWwsrdO0@nand.local","subject":"Re: [PATCH] p5311: handle spaces in wc(1) output","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-10-03T08:04:30Z","receivedAt":"2021-10-03T08:08:27Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Sun, Oct 03 2021, Taylor Blau wrote:\n\n> On Sat, Oct 02, 2021 at 10:33:18PM +0200, René Scharfe wrote:\n>> Some implementations of wc(1) align their output with leading spaces,\n>> even when just a single number is requested, e.g. with \"wc -c\". p5311\n>> runs all tests successfully on such a platform, but fails to aggregate\n>> their results and reports:\n>\n> This makes sense, and makes me think that wc's platform-specific\n> implementations are too tricky to use when we are being picky about\n> leading spaces.\n>\n> In other words, I think that your fix is absolutely correct, but I\n> wonder if test_size should be friendlier in what it accepts, and to\n> chomp off any leading space. So perhaps something like the below would\n> work without any modification to p5311.\n>\n> --- 8< ---\n>\n> Subject: [PATCH] t/perf/aggregate.perl: tolerate leading spaces\n>\n> When using `test_size` with `wc -c`, users on certain platforms can run\n> into issues when `wc` emits leading space characters in its output,\n> which confuses get_times.\n>\n> Callers could switch to use test_file_size instead of `wc -c` (the\n> former never prints leading space characters, so will always work with\n> test_size regardless of platform), but this is an easy enough spot to\n> miss that we should teach get_times to be more tolerant of the input it\n> accepts.\n>\n> Teach get_times to do just that by stripping any leading space\n> characters.\n>\n> Signed-off-by: Taylor Blau \n> ---\n> t/perf/aggregate.perl | 4 ++--\n> 1 file changed, 2 insertions(+), 2 deletions(-)\n>\n> diff --git a/t/perf/aggregate.perl b/t/perf/aggregate.perl\n> index 82c0df4553..575d2000cc 100755\n> --- a/t/perf/aggregate.perl\n> +++ b/t/perf/aggregate.perl\n> @@ -17,8 +17,8 @@ sub get_times {\n> \t\tmy $rt = ((defined $1 ? $1 : 0.0)*60+$2)*60+$3;\n> \t\treturn ($rt, $4, $5);\n> \t# size\n> -\t} elsif ($line =~ /^\\d+$/) {\n> -\t\treturn $&;\n> +\t} elsif ($line =~ /^\\s*(\\d+)$/) {\n> +\t\treturn $1;\n> \t} else {\n> \t\tdie \"bad input line: $line\";\n> \t}\n\nThis approach seems like a bit of plastering over the real problem. It's\nfine to use the output of \"wc -l\" or \"wc -c\" in the context of the\nshell's whitespace handling. That's why in various places we do:\n\n test $(wc -l <$file>) = 1\n\nOr similar, but *don't* put that $() in double-quotes. I.e. we're\nrelying on the shell's whitespace semantics.\n\nSo isn't it better to just pass this through the shell's own handling\nbefore emitting the data, something like this POC:\n\n $ stripspace() { var=$1; echo $@; }; x=$(stripspace \" hi\" \" there \"); echo \"\\\"$x\\\"\"\n \"hi there\"\n\nOf course fixing it up after that in Perl will work just as well, so I\nguess this is just an asthetic preference for having the shell handle\nthe shell's output issues with what's guaranteed to be shell-portable\nsolutions... :)\n"},{"id":"437857","messageId":"YVqws1lZMD+l1MfK@coredump.intra.peff.net","threadId":"56632","inReplyTo":"YVk8SeuDIWwsrdO0@nand.local","subject":"Re: [PATCH] p5311: handle spaces in wc(1) output","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-10-04T07:43:47Z","receivedAt":"2021-10-04T07:43:50Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Oct 03, 2021 at 01:14:49AM -0400, Taylor Blau wrote:\n\n> On Sat, Oct 02, 2021 at 10:33:18PM +0200, René Scharfe wrote:\n> > Some implementations of wc(1) align their output with leading spaces,\n> > even when just a single number is requested, e.g. with \"wc -c\". p5311\n> > runs all tests successfully on such a platform, but fails to aggregate\n> > their results and reports:\n> \n> This makes sense, and makes me think that wc's platform-specific\n> implementations are too tricky to use when we are being picky about\n> leading spaces.\n> \n> In other words, I think that your fix is absolutely correct, but I\n> wonder if test_size should be friendlier in what it accepts, and to\n> chomp off any leading space. So perhaps something like the below would\n> work without any modification to p5311.\n\nI do like this direction, because by centralizing, it's one less thing\nfor perf-script writers to mess up. And not only does it fix \"wc -c\",\nbut it is more friendly to any other tools (since test_size can really\nbe used with any scalar magnitude measurement we like; our current tests\njust happen to use wc).\n\nBut...\n\n> Subject: [PATCH] t/perf/aggregate.perl: tolerate leading spaces\n> \n> When using `test_size` with `wc -c`, users on certain platforms can run\n> into issues when `wc` emits leading space characters in its output,\n> which confuses get_times.\n> \n> Callers could switch to use test_file_size instead of `wc -c` (the\n> former never prints leading space characters, so will always work with\n> test_size regardless of platform), but this is an easy enough spot to\n> miss that we should teach get_times to be more tolerant of the input it\n> accepts.\n> \n> Teach get_times to do just that by stripping any leading space\n> characters.\n\nThis leaves the extra whitespace inside the test-results/foo.results\nfile, which is a bit unfortunate, just because anything else besides\naggregate.perl will have to do the same workaround. So we've traded one\ngotcha for another. ;)\n\nI don't have a strong opinion on which is worse. The ideal would be for\ntest_size() itself to handle it, though it's a bit awkward because it is\nliterally just redirecting the output of the test snippet into the\nresult file. It's probably not worth spending a ton of effort on that.\n\n> diff --git a/t/perf/aggregate.perl b/t/perf/aggregate.perl\n> index 82c0df4553..575d2000cc 100755\n> --- a/t/perf/aggregate.perl\n> +++ b/t/perf/aggregate.perl\n> @@ -17,8 +17,8 @@ sub get_times {\n> \t\tmy $rt = ((defined $1 ? $1 : 0.0)*60+$2)*60+$3;\n> \t\treturn ($rt, $4, $5);\n> \t# size\n> -\t} elsif ($line =~ /^\\d+$/) {\n> -\t\treturn $&;\n> +\t} elsif ($line =~ /^\\s*(\\d+)$/) {\n> +\t\treturn $1;\n\nIf we do go this route, it might be nice to ignore trailing whitespace,\ntoo (I don't think it matters for wc, but just for general\nfriendliness). I'm tempted even to say that it should just drop the\nanchors and match \"\\d+\" anywhere, but perhaps that is a recipe for\nmistakes (if somebody writes \"foo 1234\" we probably want to detect and\ncomplain).\n\n-Peff\n"},{"id":"437908","messageId":"xmqq5yucydcg.fsf@gitster.g","threadId":"56632","inReplyTo":"87wnmuo7ii.fsf@evledraar.gmail.com","subject":"Re: [PATCH] p5311: handle spaces in wc(1) output","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-10-04T16:16:47Z","receivedAt":"2021-10-04T16:16:51Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n> This approach seems like a bit of plastering over the real problem. It's\n> fine to use the output of \"wc -l\" or \"wc -c\" in the context of the\n> shell's whitespace handling. That's why in various places we do:\n\nSorry, but I am confused.\n\n> test $(wc -l <$file>) = 1\n>\n> Or similar, but *don't* put that $() in double-quotes. I.e. we're\n> relying on the shell's whitespace semantics.\n>\n> So isn't it better to just pass this through the shell's own handling\n> before emitting the data, something like this POC:\n>\n> $ stripspace() { var=$1; echo $@; }; x=$(stripspace \" hi\" \" there \"); echo \"\\\"$x\\\"\"\n> \"hi there\"\n\nAll of the above are not wrong per-se, but if I read the scaffolding\ncode correctly, the way the output from \"wc -c\" is used is not via a\nvariable, but\n\n test_size_ () {\n say >&3 \"running: $2\"\n if test_eval_ \"$2\" 3>\"$base\".result; then\n test_ok_ \"$1\"\n else\n test_failure_ \"$@\"\n fi\n }\n\n test_size () {\n test_wrapper_ test_size_ \"$@\"\n }\n\nwhere \"$2\" gets the script given to test_size, e.g.\n\n \ttest_size \"size $title\" '\n\t\twc -c Of course fixing it up after that in Perl will work just as well, so I\n> guess this is just an asthetic preference for having the shell handle\n> the shell's output issues with what's guaranteed to be shell-portable\n> solutions... :)\n\nMeaning we could rewrite aggregation in shell, then we can say we\nare not making Perl clean up after mess sh creates? I dunno...\n\nThanks.\n"}]}