Re: [PATCH v3 0/7] remote-curl: fix deadlocks when remote server disconnects

[Jeff King <peff@peff.net>]

On Tue, May 19, 2020 at 06:53:53AM -0400, Denton Liu wrote:

> Changes since v2:
> 
> * Use if-else tower in "transport: extract common fetch_pack() call"
> 
> * Rename to `lenbuf_hex` and remove null-terminator sentence in
>   "pkt-line: extern packet_length()"
> 
> * Fix "a a" typo in "remote-curl: error on incomplete packet"
> 
> * Don't send flush packet after response end packet
> 
> * Move stateless delimiter checks closer to where message processing
>   happens in do_fetch_pack_v2()

This looks good to me, modulo the minor comment fixup from Eric. I did
have one final question, though. Our discussion focused a lot on
checking the 0002 packets in the success case. But we didn't discuss how
fetch-pack would deal with an unexpected 0002 packet (i.e., the case
that the server response is truncated, but then remote-curl tacks on its
0002).

It _seems_ to work, because that's the case your invalid-shallow test is
covering. I'm just not sure if it works consistently, or what error we
might produce in some cases (e.g., saying "woah, what's the weird 0002
packet" instead of "the server response ended unexpectedly" or
something).

I suspect any remaining issues there are cosmetic, and it might be OK to
just discover them organically through use. But if you happen to have
done some poking around there, it would be nice to hear it.

Thanks for working on this. When we had the initial discussion, I was
really worried the solution was going to be quite nasty, but I think it
turned out to be reasonably nice.

-Peff