Re: http-backend fatal error message on shallow clone

From: Duy Nguyen <pclouds@gmail.com>
Date: Jun 21, 2016, 16:41 UTC
Message-ID: <CACsJy8DQGE2fAgzDdJjL82-UrS00zVwy+u1Jd06xDdYmfC94-w@mail.gmail.com>
In-Reply-To: <20160621121041.GA29156@sigill.intra.peff.net>

On Tue, Jun 21, 2016 at 2:10 PM, Jeff King <peff@peff.net> wrote:

Show 41 quoted lines

> ...
> 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...

Show 5 quoted lines

>   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.

Show 9 quoted lines

>
>      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@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

-- 
Duy