# [PATCH] pre-commit.sample: don't use [...] around a tr range 9 messages from 2009-09-21 to 2009-09-24. Participants: Jim Meyering, Alex Riesen, Jeff King, Brandon Casey. Thread: https://gitlist.dev/t/21015 ## Jim Meyering, 2009-09-21 09:09 Subject: [PATCH] pre-commit.sample: don't use [...] around a tr range Message-ID: <871vm0k6jm.fsf@meyering.net> URL: https://gitlist.dev/e/871vm0k6jm.fsf%40meyering.net ``` Using square brackets in tr ranges is risky. From the documentation of GNU tr: Ranges The notation `M-N' expands to all of the characters from M through N, in ascending order. M should collate before N; if it doesn't, an error results. As an example, `0-9' is the same as `0123456789'. GNU `tr' does not support the System V syntax that uses square brackets to enclose ranges. Translations specified in that format sometimes work as expected, since the brackets are often transliterated to themselves. However, they should be avoided because they sometimes behave unexpectedly. For example, `tr -d '[0-9]'' deletes brackets as well as digits. However, if the use of [] is deliberate, because git still cares about portability to ancient SYSV versions of tr that require that notation, then let me know and I'll undo that part of the change and add a comment to that effect. >From cd3c17c6a48c67d2a598001272f3283714b1df19 Mon Sep 17 00:00:00 2001 From: Jim Meyering Date: Mon, 21 Sep 2009 10:58:02 +0200 Subject: [PATCH] pre-commit.sample: don't use [...] around a tr range Using square brackets around a tr range is especially risky when using tr's -d (delete) option. In this case, the brackets happen to be harmless, since the two bracket bytes happen to be included in the desired range. Correct spelling and grammar. Signed-off-by: Jim Meyering --- templates/hooks--pre-commit.sample | 10 +++++----- 1 files changed, 5 insertions(+), 5 deletions(-) diff --git a/templates/hooks--pre-commit.sample b/templates/hooks--pre-commit.sample index b11ad6a..06a5afa 100755 --- a/templates/hooks--pre-commit.sample +++ b/templates/hooks--pre-commit.sample @@ -15,14 +15,14 @@ allownonascii=$(git config hooks.allownonascii) # printable range starts at the space character and ends with tilde. if [ "$allownonascii" != "true" ] && test "$(git diff --cached --name-only --diff-filter=A -z | - LC_ALL=C tr -d '[ -~]\0')" + LC_ALL=C tr -d ' -~\0')" then - echo "Error: Attempt to add a non-ascii filename." + echo "Error: Attempt to add a non-ascii file name." echo - echo "This can cause problems if you want to work together" - echo "with people on other platforms than you." + echo "This can cause problems if you want to work" + echo "with people on other platforms." echo - echo "To be portable it is adviseable to rename the file ..." + echo "To be portable it is advisable to rename the file ..." echo echo "If you know what you are doing you can disable this" echo "check using:" -- 1.6.5.rc1.214.g13c5a ``` ## Alex Riesen, 2009-09-21 09:43 Subject: Re: [PATCH] pre-commit.sample: don't use [...] around a tr range Message-ID: <81b0412b0909210243h58633509r7b427c69f62928fe@mail.gmail.com> URL: https://gitlist.dev/e/81b0412b0909210243h58633509r7b427c69f62928fe%40mail.gmail.com In-Reply-To: <871vm0k6jm.fsf@meyering.net> ``` On Mon, Sep 21, 2009 at 11:09, Jim Meyering wrote: > However, if the use of [] is deliberate, because git still > cares about portability to ancient SYSV versions of tr that > require that notation, then let me know and I'll undo that part > of the change and add a comment to that effect. We have (had?) people trying to support Git on HP-UX and SunOS. Do these count? ``` ## Jim Meyering, 2009-09-21 11:00 Subject: Re: [PATCH] pre-commit.sample: don't use [...] around a tr range Message-ID: <87ocp4imul.fsf@meyering.net> URL: https://gitlist.dev/e/87ocp4imul.fsf%40meyering.net In-Reply-To: <81b0412b0909210243h58633509r7b427c69f62928fe@mail.gmail.com> ``` Alex Riesen wrote: > On Mon, Sep 21, 2009 at 11:09, Jim Meyering wrote: >> However, if the use of [] is deliberate, because git still >> cares about portability to ancient SYSV versions of tr that >> require that notation, then let me know and I'll undo that part >> of the change and add a comment to that effect. > > We have (had?) people trying to support Git on HP-UX and SunOS. > Do these count? I had my doubts, but have just confirmed that Solaris 10's /usr/bin/tr is still doing it the SYSV way: $ echo foo | LC_ALL=C /usr/bin/tr a-z A-Z foo There, you have to use /usr/xpg4/bin/tr to get the expected behavior: $ echo foo | LC_ALL=C /usr/xpg4/bin/tr a-z A-Z FOO So you're right. Thanks! Here's an updated patch: >From 4048a5b393fa5f35dfe8157e98a6ea475b0efb2d Mon Sep 17 00:00:00 2001 From: Jim Meyering Date: Mon, 21 Sep 2009 10:58:02 +0200 Subject: [PATCH] pre-commit.sample: add comment re tr portability; fix grammar Add a comment explaining why square brackets around a tr range are not only ok, but actually required in this case. Correct spelling and grammar. Signed-off-by: Jim Meyering --- templates/hooks--pre-commit.sample | 11 +++++++---- 1 files changed, 7 insertions(+), 4 deletions(-) diff --git a/templates/hooks--pre-commit.sample b/templates/hooks--pre-commit.sample index b11ad6a..128c8cc 100755 --- a/templates/hooks--pre-commit.sample +++ b/templates/hooks--pre-commit.sample @@ -14,15 +14,18 @@ allownonascii=$(git config hooks.allownonascii) # them from being added to the repository. We exploit the fact that the # printable range starts at the space character and ends with tilde. if [ "$allownonascii" != "true" ] && + # Note that the use of brackets around a tr range is ok here, (it's + # even required, for portability to Solaris 10's /usr/bin/tr), since + # the square bracket bytes happen to fall in the designated range. test "$(git diff --cached --name-only --diff-filter=A -z | LC_ALL=C tr -d '[ -~]\0')" then - echo "Error: Attempt to add a non-ascii filename." + echo "Error: Attempt to add a non-ascii file name." echo - echo "This can cause problems if you want to work together" - echo "with people on other platforms than you." + echo "This can cause problems if you want to work" + echo "with people on other platforms." echo - echo "To be portable it is adviseable to rename the file ..." + echo "To be portable it is advisable to rename the file ..." echo echo "If you know what you are doing you can disable this" echo "check using:" -- 1.6.5.rc1.214.g13c5a ``` ## Alex Riesen, 2009-09-21 12:45 Subject: Re: [PATCH] pre-commit.sample: don't use [...] around a tr range Message-ID: <81b0412b0909210545n3da664c4r4e9250fa2eb241c4@mail.gmail.com> URL: https://gitlist.dev/e/81b0412b0909210545n3da664c4r4e9250fa2eb241c4%40mail.gmail.com In-Reply-To: <87ocp4imul.fsf@meyering.net> ``` On Mon, Sep 21, 2009 at 13:00, Jim Meyering wrote: > Alex Riesen wrote: >> On Mon, Sep 21, 2009 at 11:09, Jim Meyering wrote: >>> However, if the use of [] is deliberate, because git still >>> cares about portability to ancient SYSV versions of tr that >>> require that notation, then let me know and I'll undo that part >>> of the change and add a comment to that effect. >> >> We have (had?) people trying to support Git on HP-UX and SunOS. >> Do these count? > > I had my doubts, but have just confirmed that Solaris 10's > /usr/bin/tr is still doing it the SYSV way: > >    $ echo foo | LC_ALL=C /usr/bin/tr a-z A-Z >    foo > > There, you have to use /usr/xpg4/bin/tr to get the expected behavior: > >    $ echo foo | LC_ALL=C /usr/xpg4/bin/tr a-z A-Z >    FOO > > So you're right.  ... Sorry about that. I even made a simple tr once for Git tests exactly because of compatibility problems (look for "poor man tr" in the list archives, if needed). ``` ## Jeff King, 2009-09-21 13:44 Subject: Re: [PATCH] pre-commit.sample: don't use [...] around a tr range Message-ID: <20090921134427.GA20567@sigio.peff.net> URL: https://gitlist.dev/e/20090921134427.GA20567%40sigio.peff.net In-Reply-To: <87ocp4imul.fsf@meyering.net> ``` On Mon, Sep 21, 2009 at 01:00:34PM +0200, Jim Meyering wrote: > > We have (had?) people trying to support Git on HP-UX and SunOS. > > Do these count? > > I had my doubts, but have just confirmed that Solaris 10's > /usr/bin/tr is still doing it the SYSV way: > > $ echo foo | LC_ALL=C /usr/bin/tr a-z A-Z > foo > > There, you have to use /usr/xpg4/bin/tr to get the expected behavior: > > $ echo foo | LC_ALL=C /usr/xpg4/bin/tr a-z A-Z > FOO > > So you're right. Thanks! See: http://article.gmane.org/gmane.comp.version-control.git/76991 > + # Note that the use of brackets around a tr range is ok here, (it's > + # even required, for portability to Solaris 10's /usr/bin/tr), since > + # the square bracket bytes happen to fall in the designated range. > test "$(git diff --cached --name-only --diff-filter=A -z | > LC_ALL=C tr -d '[ -~]\0')" Does this work on non-bracket systems? I would think that enumerating the sequence would be the most portable thing. -Peff ``` ## Jim Meyering, 2009-09-21 14:10 Subject: Re: [PATCH] pre-commit.sample: don't use [...] around a tr range Message-ID: <87tyywgzhu.fsf@meyering.net> URL: https://gitlist.dev/e/87tyywgzhu.fsf%40meyering.net In-Reply-To: <20090921134427.GA20567@sigio.peff.net> ``` Jeff King wrote: > On Mon, Sep 21, 2009 at 01:00:34PM +0200, Jim Meyering wrote: > >> > We have (had?) people trying to support Git on HP-UX and SunOS. >> > Do these count? >> >> I had my doubts, but have just confirmed that Solaris 10's >> /usr/bin/tr is still doing it the SYSV way: >> >> $ echo foo | LC_ALL=C /usr/bin/tr a-z A-Z >> foo >> >> There, you have to use /usr/xpg4/bin/tr to get the expected behavior: >> >> $ echo foo | LC_ALL=C /usr/xpg4/bin/tr a-z A-Z >> FOO >> >> So you're right. Thanks! > > See: > > http://article.gmane.org/gmane.comp.version-control.git/76991 > >> + # Note that the use of brackets around a tr range is ok here, (it's >> + # even required, for portability to Solaris 10's /usr/bin/tr), since >> + # the square bracket bytes happen to fall in the designated range. >> test "$(git diff --cached --name-only --diff-filter=A -z | >> LC_ALL=C tr -d '[ -~]\0')" > > Does this work on non-bracket systems? Yes, since [] happen to fall in the range. > I would think that enumerating > the sequence would be the most portable thing. Enumerating is more portable, at the expense of readability and maintainability. In case you want to go that route, here's one more: (note that this, like the original range, treats TAB as nonportable) >From 40a368a7bcf0ac6524bbe36ba3bfdaa0711897b8 Mon Sep 17 00:00:00 2001 From: Jim Meyering Date: Mon, 21 Sep 2009 10:58:02 +0200 Subject: [PATCH] pre-commit.sample: use tr more portability; fix grammar Avoid tr's M-N range notation altogether. Instead, enumerate ascii bytes 32..126. Correct spelling and grammar. Signed-off-by: Jim Meyering --- templates/hooks--pre-commit.sample | 19 +++++++++++++------ 1 files changed, 13 insertions(+), 6 deletions(-) diff --git a/templates/hooks--pre-commit.sample b/templates/hooks--pre-commit.sample index b11ad6a..896eb9e 100755 --- a/templates/hooks--pre-commit.sample +++ b/templates/hooks--pre-commit.sample @@ -10,19 +10,26 @@ # If you want to allow non-ascii filenames set this variable to true. allownonascii=$(git config hooks.allownonascii) +printables=' !"#$%&'\''()*+,-./0123456789:;<=>?@' +printables="$printables"'ABCDEFGHIJKLMNOPQRSTUVWXYZ[\]^_`' +printables="$printables"'abcdefghijklmnopqrstuvwxyz{|}~' + # Cross platform projects tend to avoid non-ascii filenames; prevent # them from being added to the repository. We exploit the fact that the # printable range starts at the space character and ends with tilde. if [ "$allownonascii" != "true" ] && - test "$(git diff --cached --name-only --diff-filter=A -z | - LC_ALL=C tr -d '[ -~]\0')" + # Filter out printable ascii bytes, and NUL. + # If anything remains, you lose. + rem=$(git diff --cached --name-only --diff-filter=A -z | + LC_ALL=C tr -d "$printables"'\0') + test -n "$rem" then - echo "Error: Attempt to add a non-ascii filename." + echo "Error: Attempt to add a non-ascii file name." echo - echo "This can cause problems if you want to work together" - echo "with people on other platforms than you." + echo "This can cause problems if you want to work" + echo "with people on other platforms." echo - echo "To be portable it is adviseable to rename the file ..." + echo "To be portable it is advisable to rename the file ..." echo echo "If you know what you are doing you can disable this" echo "check using:" -- 1.6.5.rc1.214.g13c5a ``` ## Jeff King, 2009-09-21 14:21 Subject: Re: [PATCH] pre-commit.sample: don't use [...] around a tr range Message-ID: <20090921142157.GA20860@sigio.peff.net> URL: https://gitlist.dev/e/20090921142157.GA20860%40sigio.peff.net In-Reply-To: <87tyywgzhu.fsf@meyering.net> ``` On Mon, Sep 21, 2009 at 04:10:21PM +0200, Jim Meyering wrote: > >> test "$(git diff --cached --name-only --diff-filter=A -z | > >> LC_ALL=C tr -d '[ -~]\0')" > > > > Does this work on non-bracket systems? > > Yes, since [] happen to fall in the range. > > > I would think that enumerating > > the sequence would be the most portable thing. > > Enumerating is more portable, at the expense of > readability and maintainability. > In case you want to go that route, here's one more: Agreed. If we can avoid enumeration, we should. If your original is portable, then I think it is preferable. Thanks for looking into it. -Peff ``` ## Brandon Casey, 2009-09-21 15:58 Subject: Re: [PATCH] pre-commit.sample: don't use [...] around a tr range Message-ID: URL: https://gitlist.dev/e/n2z47RdNjxVbPeRfgJuY54nJrfzDfn43bzv8HoH6cEuAzQ6fB1MaBg%40cipher.nrlssc.navy.mil In-Reply-To: <20090921134427.GA20567@sigio.peff.net> ``` Jeff King wrote: > On Mon, Sep 21, 2009 at 01:00:34PM +0200, Jim Meyering wrote: > >>> We have (had?) people trying to support Git on HP-UX and SunOS. >>> Do these count? >> I had my doubts, but have just confirmed that Solaris 10's >> /usr/bin/tr is still doing it the SYSV way: >> >> $ echo foo | LC_ALL=C /usr/bin/tr a-z A-Z >> foo >> >> There, you have to use /usr/xpg4/bin/tr to get the expected behavior: >> >> $ echo foo | LC_ALL=C /usr/xpg4/bin/tr a-z A-Z >> FOO >> >> So you're right. Thanks! By the way, modern git inserts /usr/xpg4/bin into PATH before /usr/bin on Solaris. So /usr/xpg4/bin/tr should always be used on that platform. -brandon ``` ## Jeff King, 2009-09-24 05:55 Subject: Re: [PATCH] pre-commit.sample: don't use [...] around a tr range Message-ID: <20090924055518.GB24486@coredump.intra.peff.net> URL: https://gitlist.dev/e/20090924055518.GB24486%40coredump.intra.peff.net In-Reply-To: ``` On Mon, Sep 21, 2009 at 10:58:51AM -0500, Brandon Casey wrote: > >> I had my doubts, but have just confirmed that Solaris 10's > >> /usr/bin/tr is still doing it the SYSV way: > >> > >> $ echo foo | LC_ALL=C /usr/bin/tr a-z A-Z > >> foo > >> > >> There, you have to use /usr/xpg4/bin/tr to get the expected behavior: > >> > >> $ echo foo | LC_ALL=C /usr/xpg4/bin/tr a-z A-Z > >> FOO > >> > >> So you're right. Thanks! > > By the way, modern git inserts /usr/xpg4/bin into PATH before /usr/bin on > Solaris. So /usr/xpg4/bin/tr should always be used on that platform. Ah, I forgot about that patch. So it may not matter on Solaris. Are there other platforms which want the brackets? Maybe HP-UX, but I no longer have access to a box and I've used deep hypnosis to suppress any memories of ever having used it. Anyway, I think Jim's patch is the right fix if there are still platforms that want the brackets. -Peff ```