{"thread":{"id":"21015","subject":"[PATCH] pre-commit.sample: don't use [...] around a tr range","startedAt":"2009-09-21T09:09:49Z","lastAt":"2009-09-24T05:55:18Z","messageCount":9,"participants":["Jim Meyering","Alex Riesen","Jeff King","Brandon Casey"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"123565","messageId":"871vm0k6jm.fsf@meyering.net","threadId":"21015","inReplyTo":null,"subject":"[PATCH] pre-commit.sample: don't use [...] around a tr range","fromName":"Jim Meyering","fromEmail":"jim@meyering.net","sentAt":"2009-09-21T09:09:49Z","receivedAt":"2009-09-21T09:09:49Z","isPatch":true,"sender":{"key":"jim@meyering.net","avatar":"https://avatars.githubusercontent.com/u/710630?v=4"},"body":"Using square brackets in tr ranges is risky.\nFrom the documentation of GNU tr:\n\n Ranges\n The notation `M-N' expands to all of the characters from M through\n N, in ascending order. M should collate before N; if it doesn't,\n an error results. As an example, `0-9' is the same as\n `0123456789'.\n\n GNU `tr' does not support the System V syntax that uses square\n brackets to enclose ranges. Translations specified in that format\n sometimes work as expected, since the brackets are often\n transliterated to themselves. However, they should be avoided\n because they sometimes behave unexpectedly. For example, `tr -d\n '[0-9]'' deletes brackets as well as digits.\n\nHowever, if the use of [] is deliberate, because git still\ncares about portability to ancient SYSV versions of tr that\nrequire that notation, then let me know and I'll undo that part\nof the change and add a comment to that effect.\n\n>From cd3c17c6a48c67d2a598001272f3283714b1df19 Mon Sep 17 00:00:00 2001\nFrom: Jim Meyering \nDate: Mon, 21 Sep 2009 10:58:02 +0200\nSubject: [PATCH] pre-commit.sample: don't use [...] around a tr range\n\nUsing square brackets around a tr range is especially risky when using\ntr's -d (delete) option. In this case, the brackets happen to be harmless,\nsince the two bracket bytes happen to be included in the desired range.\nCorrect spelling and grammar.\n\nSigned-off-by: Jim Meyering \n---\n templates/hooks--pre-commit.sample | 10 +++++-----\n 1 files changed, 5 insertions(+), 5 deletions(-)\n\ndiff --git a/templates/hooks--pre-commit.sample b/templates/hooks--pre-commit.sample\nindex b11ad6a..06a5afa 100755\n--- a/templates/hooks--pre-commit.sample\n+++ b/templates/hooks--pre-commit.sample\n@@ -15,14 +15,14 @@ allownonascii=$(git config hooks.allownonascii)\n # printable range starts at the space character and ends with tilde.\n if [ \"$allownonascii\" != \"true\" ] &&\n \ttest \"$(git diff --cached --name-only --diff-filter=A -z |\n-\t LC_ALL=C tr -d '[ -~]\\0')\"\n+\t LC_ALL=C tr -d ' -~\\0')\"\n then\n-\techo \"Error: Attempt to add a non-ascii filename.\"\n+\techo \"Error: Attempt to add a non-ascii file name.\"\n \techo\n-\techo \"This can cause problems if you want to work together\"\n-\techo \"with people on other platforms than you.\"\n+\techo \"This can cause problems if you want to work\"\n+\techo \"with people on other platforms.\"\n \techo\n-\techo \"To be portable it is adviseable to rename the file ...\"\n+\techo \"To be portable it is advisable to rename the file ...\"\n \techo\n \techo \"If you know what you are doing you can disable this\"\n \techo \"check using:\"\n--\n1.6.5.rc1.214.g13c5a\n"},{"id":"123566","messageId":"81b0412b0909210243h58633509r7b427c69f62928fe@mail.gmail.com","threadId":"21015","inReplyTo":"871vm0k6jm.fsf@meyering.net","subject":"Re: [PATCH] pre-commit.sample: don't use [...] around a tr range","fromName":"Alex Riesen","fromEmail":"raa.lkml@gmail.com","sentAt":"2009-09-21T09:43:44Z","receivedAt":"2009-09-21T09:43:44Z","isPatch":true,"sender":{"key":"raa.lkml@gmail.com","avatar":"https://avatars.githubusercontent.com/u/324101?v=4"},"body":"On Mon, Sep 21, 2009 at 11:09, Jim Meyering wrote:\n> However, if the use of [] is deliberate, because git still\n> cares about portability to ancient SYSV versions of tr that\n> require that notation, then let me know and I'll undo that part\n> of the change and add a comment to that effect.\n\nWe have (had?) people trying to support Git on HP-UX and SunOS.\nDo these count?\n"},{"id":"123567","messageId":"87ocp4imul.fsf@meyering.net","threadId":"21015","inReplyTo":"81b0412b0909210243h58633509r7b427c69f62928fe@mail.gmail.com","subject":"Re: [PATCH] pre-commit.sample: don't use [...] around a tr range","fromName":"Jim Meyering","fromEmail":"jim@meyering.net","sentAt":"2009-09-21T11:00:34Z","receivedAt":"2009-09-21T11:00:34Z","isPatch":true,"sender":{"key":"jim@meyering.net","avatar":"https://avatars.githubusercontent.com/u/710630?v=4"},"body":"Alex Riesen wrote:\n> On Mon, Sep 21, 2009 at 11:09, Jim Meyering wrote:\n>> However, if the use of [] is deliberate, because git still\n>> cares about portability to ancient SYSV versions of tr that\n>> require that notation, then let me know and I'll undo that part\n>> of the change and add a comment to that effect.\n>\n> We have (had?) people trying to support Git on HP-UX and SunOS.\n> Do these count?\n\nI had my doubts, but have just confirmed that Solaris 10's\n/usr/bin/tr is still doing it the SYSV way:\n\n $ echo foo | LC_ALL=C /usr/bin/tr a-z A-Z\n foo\n\nThere, you have to use /usr/xpg4/bin/tr to get the expected behavior:\n\n $ echo foo | LC_ALL=C /usr/xpg4/bin/tr a-z A-Z\n FOO\n\nSo you're right. Thanks!\nHere's an updated patch:\n\n>From 4048a5b393fa5f35dfe8157e98a6ea475b0efb2d Mon Sep 17 00:00:00 2001\nFrom: Jim Meyering \nDate: Mon, 21 Sep 2009 10:58:02 +0200\nSubject: [PATCH] pre-commit.sample: add comment re tr portability; fix grammar\n\nAdd a comment explaining why square brackets around a tr range\nare not only ok, but actually required in this case.\nCorrect spelling and grammar.\n\nSigned-off-by: Jim Meyering \n---\n templates/hooks--pre-commit.sample | 11 +++++++----\n 1 files changed, 7 insertions(+), 4 deletions(-)\n\ndiff --git a/templates/hooks--pre-commit.sample b/templates/hooks--pre-commit.sample\nindex b11ad6a..128c8cc 100755\n--- a/templates/hooks--pre-commit.sample\n+++ b/templates/hooks--pre-commit.sample\n@@ -14,15 +14,18 @@ allownonascii=$(git config hooks.allownonascii)\n # them from being added to the repository. We exploit the fact that the\n # printable range starts at the space character and ends with tilde.\n if [ \"$allownonascii\" != \"true\" ] &&\n+\t# Note that the use of brackets around a tr range is ok here, (it's\n+\t# even required, for portability to Solaris 10's /usr/bin/tr), since\n+\t# the square bracket bytes happen to fall in the designated range.\n \ttest \"$(git diff --cached --name-only --diff-filter=A -z |\n \t LC_ALL=C tr -d '[ -~]\\0')\"\n then\n-\techo \"Error: Attempt to add a non-ascii filename.\"\n+\techo \"Error: Attempt to add a non-ascii file name.\"\n \techo\n-\techo \"This can cause problems if you want to work together\"\n-\techo \"with people on other platforms than you.\"\n+\techo \"This can cause problems if you want to work\"\n+\techo \"with people on other platforms.\"\n \techo\n-\techo \"To be portable it is adviseable to rename the file ...\"\n+\techo \"To be portable it is advisable to rename the file ...\"\n \techo\n \techo \"If you know what you are doing you can disable this\"\n \techo \"check using:\"\n--\n1.6.5.rc1.214.g13c5a\n"},{"id":"123568","messageId":"81b0412b0909210545n3da664c4r4e9250fa2eb241c4@mail.gmail.com","threadId":"21015","inReplyTo":"87ocp4imul.fsf@meyering.net","subject":"Re: [PATCH] pre-commit.sample: don't use [...] around a tr range","fromName":"Alex Riesen","fromEmail":"raa.lkml@gmail.com","sentAt":"2009-09-21T12:45:33Z","receivedAt":"2009-09-21T12:45:33Z","isPatch":true,"sender":{"key":"raa.lkml@gmail.com","avatar":"https://avatars.githubusercontent.com/u/324101?v=4"},"body":"On Mon, Sep 21, 2009 at 13:00, Jim Meyering wrote:\n> Alex Riesen wrote:\n>> On Mon, Sep 21, 2009 at 11:09, Jim Meyering wrote:\n>>> However, if the use of [] is deliberate, because git still\n>>> cares about portability to ancient SYSV versions of tr that\n>>> require that notation, then let me know and I'll undo that part\n>>> of the change and add a comment to that effect.\n>>\n>> We have (had?) people trying to support Git on HP-UX and SunOS.\n>> Do these count?\n>\n> I had my doubts, but have just confirmed that Solaris 10's\n> /usr/bin/tr is still doing it the SYSV way:\n>\n>    $ echo foo | LC_ALL=C /usr/bin/tr a-z A-Z\n>    foo\n>\n> There, you have to use /usr/xpg4/bin/tr to get the expected behavior:\n>\n>    $ echo foo | LC_ALL=C /usr/xpg4/bin/tr a-z A-Z\n>    FOO\n>\n> So you're right.  ...\n\nSorry about that. I even made a simple tr once for Git tests\nexactly because of compatibility problems (look for \"poor man tr\"\nin the list archives, if needed).\n"},{"id":"123578","messageId":"20090921134427.GA20567@sigio.peff.net","threadId":"21015","inReplyTo":"87ocp4imul.fsf@meyering.net","subject":"Re: [PATCH] pre-commit.sample: don't use [...] around a tr range","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2009-09-21T13:44:27Z","receivedAt":"2009-09-21T13:44:27Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Sep 21, 2009 at 01:00:34PM +0200, Jim Meyering wrote:\n\n> > We have (had?) people trying to support Git on HP-UX and SunOS.\n> > Do these count?\n> \n> I had my doubts, but have just confirmed that Solaris 10's\n> /usr/bin/tr is still doing it the SYSV way:\n> \n> $ echo foo | LC_ALL=C /usr/bin/tr a-z A-Z\n> foo\n> \n> There, you have to use /usr/xpg4/bin/tr to get the expected behavior:\n> \n> $ echo foo | LC_ALL=C /usr/xpg4/bin/tr a-z A-Z\n> FOO\n> \n> So you're right. Thanks!\n\nSee:\n\n http://article.gmane.org/gmane.comp.version-control.git/76991\n\n> +\t# Note that the use of brackets around a tr range is ok here, (it's\n> +\t# even required, for portability to Solaris 10's /usr/bin/tr), since\n> +\t# the square bracket bytes happen to fall in the designated range.\n> \ttest \"$(git diff --cached --name-only --diff-filter=A -z |\n> \t LC_ALL=C tr -d '[ -~]\\0')\"\n\nDoes this work on non-bracket systems? I would think that enumerating\nthe sequence would be the most portable thing.\n\n-Peff\n"},{"id":"123581","messageId":"87tyywgzhu.fsf@meyering.net","threadId":"21015","inReplyTo":"20090921134427.GA20567@sigio.peff.net","subject":"Re: [PATCH] pre-commit.sample: don't use [...] around a tr range","fromName":"Jim Meyering","fromEmail":"jim@meyering.net","sentAt":"2009-09-21T14:10:21Z","receivedAt":"2009-09-21T14:10:21Z","isPatch":true,"sender":{"key":"jim@meyering.net","avatar":"https://avatars.githubusercontent.com/u/710630?v=4"},"body":"Jeff King wrote:\n\n> On Mon, Sep 21, 2009 at 01:00:34PM +0200, Jim Meyering wrote:\n>\n>> > We have (had?) people trying to support Git on HP-UX and SunOS.\n>> > Do these count?\n>>\n>> I had my doubts, but have just confirmed that Solaris 10's\n>> /usr/bin/tr is still doing it the SYSV way:\n>>\n>> $ echo foo | LC_ALL=C /usr/bin/tr a-z A-Z\n>> foo\n>>\n>> There, you have to use /usr/xpg4/bin/tr to get the expected behavior:\n>>\n>> $ echo foo | LC_ALL=C /usr/xpg4/bin/tr a-z A-Z\n>> FOO\n>>\n>> So you're right. Thanks!\n>\n> See:\n>\n> http://article.gmane.org/gmane.comp.version-control.git/76991\n>\n>> +\t# Note that the use of brackets around a tr range is ok here, (it's\n>> +\t# even required, for portability to Solaris 10's /usr/bin/tr), since\n>> +\t# the square bracket bytes happen to fall in the designated range.\n>> \ttest \"$(git diff --cached --name-only --diff-filter=A -z |\n>> \t LC_ALL=C tr -d '[ -~]\\0')\"\n>\n> Does this work on non-bracket systems?\n\nYes, since [] happen to fall in the range.\n\n> I would think that enumerating\n> the sequence would be the most portable thing.\n\nEnumerating is more portable, at the expense of\nreadability and maintainability.\nIn case you want to go that route, here's one more:\n\n(note that this, like the original range, treats TAB as nonportable)\n\n>From 40a368a7bcf0ac6524bbe36ba3bfdaa0711897b8 Mon Sep 17 00:00:00 2001\nFrom: Jim Meyering \nDate: Mon, 21 Sep 2009 10:58:02 +0200\nSubject: [PATCH] pre-commit.sample: use tr more portability; fix grammar\n\nAvoid tr's M-N range notation altogether.\nInstead, enumerate ascii bytes 32..126.\nCorrect spelling and grammar.\n\nSigned-off-by: Jim Meyering \n---\n templates/hooks--pre-commit.sample | 19 +++++++++++++------\n 1 files changed, 13 insertions(+), 6 deletions(-)\n\ndiff --git a/templates/hooks--pre-commit.sample b/templates/hooks--pre-commit.sample\nindex b11ad6a..896eb9e 100755\n--- a/templates/hooks--pre-commit.sample\n+++ b/templates/hooks--pre-commit.sample\n@@ -10,19 +10,26 @@\n # If you want to allow non-ascii filenames set this variable to true.\n allownonascii=$(git config hooks.allownonascii)\n\n+printables=' !\"#$%&'\\''()*+,-./0123456789:;<=>?@'\n+printables=\"$printables\"'ABCDEFGHIJKLMNOPQRSTUVWXYZ[\\]^_`'\n+printables=\"$printables\"'abcdefghijklmnopqrstuvwxyz{|}~'\n+\n # Cross platform projects tend to avoid non-ascii filenames; prevent\n # them from being added to the repository. We exploit the fact that the\n # printable range starts at the space character and ends with tilde.\n if [ \"$allownonascii\" != \"true\" ] &&\n-\ttest \"$(git diff --cached --name-only --diff-filter=A -z |\n-\t LC_ALL=C tr -d '[ -~]\\0')\"\n+\t# Filter out printable ascii bytes, and NUL.\n+\t# If anything remains, you lose.\n+ rem=$(git diff --cached --name-only --diff-filter=A -z |\n+\t LC_ALL=C tr -d \"$printables\"'\\0')\n+\ttest -n \"$rem\"\n then\n-\techo \"Error: Attempt to add a non-ascii filename.\"\n+\techo \"Error: Attempt to add a non-ascii file name.\"\n \techo\n-\techo \"This can cause problems if you want to work together\"\n-\techo \"with people on other platforms than you.\"\n+\techo \"This can cause problems if you want to work\"\n+\techo \"with people on other platforms.\"\n \techo\n-\techo \"To be portable it is adviseable to rename the file ...\"\n+\techo \"To be portable it is advisable to rename the file ...\"\n \techo\n \techo \"If you know what you are doing you can disable this\"\n \techo \"check using:\"\n--\n1.6.5.rc1.214.g13c5a\n"},{"id":"123583","messageId":"20090921142157.GA20860@sigio.peff.net","threadId":"21015","inReplyTo":"87tyywgzhu.fsf@meyering.net","subject":"Re: [PATCH] pre-commit.sample: don't use [...] around a tr range","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2009-09-21T14:21:57Z","receivedAt":"2009-09-21T14:21:57Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Sep 21, 2009 at 04:10:21PM +0200, Jim Meyering wrote:\n\n> >> \ttest \"$(git diff --cached --name-only --diff-filter=A -z |\n> >> \t LC_ALL=C tr -d '[ -~]\\0')\"\n> >\n> > Does this work on non-bracket systems?\n> \n> Yes, since [] happen to fall in the range.\n> \n> > I would think that enumerating\n> > the sequence would be the most portable thing.\n> \n> Enumerating is more portable, at the expense of\n> readability and maintainability.\n> In case you want to go that route, here's one more:\n\nAgreed. If we can avoid enumeration, we should. If your original is\nportable, then I think it is preferable. Thanks for looking into it.\n\n-Peff\n"},{"id":"123585","messageId":"n2z47RdNjxVbPeRfgJuY54nJrfzDfn43bzv8HoH6cEuAzQ6fB1MaBg@cipher.nrlssc.navy.mil","threadId":"21015","inReplyTo":"20090921134427.GA20567@sigio.peff.net","subject":"Re: [PATCH] pre-commit.sample: don't use [...] around a tr range","fromName":"Brandon Casey","fromEmail":"brandon.casey.ctr@nrlssc.navy.mil","sentAt":"2009-09-21T15:58:51Z","receivedAt":"2009-09-21T15:58:51Z","isPatch":true,"sender":{"key":"brandon.casey.ctr@nrlssc.navy.mil","avatar":null},"body":"Jeff King wrote:\n> On Mon, Sep 21, 2009 at 01:00:34PM +0200, Jim Meyering wrote:\n> \n>>> We have (had?) people trying to support Git on HP-UX and SunOS.\n>>> Do these count?\n>> I had my doubts, but have just confirmed that Solaris 10's\n>> /usr/bin/tr is still doing it the SYSV way:\n>>\n>> $ echo foo | LC_ALL=C /usr/bin/tr a-z A-Z\n>> foo\n>>\n>> There, you have to use /usr/xpg4/bin/tr to get the expected behavior:\n>>\n>> $ echo foo | LC_ALL=C /usr/xpg4/bin/tr a-z A-Z\n>> FOO\n>>\n>> So you're right. Thanks!\n\nBy the way, modern git inserts /usr/xpg4/bin into PATH before /usr/bin on\nSolaris. So /usr/xpg4/bin/tr should always be used on that platform.\n\n-brandon\n"},{"id":"123710","messageId":"20090924055518.GB24486@coredump.intra.peff.net","threadId":"21015","inReplyTo":"n2z47RdNjxVbPeRfgJuY54nJrfzDfn43bzv8HoH6cEuAzQ6fB1MaBg@cipher.nrlssc.navy.mil","subject":"Re: [PATCH] pre-commit.sample: don't use [...] around a tr range","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2009-09-24T05:55:18Z","receivedAt":"2009-09-24T05:55:18Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Sep 21, 2009 at 10:58:51AM -0500, Brandon Casey wrote:\n\n> >> I had my doubts, but have just confirmed that Solaris 10's\n> >> /usr/bin/tr is still doing it the SYSV way:\n> >>\n> >> $ echo foo | LC_ALL=C /usr/bin/tr a-z A-Z\n> >> foo\n> >>\n> >> There, you have to use /usr/xpg4/bin/tr to get the expected behavior:\n> >>\n> >> $ echo foo | LC_ALL=C /usr/xpg4/bin/tr a-z A-Z\n> >> FOO\n> >>\n> >> So you're right. Thanks!\n> \n> By the way, modern git inserts /usr/xpg4/bin into PATH before /usr/bin on\n> Solaris. So /usr/xpg4/bin/tr should always be used on that platform.\n\nAh, I forgot about that patch. So it may not matter on Solaris. Are\nthere other platforms which want the brackets? Maybe HP-UX, but I no\nlonger have access to a box and I've used deep hypnosis to suppress any\nmemories of ever having used it.\n\nAnyway, I think Jim's patch is the right fix if there are still\nplatforms that want the brackets.\n\n-Peff\n"}]}