{"thread":{"id":"45577","subject":"Bug in \"git am\" when the body starts with spaces","startedAt":"2017-04-01T00:24:55Z","lastAt":"2017-04-04T06:51:14Z","messageCount":8,"participants":["Linus Torvalds","Jeff King","Junio C Hamano","Jonathan Tan"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"315972","messageId":"CA+55aFypmFkc4gOEea-AF1kKYb3K=6nosXvYS4AMGFOQHw0ZxA@mail.gmail.com","threadId":"45577","inReplyTo":null,"subject":"Bug in \"git am\" when the body starts with spaces","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2017-04-01T00:24:49Z","receivedAt":"2017-04-01T00:24:55Z","isPatch":false,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"Try applying the attached patch with\n\n git am 0001-Test-patch.patch\n\nin the git repository.\n\nAt least for me, it results in a very odd commit that has one single\nline in the commit message:\n\n Test patch This should go in the body not in the subject line\n\nwhich is obviously bogus.\n\nI think the reason is that the \"header continuation line\" logic kicks\nin because the lines in the body start with spaces, but that's\nentirely incorrect, since\n\n (a) we're not in an email header\n\n (b) there's an empty line in between anyway, so no way are those body\nlines continuation lines.\n\nI didn't check how far back this goes, I guess I'll do that next. But\nI thought I'd report it here first in case somebody else goes \"ahhh\".\n\n Linus\n\n\nFrom ad65cf7ba97ac071da1f845ec854165e7bf1efdf Mon Sep 17 00:00:00 2001\nFrom: Linus Torvalds \nDate: Fri, 31 Mar 2017 17:18:16 -0700\nSubject: [PATCH] Test patch example\n\nSubject: [PATCH] Test patch\n\n This should go in the body\n not in the subject line\n---\n Makefile | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/Makefile b/Makefile\nindex 9b36068ac..9f36c149b 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -1,3 +1,4 @@\n+\n # The default target of this Makefile is...\n all::\n \n-- \n2.12.2.401.g5d4234a49\n\n"},{"id":"315975","messageId":"CA+55aFwcQuxwhQ+LABmev2XRHgOqsbRm7YDYn3FDDXKMYcF-CA@mail.gmail.com","threadId":"45577","inReplyTo":"CA+55aFypmFkc4gOEea-AF1kKYb3K=6nosXvYS4AMGFOQHw0ZxA@mail.gmail.com","subject":"Re: Bug in \"git am\" when the body starts with spaces","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2017-04-01T00:52:00Z","receivedAt":"2017-04-01T00:52:09Z","isPatch":false,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"Ok, did a bisect, and this bisects to commit 6b4b013f1884 (\"mailinfo:\nhandle in-body header continuations\").\n\nThe continuation logic is oddly complex, and I can't follow the logic.\nBut it is completely broken in how it thinks empty lines are somehow\n\"continuations\".\n\nJonathan?\n\n Linus\n\nOn Fri, Mar 31, 2017 at 5:24 PM, Linus Torvalds\n wrote:\n>\n> I think the reason is that the \"header continuation line\" logic kicks\n> in because the lines in the body start with spaces, but that's\n> entirely incorrect, since\n>\n> (a) we're not in an email header\n>\n> (b) there's an empty line in between anyway, so no way are those body\n> lines continuation lines.\n>\n> I didn't check how far back this goes, I guess I'll do that next. But\n> I thought I'd report it here first in case somebody else goes \"ahhh\".\n"},{"id":"315988","messageId":"20170401052739.dw65gnh26izgt5u7@sigill.intra.peff.net","threadId":"45577","inReplyTo":"CA+55aFwcQuxwhQ+LABmev2XRHgOqsbRm7YDYn3FDDXKMYcF-CA@mail.gmail.com","subject":"Re: Bug in \"git am\" when the body starts with spaces","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-04-01T05:27:39Z","receivedAt":"2017-04-01T05:27:50Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Mar 31, 2017 at 05:52:00PM -0700, Linus Torvalds wrote:\n\n> Ok, did a bisect, and this bisects to commit 6b4b013f1884 (\"mailinfo:\n> handle in-body header continuations\").\n> \n> The continuation logic is oddly complex, and I can't follow the logic.\n> But it is completely broken in how it thinks empty lines are somehow\n> \"continuations\".\n\nI think the continuation logic is OK. The problem is that the newline is\nnever fed to check_inbody_header() at all. So the next line its state\nmachine sees is the indented line, which looks like a continuation.\n\nIt seems to me that ignoring that newline is a bug, and causes other\nproblems. For instance, if you have a blank line and then another\nheader-looking thing (not a continuation) after, it is respected. For\ninstance, running mailinfo on:\n\n From ...mbox header...\n From: Email Author \n Subject: email subject\n Date: whatever\n\n From: in-body author \n\n Subject: in-body subject\n\n And the rest of the message.\n\nwill use \"in-body subject\" as the subject, though I'd have thought we\nshould stop parsing in-body headers as soon as we see the first in-body\nblank line (the one after the in-body \"From\").\n\nThe blank line gets eaten by the check at the beginning of\nhandle_commit_msg(), right _before_ we pass it off to the\ncheck_inbody_header() function.\n\nIt seems like that should be more like:\n\ndiff --git a/mailinfo.c b/mailinfo.c\nindex a489d9d0f..6d89781eb 100644\n--- a/mailinfo.c\n+++ b/mailinfo.c\n@@ -757,8 +757,10 @@ static int handle_commit_msg(struct mailinfo *mi, struct strbuf *line)\n \tassert(!mi->filter_stage);\n \n \tif (mi->header_stage) {\n-\t\tif (!line->len || (line->len == 1 && line->buf[0] == '\\n'))\n+\t\tif (!line->len || (line->len == 1 && line->buf[0] == '\\n')) {\n+\t\t\tmi->header_stage = 0;\n \t\t\treturn 0;\n+\t\t}\n \t}\n \n \tif (mi->use_inbody_headers && mi->header_stage) {\n\n\nBut that breaks a bunch of tests. It looks like the loop in handle_body\nalways starts by feeding the initial header-separator (the real headers,\nnot the in-body ones) to the various parsers. So that first newline\nmakes us trigger \"no more in-body headers\" before we even start parsing\nany lines. Oops.\n\nSo doing this:\n\n@@ -960,7 +962,7 @@ static void handle_body(struct mailinfo *mi, struct strbuf *line)\n \t\t\tgoto handle_body_out;\n \t}\n \n-\tdo {\n+\twhile (!strbuf_getwholeline(line, mi->input, '\\n')) {\n \t\t/* process any boundary lines */\n \t\tif (*(mi->content_top) && is_multipart_boundary(mi, line)) {\n \t\t\t/* flush any leftover */\n@@ -1013,7 +1015,7 @@ static void handle_body(struct mailinfo *mi, struct strbuf *line)\n \n \t\tif (mi->input_error)\n \t\t\tbreak;\n-\t} while (!strbuf_getwholeline(line, mi->input, '\\n'));\n+\t}\n \n \tflush_inbody_header_accum(mi);\n \n\non top fixes that. But that still breaks a test that has blank lines at\nthe beginning of the message, before the in-body header. So probably the\nstate-machine needs an extra state: sucking up pre-inbody-header\nnewlines.\n\n-Peff\n"},{"id":"316022","messageId":"CA+55aFwmNuvh_ABtnd_uxXv5HjEjN-+HMZGh_u1OjOCa4dbpmg@mail.gmail.com","threadId":"45577","inReplyTo":"CA+55aFwcQuxwhQ+LABmev2XRHgOqsbRm7YDYn3FDDXKMYcF-CA@mail.gmail.com","subject":"Re: Bug in \"git am\" when the body starts with spaces","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2017-04-01T19:03:44Z","receivedAt":"2017-04-01T19:03:50Z","isPatch":false,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"On Fri, Mar 31, 2017 at 5:52 PM, Linus Torvalds\n wrote:\n>\n> The continuation logic is oddly complex, and I can't follow the logic.\n> But it is completely broken in how it thinks empty lines are somehow\n> \"continuations\".\n\nThe attached patch seems to work for me. Comments?\n\nThe logic is fairly simple: if we encounter an empty line, and we have\npending in-body headers, we flush the pending headers, and mark us as\nno longer in header mode.\n\nThe code used to just ignore empty lines when in header mode, which is\ngarbage, exactly because it would happily continue accumulating more\nheaders.\n\nThere may be other cases this misses, but this at least seems to fix\nthe case I encountered.\n\n Linus\n\n\n mailinfo.c | 7 ++++++-\n 1 file changed, 6 insertions(+), 1 deletion(-)\n\ndiff --git a/mailinfo.c b/mailinfo.c\nindex a489d9d0f..68037758f 100644\n--- a/mailinfo.c\n+++ b/mailinfo.c\n@@ -757,8 +757,13 @@ static int handle_commit_msg(struct mailinfo *mi, struct strbuf *line)\n \tassert(!mi->filter_stage);\n \n \tif (mi->header_stage) {\n-\t\tif (!line->len || (line->len == 1 && line->buf[0] == '\\n'))\n+\t\tif (!line->len || (line->len == 1 && line->buf[0] == '\\n')) {\n+\t\t\tif (mi->inbody_header_accum.len) {\n+\t\t\t\tflush_inbody_header_accum(mi);\n+\t\t\t\tmi->header_stage = 0;\n+\t\t\t}\n \t\t\treturn 0;\n+\t\t}\n \t}\n \n \tif (mi->use_inbody_headers && mi->header_stage) {\n"},{"id":"316036","messageId":"20170402041838.dzkmibc5lq37sm3h@sigill.intra.peff.net","threadId":"45577","inReplyTo":"CA+55aFwmNuvh_ABtnd_uxXv5HjEjN-+HMZGh_u1OjOCa4dbpmg@mail.gmail.com","subject":"Re: Bug in \"git am\" when the body starts with spaces","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-04-02T04:18:38Z","receivedAt":"2017-04-02T04:18:50Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Apr 01, 2017 at 12:03:44PM -0700, Linus Torvalds wrote:\n\n> On Fri, Mar 31, 2017 at 5:52 PM, Linus Torvalds\n> wrote:\n> >\n> > The continuation logic is oddly complex, and I can't follow the logic.\n> > But it is completely broken in how it thinks empty lines are somehow\n> > \"continuations\".\n> \n> The attached patch seems to work for me. Comments?\n> \n> The logic is fairly simple: if we encounter an empty line, and we have\n> pending in-body headers, we flush the pending headers, and mark us as\n> no longer in header mode.\n\nHmm. I think this may work. At first I thought it was too strict in\nalways checking inbody_header_accum.len, because we want this to kick in\nalways, whether there's whitespace continuation or not. But that\naccumulator has to collect preemptively, before it knows if there's\ncontinuation. So it will always be non-empty if we've seen _any_ header,\nand it will remain non-empty as long as we keep parsing (because any\ntime we flush, we do so in order to handle another line).\n\nIOW, I think this implements the state-machine thing I wrote in my\nearlier email, because the state \"are we inside in-body header parsing\"\nis always reflected by having a non-empty accumulator. It is a bit\nnon-obvious though.\n\n-Peff\n"},{"id":"316060","messageId":"xmqqinmmsoez.fsf@gitster.mtv.corp.google.com","threadId":"45577","inReplyTo":"CA+55aFwmNuvh_ABtnd_uxXv5HjEjN-+HMZGh_u1OjOCa4dbpmg@mail.gmail.com","subject":"Re: Bug in \"git am\" when the body starts with spaces","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-04-02T17:35:32Z","receivedAt":"2017-04-02T17:35:41Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Linus Torvalds writes:\n\n> On Fri, Mar 31, 2017 at 5:52 PM, Linus Torvalds\n> wrote:\n>>\n>> The continuation logic is oddly complex, and I can't follow the logic.\n>> But it is completely broken in how it thinks empty lines are somehow\n>> \"continuations\".\n>\n> The attached patch seems to work for me. Comments?\n\nWe start at header_stage set to 1, keep skipping empty lines while\nin that state, and we stay in that state as long as we see in-body\nheader (or a continuation of the in-body header we saw earlier). We\nget out of this state when we see a blank line after we are done\nwith the in-body headers. Once header_stage is set to 0 with a\nblank line, we don't do in-body headers (scissors will roll back the\nwhole thing and irrelevant to the analysis of correctness).\n\nBut you found that \"keep skipping empty\" done unconditionally is\nwrong, because we may have already seen an in-body header and are\ntrying to see if the line is a continuation (in which case\ncheck_inbody_header() would append to the previous) or another\nin-body header (in which case again check_inbody_header() would use\nit), or something else (in which case check_inbody_header() will\nreturn false, we get out of header_stage=1 state and process this\nline as the first line of the log message. An empty line we see in\nthis state must trigger \"we are no longer taking in-body header\"\nlogic, too.\n\nAnd that is exactly your patch does. The change \"feels\" correct to\nme.\n\n\n> mailinfo.c | 7 ++++++-\n> 1 file changed, 6 insertions(+), 1 deletion(-)\n>\n> diff --git a/mailinfo.c b/mailinfo.c\n> index a489d9d0f..68037758f 100644\n> --- a/mailinfo.c\n> +++ b/mailinfo.c\n> @@ -757,8 +757,13 @@ static int handle_commit_msg(struct mailinfo *mi, struct strbuf *line)\n> \tassert(!mi->filter_stage);\n> \n> \tif (mi->header_stage) {\n> -\t\tif (!line->len || (line->len == 1 && line->buf[0] == '\\n'))\n> +\t\tif (!line->len || (line->len == 1 && line->buf[0] == '\\n')) {\n> +\t\t\tif (mi->inbody_header_accum.len) {\n> +\t\t\t\tflush_inbody_header_accum(mi);\n> +\t\t\t\tmi->header_stage = 0;\n> +\t\t\t}\n> \t\t\treturn 0;\n> +\t\t}\n> \t}\n> \n> \tif (mi->use_inbody_headers && mi->header_stage) {\n"},{"id":"316100","messageId":"f3f2b6f0-c6bb-f50a-5030-61afe941eb23@google.com","threadId":"45577","inReplyTo":"20170402041838.dzkmibc5lq37sm3h@sigill.intra.peff.net","subject":"Re: Bug in \"git am\" when the body starts with spaces","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2017-04-03T17:42:46Z","receivedAt":"2017-04-03T17:42:54Z","isPatch":false,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Thanks, everyone, for looking into this.\n\nOn 04/01/2017 09:18 PM, Jeff King wrote:\n> On Sat, Apr 01, 2017 at 12:03:44PM -0700, Linus Torvalds wrote:\n>> The logic is fairly simple: if we encounter an empty line, and we have\n>> pending in-body headers, we flush the pending headers, and mark us as\n>> no longer in header mode.\n>\n> Hmm. I think this may work. At first I thought it was too strict in\n> always checking inbody_header_accum.len, because we want this to kick in\n> always, whether there's whitespace continuation or not. But that\n> accumulator has to collect preemptively, before it knows if there's\n> continuation. So it will always be non-empty if we've seen _any_ header,\n> and it will remain non-empty as long as we keep parsing (because any\n> time we flush, we do so in order to handle another line).\n>\n> IOW, I think this implements the state-machine thing I wrote in my\n> earlier email, because the state \"are we inside in-body header parsing\"\n> is always reflected by having a non-empty accumulator. It is a bit\n> non-obvious though.\n\nAbout obviousness, I think of a non-empty accumulator merely \nrepresenting that the next line could potentially be a continuation \nline. And it is coincidence that this implies \"are we inside in-body \nheader parsing\"; if not all in-body header lines could be \"continued\", \nthere would be no such implication.\n\nmi->inbody_header_accum.len is already used in check_inbody_header to \nmean \"could the next line potentially be a continuation line\" and to \ntrigger a check for a negative criterion (in this case, a scissors \nline). I think it's fine to do the same thing, the negative criterion \nhere being a blank line.\n"},{"id":"316136","messageId":"20170404065059.3p3lzxj64mrvdtpi@sigill.intra.peff.net","threadId":"45577","inReplyTo":"f3f2b6f0-c6bb-f50a-5030-61afe941eb23@google.com","subject":"Re: Bug in \"git am\" when the body starts with spaces","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-04-04T06:50:59Z","receivedAt":"2017-04-04T06:51:14Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Apr 03, 2017 at 10:42:46AM -0700, Jonathan Tan wrote:\n\n> On 04/01/2017 09:18 PM, Jeff King wrote:\n> > On Sat, Apr 01, 2017 at 12:03:44PM -0700, Linus Torvalds wrote:\n> > > The logic is fairly simple: if we encounter an empty line, and we have\n> > > pending in-body headers, we flush the pending headers, and mark us as\n> > > no longer in header mode.\n> > \n> > Hmm. I think this may work. At first I thought it was too strict in\n> > always checking inbody_header_accum.len, because we want this to kick in\n> > always, whether there's whitespace continuation or not. But that\n> > accumulator has to collect preemptively, before it knows if there's\n> > continuation. So it will always be non-empty if we've seen _any_ header,\n> > and it will remain non-empty as long as we keep parsing (because any\n> > time we flush, we do so in order to handle another line).\n> > \n> > IOW, I think this implements the state-machine thing I wrote in my\n> > earlier email, because the state \"are we inside in-body header parsing\"\n> > is always reflected by having a non-empty accumulator. It is a bit\n> > non-obvious though.\n> \n> About obviousness, I think of a non-empty accumulator merely representing\n> that the next line could potentially be a continuation line. And it is\n> coincidence that this implies \"are we inside in-body header parsing\"; if not\n> all in-body header lines could be \"continued\", there would be no such\n> implication.\n> \n> mi->inbody_header_accum.len is already used in check_inbody_header to mean\n> \"could the next line potentially be a continuation line\" and to trigger a\n> check for a negative criterion (in this case, a scissors line). I think it's\n> fine to do the same thing, the negative criterion here being a blank line.\n\nFWIW, I looked into making a single \"state\" variable, but it actually\ngot ugly pretty quickly. I think not because it's not a cleaner method\nin the long run, but just because the existing code is so dependent on\nindividual state variables that needed changing. So I'm OK with Linus's\nfix; it certainly follows the existing code patterns.\n\n-Peff\n"}]}