Re: [PATCH v2 3/3] pkt-line: do not chomp newlines for sideband messages

[Junio C Hamano <gitster@pobox.com>]

Jiang Xin <worldhello.net@gmail.com> writes:

> Add new flag "PACKET_READ_USE_SIDEBAND" for "packet_read_with_status()"
> to prevent mangling newline characters in sideband messages.
>
> Helped-by: Jonathan Tan <jonathantanmy@google.com>
> Signed-off-by: Jiang Xin <zhiyou.jx@alibaba-inc.com>
> ---
>  pkt-line.c             | 32 ++++++++++++++++++++++++++++++--
>  pkt-line.h             |  1 +
>  t/t0070-fundamental.sh |  2 +-
>  3 files changed, 32 insertions(+), 3 deletions(-)
>
> diff --git a/pkt-line.c b/pkt-line.c
> index 5943777a17..865ad19484 100644
> --- a/pkt-line.c
> +++ b/pkt-line.c
> @@ -462,8 +462,33 @@ enum packet_read_status packet_read_with_status(int fd, char **src_buffer,
>  	}
>  
>  	if ((options & PACKET_READ_CHOMP_NEWLINE) &&
> -	    len && buffer[len-1] == '\n')
> -		len--;
> +	    len && buffer[len-1] == '\n') {
> +		if (options & PACKET_READ_USE_SIDEBAND) {
> +			int band = *buffer & 0xff;
> +			switch (band) {
> +			case 1:
> +				/* Chomp newline for payload */
> +				len--;
> +				break;
> +			case 2:
> +				/* fallthrough */
> +			case 3:
> +				/*
> +				 * Do not chomp newline for progress and error
> +				 * message.
> +				 */
> +				break;
> +			default:
> +				/*
> +				 * Bad sideband, let's leave it to
> +				 * demultiplex_sideband() to catch this error.
> +				 */
> +				break;
> +			}
> +		} else {
> +			len--;
> +		}
> +	}

That's a mouthful and we could shorten it a lot, but is very easy to
follow the logic ;-)

> diff --git a/t/t0070-fundamental.sh b/t/t0070-fundamental.sh
> index a927c665d6..138c2becc1 100755
> --- a/t/t0070-fundamental.sh
> +++ b/t/t0070-fundamental.sh
> @@ -97,7 +97,7 @@ test_expect_success 'unpack-sideband with demultiplex_sideband(), no chomp newli
>  	test_cmp expect-err err
>  '
>  
> -test_expect_failure 'unpack-sideband with demultiplex_sideband(), chomp newline' '
> +test_expect_success 'unpack-sideband with demultiplex_sideband(), chomp newline' '
>  	test_when_finished "rm -f expect-out expect-err" &&
>  	test-tool pkt-line send-split-sideband >split-sideband &&
>  	test-tool pkt-line unpack-sideband \

We cannot quite see what got fixed that is outside the postimage,
but it is nice that we have one fewer test_expect_failure in the
end.

Will queue.  Thanks.