From: Junio C Hamano <gitster@pobox.com>
To: Taylor Blau <me@ttaylorr.com>
Cc: Christian Couder <christian.couder@gmail.com>,
git@vger.kernel.org,
Matthias Buehlmann <Matthias.Buehlmann@mabulous.com>,
Jonathan Tan <jonathantanmy@google.com>
Subject: Re: Bug Report: Multi-line trailers containing empty lines break parsing
Date: Tue, 16 Feb 2021 11:39:00 -0800 [thread overview]
Message-ID: <xmqqmtw3pzu3.fsf@gitster.c.googlers.com> (raw)
In-Reply-To: <YCwJ8tORQg2Air4r@nand.local> (Taylor Blau's message of "Tue, 16 Feb 2021 13:07:46 -0500")
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.
> 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...
> +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
> 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
next prev parent reply other threads:[~2021-02-16 19:39 UTC|newest]
Thread overview: 12+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-02-15 21:54 Bug Report: Multi-line trailers containing empty lines break parsing Matthias Buehlmann
2021-02-16 2:29 ` Junio C Hamano
2021-02-16 18:07 ` Taylor Blau
2021-02-16 19:39 ` Junio C Hamano [this message]
2021-02-16 19:47 ` Taylor Blau
2021-03-23 15:17 ` Christian Couder
2021-03-23 17:39 ` Junio C Hamano
2021-03-25 7:53 ` Christian Couder
2021-03-25 9:33 ` ZheNing Hu
2021-03-25 18:16 ` Junio C Hamano
2021-03-26 10:25 ` ZheNing Hu
2021-03-25 18:08 ` Junio C Hamano
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=xmqqmtw3pzu3.fsf@gitster.c.googlers.com \
--to=gitster@pobox.com \
--cc=Matthias.Buehlmann@mabulous.com \
--cc=christian.couder@gmail.com \
--cc=git@vger.kernel.org \
--cc=jonathantanmy@google.com \
--cc=me@ttaylorr.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.