{"thread":{"id":"26914","subject":"[PATCH] Portability: returning void","startedAt":"2011-03-29T17:31:30Z","lastAt":"2011-03-30T19:31:30Z","messageCount":18,"participants":["Michael Witten","Jonathan Nieder","Jeff King","Johannes Sixt","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"164611","messageId":"71372d7d-dd08-4945-a8bc-c7b981c09fb2-mfwitten@gmail.com","threadId":"26914","inReplyTo":null,"subject":"[PATCH] Portability: returning void","fromName":"Michael Witten","fromEmail":"mfwitten@gmail.com","sentAt":"2011-03-29T17:31:30Z","receivedAt":"2011-03-29T17:31:30Z","isPatch":true,"sender":{"key":"mfwitten@gmail.com","avatar":"https://avatars.githubusercontent.com/u/597101?v=4"},"body":"Currently, building git with:\n\n  CFLAGS=\"-std=c99 -pedantic -Wall -Werror -g -02\"\n\ncauses gcc 4.5.2 to fail with:\n\n  vcs-svn/svndump.c:217:3: error: ISO C forbids 'return' with\n                                  expression, in function\n                                  returning void\n\nThe line in question is this:\n\n  return repo_delete(node_ctx.dst);\n\nBecause repo_delete returns void (vcs-svn/repo_tree.h:19):\n\n  void repo_delete(uint32_t *path);\n\nit would seem like it would be OK, but I guess the\nC99 standard is quite particular:\n\n  6.8.6.4.1:\n  A return statement with an expression shall not appear in\n  a function whose return type is void. A return statement\n  without an expression shall only appear in a function\n  whose return type is void.\n\nSigned-off-by: Michael Witten <mfwitten@gmail.com>\n---\n vcs-svn/svndump.c |    3 ++-\n 1 files changed, 2 insertions(+), 1 deletions(-)\n\ndiff --git a/vcs-svn/svndump.c b/vcs-svn/svndump.c\nindex eef49ca..572a995 100644\n--- a/vcs-svn/svndump.c\n+++ b/vcs-svn/svndump.c\n@@ -214,7 +214,8 @@ static void handle_node(void)\n \t\tif (have_text || have_props || node_ctx.srcRev)\n \t\t\tdie(\"invalid dump: deletion node has \"\n \t\t\t\t\"copyfrom info, text, or properties\");\n-\t\treturn repo_delete(node_ctx.dst);\n+\t\trepo_delete(node_ctx.dst);\n+\t\treturn;\n \t}\n \tif (node_ctx.action == NODEACT_REPLACE) {\n \t\trepo_delete(node_ctx.dst);\n-- \n1.7.4.2.417.g32d76d\n"},{"id":"164619","messageId":"20110329200230.GA377@elie","threadId":"26914","inReplyTo":"71372d7d-dd08-4945-a8bc-c7b981c09fb2-mfwitten@gmail.com","subject":"Re: [PATCH] Portability: returning void","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2011-03-29T20:02:48Z","receivedAt":"2011-03-29T20:02:48Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nMichael Witten wrote:\n\n> Currently, building git with:\n>\n>   CFLAGS=\"-std=c99 -pedantic -Wall -Werror -g -02\"\n>\n> causes gcc 4.5.2 to fail with:\n>\n>   vcs-svn/svndump.c:217:3: error: ISO C forbids 'return' with\n>                                   expression, in function\n>                                   returning void\n[...]\n> Signed-off-by: Michael Witten <mfwitten@gmail.com>\n\nThanks, looks sensible (and thanks to Junio for a pointer).  Applied to\n\n  git://repo.or.cz/git/jrn.git svn-fe\n\nNext step is to figure out the longstanding mysterious bash + prove\nhang in t0081.\n\nJonathan Nieder (2):\n      vcs-svn: add missing cast to printf argument\n      tests: make sure input to sed is newline terminated\n\nMichael Witten (1):\n      vcs-svn: a void function shouldn't try to return something\n\n t/t9010-svn-fe.sh     |    8 ++++++--\n vcs-svn/fast_export.c |    3 ++-\n vcs-svn/svndump.c     |    3 ++-\n 3 files changed, 10 insertions(+), 4 deletions(-)\n"},{"id":"164636","messageId":"20110329221652.GB23510@sigill.intra.peff.net","threadId":"26914","inReplyTo":"20110329200230.GA377@elie","subject":"Re: [PATCH] Portability: returning void","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-03-29T22:16:52Z","receivedAt":"2011-03-29T22:16:52Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Mar 29, 2011 at 03:02:48PM -0500, Jonathan Nieder wrote:\n\n> Next step is to figure out the longstanding mysterious bash + prove\n> hang in t0081.\n\nThis patch solves it for me:\n\ndiff --git a/t/t0081-line-buffer.sh b/t/t0081-line-buffer.sh\nindex 1dbe1c9..7ea1317 100755\n--- a/t/t0081-line-buffer.sh\n+++ b/t/t0081-line-buffer.sh\n@@ -50,7 +50,7 @@ long_read_test () {\n \t\t{\n \t\t\tgenerate_tens_of_lines $tens_of_lines \"$line\" &&\n \t\t\tsleep 100\n-\t\t} >input &\n+\t\t} >input 5>/dev/null &\n \t} &&\n \ttest-line-buffer input <<-EOF >output &&\n \tbinary $readsize\n@@ -84,7 +84,7 @@ test_expect_success PIPE '0-length read, no input available' '\n \trm -f input &&\n \tmkfifo input &&\n \t{\n-\t\tsleep 100 >input &\n+\t\tsleep 100 >input 5>/dev/null &\n \t} &&\n \ttest-line-buffer input <<-\\EOF >actual &&\n \tbinary 0\n@@ -113,7 +113,7 @@ test_expect_success PIPE '1-byte read, no input available' '\n \t\t\tprintf \"%s\" a &&\n \t\t\tprintf \"%s\" b &&\n \t\t\tsleep 100\n-\t\t} >input &\n+\t\t} >input 5>/dev/null &\n \t} &&\n \ttest-line-buffer input <<-\\EOF >actual &&\n \tbinary 1\n\nThe problem is that the sleeps hang around for 100 seconds, and they are\nconnected to the test script's stdout. It works to run \"./t0081-*\"\nbecause bash sees the SIGCHLD and knows the script is done. But the\nprove program actually ignore the SIGCHLD and waits until stdout and\nstderr on the child are closed.\n\nI'm not sure why it hangs with bash and not dash. Perhaps it has to do\nwith one of them using an internal \"sleep\" and the other not.\nDouble-weird is that if you \"strace\" the prove process, it will still\nhang. But if you \"strace -f\", it _won't_ hang. Which makes no sense,\nbecause the only extra thing happening is strace-ing the now-zombie bash\nprocess and the sleeps which are, well, sleeping.\n\nSo there is definitely some deep mystery, but I think the fix can be\nsimpler: we should not leave processes hanging around that might have\ndescriptors open. Though the above works, I don't like scripts having to\nknow about the descriptor 5 magic above. The right solution to me is\neither:\n\n  1. Close descriptor 5 before running test code. But I'm not sure that\n     will work, since we actually execute the test code in the _current_\n     shell, not a subshell. Meaning we would be closing it for good.\n\n     It also doesn't address descriptor 4, which might also go to\n     stdout (if \"-v\" was used). I think the saving grace is that \"prove\"\n     is the only thing that cares, and it doesn't use \"-v\".\n\n  2. Tests should kill their backgrounded sleeps themselves. I think I\n     saw some \"kill $!\" lines in there, but maybe we are missing one.\n\n-Peff\n"},{"id":"164639","messageId":"20110329223618.GC23510@sigill.intra.peff.net","threadId":"26914","inReplyTo":"20110329221652.GB23510@sigill.intra.peff.net","subject":"Re: [PATCH] Portability: returning void","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-03-29T22:36:18Z","receivedAt":"2011-03-29T22:36:18Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Mar 29, 2011 at 06:16:52PM -0400, Jeff King wrote:\n\n> On Tue, Mar 29, 2011 at 03:02:48PM -0500, Jonathan Nieder wrote:\n> \n> > Next step is to figure out the longstanding mysterious bash + prove\n> > hang in t0081.\n>\n> [...]\n> \n>   2. Tests should kill their backgrounded sleeps themselves. I think I\n>      saw some \"kill $!\" lines in there, but maybe we are missing one.\n\nIndeed, those kill lines aren't doing what you expect. Try instrumenting\nlike this:\n\ndiff --git a/t/t0081-line-buffer.sh b/t/t0081-line-buffer.sh\nindex 1dbe1c9..3b2f8ce 100755\n--- a/t/t0081-line-buffer.sh\n+++ b/t/t0081-line-buffer.sh\n@@ -1,4 +1,4 @@\n-#!/bin/sh\n+#!/bin/bash\n \n test_description=\"Test the svn importer's input handling routines.\n \n@@ -56,6 +56,7 @@ long_read_test () {\n \tbinary $readsize\n \tcopy $copysize\n \tEOF\n+\techo >>/tmp/foo killing $!\n \tkill $! &&\n \ttest_line_count = $lines output &&\n \ttail -n 1 <output >actual &&\n@@ -90,6 +91,7 @@ test_expect_success PIPE '0-length read, no input available' '\n \tbinary 0\n \tcopy 0\n \tEOF\n+\techo >>/tmp/foo killing $!\n \tkill $! &&\n \ttest_cmp expect actual\n '\n@@ -119,6 +121,7 @@ test_expect_success PIPE '1-byte read, no input available' '\n \tbinary 1\n \tcopy 1\n \tEOF\n+\techo >>/tmp/foo killing $!\n \tkill $! &&\n \ttest_cmp expect actual\n '\n\nAnd then run the test under prove, and check \"ps\" for remaining sleep\nprocesses. You will see that the killed process IDs do not match up with\nthe sleep processes. Except for one sleep process, which actually got\nkilled. That is the second one in the list above. It works because it's:\n\n  {\n    sleep 100 >input &\n  } &&\n  ...\n  kill $!\n\nwhereas the other ones are like:\n\n  {\n    do_something &&\n    sleep 100\n  } >input &\n\nSo my guess is that we have to start a subshell for the latter ones, and\n_that_ is what gets killed. And that may explain the bash vs dash\nbehavior; dash, upon receiving the kill signal, presumably kills its\nchild, but bash does not (I didn't confirm that; it's just a theory).\n\nI'm not sure what the best solution is. Perhaps putting the subshell\ninto its own process group and killing the whole PGRP?\n\n-Peff\n"},{"id":"164642","messageId":"20110329234955.GB14578@elie","threadId":"26914","inReplyTo":"20110329221652.GB23510@sigill.intra.peff.net","subject":"Re: [PATCH] Portability: returning void","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2011-03-29T23:49:55Z","receivedAt":"2011-03-29T23:49:55Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Jeff King wrote:\n\n> The problem is that the sleeps hang around for 100 seconds, and they are\n> connected to the test script's stdout. It works to run \"./t0081-*\"\n> because bash sees the SIGCHLD and knows the script is done. But the\n> prove program actually ignore the SIGCHLD and waits until stdout and\n> stderr on the child are closed.\n\nStrange.  Why would prove tell its children to ignore SIGCHLD and\nSIGTERM?\n\n> Double-weird is that if you \"strace\" the prove process, it will still\n> hang. But if you \"strace -f\", it _won't_ hang.\n\nWell, it hangs for me. :)  The strangest aspect is that after 100\nseconds, all is well again, which suggests that there's more happening\nthan an unreaped process.\n\nAfter lowering the sleep to 15 seconds:\n\n| kill: 19422\n| bash(19422)---sleep(19424)\n|\n| ok 3 - 0-length read, no input available\n[...]\n| kill: 19431\n| bash(19431)---sleep(19434)\n| ok 5 - 1-byte read, no input available\n[...]\n| kill: 19438\n| bash(19438)---sleep(19441)\n| \n| ok 6 - long read (around 8192 bytes)\n[...]\n| # passed all 13 test(s)\n[...]\n| 19398 18:31:12 exit_group(0)            = ?\n| 19397 18:31:12 <... select resumed> )   = ? ERESTARTNOHAND (To be restarted)\n| 19397 18:31:12 --- SIGCHLD (Child exited) @ 0 (0) ---\n| 19397 18:31:12 select(8, [4 6], NULL, NULL, NULL <unfinished ...>\n\nThe test script exits, but \"prove\" is stuck in select and does not\nwant to start reaping yet.  So presumably the test script's children\nare adopted by init.  We wait around 13 seconds, and then:\n\n| 19424 18:31:25 <... nanosleep resumed> NULL) = 0\n| 19424 18:31:25 close(1)                 = 0\n| 19424 18:31:25 close(2)                 = 0\n| 19424 18:31:25 exit_group(0)            = ?\n| 19422 18:31:25 <... wait4 resumed> 0x7fff65d1ee6c, 0, NULL) = ? ERESTARTSYS (To be restarted)\n| 19422 18:31:25 --- SIGTERM (Terminated) @ 0 (0) ---\n\nThe first sleep wakes up and dies.  The corresponding subshell\nwakes up, reaps the child, and finally accepts SIGTERM.\n\n| 19434 18:31:26 <... nanosleep resumed> NULL) = 0\n[...]\n| 19438 18:31:26 --- SIGTERM (Terminated) @ 0 (0) ---\n\nLikewise for the second and third sleeps.\n\n| 19397 18:31:26 <... select resumed> )   = 2 (in [4 6])\n| 19397 18:31:26 read(4, \"\", 65536)       = 0\n| 19397 18:31:26 read(6, \"\", 65536)       = 0\n| 19397 18:31:26 wait4(19398, [{WIFEXITED(s) && WEXITSTATUS(s) == 0}], 0, NULL) = 19398\n\nNow \"prove\" wakes up again.\n\nThe analagous experiment for dash, mksh, etc shows that their\nsubshells exec()'d /bin/sleep so the SIGTERM reaches sleep right\naway with no trouble.\n"},{"id":"164645","messageId":"20110330001653.GA1161@sigill.intra.peff.net","threadId":"26914","inReplyTo":"20110329234955.GB14578@elie","subject":"Re: [PATCH] Portability: returning void","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-03-30T00:16:53Z","receivedAt":"2011-03-30T00:16:53Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Mar 29, 2011 at 06:49:55PM -0500, Jonathan Nieder wrote:\n\n> Jeff King wrote:\n> \n> > The problem is that the sleeps hang around for 100 seconds, and they are\n> > connected to the test script's stdout. It works to run \"./t0081-*\"\n> > because bash sees the SIGCHLD and knows the script is done. But the\n> > prove program actually ignore the SIGCHLD and waits until stdout and\n> > stderr on the child are closed.\n> \n> Strange.  Why would prove tell its children to ignore SIGCHLD and\n> SIGTERM?\n\nNo, you misunderstand. It is prove itself that ignores the SIGCHLD. It\nis stuck in the loop in TAP::Parser::Iterator::Process::_next. It has\ngotten SIGCHLD, but it keeps blocking waiting to get EOF on the child's\nstdout.\n\n> > Double-weird is that if you \"strace\" the prove process, it will still\n> > hang. But if you \"strace -f\", it _won't_ hang.\n> \n> Well, it hangs for me. :)  The strangest aspect is that after 100\n> seconds, all is well again, which suggests that there's more happening\n> than an unreaped process.\n\nDoesn't that point to an unreaped process? After 100 seconds the sleep\nprocess closes, prove gets EOF, and it completes. Lowering the \"100\" to\n\"1\" caused a 1-second hang for me.\n\n> | 19398 18:31:12 exit_group(0)            = ?\n> | 19397 18:31:12 <... select resumed> )   = ? ERESTARTNOHAND (To be restarted)\n> | 19397 18:31:12 --- SIGCHLD (Child exited) @ 0 (0) ---\n> | 19397 18:31:12 select(8, [4 6], NULL, NULL, NULL <unfinished ...>\n> \n> The test script exits, but \"prove\" is stuck in select and does not\n> want to start reaping yet.  So presumably the test script's children\n> are adopted by init.  We wait around 13 seconds, and then:\n\nRight, prove is stuck in the select. You can see it even got SIGCHLD\nabove, and if you check your process list, you will probably see the\ndefunct bash process. But instead of realizing its child has died, it\ninsists on waiting until the pipe is closed. Nothing has to be adopted\nby init. There are simply still processes with the pipe open.\n\n> | 19424 18:31:25 <... nanosleep resumed> NULL) = 0\n> | 19424 18:31:25 close(1)                 = 0\n> | 19424 18:31:25 close(2)                 = 0\n> | 19424 18:31:25 exit_group(0)            = ?\n> | 19422 18:31:25 <... wait4 resumed> 0x7fff65d1ee6c, 0, NULL) = ? ERESTARTSYS (To be restarted)\n> | 19422 18:31:25 --- SIGTERM (Terminated) @ 0 (0) ---\n> \n> The first sleep wakes up and dies.  The corresponding subshell\n> wakes up, reaps the child, and finally accepts SIGTERM.\n\nHrm. That's different than what happens on my system. On my system, the\nbash process is _already_ dead during the whole procedure, and it is\njust the stray sleeps that keep prove waiting.\n\nMaybe different bash versions? Mine is 4.1.5(1) (from debian unstable,\nbash_4.1-3).\n\n> | 19397 18:31:26 <... select resumed> )   = 2 (in [4 6])\n> | 19397 18:31:26 read(4, \"\", 65536)       = 0\n> | 19397 18:31:26 read(6, \"\", 65536)       = 0\n> | 19397 18:31:26 wait4(19398, [{WIFEXITED(s) && WEXITSTATUS(s) == 0}], 0, NULL) = 19398\n> \n> Now \"prove\" wakes up again.\n\nRight, because the pipe is finally closed.\n\nDid you try my 5>/dev/null patch? With it, I get no hang at all.\n\n-Peff\n"},{"id":"164647","messageId":"20110330002921.GC14578@elie","threadId":"26914","inReplyTo":"20110330001653.GA1161@sigill.intra.peff.net","subject":"Re: [PATCH] Portability: returning void","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2011-03-30T00:29:21Z","receivedAt":"2011-03-30T00:29:21Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"(-cc: Michael, to stop spamming a kind bug reporter :))\nJeff King wrote:\n> On Tue, Mar 29, 2011 at 06:49:55PM -0500, Jonathan Nieder wrote:\n\n>> | 19424 18:31:25 <... nanosleep resumed> NULL) = 0\n>> | 19424 18:31:25 close(1)                 = 0\n>> | 19424 18:31:25 close(2)                 = 0\n>> | 19424 18:31:25 exit_group(0)            = ?\n>> | 19422 18:31:25 <... wait4 resumed> 0x7fff65d1ee6c, 0, NULL) = ? ERESTARTSYS (To be restarted)\n>> | 19422 18:31:25 --- SIGTERM (Terminated) @ 0 (0) ---\n>> \n>> The first sleep wakes up and dies.  The corresponding subshell\n>> wakes up, reaps the child, and finally accepts SIGTERM.\n>\n> Hrm. That's different than what happens on my system. On my system, the\n> bash process is _already_ dead during the whole procedure, and it is\n> just the stray sleeps that keep prove waiting.\n>\n> Maybe different bash versions? Mine is 4.1.5(1) (from debian unstable,\n> bash_4.1-3).\n\n$ dpkg-query -W perl bash\nbash\t4.1-3\nperl\t5.10.1-18\n\nSame version here, but I had modified the test a little.  *tries the\nstock version again*  Same behavior still.  FWIW I am using the patch\nbelow[1] and invoking the tests as\n\n\tstrace -f -o trace.out prove --exec=bash -v t0081-line-buffer.sh :: -v -i\n\n> Did you try my 5>/dev/null patch? With it, I get no hang at all.\n\nHaven't tried it yet but will try.\n\nI really don't like that as a long-term solution.  Yes, it gets prove\nto stop hanging, but meanwhile we have no control over the child\nprocesses we have spawned.  I'd rather just drop the tests.\n---\ndiff --git a/t/t0081-line-buffer.sh b/t/t0081-line-buffer.sh\nindex 1dbe1c9..054bffa 100755\n--- a/t/t0081-line-buffer.sh\n+++ b/t/t0081-line-buffer.sh\n@@ -49,13 +49,14 @@ long_read_test () {\n \t{\n \t\t{\n \t\t\tgenerate_tens_of_lines $tens_of_lines \"$line\" &&\n-\t\t\tsleep 100\n+\t\t\tsleep 15\n \t\t} >input &\n \t} &&\n \ttest-line-buffer input <<-EOF >output &&\n \tbinary $readsize\n \tcopy $copysize\n \tEOF\n+\tpstree -p $! &&\n \tkill $! &&\n \ttest_line_count = $lines output &&\n \ttail -n 1 <output >actual &&\n@@ -84,12 +85,13 @@ test_expect_success PIPE '0-length read, no input available' '\n \trm -f input &&\n \tmkfifo input &&\n \t{\n-\t\tsleep 100 >input &\n+\t\tsleep 15 >input &\n \t} &&\n \ttest-line-buffer input <<-\\EOF >actual &&\n \tbinary 0\n \tcopy 0\n \tEOF\n+\tpstree -p $! &&\n \tkill $! &&\n \ttest_cmp expect actual\n '\n@@ -112,13 +114,14 @@ test_expect_success PIPE '1-byte read, no input available' '\n \t\t{\n \t\t\tprintf \"%s\" a &&\n \t\t\tprintf \"%s\" b &&\n-\t\t\tsleep 100\n+\t\t\tsleep 15\n \t\t} >input &\n \t} &&\n \ttest-line-buffer input <<-\\EOF >actual &&\n \tbinary 1\n \tcopy 1\n \tEOF\n+\tpstree -p $! &&\n \tkill $! &&\n \ttest_cmp expect actual\n '\n-- \n"},{"id":"164648","messageId":"20110330004236.GD14578@elie","threadId":"26914","inReplyTo":"20110330001653.GA1161@sigill.intra.peff.net","subject":"Re: [PATCH] Portability: returning void","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2011-03-30T00:42:36Z","receivedAt":"2011-03-30T00:42:36Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Jeff King wrote:\n\n> No, you misunderstand. It is prove itself that ignores the SIGCHLD. It\n> is stuck in the loop in TAP::Parser::Iterator::Process::_next. It has\n> gotten SIGCHLD, but it keeps blocking waiting to get EOF on the child's\n> stdout.\n\nThanks for clarifying.\n\n> Doesn't that point to an unreaped process? After 100 seconds the sleep\n> process closes, prove gets EOF, and it completes. Lowering the \"100\" to\n> \"1\" caused a 1-second hang for me.\n\nI guess it's just a matter of terminology.  To me, it points to an\nundelivered signal, since the problem is not a zombie waiting to be\nwait-ed on but a process still alive and sleeping.\n\n> But instead of realizing its child has died, it\n> insists on waiting until the pipe is closed. Nothing has to be adopted\n> by init. There are simply still processes with the pipe open.\n\nI meant that the sleep process is not a descendant of prove any\nmore.\n\n| $ ps ax | egrep 'sleep|prove'\n| 20203 pts/3    T      0:00 vim utils/prove\n| 22654 pts/1    S+     0:00 /usr/bin/perl /usr/bin/prove --exec=bash\n| t0081-line-buffer.sh\n| 22688 pts/1    S+     0:00 sleep 100\n| 22694 pts/1    S+     0:00 sleep 100\n| 22727 pts/3    S+     0:00 egrep sleep|prove\n| $ pstree -p 22654\n| prove(22654)───bash(22655)\n\n> Did you try my 5>/dev/null patch? With it, I get no hang at all.\n\nNow I've tested it and it indeed works as advertised.\n\nSorry for the lack of clarity.\n"},{"id":"164654","messageId":"20110330033017.GA18157@sigill.intra.peff.net","threadId":"26914","inReplyTo":"20110330002921.GC14578@elie","subject":"Re: [PATCH] Portability: returning void","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-03-30T03:30:17Z","receivedAt":"2011-03-30T03:30:17Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Mar 29, 2011 at 07:29:21PM -0500, Jonathan Nieder wrote:\n\n> > Did you try my 5>/dev/null patch? With it, I get no hang at all.\n> \n> Haven't tried it yet but will try.\n> \n> I really don't like that as a long-term solution.  Yes, it gets prove\n> to stop hanging, but meanwhile we have no control over the child\n> processes we have spawned.  I'd rather just drop the tests.\n\nAgreed, it's a horrible fix. I just wanted to make sure it fixed it for\nyou, too. After thinking on it more, I figured out an elegant fix.\nPatch is below.\n\n-- >8 --\nSubject: [PATCH] t0081: kill backgrounded processes more robustly\n\nThis test creates several background processes that write to\na fifo and then go to sleep for a while (so the reader of\nthe fifo does not see EOF).\n\nEach background process is made in a curly-braced block in\nthe shell, and after we are done reading from the fifo, we\nuse \"kill $!\" to kill it off.\n\nFor a simple, single-command process, this works reliably\nand kills the child sleep process. But for more complex\ncommands like \"make_some_output && sleep\", the results are\nless predictable. When executing under bash, we end up with\na subshell that gets killed by the $! but leaves the sleep\nprocess still alive.\n\nThis is bad not only for process hygeine (we are leaving\nrandom sleep processes to expire after a while), but also\ninteracts badly with the \"prove\" command. When prove\nexecutes a test, it does not realize the test is done when\nit sees SIGCHLD, but rather waits until the test's stdout\npipe is closed. The orphaned sleep process may keep that\npipe open via test-lib's file descriptor 5, causing prove to\nhang for 100 seconds.\n\nThe solution is to explicitly use a subshell and to exec the\nfinal sleep process, so that when we \"kill $!\" we get the\nprocess id of the sleep process.\n\nWhile we're at it, let's make the \"kill\" process a little\nmore robust. Specifically:\n\n  1. Wrap the \"kill\" in a test_when_finished, since we want\n     to clean up the process whether the test succeeds or not.\n\n  2. The \"kill\" is part of our && chain for test success. It\n     probably won't fail, but it can if the process has\n     expired before we manage to kill it. So let's mark it\n     as OK to fail.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nActually, my (2) above is unlikely to trigger, since the test would have\nto hang for 100 seconds first, which probably means it is failing\nanyway. But I did run across it when I screwed up my fix.\n\nAlso, is it just me, or does it seem weird that test_when_finished\nblocks failing can produce test failure? I would think you would be able\nto put any old cleanup crap into them and not care whether it worked or\nnot.\n\n t/t0081-line-buffer.sh |   18 +++++++++---------\n 1 files changed, 9 insertions(+), 9 deletions(-)\n\ndiff --git a/t/t0081-line-buffer.sh b/t/t0081-line-buffer.sh\nindex 1dbe1c9..416ccc1 100755\n--- a/t/t0081-line-buffer.sh\n+++ b/t/t0081-line-buffer.sh\n@@ -47,16 +47,16 @@ long_read_test () {\n \trm -f input &&\n \tmkfifo input &&\n \t{\n-\t\t{\n+\t\t(\n \t\t\tgenerate_tens_of_lines $tens_of_lines \"$line\" &&\n-\t\t\tsleep 100\n-\t\t} >input &\n+\t\t\texec sleep 100\n+\t\t) >input &\n \t} &&\n+\ttest_when_finished \"kill $! || true\" &&\n \ttest-line-buffer input <<-EOF >output &&\n \tbinary $readsize\n \tcopy $copysize\n \tEOF\n-\tkill $! &&\n \ttest_line_count = $lines output &&\n \ttail -n 1 <output >actual &&\n \ttest_cmp expect actual\n@@ -86,11 +86,11 @@ test_expect_success PIPE '0-length read, no input available' '\n \t{\n \t\tsleep 100 >input &\n \t} &&\n+\ttest_when_finished \"kill $! || true\" &&\n \ttest-line-buffer input <<-\\EOF >actual &&\n \tbinary 0\n \tcopy 0\n \tEOF\n-\tkill $! &&\n \ttest_cmp expect actual\n '\n \n@@ -109,17 +109,17 @@ test_expect_success PIPE '1-byte read, no input available' '\n \trm -f input &&\n \tmkfifo input &&\n \t{\n-\t\t{\n+\t\t(\n \t\t\tprintf \"%s\" a &&\n \t\t\tprintf \"%s\" b &&\n-\t\t\tsleep 100\n-\t\t} >input &\n+\t\t\texec sleep 100\n+\t\t) >input &\n \t} &&\n+\ttest_when_finished \"kill $! || true\" &&\n \ttest-line-buffer input <<-\\EOF >actual &&\n \tbinary 1\n \tcopy 1\n \tEOF\n-\tkill $! &&\n \ttest_cmp expect actual\n '\n \n-- \n1.7.4.2.7.g9407\n"},{"id":"164656","messageId":"20110330035733.GA2793@elie","threadId":"26914","inReplyTo":"20110330033017.GA18157@sigill.intra.peff.net","subject":"Re: [PATCH] Portability: returning void","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2011-03-30T03:57:33Z","receivedAt":"2011-03-30T03:57:33Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Jeff King wrote:\n\n> While we're at it, let's make the \"kill\" process a little\n> more robust. Specifically:\n>\n>   1. Wrap the \"kill\" in a test_when_finished, since we want\n>      to clean up the process whether the test succeeds or not.\n>\n>   2. The \"kill\" is part of our && chain for test success. It\n>      probably won't fail, but it can if the process has\n>      expired before we manage to kill it. So let's mark it\n>      as OK to fail.\n>\n> Signed-off-by: Jeff King <peff@peff.net>\n> ---\n> Actually, my (2) above is unlikely to trigger, since the test would have\n> to hang for 100 seconds first, which probably means it is failing\n> anyway. But I did run across it when I screwed up my fix.\n\nThat is actually how the tests distinguish between success and\nfailure.  Any idea about a better way?\n\nI was in the process of writing a commit message for the same \"exec\"\ntrick, but I'm glad you got to it first. ;-)\n\n> Also, is it just me, or does it seem weird that test_when_finished\n> blocks failing can produce test failure? I would think you would be able\n> to put any old cleanup crap into them and not care whether it worked or\n> not.\n\nI'm generally happy that it catches mistakes in cleanup code, but I\ncould easily be convinced to change it anyway.\n\n> --- a/t/t0081-line-buffer.sh\n> +++ b/t/t0081-line-buffer.sh\n> @@ -47,16 +47,16 @@ long_read_test () {\n[...]\n> +\t\t) >input &\n>  \t} &&\n> +\ttest_when_finished \"kill $! || true\" &&\n\nThe $! gets expanded when this is executed, so later background\nprocesses won't interfere.  Good.\n\nMaybe it would be good to factor out a helper to make future\nimprovements a little easier, like so:\n\n-- 8< --\nSubject: t0081: introduce helper to fill a pipe in the background\n\nThe fill_input function generates a fifo and runs a command to write\nto it and wait a while.  The intended use is to test programs that\nneed to be able to cope with input of limited length without relying\non EOF or SIGPIPE to detect its end.\n\nFor example:\n\n\tfill_input \"echo hi\" &&\n\thead -1 input\n\nwill succeed, while\n\n\tfill_input \"echo hi\" &&\n\thead -2 input\n\nwill hang waiting for more input.\n\nIt works by running the indicated commands followed by\n\"exec sleep 100\" in a background process and registering a clean-up\ncommand to kill it and check if it was killed by SIGPIPE (good) or\nran to completion (bad).  This patch adds a definition for this\nfunction and updates existing tests that used that trick to use it.\n\nSigned-off-by: Jonathan Nieder <jrnieder@gmail.com>\n---\nThe commit message assumes test_when_finished \"kill $!\", while the\npatch uses \"kill $! || :\"; one of the two would need to be fixed\nbefore applying this.\n\n t/t0081-line-buffer.sh |   51 +++++++++++++++++++----------------------------\n 1 files changed, 21 insertions(+), 30 deletions(-)\n\ndiff --git a/t/t0081-line-buffer.sh b/t/t0081-line-buffer.sh\nindex 416ccc1..218da98 100755\n--- a/t/t0081-line-buffer.sh\n+++ b/t/t0081-line-buffer.sh\n@@ -14,6 +14,21 @@ correctly.\n \n test -n \"$GIT_REMOTE_SVN_TEST_BIG_FILES\" && test_set_prereq EXPENSIVE\n \n+fill_input () {\n+\tif ! test_declared_prereq PIPE\n+\tthen\n+\t\techo >&4 \"fill_input: need to declare PIPE prerequisite\"\n+\t\treturn 127\n+\tfi &&\n+\trm -f input &&\n+\tmkfifo input &&\n+\t(\n+\t\teval \"$*\" &&\n+\t\texec sleep 100\n+\t) >input &\n+\ttest_when_finished \"kill $! || true\"\n+}\n+\n generate_tens_of_lines () {\n \ttens=$1 &&\n \tline=$2 &&\n@@ -35,24 +50,11 @@ long_read_test () {\n \tline=abcdefghi &&\n \techo \"$line\" >expect &&\n \n-\tif ! test_declared_prereq PIPE\n-\tthen\n-\t\techo >&4 \"long_read_test: need to declare PIPE prerequisite\"\n-\t\treturn 127\n-\tfi &&\n \ttens_of_lines=$(($1 / 100 + 1)) &&\n \tlines=$(($tens_of_lines * 10)) &&\n \treadsize=$((($lines - 1) * 10 + 3)) &&\n \tcopysize=7 &&\n-\trm -f input &&\n-\tmkfifo input &&\n-\t{\n-\t\t(\n-\t\t\tgenerate_tens_of_lines $tens_of_lines \"$line\" &&\n-\t\t\texec sleep 100\n-\t\t) >input &\n-\t} &&\n-\ttest_when_finished \"kill $! || true\" &&\n+\tfill_input \"generate_tens_of_lines $tens_of_lines $line\" &&\n \ttest-line-buffer input <<-EOF >output &&\n \tbinary $readsize\n \tcopy $copysize\n@@ -81,12 +83,7 @@ test_expect_success 'hello world' '\n \n test_expect_success PIPE '0-length read, no input available' '\n \tprintf \">\" >expect &&\n-\trm -f input &&\n-\tmkfifo input &&\n-\t{\n-\t\tsleep 100 >input &\n-\t} &&\n-\ttest_when_finished \"kill $! || true\" &&\n+\tfill_input : &&\n \ttest-line-buffer input <<-\\EOF >actual &&\n \tbinary 0\n \tcopy 0\n@@ -106,16 +103,10 @@ test_expect_success '0-length read, send along greeting' '\n \n test_expect_success PIPE '1-byte read, no input available' '\n \tprintf \">%s\" ab >expect &&\n-\trm -f input &&\n-\tmkfifo input &&\n-\t{\n-\t\t(\n-\t\t\tprintf \"%s\" a &&\n-\t\t\tprintf \"%s\" b &&\n-\t\t\texec sleep 100\n-\t\t) >input &\n-\t} &&\n-\ttest_when_finished \"kill $! || true\" &&\n+\tfill_input \"\n+\t\tprintf a &&\n+\t\tprintf b\n+\t\" &&\n \ttest-line-buffer input <<-\\EOF >actual &&\n \tbinary 1\n \tcopy 1\n-- \n1.7.4.2\n"},{"id":"164657","messageId":"20110330041339.GA26281@sigill.intra.peff.net","threadId":"26914","inReplyTo":"20110330035733.GA2793@elie","subject":"Re: [PATCH] Portability: returning void","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-03-30T04:13:39Z","receivedAt":"2011-03-30T04:13:39Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Mar 29, 2011 at 10:57:33PM -0500, Jonathan Nieder wrote:\n\n> Jeff King wrote:\n> \n> > While we're at it, let's make the \"kill\" process a little\n> > more robust. Specifically:\n> >\n> >   1. Wrap the \"kill\" in a test_when_finished, since we want\n> >      to clean up the process whether the test succeeds or not.\n> >\n> >   2. The \"kill\" is part of our && chain for test success. It\n> >      probably won't fail, but it can if the process has\n> >      expired before we manage to kill it. So let's mark it\n> >      as OK to fail.\n> >\n> > Signed-off-by: Jeff King <peff@peff.net>\n> > ---\n> > Actually, my (2) above is unlikely to trigger, since the test would have\n> > to hang for 100 seconds first, which probably means it is failing\n> > anyway. But I did run across it when I screwed up my fix.\n> \n> That is actually how the tests distinguish between success and\n> failure.  Any idea about a better way?\n\nAh. I was trying not to look too hard at how the tests actually work, so\nI completely missed that. Yes, if the \"kill\" is part of the actual test,\nthen it should stay in the test. We can also put in a test_when_finished\nto kill it should the test fail to make it that far. If the cleanup one\ndoes an extra kill, it shouldn't be a big deal.\n\n> I was in the process of writing a commit message for the same \"exec\"\n> trick, but I'm glad you got to it first. ;-)\n\nI don't know why I didn't think of it sooner. :)\n\n> > Also, is it just me, or does it seem weird that test_when_finished\n> > blocks failing can produce test failure? I would think you would be able\n> > to put any old cleanup crap into them and not care whether it worked or\n> > not.\n> \n> I'm generally happy that it catches mistakes in cleanup code, but I\n> could easily be convinced to change it anyway.\n\nI don't think it's a big deal. It just surprised me.\n\n> Maybe it would be good to factor out a helper to make future\n> improvements a little easier, like so:\n> \n> -- 8< --\n> Subject: t0081: introduce helper to fill a pipe in the background\n> \n> The fill_input function generates a fifo and runs a command to write\n> to it and wait a while.  The intended use is to test programs that\n> need to be able to cope with input of limited length without relying\n> on EOF or SIGPIPE to detect its end.\n\nYeah, that looks much nicer. Do you want to just re-roll that on top of\nwhat's in 'master', with the \"exec\" magic and the defensive\ntest_when_finished in it (or as a separate patch on top if you want to\nsplit the refactor and fix)? Feel free to borrow from my commit message.\n\n-Peff\n"},{"id":"164658","messageId":"20110330044116.GB2793@elie","threadId":"26914","inReplyTo":"20110330033017.GA18157@sigill.intra.peff.net","subject":"[PULL svn-fe] Re: Portability: returning void","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2011-03-30T04:41:16Z","receivedAt":"2011-03-30T04:41:16Z","isPatch":false,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Jeff King wrote:\n\n> After thinking on it more, I figured out an elegant fix.\n\nAll right, thanks again for this.\n\nI really do want to address your \"while at it\".  I'm pretty\nuncomfortable with the \"sleep 100\" --- it might be better to do \"exec\nperl -e sleep\" so there is no chance that some incredibly slow system\nwon't make the race.  Or a failure mode that does not involve a long\nhang would be even better.\n\nAnyway, to make others' life better quickly I've pushed out the easy\npart of your fix.  It sits with the fixes to other embarrasing bugs.\n\n  git://repo.or.cz/git/jrn.git svn-fe\n\nJeff King (1):\n      tests: kill backgrounded processes more robustly\n\nJonathan Nieder (2):\n      vcs-svn: add missing cast to printf argument\n      tests: make sure input to sed is newline terminated\n\nMichael Witten (1):\n      vcs-svn: a void function shouldn't try to return something\n\n t/t0081-line-buffer.sh |   12 ++++++------\n t/t9010-svn-fe.sh      |    8 ++++++--\n vcs-svn/fast_export.c  |    3 ++-\n vcs-svn/svndump.c      |    3 ++-\n 4 files changed, 16 insertions(+), 10 deletions(-)\n"},{"id":"164663","messageId":"4D92D3A8.3090301@viscovery.net","threadId":"26914","inReplyTo":"20110330041339.GA26281@sigill.intra.peff.net","subject":"Re: [PATCH] Portability: returning void","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2011-03-30T06:54:32Z","receivedAt":"2011-03-30T06:54:32Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 3/30/2011 6:13, schrieb Jeff King:\n> On Tue, Mar 29, 2011 at 10:57:33PM -0500, Jonathan Nieder wrote:\n>> I was in the process of writing a commit message for the same \"exec\"\n>> trick, but I'm glad you got to it first. ;-)\n> \n> I don't know why I didn't think of it sooner. :)\n\nNote that tests that depend on ( exec ... ) & kill $! must be marked with\nthe EXECKEEPSPID prerequisite.\n\n-- Hannes\n"},{"id":"164667","messageId":"20110330081643.GD2793@elie","threadId":"26914","inReplyTo":"4D92D3A8.3090301@viscovery.net","subject":"[PATCH/RFC svn-fe] tests: introduce helper to fill a pipe in the background","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2011-03-30T08:16:43Z","receivedAt":"2011-03-30T08:16:43Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"The fill_input function generates a fifo and runs a command to write\nto it and wait.  The intended use is to check that specialized\nprograms can find the end of their input without reading too much or\nrelying on EOF or SIGPIPE.  For example:\n\n\tfill_input \"echo hi\" &&\n\thead -1 input\n\nwill succeed, while\n\n\tfill_input \"echo hi\" &&\n\thead -2 input\n\nwill hang.\n\nIt works by running the indicated commands followed by\n\"exec sleep 100\" in a background process; the process ID is later\npassed to \"kill\" to avoid leaving behind random processes.\n\nSeveral tests already did something like that but this adds some\nimprovements:\n\n  1. Wrap the \"kill\" in a test_when_finished, since we want\n     to clean up the process whether the test succeeds or not.\n\n  2. Mark the kill as OK to fail.  These tests are not about\n     whether the input generating function dies due to SIGPIPE\n     or the system is slow enough for the timer to expire early\n     but about what happens on the downstream end.\n\n  3. Mark the relevant tests with the EXECKEEPSPID prerequisite.\n\nBased-on-patch-by: Jeff King <peff@peff.net>\nImproved-by: Johannes Sixt <j6t@kdbg.org>\nSigned-off-by: Jonathan Nieder <jrnieder@gmail.com>\n---\nJohannes Sixt wrote:\n\n> Note that tests that depend on ( exec ... ) & kill $! must be marked with\n> the EXECKEEPSPID prerequisite.\n\nHmm, that's a shame, since the point of writing t0081 was to make sure\nsome assumptions the line-buffer lib makes are valid on Windows.  So I\nsuspect a better thing to do would be to remove t0081 --- the\nline-buffer lib is simple, everyday use exercises it pretty well\nalready, and I can't imagine this script catching a bug.\n\n t/t0081-line-buffer.sh |   61 ++++++++++++++++++++++--------------------------\n 1 files changed, 28 insertions(+), 33 deletions(-)\n\ndiff --git a/t/t0081-line-buffer.sh b/t/t0081-line-buffer.sh\nindex 5067d1e..fb09ff1 100755\n--- a/t/t0081-line-buffer.sh\n+++ b/t/t0081-line-buffer.sh\n@@ -14,6 +14,25 @@ correctly.\n \n test -n \"$GIT_REMOTE_SVN_TEST_BIG_FILES\" && test_set_prereq EXPENSIVE\n \n+fill_input () {\n+\tif\n+\t\t! test_declared_prereq PIPE ||\n+\t\t! test_declared_prereq EXECKEEPSPID\n+\tthen\n+\t\techo >&4 \"fill_input requires PIPE,EXECKEEPSPID prerequisites\"\n+\t\treturn 127\n+\tfi &&\n+\trm -f input &&\n+\tmkfifo input &&\n+\t{\n+\t\t(\n+\t\t\teval \"$*\" &&\n+\t\t\texec sleep 100\n+\t\t) >input &\n+\t} &&\n+\ttest_when_finished \"kill $!\"\n+}\n+\n generate_tens_of_lines () {\n \ttens=$1 &&\n \tline=$2 &&\n@@ -35,28 +54,15 @@ long_read_test () {\n \tline=abcdefghi &&\n \techo \"$line\" >expect &&\n \n-\tif ! test_declared_prereq PIPE\n-\tthen\n-\t\techo >&4 \"long_read_test: need to declare PIPE prerequisite\"\n-\t\treturn 127\n-\tfi &&\n \ttens_of_lines=$(($1 / 100 + 1)) &&\n \tlines=$(($tens_of_lines * 10)) &&\n \treadsize=$((($lines - 1) * 10 + 3)) &&\n \tcopysize=7 &&\n-\trm -f input &&\n-\tmkfifo input &&\n-\t{\n-\t\t(\n-\t\t\tgenerate_tens_of_lines $tens_of_lines \"$line\" &&\n-\t\t\texec sleep 100\n-\t\t) >input &\n-\t} &&\n+\tfill_input \"generate_tens_of_lines $tens_of_lines $line\" &&\n \ttest-line-buffer input <<-EOF >output &&\n \tbinary $readsize\n \tcopy $copysize\n \tEOF\n-\tkill $! &&\n \ttest_line_count = $lines output &&\n \ttail -n 1 <output >actual &&\n \ttest_cmp expect actual\n@@ -79,18 +85,13 @@ test_expect_success 'hello world' '\n \ttest_cmp expect actual\n '\n \n-test_expect_success PIPE '0-length read, no input available' '\n+test_expect_success PIPE,EXECKEEPSPID '0-length read, no input available' '\n \tprintf \">\" >expect &&\n-\trm -f input &&\n-\tmkfifo input &&\n-\t{\n-\t\tsleep 100 >input &\n-\t} &&\n+\tfill_input : &&\n \ttest-line-buffer input <<-\\EOF >actual &&\n \tbinary 0\n \tcopy 0\n \tEOF\n-\tkill $! &&\n \ttest_cmp expect actual\n '\n \n@@ -104,26 +105,20 @@ test_expect_success '0-length read, send along greeting' '\n \ttest_cmp expect actual\n '\n \n-test_expect_success PIPE '1-byte read, no input available' '\n+test_expect_success PIPE,EXECKEEPSPID '1-byte read, no input available' '\n \tprintf \">%s\" ab >expect &&\n-\trm -f input &&\n-\tmkfifo input &&\n-\t{\n-\t\t(\n-\t\t\tprintf \"%s\" a &&\n-\t\t\tprintf \"%s\" b &&\n-\t\t\texec sleep 100\n-\t\t) >input &\n-\t} &&\n+\tfill_input \"\n+\t\tprintf a &&\n+\t\tprintf b\n+\t\" &&\n \ttest-line-buffer input <<-\\EOF >actual &&\n \tbinary 1\n \tcopy 1\n \tEOF\n-\tkill $! &&\n \ttest_cmp expect actual\n '\n \n-test_expect_success PIPE 'long read (around 8192 bytes)' '\n+test_expect_success PIPE,EXECKEEPSPID 'long read (around 8192 bytes)' '\n \tlong_read_test 8192\n '\n \n-- \n1.7.4.2\n"},{"id":"164669","messageId":"20110330084148.GE2793@elie","threadId":"26914","inReplyTo":"20110330041339.GA26281@sigill.intra.peff.net","subject":"Re: [PATCH] Portability: returning void","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2011-03-30T08:41:48Z","receivedAt":"2011-03-30T08:41:48Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Jeff King wrote:\n\n> Ah. I was trying not to look too hard at how the tests actually work, so\n> I completely missed that. Yes, if the \"kill\" is part of the actual test,\n> then it should stay in the test.\n\nHeh, what was the person writing that test thinking?  Allowing the\nkill to fail would be better --- some future test might have the\ngenerate_lots_of_text part dying of SIGPIPE, which would make later\nattempts to kill it fail.  Totally cryptic.\n\nWhat are these tests make to check, anyway?\n\n - \"0-length read, no input available\" makes some sense.  It's checking\n   that the platform fread can handle requests to read nothing (or that\n   line-buffer works around it if some esoteric platform cannot).\n\n - \"1-byte read, no input available\" makes less sense.  I just can't\n   see any reason for it to fail.  And it \"tests\" two functions at\n   once, for crazy historical reasons.\n\n - \"long read\" checks that we can read something around the pipe size\n   and not get totally confused.  I can imagine it failing but it\n   wouldn't be git's fault.  It also doesn't seem like a useful test.\n\nI don't think any of them is worth the fuss.  So how about this?\n\n-- 8< --\nSubject: Revert \"t0081 (line-buffer): add buffering tests\"\n\nThis (morally) reverts commit d280f68313eecb8b3838c70641a246382d5e5343,\nwhich added some tests that are a pain to maintain and are not likely\nto find bugs in git.\n\nHelped-by: Jeff King <peff@peff.net>\nHelped-by: Johannes Sixt <j6t@kdbg.org>\nSigned-off-by: Jonathan Nieder <jrnieder@gmail.com>\n---\n t/t0081-line-buffer.sh |  106 +-----------------------------------------------\n 1 files changed, 2 insertions(+), 104 deletions(-)\n\ndiff --git a/t/t0081-line-buffer.sh b/t/t0081-line-buffer.sh\nindex 5067d1e..bd83ed3 100755\n--- a/t/t0081-line-buffer.sh\n+++ b/t/t0081-line-buffer.sh\n@@ -2,74 +2,14 @@\n \n test_description=\"Test the svn importer's input handling routines.\n \n-These tests exercise the line_buffer library, but their real purpose\n-is to check the assumptions that library makes of the platform's input\n-routines.  Processes engaged in bi-directional communication would\n-hang if fread or fgets is too greedy.\n+These tests provide some simple checks that the line_buffer API\n+behaves as advertised.\n \n While at it, check that input of newlines and null bytes are handled\n correctly.\n \"\n . ./test-lib.sh\n \n-test -n \"$GIT_REMOTE_SVN_TEST_BIG_FILES\" && test_set_prereq EXPENSIVE\n-\n-generate_tens_of_lines () {\n-\ttens=$1 &&\n-\tline=$2 &&\n-\n-\ti=0 &&\n-\twhile test $i -lt \"$tens\"\n-\tdo\n-\t\tfor j in a b c d e f g h i j\n-\t\tdo\n-\t\t\techo \"$line\"\n-\t\tdone &&\n-\t\t: $((i = $i + 1)) ||\n-\t\treturn\n-\tdone\n-}\n-\n-long_read_test () {\n-\t: each line is 10 bytes, including newline &&\n-\tline=abcdefghi &&\n-\techo \"$line\" >expect &&\n-\n-\tif ! test_declared_prereq PIPE\n-\tthen\n-\t\techo >&4 \"long_read_test: need to declare PIPE prerequisite\"\n-\t\treturn 127\n-\tfi &&\n-\ttens_of_lines=$(($1 / 100 + 1)) &&\n-\tlines=$(($tens_of_lines * 10)) &&\n-\treadsize=$((($lines - 1) * 10 + 3)) &&\n-\tcopysize=7 &&\n-\trm -f input &&\n-\tmkfifo input &&\n-\t{\n-\t\t(\n-\t\t\tgenerate_tens_of_lines $tens_of_lines \"$line\" &&\n-\t\t\texec sleep 100\n-\t\t) >input &\n-\t} &&\n-\ttest-line-buffer input <<-EOF >output &&\n-\tbinary $readsize\n-\tcopy $copysize\n-\tEOF\n-\tkill $! &&\n-\ttest_line_count = $lines output &&\n-\ttail -n 1 <output >actual &&\n-\ttest_cmp expect actual\n-}\n-\n-test_expect_success 'setup: have pipes?' '\n-      rm -f frob &&\n-      if mkfifo frob\n-      then\n-\t\ttest_set_prereq PIPE\n-      fi\n-'\n-\n test_expect_success 'hello world' '\n \techo \">HELLO\" >expect &&\n \ttest-line-buffer <<-\\EOF >actual &&\n@@ -79,21 +19,6 @@ test_expect_success 'hello world' '\n \ttest_cmp expect actual\n '\n \n-test_expect_success PIPE '0-length read, no input available' '\n-\tprintf \">\" >expect &&\n-\trm -f input &&\n-\tmkfifo input &&\n-\t{\n-\t\tsleep 100 >input &\n-\t} &&\n-\ttest-line-buffer input <<-\\EOF >actual &&\n-\tbinary 0\n-\tcopy 0\n-\tEOF\n-\tkill $! &&\n-\ttest_cmp expect actual\n-'\n-\n test_expect_success '0-length read, send along greeting' '\n \techo \">HELLO\" >expect &&\n \ttest-line-buffer <<-\\EOF >actual &&\n@@ -104,33 +29,6 @@ test_expect_success '0-length read, send along greeting' '\n \ttest_cmp expect actual\n '\n \n-test_expect_success PIPE '1-byte read, no input available' '\n-\tprintf \">%s\" ab >expect &&\n-\trm -f input &&\n-\tmkfifo input &&\n-\t{\n-\t\t(\n-\t\t\tprintf \"%s\" a &&\n-\t\t\tprintf \"%s\" b &&\n-\t\t\texec sleep 100\n-\t\t) >input &\n-\t} &&\n-\ttest-line-buffer input <<-\\EOF >actual &&\n-\tbinary 1\n-\tcopy 1\n-\tEOF\n-\tkill $! &&\n-\ttest_cmp expect actual\n-'\n-\n-test_expect_success PIPE 'long read (around 8192 bytes)' '\n-\tlong_read_test 8192\n-'\n-\n-test_expect_success PIPE,EXPENSIVE 'longer read (around 65536 bytes)' '\n-\tlong_read_test 65536\n-'\n-\n test_expect_success 'read from file descriptor' '\n \trm -f input &&\n \techo hello >expect &&\n-- \n1.7.4.2.660.g270d4b.dirty\n"},{"id":"164684","messageId":"20110330124019.GA28062@sigill.intra.peff.net","threadId":"26914","inReplyTo":"20110330084148.GE2793@elie","subject":"Re: [PATCH] Portability: returning void","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-03-30T12:40:19Z","receivedAt":"2011-03-30T12:40:19Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Mar 30, 2011 at 03:41:48AM -0500, Jonathan Nieder wrote:\n\n> I don't think any of them is worth the fuss.  So how about this?\n> \n> -- 8< --\n> Subject: Revert \"t0081 (line-buffer): add buffering tests\"\n\nThat's OK with me. Should it drop test-line-buffer, too?\n\n-Peff\n"},{"id":"164707","messageId":"20110330185440.GC13440@elie","threadId":"26914","inReplyTo":"20110330124019.GA28062@sigill.intra.peff.net","subject":"Re: [PATCH] Portability: returning void","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2011-03-30T18:54:40Z","receivedAt":"2011-03-30T18:54:40Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Jeff King wrote:\n\n> That's OK with me. Should it drop test-line-buffer, too?\n\nFor now, test-line-buffer is still being used to check the API wrt\nearly EOF.\n\nIn the long term, yes, I would like to replace those tests with tests\nof svn-fe itself.  The test suite should not be an obstacle to\nchanging the line-buffer API as long as svn-fe is adjusted correctly\nat the same time.\n\nThanks for looking it over.\n"},{"id":"164715","messageId":"7vwrjgmmlp.fsf@alter.siamese.dyndns.org","threadId":"26914","inReplyTo":"20110330044116.GB2793@elie","subject":"Re: [PULL svn-fe] Re: Portability: returning void","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-03-30T19:31:30Z","receivedAt":"2011-03-30T19:31:30Z","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> I really do want to address your \"while at it\".  I'm pretty\n> uncomfortable with the \"sleep 100\" --- it might be better to do \"exec\n> perl -e sleep\" so there is no chance that some incredibly slow system\n> won't make the race.  Or a failure mode that does not involve a long\n> hang would be even better.\n>\n> Anyway, to make others' life better quickly I've pushed out the easy\n> part of your fix.  It sits with the fixes to other embarrasing bugs.\n>\n>   git://repo.or.cz/git/jrn.git svn-fe\n\nPulled, thanks.\n"}]}