{"thread":{"id":"20125","subject":"[PATCH v3] git-am: fix maildir support regression: accept email file as patch","startedAt":"2009-07-15T22:19:36Z","lastAt":"2009-08-06T17:07:50Z","messageCount":26,"participants":["Nicolas Sebrecht","Junio C Hamano","Stephen Boyd","Johannes Sixt","Nanako Shiraishi"],"isPatch":true,"patchVersion":3,"patchTotal":null},"messages":[{"id":"118049","messageId":"1654b578a7e045b564c46df4abb6e2196422c2b2.1247696294.git.nicolas.s.dev@gmx.fr","threadId":"20125","inReplyTo":null,"subject":"[PATCH v3] git-am: fix maildir support regression: accept email file as patch","fromName":"Nicolas Sebrecht","fromEmail":"nicolas.s.dev@gmx.fr","sentAt":"2009-07-15T22:19:36Z","receivedAt":"2009-07-15T22:19:36Z","isPatch":true,"sender":{"key":"nicolas.s.dev@gmx.fr","avatar":null},"body":"Patch format detection introduced by a5a6755a1d4707bf2fab7752e5c974ebf63d086a\nmay refuse valid patches.\n\nWe keep detection on the first three lines. Emails may have:\n - header fields in a random order;\n - folded lines.\n\nSigned-off-by: Nicolas Sebrecht \n---\n git-am.sh | 17 +++++++++++++++++\n 1 files changed, 17 insertions(+), 0 deletions(-)\n\ndiff --git a/git-am.sh b/git-am.sh\nindex d64d997..6190297 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_email=\n \n check_patch_format () {\n \t# early return if patch_format was set from the command line\n@@ -191,6 +192,22 @@ check_patch_format () {\n \t\t\tesac\n \t\t\t;;\n \t\tesac\n+\t\t# Keep maildir workflows support.\n+\t\t# Emails may have header fields in random order.\n+\t\tis_email='true'\n+\t\tfor line in \"$l1\" \"$l2\" \"$l3\"\n+\t\tdo\n+\t\t\tprintf \"$line\" |\n+\t\t\t\t# The line may be a folded line\n+\t\t\t\tsed -e '/^$/q' -e '/^[ ]/d' |\n+\t\t\t\tgrep -E -e '^[A-Za-z]+(-[A-Za-z]+)*:' >/dev/null ||\n+\t\t\t\tis_email='false'\n+\t\tdone\n+\t\t# next treatments don't differ from mailbox format\n+\t\tif [ $is_email == 'true' ]\n+\t\tthen\n+\t\t\tpatch_format=mbox\n+\t\tfi\n \t} < \"$1\" || clean_abort\n }\n \n-- \n1.6.4.rc0.128.g69018\n"},{"id":"118053","messageId":"20090715224308.GA12971@vidovic","threadId":"20125","inReplyTo":"1654b578a7e045b564c46df4abb6e2196422c2b2.1247696294.git.nicolas.s.dev@gmx.fr","subject":"[PATCH v3] Re: git-am: fix maildir support regression: accept email file as patch","fromName":"Nicolas Sebrecht","fromEmail":"nicolas.s.dev@gmx.fr","sentAt":"2009-07-15T22:43:08Z","receivedAt":"2009-07-15T22:43:08Z","isPatch":true,"sender":{"key":"nicolas.s.dev@gmx.fr","avatar":null},"body":"The 16/07/09, Nicolas Sebrecht wrote:\n\n> Patch format detection introduced by a5a6755a1d4707bf2fab7752e5c974ebf63d086a\n> may refuse valid patches.\n> \n> We keep detection on the first three lines. Emails may have:\n> - header fields in a random order;\n> - folded lines.\n\nI realized this patch is wrong. Please ignore it.\n\n-- \nNicolas Sebrecht\n"},{"id":"118054","messageId":"7v1voheevy.fsf@alter.siamese.dyndns.org","threadId":"20125","inReplyTo":"1654b578a7e045b564c46df4abb6e2196422c2b2.1247696294.git.nicolas.s.dev@gmx.fr","subject":"Re: [PATCH v3] git-am: fix maildir support regression: accept email file as patch","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-07-15T22:54:25Z","receivedAt":"2009-07-15T22:54:25Z","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.\n\nThis message is even worse than the previous round.\n\nBy definition what git-am does not accept is invalid, and you are\nloosening that definition to include something else newly as valid.\n\nPlease describe what that new something is.\n\nIf you are claiming a5a6755 (git-am foreign patch support: introduce\npatch_format, 2009-05-27) is an regression, and before it X, Y and Z\nformat were supported but after that only X and Y format are, then for the\nsame reason please specify what that Z you are resurrecting the support\nfor is.\n\nThat way, people who have been frustrated that their randomly formatted\nfiles were not processible without first converting them to mbox format\nwill know that now their favorite format is now/again also supported.\n\n> We keep detection on the first three lines. Emails may have:\n> - header fields in a random order;\n> - folded lines.\n>\n> Signed-off-by: Nicolas Sebrecht \n> ---\n> git-am.sh | 17 +++++++++++++++++\n> 1 files changed, 17 insertions(+), 0 deletions(-)\n>\n\nPlease do not break the thread. Make this message a response to my\nmessage you lifted the idea from, which in turn was sent as a response in\nthe thread of v2 patch.\n\n\n> +\t\tfor line in \"$l1\" \"$l2\" \"$l3\"\n> +\t\tdo\n> +\t\t\tprintf \"$line\" |\n> +\t\t\t\t# The line may be a folded line\n> +\t\t\t\tsed -e '/^$/q' -e '/^[ ]/d' |\n> +\t\t\t\tgrep -E -e '^[A-Za-z]+(-[A-Za-z]+)*:' >/dev/null ||\n> +\t\t\t\tis_email='false'\n\nRunning three independent printf piped to two processes in a loop is\nquite silly.\n\nI think you did not understand the point of the three liner I sent you.\n\n\tsed -e '/^$/q' -e '/^[ \t]/d' \"$1\" |\n\n The point of this is not to use the silly \"we only look at the first\n three lines\" rule. Instead, it ignores these l1/l2/l3, but grabs all\n the header lines, but discards second and subsequent physical lines if\n a logical line was folded. Which means that the effect of this is to\n pipe the whole header (again, without worrying about the indented\n remainder of folded lines) to downsream, which is the grep -v below\n\n grep -v -E -e '^[A-Za-z]+(-[A-Za-z]+)*:' >/dev/null ||\n\n This checks if there is a line that does _NOT_ match the usual\n e-mail header pattern. If there is such a line, it means that the\n file is not an e-mail message. If there is no such line, we say...\n\n patch_format=mbox\n\nOne caveat is that the above logic alone won't catch a random file that\ndoes not have _any_ e-mail headers in it. So you might need to do\nsomething like:\n\n\tLF='\n '\n\tcase \"$l1$LF$l2$LF$l3\" in\n *\"$LF$LF\"*)\n \t# has a completely empty line in there?\n # that means the message has only two headers at most;\n # that cannot be an email.\n\t\t;;\n\t*)\n sed -e '/^$/q' -e '/^[ \t]/d' \"$1\" |\n grep -v -E -e '^[A-Za-z]+(-[A-Za-z]+)*:' >/dev/null ||\n patch_format=mbox\n\tesac\n\nto completely replace that \"for line in...\" loop.\n"},{"id":"118055","messageId":"7vfxcxcxg1.fsf@alter.siamese.dyndns.org","threadId":"20125","inReplyTo":"7v1voheevy.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v3] git-am: fix maildir support regression: accept email file as patch","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-07-15T23:56:30Z","receivedAt":"2009-07-15T23:56:30Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Please describe what that new something is.\n>...\n> Running three independent printf piped to two processes in a loop is\n> quite silly.\n> ...\n\nThat is, something like this.\n\n-- >8 --\nSubject: mailinfo: allow individual e-mail files as input\n\nWe traditionally allowed a mbox file or a directory name of a maildir (but\nnever an individual file inside a maildir) to be given to \"git am\". Even\nthough an individual file in a maildir (or more generally, a piece of\nRFC2822 e-mail) is not a mbox file, it contains enough information to\ncreate a commit out of it, so there is no reason to reject one. Running\nmailsplit on such a file feels stupid, but it does not hurt.\n\nThis builds on top of a5a6755 (git-am foreign patch support: introduce\npatch_format, 2009-05-27) that introduced mailbox format detection. The\ncodepath to deal with a mbox requires it to begin with \"From \" line and\nalso allows it to begin with \"From: \", but a random piece of e-mail can\nand often do begin with any valid RFC2822 header lines.\n\nInstead of checking the first line, we extract all the lines up to the\nfirst empty line, and make sure they look like e-mail headers.\n\nSigned-off-by: Junio C Hamano \n---\n git-am.sh | 14 ++++++++++++++\n 1 files changed, 14 insertions(+), 0 deletions(-)\n\ndiff --git a/git-am.sh b/git-am.sh\nindex d64d997..617ca2f 100755\n--- a/git-am.sh\n+++ b/git-am.sh\n@@ -191,6 +191,20 @@ check_patch_format () {\n \t\t\tesac\n \t\t\t;;\n \t\tesac\n+\t\tif test -z \"$patch_format\" &&\n+\t\t\ttest -n \"$l1\" &&\n+\t\t\ttest -n \"$l2\" &&\n+\t\t\ttest -n \"$l3\"\n+\t\tthen\n+\t\t\t# This begins with three non-empty lines. Is this a\n+\t\t\t# piece of e-mail a-la RFC2822? Grab all the headers,\n+\t\t\t# discarding the indented remainder of folded lines,\n+\t\t\t# and see if it looks like that they all begin with the\n+\t\t\t# header field names...\n+\t\t\tsed -n -e '/^$/q' -e '/^[ \t]/d' -e p \"$1\" |\n+\t\t\tgrep -v -E -e '^[A-Za-z]+(-[A-Za-z]+)*:' >/dev/null ||\n+\t\t\tpatch_format=mbox\n+\t\tfi\n \t} < \"$1\" || clean_abort\n }\n \n"},{"id":"118059","messageId":"20090716004952.GB12971@vidovic","threadId":"20125","inReplyTo":"7v1voheevy.fsf@alter.siamese.dyndns.org","subject":"[PATCH v3] Re: git-am: fix maildir support regression: accept email file as patch","fromName":"Nicolas Sebrecht","fromEmail":"nicolas.s.dev@gmx.fr","sentAt":"2009-07-16T00:49:52Z","receivedAt":"2009-07-16T00:49:52Z","isPatch":true,"sender":{"key":"nicolas.s.dev@gmx.fr","avatar":null},"body":"The 15/07/09, Junio C Hamano wrote:\n\n> I think you did not understand the point of the three liner I sent you.\n> \n> \tsed -e '/^$/q' -e '/^[ \t]/d' \"$1\" |\n\nI'll change it to\n\n \tsed -e '/^$/q' -e '/^[[:blank:]]/d' \"$1\" |\n\nto conform to reality (and the paragraph 3.4.2 of the RFC 822).\n\n> The point of this is not to use the silly \"we only look at the first\n> three lines\" rule. Instead, it ignores these l1/l2/l3, but grabs all\n> the header lines, but discards second and subsequent physical lines if\n> a logical line was folded. Which means that the effect of this is to\n> pipe the whole header (again, without worrying about the indented\n> remainder of folded lines) to downsream, which is the grep -v below\n> \n> grep -v -E -e '^[A-Za-z]+(-[A-Za-z]+)*:' >/dev/null ||\n>\n> This checks if there is a line that does _NOT_ match the usual\n> e-mail header pattern. If there is such a line, it means that the\n> file is not an e-mail message. If there is no such line, we say...\n\nI don't see the reason to have the option -v. It's only related to\nwhat's printed to output and doesn't change the exit status which\ntell us if an expression has matched.\n\nThis gives:\n\n grep -E -e '^[A-Za-z]+(-[A-Za-z]+)*:' >/dev/null &&\n patch_format=mbox\n\n> One caveat is that the above logic alone won't catch a random file that\n> does not have _any_ e-mail headers in it. So you might need to do\n> something like:\n> \n> \tLF='\n> '\n> \tcase \"$l1$LF$l2$LF$l3\" in\n> *\"$LF$LF\"*)\n> \t# has a completely empty line in there?\n> # that means the message has only two headers at most;\n> # that cannot be an email.\n> \t\t;;\n\nI think we can strip this part. The purpose is to accept what _may_ be a\npatch. Any wrong patch or random file will be rejected later.\n\n-- \nNicolas Sebrecht\n"},{"id":"118062","messageId":"20090716010001.GC12971@vidovic","threadId":"20125","inReplyTo":"7vfxcxcxg1.fsf@alter.siamese.dyndns.org","subject":"[PATCH v3] Re: git-am: fix maildir support regression: accept email file as patch","fromName":"Nicolas Sebrecht","fromEmail":"nicolas.s.dev@gmx.fr","sentAt":"2009-07-16T01:00:01Z","receivedAt":"2009-07-16T01:00:01Z","isPatch":true,"sender":{"key":"nicolas.s.dev@gmx.fr","avatar":null},"body":"The 15/07/09, Junio C Hamano wrote:\n\n> We traditionally allowed a mbox file or a directory name of a maildir (but\n> never an individual file inside a maildir) to be given to \"git am\". Even\n> though an individual file in a maildir (or more generally, a piece of\n> RFC2822 e-mail) is not a mbox file, it contains enough information to\n> create a commit out of it, so there is no reason to reject one. Running\n> mailsplit on such a file feels stupid, but it does not hurt.\n\nJunio, I'm sorry but you missed the point.\n\nIt is not about adding a new feature. It's about keeping compatibility\nwith maildir. The current version _rejects_ good patches.\n\nBut, as it's very easy to move emails from a maildir _or_ have a symlink\nwhich links to \"maildir/cur\" or whatever, we really should rely on the\ncontent of the files.\n\n-- \nNicolas Sebrecht\n"},{"id":"118063","messageId":"20090716020632.GD12971@vidovic","threadId":"20125","inReplyTo":"20090716010001.GC12971@vidovic","subject":"[PATCH v3] Re: git-am: fix maildir support regression: accept email file as patch","fromName":"Nicolas Sebrecht","fromEmail":"nicolas.s.dev@gmx.fr","sentAt":"2009-07-16T02:06:32Z","receivedAt":"2009-07-16T02:06:32Z","isPatch":true,"sender":{"key":"nicolas.s.dev@gmx.fr","avatar":null},"body":"The 16/07/09, Nicolas Sebrecht wrote:\n\n> It is not about adding a new feature. It's about keeping compatibility\n> with maildir. The current version _rejects_ good patches.\n> \n> But, as it's very easy to move emails from a maildir _or_ have a symlink\n> which links to \"maildir/cur\" or whatever, we really should rely on the\n> content of the files.\n\nHum, it's not true. \n\nSymlink as parameter didn't work before. So, it breaks things like:\n\n\t$ git am symlink/*\n\t$ git am directory/{anything relying on shell globbing}\n\t$ git am patch1 patch2\n\nThis was traditionally permitted even if git-am was not designed to. I\nwonder if we should add this feature as it could break end-user\nworkflows.\n\n-- \nNicolas Sebrecht\n"},{"id":"118066","messageId":"7vfxcxbbqw.fsf@alter.siamese.dyndns.org","threadId":"20125","inReplyTo":"20090716010001.GC12971@vidovic","subject":"Re: [PATCH v3] Re: git-am: fix maildir support regression: accept email file as patch","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-07-16T02:30:31Z","receivedAt":"2009-07-16T02:30:31Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nicolas Sebrecht writes:\n\n> It is not about adding a new feature. It's about keeping compatibility\n> with maildir. The current version _rejects_ good patches.\n\nYou almost gave me a heart attack. I wondered if the detection code, like\nit did when it saw a stgit series, saw that the parameter was a directory\n(in which case the original code blindly took it as a maildir), and added\nan extra check to see each file in that specified directory is a mbox or\nbegins with \"From: \", and reject it otherwise.\n\nIf it were what Giuseppe's patch did, clearly that would have been a\nbreakage that rejects a good maildir. So I looked at the code again.\n\nA very early part of check_patch_format says \"a directory? we say it is\nmbox (even though we should call it maildir) and let split_patches() call\ngit-mailsplit, as it knows how to deal with a maildir\".\n\nSo I do not think there is any breakage that rejects good input with his\npatch.\n\nI am not opposed to add support for individual pieces of e-mail without\nforcing them to be in Berkeley mbox format. Not everybody uses mbox\nformat, and it is a logical thing to do. Also I do not think the amount\nof new code necessary to do so is excessive, nor such a change is risky\neven late in a cycle after -rc0.\n\nI however _do_ have issues with labeling other's patch that did not break\nany documented behaviour as a regression, even if it is to get extra\nattention to the issue. That's not how we do things.\n"},{"id":"118068","messageId":"7v7hy9bb8k.fsf@alter.siamese.dyndns.org","threadId":"20125","inReplyTo":"20090716004952.GB12971@vidovic","subject":"Re: [PATCH v3] Re: git-am: fix maildir support regression: accept email file as patch","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-07-16T02:41:31Z","receivedAt":"2009-07-16T02:41:31Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nicolas Sebrecht writes:\n\n> I don't see the reason to have the option -v. It's only related to\n> what's printed to output and doesn't change the exit status which\n> tell us if an expression has matched.\n>\n> This gives:\n>\n> grep -E -e '^[A-Za-z]+(-[A-Za-z]+)*:' >/dev/null &&\n> patch_format=mbox\n\nThat grep says \"if you see a single line that matches the pattern, even if\nall the other lines are garbage, report it as a match\".\n\nSee \"something like this\" patch in my other message. It filters the entire\nheader to make sure there is no line that does not match (that is what -v\nis about), and make it report success when there is even a signle line\nthat does not.\n\n $ (echo yes; echo no) | grep -v -e yes ; echo $?\n no\n 0\n $ (echo yes; echo yes) | grep -v -e yes ; echo $?\n 1\n\nThat is why I wrote the \"how about this\" patch with \"||\" like this:\n\n grep -v -E -e '^[A-Za-z]+(-[A-Za-z]+)*:' >/dev/null ||\n patch_format=mbox\n\nIf everything is header, grep says \"nothing matches\", and patch_format\nis set to mbox.\n"},{"id":"118069","messageId":"20090716025921.GE12971@vidovic","threadId":"20125","inReplyTo":"7vfxcxbbqw.fsf@alter.siamese.dyndns.org","subject":"[PATCH v3] Re: git-am: fix maildir support regression: accept email file as patch","fromName":"Nicolas Sebrecht","fromEmail":"nicolas.s.dev@gmx.fr","sentAt":"2009-07-16T02:59:21Z","receivedAt":"2009-07-16T02:59:21Z","isPatch":true,"sender":{"key":"nicolas.s.dev@gmx.fr","avatar":null},"body":"The 15/07/09, Junio C Hamano wrote:\n> Nicolas Sebrecht writes:\n>\n> So I do not think there is any breakage that rejects good input with his\n> patch.\n\nExact. It broke my workflow (at least) and was able to do it because the\nprevious git-am was permiting it. Looking closer to the code made me\nunderstand where I was wrong.\n\n> I am not opposed to add support for individual pieces of e-mail without\n> forcing them to be in Berkeley mbox format. Not everybody uses mbox\n> format, and it is a logical thing to do. Also I do not think the amount\n> of new code necessary to do so is excessive, nor such a change is risky\n> even late in a cycle after -rc0.\n\nWill do, then.\n\n> I however _do_ have issues with labeling other's patch that did not break\n> any documented behaviour as a regression, even if it is to get extra\n> attention to the issue. That's not how we do things.\n\nOf course. As we was able to do more than documented, I did not see it\nin first place. It's a wrong assumption coming from my initial\ngit-bisect, some \"hidden\" globbing shell, and my learning curve of the\ncurrent code.\n\nIt is _NOT_ in my intention to blame anyone.\n\n-- \nNicolas Sebrecht\n"},{"id":"118072","messageId":"eae6ce0ac7ff600e3e7ab261a5f020651e39ddfa.1247717023.git.nicolas.s.dev@gmx.fr","threadId":"20125","inReplyTo":"7v7hy9bb8k.fsf@alter.siamese.dyndns.org","subject":"[PATCH v4] git-am: allow e-mail file(s) as input","fromName":"Nicolas Sebrecht","fromEmail":"nicolas.s.dev@gmx.fr","sentAt":"2009-07-16T04:05:23Z","receivedAt":"2009-07-16T04:05:23Z","isPatch":true,"sender":{"key":"nicolas.s.dev@gmx.fr","avatar":null},"body":"We traditionally allowed a mbox file or a directory name of a maildir to be\ngiven to \"git am\". Even though file in a maildir (or more generally, a piece\nof RFC2822 e-mail) is not a mbox file, it contains enough information to create\na commit out of it, so there is no reason to reject one.\n\nThis builds on top of a5a6755 (git-am foreign patch support: introduce\npatch_format, 2009-05-27) that introduced mailbox format detection. The codepath\nto deal with a mbox requires it to begin with \"From \" line and also allows it to\nbegin with \"From: \", but a random piece of e-mail can and often do begin with\nany valid RFC2822 header lines.\n\nInstead of checking the first line, we extract all the lines up to the\nfirst empty line, and make sure they look like e-mail headers.\n\nSigned-off-by: Nicolas Sebrecht \n---\n Documentation/git-am.txt | 6 +++---\n git-am.sh | 8 ++++++++\n 2 files changed, 11 insertions(+), 3 deletions(-)\n\ndiff --git a/Documentation/git-am.txt b/Documentation/git-am.txt\nindex 32e689b..2a930a7 100644\n--- a/Documentation/git-am.txt\n+++ b/Documentation/git-am.txt\n@@ -14,7 +14,7 @@ SYNOPSIS\n \t [--ignore-date]\n \t [--whitespace=