{"thread":{"id":"42491","subject":"[RFC/PATCH 0/3] support mboxrd format","startedAt":"2016-05-30T23:21:39Z","lastAt":"2016-06-04T02:03:59Z","messageCount":18,"participants":["Eric Wong","Eric Sunshine","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"287860","messageId":"20160530232142.21098-1-e@80x24.org","threadId":"42491","inReplyTo":null,"subject":"[RFC/PATCH 0/3] support mboxrd format","fromName":"Eric Wong","fromEmail":"e@80x24.org","sentAt":"2016-05-30T23:21:39Z","receivedAt":"2016-05-30T23:21:39Z","isPatch":true,"sender":{"key":"e@80x24.org","avatar":null},"body":"Sometimes users will copy+paste an entire mbox into a\ncommit message, leading to bad splits when a patch is\noutput as an email.\n\nUnlike other mbox-family formats, mboxrd allows reversible\nround-tripping while avoiding bad splits for old \"mboxo\"\nreaders.\n\nI'm also considering altering the current\n\"From ${COMMIT} Mon Sep 17 00:00:00 2001\" line to something\nelse so mailsplit (or \"am\") can autodetect.\n\nMaybe:\n\tFrom ${COMMIT}@mboxrd Mon Sep 17 00:00:00 2001\n?\n\n\nWe may also want to default to single escaping \"From \" in\ncommit messages for --pretty=email to avoid corruption when\nsomebody copy+pastes an mbox into the commit message.\nThis is a technically incompatible change, but I think it's\npreferable to breaking splitting complete.\n\nIn other words, --pretty=email changes to output \"mboxo\"\nfor now.\n\nLong term (possibly git 3.0?), maybe mboxrd can become the\ndefault mail format. IMHO, it should've been since 2005.\n\nref: http://homepage.ntlworld.com/jonathan.deboynepollard/FGA/mail-mbox-formats.html\n\nEric Wong (3):\n pretty: support \"mboxrd\" output format\n mailsplit: support unescaping mboxrd messages\n am: support --patch-format=mboxrd\n\n Documentation/git-am.txt | 3 ++-\n Documentation/git-mailsplit.txt | 7 ++++++-\n builtin/am.c | 14 ++++++++++---\n builtin/log.c | 2 +-\n builtin/mailsplit.c | 23 +++++++++++++++++++++\n commit.h | 6 ++++++\n log-tree.c | 4 ++--\n pretty.c | 45 +++++++++++++++++++++++++++++++++--------\n t/t4014-format-patch.sh | 27 +++++++++++++++++++++++++\n t/t4150-am.sh | 20 ++++++++++++++++++\n t/t5100-mailinfo.sh | 13 ++++++++++++\n t/t5100/0001mboxrd | 4 ++++\n t/t5100/0002mboxrd | 3 +++\n t/t5100/sample.mboxrd | 17 ++++++++++++++++\n 14 files changed, 172 insertions(+), 16 deletions(-)\n"},{"id":"287861","messageId":"20160530232142.21098-2-e@80x24.org","threadId":"42491","inReplyTo":"20160530232142.21098-1-e@80x24.org","subject":"[PATCH 1/3] pretty: support \"mboxrd\" output format","fromName":"Eric Wong","fromEmail":"e@80x24.org","sentAt":"2016-05-30T23:21:40Z","receivedAt":"2016-05-30T23:21:40Z","isPatch":true,"sender":{"key":"e@80x24.org","avatar":null},"body":"This output format prevents format-patch output from breaking\nreaders if somebody copy+pasted an mbox into a commit message.\n\nUnlike the traditional \"mboxo\" format, \"mboxrd\" is designed to\nbe fully-reversible. \"mboxrd\" also gracefully degrades to\nshowing extra \">\" in existing \"mboxo\" readers.\n\nThis degradation is preferable to breaking message splitting\ncompletely, a problem I've seen in \"mboxcl\" due to having\nmultiple, non-existent, or inaccurate Content-Length headers.\n\n\"mboxcl2\" is a non-starter since it's inherits the problems\nof \"mboxcl\" while being completely incompatible with existing\ntooling based around mailsplit.\n\nref: http://homepage.ntlworld.com/jonathan.deboynepollard/FGA/mail-mbox-formats.html\n\nSigned-off-by: Eric Wong \n---\n builtin/log.c | 2 +-\n commit.h | 6 ++++++\n log-tree.c | 4 ++--\n pretty.c | 45 +++++++++++++++++++++++++++++++++++++--------\n t/t4014-format-patch.sh | 27 +++++++++++++++++++++++++++\n 5 files changed, 73 insertions(+), 11 deletions(-)\n\ndiff --git a/builtin/log.c b/builtin/log.c\nindex 099f4f7..6d6f368 100644\n--- a/builtin/log.c\n+++ b/builtin/log.c\n@@ -953,7 +953,7 @@ static void make_cover_letter(struct rev_info *rev, int use_stdout,\n \tstruct pretty_print_context pp = {0};\n \tstruct commit *head = list[0];\n \n-\tif (rev->commit_format != CMIT_FMT_EMAIL)\n+\tif (!cmit_fmt_is_mail(rev->commit_format))\n \t\tdie(_(\"Cover letter needs email format\"));\n \n \tcommitter = git_committer_info(0);\ndiff --git a/commit.h b/commit.h\nindex b06db4d..1e04d3a 100644\n--- a/commit.h\n+++ b/commit.h\n@@ -131,11 +131,17 @@ enum cmit_fmt {\n \tCMIT_FMT_FULLER,\n \tCMIT_FMT_ONELINE,\n \tCMIT_FMT_EMAIL,\n+\tCMIT_FMT_MBOXRD,\n \tCMIT_FMT_USERFORMAT,\n \n \tCMIT_FMT_UNSPECIFIED\n };\n \n+static inline int cmit_fmt_is_mail(enum cmit_fmt fmt)\n+{\n+\treturn (fmt == CMIT_FMT_EMAIL || fmt == CMIT_FMT_MBOXRD);\n+}\n+\n struct pretty_print_context {\n \t/*\n \t * Callers should tweak these to change the behavior of pp_* functions.\ndiff --git a/log-tree.c b/log-tree.c\nindex 78a5381..48daf84 100644\n--- a/log-tree.c\n+++ b/log-tree.c\n@@ -603,7 +603,7 @@ void show_log(struct rev_info *opt)\n \t * Print header line of header..\n \t */\n \n-\tif (opt->commit_format == CMIT_FMT_EMAIL) {\n+\tif (cmit_fmt_is_mail(opt->commit_format)) {\n \t\tlog_write_email_headers(opt, commit, &ctx.subject, &extra_headers,\n \t\t\t\t\t&ctx.need_8bit_cte);\n \t} else if (opt->commit_format != CMIT_FMT_USERFORMAT) {\n@@ -694,7 +694,7 @@ void show_log(struct rev_info *opt)\n \n \tif ((ctx.fmt != CMIT_FMT_USERFORMAT) &&\n \t ctx.notes_message && *ctx.notes_message) {\n-\t\tif (ctx.fmt == CMIT_FMT_EMAIL) {\n+\t\tif (cmit_fmt_is_mail(ctx.fmt)) {\n \t\t\tstrbuf_addstr(&msgbuf, \"---\\n\");\n \t\t\topt->shown_dashes = 1;\n \t\t}\ndiff --git a/pretty.c b/pretty.c\nindex 87c4497..a33e604 100644\n--- a/pretty.c\n+++ b/pretty.c\n@@ -92,6 +92,7 @@ static void setup_commit_formats(void)\n \t\t{ \"medium\",\tCMIT_FMT_MEDIUM,\t0,\t8 },\n \t\t{ \"short\",\tCMIT_FMT_SHORT,\t\t0,\t0 },\n \t\t{ \"email\",\tCMIT_FMT_EMAIL,\t\t0,\t0 },\n+\t\t{ \"mboxrd\",\tCMIT_FMT_MBOXRD,\t0,\t0 },\n \t\t{ \"fuller\",\tCMIT_FMT_FULLER,\t0,\t8 },\n \t\t{ \"full\",\tCMIT_FMT_FULL,\t\t0,\t8 },\n \t\t{ \"oneline\",\tCMIT_FMT_ONELINE,\t1,\t0 }\n@@ -444,7 +445,7 @@ void pp_user_info(struct pretty_print_context *pp,\n \tif (pp->mailmap)\n \t\tmap_user(pp->mailmap, &mailbuf, &maillen, &namebuf, &namelen);\n \n-\tif (pp->fmt == CMIT_FMT_EMAIL) {\n+\tif (cmit_fmt_is_mail(pp->fmt)) {\n \t\tif (pp->from_ident && ident_cmp(pp->from_ident, &ident)) {\n \t\t\tstruct strbuf buf = STRBUF_INIT;\n \n@@ -494,6 +495,7 @@ void pp_user_info(struct pretty_print_context *pp,\n \t\t\t show_ident_date(&ident, &pp->date_mode));\n \t\tbreak;\n \tcase CMIT_FMT_EMAIL:\n+\tcase CMIT_FMT_MBOXRD:\n \t\tstrbuf_addf(sb, \"Date: %s\\n\",\n \t\t\t show_ident_date(&ident, DATE_MODE(RFC2822)));\n \t\tbreak;\n@@ -535,7 +537,7 @@ static void add_merge_info(const struct pretty_print_context *pp,\n {\n \tstruct commit_list *parent = commit->parents;\n \n-\tif ((pp->fmt == CMIT_FMT_ONELINE) || (pp->fmt == CMIT_FMT_EMAIL) ||\n+\tif ((pp->fmt == CMIT_FMT_ONELINE) || (cmit_fmt_is_mail(pp->fmt)) ||\n \t !parent || !parent->next)\n \t\treturn;\n \n@@ -1614,7 +1616,7 @@ void pp_title_line(struct pretty_print_context *pp,\n \tif (pp->after_subject) {\n \t\tstrbuf_addstr(sb, pp->after_subject);\n \t}\n-\tif (pp->fmt == CMIT_FMT_EMAIL) {\n+\tif (cmit_fmt_is_mail(pp->fmt)) {\n \t\tstrbuf_addch(sb, '\\n');\n \t}\n \n@@ -1697,12 +1699,34 @@ static void pp_handle_indent(struct pretty_print_context *pp,\n \t\tstrbuf_add(sb, line, linelen);\n }\n \n+static regex_t *mboxrd_prepare(void)\n+{\n+\tstatic regex_t preg;\n+\tconst char re[] = \"^>*From \";\n+\tint err = regcomp(&preg, re, REG_NOSUB | REG_EXTENDED);\n+\n+\tif (err) {\n+\t\tchar errbuf[1024];\n+\n+\t\tregerror(err, &preg, errbuf, sizeof(errbuf));\n+\t\tregfree(&preg);\n+\t\tdie(\"Cannot prepare regexp `%s': %s\", re, errbuf);\n+\t}\n+\n+\treturn &preg;\n+}\n+\n void pp_remainder(struct pretty_print_context *pp,\n \t\t const char **msg_p,\n \t\t struct strbuf *sb,\n \t\t int indent)\n {\n \tint first = 1;\n+\tstatic regex_t *mboxrd_from;\n+\n+\tif (pp->fmt == CMIT_FMT_MBOXRD && !mboxrd_from)\n+\t\tmboxrd_from = mboxrd_prepare();\n+\n \tfor (;;) {\n \t\tconst char *line = *msg_p;\n \t\tint linelen = get_one_line(line);\n@@ -1725,8 +1749,13 @@ void pp_remainder(struct pretty_print_context *pp,\n \t\telse if (pp->expand_tabs_in_log)\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\telse {\n+\t\t\tif (pp->fmt == CMIT_FMT_MBOXRD &&\n+\t\t\t\t\t!regexec(mboxrd_from, line, 0, 0, 0))\n+\t\t\t\tstrbuf_addch(sb, '>');\n+\n \t\t\tstrbuf_add(sb, line, linelen);\n+\t\t}\n \t\tstrbuf_addch(sb, '\\n');\n \t}\n }\n@@ -1750,14 +1779,14 @@ void pretty_print_commit(struct pretty_print_context *pp,\n \tencoding = get_log_output_encoding();\n \tmsg = reencoded = logmsg_reencode(commit, NULL, encoding);\n \n-\tif (pp->fmt == CMIT_FMT_ONELINE || pp->fmt == CMIT_FMT_EMAIL)\n+\tif (pp->fmt == CMIT_FMT_ONELINE || cmit_fmt_is_mail(pp->fmt))\n \t\tindent = 0;\n \n \t/*\n \t * We need to check and emit Content-type: to mark it\n \t * as 8-bit if we haven't done so.\n \t */\n-\tif (pp->fmt == CMIT_FMT_EMAIL && need_8bit_cte == 0) {\n+\tif (cmit_fmt_is_mail(pp->fmt) && need_8bit_cte == 0) {\n \t\tint i, ch, in_body;\n \n \t\tfor (in_body = i = 0; (ch = msg[i]); i++) {\n@@ -1785,7 +1814,7 @@ void pretty_print_commit(struct pretty_print_context *pp,\n \tmsg = skip_empty_lines(msg);\n \n \t/* These formats treat the title line specially. */\n-\tif (pp->fmt == CMIT_FMT_ONELINE || pp->fmt == CMIT_FMT_EMAIL)\n+\tif (pp->fmt == CMIT_FMT_ONELINE || cmit_fmt_is_mail(pp->fmt))\n \t\tpp_title_line(pp, &msg, sb, encoding, need_8bit_cte);\n \n \tbeginning_of_body = sb->len;\n@@ -1802,7 +1831,7 @@ void pretty_print_commit(struct pretty_print_context *pp,\n \t * format. Make sure we did not strip the blank line\n \t * between the header and the body.\n \t */\n-\tif (pp->fmt == CMIT_FMT_EMAIL && sb->len <= beginning_of_body)\n+\tif (cmit_fmt_is_mail(pp->fmt) && sb->len <= beginning_of_body)\n \t\tstrbuf_addch(sb, '\\n');\n \n \tunuse_commit_buffer(commit, reencoded);\ndiff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh\nindex 8049cad..0a2070e 100755\n--- a/t/t4014-format-patch.sh\n+++ b/t/t4014-format-patch.sh\n@@ -1565,4 +1565,31 @@ test_expect_success 'format-patch --base overrides format.useAutoBase' '\n \ttest_cmp expected actual\n '\n \n+test_expect_success 'format-patch --pretty=mboxrd' '\n+\tcat >msg <<-INPUT_END &&\n+\tmboxrd should escape the body\n+\n+\tFrom could trip up a loose mbox parser\n+\t>From extra escape for reversibility\n+\t>>From extra escape for reversibility 2\n+\tfrom lower case not escaped\n+\tFromm bad speling not escaped\n+\t From with leading space not escaped\n+\tINPUT_END\n+\n+\tcat >expect <<-INPUT_END &&\n+\t>From could trip up a loose mbox parser\n+\t>>From extra escape for reversibility\n+\t>>>From extra escape for reversibility 2\n+\tfrom lower case not escaped\n+\tFromm bad speling not escaped\n+\t From with leading space not escaped\n+\tINPUT_END\n+\n+\tC=$(git commit-tree HEAD^^{tree} -p HEAD patch &&\n+\tgrep -A5 \"^>From could trip up a loose mbox parser\" patch >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_done\n"},{"id":"287863","messageId":"20160530232142.21098-3-e@80x24.org","threadId":"42491","inReplyTo":"20160530232142.21098-1-e@80x24.org","subject":"[PATCH 2/3] mailsplit: support unescaping mboxrd messages","fromName":"Eric Wong","fromEmail":"e@80x24.org","sentAt":"2016-05-30T23:21:41Z","receivedAt":"2016-05-30T23:21:41Z","isPatch":true,"sender":{"key":"e@80x24.org","avatar":null},"body":"This will allow us to parse the output of --pretty=mboxrd\nand the output of other mboxrd generators.\n\nSigned-off-by: Eric Wong \n---\n Documentation/git-mailsplit.txt | 7 ++++++-\n builtin/mailsplit.c | 23 +++++++++++++++++++++++\n t/t5100-mailinfo.sh | 13 +++++++++++++\n t/t5100/0001mboxrd | 4 ++++\n t/t5100/0002mboxrd | 3 +++\n t/t5100/sample.mboxrd | 17 +++++++++++++++++\n 6 files changed, 66 insertions(+), 1 deletion(-)\n create mode 100644 t/t5100/0001mboxrd\n create mode 100644 t/t5100/0002mboxrd\n create mode 100644 t/t5100/sample.mboxrd\n\ndiff --git a/Documentation/git-mailsplit.txt b/Documentation/git-mailsplit.txt\nindex 4d1b871..e3b2a88 100644\n--- a/Documentation/git-mailsplit.txt\n+++ b/Documentation/git-mailsplit.txt\n@@ -8,7 +8,8 @@ git-mailsplit - Simple UNIX mbox splitter program\n SYNOPSIS\n --------\n [verse]\n-'git mailsplit' [-b] [-f] [-d] [--keep-cr] -o [--] [(|)...]\n+'git mailsplit' [-b] [-f] [-d] [--keep-cr] [--mboxrd]\n+\t\t-o [--] [(|)...]\n \n DESCRIPTION\n -----------\n@@ -47,6 +48,10 @@ OPTIONS\n --keep-cr::\n \tDo not remove `\\r` from lines ending with `\\r\\n`.\n \n+--mboxrd::\n+\tInput is of the \"mboxrd\" format and \"^>+From \" line escaping is\n+\treversed.\n+\n GIT\n ---\n Part of the linkgit:git[1] suite\ndiff --git a/builtin/mailsplit.c b/builtin/mailsplit.c\nindex 4859ede..fad871d 100644\n--- a/builtin/mailsplit.c\n+++ b/builtin/mailsplit.c\n@@ -45,6 +45,7 @@ static int is_from_line(const char *line, int len)\n \n static struct strbuf buf = STRBUF_INIT;\n static int keep_cr;\n+static regex_t *gtfrom;\n \n /* Called with the first line (potentially partial)\n * already in buf[] -- normally that should begin with\n@@ -77,6 +78,10 @@ static int split_one(FILE *mbox, const char *name, int allow_bare)\n \t\t\tstrbuf_addch(&buf, '\\n');\n \t\t}\n \n+\t\tif (gtfrom && buf.len > (sizeof(\">From \") - 1) &&\n+\t\t\t\t!regexec(gtfrom, buf.buf, 0, 0, 0))\n+\t\t\tstrbuf_remove(&buf, 0, 1);\n+\n \t\tif (fwrite(buf.buf, 1, buf.len, output) != buf.len)\n \t\t\tdie_errno(\"cannot write output\");\n \n@@ -242,6 +247,22 @@ out:\n \treturn ret;\n }\n \n+static regex_t *gtfrom_prepare(void)\n+{\n+\tstatic regex_t preg;\n+\tconst char re[] = \"^>+From \";\n+\tint err = regcomp(&preg, re, REG_NOSUB | REG_EXTENDED);\n+\n+\tif (err) {\n+\t\tchar errbuf[1024];\n+\t\tregerror(err, &preg, errbuf, sizeof(errbuf));\n+\t\tregfree(&preg);\n+\t\tdie(\"Cannot prepare regexp `%s': %s\", re, errbuf);\n+\t}\n+\n+\treturn &preg;\n+}\n+\n int cmd_mailsplit(int argc, const char **argv, const char *prefix)\n {\n \tint nr = 0, nr_prec = 4, num = 0;\n@@ -271,6 +292,8 @@ int cmd_mailsplit(int argc, const char **argv, const char *prefix)\n \t\t\tkeep_cr = 1;\n \t\t} else if ( arg[1] == 'o' && arg[2] ) {\n \t\t\tdir = arg+2;\n+\t\t} else if (!strcmp(arg, \"--mboxrd\")) {\n+\t\t\tgtfrom = gtfrom_prepare();\n \t\t} else if ( arg[1] == '-' && !arg[2] ) {\n \t\t\targp++;\t/* -- marks end of options */\n \t\t\tbreak;\ndiff --git a/t/t5100-mailinfo.sh b/t/t5100-mailinfo.sh\nindex 85b3df5..62b442c 100755\n--- a/t/t5100-mailinfo.sh\n+++ b/t/t5100-mailinfo.sh\n@@ -111,4 +111,17 @@ test_expect_success 'mailinfo on message with quoted >From' '\n \ttest_cmp \"$TEST_DIRECTORY\"/t5100/quoted-from.expect quoted-from/msg\n '\n \n+test_expect_success 'mailinfo unescapes with --mboxrd' '\n+\tmkdir mboxrd &&\n+\tgit mailsplit -omboxrd --mboxrd \\\n+\t\t\"$TEST_DIRECTORY\"/t5100/sample.mboxrd >last &&\n+\ttest x\"$(cat last)\" = x2 &&\n+\tfor i in 0001 0002\n+\tdo\n+\t\tgit mailinfo mboxrd/msg mboxrd/patch \\\n+\t\t mboxrd/out &&\n+\t\ttest_cmp \"$TEST_DIRECTORY\"/t5100/${i}mboxrd mboxrd/msg\n+\tdone\n+'\n+\n test_done\ndiff --git a/t/t5100/0001mboxrd b/t/t5100/0001mboxrd\nnew file mode 100644\nindex 0000000..494ec55\n--- /dev/null\n+++ b/t/t5100/0001mboxrd\n@@ -0,0 +1,4 @@\n+From the beginning, mbox should have been mboxrd\n+>From escaped\n+From not mangled but this line should have been escaped\n+\ndiff --git a/t/t5100/0002mboxrd b/t/t5100/0002mboxrd\nnew file mode 100644\nindex 0000000..3c30b0b\n--- /dev/null\n+++ b/t/t5100/0002mboxrd\n@@ -0,0 +1,3 @@\n+ >From unchanged\n+ From also unchanged\n+\ndiff --git a/t/t5100/sample.mboxrd b/t/t5100/sample.mboxrd\nnew file mode 100644\nindex 0000000..75de6dd\n--- /dev/null\n+++ b/t/t5100/sample.mboxrd\n@@ -0,0 +1,17 @@\n+From mboxrd Mon Sep 17 00:00:00 2001\n+From: mboxrd writer \n+Date: Fri, 9 Jun 2006 00:44:16 -0700\n+Subject: [PATCH] a commit with escaped From lines\n+\n+>From the beginning, mbox should have been mboxrd\n+>>From escaped\n+From not mangled but this line should have been escaped\n+\n+From mboxrd Mon Sep 17 00:00:00 2001\n+From: mboxrd writer \n+Date: Fri, 9 Jun 2006 00:44:16 -0700\n+Subject: [PATCH 2/2] another with fake From lines\n+\n+ >From unchanged\n+ From also unchanged\n+\n"},{"id":"287862","messageId":"20160530232142.21098-4-e@80x24.org","threadId":"42491","inReplyTo":"20160530232142.21098-1-e@80x24.org","subject":"[PATCH 3/3] am: support --patch-format=mboxrd","fromName":"Eric Wong","fromEmail":"e@80x24.org","sentAt":"2016-05-30T23:21:42Z","receivedAt":"2016-05-30T23:21:42Z","isPatch":true,"sender":{"key":"e@80x24.org","avatar":null},"body":"Combined with \"git format-patch --pretty=mboxrd\", this should\nallow us to round-trip commit messages with embedded mbox\n\"From \" lines without corruption.\n\nSigned-off-by: Eric Wong \n---\n Documentation/git-am.txt | 3 ++-\n builtin/am.c | 14 +++++++++++---\n t/t4150-am.sh | 20 ++++++++++++++++++++\n 3 files changed, 33 insertions(+), 4 deletions(-)\n\ndiff --git a/Documentation/git-am.txt b/Documentation/git-am.txt\nindex 13cdd7f..6348c29 100644\n--- a/Documentation/git-am.txt\n+++ b/Documentation/git-am.txt\n@@ -116,7 +116,8 @@ default. You can use `--no-utf8` to override this.\n \tBy default the command will try to detect the patch format\n \tautomatically. This option allows the user to bypass the automatic\n \tdetection and specify the patch format that the patch(es) should be\n-\tinterpreted as. Valid formats are mbox, stgit, stgit-series and hg.\n+\tinterpreted as. Valid formats are mbox, mboxrd,\n+\tstgit, stgit-series and hg.\n \n -i::\n --interactive::\ndiff --git a/builtin/am.c b/builtin/am.c\nindex 3dfe70b..d5da5fe 100644\n--- a/builtin/am.c\n+++ b/builtin/am.c\n@@ -70,7 +70,8 @@ enum patch_format {\n \tPATCH_FORMAT_MBOX,\n \tPATCH_FORMAT_STGIT,\n \tPATCH_FORMAT_STGIT_SERIES,\n-\tPATCH_FORMAT_HG\n+\tPATCH_FORMAT_HG,\n+\tPATCH_FORMAT_MBOXRD\n };\n \n enum keep_type {\n@@ -712,7 +713,8 @@ done:\n * Splits out individual email patches from `paths`, where each path is either\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, int keep_cr)\n+static int split_mail_mbox(struct am_state *state, const char **paths,\n+\t\t\t\tint keep_cr, int mboxrd)\n {\n \tstruct child_process cp = CHILD_PROCESS_INIT;\n \tstruct strbuf last = STRBUF_INIT;\n@@ -724,6 +726,8 @@ static int split_mail_mbox(struct am_state *state, const char **paths, int keep_\n \targv_array_push(&cp.args, \"-b\");\n \tif (keep_cr)\n \t\targv_array_push(&cp.args, \"--keep-cr\");\n+\tif (mboxrd)\n+\t\targv_array_push(&cp.args, \"--mboxrd\");\n \targv_array_push(&cp.args, \"--\");\n \targv_array_pushv(&cp.args, paths);\n \n@@ -965,13 +969,15 @@ static int split_mail(struct am_state *state, enum patch_format patch_format,\n \n \tswitch (patch_format) {\n \tcase PATCH_FORMAT_MBOX:\n-\t\treturn split_mail_mbox(state, paths, keep_cr);\n+\t\treturn split_mail_mbox(state, paths, keep_cr, 0);\n \tcase PATCH_FORMAT_STGIT:\n \t\treturn split_mail_conv(stgit_patch_to_mail, state, paths, keep_cr);\n \tcase PATCH_FORMAT_STGIT_SERIES:\n \t\treturn split_mail_stgit_series(state, paths, keep_cr);\n \tcase PATCH_FORMAT_HG:\n \t\treturn split_mail_conv(hg_patch_to_mail, state, paths, keep_cr);\n+\tcase PATCH_FORMAT_MBOXRD:\n+\t\treturn split_mail_mbox(state, paths, keep_cr, 1);\n \tdefault:\n \t\tdie(\"BUG: invalid patch_format\");\n \t}\n@@ -2201,6 +2207,8 @@ static int parse_opt_patchformat(const struct option *opt, const char *arg, int\n \t\t*opt_value = PATCH_FORMAT_STGIT_SERIES;\n \telse if (!strcmp(arg, \"hg\"))\n \t\t*opt_value = PATCH_FORMAT_HG;\n+\telse if (!strcmp(arg, \"mboxrd\"))\n+\t\t*opt_value = PATCH_FORMAT_MBOXRD;\n \telse\n \t\treturn error(_(\"Invalid value for --patch-format: %s\"), arg);\n \treturn 0;\ndiff --git a/t/t4150-am.sh b/t/t4150-am.sh\nindex b41bd17..74e093d 100755\n--- a/t/t4150-am.sh\n+++ b/t/t4150-am.sh\n@@ -957,4 +957,24 @@ test_expect_success 'am -s unexpected trailer block' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'am --patch-format=mboxrd handles mboxrd' '\n+\trm -fr .git/rebase-apply &&\n+\tgit checkout -f first &&\n+\techo mboxrd >>file &&\n+\tgit add file &&\n+\tcat >msg <<-INPUT_END &&\n+\tmboxrd should escape the body\n+\n+\tFrom could trip up a loose mbox parser\n+\t>From extra escape for reversibility\n+\tINPUT_END\n+\tgit commit -F msg &&\n+\tgit format-patch --pretty=mboxrd --stdout -1 >mboxrd1 &&\n+\tgrep \"^>From could trip up a loose mbox parser\" mboxrd1 &&\n+\tgit checkout -f first &&\n+\tgit am --patch-format=mboxrd mboxrd1 &&\n+\tgit cat-file commit HEAD | tail -n4 >out &&\n+\ttest_cmp msg out\n+'\n+\n test_done\n"},{"id":"287868","messageId":"CAPig+cQrSJe03_RtSyf5KO2vE3Rri7t70-he8SXA9Y4oBYY_Ww@mail.gmail.com","threadId":"42491","inReplyTo":"20160530232142.21098-2-e@80x24.org","subject":"Re: [PATCH 1/3] pretty: support \"mboxrd\" output format","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2016-05-31T03:40:48Z","receivedAt":"2016-05-31T03:40:48Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Mon, May 30, 2016 at 7:21 PM, Eric Wong wrote:\n> This output format prevents format-patch output from breaking\n> readers if somebody copy+pasted an mbox into a commit message.\n>\n> Unlike the traditional \"mboxo\" format, \"mboxrd\" is designed to\n> be fully-reversible. \"mboxrd\" also gracefully degrades to\n> showing extra \">\" in existing \"mboxo\" readers.\n> [...]\n> Signed-off-by: Eric Wong \n> ---\n> diff --git a/pretty.c b/pretty.c\n> @@ -1697,12 +1699,34 @@ static void pp_handle_indent(struct pretty_print_context *pp,\n> +static regex_t *mboxrd_prepare(void)\n> +{\n> + static regex_t preg;\n> + const char re[] = \"^>*From \";\n> + int err = regcomp(&preg, re, REG_NOSUB | REG_EXTENDED);\n> +[...]\n> + return &preg;\n> +}\n> +\n> void pp_remainder(struct pretty_print_context *pp,\n> const char **msg_p,\n> struct strbuf *sb,\n> int indent)\n> {\n> + static regex_t *mboxrd_from;\n> +\n> + if (pp->fmt == CMIT_FMT_MBOXRD && !mboxrd_from)\n> + mboxrd_from = mboxrd_prepare();\n> +\n> @@ -1725,8 +1749,13 @@ void pp_remainder(struct pretty_print_context *pp,\n> else if (pp->expand_tabs_in_log)\n> strbuf_add_tabexpand(sb, pp->expand_tabs_in_log,\n> line, linelen);\n> - else\n> + else {\n> + if (pp->fmt == CMIT_FMT_MBOXRD &&\n> + !regexec(mboxrd_from, line, 0, 0, 0))\n> + strbuf_addch(sb, '>');\n\nAt first glance, this seems dangerous since it's handing 'line' to\nregexec() without paying attention to 'linelen'. For an arbitrary\nregex, this could result in erroneous matches on subsequent \"lines\",\nhowever, since this expression is anchored with '^', it's not a\nproblem. But, it is a bit subtle.\n\nI wonder if hand-coding, rather than using a regex, could be an improvement:\n\n static int is_mboxrd_from(const char *s, size_t n)\n {\n size_t f = strlen(\"From \");\n const char *t = s + n;\n\n while (s < t && *s == '>')\n s++;\n return t - s >= f && !memcmp(s, \"From \", f);\n }\n\n ...\n if (is_mboxrd_from(line, linelen)\n strbuf_addch(sb, '>');\n\nor something.\n\n> +\n> strbuf_add(sb, line, linelen);\n> + }\n> strbuf_addch(sb, '\\n');\n> }\n> }\n> diff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh\n> @@ -1565,4 +1565,31 @@ test_expect_success 'format-patch --base overrides format.useAutoBase' '\n> +test_expect_success 'format-patch --pretty=mboxrd' '\n> + cat >msg <<-INPUT_END &&\n\nMaybe use <<-\\INPUT_END to indicate that no variable interpolation is\nexpected. Ditto below.\n\n> + mboxrd should escape the body\n> +\n> + From could trip up a loose mbox parser\n> + >From extra escape for reversibility\n> + >>From extra escape for reversibility 2\n> + from lower case not escaped\n> + Fromm bad speling not escaped\n> + From with leading space not escaped\n> + INPUT_END\n> +\n> + cat >expect <<-INPUT_END &&\n> + >From could trip up a loose mbox parser\n> + >>From extra escape for reversibility\n> + >>>From extra escape for reversibility 2\n> + from lower case not escaped\n> + Fromm bad speling not escaped\n> + From with leading space not escaped\n> + INPUT_END\n> +\n> + C=$(git commit-tree HEAD^^{tree} -p HEAD + git format-patch --pretty=mboxrd --stdout -1 $C~1..$C >patch &&\n> + grep -A5 \"^>From could trip up a loose mbox parser\" patch >actual &&\n\nHmm, -A is not POSIX and is otherwise not used in Git tests. Perhaps\nyou could use 'git grep --no-index -A' instead or something?\n\n> + test_cmp expect actual\n> +'\n> +\n> test_done\n"},{"id":"287883","messageId":"20160531074506.GA8911@dcvr.yhbt.net","threadId":"42491","inReplyTo":"CAPig+cQrSJe03_RtSyf5KO2vE3Rri7t70-he8SXA9Y4oBYY_Ww@mail.gmail.com","subject":"Re: [PATCH 1/3] pretty: support \"mboxrd\" output format","fromName":"Eric Wong","fromEmail":"e@80x24.org","sentAt":"2016-05-31T07:45:06Z","receivedAt":"2016-05-31T07:45:06Z","isPatch":true,"sender":{"key":"e@80x24.org","avatar":null},"body":"Eric Sunshine wrote:\n> On Mon, May 30, 2016 at 7:21 PM, Eric Wong wrote:\n> > diff --git a/pretty.c b/pretty.c\n> > @@ -1697,12 +1699,34 @@ static void pp_handle_indent(struct pretty_print_context *pp,\n> > +static regex_t *mboxrd_prepare(void)\n> > +{\n> > + static regex_t preg;\n> > + const char re[] = \"^>*From \";\n> > + int err = regcomp(&preg, re, REG_NOSUB | REG_EXTENDED);\n> > +[...]\n> > + return &preg;\n> > +}\n> > +\n> > void pp_remainder(struct pretty_print_context *pp,\n> > const char **msg_p,\n> > struct strbuf *sb,\n> > int indent)\n> > {\n> > + static regex_t *mboxrd_from;\n> > +\n> > + if (pp->fmt == CMIT_FMT_MBOXRD && !mboxrd_from)\n> > + mboxrd_from = mboxrd_prepare();\n> > +\n> > @@ -1725,8 +1749,13 @@ void pp_remainder(struct pretty_print_context *pp,\n> > else if (pp->expand_tabs_in_log)\n> > strbuf_add_tabexpand(sb, pp->expand_tabs_in_log,\n> > line, linelen);\n> > - else\n> > + else {\n> > + if (pp->fmt == CMIT_FMT_MBOXRD &&\n> > + !regexec(mboxrd_from, line, 0, 0, 0))\n> > + strbuf_addch(sb, '>');\n> \n> At first glance, this seems dangerous since it's handing 'line' to\n> regexec() without paying attention to 'linelen'. For an arbitrary\n> regex, this could result in erroneous matches on subsequent \"lines\",\n> however, since this expression is anchored with '^', it's not a\n> problem. But, it is a bit subtle.\n\nMaybe having more context of the pp_remainder function and\nseeing the get_one_line call would've helped in the diff;\nI didn't think of this issue once I figured out where to\nplace the change.\n\nOn the other hand, not being too familiar with git C APIs, I was\nmore worried strbuf was not NUL-terminated for regexec, but it\nseems to be.\n\n> I wonder if hand-coding, rather than using a regex, could be an improvement:\n> \n> static int is_mboxrd_from(const char *s, size_t n)\n> {\n> size_t f = strlen(\"From \");\n> const char *t = s + n;\n> \n> while (s < t && *s == '>')\n> s++;\n> return t - s >= f && !memcmp(s, \"From \", f);\n> }\n> \n> or something.\n\nYikes. I mostly work in high-level languages and do my best to\navoid string parsing in C; so that scares me. A lot.\n\nI admit a regex isn't necessary, but I'm wondering if the above\ncould be made less frightening to someone like me.\n\nOr maybe I'm just easily frightened :x\n\nMaybe extra test cases + valgrind can quell my fears :)\n\n> > diff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh\n> > @@ -1565,4 +1565,31 @@ test_expect_success 'format-patch --base overrides format.useAutoBase' '\n> > +test_expect_success 'format-patch --pretty=mboxrd' '\n> > + cat >msg <<-INPUT_END &&\n> \n> Maybe use <<-\\INPUT_END to indicate that no variable interpolation is\n> expected. Ditto below.\n\nNoted, though I may add variable interpolation to test a line\nwith trailing whitespace (just \"From \") to ensure we don't\noverrun a buffer.\n\n> Hmm, -A is not POSIX and is otherwise not used in Git tests. Perhaps\n> you could use 'git grep --no-index -A' instead or something?\n\nNoted and will fix for v2. Will wait a day or two for\nfurther comments on this series.\n"},{"id":"287922","messageId":"CAPig+cQrQfxWrGhpvtb-GKYfK0tMLsx9JJ+eWRRx00F8mNXrLg@mail.gmail.com","threadId":"42491","inReplyTo":"20160531074506.GA8911@dcvr.yhbt.net","subject":"Re: [PATCH 1/3] pretty: support \"mboxrd\" output format","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2016-05-31T18:10:03Z","receivedAt":"2016-05-31T18:10:03Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, May 31, 2016 at 3:45 AM, Eric Wong wrote:\n> Eric Sunshine wrote:\n>> On Mon, May 30, 2016 at 7:21 PM, Eric Wong wrote:\n>> > + if (pp->fmt == CMIT_FMT_MBOXRD &&\n>> > + !regexec(mboxrd_from, line, 0, 0, 0))\n>> > + strbuf_addch(sb, '>');\n>>\n>> At first glance, this seems dangerous since it's handing 'line' to\n>> regexec() without paying attention to 'linelen'. For an arbitrary\n>> regex, this could result in erroneous matches on subsequent \"lines\",\n>> however, since this expression is anchored with '^', it's not a\n>> problem. But, it is a bit subtle.\n>\n> Maybe having more context of the pp_remainder function and\n> seeing the get_one_line call would've helped in the diff;\n> I didn't think of this issue once I figured out where to\n> place the change.\n\nNo, extra context wouldn't have helped. The problem is that\nget_one_line() merely returns the length of the line, which might be\nwhere the NUL-terminator is or it might be where the next newline is.\nTherefore, you can't rely upon the \"current line\" being\nNUL-terminated. So, in general, it's not \"safe\" to pass it to a\nfunction expecting the \"line\" to end at a NUL-terminator.\n\n> On the other hand, not being too familiar with git C APIs, I was\n> more worried strbuf was not NUL-terminated for regexec, but it\n> seems to be.\n\nYes, that's a guarantee, but it doesn't help in this case. Given\nline=\"foo\\nbar\", get_one_line(line) will return 4, the length of\n\"foo\\n\", but regexec() won't know to stop looking until it hits the\nNUL after the 'r'. An arbitrary regex, such as /bar/ will match beyond\nwhat get_one_line() considers the end-of-line, which is why this code\nlooks scary (wrong) at first glance.\n\n>> I wonder if hand-coding, rather than using a regex, could be an improvement:\n>>\n>> static int is_mboxrd_from(const char *s, size_t n)\n>> {\n>> size_t f = strlen(\"From \");\n>> const char *t = s + n;\n>>\n>> while (s < t && *s == '>')\n>> s++;\n>> return t - s >= f && !memcmp(s, \"From \", f);\n>> }\n>>\n>> or something.\n>\n> Yikes. I mostly work in high-level languages and do my best to\n> avoid string parsing in C; so that scares me. A lot.\n\nThe hand-coded is_mboxrd_from() above is pretty much idiomatic C and\n(I think) typical of how such a function would be coded in Git itself,\nso it looks normal and easy to grok to me (but, of course, I'm\nprobably biased since I wrote it).\n\n> I admit a regex isn't necessary, but I'm wondering if the above\n> could be made less frightening to someone like me.\n\nPerhaps, but it's difficult to say without knowing how it frightens you.\n\n> Maybe extra test cases + valgrind can quell my fears :)\n\nI can envision tests such as:\n\n \"\"\n \"F\"\n \"From\"\n \"From \"\n \"From \"\n \"From foobar\"\n\nand so on, if that's what you mean.\n"},{"id":"287927","messageId":"20160531182932.GA27021@dcvr.yhbt.net","threadId":"42491","inReplyTo":"CAPig+cQrQfxWrGhpvtb-GKYfK0tMLsx9JJ+eWRRx00F8mNXrLg@mail.gmail.com","subject":"Re: [PATCH 1/3] pretty: support \"mboxrd\" output format","fromName":"Eric Wong","fromEmail":"e@80x24.org","sentAt":"2016-05-31T18:29:32Z","receivedAt":"2016-05-31T18:29:32Z","isPatch":true,"sender":{"key":"e@80x24.org","avatar":null},"body":"Eric Sunshine wrote:\n> On Tue, May 31, 2016 at 3:45 AM, Eric Wong wrote:\n> > Eric Sunshine wrote:\n\n Ah thanks, I can see your point-of-view, now.\nI always had the '^' in my mind since I've written the same\nthing in Perl and Ruby.\n\n> >> I wonder if hand-coding, rather than using a regex, could be an improvement:\n> >>\n> >> static int is_mboxrd_from(const char *s, size_t n)\n> >> {\n> >> size_t f = strlen(\"From \");\n> >> const char *t = s + n;\n> >>\n> >> while (s < t && *s == '>')\n> >> s++;\n> >> return t - s >= f && !memcmp(s, \"From \", f);\n> >> }\n> >>\n> >> or something.\n> >\n> > Yikes. I mostly work in high-level languages and do my best to\n> > avoid string parsing in C; so that scares me. A lot.\n> \n> The hand-coded is_mboxrd_from() above is pretty much idiomatic C and\n> (I think) typical of how such a function would be coded in Git itself,\n> so it looks normal and easy to grok to me (but, of course, I'm\n> probably biased since I wrote it).\n> \n> > I admit a regex isn't necessary, but I'm wondering if the above\n> > could be made less frightening to someone like me.\n> \n> Perhaps, but it's difficult to say without knowing how it frightens you.\n\nPointer arithmetic leading to buffer overruns;\nbut yeah, I don't do string parsing in C often, if ever.\n\n> > Maybe extra test cases + valgrind can quell my fears :)\n> \n> I can envision tests such as:\n> \n> \"\"\n> \"F\"\n> \"From\"\n> \"From \"\n> \"From \"\n> \"From foobar\"\n> \n> and so on, if that's what you mean.\n\nYes, I also noticed trailing spaces are dropped anyways, so\nthere's no perfect round-tripping going on.\n"},{"id":"287931","messageId":"CAPig+cSrqs7W2hSz6Z4_O9U52gUJFCqTSfRL0E0b=d=bO9RCEQ@mail.gmail.com","threadId":"42491","inReplyTo":"20160531182932.GA27021@dcvr.yhbt.net","subject":"Re: [PATCH 1/3] pretty: support \"mboxrd\" output format","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2016-05-31T20:12:16Z","receivedAt":"2016-05-31T20:12:16Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, May 31, 2016 at 2:29 PM, Eric Wong wrote:\n> Eric Sunshine wrote:\n>> On Tue, May 31, 2016 at 3:45 AM, Eric Wong wrote:\n>> > Eric Sunshine wrote:\n> Ah thanks, I can see your point-of-view, now.\n> I always had the '^' in my mind since I've written the same\n> thing in Perl and Ruby.\n\nOn reflection, even with the '^' anchor, it isn't safe the way it's\ncoded since '^' will match following a newline, won't it? Therefore,\nbecause 'line' isn't necessarily NUL-terminated, the pattern could\nmatch on some line beyond what get_one_line() considers the \"current\nline\".\n\nThis makes the hand-coded is_mboxrd_from() even more attractive; plus\nyou can re-use it in patch 2.\n"},{"id":"287933","messageId":"CAPig+cTycj-N3FtkvRzWbm+UdYtQWBb6BCZH-798VLi+jOxy_Q@mail.gmail.com","threadId":"42491","inReplyTo":"CAPig+cSrqs7W2hSz6Z4_O9U52gUJFCqTSfRL0E0b=d=bO9RCEQ@mail.gmail.com","subject":"Re: [PATCH 1/3] pretty: support \"mboxrd\" output format","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2016-05-31T20:19:39Z","receivedAt":"2016-05-31T20:19:39Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, May 31, 2016 at 4:12 PM, Eric Sunshine wrote:\n> On reflection, even with the '^' anchor, it isn't safe the way it's\n> coded since '^' will match following a newline, won't it? Therefore,\n> because 'line' isn't necessarily NUL-terminated, the pattern could\n> match on some line beyond what get_one_line() considers the \"current\n> line\".\n\nI guess that behavior only kicks in if you specify REG_NEWLINE, which\nyou didn't.\n\n> This makes the hand-coded is_mboxrd_from() even more attractive; plus\n> you can re-use it in patch 2.\n"},{"id":"288109","messageId":"20160602075125.GA19551@dcvr.yhbt.net","threadId":"42491","inReplyTo":"20160531182932.GA27021@dcvr.yhbt.net","subject":"Re: [PATCH 1/3] pretty: support \"mboxrd\" output format","fromName":"Eric Wong","fromEmail":"e@80x24.org","sentAt":"2016-06-02T07:51:25Z","receivedAt":"2016-06-02T07:51:25Z","isPatch":true,"sender":{"key":"e@80x24.org","avatar":null},"body":"Eric Wong wrote:\n> Eric Sunshine wrote:\n> > On Tue, May 31, 2016 at 3:45 AM, Eric Wong wrote:\n> > > Eric Sunshine wrote:\n> > >> I wonder if hand-coding, rather than using a regex, could be an improvement:\n> > >>\n> > >> static int is_mboxrd_from(const char *s, size_t n)\n> > >> {\n> > >> size_t f = strlen(\"From \");\n> > >> const char *t = s + n;\n> > >>\n> > >> while (s < t && *s == '>')\n> > >> s++;\n> > >> return t - s >= f && !memcmp(s, \"From \", f);\n> > >> }\n> > >>\n> > >> or something.\n> > >\n> > > Yikes. I mostly work in high-level languages and do my best to\n> > > avoid string parsing in C; so that scares me. A lot.\n> > \n> > The hand-coded is_mboxrd_from() above is pretty much idiomatic C and\n> > (I think) typical of how such a function would be coded in Git itself,\n> > so it looks normal and easy to grok to me (but, of course, I'm\n> > probably biased since I wrote it).\n\nFor reference, here is the gfrom function from qmail (gfrom.c,\nsource package netqmail-1.06 in Debian, reformatted git style)\n\nint gfrom(char *s, int len)\n{\n\twhile ((len > 0) && (*s == '>')) {\n\t\t++s;\n\t\t--len;\n\t}\n\n\treturn (len >= 5) && !str_diffn(s, \"From \", 5);\n}\n\nSimilar to yours, but a several small things improves\nreadability for me:\n\n* the avoidance of subtraction from the \"return\" conditional\n* s/n/len/ variable name\n* extra parentheses\n* removal of \"t\" variable (t for \"terminal/termination\"?)\n\nstr_diffn is memcmp-like, I assume. My eyes glazed over\nwhen I saw that function implemented in str_diffn.c, too.\n\nJust thinking out loud, with sufficient tests I could go with\neither. Will reroll when/if I get the chance tomorrow.\n"},{"id":"288341","messageId":"CAPig+cTLVXJBn00aco0vC9oFvZuchTgtNXtGGjpLPx1LwRZz4Q@mail.gmail.com","threadId":"42491","inReplyTo":"20160602075125.GA19551@dcvr.yhbt.net","subject":"Re: [PATCH 1/3] pretty: support \"mboxrd\" output format","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2016-06-03T22:22:02Z","receivedAt":"2016-06-03T22:22:02Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"[cc:+junio]\n\nOn Thu, Jun 2, 2016 at 3:51 AM, Eric Wong wrote:\n> Eric Wong wrote:\n>> Eric Sunshine wrote:\n>> > On Tue, May 31, 2016 at 3:45 AM, Eric Wong wrote:\n>> > > Eric Sunshine wrote:\n>> > >> I wonder if hand-coding, rather than using a regex, could be an improvement:\n>> > >>\n>> > >> static int is_mboxrd_from(const char *s, size_t n)\n>> > >> {\n>> > >> size_t f = strlen(\"From \");\n>> > >> const char *t = s + n;\n>> > >>\n>> > >> while (s < t && *s == '>')\n>> > >> s++;\n>> > >> return t - s >= f && !memcmp(s, \"From \", f);\n>> > >> }\n>> > >>\n>> > >> or something.\n>> > >\n>> > > Yikes. I mostly work in high-level languages and do my best to\n>> > > avoid string parsing in C; so that scares me. A lot.\n>> >\n>> > The hand-coded is_mboxrd_from() above is pretty much idiomatic C and\n>> > (I think) typical of how such a function would be coded in Git itself,\n>> > so it looks normal and easy to grok to me (but, of course, I'm\n>> > probably biased since I wrote it).\n>\n> For reference, here is the gfrom function from qmail (gfrom.c,\n> source package netqmail-1.06 in Debian, reformatted git style)\n>\n> int gfrom(char *s, int len)\n> {\n> while ((len > 0) && (*s == '>')) {\n> ++s;\n> --len;\n> }\n>\n> return (len >= 5) && !str_diffn(s, \"From \", 5);\n> }\n\nSeems less idiomatic and less like what we might see elsewhere in the\nGit codebase, but that's subjective. Functionally, it appears correct.\n\n> Similar to yours, but a several small things improves\n> readability for me:\n>\n> * the avoidance of subtraction from the \"return\" conditional\n> * s/n/len/ variable name\n\nIdiomatic C code favors concise names such as 'i', 'j', or 'n', for\ninstance, but I don't care strongly.\n\n> * extra parentheses\n\nUnnecessary syntactic noise (consuming reviewer brain cycles).\n\n> * removal of \"t\" variable (t for \"terminal/termination\"?)\n\nHeh, no, just the next letter after 's'. Again, just an idiom, as 'i',\n'j', 'k' are often used for integers, 's' and 't' are common for\nstrings.\n\n> str_diffn is memcmp-like, I assume. My eyes glazed over\n> when I saw that function implemented in str_diffn.c, too.\n>\n> Just thinking out loud, with sufficient tests I could go with\n> either. Will reroll when/if I get the chance tomorrow.\n\nAs mentioned above, it's all subjective and, of course, I have a bias\ntoward the example I provided, but don't otherwise feel strongly about\nit. I do, however, like the idea of using a simple hand-coded matching\nfunction over the regex (but no so much that I would complain about\nit). Use whatever you and Junio feel is appropriate.\n"},{"id":"288342","messageId":"xmqqlh2loq6p.fsf@gitster.mtv.corp.google.com","threadId":"42491","inReplyTo":"CAPig+cTLVXJBn00aco0vC9oFvZuchTgtNXtGGjpLPx1LwRZz4Q@mail.gmail.com","subject":"Re: [PATCH 1/3] pretty: support \"mboxrd\" output format","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-06-03T22:36:30Z","receivedAt":"2016-06-03T22:36:30Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Sunshine writes:\n\n> On Thu, Jun 2, 2016 at 3:51 AM, Eric Wong wrote:\n>> Eric Wong wrote:\n>>> Eric Sunshine wrote:\n>>> > On Tue, May 31, 2016 at 3:45 AM, Eric Wong wrote:\n>>> > > Eric Sunshine wrote:\n>>> > >> I wonder if hand-coding, rather than using a regex, could be an improvement:\n>>> > >>\n>>> > >> static int is_mboxrd_from(const char *s, size_t n)\n>>> > >> {\n>>> > >> size_t f = strlen(\"From \");\n>>> > >> const char *t = s + n;\n>>> > >>\n>>> > >> while (s < t && *s == '>')\n>>> > >> s++;\n>>> > >> return t - s >= f && !memcmp(s, \"From \", f);\n>>> > >> }\n>>> > >>\n>>> > >> or something.\n>>> > >\n>>> > > Yikes. I mostly work in high-level languages and do my best to\n>>> > > avoid string parsing in C; so that scares me. A lot.\n>\n> As mentioned above, it's all subjective and, of course, I have a bias\n> toward the example I provided, but don't otherwise feel strongly about\n> it. I do, however, like the idea of using a simple hand-coded matching\n> function over the regex (but no so much that I would complain about\n> it). Use whatever you and Junio feel is appropriate.\n\nThis is meant to be a replacement for the original that uses\nregexec(), which in turn means the string we are checking is\nguaranteed to be NUL terminated, right?\n\n\tstatic int is_mboxrd_from(const char *line) {\n\t return starts_with(line + strspn(line, \">\"), \"From \");\n\t}\n\nis sufficiently high-level that no longer is scary, hopefully?\n\nI agree with you that regexec() is way overkill for something small\nand simple like this.\n"},{"id":"288345","messageId":"CAPig+cQXA3mgYC3rCTx2h93rB6djdKvYJ+oCXUP24-xk2PqW=Q@mail.gmail.com","threadId":"42491","inReplyTo":"xmqqlh2loq6p.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 1/3] pretty: support \"mboxrd\" output format","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2016-06-03T22:59:46Z","receivedAt":"2016-06-03T22:59:46Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Fri, Jun 3, 2016 at 6:36 PM, Junio C Hamano wrote:\n> Eric Sunshine writes:\n>> On Thu, Jun 2, 2016 at 3:51 AM, Eric Wong wrote:\n>>> Eric Wong wrote:\n>>>> Eric Sunshine wrote:\n>>>> > On Tue, May 31, 2016 at 3:45 AM, Eric Wong wrote:\n>>>> > > Eric Sunshine wrote:\n>>>> > >> I wonder if hand-coding, rather than using a regex, could be an improvement:\n>>>> > >>\n>>>> > >> static int is_mboxrd_from(const char *s, size_t n)\n>>>> > >> {\n>>>> > >> size_t f = strlen(\"From \");\n>>>> > >> const char *t = s + n;\n>>>> > >>\n>>>> > >> while (s < t && *s == '>')\n>>>> > >> s++;\n>>>> > >> return t - s >= f && !memcmp(s, \"From \", f);\n>>>> > >> }\n>>>> > >>\n>>>> > >> or something.\n>>>> > >\n>>>> > > Yikes. I mostly work in high-level languages and do my best to\n>>>> > > avoid string parsing in C; so that scares me. A lot.\n>>\n>> As mentioned above, it's all subjective and, of course, I have a bias\n>> toward the example I provided, but don't otherwise feel strongly about\n>> it. I do, however, like the idea of using a simple hand-coded matching\n>> function over the regex (but no so much that I would complain about\n>> it). Use whatever you and Junio feel is appropriate.\n>\n> This is meant to be a replacement for the original that uses\n> regexec(), which in turn means the string we are checking is\n> guaranteed to be NUL terminated, right?\n\nYes, this is meant as a replacement for the regexec(), but no, there\nis no guarantee that the \"line\" is NUL-terminated. In this case, the\n'line' in pretty.c:pp_remainder() might end at a NUL or it might just\nend at the next newline in the larger buffer.\n\nThe bit about sending the \"line\" to regexec() without taking its\nlogical end-of-line into account was what caught my attention in the\nfirst place.\n\n> static int is_mboxrd_from(const char *line) {\n> return starts_with(line + strspn(line, \">\"), \"From \");\n> }\n>\n> is sufficiently high-level that no longer is scary, hopefully?\n\nThat's nice and concise but unfortunately not useful for this case\nwhere we must respect the logical end-of-line (within the larger\nbuffer), represented by line+linelen.\n"},{"id":"288351","messageId":"xmqqzir1zvo2.fsf@gitster.mtv.corp.google.com","threadId":"42491","inReplyTo":"CAPig+cQXA3mgYC3rCTx2h93rB6djdKvYJ+oCXUP24-xk2PqW=Q@mail.gmail.com","subject":"Re: [PATCH 1/3] pretty: support \"mboxrd\" output format","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-06-03T23:42:37Z","receivedAt":"2016-06-03T23:42:37Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Sunshine writes:\n\n>> static int is_mboxrd_from(const char *line) {\n>> return starts_with(line + strspn(line, \">\"), \"From \");\n>> }\n>>\n>> is sufficiently high-level that no longer is scary, hopefully?\n>\n> That's nice and concise but unfortunately not useful for this case\n> where we must respect the logical end-of-line (within the larger\n> buffer), represented by line+linelen.\n\nHmph, none of \">\", \"F\", \"r\", \"o\", \"m\", or \" \" is \"\\n\"; would eol\nstill be an issue?\n"},{"id":"288355","messageId":"CAPig+cTQkEb6K95aeJ9vNyJ=S+8x+JnCQAYSEzydXKzbq-+Yyg@mail.gmail.com","threadId":"42491","inReplyTo":"xmqqzir1zvo2.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 1/3] pretty: support \"mboxrd\" output format","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2016-06-04T00:02:25Z","receivedAt":"2016-06-04T00:02:25Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Fri, Jun 3, 2016 at 7:42 PM, Junio C Hamano wrote:\n> Eric Sunshine writes:\n>\n>>> static int is_mboxrd_from(const char *line) {\n>>> return starts_with(line + strspn(line, \">\"), \"From \");\n>>> }\n>>>\n>>> is sufficiently high-level that no longer is scary, hopefully?\n>>\n>> That's nice and concise but unfortunately not useful for this case\n>> where we must respect the logical end-of-line (within the larger\n>> buffer), represented by line+linelen.\n>\n> Hmph, none of \">\", \"F\", \"r\", \"o\", \"m\", or \" \" is \"\\n\"; would eol\n> still be an issue?\n\nAh right. Sorry for being slow.\n\nYour concise version of is_mboxrd_from() is a nice alternative, as well.\n"},{"id":"288356","messageId":"CAPig+cQ7y7J02+PVtdJywGEudOGXsy1jgadvVwBg5creiUv6zA@mail.gmail.com","threadId":"42491","inReplyTo":"CAPig+cTQkEb6K95aeJ9vNyJ=S+8x+JnCQAYSEzydXKzbq-+Yyg@mail.gmail.com","subject":"Re: [PATCH 1/3] pretty: support \"mboxrd\" output format","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2016-06-04T00:19:07Z","receivedAt":"2016-06-04T00:19:07Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Fri, Jun 3, 2016 at 8:02 PM, Eric Sunshine wrote:\n> On Fri, Jun 3, 2016 at 7:42 PM, Junio C Hamano wrote:\n>> Eric Sunshine writes:\n>>\n>>>> static int is_mboxrd_from(const char *line) {\n>>>> return starts_with(line + strspn(line, \">\"), \"From \");\n>>>> }\n>>>>\n>>>> is sufficiently high-level that no longer is scary, hopefully?\n>>>\n>>> That's nice and concise but unfortunately not useful for this case\n>>> where we must respect the logical end-of-line (within the larger\n>>> buffer), represented by line+linelen.\n>>\n>> Hmph, none of \">\", \"F\", \"r\", \"o\", \"m\", or \" \" is \"\\n\"; would eol\n>> still be an issue?\n>\n> Ah right. Sorry for being slow.\n>\n> Your concise version of is_mboxrd_from() is a nice alternative, as well.\n\nMy only minor reservation is that it your concise version is still\nsubtle with regard to not taking 'linelen' into account. At first\nglance, it looks like a bug that it doesn't consider the logical\nend-of-line, and someone reading the code has to put in extra effort\nto convince himself that the code works as intended.\n\nFor that reason, I have a bit of a preference for a version of\nis_mboxrd_from() which does take 'linelen' into account explicitly.\n"},{"id":"288359","messageId":"CAPc5daWJ1v8EU89UN2HPK8=U17vz4LiANA9FyCvH5as7Hb1bTQ@mail.gmail.com","threadId":"42491","inReplyTo":"CAPig+cQ7y7J02+PVtdJywGEudOGXsy1jgadvVwBg5creiUv6zA@mail.gmail.com","subject":"Re: [PATCH 1/3] pretty: support \"mboxrd\" output format","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-06-04T02:03:59Z","receivedAt":"2016-06-04T02:03:59Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"On Fri, Jun 3, 2016 at 5:19 PM, Eric Sunshine wrote:\n>\n> My only minor reservation is that it your concise version is still\n> subtle with regard to not taking 'linelen' into account. At first\n> glance, it looks like a bug that it doesn't consider the logical\n> end-of-line, and someone reading the code has to put in extra effort\n> to convince himself that the code works as intended.\n>\n> For that reason, I have a bit of a preference for a version of\n> is_mboxrd_from() which does take 'linelen' into account explicitly.\n\nThat certainly is true. You could check that the end result did not consume\nmore than linelen after the single-liner computes its result, but I am not sure\nunder what condition that check would trigger. Even though we may hold the\nentire e-mail file without NUL-terminating each line, we have a NUL-termination\nat the end of the file, no?\n"}]}