Note that a push can still freeze (even with postBuffer increased) when its pack files are corrupted (ie pack-objects fails)
That will be fixed in git 2.9 (June 2016). And better managed with Git 2.25 (Q1 2020)
See commit c4b2751, commit df85757, commit 3e8b06d, commit c792d7b, commit 739cf49 (19 Apr 2016) by Jeff King (peff
).
(Merged by Junio C Hamano -- gitster
-- in commit d689301, 29 Apr 2016)
"git push
" from a corrupt repository that attempts to push a large number of refs deadlocked; the thread to relay rejection notices for these ref updates blocked on writing them to the main thread, after the main thread at the receiving end notices that the push failed and decides not to read these notices and return a failure.
Commit 739cf49 has all the details.
send-pack
: close demux pipe before finishing async process
This fixes a deadlock on the client side when pushing a large number of refs from a corrupted repo.
With Git 2.25 (Q1 2020), Error handling after "git push
" finishes sending the packdata and waits for the response to the remote side has been improved.
See commit ad7a403 (13 Nov 2019) by Jeff King (peff
).
(Merged by Junio C Hamano -- gitster
-- in commit 3ae8def, 01 Dec 2019)
send-pack
: check remote ref status on pack-objects failure
Helped-by: SZEDER Gábor
Signed-off-by: Jeff King
When we're pushing a pack and our local pack-objects fails, we enter an error code path that does a few things:
- Set the status of every ref to
REF_STATUS_NONE
- Call
receive_unpack_status()
to try to get an error report from the other side
- Return an error to the caller
If pack-objects
failed because the connection to the server dropped, there's not much more we can do than report the hangup. And indeed, step 2 will try to read a packet from the other side, which will die()
in the packet-reading code with "the remote end hung up unexpectedly
".
But if the connection didn't die, then the most common issue is that the remote index-pack
or unpack-objects
complained about our pack (we could also have a local pack-objects error, but this ends up being the same; we'd send an incomplete pack and the remote side would complain).
In that case we do report the error from the other side (because of step 2), but we fail to say anything further about the refs.
The issue is two-fold:
- in step 1, the "
NONE
" status is not "we saw an error, so we have no status".
It generally means "this ref did not match our refspecs, so we didn't try to push it". So when we print out the push status table, we won't mention any refs at all!
But even if we had a status enum for "pack-objects error", we wouldn't want to blindly set it for every ref. For example, in a non-atomic push we might have rejected some refs already on the client side (e.g., REF_STATUS_REJECT_NODELETE
) and we'd want to report that.
- in step 2, we read just the unpack status.
But receive-pack
will also tell us about each ref (usually that it rejected them because of the unpacker error).
So a much better strategy is to leave the ref status fields as they are (usually EXPECTING_REPORT)
and then actually receive (and print) the full per-ref status.
This case is actually covered in the test suite, as t5504.8, which writes a pack that will be rejected by the remote unpack-objects.
But it's racy. Because our pack is small, most of the time pack-objects manages to write the whole thing before the remote rejects it, and so it returns success and we print out the errors from the remote.
But very occasionally (or when run under --stress
), it goes slow enough to see a failure in writing, and git push
reports nothing for the refs.
With this patch, the test should behave consistently.
There shouldn't be any downside to this approach.
- If we really did see the connection drop, we'd already die in
receive_unpack_status()
, and we'll continue to do so.
- If the connection drops after we get the unpack status but before we see any ref status, we'll still print the remote unpacker error, but will now say "remote end hung up" instead of returning the error up the call-stack.
But as discussed, we weren't showing anything more useful than that with the current code. And anyway, that case is quite unlikely (the connection dropping at that point would have to be unrelated to the pack-objects error, because of the ordering of events).
In the future we might want to handle packet-read errors ourself instead of dying, which would print a full ref status table even for hangups.
But in the meantime, this patch should be a strict improvement.
git repack
andgit push
over and over, and, like the other repo, it worked. But why this error happens, I still don't know. – Deitz