{"thread":{"id":"30235","subject":"[PATCH] t5570: forward git-daemon messages in a different way","startedAt":"2012-04-14T08:44:30Z","lastAt":"2012-04-27T15:02:25Z","messageCount":20,"participants":["Zbigniew Jędrzejewski-Szmek","Clemens Buchacher","Junio C Hamano","Jeff King","Johannes Sixt"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"189253","messageId":"1334393070-7123-1-git-send-email-zbyszek@in.waw.pl","threadId":"30235","inReplyTo":null,"subject":"[PATCH] t5570: forward git-daemon messages in a different way","fromName":"Zbigniew Jędrzejewski-Szmek","fromEmail":"zbyszek@in.waw.pl","sentAt":"2012-04-14T08:44:30Z","receivedAt":"2012-04-14T08:44:30Z","isPatch":true,"sender":{"key":"zbyszek@in.waw.pl","avatar":"https://avatars.githubusercontent.com/u/349618?v=4"},"body":"git-daemon is not launched properly in t5570:\n\n$ GIT_TEST_GIT_DAEMON=t ./t5570-git-daemon.sh\nok 1 - setup repository\nok 2 - create git-accessible bare repository\nnot ok - 3 clone git repository\nnot ok - 4 fetch changes via git protocol\n...\n\nCurrent setup code to spawn git daemon (start_git_daemon() in\nlib-git-daemon.sh) redirects daemon output to a pipe, and then\nredirects input from this pipe to a different fd, which is in turn\nconnected to a terminal:\n mkfifo git_daemon_output\n git daemon ... >&3 2>git_daemon_output\n {\n ...\n cat >&4\n } \n---\n t/lib-git-daemon.sh | 5 +++--\n 1 file changed, 3 insertions(+), 2 deletions(-)\n\ndiff --git a/t/lib-git-daemon.sh b/t/lib-git-daemon.sh\nindex ef2d01f..8f9c1b6 100644\n--- a/t/lib-git-daemon.sh\n+++ b/t/lib-git-daemon.sh\n@@ -22,7 +22,7 @@ start_git_daemon() {\n \n \ttrap 'code=$?; stop_git_daemon; (exit $code); die' EXIT\n \n-\tsay >&3 \"Starting git daemon ...\"\n+\tsay >&3 \"Starting git daemon on $GIT_DAEMON_URL ...\"\n \tmkfifo git_daemon_output\n \tgit daemon --listen=127.0.0.1 --port=\"$LIB_GIT_DAEMON_PORT\" \\\n \t\t--reuseaddr --verbose \\\n@@ -33,7 +33,6 @@ start_git_daemon() {\n \t{\n \t\tread line\n \t\techo >&4 \"$line\"\n-\t\tcat >&4 &\n \n \t\t# Check expected output\n \t\tif test x\"$(expr \"$line\" : \"\\[[0-9]*\\] \\(.*\\)\")\" != x\"Ready to rumble\"\n@@ -44,6 +43,8 @@ start_git_daemon() {\n \t\t\terror \"git daemon failed to start\"\n \t\tfi\n \t} &4 &\n }\n \n stop_git_daemon() {\n-- \n1.7.10.226.gfe575\n"},{"id":"189256","messageId":"20120414121358.GA26372@ecki","threadId":"30235","inReplyTo":"1334393070-7123-1-git-send-email-zbyszek@in.waw.pl","subject":"Re: [PATCH] t5570: forward git-daemon messages in a different way","fromName":"Clemens Buchacher","fromEmail":"drizzd@aon.at","sentAt":"2012-04-14T12:13:58Z","receivedAt":"2012-04-14T12:13:58Z","isPatch":true,"sender":{"key":"drizzd@gmx.net","avatar":"https://avatars.githubusercontent.com/u/59082?v=4"},"body":"On Sat, Apr 14, 2012 at 10:44:30AM +0200, Zbigniew Jędrzejewski-Szmek wrote:\n> git-daemon is not launched properly in t5570:\n> \n> $ GIT_TEST_GIT_DAEMON=t ./t5570-git-daemon.sh\n> ok 1 - setup repository\n> ok 2 - create git-accessible bare repository\n> not ok - 3 clone git repository\n> not ok - 4 fetch changes via git protocol\n> ...\n> \n> Current setup code to spawn git daemon (start_git_daemon() in\n> lib-git-daemon.sh) redirects daemon output to a pipe, and then\n> redirects input from this pipe to a different fd, which is in turn\n> connected to a terminal:\n> mkfifo git_daemon_output\n> git daemon ... >&3 2>git_daemon_output\n> {\n> ...\n> cat >&4\n> } \n> Unfortunately, it seems that the shell (at least bash 4.1-3 from\n> debian) closes the pipe and cat doesn't really copy any messages. This\n> causes git-daemon to die.\n\nAnd as a consequence, t5570 tests fail for you? I cannot reproduce with\nbash 4.2.24(2). Which git version are you seeing this with?\n\n> Running 'strace -o log cat' instead of just 'cat' shows that no input\n> is read:\n> execve(\"/bin/cat\", ...) = 0\n> ...\n> read(0, \"\", 8192) = 0\n> close(0) = 0\n> close(1) = 0\n> close(2) = 0\n> exit_group(0) = ?\n\nWhat do you expect it to read? If git-daemon exits without error, it\ndoes not output anything.\n\n> I guess that the shell closes the redirection when exiting the\n> {}-delimited part.\n\nI am not sure about that part myself, but it seems to work for me in all\ncases.\n\n> It seems easiest to move the cat invocation outside of the\n> {}-delimited part and provide a separate redirection which will not be\n> closed.\n\nWith your patch, only the first line of output will be read from\ngit-daemon, because the pipe is broken as soon as you close the fifo for\nthe first time. You can check this by passing an invalid argument to\ngit-daemon. Only the first line of the usage string will be printed.\n\nIn order to better understand the problem on your side, can you execute\nthis script and tell me what it does for you?\n\n#!/bin/sh\n\nmkfifo fd\nyes >fd &\npid=$!\n{\n\tread line\n\techo $line\n} \n> In order to better understand the problem on your side, can you execute\n> this script and tell me what it does for you?\n\nOops, this is what I really wanted:\n\n#!/bin/sh\n\nmkfifo fd\nyes >fd &\npid=$!\n{\n\tread line\n\techo $line\n\tcat On Sat, Apr 14, 2012 at 02:13:58PM +0200, Clemens Buchacher wrote:\n>>\n>> In order to better understand the problem on your side, can you execute\n>> this script and tell me what it does for you?\n> \n> Oops, this is what I really wanted:\n> \n> #!/bin/sh\n> \n> mkfifo fd\n> yes>fd&\n> pid=$!\n> {\n> \tread line\n> \techo $line\n> \tcat } sleep 1\n> kill $pid\n> wait $pid\n> rm -f fd\n\nHi,\nmany thanks for looking into this. I'm sorry I didn't reply sooner,\nbut I was away for the weekend.\n\n> And as a consequence, t5570 tests fail for you? I cannot reproduce with\n> bash 4.2.24(2). Which git version are you seeing this with?\nYes. Example test output is:\n\n----(on master 146fe8ce2)------------------------------------------------------------------\n$ (cd t && GIT_TEST_GIT_DAEMON=t ./t5570*sh)\nok 1 - setup repository\nok 2 - create git-accessible bare repository\nnot ok - 3 clone git repository\n#\n# git clone \"$GIT_DAEMON_URL/repo.git\" clone &&\n# test_cmp file clone/file\n#\nnot ok - 4 fetch changes via git protocol\n#\n# echo content >>file &&\n# git commit -a -m two &&\n# git push public &&\n# (cd clone && git pull) &&\n# test_cmp file clone/file\n#\nnot ok 5 - remote detects correct HEAD # TODO known breakage\nok 6 - prepare pack objects\nok 7 - fetch notices corrupt pack\nok 8 - fetch notices corrupt idx\nnot ok - 9 clone non-existent\n# test_remote_error clone nowhere.git 'access denied or repository not exported'\nnot ok - 10 push disabled\n# test_remote_error push repo.git 'access denied or repository not exported'\nnot ok - 11 read access denied\n# test_remote_error -x fetch repo.git 'access denied or repository not exported'\nnot ok - 12 not exported\n# test_remote_error -n fetch repo.git 'access denied or repository not exported'\n./t5570-git-daemon.sh: 59: kill: No such process\n\nerror: git daemon exited with status: 141\n-----------------------------------------------------------------------------------------\n\nOK, I run your test scripts and found the problem (test.sh is the first\nversion, and test2.sh is the second version with 'cat' inside {}). Yikes!\nI have /bin/sh symlinked to dash, and dash behaves differently:\n\n% bash -x test2.sh | wc -l\n+ mkfifo fd\n+ pid=10685\n+ yes\n+ read line\n+ echo y\n+ sleep 1\n+ cat\n+ kill 10685\n+ wait 10685\ntest2.sh: line 13: 10685 Terminated yes > fd\n+ rm -f fd\n45400064\n\n% dash -x test2.sh | wc -l\n+ mkfifo fd\n+ pid=10738\n+ read line\n+ yes\n+ echo y\n+ sleep 1\n+ kill 10738\ntest2.sh: 12: kill: No such process\n\n+ wait 10738\n+ rm -f fd\n^C\n\nIt hangs at the end until killed with ^C. This seem to happen fairly reliably\n(nineteen times out of twenty or so). This is with dash 0.5.7-3 and 0.5.5.1-7.4\nfrom debian. With bash test2.sh seems to always run successfully.\n\nI also run test.sh for comparison, and dash runs test.sh successfully\nevery once in a while, and test.sh always fails with bash.\n\nSo my patch was totally bogus, it was just probably changing the timing.\n\nNow your patches (on top of next):\n'git-daemon wrapper to wait until daemon is ready' fixes the problem, thanks!\n\n(I now see that they are both in pu: pu runs fine too.)\n\nThanks,\nZbyszek\n"},{"id":"189435","messageId":"7vmx6bwqmj.fsf@alter.siamese.dyndns.org","threadId":"30235","inReplyTo":"4F8C3E0F.2040300@in.waw.pl","subject":"Re: [PATCH] t5570: forward git-daemon messages in a different way","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-04-16T17:09:08Z","receivedAt":"2012-04-16T17:09:08Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Zbigniew Jędrzejewski-Szmek writes:\n\n> So my patch was totally bogus, it was just probably changing the timing.\n>\n> Now your patches (on top of next):\n> 'git-daemon wrapper to wait until daemon is ready' fixes the problem, thanks!\n>\n> (I now see that they are both in pu: pu runs fine too.)\n\nSorry, I think one of the \"both\" you mean is 7122c9e (git-daemon wrapper\nto wait until daemon is ready, 2012-04-15), but which one is \"the other\none\" (which I should discard)?\n"},{"id":"189440","messageId":"20120416174230.GA19226@sigill.intra.peff.net","threadId":"30235","inReplyTo":"4F8C3E0F.2040300@in.waw.pl","subject":"Re: [PATCH] t5570: forward git-daemon messages in a different way","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-04-16T17:42:30Z","receivedAt":"2012-04-16T17:42:30Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Apr 16, 2012 at 05:43:11PM +0200, Zbigniew Jędrzejewski-Szmek wrote:\n\n> % dash -x test2.sh | wc -l\n> + mkfifo fd\n> + pid=10738\n> + read line\n> + yes\n> + echo y\n> + sleep 1\n> + kill 10738\n> test2.sh: 12: kill: No such process\n> \n> + wait 10738\n> + rm -f fd\n> ^C\n> \n> It hangs at the end until killed with ^C. This seem to happen fairly reliably\n> (nineteen times out of twenty or so). This is with dash 0.5.7-3 and 0.5.5.1-7.4\n> from debian. With bash test2.sh seems to always run successfully.\n\nHmm. t5570 seems to pass reliably on dash for me with:\n\ndiff --git a/t/lib-git-daemon.sh b/t/lib-git-daemon.sh\nindex ef2d01f..9f52cb6 100644\n--- a/t/lib-git-daemon.sh\n+++ b/t/lib-git-daemon.sh\n@@ -33,7 +33,7 @@ start_git_daemon() {\n \t{\n \t\tread line\n \t\techo >&4 \"$line\"\n-\t\tcat >&4 &\n+\t\tcat >&4 Zbigniew Jędrzejewski-Szmek writes:\n> \n> > So my patch was totally bogus, it was just probably changing the timing.\n> >\n> > Now your patches (on top of next):\n> > 'git-daemon wrapper to wait until daemon is ready' fixes the problem, thanks!\n> >\n> > (I now see that they are both in pu: pu runs fine too.)\n> \n> Sorry, I think one of the \"both\" you mean is 7122c9e (git-daemon wrapper\n> to wait until daemon is ready, 2012-04-15), but which one is \"the other\n> one\" (which I should discard)?\n\nI believe he is referring to 1bcb0ab4 (t5570: use explicit push\nrefspec), which is also necessary to run the tests on pu.\n"},{"id":"189473","messageId":"4F8C9801.9030607@in.waw.pl","threadId":"30235","inReplyTo":"20120416212215.GA5351@ecki","subject":"Re: [PATCH] t5570: forward git-daemon messages in a different way","fromName":"Zbigniew Jędrzejewski-Szmek","fromEmail":"zbyszek@in.waw.pl","sentAt":"2012-04-16T22:06:57Z","receivedAt":"2012-04-16T22:06:57Z","isPatch":true,"sender":{"key":"zbyszek@in.waw.pl","avatar":"https://avatars.githubusercontent.com/u/349618?v=4"},"body":"On 04/16/2012 11:22 PM, Clemens Buchacher wrote:\n> On Mon, Apr 16, 2012 at 10:09:08AM -0700, Junio C Hamano wrote:\n>> Zbigniew Jędrzejewski-Szmek writes:\n>>\n>>> So my patch was totally bogus, it was just probably changing the timing.\n>>>\n>>> Now your patches (on top of next):\n>>> 'git-daemon wrapper to wait until daemon is ready' fixes the problem, thanks!\n>>>\n>>> (I now see that they are both in pu: pu runs fine too.)\n>>\n>> Sorry, I think one of the \"both\" you mean is 7122c9e (git-daemon wrapper\n>> to wait until daemon is ready, 2012-04-15), but which one is \"the other\n>> one\" (which I should discard)?\n>\n> I believe he is referring to 1bcb0ab4 (t5570: use explicit push\n> refspec), which is also necessary to run the tests on pu.\nExactly.\n\n-\nZbyszek\n"},{"id":"189489","messageId":"20120416224424.GA10314@ecki","threadId":"30235","inReplyTo":"20120416174230.GA19226@sigill.intra.peff.net","subject":"Re: [PATCH] t5570: forward git-daemon messages in a different way","fromName":"Clemens Buchacher","fromEmail":"drizzd@aon.at","sentAt":"2012-04-16T22:44:25Z","receivedAt":"2012-04-16T22:44:25Z","isPatch":true,"sender":{"key":"drizzd@gmx.net","avatar":"https://avatars.githubusercontent.com/u/59082?v=4"},"body":"On Mon, Apr 16, 2012 at 01:42:30PM -0400, Jeff King wrote:\n> \n> Hmm. t5570 seems to pass reliably on dash for me with:\n> \n> diff --git a/t/lib-git-daemon.sh b/t/lib-git-daemon.sh\n> index ef2d01f..9f52cb6 100644\n> --- a/t/lib-git-daemon.sh\n> +++ b/t/lib-git-daemon.sh\n> @@ -33,7 +33,7 @@ start_git_daemon() {\n> \t{\n> \t\tread line\n> \t\techo >&4 \"$line\"\n> -\t\tcat >&4 &\n> +\t\tcat >&4 \n> \t\t# Check expected output\n> \t\tif test x\"$(expr \"$line\" : \"\\[[0-9]*\\] \\(.*\\)\")\" != x\"Ready to rumble\"\n\nYes, me too. I can reproduce reliably with dash and the above fixes it\nreliably.\n\n> But the test above does fail.\n\nWhich one do you mean? The output check works for me.\n\n> Is it purely luck of the timing that git-daemon never gets SIGPIPE? I\n> guess the problem is that the {}-section can finish before \"cat\n> I'd just feel better about the solution if we were sure we understood\n> the exact problem.\n\nYeah. I have to admit that I have a strongly empirical approach to these\nthings and no true understanding of the inner workings.\n"},{"id":"189529","messageId":"7vy5punz31.fsf@alter.siamese.dyndns.org","threadId":"30235","inReplyTo":"4F8C9801.9030607@in.waw.pl","subject":"Re: [PATCH] t5570: forward git-daemon messages in a different way","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-04-17T15:43:30Z","receivedAt":"2012-04-17T15:43:30Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Zbigniew Jędrzejewski-Szmek writes:\n\n>>> Sorry, I think one of the \"both\" you mean is 7122c9e (git-daemon wrapper\n>>> to wait until daemon is ready, 2012-04-15), but which one is \"the other\n>>> one\" (which I should discard)?\n>>\n>> I believe he is referring to 1bcb0ab4 (t5570: use explicit push\n>> refspec), which is also necessary to run the tests on pu.\n> Exactly.\n\nThanks both for clarifications..\n"},{"id":"189682","messageId":"20120419060326.GA13982@sigill.intra.peff.net","threadId":"30235","inReplyTo":"20120416224424.GA10314@ecki","subject":"Re: [PATCH] t5570: forward git-daemon messages in a different way","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-04-19T06:03:26Z","receivedAt":"2012-04-19T06:03:26Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Apr 17, 2012 at 12:44:25AM +0200, Clemens Buchacher wrote:\n\n> On Mon, Apr 16, 2012 at 01:42:30PM -0400, Jeff King wrote:\n> > \n> > Hmm. t5570 seems to pass reliably on dash for me with:\n> > \n> > diff --git a/t/lib-git-daemon.sh b/t/lib-git-daemon.sh\n> > index ef2d01f..9f52cb6 100644\n> > --- a/t/lib-git-daemon.sh\n> > +++ b/t/lib-git-daemon.sh\n> > @@ -33,7 +33,7 @@ start_git_daemon() {\n> > \t{\n> > \t\tread line\n> > \t\techo >&4 \"$line\"\n> > -\t\tcat >&4 &\n> > +\t\tcat >&4 > \n> > \t\t# Check expected output\n> > \t\tif test x\"$(expr \"$line\" : \"\\[[0-9]*\\] \\(.*\\)\")\" != x\"Ready to rumble\"\n> \n> Yes, me too. I can reproduce reliably with dash and the above fixes it\n> reliably.\n> \n> > But the test above does fail.\n> \n> Which one do you mean? The output check works for me.\n\nSorry, I meant the test you posted with \"yes\":\n\nmkfifo fd\nyes >fd &\npid=$!\n{\n read line\n echo $line\n cat > Is it purely luck of the timing that git-daemon never gets SIGPIPE? I\n> > guess the problem is that the {}-section can finish before \"cat\n> > \n> No clue. But shouldn't the fork return only after the fd's have been\n> opened successfully? If I change cat to \"(echo di; cat; echo do); sleep\n> 1; pgrep yes\", then one can see that cat terminates right away, even\n> though yes is still running. It's as if cat never gets to read from the\n> pipe, but from /dev/null instead. A bug in dash?\n\nHmm. Yeah, if you strace the cat, it gets an immediate EOF. And even\nweirder, I notice this in the strace output:\n\n clone(...)\n close(0) = 0\n open(\"/dev/null\", O_RDONLY) = 0\n ...\n execve(\"/bin/cat\", [\"cat\"], [/* 50 vars */]) = 0\n\nWhat? The shell is literally redirecting the cat process's stdin from\n/dev/null. I'm totally confused. If you do \"cat mkfifo fd\n> yes >fd &\n> pid=$!\n> {\n> read line\n> echo $line\n> cat } sleep 1\n> kill $pid\n> wait $pid\n> rm -f fd\n...\n> Hmm. Yeah, if you strace the cat, it gets an immediate EOF. And even\n> weirder, I notice this in the strace output:\n> \n> clone(...)\n> close(0) = 0\n> open(\"/dev/null\", O_RDONLY) = 0\n> ...\n> execve(\"/bin/cat\", [\"cat\"], [/* 50 vars */]) = 0\n> \n> What? The shell is literally redirecting the cat process's stdin from\n> /dev/null. I'm totally confused.\n\nYou don't have to be; it's mandated by POSIX:\n\nhttp://pubs.opengroup.org/onlinepubs/9699919799/utilities/V3_chap02.html#tag_18_09_03_02\n\n-- Hannes\n"},{"id":"190109","messageId":"20120426130129.GA27785@sigill.intra.peff.net","threadId":"30235","inReplyTo":"4F8FB779.60004@viscovery.net","subject":"Re: [PATCH] t5570: forward git-daemon messages in a different way","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-04-26T13:01:29Z","receivedAt":"2012-04-26T13:01:29Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Apr 19, 2012 at 08:58:01AM +0200, Johannes Sixt wrote:\n\n> > Hmm. Yeah, if you strace the cat, it gets an immediate EOF. And even\n> > weirder, I notice this in the strace output:\n> > \n> > clone(...)\n> > close(0) = 0\n> > open(\"/dev/null\", O_RDONLY) = 0\n> > ...\n> > execve(\"/bin/cat\", [\"cat\"], [/* 50 vars */]) = 0\n> > \n> > What? The shell is literally redirecting the cat process's stdin from\n> > /dev/null. I'm totally confused.\n> \n> You don't have to be; it's mandated by POSIX:\n> \n> http://pubs.opengroup.org/onlinepubs/9699919799/utilities/V3_chap02.html#tag_18_09_03_02\n\nSorry for the delayed response.\n\nThanks for the pointer; I looked in POSIX but couldn't find that\npassage. It does say \"In all cases, explicit redirection of standard\ninput shall override this activity\". It looks like dash interprets that\nas \"open /dev/null, then open the redirected stdin\". Which leaves a race\ncondition. So I think a custom wrapper like the one posted by Clemens\nis our only portable option.\n\nAs an aside, should git-daemon be respecting SIGPIPE at all? It seems\npointless at best, as it should be checking all of its writes, and a\nliability at worst, as something like failing to log to stderr can kill\nthe whole process.\n\n(Ignoring SIGPIPE would downgrade the severity of this problem, but I\n think we would still want Clemens' solution. Otherwise the error output\n in the test could be truncated).\n\n-Peff\n"},{"id":"190128","messageId":"4F999105.200@kdbg.org","threadId":"30235","inReplyTo":"20120426130129.GA27785@sigill.intra.peff.net","subject":"Re: [PATCH] t5570: forward git-daemon messages in a different way","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2012-04-26T18:16:37Z","receivedAt":"2012-04-26T18:16:37Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 26.04.2012 15:01, schrieb Jeff King:\n> On Thu, Apr 19, 2012 at 08:58:01AM +0200, Johannes Sixt wrote:\n> \n>>> What? The shell is literally redirecting the cat process's stdin from\n>>> /dev/null. I'm totally confused.\n>>\n>> You don't have to be; it's mandated by POSIX:\n>>\n>> http://pubs.opengroup.org/onlinepubs/9699919799/utilities/V3_chap02.html#tag_18_09_03_02\n> \n> Sorry for the delayed response.\n> \n> Thanks for the pointer; I looked in POSIX but couldn't find that\n> passage. It does say \"In all cases, explicit redirection of standard\n> input shall override this activity\". It looks like dash interprets that\n> as \"open /dev/null, then open the redirected stdin\". Which leaves a race\n> condition.\n\nI don't see a race condition. The specs are clear: First redirect stdin\nto /dev/null, and if there are other redirections, apply them later.\nBut in our code we have only:\n\n\tcat >&4 &\n\ni.e., there are no other redirections. It does not matter that the\nwhole block where this command occurs is redirected from\ngit_daemon_output; that redirection was applied before the command was\nexecuted, and it was already overridden by the implicit /dev/null\nredirection.\n\n> So I think a custom wrapper like the one posted by Clemens\n> is our only portable option.\n\nI don't think so. How about this?\n\ndiff --git a/t/lib-git-daemon.sh b/t/lib-git-daemon.sh\nindex ef2d01f..7245ab3 100644\n--- a/t/lib-git-daemon.sh\n+++ b/t/lib-git-daemon.sh\n@@ -30,10 +30,10 @@ start_git_daemon() {\n \t\t\"$@\" \"$GIT_DAEMON_DOCUMENT_ROOT_PATH\" \\\n \t\t>&3 2>git_daemon_output &\n \tGIT_DAEMON_PID=$!\n+\texec 7&4 \"$line\"\n-\t\tcat >&4 &\n \n \t\t# Check expected output\n \t\tif test x\"$(expr \"$line\" : \"\\[[0-9]*\\] \\(.*\\)\")\" != x\"Ready to rumble\"\n@@ -43,7 +43,9 @@ start_git_daemon() {\n \t\t\ttrap 'die' EXIT\n \t\t\terror \"git daemon failed to start\"\n \t\tfi\n-\t} &4 &\n+\t\texec 7<&-\n+\t}\n }\n \n stop_git_daemon() {\n\n\ni.e., we open the readable end of the pipe in the shell, and dup\nit from there to 'read' and later to 'cat'. Finally, we can\nclose it, because 'cat' has it still open in the background.\n\nThis works with dash, bash, and (half-way through) ksh. (The failure\nwith ksh is an unrelated problem.)\n\n-- Hannes\n"},{"id":"190148","messageId":"20120426195503.GA29526@ecki","threadId":"30235","inReplyTo":"4F999105.200@kdbg.org","subject":"Re: [PATCH] t5570: forward git-daemon messages in a different way","fromName":"Clemens Buchacher","fromEmail":"drizzd@aon.at","sentAt":"2012-04-26T19:55:04Z","receivedAt":"2012-04-26T19:55:04Z","isPatch":true,"sender":{"key":"drizzd@gmx.net","avatar":"https://avatars.githubusercontent.com/u/59082?v=4"},"body":"On Thu, Apr 26, 2012 at 08:16:37PM +0200, Johannes Sixt wrote:\n> \n> @@ -30,10 +30,10 @@ start_git_daemon() {\n> \t\t\"$@\" \"$GIT_DAEMON_DOCUMENT_ROOT_PATH\" \\\n> \t\t>&3 2>git_daemon_output &\n> \tGIT_DAEMON_PID=$!\n> +\texec 7 \t{\n> -\t\tread line\n> +\t\tread line <&7\n> \t\techo >&4 \"$line\"\n> -\t\tcat >&4 &\n> \n> \t\t# Check expected output\n> \t\tif test x\"$(expr \"$line\" : \"\\[[0-9]*\\] \\(.*\\)\")\" != x\"Ready to rumble\"\n> @@ -43,7 +43,9 @@ start_git_daemon() {\n> \t\t\ttrap 'die' EXIT\n> \t\t\terror \"git daemon failed to start\"\n> \t\tfi\n> -\t} +\t\tcat <&7 >&4 &\n> +\t\texec 7<&-\n> +\t}\n\nI won't pretend to understand why this works. I have to study this some\nmore. But if this is 'correct', then it is obviously preferable to the\ncomparatively complicated wrapper.\n\nWe should move the cat <&7 >&4 & and exec 7<&- part in front of the\noutput check, otherwise output would be truncated in an error condition.\nThis can be tested by passing an invalid argument to git daemon above,\nfor example.\n\nClemens\n"},{"id":"190155","messageId":"4F99B777.4020103@kdbg.org","threadId":"30235","inReplyTo":"20120426195503.GA29526@ecki","subject":"[PATCH] t5570: fix forwarding of git-daemon messages via cat","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2012-04-26T21:00:39Z","receivedAt":"2012-04-26T21:00:39Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"The shell function that starts git-daemon wants to read the first line of\nthe daemon's stderr to ensure that it started correctly. Subsequent daemon\nerrors should be redirected to fd 4 (which is the terminal in verbose mode\nor /dev/null in quiet mode). To that end the shell script used 'read' to\nget the first line of output, and then 'cat &' to forward everything else\nin a background process.\n\nThe problem is, that 'cat >&4 &' does not produce any output because the\nshell redirects a background process's stdin to /dev/null. To have this\ncommand invocation do anything useful, we have to redirect its stdin\nexplicitly (which overrides the /dev/null redirection).\n\nThe shell function connects the daemon's stderr to its consumers via a\nFIFO. We cannot just do this:\n\n read line &4 &\n\nbecause after the first redirection the pipe is closed and the daemon\ncould receive SIGPIPE if it writes at the wrong moment. Therefore, we open\nthe readable end of the FIFO only once on fd 7 in the shell and dup from\nthere to the stdin of the two consumers.\n\nSigned-off-by: Johannes Sixt \n---\nAm 26.04.2012 21:55, schrieb Clemens Buchacher:\n> I won't pretend to understand why this works. I have to study this some\n> more. But if this is 'correct', then it is obviously preferable to the\n> comparatively complicated wrapper.\n> \n> We should move the cat <&7 >&4 & and exec 7<&- part in front of the\n> output check, otherwise output would be truncated in an error condition.\n> This can be tested by passing an invalid argument to git daemon above,\n> for example.\n\nHow about this?\n\n t/lib-git-daemon.sh | 22 +++++++++++-----------\n 1 file changed, 11 insertions(+), 11 deletions(-)\n\ndiff --git a/t/lib-git-daemon.sh b/t/lib-git-daemon.sh\nindex ef2d01f..87f0ad8 100644\n--- a/t/lib-git-daemon.sh\n+++ b/t/lib-git-daemon.sh\n@@ -31,19 +31,19 @@ start_git_daemon() {\n \t\t>&3 2>git_daemon_output &\n \tGIT_DAEMON_PID=$!\n \t{\n-\t\tread line\n+\t\tread line <&7\n \t\techo >&4 \"$line\"\n-\t\tcat >&4 &\n+\t\tcat <&7 >&4 &\n+\t} 7 The shell function that starts git-daemon wants to read the first line of\n> the daemon's stderr to ensure that it started correctly. Subsequent daemon\n> errors should be redirected to fd 4 (which is the terminal in verbose mode\n> or /dev/null in quiet mode). To that end the shell script used 'read' to\n> get the first line of output, and then 'cat &' to forward everything else\n> in a background process.\n> \n> The problem is, that 'cat >&4 &' does not produce any output because the\n> shell redirects a background process's stdin to /dev/null. To have this\n> command invocation do anything useful, we have to redirect its stdin\n> explicitly (which overrides the /dev/null redirection).\n> \n> The shell function connects the daemon's stderr to its consumers via a\n> FIFO. We cannot just do this:\n> \n> read line cat &4 &\n> \n> because after the first redirection the pipe is closed and the daemon\n> could receive SIGPIPE if it writes at the wrong moment. Therefore, we open\n> the readable end of the FIFO only once on fd 7 in the shell and dup from\n> there to the stdin of the two consumers.\n> \n> Signed-off-by: Johannes Sixt \nBeautiful explanation. Thanks!\nI can confirm that this fix works for me.\n\n-\nZbyszek\n"},{"id":"190192","messageId":"20120427075551.GA12092@sigill.intra.peff.net","threadId":"30235","inReplyTo":"4F999105.200@kdbg.org","subject":"Re: [PATCH] t5570: forward git-daemon messages in a different way","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-04-27T07:55:51Z","receivedAt":"2012-04-27T07:55:51Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Apr 26, 2012 at 08:16:37PM +0200, Johannes Sixt wrote:\n\n> > Thanks for the pointer; I looked in POSIX but couldn't find that\n> > passage. It does say \"In all cases, explicit redirection of standard\n> > input shall override this activity\". It looks like dash interprets that\n> > as \"open /dev/null, then open the redirected stdin\". Which leaves a race\n> > condition.\n> \n> I don't see a race condition.\n\nOne of the proposed solutions was:\n\n {\n read line\n cat The specs are clear: First redirect stdin\n> to /dev/null, and if there are other redirections, apply them later.\n> But in our code we have only:\n> \n> \tcat >&4 &\n\nYes. But it also fails sometimes with the solution above, in which we\nexplicitly redirect from the fifo. The issue is not that we redirect\nfrom /dev/null in the long term, but that there is a moment where we\nhave closed the old stdin and not yet opened the new one.\n\n> I don't think so. How about this?\n> \n> diff --git a/t/lib-git-daemon.sh b/t/lib-git-daemon.sh\n> index ef2d01f..7245ab3 100644\n> --- a/t/lib-git-daemon.sh\n> +++ b/t/lib-git-daemon.sh\n> @@ -30,10 +30,10 @@ start_git_daemon() {\n> \t\t\"$@\" \"$GIT_DAEMON_DOCUMENT_ROOT_PATH\" \\\n> \t\t>&3 2>git_daemon_output &\n> \tGIT_DAEMON_PID=$!\n> +\texec 7 \t{\n> -\t\tread line\n> +\t\tread line <&7\n> \t\techo >&4 \"$line\"\n> -\t\tcat >&4 &\n> \n> \t\t# Check expected output\n> \t\tif test x\"$(expr \"$line\" : \"\\[[0-9]*\\] \\(.*\\)\")\" != x\"Ready to rumble\"\n> @@ -43,7 +43,9 @@ start_git_daemon() {\n> \t\t\ttrap 'die' EXIT\n> \t\t\terror \"git daemon failed to start\"\n> \t\tfi\n> -\t} +\t\tcat <&7 >&4 &\n> +\t\texec 7<&-\n> +\t}\n> }\n> \n> stop_git_daemon() {\n> \n> \n> i.e., we open the readable end of the pipe in the shell, and dup\n> it from there to 'read' and later to 'cat'. Finally, we can\n> close it, because 'cat' has it still open in the background.\n\nYes, I believe this will work reliably. Though there is one subtle thing\nhappening. At first glance, I thought you might have the same race I\nmentioned above; there's no guarantee that the shell subprocess has\nactually set up its stdin before your \"exec 7<&-\" runs. But because\nthe subprocess has inherited descriptor 7, and because it never\nexplicitly closes descriptor 7 (as it does for descriptor 0 while\nsetting up the command's redirections), the pipe is always open. As fd 7\nbefore exec'ing cat, and as both 7 and 0 afterwards.\n\nSo I think you could even get rid of your \"exec\" lines entirely, and\njust do:\n\n {\n read line <&7\n cat <&7 &\n } 7fd &\n pid=$!\n {\n read line\n echo $line\n wc The shell function connects the daemon's stderr to its consumers via a\n> FIFO. We cannot just do this:\n> \n> read line cat &4 &\n> \n> because after the first redirection the pipe is closed and the daemon\n> could receive SIGPIPE if it writes at the wrong moment. Therefore, we open\n> the readable end of the FIFO only once on fd 7 in the shell and dup from\n> there to the stdin of the two consumers.\n>\n> [...]\n> \t{\n> -\t\tread line\n> +\t\tread line <&7\n> \t\techo >&4 \"$line\"\n> -\t\tcat >&4 &\n> +\t\tcat <&7 >&4 &\n> +\t} 7\n\n-Peff\n"},{"id":"190215","messageId":"xmqqzk9x9q0u.fsf@junio.mtv.corp.google.com","threadId":"30235","inReplyTo":"4F99B777.4020103@kdbg.org","subject":"Re: [PATCH] t5570: fix forwarding of git-daemon messages via cat","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-04-27T15:02:25Z","receivedAt":"2012-04-27T15:02:25Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Sixt writes:\n\n> The shell function that starts git-daemon wants to read the first line of\n> the daemon's stderr to ensure that it started correctly. Subsequent daemon\n> errors should be redirected to fd 4 (which is the terminal in verbose mode\n> or /dev/null in quiet mode). To that end the shell script used 'read' to\n> get the first line of output, and then 'cat &' to forward everything else\n> in a background process.\n>\n> The problem is, that 'cat >&4 &' does not produce any output because the\n> shell redirects a background process's stdin to /dev/null. To have this\n> command invocation do anything useful, we have to redirect its stdin\n> explicitly (which overrides the /dev/null redirection).\n>\n> The shell function connects the daemon's stderr to its consumers via a\n> FIFO. We cannot just do this:\n>\n> read line cat &4 &\n>\n> because after the first redirection the pipe is closed and the daemon\n> could receive SIGPIPE if it writes at the wrong moment. Therefore, we open\n> the readable end of the FIFO only once on fd 7 in the shell and dup from\n> there to the stdin of the two consumers.\n>\n> Signed-off-by: Johannes Sixt \n\nVery clearly explained and fixed; thanks.\n\nWill replace cb/daemon-test-race-fix and queue.\n"}]}