> Jonathan Tan <jonathantanmy@xxxxxxxxxx> 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.
OK - after reading the later comments, switched to demultiplex_sideband.
> > + switch (retval) {
> > + case SIDEBAND_PRIMARY:
>
> Dedent; case/default arms align with 's' in switch in our codebase.
Done.
> > + 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?
Done.
> > +/*
> > + * 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.
Thanks, although the credit should be given to the original author - most of
this was moved.
> > 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.
That's reasonable. If it's OK, I would like to limit the scope of this
patch to splitting the existing recv_sideband() into recv_sideband() and
demultiplex_sideband() (the latter is quite close to the old
recv_sideband(), and the diff looks larger only because most of it is a
dedent).
> 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.
Changed to demultiplex_sideband.
> > +/*
> > + * 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?
Done.
