Re: [PATCH 1/1] upload-pack: buffer ref advertisement writes

From: Jeff King <peff@peff.net>
Date: Aug 25, 2021, 00:44 UTC
Message-ID: <YSWSd5fMbSD5duOU@coredump.intra.peff.net>
In-Reply-To: <xmqqczq2mseb.fsf@gitster.g>

On Tue, Aug 24, 2021 at 02:42:20PM -0700, Junio C Hamano wrote:

Show 13 quoted lines

> > None of this looks wrong to me, but it might be nice to teach the
> > packet writer a buffered mode that would handle this for us. It would be
> > especially nice to bolt the final flush onto packet_flush(), since it is
> > otherwise easy to forget to do.
> 
> FWIW, the packet-line part of the system was from the beginning
> written with an eye to allow buffering until _flush() comes; we may
> have added some buggy conversation path that deadlocks if we make
> the non-flush packets fully buffered, so there may need some fixes,
> but I do not expect the fallout would be too hard to diagnose.
> 
> It may be worth trying that avenue first before piling on the user
> level buffering like this patch does.

Yeah, I had the same thought. It also feels like this is a problem already solved by stdio. I.e., a lot of the packet_* functions can handle descriptors or strbufs. Why not "FILE *" handles?

It would probably involve using the original descriptor _and_ the filehandle in some cases (e.g., ref advertisement over the handle, and then muxing pack-objects output straight to the descriptor). But that's OK as long we are sensible about flushing at the right moments.

It may not be much less complex than just implementing buffering in the packet_* interfaces, though. The tricky part is likely to be the interface (not itself, but avoiding repetition between all the fd/strbuf/buffered variants).

-Peff