Re: [WIP RFC 4/5] upload-pack: refactor writing of "packfile" line

From: Junio C Hamano <gitster@pobox.com>
Date: Dec 6, 2018, 06:35 UTC
Message-ID: <xmqqzhtj88nw.fsf@gitster-ct.c.googlers.com>
In-Reply-To: <1d678a78a63b7e58988b52c8c0efab38c34a6848.1543879256.git.jonathantanmy@google.com>

Jonathan Tan <jonathantanmy@google.com> writes:

Show 8 quoted lines

> @@ -126,6 +129,12 @@ static int read_pack_objects_stdout(int outfd, struct output_state *os)
>  	}
>  	os->used += readsz;
>  
> +	if (!os->packfile_started) {
> +		os->packfile_started = 1;
> +		if (use_protocol_v2)
> +			packet_write_fmt(1, "packfile\n");

If we fix this function so that the only byte in the buffer is held back without emitted when os->used == 1 as I alluded to, this may have to be done a bit later, as with such a change, it is no longer guaranteed that send_client_data() will be called after this point.

Show 5 quoted lines

> +	}
> +
>  	if (os->used > 1) {
>  		send_client_data(1, os->buffer, os->used - 1);
>  		os->buffer[0] = os->buffer[os->used - 1];

Show 7 quoted lines

> +static void flush_progress_buf(struct strbuf *progress_buf)
> +{
> +	if (!progress_buf->len)
> +		return;
> +	send_client_data(2, progress_buf->buf, progress_buf->len);
> +	strbuf_reset(progress_buf);
> +}

Interesting.

Show 20 quoted lines

>  static void create_pack_file(const struct object_array *have_obj,
> -			     const struct object_array *want_obj)
> +			     const struct object_array *want_obj,
> +			     int use_protocol_v2)
>  {
>  	struct child_process pack_objects = CHILD_PROCESS_INIT;
>  	struct output_state output_state = {0};
> @@ -260,9 +278,13 @@ static void create_pack_file(const struct object_array *have_obj,
>  			 */
>  			sz = xread(pack_objects.err, progress,
>  				  sizeof(progress));
> -			if (0 < sz)
> -				send_client_data(2, progress, sz);
> -			else if (sz == 0) {
> +			if (0 < sz) {
> +				if (output_state.packfile_started)
> +					send_client_data(2, progress, sz);
> +				else
> +					strbuf_add(&output_state.progress_buf,
> +						   progress, sz);

Isn't progress output that goes to the channel #2 pretty much independent from the payload stream that carries the pkt-line command like "packfile" plus the raw pack stream? It somehow feels like an oxymoron to _buffer_ progress indicator, as it defeats the whole point of progress report to buffer it.