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

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

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

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

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