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

Commit Message

Jiang Xin June 12, 2021, 5:07 a.m. UTC

From: Jiang Xin <[e-mail]>

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

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 <[e-mail]>
---
 sideband.c | 4 ++++
 1 file changed, 4 insertions(+)

Comments

Ævar Arnfjörð Bjarmason June 13, 2021, 7:47 a.m. UTC | #1

On Sat, Jun 12 2021, Jiang Xin wrote:

> From: Jiang Xin <[e-mail]>
>
> 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. 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)
>
> 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.

Nice, i.e. let's generally fix the output instead.

> Signed-off-by: Jiang Xin <[e-mail]>
> ---
>  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);
>  			if (linelen > 0) {

I haven't thought about this carefully but isn't there some way to
combine these if/else if/if statementsn that's clearer?

I.e. here we're doing an "if" check for a !lineline and then an "if"
that can't be true if !linelen.

Isn't this the same as:

if (!scratch->len) {
    ...
} else {
        if (!linelen)
            ...
        else if (linelen > 0)
            ...
}

Or are there cases where we take that "linelen > 0" arm if
!scratch->len?

>  				maybe_colorize_sideband(scratch, b, linelen);
>  				strbuf_addstr(scratch, suffix);

Junio C Hamano June 14, 2021, 3:50 a.m. UTC | #2

Jiang Xin <[e-mail]> writes:

> From: Jiang Xin <[e-mail]>
>
> 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 <[e-mail]>
> ---
>  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);

Jiang Xin June 14, 2021, 11:51 a.m. UTC | #3

Junio C Hamano <[e-mail]> 于2021年6月14日周一 上午11:50写道：
>
> Jiang Xin <[e-mail]> writes:
>
> > From: Jiang Xin <[e-mail]>
> >
> > 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 <[e-mail]>
> > ---
> >  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);

The above changes will add suffix to the end of each line, and even an
empty lines.  However, according to the comment in commit ebe8fa738d
(fix display overlap between remote and local progress, 2007-11-04)
which introduced the suffix implementation for the first time, no
suffix should be appended for empty lines.

    /*
     * Let's insert a suffix to clear the end
     * of the screen line, but only if current
     * line data actually contains something.
     */

So my implementation is to try not to break the original
implementation, and keep the linelen unchanged.

The strbuf "scratch" will be reset at line 18th in the while block, so
the nonempty scratch at line 7 indicates the parameter scratch of
demultiplex_sideband() is not empty. With the following patch,
additional suffix is only added before a leading CR in a packet which
is seperated with its message by packet boundary.

```
01    while ((brk = strpbrk(b, "\n\r"))) {
02            int linelen = brk - b;
03
04 +         /* Has no empty scratch from last call of "demultiplex_sideband"
05 +          * and has a leading CR in buf.
06 +          */
07 +         if (scratch->len && !linelen)
08 +                   strbuf_addstr(scratch, suffix);
09            if (!scratch->len)
10                    strbuf_addstr(scratch, DISPLAY_PREFIX);
11            if (linelen > 0) {
12                    maybe_colorize_sideband(scratch, b, linelen);
13                    strbuf_addstr(scratch, suffix);
14            }
15
16            strbuf_addch(scratch, *brk);
17            xwrite(2, scratch->buf, scratch->len);
18            strbuf_reset(scratch);
19
20            b = brk + 1;
21    }
```

Junio C Hamano June 15, 2021, 1:17 a.m. UTC | #4

Jiang Xin <[e-mail]> writes:

>     /*
>      * Let's insert a suffix to clear the end
>      * of the screen line, but only if current
>      * line data actually contains something.
>      */
>
> So my implementation is to try not to break the original
> implementation, and keep the linelen unchanged.

I knew what you wanted to do from your code---I am questioning if
that "only when something is there" was really sensible, or if it
was just attracting bugs.

Thanks.

Jiang Xin June 15, 2021, 1:47 a.m. UTC | #5

Junio C Hamano <[e-mail]> 于2021年6月15日周二 上午9:17写道：
>
> Jiang Xin <[e-mail]> writes:
>
> >     /*
> >      * Let's insert a suffix to clear the end
> >      * of the screen line, but only if current
> >      * line data actually contains something.
> >      */
> >
> > So my implementation is to try not to break the original
> > implementation, and keep the linelen unchanged.
>
> I knew what you wanted to do from your code---I am questioning if
> that "only when something is there" was really sensible, or if it
> was just attracting bugs.
>

@Nicolas, what's your opinion? Is it ok to add clear-to-eol suffix to
each line even empty ones?

--
Jiang Xin

Nicolas Pitre June 15, 2021, 2:11 a.m. UTC | #6

On Tue, 15 Jun 2021, Jiang Xin wrote:

> Junio C Hamano <[e-mail]> 于2021年6月15日周二 上午9:17写道：
> >
> > Jiang Xin <[e-mail]> writes:
> >
> > >     /*
> > >      * Let's insert a suffix to clear the end
> > >      * of the screen line, but only if current
> > >      * line data actually contains something.
> > >      */
> > >
> > > So my implementation is to try not to break the original
> > > implementation, and keep the linelen unchanged.
> >
> > I knew what you wanted to do from your code---I am questioning if
> > that "only when something is there" was really sensible, or if it
> > was just attracting bugs.
> >
> 
> @Nicolas, what's your opinion? Is it ok to add clear-to-eol suffix to
> each line even empty ones?

That would be the simplest thing to do.

But there must have been a reason for doing it otherwise. I just don't 
remember anymore.

Maybe it had to do with progress reporting that does a bunch of 
percentage updates followed by '\r' to remain on the same line, and at 
the end a single '\n' to move to the next line without erasing the final 
status report line. That would be a case for not clearing empty lines.


Nicolas

Jiang Xin June 15, 2021, 3:04 a.m. UTC | #7

Nicolas Pitre <[e-mail]> 于2021年6月15日周二 上午10:11写道：
>
> On Tue, 15 Jun 2021, Jiang Xin wrote:
>
> > Junio C Hamano <[e-mail]> 于2021年6月15日周二 上午9:17写道：
> > >
> > > Jiang Xin <[e-mail]> writes:
> > >
> > > >     /*
> > > >      * Let's insert a suffix to clear the end
> > > >      * of the screen line, but only if current
> > > >      * line data actually contains something.
> > > >      */
> > > >
> > > > So my implementation is to try not to break the original
> > > > implementation, and keep the linelen unchanged.
> > >
> > > I knew what you wanted to do from your code---I am questioning if
> > > that "only when something is there" was really sensible, or if it
> > > was just attracting bugs.
> > >
> >
> > @Nicolas, what's your opinion? Is it ok to add clear-to-eol suffix to
> > each line even empty ones?
>
> That would be the simplest thing to do.
>
> But there must have been a reason for doing it otherwise. I just don't
> remember anymore.
>
> Maybe it had to do with progress reporting that does a bunch of
> percentage updates followed by '\r' to remain on the same line, and at
> the end a single '\n' to move to the next line without erasing the final
> status report line. That would be a case for not clearing empty lines.
>

Thank @Nicolas for helping me understand the story behinds the code.

If there are two sideband #2 packets like this:

    PKTLINE(\2 "<progress-1>" CR "<progress-2>" CR)
    PKTLINE(\2 "<message-3>" LF "<message-4>" LF)

We should append clear-to-eol suffix to "<progress-1>", "<progess-2>"
and "<message-3>" to erase the last message displayed on the same
line.  Even though there is no need to add the clear-to-eol suffix to
"<message-4>", always adding suffix before line breaks (CR or LF) of
nonempty message make it simple to program.

If there are empty messages in sideband #2 packets like this:

    PKTLINE(\2 "<progress-1>" CR LF "<message-2>" LF)
    PKTLINE(\2 "<message-3>" LF)

For the empty message between "<progress-1>" and "<message-2>",
nothing to display and no need to add clear-to-eol suffix.

The issue this patch try to fix is like the following example:

    PKTLINE(\2 "<progress-1>" CR "<progress-2>")
    PKTLINE(\2 CR "<message-3>" LF)

The message "<progress-2>" is displayed without a proper clear-to-eol
suffix, because it's eol (CR) is in another pktline.

Since we can distinguished this case by checking the size of
"scratch", IMHO, it better not add suffix before all line breaks.

--
Jiang Xin

Nicolas Pitre June 15, 2021, 3:26 a.m. UTC | #8

On Tue, 15 Jun 2021, Jiang Xin wrote:

> The issue this patch try to fix is like the following example:
> 
>     PKTLINE(\2 "<progress-1>" CR "<progress-2>")
>     PKTLINE(\2 CR "<message-3>" LF)
> 
> The message "<progress-2>" is displayed without a proper clear-to-eol
> suffix, because it's eol (CR) is in another pktline.

I'd fix this issue with the following logic:

bool pending_clear_to_eol;

my_putchar(c) {
	switch (c) {
	case '\r':
	case '\n':
		pending_clear_to_eol = true;
		break;
	default:
		if (pending_clear_to_eol) {
			clear_to_eol();
			pending_clear_to_eol = false;
		}
		break;
	}
	putchar(c);
}

In other words, you clear the line after printing "remote:" but only if 
there is a non \n or \r coming next.


Nicolas

Junio C Hamano June 15, 2021, 4:46 a.m. UTC | #9

Nicolas Pitre <[e-mail]> writes:

> On Tue, 15 Jun 2021, Jiang Xin wrote:
>
>> The issue this patch try to fix is like the following example:
>> 
>>     PKTLINE(\2 "<progress-1>" CR "<progress-2>")
>>     PKTLINE(\2 CR "<message-3>" LF)
>> 
>> The message "<progress-2>" is displayed without a proper clear-to-eol
>> suffix, because it's eol (CR) is in another pktline.
>
> I'd fix this issue with the following logic:
>
> bool pending_clear_to_eol;
>
> my_putchar(c) {
> 	switch (c) {
> 	case '\r':
> 	case '\n':
> 		pending_clear_to_eol = true;
> 		break;
> 	default:
> 		if (pending_clear_to_eol) {
> 			clear_to_eol();
> 			pending_clear_to_eol = false;
> 		}
> 		break;
> 	}
> 	putchar(c);
> }
>
> In other words, you clear the line after printing "remote:" but only if 
> there is a non \n or \r coming next.

What puzzles me the most in this discussion is why we do this for
LF.  I do understand why we need it for CR---the line we are going
to show message on after emitting CR would be full of leftover
letters we previously have written before emitting CR, so we'd show
the message (to overwrite the initial part enough to show our own
message) and then clear to the end with either ANSI sequence of
sufficient number of whitespaces.  But line feed would take us to a
fresh and blank line---there is nothing to clear, no?

Thanks.

Jiang Xin June 15, 2021, 7:17 a.m. UTC | #10

Junio C Hamano <[e-mail]> 于2021年6月15日周二 下午12:46写道：
>
> Nicolas Pitre <[e-mail]> writes:
>
> > On Tue, 15 Jun 2021, Jiang Xin wrote:
> >
> >> The issue this patch try to fix is like the following example:
> >>
> >>     PKTLINE(\2 "<progress-1>" CR "<progress-2>")
> >>     PKTLINE(\2 CR "<message-3>" LF)
> >>
> >> The message "<progress-2>" is displayed without a proper clear-to-eol
> >> suffix, because it's eol (CR) is in another pktline.
> >
> > I'd fix this issue with the following logic:
> >
> > bool pending_clear_to_eol;
> >
> > my_putchar(c) {
> >       switch (c) {
> >       case '\r':
> >       case '\n':
> >               pending_clear_to_eol = true;
> >               break;
> >       default:
> >               if (pending_clear_to_eol) {
> >                       clear_to_eol();
> >                       pending_clear_to_eol = false;
> >               }
> >               break;
> >       }
> >       putchar(c);
> > }
> >
> > In other words, you clear the line after printing "remote:" but only if
> > there is a non \n or \r coming next.
>
> What puzzles me the most in this discussion is why we do this for
> LF.  I do understand why we need it for CR---the line we are going
> to show message on after emitting CR would be full of leftover
> letters we previously have written before emitting CR, so we'd show
> the message (to overwrite the initial part enough to show our own
> message) and then clear to the end with either ANSI sequence of
> sufficient number of whitespaces.  But line feed would take us to a
> fresh and blank line---there is nothing to clear, no?

I guess this may because sideband #2 messages are printed on the
screen in a background process, it never know a line where it starts
to print has characters on the right.  So it is safe to write an
additional clear-to-eol suffix no matter the message ends with CR or
LF.

--
Jiang Xin

Nicolas Pitre June 15, 2021, 2:46 p.m. UTC | #11

On Tue, 15 Jun 2021, Junio C Hamano wrote:

> Nicolas Pitre <[e-mail]> writes:
> 
> > On Tue, 15 Jun 2021, Jiang Xin wrote:
> >
> >> The issue this patch try to fix is like the following example:
> >> 
> >>     PKTLINE(\2 "<progress-1>" CR "<progress-2>")
> >>     PKTLINE(\2 CR "<message-3>" LF)
> >> 
> >> The message "<progress-2>" is displayed without a proper clear-to-eol
> >> suffix, because it's eol (CR) is in another pktline.
> >
> > I'd fix this issue with the following logic:
> >
> > bool pending_clear_to_eol;
> >
> > my_putchar(c) {
> > 	switch (c) {
> > 	case '\r':
> > 	case '\n':
> > 		pending_clear_to_eol = true;
> > 		break;
> > 	default:
> > 		if (pending_clear_to_eol) {
> > 			clear_to_eol();
> > 			pending_clear_to_eol = false;
> > 		}
> > 		break;
> > 	}
> > 	putchar(c);
> > }
> >
> > In other words, you clear the line after printing "remote:" but only if 
> > there is a non \n or \r coming next.
> 
> What puzzles me the most in this discussion is why we do this for
> LF.  I do understand why we need it for CR---the line we are going
> to show message on after emitting CR would be full of leftover
> letters we previously have written before emitting CR, so we'd show
> the message (to overwrite the initial part enough to show our own
> message) and then clear to the end with either ANSI sequence of
> sufficient number of whitespaces.  But line feed would take us to a
> fresh and blank line---there is nothing to clear, no?

Depends. Suppose the local process is doing the progress report with CR.

Then the remote sends a single line with LF.

You expects the remote line to be displayed over the local progress 
report and the local progress report to be resumed on the following 
line. Without the line clearing you might have leftover garbage on the 
remote message line.


Nicolas

Patch

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);
 			if (linelen > 0) {
 				maybe_colorize_sideband(scratch, b, linelen);
 				strbuf_addstr(scratch, suffix);