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

[Christian Couder <christian.couder@gmail.com>]

On Tue, Feb 16, 2021 at 8:47 PM Taylor Blau <me@ttaylorr.com> wrote:
>
> 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.

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.

> >  - 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.