git/send-pack.c, branch v2.25.2

Merge branch 'jk/send-pack-check-negative-with-quick'

2019-12-06T23:09:22Z

Performance tweak on "git push" into a repository with many refs that point at objects we have never heard of. * jk/send-pack-check-negative-with-quick: send-pack: use OBJECT_INFO_QUICK to check negative objects

Merge branch 'jk/send-pack-remote-failure'

2019-12-01T17:04:40Z

Error handling after "git push" finishes sending the packdata and waits for the response to the remote side has been improved. * jk/send-pack-remote-failure: send-pack: check remote ref status on pack-objects failure

send-pack: use OBJECT_INFO_QUICK to check negative objects

2019-11-30T17:10:39Z

When pushing, we feed pack-objects a list of both positive and negative objects. The positive objects are what we want to send, and the negative objects are what the other side told us they have, which we can use to limit the size of the push. Before passing along a negative object, send_pack() will make sure we actually have it (since we only know about it because the remote mentioned it, not because it's one of our refs). So it's expected that some of these objects will be missing on the local side. But looking for a missing object is more expensive than one that we have: it triggers reprepare_packed_git() to handle a racy repack, plus it has to explore every alternate's loose object tree (which can be slow if you have a lot of them, or have a high-latency filesystem). This isn't usually a big problem, since repositories you're pushing to don't generally have a large number of refs that are unrelated to what the client has. But there's no reason such a setup is wrong, and it currently performs poorly. We can fix this by using OBJECT_INFO_QUICK, which tells the lookup code that we expect objects to be missing. Notably, it will not re-scan the packs, and it will use the loose cache from 61c7711cfe (sha1-file: use loose object cache for quick existence check, 2018-11-12). The downside is that in the rare case that we race with a local repack, we might fail to feed some objects to pack-objects, making the resulting push larger. But we'd never produce an invalid or incorrect push, just a less optimal one. That seems like a reasonable tradeoff, and we already do similar things on the fetch side (e.g., when marking COMPLETE commits). Signed-off-by: Jeff King Signed-off-by: Junio C Hamano

send-pack: check remote ref status on pack-objects failure

2019-11-13T07:13:03Z

When we're pushing a pack and our local pack-objects fails, we enter an error code path that does a few things: 1. Set the status of every ref to REF_STATUS_NONE 2. Call receive_unpack_status() to try to get an error report from the other side 3. 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. Helped-by: SZEDER Gábor Signed-off-by: Jeff King Signed-off-by: Junio C Hamano

send-pack: never fetch when checking exclusions

2019-10-09T01:46:41Z

When building the packfile to be sent, send_pack() is given a list of remote refs to be used as exclusions. For each ref, it first checks if the ref exists locally, and if it does, passes it with a "^" prefix to pack-objects. However, in a partial clone, the check may trigger a lazy fetch. The additional commit ancestry information obtained during such fetches may show that certain objects that would have been sent are already known to the server, resulting in a smaller pack being sent. But this is at the cost of fetching from many possibly unrelated refs, and the lazy fetches do not help at all in the typical case where the client is up-to-date with the upstream of the branch being pushed. Ensure that these lazy fetches do not occur. Signed-off-by: Jonathan Tan Signed-off-by: Junio C Hamano

Merge branch 'jk/loose-object-cache-oid'

2019-02-07T06:05:27Z

Code clean-up. * jk/loose-object-cache-oid: prefer "hash mismatch" to "sha1 mismatch" sha1-file: avoid "sha1 file" for generic use in messages sha1-file: prefer "loose object file" to "sha1 file" in messages sha1-file: drop has_sha1_file() convert has_sha1_file() callers to has_object_file() sha1-file: convert pass-through functions to object_id sha1-file: modernize loose header/stream functions sha1-file: modernize loose object file functions http: use struct object_id instead of bare sha1 update comment references to sha1_object_info() sha1-file: fix outdated sha1 comment references

convert has_sha1_file() callers to has_object_file()

2019-01-08T17:41:06Z

The only remaining callers of has_sha1_file() actually have an object_id already. They can use the "object" variant, rather than dereferencing the hash themselves. The code changes here were completely generated by the included coccinelle patch. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano

pack-protocol.txt: accept error packets in any context

2019-01-02T21:05:30Z

In the Git pack protocol definition, an error packet may appear only in a certain context. However, servers can face a runtime error (e.g. I/O error) at an arbitrary timing. This patch changes the protocol to allow an error packet to be sent instead of any packet. Without this protocol spec change, when a server cannot process a request, there's no way to tell that to a client. Since the server cannot produce a valid response, it would be forced to cut a connection without telling why. With this protocol spec change, the server can be more gentle in this situation. An old client may see these error packets as an unexpected packet, but this is not worse than having an unexpected EOF. Following this protocol spec change, the error packet handling code is moved to pkt-line.c. Implementation wise, this implementation uses pkt-line to communicate with a subprocess. Since this is not a part of Git protocol, it's possible that a packet that is not supposed to be an error packet is mistakenly parsed as an error packet. This error packet handling is enabled only for the Git pack protocol parsing code considering this. Signed-off-by: Masaya Suzuki Signed-off-by: Junio C Hamano

Use packet_reader instead of packet_read_line

2019-01-02T21:05:27Z

By using and sharing a packet_reader while handling a Git pack protocol request, the same reader option is used throughout the code. This makes it easy to set a reader option to the request parsing code. Signed-off-by: Masaya Suzuki Signed-off-by: Junio C Hamano

send-pack.c: move async's #ifdef NO_PTHREADS back to run-command.c

2018-11-05T04:42:11Z

On systems that do not support multithread, start_async() is implemented with fork(). This implementation details unfortunately leak out at least in send-pack.c [1]. To keep the code base clean of NO_PTHREADS, move the this #ifdef back to run-command.c. The new wrapper function async_with_fork() at least helps suggest that this special "close()" is related to async in fork mode. [1] 09c9957cf7 (send-pack: avoid deadlock when pack-object dies early - 2011-04-25) Signed-off-by: Nguyễn Thái Ngọc Duy Signed-off-by: Junio C Hamano