Re: [PATCH 1/4] pkt-line: introduce struct packet_writer

From: Jonathan Tan <jonathantanmy@google.com>
Date: Jan 15, 2019, 18:18 UTC
Message-ID: <20190115181810.249142-1-jonathantanmy@google.com>
In-Reply-To: <xmqqbm4juhoc.fsf@gitster-ct.c.googlers.com>

Show 12 quoted lines

> Jonathan Tan <jonathantanmy@google.com> writes:
> 
> The patch itself look quite noisy, but in esssense, at the lowest
> level we used to have a single format_packet() that was used to
> write out normal payload and an error message prefixed with "ERR ";
> now the users of the function are updated to call one of the two
> helper functions, packet_writer_write() or packet_writer_error().
> Most of the patch noise comes from the helper functions at higher
> levels getting updated to pass the packet_writer struct through the
> callchain.
> 
> Which makes tons of sense.

Yes, that's right. Thanks.

Show 8 quoted lines

> > It will be convenient for a future patch if writing options
> > (specifically, whether the written data is to be multiplexed) could be
> > controlled from a single place, so create struct packet_writer to serve
> > as that place, and modify upload-pack to use it.
> 
> I've singled out "ERR " in my comment above, but this only refers to
> "multiplexed".  Are there reasons why we want multiplexing other
> than the "are we sending payload, or an error message"?

The main reason is to allow progress and keepalive messages at any time, actually. I'll add this paragraph at the start of the commit message:

    A future patch will allow the client to request multiplexing of the
    entire fetch response (and not only during packfile transmission), which
    in turn allows the server to send progress and keepalive messages at any
    time during the response.