Solaris test failure for git-1.5.4.4 -- t0021-conversion.sh

Solaris test failure for git-1.5.4.4 -- t0021-conversion.sh

[Whit Armstrong]

Git Version: git-1.5.4.4

xs5-trd-p1.grn:warmstro> uname -a
SunOS xs5-trd-p1.grn.tudor.com 5.9 Generic_118558-38 sun4u sparc
SUNW,Sun-Fire-V240

looks like most errors below are related to tr.  The tr I'm using is in xpg4.

xs5-trd-p1.grn:warmstro> which tr
/usr/xpg4/bin/tr



xs5-trd-p1.grn:warmstro> sh t0021-conversion.sh -i -v
* expecting success:
        git config filter.rot13.smudge ./rot13.sh &&
        git config filter.rot13.clean ./rot13.sh &&

        {
            echo "*.t filter=rot13"
            echo "*.i ident"
        } >.gitattributes &&

        {
            echo a b c d e f g h i j k l m
            echo n o p q r s t u v w x y z
            echo '$Id$'
        } >test &&
        cat test >test.t &&
        cat test >test.o &&
        cat test >test.i &&
        git add test test.t test.i &&
        rm -f test test.t test.i &&
        git checkout -- test test.t test.i

tr: Bad string.
error: external filter ./rot13.sh failed 1
error: waitpid (async) failed
error: external filter ./rot13.sh failed
tr: Bad string.
error: external filter ./rot13.sh failed 1
error: waitpid (async) failed
error: external filter ./rot13.sh failed
tr: Bad string.
error: external filter ./rot13.sh failed 1
error: waitpid (async) failed
error: external filter ./rot13.sh failed
*   ok 1: setup

* expecting success:

        cmp test.o test &&
        cmp test.o test.t &&

        # ident should be stripped in the repository
        git diff --raw --exit-code :test :test.i &&
        id=$(git rev-parse --verify :test) &&
        embedded=$(sed -ne "$script" test.i) &&
        test "z$id" = "z$embedded" &&

        git cat-file blob :test.t > test.r &&

        ./rot13.sh < test.o > test.t &&
        cmp test.r test.t

tr: Bad string.
* FAIL 2: check


                cmp test.o test &&
                cmp test.o test.t &&

                # ident should be stripped in the repository
                git diff --raw --exit-code :test :test.i &&
                id=$(git rev-parse --verify :test) &&
                embedded=$(sed -ne "$script" test.i) &&
                test "z$id" = "z$embedded" &&

                git cat-file blob :test.t > test.r &&

                ./rot13.sh < test.o > test.t &&
                cmp test.r test.t

xs5-trd-p1.grn:warmstro>

Re: Solaris test failure for git-1.5.4.4 -- t0021-conversion.sh

[Johannes Schindelin]

Hi,

On Tue, 11 Mar 2008, Whit Armstrong wrote:

> tr: Bad string.

What string?

> tr: Bad string.

What string?

> tr: Bad string.

What...

you get the idea.

Ciao,
Dscho

Re: Solaris test failure for git-1.5.4.4 -- t0021-conversion.sh

[Johannes Sixt]

Johannes Schindelin schrieb:
> Hi,
> 
> On Tue, 11 Mar 2008, Whit Armstrong wrote:
> 
>> tr: Bad string.
> 
> What string?

It's this tr, which is the implementation of ./rot13.sh:

   tr '[a-zA-Z]' '[n-za-mN-ZA-M]'

I can't tell what's wrong with it on Solaris, though.

-- Hannes

Re: Solaris test failure for git-1.5.4.4 -- t0021-conversion.sh

[David Kastrup]

Johannes Sixt <j.sixt@viscovery.net> writes:

> Johannes Schindelin schrieb:
>> Hi,
>> 
>> On Tue, 11 Mar 2008, Whit Armstrong wrote:
>> 
>>> tr: Bad string.
>> 
>> What string?
>
> It's this tr, which is the implementation of ./rot13.sh:
>
>    tr '[a-zA-Z]' '[n-za-mN-ZA-M]'
>
> I can't tell what's wrong with it on Solaris, though.

The brackets?

-- 
David Kastrup

Re: Solaris test failure for git-1.5.4.4 -- t0021-conversion.sh

[Asheesh Laroia]

On Tue, 11 Mar 2008, Whit Armstrong wrote:

> xs5-trd-p1.grn:warmstro> sh t0021-conversion.sh -i -v

Try running this as "sh -x t0021-conversion.sh -i -v".  That will print 
the exact commands run just before they are run, letting you figure 
out what tr is complaining about.

-- Asheesh.

-- 
I know what "custody" [of the children] means.  "Get even."  That's all
custody means.  Get even with your old lady.
 		-- Lenny Bruce

Re: Solaris test failure for git-1.5.4.4 -- t0021-conversion.sh

[Whit Armstrong]

Thanks for the suggestion.  I found a few things in trash:

xs5-trd-p1.grn:warmstro> pwd
/home/warmstro/download/solaris/git-1.5.4.4/t/trash
xs5-trd-p1.grn:warmstro> cat rot13.sh
tr '[a-zA-Z]' '[n-za-mN-ZA-M]'
xs5-trd-p1.grn:warmstro> tr '[a-zA-Z]' '[n-za-mN-ZA-M]'
tr: Bad string.
xs5-trd-p1.grn:warmstro>

does that help, or do you need a bit more than that?

-Whit





On Tue, Mar 11, 2008 at 12:55 PM, Asheesh Laroia <asheesh@asheesh.org> wrote:
> On Tue, 11 Mar 2008, Whit Armstrong wrote:
>
>
> > xs5-trd-p1.grn:warmstro> sh t0021-conversion.sh -i -v
>
>  Try running this as "sh -x t0021-conversion.sh -i -v".  That will print
>  the exact commands run just before they are run, letting you figure
>  out what tr is complaining about.
>
>  -- Asheesh.
>
>  --
>  I know what "custody" [of the children] means.  "Get even."  That's all
>  custody means.  Get even with your old lady.
>                 -- Lenny Bruce
>

Re: Solaris test failure for git-1.5.4.4 -- t0021-conversion.sh

[Jeff King]

On Tue, Mar 11, 2008 at 01:14:39PM -0400, Whit Armstrong wrote:

> Thanks for the suggestion.  I found a few things in trash:
> 
> xs5-trd-p1.grn:warmstro> pwd
> /home/warmstro/download/solaris/git-1.5.4.4/t/trash
> xs5-trd-p1.grn:warmstro> cat rot13.sh
> tr '[a-zA-Z]' '[n-za-mN-ZA-M]'
> xs5-trd-p1.grn:warmstro> tr '[a-zA-Z]' '[n-za-mN-ZA-M]'
> tr: Bad string.
> xs5-trd-p1.grn:warmstro>
> 
> does that help, or do you need a bit more than that?

Looks like /usr/ucb/tr is OK with this, but /usr/bin/tr is not. Both
seem to handle the more verbose:

tr \
  'abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ' \
  'nopqrstuvwxyzabcdefghijklmNOPQRSTUVWXYZABCDEFGHIJKLM'

just fine.

-Peff

Re: Solaris test failure for git-1.5.4.4 -- t0021-conversion.sh

[Jeff King]

On Tue, Mar 11, 2008 at 01:35:37PM -0400, Jeff King wrote:

> Looks like /usr/ucb/tr is OK with this, but /usr/bin/tr is not. Both
> seem to handle the more verbose:
> 
> tr \
>   'abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ' \
>   'nopqrstuvwxyzabcdefghijklmNOPQRSTUVWXYZABCDEFGHIJKLM'

And here it is in patch form.

-- >8 --
t0021: tr portability fix for Solaris

Solaris' /usr/bin/tr doesn't seem to like multiple character
ranges in brackets (it simply prints "Bad string"). Instead,
let's just enumerate the transformation we want.

Signed-off-by: Jeff King <peff@peff.net>
---
 t/t0021-conversion.sh |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/t/t0021-conversion.sh b/t/t0021-conversion.sh
index cb86029..8fc39d7 100755
--- a/t/t0021-conversion.sh
+++ b/t/t0021-conversion.sh
@@ -5,7 +5,9 @@ test_description='blob conversion via gitattributes'
 . ./test-lib.sh
 
 cat <<\EOF >rot13.sh
-tr '[a-zA-Z]' '[n-za-mN-ZA-M]'
+tr \
+  'abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ' \
+  'nopqrstuvwxyzabcdefghijklmNOPQRSTUVWXYZABCDEFGHIJKLM'
 EOF
 chmod +x rot13.sh
 
-- 
1.5.4.3.621.g34987

Re: Solaris test failure for git-1.5.4.4 -- t0021-conversion.sh

[Whit Armstrong]

works great.  thanks.

running rest of test suite now.


On Tue, Mar 11, 2008 at 1:40 PM, Jeff King <peff@peff.net> wrote:
> On Tue, Mar 11, 2008 at 01:35:37PM -0400, Jeff King wrote:
>
>  > Looks like /usr/ucb/tr is OK with this, but /usr/bin/tr is not. Both
>  > seem to handle the more verbose:
>  >
>  > tr \
>  >   'abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ' \
>  >   'nopqrstuvwxyzabcdefghijklmNOPQRSTUVWXYZABCDEFGHIJKLM'
>
>  And here it is in patch form.
>
>  -- >8 --
>  t0021: tr portability fix for Solaris
>
>  Solaris' /usr/bin/tr doesn't seem to like multiple character
>  ranges in brackets (it simply prints "Bad string"). Instead,
>  let's just enumerate the transformation we want.
>
>  Signed-off-by: Jeff King <peff@peff.net>
>  ---
>   t/t0021-conversion.sh |    4 +++-
>   1 files changed, 3 insertions(+), 1 deletions(-)
>
>  diff --git a/t/t0021-conversion.sh b/t/t0021-conversion.sh
>  index cb86029..8fc39d7 100755
>  --- a/t/t0021-conversion.sh
>  +++ b/t/t0021-conversion.sh
>  @@ -5,7 +5,9 @@ test_description='blob conversion via gitattributes'
>   . ./test-lib.sh
>
>   cat <<\EOF >rot13.sh
>
> -tr '[a-zA-Z]' '[n-za-mN-ZA-M]'
>  +tr \
>  +  'abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ' \
>  +  'nopqrstuvwxyzabcdefghijklmNOPQRSTUVWXYZABCDEFGHIJKLM'
>   EOF
>   chmod +x rot13.sh
>
>  --
>  1.5.4.3.621.g34987
>
>

Re: Solaris test failure for git-1.5.4.4 -- t0021-conversion.sh

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

>> Looks like /usr/ucb/tr is OK with this, but /usr/bin/tr is not. Both
>> seem to handle the more verbose:
>> 
>> tr \
>>   'abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ' \
>>   'nopqrstuvwxyzabcdefghijklmNOPQRSTUVWXYZABCDEFGHIJKLM'

Besides tests, I think we have invocations of "tr" that Solaris people
might want to audit in the real scripts.  POSIX does not require [] around
the strings, but when from-string and to-string are of the same length,
having [] around them would not hurt (they transliterate to themselves)
and that is the only reason why we tend to use [] form to help potential
porters to ancient System V systems ;-)

filter-branch uses A-Z (BSD style) range which also is Kosher in POSIX but
will break historical System V that requires [] around ranges.

bisect has "tr '[\012]' ' '" which I think we can get rid of by rewriting
the users of the resulting string.

Re: Solaris test failure for git-1.5.4.4 -- t0021-conversion.sh

[Jeff King]

On Tue, Mar 11, 2008 at 09:11:28PM -0700, Junio C Hamano wrote:

> Besides tests, I think we have invocations of "tr" that Solaris people
> might want to audit in the real scripts.  POSIX does not require [] around
> the strings, but when from-string and to-string are of the same length,
> having [] around them would not hurt (they transliterate to themselves)
> and that is the only reason why we tend to use [] form to help potential
> porters to ancient System V systems ;-)

I'm not sure that it is always OK to just put in extra brackets, at
least in the case of multiple ranges. Consider the patch I just posted:

$ uname -a
SunOS tokyo 5.8 Generic_117350-47 sun4u sparc SUNW,Ultra-4

# what we have now, ok with bsd
$ echo foo | /usr/ucb/tr '[A-Za-z]' '[N-ZA-Mn-za-m]'
sbb

# what we have now, broken with SysV because of multiple ranges
echo foo | /usr/bin/tr '[A-Za-z]' '[N-ZA-Mn-za-m]'
Bad string

# potential fix, break multiple ranges. works with SysV
echo foo | /usr/bin/tr '[A-Z][a-z]' '[N-Z][A-M][n-z][a-m]'
sbb

# but here it breaks bsd
echo foo | /usr/ucb/tr '[A-Z][a-z]' '[N-Z][A-M][n-z][a-m]'
qzz


which makes no sense to me at all. If it were ignoring the duplicated
'[' on input, then I could understand a shift of one character. But
there is a shift of two characters.

> bisect has "tr '[\012]' ' '" which I think we can get rid of by rewriting
> the users of the resulting string.

This one is just being overzealous with the brackets. There is no need
for them unless there is a character sequence. This works correctly as

   tr '\012' ' '

at least using the Solaris SysV tr.

Below is a patch that fixes all of the problematic sites I found. Each
fix type was hand-tested with the Solaris SysV and UCB versions of tr,
and the tests ran successfully on Linux. On Solaris, I tried the tests
using /usr/bin/tr. However, I ran into problems with t1300, which tries:

  tr '\000' 'Q'

It seems that /usr/bin/tr silently removes NULs from the input stream:

  $ printf 'foo\000bar' | xxd
  0000000: 666f 6f00 6261 72                        foo.bar
  $ printf 'foo\000bar' | /usr/bin/tr '\000' 'Q' | xxd
  0000000: 666f 6f62 6172                           foobar

I don't think there is an easy way around this; we might have to switch
to using perl.

-- >8 --
tr portability fixes

Specifying character ranges in tr differs between System V
and POSIX. In System V, brackets are required (e.g.,
'[A-Z]'), whereas in POSIX they are not.

We can mostly get around this by just using the bracket form
for both sets, as in:

  tr '[A-Z] '[a-z]'

in which case POSIX interpets this as "'[' becomes '['",
which is OK.

However, this doesn't work with multiple sequences, like:

  # rot13
  tr '[A-Z][a-z]' '[N-Z][A-M][n-z][a-m]'

where the POSIX version does not behave the same as the
System V version. In this case, we must simply enumerate the
sequence.

This patch fixes problematic uses of tr in git scripts and
test scripts in one of three ways:

  - if a single sequence, make sure it uses brackets
  - if multiple sequences, enumerate
  - if extra brackets (e.g., tr '[A]' 'a'), eliminate
    brackets
---
 git-bisect.sh            |    4 ++--
 git-filter-branch.sh     |    4 ++--
 t/t4022-diff-rewrite.sh  |    5 ++++-
 t/t7003-filter-branch.sh |    2 +-
 4 files changed, 9 insertions(+), 6 deletions(-)

diff --git a/git-bisect.sh b/git-bisect.sh
index 2c32d0b..48fb92d 100755
--- a/git-bisect.sh
+++ b/git-bisect.sh
@@ -293,14 +293,14 @@ bisect_next() {
 	bisect_next_check good
 
 	skip=$(git for-each-ref --format='%(objectname)' \
-		"refs/bisect/skip-*" | tr '[\012]' ' ') || exit
+		"refs/bisect/skip-*" | tr '\012' ' ') || exit
 
 	BISECT_OPT=''
 	test -n "$skip" && BISECT_OPT='--bisect-all'
 
 	bad=$(git rev-parse --verify refs/bisect/bad) &&
 	good=$(git for-each-ref --format='^%(objectname)' \
-		"refs/bisect/good-*" | tr '[\012]' ' ') &&
+		"refs/bisect/good-*" | tr '\012' ' ') &&
 	eval="git rev-list --bisect-vars $BISECT_OPT $good $bad --" &&
 	eval="$eval $(cat "$GIT_DIR/BISECT_NAMES")" &&
 	eval=$(filter_skipped "$eval" "$skip") &&
diff --git a/git-filter-branch.sh b/git-filter-branch.sh
index 010353a..59cf023 100755
--- a/git-filter-branch.sh
+++ b/git-filter-branch.sh
@@ -58,8 +58,8 @@ eval "$functions"
 # "author" or "committer
 
 set_ident () {
-	lid="$(echo "$1" | tr "A-Z" "a-z")"
-	uid="$(echo "$1" | tr "a-z" "A-Z")"
+	lid="$(echo "$1" | tr "[A-Z]" "[a-z]")"
+	uid="$(echo "$1" | tr "[a-z]" "[A-Z]")"
 	pick_id_script='
 		/^'$lid' /{
 			s/'\''/'\''\\'\'\''/g
diff --git a/t/t4022-diff-rewrite.sh b/t/t4022-diff-rewrite.sh
index 6de4acb..bf996fc 100755
--- a/t/t4022-diff-rewrite.sh
+++ b/t/t4022-diff-rewrite.sh
@@ -8,7 +8,10 @@ test_expect_success setup '
 
 	cat ../../COPYING >test &&
 	git add test &&
-	tr 'a-zA-Z' 'n-za-mN-ZA-M' <../../COPYING >test
+	tr \
+	  "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ" \
+	  "nopqrstuvwxyzabcdefghijklmNOPQRSTUVWXYZABCDEFGHIJKLM" \
+	  <../../COPYING >test
 
 '
 
diff --git a/t/t7003-filter-branch.sh b/t/t7003-filter-branch.sh
index 6e14bf1..553131f 100755
--- a/t/t7003-filter-branch.sh
+++ b/t/t7003-filter-branch.sh
@@ -4,7 +4,7 @@ test_description='git-filter-branch'
 . ./test-lib.sh
 
 make_commit () {
-	lower=$(echo $1 | tr A-Z a-z)
+	lower=$(echo $1 | tr '[A-Z]' '[a-z]')
 	echo $lower > $lower
 	git add $lower
 	test_tick
-- 
1.5.4.3.613.gdd757

Re: Solaris test failure for git-1.5.4.4 -- t0021-conversion.sh

[Jeff King]

On Wed, Mar 12, 2008 at 08:43:36AM -0400, Jeff King wrote:

> It seems that /usr/bin/tr silently removes NULs from the input stream:
> 
>   $ printf 'foo\000bar' | xxd
>   0000000: 666f 6f00 6261 72                        foo.bar
>   $ printf 'foo\000bar' | /usr/bin/tr '\000' 'Q' | xxd
>   0000000: 666f 6f62 6172                           foobar
> 
> I don't think there is an easy way around this; we might have to switch
> to using perl.

Even more exciting, /usr/bin/tr actually works going the opposite
direction, but /usr/ucb/tr doesn't:

  $ echo x | /usr/bin/tr x '\000' | xxd
  0000000: 000a                                     ..
  $ echo x | /usr/ucb/tr x '\000' | xxd
  0000000: 780a                                     x.

but other characters seem to work:

  $ echo x | /usr/ucb/tr x '\012' | xxd
  0000000: 0a0a                                     ..

-Peff

Re: Solaris test failure for git-1.5.4.4 -- t0021-conversion.sh

[Johannes Schindelin]

Hi,

On Wed, 12 Mar 2008, Jeff King wrote:

> On Wed, Mar 12, 2008 at 08:43:36AM -0400, Jeff King wrote:
> 
> > It seems that /usr/bin/tr silently removes NULs from the input stream:
> > 
> >   $ printf 'foo\000bar' | xxd
> >   0000000: 666f 6f00 6261 72                        foo.bar
> >   $ printf 'foo\000bar' | /usr/bin/tr '\000' 'Q' | xxd
> >   0000000: 666f 6f62 6172                           foobar
> > 
> > I don't think there is an easy way around this; we might have to switch
> > to using perl.
> 
> Even more exciting, /usr/bin/tr actually works going the opposite
> direction, but /usr/ucb/tr doesn't:
> 
>   $ echo x | /usr/bin/tr x '\000' | xxd
>   0000000: 000a                                     ..
>   $ echo x | /usr/ucb/tr x '\000' | xxd
>   0000000: 780a                                     x.
> 
> but other characters seem to work:
> 
>   $ echo x | /usr/ucb/tr x '\012' | xxd
>   0000000: 0a0a                                     ..

All the more reson to make more things builtin.  (I agree that for tests, 
we should stay with tr, though.)

Ciao,
Dscho