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

Jiang Xin [off-list ref] 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.

On Tue, 15 Jun 2021, Jiang Xin wrote:

Junio C Hamano [off-list ref] 于2021年6月15日周二 上午9:17写道：

quoted

Jiang Xin [off-list ref] writes:

quoted

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

Junio C Hamano [off-list ref] 于2021年6月15日周二 上午9:17写道：

Jiang Xin [off-list ref] writes:

quoted

    /*
     * 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 [off-list ref] 于2021年6月15日周二 上午10:11写道：

On Tue, 15 Jun 2021, Jiang Xin wrote:

quoted

Junio C Hamano [off-list ref] 于2021年6月15日周二 上午9:17写道：

quoted

Jiang Xin [off-list ref] writes:

quoted

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

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