{"thread":{"id":"55151","subject":"Bug Report: Multi-line trailers containing empty lines break parsing","startedAt":"2021-02-15T21:56:26Z","lastAt":"2021-03-26T10:26:25Z","messageCount":12,"participants":["Matthias Buehlmann","Junio C Hamano","Taylor Blau","Christian Couder","ZheNing Hu"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"416986","messageId":"CALz+XyW+XU++58eEYm5=jxTckK-VuuPoA-ecj4QCZw1o44JFUQ@mail.gmail.com","threadId":"55151","inReplyTo":null,"subject":"Bug Report: Multi-line trailers containing empty lines break parsing","fromName":"Matthias Buehlmann","fromEmail":"matthias.buehlmann@mabulous.com","sentAt":"2021-02-15T21:54:52Z","receivedAt":"2021-02-15T21:56:26Z","isPatch":false,"sender":{"key":"matthias.buehlmann@mabulous.com","avatar":null},"body":"Thank you for filling out a Git bug report!\nPlease answer the following questions to help us understand your issue.\n\nWhat did you do before the bug happened? (Steps to reproduce your issue)\n\n    create a file containing multiline trailer:\n\n    $ echo \"Test\" > test.txt\n    $ git interpret-trailers --where end --if-exists addIfDifferent\n--trailer \"SingleLineTrailer: This is a single line trailer\"\n--in-place test.txt\n    $ git interpret-trailers --where end --if-exists addIfDifferent\n--trailer \"MultiLineTrailer: This is\"$'\\n a folded multi-line\\n\ntrailer' --in-place test.txt\n\n    parse trailers:\n\n    $ git interpret-trailers --parse test.txt\n    SingleLineTrailer: This is a single line trailer\n    MultiLineTrailer: This is a folded multi-line trailer\n\n    (so far, all is as expected)\n\n    now, adding multiline trailer containing empty line:\n\n    $ git interpret-trailers --where end --if-exists addIfDifferent\n--trailer \"MultiLineTrailer: This is\"$'\\n a folded multi-line\\n\ntrailer which\\n \\n contains an\\n empty line' --in-place test.txt\n\n    parse trailers again:\n\n    $ git interpret-trailers --parse test.txt\n\nWhat did you expect to happen? (Expected behavior)\n\nI would expect the following output:\n\n    $ git interpret-trailers --parse test.txt\n    SingleLineTrailer: This is a single line trailer\n    MultiLineTrailer: This is a folded multi-line trailer\n    MultiLineTrailer: This is a folded multi-line trailer which\ncontains an empty line\n\nWhat happened instead? (Actual behavior)\n\n    no output is generated, but exit code is nevertheless 0:\n\n    $ git interpret-trailers --parse test.txt\n    $ echo $?\n    0\n\nWhat's different between what you expected and what actually happened?\n\n    I would expect either to get an output or the call to exit non-zero\n\nAnything else you want to add:\n\n    According to my interpretation of the documentation of git\nintepret-trailers, empty lines should be supported if properly folded,\nin the same way as for example the PGP signature added to the commit\nheader contains a (properly folded) empty line\n\nPlease review the rest of the bug report below.\nYou can delete any lines you don't wish to share.\n\n\n[System Info]\ngit version:\ngit version 2.30.0.windows.2\ncpu: x86_64\nbuilt from commit: f8cbc844b81bf6b9e72178bbe891a86c8bf5e9e7\nsizeof-long: 4\nsizeof-size_t: 8\nshell-path: /bin/sh\nuname: Windows 10.0 18363\ncompiler info: gnuc: 10.2\nlibc info: no libc information available\n$SHELL (typically, interactive shell): C:\\Program Files\\Git\\usr\\bin\\bash.exe\n\n\n[Enabled Hooks]\npost-commit\n"},{"id":"417001","messageId":"xmqqczx0sq1o.fsf@gitster.c.googlers.com","threadId":"55151","inReplyTo":"CALz+XyW+XU++58eEYm5=jxTckK-VuuPoA-ecj4QCZw1o44JFUQ@mail.gmail.com","subject":"Re: Bug Report: Multi-line trailers containing empty lines break parsing","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-02-16T02:29:55Z","receivedAt":"2021-02-16T02:30:42Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Matthias Buehlmann <Matthias.Buehlmann@mabulous.com> writes:\n\n> Thank you for filling out a Git bug report!\n> Please answer the following questions to help us understand your issue.\n\nThanks; let's ping our resident trailers expert ;-)\n\nI somehow thought that a trailer is always a single-line thing,\nwithout funky line-folding that we have to deal with when we are\ndealing with e-mail headers.\n\n\n> What did you do before the bug happened? (Steps to reproduce your issue)\n>\n>     create a file containing multiline trailer:\n>\n>     $ echo \"Test\" > test.txt\n>     $ git interpret-trailers --where end --if-exists addIfDifferent\n> --trailer \"SingleLineTrailer: This is a single line trailer\"\n> --in-place test.txt\n>     $ git interpret-trailers --where end --if-exists addIfDifferent\n> --trailer \"MultiLineTrailer: This is\"$'\\n a folded multi-line\\n\n> trailer' --in-place test.txt\n>\n>     parse trailers:\n>\n>     $ git interpret-trailers --parse test.txt\n>     SingleLineTrailer: This is a single line trailer\n>     MultiLineTrailer: This is a folded multi-line trailer\n>\n>     (so far, all is as expected)\n>\n>     now, adding multiline trailer containing empty line:\n>\n>     $ git interpret-trailers --where end --if-exists addIfDifferent\n> --trailer \"MultiLineTrailer: This is\"$'\\n a folded multi-line\\n\n> trailer which\\n \\n contains an\\n empty line' --in-place test.txt\n>\n>     parse trailers again:\n>\n>     $ git interpret-trailers --parse test.txt\n>\n> What did you expect to happen? (Expected behavior)\n>\n> I would expect the following output:\n>\n>     $ git interpret-trailers --parse test.txt\n>     SingleLineTrailer: This is a single line trailer\n>     MultiLineTrailer: This is a folded multi-line trailer\n>     MultiLineTrailer: This is a folded multi-line trailer which\n> contains an empty line\n>\n> What happened instead? (Actual behavior)\n>\n>     no output is generated, but exit code is nevertheless 0:\n>\n>     $ git interpret-trailers --parse test.txt\n>     $ echo $?\n>     0\n>\n> What's different between what you expected and what actually happened?\n>\n>     I would expect either to get an output or the call to exit non-zero\n>\n> Anything else you want to add:\n>\n>     According to my interpretation of the documentation of git\n> intepret-trailers, empty lines should be supported if properly folded,\n> in the same way as for example the PGP signature added to the commit\n> header contains a (properly folded) empty line\n>\n> Please review the rest of the bug report below.\n> You can delete any lines you don't wish to share.\n>\n>\n> [System Info]\n> git version:\n> git version 2.30.0.windows.2\n> cpu: x86_64\n> built from commit: f8cbc844b81bf6b9e72178bbe891a86c8bf5e9e7\n> sizeof-long: 4\n> sizeof-size_t: 8\n> shell-path: /bin/sh\n> uname: Windows 10.0 18363\n> compiler info: gnuc: 10.2\n> libc info: no libc information available\n> $SHELL (typically, interactive shell): C:\\Program Files\\Git\\usr\\bin\\bash.exe\n>\n>\n> [Enabled Hooks]\n> post-commit\n"},{"id":"417079","messageId":"YCwJ8tORQg2Air4r@nand.local","threadId":"55151","inReplyTo":"xmqqczx0sq1o.fsf@gitster.c.googlers.com","subject":"Re: Bug Report: Multi-line trailers containing empty lines break parsing","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2021-02-16T18:07:46Z","receivedAt":"2021-02-16T18:08:44Z","isPatch":false,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Mon, Feb 15, 2021 at 06:29:55PM -0800, Junio C Hamano wrote:\n> Matthias Buehlmann <Matthias.Buehlmann@mabulous.com> writes:\n>\n> > Thank you for filling out a Git bug report!\n> > Please answer the following questions to help us understand your issue.\n>\n> Thanks; let's ping our resident trailers expert ;-)\n\nI'm not Christian, but hopefully I'm an OK substitute :).\n\nI originally thought that this was an ambiguous test, since you could\nreasonably say the trailers begin after the blank line in the second\n\"MultiLineTrailer\" block. In that case, neither of the following lines\nlook like a trailer, so 'git interpret-trailers' could reasonably print\nnothing.\n\nBut I was being tricked, since I looked at \"test.txt\" in my editor,\nwhich automatically replaces blank lines (zero or more space characters\nending in a newline) with a single newline. In fact, this isn't\nambiguous at all, since the blank lines are continuations (they are a\nsingle space character and then a newline):\n\n\t\t00000090  65 64 20 6d 75 6c 74 69  2d 6c 69 6e 65 0a 20 74  |ed multi-line. t|\n\t\t000000a0  72 61 69 6c 65 72 20 77  68 69 63 68 0a 20 0a 20  |railer which. . |\n\n(see the repeated '0a 20' space + newline pair after \"which\").\n\nI think that this is a legitimate bug in 'interpret-trailers' that it\ndoesn't know to continue multi-line trailers that have empty lines in\nthem.\n\nI thought that this might have dated back to 022349c3b0 (trailer: avoid\nunnecessary splitting on lines, 2016-11-02), but checking out that\ncommit's first parent shows the bug (albeit without --parse, which\ndidn't exist then).\n\nAnyway, I'm pretty sure the problem is that\ntrailer.c:find_trailer_start() doesn't disambiguate between a blank line\nand one that contains only space characters.\n\nThis patch might do the trick:\n\n--- 8< ---\n\nSubject: [PATCH] trailer.c: handle empty continuation lines\n\nIn a multi-line trailer, it is possible to have a continuation line\nwhich contains at least one space character, terminating in a newline.\n\nIn this case, the trailer should continue across the blank continuation\nline, but 'trailer.c:find_trailer_start()' handles this case\nincorrectly.\n\nWhen it encounters a blank line, find_trailer_start() assumes that the\ntrailers must begin on the line following the one it's looking at. But\nthis isn't the case if the line is a non-empty continuation, in which\nthe line may be part of a trailer.\n\nFix this by only considering a blank line which has exactly zero space\ncharacters before the LF as delimiting the start of trailers.\n\nReported-by: Matthias Buehlmann <Matthias.Buehlmann@mabulous.com>\nSigned-off-by: Taylor Blau <me@ttaylorr.com>\n---\n t/t7513-interpret-trailers.sh | 23 +++++++++++++++++++++++\n trailer.c                     |  2 +-\n 2 files changed, 24 insertions(+), 1 deletion(-)\n\ndiff --git a/t/t7513-interpret-trailers.sh b/t/t7513-interpret-trailers.sh\nindex 6602790b5f..af602ff329 100755\n--- a/t/t7513-interpret-trailers.sh\n+++ b/t/t7513-interpret-trailers.sh\n@@ -1476,4 +1476,27 @@ test_expect_success 'suppress --- handling' '\n \ttest_cmp expected actual\n '\n\n+test_expect_success 'handling of empty continuations lines' '\n+\ttr _ \" \" >input <<-\\EOF &&\n+\tsubject\n+\n+\tbody\n+\n+\ttrailer: single\n+\tmulti: one\n+\t_two\n+\tmulti: one\n+\t_\n+\t_two\n+\t_three\n+\tEOF\n+\tcat >expect <<-\\EOF &&\n+\ttrailer: single\n+\tmulti: one two\n+\tmulti: one two three\n+\tEOF\n+\tgit interpret-trailers --parse <input >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_done\ndiff --git a/trailer.c b/trailer.c\nindex 249ed618ed..7ca7200aec 100644\n--- a/trailer.c\n+++ b/trailer.c\n@@ -846,7 +846,7 @@ static size_t find_trailer_start(const char *buf, size_t len)\n \t\t\tpossible_continuation_lines = 0;\n \t\t\tcontinue;\n \t\t}\n-\t\tif (is_blank_line(bol)) {\n+\t\tif (is_blank_line(bol) && *bol == '\\n') {\n \t\t\tif (only_spaces)\n \t\t\t\tcontinue;\n \t\t\tnon_trailer_lines += possible_continuation_lines;\n--\n2.30.0.667.g81c0cbc6fd\n\n"},{"id":"417097","messageId":"xmqqmtw3pzu3.fsf@gitster.c.googlers.com","threadId":"55151","inReplyTo":"YCwJ8tORQg2Air4r@nand.local","subject":"Re: Bug Report: Multi-line trailers containing empty lines break parsing","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-02-16T19:39:00Z","receivedAt":"2021-02-16T19:39:53Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Taylor Blau <me@ttaylorr.com> writes:\n\n> Anyway, I'm pretty sure the problem is that\n> trailer.c:find_trailer_start() doesn't disambiguate between a blank line\n> and one that contains only space characters.\n\nI saw that while reviewing another topic the other day and found it\na bit strange, but kept it as I thought it was deliberate and the\nbehaviour (i.e. a line with only blanks and a line that is empty are\ntreated the same) was protected with some tests, but looking at your\npatch below, I guess there is no such test.\n\n> When it encounters a blank line, find_trailer_start() assumes that the\n> trailers must begin on the line following the one it's looking at. But\n> this isn't the case if the line is a non-empty continuation, in which\n> the line may be part of a trailer.\n>\n> Fix this by only considering a blank line which has exactly zero space\n> characters before the LF as delimiting the start of trailers.\n\nHmph...\n\n> +test_expect_success 'handling of empty continuations lines' '\n> +\ttr _ \" \" >input <<-\\EOF &&\n> +\tsubject\n> +\n> +\tbody\n> +\n> +\ttrailer: single\n> +\tmulti: one\n> +\t_two\n> +\tmulti: one\n> +\t_\n> +\t_two\n> +\t_three\n> +\tEOF\n> +\tcat >expect <<-\\EOF &&\n> +\ttrailer: single\n> +\tmulti: one two\n> +\tmulti: one two three\n> +\tEOF\n> +\tgit interpret-trailers --parse <input >actual &&\n> +\ttest_cmp expect actual\n> +'\n\nA few comments (not pointing out bugs, but just sharing\nobservations).\n\n - if the line before \"trailer: single\" were not an empty line but a\n   line with a single SP on it (which is is_blank_line()), would the\n   new logic get confused?\n\n - if the second \"multi:\" trailer did not have the funny blank line\n   before \"_two\", the expected output would still be \"multi:\"\n   followed by \"one two three\", iow, the line after the second\n   \"multi: one\" is a total no-op?  If we added many more \" \\n\" lines\n   there, they are all absorbed and ignored?  It somehow feels wrong\n\n> diff --git a/trailer.c b/trailer.c\n> index 249ed618ed..7ca7200aec 100644\n> --- a/trailer.c\n> +++ b/trailer.c\n> @@ -846,7 +846,7 @@ static size_t find_trailer_start(const char *buf, size_t len)\n>  \t\t\tpossible_continuation_lines = 0;\n>  \t\t\tcontinue;\n>  \t\t}\n> -\t\tif (is_blank_line(bol)) {\n> +\t\tif (is_blank_line(bol) && *bol == '\\n') {\n>  \t\t\tif (only_spaces)\n>  \t\t\t\tcontinue;\n>  \t\t\tnon_trailer_lines += possible_continuation_lines;\n> --\n> 2.30.0.667.g81c0cbc6fd\n"},{"id":"417100","messageId":"YCwhPG6RaAhU9ljg@nand.local","threadId":"55151","inReplyTo":"xmqqmtw3pzu3.fsf@gitster.c.googlers.com","subject":"Re: Bug Report: Multi-line trailers containing empty lines break parsing","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2021-02-16T19:47:08Z","receivedAt":"2021-02-16T19:47:54Z","isPatch":false,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Tue, Feb 16, 2021 at 11:39:00AM -0800, Junio C Hamano wrote:\n> A few comments (not pointing out bugs, but just sharing\n> observations).\n>\n>  - if the line before \"trailer: single\" were not an empty line but a\n>    line with a single SP on it (which is is_blank_line()), would the\n>    new logic get confused?\n\nOof. That breaks the new test, but it makes me worried about whether\nthis can be parsed without ambiguity. I think not, but here I'd defer to\nChristian or Jonathan Tan.\n\n>  - if the second \"multi:\" trailer did not have the funny blank line\n>    before \"_two\", the expected output would still be \"multi:\"\n>    followed by \"one two three\", iow, the line after the second\n>    \"multi: one\" is a total no-op?  If we added many more \" \\n\" lines\n>    there, they are all absorbed and ignored?  It somehow feels wrong\n\nThat's definitely the outcome of this patch, but I agree it feels wrong.\nI'm not sure that we define the behavior that strictly in\ngit-interpret-trailers(1), so we have some wiggle room, I guess.\n\nThanks,\nTaylor\n"},{"id":"420038","messageId":"CAP8UFD1QG_b6ax-HodLRRcdLKgWJhPDghjLfjnyan1Zi80en7A@mail.gmail.com","threadId":"55151","inReplyTo":"YCwhPG6RaAhU9ljg@nand.local","subject":"Re: Bug Report: Multi-line trailers containing empty lines break parsing","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2021-03-23T15:17:46Z","receivedAt":"2021-03-23T15:19:02Z","isPatch":false,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Tue, Feb 16, 2021 at 8:47 PM Taylor Blau <me@ttaylorr.com> wrote:\n>\n> On Tue, Feb 16, 2021 at 11:39:00AM -0800, Junio C Hamano wrote:\n> > A few comments (not pointing out bugs, but just sharing\n> > observations).\n> >\n> >  - if the line before \"trailer: single\" were not an empty line but a\n> >    line with a single SP on it (which is is_blank_line()), would the\n> >    new logic get confused?\n>\n> Oof. That breaks the new test, but it makes me worried about whether\n> this can be parsed without ambiguity. I think not, but here I'd defer to\n> Christian or Jonathan Tan.\n\nSorry for the late answer on this. It looks like this fell into my\nemail reading cracks.\n\nMy opinion, when I worked on this, was that it's very difficult to\navoid ambiguity, so it's better if `git interpret-trailers` is strict\nby default, which could be relaxed later in special cases where there\nis not much risk of ambiguity.\n\nIt's especially ambiguous because many commit message subjects or even\nbodies can be of the form \"area: change\" which can look like a\ntrailer. And some people might want to process whole commit messages,\nwhile others might want to process templates that might contain only\ntrailers.\n\nSo I thought that blank lines should not appear in the trailers. And\nif any appears, it means that the trailers should start after the last\nblank line. This might actually have been already relaxed a bit.\n\n> >  - if the second \"multi:\" trailer did not have the funny blank line\n> >    before \"_two\", the expected output would still be \"multi:\"\n> >    followed by \"one two three\", iow, the line after the second\n> >    \"multi: one\" is a total no-op?  If we added many more \" \\n\" lines\n> >    there, they are all absorbed and ignored?  It somehow feels wrong\n>\n> That's definitely the outcome of this patch, but I agree it feels wrong.\n> I'm not sure that we define the behavior that strictly in\n> git-interpret-trailers(1), so we have some wiggle room, I guess.\n\nAny patch to relax how blank lines and other aspects of trailers\nparsing in my opinion should come with some documentation change to\nexplain what we now accept and what we don't accept, and also tests to\nenforce that.\n"},{"id":"420052","messageId":"xmqqsg4l3h32.fsf@gitster.g","threadId":"55151","inReplyTo":"CAP8UFD1QG_b6ax-HodLRRcdLKgWJhPDghjLfjnyan1Zi80en7A@mail.gmail.com","subject":"Re: Bug Report: Multi-line trailers containing empty lines break parsing","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-03-23T17:39:13Z","receivedAt":"2021-03-23T17:40:21Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Christian Couder <christian.couder@gmail.com> writes:\n\n> So I thought that blank lines should not appear in the trailers. And\n> if any appears, it means that the trailers should start after the last\n> blank line.\n\nI think that is a good principle to stick to.\n\n>> >  - if the second \"multi:\" trailer did not have the funny blank line\n>> >    before \"_two\", the expected output would still be \"multi:\"\n>> >    followed by \"one two three\", iow, the line after the second\n>> >    \"multi: one\" is a total no-op?  If we added many more \" \\n\" lines\n>> >    there, they are all absorbed and ignored?  It somehow feels wrong\n>>\n>> That's definitely the outcome of this patch, but I agree it feels wrong.\n>> I'm not sure that we define the behavior that strictly in\n>> git-interpret-trailers(1), so we have some wiggle room, I guess.\n>\n> Any patch to relax how blank lines and other aspects of trailers\n> parsing in my opinion should come with some documentation change to\n> explain what we now accept and what we don't accept, and also tests to\n> enforce that.\n\nOK.  But do we document clearly what we accept and we don't before\nany change?\n\n"},{"id":"420178","messageId":"CAP8UFD0rRff7oCMH=DeTQ-tZw7STLwSHLecWxRr_rQVyHuxJuA@mail.gmail.com","threadId":"55151","inReplyTo":"xmqqsg4l3h32.fsf@gitster.g","subject":"Re: Bug Report: Multi-line trailers containing empty lines break parsing","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2021-03-25T07:53:04Z","receivedAt":"2021-03-25T07:54:06Z","isPatch":false,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Tue, Mar 23, 2021 at 6:39 PM Junio C Hamano <gitster@pobox.com> wrote:\n>\n> Christian Couder <christian.couder@gmail.com> writes:\n>\n> > So I thought that blank lines should not appear in the trailers. And\n> > if any appears, it means that the trailers should start after the last\n> > blank line.\n>\n> I think that is a good principle to stick to.\n>\n> >> >  - if the second \"multi:\" trailer did not have the funny blank line\n> >> >    before \"_two\", the expected output would still be \"multi:\"\n> >> >    followed by \"one two three\", iow, the line after the second\n> >> >    \"multi: one\" is a total no-op?  If we added many more \" \\n\" lines\n> >> >    there, they are all absorbed and ignored?  It somehow feels wrong\n> >>\n> >> That's definitely the outcome of this patch, but I agree it feels wrong.\n> >> I'm not sure that we define the behavior that strictly in\n> >> git-interpret-trailers(1), so we have some wiggle room, I guess.\n> >\n> > Any patch to relax how blank lines and other aspects of trailers\n> > parsing in my opinion should come with some documentation change to\n> > explain what we now accept and what we don't accept, and also tests to\n> > enforce that.\n>\n> OK.  But do we document clearly what we accept and we don't before\n> any change?\n\nMaybe it's not enough, but the doc already has the following:\n\n------\nExisting trailers are extracted from the input message by looking for\na group of one or more lines that (i) is all trailers, or (ii) contains at\nleast one Git-generated or user-configured trailer and consists of at\nleast 25% trailers.\nThe group must be preceded by one or more empty (or whitespace-only) lines.\nThe group must either be at the end of the message or be the last\nnon-whitespace lines before a line that starts with '---' (followed by a\nspace or the end of the line). Such three minus signs start the patch\npart of the message. See also `--no-divider` below.\n\nWhen reading trailers, there can be whitespaces after the\ntoken, the separator and the value. There can also be whitespaces\ninside the token and the value. The value may be split over multiple lines with\neach subsequent line starting with whitespace, like the \"folding\" in RFC 822.\n------\n"},{"id":"420181","messageId":"CAOLTT8TFTcw9fkfPk-vi3REtZqCWYS28LEP5jfy-TmYuWf747g@mail.gmail.com","threadId":"55151","inReplyTo":"CAP8UFD0rRff7oCMH=DeTQ-tZw7STLwSHLecWxRr_rQVyHuxJuA@mail.gmail.com","subject":"Re: Bug Report: Multi-line trailers containing empty lines break parsing","fromName":"ZheNing Hu","fromEmail":"adlternative@gmail.com","sentAt":"2021-03-25T09:33:23Z","receivedAt":"2021-03-25T09:34:22Z","isPatch":false,"sender":{"key":"adlternative@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58138461?v=4"},"body":"Christian Couder <christian.couder@gmail.com> 于2021年3月25日周四 下午3:54写道：\n>\n> On Tue, Mar 23, 2021 at 6:39 PM Junio C Hamano <gitster@pobox.com> wrote:\n> >\n> > Christian Couder <christian.couder@gmail.com> writes:\n> >\n> > > So I thought that blank lines should not appear in the trailers. And\n> > > if any appears, it means that the trailers should start after the last\n> > > blank line.\n> >\n> > I think that is a good principle to stick to.\n> >\n> > >> >  - if the second \"multi:\" trailer did not have the funny blank line\n> > >> >    before \"_two\", the expected output would still be \"multi:\"\n> > >> >    followed by \"one two three\", iow, the line after the second\n> > >> >    \"multi: one\" is a total no-op?  If we added many more \" \\n\" lines\n> > >> >    there, they are all absorbed and ignored?  It somehow feels wrong\n> > >>\n> > >> That's definitely the outcome of this patch, but I agree it feels wrong.\n> > >> I'm not sure that we define the behavior that strictly in\n> > >> git-interpret-trailers(1), so we have some wiggle room, I guess.\n> > >\n> > > Any patch to relax how blank lines and other aspects of trailers\n> > > parsing in my opinion should come with some documentation change to\n> > > explain what we now accept and what we don't accept, and also tests to\n> > > enforce that.\n> >\n> > OK.  But do we document clearly what we accept and we don't before\n> > any change?\n>\n> Maybe it's not enough, but the doc already has the following:\n>\n> ------\n> Existing trailers are extracted from the input message by looking for\n> a group of one or more lines that (i) is all trailers, or (ii) contains at\n> least one Git-generated or user-configured trailer and consists of at\n> least 25% trailers.\n> The group must be preceded by one or more empty (or whitespace-only) lines.\n> The group must either be at the end of the message or be the last\n> non-whitespace lines before a line that starts with '---' (followed by a\n> space or the end of the line). Such three minus signs start the patch\n> part of the message. See also `--no-divider` below.\n>\n> When reading trailers, there can be whitespaces after the\n> token, the separator and the value. There can also be whitespaces\n> inside the token and the value. The value may be split over multiple lines with\n> each subsequent line starting with whitespace, like the \"folding\" in RFC 822.\n> ------\n\n\nMaybe I don't have enough right to speak on this issue, but I always think that\n the empty line is intentional by the designer, especially when I test it.\n"},{"id":"420196","messageId":"xmqqr1k3w1g7.fsf@gitster.g","threadId":"55151","inReplyTo":"CAP8UFD0rRff7oCMH=DeTQ-tZw7STLwSHLecWxRr_rQVyHuxJuA@mail.gmail.com","subject":"Re: Bug Report: Multi-line trailers containing empty lines break parsing","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-03-25T18:08:40Z","receivedAt":"2021-03-25T18:09:19Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Christian Couder <christian.couder@gmail.com> writes:\n\n>> > Any patch to relax how blank lines and other aspects of trailers\n>> > parsing in my opinion should come with some documentation change to\n>> > explain what we now accept and what we don't accept, and also tests to\n>> > enforce that.\n>>\n>> OK.  But do we document clearly what we accept and we don't before\n>> any change?\n>\n> Maybe it's not enough, but the doc already has the following:\n\nOK.\n\nThe next step is to find out if there is anything unclear in the\ndescription of the current behaviour in that paragraph that may have\nresulted in the report of the \"bug\" that turned out to be a non-bug.\n\n> ------\n> Existing trailers are extracted from the input message by looking for\n> a group of one or more lines that (i) is all trailers, or (ii) contains at\n> least one Git-generated or user-configured trailer and consists of at\n> least 25% trailers.\n> The group must be preceded by one or more empty (or whitespace-only) lines.\n> The group must either be at the end of the message or be the last\n> non-whitespace lines before a line that starts with '---' (followed by a\n> space or the end of the line). Such three minus signs start the patch\n> part of the message. See also `--no-divider` below.\n>\n> When reading trailers, there can be whitespaces after the\n> token, the separator and the value. There can also be whitespaces\n> inside the token and the value. The value may be split over multiple lines with\n> each subsequent line starting with whitespace, like the \"folding\" in RFC 822.\n> ------\n\nThanks.\n"},{"id":"420197","messageId":"xmqqmturw13r.fsf@gitster.g","threadId":"55151","inReplyTo":"CAOLTT8TFTcw9fkfPk-vi3REtZqCWYS28LEP5jfy-TmYuWf747g@mail.gmail.com","subject":"Re: Bug Report: Multi-line trailers containing empty lines break parsing","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-03-25T18:16:08Z","receivedAt":"2021-03-25T18:16:51Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"ZheNing Hu <adlternative@gmail.com> writes:\n\n> Christian Couder <christian.couder@gmail.com> 于2021年3月25日周四 下午3:54写道：\n>>\n>> Maybe it's not enough, but the doc already has the following:\n>>\n>> ------\n>> Existing trailers are extracted from the input message by looking for\n>> a group of one or more lines that (i) is all trailers, or (ii) contains at\n>> least one Git-generated or user-configured trailer and consists of at\n>> least 25% trailers.\n>> The group must be preceded by one or more empty (or whitespace-only) lines.\n>> The group must either be at the end of the message or be the last\n>> non-whitespace lines before a line that starts with '---' (followed by a\n>> space or the end of the line). Such three minus signs start the patch\n>> part of the message. See also `--no-divider` below.\n>>\n>> When reading trailers, there can be whitespaces after the\n>> token, the separator and the value. There can also be whitespaces\n>> inside the token and the value. The value may be split over multiple lines with\n>> each subsequent line starting with whitespace, like the \"folding\" in RFC 822.\n>> ------\n>\n>\n> Maybe I don't have enough right to speak on this issue, but I always think that\n>  the empty line is intentional by the designer, especially when I test it.\n\nPeople like you, who is relatively new to the system and the list,\nare valuable source of information for us to learn where in the\ncurrent system and documentation we have room to improve and\nclarify.  You do have right, and we welcome your input.\n\nCare to clarify what you mean by \"the empty line is intentional by\nthe designer\"?  The designer of the current \"trailer\" intended to\nmake the \"last paragraph\" (where \"paragraph\" is defined as a run of\nlines without an empty line in it, so that one or more continguous\nempty lines separate \"paragraphs\") where the trailers sit in the log\nmessage.  Is that what you mean?  Or something else?\n"},{"id":"420246","messageId":"CAOLTT8QPJJWh_Xm=n+i7z5+5w=eBq9tcr5-yBmhovzqOXP=jnQ@mail.gmail.com","threadId":"55151","inReplyTo":"xmqqmturw13r.fsf@gitster.g","subject":"Re: Bug Report: Multi-line trailers containing empty lines break parsing","fromName":"ZheNing Hu","fromEmail":"adlternative@gmail.com","sentAt":"2021-03-26T10:25:26Z","receivedAt":"2021-03-26T10:26:25Z","isPatch":false,"sender":{"key":"adlternative@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58138461?v=4"},"body":"Junio C Hamano <gitster@pobox.com> 于2021年3月26日周五 上午2:16写道：\n>\n> ZheNing Hu <adlternative@gmail.com> writes:\n>\n> > Christian Couder <christian.couder@gmail.com> 于2021年3月25日周四 下午3:54写道：\n> >>\n> >> Maybe it's not enough, but the doc already has the following:\n> >>\n> >> ------\n> >> Existing trailers are extracted from the input message by looking for\n> >> a group of one or more lines that (i) is all trailers, or (ii) contains at\n> >> least one Git-generated or user-configured trailer and consists of at\n> >> least 25% trailers.\n> >> The group must be preceded by one or more empty (or whitespace-only) lines.\n> >> The group must either be at the end of the message or be the last\n> >> non-whitespace lines before a line that starts with '---' (followed by a\n> >> space or the end of the line). Such three minus signs start the patch\n> >> part of the message. See also `--no-divider` below.\n> >>\n> >> When reading trailers, there can be whitespaces after the\n> >> token, the separator and the value. There can also be whitespaces\n> >> inside the token and the value. The value may be split over multiple lines with\n> >> each subsequent line starting with whitespace, like the \"folding\" in RFC 822.\n> >> ------\n> >\n> >\n> > Maybe I don't have enough right to speak on this issue, but I always think that\n> >  the empty line is intentional by the designer, especially when I test it.\n>\n> People like you, who is relatively new to the system and the list,\n> are valuable source of information for us to learn where in the\n> current system and documentation we have room to improve and\n> clarify.  You do have right, and we welcome your input.\n>\n\nThanks:)\n\n> Care to clarify what you mean by \"the empty line is intentional by\n> the designer\"?  The designer of the current \"trailer\" intended to\n> make the \"last paragraph\" (where \"paragraph\" is defined as a run of\n> lines without an empty line in it, so that one or more continguous\n> empty lines separate \"paragraphs\") where the trailers sit in the log\n> message.  Is that what you mean?  Or something else?\n\nEmmm, I mean generally speaking, the entire commit infomations is divided into\nthree paragraphs: \"subject\"/\"message\"/\"trailers\".\n\nWhen we use `--parse` to get those trailers, normally it can indeed be obtained,\nBut if in the middle of the trailers have a extra empty lines or lines with only\nwhitespaces, All trailers before the blank line will be discarded, I think it is\nacceptable.Because It seems that the previous content belongs to the message\nsection.\n\nLike this:\n------\n(subject)\n\nFirst paragraph:\nhello world\n\nSecond paragraph:\nwhat happen if some thing like\n\nthis: Use git to commit code to git\n\nSigned-off-by: CoCo <cc@gg.com>\nDeleted-by: ADL <a@gg.com>\n-----\n\n\"this: Use git to commit code to git\" seem like a trailer,\nbut it's user's message.\n\nSo I think the purpose of these blank lines is to separate the\nthree parts of the commit information. It is normal for the blank\nlines inside trailers to cause ambiguity.\n\nPlease correct me if I what I said is wrong.\n--\nZheNing Hu\n"}]}