{"thread":{"id":"50972","subject":"do people find t5504.8 flaky?","startedAt":"2019-04-23T02:45:22Z","lastAt":"2019-11-18T23:25:23Z","messageCount":9,"participants":["Junio C Hamano","Jeff King","Johannes Schindelin","SZEDER Gábor","Randall S. Becker"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"374291","messageId":"xmqqmukh5tj6.fsf@gitster-ct.c.googlers.com","threadId":"50972","inReplyTo":null,"subject":"do people find t5504.8 flaky?","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-04-23T02:45:17Z","receivedAt":"2019-04-23T02:45:22Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"I have been seeing occasional failures of t5504-fetch-receive-strict\ntest on the cc/replace-graft-peel-tags topic, but it seems that the\nfork point of that topic from the mainline already fails the same\nstep #8, only less frequently.\n\nThe push is rejected as expected, but the remote side that receives\nthe \"push\" fails and the local side does not leave an expected\noutput we expect when the test fails.\n\n"},{"id":"374292","messageId":"20190423030254.GA19604@sigill.intra.peff.net","threadId":"50972","inReplyTo":"xmqqmukh5tj6.fsf@gitster-ct.c.googlers.com","subject":"Re: do people find t5504.8 flaky?","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-04-23T03:02:54Z","receivedAt":"2019-04-23T03:02:58Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Apr 23, 2019 at 11:45:17AM +0900, Junio C Hamano wrote:\n\n> I have been seeing occasional failures of t5504-fetch-receive-strict\n> test on the cc/replace-graft-peel-tags topic, but it seems that the\n> fork point of that topic from the mainline already fails the same\n> step #8, only less frequently.\n> \n> The push is rejected as expected, but the remote side that receives\n> the \"push\" fails and the local side does not leave an expected\n> output we expect when the test fails.\n\nNo, I haven't seen it fail, nor does running with --stress turn up\nanything. But looking at the test I would not be at all surprised if we\nhave races around error hangups. I believe that index-pack will die\nunceremoniously as soon as something fails to pass its fsck check.\n\nThe client will keep sending data, and may hit a SIGPIPE (or the network\nequivalent), depending on how much slack there is in the buffers,\nwhether we hit the problem as a base object or after we receive\neverything and start resolving deltas, etc.\n\nI think after seeing a fatal error we probably ought to consider pumping\nthe rest of the bytes from the client to /dev/null. That's wasteful, but\nit's the only clean way to get a message back, I think. It would also\ngive us the opportunity to complain about other objects, too, if there\nare multiple (it might make sense to abort before resolving deltas,\nthough; at that point we have all of the data and that phase is very CPU\nintensive).\n\n-Peff\n"},{"id":"374640","messageId":"nycvar.QRO.7.76.6.1904290927210.45@tvgsbejvaqbjf.bet","threadId":"50972","inReplyTo":"xmqqmukh5tj6.fsf@gitster-ct.c.googlers.com","subject":"Re: do people find t5504.8 flaky?","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2019-04-29T13:36:25Z","receivedAt":"2019-04-29T13:36:32Z","isPatch":false,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Tue, 23 Apr 2019, Junio C Hamano wrote:\n\n> I have been seeing occasional failures of t5504-fetch-receive-strict\n> test on the cc/replace-graft-peel-tags topic, but it seems that the\n> fork point of that topic from the mainline already fails the same\n> step #8, only less frequently.\n\nSounds familiar. IIRC it fails on Azure Pipelines from time to time, most\noften in linux-clang, less often in linux32. But I don't have hard data on\nthat, I am still trying to get a grip on making the CI builds more robust\nwithout having to ask you to integrate azure-pipelines.yml changes into\n*all* branches.\n\nCurrently, my idea is to have two different Azure Pipelines set up: one\nthat performs the continuous testing using azure-pipelines.yml on your\nmaint, master, next and pu (do you want jch, too?) [*1*], and one that has\na separate build definition and tries to work around many (all?) known\nissues with existing branches, e.g. coping with the known SIGPIPE issues\nin the git-daemon tests on macOS that *have* been addressed in master but\nnot in some of the other branches that are accumulated into pu [*2*]. This\nlatter build is *not* using azure-pipelines.yml, but a build definition\nthat is maintained directly in Azure Pipelines (\"Visual Designer\" is what\nthis feature is called in Azure Pipelines).\n\nCiao,\nDscho\n\nFootnote *1*: https://dev.azure.com/gitgitgadget/git/_build?definitionId=6\n\nFootnote *2*: https://dev.azure.com/gitgitgadget/git/_build?definitionId=4\n"},{"id":"386067","messageId":"20191113000747.GQ4348@szeder.dev","threadId":"50972","inReplyTo":"20190423030254.GA19604@sigill.intra.peff.net","subject":"Re: do people find t5504.8 flaky?","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2019-11-13T00:07:47Z","receivedAt":"2019-11-13T00:07:56Z","isPatch":false,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Mon, Apr 22, 2019 at 11:02:54PM -0400, Jeff King wrote:\n> On Tue, Apr 23, 2019 at 11:45:17AM +0900, Junio C Hamano wrote:\n> \n> > I have been seeing occasional failures of t5504-fetch-receive-strict\n> > test on the cc/replace-graft-peel-tags topic, but it seems that the\n> > fork point of that topic from the mainline already fails the same\n> > step #8, only less frequently.\n> > \n> > The push is rejected as expected, but the remote side that receives\n> > the \"push\" fails and the local side does not leave an expected\n> > output we expect when the test fails.\n\nI've seen it fail a few times on Travis CI, but it's rare, much rarer\nthan our \"avarage\" flaky test failures.\n\nThe subsequent test t5504.9 is flaky as well: the two tests are\nessentially the same, they only differ in the configuration variable\nthat enables the fsck checks.\n\n> No, I haven't seen it fail, nor does running with --stress turn up\n> anything.\n\nI can reproduce the failure fairly quickly with '-r 1,8 --stress' (and\nnr of jobs = 4x cores).\n\nFWIW, I enabled GIT_TRACE_PACKET and the relevant part of the failure\nlooks like this [1]:\n\n  + test_must_fail env GIT_TRACE_PACKET=/home/szeder/src/git/t/trash directory.t5504-fetch-receive-strict.stress-8/trace-packet git push --porcelain dst master:refs/heads/test\n  remote: fatal: object of unexpected type        \n  error: remote unpack failed: unpack-objects abnormal exit\n  error: failed to push some refs to 'dst'\n  + cat trace-packet\n  packet: receive-pack> 0000000000000000000000000000000000000000 capabilities^{}\\0report-status delete-refs side-band-64k quiet atomic ofs-delta agent=git/2.24.0.1.g52e0cf1d06\n  packet: receive-pack> 0000\n  packet:         push< 0000000000000000000000000000000000000000 capabilities^{}\\0report-status delete-refs side-band-64k quiet atomic ofs-delta agent=git/2.24.0.1.g52e0cf1d06\n  packet:         push< 0000\n  packet:         push> 0000000000000000000000000000000000000000 a7943252b7679bec6b9679dbc7863c08610ac2bc refs/heads/test\\0 report-status side-band-64k quiet agent=git/2.24.0.1.g52e0cf1d06\n  packet:         push> 0000\n  packet: receive-pack< 0000000000000000000000000000000000000000 a7943252b7679bec6b9679dbc7863c08610ac2bc refs/heads/test\\0 report-status side-band-64k quiet agent=git/2.24.0.1.g52e0cf1d06\n  packet: receive-pack< 0000\n  packet:     sideband< \\2fatal: object of unexpected type\n  packet: receive-pack> unpack unpack-objects abnormal exit\n  packet: receive-pack> ng refs/heads/test unpacker error\n  packet: receive-pack> 0000\n  packet:     sideband< \\10028unpack unpack-objects abnormal exit0026ng refs/heads/test unpacker error0000\n  packet: receive-pack> 0000\n  packet:     sideband< 0000\n  packet:         push< unpack unpack-objects abnormal exit\n  + test_cmp exp act\n  --- exp 2019-11-12 23:40:33.131679990 +0000\n  +++ act 2019-11-12 23:40:33.203680114 +0000\n  @@ -1,2 +0,0 @@\n  -To dst\n  -!      refs/heads/master:refs/heads/test       [remote rejected] (unpacker error)\n  error: last command exited with $?=1\n  not ok 8 - push with receive.fsckobjects\n\nNote that 'sideband< 0000' is not the last packet.\n\nFor comparison, here is the packet trace from a successful test run:\n\n  + cat trace-packet\n  packet: receive-pack> 0000000000000000000000000000000000000000 capabilities^{}\\0report-status delete-refs side-band-64k quiet atomic ofs-delta agent=git/2.24.0.1.g52e0cf1d06\n  packet: receive-pack> 0000\n  packet:         push< 0000000000000000000000000000000000000000 capabilities^{}\\0report-status delete-refs side-band-64k quiet atomic ofs-delta agent=git/2.24.0.1.g52e0cf1d06\n  packet:         push< 0000\n  packet:         push> 0000000000000000000000000000000000000000 38af865a0f3f0170ef7a18edcb3088d3f7961b21 refs/heads/test\\0 report-status side-band-64k quiet agent=git/2.24.0.1.g52e0cf1d06\n  packet:         push> 0000\n  packet: receive-pack< 0000000000000000000000000000000000000000 38af865a0f3f0170ef7a18edcb3088d3f7961b21 refs/heads/test\\0 report-status side-band-64k quiet agent=git/2.24.0.1.g52e0cf1d06\n  packet: receive-pack< 0000\n  packet:     sideband< \\2fatal: object of unexpected type\n  packet: receive-pack> unpack unpack-objects abnormal exit\n  packet: receive-pack> ng refs/heads/test unpacker error\n  packet: receive-pack> 0000\n  packet:     sideband< \\10028unpack unpack-objects abnormal exit0026ng refs/heads/test unpacker error0000\n  packet:         push< unpack unpack-objects abnormal exit\n  packet:         push< ng refs/heads/test unpacker error\n  packet:         push< 0000\n  packet: receive-pack> 0000\n  packet:     sideband< 0000\n\nNote that 'sideband< 0000' is the final packet.\n\nWhether this confirms Peff's theories below, I don't know; sideband\nalways makes me dizzy :)\n\nFWIW, I could reproduce the failure on ef7e93d908 (do not override\nreceive-pack errors, 2012-02-13) as well, i.e. on the commit that\nstarted checking 'git push's output.\n\nHope it helps.\n\n\n[1] Note the lack of a dozen or so '-x' trace lines from\n    'test_must_fail' and 'test_cmp' ;)  Current WIP patch at:\n\n    https://github.com/szeder/git/commit/52e0cf1d0695c107142e36905dfdbaceacdacf8c\n\n> But looking at the test I would not be at all surprised if we\n> have races around error hangups. I believe that index-pack will die\n> unceremoniously as soon as something fails to pass its fsck check.\n> \n> The client will keep sending data, and may hit a SIGPIPE (or the network\n> equivalent), depending on how much slack there is in the buffers,\n> whether we hit the problem as a base object or after we receive\n> everything and start resolving deltas, etc.\n> \n> I think after seeing a fatal error we probably ought to consider pumping\n> the rest of the bytes from the client to /dev/null. That's wasteful, but\n> it's the only clean way to get a message back, I think. It would also\n> give us the opportunity to complain about other objects, too, if there\n> are multiple (it might make sense to abort before resolving deltas,\n> though; at that point we have all of the data and that phase is very CPU\n> intensive).\n> \n> -Peff\n"},{"id":"386074","messageId":"20191113010337.GA20023@sigill.intra.peff.net","threadId":"50972","inReplyTo":"20191113000747.GQ4348@szeder.dev","subject":"Re: do people find t5504.8 flaky?","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-11-13T01:03:37Z","receivedAt":"2019-11-13T01:03:39Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Nov 13, 2019 at 01:07:47AM +0100, SZEDER Gábor wrote:\n\n> > No, I haven't seen it fail, nor does running with --stress turn up\n> > anything.\n> \n> I can reproduce the failure fairly quickly with '-r 1,8 --stress' (and\n> nr of jobs = 4x cores).\n\nI was able to reproduce with that, too (I wonder why I had so much\ntrouble before? Maybe \"-r 1,8\" simply gives many more opportunities per\nsecond).\n\nRunning with \"-x\", I don't see any sign of a premature SIGPIPE death,\nwhich is good:\n\n  + git push --porcelain dst master:refs/heads/test\n  remote: fatal: object of unexpected type        \n  error: remote unpack failed: unpack-objects abnormal exit\n  error: failed to push some refs to 'dst'\n  + exit_code=1\n\nThe \"exit_code\" assignment there is from test_must_fail. And looking at\nthe code, I think we handle a remote hangup well, because the writer is\nactually a pack-objects sub-process. So we hit this code in send-pack.c:\n\n          rc = finish_command(&po);\n          if (rc) {\n                  /*\n                   * For a normal non-zero exit, we assume pack-objects wrote\n                   * something useful to stderr. For death by signal, though,\n                   * we should mention it to the user. The exception is SIGPIPE\n                   * (141), because that's a normal occurrence if the remote end\n                   * hangs up (and we'll report that by trying to read the unpack\n                   * status).\n                   */\n                  if (rc > 128 && rc != 141)\n                          error(\"pack-objects died of signal %d\", rc - 128);\n                  return -1;\n          }\n\nBut that error return might explain things.\n\nThe difference in your traces seems to be that in the failing case we\nexit immediately after seeing the unpack error:\n\n>   packet:     sideband< \\2fatal: object of unexpected type\n>   packet: receive-pack> unpack unpack-objects abnormal exit\n>   packet: receive-pack> ng refs/heads/test unpacker error\n>   packet: receive-pack> 0000\n>   packet:     sideband< \\10028unpack unpack-objects abnormal exit0026ng refs/heads/test unpacker error0000\n>   packet: receive-pack> 0000\n>   packet:     sideband< 0000\n>   packet:         push< unpack unpack-objects abnormal exit\n>   + test_cmp exp act\n>   --- exp 2019-11-12 23:40:33.131679990 +0000\n>   +++ act 2019-11-12 23:40:33.203680114 +0000\n>   @@ -1,2 +0,0 @@\n>   -To dst\n>   -!      refs/heads/master:refs/heads/test       [remote rejected] (unpacker error)\n>   error: last command exited with $?=1\n>   not ok 8 - push with receive.fsckobjects\n\nNote that \"receive-pack\" sent us the \"ng refs/heads/test\" line. And our\nsideband demuxer even saw it! But it never made it to push.\n\nWhereas in the successful case:\n\n>   packet:     sideband< \\2fatal: object of unexpected type\n>   packet: receive-pack> unpack unpack-objects abnormal exit\n>   packet: receive-pack> ng refs/heads/test unpacker error\n>   packet: receive-pack> 0000\n>   packet:     sideband< \\10028unpack unpack-objects abnormal exit0026ng refs/heads/test unpacker error0000\n>   packet:         push< unpack unpack-objects abnormal exit\n>   packet:         push< ng refs/heads/test unpacker error\n>   packet:         push< 0000\n>   packet: receive-pack> 0000\n>   packet:     sideband< 0000\n\nWe do see \"push\" getting it.\n\nSo for some reason we're deciding not to read the status from out\nsideband demuxer. And that is probably because of this code in\nsend_pack():\n\n                  if (pack_objects(out, remote_refs, extra_have, args) < 0) {\n                          for (ref = remote_refs; ref; ref = ref->next)\n                                  ref->status = REF_STATUS_NONE;\n                          if (args->stateless_rpc)\n                                  close(out);\n                          if (git_connection_is_socket(conn))\n                                  shutdown(fd[0], SHUT_WR);\n  \n                          /*\n                           * Do not even bother with the return value; we know we\n                           * are failing, and just want the error() side effects.\n                           */\n                          if (status_report)\n                                  receive_unpack_status(&reader);\n  \n                          if (use_sideband) {\n                                  close(demux.out);\n                                  finish_async(&demux);\n                          }\n                          fd[1] = -1;\n                          return -1;\n                  }\n\nIf we saw a pack-objects error, then we do bother to receive the unpack\nstatus, but we don't stick around to get the individual ref status, and\neverything stays as REF_STATUS_NONE.\n\nAnd looking in transport_print_push_status(), NONE is skipped when\nprinting. We probably ought to be seeding it with a different status\nthere, but there's not really one for \"hey, _we_ failed\". Let's hack it\nto use REMOTE_REJECT, put put in our own message, like so:\n\ndiff --git a/send-pack.c b/send-pack.c\nindex 34c77cbb1a..f6d609a244 100644\n--- a/send-pack.c\n+++ b/send-pack.c\n@@ -565,8 +565,10 @@ int send_pack(struct send_pack_args *args,\n \n \tif (need_pack_data && cmds_sent) {\n \t\tif (pack_objects(out, remote_refs, extra_have, args) < 0) {\n-\t\t\tfor (ref = remote_refs; ref; ref = ref->next)\n-\t\t\t\tref->status = REF_STATUS_NONE;\n+\t\t\tfor (ref = remote_refs; ref; ref = ref->next) {\n+\t\t\t\tref->status = REF_STATUS_REMOTE_REJECT;\n+\t\t\t\tref->remote_status = xstrdup(\"pack-objects error\");\n+\t\t\t}\n \t\t\tif (args->stateless_rpc)\n \t\t\t\tclose(out);\n \t\t\tif (git_connection_is_socket(conn))\n\nNow when the stress-test fails, I get:\n\n  --- exp\t2019-11-13 00:48:53.269943875 +0000\n  +++ act\t2019-11-13 00:48:53.317943957 +0000\n  @@ -1,2 +1,2 @@\n   To dst\n  -!\trefs/heads/master:refs/heads/test\t[remote rejected] (unpacker error)\n  +!\trefs/heads/master:refs/heads/test\t[remote rejected] (pack-objects error)\n\nSo we're definitely on the right track! Of course we could say \"unpacker\nerror\" and the test failure would go away. But it feels a bit like\nlying, since the remote didn't tell us that.\n\nSo really, what we want for this case is to just get the remote status,\nlike so:\n\ndiff --git a/send-pack.c b/send-pack.c\nindex 34c77cbb1a..d4db965b96 100644\n--- a/send-pack.c\n+++ b/send-pack.c\n@@ -565,19 +565,19 @@ int send_pack(struct send_pack_args *args,\n \n \tif (need_pack_data && cmds_sent) {\n \t\tif (pack_objects(out, remote_refs, extra_have, args) < 0) {\n-\t\t\tfor (ref = remote_refs; ref; ref = ref->next)\n-\t\t\t\tref->status = REF_STATUS_NONE;\n \t\t\tif (args->stateless_rpc)\n \t\t\t\tclose(out);\n \t\t\tif (git_connection_is_socket(conn))\n \t\t\t\tshutdown(fd[0], SHUT_WR);\n \n \t\t\t/*\n \t\t\t * Do not even bother with the return value; we know we\n-\t\t\t * are failing, and just want the error() side effects.\n+\t\t\t * are failing, and just want the error() side effects,\n+\t\t\t * as well as marking refs with the remote status (if\n+\t\t\t * we get one).\n \t\t\t */\n \t\t\tif (status_report)\n-\t\t\t\treceive_unpack_status(&reader);\n+\t\t\t\treceive_status(&reader, remote_refs);\n \n \t\t\tif (use_sideband) {\n \t\t\t\tclose(demux.out);\n\nI was worried at first that we might make things worse for the case that\nthe network has hung up completely (which would cause pack-objects to\nfail, but also cause us to not get anything from the remote). But this\nis really no worse. Even in the existing code, we'd complain to stderr\nabout trying to read the unpack status. And then when we read the remote\nref status, as soon as we see a bad packet we just quietly stop reading\n(thus leaving any unmentioned refs as EXPECTING_REPORT).\n\nSo with that second patch above, the test failure goes away for me.\n\n-Peff\n"},{"id":"386083","messageId":"20191113020719.GA435@sigill.intra.peff.net","threadId":"50972","inReplyTo":"20191113010337.GA20023@sigill.intra.peff.net","subject":"Re: do people find t5504.8 flaky?","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-11-13T02:07:19Z","receivedAt":"2019-11-13T02:07:23Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Nov 12, 2019 at 08:03:37PM -0500, Jeff King wrote:\n\n> So with that second patch above, the test failure goes away for me.\n\nAfter poking at the behavior around connection dropping, I've convinced\nmyself this is the right thing to do. So here it is with a commit\nmessage. I can't get the flaky test to fail anymore, but please let me\nknow if you do.\n\nI didn't see a good way to use stock git to simulate a hung-up\nconnection (because it has to happen after the advertisement, while the\nclient is sending the pack). So I stuck an exit(0) at the end of\nparse_pack_header() in receive-pack, just to verify how things work\nbefore and after my patch (which, spoiler alert, is exactly the same).\n\n-- >8 --\nSubject: [PATCH] send-pack: check remote ref status on pack-objects failure\n\nWhen we're pushing a pack and our local pack-objects fails, we enter an\nerror code path that does a few things:\n\n  1. Set the status of every ref to REF_STATUS_NONE\n\n  2. Call receive_unpack_status() to try to get an error report from\n     the other side\n\n  3. Return an error to the caller\n\nIf pack-objects failed because the connection to the server dropped,\nthere's not much more we can do than report the hangup. And indeed, step\n2 will try to read a packet from the other side, which will die() in the\npacket-reading code with \"the remote end hung up unexpectedly\".\n\nBut if the connection _didn't_ die, then the most common issue is that\nthe remote index-pack or unpack-objects complained about our pack (we\ncould also have a local pack-objects error, but this ends up being the\nsame; we'd send an incomplete pack and the remote side would complain).\n\nIn that case we do report the error from the other side (because of step\n2), but we fail to say anything further about the refs. The issue is\ntwo-fold:\n\n  - in step 1, the \"NONE\" status is not \"we saw an error, so we have no\n    status\". It generally means \"this ref did not match our refspecs, so\n    we didn't try to push it\". So when we print out the push status\n    table, we won't mention any refs at all!\n\n    But even if we had a status enum for \"pack-objects error\", we\n    wouldn't want to blindly set it for every ref. For example, in a\n    non-atomic push we might have rejected some refs already on the\n    client side (e.g., REF_STATUS_REJECT_NODELETE) and we'd want to\n    report that.\n\n  - in step 2, we read just the unpack status. But receive-pack will\n    also tell us about each ref (usually that it rejected them because\n    of the unpacker error).\n\nSo a much better strategy is to leave the ref status fields as they are\n(usually EXPECTING_REPORT) and then actually receive (and print) the\nfull per-ref status.\n\nThis case is actually covered in the test suite, as t5504.8, which\nwrites a pack that will be rejected by the remote unpack-objects. But\nit's racy. Because our pack is small, most of the time pack-objects\nmanages to write the whole thing before the remote rejects it, and so it\nreturns success and we print out the errors from the remote. But very\noccasionally (or when run under --stress) it goes slow enough to see a\nfailure in writing, and git-push reports nothing for the refs.\n\nWith this patch, the test should behave consistently.\n\nThere shouldn't be any downside to this approach. If we really did see\nthe connection drop, we'd already die in receive_unpack_status(), and\nwe'll continue to do so. If the connection drops _after_ we get the\nunpack status but before we see any ref status, we'll still print the\nremote unpacker error, but will now say \"remote end hung up\" instead of\nreturning the error up the call-stack. But as discussed, we weren't\nshowing anything more useful than that with the current code. And\nanyway, that case is quite unlikely (the connection dropping at that\npoint would have to be unrelated to the pack-objects error, because of\nthe ordering of events).\n\nIn the future we might want to handle packet-read errors ourself instead\nof dying, which would print a full ref status table even for hangups.\nBut in the meantime, this patch should be a strict improvement.\n\nHelped-by: SZEDER Gábor <szeder.dev@gmail.com>\nSigned-off-by: Jeff King <peff@peff.net>\n---\n send-pack.c | 8 ++++----\n 1 file changed, 4 insertions(+), 4 deletions(-)\n\ndiff --git a/send-pack.c b/send-pack.c\nindex 34c77cbb1a..a7322d3278 100644\n--- a/send-pack.c\n+++ b/send-pack.c\n@@ -565,8 +565,6 @@ int send_pack(struct send_pack_args *args,\n \n \tif (need_pack_data && cmds_sent) {\n \t\tif (pack_objects(out, remote_refs, extra_have, args) < 0) {\n-\t\t\tfor (ref = remote_refs; ref; ref = ref->next)\n-\t\t\t\tref->status = REF_STATUS_NONE;\n \t\t\tif (args->stateless_rpc)\n \t\t\t\tclose(out);\n \t\t\tif (git_connection_is_socket(conn))\n@@ -574,10 +572,12 @@ int send_pack(struct send_pack_args *args,\n \n \t\t\t/*\n \t\t\t * Do not even bother with the return value; we know we\n-\t\t\t * are failing, and just want the error() side effects.\n+\t\t\t * are failing, and just want the error() side effects,\n+\t\t\t * as well as marking refs with their remote status (if\n+\t\t\t * we get one).\n \t\t\t */\n \t\t\tif (status_report)\n-\t\t\t\treceive_unpack_status(&reader);\n+\t\t\t\treceive_status(&reader, remote_refs);\n \n \t\t\tif (use_sideband) {\n \t\t\t\tclose(demux.out);\n-- \n2.24.0.739.gb5632e4929\n\n"},{"id":"386091","messageId":"xmqqh838bfo3.fsf@gitster-ct.c.googlers.com","threadId":"50972","inReplyTo":"20191113010337.GA20023@sigill.intra.peff.net","subject":"Re: do people find t5504.8 flaky?","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-11-13T03:47:56Z","receivedAt":"2019-11-13T03:48:04Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> So really, what we want for this case is to just get the remote status,\n> like so:\n>\n> diff --git a/send-pack.c b/send-pack.c\n> index 34c77cbb1a..d4db965b96 100644\n> --- a/send-pack.c\n> +++ b/send-pack.c\n> @@ -565,19 +565,19 @@ int send_pack(struct send_pack_args *args,\n>  \n>  \tif (need_pack_data && cmds_sent) {\n>  \t\tif (pack_objects(out, remote_refs, extra_have, args) < 0) {\n> -\t\t\tfor (ref = remote_refs; ref; ref = ref->next)\n> -\t\t\t\tref->status = REF_STATUS_NONE;\n>  \t\t\tif (args->stateless_rpc)\n>  \t\t\t\tclose(out);\n>  \t\t\tif (git_connection_is_socket(conn))\n>  \t\t\t\tshutdown(fd[0], SHUT_WR);\n>  \n>  \t\t\t/*\n>  \t\t\t * Do not even bother with the return value; we know we\n> -\t\t\t * are failing, and just want the error() side effects.\n> +\t\t\t * are failing, and just want the error() side effects,\n> +\t\t\t * as well as marking refs with the remote status (if\n> +\t\t\t * we get one).\n>  \t\t\t */\n>  \t\t\tif (status_report)\n> -\t\t\t\treceive_unpack_status(&reader);\n> +\t\t\t\treceive_status(&reader, remote_refs);\n>  \n>  \t\t\tif (use_sideband) {\n>  \t\t\t\tclose(demux.out);\n>\n> I was worried at first that we might make things worse for the case that\n> the network has hung up completely (which would cause pack-objects to\n> fail, but also cause us to not get anything from the remote). But this\n> is really no worse. Even in the existing code, we'd complain to stderr\n> about trying to read the unpack status. And then when we read the remote\n> ref status, as soon as we see a bad packet we just quietly stop reading\n> (thus leaving any unmentioned refs as EXPECTING_REPORT).\n>\n> So with that second patch above, the test failure goes away for me.\n\nNicely analyzed and patched.\n\n"},{"id":"386472","messageId":"20191118223020.GB23183@szeder.dev","threadId":"50972","inReplyTo":"20191113020719.GA435@sigill.intra.peff.net","subject":"Re: do people find t5504.8 flaky?","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2019-11-18T22:30:20Z","receivedAt":"2019-11-18T22:30:27Z","isPatch":false,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Tue, Nov 12, 2019 at 09:07:19PM -0500, Jeff King wrote:\n> On Tue, Nov 12, 2019 at 08:03:37PM -0500, Jeff King wrote:\n> \n> > So with that second patch above, the test failure goes away for me.\n> \n> After poking at the behavior around connection dropping, I've convinced\n> myself this is the right thing to do. So here it is with a commit\n> message. I can't get the flaky test to fail anymore, but please let me\n> know if you do.\n\nFWIW neither can I.  I clocked in ~3 hours of --stress with the patch\napplied, and no failures  (before it usually failed within 30s).\n\n"},{"id":"386475","messageId":"024a01d59e67$6c08bdf0$441a39d0$@nexbridge.com","threadId":"50972","inReplyTo":"20191118223020.GB23183@szeder.dev","subject":"RE: do people find t5504.8 flaky?","fromName":"Randall S. Becker","fromEmail":"rsbecker@nexbridge.com","sentAt":"2019-11-18T23:25:05Z","receivedAt":"2019-11-18T23:25:23Z","isPatch":false,"sender":{"key":"randall.becker@nexbridge.ca","avatar":"https://avatars.githubusercontent.com/u/28956764?v=4"},"body":"On November 18, 2019 5:30 PM, SZEDER Gábor wrote\n> On Tue, Nov 12, 2019 at 09:07:19PM -0500, Jeff King wrote:\n> > On Tue, Nov 12, 2019 at 08:03:37PM -0500, Jeff King wrote:\n> >\n> > > So with that second patch above, the test failure goes away for me.\n> >\n> > After poking at the behavior around connection dropping, I've\n> > convinced myself this is the right thing to do. So here it is with a\n> > commit message. I can't get the flaky test to fail anymore, but please\n> > let me know if you do.\n> \n> FWIW neither can I.  I clocked in ~3 hours of --stress with the patch applied,\n> and no failures  (before it usually failed within 30s).\n\nNothing flaky so far on either NonStop ports.\n\n"}]}