{"thread":{"id":"49855","subject":"[PATCH] t5562: skip if NO_CURL is enabled","startedAt":"2018-11-19T10:15:41Z","lastAt":"2018-12-01T19:53:41Z","messageCount":35,"participants":["Carlo Marcelo Arenas Belón","Ævar Arnfjörð Bjarmason","Max Kirillov","Carlo Arenas","Jeff King","Junio C Hamano","SZEDER Gábor"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"363622","messageId":"20181119101535.16538-1-carenas@gmail.com","threadId":"49855","inReplyTo":null,"subject":"[PATCH] t5562: skip if NO_CURL is enabled","fromName":"Carlo Marcelo Arenas Belón","fromEmail":"carenas@gmail.com","sentAt":"2018-11-19T10:15:35Z","receivedAt":"2018-11-19T10:15:41Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"6c213e863a (\"http-backend: respect CONTENT_LENGTH for receive-pack\", 2018-07-27)\nintroduced all tests but without a check for CURL support from git.\n\nSigned-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>\n---\n t/t5562-http-backend-content-length.sh | 6 ++++++\n 1 file changed, 6 insertions(+)\n\ndiff --git a/t/t5562-http-backend-content-length.sh b/t/t5562-http-backend-content-length.sh\nindex b24d8b05a4..7594899471 100755\n--- a/t/t5562-http-backend-content-length.sh\n+++ b/t/t5562-http-backend-content-length.sh\n@@ -3,6 +3,12 @@\n test_description='test git-http-backend respects CONTENT_LENGTH'\n . ./test-lib.sh\n \n+if test -n \"$NO_CURL\"\n+then\n+\tskip_all='skipping test, git built without http support'\n+\ttest_done\n+fi\n+\n test_lazy_prereq GZIP 'gzip --version'\n \n verify_http_result() {\n-- \n2.20.0.rc0\n\n"},{"id":"363624","messageId":"87a7m51h74.fsf@evledraar.gmail.com","threadId":"49855","inReplyTo":"20181119101535.16538-1-carenas@gmail.com","subject":"Re: [PATCH] t5562: skip if NO_CURL is enabled","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-11-19T10:42:55Z","receivedAt":"2018-11-19T10:43:01Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Mon, Nov 19 2018, Carlo Marcelo Arenas Belón wrote:\n\n> 6c213e863a (\"http-backend: respect CONTENT_LENGTH for receive-pack\", 2018-07-27)\n> introduced all tests but without a check for CURL support from git.\n>\n> Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>\n> ---\n>  t/t5562-http-backend-content-length.sh | 6 ++++++\n>  1 file changed, 6 insertions(+)\n>\n> diff --git a/t/t5562-http-backend-content-length.sh b/t/t5562-http-backend-content-length.sh\n> index b24d8b05a4..7594899471 100755\n> --- a/t/t5562-http-backend-content-length.sh\n> +++ b/t/t5562-http-backend-content-length.sh\n> @@ -3,6 +3,12 @@\n>  test_description='test git-http-backend respects CONTENT_LENGTH'\n>  . ./test-lib.sh\n\nThis seems like the wrong fix for whatever bug you're encountering. I\njust built with NO_CURL and:\n\n    $ ./t5561-http-backend.sh\n    1..0 # SKIP skipping test, git built without http support\n    $ ./t5562-http-backend-content-length.sh\n    ok 1 - setup\n    ok 2 - setup, compression related\n    ok 3 - fetch plain\n    ok 4 - fetch plain truncated\n    ok 5 - fetch plain empty\n    ok 6 - fetch gzipped\n    ok 7 - fetch gzipped truncated\n    ok 8 - fetch gzipped empty\n    ok 9 - push plain\n    ok 10 - push plain truncated\n    ok 11 - push plain empty\n    ok 12 - push gzipped\n    ok 13 - push gzipped truncated\n    ok 14 - push gzipped empty\n    ok 15 - CONTENT_LENGTH overflow ssite_t\n    ok 16 - empty CONTENT_LENGTH\n    # passed all 16 test(s)\n    1..16\n\nSo all these test pass.\n\nOf courses I still have curl on my system, but I don't see the curl(1)\nutility used in the test, and my git at this point can't operate on\nhttps?:// URLs, so what error are you getting? Can you paste the test\noutput with -x -v?\n\n> +if test -n \"$NO_CURL\"\n> +then\n> +\tskip_all='skipping test, git built without http support'\n> +\ttest_done\n> +fi\n> +\n>  test_lazy_prereq GZIP 'gzip --version'\n>\n>  verify_http_result() {\n\nIf we do end up needing this after all it seems better to do something\nlike:\n\n    diff --git a/t/lib-httpd.sh b/t/lib-httpd.sh\n    index a8729f8232..adad654277 100644\n    --- a/t/lib-httpd.sh\n    +++ b/t/lib-httpd.sh\n    @@ -30,11 +30,7 @@\n     # Copyright (c) 2008 Clemens Buchacher <drizzd@aon.at>\n     #\n\n    -if test -n \"$NO_CURL\"\n    -then\n    -       skip_all='skipping test, git built without http support'\n    -       test_done\n    -fi\n    +. \"$TEST_DIRECTORY\"/lib-no-curl.sh\n\n     if test -n \"$NO_EXPAT\" && test -n \"$LIB_HTTPD_DAV\"\n     then\n    diff --git a/t/lib-no-curl.sh b/t/lib-no-curl.sh\n    new file mode 100644\n    index 0000000000..014947aa2d\n    --- /dev/null\n    +++ b/t/lib-no-curl.sh\n    @@ -0,0 +1,5 @@\n    +if test -n \"$NO_CURL\"\n    +then\n    +       skip_all='skipping test, git built without http support'\n    +       test_done\n    +fi\n    diff --git a/t/t5562-http-backend-content-length.sh b/t/t5562-http-backend-content-length.sh\n    index b24d8b05a4..cffb460673 100755\n    --- a/t/t5562-http-backend-content-length.sh\n    +++ b/t/t5562-http-backend-content-length.sh\n    @@ -2,6 +2,7 @@\n\n     test_description='test git-http-backend respects CONTENT_LENGTH'\n     . ./test-lib.sh\n    +. ./lib-no-curl.sh\n\n     test_lazy_prereq GZIP 'gzip --version'\n\nNot really a problem with your patch, we have lots of this copy/pasting\nall over the place already. I.e. stuff like:\n\n    if test -n \"$X\"\n    then\n    \tskip_all=\"$Y\"\n    \ttest_done\n    fi\n\nor:\n\n    if ! test_have_prereq \"$X\"\n    then\n    \tskip_all=\"$Y\"\n    \ttest_done\n    fi\n\nMaybe we should make more use of test_lazy_prereq and factor all that\ninto a new helper like:\n\n    test_have_prereq_or_skip_all \"$X\" \"$Y\"\n\nWhich could be put at the top of these various tests...\n"},{"id":"363654","messageId":"20181119184018.GA5348@jessie.local","threadId":"49855","inReplyTo":"20181119101535.16538-1-carenas@gmail.com","subject":"Re: [PATCH] t5562: skip if NO_CURL is enabled","fromName":"Max Kirillov","fromEmail":"max@max630.net","sentAt":"2018-11-19T18:40:18Z","receivedAt":"2018-11-19T18:47:46Z","isPatch":true,"sender":{"key":"max@max630.net","avatar":"https://avatars.githubusercontent.com/u/381560?v=4"},"body":"On Mon, Nov 19, 2018 at 02:15:35AM -0800, Carlo Marcelo Arenas Belón wrote:\n> 6c213e863a (\"http-backend: respect CONTENT_LENGTH for receive-pack\", 2018-07-27)\n> introduced all tests but without a check for CURL support from git.\n\nThe tests should not be using curl, they just pipe data to\nhttp-backend's standard input.\n\n-- \nMax\n"},{"id":"363660","messageId":"CAPUEsphLMBpxtJakAhQmdKf04H9X4m-8sBSHNFE_eAngn-44Ow@mail.gmail.com","threadId":"49855","inReplyTo":"20181119184018.GA5348@jessie.local","subject":"Re: [PATCH] t5562: skip if NO_CURL is enabled","fromName":"Carlo Arenas","fromEmail":"carenas@gmail.com","sentAt":"2018-11-19T19:36:08Z","receivedAt":"2018-11-19T19:36:27Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"On Mon, Nov 19, 2018 at 10:40 AM Max Kirillov <max@max630.net> wrote:\n>\n> On Mon, Nov 19, 2018 at 02:15:35AM -0800, Carlo Marcelo Arenas Belón wrote:\n> > 6c213e863a (\"http-backend: respect CONTENT_LENGTH for receive-pack\", 2018-07-27)\n> > introduced all tests but without a check for CURL support from git.\n>\n> The tests should not be using curl, they just pipe data to\n> http-backend's standard input.\n\nNO_CURL reflects the build setting (for http support); CURL checks for\nthe curl binary, but as Ævar points out the requirements must be from\nsomewhere else since a NO_CURL=1 build (tested in macOS) still passes\nthe test, but not in NetBSD.\n\ntests 3-8 seem to fail because perl is hardcoded to /urs/bin/perl in\nt5562/invoke-with-content-length.pl, while I seem to be getting some\nsporadic errors in 9 with the following output :\n\n++ env CONTENT_TYPE=application/x-git-receive-pack-request\nQUERY_STRING=/repo.git/git-receive-pack\n'PATH_TRANSLATED=/home/carenas/src/git/t/trash\ndirectory.t5562-http-backend-content-length/.git/git-receive-pack'\nGIT_HTTP_EXPORT_ALL=TRUE REQUEST_METHOD=POST\n/home/carenas/src/git/t/t5562/invoke-with-content-length.pl push_body\ngit http-backend\n++ verify_http_result '200 OK'\n++ grep fatal: act.err\nBinary file act.err matches\n++ return 1\nerror: last command exited with $?=1\nnot ok 9 - push plain\n\nand the following output in act.err (with a 200 in act)\n\nfatal: the remote end hung up unexpectedly\n\nCarlo\n"},{"id":"363678","messageId":"20181119212603.GC5348@jessie.local","threadId":"49855","inReplyTo":"CAPUEsphLMBpxtJakAhQmdKf04H9X4m-8sBSHNFE_eAngn-44Ow@mail.gmail.com","subject":"Re: [PATCH] t5562: skip if NO_CURL is enabled","fromName":"Max Kirillov","fromEmail":"max@max630.net","sentAt":"2018-11-19T21:26:03Z","receivedAt":"2018-11-19T21:33:26Z","isPatch":true,"sender":{"key":"max@max630.net","avatar":"https://avatars.githubusercontent.com/u/381560?v=4"},"body":"On Mon, Nov 19, 2018 at 11:36:08AM -0800, Carlo Arenas wrote:\n> NO_CURL reflects the build setting (for http support); CURL checks for\n> the curl binary, but as Ævar points out the requirements must be from\n> somewhere else since a NO_CURL=1 build (tested in macOS) still passes\n> the test, but not in NetBSD.\n> \n> tests 3-8 seem to fail because perl is hardcoded to /urs/bin/perl in\n> t5562/invoke-with-content-length.pl,\n\nI see.\n\nIn other perl files I can see either '#!/usr/bin/perl' or\n'#!/ust/bin/env perl'. The second one should be more\nportable. Does the latter work on the NetBSD?\n\nTo all: what is supposed to be done about it?\n\n> while I seem to be getting some\n> sporadic errors in 9 with the following output :\n\nThis is more complicated.\n\nDoes it happen often?\n\nDoes test 12 (\"push gzipped\") ever fail?\n\nSo far I can imagine either a buffering issue or some\nmistake in length calculation.\n"},{"id":"363680","messageId":"20181119213924.GA2318@sigill.intra.peff.net","threadId":"49855","inReplyTo":"20181119212603.GC5348@jessie.local","subject":"Re: [PATCH] t5562: skip if NO_CURL is enabled","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-11-19T21:39:24Z","receivedAt":"2018-11-19T21:39:28Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Nov 19, 2018 at 11:26:03PM +0200, Max Kirillov wrote:\n\n> On Mon, Nov 19, 2018 at 11:36:08AM -0800, Carlo Arenas wrote:\n> > NO_CURL reflects the build setting (for http support); CURL checks for\n> > the curl binary, but as Ævar points out the requirements must be from\n> > somewhere else since a NO_CURL=1 build (tested in macOS) still passes\n> > the test, but not in NetBSD.\n> > \n> > tests 3-8 seem to fail because perl is hardcoded to /urs/bin/perl in\n> > t5562/invoke-with-content-length.pl,\n> \n> I see.\n> \n> In other perl files I can see either '#!/usr/bin/perl' or\n> '#!/ust/bin/env perl'. The second one should be more\n> portable. Does the latter work on the NetBSD?\n> \n> To all: what is supposed to be done about it?\n\nYou should swap this out for $PERL_PATH. You can use write_script() to\nhelp if you're copying the script around anyway. Though it looks like\nyou just run it from the one function. So maybe just:\n\ndiff --git a/t/t5562-http-backend-content-length.sh b/t/t5562-http-backend-content-length.sh\nindex b24d8b05a4..90d890d02f 100755\n--- a/t/t5562-http-backend-content-length.sh\n+++ b/t/t5562-http-backend-content-length.sh\n@@ -31,6 +31,7 @@ test_http_env() {\n \t\tPATH_TRANSLATED=\"$PWD/.git/git-$handler_type-pack\" \\\n \t\tGIT_HTTP_EXPORT_ALL=TRUE \\\n \t\tREQUEST_METHOD=POST \\\n+\t\t\"$PERL_PATH\" \\\n \t\t\"$TEST_DIRECTORY\"/t5562/invoke-with-content-length.pl \\\n \t\t    \"$request_body\" git http-backend >act.out 2>act.err\n }\n\n(note that it's normally OK to just run \"perl\", because we use a\nshell-function wrapper that respects $PERL_PATH, but here we're actually\npassing it to \"env\").\n\nYou could also lose the executable bit on the script at that point. It\ndoesn't matter much, but it would catch an erroneous call relying on the\nshebang line.\n\n-Peff\n"},{"id":"363711","messageId":"20181120091107.GA30542@sigill.intra.peff.net","threadId":"49855","inReplyTo":"CAPUEsphLMBpxtJakAhQmdKf04H9X4m-8sBSHNFE_eAngn-44Ow@mail.gmail.com","subject":"Re: [PATCH] t5562: skip if NO_CURL is enabled","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-11-20T09:11:08Z","receivedAt":"2018-11-20T09:11:12Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Nov 19, 2018 at 11:36:08AM -0800, Carlo Arenas wrote:\n\n> tests 3-8 seem to fail because perl is hardcoded to /urs/bin/perl in\n> t5562/invoke-with-content-length.pl, while I seem to be getting some\n> sporadic errors in 9 with the following output :\n> \n> ++ env CONTENT_TYPE=application/x-git-receive-pack-request\n> QUERY_STRING=/repo.git/git-receive-pack\n> 'PATH_TRANSLATED=/home/carenas/src/git/t/trash\n> directory.t5562-http-backend-content-length/.git/git-receive-pack'\n> GIT_HTTP_EXPORT_ALL=TRUE REQUEST_METHOD=POST\n> /home/carenas/src/git/t/t5562/invoke-with-content-length.pl push_body\n> git http-backend\n> ++ verify_http_result '200 OK'\n> ++ grep fatal: act.err\n> Binary file act.err matches\n> ++ return 1\n> error: last command exited with $?=1\n> not ok 9 - push plain\n> \n> and the following output in act.err (with a 200 in act)\n> \n> fatal: the remote end hung up unexpectedly\n\nThis bit me today, too, and I can reproduce it by running under my\nstress-testing script.\n\nCuriously, the act.err file also has 54 NUL bytes before the \"fatal:\"\nmessage. I tried adding an \"strace\" to see who was producing that\noutput, but I can't seem to get it to fail when running under strace\n(presumably because the timing is quite different, and this likely some\nkind of pipe race).\n\n-Peff\n"},{"id":"363819","messageId":"CAPUEsphaYBXp4V2FYqoB8-A2dyqppH=hSAaoQXGk4NMwXznCiA@mail.gmail.com","threadId":"49855","inReplyTo":"20181120091107.GA30542@sigill.intra.peff.net","subject":"Re: [PATCH] t5562: skip if NO_CURL is enabled","fromName":"Carlo Arenas","fromEmail":"carenas@gmail.com","sentAt":"2018-11-21T12:02:04Z","receivedAt":"2018-11-21T12:02:20Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"FWIW the issue goes away when more than 1 CPU is used in NetBSD 8,0\n(32-bit) and for some tracing, it would seem that it gets 0 when\ntrying to read 4 bytes from what I think is a pipe that connects to a\nchild that has been gone already for a while.\n\nCarlo\n"},{"id":"363853","messageId":"20181121224929.GD5348@jessie.local","threadId":"49855","inReplyTo":"CAPUEsphaYBXp4V2FYqoB8-A2dyqppH=hSAaoQXGk4NMwXznCiA@mail.gmail.com","subject":"Re: [PATCH] t5562: skip if NO_CURL is enabled","fromName":"Max Kirillov","fromEmail":"max@max630.net","sentAt":"2018-11-21T22:49:29Z","receivedAt":"2018-11-21T22:49:34Z","isPatch":true,"sender":{"key":"max@max630.net","avatar":"https://avatars.githubusercontent.com/u/381560?v=4"},"body":"On Wed, Nov 21, 2018 at 04:02:04AM -0800, Carlo Arenas wrote:\n> for some tracing, it would seem that it gets 0 when\n> trying to read 4 bytes from what I think is a pipe that connects to a\n> child that has been gone already for a while.\n\nCould you clarify it? I'm afraid I don't understand.\n\nMeanwhile, I've been staring at code and so far don't have any\nassumption where it could fail. Except basic things like something is\nwrong with forking or reading/writing pipes, but then it would have\nbigger consequences.\n\nAlso, I tried to look at it with NetBSD but cannot get past\nerror, while running tests:\n\n> ./test-lib.sh: 327: Syntax error: Bad substitution\n\nThere is the following code there:\n\n-----\n                if test -z \"$test_untraceable\" || {\n                     test -n \"$BASH_VERSION\" && {\n                       test ${BASH_VERSINFO[0]} -gt 4 || { # line 327\n                         test ${BASH_VERSINFO[0]} -eq 4 &&\n                         test ${BASH_VERSINFO[1]} -ge 1\n\n-----\n\nShould I install bash for it to work? I cannot say I understand what the message is about.\n"},{"id":"363854","messageId":"CAPUEspjpHJ01M9UM5w7P6n3g_Yi+WF4wYGUG-iG936g4vfuhJQ@mail.gmail.com","threadId":"49855","inReplyTo":"20181121224929.GD5348@jessie.local","subject":"Re: [PATCH] t5562: skip if NO_CURL is enabled","fromName":"Carlo Arenas","fromEmail":"carenas@gmail.com","sentAt":"2018-11-21T23:36:03Z","receivedAt":"2018-11-21T23:36:17Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"On Wed, Nov 21, 2018 at 2:49 PM Max Kirillov <max@max630.net> wrote:\n>\n> Should I install bash for it to work? I cannot say I understand what the message is about.\n\nyes, you need to install bash and use SHELL_PATH=/usr/pkg/bin/bash;\nPERL_PATH=/usr/pkg/bin/perl for the perl script\n\nCarlo\n"},{"id":"363857","messageId":"CAPUEspjeiT=Odc7ENd0Qjeg=8w-+Qh9uGjL+BQXihiK1G1vkjA@mail.gmail.com","threadId":"49855","inReplyTo":"20181121224929.GD5348@jessie.local","subject":"Re: [PATCH] t5562: skip if NO_CURL is enabled","fromName":"Carlo Arenas","fromEmail":"carenas@gmail.com","sentAt":"2018-11-22T01:04:25Z","receivedAt":"2018-11-22T01:04:39Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"On Wed, Nov 21, 2018 at 2:49 PM Max Kirillov <max@max630.net> wrote:\n>\n> On Wed, Nov 21, 2018 at 04:02:04AM -0800, Carlo Arenas wrote:\n> > for some tracing, it would seem that it gets 0 when\n> > trying to read 4 bytes from what I think is a pipe that connects to a\n> > child that has been gone already for a while.\n>\n> Could you clarify it? I'm afraid I don't understand.\n\nthe error that gets eventually to stderr in the caller comes from\nget_packet_data, who is trying to read 4 bytes and gets 0.\nwhen looking at the trace (obtained with ktrace) I see there is no\nlonger any other process running,\n\nthe last child of it is long gone with an error as shown by :\n\n  9255      1 git-http-backend CALL  close(1)\n  9255      1 git-http-backend RET   close 0\n  9255      1 git-http-backend CALL  read(0,0xbfb2bb14,0)\n  9255      1 git-http-backend GIO   fd 0 read 0 bytes\n       \"\"\n  9255      1 git-http-backend RET   read 0\n  9255      1 git-http-backend CALL  write(2,0xbfb2a604,0x36)\n  9255      1 git-http-backend GIO   fd 2 wrote 54 bytes\n       \"fatal: request ended in the middle of the gzip stream\\n\"\n  9255      1 git-http-backend RET   write 54/0x36\n  9255      1 git-http-backend CALL  write(1,0xb781f0e0,0x94)\n  9255      1 git-http-backend RET   write -1 errno 9 Bad file descriptor\n\nnot sure how it got into that state, though\n\nCarlo\n"},{"id":"363872","messageId":"20181122063714.GE5348@jessie.local","threadId":"49855","inReplyTo":"CAPUEspjeiT=Odc7ENd0Qjeg=8w-+Qh9uGjL+BQXihiK1G1vkjA@mail.gmail.com","subject":"Re: [PATCH] t5562: skip if NO_CURL is enabled","fromName":"Max Kirillov","fromEmail":"max@max630.net","sentAt":"2018-11-22T06:37:14Z","receivedAt":"2018-11-22T06:44:36Z","isPatch":true,"sender":{"key":"max@max630.net","avatar":"https://avatars.githubusercontent.com/u/381560?v=4"},"body":"On Wed, Nov 21, 2018 at 05:04:25PM -0800, Carlo Arenas wrote:\n> the error that gets eventually to stderr in the caller comes from\n> get_packet_data, who is trying to read 4 bytes and gets 0.\n> when looking at the trace (obtained with ktrace)\n\nYes too early close of the input data is the thing which\ntriggers the \"remote end hung up unexpectedly\" message.\n\n> I see there is no\n> longer any other process running,\n\ndo you mean git receive-pack? This is strange, all its\nparents should be waiting for it to exit.\n\n> the last child of it is long gone with an error as shown by :\n> \n>   9255      1 git-http-backend CALL  close(1)\n...\n>   9255      1 git-http-backend CALL  write(2,0xbfb2a604,0x36)\n>   9255      1 git-http-backend GIO   fd 2 wrote 54 bytes\n>        \"fatal: request ended in the middle of the gzip stream\\n\"\n\nThis should be some other test than push_plain, some of the\ngzip related ones. Are there other tests failing?\n\n>   9255      1 git-http-backend RET   write 54/0x36\n>   9255      1 git-http-backend CALL  write(1,0xb781f0e0,0x94)\n>   9255      1 git-http-backend RET   write -1 errno 9 Bad file descriptor\n\nThis is interesting. http-backend for some reason closes its\nstdout. Here it then tries to write there something. I have\nnot seen it in my push_plain run. Maybe it worth redirecting instead\nto stderr, to avoid losing some diagnostics?\n\n> \n> not sure how it got into that state, though\n> \n> Carlo\n"},{"id":"363877","messageId":"CAPUEsph7z3nHjJ=idq5v0RPPjWwmGGMsbmPoyUChxUitBPeEBQ@mail.gmail.com","threadId":"49855","inReplyTo":"20181122063714.GE5348@jessie.local","subject":"Re: [PATCH] t5562: skip if NO_CURL is enabled","fromName":"Carlo Arenas","fromEmail":"carenas@gmail.com","sentAt":"2018-11-22T10:17:01Z","receivedAt":"2018-11-22T10:17:15Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"On Wed, Nov 21, 2018 at 10:37 PM Max Kirillov <max@max630.net> wrote:\n>\n> On Wed, Nov 21, 2018 at 05:04:25PM -0800, Carlo Arenas wrote:\n> > the last child of its children long gone with an error as shown by :\n> >\n> >   9255      1 git-http-backend CALL  close(1)\n> ...\n> >   9255      1 git-http-backend CALL  write(2,0xbfb2a604,0x36)\n> >   9255      1 git-http-backend GIO   fd 2 wrote 54 bytes\n> >        \"fatal: request ended in the middle of the gzip stream\\n\"\n>\n> This should be some other test than push_plain, some of the\n> gzip related ones. Are there other tests failing?\n\nit should, but I should note that for test 9 to fail, then either (or both)\ntests 7 and 8 should first succeed; not that I'd seen any other test fail (after\nI locally patched the perl path, of course) even when reordering them and\nwhile making sure tests 1 and 2 run first to create the dependencies\nfor the rest\n\nPeff, could you elaborate on your \"load testing\" setup? which could\ngive us any hints\non what to look for?, FWIW I hadn't been able to reproduce the problem anywhere\nelse (and not for a lack of trying)\n\n> >   9255      1 git-http-backend RET   write 54/0x36\n> >   9255      1 git-http-backend CALL  write(1,0xb781f0e0,0x94)\n> >   9255      1 git-http-backend RET   write -1 errno 9 Bad file descriptor\n>\n> This is interesting. http-backend for some reason closes its\n> stdout. Here it then tries to write there something. I have\n> not seen it in my push_plain run. Maybe it worth redirecting instead\n> to stderr, to avoid losing some diagnostics?\n\nthat should help with the garbled output from stderr, AFAIK the\nprocess API allows creating\na pipe specifically for that with would be better than redirecting\nstderr into stdout.\n\nthe fact we got EBADF means that there is a problem somewhere though\nin the way the\nprevious failure that closed stdout got handled (which should had been\nmost likely in\nthe call to die)\n\nCarlo\n\nPS. upstreaming the PERL_PATH fix is likely to be good to do soonish\nas I presume at least all BSD might be affected, let me know if you\nwould rather me do that instead as I suspect we might be deadlocked\notherwise ;)\n"},{"id":"363910","messageId":"20181122161722.GC28192@sigill.intra.peff.net","threadId":"49855","inReplyTo":"CAPUEsph7z3nHjJ=idq5v0RPPjWwmGGMsbmPoyUChxUitBPeEBQ@mail.gmail.com","subject":"Re: [PATCH] t5562: skip if NO_CURL is enabled","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-11-22T16:17:22Z","receivedAt":"2018-11-22T16:17:26Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Nov 22, 2018 at 02:17:01AM -0800, Carlo Arenas wrote:\n\n> Peff, could you elaborate on your \"load testing\" setup? which could\n> give us any hints\n> on what to look for?, FWIW I hadn't been able to reproduce the problem anywhere\n> else (and not for a lack of trying)\n\nThe script I use is at:\n\n  https://github.com/peff/git/blob/meta/stress\n\nwhich you invoke like \"/path/to/stress t5562\" from the top-level of a\ngit.git checkout.  It basically just runs a loop of twice as many\nsimultaneous invocations of the test script as you have CPUs, and waits\nfor one to fail. The load created by all of the runs tends to flush out\ntiming effects after a while.\n\nIt fails for me on t5562 within 30 seconds or so (but note that in this\nparticular case it sometimes takes a while to produce the final output\nbecause invoke-with-content-length misses the expected SIGCLD and sleeps\nthe full 60 seconds).\n\nYou'll probably need to tweak the variables at the top of the script for\nyour system.\n\n> PS. upstreaming the PERL_PATH fix is likely to be good to do soonish\n> as I presume at least all BSD might be affected, let me know if you\n> would rather me do that instead as I suspect we might be deadlocked\n> otherwise ;)\n\nYeah, the $PERL_PATH thing is totally orthogonal, and should graduate\nseparately.\n\n-Peff\n"},{"id":"363952","messageId":"20181122233821.17871-1-max@max630.net","threadId":"49855","inReplyTo":"20181119213924.GA2318@sigill.intra.peff.net","subject":"[PATCH] t5562: fix perl path","fromName":"Max Kirillov","fromEmail":"max@max630.net","sentAt":"2018-11-22T23:38:21Z","receivedAt":"2018-11-22T23:38:38Z","isPatch":true,"sender":{"key":"max@max630.net","avatar":"https://avatars.githubusercontent.com/u/381560?v=4"},"body":"From: Jeff King <peff@peff.net>\n\nSome systems do not have perl installed to /usr/bin. Use the variable\nfrom the build settiings, and call perl directly than via shebang.\n\nSigned-off-by: Max Kirillov <max@max630.net>\n---\nSubmitting. Could you sign-off? Also removed shebang from the script as it is not needed\n t/t5562-http-backend-content-length.sh | 1 +\n t/t5562/invoke-with-content-length.pl  | 1 -\n 2 files changed, 1 insertion(+), 1 deletion(-)\n mode change 100755 => 100644 t/t5562/invoke-with-content-length.pl\n\ndiff --git a/t/t5562-http-backend-content-length.sh b/t/t5562-http-backend-content-length.sh\nindex b24d8b05a4..90d890d02f 100755\n--- a/t/t5562-http-backend-content-length.sh\n+++ b/t/t5562-http-backend-content-length.sh\n@@ -31,6 +31,7 @@ test_http_env() {\n \t\tPATH_TRANSLATED=\"$PWD/.git/git-$handler_type-pack\" \\\n \t\tGIT_HTTP_EXPORT_ALL=TRUE \\\n \t\tREQUEST_METHOD=POST \\\n+\t\t\"$PERL_PATH\" \\\n \t\t\"$TEST_DIRECTORY\"/t5562/invoke-with-content-length.pl \\\n \t\t    \"$request_body\" git http-backend >act.out 2>act.err\n }\ndiff --git a/t/t5562/invoke-with-content-length.pl b/t/t5562/invoke-with-content-length.pl\nold mode 100755\nnew mode 100644\nindex 6c2aae7692..0943474af2\n--- a/t/t5562/invoke-with-content-length.pl\n+++ b/t/t5562/invoke-with-content-length.pl\n@@ -1,4 +1,3 @@\n-#!/usr/bin/perl\n use 5.008;\n use strict;\n use warnings;\n-- \n2.19.0.1202.g68e1e8f04e\n\n"},{"id":"363953","messageId":"20181122234346.GF5348@jessie.local","threadId":"49855","inReplyTo":"20181122161722.GC28192@sigill.intra.peff.net","subject":"Re: [PATCH] t5562: skip if NO_CURL is enabled","fromName":"Max Kirillov","fromEmail":"max@max630.net","sentAt":"2018-11-22T23:43:46Z","receivedAt":"2018-11-22T23:43:50Z","isPatch":true,"sender":{"key":"max@max630.net","avatar":"https://avatars.githubusercontent.com/u/381560?v=4"},"body":"On Thu, Nov 22, 2018 at 11:17:22AM -0500, Jeff King wrote:\n> The script I use is at:\n> \n>   https://github.com/peff/git/blob/meta/stress\n> \n> which you invoke like \"/path/to/stress t5562\" from the top-level of a\n> git.git checkout.  It basically just runs a loop of twice as many\n> simultaneous invocations of the test script as you have CPUs, and waits\n> for one to fail. The load created by all of the runs tends to flush out\n> timing effects after a while.\n> \n> It fails for me on t5562 within 30 seconds or so (but note that in this\n> particular case it sometimes takes a while to produce the final output\n> because invoke-with-content-length misses the expected SIGCLD and sleeps\n> the full 60 seconds).\n\nI have observed it caught failure at the very first run.\nHowever I could not fail t again. I tried running up to 20\ninstances, with 1 or 2 active cores (that's all I have\nhere), also edited the test to include only push_plain case,\nand repeat it several times, to avoid running irrelevant\ncases, the failure never happened again.\n\nThe first failure was a bit unusual, in the ouput actually\nall tests were marked as passed, but it still failed\nsomehow. Unfortunately, I did not save the output.\n\nI submitted the perl patch\n\n-- \nMax\n"},{"id":"363974","messageId":"CAPUEspi67=Kt=mx21bjG2oCATnU+byO5nkvbMdQkN03yBGZMsA@mail.gmail.com","threadId":"49855","inReplyTo":"20181122234346.GF5348@jessie.local","subject":"Re: [PATCH] t5562: skip if NO_CURL is enabled","fromName":"Carlo Arenas","fromEmail":"carenas@gmail.com","sentAt":"2018-11-23T12:57:43Z","receivedAt":"2018-11-23T12:57:56Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"On Thu, Nov 22, 2018 at 3:43 PM Max Kirillov <max@max630.net> wrote:\n> also edited the test to include only push_plain case,\n> and repeat it several times, to avoid running irrelevant\n> cases, the failure never happened again.\n\nas I explained previously[1] and as odd as it might seem the\npush_plain case ONLY\nfails if your run them together with the other tests that return\nerrors with compressed input\n\nfrankly I don't understand how one could affect the other as they\nshould be running in independent processes but it happens fairly\nconsistently in NetBSD (tested 7.1, 7.2, 8) with only one CPU (tested\ni386 and amd64)\n\nCarlo\n\n[1] https://public-inbox.org/git/20181119213924.GA2318@sigill.intra.peff.net/T/#m041e9703432c39dcb04fe10e86fc53d5254474b4\n"},{"id":"363975","messageId":"CAPUEspgnTMQbWvfOaJV4bhnQX7g=cs5pnKwegZFAtdV0=zLpgw@mail.gmail.com","threadId":"49855","inReplyTo":"20181122233821.17871-1-max@max630.net","subject":"Re: [PATCH] t5562: fix perl path","fromName":"Carlo Arenas","fromEmail":"carenas@gmail.com","sentAt":"2018-11-23T14:31:05Z","receivedAt":"2018-11-23T14:31:20Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"Tested-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>\n\nIMHO leaving the shebang might be better if only for consistency but\ncould go eitherway\n\nCarlo\n"},{"id":"363990","messageId":"20181124070428.18571-1-max@max630.net","threadId":"49855","inReplyTo":"CAPUEspi67=Kt=mx21bjG2oCATnU+byO5nkvbMdQkN03yBGZMsA@mail.gmail.com","subject":"[PATCH] t5562: do not reuse output files","fromName":"Max Kirillov","fromEmail":"max@max630.net","sentAt":"2018-11-24T07:04:28Z","receivedAt":"2018-11-24T07:04:39Z","isPatch":true,"sender":{"key":"max@max630.net","avatar":"https://avatars.githubusercontent.com/u/381560?v=4"},"body":"Some expected failures of git-http-backend leave running its children\n(receive-pack or upload-pack) which still hold opened descriptors\nto act.err and with some probability they live long enough to write\ntheir failure messages after next test has already truncated\nthe files. This causes occasional failures of the test script.\n\nAvoid the issue by unlinking the older files before writing to them.\n\nReported-by: Carlo Arenas <carenas@gmail.com>\nHelped-by: Carlo Arenas <carenas@gmail.com>\nSigned-off-by: Max Kirillov <max@max630.net>\n---\nThanks for the analysis. I seem to have guessed the reason.\nThis patch should prevent it.\n\nI think the tests should somehow make sure there are no such late\nprocesses. I can see 2 options:\n* somehow find out in the tests all children and wait for them. I have no idea how.\n* make http-backend close handle to its child and wait for it to exit before dying.\n  This would not prevent childrenc in general, because http-backend may be killed,\n  but not in our expected failure cases\n\nActually, don't the children receive some SIGHUP? Maybe thy should. However, it\nwould still take some time for them to handle it, so it does not fully solve the issue\n t/t5562-http-backend-content-length.sh | 4 ++++\n 1 file changed, 4 insertions(+)\n\ndiff --git a/t/t5562-http-backend-content-length.sh b/t/t5562-http-backend-content-length.sh\nindex 90d890d02f..bb53f82c0c 100755\n--- a/t/t5562-http-backend-content-length.sh\n+++ b/t/t5562-http-backend-content-length.sh\n@@ -25,6 +25,8 @@ test_http_env() {\n \thandler_type=\"$1\"\n \trequest_body=\"$2\"\n \tshift\n+\t(rm -f act.out || true) &&\n+\t(rm -f act.err || true) &&\n \tenv \\\n \t\tCONTENT_TYPE=\"application/x-git-$handler_type-pack-request\" \\\n \t\tQUERY_STRING=\"/repo.git/git-$handler_type-pack\" \\\n@@ -155,6 +157,8 @@ test_expect_success 'CONTENT_LENGTH overflow ssite_t' '\n '\n \n test_expect_success 'empty CONTENT_LENGTH' '\n+\t(rm -f act.out || true) &&\n+\t(rm -f act.err || true) &&\n \tenv \\\n \t\tQUERY_STRING=\"service=git-receive-pack\" \\\n \t\tPATH_TRANSLATED=\"$PWD\"/.git/info/refs \\\n-- \n2.19.0.1202.g68e1e8f04e\n\n"},{"id":"363991","messageId":"xmqqbm6f2ajn.fsf@gitster-ct.c.googlers.com","threadId":"49855","inReplyTo":"20181124070428.18571-1-max@max630.net","subject":"Re: [PATCH] t5562: do not reuse output files","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-11-24T07:34:52Z","receivedAt":"2018-11-24T07:34:57Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Max Kirillov <max@max630.net> writes:\n\n> diff --git a/t/t5562-http-backend-content-length.sh b/t/t5562-http-backend-content-length.sh\n> index 90d890d02f..bb53f82c0c 100755\n> --- a/t/t5562-http-backend-content-length.sh\n> +++ b/t/t5562-http-backend-content-length.sh\n> @@ -25,6 +25,8 @@ test_http_env() {\n>  \thandler_type=\"$1\"\n>  \trequest_body=\"$2\"\n>  \tshift\n> +\t(rm -f act.out || true) &&\n> +\t(rm -f act.err || true) &&\n\nWhy \"||true\"?  If the named file doesn't exist, \"rm -f\" would\nsucceed, and if it does exist but somehow we fail to remove, then\nthese added lines are not preveting the next part from reusing,\ni.e. they are not doing what they are supposed to be doing, so we\nshould detect such a failure (if happens) as an error, no?\n\nIOW, shouldn't it just be more like\n\n\t+\trm -f act.out act.err &&\n\nThe same comment applies to the other hunk.\n\n\n>  \tenv \\\n>  \t\tCONTENT_TYPE=\"application/x-git-$handler_type-pack-request\" \\\n>  \t\tQUERY_STRING=\"/repo.git/git-$handler_type-pack\" \\\n> @@ -155,6 +157,8 @@ test_expect_success 'CONTENT_LENGTH overflow ssite_t' '\n>  '\n>  \n>  test_expect_success 'empty CONTENT_LENGTH' '\n> +\t(rm -f act.out || true) &&\n> +\t(rm -f act.err || true) &&\n>  \tenv \\\n>  \t\tQUERY_STRING=\"service=git-receive-pack\" \\\n>  \t\tPATH_TRANSLATED=\"$PWD\"/.git/info/refs \\\n"},{"id":"363992","messageId":"xmqq7eh23ojc.fsf@gitster-ct.c.googlers.com","threadId":"49855","inReplyTo":"xmqqbm6f2ajn.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH] t5562: do not reuse output files","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-11-24T07:47:19Z","receivedAt":"2018-11-24T07:47:26Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> Max Kirillov <max@max630.net> writes:\n>\n>> diff --git a/t/t5562-http-backend-content-length.sh b/t/t5562-http-backend-content-length.sh\n>> index 90d890d02f..bb53f82c0c 100755\n>> --- a/t/t5562-http-backend-content-length.sh\n>> +++ b/t/t5562-http-backend-content-length.sh\n>> @@ -25,6 +25,8 @@ test_http_env() {\n>>  \thandler_type=\"$1\"\n>>  \trequest_body=\"$2\"\n>>  \tshift\n>> +\t(rm -f act.out || true) &&\n>> +\t(rm -f act.err || true) &&\n>\n> Why \"||true\"?  If the named file doesn't exist, \"rm -f\" would\n> succeed, and if it does exist but somehow we fail to remove, then\n> these added lines are not preveting the next part from reusing,\n> i.e. they are not doing what they are supposed to be doing,...\n\nAnother thing.  The analysis in your log message talks about a stray\nprocess holding open filehandles to these files.  An attempt to remove\nthem in such a stat would fail on some systems, so \"||true\" would not\nhelp, would it?  It just hides the failure to remove, and when ||true\nis useful in hiding th failure _is_ when such a stray process is still\nthere, waiting to corrupt the output of the next request and breaking\nthe test, no?\n\nI do agree that forcing the parent to wait, like you described in\nthe comment, would be far more preferrable, but until that happens,\na better workaround might be to write into unique output filenames\n(act1.out, act2.out, etc.); that way, you do not have to worry about\nthe output file for the next request getting clobbered by a stale\nprocess handling the previous request.  But at the same time,\nwouldn't this suggest that the test or the previous request may see\nan incomplete output, as the analysed problem is that such a process\nis writing to the output file very late, while we are preparing to\ntest the enxt request, which meas we have already checked the output\nfile for the previous request, right?  So even without ||true, I am\nnot sure how true a \"solution\" this change is to the issue.\n\n"},{"id":"363993","messageId":"20181124075243.27372-1-max@max630.net","threadId":"49855","inReplyTo":"xmqqbm6f2ajn.fsf@gitster-ct.c.googlers.com","subject":"[PATCH v2] t5562: do not reuse output files","fromName":"Max Kirillov","fromEmail":"max@max630.net","sentAt":"2018-11-24T07:52:43Z","receivedAt":"2018-11-24T07:52:51Z","isPatch":true,"sender":{"key":"max@max630.net","avatar":"https://avatars.githubusercontent.com/u/381560?v=4"},"body":"Some expected failures of git-http-backend leaves running its children\n(receive-pack or upload-pack) which still hold opened descriptors\nto act.err and with some probability they live long enough to write\nthere their failure messages after next test has already truncated\nthe files. This causes occasional failures of the test script.\n\nAvoid the issue by unlinking the older files before writing to them.\n\nReported-by: Carlo Arenas <carenas@gmail.com>\nHelped-by: Carlo Arenas <carenas@gmail.com>\nHelped-by: Junio C Hamano <gitster@pobox.com>\nSigned-off-by: Max Kirillov <max@max630.net>\n---\nThanks. Updated\n t/t5562-http-backend-content-length.sh | 2 ++\n 1 file changed, 2 insertions(+)\n\ndiff --git a/t/t5562-http-backend-content-length.sh b/t/t5562-http-backend-content-length.sh\nindex 90d890d02f..3a9f7a14e2 100755\n--- a/t/t5562-http-backend-content-length.sh\n+++ b/t/t5562-http-backend-content-length.sh\n@@ -25,6 +25,7 @@ test_http_env() {\n \thandler_type=\"$1\"\n \trequest_body=\"$2\"\n \tshift\n+\trm -f act.out act.err &&\n \tenv \\\n \t\tCONTENT_TYPE=\"application/x-git-$handler_type-pack-request\" \\\n \t\tQUERY_STRING=\"/repo.git/git-$handler_type-pack\" \\\n@@ -155,6 +156,7 @@ test_expect_success 'CONTENT_LENGTH overflow ssite_t' '\n '\n \n test_expect_success 'empty CONTENT_LENGTH' '\n+\trm -f act.out act.err &&\n \tenv \\\n \t\tQUERY_STRING=\"service=git-receive-pack\" \\\n \t\tPATH_TRANSLATED=\"$PWD\"/.git/info/refs \\\n-- \n2.19.0.1202.g68e1e8f04e\n\n"},{"id":"363994","messageId":"20181124075838.GG5348@jessie.local","threadId":"49855","inReplyTo":"xmqq7eh23ojc.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH] t5562: do not reuse output files","fromName":"Max Kirillov","fromEmail":"max@max630.net","sentAt":"2018-11-24T07:58:38Z","receivedAt":"2018-11-24T07:58:44Z","isPatch":true,"sender":{"key":"max@max630.net","avatar":"https://avatars.githubusercontent.com/u/381560?v=4"},"body":"On Sat, Nov 24, 2018 at 04:47:19PM +0900, Junio C Hamano wrote:\n> Junio C Hamano <gitster@pobox.com> writes:\n> a better workaround might be to write into unique output filenames\n> (act1.out, act2.out, etc.); that way, you do not have to worry about\n> the output file for the next request getting clobbered by a stale\n> process handling the previous request.\n\nYes I agree\n\n> But at the same time,\n> wouldn't this suggest that the test or the previous request may see\n> an incomplete output,\n\nYes it may miss the child's message. But in this case of failed\nhttp-backend process, there should be already one \"fatal:\"\nmessage in the act.err from the parent, and missing another\none does not change the outcome.\n"},{"id":"363996","messageId":"20181124093719.10705-1-max@max630.net","threadId":"49855","inReplyTo":"xmqq7eh23ojc.fsf@gitster-ct.c.googlers.com","subject":"[PATCH v3] t5562: do not reuse output files","fromName":"Max Kirillov","fromEmail":"max@max630.net","sentAt":"2018-11-24T09:37:19Z","receivedAt":"2018-11-24T09:37:31Z","isPatch":true,"sender":{"key":"max@max630.net","avatar":"https://avatars.githubusercontent.com/u/381560?v=4"},"body":"Some expected failures of git-http-backend leaves running its children\n(receive-pack or upload-pack) which still hold opened descriptors\nto act.err and with some probability they live long enough to write\nthere their failure messages after next test has already truncated\nthe files. This causes occasional failures of the test script.\n\nAvoid the issue by using separated output and error file for each test,\napprending the test number to their name.\n\nReported-by: Carlo Arenas <carenas@gmail.com>\nHelped-by: Carlo Arenas <carenas@gmail.com>\nHelped-by: Junio C Hamano <gitster@pobox.com>\nSigned-off-by: Max Kirillov <max@max630.net>\n---\nUse another output and error files for each test\n t/t5562-http-backend-content-length.sh | 8 ++++----\n 1 file changed, 4 insertions(+), 4 deletions(-)\n\ndiff --git a/t/t5562-http-backend-content-length.sh b/t/t5562-http-backend-content-length.sh\nindex 90d890d02f..9ebbd77bbb 100755\n--- a/t/t5562-http-backend-content-length.sh\n+++ b/t/t5562-http-backend-content-length.sh\n@@ -8,12 +8,12 @@ test_lazy_prereq GZIP 'gzip --version'\n verify_http_result() {\n \t# some fatal errors still produce status 200\n \t# so check if there is the error message\n-\tif grep 'fatal:' act.err\n+\tif grep 'fatal:' act.err.$test_count\n \tthen\n \t\treturn 1\n \tfi\n \n-\tif ! grep \"Status\" act.out >act\n+\tif ! grep \"Status\" act.out.$test_count >act\n \tthen\n \t\tprintf \"Status: 200 OK\\r\\n\" >act\n \tfi\n@@ -33,7 +33,7 @@ test_http_env() {\n \t\tREQUEST_METHOD=POST \\\n \t\t\"$PERL_PATH\" \\\n \t\t\"$TEST_DIRECTORY\"/t5562/invoke-with-content-length.pl \\\n-\t\t    \"$request_body\" git http-backend >act.out 2>act.err\n+\t\t    \"$request_body\" git http-backend >act.out.$test_count 2>act.err.$test_count\n }\n \n ssize_b100dots() {\n@@ -161,7 +161,7 @@ test_expect_success 'empty CONTENT_LENGTH' '\n \t\tGIT_HTTP_EXPORT_ALL=TRUE \\\n \t\tREQUEST_METHOD=GET \\\n \t\tCONTENT_LENGTH=\"\" \\\n-\t\tgit http-backend <empty_body >act.out 2>act.err &&\n+\t\tgit http-backend <empty_body >act.out.$test_count 2>act.err.$test_count &&\n \tverify_http_result \"200 OK\"\n '\n \n-- \n2.19.0.1202.g68e1e8f04e\n\n"},{"id":"364000","messageId":"20181124121036.GC19257@sigill.intra.peff.net","threadId":"49855","inReplyTo":"20181122233821.17871-1-max@max630.net","subject":"Re: [PATCH] t5562: fix perl path","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-11-24T12:10:37Z","receivedAt":"2018-11-24T12:10:40Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Nov 23, 2018 at 01:38:21AM +0200, Max Kirillov wrote:\n\n> From: Jeff King <peff@peff.net>\n> \n> Some systems do not have perl installed to /usr/bin. Use the variable\n> from the build settiings, and call perl directly than via shebang.\n> \n> Signed-off-by: Max Kirillov <max@max630.net>\n> ---\n> Submitting. Could you sign-off? Also removed shebang from the script as it is not needed\n\nYep:\n\n  Signed-off-by: Jeff King <peff@peff.net>\n\nAs Carlos mentioned, I think you could leave the shebang as\ndocumentation, but I'm OK either way.\n\n-Peff\n"},{"id":"364001","messageId":"20181124121445.GD19257@sigill.intra.peff.net","threadId":"49855","inReplyTo":"xmqq7eh23ojc.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH] t5562: do not reuse output files","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-11-24T12:14:45Z","receivedAt":"2018-11-24T12:14:48Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Nov 24, 2018 at 04:47:19PM +0900, Junio C Hamano wrote:\n\n> I do agree that forcing the parent to wait, like you described in\n> the comment, would be far more preferrable, [...]\n\nStray processes can sometimes have funny effects on an outer test\nharness, too. E.g., I think I've seen hangs running t5562 under prove,\nbecause some process is holding open a pipe descriptor. This would\nprobably fix that, too.\n\n> but until that happens,[...]\n\nBut if we can't do that immediately for some reason, I do agree with\neverything else you said here. ;)\n\n-Peff\n"},{"id":"364002","messageId":"20181124130337.GH5348@jessie.local","threadId":"49855","inReplyTo":"xmqq7eh23ojc.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH] t5562: do not reuse output files","fromName":"Max Kirillov","fromEmail":"max@max630.net","sentAt":"2018-11-24T13:03:37Z","receivedAt":"2018-11-24T13:03:42Z","isPatch":true,"sender":{"key":"max@max630.net","avatar":"https://avatars.githubusercontent.com/u/381560?v=4"},"body":"On Sat, Nov 24, 2018 at 04:47:19PM +0900, Junio C Hamano wrote:\n> I do agree that forcing the parent to wait, like you described in\n> the comment, would be far more preferrable,\n\nIt looks like it can be done as simple as:\n\n--- a/http-backend.c\n+++ b/http-backend.c\n@@ -486,6 +486,8 @@ static void run_service(const char **argv, int buffer_input)\n        if (buffer_input || gzipped_request || req_len >= 0)\n                cld.in = -1;\n        cld.git_cmd = 1;\n+       cld.clean_on_exit = 1;\n+       cld.wait_after_clean = 1;\n        if (start_command(&cld))\n                exit(1);\n\nat least according to strate it does what it should.\n"},{"id":"364003","messageId":"20181124134827.13932-1-max@max630.net","threadId":"49855","inReplyTo":"20181124130337.GH5348@jessie.local","subject":"[PATCH] http-backend: enable cleaning up forked upload/receive-pack on exit","fromName":"Max Kirillov","fromEmail":"max@max630.net","sentAt":"2018-11-24T13:48:27Z","receivedAt":"2018-11-24T13:48:34Z","isPatch":true,"sender":{"key":"max@max630.net","avatar":"https://avatars.githubusercontent.com/u/381560?v=4"},"body":"If http-backend dies because of errors, started upload-pack or\nreceive-pack are not killed and waited, but rather stay running for somtime\nuntil they exits because of closed stdin. It may be undesirable in working\nenvironment, and it also causes occasional failure of t5562, because the\nprocesses keep opened act.err, and sometimes write there errors after next test\nstarted using the file.\n\nFix by enabling cleaning of the command at http-backed exit.\n\nReported-by: Carlo Arenas <carenas@gmail.com>\nHelped-by: Carlo Arenas <carenas@gmail.com>\nSigned-off-by: Max Kirillov <max@max630.net>\n---\nThis seems to fix the issue at NetBSD. I verified it manually with strace but could\nnot catch the visible timing effect in tests at Linux. So no tests for it.\n\nthe \"t5562: do not reuse output files\" patches are not needed then\n http-backend.c | 2 ++\n 1 file changed, 2 insertions(+)\n\ndiff --git a/http-backend.c b/http-backend.c\nindex 9e894f197f..29e68e38b5 100644\n--- a/http-backend.c\n+++ b/http-backend.c\n@@ -486,6 +486,8 @@ static void run_service(const char **argv, int buffer_input)\n \tif (buffer_input || gzipped_request || req_len >= 0)\n \t\tcld.in = -1;\n \tcld.git_cmd = 1;\n+\tcld.clean_on_exit = 1;\n+\tcld.wait_after_clean = 1;\n \tif (start_command(&cld))\n \t\texit(1);\n \n-- \n2.19.0.1202.g68e1e8f04e\n\n"},{"id":"364046","messageId":"xmqqlg5g1tjj.fsf@gitster-ct.c.googlers.com","threadId":"49855","inReplyTo":"20181124130337.GH5348@jessie.local","subject":"Re: [PATCH] t5562: do not reuse output files","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-11-26T02:06:40Z","receivedAt":"2018-11-26T02:06:52Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Max Kirillov <max@max630.net> writes:\n\n> On Sat, Nov 24, 2018 at 04:47:19PM +0900, Junio C Hamano wrote:\n>> I do agree that forcing the parent to wait, like you described in\n>> the comment, would be far more preferrable,\n>\n> It looks like it can be done as simple as:\n>\n> --- a/http-backend.c\n> +++ b/http-backend.c\n> @@ -486,6 +486,8 @@ static void run_service(const char **argv, int buffer_input)\n>         if (buffer_input || gzipped_request || req_len >= 0)\n>                 cld.in = -1;\n>         cld.git_cmd = 1;\n> +       cld.clean_on_exit = 1;\n> +       cld.wait_after_clean = 1;\n>         if (start_command(&cld))\n>                 exit(1);\n>\n> at least according to strate it does what it should.\n\nSounds sane.\n\nI am offhand not sure what the right value of wait_after_clean for\nthis codepath be, though.  46df6906 (\"execv_dashed_external: wait\nfor child on signal death\", 2017-01-06) made this non-default but\nturned it on for dashed externals (especially to help the case where\nthey spawn a pager), as the parent has nothing other than to wait\nfor the child to exit in that codepath.  Does the same reasoning\napply here, too?\n\nThis is a meta point, but I wonder if there is an easy way to \"grep\"\nfor uses of run-command interface that do *not* set clean_on_exit.\nThe pager that can outlive us long after we exit, kept alive while\nthe user views our output, is an example cited by afe19ff7\n(\"run-command: optionally kill children on exit\", 2012-01-07), and\nwhile I am wondering if the default should hae been to kill the\nchildren instead, such a \"grep\" would have been very useful to know\nwhat codepaths would be affected if we flipped the default.\n"},{"id":"364047","messageId":"xmqqh8g41tdk.fsf@gitster-ct.c.googlers.com","threadId":"49855","inReplyTo":"20181124134827.13932-1-max@max630.net","subject":"Re: [PATCH] http-backend: enable cleaning up forked upload/receive-pack on exit","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-11-26T02:10:15Z","receivedAt":"2018-11-26T02:10:21Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Max Kirillov <max@max630.net> writes:\n\n> If http-backend dies because of errors, started upload-pack or\n> receive-pack are not killed and waited, but rather stay running for somtime\n\n\"sometime\" (will fix locally, no reason for a resend).\n\n> until they exits because of closed stdin. It may be undesirable in working\n\n\"they exit\" (ditto)\n\n> environment, and it also causes occasional failure of t5562, because the\n> processes keep opened act.err, and sometimes write there errors after next test\n> started using the file.\n>\n> Fix by enabling cleaning of the command at http-backed exit.\n\nThanks for a clear explanation.\n\nWill queue.\n\n> Reported-by: Carlo Arenas <carenas@gmail.com>\n> Helped-by: Carlo Arenas <carenas@gmail.com>\n> Signed-off-by: Max Kirillov <max@max630.net>\n> ---\n> This seems to fix the issue at NetBSD. I verified it manually with strace but could\n> not catch the visible timing effect in tests at Linux. So no tests for it.\n>\n> the \"t5562: do not reuse output files\" patches are not needed then\n>  http-backend.c | 2 ++\n>  1 file changed, 2 insertions(+)\n>\n> diff --git a/http-backend.c b/http-backend.c\n> index 9e894f197f..29e68e38b5 100644\n> --- a/http-backend.c\n> +++ b/http-backend.c\n> @@ -486,6 +486,8 @@ static void run_service(const char **argv, int buffer_input)\n>  \tif (buffer_input || gzipped_request || req_len >= 0)\n>  \t\tcld.in = -1;\n>  \tcld.git_cmd = 1;\n> +\tcld.clean_on_exit = 1;\n> +\tcld.wait_after_clean = 1;\n>  \tif (start_command(&cld))\n>  \t\texit(1);\n"},{"id":"364186","messageId":"20181128041737.GI5348@jessie.local","threadId":"49855","inReplyTo":"xmqqlg5g1tjj.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH] t5562: do not reuse output files","fromName":"Max Kirillov","fromEmail":"max@max630.net","sentAt":"2018-11-28T04:17:37Z","receivedAt":"2018-11-28T04:17:42Z","isPatch":true,"sender":{"key":"max@max630.net","avatar":"https://avatars.githubusercontent.com/u/381560?v=4"},"body":"On Mon, Nov 26, 2018 at 11:06:40AM +0900, Junio C Hamano wrote:\n> I am offhand not sure what the right value of wait_after_clean for\n> this codepath be, though.  46df6906 (\"execv_dashed_external: wait\n> for child on signal death\", 2017-01-06) made this non-default but\n> turned it on for dashed externals (especially to help the case where\n> they spawn a pager), as the parent has nothing other than to wait\n> for the child to exit in that codepath.  Does the same reasoning\n> apply here, too?\n\nAs far as I understand, the reason to _not_ wait was that\nthe child might still be expecting closed stdin even after\nreceiving SIGTERM, so parent would wait forever. But this is\nnot clearly the case here. And otherwise there should be no\nreason to not wait, as long as we are interested in\nsynchronous exiting of the child.\n\nIn my Linux experiments the child was exiting because of signal\nearlier than because of closed stdin, but who knows, maybe\nwith some bad luck the signal would be delivered after the\nclosed stdin, and we would still have the issue. After all,\nat Linux it was mostly working even without fix.\n"},{"id":"364219","messageId":"20181128132708.GE30222@szeder.dev","threadId":"49855","inReplyTo":"20181120091107.GA30542@sigill.intra.peff.net","subject":"Re: [PATCH] t5562: skip if NO_CURL is enabled","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2018-11-28T13:27:08Z","receivedAt":"2018-11-28T13:27:14Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Tue, Nov 20, 2018 at 04:11:08AM -0500, Jeff King wrote:\n> On Mon, Nov 19, 2018 at 11:36:08AM -0800, Carlo Arenas wrote:\n> \n> > tests 3-8 seem to fail because perl is hardcoded to /urs/bin/perl in\n> > t5562/invoke-with-content-length.pl, while I seem to be getting some\n> > sporadic errors in 9 with the following output :\n> > \n> > ++ env CONTENT_TYPE=application/x-git-receive-pack-request\n> > QUERY_STRING=/repo.git/git-receive-pack\n> > 'PATH_TRANSLATED=/home/carenas/src/git/t/trash\n> > directory.t5562-http-backend-content-length/.git/git-receive-pack'\n> > GIT_HTTP_EXPORT_ALL=TRUE REQUEST_METHOD=POST\n> > /home/carenas/src/git/t/t5562/invoke-with-content-length.pl push_body\n> > git http-backend\n> > ++ verify_http_result '200 OK'\n> > ++ grep fatal: act.err\n> > Binary file act.err matches\n> > ++ return 1\n> > error: last command exited with $?=1\n> > not ok 9 - push plain\n> > \n> > and the following output in act.err (with a 200 in act)\n> > \n> > fatal: the remote end hung up unexpectedly\n> \n> This bit me today, too, and I can reproduce it by running under my\n> stress-testing script.\n\nI saw this a few times on Travis CI as well.\n\n> Curiously, the act.err file also has 54 NUL bytes before the \"fatal:\"\n> message.\n\nI think those NUL bytes come from the file system.\n\nThe contents of 'act.err' from the previous test ('fetch gzipped\nempty') is usually:\n\n  fatal: request ended in the middle of the gzip stream\n  fatal: the remote end hung up unexpectedly\n\nNotice that the length of the first line is 54 bytes (including the\ntrailing newline).  So I suspect that the following is happening:\n\n  - http-backend in the previous test writes the first line,\n  - that test finishes and this one starts,\n  - this test truncates 'act.err',\n  - and then the still-running http-backend from the previous test\n    finally writes the second line.\n\nSo at this point 'act.err' is empty, but the offset of the fd of the\nredirection still open from the previous test is at 54, so the file\nsystem fills those bytes with NULs.\n\n\n> I tried adding an \"strace\" to see who was producing that\n> output, but I can't seem to get it to fail when running under strace\n> (presumably because the timing is quite different, and this likely some\n> kind of pipe race).\n> \n> -Peff\n"},{"id":"364223","messageId":"878t1dz1wi.fsf@evledraar.gmail.com","threadId":"49855","inReplyTo":"20181122161722.GC28192@sigill.intra.peff.net","subject":"Re: [PATCH] t5562: skip if NO_CURL is enabled","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-11-28T14:56:29Z","receivedAt":"2018-11-28T14:56:34Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Thu, Nov 22 2018, Jeff King wrote:\n\n> On Thu, Nov 22, 2018 at 02:17:01AM -0800, Carlo Arenas wrote:\n>> PS. upstreaming the PERL_PATH fix is likely to be good to do soonish\n>> as I presume at least all BSD might be affected, let me know if you\n>> would rather me do that instead as I suspect we might be deadlocked\n>> otherwise ;)\n>\n> Yeah, the $PERL_PATH thing is totally orthogonal, and should graduate\n> separately.\n\nOn the subject of orthagonal things: This test fails on AIX with /bin/sh\n(but not /bin/bash) due to some interaction of ssize_b100dots and the\nbuild_option function. On that system:\n\n    $ ./git version --build-options\n    git version 2.20.0-rc1\n    cpu: 00FA74164C00\n    no commit associated with this build\n    sizeof-long: 4\n    sizeof-size_t: 4\n\nBut it somehow ends up in the 'die' condition in that case statement. I\ndug around briefly but couldn't find the cause, probably some limitation\nin the shell constructs it supports. Just leaving a note about this...\n"},{"id":"364422","messageId":"20181201195037.GA29120@sigill.intra.peff.net","threadId":"49855","inReplyTo":"20181128132708.GE30222@szeder.dev","subject":"Re: [PATCH] t5562: skip if NO_CURL is enabled","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-12-01T19:50:37Z","receivedAt":"2018-12-01T19:50:40Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Nov 28, 2018 at 02:27:08PM +0100, SZEDER Gábor wrote:\n\n> > Curiously, the act.err file also has 54 NUL bytes before the \"fatal:\"\n> > message.\n> \n> I think those NUL bytes come from the file system.\n> \n> The contents of 'act.err' from the previous test ('fetch gzipped\n> empty') is usually:\n> \n>   fatal: request ended in the middle of the gzip stream\n>   fatal: the remote end hung up unexpectedly\n> \n> Notice that the length of the first line is 54 bytes (including the\n> trailing newline).  So I suspect that the following is happening:\n> \n>   - http-backend in the previous test writes the first line,\n>   - that test finishes and this one starts,\n>   - this test truncates 'act.err',\n>   - and then the still-running http-backend from the previous test\n>     finally writes the second line.\n> \n> So at this point 'act.err' is empty, but the offset of the fd of the\n> redirection still open from the previous test is at 54, so the file\n> system fills those bytes with NULs.\n\nRight, good thinking. Thanks for the explanation!\n\n-Peff\n"},{"id":"364423","messageId":"20181201195336.GB29120@sigill.intra.peff.net","threadId":"49855","inReplyTo":"878t1dz1wi.fsf@evledraar.gmail.com","subject":"Re: [PATCH] t5562: skip if NO_CURL is enabled","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-12-01T19:53:36Z","receivedAt":"2018-12-01T19:53:41Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Nov 28, 2018 at 03:56:29PM +0100, Ævar Arnfjörð Bjarmason wrote:\n\n> \n> On Thu, Nov 22 2018, Jeff King wrote:\n> \n> > On Thu, Nov 22, 2018 at 02:17:01AM -0800, Carlo Arenas wrote:\n> >> PS. upstreaming the PERL_PATH fix is likely to be good to do soonish\n> >> as I presume at least all BSD might be affected, let me know if you\n> >> would rather me do that instead as I suspect we might be deadlocked\n> >> otherwise ;)\n> >\n> > Yeah, the $PERL_PATH thing is totally orthogonal, and should graduate\n> > separately.\n> \n> On the subject of orthagonal things: This test fails on AIX with /bin/sh\n> (but not /bin/bash) due to some interaction of ssize_b100dots and the\n> build_option function. On that system:\n> \n>     $ ./git version --build-options\n>     git version 2.20.0-rc1\n>     cpu: 00FA74164C00\n>     no commit associated with this build\n>     sizeof-long: 4\n>     sizeof-size_t: 4\n> \n> But it somehow ends up in the 'die' condition in that case statement. I\n> dug around briefly but couldn't find the cause, probably some limitation\n> in the shell constructs it supports. Just leaving a note about this...\n\nThat's weird. The functions involved are pretty vanilla. I'd suspect\nsomething funny with the sed invocation:\n\n  build_option () {\n        git version --build-options |\n        sed -ne \"s/^$1: //p\"\n  }\n\nbut that's the one thing that shouldn't be dependent on the shell in\nuse.\n\nCan you manually replicate the shell commands to see where it goes\nwrong?\n\n-Peff\n"}]}