{"thread":{"id":"44859","subject":"[PATCH 1/2] mailinfo: Add support for keep_cr","startedAt":"2017-01-12T07:23:20Z","lastAt":"2017-01-12T20:18:48Z","messageCount":7,"participants":["Matthew Wilcox","Jonathan Tan","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"309257","messageId":"1484212824-14108-1-git-send-email-mawilcox@linuxonhyperv.com","threadId":"44859","inReplyTo":null,"subject":"[PATCH 1/2] mailinfo: Add support for keep_cr","fromName":"Matthew Wilcox","fromEmail":"mawilcox@linuxonhyperv.com","sentAt":"2017-01-12T09:20:23Z","receivedAt":"2017-01-12T07:23:20Z","isPatch":true,"sender":{"key":"mawilcox@linuxonhyperv.com","avatar":null},"body":"From: Matthew Wilcox \n\nIf you have a base-64 encoded patch with CRLF endings (as produced\nby forwarding a patch from Outlook to a Linux machine, for example),\nthe keep_cr setting is not honoured because keep_cr is only passed\nto mailsplit, which does not look through the encoding. The keep_cr\nlogic needs to be applied after the base64 decode. I copied that\nlogic to handle_filter(), and rather than add a new keep_cr parameter\nto handle_filter, I opted to add keep_cr to struct mailinfo; it seemed\nappropriate given use_scissors was already there.\n\nThen I needed to initialise keep_cr in the struct mailinfo passed from\ngit-am, and rather than thread a 'keep_cr' parameter all the way through\nto parse_mail(), I decided to add keep_cr to struct am_state, which let\nit be removed as a parameter from five other functions.\n\nSigned-off-by: Matthew Wilcox \n---\n builtin/am.c | 49 ++++++++++++++++++++++++-------------------------\n mailinfo.c | 6 ++++++\n mailinfo.h | 1 +\n 3 files changed, 31 insertions(+), 25 deletions(-)\n\ndiff --git a/builtin/am.c b/builtin/am.c\nindex 31fb60578..6cb6e6ca8 100644\n--- a/builtin/am.c\n+++ b/builtin/am.c\n@@ -124,6 +124,7 @@ struct am_state {\n \tint keep; /* enum keep_type */\n \tint message_id;\n \tint scissors; /* enum scissors_type */\n+\tint keep_cr;\n \tstruct argv_array git_apply_opts;\n \tconst char *resolvemsg;\n \tint committer_date_is_author_date;\n@@ -143,6 +144,7 @@ static void am_state_init(struct am_state *state, const char *dir)\n \n \tmemset(state, 0, sizeof(*state));\n \n+\tstate->keep_cr = -1;\n \tassert(dir);\n \tstate->dir = xstrdup(dir);\n \n@@ -697,7 +699,7 @@ static int detect_patch_format(const char **paths)\n * a mbox file or a Maildir. Returns 0 on success, -1 on failure.\n */\n static int split_mail_mbox(struct am_state *state, const char **paths,\n-\t\t\t\tint keep_cr, int mboxrd)\n+\t\t\t\tint mboxrd)\n {\n \tstruct child_process cp = CHILD_PROCESS_INIT;\n \tstruct strbuf last = STRBUF_INIT;\n@@ -707,7 +709,7 @@ static int split_mail_mbox(struct am_state *state, const char **paths,\n \targv_array_pushf(&cp.args, \"-d%d\", state->prec);\n \targv_array_pushf(&cp.args, \"-o%s\", state->dir);\n \targv_array_push(&cp.args, \"-b\");\n-\tif (keep_cr)\n+\tif (state->keep_cr)\n \t\targv_array_push(&cp.args, \"--keep-cr\");\n \tif (mboxrd)\n \t\targv_array_push(&cp.args, \"--mboxrd\");\n@@ -737,7 +739,7 @@ typedef int (*mail_conv_fn)(FILE *out, FILE *in, int keep_cr);\n * Returns 0 on success, -1 on failure.\n */\n static int split_mail_conv(mail_conv_fn fn, struct am_state *state,\n-\t\t\tconst char **paths, int keep_cr)\n+\t\t\tconst char **paths)\n {\n \tstatic const char *stdin_only[] = {\"-\", NULL};\n \tint i;\n@@ -766,7 +768,7 @@ static int split_mail_conv(mail_conv_fn fn, struct am_state *state,\n \t\t\treturn error_errno(_(\"could not open '%s' for writing\"),\n \t\t\t\t\t mail);\n \n-\t\tret = fn(out, in, keep_cr);\n+\t\tret = fn(out, in, state->keep_cr);\n \n \t\tfclose(out);\n \t\tfclose(in);\n@@ -826,8 +828,7 @@ static int stgit_patch_to_mail(FILE *out, FILE *in, int keep_cr)\n *\n * Returns 0 on success, -1 on failure.\n */\n-static int split_mail_stgit_series(struct am_state *state, const char **paths,\n-\t\t\t\t\tint keep_cr)\n+static int split_mail_stgit_series(struct am_state *state, const char **paths)\n {\n \tconst char *series_dir;\n \tchar *series_dir_buf;\n@@ -857,7 +858,7 @@ static int split_mail_stgit_series(struct am_state *state, const char **paths,\n \tstrbuf_release(&sb);\n \tfree(series_dir_buf);\n \n-\tret = split_mail_conv(stgit_patch_to_mail, state, patches.argv, keep_cr);\n+\tret = split_mail_conv(stgit_patch_to_mail, state, patches.argv);\n \n \targv_array_clear(&patches);\n \treturn ret;\n@@ -937,30 +938,27 @@ static int hg_patch_to_mail(FILE *out, FILE *in, int keep_cr)\n * state->cur will be set to the index of the first mail, and state->last will\n * be set to the index of the last mail.\n *\n- * Set keep_cr to 0 to convert all lines ending with \\r\\n to end with \\n, 1\n- * to disable this behavior, -1 to use the default configured setting.\n- *\n * Returns 0 on success, -1 on failure.\n */\n static int split_mail(struct am_state *state, enum patch_format patch_format,\n-\t\t\tconst char **paths, int keep_cr)\n+\t\t\tconst char **paths)\n {\n-\tif (keep_cr < 0) {\n-\t\tkeep_cr = 0;\n-\t\tgit_config_get_bool(\"am.keepcr\", &keep_cr);\n+\tif (state->keep_cr < 0) {\n+\t\tstate->keep_cr = 0;\n+\t\tgit_config_get_bool(\"am.keepcr\", &state->keep_cr);\n \t}\n \n \tswitch (patch_format) {\n \tcase PATCH_FORMAT_MBOX:\n-\t\treturn split_mail_mbox(state, paths, keep_cr, 0);\n+\t\treturn split_mail_mbox(state, paths, 0);\n \tcase PATCH_FORMAT_STGIT:\n-\t\treturn split_mail_conv(stgit_patch_to_mail, state, paths, keep_cr);\n+\t\treturn split_mail_conv(stgit_patch_to_mail, state, paths);\n \tcase PATCH_FORMAT_STGIT_SERIES:\n-\t\treturn split_mail_stgit_series(state, paths, keep_cr);\n+\t\treturn split_mail_stgit_series(state, paths);\n \tcase PATCH_FORMAT_HG:\n-\t\treturn split_mail_conv(hg_patch_to_mail, state, paths, keep_cr);\n+\t\treturn split_mail_conv(hg_patch_to_mail, state, paths);\n \tcase PATCH_FORMAT_MBOXRD:\n-\t\treturn split_mail_mbox(state, paths, keep_cr, 1);\n+\t\treturn split_mail_mbox(state, paths, 1);\n \tdefault:\n \t\tdie(\"BUG: invalid patch_format\");\n \t}\n@@ -971,7 +969,7 @@ static int split_mail(struct am_state *state, enum patch_format patch_format,\n * Setup a new am session for applying patches\n */\n static void am_setup(struct am_state *state, enum patch_format patch_format,\n-\t\t\tconst char **paths, int keep_cr)\n+\t\t\tconst char **paths)\n {\n \tstruct object_id curr_head;\n \tconst char *str;\n@@ -988,7 +986,7 @@ static void am_setup(struct am_state *state, enum patch_format patch_format,\n \tif (mkdir(state->dir, 0777) < 0 && errno != EEXIST)\n \t\tdie_errno(_(\"failed to create directory '%s'\"), state->dir);\n \n-\tif (split_mail(state, patch_format, paths, keep_cr) < 0) {\n+\tif (split_mail(state, patch_format, paths) < 0) {\n \t\tam_destroy(state);\n \t\tdie(_(\"Failed to split patches.\"));\n \t}\n@@ -1276,6 +1274,8 @@ static int parse_mail(struct am_state *state, const char *mail)\n \t\tdie(\"BUG: invalid value for state->scissors\");\n \t}\n \n+\tmi.keep_cr = state->keep_cr;\n+\n \tmi.input = fopen(mail, \"r\");\n \tif (!mi.input)\n \t\tdie(\"could not open input\");\n@@ -2224,7 +2224,6 @@ int cmd_am(int argc, const char **argv, const char *prefix)\n {\n \tstruct am_state state;\n \tint binary = -1;\n-\tint keep_cr = -1;\n \tint patch_format = PATCH_FORMAT_UNKNOWN;\n \tenum resume_mode resume = RESUME_FALSE;\n \tint in_progress;\n@@ -2254,10 +2253,10 @@ int cmd_am(int argc, const char **argv, const char *prefix)\n \t\t\tN_(\"pass -b flag to git-mailinfo\"), KEEP_NON_PATCH),\n \t\tOPT_BOOL('m', \"message-id\", &state.message_id,\n \t\t\tN_(\"pass -m flag to git-mailinfo\")),\n-\t\t{ OPTION_SET_INT, 0, \"keep-cr\", &keep_cr, NULL,\n+\t\t{ OPTION_SET_INT, 0, \"keep-cr\", &state.keep_cr, NULL,\n \t\t N_(\"pass --keep-cr flag to git-mailsplit for mbox format\"),\n \t\t PARSE_OPT_NOARG | PARSE_OPT_NONEG, NULL, 1},\n-\t\t{ OPTION_SET_INT, 0, \"no-keep-cr\", &keep_cr, NULL,\n+\t\t{ OPTION_SET_INT, 0, \"no-keep-cr\", &state.keep_cr, NULL,\n \t\t N_(\"do not pass --keep-cr flag to git-mailsplit independent of am.keepcr\"),\n \t\t PARSE_OPT_NOARG | PARSE_OPT_NONEG, NULL, 0},\n \t\tOPT_BOOL('c', \"scissors\", &state.scissors,\n@@ -2392,7 +2391,7 @@ int cmd_am(int argc, const char **argv, const char *prefix)\n \t\t\t\targv_array_push(&paths, mkpath(\"%s/%s\", prefix, argv[i]));\n \t\t}\n \n-\t\tam_setup(&state, patch_format, paths.argv, keep_cr);\n+\t\tam_setup(&state, patch_format, paths.argv);\n \n \t\targv_array_clear(&paths);\n \t}\ndiff --git a/mailinfo.c b/mailinfo.c\nindex a489d9d0f..2059704a8 100644\n--- a/mailinfo.c\n+++ b/mailinfo.c\n@@ -812,6 +812,12 @@ static void handle_patch(struct mailinfo *mi, const struct strbuf *line)\n \n static void handle_filter(struct mailinfo *mi, struct strbuf *line)\n {\n+\tif (!mi->keep_cr && line->len > 1 &&\n+\t\t\tline->buf[line->len - 1] == '\\n' &&\n+\t\t\tline->buf[line->len - 2] == '\\r') {\n+\t\tstrbuf_setlen(line, line->len - 2);\n+\t\tstrbuf_addch(line, '\\n');\n+\t}\n \tswitch (mi->filter_stage) {\n \tcase 0:\n \t\tif (!handle_commit_msg(mi, line))\ndiff --git a/mailinfo.h b/mailinfo.h\nindex 04a25351d..9fddcf684 100644\n--- a/mailinfo.h\n+++ b/mailinfo.h\n@@ -12,6 +12,7 @@ struct mailinfo {\n \tstruct strbuf email;\n \tint keep_subject;\n \tint keep_non_patch_brackets_in_subject;\n+\tint keep_cr;\n \tint add_message_id;\n \tint use_scissors;\n \tint use_inbody_headers;\n-- \n2.11.0\n\n"},{"id":"309258","messageId":"1484212824-14108-2-git-send-email-mawilcox@linuxonhyperv.com","threadId":"44859","inReplyTo":"1484212824-14108-1-git-send-email-mawilcox@linuxonhyperv.com","subject":"[PATCH 2/2] mailinfo: Understand forwarded patches","fromName":"Matthew Wilcox","fromEmail":"mawilcox@linuxonhyperv.com","sentAt":"2017-01-12T09:20:24Z","receivedAt":"2017-01-12T07:23:23Z","isPatch":true,"sender":{"key":"mawilcox@linuxonhyperv.com","avatar":null},"body":"From: Matthew Wilcox \n\nExtend the --scissors mechanism to strip off the preamble created by\nforwarding a patch. There are a couple of extra headers (\"Sent\" and\n\"To\") added by forwarding, but other than that, the --scissors option\nwill now remove patches forwarded from Microsoft Outlook to a Linux\nemail account.\n\nSigned-off-by: Matthew Wilcox \n---\n mailinfo.c | 9 ++++++++-\n 1 file changed, 8 insertions(+), 1 deletion(-)\n\ndiff --git a/mailinfo.c b/mailinfo.c\nindex 2059704a8..fc1275532 100644\n--- a/mailinfo.c\n+++ b/mailinfo.c\n@@ -332,7 +332,7 @@ static void cleanup_subject(struct mailinfo *mi, struct strbuf *subject)\n \n #define MAX_HDR_PARSED 10\n static const char *header[MAX_HDR_PARSED] = {\n-\t\"From\",\"Subject\",\"Date\",\n+\t\"From\",\"Subject\",\"Date\",\"Sent\",\"To\",\n };\n \n static inline int cmp_header(const struct strbuf *line, const char *hdr)\n@@ -685,6 +685,13 @@ static int is_scissors_line(const char *line)\n \t\t\tc++;\n \t\t\tcontinue;\n \t\t}\n+\t\tif (!memcmp(c, \"Original Message\", 16)) {\n+\t\t\tin_perforation = 1;\n+\t\t\tperforation += 16;\n+\t\t\tscissors += 16;\n+\t\t\tc += 15;\n+\t\t\tcontinue;\n+\t\t}\n \t\tin_perforation = 0;\n \t}\n \n-- \n2.11.0\n\n"},{"id":"309276","messageId":"8f2be1e9-9199-44af-9d57-41763cb4d666@google.com","threadId":"44859","inReplyTo":"1484212824-14108-1-git-send-email-mawilcox@linuxonhyperv.com","subject":"Re: [PATCH 1/2] mailinfo: Add support for keep_cr","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2017-01-12T18:04:36Z","receivedAt":"2017-01-12T18:05:06Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"On 01/12/2017 01:20 AM, Matthew Wilcox wrote:\n> From: Matthew Wilcox \n>\n> If you have a base-64 encoded patch with CRLF endings (as produced\n> by forwarding a patch from Outlook to a Linux machine, for example),\n> the keep_cr setting is not honoured because keep_cr is only passed\n> to mailsplit, which does not look through the encoding. The keep_cr\n> logic needs to be applied after the base64 decode. I copied that\n> logic to handle_filter(), and rather than add a new keep_cr parameter\n> to handle_filter, I opted to add keep_cr to struct mailinfo; it seemed\n> appropriate given use_scissors was already there.\n>\n> Then I needed to initialise keep_cr in the struct mailinfo passed from\n> git-am, and rather than thread a 'keep_cr' parameter all the way through\n> to parse_mail(), I decided to add keep_cr to struct am_state, which let\n> it be removed as a parameter from five other functions.\n>\n> Signed-off-by: Matthew Wilcox \n\nA test exercising the new functionality would be nice.\n\nAlso, maybe a more descriptive title like \"mailinfo: also respect \nkeep_cr after base64 decode\" (50 characters) is better.\n\n> @@ -143,6 +144,7 @@ static void am_state_init(struct am_state *state, const char *dir)\n>\n> \tmemset(state, 0, sizeof(*state));\n>\n> +\tstate->keep_cr = -1;\n\nMaybe query the git config here (instead of later) so that we never have \nto worry about state->keep_cr being neither 0 nor 1? This function \nalready queries the git config anyway.\n\n> diff --git a/mailinfo.h b/mailinfo.h\n> index 04a25351d..9fddcf684 100644\n> --- a/mailinfo.h\n> +++ b/mailinfo.h\n> @@ -12,6 +12,7 @@ struct mailinfo {\n> \tstruct strbuf email;\n> \tint keep_subject;\n> \tint keep_non_patch_brackets_in_subject;\n> +\tint keep_cr;\n> \tint add_message_id;\n> \tint use_scissors;\n> \tint use_inbody_headers;\n\nI personally would write \"unsigned keep_cr : 1\" to further emphasize \nthat this can only be 0 or 1, but I don't know if it's better to keep \nwith the style existing in the file (that is, using int).\n"},{"id":"309277","messageId":"866bff14-ea25-c644-b8d2-1529f31e6461@google.com","threadId":"44859","inReplyTo":"1484212824-14108-2-git-send-email-mawilcox@linuxonhyperv.com","subject":"Re: [PATCH 2/2] mailinfo: Understand forwarded patches","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2017-01-12T18:17:50Z","receivedAt":"2017-01-12T18:18:55Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"On 01/12/2017 01:20 AM, Matthew Wilcox wrote:\n> From: Matthew Wilcox \n>\n> Extend the --scissors mechanism to strip off the preamble created by\n> forwarding a patch. There are a couple of extra headers (\"Sent\" and\n> \"To\") added by forwarding, but other than that, the --scissors option\n> will now remove patches forwarded from Microsoft Outlook to a Linux\n> email account.\n>\n> Signed-off-by: Matthew Wilcox \n\nAlso add a test showing the kind of message that the current code \ndoesn't handle, and that this commit addresses.\n\n> ---\n> mailinfo.c | 9 ++++++++-\n> 1 file changed, 8 insertions(+), 1 deletion(-)\n>\n> diff --git a/mailinfo.c b/mailinfo.c\n> index 2059704a8..fc1275532 100644\n> --- a/mailinfo.c\n> +++ b/mailinfo.c\n> @@ -332,7 +332,7 @@ static void cleanup_subject(struct mailinfo *mi, struct strbuf *subject)\n>\n> #define MAX_HDR_PARSED 10\n> static const char *header[MAX_HDR_PARSED] = {\n> -\t\"From\",\"Subject\",\"Date\",\n> +\t\"From\",\"Subject\",\"Date\",\"Sent\",\"To\",\n\nAre these extra headers used in both the \"real\" e-mail headers and the \nin-body headers, or only one of them? (If the latter, they should \nprobably be handled only in the relevant function - my previous patches \nto this file were in that direction too, if I remember correctly.) Also, \nI suspect that these will have to be handled differently to the other 3, \nbut that will be clearer when you add the test with an example message.\n\n> };\n>\n> static inline int cmp_header(const struct strbuf *line, const char *hdr)\n> @@ -685,6 +685,13 @@ static int is_scissors_line(const char *line)\n> \t\t\tc++;\n> \t\t\tcontinue;\n> \t\t}\n> +\t\tif (!memcmp(c, \"Original Message\", 16)) {\n\n1) You can use starts_with or skip_prefix.\n2) This seems vulnerable to false positives. If \"Original Message\" \nalways follows a certain kind of line, it might be better to check for \nthat. (Again, it will be clearer when we have an example message.)\n\n> +\t\t\tin_perforation = 1;\n> +\t\t\tperforation += 16;\n> +\t\t\tscissors += 16;\n> +\t\t\tc += 15;\n\nWhy 15? Also, can skip_prefix avoid these magic numbers?\n\n> +\t\t\tcontinue;\n> +\t\t}\n> \t\tin_perforation = 0;\n> \t}\n>\n>\n"},{"id":"309283","messageId":"BY2PR21MB0036E89DDFC5EFC8F17CD77DCB790@BY2PR21MB0036.namprd21.prod.outlook.com","threadId":"44859","inReplyTo":"8f2be1e9-9199-44af-9d57-41763cb4d666@google.com","subject":"RE: [PATCH 1/2] mailinfo: Add support for keep_cr","fromName":"Matthew Wilcox","fromEmail":"mawilcox@microsoft.com","sentAt":"2017-01-12T19:06:27Z","receivedAt":"2017-01-12T19:06:34Z","isPatch":true,"sender":{"key":"mawilcox@microsoft.com","avatar":null},"body":"From: Jonathan Tan [mailto:jonathantanmy@google.com]\n> On 01/12/2017 01:20 AM, Matthew Wilcox wrote:\n> A test exercising the new functionality would be nice.\n\nRoger.\n\n> Also, maybe a more descriptive title like \"mailinfo: also respect\n> keep_cr after base64 decode\" (50 characters) is better.\n\nNo problem.\n\n> > @@ -143,6 +144,7 @@ static void am_state_init(struct am_state *state,\n> const char *dir)\n> >\n> > \tmemset(state, 0, sizeof(*state));\n> >\n> > +\tstate->keep_cr = -1;\n> \n> Maybe query the git config here (instead of later) so that we never have\n> to worry about state->keep_cr being neither 0 nor 1? This function\n> already queries the git config anyway.\n\nI wondered why the existing code didn't do that, but I wanted to make a minimal change rather than clean up an older mistake. I'm happy to do it that way.\n\n> > diff --git a/mailinfo.h b/mailinfo.h\n> > index 04a25351d..9fddcf684 100644\n> > --- a/mailinfo.h\n> > +++ b/mailinfo.h\n> > @@ -12,6 +12,7 @@ struct mailinfo {\n> > \tstruct strbuf email;\n> > \tint keep_subject;\n> > \tint keep_non_patch_brackets_in_subject;\n> > +\tint keep_cr;\n> > \tint add_message_id;\n> > \tint use_scissors;\n> > \tint use_inbody_headers;\n> \n> I personally would write \"unsigned keep_cr : 1\" to further emphasize\n> that this can only be 0 or 1, but I don't know if it's better to keep\n> with the style existing in the file (that is, using int).\n\nProbably best to stick to the existing file ... someone can always do a cleanup patch later, and having this match the others will make that easier, not harder.\n\nThanks for the review.\n\n"},{"id":"309285","messageId":"BY2PR21MB00361F50AA0ABEA01F5630EFCB790@BY2PR21MB0036.namprd21.prod.outlook.com","threadId":"44859","inReplyTo":"866bff14-ea25-c644-b8d2-1529f31e6461@google.com","subject":"RE: [PATCH 2/2] mailinfo: Understand forwarded patches","fromName":"Matthew Wilcox","fromEmail":"mawilcox@microsoft.com","sentAt":"2017-01-12T19:35:10Z","receivedAt":"2017-01-12T19:35:40Z","isPatch":true,"sender":{"key":"mawilcox@microsoft.com","avatar":null},"body":"From: Jonathan Tan [mailto:jonathantanmy@google.com]\n> On 01/12/2017 01:20 AM, Matthew Wilcox wrote:\n> > From: Matthew Wilcox \n> >\n> > Extend the --scissors mechanism to strip off the preamble created by\n> > forwarding a patch. There are a couple of extra headers (\"Sent\" and\n> > \"To\") added by forwarding, but other than that, the --scissors option\n> > will now remove patches forwarded from Microsoft Outlook to a Linux\n> > email account.\n> >\n> > Signed-off-by: Matthew Wilcox \n> \n> Also add a test showing the kind of message that the current code\n> doesn't handle, and that this commit addresses.\n\nOK. For the sake of discussion, here's what the base64-decoded message looks like:\n\n--- 8< ---\n\n-----Original Message-----\nFrom: Rehas Sachdeva [mailto:aquannie@gmail.com]\nSent: Wednesday, January 4, 2017 11:55 AM\nTo: Matthew Wilcox ; riel@surriel.com\nSubject: [PATCH v3] radix tree test suite: Dial down verbosity with -v\n\nMake the output of radix tree test suite less verbose by default and add\n-v and -vv command line options for increasing level of verbosity.\n\n--- >8 ---\n\n> > ---\n> > mailinfo.c | 9 ++++++++-\n> > 1 file changed, 8 insertions(+), 1 deletion(-)\n> >\n> > diff --git a/mailinfo.c b/mailinfo.c\n> > index 2059704a8..fc1275532 100644\n> > --- a/mailinfo.c\n> > +++ b/mailinfo.c\n> > @@ -332,7 +332,7 @@ static void cleanup_subject(struct mailinfo *mi,\n> struct strbuf *subject)\n> >\n> > #define MAX_HDR_PARSED 10\n> > static const char *header[MAX_HDR_PARSED] = {\n> > -\t\"From\",\"Subject\",\"Date\",\n> > +\t\"From\",\"Subject\",\"Date\",\"Sent\",\"To\",\n> \n> Are these extra headers used in both the \"real\" e-mail headers and the\n> in-body headers, or only one of them? (If the latter, they should\n> probably be handled only in the relevant function - my previous patches\n> to this file were in that direction too, if I remember correctly.) Also,\n> I suspect that these will have to be handled differently to the other 3,\n> but that will be clearer when you add the test with an example message.\n\nWithout this part of the patch, using --scissors means it stops parsing headers when it hits the 'Sent' line. So all I'm looking for is to have 'is_inbody_header()' return true. If there's a better way to achieve that, I'm all for it. Without this hunk of the patch, the commit looks like this:\n\nWithout any of this patch, the commit looks like this:\n\nAuthor: Matthew Wilcox \nDate: Sat Jan 7 18:33:43 2017 +0000\n\n FW: [PATCH v3] radix tree test suite: Dial down verbosity with -v\n\n -----Original Message-----\n From: Rehas Sachdeva [mailto:aquannie@gmail.com]\n Sent: Wednesday, January 4, 2017 11:55 AM\n To: Matthew Wilcox ; riel@surriel.com\n Subject: [PATCH v3] radix tree test suite: Dial down verbosity with -v\n\n Make the output of radix tree test suite less verbose by default and add\n -v and -vv command line options for increasing level of verbosity.\n\nWithout this hunk of the patch, the commit looks like this:\n\nAuthor: Rehas Sachdeva <[mailto:aquannie@gmail.com]>\nDate: Sat Jan 7 18:33:43 2017 +0000\n\n FW: [PATCH v3] radix tree test suite: Dial down verbosity with -v\n\n Sent: Wednesday, January 4, 2017 11:55 AM\n To: Matthew Wilcox ; riel@surriel.com\n Subject: [PATCH v3] radix tree test suite: Dial down verbosity with -v\n\n Make the output of radix tree test suite less verbose by default and add\n -v and -vv command line options for increasing level of verbosity.\n\nAnd with this hunk, it turns into this:\n\nAuthor: Rehas Sachdeva <[mailto:aquannie@gmail.com]>\nDate: Sat Jan 7 18:33:43 2017 +0000\n\n radix tree test suite: Dial down verbosity with -v\n\n Make the output of radix tree test suite less verbose by default and add\n -v and -vv command line options for increasing level of verbosity.\n\n\nThere's more work to do here, turning that silly <[mailto:]> into a proper email address, for example, and parsing Sent: like we parse Date:, but I thought it worth sending out patches 1 & 2 before writing patch 3.\n\n> > @@ -685,6 +685,13 @@ static int is_scissors_line(const char *line)\n> > \t\t\tc++;\n> > \t\t\tcontinue;\n> > \t\t}\n> > +\t\tif (!memcmp(c, \"Original Message\", 16)) {\n> \n> 1) You can use starts_with or skip_prefix.\n\nI was following the existing logic in this function that looks for 8< or >8 or ...\n\n> 2) This seems vulnerable to false positives. If \"Original Message\"\n> always follows a certain kind of line, it might be better to check for\n> that. (Again, it will be clearer when we have an example message.)\n\nI don't think it's terribly vulnerable to false positives. The logic at the end of the function tries to make sure that the scissor marks make up a substantial component of the line.\n\nWe could do this differently; I'm not sure if there's a regex library built into git, but we could even custom-write a separate matcher that looks only for ^-{3,}Original Message-{3,}$\n\nOr we could use a different option; eg --forwarded that will trim off the extra gunk associated with forwarding messages, instead of overloading --scissors.\n\n> > +\t\t\tin_perforation = 1;\n> > +\t\t\tperforation += 16;\n> > +\t\t\tscissors += 16;\n> > +\t\t\tc += 15;\n> \n> Why 15? Also, can skip_prefix avoid these magic numbers?\n\nAgain, following the rest of the function. c has already been advanced by 1, now it needs to be advanced to the end of the 16 character string \"Original Message\".\n\n"},{"id":"309286","messageId":"xmqqfukojalj.fsf@gitster.mtv.corp.google.com","threadId":"44859","inReplyTo":"1484212824-14108-2-git-send-email-mawilcox@linuxonhyperv.com","subject":"Re: [PATCH 2/2] mailinfo: Understand forwarded patches","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-01-12T20:18:32Z","receivedAt":"2017-01-12T20:18:48Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Matthew Wilcox writes:\n\n> From: Matthew Wilcox \n>\n> Extend the --scissors mechanism to strip off the preamble created by\n> forwarding a patch. There are a couple of extra headers (\"Sent\" and\n> \"To\") added by forwarding, but other than that, the --scissors option\n> will now remove patches forwarded from Microsoft Outlook to a Linux\n> email account.\n>\n> Signed-off-by: Matthew Wilcox \n> ---\n> mailinfo.c | 9 ++++++++-\n> 1 file changed, 8 insertions(+), 1 deletion(-)\n\nTo be quite honest, I am not enthused to even having to think about\nthis kind of change. There seems to be no standard way MUAs produce\n\"forwarded\" messages, and this adds support for only one specific\nMUA, that happens to say \"Original Message\". Why should such a thing\nhardcoded in this codepath?\n\nI think I am OK with a patch that lets users customize\nis_scissors_line(), perhaps accepting a regexp that ought to match a\nline to consider a scissors line. When such a regexp is given,\ncheck for a match before doing the \"do we have a line filled with\n'-' with the scissors marker and is the mark long enough?\" check.\n\nThen you can do\n\n\tmailinfo --scissors='^-{5}Original Message-{5}$'\n\nor something like that. Perhaps allow multiple regexps, or even\nallow them to come from a multi-valued configuration variable.\n\n> diff --git a/mailinfo.c b/mailinfo.c\n> index 2059704a8..fc1275532 100644\n> --- a/mailinfo.c\n> +++ b/mailinfo.c\n> @@ -332,7 +332,7 @@ static void cleanup_subject(struct mailinfo *mi, struct strbuf *subject)\n> \n> #define MAX_HDR_PARSED 10\n> static const char *header[MAX_HDR_PARSED] = {\n> -\t\"From\",\"Subject\",\"Date\",\n> +\t\"From\",\"Subject\",\"Date\",\"Sent\",\"To\",\n> };\n\nThis array lists fields whose value we _care_ about. Do not put\nrandom garbage whose value we do not use in it.\n\nEven though I am not enthused to see support for messages forwarded\nby Outlook bolted onto this codepath, I think it may make sense to\nallow random garbage that looks like an e-mail header to appear\nimmediately after a scissors line (and ignore them). For that,\nperhaps you would instead want to extend is_inbody_header() so that\nafter it decides that the given line is *NOT* one of the header\nfield we care about, return a value that is not 0 or 1. Its caller\ncurrently expects to see 1 if it is a relevant in-body header line,\nor 0 if the line signals the end of the in-body header block. You'd\nbe adding another class of lines that are not a header line with a\nmeaning but do not terminate the in-body header block.\n\n\n> static inline int cmp_header(const struct strbuf *line, const char *hdr)\n> @@ -685,6 +685,13 @@ static int is_scissors_line(const char *line)\n> \t\t\tc++;\n> \t\t\tcontinue;\n> \t\t}\n> +\t\tif (!memcmp(c, \"Original Message\", 16)) {\n> +\t\t\tin_perforation = 1;\n> +\t\t\tperforation += 16;\n> +\t\t\tscissors += 16;\n> +\t\t\tc += 15;\n> +\t\t\tcontinue;\n> +\t\t}\n> \t\tin_perforation = 0;\n> \t}\n"}]}