{"thread":{"id":"43987","subject":"[PATCH] sequencer: support folding in rfc2822 footer","startedAt":"2016-09-02T19:58:54Z","lastAt":"2016-09-17T03:48:25Z","messageCount":24,"participants":["Jonathan Tan","Junio C Hamano","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"300910","messageId":"1472846322-5592-1-git-send-email-jonathantanmy@google.com","threadId":"43987","inReplyTo":null,"subject":"[PATCH] sequencer: support folding in rfc2822 footer","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2016-09-02T19:58:42Z","receivedAt":"2016-09-02T19:58:54Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"When running `git cherry-pick -x`, a blank line would be inserted if a line in\nthe footer was broken into multiple lines (which is inconsistent with its\nbehavior if no lines were broken). For example, this command would produce:\n\n---\nSample-field: no blank line below\n(cherry picked from commit ...)\n---\n\nbut would also produce:\n\n---\nSample-field: multiple-line field body\n that causes a blank line below\n\n(cherry picked from commit ...)\n---\n\nThis, among other things, trips up tools that look for the last paragraph\n(including sequencer.c itself).\n\nRFC 2822 allows field bodies to be split into multiple lines, especially (but\nnot only) to deal with the line-width limitations described in the\nspecification, referring to this as \"folding\".\n\nTeach sequencer.c to treat split and unsplit field bodies in the same way (that\nis, to not include the blank line).\n\nSigned-off-by: Jonathan Tan \n---\n sequencer.c | 19 +++++++++++++++++--\n t/t3511-cherry-pick-x.sh | 19 +++++++++++++++++++\n 2 files changed, 36 insertions(+), 2 deletions(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex 3804fa9..411dd50 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -26,10 +26,20 @@ static GIT_PATH_FUNC(git_path_opts_file, SEQ_OPTS_FILE)\n static GIT_PATH_FUNC(git_path_seq_dir, SEQ_DIR)\n static GIT_PATH_FUNC(git_path_head_file, SEQ_HEAD_FILE)\n \n-static int is_rfc2822_line(const char *buf, int len)\n+static int is_rfc2822_line(const char *buf, int len,\n+\t\t\t int allow_folding_continuation)\n {\n \tint i;\n \n+\t/*\n+\t * Section 2.2.3 of RFC 2822 allows field bodies to continue onto\n+\t * multiple lines, referred to as \"folding\". Such continuation lines\n+\t * start with whitespace.\n+\t */\n+\tif (allow_folding_continuation)\n+\t\tif (len && (buf[0] == ' ' || buf[0] == '\\t'))\n+\t\t\treturn 1;\n+\n \tfor (i = 0; i < len; i++) {\n \t\tint ch = buf[i];\n \t\tif (ch == ':')\n@@ -64,6 +74,7 @@ static int has_conforming_footer(struct strbuf *sb, struct strbuf *sob,\n \tint len = sb->len - ignore_footer;\n \tconst char *buf = sb->buf;\n \tint found_sob = 0;\n+\tint allow_folding_continuation = 0;\n \n \t/* footer must end with newline */\n \tif (!len || buf[len - 1] != '\\n')\n@@ -92,7 +103,11 @@ static int has_conforming_footer(struct strbuf *sb, struct strbuf *sob,\n \t\t\t; /* do nothing */\n \t\tk++;\n \n-\t\tfound_rfc2822 = is_rfc2822_line(buf + i, k - i - 1);\n+\t\tfound_rfc2822 = is_rfc2822_line(buf + i,\n+\t\t\t\t k - i - 1,\n+\t\t\t\t allow_folding_continuation);\n+\t\tallow_folding_continuation = 1;\n+\n \t\tif (found_rfc2822 && sob &&\n \t\t !strncmp(buf + i, sob->buf, sob->len))\n \t\t\tfound_sob = k;\ndiff --git a/t/t3511-cherry-pick-x.sh b/t/t3511-cherry-pick-x.sh\nindex 9cce5ae..1a50339 100755\n--- a/t/t3511-cherry-pick-x.sh\n+++ b/t/t3511-cherry-pick-x.sh\n@@ -36,6 +36,11 @@ mesg_with_cherry_footer=\"$mesg_with_footer_sob\n (cherry picked from commit da39a3ee5e6b4b0d3255bfef95601890afd80709)\n Tested-by: C.U. Thor \"\n \n+mesg_with_folding_footer=\"$mesg_no_footer\n+\n+Field: This is a very long field body\n+ that is continued onto another line\"\n+\n mesg_unclean=\"$mesg_one_line\n \n \n@@ -68,6 +73,8 @@ test_expect_success setup '\n \tgit reset --hard initial &&\n \ttest_commit \"$mesg_with_cherry_footer\" foo b mesg-with-cherry-footer &&\n \tgit reset --hard initial &&\n+\ttest_commit \"$mesg_with_folding_footer\" foo b mesg-with-folding-footer &&\n+\tgit reset --hard initial &&\n \ttest_config commit.cleanup verbatim &&\n \ttest_commit \"$mesg_unclean\" foo b mesg-unclean &&\n \ttest_unconfig commit.cleanup &&\n@@ -234,6 +241,18 @@ test_expect_success 'cherry-pick -x -s treats \"(cherry picked from...\" line as p\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'cherry-pick -x does not insert blank line when folding footer is found' '\n+\tpristine_detach initial &&\n+\tsha1=$(git rev-parse mesg-with-folding-footer^0) &&\n+\tgit cherry-pick -x mesg-with-folding-footer &&\n+\tcat <<-EOF >expect &&\n+\t\t$mesg_with_folding_footer\n+\t\t(cherry picked from commit $sha1)\n+\tEOF\n+\tgit log -1 --pretty=format:%B >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_expect_success 'cherry-pick preserves commit message' '\n \tpristine_detach initial &&\n \tprintf \"$mesg_unclean\" >expect &&\n-- \n2.8.0.rc3.226.g39d4020\n\n"},{"id":"300937","messageId":"xmqqy439rabb.fsf@gitster.mtv.corp.google.com","threadId":"43987","inReplyTo":"1472846322-5592-1-git-send-email-jonathantanmy@google.com","subject":"Re: [PATCH] sequencer: support folding in rfc2822 footer","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-09-03T02:23:04Z","receivedAt":"2016-09-03T02:23:12Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Tan writes:\n\n> Sample-field: multiple-line field body\n> that causes a blank line below\n\nI am not sure this is unconditionally good, or may cause problems to\nthose with workflows you did not consider when you wrote this patch.\n\nNot being too lenient here historically has been a deliberate\ndecision to avoid misidentification of non \"footers\". Does Git\nitself produce some folded footer line? If Git itself produced such\nfolded lines, I'd be a lot more receptive to this change, but I do\nnot think that is the case here.\n\nA slightly related tangent. An unconditionally good change you\ncould make is to allow folding of in-body headers. I.e. you can\nhave e.g.\n\n\t-- >8 --\n\tSubject: [PATCH] sequencer: support in-body headers that are\n folded according to RFC2822 rules\n\n\tThe first paragraph after the above long title begins\n\there...\n\nin the body of the msssage, and I _think_ we do not fold it properly\nwhen applying such a patch. We should, as that is something that\nappears in format-patch output (i.e. something Git itself produces,\nunlike the folded \"footer\").\n"},{"id":"301235","messageId":"29cb0f55-f729-80af-cdca-64e927fa97c0@google.com","threadId":"43987","inReplyTo":"xmqqy439rabb.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] sequencer: support folding in rfc2822 footer","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2016-09-06T22:08:05Z","receivedAt":"2016-09-06T22:08:14Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"On 09/02/2016 07:23 PM, Junio C Hamano wrote:\n> Jonathan Tan writes:\n>\n>> Sample-field: multiple-line field body\n>> that causes a blank line below\n>\n> I am not sure this is unconditionally good, or may cause problems to\n> those with workflows you did not consider when you wrote this patch.\n>\n> Not being too lenient here historically has been a deliberate\n> decision to avoid misidentification of non \"footers\". Does Git\n> itself produce some folded footer line? If Git itself produced such\n> folded lines, I'd be a lot more receptive to this change, but I do\n> not think that is the case here.\n\nI don't think Git produces any folded lines, but folded lines do appear \nin footers in projects that use Git. For example, some Android commits \nhave multi-line \"Test:\" fields (example, [1]) and some Linux commits \nhave multi-line \"Tested-by:\" fields (example, [2]).\n\nTaking the Android commit as an example, this would mean that \ncherrypicking that commit would create a whole new footer, and tripping \nup tools (for example, Gerrit, which looks for \"Change-Id:\" in the last \nparagraph). But this would not happen if \"Test:\" was single-line instead \nof multi-line - which seems inconsistent.\n\n[1] \nhttps://android.googlesource.com/platform/frameworks/base/+/4c5281862f750cbc9d7355a07ef1a5545b9b3523\n[2] \nhttps://kernel.googlesource.com/pub/scm/linux/kernel/git/stable/linux-stable/+/69f92f67b68ab7028ffe15f0eea76b59f8859383\n\n> A slightly related tangent. An unconditionally good change you\n> could make is to allow folding of in-body headers. I.e. you can\n> have e.g.\n>\n> \t-- >8 --\n> \tSubject: [PATCH] sequencer: support in-body headers that are\n> folded according to RFC2822 rules\n>\n> \tThe first paragraph after the above long title begins\n> \there...\n>\n> in the body of the msssage, and I _think_ we do not fold it properly\n> when applying such a patch. We should, as that is something that\n> appears in format-patch output (i.e. something Git itself produces,\n> unlike the folded \"footer\").\n\nOK, I'll take a look at this.\n"},{"id":"301236","messageId":"fbdda11c-c53e-f9fc-8b3a-934810215c5f@google.com","threadId":"43987","inReplyTo":"29cb0f55-f729-80af-cdca-64e927fa97c0@google.com","subject":"Re: [PATCH] sequencer: support folding in rfc2822 footer","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2016-09-06T23:30:24Z","receivedAt":"2016-09-06T23:30:35Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"On 09/06/2016 03:08 PM, Jonathan Tan wrote:\n> On 09/02/2016 07:23 PM, Junio C Hamano wrote:\n>> A slightly related tangent. An unconditionally good change you\n>> could make is to allow folding of in-body headers. I.e. you can\n>> have e.g.\n>>\n>> -- >8 --\n>> Subject: [PATCH] sequencer: support in-body headers that are\n>> folded according to RFC2822 rules\n>>\n>> The first paragraph after the above long title begins\n>> here...\n>>\n>> in the body of the msssage, and I _think_ we do not fold it properly\n>> when applying such a patch. We should, as that is something that\n>> appears in format-patch output (i.e. something Git itself produces,\n>> unlike the folded \"footer\").\n>\n> OK, I'll take a look at this.\n\nIt turns out that Git seems to already do this, at least for Subject. \nTranscript below:\n\n$ echo one > file.txt\n$ git add file.txt\n$ git commit -m x\n[master (root-commit) 2389483] x\n 1 file changed, 1 insertion(+)\n create mode 100644 file.txt\n$ echo two > file.txt\n$ git commit -am 'this is a very long subject to test line wrapping this \nis a very long subject to test line wrapping'\n[master ca86792] this is a very long subject to test line wrapping this \nis a very long subject to test line wrapping\n 1 file changed, 1 insertion(+), 1 deletion(-)\n$ git format-patch HEAD^\n0001-this-is-a-very-long-subject-to-test-line-wrapping-th.patch\n$ cat 0001-this-is-a-very-long-subject-to-test-line-wrapping-th.patch\n\nSubject: [PATCH] this is a very long subject to test line wrapping this is a\n very long subject to test line wrapping\n\n"},{"id":"301240","messageId":"20160907063819.dd7aulnlsytcuyqj@sigill.intra.peff.net","threadId":"43987","inReplyTo":"fbdda11c-c53e-f9fc-8b3a-934810215c5f@google.com","subject":"Re: [PATCH] sequencer: support folding in rfc2822 footer","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-09-07T06:38:19Z","receivedAt":"2016-09-07T06:38:27Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Sep 06, 2016 at 04:30:24PM -0700, Jonathan Tan wrote:\n\n> On 09/06/2016 03:08 PM, Jonathan Tan wrote:\n> > On 09/02/2016 07:23 PM, Junio C Hamano wrote:\n> > > A slightly related tangent. An unconditionally good change you\n> > > could make is to allow folding of in-body headers. I.e. you can\n> > > have e.g.\n> > > \n> > > -- >8 --\n> > > Subject: [PATCH] sequencer: support in-body headers that are\n> > > folded according to RFC2822 rules\n> > > \n> > > The first paragraph after the above long title begins\n> > > here...\n> > > \n> > > in the body of the msssage, and I _think_ we do not fold it properly\n> > > when applying such a patch. We should, as that is something that\n> > > appears in format-patch output (i.e. something Git itself produces,\n> > > unlike the folded \"footer\").\n> > \n> > OK, I'll take a look at this.\n> \n> It turns out that Git seems to already do this, at least for Subject.\n\nRight, because \"Subject\" is actually a real RFC 2822 header in the\ngenerated email message. Not only do we expect things like mail readers\nto handle this, but we _have_ to wrap at a certain point to meet the\nstandard[1].\n\nI don't think any part of Git ever shunts \"Subject\" to an in-body\nheader, though I'd guess people do it manually all the time. \n\n> $ git format-patch HEAD^\n> 0001-this-is-a-very-long-subject-to-test-line-wrapping-th.patch\n> $ cat 0001-this-is-a-very-long-subject-to-test-line-wrapping-th.patch\n> \n> Subject: [PATCH] this is a very long subject to test line wrapping this is a\n> very long subject to test line wrapping\n> \n\nSo the interesting bit is what happens with:\n\n git checkout master^\n git am 0001-*\n\nand with:\n\n perl -lpe '\n # Bump subject down to in-body header.\n if (/^Subject:/) {\n\tprint \"Subject: real subject\";\n\tprint \"\";\n }\n ' 0001-* >patch\n git checkout master^\n git am patch\n\nIt looks like we get the first one right, but not the second.\n\n-Peff\n\n[1] A careful reader may note that arbitrarily-long body lines,\n including in-body headers and footers, may _also_ run afoul of\n the body line-length limits. The \"right\" solution there is\n probably quoted-printable, but it's ugly enough that I wouldn't do\n so unless we see a real-world case where the line lengths are a\n problem.\n"},{"id":"302058","messageId":"cover.1474047135.git.jonathantanmy@google.com","threadId":"43987","inReplyTo":"20160907063819.dd7aulnlsytcuyqj@sigill.intra.peff.net","subject":"[RFC/PATCH 0/3] handle multiline in-body headers","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2016-09-16T17:37:21Z","receivedAt":"2016-09-16T17:37:33Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Thanks, Peff, for the explanation and the method to reproduce the issue.\n\nThe issue seems to be in mailinfo.c - this patch set addresses that, and I have\nalso included a test for \"git am\" in t/t4150-am.sh to show the effect of this\npatch set on that command.\n\nJonathan Tan (3):\n mailinfo: refactor commit message processing\n mailinfo: correct malformed test example\n mailinfo: handle in-body header continuations\n\n mailinfo.c | 165 ++++++++++++++++++++++++++++-------\n mailinfo.h | 1 +\n t/t4150-am.sh | 23 +++++\n t/t5100-mailinfo.sh | 4 +-\n t/t5100/info0008--no-inbody-headers | 5 ++\n t/t5100/info0018 | 5 ++\n t/t5100/msg0008--no-inbody-headers | 6 ++\n t/t5100/msg0015--no-inbody-headers | 1 +\n t/t5100/msg0018 | 2 +\n t/t5100/patch0008--no-inbody-headers | 0\n t/t5100/patch0018 | 6 ++\n t/t5100/sample.mbox | 20 +++++\n 12 files changed, 206 insertions(+), 32 deletions(-)\n create mode 100644 t/t5100/info0008--no-inbody-headers\n create mode 100644 t/t5100/info0018\n create mode 100644 t/t5100/msg0008--no-inbody-headers\n create mode 100644 t/t5100/msg0018\n create mode 100644 t/t5100/patch0008--no-inbody-headers\n create mode 100644 t/t5100/patch0018\n\n-- \n2.10.0.rc2.20.g5b18e70\n\n"},{"id":"302059","messageId":"7dbb4bc0659056211b27f0033c73f0d558efdb54.1474047135.git.jonathantanmy@google.com","threadId":"43987","inReplyTo":"cover.1474047135.git.jonathantanmy@google.com","subject":"[RFC/PATCH 1/3] mailinfo: refactor commit message processing","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2016-09-16T17:37:22Z","receivedAt":"2016-09-16T17:37:36Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Within the processing of the commit message, check for a scissors line\nor a patchbreak line first (before checking for in-body headers) so that\na subsequent patch modifying the processing of in-body headers would not\ncause a scissors line or patchbreak line to be misidentified.\n\nIf a line could be both an in-body header and a scissors line (for\nexample, \"From: -- >8 --\"), this is considered a fatal error\n(previously, it would be interpreted as an in-body header). (It is not\npossible for a line to be both an in-body header and a patchbreak line,\nsince both require different prefixes.)\n\nThe following enumeration shows that processing is the same except (as\ndescribed above) the in-body header + scissors line case.\n\no in-body header (check_header OK)\n o passes UTF-8 conversion\n o [described above] is scissors line\n o [not possible] is patchbreak line\n o [not possible] is blank line\n o is none of the above - processed as header\n o fails UTF-8 conversion - processed as header\no not in-body header\n o passes UTF-8 conversion\n o is scissors line - processed as scissors\n o is patchbreak line - processed as patchbreak\n o is blank line - ignored if in header_stage\n o is none of the above - log message\n o fails UTF-8 conversion - input error\n\nAs for the result left in \"line\" (after the invocation of\nhandle_commit_msg), it is unused (by its caller, handle_filter, and by\nhandle_filter's callers, handle_boundary and handle_body) unless this\nline is a patchbreak line, in which case handle_patch is subsequently\ncalled (in handle_filter) on \"line\". In this case, \"line\" must have\npassed UTF-8 conversion both before and after this patch, so the result\nis still the same overall.\n\nSigned-off-by: Jonathan Tan \n---\n mailinfo.c | 145 ++++++++++++++++++++++++++++++++++++++++++++++++-------------\n 1 file changed, 115 insertions(+), 30 deletions(-)\n\ndiff --git a/mailinfo.c b/mailinfo.c\nindex e19abe3..23a56c2 100644\n--- a/mailinfo.c\n+++ b/mailinfo.c\n@@ -340,23 +340,56 @@ static struct strbuf *decode_b_segment(const struct strbuf *b_seg)\n \treturn out;\n }\n \n-static int convert_to_utf8(struct mailinfo *mi,\n-\t\t\t struct strbuf *line, const char *charset)\n+/*\n+ * Attempts to convert line into UTF-8, storing the result in line.\n+ *\n+ * This differs from convert_to_utf8 in that conversion non-success is not\n+ * considered an error case - mi->input_error is not set, and no error message\n+ * is printed.\n+ *\n+ * If the conversion is unnecessary, returns 0 and stores NULL in old_buf (if\n+ * old_buf is not NULL).\n+ *\n+ * If the conversion is successful, returns 0 and stores the unconverted string\n+ * in old_buf and old_len (if they are respectively not NULL).\n+ *\n+ * If the conversion is unsuccessful, returns -1.\n+ */\n+static int try_convert_to_utf8(const struct mailinfo *mi, struct strbuf *line,\n+\t\t\t const char *charset, char **old_buf,\n+\t\t\t size_t *old_len)\n {\n-\tchar *out;\n+\tchar *utf8;\n \n-\tif (!mi->metainfo_charset || !charset || !*charset)\n+\tif (!mi->metainfo_charset || !charset || !*charset ||\n+\t same_encoding(mi->metainfo_charset, charset)) {\n+\t\tif (old_buf)\n+\t\t\t*old_buf = NULL;\n \t\treturn 0;\n+\t}\n \n-\tif (same_encoding(mi->metainfo_charset, charset))\n+\tutf8 = reencode_string(line->buf, mi->metainfo_charset, charset);\n+\tif (utf8) {\n+\t\tchar *temp = strbuf_detach(line, old_len);\n+\t\tif (old_buf)\n+\t\t\t*old_buf = temp;\n+\t\tstrbuf_attach(line, utf8, strlen(utf8), strlen(utf8));\n \t\treturn 0;\n-\tout = reencode_string(line->buf, mi->metainfo_charset, charset);\n-\tif (!out) {\n+\t}\n+\treturn -1;\n+}\n+\n+/*\n+ * Converts line into UTF-8, setting mi->input_error to -1 upon failure.\n+ */\n+static int convert_to_utf8(struct mailinfo *mi,\n+\t\t\t struct strbuf *line, const char *charset)\n+{\n+\tif (try_convert_to_utf8(mi, line, charset, NULL, NULL)) {\n \t\tmi->input_error = -1;\n \t\treturn error(\"cannot convert from %s to %s\",\n \t\t\t charset, mi->metainfo_charset);\n \t}\n-\tstrbuf_attach(line, out, strlen(out), strlen(out));\n \treturn 0;\n }\n \n@@ -515,6 +548,13 @@ static int check_header(struct mailinfo *mi,\n \treturn ret;\n }\n \n+static int check_header_raw(struct mailinfo *mi,\n+\t\t\t char *buf, size_t len,\n+\t\t\t struct strbuf *hdr_data[], int overwrite) {\n+\tconst struct strbuf sb = {0, len, buf};\n+\treturn check_header(mi, &sb, hdr_data, overwrite);\n+}\n+\n static void decode_transfer_encoding(struct mailinfo *mi, struct strbuf *line)\n {\n \tstruct strbuf *ret;\n@@ -623,32 +663,48 @@ static int is_scissors_line(const struct strbuf *line)\n \t\tgap * 2 < perforation);\n }\n \n-static int handle_commit_msg(struct mailinfo *mi, struct strbuf *line)\n+static int resembles_rfc2822_header(const struct strbuf *line)\n {\n-\tassert(!mi->filter_stage);\n+\tchar *c;\n \n-\tif (mi->header_stage) {\n-\t\tif (!line->len || (line->len == 1 && line->buf[0] == '\\n'))\n+\tif (!isalpha(line->buf[0]))\n+\t\treturn 0;\n+\n+\tfor (c = line->buf + 1; *c != 0; c++) {\n+\t\tif (*c == ':')\n+\t\t\treturn 1;\n+\t\telse if (*c != '-' && !isalpha(*c))\n \t\t\treturn 0;\n \t}\n+\treturn 0;\n+}\n \n-\tif (mi->use_inbody_headers && mi->header_stage) {\n-\t\tmi->header_stage = check_header(mi, line, mi->s_hdr_data, 0);\n-\t\tif (mi->header_stage)\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\tmi->header_stage = 0;\n+static int handle_commit_msg(struct mailinfo *mi, struct strbuf *line)\n+{\n+\tint ret = 0;\n+\tint utf8_result;\n+\tchar *old_buf;\n+\tsize_t old_len;\n+\n+\tassert(!mi->filter_stage);\n \n-\t/* normalize the log message to UTF-8. */\n-\tif (convert_to_utf8(mi, line, mi->charset.buf))\n-\t\treturn 0; /* mi->input_error already set */\n+\t/*\n+\t * Obtain UTF8 for scissors line and patchbreak checks, but retain the\n+\t * undecoded line in case we need to process it as an in-body header.\n+\t */\n+\tutf8_result = try_convert_to_utf8(mi, line, mi->charset.buf, &old_buf,\n+\t\t\t\t\t &old_len);\n \n-\tif (mi->use_scissors && is_scissors_line(line)) {\n+\tif (!utf8_result && mi->use_scissors && is_scissors_line(line)) {\n \t\tint i;\n \n+\t\tif (resembles_rfc2822_header(line))\n+\t\t\t/*\n+\t\t\t * Explicitly reject scissor lines that resemble a RFC\n+\t\t\t * 2822 header, to avoid being prone to error.\n+\t\t\t */\n+\t\t\tdie(\"scissors line resembles RFC 2822 header\");\n+\n \t\tstrbuf_setlen(&mi->log_message, 0);\n \t\tmi->header_stage = 1;\n \n@@ -661,18 +717,47 @@ static int handle_commit_msg(struct mailinfo *mi, struct strbuf *line)\n \t\t\t\tstrbuf_release(mi->s_hdr_data[i]);\n \t\t\tmi->s_hdr_data[i] = NULL;\n \t\t}\n-\t\treturn 0;\n+\t\tgoto handle_commit_msg_out;\n \t}\n-\n-\tif (patchbreak(line)) {\n+\tif (!utf8_result && patchbreak(line)) {\n \t\tif (mi->message_id)\n \t\t\tstrbuf_addf(&mi->log_message,\n \t\t\t\t \"Message-Id: %s\\n\", mi->message_id);\n-\t\treturn 1;\n+\t\tret = 1;\n+\t\tgoto handle_commit_msg_out;\n \t}\n \n+\tif (mi->header_stage) {\n+\t\tchar *buf = old_buf ? old_buf : line->buf;\n+\t\tif (buf[0] == 0 || (buf[0] == '\\n' && buf[1] == 0))\n+\t\t\tgoto handle_commit_msg_out;\n+\t}\n+\n+\tif (mi->use_inbody_headers && mi->header_stage) {\n+\t\tchar *buf = old_buf ? old_buf : line->buf;\n+\t\tsize_t len = old_buf ? old_len : line->len;\n+\t\tmi->header_stage = check_header_raw(mi, buf, len,\n+\t\t\t\t\t\t mi->s_hdr_data, 0);\n+\t\tif (mi->header_stage)\n+\t\t\tgoto handle_commit_msg_out;\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\tmi->header_stage = 0;\n+\n+\t/* If adding as a log message, conversion to UTF-8 is required. */\n+\tif (utf8_result) {\n+\t\tmi->input_error = -1;\n+\t\terror(\"cannot convert from %s to %s\",\n+\t\t mi->charset.buf, mi->metainfo_charset);\n+\t\tgoto handle_commit_msg_out;\n+\t}\n \tstrbuf_addbuf(&mi->log_message, line);\n-\treturn 0;\n+\n+handle_commit_msg_out:\n+\tfree(old_buf);\n+\treturn ret;\n }\n \n static void handle_patch(struct mailinfo *mi, const struct strbuf *line)\n-- \n2.10.0.rc2.20.g5b18e70\n\n"},{"id":"302060","messageId":"5dbb0b0f64906fd18c217908cd2c04e74d80fa68.1474047135.git.jonathantanmy@google.com","threadId":"43987","inReplyTo":"cover.1474047135.git.jonathantanmy@google.com","subject":"[RFC/PATCH 2/3] mailinfo: correct malformed test example","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2016-09-16T17:37:23Z","receivedAt":"2016-09-16T17:37:43Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"An existing sample message (0015) in the tests for mailinfo contains an\nindented line immediately after an in-body header (without any\nintervening blank line). Correct this by adding the blank line, in\npreparation for a subsequent patch that will treat such indented lines\nas RFC 2822 continuation lines (instead of as part of the commit\nmessage).\n\nTo ensure that non-indented lines immediately after an in-body header\nare still treated correctly (now and in the future), 0008 has been\nupdated to test both the case when in-body headers are used and the case\nwhen they are not used.\n\nSigned-off-by: Jonathan Tan \n---\n t/t5100/info0008--no-inbody-headers | 5 +++++\n t/t5100/msg0008--no-inbody-headers | 6 ++++++\n t/t5100/msg0015--no-inbody-headers | 1 +\n t/t5100/patch0008--no-inbody-headers | 0\n t/t5100/sample.mbox | 1 +\n 5 files changed, 13 insertions(+)\n create mode 100644 t/t5100/info0008--no-inbody-headers\n create mode 100644 t/t5100/msg0008--no-inbody-headers\n create mode 100644 t/t5100/patch0008--no-inbody-headers\n\ndiff --git a/t/t5100/info0008--no-inbody-headers b/t/t5100/info0008--no-inbody-headers\nnew file mode 100644\nindex 0000000..e8a2951\n--- /dev/null\n+++ b/t/t5100/info0008--no-inbody-headers\n@@ -0,0 +1,5 @@\n+Author: Junio C Hamano\n+Email: junio@kernel.org\n+Subject: another patch\n+Date: Fri, 9 Jun 2006 00:44:16 -0700\n+\ndiff --git a/t/t5100/msg0008--no-inbody-headers b/t/t5100/msg0008--no-inbody-headers\nnew file mode 100644\nindex 0000000..d6e950e\n--- /dev/null\n+++ b/t/t5100/msg0008--no-inbody-headers\n@@ -0,0 +1,6 @@\n+From: A U Thor \n+Subject: [PATCH] another patch\n+>Here is an empty patch from A U Thor.\n+\n+Hey you forgot the patch!\n+\ndiff --git a/t/t5100/msg0015--no-inbody-headers b/t/t5100/msg0015--no-inbody-headers\nindex be5115b..44a6ce7 100644\n--- a/t/t5100/msg0015--no-inbody-headers\n+++ b/t/t5100/msg0015--no-inbody-headers\n@@ -1,3 +1,4 @@\n From: bogosity\n+\n - a list\n - of stuff\ndiff --git a/t/t5100/patch0008--no-inbody-headers b/t/t5100/patch0008--no-inbody-headers\nnew file mode 100644\nindex 0000000..e69de29\ndiff --git a/t/t5100/sample.mbox b/t/t5100/sample.mbox\nindex 8b2ae06..ba8b208 100644\n--- a/t/t5100/sample.mbox\n+++ b/t/t5100/sample.mbox\n@@ -656,6 +656,7 @@ Subject: check bogus body header (from)\n Date: Fri, 9 Jun 2006 00:44:16 -0700\n \n From: bogosity\n+\n - a list\n - of stuff\n ---\n-- \n2.10.0.rc2.20.g5b18e70\n\n"},{"id":"302061","messageId":"0152df30db0972d61ff45b2b099ad1242aacd431.1474047135.git.jonathantanmy@google.com","threadId":"43987","inReplyTo":"cover.1474047135.git.jonathantanmy@google.com","subject":"[RFC/PATCH 3/3] mailinfo: handle in-body header continuations","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2016-09-16T17:37:24Z","receivedAt":"2016-09-16T17:37:46Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Mailinfo currently handles multi-line headers, but it does not handle\nmulti-line in-body headers. Teach it to handle such headers, for\nexample, for this input:\n\n Subject: a very long\n broken line\n\n Subject: another very long\n broken line\n\ninterpret the in-body subject to be \"another very long broken line\"\ninstead of \"another very long\".\n\nInstead of repeatedly calling \"check_header\" (as in this patch), one\nalternate method to accomplish this would be to have a buffer of\npotential header text in struct mailinfo to be flushed whenever a header\nis known to end (for example, if we detect the start of a new header),\nbut this makes the logic more complicated - for example, the flushing\nwould not only invoke check_header but would also need to reconstruct\nthe original lines, possibly decode them into UTF-8, and store them in\nlog_message, and any failures would be noticed a few \"lines\" away from\nthe original failure point. Also, care would need to be taken to flush\nthe buffer at all appropriate places.\n\nAnother alternate would be to modify \"read_one_header_line\" to accept\nstrings of lines instead of reading its own from a FILE pointer, but\nthis would also require a buffer, with the same issues.\n\nSigned-off-by: Jonathan Tan \n---\n mailinfo.c | 24 ++++++++++++++++++++++--\n mailinfo.h | 1 +\n t/t4150-am.sh | 23 +++++++++++++++++++++++\n t/t5100-mailinfo.sh | 4 ++--\n t/t5100/info0018 | 5 +++++\n t/t5100/msg0018 | 2 ++\n t/t5100/patch0018 | 6 ++++++\n t/t5100/sample.mbox | 19 +++++++++++++++++++\n 8 files changed, 80 insertions(+), 4 deletions(-)\n create mode 100644 t/t5100/info0018\n create mode 100644 t/t5100/msg0018\n create mode 100644 t/t5100/patch0018\n\ndiff --git a/mailinfo.c b/mailinfo.c\nindex 23a56c2..3bbdf74 100644\n--- a/mailinfo.c\n+++ b/mailinfo.c\n@@ -729,8 +729,10 @@ static int handle_commit_msg(struct mailinfo *mi, struct strbuf *line)\n \n \tif (mi->header_stage) {\n \t\tchar *buf = old_buf ? old_buf : line->buf;\n-\t\tif (buf[0] == 0 || (buf[0] == '\\n' && buf[1] == 0))\n+\t\tif (buf[0] == 0 || (buf[0] == '\\n' && buf[1] == 0)) {\n+\t\t\tstrbuf_reset(&mi->last_inbody_header);\n \t\t\tgoto handle_commit_msg_out;\n+\t\t}\n \t}\n \n \tif (mi->use_inbody_headers && mi->header_stage) {\n@@ -738,8 +740,24 @@ static int handle_commit_msg(struct mailinfo *mi, struct strbuf *line)\n \t\tsize_t len = old_buf ? old_len : line->len;\n \t\tmi->header_stage = check_header_raw(mi, buf, len,\n \t\t\t\t\t\t mi->s_hdr_data, 0);\n-\t\tif (mi->header_stage)\n+\t\tif (mi->header_stage) {\n+\t\t\tstrbuf_reset(&mi->last_inbody_header);\n+\t\t\tstrbuf_add(&mi->last_inbody_header, buf, len);\n \t\t\tgoto handle_commit_msg_out;\n+\t\t}\n+\n+\t\tif (mi->last_inbody_header.len &&\n+\t\t (buf[0] == ' ' || buf[0] == '\\t')) {\n+\t\t\tstrbuf_strip_suffix(&mi->last_inbody_header, \"\\n\");\n+\t\t\tstrbuf_add(&mi->last_inbody_header, buf, len);\n+\t\t\tmi->header_stage = check_header(mi,\n+\t\t\t\t\t\t\t&mi->last_inbody_header,\n+\t\t\t\t\t\t\tmi->s_hdr_data, 1);\n+\t\t\tif (mi->header_stage)\n+\t\t\t\tgoto handle_commit_msg_out;\n+\t\t}\n+\n+\t\tmi->header_stage = 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@@ -1086,6 +1104,7 @@ void setup_mailinfo(struct mailinfo *mi)\n \tstrbuf_init(&mi->email, 0);\n \tstrbuf_init(&mi->charset, 0);\n \tstrbuf_init(&mi->log_message, 0);\n+\tstrbuf_init(&mi->last_inbody_header, 0);\n \tmi->header_stage = 1;\n \tmi->use_inbody_headers = 1;\n \tmi->content_top = mi->content;\n@@ -1099,6 +1118,7 @@ void clear_mailinfo(struct mailinfo *mi)\n \tstrbuf_release(&mi->name);\n \tstrbuf_release(&mi->email);\n \tstrbuf_release(&mi->charset);\n+\tstrbuf_release(&mi->last_inbody_header);\n \tfree(mi->message_id);\n \n \tfor (i = 0; mi->p_hdr_data[i]; i++)\ndiff --git a/mailinfo.h b/mailinfo.h\nindex 93776a7..ab2d0dd 100644\n--- a/mailinfo.h\n+++ b/mailinfo.h\n@@ -27,6 +27,7 @@ struct mailinfo {\n \tint patch_lines;\n \tint filter_stage; /* still reading log or are we copying patch? */\n \tint header_stage; /* still checking in-body headers? */\n+\tstruct strbuf last_inbody_header;\n \tstruct strbuf **p_hdr_data;\n \tstruct strbuf **s_hdr_data;\n \ndiff --git a/t/t4150-am.sh b/t/t4150-am.sh\nindex 9ce9424..89a5bac 100755\n--- a/t/t4150-am.sh\n+++ b/t/t4150-am.sh\n@@ -977,4 +977,27 @@ test_expect_success 'am --patch-format=mboxrd handles mboxrd' '\n \ttest_cmp msg out\n '\n \n+test_expect_success 'am works with multi-line in-body headers' '\n+\tFORTY=\"String that has a length of more than forty characters\" &&\n+\tLONG=\"$FORTY $FORTY\" &&\n+\trm -fr .git/rebase-apply &&\n+\tgit checkout -f first &&\n+\techo one >> file &&\n+\tgit commit -am \"$LONG\" --author=\"$LONG \" &&\n+\tgit format-patch --stdout -1 >patch &&\n+\t# bump from, date, and subject down to in-body header\n+\tperl -lpe \"\n+\t\tif (/^From:/) {\n+\t\t\tprint \\\"From: x \\\";\n+\t\t\tprint \\\"Date: Sat, 1 Jan 2000 00:00:00 +0000\\\";\n+\t\t\tprint \\\"Subject: x\\n\\\";\n+\t\t}\n+\t\" patch >msg &&\n+\tgit checkout HEAD^ &&\n+\tgit am msg &&\n+\t# Ensure that the author and full message are present\n+\tgit cat-file commit HEAD | grep \"^author.*long@example.com\" &&\n+\tgit cat-file commit HEAD | grep \"^$LONG\"\n+'\n+\n test_done\ndiff --git a/t/t5100-mailinfo.sh b/t/t5100-mailinfo.sh\nindex 1a5a546..99e8722 100755\n--- a/t/t5100-mailinfo.sh\n+++ b/t/t5100-mailinfo.sh\n@@ -11,7 +11,7 @@ test_expect_success 'split sample box' \\\n \t'git mailsplit -o. \"$TEST_DIRECTORY\"/t5100/sample.mbox >last &&\n \tlast=$(cat last) &&\n \techo total is $last &&\n-\ttest $(cat last) = 17'\n+\ttest $(cat last) = 18'\n \n check_mailinfo () {\n \tmail=$1 opt=$2\n@@ -51,7 +51,7 @@ test_expect_success 'split box with rfc2047 samples' \\\n \techo total is $last &&\n \ttest $(cat rfc2047/last) = 11'\n \n-for mail in rfc2047/00*\n+for mail in rfc2047/0001\n do\n \ttest_expect_success \"mailinfo $mail\" '\n \t\tgit mailinfo -u $mail-msg $mail-patch <$mail >$mail-info &&\ndiff --git a/t/t5100/info0018 b/t/t5100/info0018\nnew file mode 100644\nindex 0000000..d53e749\n--- /dev/null\n+++ b/t/t5100/info0018\n@@ -0,0 +1,5 @@\n+Author: Another Thor\n+Email: a.thor@example.com\n+Subject: This one contains a tab and a space\n+Date: Fri, 9 Jun 2006 00:44:16 -0700\n+\ndiff --git a/t/t5100/msg0018 b/t/t5100/msg0018\nnew file mode 100644\nindex 0000000..56de83d\n--- /dev/null\n+++ b/t/t5100/msg0018\n@@ -0,0 +1,2 @@\n+a commit message\n+\ndiff --git a/t/t5100/patch0018 b/t/t5100/patch0018\nnew file mode 100644\nindex 0000000..789df6d\n--- /dev/null\n+++ b/t/t5100/patch0018\n@@ -0,0 +1,6 @@\n+diff --git a/foo b/foo\n+index e69de29..d95f3ad 100644\n+--- a/foo\n++++ b/foo\n+@@ -0,0 +1 @@\n++content\ndiff --git a/t/t5100/sample.mbox b/t/t5100/sample.mbox\nindex ba8b208..ae61497 100644\n--- a/t/t5100/sample.mbox\n+++ b/t/t5100/sample.mbox\n@@ -700,3 +700,22 @@ index e69de29..d95f3ad 100644\n +++ b/foo\n @@ -0,0 +1 @@\n +New content\n+From nobody Mon Sep 17 00:00:00 2001\n+From: A U Thor \n+Subject: check multiline inbody headers\n+Date: Fri, 9 Jun 2006 00:44:16 -0700\n+\n+From: Another Thor\n+ \n+Subject: This one contains\n+\ta tab\n+ and a space\n+\n+a commit message\n+\n+diff --git a/foo b/foo\n+index e69de29..d95f3ad 100644\n+--- a/foo\n++++ b/foo\n+@@ -0,0 +1 @@\n++content\n-- \n2.10.0.rc2.20.g5b18e70\n\n"},{"id":"302067","messageId":"xmqqshszk8ag.fsf@gitster.mtv.corp.google.com","threadId":"43987","inReplyTo":"cover.1474047135.git.jonathantanmy@google.com","subject":"Re: [RFC/PATCH 0/3] handle multiline in-body headers","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-09-16T18:29:27Z","receivedAt":"2016-09-16T18:29:41Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Tan writes:\n\n> Thanks, Peff, for the explanation and the method to reproduce the issue.\n>\n> The issue seems to be in mailinfo.c - this patch set addresses that, and I have\n> also included a test for \"git am\" in t/t4150-am.sh to show the effect of this\n> patch set on that command.\n\nThanks, will take a look.\n\n> Jonathan Tan (3):\n> mailinfo: refactor commit message processing\n> mailinfo: correct malformed test example\n> mailinfo: handle in-body header continuations\n>\n> mailinfo.c | 165 ++++++++++++++++++++++++++++-------\n> mailinfo.h | 1 +\n> t/t4150-am.sh | 23 +++++\n> t/t5100-mailinfo.sh | 4 +-\n> t/t5100/info0008--no-inbody-headers | 5 ++\n> t/t5100/info0018 | 5 ++\n> t/t5100/msg0008--no-inbody-headers | 6 ++\n> t/t5100/msg0015--no-inbody-headers | 1 +\n> t/t5100/msg0018 | 2 +\n> t/t5100/patch0008--no-inbody-headers | 0\n> t/t5100/patch0018 | 6 ++\n> t/t5100/sample.mbox | 20 +++++\n> 12 files changed, 206 insertions(+), 32 deletions(-)\n> create mode 100644 t/t5100/info0008--no-inbody-headers\n> create mode 100644 t/t5100/info0018\n> create mode 100644 t/t5100/msg0008--no-inbody-headers\n> create mode 100644 t/t5100/msg0018\n> create mode 100644 t/t5100/patch0008--no-inbody-headers\n> create mode 100644 t/t5100/patch0018\n"},{"id":"302069","messageId":"xmqqoa3nk6a5.fsf@gitster.mtv.corp.google.com","threadId":"43987","inReplyTo":"7dbb4bc0659056211b27f0033c73f0d558efdb54.1474047135.git.jonathantanmy@google.com","subject":"Re: [RFC/PATCH 1/3] mailinfo: refactor commit message processing","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-09-16T19:12:50Z","receivedAt":"2016-09-16T19:12:59Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Tan writes:\n\n> Within the processing of the commit message, check for a scissors line\n> or a patchbreak line first (before checking for in-body headers) so that\n> a subsequent patch modifying the processing of in-body headers would not\n> cause a scissors line or patchbreak line to be misidentified.\n>\n> If a line could be both an in-body header and a scissors line (for\n> example, \"From: -- >8 --\"), this is considered a fatal error\n> (previously, it would be interpreted as an in-body header).\n\nThe scissors line is designed to allow garbage other than scissors\nand perforation marks to be on the same line, i.e.\n\n /*\n * The mark must be at least 8 bytes long (e.g. \"-- >8 --\").\n * Even though there can be arbitrary cruft on the same line\n * (e.g. \"cut here\"), in order to avoid misidentification, the\n * perforation must occupy more than a third of the visible\n * width of the line, and dashes and scissors must occupy more\n * than half of the perforation.\n */\n\nEven though it is not likely for people to do so, it would probably\nbe nicer if we can treat\n\n\tFrom: -- >8 -- cut -- >8 -- >8 -- here -- >8 --\n\nas a scissors line instead of making it a fatal error, by treating\nthat \"From:\" as just a random garbage.\n\nBut this is a minor point. It is not worth to make it work like so\nif the resulting code will become messier.\n\n> The following enumeration shows that processing is the same except (as\n> described above) the in-body header + scissors line case.\n>\n> o in-body header (check_header OK)\n> o passes UTF-8 conversion\n> o [described above] is scissors line\n> o [not possible] is patchbreak line\n> o [not possible] is blank line\n> o is none of the above - processed as header\n> o fails UTF-8 conversion - processed as header\n> o not in-body header\n> o passes UTF-8 conversion\n> o is scissors line - processed as scissors\n> o is patchbreak line - processed as patchbreak\n> o is blank line - ignored if in header_stage\n> o is none of the above - log message\n> o fails UTF-8 conversion - input error\n>\n> As for the result left in \"line\" (after the invocation of\n> handle_commit_msg), it is unused (by its caller, handle_filter, and by\n> handle_filter's callers, handle_boundary and handle_body) unless this\n> line is a patchbreak line, in which case handle_patch is subsequently\n> called (in handle_filter) on \"line\". In this case, \"line\" must have\n> passed UTF-8 conversion both before and after this patch, so the result\n> is still the same overall.\n>\n> Signed-off-by: Jonathan Tan \n> ---\n> mailinfo.c | 145 ++++++++++++++++++++++++++++++++++++++++++++++++-------------\n> 1 file changed, 115 insertions(+), 30 deletions(-)\n>\n> diff --git a/mailinfo.c b/mailinfo.c\n> index e19abe3..23a56c2 100644\n> --- a/mailinfo.c\n> +++ b/mailinfo.c\n> @@ -340,23 +340,56 @@ static struct strbuf *decode_b_segment(const struct strbuf *b_seg)\n> \treturn out;\n> }\n> \n> -static int convert_to_utf8(struct mailinfo *mi,\n> -\t\t\t struct strbuf *line, const char *charset)\n> +/*\n> + * Attempts to convert line into UTF-8, storing the result in line.\n> + *\n> + * This differs from convert_to_utf8 in that conversion non-success is not\n> + * considered an error case - mi->input_error is not set, and no error message\n> + * is printed.\n> + *\n> + * If the conversion is unnecessary, returns 0 and stores NULL in old_buf (if\n> + * old_buf is not NULL).\n> + *\n> + * If the conversion is successful, returns 0 and stores the unconverted string\n> + * in old_buf and old_len (if they are respectively not NULL).\n> + *\n> + * If the conversion is unsuccessful, returns -1.\n> + */\n> +static int try_convert_to_utf8(const struct mailinfo *mi, struct strbuf *line,\n> +\t\t\t const char *charset, char **old_buf,\n> +\t\t\t size_t *old_len)\n> {\n> -\tchar *out;\n> +\tchar *utf8;\n> \n> -\tif (!mi->metainfo_charset || !charset || !*charset)\n> +\tif (!mi->metainfo_charset || !charset || !*charset ||\n> +\t same_encoding(mi->metainfo_charset, charset)) {\n> +\t\tif (old_buf)\n> +\t\t\t*old_buf = NULL;\n> \t\treturn 0;\n> +\t}\n> \n> -\tif (same_encoding(mi->metainfo_charset, charset))\n> +\tutf8 = reencode_string(line->buf, mi->metainfo_charset, charset);\n> +\tif (utf8) {\n> +\t\tchar *temp = strbuf_detach(line, old_len);\n> +\t\tif (old_buf)\n> +\t\t\t*old_buf = temp;\n> +\t\tstrbuf_attach(line, utf8, strlen(utf8), strlen(utf8));\n> \t\treturn 0;\n> -\tout = reencode_string(line->buf, mi->metainfo_charset, charset);\n> -\tif (!out) {\n> +\t}\n> +\treturn -1;\n> +}\n> +\n> +/*\n> + * Converts line into UTF-8, setting mi->input_error to -1 upon failure.\n> + */\n> +static int convert_to_utf8(struct mailinfo *mi,\n> +\t\t\t struct strbuf *line, const char *charset)\n> +{\n> +\tif (try_convert_to_utf8(mi, line, charset, NULL, NULL)) {\n> \t\tmi->input_error = -1;\n> \t\treturn error(\"cannot convert from %s to %s\",\n> \t\t\t charset, mi->metainfo_charset);\n> \t}\n> -\tstrbuf_attach(line, out, strlen(out), strlen(out));\n> \treturn 0;\n> }\n\nPlease split this part into its own patch. IIUC, it moves the meat\nof convert_to_utf8() to a more silent try_convert_to_utf8() and then\nmakes the former a thin wrapper of the latter. Which by itself is a\ngood change but does not have anything to do with \"fix handling of\nthe in-body headers\", other than that the main fix wants to have\nsuch a more silent helper for its own use.\n\n\n> @@ -515,6 +548,13 @@ static int check_header(struct mailinfo *mi,\n> \treturn ret;\n> }\n> \n> +static int check_header_raw(struct mailinfo *mi,\n> +\t\t\t char *buf, size_t len,\n> +\t\t\t struct strbuf *hdr_data[], int overwrite) {\n> +\tconst struct strbuf sb = {0, len, buf};\n> +\treturn check_header(mi, &sb, hdr_data, overwrite);\n> +}\n\nIIUC, this is a helper for callers that do not have a strbuf but\ninstead have pair to perform the same check_header() the\ncallers that have strbuf can do.\n\nAs check_header() uses the strbuf as a read-only entity, wrapping\nthe pair in a temporary strbuf like this is safe.\n\nThe incoming should conceptually be \"const char *\", but it's\nOK.\n\nIf check_header() didn't call any helper function that gets passed\n&sb as a strbuf, or if convertiong the helper function to take a\n pair instead, I would actually suggest refactoring this\nthe other way around, though. That is, move the implementation of\ncheck_header() to this function, updating its reference to line->buf\nand line->len to reference to and , and then make\ncheck_header() a thin wrapper that does\n\n check_header(mi, const struct strbuf *line,\n struct strbuf *hdr_data[], int overwrite)\n {\n return check_header_raw(mi, line->buf, line->len,\n hdr_data, overwrite);\n }\n\nI didn't check how involved to update cmp_header() to take \npair. If it does not look too bad, then I think I would prefer to\ndo it that way, and as before, make that conversion a separate\npreparatory patch.\n\n> @@ -623,32 +663,48 @@ static int is_scissors_line(const struct strbuf *line)\n> \t\tgap * 2 < perforation);\n> }\n> \n> -static int handle_commit_msg(struct mailinfo *mi, struct strbuf *line)\n> +static int resembles_rfc2822_header(const struct strbuf *line)\n> {\n> -\tassert(!mi->filter_stage);\n> +\tchar *c;\n> \n> -\tif (mi->header_stage) {\n> -\t\tif (!line->len || (line->len == 1 && line->buf[0] == '\\n'))\n> +\tif (!isalpha(line->buf[0]))\n> +\t\treturn 0;\n> +\n> +\tfor (c = line->buf + 1; *c != 0; c++) {\n> +\t\tif (*c == ':')\n> +\t\t\treturn 1;\n> +\t\telse if (*c != '-' && !isalpha(*c))\n> \t\t\treturn 0;\n> \t}\n> +\treturn 0;\n> +}\n\nIs this helper supposed to handle any rfc2822 looking header, or\nonly the ones we expect to see as in-body header?\n\n\n> -\tif (mi->use_inbody_headers && mi->header_stage) {\n> -\t\tmi->header_stage = check_header(mi, line, mi->s_hdr_data, 0);\n> -\t\tif (mi->header_stage)\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\tmi->header_stage = 0;\n> +static int handle_commit_msg(struct mailinfo *mi, struct strbuf *line)\n> +{\n> +\tint ret = 0;\n> +\tint utf8_result;\n> +\tchar *old_buf;\n> +\tsize_t old_len;\n> +\n> +\tassert(!mi->filter_stage);\n> \n> -\t/* normalize the log message to UTF-8. */\n> -\tif (convert_to_utf8(mi, line, mi->charset.buf))\n> -\t\treturn 0; /* mi->input_error already set */\n> +\t/*\n> +\t * Obtain UTF8 for scissors line and patchbreak checks, but retain the\n> +\t * undecoded line in case we need to process it as an in-body header.\n> +\t */\n> +\tutf8_result = try_convert_to_utf8(mi, line, mi->charset.buf, &old_buf,\n> +\t\t\t\t\t &old_len);\n\nJust a minor style suggestion. As come in a\npair, fold the line before them, so that the readers can easily\nsee the association between them. I.e.\n\n utf8_result = try_convert_to_utf8(mi, line, mi->charset.buf,\n &old_buf, &old_len);\n\n\n> -\tif (mi->use_scissors && is_scissors_line(line)) {\n> +\tif (!utf8_result && mi->use_scissors && is_scissors_line(line)) {\n> \t\tint i;\n> \n> +\t\tif (resembles_rfc2822_header(line))\n> +\t\t\t/*\n> +\t\t\t * Explicitly reject scissor lines that resemble a RFC\n> +\t\t\t * 2822 header, to avoid being prone to error.\n> +\t\t\t */\n> +\t\t\tdie(\"scissors line resembles RFC 2822 header\");\n> +\n\nI guess \"disambiguate to favor scissors\" is not that difficult ;-)\n\n> \t\tstrbuf_setlen(&mi->log_message, 0);\n> \t\tmi->header_stage = 1;\n> \n> @@ -661,18 +717,47 @@ static int handle_commit_msg(struct mailinfo *mi, struct strbuf *line)\n> \t\t\t\tstrbuf_release(mi->s_hdr_data[i]);\n> \t\t\tmi->s_hdr_data[i] = NULL;\n> \t\t}\n> -\t\treturn 0;\n> +\t\tgoto handle_commit_msg_out;\n> \t}\n> -\n> -\tif (patchbreak(line)) {\n> +\tif (!utf8_result && patchbreak(line)) {\n> \t\tif (mi->message_id)\n> \t\t\tstrbuf_addf(&mi->log_message,\n> \t\t\t\t \"Message-Id: %s\\n\", mi->message_id);\n> -\t\treturn 1;\n> +\t\tret = 1;\n> +\t\tgoto handle_commit_msg_out;\n> \t}\n> \n> +\tif (mi->header_stage) {\n> +\t\tchar *buf = old_buf ? old_buf : line->buf;\n> +\t\tif (buf[0] == 0 || (buf[0] == '\\n' && buf[1] == 0))\n> +\t\t\tgoto handle_commit_msg_out;\n> +\t}\n> +\n> +\tif (mi->use_inbody_headers && mi->header_stage) {\n> +\t\tchar *buf = old_buf ? old_buf : line->buf;\n> +\t\tsize_t len = old_buf ? old_len : line->len;\n> +\t\tmi->header_stage = check_header_raw(mi, buf, len,\n> +\t\t\t\t\t\t mi->s_hdr_data, 0);\n\nJust a minor comment, but I guess check_header_raw() refactoring is\nnot strictly needed after all, as this callsite can wrap \ninto a temporary strbuf.\n\nUnlike the real header that is read in read_one_header_line() inside\na loop to implement line folding before check_header() is called, we\ncall check_header() before possibly-foldable header lines is fully\nassembled into one header. Probably it comes in later patches, I\nguess.\n\nIt is not immediately obvious to me how this step helps further work\ndone by later patches in the series until I read them, but so far\nwhat this patch did looks understandable to me ;-)\n\nThanks.\n\n\n> +\t\tif (mi->header_stage)\n> +\t\t\tgoto handle_commit_msg_out;\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\tmi->header_stage = 0;\n> +\n> +\t/* If adding as a log message, conversion to UTF-8 is required. */\n> +\tif (utf8_result) {\n> +\t\tmi->input_error = -1;\n> +\t\terror(\"cannot convert from %s to %s\",\n> +\t\t mi->charset.buf, mi->metainfo_charset);\n> +\t\tgoto handle_commit_msg_out;\n> +\t}\n> \tstrbuf_addbuf(&mi->log_message, line);\n> -\treturn 0;\n> +\n> +handle_commit_msg_out:\n> +\tfree(old_buf);\n> +\treturn ret;\n> }\n> \n> static void handle_patch(struct mailinfo *mi, const struct strbuf *line)\n"},{"id":"302070","messageId":"xmqqk2ebk5zh.fsf@gitster.mtv.corp.google.com","threadId":"43987","inReplyTo":"5dbb0b0f64906fd18c217908cd2c04e74d80fa68.1474047135.git.jonathantanmy@google.com","subject":"Re: [RFC/PATCH 2/3] mailinfo: correct malformed test example","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-09-16T19:19:14Z","receivedAt":"2016-09-16T19:20:16Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Tan writes:\n\n> An existing sample message (0015) in the tests for mailinfo contains an\n> indented line immediately after an in-body header (without any\n> intervening blank line).\n\nThis comes from d25e5159 (\"git am/mailinfo: Don't look at in-body\nheaders when rebasing\", 2009-11-20), where we want to make sure that\na \"From: bogosity\" that isn't meant to be an in-body header is not\nidentified as such, even when it is immediately followed by a\nnon-blank line. \"From: bogosity\" is for msg0015 but the same\napplies to the header-looking block for msg0008.\n\nAdding a blank line there will defeat the whole point of the test,\nwhich is to make sure we don't do anything funky when --no-inbody-headers\nis asked for, no?\n\n> diff --git a/t/t5100/info0008--no-inbody-headers b/t/t5100/info0008--no-inbody-headers\n> new file mode 100644\n> index 0000000..e8a2951\n> --- /dev/null\n> +++ b/t/t5100/info0008--no-inbody-headers\n> @@ -0,0 +1,5 @@\n> +Author: Junio C Hamano\n> +Email: junio@kernel.org\n> +Subject: another patch\n> +Date: Fri, 9 Jun 2006 00:44:16 -0700\n> +\n> diff --git a/t/t5100/msg0008--no-inbody-headers b/t/t5100/msg0008--no-inbody-headers\n> new file mode 100644\n> index 0000000..d6e950e\n> --- /dev/null\n> +++ b/t/t5100/msg0008--no-inbody-headers\n> @@ -0,0 +1,6 @@\n> +From: A U Thor \n> +Subject: [PATCH] another patch\n> +>Here is an empty patch from A U Thor.\n> +\n> +Hey you forgot the patch!\n> +\n> diff --git a/t/t5100/msg0015--no-inbody-headers b/t/t5100/msg0015--no-inbody-headers\n> index be5115b..44a6ce7 100644\n> --- a/t/t5100/msg0015--no-inbody-headers\n> +++ b/t/t5100/msg0015--no-inbody-headers\n> @@ -1,3 +1,4 @@\n> From: bogosity\n> +\n> - a list\n> - of stuff\n> diff --git a/t/t5100/patch0008--no-inbody-headers b/t/t5100/patch0008--no-inbody-headers\n> new file mode 100644\n> index 0000000..e69de29\n> diff --git a/t/t5100/sample.mbox b/t/t5100/sample.mbox\n> index 8b2ae06..ba8b208 100644\n> --- a/t/t5100/sample.mbox\n> +++ b/t/t5100/sample.mbox\n> @@ -656,6 +656,7 @@ Subject: check bogus body header (from)\n> Date: Fri, 9 Jun 2006 00:44:16 -0700\n> \n> From: bogosity\n> +\n> - a list\n> - of stuff\n> ---\n"},{"id":"302071","messageId":"xmqq8turk3aw.fsf@gitster.mtv.corp.google.com","threadId":"43987","inReplyTo":"0152df30db0972d61ff45b2b099ad1242aacd431.1474047135.git.jonathantanmy@google.com","subject":"Re: [RFC/PATCH 3/3] mailinfo: handle in-body header continuations","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-09-16T20:17:11Z","receivedAt":"2016-09-16T20:17:20Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Tan writes:\n\n> Instead of repeatedly calling \"check_header\" (as in this patch), one\n> alternate method to accomplish this would be to have a buffer of\n> potential header text in struct mailinfo to be flushed whenever a header\n> is known to end (for example, if we detect the start of a new header),\n> but this makes the logic more complicated - for example, the flushing\n> would not only invoke check_header but would also need to reconstruct\n> the original lines, possibly decode them into UTF-8, and store them in\n> log_message, and any failures would be noticed a few \"lines\" away from\n> the original failure point. Also, care would need to be taken to flush\n> the buffer at all appropriate places.\n\nI am not sure how much the UTF-8 decoding argument above matters.\n\nThe current way handle_commit_msg() is structured (before any of\nyour patches) is for it to take one raw line at a time and:\n\n - If we haven't seen a non-header line (i.e. at the beginning,\n or we were reading in-body headers), return without doing\n anything.\n\n - If we are told to honor in-body headers and if we haven't seen\n a non-header line, see if the line itself looks like a header\n and if so, handle it as an in-body header and return. If that\n line is not an in-body header, continue processing.\n\n - If the processing reaches at this point, we are done with the\n headers (i.e. mi->header_stage is set to 0).\n\n - Make sure the line is in utf8.\n\n - If it is a scissors line and we are told to honor scissors\n lines, ignore what we have read so far and go back to \"we\n haven't seen a non-header line\" state and return.\n\n - If it is a patch break, return and signal the caller we are\n done with the log message.\n\n - Otherwise accumulate the line as part of the log message.\n\nThe bug we want to address is in the second step. We only look at\nthe first line of folded in-body header line, because we are fed one\nline at a time.\n\nIf we keep the location of UTF8 conversion, and buffered the in-body\nheader in \"struct mailinfo *mi\" (like you seem to do in this patch),\nwhat we will queue there will be _before_ conversion. We'd call\ncheck_header() on it once we know one logical line of a header is\naccumulated, and check_header() would do the right conversion via\ndecode_header() etc., so I do not see why you need to worry about\nthe encoding issues at all.\n\nI wonder if the simplest would be to introduce another state in the\nstate machine that is \"we know we are processing in-body header, and\nwe have read early part of an in-body header line that may not be\ncomplete\".\n\nIn other words, wouldn't something like the illustration at the end\nof this message sufficient? If the body consists solely of in-body\nheader without any patch or patchbreak, we may reach EOF with\nsomething in mi->in_line_header buffer and nothing in\nmi->log_message and without this function getting any chance to\nreturn 1, so a careful caller may want to flush in_line_header, but\nthe overall result of the mailinfo subsystem in such a case would be\nan error (\"you didn't have any patch or a message?\"), so it may not\nmatter too much.\n\nWhat am I missing?\n\nhandle_commit_msg(...)\n{\n\tif (mi->in_line_header->len) {\n\t\t/* we have read the beginning of one in-line header */\n\t\tif (line->len && isspace(*line->buf))\n\t\t\tappend to mi->in_line_header strbuf;\n return 0;\n\t\t/* otherwise we know mi->in_line_header is now complete */\n\t\tcheck_header(mi, mi->in_line_header, ...);\n\t\tstrbuf_reset(&mi->in_line_header);\n\t}\n\n\tif (mi->header_stage && (it is a blank line))\n\t\treturn 0;\n\n\tif (mi->use_inbody_headers && mi->header_stage &&\n\t (the line looks like beginning of 2822 header)) {\n\t\tstrbuf_addbuf(&mi->in_line_header, line);\n\t\treturn 0;\n\t}\n /* otherwise we are no longer looking at headers */\n mi->header_stage = 0;\n\n\t/* normalize the log message to UTF-8. */\n\tif (convert_to_utf8(mi, line, mi->charset.buf))\n\t\treturn 0; /* mi->input_error already set */\n\n\tif (mi->use_scissors && is_scissors_line(line)) {\n\t\tint i;\n\n\t\tstrbuf_setlen(&mi->log_message, 0);\n\t\tmi->header_stage = 1;\n\n\t\t/*\n\t\t * We may have already read \"secondary headers\"; purge\n\t\t * them to give ourselves a clean restart.\n\t\t */\n\t\tfor (i = 0; header[i]; i++) {\n\t\t\tif (mi->s_hdr_data[i])\n\t\t\t\tstrbuf_release(mi->s_hdr_data[i]);\n\t\t\tmi->s_hdr_data[i] = NULL;\n\t\t}\n\t\treturn 0;\n\t}\n\n\tif (patchbreak(line)) {\n\t\tif (mi->message_id)\n\t\t\tstrbuf_addf(&mi->log_message,\n\t\t\t\t \"Message-Id: %s\\n\", mi->message_id);\n\t\treturn 1;\n\t}\n\n\tstrbuf_addbuf(&mi->log_message, line);\n\treturn 0;\n}\n \n"},{"id":"302073","messageId":"1b392241-461e-3b87-400d-70d66903e3d7@google.com","threadId":"43987","inReplyTo":"xmqq8turk3aw.fsf@gitster.mtv.corp.google.com","subject":"Re: [RFC/PATCH 3/3] mailinfo: handle in-body header continuations","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2016-09-16T20:49:18Z","receivedAt":"2016-09-16T20:49:25Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"On 09/16/2016 01:17 PM, Junio C Hamano wrote:\n> In other words, wouldn't something like the illustration at the end\n> of this message sufficient? If the body consists solely of in-body\n> header without any patch or patchbreak, we may reach EOF with\n> something in mi->in_line_header buffer and nothing in\n> mi->log_message and without this function getting any chance to\n> return 1, so a careful caller may want to flush in_line_header, but\n> the overall result of the mailinfo subsystem in such a case would be\n> an error (\"you didn't have any patch or a message?\"), so it may not\n> matter too much.\n\nNoted. (This was one of my concerns - that the caller should, but did \nnot, flush.)\n\n> What am I missing?\n>\n> handle_commit_msg(...)\n> {\n> \tif (mi->in_line_header->len) {\n> \t\t/* we have read the beginning of one in-line header */\n> \t\tif (line->len && isspace(*line->buf))\n\nThis would mean that a message like the following:\n\n From: Me \n -- 8< -- this scissors line will be treated as part of \"From\"\n\nwould have its scissors line treated as a header.\n\nThe main reason why I reordered the checks (in RFC/PATCH 1/3) is to \navoid this (treating a scissors line with an initial space immediately \nfollowing an in-body header as part of a header).\n\n(If this is not a concern then yes, I agree that the way you described \nis simpler and better.)\n\n> \t\t\tappend to mi->in_line_header strbuf;\n> return 0;\n> \t\t/* otherwise we know mi->in_line_header is now complete */\n> \t\tcheck_header(mi, mi->in_line_header, ...);\n> \t\tstrbuf_reset(&mi->in_line_header);\n> \t}\n>\n> \tif (mi->header_stage && (it is a blank line))\n> \t\treturn 0;\n>\n> \tif (mi->use_inbody_headers && mi->header_stage &&\n> \t (the line looks like beginning of 2822 header)) {\n> \t\tstrbuf_addbuf(&mi->in_line_header, line);\n> \t\treturn 0;\n> \t}\n> /* otherwise we are no longer looking at headers */\n> mi->header_stage = 0;\n>\n> \t/* normalize the log message to UTF-8. */\n> \tif (convert_to_utf8(mi, line, mi->charset.buf))\n> \t\treturn 0; /* mi->input_error already set */\n>\n> \tif (mi->use_scissors && is_scissors_line(line)) {\n> \t\tint i;\n>\n> \t\tstrbuf_setlen(&mi->log_message, 0);\n> \t\tmi->header_stage = 1;\n>\n> \t\t/*\n> \t\t * We may have already read \"secondary headers\"; purge\n> \t\t * them to give ourselves a clean restart.\n> \t\t */\n> \t\tfor (i = 0; header[i]; i++) {\n> \t\t\tif (mi->s_hdr_data[i])\n> \t\t\t\tstrbuf_release(mi->s_hdr_data[i]);\n> \t\t\tmi->s_hdr_data[i] = NULL;\n> \t\t}\n> \t\treturn 0;\n> \t}\n>\n> \tif (patchbreak(line)) {\n> \t\tif (mi->message_id)\n> \t\t\tstrbuf_addf(&mi->log_message,\n> \t\t\t\t \"Message-Id: %s\\n\", mi->message_id);\n> \t\treturn 1;\n> \t}\n>\n> \tstrbuf_addbuf(&mi->log_message, line);\n> \treturn 0;\n> }\n>\n>\n"},{"id":"302075","messageId":"xmqqshszimrh.fsf@gitster.mtv.corp.google.com","threadId":"43987","inReplyTo":"1b392241-461e-3b87-400d-70d66903e3d7@google.com","subject":"Re: [RFC/PATCH 3/3] mailinfo: handle in-body header continuations","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-09-16T20:59:46Z","receivedAt":"2016-09-16T21:00:08Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Tan writes:\n\n>> handle_commit_msg(...)\n>> {\n>> \tif (mi->in_line_header->len) {\n>> \t\t/* we have read the beginning of one in-line header */\n>> \t\tif (line->len && isspace(*line->buf))\n>\n> This would mean that a message like the following:\n>\n> From: Me \n> -- 8< -- this scissors line will be treated as part of \"From\"\n>\n> would have its scissors line treated as a header.\n>\n> The main reason why I reordered the checks (in RFC/PATCH 1/3) is to\n> avoid this (treating a scissors line with an initial space immediately\n> following an in-body header as part of a header).\n>\n> (If this is not a concern then yes, I agree that the way you described\n> is simpler and better.)\n\nAhh, OK. I do not think anybody sane would do the \"From:\" thing,\nbut with the \"does it look like 2822 header\" check to decide if the\nfirst header-looking line should be queued, another failure mode may\nbe:\n\n\tany-random-alpha-and-dash-string: \n -- >8 -- cut here -- >8 --\n Subject: real subject\n\n The first line of the real message\n \nI personally do not think it matters that much, but if we wanted to\nprotect us from it we could easily do\n\n if (mi->in_line_header->len) {\n /* we have read the beginning of one in-line header */\n if (line->len && isspace(*line->buf) &&\n !(mi->use_scissors && is_scissors_line(line))) {\n append to mi->in_line_header strbuf;\n return 0;\n }\n /* otherwise we know mi->in_line_header is now complete */\n check_header(mi, mi->in_line_header, ...);\n strbuf_reset(&mi->in_line_header);\n }\n\t...\n\ninstead, I think.\n"},{"id":"302077","messageId":"20160916214641.ib7khsl3fbtn6smg@sigill.intra.peff.net","threadId":"43987","inReplyTo":"xmqqoa3nk6a5.fsf@gitster.mtv.corp.google.com","subject":"Re: [RFC/PATCH 1/3] mailinfo: refactor commit message processing","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-09-16T21:46:41Z","receivedAt":"2016-09-16T21:46:49Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Sep 16, 2016 at 12:12:50PM -0700, Junio C Hamano wrote:\n\n> > +static int check_header_raw(struct mailinfo *mi,\n> > +\t\t\t char *buf, size_t len,\n> > +\t\t\t struct strbuf *hdr_data[], int overwrite) {\n> > +\tconst struct strbuf sb = {0, len, buf};\n> > +\treturn check_header(mi, &sb, hdr_data, overwrite);\n> > +}\n> \n> IIUC, this is a helper for callers that do not have a strbuf but\n> instead have pair to perform the same check_header() the\n> callers that have strbuf can do.\n> \n> As check_header() uses the strbuf as a read-only entity, wrapping\n> the pair in a temporary strbuf like this is safe.\n> \n> The incoming should conceptually be \"const char *\", but it's\n> OK.\n\nI think the \"right\" way to do this would be to continue taking a \"char\n*\", and then strbuf_attach() it. That saves us from unexpectedly\nviolating any strbuf invariants.\n\nIf our assumption that check_header() does not touch the\ncontents turns out to be wrong, neither strategy would inform our\ncaller, though. I think you'd want something like:\n\n assert(sb.buf == buf);\n\nafter check_header() returns (though I guess we are in theory protected\nby the \"const\").\n\nThat being said...\n\n> If check_header() didn't call any helper function that gets passed\n> &sb as a strbuf, or if convertiong the helper function to take a\n> pair instead, I would actually suggest refactoring this\n> the other way around, though. That is, move the implementation of\n> check_header() to this function, updating its reference to line->buf\n> and line->len to reference to and , and then make\n> check_header() a thin wrapper that does\n> \n> check_header(mi, const struct strbuf *line,\n> struct strbuf *hdr_data[], int overwrite)\n> {\n> return check_header_raw(mi, line->buf, line->len,\n> hdr_data, overwrite);\n> }\n\nThis is _way_ better, and it looks like check_header() could handle it\neasily. Looking at it, I also suspect the cascading if in that function\ncould be made more pleasant by modeling cmp_header()'s interface after\nskip_prefix_mem(), but that is totally orthogonal and optional.\n\n-Peff\n"},{"id":"302078","messageId":"20160916215140.eocblts556lzdk7k@sigill.intra.peff.net","threadId":"43987","inReplyTo":"0152df30db0972d61ff45b2b099ad1242aacd431.1474047135.git.jonathantanmy@google.com","subject":"Re: [RFC/PATCH 3/3] mailinfo: handle in-body header continuations","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-09-16T21:51:41Z","receivedAt":"2016-09-16T21:51:49Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Sep 16, 2016 at 10:37:24AM -0700, Jonathan Tan wrote:\n\n> Mailinfo currently handles multi-line headers, but it does not handle\n> multi-line in-body headers. Teach it to handle such headers, for\n> example, for this input:\n> \n> Subject: a very long\n> broken line\n> \n> Subject: another very long\n> broken line\n> \n> interpret the in-body subject to be \"another very long broken line\"\n> instead of \"another very long\".\n\nThis puzzled me; we should stop parsing in-body headers after the first\nblank line. But then I realized you probably meant the first \"Subject\"\nto be the real mail header.\n\nI wonder if it would be more obvious with an example like:\n\n From: ...\n Date: ...\n Subject: the actual mail subject\n\n Subject: a very long\n broken line\n\nOr something.\n\n-Peff\n"},{"id":"302082","messageId":"128036e3-47b4-b7ae-034f-8d227a62729f@google.com","threadId":"43987","inReplyTo":"xmqqshszimrh.fsf@gitster.mtv.corp.google.com","subject":"Re: [RFC/PATCH 3/3] mailinfo: handle in-body header continuations","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2016-09-16T22:36:05Z","receivedAt":"2016-09-16T22:36:27Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"On 09/16/2016 01:59 PM, Junio C Hamano wrote:\n> if (mi->in_line_header->len) {\n> /* we have read the beginning of one in-line header */\n> if (line->len && isspace(*line->buf) &&\n> !(mi->use_scissors && is_scissors_line(line))) {\n\nMinor note: this means that the scissors check appears twice in the \ncode, once here and once below (for the non-header case).\n\n> append to mi->in_line_header strbuf;\n> return 0;\n> }\n> /* otherwise we know mi->in_line_header is now complete */\n> check_header(mi, mi->in_line_header, ...);\n\n(Sorry - should have also noticed this in your original e-mail.)\n\nI'm concerned about what happens if check_header fails - we would then \nhave some lines which need to be treated as log messages. (At least, \nthey are currently treated that way.)\n\nTo treat them as log messages, we would need to convert them into UTF-8, \nwhich may possibly fail, so we would have to figure out how to clean up \n(we have to clean up because we cannot `die` immediately, at least to \npreserve the current behavior). Also, we are likely to detect such a \nfailure only while processing a subsequent line - this non-\"fail fast\" \ncurrently is fine, but I'm concerned that it will hinder future \ndevelopment (especially when debugging).\n\nMinor note: the buffer would also need to be more complicated (instead \nof the current single buffer), either:\n\no store newlines in that buffer (and we would need to remove all\n newlines before passing to check_header), or\no 2 buffers: one with newlines (for log messages) and one without (for\n check_header).\n\nIn light of the above (multiple scissors checks, late detection of \nfailure, more complicated buffer), it seems clearer to me to just change \nthe order of the checks (as in RFC/PATCH 1/3). This necessitates holding \non to the old un-decoded buf and len, but this seems easier to me than \nthe above.\n\n> strbuf_reset(&mi->in_line_header);\n> }\n> \t...\n"},{"id":"302084","messageId":"2bfc2fc7-f16b-6d51-7353-54d38353464a@google.com","threadId":"43987","inReplyTo":"xmqqk2ebk5zh.fsf@gitster.mtv.corp.google.com","subject":"Re: [RFC/PATCH 2/3] mailinfo: correct malformed test example","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2016-09-16T22:42:48Z","receivedAt":"2016-09-16T22:42:55Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"On 09/16/2016 12:19 PM, Junio C Hamano wrote:\n> Jonathan Tan writes:\n>\n>> An existing sample message (0015) in the tests for mailinfo contains an\n>> indented line immediately after an in-body header (without any\n>> intervening blank line).\n>\n> This comes from d25e5159 (\"git am/mailinfo: Don't look at in-body\n> headers when rebasing\", 2009-11-20), where we want to make sure that\n> a \"From: bogosity\" that isn't meant to be an in-body header is not\n> identified as such, even when it is immediately followed by a\n> non-blank line. \"From: bogosity\" is for msg0015 but the same\n> applies to the header-looking block for msg0008.\n>\n> Adding a blank line there will defeat the whole point of the test,\n> which is to make sure we don't do anything funky when --no-inbody-headers\n> is asked for, no?\n\nBefore I revise the patch set...I think that the point of 0015 would be \nhandled by 0008 (after this patch is applied), but if you prefer that \n0015 retain its purpose, I can unindent the bullet list in 0015 instead \nof adding the extra line (and then dropping all 0008 changes). Would \nthat be better? (0015 needs to be changed somehow, because its indented \nline would be interpreted as a continuation line after RFC/PATCH 3/3 is \napplied.)\n"},{"id":"302086","messageId":"xmqqbmznihe0.fsf@gitster.mtv.corp.google.com","threadId":"43987","inReplyTo":"2bfc2fc7-f16b-6d51-7353-54d38353464a@google.com","subject":"Re: [RFC/PATCH 2/3] mailinfo: correct malformed test example","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-09-16T22:55:51Z","receivedAt":"2016-09-16T22:55:59Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Tan writes:\n\n> On 09/16/2016 12:19 PM, Junio C Hamano wrote:\n>> Jonathan Tan writes:\n>>\n>>> An existing sample message (0015) in the tests for mailinfo contains an\n>>> indented line immediately after an in-body header (without any\n>>> intervening blank line).\n>>\n>> This comes from d25e5159 (\"git am/mailinfo: Don't look at in-body\n>> headers when rebasing\", 2009-11-20), where we want to make sure that\n>> a \"From: bogosity\" that isn't meant to be an in-body header is not\n>> identified as such, even when it is immediately followed by a\n>> non-blank line. \"From: bogosity\" is for msg0015 but the same\n>> applies to the header-looking block for msg0008.\n>>\n>> Adding a blank line there will defeat the whole point of the test,\n>> which is to make sure we don't do anything funky when --no-inbody-headers\n>> is asked for, no?\n>\n> Before I revise the patch set...I think that the point of 0015 would\n> be handled by 0008 (after this patch is applied), but if you prefer\n> that 0015 retain its purpose, I can unindent the bullet list in 0015\n> instead of adding the extra line (and then dropping all 0008\n> changes). Would that be better? (0015 needs to be changed somehow,\n> because its indented line would be interpreted as a continuation line\n> after RFC/PATCH 3/3 is applied.)\n\nHmph, these:\n\n t/t5100/info0008--no-inbody-headers | 5 +++++\n t/t5100/msg0008--no-inbody-headers | 6 ++++++\n t/t5100/msg0015--no-inbody-headers | 1 +\n\nhave --no-inbody-headers in their names; wouldn't that an indication\nthat they are expected output when mailinfo is run while in-body\nheader feature disabled?\n\nI would have expected that it would make more sense to make no\nchange to sample.mbox and have updated expectation to outputs in the\ncase where in-body header feature is enabled.\n\nTo make sure this new feature will not break in the future, we would\nwant a brand new message with a folded in-body header added to the\nsample.mbox, and see how it is parsed by mailinfo with in-body\nheader feature enabled (and disabled).\n\n"},{"id":"302088","messageId":"xmqq7fabigzf.fsf@gitster.mtv.corp.google.com","threadId":"43987","inReplyTo":"128036e3-47b4-b7ae-034f-8d227a62729f@google.com","subject":"Re: [RFC/PATCH 3/3] mailinfo: handle in-body header continuations","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-09-16T23:04:36Z","receivedAt":"2016-09-16T23:04:55Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Tan writes:\n\n> On 09/16/2016 01:59 PM, Junio C Hamano wrote:\n>> if (mi->in_line_header->len) {\n>> /* we have read the beginning of one in-line header */\n>> if (line->len && isspace(*line->buf) &&\n>> !(mi->use_scissors && is_scissors_line(line))) {\n>\n> Minor note: this means that the scissors check appears twice in the\n> code, once here and once below (for the non-header case).\n\nYes. I actually was wondering if it is even more sensible to always\nhave the scissors check at the very beginning. Even if we saw a\nhalf-written in-body header already in the message, when we see a\nscissors line, we clear the slate and restart as if the line after\nthe scissors is the first line in the body of the message.\n\n>> append to mi->in_line_header strbuf;\n>> return 0;\n>> }\n>> /* otherwise we know mi->in_line_header is now complete */\n>> check_header(mi, mi->in_line_header, ...);\n>\n> (Sorry - should have also noticed this in your original e-mail.)\n>\n> I'm concerned about what happens if check_header fails - we would then\n> have some lines which need to be treated as log messages. (At least,\n> they are currently treated that way.)\n\nI actually think we should refactor check_header() further so that\nin-body header processing does not even see things that shouldn't be\nchanged. The current check_header() should be used only for real\nmail headers, and then a reduced version of check_header() that is\ncalled for in-body will _ONLY_ handle the header lines that are\nhandled by the first \"search for the interesting parts\" loop.\n\nAnd of course we would update your \"does it look like rfc2822?\" to\nmatch what are handled by the \"interesting parts\" loop. That I\nthink would match the current behaviour much better, I would think.\n\nThe \">From \" and \"[PATCH]\" cases in check_header() should not even\nbe there. We should handle them inside handle_commit_msg(), as\nthese two cases should never appear in the real header part of a\nmessage.\n\nAnd if we clean it up like that, I do not think we would ever need\nto worry about \"ah, it looked like a header but it is not after\nall\". And not having to worry about it is a good thing and should\nbe one of the primary goals in this conversion, I whould think.\n\nThanks.\n\n\n"},{"id":"302091","messageId":"d5eab197-e079-4c95-2661-9a576b9b5707@google.com","threadId":"43987","inReplyTo":"xmqq7fabigzf.fsf@gitster.mtv.corp.google.com","subject":"Re: [RFC/PATCH 3/3] mailinfo: handle in-body header continuations","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2016-09-17T00:22:25Z","receivedAt":"2016-09-17T00:22:33Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"On 09/16/2016 04:04 PM, Junio C Hamano wrote:\n> Jonathan Tan writes:\n>> I'm concerned about what happens if check_header fails - we would then\n>> have some lines which need to be treated as log messages. (At least,\n>> they are currently treated that way.)\n>\n> I actually think we should refactor check_header() further so that\n> in-body header processing does not even see things that shouldn't be\n> changed. The current check_header() should be used only for real\n> mail headers, and then a reduced version of check_header() that is\n> called for in-body will _ONLY_ handle the header lines that are\n> handled by the first \"search for the interesting parts\" loop.\n>\n> And of course we would update your \"does it look like rfc2822?\" to\n> match what are handled by the \"interesting parts\" loop. That I\n> think would match the current behaviour much better, I would think.\n\nThere would be a bit of code duplication in that this \"does it look like \nrfc2822\" function would also need to account for duplicate headers (e.g. \n2 \"Subject:\" lines in the in-body headers) because check_header would \nreject the 2nd one, but that is minor. (Alternatively, we could just \nallow duplicate headers in the in-body headers.)\n\n> The \">From \" and \"[PATCH]\" cases in check_header() should not even\n> be there. We should handle them inside handle_commit_msg(), as\n> these two cases should never appear in the real header part of a\n> message.\n\n> And if we clean it up like that, I do not think we would ever need\n> to worry about \"ah, it looked like a header but it is not after\n> all\". And not having to worry about it is a good thing and should\n> be one of the primary goals in this conversion, I whould think.\n\nYes, this makes sense. I'll go ahead and make a patch set implementing \nthis (unless anyone has any objections).\n"},{"id":"302092","messageId":"3a27685f-a53b-03a7-93d5-0492638faf51@google.com","threadId":"43987","inReplyTo":"xmqqbmznihe0.fsf@gitster.mtv.corp.google.com","subject":"Re: [RFC/PATCH 2/3] mailinfo: correct malformed test example","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2016-09-17T00:31:44Z","receivedAt":"2016-09-17T00:31:52Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"On 09/16/2016 03:55 PM, Junio C Hamano wrote:\n> Hmph, these:\n>\n> t/t5100/info0008--no-inbody-headers | 5 +++++\n> t/t5100/msg0008--no-inbody-headers | 6 ++++++\n> t/t5100/msg0015--no-inbody-headers | 1 +\n>\n> have --no-inbody-headers in their names; wouldn't that an indication\n> that they are expected output when mailinfo is run while in-body\n> header feature disabled?\n\nYes, that's correct (they are the test data for when the in-body header \nfeature is disabled).\n\n> I would have expected that it would make more sense to make no\n> change to sample.mbox and have updated expectation to outputs in the\n> case where in-body header feature is enabled.\n\nThe sample.mbox file contains the following:\n\n From nobody Mon Sep 17 00:00:00 2001\n From: A U Thor \n Subject: check bogus body header (from)\n Date: Fri, 9 Jun 2006 00:44:16 -0700\n\n From: bogosity\n - a list\n - of stuff\n\nUnchanged, the subsequent patch would break this test because it would \ninterpret that as a multi-line \"From\" in-body header when in-body \nheaders are *not* disabled.\n\nBesides changing sample.mbox, the other way to make sure that this test \npasses is to suppress the test when in-body headers are *not* disabled, \nbut looking at t5100* (directory and script), it seemed more \nstraightforward to modify sample.mbox.\n\nThe patch I sent added a blank line after \"From: bogosity\", but removing \nthe spaces before \"- a list\" and \"- of stuff\" would work too.\n\n> To make sure this new feature will not break in the future, we would\n> want a brand new message with a folded in-body header added to the\n> sample.mbox, and see how it is parsed by mailinfo with in-body\n> header feature enabled (and disabled).\n\nOK, I'll add this test. (The subsequent patch already has the brand new \nmessage, but not the test where in-body headers are disabled.)\n"},{"id":"302096","messageId":"xmqqpoo3gpa7.fsf@gitster.mtv.corp.google.com","threadId":"43987","inReplyTo":"3a27685f-a53b-03a7-93d5-0492638faf51@google.com","subject":"Re: [RFC/PATCH 2/3] mailinfo: correct malformed test example","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-09-17T03:48:16Z","receivedAt":"2016-09-17T03:48:25Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Tan writes:\n\n> From: bogosity\n> - a list\n> - of stuff\n>\n> Unchanged, the subsequent patch would break this test because it would\n> interpret that as a multi-line \"From\" in-body header when in-body\n> headers are *not* disabled.\n\nYes, that is totally expected. So I would be perfectly fine if your\npatch changed the test vector for that case, saying \"Allowing a\nfolded in-body header means the expected result for the above three\nlines has to change\".\n\n"}]}