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

From: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
To: Jiang Xin <worldhello.net@gmail.com>
Cc: Junio C Hamano <gitster@pobox.com>,
	Git List <git@vger.kernel.org>,
	Jiang Xin <zhiyou.jx@alibaba-inc.com>, Jeff King <peff@peff.net>
Subject: Re: [PATCH v2 3/4] sideband: append suffix for message whose CR in next pktline
Date: Sun, 13 Jun 2021 09:47:22 +0200	[thread overview]
Message-ID: <87eed619w3.fsf@evledraar.gmail.com> (raw)
In-Reply-To: <20210612050711.4057-4-worldhello.net@gmail.com>

On Sat, Jun 12 2021, Jiang Xin wrote:

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