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

From: Christian Couder <christian.couder@gmail.com>
Date: Mar 23, 2021, 15:17 UTC
Message-ID: <CAP8UFD1QG_b6ax-HodLRRcdLKgWJhPDghjLfjnyan1Zi80en7A@mail.gmail.com>
In-Reply-To: <YCwhPG6RaAhU9ljg@nand.local>

On Tue, Feb 16, 2021 at 8:47 PM Taylor Blau <me@ttaylorr.com> wrote:

Show 12 quoted lines

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

Show 9 quoted lines

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