threads / patch / 21015

patchpre-commit.sample: don't use [...] around a tr range

Subject: [PATCH] pre-commit.sample: don't use [...] around a tr range

## tl;dr

9 messages between Sep 21, 2009 and Sep 24, 2009. Diffs are folded; open one to read it.

replies: 8people: 4as markdown or json

Jim Meyering· Sep 21, 2009, 09:09 UTC · lore

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 <meyering@redhat.com>
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 <meyering@redhat.com>
---
 templates/hooks--pre-commit.sample |   10 +++++-----
 1 files changed, 5 insertions(+), 5 deletions(-)
Show changes to templates/hooks--pre-commit.sample +5 −6
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· Sep 21, 2009, 09:43 UTC · re: Jim Meyering · lore

Re: [PATCH] pre-commit.sample: don't use [...] around a tr range

On Mon, Sep 21, 2009 at 11:09, Jim Meyering <jim@meyering.net> 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· Sep 21, 2009, 11:00 UTC · re: Alex Riesen · lore

Re: [PATCH] pre-commit.sample: don't use [...] around a tr range

Alex Riesen wrote:
Show 8 quoted lines
> On Mon, Sep 21, 2009 at 11:09, Jim Meyering <jim@meyering.net> 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 <meyering@redhat.com>
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 <meyering@redhat.com>
---
 templates/hooks--pre-commit.sample |   11 +++++++----
 1 files changed, 7 insertions(+), 4 deletions(-)
Show changes to templates/hooks--pre-commit.sample +7 −5
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· Sep 21, 2009, 12:45 UTC · re: Jim Meyering · lore

Re: [PATCH] pre-commit.sample: don't use [...] around a tr range

On Mon, Sep 21, 2009 at 13:00, Jim Meyering <jim@meyering.net> wrote:
Show 22 quoted lines
> Alex Riesen wrote:
>> On Mon, Sep 21, 2009 at 11:09, Jim Meyering <jim@meyering.net> 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· Sep 21, 2009, 13:44 UTC · re: Jim Meyering · lore

Re: [PATCH] pre-commit.sample: don't use [...] around a tr range

On Mon, Sep 21, 2009 at 01:00:34PM +0200, Jim Meyering wrote:
Show 15 quoted lines
> > 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
Show 5 quoted lines
> +	# 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· Sep 21, 2009, 14:10 UTC · re: Jeff King · lore

Re: [PATCH] pre-commit.sample: don't use [...] around a tr range

Jeff King wrote:
Show 29 quoted lines
> 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 <meyering@redhat.com>
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 <meyering@redhat.com>
---
 templates/hooks--pre-commit.sample |   19 +++++++++++++------
 1 files changed, 13 insertions(+), 6 deletions(-)
Show changes to templates/hooks--pre-commit.sample +13 −7
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· Sep 21, 2009, 14:21 UTC · re: Jim Meyering · lore

Re: [PATCH] pre-commit.sample: don't use [...] around a tr range

On Mon, Sep 21, 2009 at 04:10:21PM +0200, Jim Meyering wrote:
Show 13 quoted lines
> >>  	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· Sep 21, 2009, 15:58 UTC · re: Jeff King · lore

Re: [PATCH] pre-commit.sample: don't use [...] around a tr range

Jeff King wrote:
Show 16 quoted lines
> 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· Sep 24, 2009, 05:55 UTC · re: Brandon Casey · lore

Re: [PATCH] pre-commit.sample: don't use [...] around a tr range

On Mon, Sep 21, 2009 at 10:58:51AM -0500, Brandon Casey wrote:
Show 15 quoted lines
> >> 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

← back to recent threads