On Tue, Jun 21, 2016 at 2:10 PM, Jeff King <peff@xxxxxxxx> wrote: > ... > So this request actually takes _two_ upload-pack instances to serve > (which is not uncommon when we need multiple rounds of > get_common_commits(), though I am a little surprised that would be the > case for a clone). And the first one seems to be missing a closing > "0000" flush packet from the client to end it. > > If that analysis is correct, this isn't affecting operation in any way; > it's just giving a useless message from upload-pack (and as you note, > that could trigger http-backend to exit(1), which may make the webserver > unhappy). > > If we further instrument upload-pack to set GIT_TRACE_PACKET on the > server side, we can see the two requests: > > packet: upload-pack< want 379518c0c94e3b1a0710129d03d5560814a0ba6f multi_ack_detailed no-done side-band-64k thin-pack include-tag ofs-delta agent=git/2.9.0.37.gb3ad8ab.dirty > packet: upload-pack< deepen 1 > packet: upload-pack< 0000 > packet: upload-pack> shallow 379518c0c94e3b1a0710129d03d5560814a0ba6f > packet: upload-pack> 0000 > > packet: upload-pack< want 379518c0c94e3b1a0710129d03d5560814a0ba6f multi_ack_detailed no-done side-band-64k thin-pack include-tag ofs-delta agent=git/2.9.0.37.gb3ad8ab.dirty > packet: upload-pack< deepen 1 > packet: upload-pack< 0000 > packet: upload-pack> shallow 379518c0c94e3b1a0710129d03d5560814a0ba6f > packet: upload-pack> 0000 > packet: upload-pack< done > packet: upload-pack> NAK > packet: upload-pack> 0000 > > I think in the first one we would get "deepen 1" from the client in > receive_needs(), and similarly write out our "shallow" line. But then we > go into get_common_commits() and the client has hung up, which causes > the message. Whereas in the second line it gives us a "done", which > completes the negotiation. > > So my not-very-educated thoughts are: > > 1. The client should probably be sending an extra flush in the first > request. Alternatively, in the stateless-rpc case we should just > accept the lack of flush as an acceptable end to the request. Our pkt-line.c can't deal with eof if I remember correctly (I tried to use pkt-line for the parallel checkout stuff, where workers can also exit early...) so sending extra flush may be easier. Old upload-pack will not have a problem with this extra flush right? I haven't checked upload-pack.c yet... > 2. Presumably the shallowness is what causes the double-request, as > fetch-pack wants to see the shallow list before proceeding. But > since it has no actual commits to negotiate, the negotiation is a > noop. So probably this case could actually happen in a single > request. I seem to recall our discussion a few months(?) ago about quickfetch() where shallow clone would force another fetch initiated from backfill_tags(). I guess that's why you see two fetches. > > I suspect that other fetches could not, though, so I'm not sure how > much effort is worth putting into optimizing. > > -Peff > -- > To unsubscribe from this list: send the line "unsubscribe git" in > the body of a message to majordomo@xxxxxxxxxxxxxxx > More majordomo info at http://vger.kernel.org/majordomo-info.html -- Duy -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html