Re: [PATCH v2 3/4] sideband: append suffix for message whose CR in next pktline

>
> Jiang Xin <worldhello.net@gmail.com> writes:
>
> > From: Jiang Xin <zhiyou.jx@alibaba-inc.com>
> >
> > When calling "demultiplex_sideband" on a sideband-2 message, will try to
> > split the message by line breaks, and append a suffix to each nonempty
> > line to clear the end of the screen line.
>
> Subject of "will try" and "append" is missing.  Do you mean that
> the helper function in question does these two things?  I.e.
>
>         demultiplex_sideband() used on a sideband #2 will try
>         to... and appends ...
>
> > But in the following example,
> > there will be no suffix (8 spaces) for "<message-3>":
> >
> >     PKT-LINE(\2 <message-1> CR <message-2> CR <message-3>)
> >     PKT-LINE(\2 CR <message-4> CR <message-5> CR)
>
> That description may mechanically correct, but
>
>    after <message-3>, we fail to clear to the end of line
>
> may make it easier to understand what the problem we are trying to
> solve for those who do not remember what these suffix games are
> about.
>
> > This is because the line break of "<message-3>" is placed in the next
> > pktline message.
> >
> > Without this fix, t5411 must remove trailing spaces of the actual output
> > of "git-push" command before comparing.
> >
> > Signed-off-by: Jiang Xin <zhiyou.jx@alibaba-inc.com>
> > ---
> >  sideband.c | 4 ++++
> >  1 file changed, 4 insertions(+)
> >
> > diff --git a/sideband.c b/sideband.c
> > index 6f9e026732..abf2be98e1 100644
> > --- a/sideband.c
> > +++ b/sideband.c
> > @@ -185,6 +185,10 @@ int demultiplex_sideband(const char *me, int status,
> >
> >                       if (!scratch->len)
> >                               strbuf_addstr(scratch, DISPLAY_PREFIX);
> > +                     else if (!linelen)
> > +                             /* buf has a leading CR which ends the remaining
> > +                              * scratch of last round of "demultiplex_sideband" */
> > +                             strbuf_addstr(scratch, suffix);
>
> The style of multi-line comment needs fixing, but the contents of
> the comment is a bit hard to grok.
>
> >                       if (linelen > 0) {
> >                               maybe_colorize_sideband(scratch, b, linelen);
> >                               strbuf_addstr(scratch, suffix);
>
> I wonder if the following is simpler to read, though.
>
> -- >8 --
> Subject: [PATCH] sideband: don't lose clear-to-eol at packet boundary
>
> When demultiplex_sideband() sees a CR or LF on the sideband #2, it
> adds "suffix" string to clear to the end of the current line, which
> helps when relaying a progress display whose records are terminated
> with CRs.
>
> The code however forgot that depending on the length of the payload
> line, such a CR may fall exactly at the packet boundary and the
> number of bytes before the CR from the beginning of the packet could
> be zero.  In such a case, the message that was terminated by the CR
> were leftover in the "scratch" buffer in the previous call to the
> function and we still need to clear to the end of the current line.
>
> Just remove the unnecessary check on linelen; maybe_colorize_sideband()
> on 0-byte payload turns into a no-op, and we should be adding clear-to-eol
> for each and every CR/LF anyway.
>
>  sideband.c | 7 +++----
>  1 file changed, 3 insertions(+), 4 deletions(-)
>
> diff --git c/sideband.c w/sideband.c
> index 6f9e026732..1575bf16dd 100644
> --- c/sideband.c
> +++ w/sideband.c
> @@ -185,10 +185,9 @@ int demultiplex_sideband(const char *me, int status,
>
>                         if (!scratch->len)
>                                 strbuf_addstr(scratch, DISPLAY_PREFIX);
> -                       if (linelen > 0) {
> -                               maybe_colorize_sideband(scratch, b, linelen);
> -                               strbuf_addstr(scratch, suffix);
> -                       }
> +
> +                       maybe_colorize_sideband(scratch, b, linelen);
> +                       strbuf_addstr(scratch, suffix);
>
>                         strbuf_addch(scratch, *brk);
>                         xwrite(2, scratch->buf, scratch->len);