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

[Junio C Hamano <gitster@pobox.com>]

Jonathan Tan <jonathantanmy@google.com> writes:

> +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?


> +/*
> + * 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.

> 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.

> +	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.

> 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?


> + * 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