Taylor Blau [off-list ref] 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
quoted hunk
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
On Tue, Feb 16, 2021 at 11:39:00AM -0800, Junio C Hamano wrote:
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?
Oof. That breaks the new test, but it makes me worried about whether
this can be parsed without ambiguity. I think not, but here I'd defer to
Christian or Jonathan Tan.
- 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
That's definitely the outcome of this patch, but I agree it feels wrong.
I'm not sure that we define the behavior that strictly in
git-interpret-trailers(1), so we have some wiggle room, I guess.
Thanks,
Taylor
On Tue, Feb 16, 2021 at 8:47 PM Taylor Blau [off-list ref] wrote:
On Tue, Feb 16, 2021 at 11:39:00AM -0800, Junio C Hamano wrote:
quoted
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?
Oof. That breaks the new test, but it makes me worried about whether
this can be parsed without ambiguity. I think not, but here I'd defer to
Christian or Jonathan Tan.
Sorry for the late answer on this. It looks like this fell into my
email reading cracks.
My opinion, when I worked on this, was that it's very difficult to
avoid ambiguity, so it's better if `git interpret-trailers` is strict
by default, which could be relaxed later in special cases where there
is not much risk of ambiguity.
It's especially ambiguous because many commit message subjects or even
bodies can be of the form "area: change" which can look like a
trailer. And some people might want to process whole commit messages,
while others might want to process templates that might contain only
trailers.
So I thought that blank lines should not appear in the trailers. And
if any appears, it means that the trailers should start after the last
blank line. This might actually have been already relaxed a bit.
quoted
- 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
That's definitely the outcome of this patch, but I agree it feels wrong.
I'm not sure that we define the behavior that strictly in
git-interpret-trailers(1), so we have some wiggle room, I guess.
Any patch to relax how blank lines and other aspects of trailers
parsing in my opinion should come with some documentation change to
explain what we now accept and what we don't accept, and also tests to
enforce that.