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

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

[Jim Meyering]

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(-)

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

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

[Alex Riesen]

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?

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

[Jim Meyering]

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.  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(-)

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

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

[Alex Riesen]

On Mon, Sep 21, 2009 at 13:00, Jim Meyering <jim@meyering.net> wrote:
> 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).

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

[Jeff King]

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

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

[Jim Meyering]

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 <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(-)

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

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

[Jeff King]

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

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

[Brandon Casey]

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

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

[Jeff King]

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