{"thread":{"id":"27193","subject":"[RFC/PATCH] t0081-*.sh: Fix failure of the 'long read' tests","startedAt":"2011-04-26T18:05:38Z","lastAt":"2011-05-26T16:03:22Z","messageCount":7,"participants":["Ramsay Jones","Jonathan Nieder","Jeff King","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"166405","messageId":"4DB70972.20308@ramsay1.demon.co.uk","threadId":"27193","inReplyTo":null,"subject":"[RFC/PATCH] t0081-*.sh: Fix failure of the 'long read' tests","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsay1.demon.co.uk","sentAt":"2011-04-26T18:05:38Z","receivedAt":"2011-04-26T18:05:38Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"\nIn particular, the subshell feeding the input pipe, using the\nshell function generate_tens_of_lines(), dies early due to a\nbroken pipe (SIGPIPE). The test-line-buffer command reads all\nof the data it needs and closes the pipe, but the generate_\\\ntens_of_lines() function is still attempting to write to the\npipe, since it is in an infinite loop. This causes the later\n\"kill $!\" to fail, since the process no longer exists.\n\nThe infinite loop is caused by the loop variable increment\nexpression, which looks something like this:\n\n    : $((i = $i + 1)) || return\n\nThis expression does not actually increment the loop variable\nat all, so $i remains at zero, leading to the infinite loop.\n\nA simple fix, for some shells, would be to remove the '$'\nfrom the '$i' within the arithmetic expansion. However, some\nolder shells don't support assignment as part of an arithmetic\nexpansion. Therefore, in order to fix the problem, we replace\nthe loop increment expression with the simpler:\n\n    i=$(($i + 1))\n\nwhich solves the problem, while also being more portable.\n\nSigned-off-by: Ramsay Jones <ramsay@ramsay1.demon.co.uk>\n---\n\nJunio,\n\nI only remembered last night that I have been skipping some tests\non Linux (oops!); this fixes one of the failures for me. One of\nthe reasons for me simply skipping this test, rather than fixing\nit up earlier, was that it looked like (indeed *is*) another\nexample of a syntax error due to using an older dash shell.\n(Also, I thought there was some talk of scrapping this test).\n\nHowever, when I tried bash and an up-to-date dash, the problem\nchanged from a syntax error to an infinite loop! ;-P\n\nThe reason for the RFC is that I don't understand why (apparently)\nI'm the only person seeing this failure; I suspect that a newer\nversion of bash than I have does not have this problem (ie the\noriginal arithmetic expansion actually works). dunno :-(\n\nAlso, I considered changing the kill statement thus:\n\n    -\tkill $! &&\n    +\tif kill -0 $! 2>/dev/null\n    +\tthen\n    +\t\tkill $!\n    +\tfi &&\n\nbut I'm not sure that actually helps, and was not necessary to\nfix the failure for me.\n\n[BTW, the other tests failing for me are t4034 tests #21 (diff \ndriver 'bibtex'), #25 (diff driver 'html); I haven't had time to\ninvestigate yet]\n\nATB,\nRamsay Jones\n\n t/t0081-line-buffer.sh |    3 +--\n 1 files changed, 1 insertions(+), 2 deletions(-)\n\ndiff --git a/t/t0081-line-buffer.sh b/t/t0081-line-buffer.sh\nindex 5067d1e..9b79e3b 100755\n--- a/t/t0081-line-buffer.sh\n+++ b/t/t0081-line-buffer.sh\n@@ -25,8 +25,7 @@ generate_tens_of_lines () {\n \t\tdo\n \t\t\techo \"$line\"\n \t\tdone &&\n-\t\t: $((i = $i + 1)) ||\n-\t\treturn\n+\t\ti=$(($i + 1))\n \tdone\n }\n \n-- \n1.7.4\n"},{"id":"166408","messageId":"20110426193539.GA2616@elie","threadId":"27193","inReplyTo":"4DB70972.20308@ramsay1.demon.co.uk","subject":"Re: [RFC/PATCH] t0081-*.sh: Fix failure of the 'long read' tests","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2011-04-26T19:35:39Z","receivedAt":"2011-04-26T19:35:39Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Ramsay Jones wrote:\n\n> --- a/t/t0081-line-buffer.sh\n> +++ b/t/t0081-line-buffer.sh\n> @@ -25,8 +25,7 @@ generate_tens_of_lines () {\n>  \t\tdo\n>  \t\t\techo \"$line\"\n>  \t\tdone &&\n> -\t\t: $((i = $i + 1)) ||\n> -\t\treturn\n> +\t\ti=$(($i + 1))\n>  \tdone\n\nThis test is a mess.  Could you try the patch from the tip of\n\n\tgit://repo.or.cz/git/jrn.git svn-fe\n\nwhich just gets rid of it instead?\n\nThanks for a reminder.\n"},{"id":"166421","messageId":"20110426234850.GC32491@sigill.intra.peff.net","threadId":"27193","inReplyTo":"4DB70972.20308@ramsay1.demon.co.uk","subject":"Re: [RFC/PATCH] t0081-*.sh: Fix failure of the 'long read' tests","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-04-26T23:48:51Z","receivedAt":"2011-04-26T23:48:51Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Apr 26, 2011 at 07:05:38PM +0100, Ramsay Jones wrote:\n\n> The infinite loop is caused by the loop variable increment\n> expression, which looks something like this:\n> \n>     : $((i = $i + 1)) || return\n>\n> [...]\n>\n> The reason for the RFC is that I don't understand why (apparently)\n> I'm the only person seeing this failure; I suspect that a newer\n> version of bash than I have does not have this problem (ie the\n> original arithmetic expansion actually works). dunno :-(\n\nYeah, that syntax is handled just fine by my bash and dash:\n\n  $ cat >foo.sh <<'EOF'\n  i=1\n  : $((i = $i + 1))\n  echo $i\n  EOF\n\n  $ bash foo.sh\n  2\n  $ bash --version | head -n 1\n  GNU bash, version 4.1.5(1)-release (x86_64-pc-linux-gnu)\n\n  $ dash foo.sh\n  2\n\nBut I think your i=$(($i + 1)) is the right solution.\n\n-Peff\n"},{"id":"166801","messageId":"4DBC4310.4000105@ramsay1.demon.co.uk","threadId":"27193","inReplyTo":"20110426193539.GA2616@elie","subject":"Re: [RFC/PATCH] t0081-*.sh: Fix failure of the 'long read' tests","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsay1.demon.co.uk","sentAt":"2011-04-30T17:12:48Z","receivedAt":"2011-04-30T17:12:48Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"Jonathan Nieder wrote:\n> Ramsay Jones wrote:\n> \n>> --- a/t/t0081-line-buffer.sh\n>> +++ b/t/t0081-line-buffer.sh\n>> @@ -25,8 +25,7 @@ generate_tens_of_lines () {\n>>  \t\tdo\n>>  \t\t\techo \"$line\"\n>>  \t\tdone &&\n>> -\t\t: $((i = $i + 1)) ||\n>> -\t\treturn\n>> +\t\ti=$(($i + 1))\n>>  \tdone\n> \n> This test is a mess.  Could you try the patch from the tip of\n> \n> \tgit://repo.or.cz/git/jrn.git svn-fe\n> \n> which just gets rid of it instead?\n\nAh, so I didn't imagine the discussion to remove this test!\n\nYes, this works great for me. So, I will keep skipping the test\nuntil this patch makes it into git.git.\n\nThanks!\n\nATB,\nRamsay Jones\n"},{"id":"166802","messageId":"4DBC45F9.7090804@ramsay1.demon.co.uk","threadId":"27193","inReplyTo":"20110426234850.GC32491@sigill.intra.peff.net","subject":"Re: [RFC/PATCH] t0081-*.sh: Fix failure of the 'long read' tests","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsay1.demon.co.uk","sentAt":"2011-04-30T17:25:13Z","receivedAt":"2011-04-30T17:25:13Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"Jeff King wrote:\n> Yeah, that syntax is handled just fine by my bash and dash:\n> \n>   $ cat >foo.sh <<'EOF'\n>   i=1\n>   : $((i = $i + 1))\n>   echo $i\n>   EOF\n> \n>   $ bash foo.sh\n>   2\n>   $ bash --version | head -n 1\n>   GNU bash, version 4.1.5(1)-release (x86_64-pc-linux-gnu)\n> \n>   $ dash foo.sh\n>   2\n\nEr, ... yeah, it works for my bash and (up-to-date) dash too!\n[not the installed dash, of course, for which it is a syntax error]\n\nAhem ... *blush*\n\n> But I think your i=$(($i + 1)) is the right solution.\n\nYes, this fixes the problem and does not introduce a regression.\n\nSo, the patch is correct, but (apart from the last sentence) the\ncommit message is *absolute rubbish*. I won't bore you with the\ndetails of my lunacy! :-P\n\nHowever, I much prefer Jonathan's patch which removes this test\ncompletely!\n\nATB,\nRamsay Jones\n"},{"id":"168747","messageId":"20110526043335.GB17479@elie","threadId":"27193","inReplyTo":"4DBC45F9.7090804@ramsay1.demon.co.uk","subject":"[PULL svn-fe/maint] t0081-*.sh: Fix failure of the 'long read' tests","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2011-05-26T04:33:35Z","receivedAt":"2011-05-26T04:33:35Z","isPatch":false,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nRamsay Jones wrote:\n\n> So, the patch is correct, but (apart from the last sentence) the\n> commit message is *absolute rubbish*. I won't bore you with the\n> details of my lunacy! :-P\n>\n> However, I much prefer Jonathan's patch which removes this test\n> completely!\n\nJunio, please pull\n\n  git://repo.or.cz/git/jrn.git svn-fe-maint\n\nto receive the following fix.  The patch first visited the list two\nmonths ago[1] and was discussed again last month[2] and seems to have\nbeen well liked both times (well, I know I like it).\n\nThe tests it removes\n\n - are missing an EXECKEEPSPID prerequisite on Windows\n - use a : $((i = i + 1)) construct which does not seem to be portable\n   to old versions of dash\n - are pointless, an eyesore, and a pain to maintain\n\nWhat was the author thinking?  Sorry to let this sit for so long.  \n\nJonathan Nieder (1):\n      Revert \"t0081 (line-buffer): add buffering tests\"\n\n t/t0081-line-buffer.sh |  106 +-----------------------------------------------\n 1 files changed, 2 insertions(+), 104 deletions(-)\n\n[1] http://thread.gmane.org/gmane.comp.version-control.git/170307/focus=170365\n[2] http://thread.gmane.org/gmane.comp.version-control.git/172120/focus=172523\n"},{"id":"168792","messageId":"7v8vttzbxx.fsf@alter.siamese.dyndns.org","threadId":"27193","inReplyTo":"20110526043335.GB17479@elie","subject":"Re: [PULL svn-fe/maint] t0081-*.sh: Fix failure of the 'long read' tests","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-05-26T16:03:22Z","receivedAt":"2011-05-26T16:03:22Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Nieder <jrnieder@gmail.com> writes:\n\n> Junio, please pull\n>\n>   git://repo.or.cz/git/jrn.git svn-fe-maint\n>\n> to receive the following fix.  The patch first visited the list two\n> months ago[1] and was discussed again last month[2] and seems to have\n> been well liked both times (well, I know I like it).\n>\n> The tests it removes\n>\n>  - are missing an EXECKEEPSPID prerequisite on Windows\n>  - use a : $((i = i + 1)) construct which does not seem to be portable\n>    to old versions of dash\n>  - are pointless, an eyesore, and a pain to maintain\n\nDone, and thanks.\n"}]}