{"thread":{"id":"42909","subject":"[PATCH] format-patch: escape \"From \" lines recognized by mailsplit","startedAt":"2016-07-22T22:47:44Z","lastAt":"2016-07-25T20:49:13Z","messageCount":17,"participants":["Eric Wong","Johannes Schindelin","Junio C Hamano","Andreas Schwab","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"292022","messageId":"20160722224739.GA22961@whir","threadId":"42909","inReplyTo":null,"subject":"[PATCH] format-patch: escape \"From \" lines recognized by mailsplit","fromName":"Eric Wong","fromEmail":"e@80x24.org","sentAt":"2016-07-22T22:47:39Z","receivedAt":"2016-07-22T22:47:44Z","isPatch":true,"sender":{"key":"e@80x24.org","avatar":null},"body":"Users have mistakenly copied \"From \" lines into commit messages\nin the past, and will certainly make the same mistakes in the\nfuture. Since not everyone uses mboxrd, yet, we should at least\nprevent miss-split mails by always escaping \"From \" lines based\non the check used by mailsplit.\n\nmailsplit will not perform unescaping by default, yet, as it\ncould cause further invocations of format-patch from old\nversions of git to generate bad output. Propagating the mboxo\nescaping is preferable to miss-split patches. Unescaping may\nstill be performed via \"--mboxrd\".\n\nref: https://public-inbox.org/git/20160606230248.GA15906@dcvr.yhbt.net/T/#u\n\nSigned-off-by: Eric Wong \n---\n builtin/mailsplit.c | 32 +-------------------------------\n mailinfo.c | 31 +++++++++++++++++++++++++++++++\n mailinfo.h | 1 +\n pretty.c | 16 +++++++++++++---\n t/t4014-format-patch.sh | 14 ++++++++++++++\n 5 files changed, 60 insertions(+), 34 deletions(-)\n\ndiff --git a/builtin/mailsplit.c b/builtin/mailsplit.c\nindex 3068168..bb8f9c9 100644\n--- a/builtin/mailsplit.c\n+++ b/builtin/mailsplit.c\n@@ -8,41 +8,11 @@\n #include \"builtin.h\"\n #include \"string-list.h\"\n #include \"strbuf.h\"\n+#include \"mailinfo.h\"\n \n static const char git_mailsplit_usage[] =\n \"git mailsplit [-d] [-f] [-b] [--keep-cr] -o [(|)...]\";\n \n-static int is_from_line(const char *line, int len)\n-{\n-\tconst char *colon;\n-\n-\tif (len < 20 || memcmp(\"From \", line, 5))\n-\t\treturn 0;\n-\n-\tcolon = line + len - 2;\n-\tline += 5;\n-\tfor (;;) {\n-\t\tif (colon < line)\n-\t\t\treturn 0;\n-\t\tif (*--colon == ':')\n-\t\t\tbreak;\n-\t}\n-\n-\tif (!isdigit(colon[-4]) ||\n-\t !isdigit(colon[-2]) ||\n-\t !isdigit(colon[-1]) ||\n-\t !isdigit(colon[ 1]) ||\n-\t !isdigit(colon[ 2]))\n-\t\treturn 0;\n-\n-\t/* year */\n-\tif (strtol(colon+3, NULL, 10) <= 90)\n-\t\treturn 0;\n-\n-\t/* Ok, close enough */\n-\treturn 1;\n-}\n-\n static struct strbuf buf = STRBUF_INIT;\n static int keep_cr;\n static int mboxrd;\ndiff --git a/mailinfo.c b/mailinfo.c\nindex 9f19ca1..0ebd953 100644\n--- a/mailinfo.c\n+++ b/mailinfo.c\n@@ -1035,3 +1035,34 @@ void clear_mailinfo(struct mailinfo *mi)\n \n \tstrbuf_release(&mi->log_message);\n }\n+\n+int is_from_line(const char *line, int len)\n+{\n+\tconst char *colon;\n+\n+\tif (len < 20 || memcmp(\"From \", line, 5))\n+\t\treturn 0;\n+\n+\tcolon = line + len - 2;\n+\tline += 5;\n+\tfor (;;) {\n+\t\tif (colon < line)\n+\t\t\treturn 0;\n+\t\tif (*--colon == ':')\n+\t\t\tbreak;\n+\t}\n+\n+\tif (!isdigit(colon[-4]) ||\n+\t !isdigit(colon[-2]) ||\n+\t !isdigit(colon[-1]) ||\n+\t !isdigit(colon[ 1]) ||\n+\t !isdigit(colon[ 2]))\n+\t\treturn 0;\n+\n+\t/* year */\n+\tif (strtol(colon+3, NULL, 10) <= 90)\n+\t\treturn 0;\n+\n+\t/* Ok, close enough */\n+\treturn 1;\n+}\ndiff --git a/mailinfo.h b/mailinfo.h\nindex 93776a7..c1430a0 100644\n--- a/mailinfo.h\n+++ b/mailinfo.h\n@@ -37,5 +37,6 @@ struct mailinfo {\n extern void setup_mailinfo(struct mailinfo *);\n extern int mailinfo(struct mailinfo *, const char *msg, const char *patch);\n extern void clear_mailinfo(struct mailinfo *);\n+int is_from_line(const char *line, int len);\n \n #endif /* MAILINFO_H */\ndiff --git a/pretty.c b/pretty.c\nindex 9fa42c2..898c0a3 100644\n--- a/pretty.c\n+++ b/pretty.c\n@@ -10,6 +10,7 @@\n #include \"color.h\"\n #include \"reflog-walk.h\"\n #include \"gpg-interface.h\"\n+#include \"mailinfo.h\"\n \n static char *user_format;\n static struct cmt_fmt_map {\n@@ -1745,9 +1746,18 @@ void pp_remainder(struct pretty_print_context *pp,\n \t\t\tstrbuf_add_tabexpand(sb, pp->expand_tabs_in_log,\n \t\t\t\t\t line, linelen);\n \t\telse {\n-\t\t\tif (pp->fmt == CMIT_FMT_MBOXRD &&\n-\t\t\t\t\tis_mboxrd_from(line, linelen))\n-\t\t\t\tstrbuf_addch(sb, '>');\n+\t\t\tswitch (pp->fmt) {\n+\t\t\tcase CMIT_FMT_EMAIL:\n+\t\t\t\tif (is_from_line(line, linelen))\n+\t\t\t\t\tstrbuf_addch(sb, '>');\n+\t\t\t\tbreak;\n+\t\t\tcase CMIT_FMT_MBOXRD:\n+\t\t\t\tif (is_mboxrd_from(line, linelen))\n+\t\t\t\t\tstrbuf_addch(sb, '>');\n+\t\t\t\tbreak;\n+\t\t\tdefault:\n+\t\t\t\tbreak;\n+\t\t\t}\n \n \t\t\tstrbuf_add(sb, line, linelen);\n \t\t}\ndiff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh\nindex 1206c48..8fa3982 100755\n--- a/t/t4014-format-patch.sh\n+++ b/t/t4014-format-patch.sh\n@@ -1606,4 +1606,18 @@ test_expect_success 'format-patch --pretty=mboxrd' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'format-patch From escaping' '\n+\tcat >msg <<-INPUT_END &&\n+\tsomebody pasted format-patch output into a body\n+\n+\tFrom 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001\n+\tINPUT_END\n+\n+\tC=$(git commit-tree HEAD^^{tree} -p HEAD patch &&\n+\tgit grep -h --no-index \\\n+\t\t\">From 0000000000000000000000000000000000000000 \" \\\n+\t\tpatch\n+'\n+\n test_done\n-- \nEW\n"},{"id":"292035","messageId":"alpine.DEB.2.20.1607231056150.14111@virtualbox","threadId":"42909","inReplyTo":"20160722224739.GA22961@whir","subject":"Re: [PATCH] format-patch: escape \"From \" lines recognized by mailsplit","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-07-23T08:59:49Z","receivedAt":"2016-07-23T09:00:44Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Eric,\n\nI think that this change:\n\nOn Fri, 22 Jul 2016, Eric Wong wrote:\n\n> diff --git a/builtin/mailsplit.c b/builtin/mailsplit.c\n> index 3068168..bb8f9c9 100644\n> --- a/builtin/mailsplit.c\n> +++ b/builtin/mailsplit.c\n> @@ -8,41 +8,11 @@\n> #include \"builtin.h\"\n> #include \"string-list.h\"\n> #include \"strbuf.h\"\n> +#include \"mailinfo.h\"\n> \n> static const char git_mailsplit_usage[] =\n> \"git mailsplit [-d] [-f] [-b] [--keep-cr] -o [(|)...]\";\n> \n> -static int is_from_line(const char *line, int len)\n> -{\n> -\tconst char *colon;\n> -\n> -\tif (len < 20 || memcmp(\"From \", line, 5))\n> -\t\treturn 0;\n> -\n> -\tcolon = line + len - 2;\n> -\tline += 5;\n> -\tfor (;;) {\n> -\t\tif (colon < line)\n> -\t\t\treturn 0;\n> -\t\tif (*--colon == ':')\n> -\t\t\tbreak;\n> -\t}\n> -\n> -\tif (!isdigit(colon[-4]) ||\n> -\t !isdigit(colon[-2]) ||\n> -\t !isdigit(colon[-1]) ||\n> -\t !isdigit(colon[ 1]) ||\n> -\t !isdigit(colon[ 2]))\n> -\t\treturn 0;\n> -\n> -\t/* year */\n> -\tif (strtol(colon+3, NULL, 10) <= 90)\n> -\t\treturn 0;\n> -\n> -\t/* Ok, close enough */\n> -\treturn 1;\n> -}\n> -\n> static struct strbuf buf = STRBUF_INIT;\n> static int keep_cr;\n> static int mboxrd;\n> diff --git a/mailinfo.c b/mailinfo.c\n> index 9f19ca1..0ebd953 100644\n> --- a/mailinfo.c\n> +++ b/mailinfo.c\n> @@ -1035,3 +1035,34 @@ void clear_mailinfo(struct mailinfo *mi)\n> \n> \tstrbuf_release(&mi->log_message);\n> }\n> +\n> +int is_from_line(const char *line, int len)\n> +{\n> +\tconst char *colon;\n> +\n> +\tif (len < 20 || memcmp(\"From \", line, 5))\n> +\t\treturn 0;\n> +\n> +\tcolon = line + len - 2;\n> +\tline += 5;\n> +\tfor (;;) {\n> +\t\tif (colon < line)\n> +\t\t\treturn 0;\n> +\t\tif (*--colon == ':')\n> +\t\t\tbreak;\n> +\t}\n> +\n> +\tif (!isdigit(colon[-4]) ||\n> +\t !isdigit(colon[-2]) ||\n> +\t !isdigit(colon[-1]) ||\n> +\t !isdigit(colon[ 1]) ||\n> +\t !isdigit(colon[ 2]))\n> +\t\treturn 0;\n> +\n> +\t/* year */\n> +\tif (strtol(colon+3, NULL, 10) <= 90)\n> +\t\treturn 0;\n> +\n> +\t/* Ok, close enough */\n> +\treturn 1;\n> +}\n> diff --git a/mailinfo.h b/mailinfo.h\n> index 93776a7..c1430a0 100644\n> --- a/mailinfo.h\n> +++ b/mailinfo.h\n> @@ -37,5 +37,6 @@ struct mailinfo {\n> extern void setup_mailinfo(struct mailinfo *);\n> extern int mailinfo(struct mailinfo *, const char *msg, const char *patch);\n> extern void clear_mailinfo(struct mailinfo *);\n> +int is_from_line(const char *line, int len);\n> \n> #endif /* MAILINFO_H */\n\ndeserves to live in a separate patch... It would make the real change\nstick out better.\n\nCiao,\nDscho\n"},{"id":"292042","messageId":"20160724031409.GA32480@starla","threadId":"42909","inReplyTo":"alpine.DEB.2.20.1607231056150.14111@virtualbox","subject":"Re: [PATCH] format-patch: escape \"From \" lines recognized by mailsplit","fromName":"Eric Wong","fromEmail":"e@80x24.org","sentAt":"2016-07-24T03:14:09Z","receivedAt":"2016-07-24T03:14:14Z","isPatch":true,"sender":{"key":"e@80x24.org","avatar":null},"body":"Johannes Schindelin wrote:\n> I think that this change:\n\n\n\n> deserves to live in a separate patch... It would make the real change\n> stick out better.\n\nGood point, I actually wrote a looser check based on\nis_mboxrd_from before realizing the stricter check from\nmailsplit would probably be more appropriate to use by default.\n\n\nThe following changes since commit 08bb3500a2a718c3c78b0547c68601cafa7a8fd9:\n\n Sixth batch of topics for 2.10 (2016-07-19 13:26:16 -0700)\n\nare available in the git repository at:\n\n git://bogomips.org/git-svn.git mboxo\n\nfor you to fetch changes up to 4f769f1799db11f135948bf517f2d8d864fa778f:\n\n format-patch: escape \"From \" lines recognized by mailsplit (2016-07-23 21:38:40 +0000)\n\n----------------------------------------------------------------\nEric Wong (2):\n mailinfo: extract is_from_line from mailsplit\n format-patch: escape \"From \" lines recognized by mailsplit\n\n builtin/mailsplit.c | 32 +-------------------------------\n mailinfo.c | 31 +++++++++++++++++++++++++++++++\n mailinfo.h | 1 +\n pretty.c | 16 +++++++++++++---\n t/t4014-format-patch.sh | 14 ++++++++++++++\n 5 files changed, 60 insertions(+), 34 deletions(-)\n"},{"id":"292043","messageId":"20160724031518.GB32480@starla","threadId":"42909","inReplyTo":"20160724031409.GA32480@starla","subject":"[PATCH 1/2] mailinfo: extract is_from_line from mailsplit","fromName":"Eric Wong","fromEmail":"e@80x24.org","sentAt":"2016-07-24T03:15:18Z","receivedAt":"2016-07-24T03:15:23Z","isPatch":true,"sender":{"key":"e@80x24.org","avatar":null},"body":"We will be reusing is_from_line for quoting format-patch output\nin the next commit.\n\nSuggested-by: Johannes Schindelin \nSigned-off-by: Eric Wong \n---\n builtin/mailsplit.c | 32 +-------------------------------\n mailinfo.c | 31 +++++++++++++++++++++++++++++++\n mailinfo.h | 1 +\n 3 files changed, 33 insertions(+), 31 deletions(-)\n\ndiff --git a/builtin/mailsplit.c b/builtin/mailsplit.c\nindex 3068168..bb8f9c9 100644\n--- a/builtin/mailsplit.c\n+++ b/builtin/mailsplit.c\n@@ -8,41 +8,11 @@\n #include \"builtin.h\"\n #include \"string-list.h\"\n #include \"strbuf.h\"\n+#include \"mailinfo.h\"\n \n static const char git_mailsplit_usage[] =\n \"git mailsplit [-d] [-f] [-b] [--keep-cr] -o [(|)...]\";\n \n-static int is_from_line(const char *line, int len)\n-{\n-\tconst char *colon;\n-\n-\tif (len < 20 || memcmp(\"From \", line, 5))\n-\t\treturn 0;\n-\n-\tcolon = line + len - 2;\n-\tline += 5;\n-\tfor (;;) {\n-\t\tif (colon < line)\n-\t\t\treturn 0;\n-\t\tif (*--colon == ':')\n-\t\t\tbreak;\n-\t}\n-\n-\tif (!isdigit(colon[-4]) ||\n-\t !isdigit(colon[-2]) ||\n-\t !isdigit(colon[-1]) ||\n-\t !isdigit(colon[ 1]) ||\n-\t !isdigit(colon[ 2]))\n-\t\treturn 0;\n-\n-\t/* year */\n-\tif (strtol(colon+3, NULL, 10) <= 90)\n-\t\treturn 0;\n-\n-\t/* Ok, close enough */\n-\treturn 1;\n-}\n-\n static struct strbuf buf = STRBUF_INIT;\n static int keep_cr;\n static int mboxrd;\ndiff --git a/mailinfo.c b/mailinfo.c\nindex 9f19ca1..0ebd953 100644\n--- a/mailinfo.c\n+++ b/mailinfo.c\n@@ -1035,3 +1035,34 @@ void clear_mailinfo(struct mailinfo *mi)\n \n \tstrbuf_release(&mi->log_message);\n }\n+\n+int is_from_line(const char *line, int len)\n+{\n+\tconst char *colon;\n+\n+\tif (len < 20 || memcmp(\"From \", line, 5))\n+\t\treturn 0;\n+\n+\tcolon = line + len - 2;\n+\tline += 5;\n+\tfor (;;) {\n+\t\tif (colon < line)\n+\t\t\treturn 0;\n+\t\tif (*--colon == ':')\n+\t\t\tbreak;\n+\t}\n+\n+\tif (!isdigit(colon[-4]) ||\n+\t !isdigit(colon[-2]) ||\n+\t !isdigit(colon[-1]) ||\n+\t !isdigit(colon[ 1]) ||\n+\t !isdigit(colon[ 2]))\n+\t\treturn 0;\n+\n+\t/* year */\n+\tif (strtol(colon+3, NULL, 10) <= 90)\n+\t\treturn 0;\n+\n+\t/* Ok, close enough */\n+\treturn 1;\n+}\ndiff --git a/mailinfo.h b/mailinfo.h\nindex 93776a7..c1430a0 100644\n--- a/mailinfo.h\n+++ b/mailinfo.h\n@@ -37,5 +37,6 @@ struct mailinfo {\n extern void setup_mailinfo(struct mailinfo *);\n extern int mailinfo(struct mailinfo *, const char *msg, const char *patch);\n extern void clear_mailinfo(struct mailinfo *);\n+int is_from_line(const char *line, int len);\n \n #endif /* MAILINFO_H */\n-- \nEW\n"},{"id":"292044","messageId":"20160724031543.GC32480@starla","threadId":"42909","inReplyTo":"20160724031409.GA32480@starla","subject":"[PATCH 2/2] format-patch: escape \"From \" lines recognized by mailsplit","fromName":"Eric Wong","fromEmail":"e@80x24.org","sentAt":"2016-07-24T03:15:43Z","receivedAt":"2016-07-24T03:15:47Z","isPatch":true,"sender":{"key":"e@80x24.org","avatar":null},"body":"Users have mistakenly copied \"From \" lines into commit messages\nin the past, and will certainly make the same mistakes in the\nfuture. Since not everyone uses mboxrd, yet, we should at least\nprevent miss-split mails by always escaping \"From \" lines based\non the check used by mailsplit.\n\nmailsplit will not perform unescaping by default, yet, as it\ncould cause further invocations of format-patch from old\nversions of git to generate bad output. Propagating the mboxo\nescaping is preferable to miss-split patches. Unescaping may\nstill be performed via \"--mboxrd\".\n\nref: https://public-inbox.org/git/20160606230248.GA15906@dcvr.yhbt.net/T/#u\n\nSigned-off-by: Eric Wong \n---\n pretty.c | 16 +++++++++++++---\n t/t4014-format-patch.sh | 14 ++++++++++++++\n 2 files changed, 27 insertions(+), 3 deletions(-)\n\ndiff --git a/pretty.c b/pretty.c\nindex 9fa42c2..898c0a3 100644\n--- a/pretty.c\n+++ b/pretty.c\n@@ -10,6 +10,7 @@\n #include \"color.h\"\n #include \"reflog-walk.h\"\n #include \"gpg-interface.h\"\n+#include \"mailinfo.h\"\n \n static char *user_format;\n static struct cmt_fmt_map {\n@@ -1745,9 +1746,18 @@ void pp_remainder(struct pretty_print_context *pp,\n \t\t\tstrbuf_add_tabexpand(sb, pp->expand_tabs_in_log,\n \t\t\t\t\t line, linelen);\n \t\telse {\n-\t\t\tif (pp->fmt == CMIT_FMT_MBOXRD &&\n-\t\t\t\t\tis_mboxrd_from(line, linelen))\n-\t\t\t\tstrbuf_addch(sb, '>');\n+\t\t\tswitch (pp->fmt) {\n+\t\t\tcase CMIT_FMT_EMAIL:\n+\t\t\t\tif (is_from_line(line, linelen))\n+\t\t\t\t\tstrbuf_addch(sb, '>');\n+\t\t\t\tbreak;\n+\t\t\tcase CMIT_FMT_MBOXRD:\n+\t\t\t\tif (is_mboxrd_from(line, linelen))\n+\t\t\t\t\tstrbuf_addch(sb, '>');\n+\t\t\t\tbreak;\n+\t\t\tdefault:\n+\t\t\t\tbreak;\n+\t\t\t}\n \n \t\t\tstrbuf_add(sb, line, linelen);\n \t\t}\ndiff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh\nindex 1206c48..8fa3982 100755\n--- a/t/t4014-format-patch.sh\n+++ b/t/t4014-format-patch.sh\n@@ -1606,4 +1606,18 @@ test_expect_success 'format-patch --pretty=mboxrd' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'format-patch From escaping' '\n+\tcat >msg <<-INPUT_END &&\n+\tsomebody pasted format-patch output into a body\n+\n+\tFrom 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001\n+\tINPUT_END\n+\n+\tC=$(git commit-tree HEAD^^{tree} -p HEAD patch &&\n+\tgit grep -h --no-index \\\n+\t\t\">From 0000000000000000000000000000000000000000 \" \\\n+\t\tpatch\n+'\n+\n test_done\n-- \nEW\n"},{"id":"292046","messageId":"xmqqk2gb8q81.fsf@gitster.mtv.corp.google.com","threadId":"42909","inReplyTo":"20160722224739.GA22961@whir","subject":"Re: [PATCH] format-patch: escape \"From \" lines recognized by mailsplit","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-07-24T07:11:58Z","receivedAt":"2016-07-24T07:12:11Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Wong writes:\n\n> Users have mistakenly copied \"From \" lines into commit messages\n> in the past, and will certainly make the same mistakes in the\n> future. Since not everyone uses mboxrd, yet, we should at least\n> prevent miss-split mails by always escaping \"From \" lines based\n> on the check used by mailsplit.\n>\n> mailsplit will not perform unescaping by default, yet, as it\n> could cause further invocations of format-patch from old\n> versions of git to generate bad output. Propagating the mboxo\n> escaping is preferable to miss-split patches. Unescaping may\n> still be performed via \"--mboxrd\".\n\nAs a tool to produce mbox file, quoting like this in format-patch\noutput may make sense, I would think, but shouldn't send-email undo\nthis when sending individual patches?\n"},{"id":"292047","messageId":"m2poq3zdu3.fsf@linux-m68k.org","threadId":"42909","inReplyTo":"20160724031518.GB32480@starla","subject":"Re: [PATCH 1/2] mailinfo: extract is_from_line from mailsplit","fromName":"Andreas Schwab","fromEmail":"schwab@linux-m68k.org","sentAt":"2016-07-24T07:37:24Z","receivedAt":"2016-07-24T07:37:36Z","isPatch":true,"sender":{"key":"schwab@linux-m68k.org","avatar":"https://avatars.githubusercontent.com/u/2175493?v=4"},"body":"Eric Wong writes:\n\n> diff --git a/mailinfo.c b/mailinfo.c\n> index 9f19ca1..0ebd953 100644\n> --- a/mailinfo.c\n> +++ b/mailinfo.c\n> @@ -1035,3 +1035,34 @@ void clear_mailinfo(struct mailinfo *mi)\n> \n> \tstrbuf_release(&mi->log_message);\n> }\n> +\n> +int is_from_line(const char *line, int len)\n> +{\n> +\tconst char *colon;\n> +\n> +\tif (len < 20 || memcmp(\"From \", line, 5))\n> +\t\treturn 0;\n> +\n> +\tcolon = line + len - 2;\n> +\tline += 5;\n> +\tfor (;;) {\n> +\t\tif (colon < line)\n> +\t\t\treturn 0;\n> +\t\tif (*--colon == ':')\n> +\t\t\tbreak;\n> +\t}\n> +\n> +\tif (!isdigit(colon[-4]) ||\n> +\t !isdigit(colon[-2]) ||\n> +\t !isdigit(colon[-1]) ||\n> +\t !isdigit(colon[ 1]) ||\n> +\t !isdigit(colon[ 2]))\n> +\t\treturn 0;\n> +\n> +\t/* year */\n> +\tif (strtol(colon+3, NULL, 10) <= 90)\n> +\t\treturn 0;\n> +\n> +\t/* Ok, close enough */\n> +\treturn 1;\n> +}\n\nShould this be made more strict, like by checking for a space before the\nyear?\n\nAndreas.\n\n-- \nAndreas Schwab, schwab@linux-m68k.org\nGPG Key fingerprint = 58CA 54C7 6D53 942B 1756 01D3 44D5 214B 8276 4ED5\n\"And now for something completely different.\"\n"},{"id":"292048","messageId":"alpine.DEB.2.20.1607240926500.14111@virtualbox","threadId":"42909","inReplyTo":"20160724031543.GC32480@starla","subject":"Re: [PATCH 2/2] format-patch: escape \"From \" lines recognized by mailsplit","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-07-24T07:37:57Z","receivedAt":"2016-07-24T07:38:16Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Eric,\n\nOn Sun, 24 Jul 2016, Eric Wong wrote:\n\n> @@ -1745,9 +1746,18 @@ void pp_remainder(struct pretty_print_context *pp,\n> \t\t\tstrbuf_add_tabexpand(sb, pp->expand_tabs_in_log,\n> \t\t\t\t\t line, linelen);\n> \t\telse {\n> -\t\t\tif (pp->fmt == CMIT_FMT_MBOXRD &&\n> -\t\t\t\t\tis_mboxrd_from(line, linelen))\n> -\t\t\t\tstrbuf_addch(sb, '>');\n> +\t\t\tswitch (pp->fmt) {\n> +\t\t\tcase CMIT_FMT_EMAIL:\n> +\t\t\t\tif (is_from_line(line, linelen))\n> +\t\t\t\t\tstrbuf_addch(sb, '>');\n> +\t\t\t\tbreak;\n> +\t\t\tcase CMIT_FMT_MBOXRD:\n> +\t\t\t\tif (is_mboxrd_from(line, linelen))\n> +\t\t\t\t\tstrbuf_addch(sb, '>');\n> +\t\t\t\tbreak;\n> +\t\t\tdefault:\n> +\t\t\t\tbreak;\n> +\t\t\t}\n\nSorry to be nitpicking once again; I think this would be conciser (and\neasier to read at least for me) as:\n\n-\t\t\tif (pp->fmt == CMIT_FMT_MBOXRD &&\n-\t\t\t\t\tis_mboxrd_from(line, linelen))\n+\t\t\tif ((pp->fmt == CMIT_FMT_MBOXRD &&\n+\t\t\t is_mboxrd_from(line, linelen)) ||\n+\t\t\t (pp->fmt == CMIT_FMT_EMAIL &&\n+\t\t\t is_from_line(line, linelen)))\n \t\t\t\tstrbuf_addch(sb, '>');\n\n> diff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh\n> index 1206c48..8fa3982 100755\n> --- a/t/t4014-format-patch.sh\n> +++ b/t/t4014-format-patch.sh\n> @@ -1606,4 +1606,18 @@ test_expect_success 'format-patch --pretty=mboxrd' '\n> \ttest_cmp expect actual\n> '\n> \n> +test_expect_success 'format-patch From escaping' '\n> +\tcat >msg <<-INPUT_END &&\n> +\tsomebody pasted format-patch output into a body\n> +\n> +\tFrom 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001\n> +\tINPUT_END\n> +\n> +\tC=$(git commit-tree HEAD^^{tree} -p HEAD +\tgit format-patch --stdout -1 $C~1..$C >patch &&\n\nEither \"-1 $C\" or \"$C~1..$C\", not both...\n\n> +\tgit grep -h --no-index \\\n> +\t\t\">From 0000000000000000000000000000000000000000 \" \\\n> +\t\tpatch\n> +'\n> +\n> test_done\n> -- \n> EW\n\nHeh, that's a nice Git version ;-)\n\nCiao,\nDscho\n"},{"id":"292049","messageId":"alpine.DEB.2.20.1607241003480.14111@virtualbox","threadId":"42909","inReplyTo":"m2poq3zdu3.fsf@linux-m68k.org","subject":"Re: [PATCH 1/2] mailinfo: extract is_from_line from mailsplit","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-07-24T08:05:16Z","receivedAt":"2016-07-24T08:05:41Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Andreas,\n\nOn Sun, 24 Jul 2016, Andreas Schwab wrote:\n\n> Eric Wong writes:\n> \n> > diff --git a/mailinfo.c b/mailinfo.c\n> > index 9f19ca1..0ebd953 100644\n> > --- a/mailinfo.c\n> > +++ b/mailinfo.c\n> > @@ -1035,3 +1035,34 @@ void clear_mailinfo(struct mailinfo *mi)\n> > \n> > \tstrbuf_release(&mi->log_message);\n> > }\n> > +\n> > +int is_from_line(const char *line, int len)\n> > +{\n> > +\tconst char *colon;\n> > +\n> > +\tif (len < 20 || memcmp(\"From \", line, 5))\n> > +\t\treturn 0;\n> > +\n> > +\tcolon = line + len - 2;\n> > +\tline += 5;\n> > +\tfor (;;) {\n> > +\t\tif (colon < line)\n> > +\t\t\treturn 0;\n> > +\t\tif (*--colon == ':')\n> > +\t\t\tbreak;\n> > +\t}\n> > +\n> > +\tif (!isdigit(colon[-4]) ||\n> > +\t !isdigit(colon[-2]) ||\n> > +\t !isdigit(colon[-1]) ||\n> > +\t !isdigit(colon[ 1]) ||\n> > +\t !isdigit(colon[ 2]))\n> > +\t\treturn 0;\n> > +\n> > +\t/* year */\n> > +\tif (strtol(colon+3, NULL, 10) <= 90)\n> > +\t\treturn 0;\n> > +\n> > +\t/* Ok, close enough */\n> > +\treturn 1;\n> > +}\n> \n> Should this be made more strict, like by checking for a space before the\n> year?\n\nThis patch only moves the function, so it would be inappropriate to change\nit.\n\nIf you want to make it stricter, you will have to submit a separate patch.\n\nCiao,\nJohannes\n"},{"id":"292050","messageId":"m2lh0rzc5w.fsf@linux-m68k.org","threadId":"42909","inReplyTo":"alpine.DEB.2.20.1607241003480.14111@virtualbox","subject":"Re: [PATCH 1/2] mailinfo: extract is_from_line from mailsplit","fromName":"Andreas Schwab","fromEmail":"schwab@linux-m68k.org","sentAt":"2016-07-24T08:13:31Z","receivedAt":"2016-07-24T08:13:42Z","isPatch":true,"sender":{"key":"schwab@linux-m68k.org","avatar":"https://avatars.githubusercontent.com/u/2175493?v=4"},"body":"Johannes Schindelin writes:\n\n> Hi Andreas,\n>\n> On Sun, 24 Jul 2016, Andreas Schwab wrote:\n>\n>> Eric Wong writes:\n>> \n>> > diff --git a/mailinfo.c b/mailinfo.c\n>> > index 9f19ca1..0ebd953 100644\n>> > --- a/mailinfo.c\n>> > +++ b/mailinfo.c\n>> > @@ -1035,3 +1035,34 @@ void clear_mailinfo(struct mailinfo *mi)\n>> > \n>> > \tstrbuf_release(&mi->log_message);\n>> > }\n>> > +\n>> > +int is_from_line(const char *line, int len)\n>> > +{\n>> > +\tconst char *colon;\n>> > +\n>> > +\tif (len < 20 || memcmp(\"From \", line, 5))\n>> > +\t\treturn 0;\n>> > +\n>> > +\tcolon = line + len - 2;\n>> > +\tline += 5;\n>> > +\tfor (;;) {\n>> > +\t\tif (colon < line)\n>> > +\t\t\treturn 0;\n>> > +\t\tif (*--colon == ':')\n>> > +\t\t\tbreak;\n>> > +\t}\n>> > +\n>> > +\tif (!isdigit(colon[-4]) ||\n>> > +\t !isdigit(colon[-2]) ||\n>> > +\t !isdigit(colon[-1]) ||\n>> > +\t !isdigit(colon[ 1]) ||\n>> > +\t !isdigit(colon[ 2]))\n>> > +\t\treturn 0;\n>> > +\n>> > +\t/* year */\n>> > +\tif (strtol(colon+3, NULL, 10) <= 90)\n>> > +\t\treturn 0;\n>> > +\n>> > +\t/* Ok, close enough */\n>> > +\treturn 1;\n>> > +}\n>> \n>> Should this be made more strict, like by checking for a space before the\n>> year?\n>\n> This patch only moves the function, so it would be inappropriate to change\n> it.\n>\n> If you want to make it stricter, you will have to submit a separate patch.\n\nYou didn't answer my question.\n\nAndreas.\n\n-- \nAndreas Schwab, schwab@linux-m68k.org\nGPG Key fingerprint = 58CA 54C7 6D53 942B 1756 01D3 44D5 214B 8276 4ED5\n\"And now for something completely different.\"\n"},{"id":"292053","messageId":"20160724105833.GA25507@starla","threadId":"42909","inReplyTo":"xmqqk2gb8q81.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] format-patch: escape \"From \" lines recognized by mailsplit","fromName":"Eric Wong","fromEmail":"e@80x24.org","sentAt":"2016-07-24T10:58:33Z","receivedAt":"2016-07-24T10:58:38Z","isPatch":true,"sender":{"key":"e@80x24.org","avatar":null},"body":"Junio C Hamano wrote:\n> As a tool to produce mbox file, quoting like this in format-patch\n> output may make sense, I would think, but shouldn't send-email undo\n> this when sending individual patches?\n\nYes, that sounds like a good idea. Thanks.\nWill followup in a day or two.\n"},{"id":"292059","messageId":"20160724153035.GA32304@sigill.intra.peff.net","threadId":"42909","inReplyTo":"alpine.DEB.2.20.1607240926500.14111@virtualbox","subject":"Re: [PATCH 2/2] format-patch: escape \"From \" lines recognized by mailsplit","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-07-24T15:30:36Z","receivedAt":"2016-07-24T15:34:47Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Jul 24, 2016 at 09:37:57AM +0200, Johannes Schindelin wrote:\n\n> On Sun, 24 Jul 2016, Eric Wong wrote:\n> \n> > @@ -1745,9 +1746,18 @@ void pp_remainder(struct pretty_print_context *pp,\n> > \t\t\tstrbuf_add_tabexpand(sb, pp->expand_tabs_in_log,\n> > \t\t\t\t\t line, linelen);\n> > \t\telse {\n> > -\t\t\tif (pp->fmt == CMIT_FMT_MBOXRD &&\n> > -\t\t\t\t\tis_mboxrd_from(line, linelen))\n> > -\t\t\t\tstrbuf_addch(sb, '>');\n> > +\t\t\tswitch (pp->fmt) {\n> > +\t\t\tcase CMIT_FMT_EMAIL:\n> > +\t\t\t\tif (is_from_line(line, linelen))\n> > +\t\t\t\t\tstrbuf_addch(sb, '>');\n> > +\t\t\t\tbreak;\n> > +\t\t\tcase CMIT_FMT_MBOXRD:\n> > +\t\t\t\tif (is_mboxrd_from(line, linelen))\n> > +\t\t\t\t\tstrbuf_addch(sb, '>');\n> > +\t\t\t\tbreak;\n> > +\t\t\tdefault:\n> > +\t\t\t\tbreak;\n> > +\t\t\t}\n> \n> Sorry to be nitpicking once again; I think this would be conciser (and\n> easier to read at least for me) as:\n> \n> -\t\t\tif (pp->fmt == CMIT_FMT_MBOXRD &&\n> -\t\t\t\t\tis_mboxrd_from(line, linelen))\n> +\t\t\tif ((pp->fmt == CMIT_FMT_MBOXRD &&\n> +\t\t\t is_mboxrd_from(line, linelen)) ||\n> +\t\t\t (pp->fmt == CMIT_FMT_EMAIL &&\n> +\t\t\t is_from_line(line, linelen)))\n> \t\t\t\tstrbuf_addch(sb, '>');\n\nSince we are nitpicking, I think:\n\n static int needs_from_quoting(enum cmit_fmt fmt,\n const char *line, size_t len)\n {\n\tif (fmt == CMIT_FMT_MBOXRD && is_mboxrd_from(line, linelen))\n\t\treturn 1;\n\tif (fmt == CMIT_FMT_EMAIL && is_from_line(line, linelen))\n\t\treturn 1;\n\treturn 0;\n }\n\n ...\n if (needs_from_quoting(pp->fmt, line, linelen))\n\tstrbuf_addch(sb, '>');\n\nis even nicer. There are lots of alternatives to write the helper\nfunction, and I do not care much which one is chosen. But splitting it\nout into a concise \"do we need to do this?\" query function makes the\nflow in pp_remainder much easier to follow.\n\n-Peff\n"},{"id":"292060","messageId":"xmqqd1m3825y.fsf@gitster.mtv.corp.google.com","threadId":"42909","inReplyTo":"xmqqk2gb8q81.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] format-patch: escape \"From \" lines recognized by mailsplit","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-07-24T15:51:37Z","receivedAt":"2016-07-24T15:51:45Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Eric Wong writes:\n>\n>> Users have mistakenly copied \"From \" lines into commit messages\n>> in the past, and will certainly make the same mistakes in the\n>> future. Since not everyone uses mboxrd, yet, we should at least\n>> prevent miss-split mails by always escaping \"From \" lines based\n>> on the check used by mailsplit.\n>>\n>> mailsplit will not perform unescaping by default, yet, as it\n>> could cause further invocations of format-patch from old\n>> versions of git to generate bad output. Propagating the mboxo\n>> escaping is preferable to miss-split patches. Unescaping may\n>> still be performed via \"--mboxrd\".\n>\n> As a tool to produce mbox file, quoting like this in format-patch\n> output may make sense, I would think, but shouldn't send-email undo\n> this when sending individual patches?\n\nAlso, doesn't it break \"git rebase\" (non-interactive), or anything\nthat internally runs format-patch to individual files and then runs\nam on each of them, anything that knows that each output file from\nformat-patch corresponds to a single change and there is no need to\nsplit, badly if we do this unconditionally?\n\nIOW, shouldn't this be an optional feature to format-patch that is\ntriggered by passing a new command line option that currently nobody\nis passing?\n"},{"id":"292087","messageId":"20160725084357.GA8025@starla","threadId":"42909","inReplyTo":"xmqqd1m3825y.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] format-patch: escape \"From \" lines recognized by mailsplit","fromName":"Eric Wong","fromEmail":"e@80x24.org","sentAt":"2016-07-25T08:43:57Z","receivedAt":"2016-07-25T08:44:02Z","isPatch":true,"sender":{"key":"e@80x24.org","avatar":null},"body":"Junio C Hamano wrote:\n> Junio C Hamano writes:\n> > Eric Wong writes:\n> >\n> >> Users have mistakenly copied \"From \" lines into commit messages\n> >> in the past, and will certainly make the same mistakes in the\n> >> future. Since not everyone uses mboxrd, yet, we should at least\n> >> prevent miss-split mails by always escaping \"From \" lines based\n> >> on the check used by mailsplit.\n> >>\n> >> mailsplit will not perform unescaping by default, yet, as it\n> >> could cause further invocations of format-patch from old\n> >> versions of git to generate bad output. Propagating the mboxo\n> >> escaping is preferable to miss-split patches. Unescaping may\n> >> still be performed via \"--mboxrd\".\n> >\n> > As a tool to produce mbox file, quoting like this in format-patch\n> > output may make sense, I would think, but shouldn't send-email undo\n> > this when sending individual patches?\n> \n> Also, doesn't it break \"git rebase\" (non-interactive), or anything\n> that internally runs format-patch to individual files and then runs\n> am on each of them, anything that knows that each output file from\n> format-patch corresponds to a single change and there is no need to\n> split, badly if we do this unconditionally?\n\nYes, rebase should probably unescape is_from_line matches.\n\nAnything which spawns an editor should probably warn/reprompt\nusers on is_from_line() matches, too, to prevent user errors\nfrom sneaking in.\n\n> IOW, shouldn't this be an optional feature to format-patch that is\n> triggered by passing a new command line option that currently nobody\n> is passing?\n\nI added --pretty=mboxrd as the optional feature for this reason.\nIt'll take a while for people to start using it (or perhaps make\nit the default in git 3.0).\nIn the meantime, I would prefer extra \">\" being injected rather\nthan breaking mailsplit completely.\n"},{"id":"292110","messageId":"xmqqzip562s3.fsf@gitster.mtv.corp.google.com","threadId":"42909","inReplyTo":"20160725084357.GA8025@starla","subject":"Re: [PATCH] format-patch: escape \"From \" lines recognized by mailsplit","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-07-25T17:33:32Z","receivedAt":"2016-07-25T17:33:50Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Wong writes:\n\n>> Also, doesn't it break \"git rebase\" (non-interactive), or anything\n>> that internally runs format-patch to individual files and then runs\n>> am on each of them, anything that knows that each output file from\n>> format-patch corresponds to a single change and there is no need to\n>> split, badly if we do this unconditionally?\n>\n> Yes, rebase should probably unescape is_from_line matches.\n\nThis shouldn't matter for \"git rebase\", as it only reads from the\nmbox \"From \" line to learn the\noriginal commit and extract the log message directly from there.\n\nBut a third-party script that wants to read format-patch output\nwould be forced to upgrade, which is a pain if we make this change\nunconditionally.\n\n>> IOW, shouldn't this be an optional feature to format-patch that\n>> is triggered by passing a new command line option that currently\n>> nobody is passing?\n"},{"id":"292114","messageId":"xmqqmvl561ez.fsf@gitster.mtv.corp.google.com","threadId":"42909","inReplyTo":"m2poq3zdu3.fsf@linux-m68k.org","subject":"Re: [PATCH 1/2] mailinfo: extract is_from_line from mailsplit","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-07-25T18:03:00Z","receivedAt":"2016-07-25T18:03:10Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Andreas Schwab writes:\n\n>> +\tcolon = line + len - 2;\n>> +\tline += 5;\n>> +\tfor (;;) {\n>> +\t\tif (colon < line)\n>> +\t\t\treturn 0;\n>> +\t\tif (*--colon == ':')\n>> +\t\t\tbreak;\n>> +\t}\n>> +\n>> +\tif (!isdigit(colon[-4]) ||\n>> +\t !isdigit(colon[-2]) ||\n>> +\t !isdigit(colon[-1]) ||\n>> +\t !isdigit(colon[ 1]) ||\n>> +\t !isdigit(colon[ 2]))\n>> +\t\treturn 0;\n>> +\n>> +\t/* year */\n>> +\tif (strtol(colon+3, NULL, 10) <= 90)\n>> +\t\treturn 0;\n>> +\n>> +\t/* Ok, close enough */\n>> +\treturn 1;\n>> +}\n>\n> Should this be made more strict, like by checking for a space before the\n> year?\n\nThe function seems to judge the line to be \"close enough\" by\nchecking these places (please view with fixed-width font ;-):\n\n From me Mon Jul 25 01:23:45 2016\n xxxxx x xxxxx ....\n\nWe only see if the year part is more than 90 but we do not even\nensure that it is at the end of the line without any further\ngarbage, which I think is probably OK for the purpose of \"close\nenough\".\n\nWe also do not bother rejecting a single-digit hour, or there is a\ncolon between the hour and minute part.\n\nI would say requiring SP between the year and the second, and\nrequiring colon between hour and minute, would probably be a good\nchange, i.e. something like this:\n\n\tif (!isdigit(colon[-4]) ||\n\t colon[-3] != ':' ||\n\t !isdigit(colon[-2]) ||\n\t !isdigit(colon[-1]) ||\n\t !isdigit(colon[ 1]) ||\n\t !isdigit(colon[ 2]) ||\n\t colon[3] != ' ')\n\nwould be safe enough without increasing the false negatives too\nmuch.\n"},{"id":"292142","messageId":"20160725204904.GA1424@starla","threadId":"42909","inReplyTo":"xmqqzip562s3.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] format-patch: escape \"From \" lines recognized by mailsplit","fromName":"Eric Wong","fromEmail":"e@80x24.org","sentAt":"2016-07-25T20:49:04Z","receivedAt":"2016-07-25T20:49:13Z","isPatch":true,"sender":{"key":"e@80x24.org","avatar":null},"body":"Junio C Hamano wrote:\n> Eric Wong writes:\n> \n> >> Also, doesn't it break \"git rebase\" (non-interactive), or anything\n> >> that internally runs format-patch to individual files and then runs\n> >> am on each of them, anything that knows that each output file from\n> >> format-patch corresponds to a single change and there is no need to\n> >> split, badly if we do this unconditionally?\n> >\n> > Yes, rebase should probably unescape is_from_line matches.\n> \n> This shouldn't matter for \"git rebase\", as it only reads from the\n> mbox \"From \" line to learn the\n> original commit and extract the log message directly from there.\n\nOK.\n\n> But a third-party script that wants to read format-patch output\n> would be forced to upgrade, which is a pain if we make this change\n> unconditionally.\n\nI suppose unconditionally having mailsplit unescape\n\">From ... $DATE\" lines might be acceptable?\n\nIt'll still propagate these mistakes to older versions of git,\nbut those installations will eventually become fewer.\n\n> >> IOW, shouldn't this be an optional feature to format-patch that\n> >> is triggered by passing a new command line option that currently\n> >> nobody is passing?\n"}]}