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

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.