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

Matthias Buehlmann [off-list ref] writes:

Thank you for filling out a Git bug report!
Please answer the following questions to help us understand your issue.

Thanks; let's ping our resident trailers expert ;-)

I somehow thought that a trailer is always a single-line thing,
without funky line-folding that we have to deal with when we are
dealing with e-mail headers.

What did you do before the bug happened? (Steps to reproduce your issue)

    create a file containing multiline trailer:

    $ echo "Test" > test.txt
    $ git interpret-trailers --where end --if-exists addIfDifferent
--trailer "SingleLineTrailer: This is a single line trailer"
--in-place test.txt
    $ git interpret-trailers --where end --if-exists addIfDifferent
--trailer "MultiLineTrailer: This is"$'\n a folded multi-line\n
trailer' --in-place test.txt

    parse trailers:

    $ git interpret-trailers --parse test.txt
    SingleLineTrailer: This is a single line trailer
    MultiLineTrailer: This is a folded multi-line trailer

    (so far, all is as expected)

    now, adding multiline trailer containing empty line:

    $ git interpret-trailers --where end --if-exists addIfDifferent
--trailer "MultiLineTrailer: This is"$'\n a folded multi-line\n
trailer which\n \n contains an\n empty line' --in-place test.txt

    parse trailers again:

    $ git interpret-trailers --parse test.txt

What did you expect to happen? (Expected behavior)

I would expect the following output:

    $ git interpret-trailers --parse test.txt
    SingleLineTrailer: This is a single line trailer
    MultiLineTrailer: This is a folded multi-line trailer
    MultiLineTrailer: This is a folded multi-line trailer which
contains an empty line

What happened instead? (Actual behavior)

    no output is generated, but exit code is nevertheless 0:

    $ git interpret-trailers --parse test.txt
    $ echo $?
    0

What's different between what you expected and what actually happened?

    I would expect either to get an output or the call to exit non-zero

Anything else you want to add:

    According to my interpretation of the documentation of git
intepret-trailers, empty lines should be supported if properly folded,
in the same way as for example the PGP signature added to the commit
header contains a (properly folded) empty line

Please review the rest of the bug report below.
You can delete any lines you don't wish to share.


[System Info]
git version:
git version 2.30.0.windows.2
cpu: x86_64
built from commit: f8cbc844b81bf6b9e72178bbe891a86c8bf5e9e7
sizeof-long: 4
sizeof-size_t: 8
shell-path: /bin/sh
uname: Windows 10.0 18363
compiler info: gnuc: 10.2
libc info: no libc information available
$SHELL (typically, interactive shell): C:\Program Files\Git\usr\bin\bash.exe


[Enabled Hooks]
post-commit

On Mon, Feb 15, 2021 at 06:29:55PM -0800, Junio C Hamano wrote:

Matthias Buehlmann [off-list ref] writes:

quoted

Thank you for filling out a Git bug report!
Please answer the following questions to help us understand your issue.

Thanks; let's ping our resident trailers expert ;-)

I'm not Christian, but hopefully I'm an OK substitute :).

I originally thought that this was an ambiguous test, since you could
reasonably say the trailers begin after the blank line in the second
"MultiLineTrailer" block. In that case, neither of the following lines
look like a trailer, so 'git interpret-trailers' could reasonably print
nothing.

But I was being tricked, since I looked at "test.txt" in my editor,
which automatically replaces blank lines (zero or more space characters
ending in a newline) with a single newline. In fact, this isn't
ambiguous at all, since the blank lines are continuations (they are a
single space character and then a newline):

		00000090  65 64 20 6d 75 6c 74 69  2d 6c 69 6e 65 0a 20 74  |ed multi-line. t|
		000000a0  72 61 69 6c 65 72 20 77  68 69 63 68 0a 20 0a 20  |railer which. . |

(see the repeated '0a 20' space + newline pair after "which").

I think that this is a legitimate bug in 'interpret-trailers' that it
doesn't know to continue multi-line trailers that have empty lines in
them.

I thought that this might have dated back to 022349c3b0 (trailer: avoid
unnecessary splitting on lines, 2016-11-02), but checking out that
commit's first parent shows the bug (albeit without --parse, which
didn't exist then).

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.

This patch might do the trick:

--- 8< ---

Subject: [PATCH] trailer.c: handle empty continuation lines

In a multi-line trailer, it is possible to have a continuation line
which contains at least one space character, terminating in a newline.

In this case, the trailer should continue across the blank continuation
line, but 'trailer.c:find_trailer_start()' handles this case
incorrectly.

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.

Reported-by: Matthias Buehlmann <redacted>
Signed-off-by: Taylor Blau <redacted>
---
 t/t7513-interpret-trailers.sh | 23 +++++++++++++++++++++++
 trailer.c                     |  2 +-
 2 files changed, 24 insertions(+), 1 deletion(-)

diff --git a/t/t7513-interpret-trailers.sh b/t/t7513-interpret-trailers.sh
index 6602790b5f..af602ff329 100755
--- a/t/t7513-interpret-trailers.sh
+++ b/t/t7513-interpret-trailers.sh

@@ -1476,4 +1476,27 @@ test_expect_success 'suppress --- handling' '
 	test_cmp expected actual
 '

+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
+'
+
 test_done

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