Re: [PATCH 3/7] pack-protocol.txt: Mark all LFs in push-cert as required

From: Jeff King <peff@peff.net>
Date: Jul 3, 2015, 18:07 UTC
Message-ID: <20150703180718.GB9223@peff.net>
In-Reply-To: <xmqq38155e3s.fsf@gitster.dls.corp.google.com>

On Fri, Jul 03, 2015 at 10:45:59AM -0700, Junio C Hamano wrote:

Show 16 quoted lines

> > Usually flush packets are "0000", and an empty data packet
> > is "0004". Or are you talking about some kind of flush inside the
> > pkt-data stream?
> 
> Neither.  At the wire level there is a difference, but the callers
> of most often used function in pkt-line API, packet_read(), says
> 
> 	while (1) {
> 		len = packet_read();
> 	        if (!len) {
> 	        	/* flush */
> 	                break;
> 		}
> 	        ... do things on the "len" bytes received ...
> 		... and then on to the next packet ...
> 	}

Ah, I see. Yeah, that is a problem. The solutions you proposed seem like good workarounds to me, but we are unfortunately stuck with existing clients and servers which did not behave that way.

I wondered briefly whether this impacted the keepalives we added to `upload-pack` in 05e9515; those are implemented as 0-byte data packets, which we send during the potentially long counting/delta-compression phase before we send out pack data. It works there because the packets actually contain a single sideband byte, so they are never mistaken for a flush packet.

Related, I recently ran into a case where I think we should do the same for pushes. After receiving the pack, `index-pack` may chew on the result for literally minutes (try pushing up the entire linux.git history sometime). We say nothing at all on the wire until we've finished that, run check_everything_connected, and run all hooks. Some clients (or intermediates on the connection) may give up after a few minutes of silence.

I think we should have:

  1. Some progress eye-candy from the server to tell us that something
     is happening, and how close we are to finishing (basically
     "index-pack -v").

  2. When progress is disabled, similar keepalive packets saying "nope,
     no output yet".

For (2), hopefully we can implement it in the same way, and rely on empty sideband-0 packets. I haven't tested it in practice, though (I have some very rough patches for (1) already).

-Peff