{"thread":{"id":"20106","subject":"[PATCH] git-am: less strong format \"mbox\" detection","startedAt":"2009-07-14T06:40:47Z","lastAt":"2009-07-15T16:19:26Z","messageCount":10,"participants":["Nicolas Sebrecht","Giuseppe Bilotta","Johannes Sixt","Junio C Hamano","Derek Fawcus"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"117935","messageId":"bb3a84e2b817268a88832dc7043383e4b91a3df3.1247553623.git.ni.s@laposte.net","threadId":"20106","inReplyTo":null,"subject":"[PATCH] git-am: less strong format \"mbox\" detection","fromName":"Nicolas Sebrecht","fromEmail":"ni.s@laposte.net","sentAt":"2009-07-14T06:40:47Z","receivedAt":"2009-07-14T06:40:47Z","isPatch":true,"sender":{"key":"nicolas.s.dev@gmx.fr","avatar":null},"body":"Thunderbird (v1.* at least) likes to start e-mails with \"X-Account-Key:\".\nAlso, I have some emails starting with \"Return-Path:\" or \"\"Delivered-To:\".\n\nSigned-off-by: Nicolas Sebrecht \n---\n git-am.sh | 2 +-\n 1 files changed, 1 insertions(+), 1 deletions(-)\n\ndiff --git a/git-am.sh b/git-am.sh\nindex d64d997..d10a8e0 100755\n--- a/git-am.sh\n+++ b/git-am.sh\n@@ -169,7 +169,7 @@ check_patch_format () {\n \t\tread l2\n \t\tread l3\n \t\tcase \"$l1\" in\n-\t\t\"From \"* | \"From: \"*)\n+\t\t\"From \"* | \"From: \"* | \"X-Account-Key:\"* | \"Return-Path:\"* | \"Delivered-To:\"*)\n \t\t\tpatch_format=mbox\n \t\t\t;;\n \t\t'# This series applies on GIT commit'*)\n-- \n1.6.4.rc0.121.g2937a.dirty\n"},{"id":"117937","messageId":"cb7bb73a0907140016r4807c008h9c98f76200e9c3a5@mail.gmail.com","threadId":"20106","inReplyTo":"bb3a84e2b817268a88832dc7043383e4b91a3df3.1247553623.git.ni.s@laposte.net","subject":"Re: [PATCH] git-am: less strong format \"mbox\" detection","fromName":"Giuseppe Bilotta","fromEmail":"giuseppe.bilotta@gmail.com","sentAt":"2009-07-14T07:16:24Z","receivedAt":"2009-07-14T07:16:24Z","isPatch":true,"sender":{"key":"giuseppe.bilotta@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1464?v=4"},"body":"On Tue, Jul 14, 2009 at 8:40 AM, Nicolas Sebrecht wrote:\n> Thunderbird (v1.* at least) likes to start e-mails with \"X-Account-Key:\".\n> Also, I have some emails starting with \"Return-Path:\" or \"\"Delivered-To:\".\n>\n> Signed-off-by: Nicolas Sebrecht \n> ---\n>  git-am.sh |    2 +-\n>  1 files changed, 1 insertions(+), 1 deletions(-)\n>\n> diff --git a/git-am.sh b/git-am.sh\n> index d64d997..d10a8e0 100755\n> --- a/git-am.sh\n> +++ b/git-am.sh\n> @@ -169,7 +169,7 @@ check_patch_format () {\n>                read l2\n>                read l3\n>                case \"$l1\" in\n> -               \"From \"* | \"From: \"*)\n> +               \"From \"* | \"From: \"* | \"X-Account-Key:\"* | \"Return-Path:\"* | \"Delivered-To:\"*)\n\nNitpick: for consistency, should we either expect a space after the\ncolon also in the new keys, or not expect i in the From: key either. I\ndon't think the RFC requires a space, but most clients probably add\nit.\n\n-- \nGiuseppe \"Oblomov\" Bilotta\n"},{"id":"117939","messageId":"20090714082059.GA13808@vidovic","threadId":"20106","inReplyTo":"cb7bb73a0907140016r4807c008h9c98f76200e9c3a5@mail.gmail.com","subject":"[PATCH] Re: git-am: less strong format \"mbox\" detection","fromName":"Nicolas Sebrecht","fromEmail":"nicolas.s.dev@gmx.fr","sentAt":"2009-07-14T08:20:59Z","receivedAt":"2009-07-14T08:20:59Z","isPatch":true,"sender":{"key":"nicolas.s.dev@gmx.fr","avatar":null},"body":"Le 14/07/09, Giuseppe Bilotta a écrit :\n\n> > diff --git a/git-am.sh b/git-am.sh\n> > index d64d997..d10a8e0 100755\n> > --- a/git-am.sh\n> > +++ b/git-am.sh\n> > @@ -169,7 +169,7 @@ check_patch_format () {\n> >                read l2\n> >                read l3\n> >                case \"$l1\" in\n> > -               \"From \"* | \"From: \"*)\n> > +               \"From \"* | \"From: \"* | \"X-Account-Key:\"* | \"Return-Path:\"* | \"Delivered-To:\"*)\n> \n> Nitpick: for consistency, should we either expect a space after the\n> colon also in the new keys, or not expect i in the From: key either. I\n> don't think the RFC requires a space, but most clients probably add\n> it.\n\nRFC 822 says:\n \n\" 3.4.2. WHITE SPACE\n \n Note: In structured field bodies, multiple linear space ASCII\n characters (namely HTABs and SPACEs) are treated as\n single spaces and may freely surround any symbol. In\n all header fields, the only place in which at least one\n LWSP-char is REQUIRED is at the beginning of continua-\n tion lines in a folded field.\n\"\n\nA trailing space after the colon is not required. I'll remove it and\nresend a patch.\n\nAnd why should we accept \"From \"?\n\n-- \nNicolas Sebrecht\n"},{"id":"117942","messageId":"4A5C436E.8000304@viscovery.net","threadId":"20106","inReplyTo":"20090714082059.GA13808@vidovic","subject":"Re: [PATCH] Re: git-am: less strong format \"mbox\" detection","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2009-07-14T08:35:58Z","receivedAt":"2009-07-14T08:35:58Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Nicolas Sebrecht schrieb:\n> Le 14/07/09, Giuseppe Bilotta a écrit :\n> \n>>> diff --git a/git-am.sh b/git-am.sh\n>>> index d64d997..d10a8e0 100755\n>>> --- a/git-am.sh\n>>> +++ b/git-am.sh\n>>> @@ -169,7 +169,7 @@ check_patch_format () {\n>>> read l2\n>>> read l3\n>>> case \"$l1\" in\n>>> - \"From \"* | \"From: \"*)\n>>> + \"From \"* | \"From: \"* | \"X-Account-Key:\"* | \"Return-Path:\"* | \"Delivered-To:\"*)\n> \n> And why should we accept \"From \"?\n\nBecause mbox format must begin with \"From \".\n\nThe question is rather: Why should we accept anything else? The case arm\nin question is about detecting mbox format.\n\n-- Hannes\n"},{"id":"117943","messageId":"7v8wirirki.fsf@alter.siamese.dyndns.org","threadId":"20106","inReplyTo":"20090714082059.GA13808@vidovic","subject":"Re: [PATCH] Re: git-am: less strong format \"mbox\" detection","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-07-14T08:42:21Z","receivedAt":"2009-07-14T08:42:21Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nicolas Sebrecht writes:\n\n> And why should we accept \"From \"?\n\nYou are going totally in a wrong way around with this.\n\nBerkeley mbox format is what we support, and \"From \" is the _only_\ndelimiter between pieces of e-mail in the file. We happen to allow \"From:\n\" in order to merely be extra nice for people who create mbox looking file\nby hand; I think it is an improvement not to require an optional SP after\nthe colon there, but that is totally an independent issue.\n\nI cannot offhand say if allowing anything but \"From:\" is necessarily an\nimprovement, or making the format detection unnecessarily risky of\nmisidentification. I do not particularly like the idea of allowing only\nsome randomly selected fields like Return-Path and Delibered-To and not\naccepting others, let alone totally nonstandard X-Foo fields.\n"},{"id":"117954","messageId":"20090714122354.GA13806@vidovic","threadId":"20106","inReplyTo":"7v8wirirki.fsf@alter.siamese.dyndns.org","subject":"[PATCH] Re: git-am: less strong format \"mbox\" detection","fromName":"Nicolas Sebrecht","fromEmail":"nicolas.s.dev@gmx.fr","sentAt":"2009-07-14T12:23:54Z","receivedAt":"2009-07-14T12:23:54Z","isPatch":true,"sender":{"key":"nicolas.s.dev@gmx.fr","avatar":null},"body":"The 14/07/09, Junio C Hamano wrote:\n\n> Nicolas Sebrecht writes:\n> \n> > And why should we accept \"From \"?\n> \n> You are going totally in a wrong way around with this.\n> \n> Berkeley mbox format is what we support, and \"From \" is the _only_\n> delimiter between pieces of e-mail in the file. We happen to allow \"From:\n> \" in order to merely be extra nice for people who create mbox looking file\n> by hand; I think it is an improvement not to require an optional SP after\n> the colon there, but that is totally an independent issue.\n\nI see, thank you.\n\n> I cannot offhand say if allowing anything but \"From:\" is necessarily an\n> improvement, or making the format detection unnecessarily risky of\n> misidentification. I do not particularly like the idea of allowing only\n> some randomly selected fields like Return-Path and Delibered-To and not\n> accepting others, let alone totally nonstandard X-Foo fields.\n\nI'll look at the source closer to know how it can be done the smart way.\n\nAs the manual of git-am says it accepts maildir format too (checked\nnow), we have a regression since\n\n\ta5a6755a1d4707bf2fab7752e5c974ebf63d086a\n\nin case of maildir _and_ \"verbatim\" emails starting with anything else that\n\"From \" or \"From: \".\n\nI think the RFC doesn't require any fixed order for the header fields\n(will check). So, I'm not very optimist because format detection starts\nwith\n\n\tread l1\n\tread l2\n\tread l3\n\nwich doesn't help to make a difference between mailbox and maildir.\n\n\n-- \nNicolas Sebrecht\n"},{"id":"117999","messageId":"2433101adeafddeab78815083446552ff3ea9f49.1247636959.git.nicolas.s.dev@gmx.fr","threadId":"20106","inReplyTo":"20090714122354.GA13806@vidovic","subject":"[PATCH v2] git-am: fix maildir support regression for unordered headers in emails","fromName":"Nicolas Sebrecht","fromEmail":"nicolas.s.dev@gmx.fr","sentAt":"2009-07-15T05:52:36Z","receivedAt":"2009-07-15T05:52:36Z","isPatch":true,"sender":{"key":"nicolas.s.dev@gmx.fr","avatar":null},"body":"Patch format detection introduced by a5a6755a1d4707bf2fab7752e5c974ebf63d086a\nmay refuse valid patches from verbatim emails.\n\nEmails may have header fields in a random order.\n\nSigned-off-by: Nicolas Sebrecht \n---\n git-am.sh | 10 ++++++++++\n 1 files changed, 10 insertions(+), 0 deletions(-)\n\ndiff --git a/git-am.sh b/git-am.sh\nindex d64d997..18e53d0 100755\n--- a/git-am.sh\n+++ b/git-am.sh\n@@ -146,6 +146,7 @@ clean_abort () {\n }\n \n patch_format=\n+is_verbatim_email=\n \n check_patch_format () {\n \t# early return if patch_format was set from the command line\n@@ -191,6 +192,15 @@ check_patch_format () {\n \t\t\tesac\n \t\t\t;;\n \t\tesac\n+\t\t# Keep maildir workflows support.\n+\t\t# Verbatim emails may have header fields in random order.\n+\t\tis_verbatim_email='true'\n+\t\tfor line in \"$l1\" \"$l2\" \"$l3\"; do\n+\t\t\tprintf \"$line\" | grep --quiet --extended-regexp '^([^\\ ])+: +.*' ||\n+\t\t\t\tis_verbatim_email='false'\n+\t\tdone\n+\t\t# next treatments don't differ from mailbox format\n+\t\t[[ $is_verbatim_email == 'true' ]] && patch_format=mbox\n \t} < \"$1\" || clean_abort\n }\n \n-- \n1.6.4.rc0.129.gf738\n"},{"id":"118004","messageId":"7vljmqflti.fsf@alter.siamese.dyndns.org","threadId":"20106","inReplyTo":"2433101adeafddeab78815083446552ff3ea9f49.1247636959.git.nicolas.s.dev@gmx.fr","subject":"Re: [PATCH v2] git-am: fix maildir support regression for unordered headers in emails","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-07-15T07:27:05Z","receivedAt":"2009-07-15T07:27:05Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nicolas Sebrecht writes:\n\n> Patch format detection introduced by a5a6755a1d4707bf2fab7752e5c974ebf63d086a\n> may refuse valid patches from verbatim emails.\n\nIt is unclear what you meant by \"verbatim email\". A verbatim e-mail\nin mbox begins with \"From \" header that is already covered in the existing\ncode long before support for stgit/hg was added.\n\n> +\t\t# Keep maildir workflows support.\n> +\t\t# Verbatim emails may have header fields in random order.\n> +\t\tis_verbatim_email='true'\n\nWe do not fold lines like this,\n\n> +\t\tfor line in \"$l1\" \"$l2\" \"$l3\"; do\n\nInstead, we write like this:\n\n\tfor x in a b c\n\tdo\n\t\tcmd ...\n\n> +\t\t\tprintf \"$line\" | grep --quiet --extended-regexp '^([^\\ ])+: +.*' ||\n\nWe use GNUism spelling --extended-regexp nowhere in scripted Porcelains;\njust say -E here, and do not omit -e before the pattern.\n\nLikewise for --quiet. Just say -q. When in doubt, be conservative and\nstick to POSIX for portability.\n\n http://www.opengroup.org/onlinepubs/9699919799/utilities/grep.html#tag_20_55\n\nYour regexp has too many issues.\n\n - It is too loose and too strict at the same time. RFC 2822 section 2.2\n and 3.6.8 specify that a field name must be composed of printable\n US-ASCII characters except colon and space, so if you really want to be\n lenient, it should instead begin with [^: ]+: (and you do not need any\n capture).\n\n - I think however starting the regexp with \"^[A-Za-z]+(-[A-Za-z]+)*:\"\n would be more appropriate in practice, though.\n\n - You do not need to end the expression with \".*\"; omitting that would\n match the same set of lines anyway.\n\n - I thought you were advocating for not requiring SP after the colon?\n\n - What happens if $l2 or $l3 is a subsequent folded line? For example,\n in my MUA edit buffer, this message begins with:\n\n To: Nicolas Sebrecht \n Cc: ,\n Giuseppe Bilotta ,\n Johannes Sixt \n Subject: Re: [PATCH v2] git-am: fix maildir support ...\n\n The third line would not match your regexp.\n\n> +\t\t\t\tis_verbatim_email='false'\n> +\t\tdone\n> +\t\t# next treatments don't differ from mailbox format\n> +\t\t[[ $is_verbatim_email == 'true' ]] && patch_format=mbox\n\nWe do not use non-portable [[ ]] anywhere in our shell script. Write\n\n\tif test true = \"$is_verbatim_email\"\n then\n \tpatch_format=mbox\n\tfi\n\nif you really want to keep this code structure.\n\nI actually do not think you would even need an extra is_verbatim_email\nvariable, though. Assuming that I understand what you are trying to do,\nthis is probably how I would write it:\n\n\tsed -e '/^$/q' -e '/^[ \t]/d' \"$1\" |\n grep -v -E -e '^[A-Za-z]+(-[A-Za-z]+)*:' >/dev/null ||\n patch_format=mbox\n\nBut I am not convinced that I understand what _problem_ you are trying to\nsolve in the first place.\n"},{"id":"118019","messageId":"20090715125419.GA21811@gpk-lds-007.cisco.com","threadId":"20106","inReplyTo":"7vljmqflti.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v2] git-am: fix maildir support regression for unordered headers in emails","fromName":"Derek Fawcus","fromEmail":"dfawcus@cisco.com","sentAt":"2009-07-15T12:54:19Z","receivedAt":"2009-07-15T12:54:19Z","isPatch":true,"sender":{"key":"dfawcus@cisco.com","avatar":null},"body":"On Wed, Jul 15, 2009 at 12:27:05AM -0700, Junio C Hamano wrote:\n> Nicolas Sebrecht writes:\n> \n> > Patch format detection introduced by a5a6755a1d4707bf2fab7752e5c974ebf63d086a\n> > may refuse valid patches from verbatim emails.\n> \n> It is unclear what you meant by \"verbatim email\". A verbatim e-mail\n> in mbox begins with \"From \" header that is already covered in the existing\n> code long before support for stgit/hg was added.\n\nI believe he is referring to the claimed support for maildir format boxes.\n\n> But I am not convinced that I understand what _problem_ you are trying to\n> solve in the first place.\n\nAssuming it is maildir support, then there is no 'header' as such in the\nfile which can be detected. One could try and detect that the contents\nare structured as an RFC822 message (but with local line ends), or one\ncould try and detect that the file is within a maildir folder.\n\nIt seems this patch is taking the former approach and trying to ensure\nthe file consists of header fields.\n"},{"id":"118037","messageId":"20090715161926.GA12935@vidovic","threadId":"20106","inReplyTo":"20090715125419.GA21811@gpk-lds-007.cisco.com","subject":"[PATCH v2] Re: git-am: fix maildir support regression for unordered headers in emails","fromName":"Nicolas Sebrecht","fromEmail":"nicolas.s.dev@gmx.fr","sentAt":"2009-07-15T16:19:26Z","receivedAt":"2009-07-15T16:19:26Z","isPatch":true,"sender":{"key":"nicolas.s.dev@gmx.fr","avatar":null},"body":"The 15/07/09, Derek Fawcus wrote:\n> On Wed, Jul 15, 2009 at 12:27:05AM -0700, Junio C Hamano wrote:\n>\n> > It is unclear what you meant by \"verbatim email\". A verbatim e-mail\n> > in mbox begins with \"From \" header that is already covered in the existing\n> > code long before support for stgit/hg was added.\n> \n> I believe he is referring to the claimed support for maildir format boxes.\n\nYou're right. In a maildir each email is the file as is.\n\n> > But I am not convinced that I understand what _problem_ you are trying to\n> > solve in the first place.\n> \n> Assuming it is maildir support, then there is no 'header' as such in the\n> file which can be detected.\n\nTrue.\n\n> One could try and detect that the contents\n> are structured as an RFC822 message (but with local line ends), or one\n> could try and detect that the file is within a maildir folder.\n> \n> It seems this patch is taking the former approach and trying to ensure\n> the file consists of header fields.\n\nYou're perfectly right. I think it's the best approach because if the\nfiles are moved to another folder (the repo?), they are still valid\npatches.\n\n\n-- \nNicolas Sebrecht\n"}]}