{"thread":{"id":"24048","subject":"[PATCH] format-patch: Properly escape From_ lines when creating an mbox.","startedAt":"2010-06-09T01:01:45Z","lastAt":"2010-06-10T16:30:00Z","messageCount":11,"participants":["Carl Worth","Junio C Hamano","H. Peter Anvin"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"143300","messageId":"1276045305-20743-1-git-send-email-cworth@cworth.org","threadId":"24048","inReplyTo":null,"subject":"[PATCH] format-patch: Properly escape From_ lines when creating an mbox.","fromName":"Carl Worth","fromEmail":"cworth@cworth.org","sentAt":"2010-06-09T01:01:45Z","receivedAt":"2010-06-09T01:01:45Z","isPatch":true,"sender":{"key":"cworth@cworth.org","avatar":"https://gravatar.com/avatar/3746dc28cde609bdbd7f939058356e7e2bbd16d21e32274df0725eb3d998bc5b?d=mp&s=160"},"body":"This uses the mboxrd style of quoting as documented here:\nhttp://homepage.ntlworld.com/jonathan.deboynepollard/FGA/mail-mbox-formats.html\n\nThis matches the mboxrd-style un-escaping recently added to \"git am\".\n\nTest 4152 is now extended to verify that format-patch does the proper\nescaping. It also now relies directly on the escaped output from\nformat-patch to verify that \"git am\" does the proper unescaping,\n(where previously, it faked the escaped output with sed).\n\nSigned-off-by: Carl Worth \n---\n\nThis patch is on top of the three patches I sent earlier. With this patch,\nthe series to make git use mbox in a robust fashion (and to avoid using mbox\nwhere possible) is complete.\n\nThe entire test suite passes, and new tests are added for all new\nfunctionality.\n\nAll of the features and caveats I mentioned earlier are taken care of. The\nonly potentially missing piece is that git-send-email doesn't have code\nto un-escape From_ lines in an mbox. But this is irrelevant since send-\nemail doesn't even know how to handle an mbox anyway, (it will treat it\nas one large email message instead).\n\nWithout this patch series, there's no documented way that an external\ntool can use to reliably construct an mbox that will be correctly handled\nby \"git am\". The best one could do is to peek inside the git implementation\nand notice that it wants unescaped \"From \" lines, that it will ignore any\n\"From \" line that doesn't end with something very much like asctime format,\nand then somehow ensure that no messages in the mbox have lines that begin\nwith \"From \" and end with something like asctime format, (which won't be\npossible in all cases without corrupting the message).\n\nWith this patch series, one can instead document that \"git am\" accepts an\nmbox in \"mboxrd\" format as documented at the URL above, but with the caveat\nthat no additional characters are allowed after the asctime portion of the\n\"From \" line. This requirement allows git to continue to accept mbox files\ncreated by old versions of git, (with a very minor chance of corruption).\n\nMbox files create and consumed by versions of git after this patch series\nshould have no corruption by design.\n\n builtin/log.c | 2 +-\n commit.h | 5 ++-\n log-tree.c | 1 +\n pretty.c | 14 +++++++++++-\n ....sh => t4152-format-patch-am-From_-escaping.sh} | 21 ++++++++++++-------\n 5 files changed, 30 insertions(+), 13 deletions(-)\n rename t/{t4152-am-From_.sh => t4152-format-patch-am-From_-escaping.sh} (78%)\n mode change 100755 => 100644\n\ndiff --git a/builtin/log.c b/builtin/log.c\nindex adbec9f..36b2f5a 100644\n--- a/builtin/log.c\n+++ b/builtin/log.c\n@@ -763,7 +763,7 @@ static void make_cover_letter(struct rev_info *rev, int use_stdout,\n \t\t encoding);\n \tpp_title_line(CMIT_FMT_EMAIL, &msg, &sb, subject_start, extra_headers,\n \t\t encoding, need_8bit_cte);\n-\tpp_remainder(CMIT_FMT_EMAIL, &msg, &sb, 0);\n+\tpp_remainder(CMIT_FMT_EMAIL, &msg, &sb, 0, 0);\n \tprintf(\"%s\\n\", sb.buf);\n \n \tstrbuf_release(&sb);\ndiff --git a/commit.h b/commit.h\nindex 6ef88dc..18e7197 100644\n--- a/commit.h\n+++ b/commit.h\n@@ -70,6 +70,7 @@ struct pretty_print_context\n \tconst char *after_subject;\n \tenum date_mode date_mode;\n \tint need_8bit_cte;\n+\tint need_from_escaping;\n \tint show_notes;\n \tstruct reflog_walk_info *reflog_info;\n };\n@@ -103,8 +104,8 @@ void pp_title_line(enum cmit_fmt fmt,\n void pp_remainder(enum cmit_fmt fmt,\n \t\t const char **msg_p,\n \t\t struct strbuf *sb,\n-\t\t int indent);\n-\n+\t\t int indent,\n+\t\t int need_from_escaping);\n \n /** Removes the first commit from a list sorted by date, and adds all\n * of its parents.\ndiff --git a/log-tree.c b/log-tree.c\nindex 6aab273..1a179dc 100644\n--- a/log-tree.c\n+++ b/log-tree.c\n@@ -415,6 +415,7 @@ void show_log(struct rev_info *opt)\n \tctx.abbrev = opt->diffopt.abbrev;\n \tctx.after_subject = extra_headers;\n \tctx.reflog_info = opt->reflog_info;\n+\tctx.need_from_escaping = opt->format_mbox;\n \tpretty_print_commit(opt->commit_format, commit, &msgbuf, &ctx);\n \n \tif (opt->add_signoff)\ndiff --git a/pretty.c b/pretty.c\nindex 74cda1b..62b376b 100644\n--- a/pretty.c\n+++ b/pretty.c\n@@ -1011,7 +1011,8 @@ void pp_title_line(enum cmit_fmt fmt,\n void pp_remainder(enum cmit_fmt fmt,\n \t\t const char **msg_p,\n \t\t struct strbuf *sb,\n-\t\t int indent)\n+\t\t int indent,\n+\t\t int need_from_escaping)\n {\n \tint first = 1;\n \tfor (;;) {\n@@ -1030,6 +1031,15 @@ void pp_remainder(enum cmit_fmt fmt,\n \t\t}\n \t\tfirst = 0;\n \n+\t\tif (need_from_escaping && (*line == '>' || *line == 'F'))\n+\t\t{\n+\t\t\tconst char *s = line;\n+\t\t\twhile (*s == '>')\n+\t\t\t\ts++;\n+\t\t\tif (strncmp (s, \"From \", 5) == 0)\n+\t\t\t\tstrbuf_addch(sb, '>');\n+\t\t}\n+\n \t\tstrbuf_grow(sb, linelen + indent + 20);\n \t\tif (indent) {\n \t\t\tmemset(sb->buf + sb->len, ' ', indent);\n@@ -1117,7 +1127,7 @@ void pretty_print_commit(enum cmit_fmt fmt, const struct commit *commit,\n \n \tbeginning_of_body = sb->len;\n \tif (fmt != CMIT_FMT_ONELINE)\n-\t\tpp_remainder(fmt, &msg, sb, indent);\n+\t\tpp_remainder(fmt, &msg, sb, indent, context->need_from_escaping);\n \tstrbuf_rtrim(sb);\n \n \t/* Make sure there is an EOLN for the non-oneline case */\ndiff --git a/t/t4152-am-From_.sh b/t/t4152-format-patch-am-From_-escaping.sh\nold mode 100755\nnew mode 100644\nsimilarity index 78%\nrename from t/t4152-am-From_.sh\nrename to t/t4152-format-patch-am-From_-escaping.sh\nindex 02821ee..dc013bb\n--- a/t/t4152-am-From_.sh\n+++ b/t/t4152-format-patch-am-From_-escaping.sh\n@@ -34,13 +34,18 @@ test_expect_success setup '\n \ttest_tick &&\n \tgit commit -s -F msg &&\n \tgit tag second &&\n-\tgit format-patch --stdout first | sed -e \"1{p;d};s/^\\(>*From \\)/>\\1/\" > From_ &&\n-\t{\n-\t\techo \"X-Fake-Field: Line One\" &&\n-\t\techo \"X-Fake-Field: Line Two\" &&\n-\t\techo \"X-Fake-Field: Line Three\" &&\n-\t\tgit format-patch --stdout first | sed -e \"1d\"\n-\t} > From_.eml\n+\tgit format-patch --stdout first > From_ &&\n+\tgit format-patch first\n+'\n+\n+test_expect_success 'format-patch escapes From_ lines in mbox' '\n+\thead -1 From_ | grep \"^From \" &&\n+\ttest \"$(grep \"^From \" From_ | wc -l)\" = \"1\"\n+'\n+\n+test_expect_success 'format-patch does not escapes From_ lines in email' '\n+\thead -1 0001-From_-lines.patch | grep -v \"^From \" >/dev/null &&\n+\ttest \"$(grep \"^From \" 0001-From_-lines.patch | wc -l)\" = \"1\"\n '\n \n test_expect_success 'am unescapes From_ lines from mbox' '\n@@ -54,7 +59,7 @@ test_expect_success 'am unescapes From_ lines from mbox' '\n \n test_expect_success 'am does not unescape From_ lines from email' '\n \tgit checkout first &&\n-\tgit am From_.eml &&\n+\tgit am 0001-From_-lines.patch &&\n \t! test -d .git/rebase-apply &&\n \ttest -z \"$(git diff second)\" &&\n \ttest \"$(git rev-parse second)\" = \"$(git rev-parse HEAD)\" &&\n-- \n1.7.0.4\n"},{"id":"143301","messageId":"7vljaorhjq.fsf@alter.siamese.dyndns.org","threadId":"24048","inReplyTo":"1276045305-20743-1-git-send-email-cworth@cworth.org","subject":"Re: [PATCH] format-patch: Properly escape From_ lines when creating an mbox.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-06-09T03:50:01Z","receivedAt":"2010-06-09T03:50:01Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Carl Worth writes:\n\n> Without this patch series, there's no documented way that an external\n> tool can use to reliably construct an mbox that will be correctly handled\n> by \"git am\". The best one could do is to peek inside the git implementation\n> and notice that it wants unescaped \"From \" lines, that it will ignore any\n> \"From \" line that doesn't end with something very much like asctime format,\n> and then somehow ensure that no messages in the mbox have lines that begin\n> with \"From \" and end with something like asctime format, (which won't be\n> possible in all cases without corrupting the message).\n\nI have this small suspicion that mboxrd may be a suboptimal choice, when\nyou consider how robustly we can notice a failure (and to a lessor extent,\nrecover from it) when using output from \"format-patch --stdout\" to\nsneakernet between existing and updated versions of git. Especially\nbecause your implementation quotes lines that begin with \"From \"\nunconditionally (even when the tail end of the line would never be a\nvalid-looking timestamp). Such an output will confuse existing mailsplit,\nbut the worst part of the story is that somebody who is applying a series\nof patches will _not_ notice the breakage. The payload of the second and\nsubsequent messages will likely be concatenated as if it were part of the\nfirst message, ignoring cruft between patches, but the resulting tree\nwould likely to be the same as what the sending end intended.\n\nCompared to that, I think a failure to split a message in the middle (iow,\ncommit message happened to have a line that begins with \"From \" and ends\nwith a timestamp-looking string) is much easier to notice (because the\nfirst part of the message that was incorrectly split at such a line will\nnot have any patch, so \"git am\" will stop). IOW, failure to split is\neasier to notice than splitting too eagerly.\n\nPerhaps perfect is an enemy of good?\n"},{"id":"143302","messageId":"87eiggiy8g.fsf@yoom.home.cworth.org","threadId":"24048","inReplyTo":"7vljaorhjq.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] format-patch: Properly escape From_ lines when creating an mbox.","fromName":"Carl Worth","fromEmail":"cworth@cworth.org","sentAt":"2010-06-09T05:14:23Z","receivedAt":"2010-06-09T05:14:23Z","isPatch":true,"sender":{"key":"cworth@cworth.org","avatar":"https://gravatar.com/avatar/3746dc28cde609bdbd7f939058356e7e2bbd16d21e32274df0725eb3d998bc5b?d=mp&s=160"},"body":"On Tue, 08 Jun 2010 20:50:01 -0700, Junio C Hamano wrote:\n> Carl Worth writes:\n> Especially because your implementation quotes lines that begin with \"From \"\n> unconditionally (even when the tail end of the line would never be a\n> valid-looking timestamp). Such an output will confuse existing mailsplit,\n> but the worst part of the story is that somebody who is applying a series\n> of patches will _not_ notice the breakage. The payload of the second and\n> subsequent messages will likely be concatenated as if it were part of the\n> first message, ignoring cruft between patches, but the resulting tree\n> would likely to be the same as what the sending end intended.\n\nI agree that anything that results in multiple patches being (silently!)\nconcatenated would be catastrophic and I do not recommend accepting any\npatches that could result in failures like that.\n\nCould you describe in more detail how the implementation could lead to a\ncase like that? I'm not seeing it myself. But if you can show me, I'll\nbe happy to attempt a fix.\n\nIn particular, I don't see how any of the new quoting will confuse\nexisting mailsplit. The splitting itself shouldn't be changed. And at\nworst, using new \"git format-patch\" with old mailsplit could result in a\n\">From \" getting into a commit message where a \"From \" should be.\n\nWe could reduce the occurrence of that problem by being less aggressive\nwith \"From \" quoting, (for example, examining whether the tail of the\nline looks like a timestamp before quoting). The cost there would be\nfairly minor. It would increase the occurrence of a failure to pass a\n\">From \" correctly from a new \"git am\" to a new \"git mailsplit\". [*]\n\nI don't see a way to eliminate both problems other than specifying that\ngit's mbox format is a non-standard mbox format that looks specifically\nfor From_ lines ending in timestamps and is not capable of containing an\narbitrary message, (namely messages with lines that begin with \"From \"\nand end with timestamps).\n\nThat would be a particularly unsatisfying solution for me, since I'm\ntrying to implement an mbox-export option in a mail client as a general\nfeature (that happens to work with git) rather than implementing a\ngit-specific export option.\n\n-Carl\n\n[*] It would seem a strange strategy to make new git compatible with old\ngit while not being perfectly compatible with itself going forward, but\nthat is a possibility.\n\n-- \ncarl.d.worth@intel.com\n"},{"id":"143303","messageId":"4C0F2B3C.4060203@zytor.com","threadId":"24048","inReplyTo":"7vljaorhjq.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] format-patch: Properly escape From_ lines when creating an mbox.","fromName":"H. Peter Anvin","fromEmail":"hpa@zytor.com","sentAt":"2010-06-09T05:48:44Z","receivedAt":"2010-06-09T05:48:44Z","isPatch":true,"sender":{"key":"hpa@zytor.com","avatar":null},"body":"On 06/08/2010 08:50 PM, Junio C Hamano wrote:\n> Carl Worth writes:\n> \n>> Without this patch series, there's no documented way that an external\n>> tool can use to reliably construct an mbox that will be correctly handled\n>> by \"git am\". The best one could do is to peek inside the git implementation\n>> and notice that it wants unescaped \"From \" lines, that it will ignore any\n>> \"From \" line that doesn't end with something very much like asctime format,\n>> and then somehow ensure that no messages in the mbox have lines that begin\n>> with \"From \" and end with something like asctime format, (which won't be\n>> possible in all cases without corrupting the message).\n> \n> I have this small suspicion that mboxrd may be a suboptimal choice, when\n> you consider how robustly we can notice a failure (and to a lessor extent,\n> recover from it) when using output from \"format-patch --stdout\" to\n> sneakernet between existing and updated versions of git. Especially\n> because your implementation quotes lines that begin with \"From \"\n> unconditionally (even when the tail end of the line would never be a\n> valid-looking timestamp). Such an output will confuse existing mailsplit,\n> but the worst part of the story is that somebody who is applying a series\n> of patches will _not_ notice the breakage. The payload of the second and\n> subsequent messages will likely be concatenated as if it were part of the\n> first message, ignoring cruft between patches, but the resulting tree\n> would likely to be the same as what the sending end intended.\n> \n> Compared to that, I think a failure to split a message in the middle (iow,\n> commit message happened to have a line that begins with \"From \" and ends\n> with a timestamp-looking string) is much easier to notice (because the\n> first part of the message that was incorrectly split at such a line will\n> not have any patch, so \"git am\" will stop). IOW, failure to split is\n> easier to notice than splitting too eagerly.\n> \n> Perhaps perfect is an enemy of good?\n\nFor production perhaps we should do the MIME-escape thing?\n\nFor consumption, it's not so clear...\n\n\t-hpa\n\n-- \nH. Peter Anvin, Intel Open Source Technology Center\nI work for Intel. I don't speak on their behalf.\n"},{"id":"143307","messageId":"87bpbkit5l.fsf@yoom.home.cworth.org","threadId":"24048","inReplyTo":"4C0F2B3C.4060203@zytor.com","subject":"Re: [PATCH] format-patch: Properly escape From_ lines when creating an mbox.","fromName":"Carl Worth","fromEmail":"cworth@cworth.org","sentAt":"2010-06-09T07:04:06Z","receivedAt":"2010-06-09T07:04:06Z","isPatch":true,"sender":{"key":"cworth@cworth.org","avatar":"https://gravatar.com/avatar/3746dc28cde609bdbd7f939058356e7e2bbd16d21e32274df0725eb3d998bc5b?d=mp&s=160"},"body":"On Tue, 08 Jun 2010 22:48:44 -0700, \"H. Peter Anvin\" wrote:\n> > Perhaps perfect is an enemy of good?\n> \n> For production perhaps we should do the MIME-escape thing?\n> \n> For consumption, it's not so clear...\n\nI suggest as a first step accepting the following:\n\n\tformat-patch: Emit bare email rather than mbox for single messages.\n\t\n\nThat patch should be entirely uncontroversial since it doesn't introduce\nany new escaping, neither on the production nor on the consumption side.\n\nIt has the tremendous benefit of removing the mbox format entirely from\nthe \"git send-email\" workflow, (which will just use bare messages\ninstead).\n\nWith that patch in place, the only place that git will still generate\nmbox files is \"format-patch --stdout\". And the most common use of that\nis within git-rebase. For git-rebase, it doesn't matter what kind of\nmbox is used as long as it's consistent, since it's practically\nguaranteed that git-rebase will be using consistent versions of both\n\"git format-patch\" and \"git am\".\n\nAt that point, I think discussion of confusion from new format-patch and\nold am becomes almost meaningless as such interaction will most likely\nbe happening through bare messages rather than mbox files. When an mbox\nfile *is* involved I think it will be even more likely to happen through\nsome external program, (such as an MUA collecting a thread of\ngit-send-email messages and presenting them to \"git am\" as an mbox).\n\n-Carl\n\n-- \ncarl.d.worth@intel.com\n"},{"id":"143356","messageId":"877hm8i1qd.fsf@yoom.home.cworth.org","threadId":"24048","inReplyTo":"87eiggiy8g.fsf@yoom.home.cworth.org","subject":"Re: [PATCH] format-patch: Properly escape From_ lines when creating an mbox.","fromName":"Carl Worth","fromEmail":"cworth@cworth.org","sentAt":"2010-06-09T16:56:26Z","receivedAt":"2010-06-09T16:56:26Z","isPatch":true,"sender":{"key":"cworth@cworth.org","avatar":"https://gravatar.com/avatar/3746dc28cde609bdbd7f939058356e7e2bbd16d21e32274df0725eb3d998bc5b?d=mp&s=160"},"body":"On Tue, 08 Jun 2010 22:14:23 -0700, Carl Worth wrote:\n> On Tue, 08 Jun 2010 20:50:01 -0700, Junio C Hamano wrote:\n> > Carl Worth writes:\n> > Especially because your implementation quotes lines that begin with \"From \"\n> > unconditionally (even when the tail end of the line would never be a\n> > valid-looking timestamp). Such an output will confuse existing mailsplit,\n> > but the worst part of the story is that somebody who is applying a series\n> > of patches will _not_ notice the breakage. The payload of the second and\n> > subsequent messages will likely be concatenated as if it were part of the\n> > first message, ignoring cruft between patches, but the resulting tree\n> > would likely to be the same as what the sending end intended.\n...\n> Could you describe in more detail how the implementation could lead to a\n> case like that? I'm not seeing it myself. But if you can show me, I'll\n> be happy to attempt a fix.\n\nOh, perhaps I understand what you were getting at here.\n\nIf a commit is created (by whatever means) with a commit message that\nhas a line of the form:\n\n\t\"From ... \"\n\nthen with the existing code, there will be a failure if someone does a\nformat-patch and a git-am of that commit. And that might raise attention\nthat perhaps something went wrong.\n\nBut with my patch series, that commit will transfer through the\nformat-patch and git-am just fine.\n\nI would contend that preserving this commit is the right (and \"robust\")\nthing to do. For example, looking at the log recent of git.git master I\nsee 5 commits that have a \"From ... \" line in the commit\nmessage. \n\n\t34122b57eca747022336f5a3dc1aa80377d1ce56\n\t48027a918d89bad6735897a2c3da77c0451a038c\n 19a8721ef8f82153fee93c62bd050659cf718d6d\n\t3dc1383290f9db3371a13ae8009ce4fcd5ffc93a\n\t1dfcfbce2d643b7c7b56dc828f36ced9de2bf9f2\n\nThey all look to me like mistakes, some worse than others. But now that\nthey are part of the history of the project, it would be better and more\nrobust of git to actually be able to replay these successfully.\n\nGit has various tools for rewriting history, which are useful for\nvarious reasons. But these tools will get tripped up on a commit like\none of the above. For example, taking the most recent commit from above,\n\"git rebase\" is unable to replay it successfully:\n\n\t$ git checkout -b tmp 34122b57eca747022336f5a3dc1aa80377d1ce56\n\tSwitched to a new branch 'tmp'\n\t$ git rebase --onto HEAD~2 HEAD~1\n\tFirst, rewinding head to replay your work on top of it...\n\tPatch is empty. Was it split wrong?\n\nAfter my patch series this rebase works:\n\n\t$ git checkout -b tmp 34122b57eca747022336f5a3dc1aa80377d1ce56\n\tSwitched to a new branch 'tmp'\n\t0:~/src/git:(tmp)$ git rebase --onto HEAD~2 HEAD~1\n\tFirst, rewinding head to replay your work on top of it...\n\tApplying: gitweb: Always use three argument form of open\n\nThat's git being demonstrably more robust. And an operation like that\nwould make a good test for git's test suite.\n\nNow, it's likely git could also use some help to avoid whatever mistakes\ncaused these commits to be created in the first place, but that's an\northogonal issue.\n\nAlso, there is one commit that is more particularly broken than any of\nthe others. Even my patch series is not sufficient to successfully\nreplay the following commit:\n\n\t1dfcfbce2d643b7c7b56dc828f36ced9de2bf9f2\n\nThat's because in addition to the From_ line in the commit message, this\ncommit also has an entire additional patch within the commit\nmessage. And git's \"patch as email\" format has an additional quoting\nproblem with the \"---\" delimiter to separate the commit message from the\npatch. And again, that's orthogonal from the mbox quoting I'm currently\ntrying to solve.\n\n-Carl\n\n-- \ncarl.d.worth@intel.com\n"},{"id":"143440","messageId":"7vaar3nds1.fsf@alter.siamese.dyndns.org","threadId":"24048","inReplyTo":"87eiggiy8g.fsf@yoom.home.cworth.org","subject":"Re: [PATCH] format-patch: Properly escape From_ lines when creating an mbox.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-06-10T14:49:34Z","receivedAt":"2010-06-10T14:49:34Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Carl Worth writes:\n\n> On Tue, 08 Jun 2010 20:50:01 -0700, Junio C Hamano wrote:\n>> Carl Worth writes:\n>> Especially because your implementation quotes lines that begin with \"From \"\n>> unconditionally (even when the tail end of the line would never be a\n>> valid-looking timestamp). Such an output will confuse existing mailsplit,\n>> but the worst part of the story is that somebody who is applying a series\n>> of patches will _not_ notice the breakage. The payload of the second and\n>> subsequent messages will likely be concatenated as if it were part of the\n>> first message, ignoring cruft between patches, but the resulting tree\n>> would likely to be the same as what the sending end intended.\n\nPlease disregard the above; I wasn't thinking straight.\n\nIf your format-patch quotes \">*From \" in the log message, and you unquote\nit somewhere in mailsplit to mailinfo pipeline, then the only time any\nfunny interaction between the current git and your git would happen is\nwhen your git formats a commit with a line in its log that begins with\n\"From \" that cannot be a mistaken as a UNIX-From line and you use \"am\"\nfrom the current git on the output; the resulting commit would get an\nextra \">\" left in the message, but that is a small price to pay. There is\nno other downside I can see (and the upside is that the output from your\nformat-patch won't be split incorrectly, of course).\n\nI find that the change to format-patch not to emit the UNIX-From line when\ngenerating one file per commit is somewhat iffy. An upside is that the\nexisting mailsplit-mailinfo pipeline already knows not to split such an\ninput, so the change makes\n\n git format-patch |\n while read path\n do\n git am $path || break\n done\n\n(which is essentially what an old rebase did) do the right thing, even in\nthe presense of a confusing \"From \" in the log message.\n\nThat change however is not without downsides. You may potentially be\nbreaking people's existing scripts in various ways. They may be relying\non the presense of the line by:\n\n - using it to pick up the original commit object name from, just like\n \"rebase\" does;\n\n - using it as the \"magic\" number to protect them from being fed a bad\n input;\n\n - stripping the first line unconditionally, assuming it is that UNIX-From\n line they shouldn't cut and paste into the MUA.\n\nIt is nice at the conceptual level, though. By declaring individual file\nRFC2822 message (not mbox), it makes it very clear that it is MUA's\nresponsibility to quote \"From \" in the payload when the output is used by\nMUA to compose and send a message. IOW, we shouldn't be doing the quote\nin our output when operating in that mode.\n\nThanks.\n"},{"id":"143444","messageId":"87pqzyhpl2.fsf@yoom.home.cworth.org","threadId":"24048","inReplyTo":"7vaar3nds1.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] format-patch: Properly escape From_ lines when creating an mbox.","fromName":"Carl Worth","fromEmail":"cworth@cworth.org","sentAt":"2010-06-10T15:31:05Z","receivedAt":"2010-06-10T15:31:05Z","isPatch":true,"sender":{"key":"cworth@cworth.org","avatar":"https://gravatar.com/avatar/3746dc28cde609bdbd7f939058356e7e2bbd16d21e32274df0725eb3d998bc5b?d=mp&s=160"},"body":"On Thu, 10 Jun 2010 07:49:34 -0700, Junio C Hamano wrote:\n> Carl Worth writes:\n> Please disregard the above; I wasn't thinking straight.\n\nNo worries.\n\n> If your format-patch quotes \">*From \" in the log message, and you unquote\n> it somewhere in mailsplit to mailinfo pipeline, then the only time any\n> funny interaction between the current git and your git would happen is\n> when your git formats a commit with a line in its log that begins with\n> \"From \" that cannot be a mistaken as a UNIX-From line and you use \"am\"\n> from the current git on the output; the resulting commit would get an\n> extra \">\" left in the message, but that is a small price to pay.\n\nCorrect. That's the only downside I see. And it's clearly not a huge\nprice. Checking the entire git.git history, I found only 12 messages\nthat have a line beginning with \"From \" in the commit message, (ignoring\nthe 5 I referred to earlier where the mbox line actually ended up in the\ncommit message). Of these, three already have a \">From\" in them.\n\nSo clearly this kind of thing is happening already. People are sometimes\nusing systems that quote these lines and git isn't un-quoting. But it's\nreally not that objectionable in the end.\n\n> There is\n> no other downside I can see (and the upside is that the output from your\n> format-patch won't be split incorrectly, of course).\n\nGood. And this series does fix actual bugs like the failure to rebase\nacross a message with a \"From ... \" line as mentioned before.\n\n> I find that the change to format-patch not to emit the UNIX-From line when\n> generating one file per commit is somewhat iffy. An upside is that the\n> existing mailsplit-mailinfo pipeline already knows not to split such an\n> input, so the change makes\n\nThis patch was written to reduce the likelihood of the downside\nmentioned above. But I understand the potential costs you outlined.\nI'll let you make the call on whether to include it. The rest of the\npatch series will work fine without this one. But leaving it out will\nlead to more occurrences of undesired \">From \" in commit messages, (even\nwith new \"git am\" as send-email or some other MUA will see the quoted\n\">From \" and treat it as the intended payload, not an escaped \"From \").\n\n> It is nice at the conceptual level, though. By declaring individual file\n> RFC2822 message (not mbox), it makes it very clear that it is MUA's\n> responsibility to quote \"From \" in the payload when the output is used by\n> MUA to compose and send a message. IOW, we shouldn't be doing the quote\n> in our output when operating in that mode.\n\nI suppose we could maintain compatibility with any scripts, etc. by\nstill emitting the initial \"From \" line, but declaring these files as\nmessages (not mbox) and avoiding doing any quoting for them.\n\nI think that gets us all the upsides with no downsides. I'll send one\nlast patch for that.\n\n-Carl\n\n-- \ncarl.d.worth@intel.com\n"},{"id":"143445","messageId":"87mxv2hola.fsf@yoom.home.cworth.org","threadId":"24048","inReplyTo":"87pqzyhpl2.fsf@yoom.home.cworth.org","subject":"Re: [PATCH] format-patch: Properly escape From_ lines when creating an mbox.","fromName":"Carl Worth","fromEmail":"cworth@cworth.org","sentAt":"2010-06-10T15:52:33Z","receivedAt":"2010-06-10T15:52:33Z","isPatch":true,"sender":{"key":"cworth@cworth.org","avatar":"https://gravatar.com/avatar/3746dc28cde609bdbd7f939058356e7e2bbd16d21e32274df0725eb3d998bc5b?d=mp&s=160"},"body":"On Thu, 10 Jun 2010 08:31:05 -0700, Carl Worth wrote:\n> I suppose we could maintain compatibility with any scripts, etc. by\n> still emitting the initial \"From \" line, but declaring these files as\n> messages (not mbox) and avoiding doing any quoting for them.\n> \n> I think that gets us all the upsides with no downsides. I'll send one\n> last patch for that.\n\nThinking about implementing and testing this, I realized that a file\nthat looks like an mbox but isn't an mbox will confuse \"git am\"\nslightly. It will think that it should unquote any \">From \" lines, but\nthat would end up being the technically wrong thing to do since the\nlines aren't quoted.\n\nI'm not sure what to do here that would cause the least undesirable\nbreakage. Ignore this problem? Emit a line that still contains anything\nthat scripts might be looking for but that \"git am\" could key off of as\n\"not actually an mbox\"?\n\nI suppose we could put a magic timestamp there, but that feels pretty\ncreepy and fragile.\n\nAnother option would be to just emit RFC2822 messages unless the user\npasses an explicit option to format-patch (such as --mbox, which would\nbe implied by --stdout). Then git would generate legitimate (unqoted)\nmessages and legitimate (quoted) mbox files.\n\nI'd leave it to you to decide whether the --mbox option should be on by\ndefault or phased in with a warning or whatever.\n\nWhat do you think?\n\n-Carl\n\n-- \ncarl.d.worth@intel.com\n"},{"id":"143448","messageId":"7vhblan9xw.fsf@alter.siamese.dyndns.org","threadId":"24048","inReplyTo":"87mxv2hola.fsf@yoom.home.cworth.org","subject":"Re: [PATCH] format-patch: Properly escape From_ lines when creating an mbox.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-06-10T16:12:27Z","receivedAt":"2010-06-10T16:12:27Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Carl Worth writes:\n\n> Another option would be to just emit RFC2822 messages unless the user\n> passes an explicit option to format-patch (such as --mbox, which would\n> be implied by --stdout). Then git would generate legitimate (unqoted)\n> messages and legitimate (quoted) mbox files.\n>\n> I'd leave it to you to decide whether the --mbox option should be on by\n> default or phased in with a warning or whatever.\n>\n> What do you think?\n\nIt sounds like that one good way to transition is to phase in --mbox as an\noptional feature and at a revision bump later make that the default (which\nwould mean that we might need a --not-a-mbox option). I haven't thought\nthings through though...\n\nThanks.\n"},{"id":"143449","messageId":"87k4q6hmuv.fsf@yoom.home.cworth.org","threadId":"24048","inReplyTo":"7vhblan9xw.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] format-patch: Properly escape From_ lines when creating an mbox.","fromName":"Carl Worth","fromEmail":"cworth@cworth.org","sentAt":"2010-06-10T16:30:00Z","receivedAt":"2010-06-10T16:30:00Z","isPatch":true,"sender":{"key":"cworth@cworth.org","avatar":"https://gravatar.com/avatar/3746dc28cde609bdbd7f939058356e7e2bbd16d21e32274df0725eb3d998bc5b?d=mp&s=160"},"body":"On Thu, 10 Jun 2010 09:12:27 -0700, Junio C Hamano wrote:\n> It sounds like that one good way to transition is to phase in --mbox as an\n> optional feature and at a revision bump later make that the default (which\n> would mean that we might need a --not-a-mbox option). I haven't thought\n> things through though...\n\nOK. I'll put together a new, complete patch series implementing my best\nattempt at all of this.\n\nI'll make each commit address an actual bug in git as much as\npossible.\n\nFor example, I suspect that \"git am\" is still passing a bare email to\nmailsplit which can cause broken splitting for a message with a \"From\n... timestamp\" line it. My current series doesn't test that, (nor try to\nfix it), but that's all in the same family of bugs here.\n\nThanks for all the feedback,\n\n-Carl\n\n-- \ncarl.d.worth@intel.com\n"}]}