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

3 messages, 3 authors, 2021-06-15 · open the first message on its own page

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

From: Junio C Hamano <hidden>
Date: 2021-06-15 04:46:27

Nicolas Pitre [off-list ref] writes:
On Tue, 15 Jun 2021, Jiang Xin wrote:
quoted
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.

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

From: Jiang Xin <hidden>
Date: 2021-06-15 07:17:29

Junio C Hamano [off-list ref] 于2021年6月15日周二 下午12:46写道:
Nicolas Pitre [off-list ref] writes:
quoted
On Tue, 15 Jun 2021, Jiang Xin wrote:
quoted
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

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

From: Nicolas Pitre <nico@fluxnic.net>
Date: 2021-06-15 14:46:14

On Tue, 15 Jun 2021, Junio C Hamano wrote:
Nicolas Pitre [off-list ref] writes:
quoted
On Tue, 15 Jun 2021, Jiang Xin wrote:
quoted
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
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help