{"thread":{"id":"22713","subject":"'git mailinfo' whitespace bug","startedAt":"2010-02-18T18:05:27Z","lastAt":"2010-02-22T19:57:07Z","messageCount":5,"participants":["Linus Torvalds","Lukas Sandström","Junio C Hamano","Don Zickus"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"134981","messageId":"alpine.LFD.2.00.1002180936240.4141@localhost.localdomain","threadId":"22713","inReplyTo":null,"subject":"'git mailinfo' whitespace bug","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2010-02-18T18:05:27Z","receivedAt":"2010-02-18T18:05:27Z","isPatch":false,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n'git mailinfo' removes the whitespace from the beginning of the email \nbody, but it does it incorrectly.\n\nIn particular, some people use indented paragraphs, like this:\n\n\t  Four-score and Four score and seven years ago our fathers \n   brought forth, upon this continent, a new nation, conceived in Liberty, \n   and dedicated to the proposition that all men are created equal.\n\n\tNow we are engaged in a great civil war, testing whether that \n   nation, or any nation so conceived, and so dedicated, can long endure. \n   We are met here on a great battlefield of that war. We have come to \n   dedicate a portion of it as a final resting place for those who here \n   gave their lives that that nation might live. It is altogether fitting \n   and proper that we should do this.\n\n   ...\n\nand mailinfo will not just remove empty lines from the beginning of the \nemail body, it will also remove the _first_ indentation (but not any \nothers). Which makes the whole thing come out wrong.\n\nI bisected it, and this bug was introduced almost two years ago. In commit \n3b6121f69b2 (\"git-mailinfo: use strbuf's instead of fixed buffers\"), to be \nexact. I'm pretty sure the bug is that handle_commit_msg() was changed to \nuse 'strbuf_ltrim()' for the 'still_looking' case.\n\nBefore commit 3b6121f69b2, it would create a new variable that had the \ntrimmed results (\"char *cp = line;\"), after that commit it would just trim \nthe line itself. Which is correct for the case of it being a header, but \nif it's the first non-header line, it's wrong.\n\n\t\t\tLinus\n"},{"id":"135073","messageId":"4B7E7BDA.4040701@gmail.com","threadId":"22713","inReplyTo":"alpine.LFD.2.00.1002180936240.4141@localhost.localdomain","subject":"[PATCH] mailinfo: don't trim whitespace in the commit message","fromName":"Lukas Sandström","fromEmail":"luksan@gmail.com","sentAt":"2010-02-19T11:54:02Z","receivedAt":"2010-02-19T11:54:02Z","isPatch":true,"sender":{"key":"luksan@gmail.com","avatar":"https://avatars.githubusercontent.com/u/152281?v=4"},"body":"Previously any whitespace in the beginning of the first line\nof the commit message was trimmed, destroying any paragraph\nindentation. Move the whitespace trimming to check_header()\ninstead, and preserve all commit message lines as-is in\nhandle_commit_msg().\n\nSigned-off-by: Lukas Sandström <luksan@gmail.com>\n---\n\nOn 2010-02-18 19:05, Linus Torvalds wrote:\n> 'git mailinfo' removes the whitespace from the beginning of the email\n> body, but it does it incorrectly.\n>\n> In particular, some people use indented paragraphs, like this:\n>\n> \t  Four-score and Four score and seven years ago our fathers\n>    brought forth, upon this continent, a new nation, conceived in Liberty,\n>    and dedicated to the proposition that all men are created equal.\n>\n> \tNow we are engaged in a great civil war, testing whether that\n>    nation, or any nation so conceived, and so dedicated, can long endure.\n>    We are met here on a great battlefield of that war. We have come to\n>    dedicate a portion of it as a final resting place for those who here\n>    gave their lives that that nation might live. It is altogether fitting\n>    and proper that we should do this.\n>\n>    ...\n>\n> and mailinfo will not just remove empty lines from the beginning of the\n> email body, it will also remove the _first_ indentation (but not any\n> others). Which makes the whole thing come out wrong.\n>\n> I bisected it, and this bug was introduced almost two years ago. In commit\n> 3b6121f69b2 (\"git-mailinfo: use strbuf's instead of fixed buffers\"), to be\n> exact. I'm pretty sure the bug is that handle_commit_msg() was changed to\n> use 'strbuf_ltrim()' for the 'still_looking' case.\n>\n> Before commit 3b6121f69b2, it would create a new variable that had the\n> trimmed results (\"char *cp = line;\"), after that commit it would just trim\n> the line itself. Which is correct for the case of it being a header, but\n> if it's the first non-header line, it's wrong.\n>\n\nThis patch should fix it. Note that there was a test-case explicitly\nchecking for this \"trim first line\" behaviour.\n\n/Lukas\n\n builtin-mailinfo.c |   41 ++++++++++++++++++++++-------------------\n t/t5100/msg0015    |    2 +-\n 2 files changed, 23 insertions(+), 20 deletions(-)\n\ndiff --git a/builtin-mailinfo.c b/builtin-mailinfo.c\nindex a50ac22..954dc11 100644\n--- a/builtin-mailinfo.c\n+++ b/builtin-mailinfo.c\n@@ -283,11 +283,15 @@ static inline int cmp_header(const struct strbuf *line, const char *hdr)\n \t\t\tline->buf[len] == ':' && isspace(line->buf[len + 1]);\n }\n\n-static int check_header(const struct strbuf *line,\n+static int check_header(const struct strbuf *line_in,\n \t\t\t\tstruct strbuf *hdr_data[], int overwrite)\n {\n \tint i, ret = 0, len;\n-\tstruct strbuf sb = STRBUF_INIT;\n+\tstruct strbuf sb = STRBUF_INIT, sb2 = STRBUF_INIT, *line = &sb2;\n+\n+\tstrbuf_addbuf(line, line_in);\n+\tstrbuf_ltrim(line);\n+\n \t/* search for the interesting parts */\n \tfor (i = 0; header[i]; i++) {\n \t\tint len = strlen(header[i]);\n@@ -339,6 +343,7 @@ static int check_header(const struct strbuf *line,\n\n check_header_out:\n \tstrbuf_release(&sb);\n+\tstrbuf_release(&sb2);\n \treturn ret;\n }\n\n@@ -773,27 +778,25 @@ static int is_scissors_line(const struct strbuf *line)\n\n static int handle_commit_msg(struct strbuf *line)\n {\n-\tstatic int still_looking = 1;\n+\tstatic int first_msg_line_found = 0;\n+\tsize_t cnt;\n\n \tif (!cmitmsg)\n-\t\treturn 0;\n-\n-\tif (still_looking) {\n-\t\tstrbuf_ltrim(line);\n-\t\tif (!line->len)\n+\t\treturn 0; /* FIXME: shouldn't this be: return 1? */\n+\n+\tif (!first_msg_line_found) {\n+\t\tif (use_inbody_headers)\n+\t\t\tif (check_header(line, s_hdr_data, 0))\n+\t\t\t\treturn 0;\n+\t\t/* Check if the first line is all whitespace */\n+\t\tfor (cnt = 0; isspace(line->buf[cnt]); cnt++)\n+\t\t\t; /* nothing */\n+\t\tif (line->len == cnt)\n+\t\t\t/* Ignore the first line if it's only whitespace */\n \t\t\treturn 0;\n+\t\tfirst_msg_line_found = 1;\n \t}\n\n-\tif (use_inbody_headers && still_looking) {\n-\t\tstill_looking = check_header(line, s_hdr_data, 0);\n-\t\tif (still_looking)\n-\t\t\treturn 0;\n-\t} else\n-\t\t/* Only trim the first (blank) line of the commit message\n-\t\t * when ignoring in-body headers.\n-\t\t */\n-\t\tstill_looking = 0;\n-\n \t/* normalize the log message to UTF-8. */\n \tif (metainfo_charset)\n \t\tconvert_to_utf8(line, charset.buf);\n@@ -804,7 +807,7 @@ static int handle_commit_msg(struct strbuf *line)\n \t\t\tdie_errno(\"Could not rewind output message file\");\n \t\tif (ftruncate(fileno(cmitmsg), 0))\n \t\t\tdie_errno(\"Could not truncate output message file at scissors\");\n-\t\tstill_looking = 1;\n+\t\tfirst_msg_line_found = 0;\n\n \t\t/*\n \t\t * We may have already read \"secondary headers\"; purge\ndiff --git a/t/t5100/msg0015 b/t/t5100/msg0015\nindex 9577238..4abb3d5 100644\n--- a/t/t5100/msg0015\n+++ b/t/t5100/msg0015\n@@ -1,2 +1,2 @@\n-- a list\n+  - a list\n   - of stuff\n-- \n1.6.6.1\n"},{"id":"135144","messageId":"7vzl343160.fsf@alter.siamese.dyndns.org","threadId":"22713","inReplyTo":"alpine.LFD.2.00.1002180936240.4141@localhost.localdomain","subject":"Re: 'git mailinfo' whitespace bug","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-02-20T05:51:19Z","receivedAt":"2010-02-20T05:51:19Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Linus Torvalds <torvalds@linux-foundation.org> writes:\n\n> I bisected it, and this bug was introduced almost two years ago. In commit \n> 3b6121f69b2 (\"git-mailinfo: use strbuf's instead of fixed buffers\"), to be \n> exact. I'm pretty sure the bug is that handle_commit_msg() was changed to \n> use 'strbuf_ltrim()' for the 'still_looking' case.\n>\n> Before commit 3b6121f69b2, it would create a new variable that had the \n> trimmed results (\"char *cp = line;\"), after that commit it would just trim \n> the line itself. Which is correct for the case of it being a header, but \n> if it's the first non-header line, it's wrong.\n\nTrue; trimming the body is obviously wrong.\n\nBut when is it correct to ltrim a header line?  It means we are going to\naccept a header (or header-looking line in body) that is indented.  I\ndon't know why 87ab799 (builtin-mailinfo.c 2007-03-12) was coded that way.\n\n\n builtin-mailinfo.c |    3 +--\n t/t5100/msg0015    |    2 +-\n 2 files changed, 2 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin-mailinfo.c b/builtin-mailinfo.c\nindex a50ac22..ce2ef6b 100644\n--- a/builtin-mailinfo.c\n+++ b/builtin-mailinfo.c\n@@ -779,8 +779,7 @@ static int handle_commit_msg(struct strbuf *line)\n \t\treturn 0;\n \n \tif (still_looking) {\n-\t\tstrbuf_ltrim(line);\n-\t\tif (!line->len)\n+\t\tif (!line->len || (line->len == 1 && line->buf[0] == '\\n'))\n \t\t\treturn 0;\n \t}\n \ndiff --git a/t/t5100/msg0015 b/t/t5100/msg0015\nindex 9577238..4abb3d5 100644\n--- a/t/t5100/msg0015\n+++ b/t/t5100/msg0015\n@@ -1,2 +1,2 @@\n-- a list\n+  - a list\n   - of stuff\n"},{"id":"135317","messageId":"20100222151344.GK3062@redhat.com","threadId":"22713","inReplyTo":"7vzl343160.fsf@alter.siamese.dyndns.org","subject":"Re: 'git mailinfo' whitespace bug","fromName":"Don Zickus","fromEmail":"dzickus@redhat.com","sentAt":"2010-02-22T15:13:44Z","receivedAt":"2010-02-22T15:13:44Z","isPatch":false,"sender":{"key":"dzickus@redhat.com","avatar":null},"body":"On Fri, Feb 19, 2010 at 09:51:19PM -0800, Junio C Hamano wrote:\n> Linus Torvalds <torvalds@linux-foundation.org> writes:\n> \n> > I bisected it, and this bug was introduced almost two years ago. In commit \n> > 3b6121f69b2 (\"git-mailinfo: use strbuf's instead of fixed buffers\"), to be \n> > exact. I'm pretty sure the bug is that handle_commit_msg() was changed to \n> > use 'strbuf_ltrim()' for the 'still_looking' case.\n> >\n> > Before commit 3b6121f69b2, it would create a new variable that had the \n> > trimmed results (\"char *cp = line;\"), after that commit it would just trim \n> > the line itself. Which is correct for the case of it being a header, but \n> > if it's the first non-header line, it's wrong.\n> \n> True; trimming the body is obviously wrong.\n> \n> But when is it correct to ltrim a header line?  It means we are going to\n> accept a header (or header-looking line in body) that is indented.  I\n> don't know why 87ab799 (builtin-mailinfo.c 2007-03-12) was coded that way.\n\nIn regards to 87ab799, I just deleted and pasted it from the function\nhandle_inbody_header (which you can see from that commit).  The original\ncode for those lines came from ae448e3854d8b6e7e37aa88fa3917f5dd97f3210.\nPerhaps I misused it when I moved it.\n\nYour patch belows seems to make sense for what its worth.\n\nCheers,\nDon\n\n> \n> \n>  builtin-mailinfo.c |    3 +--\n>  t/t5100/msg0015    |    2 +-\n>  2 files changed, 2 insertions(+), 3 deletions(-)\n> \n> diff --git a/builtin-mailinfo.c b/builtin-mailinfo.c\n> index a50ac22..ce2ef6b 100644\n> --- a/builtin-mailinfo.c\n> +++ b/builtin-mailinfo.c\n> @@ -779,8 +779,7 @@ static int handle_commit_msg(struct strbuf *line)\n>  \t\treturn 0;\n>  \n>  \tif (still_looking) {\n> -\t\tstrbuf_ltrim(line);\n> -\t\tif (!line->len)\n> +\t\tif (!line->len || (line->len == 1 && line->buf[0] == '\\n'))\n>  \t\t\treturn 0;\n>  \t}\n>  \n> diff --git a/t/t5100/msg0015 b/t/t5100/msg0015\n> index 9577238..4abb3d5 100644\n> --- a/t/t5100/msg0015\n> +++ b/t/t5100/msg0015\n> @@ -1,2 +1,2 @@\n> -- a list\n> +  - a list\n>    - of stuff\n"},{"id":"299165","messageId":"7vocjhcacs.fsf@alter.siamese.dyndns.org","threadId":"22713","inReplyTo":"20100222151344.GK3062@redhat.com","subject":"Re: 'git mailinfo' whitespace bug","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-02-22T19:57:07Z","receivedAt":"2010-02-22T19:57:07Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Don Zickus <dzickus@redhat.com> writes:\n\n>> But when is it correct to ltrim a header line?  It means we are going to\n>> accept a header (or header-looking line in body) that is indented.  I\n>> don't know why 87ab799 (builtin-mailinfo.c 2007-03-12) was coded that way.\n>\n> In regards to 87ab799, I just deleted and pasted it from the function\n> handle_inbody_header (which you can see from that commit).  The original\n> code for those lines came from ae448e3854d8b6e7e37aa88fa3917f5dd97f3210.\n\nThanks for clarifying.  The one in ae448e3 (mailinfo: ignore blanks after\nin-body headers., 2006-06-17) was about removing blank lines before the\nin-body headers begin, and never about removing indentation of the first\nin-body header.  Admittedly, it was also being lenient and skipped over\nlines that are not empty but all whitespaces, and if we apply the quoted\npatch we will be retroactively tightening the rule, but I somehow do not\nthink people would care.\n\n>>  builtin-mailinfo.c |    3 +--\n>>  t/t5100/msg0015    |    2 +-\n>>  2 files changed, 2 insertions(+), 3 deletions(-)\n>> \n>> diff --git a/builtin-mailinfo.c b/builtin-mailinfo.c\n>> index a50ac22..ce2ef6b 100644\n>> --- a/builtin-mailinfo.c\n>> +++ b/builtin-mailinfo.c\n>> @@ -779,8 +779,7 @@ static int handle_commit_msg(struct strbuf *line)\n>>  \t\treturn 0;\n>>  \n>>  \tif (still_looking) {\n>> -\t\tstrbuf_ltrim(line);\n>> -\t\tif (!line->len)\n>> +\t\tif (!line->len || (line->len == 1 && line->buf[0] == '\\n'))\n>>  \t\t\treturn 0;\n>>  \t}\n\nWe probably could do something like\n\n\tif (still_looking) {\n        \tif (strspn(line->buf, \" \\t\\n\") == line->len)\n\t\t\treturn 0;\n\t}\n\nto keep people with blank but not empty lines before the first in-body\nheader happy.  I somehow don't think it is worth it, but what would I\nknow...\n"}]}