{"thread":{"id":"21053","subject":"[PATCH 1/2] git-am: fixed patch_format detection according to RFC2822","startedAt":"2009-09-25T15:14:31Z","lastAt":"2009-09-28T12:09:49Z","messageCount":11,"participants":["Christian Himpel","Jeff King","Johannes Sixt"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"123781","messageId":"d7d039c304614d8d39fd2e7ad8d036a77121cc4f.1253891124.git.chressie@gmail.com","threadId":"21053","inReplyTo":null,"subject":"[PATCH 1/2] git-am: fixed patch_format detection according to RFC2822","fromName":"Christian Himpel","fromEmail":"chressie@googlemail.com","sentAt":"2009-09-25T15:14:31Z","receivedAt":"2009-09-25T15:14:31Z","isPatch":true,"sender":{"key":"chressie@googlemail.com","avatar":"https://avatars.githubusercontent.com/u/227883?v=4"},"body":"RFC2822 specifies in paragraph 3.6.8, that optional header fields are\nmade up of any printable US-ASCII character except ' ' (space) and ':'\n(colon).\n\nThe pattern for the egrep command is changed to match all of these\ncharacters.\n\nSigned-off-by: Christian Himpel \n---\n\nHi,\n\nI had a problem with applying a patch with 'git am', because the one\nheader fields in the patch-e-mail contained numbers. So I read the\nRFC2822 what they say about header fields. Unbelievable but any\nprintable character except space and colon are allowed in header fields.\nSo I changed the egrep expression according to this rule.\n\nRegards,\nchressie\n\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 26ffe70..0ddd80f 100755\n--- a/git-am.sh\n+++ b/git-am.sh\n@@ -205,7 +205,7 @@ check_patch_format () {\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\tegrep -v '^[A-Za-z]+(-[A-Za-z]+)*:' >/dev/null ||\n+\t\t\tegrep -v '^[!-9;-~]+:' >/dev/null ||\n \t\t\tpatch_format=mbox\n \t\tfi\n \t} < \"$1\" || clean_abort\n-- \n1.6.4.4\n"},{"id":"123782","messageId":"dc8a4205128d65e69587292a42c714792315a9df.1253891124.git.chressie@gmail.com","threadId":"21053","inReplyTo":"d7d039c304614d8d39fd2e7ad8d036a77121cc4f.1253891124.git.chressie@gmail.com","subject":"[PATCH 2/2] git-am: force egrep to use correct characters set","fromName":"Christian Himpel","fromEmail":"chressie@googlemail.com","sentAt":"2009-09-25T15:17:31Z","receivedAt":"2009-09-25T15:17:31Z","isPatch":true,"sender":{"key":"chressie@googlemail.com","avatar":"https://avatars.githubusercontent.com/u/227883?v=4"},"body":"According to egrep(1) the US-ASCII table is used when LC_ALL=C is set.\nWe do not rely here on the LC_ALL value we get from the environment.\n\nSigned-off-by: Christian Himpel \n---\n\nI don't know if this kind of patch is desired, but according to egrep(1)\nit's not reliable to use the range expression with different character\nsets than US-ASCII.\n\nSo this patch forces the usage of US-ASCII.\n\nRegards,\nchressie\n\n\n git-am.sh | 4 ++++\n 1 files changed, 4 insertions(+), 0 deletions(-)\n\ndiff --git a/git-am.sh b/git-am.sh\nindex 0ddd80f..e4dd49a 100755\n--- a/git-am.sh\n+++ b/git-am.sh\n@@ -204,9 +204,13 @@ check_patch_format () {\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\t_tmp_locale=$LC_ALL\n+\t\t\texport LC_ALL=C\n \t\t\tsed -n -e '/^$/q' -e '/^[ \t]/d' -e p \"$1\" |\n \t\t\tegrep -v '^[!-9;-~]+:' >/dev/null ||\n \t\t\tpatch_format=mbox\n+\t\t\texport LC_ALL=$_tmp_locale\n+\t\t\tunset $_tmp_locale\n \t\tfi\n \t} < \"$1\" || clean_abort\n }\n-- \n1.6.4.4\n"},{"id":"123784","messageId":"215cc4f241162377b9249c2b3d74050cc77bac16.1253893253.git.chressie@gmail.com","threadId":"21053","inReplyTo":"dc8a4205128d65e69587292a42c714792315a9df.1253891124.git.chressie@gmail.com","subject":"[PATCH 2/2 v2] git-am: force egrep to use correct characters set","fromName":"Christian Himpel","fromEmail":"chressie@googlemail.com","sentAt":"2009-09-25T15:45:39Z","receivedAt":"2009-09-25T15:45:39Z","isPatch":true,"sender":{"key":"chressie@googlemail.com","avatar":"https://avatars.githubusercontent.com/u/227883?v=4"},"body":"According to egrep(1) the US-ASCII table is used when LC_ALL=C is set.\nWe do not rely here on the LC_ALL value we get from the environment.\n---\n\nThis is probably a saner approach :)\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 0ddd80f..c132f50 100755\n--- a/git-am.sh\n+++ b/git-am.sh\n@@ -205,7 +205,7 @@ check_patch_format () {\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\tegrep -v '^[!-9;-~]+:' >/dev/null ||\n+\t\t\tLC_ALL=C egrep -v '^[!-9;-~]+:' >/dev/null ||\n \t\t\tpatch_format=mbox\n \t\tfi\n \t} < \"$1\" || clean_abort\n-- \n1.6.4.4\n"},{"id":"123788","messageId":"f0bd48168975c3b2328cf26f9a37a0f54b898473.1253896646.git.chressie@gmail.com","threadId":"21053","inReplyTo":"215cc4f241162377b9249c2b3d74050cc77bac16.1253893253.git.chressie@gmail.com","subject":"[PATCH] git-am: force egrep to use correct characters set","fromName":"Christian Himpel","fromEmail":"chressie@googlemail.com","sentAt":"2009-09-25T16:43:20Z","receivedAt":"2009-09-25T16:43:20Z","isPatch":true,"sender":{"key":"chressie@googlemail.com","avatar":"https://avatars.githubusercontent.com/u/227883?v=4"},"body":"According to egrep(1) the US-ASCII table is used when LC_ALL=C is set.\nWe do not rely here on the LC_ALL value we get from the environment.\n\nSigned-off-by: Christian Himpel \n---\n\nsorry for being so noisy, but forgot to signoff last time\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 0ddd80f..c132f50 100755\n--- a/git-am.sh\n+++ b/git-am.sh\n@@ -205,7 +205,7 @@ check_patch_format () {\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\tegrep -v '^[!-9;-~]+:' >/dev/null ||\n+\t\t\tLC_ALL=C egrep -v '^[!-9;-~]+:' >/dev/null ||\n \t\t\tpatch_format=mbox\n \t\tfi\n \t} < \"$1\" || clean_abort\n-- \n1.6.4.4\n"},{"id":"123885","messageId":"20090927074015.GB15393@coredump.intra.peff.net","threadId":"21053","inReplyTo":"f0bd48168975c3b2328cf26f9a37a0f54b898473.1253896646.git.chressie@gmail.com","subject":"Re: [PATCH] git-am: force egrep to use correct characters set","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2009-09-27T07:40:15Z","receivedAt":"2009-09-27T07:40:15Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Sep 25, 2009 at 06:43:20PM +0200, Christian Himpel wrote:\n\n> According to egrep(1) the US-ASCII table is used when LC_ALL=C is set.\n> We do not rely here on the LC_ALL value we get from the environment.\n\nHmm. Probably makes sense here, as it is a wide enough range that it may\npick up other stray non-ascii characters in other charsets (though as\nthe manpage notes, the likely thing is to pick up A-Z along with a-z,\nwhich is OK here as we encompass both in our range).\n\nThere are two other calls to egrep with brackets (both in\ngit-submodule.sh), but they are just [0-7], which is presumably OK in\njust about any charset.\n\nDo you happen to know a charset in which this is a problem, just for\nreference?\n\n-Peff\n"},{"id":"123919","messageId":"20090928065519.GA24773@lamagra.informatik.uni-ulm.de","threadId":"21053","inReplyTo":"20090927074015.GB15393@coredump.intra.peff.net","subject":"Re: [PATCH] git-am: force egrep to use correct characters set","fromName":"Christian Himpel","fromEmail":"chressie@googlemail.com","sentAt":"2009-09-28T06:55:19Z","receivedAt":"2009-09-28T06:55:19Z","isPatch":true,"sender":{"key":"chressie@googlemail.com","avatar":"https://avatars.githubusercontent.com/u/227883?v=4"},"body":"On Sun, Sep 27, 2009 at 03:40:15AM -0400, Jeff King wrote:\n> On Fri, Sep 25, 2009 at 06:43:20PM +0200, Christian Himpel wrote:\n> \n> > According to egrep(1) the US-ASCII table is used when LC_ALL=C is set.\n> > We do not rely here on the LC_ALL value we get from the environment.\n> \n> Hmm. Probably makes sense here, as it is a wide enough range that it may\n> pick up other stray non-ascii characters in other charsets (though as\n> the manpage notes, the likely thing is to pick up A-Z along with a-z,\n> which is OK here as we encompass both in our range).\n> \n> There are two other calls to egrep with brackets (both in\n> git-submodule.sh), but they are just [0-7], which is presumably OK in\n> just about any charset.\n> \n> Do you happen to know a charset in which this is a problem, just for\n> reference?\n\nNo, I don't know any charset with stray ascii-chars. I just listened\nattentively, when I read the part about the mixed alphabet characters in\nthe grep(1) manpage.\n\nI did some quick checks just now. It seems the characters (' ' to '~')\nare in any locale, offered by glibc, at the same place.\n\nMaybe, we can just leave the charset as it is and ignore this patch,\nuntil someone complains.\n\n\nRegards,\nchressie\n\n> \n> -Peff\n"},{"id":"123920","messageId":"20090928071609.GB20483@coredump.intra.peff.net","threadId":"21053","inReplyTo":"20090928065519.GA24773@lamagra.informatik.uni-ulm.de","subject":"Re: [PATCH] git-am: force egrep to use correct characters set","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2009-09-28T07:16:09Z","receivedAt":"2009-09-28T07:16:09Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Sep 28, 2009 at 08:55:19AM +0200, Christian Himpel wrote:\n\n> > Do you happen to know a charset in which this is a problem, just for\n> > reference?\n> \n> No, I don't know any charset with stray ascii-chars. I just listened\n> attentively, when I read the part about the mixed alphabet characters in\n> the grep(1) manpage.\n> \n> I did some quick checks just now. It seems the characters (' ' to '~')\n> are in any locale, offered by glibc, at the same place.\n> \n> Maybe, we can just leave the charset as it is and ignore this patch,\n> until someone complains.\n\nThanks for looking into it. My question was more of a \"how bad is this,\nand should we be fixing these other callsites, too\". But I think 0-7 is\nprobably a pretty safe range in any charset.\n\nUsually with portability issues, I am inclined to say \"wait for a\nproblem\", as we try to code to match actual reality instead of\ndocumentation or standards. But in this case, we are unlikely to test\nwith strange charsets (or even know which strange charsets exist;\nchecking what's in glibc is reasonable, but I have no idea what else is\nout there in other countries), and that the resulting bug would be\nsubtle and hard to find, it probably makes sense to be a little\ndefensive.\n\n-Peff\n"},{"id":"123924","messageId":"4AC06FFF.20008@viscovery.net","threadId":"21053","inReplyTo":"20090927074015.GB15393@coredump.intra.peff.net","subject":"Re: [PATCH] git-am: force egrep to use correct characters set","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2009-09-28T08:12:47Z","receivedAt":"2009-09-28T08:12:47Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Jeff King schrieb:\n> On Fri, Sep 25, 2009 at 06:43:20PM +0200, Christian Himpel wrote:\n> \n>> According to egrep(1) the US-ASCII table is used when LC_ALL=C is set.\n>> We do not rely here on the LC_ALL value we get from the environment.\n> \n> Hmm. Probably makes sense here, as it is a wide enough range that it may\n> pick up other stray non-ascii characters in other charsets (though as\n> the manpage notes, the likely thing is to pick up A-Z along with a-z,\n> which is OK here as we encompass both in our range).\n> \n> There are two other calls to egrep with brackets (both in\n> git-submodule.sh), but they are just [0-7], which is presumably OK in\n> just about any charset.\n> \n> Do you happen to know a charset in which this is a problem, just for\n> reference?\n\nIt's not so much about charsets than about languages:\n\n Within a bracket expression, a range expression consists\n of two characters separated by a hyphen. It matches any\n single character that sorts between the two characters,\n inclusive, using the locale's collating sequence and\n character set. For example, in the default C locale,\n [a-d] is equivalent to [abcd]. Many locales sort char-\n acters in dictionary order, and in these locales [a-d]\n is typically not equivalent to [abcd]; it might be\n equivalent to [aBbCcDd], for example. To obtain the\n traditional interpretation of bracket expressions, you\n can use the C locale by setting the LC_ALL environment\n variable to the value C.\n\nFor example, in locale de_DE.UTF-8, GNU grep '[a-z]' matches lowercase\nletters, uppercase letters (!), and umlauts (!!) because in dictionary\norder, 'A' and 'a' are equivalent and 'Ä' sorts after 'A'. (The input must\nbe UTF-8, of course.)\n\nGiven that this applies not only to egrep, but to grep in general (and\nperhaps even to other tools that support ranges, like sed), it may be\nnecessary to audit all range expressions.\n\nThe case identified by Christian is certainly important because it is\napplied to a file whose contents can be anything, and the purpose of the\ncheck is to identify the text as an mbox file, whose header section can be\nonly US-ASCII by definition. So, I think it has merit to apply the patch.\n\n-- Hannes\n"},{"id":"123928","messageId":"20090928093216.GA31459@lamagra.informatik.uni-ulm.de","threadId":"21053","inReplyTo":"4AC06FFF.20008@viscovery.net","subject":"Re: [PATCH] git-am: force egrep to use correct characters set","fromName":"Christian Himpel","fromEmail":"chressie@googlemail.com","sentAt":"2009-09-28T09:32:16Z","receivedAt":"2009-09-28T09:32:16Z","isPatch":true,"sender":{"key":"chressie@googlemail.com","avatar":"https://avatars.githubusercontent.com/u/227883?v=4"},"body":"On Mon, Sep 28, 2009 at 10:12:47AM +0200, Johannes Sixt wrote:\n> Jeff King schrieb:\n> > On Fri, Sep 25, 2009 at 06:43:20PM +0200, Christian Himpel wrote:\n> > \n> >> According to egrep(1) the US-ASCII table is used when LC_ALL=C is set.\n> >> We do not rely here on the LC_ALL value we get from the environment.\n> > \n> > Hmm. Probably makes sense here, as it is a wide enough range that it may\n> > pick up other stray non-ascii characters in other charsets (though as\n> > the manpage notes, the likely thing is to pick up A-Z along with a-z,\n> > which is OK here as we encompass both in our range).\n> > \n> > There are two other calls to egrep with brackets (both in\n> > git-submodule.sh), but they are just [0-7], which is presumably OK in\n> > just about any charset.\n> > \n> > Do you happen to know a charset in which this is a problem, just for\n> > reference?\n> \n> It's not so much about charsets than about languages:\n> \n> Within a bracket expression, a range expression consists\n> of two characters separated by a hyphen. It matches any\n> single character that sorts between the two characters,\n> inclusive, using the locale's collating sequence and\n> character set. For example, in the default C locale,\n> [a-d] is equivalent to [abcd]. Many locales sort char-\n> acters in dictionary order, and in these locales [a-d]\n> is typically not equivalent to [abcd]; it might be\n> equivalent to [aBbCcDd], for example. To obtain the\n> traditional interpretation of bracket expressions, you\n> can use the C locale by setting the LC_ALL environment\n> variable to the value C.\n> \n> For example, in locale de_DE.UTF-8, GNU grep '[a-z]' matches lowercase\n> letters, uppercase letters (!), and umlauts (!!) because in dictionary\n> order, 'A' and 'a' are equivalent and 'Ä' sorts after 'A'. (The input must\n> be UTF-8, of course.)\n\nThanks for pointing this out. You are right. I must have read the\n\"dictonary order\" part over.\n\n> Given that this applies not only to egrep, but to grep in general (and\n> perhaps even to other tools that support ranges, like sed), it may be\n> necessary to audit all range expressions.\n\nAfter doing a quick:\n\nLC_ALL=C find . -name '*.sh' -exec \\\n egrep -Hne '(grep|awk|sed).*\\[.*-.*\\]' {} \\;\n\nAs far as I can see, range expressions are used:\n\n1. to replace or grep hexadecimal numbers (SHA1 sums). This shouldn't\nbe a problem, if we can assume that these numbers are never malformed.\n\n2. to replace or grep numbers (with digits). This shouldn't be a\nproblem, since digits should be in dictionary order in every language\n(?!).\n\n3. in git-rebase--interactive.sh:742 to grep for a previously generated\nstring. So it should be safe here.\n\n> The case identified by Christian is certainly important because it is\n> applied to a file whose contents can be anything, and the purpose of the\n> check is to identify the text as an mbox file, whose header section can be\n> only US-ASCII by definition. So, I think it has merit to apply the patch.\n\nYes. It seems that this is the only place where it is important to match\njust the ASCII printable characters.\n\n\nRegards,\nchressie\n"},{"id":"123930","messageId":"4AC0877E.7020204@viscovery.net","threadId":"21053","inReplyTo":"20090928093216.GA31459@lamagra.informatik.uni-ulm.de","subject":"Re: [PATCH] git-am: force egrep to use correct characters set","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2009-09-28T09:53:02Z","receivedAt":"2009-09-28T09:53:02Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Christian Himpel schrieb:\n> After doing a quick:\n> \n> LC_ALL=C find . -name '*.sh' -exec \\\n> egrep -Hne '(grep|awk|sed).*\\[.*-.*\\]' {} \\;\n> \n> As far as I can see, range expressions are used:\n\n0. The test suite scripts are not critical because test-lib.sh sets LANG=C\nLC_ALL=C\n\n> 1. to replace or grep hexadecimal numbers (SHA1 sums). This shouldn't\n> be a problem, if we can assume that these numbers are never malformed.\n\nThe assumption is valid if the input is piped from a git command, and such\nuses aren't critical.\n\n> 2. to replace or grep numbers (with digits). This shouldn't be a\n> problem, since digits should be in dictionary order in every language\n> (?!).\n\nI agree.\n\n> 3. in git-rebase--interactive.sh:742 to grep for a previously generated\n> string. So it should be safe here.\n\nI agree.\n\n>> The case identified by Christian is certainly important because it is\n>> applied to a file whose contents can be anything, and the purpose of the\n>> check is to identify the text as an mbox file, whose header section can be\n>> only US-ASCII by definition. So, I think it has merit to apply the patch.\n> \n> Yes. It seems that this is the only place where it is important to match\n> just the ASCII printable characters.\n\nThere is another place in git-am.sh where a sed expression with a range\nlooks at the input file, doesn't it? Isn't it critical, too?\n\n\t\tif test -f \"$dotest/rebasing\" &&\n\t\t\tcommit=$(sed -e 's/^From \\([0-9a-f]*\\) .*/\\1/' \\\n\t\t\t\t-e q \"$dotest/$msgnum\") &&\n\t\t\ttest \"$(git cat-file -t \"$commit\")\" = commit\n\t\tthen ...\n\n-- Hannes\n"},{"id":"123944","messageId":"20090928120949.GA3964@mrslave","threadId":"21053","inReplyTo":"4AC0877E.7020204@viscovery.net","subject":"Re: [PATCH] git-am: force egrep to use correct characters set","fromName":"Christian Himpel","fromEmail":"chressie@googlemail.com","sentAt":"2009-09-28T12:09:49Z","receivedAt":"2009-09-28T12:09:49Z","isPatch":true,"sender":{"key":"chressie@googlemail.com","avatar":"https://avatars.githubusercontent.com/u/227883?v=4"},"body":"On Mon, Sep 28, 2009 at 11:53:02AM +0200, Johannes Sixt wrote:\n> Christian Himpel schrieb:\n> >> The case identified by Christian is certainly important because it is\n> >> applied to a file whose contents can be anything, and the purpose of the\n> >> check is to identify the text as an mbox file, whose header section can be\n> >> only US-ASCII by definition. So, I think it has merit to apply the patch.\n> > \n> > Yes. It seems that this is the only place where it is important to match\n> > just the ASCII printable characters.\n> \n> There is another place in git-am.sh where a sed expression with a range\n> looks at the input file, doesn't it? Isn't it critical, too?\n> \n> \t\tif test -f \"$dotest/rebasing\" &&\n> \t\t\tcommit=$(sed -e 's/^From \\([0-9a-f]*\\) .*/\\1/' \\\n> \t\t\t\t-e q \"$dotest/$msgnum\") &&\n> \t\t\ttest \"$(git cat-file -t \"$commit\")\" = commit\n> \t\tthen ...\n\nIt seems to be the line generated from 'git format-patch' that is tested\nhere. It specifies the SHA1 of the commit that is converted into a mbox\npatch by 'format-patch'. Hence, I don't see it critical here (until\nsomeone edits this line by hand).\n\n\nRegards,\nchressie\n"}]}