Re: Bug Report: Multi-line trailers containing empty lines break parsing

From: Junio C Hamano <gitster@pobox.com>
Date: Feb 16, 2021, 19:39 UTC
Message-ID: <xmqqmtw3pzu3.fsf@gitster.c.googlers.com>
In-Reply-To: <YCwJ8tORQg2Air4r@nand.local>

Taylor Blau <me@ttaylorr.com> writes:

> Anyway, I'm pretty sure the problem is that
> trailer.c:find_trailer_start() doesn't disambiguate between a blank line
> and one that contains only space characters.

I saw that while reviewing another topic the other day and found it a bit strange, but kept it as I thought it was deliberate and the behaviour (i.e. a line with only blanks and a line that is empty are treated the same) was protected with some tests, but looking at your patch below, I guess there is no such test.

Show 7 quoted lines

> When it encounters a blank line, find_trailer_start() assumes that the
> trailers must begin on the line following the one it's looking at. But
> this isn't the case if the line is a non-empty continuation, in which
> the line may be part of a trailer.
>
> Fix this by only considering a blank line which has exactly zero space
> characters before the LF as delimiting the start of trailers.

Hmph...

Show 22 quoted lines

> +test_expect_success 'handling of empty continuations lines' '
> +	tr _ " " >input <<-\EOF &&
> +	subject
> +
> +	body
> +
> +	trailer: single
> +	multi: one
> +	_two
> +	multi: one
> +	_
> +	_two
> +	_three
> +	EOF
> +	cat >expect <<-\EOF &&
> +	trailer: single
> +	multi: one two
> +	multi: one two three
> +	EOF
> +	git interpret-trailers --parse <input >actual &&
> +	test_cmp expect actual
> +'

A few comments (not pointing out bugs, but just sharing observations).

 - if the line before "trailer: single" were not an empty line but a
   line with a single SP on it (which is is_blank_line()), would the
   new logic get confused?

 - if the second "multi:" trailer did not have the funny blank line
   before "_two", the expected output would still be "multi:"
   followed by "one two three", iow, the line after the second
   "multi: one" is a total no-op?  If we added many more " \n" lines
   there, they are all absorbed and ignored?  It somehow feels wrong

Show 15 quoted lines

> diff --git a/trailer.c b/trailer.c
> index 249ed618ed..7ca7200aec 100644
> --- a/trailer.c
> +++ b/trailer.c
> @@ -846,7 +846,7 @@ static size_t find_trailer_start(const char *buf, size_t len)
>  			possible_continuation_lines = 0;
>  			continue;
>  		}
> -		if (is_blank_line(bol)) {
> +		if (is_blank_line(bol) && *bol == '\n') {
>  			if (only_spaces)
>  				continue;
>  			non_trailer_lines += possible_continuation_lines;
> --
> 2.30.0.667.g81c0cbc6fd