{"thread":{"id":"21662","subject":"git-mailinfo doesn't stop parsing at the end of the header","startedAt":"2009-11-18T14:20:48Z","lastAt":"2009-11-20T16:12:47Z","messageCount":13,"participants":["Philip Hofstetter","Jeff King","Jakub Narebski","Lukas Sandström"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"127853","messageId":"aa2993680911180620g151d8a07t11144d150cd6e29e@mail.gmail.com","threadId":"21662","inReplyTo":null,"subject":"git-mailinfo doesn't stop parsing at the end of the header","fromName":"Philip Hofstetter","fromEmail":"phofstetter@sensational.ch","sentAt":"2009-11-18T14:20:48Z","receivedAt":"2009-11-18T14:20:48Z","isPatch":false,"sender":{"key":"phofstetter@sensational.ch","avatar":"https://gravatar.com/avatar/d3c429f6b5ff14fbbe6e873234d407f028cc686c4ce11dd921ed4f117db73a6b?d=mp&s=160"},"body":"Hello,\n\ntoday, after working on a topic branch and trying to rebase it on top\nof the updated master, the rebase failed, complaining about an invalid\nemail address.\n\nSome investigating revealed an interesting quirk in git-mailinfo which\nseems to be a bit too eager to extract author information: Instead of\njust looking at the From:-Line in a mails header (git-rebase seems to\nuse git-am which in turn uses git-mailinfo), it searches for \"from:\"\n*anywhere* in the mail and uses the last found information as the\nsource for the author information.\n\nIn this case, git-format-patch has generated a file that looks\nsomething like this:\n\n--------------8<---------------\n\nFrom d28f21ea8ca64681ba7756417799ceea81ad6873 Mon Sep 17 00:00:00 2001\nFrom: Foo Bar \nDate: Tue, 17 Nov 2009 15:27:25 +0100\nSubject: blah, blah, blah\n\nfrom:\n- this is a\n- list for stuff\n---\n list/of/changed/files | 1 -\n list/of/changed/files2 | 1 -\n 2 files changed, 0 insertions(+), 2 deletions(-)\n\nthe actual diff down here\n\n--------------8<---------------\n\nAnd when you feed this into mailinfo, this is what you get:\n\npilif@celes ~/git % git mailinfo /dev/null /dev/null < somepatch.patch\nAuthor:\nEmail:\nSubject: blah, blah, blah\nDate: Tue, 17 Nov 2009 15:27:25 +0100\n\npilif@celes ~/git %\n\nand consequently, anything that depends on the correct author being\nextracted then fails.\n\nWhile I know it's rude to have a line beginning with \"from:\" (and it's\neven ruder to have a line beginning with \"from \"), IMHO the header\nends at the first blank line and I see no reason to extract author\ninformation past the header.\n\nAnd if this is in fact intended behavior, it should probably not be\npermitted to create a commit that later on can't be rebased or applied\nusing git-am.\n\nI had a look at the source of git-mailinfo to fix it myself, but this\nthing does too much for my minimal knowledge in C.\n\nPhilip\n\n-- \nSensational AG\nGiesshübelstrasse 62c, Postfach 1966, 8021 Zürich\nTel. +41 43 544 09 60, Mobile +41 79 341 01 99\ninfo@sensational.ch, http://www.sensational.ch\n"},{"id":"127860","messageId":"20091118155154.GA15184@coredump.intra.peff.net","threadId":"21662","inReplyTo":"aa2993680911180620g151d8a07t11144d150cd6e29e@mail.gmail.com","subject":"Re: git-mailinfo doesn't stop parsing at the end of the header","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2009-11-18T15:51:54Z","receivedAt":"2009-11-18T15:51:54Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Nov 18, 2009 at 03:20:48PM +0100, Philip Hofstetter wrote:\n\n> Some investigating revealed an interesting quirk in git-mailinfo which\n> seems to be a bit too eager to extract author information: Instead of\n> just looking at the From:-Line in a mails header (git-rebase seems to\n> use git-am which in turn uses git-mailinfo), it searches for \"from:\"\n> *anywhere* in the mail and uses the last found information as the\n> source for the author information.\n\nIt is not quite \"anywhere\"; extra headers are respected at the very top\nof the message body. This is intentional, to allow one to indicate that\na patch you are sending was authored by somebody else.\n\nSo the problem is slightly less severe; the body of your commit message\nhas to _start_ with \"From:\". Still, it is awfully ugly to hit a parsing\nambiguity like this when you are trying to do something as simple as\nrebase.\n\nSome solutions I can think of are:\n\n 1. Improve the header-finding heuristic to actually look for something\n more sane, like \"From:.*<.*@.*>\" (I don't recall off the top of my\n head which other headers we handle in this position. Probably\n Date, too).\n\n 2. Give mailinfo a \"--strict\" mode to indicate that it is directly\n parsing the output of format-patch, and not some random email. Use\n --strict when invoking \"git am\" via \"git rebase\".\n\n> While I know it's rude to have a line beginning with \"from:\" (and it's\n> even ruder to have a line beginning with \"from \"), IMHO the header\n> ends at the first blank line and I see no reason to extract author\n> information past the header.\n\nAs I explained above, there is a reason, but I don't think it's rude to\nhave either of those lines. You were, after all, writing a commit\nmessage, not an email (and even if you were, it is a failure of the\nstorage format if it can't represent your data correctly). So I think\ngit is to blame here.\n\n-Peff\n"},{"id":"127863","messageId":"20091118164208.GB15184@coredump.intra.peff.net","threadId":"21662","inReplyTo":"20091118155154.GA15184@coredump.intra.peff.net","subject":"Re: git-mailinfo doesn't stop parsing at the end of the header","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2009-11-18T16:42:08Z","receivedAt":"2009-11-18T16:42:08Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Nov 18, 2009 at 10:51:54AM -0500, Jeff King wrote:\n\n> So the problem is slightly less severe; the body of your commit message\n> has to _start_ with \"From:\". Still, it is awfully ugly to hit a parsing\n> ambiguity like this when you are trying to do something as simple as\n> rebase.\n> \n> Some solutions I can think of are:\n> \n> 1. Improve the header-finding heuristic to actually look for something\n> more sane, like \"From:.*<.*@.*>\" (I don't recall off the top of my\n> head which other headers we handle in this position. Probably\n> Date, too).\n> \n> 2. Give mailinfo a \"--strict\" mode to indicate that it is directly\n> parsing the output of format-patch, and not some random email. Use\n> --strict when invoking \"git am\" via \"git rebase\".\n\nSolution (2) seemed like a lot of work, so here is the relatively small\nsolution (1). I think looking for <.*@.*> is too restrictive, as people\nmay be using:\n\n From: bare@example.com\n\nwhich should remain valid. I just look for an '@' instead.\n\nNote that this validation also applies to actual headers. Should we turn\nit off for them? As it is, it breaks t3400, which uses a bogus email\naddress. I suppose we should probably preserve such bogosities if they\nare in the official headers.\n\ndiff --git a/builtin-mailinfo.c b/builtin-mailinfo.c\nindex c90cd31..6d69ef3 100644\n--- a/builtin-mailinfo.c\n+++ b/builtin-mailinfo.c\n@@ -275,6 +275,17 @@ static inline int cmp_header(const struct strbuf *line, const char *hdr)\n \t\t\tline->buf[len] == ':' && isspace(line->buf[len + 1]);\n }\n \n+static int validate_header(const char *header, const struct strbuf *data)\n+{\n+\tif (!strcmp(header, \"From\"))\n+\t\treturn !!strchr(data->buf, '@');\n+\tif (!strcmp(header, \"Date\")) {\n+\t\tchar buf[50];\n+\t\treturn parse_date(data->buf, buf, sizeof(buf)) >= 0;\n+\t}\n+\treturn 1;\n+}\n+\n static int check_header(const struct strbuf *line,\n \t\t\t\tstruct strbuf *hdr_data[], int overwrite)\n {\n@@ -289,8 +300,10 @@ static int check_header(const struct strbuf *line,\n \t\t\t */\n \t\t\tstrbuf_add(&sb, line->buf + len + 2, line->len - len - 2);\n \t\t\tdecode_header(&sb);\n-\t\t\thandle_header(&hdr_data[i], &sb);\n-\t\t\tret = 1;\n+\t\t\tif (validate_header(header[i], &sb)) {\n+\t\t\t\tret = 1;\n+\t\t\t\thandle_header(&hdr_data[i], &sb);\n+\t\t\t}\n \t\t\tgoto check_header_out;\n \t\t}\n \t}\ndiff --git a/t/t5100-mailinfo.sh b/t/t5100-mailinfo.sh\nindex 0279d07..be06e0f 100755\n--- a/t/t5100-mailinfo.sh\n+++ b/t/t5100-mailinfo.sh\n@@ -11,7 +11,7 @@ test_expect_success 'split sample box' \\\n \t'git mailsplit -o. \"$TEST_DIRECTORY\"/t5100/sample.mbox >last &&\n \tlast=`cat last` &&\n \techo total is $last &&\n-\ttest `cat last` = 14'\n+\ttest `cat last` = 16'\n \n check_mailinfo () {\n \tmail=$1 opt=$2\ndiff --git a/t/t5100/info0015 b/t/t5100/info0015\nnew file mode 100644\nindex 0000000..c4d8d77\n--- /dev/null\n+++ b/t/t5100/info0015\n@@ -0,0 +1,5 @@\n+Author: A U Thor\n+Email: a.u.thor@example.com\n+Subject: check bogus body header (from)\n+Date: Fri, 9 Jun 2006 00:44:16 -0700\n+\ndiff --git a/t/t5100/info0016 b/t/t5100/info0016\nnew file mode 100644\nindex 0000000..f4857d4\n--- /dev/null\n+++ b/t/t5100/info0016\n@@ -0,0 +1,5 @@\n+Author: A U Thor\n+Email: a.u.thor@example.com\n+Subject: check bogus body header (date)\n+Date: Fri, 9 Jun 2006 00:44:16 -0700\n+\ndiff --git a/t/t5100/msg0015 b/t/t5100/msg0015\nnew file mode 100644\nindex 0000000..be5115b\n--- /dev/null\n+++ b/t/t5100/msg0015\n@@ -0,0 +1,3 @@\n+From: bogosity\n+ - a list\n+ - of stuff\ndiff --git a/t/t5100/msg0016 b/t/t5100/msg0016\nnew file mode 100644\nindex 0000000..1063f51\n--- /dev/null\n+++ b/t/t5100/msg0016\n@@ -0,0 +1,4 @@\n+Date: bogus\n+\n+and some content\n+\ndiff --git a/t/t5100/patch0015 b/t/t5100/patch0015\nnew file mode 100644\nindex 0000000..ad64848\n--- /dev/null\n+++ b/t/t5100/patch0015\n@@ -0,0 +1,8 @@\n+---\n+diff --git a/foo b/foo\n+index e69de29..d95f3ad 100644\n+--- a/foo\n++++ b/foo\n+@@ -0,0 +1 @@\n++content\n+\ndiff --git a/t/t5100/patch0016 b/t/t5100/patch0016\nnew file mode 100644\nindex 0000000..ad64848\n--- /dev/null\n+++ b/t/t5100/patch0016\n@@ -0,0 +1,8 @@\n+---\n+diff --git a/foo b/foo\n+index e69de29..d95f3ad 100644\n+--- a/foo\n++++ b/foo\n+@@ -0,0 +1 @@\n++content\n+\ndiff --git a/t/t5100/sample.mbox b/t/t5100/sample.mbox\nindex 13fa4ae..de10312 100644\n--- a/t/t5100/sample.mbox\n+++ b/t/t5100/sample.mbox\n@@ -650,3 +650,36 @@ index b0b5d8f..461c47e 100644\n \t\tconvert_to_utf8(line, charset.buf);\n -- \n 1.6.4.1\n+From nobody Mon Sep 17 00:00:00 2001\n+From: A U Thor \n+Subject: check bogus body header (from)\n+Date: Fri, 9 Jun 2006 00:44:16 -0700\n+\n+From: bogosity\n+ - a list\n+ - of stuff\n+---\n+diff --git a/foo b/foo\n+index e69de29..d95f3ad 100644\n+--- a/foo\n++++ b/foo\n+@@ -0,0 +1 @@\n++content\n+\n+From nobody Mon Sep 17 00:00:00 2001\n+From: A U Thor \n+Subject: check bogus body header (date)\n+Date: Fri, 9 Jun 2006 00:44:16 -0700\n+\n+Date: bogus\n+\n+and some content\n+\n+---\n+diff --git a/foo b/foo\n+index e69de29..d95f3ad 100644\n+--- a/foo\n++++ b/foo\n+@@ -0,0 +1 @@\n++content\n+\n"},{"id":"127864","messageId":"aa2993680911180911o7e3af804m4ebdc20096baa609@mail.gmail.com","threadId":"21662","inReplyTo":"20091118155154.GA15184@coredump.intra.peff.net","subject":"Re: git-mailinfo doesn't stop parsing at the end of the header","fromName":"Philip Hofstetter","fromEmail":"phofstetter@sensational.ch","sentAt":"2009-11-18T17:11:36Z","receivedAt":"2009-11-18T17:11:36Z","isPatch":false,"sender":{"key":"phofstetter@sensational.ch","avatar":"https://gravatar.com/avatar/d3c429f6b5ff14fbbe6e873234d407f028cc686c4ce11dd921ed4f117db73a6b?d=mp&s=160"},"body":"Hello,\n\nOn Wed, Nov 18, 2009 at 4:51 PM, Jeff King wrote:\n> On Wed, Nov 18, 2009 at 03:20:48PM +0100, Philip Hofstetter wrote:\n\n>  1. Improve the header-finding heuristic to actually look for something\n>     more sane, like \"From:.*<.*@.*>\" (I don't recall off the top of my\n>     head which other headers we handle in this position. Probably\n>     Date, too).\n\nor at least don't prefer obviously invalid data over valid data that\nhas already been seen.\n\n>  2. Give mailinfo a \"--strict\" mode to indicate that it is directly\n>     parsing the output of format-patch, and not some random email. Use\n>     --strict when invoking \"git am\" via \"git rebase\".\n\nThat would solve the problem too, though it feels like adding yet\nanother switch to guard against one specific issue. The purpose behind\noptions like this tends to get forgotten over time.\n\n> As I explained above, there is a reason, but I don't think it's rude to\n> have either of those lines. You were, after all, writing a commit\n> message, not an email (and even if you were, it is a failure of the\n> storage format if it can't represent your data correctly). So I think\n> git is to blame here.\n\nIMHO, another workable solution would be to reject a commit that later\ncan't be handled. That way the current attempts at getting an email\naddress can remain intact and the (much more) unlikely case that\nsomebody begins the commit message with from: will be caught before\ndamage is done.\n\nSo, just check that from-line for a valid email address at commit\ntime. If it is, ok. If not, treat it as an error and inform the user\nthat an invalid email address was given in the commit message.\n\nAlso, the error message by rebase (which is actually the message\nprinted by am) could have been a bit more helpful. If am fails during\na rebase, rebase could explicitly tell which commit am failed at. The\noutput I got made me suspect the problem to be in the first commit (as\nthat was the last one printed) when in fact it was in the second one\n(which was not printed).\n\nBut that's just nit-picking.\n\nPhilip\n"},{"id":"127865","messageId":"20091118172424.GA24416@coredump.intra.peff.net","threadId":"21662","inReplyTo":"aa2993680911180911o7e3af804m4ebdc20096baa609@mail.gmail.com","subject":"Re: git-mailinfo doesn't stop parsing at the end of the header","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2009-11-18T17:24:24Z","receivedAt":"2009-11-18T17:24:24Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Nov 18, 2009 at 06:11:36PM +0100, Philip Hofstetter wrote:\n\n> > As I explained above, there is a reason, but I don't think it's rude to\n> > have either of those lines. You were, after all, writing a commit\n> > message, not an email (and even if you were, it is a failure of the\n> > storage format if it can't represent your data correctly). So I think\n> > git is to blame here.\n> \n> IMHO, another workable solution would be to reject a commit that later\n> can't be handled. That way the current attempts at getting an email\n> address can remain intact and the (much more) unlikely case that\n> somebody begins the commit message with from: will be caught before\n> damage is done.\n\nI'm not sure I like that solution for a few reasons:\n\n 1. It creates a bad user experience. You are not unreasonable for\n wanting to put some specific text in your commit message. Having\n git come back and say \"oops, I might get confused by this later\"\n just seems like an annoyance to the user.\n\n 2. Mailinfo has to deal with data created by older versions of git. So\n in your case, the rebase was a bomb waiting to go off. If we can\n fix it so that an existing bomb doesn't go off, rather than not\n creating the bomb in the first place, then we are better off.\n\n 3. Commit has to know about rules for mailinfo, even versions of\n mailinfo that will exist in the future. Probably the rules aren't\n going to change much, but it is a weakness.\n\n 4. Commit messages can come from other places than \"git commit\". What\n should we do with a commit message like this that is imported from\n SVN? Reject the import? Munge the message?\n\nOf course all of that presupposes that we can correctly handle the\nexisting data after the fact. Even with my patch, you still can't write\n\"From: foo@example.com\" as the first line of your commit body. But that\nis, IMHO, getting even more unlikely than your \"From:\" (which already\nseems fairly unlikely).\n\nI also think \"git commit\" would not be the right time for such a\nfeature. The problem is not that you have this text in your commit\nmessage. The problem is that the \"format-patch | am\" transport is lossy.\nYou would do better to have format-patch say \"Ah, this is going to\ncreate a bogus email address\" and somehow quote it appropriately.\n\n-Peff\n"},{"id":"127866","messageId":"m3bpizd8ua.fsf@localhost.localdomain","threadId":"21662","inReplyTo":"20091118172424.GA24416@coredump.intra.peff.net","subject":"Re: git-mailinfo doesn't stop parsing at the end of the header","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2009-11-18T17:46:43Z","receivedAt":"2009-11-18T17:46:43Z","isPatch":false,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"Jeff King writes:\n\n> I also think \"git commit\" would not be the right time for such a\n> feature. The problem is not that you have this text in your commit\n> message. The problem is that the \"format-patch | am\" transport is lossy.\n> You would do better to have format-patch say \"Ah, this is going to\n> create a bogus email address\" and somehow quote it appropriately.\n\nDoesn't mbox format have some way of escaping \"From:\" (or is it \"From \")?\nIf I remember correctly it uses \">From \" or something for that.\ngit-format-patch could do this also (perhaps only with --rebasing\noption).\n\nP.S. As git-format-patch / git-am have hidden --rebasing option,\nperhaps git-mailinfo should have it as well (even if it is called\n--strict).\n\n-- \nJakub Narebski\nPoland\nShadeHawk on #git\n"},{"id":"127867","messageId":"20091118184231.GA29999@coredump.intra.peff.net","threadId":"21662","inReplyTo":"m3bpizd8ua.fsf@localhost.localdomain","subject":"Re: git-mailinfo doesn't stop parsing at the end of the header","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2009-11-18T18:42:32Z","receivedAt":"2009-11-18T18:42:32Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Nov 18, 2009 at 09:46:43AM -0800, Jakub Narebski wrote:\n\n> > I also think \"git commit\" would not be the right time for such a\n> > feature. The problem is not that you have this text in your commit\n> > message. The problem is that the \"format-patch | am\" transport is lossy.\n> > You would do better to have format-patch say \"Ah, this is going to\n> > create a bogus email address\" and somehow quote it appropriately.\n> \n> Doesn't mbox format have some way of escaping \"From:\" (or is it \"From \")?\n> If I remember correctly it uses \">From \" or something for that.\n> git-format-patch could do this also (perhaps only with --rebasing\n> option).\n\nIt's for \"From \" lines, which are the mbox separator. This would be\nsomewhat different. It has nothing at all to do with the mail format\nitself, but rather is about git treating the body of the message\nspecially. I don't think a quoting mechanism currently exists to handle\nthis.\n\n-Peff\n"},{"id":"127871","messageId":"aa2993680911181157y750eae95sc2932b03d938d6fb@mail.gmail.com","threadId":"21662","inReplyTo":"20091118172424.GA24416@coredump.intra.peff.net","subject":"Re: git-mailinfo doesn't stop parsing at the end of the header","fromName":"Philip Hofstetter","fromEmail":"phofstetter@sensational.ch","sentAt":"2009-11-18T19:57:16Z","receivedAt":"2009-11-18T19:57:16Z","isPatch":false,"sender":{"key":"phofstetter@sensational.ch","avatar":"https://gravatar.com/avatar/d3c429f6b5ff14fbbe6e873234d407f028cc686c4ce11dd921ed4f117db73a6b?d=mp&s=160"},"body":"Hello,\n\nOn Wed, Nov 18, 2009 at 6:24 PM, Jeff King wrote:\n\n>  1. It creates a bad user experience. You are not unreasonable for\n>     wanting to put some specific text in your commit message. Having\n>     git come back and say \"oops, I might get confused by this later\"\n>     just seems like an annoyance to the user.\n\nagreed, though it's not that bad: when learning git, you will be\nconfronted with the fact that the commit message has a few things that\nare special (well. it's doesn't break git, but the first line should\nbe < 56 chars in length for example).\n\nNot being able to have From: lines in them that are not describing an\nauthor would then just be one of them.\n\n>  2. Mailinfo has to deal with data created by older versions of git. So\n>     in your case, the rebase was a bomb waiting to go off. If we can\n>     fix it so that an existing bomb doesn't go off, rather than not\n>     creating the bomb in the first place, then we are better off.\n\nThis is a very good point. I didn't quite think about that.\n\n>  4. Commit messages can come from other places than \"git commit\". What\n>     should we do with a commit message like this that is imported from\n>     SVN? Reject the import? Munge the message?\n\nI would leave that to the tool that does the import. Probably it would\nhave to munge it. Yes.\n\nI DO see though that implementing the check at commit time would lead\nto problems popping up at other places.\n\n> Of course all of that presupposes that we can correctly handle the\n> existing data after the fact. Even with my patch, you still can't write\n> \"From: foo@example.com\" as the first line of your commit body. But that\n\ncan't you? IMHO it would just attribute the commit to foo@example.com\nwhich can be an equally bad, if not worse thing (I'm saying that\nwithout the needed knowledge about git internals to really be sure, so\ntake this with a grain of salt)\n\nI just have a bad feeling about trying out heuristics to see whether\nthing thing after from: is an email address or not as email addresses\nare notoriously hard to detect.\n\nTyping a commit message and applying a patch from an email should be\nseparate things and should be handled separately. Currently they are\nnot and this is what's causing the problem in the first place.\n\nMaybe that --strict thing is actually a good thing in the long run,\neven though I don't quite like it either :-)\n\nInteresting problem to have though.\n\nPhilip\n"},{"id":"127878","messageId":"4B0478ED.30306@gmail.com","threadId":"21662","inReplyTo":"20091118164208.GB15184@coredump.intra.peff.net","subject":"[PATCH] git am/mailinfo: Don't look at in-body headers when rebasing","fromName":"Lukas Sandström","fromEmail":"luksan@gmail.com","sentAt":"2009-11-18T22:45:01Z","receivedAt":"2009-11-18T22:45:01Z","isPatch":true,"sender":{"key":"luksan@gmail.com","avatar":"https://avatars.githubusercontent.com/u/152281?v=4"},"body":"When we are rebasing we know that the header lines in the\npatch are good and that we don't need to pick up any headers\nfrom the body of the patch.\n\nThis makes it possible to rebase commits whose commit message\nstart with \"From\" or \"Date\".\n\nTest vectors by Jeff King.\n\nSigned-off-by: Lukas Sandström \n---\n\nJeff King wrote:\n>> Some solutions I can think of are:\n>>\n>> 1. Improve the header-finding heuristic to actually look for something\n>> more sane, like \"From:.*<.*@.*>\" (I don't recall off the top of my\n>> head which other headers we handle in this position. Probably\n>> Date, too).\n>>\n>> 2. Give mailinfo a \"--strict\" mode to indicate that it is directly\n>> parsing the output of format-patch, and not some random email. Use\n>> --strict when invoking \"git am\" via \"git rebase\".\n> \n> Solution (2) seemed like a lot of work, so here is the relatively small\n> solution (1). I think looking for <.*@.*> is too restrictive, as people\n> may be using:\n> \n\nThis is an implementation of solution (2). Not much work, but I might\nhave missed something. git-mailinfo usally breaks when I touch it,\nbut the testsuite passes with this patch, including the extra test\nvectors from Jeff.\n\nThe actual change is that mailinfo doesn't look for in-body headers\nat all if --no-inbody-headers is passed. git-am now passes this option\nto mailinfo when rebasing.\n\nThis won't handle the case when a \"bad\" patch is passed to git-am from\nsomewhere else than git rebase.\n\n/Lukas\n\n\n builtin-mailinfo.c | 5 +++++\n git-am.sh | 13 ++++++++++---\n t/t5100-mailinfo.sh | 6 +++++-\n t/t5100/info0015 | 5 +++++\n t/t5100/info0015--no-inbody-headers | 5 +++++\n t/t5100/info0016 | 5 +++++\n t/t5100/info0016--no-inbody-headers | 5 +++++\n t/t5100/msg0015 | 2 ++\n t/t5100/msg0015--no-inbody-headers | 3 +++\n t/t5100/msg0016 | 2 ++\n t/t5100/msg0016--no-inbody-headers | 4 ++++\n t/t5100/patch0015 | 8 ++++++++\n t/t5100/patch0015--no-inbody-headers | 8 ++++++++\n t/t5100/patch0016 | 8 ++++++++\n t/t5100/patch0016--no-inbody-headers | 8 ++++++++\n t/t5100/sample.mbox | 33\n+++++++++++++++++++++++++++++++++\n 16 files changed, 116 insertions(+), 4 deletions(-)\n create mode 100644 t/t5100/info0015\n create mode 100644 t/t5100/info0015--no-inbody-headers\n create mode 100644 t/t5100/info0016\n create mode 100644 t/t5100/info0016--no-inbody-headers\n create mode 100644 t/t5100/msg0015\n create mode 100644 t/t5100/msg0015--no-inbody-headers\n create mode 100644 t/t5100/msg0016\n create mode 100644 t/t5100/msg0016--no-inbody-headers\n create mode 100644 t/t5100/patch0015\n create mode 100644 t/t5100/patch0015--no-inbody-headers\n create mode 100644 t/t5100/patch0016\n create mode 100644 t/t5100/patch0016--no-inbody-headers\n\ndiff --git a/builtin-mailinfo.c b/builtin-mailinfo.c\nindex c90cd31..a81526e 100644\n--- a/builtin-mailinfo.c\n+++ b/builtin-mailinfo.c\n@@ -26,6 +26,7 @@ static struct strbuf charset = STRBUF_INIT;\n static int patch_lines;\n static struct strbuf **p_hdr_data, **s_hdr_data;\n static int use_scissors;\n+static int use_inbody_headers = 1;\n\n #define MAX_HDR_PARSED 10\n #define MAX_BOUNDARIES 5\n@@ -771,6 +772,8 @@ static int handle_commit_msg(struct strbuf *line)\n \t\treturn 0;\n\n \tif (still_looking) {\n+\t\tif (!use_inbody_headers)\n+\t\t\tstill_looking = 0;\n \t\tstrbuf_ltrim(line);\n \t\tif (!line->len)\n \t\t\treturn 0;\n@@ -1033,6 +1036,8 @@ int cmd_mailinfo(int argc, const char **argv,\nconst char *prefix)\n \t\t\tuse_scissors = 1;\n \t\telse if (!strcmp(argv[1], \"--no-scissors\"))\n \t\t\tuse_scissors = 0;\n+\t\telse if (!strcmp(argv[1], \"--no-inbody-headers\"))\n+\t\t\tuse_inbody_headers = 0;\n \t\telse\n \t\t\tusage(mailinfo_usage);\n \t\targc--; argv++;\ndiff --git a/git-am.sh b/git-am.sh\nindex c132f50..96869a2 100755\n--- a/git-am.sh\n+++ b/git-am.sh\n@@ -289,7 +289,7 @@ split_patches () {\n prec=4\n dotest=\"$GIT_DIR/rebase-apply\"\n sign= utf8=t keep= skip= interactive= resolved= rebasing= abort=\n-resolvemsg= resume= scissors=\n+resolvemsg= resume= scissors= no_inbody_headers=\n git_apply_opt=\n committer_date_is_author_date=\n ignore_date=\n@@ -322,7 +322,7 @@ do\n \t--abort)\n \t\tabort=t ;;\n \t--rebasing)\n-\t\trebasing=t threeway=t keep=t scissors=f ;;\n+\t\trebasing=t threeway=t keep=t scissors=f no_inbody_headers=t ;;\n \t-d|--dotest)\n \t\tdie \"-d option is no longer supported. Do not use.\"\n \t\t;;\n@@ -448,6 +448,7 @@ else\n \techo \"$utf8\" >\"$dotest/utf8\"\n \techo \"$keep\" >\"$dotest/keep\"\n \techo \"$scissors\" >\"$dotest/scissors\"\n+\techo \"$no_inbody_headers\" >\"$dotest/no_inbody_headers\"\n \techo \"$GIT_QUIET\" >\"$dotest/quiet\"\n \techo 1 >\"$dotest/next\"\n \tif test -n \"$rebasing\"\n@@ -495,6 +496,12 @@ t)\n f)\n \tscissors=--no-scissors ;;\n esac\n+if test \"$(cat \"$dotest/no_inbody_headers\")\" = t\n+then\n+\tno_inbody_headers=--no-inbody-headers\n+else\n+\tno_inbody_headers=\n+fi\n if test \"$(cat \"$dotest/quiet\")\" = t\n then\n \tGIT_QUIET=t\n@@ -549,7 +556,7 @@ do\n \t# by the user, or the user can tell us to do so by --resolved flag.\n \tcase \"$resume\" in\n \t'')\n-\t\tgit mailinfo $keep $scissors $utf8 \"$dotest/msg\" \"$dotest/patch\" \\\n+\t\tgit mailinfo $keep $no_inbody_headers $scissors $utf8 \"$dotest/msg\" \"$dotest/patch\" \\\n \t\t\t<\"$dotest/$msgnum\" >\"$dotest/info\" ||\n \t\t\tstop_here $this\n\ndiff --git a/t/t5100-mailinfo.sh b/t/t5100-mailinfo.sh\nindex 0279d07..50e13c1 100755\n--- a/t/t5100-mailinfo.sh\n+++ b/t/t5100-mailinfo.sh\n@@ -11,7 +11,7 @@ test_expect_success 'split sample box' \\\n \t'git mailsplit -o. \"$TEST_DIRECTORY\"/t5100/sample.mbox >last &&\n \tlast=`cat last` &&\n \techo total is $last &&\n-\ttest `cat last` = 14'\n+\ttest `cat last` = 16'\n\n check_mailinfo () {\n \tmail=$1 opt=$2\n@@ -30,6 +30,10 @@ do\n \t\tif test -f \"$TEST_DIRECTORY\"/t5100/msg$mail--scissors\n \t\tthen\n \t\t\tcheck_mailinfo $mail --scissors\n+\t\tfi &&\n+\t\tif test -f \"$TEST_DIRECTORY\"/t5100/msg$mail--use-first-header\n+\t\tthen\n+\t\t\tcheck_mailinfo $mail --no-inbody-headers\n \t\tfi\n \t'\n done\ndiff --git a/t/t5100/info0015 b/t/t5100/info0015\nnew file mode 100644\nindex 0000000..0114f10\n--- /dev/null\n+++ b/t/t5100/info0015\n@@ -0,0 +1,5 @@\n+Author:\n+Email:\n+Subject: check bogus body header (from)\n+Date: Fri, 9 Jun 2006 00:44:16 -0700\n+\ndiff --git a/t/t5100/info0015--no-inbody-headers\nb/t/t5100/info0015--no-inbody-headers\nnew file mode 100644\nindex 0000000..c4d8d77\n--- /dev/null\n+++ b/t/t5100/info0015--no-inbody-headers\n@@ -0,0 +1,5 @@\n+Author: A U Thor\n+Email: a.u.thor@example.com\n+Subject: check bogus body header (from)\n+Date: Fri, 9 Jun 2006 00:44:16 -0700\n+\ndiff --git a/t/t5100/info0016 b/t/t5100/info0016\nnew file mode 100644\nindex 0000000..38ccd0d\n--- /dev/null\n+++ b/t/t5100/info0016\n@@ -0,0 +1,5 @@\n+Author: A U Thor\n+Email: a.u.thor@example.com\n+Subject: check bogus body header (date)\n+Date: bogus\n+\ndiff --git a/t/t5100/info0016--no-inbody-headers\nb/t/t5100/info0016--no-inbody-headers\nnew file mode 100644\nindex 0000000..f4857d4\n--- /dev/null\n+++ b/t/t5100/info0016--no-inbody-headers\n@@ -0,0 +1,5 @@\n+Author: A U Thor\n+Email: a.u.thor@example.com\n+Subject: check bogus body header (date)\n+Date: Fri, 9 Jun 2006 00:44:16 -0700\n+\ndiff --git a/t/t5100/msg0015 b/t/t5100/msg0015\nnew file mode 100644\nindex 0000000..9577238\n--- /dev/null\n+++ b/t/t5100/msg0015\n@@ -0,0 +1,2 @@\n+- a list\n+ - of stuff\ndiff --git a/t/t5100/msg0015--no-inbody-headers\nb/t/t5100/msg0015--no-inbody-headers\nnew file mode 100644\nindex 0000000..be5115b\n--- /dev/null\n+++ b/t/t5100/msg0015--no-inbody-headers\n@@ -0,0 +1,3 @@\n+From: bogosity\n+ - a list\n+ - of stuff\ndiff --git a/t/t5100/msg0016 b/t/t5100/msg0016\nnew file mode 100644\nindex 0000000..0d9adad\n--- /dev/null\n+++ b/t/t5100/msg0016\n@@ -0,0 +1,2 @@\n+and some content\n+\ndiff --git a/t/t5100/msg0016--no-inbody-headers\nb/t/t5100/msg0016--no-inbody-headers\nnew file mode 100644\nindex 0000000..1063f51\n--- /dev/null\n+++ b/t/t5100/msg0016--no-inbody-headers\n@@ -0,0 +1,4 @@\n+Date: bogus\n+\n+and some content\n+\ndiff --git a/t/t5100/patch0015 b/t/t5100/patch0015\nnew file mode 100644\nindex 0000000..ad64848\n--- /dev/null\n+++ b/t/t5100/patch0015\n@@ -0,0 +1,8 @@\n+---\n+diff --git a/foo b/foo\n+index e69de29..d95f3ad 100644\n+--- a/foo\n++++ b/foo\n+@@ -0,0 +1 @@\n++content\n+\ndiff --git a/t/t5100/patch0015--no-inbody-headers\nb/t/t5100/patch0015--no-inbody-headers\nnew file mode 100644\nindex 0000000..ad64848\n--- /dev/null\n+++ b/t/t5100/patch0015--no-inbody-headers\n@@ -0,0 +1,8 @@\n+---\n+diff --git a/foo b/foo\n+index e69de29..d95f3ad 100644\n+--- a/foo\n++++ b/foo\n+@@ -0,0 +1 @@\n++content\n+\ndiff --git a/t/t5100/patch0016 b/t/t5100/patch0016\nnew file mode 100644\nindex 0000000..ad64848\n--- /dev/null\n+++ b/t/t5100/patch0016\n@@ -0,0 +1,8 @@\n+---\n+diff --git a/foo b/foo\n+index e69de29..d95f3ad 100644\n+--- a/foo\n++++ b/foo\n+@@ -0,0 +1 @@\n++content\n+\ndiff --git a/t/t5100/patch0016--no-inbody-headers\nb/t/t5100/patch0016--no-inbody-headers\nnew file mode 100644\nindex 0000000..ad64848\n--- /dev/null\n+++ b/t/t5100/patch0016--no-inbody-headers\n@@ -0,0 +1,8 @@\n+---\n+diff --git a/foo b/foo\n+index e69de29..d95f3ad 100644\n+--- a/foo\n++++ b/foo\n+@@ -0,0 +1 @@\n++content\n+\ndiff --git a/t/t5100/sample.mbox b/t/t5100/sample.mbox\nindex 13fa4ae..de10312 100644\n--- a/t/t5100/sample.mbox\n+++ b/t/t5100/sample.mbox\n@@ -650,3 +650,36 @@ index b0b5d8f..461c47e 100644\n \t\tconvert_to_utf8(line, charset.buf);\n --\n 1.6.4.1\n+From nobody Mon Sep 17 00:00:00 2001\n+From: A U Thor \n+Subject: check bogus body header (from)\n+Date: Fri, 9 Jun 2006 00:44:16 -0700\n+\n+From: bogosity\n+ - a list\n+ - of stuff\n+---\n+diff --git a/foo b/foo\n+index e69de29..d95f3ad 100644\n+--- a/foo\n++++ b/foo\n+@@ -0,0 +1 @@\n++content\n+\n+From nobody Mon Sep 17 00:00:00 2001\n+From: A U Thor \n+Subject: check bogus body header (date)\n+Date: Fri, 9 Jun 2006 00:44:16 -0700\n+\n+Date: bogus\n+\n+and some content\n+\n+---\n+diff --git a/foo b/foo\n+index e69de29..d95f3ad 100644\n+--- a/foo\n++++ b/foo\n+@@ -0,0 +1 @@\n++content\n+\n-- \n1.6.4.4\n"},{"id":"127880","messageId":"aa2993680911181547p4cbbf12cq74b482f63e59d007@mail.gmail.com","threadId":"21662","inReplyTo":"4B0478ED.30306@gmail.com","subject":"Re: [PATCH] git am/mailinfo: Don't look at in-body headers when rebasing","fromName":"Philip Hofstetter","fromEmail":"phofstetter@sensational.ch","sentAt":"2009-11-18T23:47:15Z","receivedAt":"2009-11-18T23:47:15Z","isPatch":true,"sender":{"key":"phofstetter@sensational.ch","avatar":"https://gravatar.com/avatar/d3c429f6b5ff14fbbe6e873234d407f028cc686c4ce11dd921ed4f117db73a6b?d=mp&s=160"},"body":"Hi,\n\nOn Wed, Nov 18, 2009 at 11:45 PM, Lukas Sandström wrote:\n\n> The actual change is that mailinfo doesn't look for in-body headers\n> at all if --no-inbody-headers is passed. git-am now passes this option\n> to mailinfo when rebasing.\n\nafter all the earlier discussion and a lot of thinking, I have to say,\nthat IMHO, this is the best option as it doesn't rely on heuristics\nand now that you chose a descriptive command line switch, even the\nsmall problem of \"why exactly is this switch here?\" seems to go away.\n\nAs I have no experience in git's codebase at all, I'll leave the\ncommenting on the patch itself to the people with clue, but\nconceptionally, this feels much better than the method 1\n\n> This won't handle the case when a \"bad\" patch is passed to git-am from\n> somewhere else than git rebase.\n\nof course, that leaves the question what \"somewhere else\" can contain.\nIf it's just manual calls to git-am, this is a non-issue as it's\neasily fixed by the caller. If it's being called from other\nhigher-level operations though, you might run into the same issue\nagain.\n\nHere too, I can't really provide any meaningful input though as I just\ndon't know well enough what really makes git tick.\n\nJust my two cents :-)\n\nPhilip\n"},{"id":"127889","messageId":"4B050718.8070506@gmail.com","threadId":"21662","inReplyTo":"aa2993680911181547p4cbbf12cq74b482f63e59d007@mail.gmail.com","subject":"[PATCH v2] git am/mailinfo: Don't look at in-body headers when rebasing","fromName":"Lukas Sandström","fromEmail":"luksan@gmail.com","sentAt":"2009-11-19T08:51:36Z","receivedAt":"2009-11-19T08:51:36Z","isPatch":true,"sender":{"key":"luksan@gmail.com","avatar":"https://avatars.githubusercontent.com/u/152281?v=4"},"body":"When we are rebasing we know that the header lines in the\npatch are good and that we don't need to pick up any headers\nfrom the body of the patch.\n\nThis makes it possible to rebase commits whose commit message\nstart with \"From\" or \"Date\".\n\nTest vectors by Jeff King.\n\nSigned-off-by: Lukas Sandström \n---\n\nArgh. I just realized that the change to t5100 in the previous patch\ndoesn't actually test the new option, since I forgot to change the\n+\t\tif test -f \"$TEST_DIRECTORY\"/t5100/msg$mail--use-first-header\nline to \"--no-inbody-headers\", after my first attempt at an option name.\n\nThis time I checked that the tests actually fail when the test is broken.\nStill passes, just one line changed since the previous version.\n\n/Lukas\n\n builtin-mailinfo.c | 5 +++++\n git-am.sh | 13 ++++++++++---\n t/t5100-mailinfo.sh | 6 +++++-\n t/t5100/info0015 | 5 +++++\n t/t5100/info0015--no-inbody-headers | 5 +++++\n t/t5100/info0016 | 5 +++++\n t/t5100/info0016--no-inbody-headers | 5 +++++\n t/t5100/msg0015 | 2 ++\n t/t5100/msg0015--no-inbody-headers | 3 +++\n t/t5100/msg0016 | 2 ++\n t/t5100/msg0016--no-inbody-headers | 4 ++++\n t/t5100/patch0015 | 8 ++++++++\n t/t5100/patch0015--no-inbody-headers | 8 ++++++++\n t/t5100/patch0016 | 8 ++++++++\n t/t5100/patch0016--no-inbody-headers | 8 ++++++++\n t/t5100/sample.mbox | 33 +++++++++++++++++++++++++++++++++\n 16 files changed, 116 insertions(+), 4 deletions(-)\n create mode 100644 t/t5100/info0015\n create mode 100644 t/t5100/info0015--no-inbody-headers\n create mode 100644 t/t5100/info0016\n create mode 100644 t/t5100/info0016--no-inbody-headers\n create mode 100644 t/t5100/msg0015\n create mode 100644 t/t5100/msg0015--no-inbody-headers\n create mode 100644 t/t5100/msg0016\n create mode 100644 t/t5100/msg0016--no-inbody-headers\n create mode 100644 t/t5100/patch0015\n create mode 100644 t/t5100/patch0015--no-inbody-headers\n create mode 100644 t/t5100/patch0016\n create mode 100644 t/t5100/patch0016--no-inbody-headers\n\ndiff --git a/builtin-mailinfo.c b/builtin-mailinfo.c\nindex c90cd31..a81526e 100644\n--- a/builtin-mailinfo.c\n+++ b/builtin-mailinfo.c\n@@ -26,6 +26,7 @@ static struct strbuf charset = STRBUF_INIT;\n static int patch_lines;\n static struct strbuf **p_hdr_data, **s_hdr_data;\n static int use_scissors;\n+static int use_inbody_headers = 1;\n\n #define MAX_HDR_PARSED 10\n #define MAX_BOUNDARIES 5\n@@ -771,6 +772,8 @@ static int handle_commit_msg(struct strbuf *line)\n \t\treturn 0;\n\n \tif (still_looking) {\n+\t\tif (!use_inbody_headers)\n+\t\t\tstill_looking = 0;\n \t\tstrbuf_ltrim(line);\n \t\tif (!line->len)\n \t\t\treturn 0;\n@@ -1033,6 +1036,8 @@ int cmd_mailinfo(int argc, const char **argv, const char *prefix)\n \t\t\tuse_scissors = 1;\n \t\telse if (!strcmp(argv[1], \"--no-scissors\"))\n \t\t\tuse_scissors = 0;\n+\t\telse if (!strcmp(argv[1], \"--no-inbody-headers\"))\n+\t\t\tuse_inbody_headers = 0;\n \t\telse\n \t\t\tusage(mailinfo_usage);\n \t\targc--; argv++;\ndiff --git a/git-am.sh b/git-am.sh\nindex c132f50..96869a2 100755\n--- a/git-am.sh\n+++ b/git-am.sh\n@@ -289,7 +289,7 @@ split_patches () {\n prec=4\n dotest=\"$GIT_DIR/rebase-apply\"\n sign= utf8=t keep= skip= interactive= resolved= rebasing= abort=\n-resolvemsg= resume= scissors=\n+resolvemsg= resume= scissors= no_inbody_headers=\n git_apply_opt=\n committer_date_is_author_date=\n ignore_date=\n@@ -322,7 +322,7 @@ do\n \t--abort)\n \t\tabort=t ;;\n \t--rebasing)\n-\t\trebasing=t threeway=t keep=t scissors=f ;;\n+\t\trebasing=t threeway=t keep=t scissors=f no_inbody_headers=t ;;\n \t-d|--dotest)\n \t\tdie \"-d option is no longer supported. Do not use.\"\n \t\t;;\n@@ -448,6 +448,7 @@ else\n \techo \"$utf8\" >\"$dotest/utf8\"\n \techo \"$keep\" >\"$dotest/keep\"\n \techo \"$scissors\" >\"$dotest/scissors\"\n+\techo \"$no_inbody_headers\" >\"$dotest/no_inbody_headers\"\n \techo \"$GIT_QUIET\" >\"$dotest/quiet\"\n \techo 1 >\"$dotest/next\"\n \tif test -n \"$rebasing\"\n@@ -495,6 +496,12 @@ t)\n f)\n \tscissors=--no-scissors ;;\n esac\n+if test \"$(cat \"$dotest/no_inbody_headers\")\" = t\n+then\n+\tno_inbody_headers=--no-inbody-headers\n+else\n+\tno_inbody_headers=\n+fi\n if test \"$(cat \"$dotest/quiet\")\" = t\n then\n \tGIT_QUIET=t\n@@ -549,7 +556,7 @@ do\n \t# by the user, or the user can tell us to do so by --resolved flag.\n \tcase \"$resume\" in\n \t'')\n-\t\tgit mailinfo $keep $scissors $utf8 \"$dotest/msg\" \"$dotest/patch\" \\\n+\t\tgit mailinfo $keep $no_inbody_headers $scissors $utf8 \"$dotest/msg\" \"$dotest/patch\" \\\n \t\t\t<\"$dotest/$msgnum\" >\"$dotest/info\" ||\n \t\t\tstop_here $this\n\ndiff --git a/t/t5100-mailinfo.sh b/t/t5100-mailinfo.sh\nindex 0279d07..ebc36c1 100755\n--- a/t/t5100-mailinfo.sh\n+++ b/t/t5100-mailinfo.sh\n@@ -11,7 +11,7 @@ test_expect_success 'split sample box' \\\n \t'git mailsplit -o. \"$TEST_DIRECTORY\"/t5100/sample.mbox >last &&\n \tlast=`cat last` &&\n \techo total is $last &&\n-\ttest `cat last` = 14'\n+\ttest `cat last` = 16'\n\n check_mailinfo () {\n \tmail=$1 opt=$2\n@@ -30,6 +30,10 @@ do\n \t\tif test -f \"$TEST_DIRECTORY\"/t5100/msg$mail--scissors\n \t\tthen\n \t\t\tcheck_mailinfo $mail --scissors\n+\t\tfi &&\n+\t\tif test -f \"$TEST_DIRECTORY\"/t5100/msg$mail--no-inbody-headers\n+\t\tthen\n+\t\t\tcheck_mailinfo $mail --no-inbody-headers\n \t\tfi\n \t'\n done\ndiff --git a/t/t5100/info0015 b/t/t5100/info0015\nnew file mode 100644\nindex 0000000..0114f10\n--- /dev/null\n+++ b/t/t5100/info0015\n@@ -0,0 +1,5 @@\n+Author:\n+Email:\n+Subject: check bogus body header (from)\n+Date: Fri, 9 Jun 2006 00:44:16 -0700\n+\ndiff --git a/t/t5100/info0015--no-inbody-headers b/t/t5100/info0015--no-inbody-headers\nnew file mode 100644\nindex 0000000..c4d8d77\n--- /dev/null\n+++ b/t/t5100/info0015--no-inbody-headers\n@@ -0,0 +1,5 @@\n+Author: A U Thor\n+Email: a.u.thor@example.com\n+Subject: check bogus body header (from)\n+Date: Fri, 9 Jun 2006 00:44:16 -0700\n+\ndiff --git a/t/t5100/info0016 b/t/t5100/info0016\nnew file mode 100644\nindex 0000000..38ccd0d\n--- /dev/null\n+++ b/t/t5100/info0016\n@@ -0,0 +1,5 @@\n+Author: A U Thor\n+Email: a.u.thor@example.com\n+Subject: check bogus body header (date)\n+Date: bogus\n+\ndiff --git a/t/t5100/info0016--no-inbody-headers b/t/t5100/info0016--no-inbody-headers\nnew file mode 100644\nindex 0000000..f4857d4\n--- /dev/null\n+++ b/t/t5100/info0016--no-inbody-headers\n@@ -0,0 +1,5 @@\n+Author: A U Thor\n+Email: a.u.thor@example.com\n+Subject: check bogus body header (date)\n+Date: Fri, 9 Jun 2006 00:44:16 -0700\n+\ndiff --git a/t/t5100/msg0015 b/t/t5100/msg0015\nnew file mode 100644\nindex 0000000..9577238\n--- /dev/null\n+++ b/t/t5100/msg0015\n@@ -0,0 +1,2 @@\n+- a list\n+ - of stuff\ndiff --git a/t/t5100/msg0015--no-inbody-headers b/t/t5100/msg0015--no-inbody-headers\nnew file mode 100644\nindex 0000000..be5115b\n--- /dev/null\n+++ b/t/t5100/msg0015--no-inbody-headers\n@@ -0,0 +1,3 @@\n+From: bogosity\n+ - a list\n+ - of stuff\ndiff --git a/t/t5100/msg0016 b/t/t5100/msg0016\nnew file mode 100644\nindex 0000000..0d9adad\n--- /dev/null\n+++ b/t/t5100/msg0016\n@@ -0,0 +1,2 @@\n+and some content\n+\ndiff --git a/t/t5100/msg0016--no-inbody-headers b/t/t5100/msg0016--no-inbody-headers\nnew file mode 100644\nindex 0000000..1063f51\n--- /dev/null\n+++ b/t/t5100/msg0016--no-inbody-headers\n@@ -0,0 +1,4 @@\n+Date: bogus\n+\n+and some content\n+\ndiff --git a/t/t5100/patch0015 b/t/t5100/patch0015\nnew file mode 100644\nindex 0000000..ad64848\n--- /dev/null\n+++ b/t/t5100/patch0015\n@@ -0,0 +1,8 @@\n+---\n+diff --git a/foo b/foo\n+index e69de29..d95f3ad 100644\n+--- a/foo\n++++ b/foo\n+@@ -0,0 +1 @@\n++content\n+\ndiff --git a/t/t5100/patch0015--no-inbody-headers b/t/t5100/patch0015--no-inbody-headers\nnew file mode 100644\nindex 0000000..ad64848\n--- /dev/null\n+++ b/t/t5100/patch0015--no-inbody-headers\n@@ -0,0 +1,8 @@\n+---\n+diff --git a/foo b/foo\n+index e69de29..d95f3ad 100644\n+--- a/foo\n++++ b/foo\n+@@ -0,0 +1 @@\n++content\n+\ndiff --git a/t/t5100/patch0016 b/t/t5100/patch0016\nnew file mode 100644\nindex 0000000..ad64848\n--- /dev/null\n+++ b/t/t5100/patch0016\n@@ -0,0 +1,8 @@\n+---\n+diff --git a/foo b/foo\n+index e69de29..d95f3ad 100644\n+--- a/foo\n++++ b/foo\n+@@ -0,0 +1 @@\n++content\n+\ndiff --git a/t/t5100/patch0016--no-inbody-headers b/t/t5100/patch0016--no-inbody-headers\nnew file mode 100644\nindex 0000000..ad64848\n--- /dev/null\n+++ b/t/t5100/patch0016--no-inbody-headers\n@@ -0,0 +1,8 @@\n+---\n+diff --git a/foo b/foo\n+index e69de29..d95f3ad 100644\n+--- a/foo\n++++ b/foo\n+@@ -0,0 +1 @@\n++content\n+\ndiff --git a/t/t5100/sample.mbox b/t/t5100/sample.mbox\nindex 13fa4ae..de10312 100644\n--- a/t/t5100/sample.mbox\n+++ b/t/t5100/sample.mbox\n@@ -650,3 +650,36 @@ index b0b5d8f..461c47e 100644\n \t\tconvert_to_utf8(line, charset.buf);\n --\n 1.6.4.1\n+From nobody Mon Sep 17 00:00:00 2001\n+From: A U Thor \n+Subject: check bogus body header (from)\n+Date: Fri, 9 Jun 2006 00:44:16 -0700\n+\n+From: bogosity\n+ - a list\n+ - of stuff\n+---\n+diff --git a/foo b/foo\n+index e69de29..d95f3ad 100644\n+--- a/foo\n++++ b/foo\n+@@ -0,0 +1 @@\n++content\n+\n+From nobody Mon Sep 17 00:00:00 2001\n+From: A U Thor \n+Subject: check bogus body header (date)\n+Date: Fri, 9 Jun 2006 00:44:16 -0700\n+\n+Date: bogus\n+\n+and some content\n+\n+---\n+diff --git a/foo b/foo\n+index e69de29..d95f3ad 100644\n+--- a/foo\n++++ b/foo\n+@@ -0,0 +1 @@\n++content\n+\n-- \n1.6.4.4\n"},{"id":"127908","messageId":"20091119153622.GC6877@coredump.intra.peff.net","threadId":"21662","inReplyTo":"4B050718.8070506@gmail.com","subject":"Re: [PATCH v2] git am/mailinfo: Don't look at in-body headers when rebasing","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2009-11-19T15:36:23Z","receivedAt":"2009-11-19T15:36:23Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Nov 19, 2009 at 09:51:36AM +0100, Lukas Sandström wrote:\n\n> When we are rebasing we know that the header lines in the\n> patch are good and that we don't need to pick up any headers\n> from the body of the patch.\n> \n> This makes it possible to rebase commits whose commit message\n> start with \"From\" or \"Date\".\n> \n> Test vectors by Jeff King.\n\nThanks, it did end up being a pretty small change. Though I think we may\nbe better off with _both_ patches. Your patch protects the message\nabsolutely during rebasing, and my patch improves the heuristic when\napplying non-rebase patches.\n\n> @@ -771,6 +772,8 @@ static int handle_commit_msg(struct strbuf *line)\n> \t\treturn 0;\n> \n> \tif (still_looking) {\n> +\t\tif (!use_inbody_headers)\n> +\t\t\tstill_looking = 0;\n> \t\tstrbuf_ltrim(line);\n> \t\tif (!line->len)\n> \t\t\treturn 0;\n\nHmm. But we still end up in this conditional for the very first line.\nWhich I guess happens to work because the first line we feed is\npresumably the empty blank line (but I didn't check). Still, wouldn't it\nbe more clear as:\n\n if (use_inbody_headers && still_looking) {\n ...\n\nin which case still_looking simply becomes irrelevant when the feature\nis disabled?\n\n> +From nobody Mon Sep 17 00:00:00 2001\n> +From: A U Thor \n> +Subject: check bogus body header (from)\n> +Date: Fri, 9 Jun 2006 00:44:16 -0700\n> +\n> +From: bogosity\n> + - a list\n> + - of stuff\n> +---\n\nSince your feature is meant to prevent us looking at inbody headers no\nmatter if they are valid-looking or not, wouldn't a better test be to\nactually have:\n\n From: Other Author \n\nOtherwise, you don't know if it is your feature blocking it, or my patch\n(if it gets applied on top).\n\n> +From nobody Mon Sep 17 00:00:00 2001\n> +From: A U Thor \n> +Subject: check bogus body header (date)\n> +Date: Fri, 9 Jun 2006 00:44:16 -0700\n> +\n> +Date: bogus\n> +\n> +and some content\n> +\n\nAnd ditto for the Date here.\n\n-Peff\n"},{"id":"128028","messageId":"4B06BFFF.3020807@gmail.com","threadId":"21662","inReplyTo":"20091119153622.GC6877@coredump.intra.peff.net","subject":"[PATCH] git am/mailinfo: Don't look at in-body headers when rebasing","fromName":"Lukas Sandström","fromEmail":"luksan@gmail.com","sentAt":"2009-11-20T16:12:47Z","receivedAt":"2009-11-20T16:12:47Z","isPatch":true,"sender":{"key":"luksan@gmail.com","avatar":"https://avatars.githubusercontent.com/u/152281?v=4"},"body":"When we are rebasing we know that the header lines in the\npatch are good and that we don't need to pick up any headers\nfrom the body of the patch.\n\nThis makes it possible to rebase commits whose commit message\nstart with \"From\" or \"Date\".\n\nTest vectors by Jeff King.\n\nSigned-off-by: Lukas Sandström \n---\n\nJeff King wrote:\n> On Thu, Nov 19, 2009 at 09:51:36AM +0100, Lukas Sandström wrote:\n> Thanks, it did end up being a pretty small change. Though I think we may\n> be better off with _both_ patches. Your patch protects the message\n> absolutely during rebasing, and my patch improves the heuristic when\n> applying non-rebase patches.\n> \n\nI looked a bit at using your extra safety check, but it would be a change\nin behavior to only accept valid email adresses, and mailinfo\nis riddled with corner cases. If this changed is made someone\nwill propably complain, saying that they rely on invalid dates in their\ncommit messages.\n\nI don't know when to apply your stricter check whithout breaking any tests.\n\n>> @@ -771,6 +772,8 @@ static int handle_commit_msg(struct strbuf *line)\n>> \t\treturn 0;\n>>\n>> \tif (still_looking) {\n>> +\t\tif (!use_inbody_headers)\n>> +\t\t\tstill_looking = 0;\n>> \t\tstrbuf_ltrim(line);\n>> \t\tif (!line->len)\n>> \t\t\treturn 0;\n> \n> Hmm. But we still end up in this conditional for the very first line.\n> Which I guess happens to work because the first line we feed is\n> presumably the empty blank line (but I didn't check). Still, wouldn't it\n> be more clear as:\n> \n> if (use_inbody_headers && still_looking) {\n> ...\n> \n> in which case still_looking simply becomes irrelevant when the feature\n> is disabled?\n\nWhen rebasing the first line passed to handle_commit_msg will be the blank\nline between the headers and commit message. This should be removed. I\nrewrote this part to make it a bit more obvious.\n\n> \n>> +From nobody Mon Sep 17 00:00:00 2001\n>> +From: A U Thor \n>> +Subject: check bogus body header (from)\n>> +Date: Fri, 9 Jun 2006 00:44:16 -0700\n>> +\n>> +From: bogosity\n>> + - a list\n>> + - of stuff\n>> +---\n> \n> Since your feature is meant to prevent us looking at inbody headers no\n> matter if they are valid-looking or not, wouldn't a better test be to\n> actually have:\n> \n> From: Other Author \n> \n> Otherwise, you don't know if it is your feature blocking it, or my patch\n> (if it gets applied on top).\n> \n\nI kept the tests as is for now, since they show the problem\noriginally reported.\n\n/Lukas\n\n\n builtin-mailinfo.c | 12 +++++++++++-\n git-am.sh | 13 ++++++++++---\n t/t5100-mailinfo.sh | 6 +++++-\n t/t5100/info0015 | 5 +++++\n t/t5100/info0015--no-inbody-headers | 5 +++++\n t/t5100/info0016 | 5 +++++\n t/t5100/info0016--no-inbody-headers | 5 +++++\n t/t5100/msg0015 | 2 ++\n t/t5100/msg0015--no-inbody-headers | 3 +++\n t/t5100/msg0016 | 2 ++\n t/t5100/msg0016--no-inbody-headers | 4 ++++\n t/t5100/patch0015 | 8 ++++++++\n t/t5100/patch0015--no-inbody-headers | 8 ++++++++\n t/t5100/patch0016 | 8 ++++++++\n t/t5100/patch0016--no-inbody-headers | 8 ++++++++\n t/t5100/sample.mbox | 33 +++++++++++++++++++++++++++++++++\n 16 files changed, 122 insertions(+), 5 deletions(-)\n create mode 100644 t/t5100/info0015\n create mode 100644 t/t5100/info0015--no-inbody-headers\n create mode 100644 t/t5100/info0016\n create mode 100644 t/t5100/info0016--no-inbody-headers\n create mode 100644 t/t5100/msg0015\n create mode 100644 t/t5100/msg0015--no-inbody-headers\n create mode 100644 t/t5100/msg0016\n create mode 100644 t/t5100/msg0016--no-inbody-headers\n create mode 100644 t/t5100/patch0015\n create mode 100644 t/t5100/patch0015--no-inbody-headers\n create mode 100644 t/t5100/patch0016\n create mode 100644 t/t5100/patch0016--no-inbody-headers\n\ndiff --git a/builtin-mailinfo.c b/builtin-mailinfo.c\nindex c90cd31..3c4f075 100644\n--- a/builtin-mailinfo.c\n+++ b/builtin-mailinfo.c\n@@ -26,6 +26,7 @@ static struct strbuf charset = STRBUF_INIT;\n static int patch_lines;\n static struct strbuf **p_hdr_data, **s_hdr_data;\n static int use_scissors;\n+static int use_inbody_headers = 1;\n\n #define MAX_HDR_PARSED 10\n #define MAX_BOUNDARIES 5\n@@ -774,10 +775,17 @@ static int handle_commit_msg(struct strbuf *line)\n \t\tstrbuf_ltrim(line);\n \t\tif (!line->len)\n \t\t\treturn 0;\n+\t}\n+\n+\tif (use_inbody_headers && still_looking) {\n \t\tstill_looking = check_header(line, s_hdr_data, 0);\n \t\tif (still_looking)\n \t\t\treturn 0;\n-\t}\n+\t} else\n+\t\t/* Only trim the first (blank) line of the commit message\n+\t\t * when ignoring in-body headers.\n+\t\t */\n+\t\tstill_looking = 0;\n\n \t/* normalize the log message to UTF-8. */\n \tif (metainfo_charset)\n@@ -1033,6 +1041,8 @@ int cmd_mailinfo(int argc, const char **argv, const char *prefix)\n \t\t\tuse_scissors = 1;\n \t\telse if (!strcmp(argv[1], \"--no-scissors\"))\n \t\t\tuse_scissors = 0;\n+\t\telse if (!strcmp(argv[1], \"--no-inbody-headers\"))\n+\t\t\tuse_inbody_headers = 0;\n \t\telse\n \t\t\tusage(mailinfo_usage);\n \t\targc--; argv++;\ndiff --git a/git-am.sh b/git-am.sh\nindex c132f50..96869a2 100755\n--- a/git-am.sh\n+++ b/git-am.sh\n@@ -289,7 +289,7 @@ split_patches () {\n prec=4\n dotest=\"$GIT_DIR/rebase-apply\"\n sign= utf8=t keep= skip= interactive= resolved= rebasing= abort=\n-resolvemsg= resume= scissors=\n+resolvemsg= resume= scissors= no_inbody_headers=\n git_apply_opt=\n committer_date_is_author_date=\n ignore_date=\n@@ -322,7 +322,7 @@ do\n \t--abort)\n \t\tabort=t ;;\n \t--rebasing)\n-\t\trebasing=t threeway=t keep=t scissors=f ;;\n+\t\trebasing=t threeway=t keep=t scissors=f no_inbody_headers=t ;;\n \t-d|--dotest)\n \t\tdie \"-d option is no longer supported. Do not use.\"\n \t\t;;\n@@ -448,6 +448,7 @@ else\n \techo \"$utf8\" >\"$dotest/utf8\"\n \techo \"$keep\" >\"$dotest/keep\"\n \techo \"$scissors\" >\"$dotest/scissors\"\n+\techo \"$no_inbody_headers\" >\"$dotest/no_inbody_headers\"\n \techo \"$GIT_QUIET\" >\"$dotest/quiet\"\n \techo 1 >\"$dotest/next\"\n \tif test -n \"$rebasing\"\n@@ -495,6 +496,12 @@ t)\n f)\n \tscissors=--no-scissors ;;\n esac\n+if test \"$(cat \"$dotest/no_inbody_headers\")\" = t\n+then\n+\tno_inbody_headers=--no-inbody-headers\n+else\n+\tno_inbody_headers=\n+fi\n if test \"$(cat \"$dotest/quiet\")\" = t\n then\n \tGIT_QUIET=t\n@@ -549,7 +556,7 @@ do\n \t# by the user, or the user can tell us to do so by --resolved flag.\n \tcase \"$resume\" in\n \t'')\n-\t\tgit mailinfo $keep $scissors $utf8 \"$dotest/msg\" \"$dotest/patch\" \\\n+\t\tgit mailinfo $keep $no_inbody_headers $scissors $utf8 \"$dotest/msg\" \"$dotest/patch\" \\\n \t\t\t<\"$dotest/$msgnum\" >\"$dotest/info\" ||\n \t\t\tstop_here $this\n\ndiff --git a/t/t5100-mailinfo.sh b/t/t5100-mailinfo.sh\nindex 0279d07..ebc36c1 100755\n--- a/t/t5100-mailinfo.sh\n+++ b/t/t5100-mailinfo.sh\n@@ -11,7 +11,7 @@ test_expect_success 'split sample box' \\\n \t'git mailsplit -o. \"$TEST_DIRECTORY\"/t5100/sample.mbox >last &&\n \tlast=`cat last` &&\n \techo total is $last &&\n-\ttest `cat last` = 14'\n+\ttest `cat last` = 16'\n\n check_mailinfo () {\n \tmail=$1 opt=$2\n@@ -30,6 +30,10 @@ do\n \t\tif test -f \"$TEST_DIRECTORY\"/t5100/msg$mail--scissors\n \t\tthen\n \t\t\tcheck_mailinfo $mail --scissors\n+\t\tfi &&\n+\t\tif test -f \"$TEST_DIRECTORY\"/t5100/msg$mail--no-inbody-headers\n+\t\tthen\n+\t\t\tcheck_mailinfo $mail --no-inbody-headers\n \t\tfi\n \t'\n done\ndiff --git a/t/t5100/info0015 b/t/t5100/info0015\nnew file mode 100644\nindex 0000000..0114f10\n--- /dev/null\n+++ b/t/t5100/info0015\n@@ -0,0 +1,5 @@\n+Author:\n+Email:\n+Subject: check bogus body header (from)\n+Date: Fri, 9 Jun 2006 00:44:16 -0700\n+\ndiff --git a/t/t5100/info0015--no-inbody-headers b/t/t5100/info0015--no-inbody-headers\nnew file mode 100644\nindex 0000000..c4d8d77\n--- /dev/null\n+++ b/t/t5100/info0015--no-inbody-headers\n@@ -0,0 +1,5 @@\n+Author: A U Thor\n+Email: a.u.thor@example.com\n+Subject: check bogus body header (from)\n+Date: Fri, 9 Jun 2006 00:44:16 -0700\n+\ndiff --git a/t/t5100/info0016 b/t/t5100/info0016\nnew file mode 100644\nindex 0000000..38ccd0d\n--- /dev/null\n+++ b/t/t5100/info0016\n@@ -0,0 +1,5 @@\n+Author: A U Thor\n+Email: a.u.thor@example.com\n+Subject: check bogus body header (date)\n+Date: bogus\n+\ndiff --git a/t/t5100/info0016--no-inbody-headers b/t/t5100/info0016--no-inbody-headers\nnew file mode 100644\nindex 0000000..f4857d4\n--- /dev/null\n+++ b/t/t5100/info0016--no-inbody-headers\n@@ -0,0 +1,5 @@\n+Author: A U Thor\n+Email: a.u.thor@example.com\n+Subject: check bogus body header (date)\n+Date: Fri, 9 Jun 2006 00:44:16 -0700\n+\ndiff --git a/t/t5100/msg0015 b/t/t5100/msg0015\nnew file mode 100644\nindex 0000000..9577238\n--- /dev/null\n+++ b/t/t5100/msg0015\n@@ -0,0 +1,2 @@\n+- a list\n+ - of stuff\ndiff --git a/t/t5100/msg0015--no-inbody-headers b/t/t5100/msg0015--no-inbody-headers\nnew file mode 100644\nindex 0000000..be5115b\n--- /dev/null\n+++ b/t/t5100/msg0015--no-inbody-headers\n@@ -0,0 +1,3 @@\n+From: bogosity\n+ - a list\n+ - of stuff\ndiff --git a/t/t5100/msg0016 b/t/t5100/msg0016\nnew file mode 100644\nindex 0000000..0d9adad\n--- /dev/null\n+++ b/t/t5100/msg0016\n@@ -0,0 +1,2 @@\n+and some content\n+\ndiff --git a/t/t5100/msg0016--no-inbody-headers b/t/t5100/msg0016--no-inbody-headers\nnew file mode 100644\nindex 0000000..1063f51\n--- /dev/null\n+++ b/t/t5100/msg0016--no-inbody-headers\n@@ -0,0 +1,4 @@\n+Date: bogus\n+\n+and some content\n+\ndiff --git a/t/t5100/patch0015 b/t/t5100/patch0015\nnew file mode 100644\nindex 0000000..ad64848\n--- /dev/null\n+++ b/t/t5100/patch0015\n@@ -0,0 +1,8 @@\n+---\n+diff --git a/foo b/foo\n+index e69de29..d95f3ad 100644\n+--- a/foo\n++++ b/foo\n+@@ -0,0 +1 @@\n++content\n+\ndiff --git a/t/t5100/patch0015--no-inbody-headers b/t/t5100/patch0015--no-inbody-headers\nnew file mode 100644\nindex 0000000..ad64848\n--- /dev/null\n+++ b/t/t5100/patch0015--no-inbody-headers\n@@ -0,0 +1,8 @@\n+---\n+diff --git a/foo b/foo\n+index e69de29..d95f3ad 100644\n+--- a/foo\n++++ b/foo\n+@@ -0,0 +1 @@\n++content\n+\ndiff --git a/t/t5100/patch0016 b/t/t5100/patch0016\nnew file mode 100644\nindex 0000000..ad64848\n--- /dev/null\n+++ b/t/t5100/patch0016\n@@ -0,0 +1,8 @@\n+---\n+diff --git a/foo b/foo\n+index e69de29..d95f3ad 100644\n+--- a/foo\n++++ b/foo\n+@@ -0,0 +1 @@\n++content\n+\ndiff --git a/t/t5100/patch0016--no-inbody-headers b/t/t5100/patch0016--no-inbody-headers\nnew file mode 100644\nindex 0000000..ad64848\n--- /dev/null\n+++ b/t/t5100/patch0016--no-inbody-headers\n@@ -0,0 +1,8 @@\n+---\n+diff --git a/foo b/foo\n+index e69de29..d95f3ad 100644\n+--- a/foo\n++++ b/foo\n+@@ -0,0 +1 @@\n++content\n+\ndiff --git a/t/t5100/sample.mbox b/t/t5100/sample.mbox\nindex 13fa4ae..de10312 100644\n--- a/t/t5100/sample.mbox\n+++ b/t/t5100/sample.mbox\n@@ -650,3 +650,36 @@ index b0b5d8f..461c47e 100644\n \t\tconvert_to_utf8(line, charset.buf);\n --\n 1.6.4.1\n+From nobody Mon Sep 17 00:00:00 2001\n+From: A U Thor \n+Subject: check bogus body header (from)\n+Date: Fri, 9 Jun 2006 00:44:16 -0700\n+\n+From: bogosity\n+ - a list\n+ - of stuff\n+---\n+diff --git a/foo b/foo\n+index e69de29..d95f3ad 100644\n+--- a/foo\n++++ b/foo\n+@@ -0,0 +1 @@\n++content\n+\n+From nobody Mon Sep 17 00:00:00 2001\n+From: A U Thor \n+Subject: check bogus body header (date)\n+Date: Fri, 9 Jun 2006 00:44:16 -0700\n+\n+Date: bogus\n+\n+and some content\n+\n+---\n+diff --git a/foo b/foo\n+index e69de29..d95f3ad 100644\n+--- a/foo\n++++ b/foo\n+@@ -0,0 +1 @@\n++content\n+\n-- \n1.6.4.4\n"}]}