Re: [PATCH] Avoid using non-portable `echo -n` in tests.

Re: [PATCH] Avoid using non-portable `echo -n` in tests.

[Francis Galiegue]

Le Friday 31 October 2008 19:39:33 Jeff King, vous avez écrit :
[...]
>
> Agreed, and actually I found such a bashism (test ==) last week (though
> of course it also broke on FreeBSD).
>

As for bash-isms, a hunt for $(...) also looks necessary...

$ grep -rl '\$([^)]\+)' $(find -type f)|wc -l
272

Unless I'm mistaken (and I probably am), the $(...) construct is 
bash-specific, isn't it?

-- 
fge

Re: [PATCH] Avoid using non-portable `echo -n` in tests.

[Junio C Hamano]

Francis Galiegue <fg@one2team.net> writes:

> Unless I'm mistaken (and I probably am), the $(...) construct is 
> bash-specific, isn't it?

You are.

Re: [PATCH] Avoid using non-portable `echo -n` in tests.

[Ian Hilt]

On Fri, Oct 31, 2008 at 08:50:31PM +0100, Francis Galiegue wrote:
> Le Friday 31 October 2008 19:39:33 Jeff King, vous avez écrit :
> [...]
> >
> > Agreed, and actually I found such a bashism (test ==) last week (though
> > of course it also broke on FreeBSD).
> >
> 
> As for bash-isms, a hunt for $(...) also looks necessary...
> 
> $ grep -rl '\$([^)]\+)' $(find -type f)|wc -l
> 272
> 
> Unless I'm mistaken (and I probably am), the $(...) construct is 
> bash-specific, isn't it?

Nope.  Read section 2.6.3 Command Substitution here,

<http://www.opengroup.org/onlinepubs/009695399/utilities/xcu_chap02.html>

Re: [PATCH] Avoid using non-portable `echo -n` in tests.

[Francis Galiegue]

Le Friday 31 October 2008 21:11:27 Ian Hilt, vous avez écrit :
[...]
> >
> > Unless I'm mistaken (and I probably am), the $(...) construct is
> > bash-specific, isn't it?
>
> Nope.  Read section 2.6.3 Command Substitution here,
>
> <http://www.opengroup.org/onlinepubs/009695399/utilities/xcu_chap02.html>

My bad, then. I'm too "old-school", I guess ;)

-- 
fge

Re: [PATCH] t4030: Don't use echo -n

[Brian Gernhardt]

On Oct 30, 2008, at 7:52 PM, Ian Hilt wrote:

> On Thu, Oct 30, 2008 at 06:12:56PM -0400, Brian Gernhardt wrote:
>> Signed-off-by: Brian Gernhardt <benji@silverinsanity.com>
>
> No commit message???
>>

"Don't use echo -n" seemed simple enough to me, as it's the only thing  
it does.  And the reason is that echo -n isn't portable, as has been  
determined previously in git.  I'll update this and use the better  
replacement "printf" anyway.

> This doesn't apply for me.  My tip is up-to-date, but it doesn't  
> even have
> this file it t/.

This is based on next and is for a file that isn't in master.

~~ Brian

[PATCH] Avoid using non-portable `echo -n` in tests.

[Brian Gernhardt]

Not all /bin/sh have a builtin echo that recognizes -n.  Using printf
is far more portable.

Discovered on OS X 10.5.5 in t4030-diff-textconv.sh and changed in all
the test scripts.

Signed-off-by: Brian Gernhardt <benji@silverinsanity.com>
---
 t/t2005-checkout-index-symlinks.sh |    2 +-
 t/t2102-update-index-symlinks.sh   |    4 ++--
 t/t4030-diff-textconv.sh           |    2 +-
 t/t6025-merge-symlinks.sh          |    4 ++--
 t/t9400-git-cvsserver-server.sh    |    2 +-
 5 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/t/t2005-checkout-index-symlinks.sh b/t/t2005-checkout-index-symlinks.sh
index ed12c4d..9fa5610 100755
--- a/t/t2005-checkout-index-symlinks.sh
+++ b/t/t2005-checkout-index-symlinks.sh
@@ -13,7 +13,7 @@ file if core.symlinks is false.'
 test_expect_success \
 'preparation' '
 git config core.symlinks false &&
-l=$(echo -n file | git hash-object -t blob -w --stdin) &&
+l=$(printf file | git hash-object -t blob -w --stdin) &&
 echo "120000 $l	symlink" | git update-index --index-info'
 
 test_expect_success \
diff --git a/t/t2102-update-index-symlinks.sh b/t/t2102-update-index-symlinks.sh
index f195aef..1ed44ee 100755
--- a/t/t2102-update-index-symlinks.sh
+++ b/t/t2102-update-index-symlinks.sh
@@ -13,12 +13,12 @@ even if a plain file is in the working tree if core.symlinks is false.'
 test_expect_success \
 'preparation' '
 git config core.symlinks false &&
-l=$(echo -n file | git hash-object -t blob -w --stdin) &&
+l=$(printf file | git hash-object -t blob -w --stdin) &&
 echo "120000 $l	symlink" | git update-index --index-info'
 
 test_expect_success \
 'modify the symbolic link' '
-echo -n new-file > symlink &&
+printf new-file > symlink &&
 git update-index symlink'
 
 test_expect_success \
diff --git a/t/t4030-diff-textconv.sh b/t/t4030-diff-textconv.sh
index 3945731..a235955 100755
--- a/t/t4030-diff-textconv.sh
+++ b/t/t4030-diff-textconv.sh
@@ -105,7 +105,7 @@ index ad8b3d2..67be421
 EOF
 # make a symlink the hard way that works on symlink-challenged file systems
 test_expect_success 'textconv does not act on symlinks' '
-	echo -n frotz > file &&
+	printf frotz > file &&
 	git add file &&
 	git ls-files -s | sed -e s/100644/120000/ |
 		git update-index --index-info &&
diff --git a/t/t6025-merge-symlinks.sh b/t/t6025-merge-symlinks.sh
index 53892a5..433c4de 100755
--- a/t/t6025-merge-symlinks.sh
+++ b/t/t6025-merge-symlinks.sh
@@ -18,11 +18,11 @@ git add file &&
 git commit -m initial &&
 git branch b-symlink &&
 git branch b-file &&
-l=$(echo -n file | git hash-object -t blob -w --stdin) &&
+l=$(printf file | git hash-object -t blob -w --stdin) &&
 echo "120000 $l	symlink" | git update-index --index-info &&
 git commit -m master &&
 git checkout b-symlink &&
-l=$(echo -n file-different | git hash-object -t blob -w --stdin) &&
+l=$(printf file-different | git hash-object -t blob -w --stdin) &&
 echo "120000 $l	symlink" | git update-index --index-info &&
 git commit -m b-symlink &&
 git checkout b-file &&
diff --git a/t/t9400-git-cvsserver-server.sh b/t/t9400-git-cvsserver-server.sh
index c1850d2..f6a2dbd 100755
--- a/t/t9400-git-cvsserver-server.sh
+++ b/t/t9400-git-cvsserver-server.sh
@@ -424,7 +424,7 @@ cd "$WORKDIR"
 test_expect_success 'cvs update (-p)' '
     touch really-empty &&
     echo Line 1 > no-lf &&
-    echo -n Line 2 >> no-lf &&
+    printf Line 2 >> no-lf &&
     git add really-empty no-lf &&
     git commit -q -m "Update -p test" &&
     git push gitcvs.git >/dev/null &&
-- 
1.6.0.3.757.g1e5a4

Re: [PATCH] Avoid using non-portable `echo -n` in tests.

[Junio C Hamano]

Brian Gernhardt <benji@silverinsanity.com> writes:

> Not all /bin/sh have a builtin echo that recognizes -n.  Using printf
> is far more portable.

This is much better (minor nit: we do not care if echo is built-in or
/bin/echo); the point of t4030 is to produce an incomplete line ;-).

Will queue, thanks.

Re: [PATCH] Avoid using non-portable `echo -n` in tests.

[Brian Gernhardt]

On Oct 31, 2008, at 4:15 AM, Junio C Hamano wrote:

> Brian Gernhardt <benji@silverinsanity.com> writes:
>
>> Not all /bin/sh have a builtin echo that recognizes -n.  Using printf
>> is far more portable.
>
> This is much better (minor nit: we do not care if echo is built-in or
> /bin/echo); the point of t4030 is to produce an incomplete line ;-).

Well, yes.  The reason I phrased it that way was that my /bin/echo  
recognizes -n but /bin/sh does not.

~~ Brian

Re: [PATCH] Avoid using non-portable `echo -n` in tests.

[Junio C Hamano]

Brian Gernhardt <benji@silverinsanity.com> writes:

> Not all /bin/sh have a builtin echo that recognizes -n.  Using printf
> is far more portable.
>
> Discovered on OS X 10.5.5 in t4030-diff-textconv.sh and changed in all
> the test scripts.

I had an impression that OS X was BSDish.  Wasn't "echo -n" a BSDism?

Re: [PATCH] Avoid using non-portable `echo -n` in tests.

[Shawn O. Pearce]

Brian Gernhardt <benji@silverinsanity.com> wrote:
> Not all /bin/sh have a builtin echo that recognizes -n.  Using printf
> is far more portable.
 
> diff --git a/t/t9400-git-cvsserver-server.sh b/t/t9400-git-cvsserver-server.sh
> index c1850d2..f6a2dbd 100755
> --- a/t/t9400-git-cvsserver-server.sh
> +++ b/t/t9400-git-cvsserver-server.sh
> @@ -424,7 +424,7 @@ cd "$WORKDIR"
>  test_expect_success 'cvs update (-p)' '
>      touch really-empty &&
>      echo Line 1 > no-lf &&
> -    echo -n Line 2 >> no-lf &&
> +    printf Line 2 >> no-lf &&

That needs to be:

	printf 'Line 2'

to have the same result.  Fortunately I don't think it matters
in this test, but it does read odd.

-- 
Shawn.

Re: [PATCH] Avoid using non-portable `echo -n` in tests.

[Jeff King]

On Fri, Oct 31, 2008 at 01:09:13AM -0400, Brian Gernhardt wrote:

> Not all /bin/sh have a builtin echo that recognizes -n.  Using printf
> is far more portable.
> 
> Discovered on OS X 10.5.5 in t4030-diff-textconv.sh and changed in all
> the test scripts.

Hmph. I think this is a good patch, and there is precedent in the past
(20fa04ea, 2aad957, 9754563). But I am surprised this was not caught by
our recent autobuilding project.

However, it seems to work on FreeBSD (which makes it doubly weird that
it is broken on OS X). On Solaris, the /bin/sh is so horribly broken
that I have to use bash anyway. Commit 9754563 claims breakage on AIX,
but it looks like Mike is doing the AIX builds with bash.

So I guess we just need an OS X autobuild. ;)

>  t/t4030-diff-textconv.sh           |    2 +-

And of course this one was me, but I blame JSixt, whose test I just
mindlessly copied. ;)

-Peff

Re: [PATCH] Avoid using non-portable `echo -n` in tests.

[Pierre Habouzit]

[-- Attachment #1: Type: text/plain, Size: 1108 bytes --]

On Fri, Oct 31, 2008 at 06:24:56PM +0000, Jeff King wrote:
> On Fri, Oct 31, 2008 at 01:09:13AM -0400, Brian Gernhardt wrote:
> 
> > Not all /bin/sh have a builtin echo that recognizes -n.  Using printf
> > is far more portable.
> > 
> > Discovered on OS X 10.5.5 in t4030-diff-textconv.sh and changed in all
> > the test scripts.
> 
> Hmph. I think this is a good patch, and there is precedent in the past
> (20fa04ea, 2aad957, 9754563). But I am surprised this was not caught by
> our recent autobuilding project.

Set up a Debian autobuilder with dash as a /bin/sh (apt-get install
dash, dpkg-reconfigure -plow dash and say 'yes'). You'll see those kind
of problems arise immediately.

Dash is a POSIX compatible shell, with almost no extension added (in
particular its echo has no -n option) which helps to find those kind of
issues.

It would help detecting git shell scripts that use bashism as well.

-- 
·O·  Pierre Habouzit
··O                                                madcoder@debian.org
OOO                                                http://www.madism.org

[-- Attachment #2: Type: application/pgp-signature, Size: 197 bytes --]

Re: [PATCH] Avoid using non-portable `echo -n` in tests.

[Jeff King]

On Fri, Oct 31, 2008 at 07:36:01PM +0100, Pierre Habouzit wrote:

> Set up a Debian autobuilder with dash as a /bin/sh (apt-get install
> dash, dpkg-reconfigure -plow dash and say 'yes'). You'll see those kind
> of problems arise immediately.

I don't need to; my development box is Debian with dash as /bin/sh. :)

> Dash is a POSIX compatible shell, with almost no extension added (in
> particular its echo has no -n option) which helps to find those kind of
> issues.
> 
> It would help detecting git shell scripts that use bashism as well.

Agreed, and actually I found such a bashism (test ==) last week (though
of course it also broke on FreeBSD).

-Peff

Re: [PATCH] Avoid using non-portable `echo -n` in tests.

[Francis Galiegue]

Le Friday 31 October 2008 19:39:33 Jeff King, vous avez écrit :
[...]
>
> Agreed, and actually I found such a bashism (test ==) last week (though
> of course it also broke on FreeBSD).
>

As for bash-isms, a hunt for $(...) also looks necessary...

$ grep -rl '\$([^)]\+)' $(find -type f)|wc -l
272

Unless I'm mistaken (and I probably am), the $(...) construct is 
bash-specific, isn't it?

-- 
Francis Galiegue
ONE2TEAM
Ingénieur système
Mob : +33 (0) 6 83 87 78 75
Tel : +33 (0) 1 78 94 55 52
fge@one2team.com
40 avenue Raymond Poincaré
75116 Paris

Re: [PATCH] Avoid using non-portable `echo -n` in tests.

[Johannes Schindelin]

Hi,

On Fri, 31 Oct 2008, Francis Galiegue wrote:

> Unless I'm mistaken (and I probably am), the $(...) construct is 
> bash-specific, isn't it?

You're mistaken,
Dscho

P.S.: 
http://www.opengroup.org/onlinepubs/000095399/utilities/xcu_chap02.html#tag_02_06_03

Re: [PATCH] Avoid using non-portable `echo -n` in tests.

[Mike Ralphson]

2008/10/31 Jeff King <peff@peff.net>:
> On Fri, Oct 31, 2008 at 01:09:13AM -0400, Brian Gernhardt wrote:
>
>> Not all /bin/sh have a builtin echo that recognizes -n.  Using printf
>> is far more portable.
>>
>> Discovered on OS X 10.5.5 in t4030-diff-textconv.sh and changed in all
>> the test scripts.
>
> Hmph. I think this is a good patch, and there is precedent in the past
> (20fa04ea, 2aad957, 9754563). But I am surprised this was not caught by
> our recent autobuilding project.
>
> However, it seems to work on FreeBSD (which makes it doubly weird that
> it is broken on OS X). On Solaris, the /bin/sh is so horribly broken
> that I have to use bash anyway. Commit 9754563 claims breakage on AIX,
> but it looks like Mike is doing the AIX builds with bash.

I've just retested with AIX's standard sh (ksh) instead of bash and
there were only two issues.

Oddly enough, one was with the (original) printfs in
t4030-diff-textconv.sh. AIX seems to ship with a perfectly good
printf, but if you install the GNU tools from the 'IBM toolbox for
AIX' (and prepend them to your PATH) it replaces that printf with one
which doesn't grok the "\\1\\n" syntax, but I think wants it to be
"\\01\\n".

The other problem is the 'trap exit' one discussed here:
http://thread.gmane.org/gmane.comp.version-control.git/92748/focus=92944

Mike