Re: [PATCH 2/4] sideband: reverse its dependency on pkt-line

From: Junio C Hamano <gitster@pobox.com>
Date: Jan 14, 2019, 20:07 UTC
Message-ID: <xmqq4labuho5.fsf@gitster-ct.c.googlers.com>
In-Reply-To: <4079a1f110086bd412fb4f56bc8b7f8a6a7a8f9f.1547244620.git.jonathantanmy@google.com>

Jonathan Tan <jonathantanmy@google.com> writes:

Show 9 quoted lines

> +int recv_sideband(const char *me, int in_stream, int out)
> +{
> +	char buf[LARGE_PACKET_MAX + 1];
> +	int retval = 0;
> +	int len;
> +
> +	while (1) {
> +		len = packet_read(in_stream, NULL, NULL, buf, LARGE_PACKET_MAX, 0);
> +		retval = diagnose_sideband(me, buf, len);

This name of the helper does not convey much useful information. Let's think about it more later.

> +		switch (retval) {
> +			case SIDEBAND_PRIMARY:

Dedent; case/default arms align with 's' in switch in our codebase.

> +				write_or_die(out, buf + 1, len - 1);
> +				break;

OK, so PRIMARY return (band#1) gives us the payload unwrapped back in "out" and we are responsible for writing it out here.

> +			case SIDEBAND_PROGRESS:
> +				/* already written by diagnose_sideband() */

True, that happens in "switch(band) { case 2: }" in the other helper.

> +				break;
> +			default: /* flush or error */
> +				return retval;

Lack of corresponding comment bothers readers. In all of REMOTE_ERROR, PROGRESS and PROTOCOL_ERROR cases, the other helper stuffs the message in outbuf in "switch (band) { ... }" and writes it out with xwrite(2, outbuf.buf, outbuf.len) [*1*], so I can see there is no need for us to write anything out here. Perhaps

		case SIDEBAND_FLUSH:
		default: /* errors: message already written */
			return retval;

or something to clarify?

Show 14 quoted lines

> +/*
> + * Receive multiplexed output stream over git native protocol.
> + * in_stream is the input stream from the remote, which carries data
> + * in pkt_line format with band designator.  Demultiplex it into out
> + * and err and return error appropriately.  Band #1 carries the
> + * primary payload.  Things coming over band #2 is not necessarily
> + * error; they are usually informative message on the standard error
> + * stream, aka "verbose").  A message over band #3 is a signal that
> + * the remote died unexpectedly.  A flush() concludes the stream.
> + *
> + * Returns SIDEBAND_FLUSH upon a normal conclusion, and SIDEBAND_PROTOCOL_ERROR
> + * or SIDEBAND_REMOTE_ERROR if an error occurred.
> + */
> +int recv_sideband(const char *me, int in_stream, int out);

This is well described.

Show 19 quoted lines

> diff --git a/sideband.c b/sideband.c
> index 368647acf8..842a92e975 100644
> --- a/sideband.c
> +++ b/sideband.c
> ...
> +int diagnose_sideband(const char *me, char *buf, int len)
>  {
> +	static const char *suffix;
>  	struct strbuf outbuf = STRBUF_INIT;
>  	int retval = 0;
> +	const char *b, *brk;
> +	int band;
> +
> +	if (!suffix) {
> +		if (isatty(2) && !is_terminal_dumb())
> +			suffix = ANSI_SUFFIX;
> +		else
> +			suffix = DUMB_SUFFIX;
> +	}

It may be worth considering a further clean-up for the progress code, that consumes lines in the "switch(band)" below that are disproportionate to what it does in this function by introducing another helper function that is called from here. When it happens, the above "suffix" thing should move to the helper function, too.

Show 27 quoted lines

> +	if (len == 0) {
> +		retval = SIDEBAND_FLUSH;
> +		goto cleanup;
> +	}
> +	if (len < 1) {
> +		strbuf_addf(&outbuf,
> +			    "%s%s: protocol error: no band designator",
> +			    outbuf.len ? "\n" : "", me);
> +		retval = SIDEBAND_PROTOCOL_ERROR;
> +		goto cleanup;
> +	}
> +	band = buf[0] & 0xff;
> +	buf[len] = '\0';
> +	len--;
> +	switch (band) {
> +	case 3:
> +...
> +	case 2:
> +...
> +	case 1:
> +		retval = SIDEBAND_PRIMARY;
> +		break;
> +	default:
> +		strbuf_addf(&outbuf, "%s%s: protocol error: bad band #%d",
> +			    outbuf.len ? "\n" : "", me, band);
> +		retval = SIDEBAND_PROTOCOL_ERROR;
> +		break;

So, the point of this helper is to inspect a single packet-line data and dispatch the payload of the packet based on which band it is sent to. Perhaps call it with a name with demultiplex or dispatch in it? "diagnose" is a bit unclear in that what trait you are diagnosing and for what purpose.

Show 17 quoted lines

> diff --git a/sideband.h b/sideband.h
> index 7a8146f161..a56cd86287 100644
> --- a/sideband.h
> +++ b/sideband.h
> @@ -3,8 +3,21 @@
>  
>  #define SIDEBAND_PROTOCOL_ERROR -2
>  #define SIDEBAND_REMOTE_ERROR -1
> +#define SIDEBAND_FLUSH 0
> +#define SIDEBAND_PRIMARY 1
> +#define SIDEBAND_PROGRESS 2
> +
> +/*
> + * buf and len should be the result of reading a line from a remote sending
> + * multiplexed data.
> + *
> + * Determines the nature of the result and returns it. If

"the result" may be from the point of view of the implementor or the "recv_sideband()" function who called packet_read(), but a casual reader would perceive it more natural if you referred it as "a packet read from a remote". "Inspect a packet read from the remote and returns which sideband it is for", perhaps?

Show 10 quoted lines

> + * SIDEBAND_PROTOCOL_ERROR, SIDEBAND_REMOTE_ERROR, or SIDEBAND_PROGRESS, also
> + * prints a message (or the formatted contents of the notice in the case of
> + * SIDEBAND_PROGRESS) to stderr.
> + */
> +int diagnose_sideband(const char *me, char *buf, int len);
>  
> -int recv_sideband(const char *me, int in_stream, int out);
>  void send_sideband(int fd, int band, const char *data, ssize_t sz, int packet_max);
>  
>  #endif