How do I run tests under Valgrind?

How do I run tests under Valgrind?

[Ramkumar Ramachandra]

Hi,

I tried running `make valgind` inside t/ and got:

    bug in test framework: multiple prerequisite tags do not work reliably

which means that even the basic tests don't pass.  Am I doing something wrong?

Ram

Re: How do I run tests under Valgrind?

[Jeff King]

On Mon, Sep 17, 2012 at 10:31:07PM +0530, Ramkumar Ramachandra wrote:

> I tried running `make valgind` inside t/ and got:
> 
>     bug in test framework: multiple prerequisite tags do not work reliably
> 
> which means that even the basic tests don't pass.  Am I doing something wrong?

No, that should work (and it does work here). I assume you can pass
t0000 without --valgrind?

It seems very odd that valgrind would have an impact here, since those
tests are not even running git.

-Peff

Re: How do I run tests under Valgrind?

[Ramkumar Ramachandra]

Hi Peff,

Jeff King wrote:
> No, that should work (and it does work here). I assume you can pass
> t0000 without --valgrind?

Yes.  Here's the output from running it with --valgrind.  I'm digging
deeper to see what went wrong.

make_valgrind_symlink:6: permission denied:
/home/artagnon/src/git/t/../git-instaweb
./test-lib.sh:487: no matches found:
/home/artagnon/svn/prefix/svn-trunk/bin:/home/artagnon/bin:/home/artagnon/bin/depot_tools:/home/artagnon/.ruby/bin:/home/artagnon/.python/bin:/home/artagnon/.cabal/bin:/home/artagnon/svn/prefix/svn-trunk/bin:/home/artagnon/bin:/home/artagnon/bin/depot_tools:/home/artagnon/.ruby/bin:/home/artagnon/.python/bin:/home/artagnon/.cabal/bin:/home/artagnon/bin:/usr/local/bin:/usr/bin:/bin:/usr/games:/home/artagnon/.ec2/bin:/home/artagnon/.ec2/bin/git-*
Initialized empty Git repository in /home/artagnon/src/git/t/trash
directory.test-lib/.git/
expecting success:
        find .git/objects -type f -print >should-be-empty &&
        test_line_count = 0 should-be-empty

ok 1 - .git/objects should be empty after git init in an empty repo

expecting success:
        find .git/objects -type d -print >full-of-directories &&
        test_line_count = 3 full-of-directories

ok 2 - .git/objects should have 3 subdirectories

expecting success:
        :

ok 3 - success is reported like this

checking known breakage:
        false

not ok 4 - pretend we have a known breakage # TODO known breakage

expecting success:
        mkdir passing-todo &&
        (cd passing-todo &&
        cat >passing-todo.sh <<-EOF &&
        #!/bin/sh

        test_description='A passing TODO test

        This is run in a sub test-lib so that we do not get incorrect
        passing metrics
        '

        # Point to the t/test-lib.sh, which isn't in ../ as usual
        TEST_DIRECTORY="/home/artagnon/src/git/t"
        . "$TEST_DIRECTORY"/test-lib.sh

        test_expect_failure 'pretend we have fixed a known breakage' '
                :
        '

        test_done
        EOF
        chmod +x passing-todo.sh &&
        ./passing-todo.sh >out 2>err &&
        ! test -s err &&
        sed -e 's/^> //' >expect <<-\EOF &&
        > ok 1 - pretend we have fixed a known breakage # TODO known breakage
        > # fixed 1 known breakage(s)
        > # passed all 1 test(s)
        > 1..1
        EOF
        test_cmp expect out)

test_cmp:1: command not found: diff -u
not ok - 5 pretend we have fixed a known breakage (run in sub test-lib)
#
#               mkdir passing-todo &&
#               (cd passing-todo &&
#               cat >passing-todo.sh <<-EOF &&
#               #!/bin/sh
#
#               test_description='A passing TODO test
#
#               This is run in a sub test-lib so that we do not get incorrect
#               passing metrics
#               '
#
#               # Point to the t/test-lib.sh, which isn't in ../ as usual
#               TEST_DIRECTORY="/home/artagnon/src/git/t"
#               . "$TEST_DIRECTORY"/test-lib.sh
#
#               test_expect_failure 'pretend we have fixed a known breakage' '
#                       :
#               '
#
#               test_done
#               EOF
#               chmod +x passing-todo.sh &&
#               ./passing-todo.sh >out 2>err &&
#               ! test -s err &&
#               sed -e 's/^> //' >expect <<-\EOF &&
#               > ok 1 - pretend we have fixed a known breakage # TODO
known breakage
#               > # fixed 1 known breakage(s)
#               > # passed all 1 test(s)
#               > 1..1
#               EOF
#               test_cmp expect out)
#

expecting success:
        test_have_prereq HAVEIT &&
        haveit=yes

ok 6 - test runs if prerequisite is satisfied

skipping test: unmet prerequisite causes test to be skipped
        donthaveit=no

ok 7 # skip unmet prerequisite causes test to be skipped (missing DONTHAVEIT)

skipping test: test runs if prerequisites are satisfied
        test_have_prereq HAVEIT &&
        test_have_prereq HAVETHIS &&
        haveit=yes

ok 8 # skip test runs if prerequisites are satisfied (missing HAVETHIS,HAVEIT)

skipping test: unmet prerequisites causes test to be skipped
        donthaveit=no

ok 9 # skip unmet prerequisites causes test to be skipped (missing
HAVEIT,DONTHAVEIT)

skipping test: unmet prerequisites causes test to be skipped
        donthaveiteither=no

ok 10 # skip unmet prerequisites causes test to be skipped (missing
DONTHAVEIT,HAVEIT)

bug in test framework: multiple prerequisite tags do not work reliably
FATAL: Unexpected exit with code 1

> It seems very odd that valgrind would have an impact here, since those
> tests are not even running git.

Yeah, I know!

Ram

Re: How do I run tests under Valgrind?

[Jeff King]

On Mon, Sep 17, 2012 at 10:53:18PM +0530, Ramkumar Ramachandra wrote:

> Hi Peff,
> 
> Jeff King wrote:
> > No, that should work (and it does work here). I assume you can pass
> > t0000 without --valgrind?
> 
> Yes.  Here's the output from running it with --valgrind.  I'm digging
> deeper to see what went wrong.
> 
> make_valgrind_symlink:6: permission denied:
> /home/artagnon/src/git/t/../git-instaweb
> ./test-lib.sh:487: no matches found:
> /home/artagnon/svn/prefix/svn-trunk/bin:/home/artagnon/bin:/home/artagnon/bin/depot_tools:/home/artagnon/.ruby/bin:/home/artagnon/.python/bin:/home/artagnon/.cabal/bin:/home/artagnon/svn/prefix/svn-trunk/bin:/home/artagnon/bin:/home/artagnon/bin/depot_tools:/home/artagnon/.ruby/bin:/home/artagnon/.python/bin:/home/artagnon/.cabal/bin:/home/artagnon/bin:/usr/local/bin:/usr/bin:/bin:/usr/games:/home/artagnon/.ec2/bin:/home/artagnon/.ec2/bin/git-*

That's certainly odd. It sounds like the valgrind setup is broken for
you. Can you run:

  sh -x t0000-basic.sh --valgrind

and see what's happening near those weird errors?

> test_cmp:1: command not found: diff -u

Lack of diff is going to be a problem. What OS is this? Do you really
not have diff? Or is there something funny going on with your PATH?

-Peff

Re: How do I run tests under Valgrind?

[Ramkumar Ramachandra]

Hi again,

Jeff King wrote:
> That's certainly odd. It sounds like the valgrind setup is broken for
> you. Can you run:
>
>   sh -x t0000-basic.sh --valgrind
>
> and see what's happening near those weird errors?

Not helpful:

+ . ./test-lib.sh
+ mkdir -p test-results
+ basename t0000-basic.sh .sh
+ BASE=test-results/t0000-basic
+ GIT_TEST_TEE_STARTED=done /usr/bin/zsh t0000-basic.sh --valgrind
+ tee test-results/t0000-basic.out

>> test_cmp:1: command not found: diff -u
>
> Lack of diff is going to be a problem. What OS is this? Do you really
> not have diff? Or is there something funny going on with your PATH?

It's plain Ubuntu.  Ofcourse I have `diff`- I don't know what's going on.

Ram

Re: How do I run tests under Valgrind?

[Jeff King]

On Mon, Sep 17, 2012 at 11:09:27PM +0530, Ramkumar Ramachandra wrote:

> Hi again,
> 
> Jeff King wrote:
> > That's certainly odd. It sounds like the valgrind setup is broken for
> > you. Can you run:
> >
> >   sh -x t0000-basic.sh --valgrind
> >
> > and see what's happening near those weird errors?
> 
> Not helpful:
> 
> + . ./test-lib.sh
> + mkdir -p test-results
> + basename t0000-basic.sh .sh
> + BASE=test-results/t0000-basic
> + GIT_TEST_TEE_STARTED=done /usr/bin/zsh t0000-basic.sh --valgrind
> + tee test-results/t0000-basic.out

Oh, bleh. Stupid automatic --tee for valgrind. Try this:

  SHELL="/usr/bin/zsh -x" ./t0000-basic.sh --valgrind

I am also doing my tests with "dash" as my shell. You might try setting
your SHELL to /bin/sh to see if it makes a difference.

> >> test_cmp:1: command not found: diff -u
> >
> > Lack of diff is going to be a problem. What OS is this? Do you really
> > not have diff? Or is there something funny going on with your PATH?
> 
> It's plain Ubuntu.  Ofcourse I have `diff`- I don't know what's going on.

Maybe the PATH-munging in test-lib.sh is screwing up your PATH somehow.
But it all looks pretty simple. The "sh -x" output can maybe tell us
more.

-Peff

Re: How do I run tests under Valgrind?

[Johannes Sixt]

Am 17.09.2012 19:44, schrieb Jeff King:
> Oh, bleh. Stupid automatic --tee for valgrind. Try this:
> 
>   SHELL="/usr/bin/zsh -x" ./t0000-basic.sh --valgrind
> 
> I am also doing my tests with "dash" as my shell. You might try setting
> your SHELL to /bin/sh to see if it makes a difference.

Shouldn't -v be used as well? Or is --valgrind different?

-- Hannes

Re: How do I run tests under Valgrind?

[Jeff King]

On Mon, Sep 17, 2012 at 07:55:24PM +0200, Johannes Sixt wrote:

> Am 17.09.2012 19:44, schrieb Jeff King:
> > Oh, bleh. Stupid automatic --tee for valgrind. Try this:
> > 
> >   SHELL="/usr/bin/zsh -x" ./t0000-basic.sh --valgrind
> > 
> > I am also doing my tests with "dash" as my shell. You might try setting
> > your SHELL to /bin/sh to see if it makes a difference.
> 
> Shouldn't -v be used as well? Or is --valgrind different?

It turns "-v" on automatically.

-Peff

Re: How do I run tests under Valgrind?

[Ramkumar Ramachandra]

Hi,

Jeff King wrote:
> Oh, bleh. Stupid automatic --tee for valgrind. Try this:
>
>   SHELL="/usr/bin/zsh -x" ./t0000-basic.sh --valgrind

I got tons of output, but found it unhelpful.  Any other ideas?

Ram

Re: How do I run tests under Valgrind?

[Jeff King]

On Mon, Sep 17, 2012 at 11:19:35PM +0530, Ramkumar Ramachandra wrote:

> +./test-lib.sh:477> file=/home/artagnon/src/git/t/../git-instaweb
> +./test-lib.sh:479> make_valgrind_symlink
> /home/artagnon/src/git/t/../git-instaweb
> +make_valgrind_symlink:5> test -x /home/artagnon/src/git/t/../git-instaweb
> make_valgrind_symlink:6: permission denied:
> /home/artagnon/src/git/t/../git-instaweb
> +make_valgrind_symlink:6> test '#!' '=' ''
> +make_valgrind_symlink:7> return

Weird. The line in question is this:

  test "#!" != "$(head -c 2 < "$symlink_target")"

Is there some problem with running "head" on your system? If it were
head failing to open git-instaweb, it would look more like:

  $ head -c 2 /etc/shadow
  head: cannot open `/etc/shadow' for reading: Permission denied

-Peff

Re: How do I run tests under Valgrind?

[Ramkumar Ramachandra]

Hi Peff,

I was able to reproduce the problem on all my machines, and I consider
this very disturbing.  However, I was successfully able to corner the
issue. I have an overtly long $PATH that's not getting split properly
by `IFS=:` in one corner case -- in other words, this shell script
fails to execute properly when called with `--tee` (just set a really
long $PATH and try):


    case "$GIT_TEST_TEE_STARTED, $* " in
    done,*)
    	# do not redirect again
    	;;    #!/bin/sh

    case "$GIT_TEST_TEE_STARTED, $* " in
    done,*)
    	# do not redirect again
    	;;
    *' --tee '*|*' --va'*)
    	mkdir -p test-results
    	BASE=test-results/$(basename "$0" .sh)
    	(GIT_TEST_TEE_STARTED=done ${SHELL-sh} "$0" "$@" 2>&1;
    	 echo $? > $BASE.exit) | tee $BASE.out
    	test "$(cat $BASE.exit)" = 0
    	exit
    	;;
    esac

    OLDIFS=$IFS
    IFS=:
    for path in $PATH
    do
        ls "$path"/git-* 2> /dev/null |
        while read file
        do
    	echo $file
        done
    done

I'm still trying to figure out what exactly the problem is, and how to patch it.

Thanks.

Ram

Re: How do I run tests under Valgrind?

[Ramkumar Ramachandra]

Hi again,

Ramkumar Ramachandra wrote:
> I was able to reproduce the problem on all my machines, and I consider
> this very disturbing.  However, I was successfully able to corner the
> issue. I have an overtly long $PATH that's not getting split properly
> by `IFS=:` in one corner case -- in other words, this shell script
> fails to execute properly when called with `--tee` (just set a really
> long $PATH and try):

Oops.  Looks like it has nothing to do with an overtly long $PATH.  It
has something to do with $SHELL being zsh though, because other shells
work.  Looking deeper into this.

Ram

Re: How do I run tests under Valgrind?

[Stefano Lattarini]

On 09/21/2012 09:58 PM, Ramkumar Ramachandra wrote:
> Hi again,
> 
> Ramkumar Ramachandra wrote:
>> I was able to reproduce the problem on all my machines, and I consider
>> this very disturbing.  However, I was successfully able to corner the
>> issue. I have an overtly long $PATH that's not getting split properly
>> by `IFS=:` in one corner case -- in other words, this shell script
>> fails to execute properly when called with `--tee` (just set a really
>> long $PATH and try):
> 
> Oops.  Looks like it has nothing to do with an overtly long $PATH.  It
> has something to do with $SHELL being zsh though, because other shells
> work.  Looking deeper into this.
> 
Zsh doesn't do word-splitting by default on variable expansions:

    $ zsh -c 'v="1 2 3"; for x in $v; do echo "$x"; done'
    1 2 3

unless you set the SH_WORD_SPLIT option, or put Zsh in Bourne-compatibility
mode somehow:

    $ zsh -o SH_WORD_SPLIT -c 'v="1 2 3"; for x in $v; do echo "$x"; done'
    1
    2
    3

    $ zsh -c 'emulate sh; v="1 2 3"; for x in $v; do echo "$x"; done'
    1
    2
    3

More info at: <http://zsh.sourceforge.net/FAQ/zshfaq02.html>

HTH,
  Stefano

Re: How do I run tests under Valgrind?

[Ramkumar Ramachandra]

Hi Stefano,

Stefano Lattarini wrote:
> Zsh doesn't do word-splitting by default on variable expansions:
>
>     $ zsh -c 'v="1 2 3"; for x in $v; do echo "$x"; done'
>     1 2 3
>
> unless you set the SH_WORD_SPLIT option, or put Zsh in Bourne-compatibility
> mode somehow:

... but didn't we set $IFS for this purpose?  The following segment of
code works:

    IFS=:
    for path in $PATH
    do
        ls "$path"/git-* 2> /dev/null |
        while read file
        do
    	    echo $file
        done
    done

Ram

Re: How do I run tests under Valgrind?

[Ramkumar Ramachandra]

Hi again,

Ramkumar Ramachandra wrote:
> ... but didn't we set $IFS for this purpose?  The following segment of
> code works:

I'm sorry, it doesn't.  That is the problem.

Ram

Re: How do I run tests under Valgrind?

[Jeff King]

On Fri, Sep 21, 2012 at 10:13:09PM +0200, Stefano Lattarini wrote:

> On 09/21/2012 09:58 PM, Ramkumar Ramachandra wrote:
> > Hi again,
> > 
> > Ramkumar Ramachandra wrote:
> >> I was able to reproduce the problem on all my machines, and I consider
> >> this very disturbing.  However, I was successfully able to corner the
> >> issue. I have an overtly long $PATH that's not getting split properly
> >> by `IFS=:` in one corner case -- in other words, this shell script
> >> fails to execute properly when called with `--tee` (just set a really
> >> long $PATH and try):
> > 
> > Oops.  Looks like it has nothing to do with an overtly long $PATH.  It
> > has something to do with $SHELL being zsh though, because other shells
> > work.  Looking deeper into this.
> > 
> Zsh doesn't do word-splitting by default on variable expansions:
> 
>     $ zsh -c 'v="1 2 3"; for x in $v; do echo "$x"; done'
>     1 2 3
> 
> unless you set the SH_WORD_SPLIT option, or put Zsh in Bourne-compatibility
> mode somehow:

Oh. It sounds like setting $SHELL to zsh is really the problem, then. If
it is not Bourne-compatible when called as "zsh", then it really should
be called in a way that turns on compatibility mode (bash will do this
when called as "sh", but you can also do it with "bash --posix").

-Peff

Re: How do I run tests under Valgrind?

[Stefano Lattarini]

On 09/21/2012 10:49 PM, Jeff King wrote:
>
> Oh. It sounds like setting $SHELL to zsh is really the problem, then. If
> it is not Bourne-compatible when called as "zsh", then it really should
> be called in a way that turns on compatibility mode (bash will do this
> when called as "sh", but you can also do it with "bash --posix").
>
AFAIK, if Zsh is called as "sh", it too will run in Bourne compatibility
mode; not sure how to force this compatibility from the command line though
(albeit I'd guess there is some way to do so).

As further reference, here is the trick autoconf uses to (try to) put Zsh
in Bourne compatibility mode after it has been invoked:

    if test -n "${ZSH_VERSION+set}" && (emulate sh) >/dev/null 2>&1; then
       emulate sh
       NULLCMD=:
       # Pre-4.2 versions of Zsh do word splitting on ${1+"$@"}, which
       # is contrary to our usage.  Disable this feature.
       alias -g '${1+"$@"}'='"$@"'
       setopt NO_GLOB_SUBST
    fi

You might think to use something like that in 't/test-lib.sh' (and other "shell
libraries" of the testsuite), but sadly that would not be enough; this excerpt
from the Automake suite shows why:

   # If Zsh is not started directly in POSIX-compatibility mode, it has some
   # incompatibilities in the handling of $0 that conflict with our usage;
   # i.e., $0 inside a file sourced with the '.' builtin is temporarily set
   # to the name of the sourced file.  Work around that.  The apparently
   # useless 'eval' here is needed by at least dash 0.5.2, to prevent it
   # from bailing out with an error like "Syntax error: Bad substitution".
   # Note that a bug in some versions of Zsh prevents us from resetting $0
   # in a sourced script, so the use of $argv0.  For more info see:
   #  <http://www.zsh.org/mla/workers/2009/msg01140.html>
   eval 'argv0=${functrace[-1]%:*}' && test -f "$argv0" || {
     echo "Cannot determine the path of running test script." >&2
     echo "Your Zsh (version $ZSH_VERSION) is probably too old." >&2
     exit 99
   }

Since I see that '$0' is used in (at least) 't/perf/perf-lib.sh' and
't/test-lib.sh', you'd need to copy the snippet above (or write an
equivalent one) in those files, and change them to use '$argv0' instead
of '$0' in few (but not all) places.  Not sure whether it's worth it
though -- given that you seems to have solved the issue already with a
simpler change, I'd say it's not.

Regards,
  Stefano

Re: How do I run tests under Valgrind?

[Jeff King]

On Sat, Sep 22, 2012 at 03:03:58PM +0200, Stefano Lattarini wrote:

> On 09/21/2012 10:49 PM, Jeff King wrote:
> >
> > Oh. It sounds like setting $SHELL to zsh is really the problem, then. If
> > it is not Bourne-compatible when called as "zsh", then it really should
> > be called in a way that turns on compatibility mode (bash will do this
> > when called as "sh", but you can also do it with "bash --posix").
> >
> AFAIK, if Zsh is called as "sh", it too will run in Bourne compatibility
> mode; not sure how to force this compatibility from the command line though
> (albeit I'd guess there is some way to do so).
> [...]

Thanks for digging. I think this case, though, is that we were simply
using the wrong variable ($SHELL instead of $SHELL_PATH). Your
workarounds would help if somebody put zsh into $SHELL_PATH, but
fundamentally that is not a sane thing to be doing, so I think we can
just consider doing so user error and not bother working around it.

-Peff

Re: How do I run tests under Valgrind?

[Stefano Lattarini]

On 09/22/2012 07:47 PM, Jeff King wrote:
> On Sat, Sep 22, 2012 at 03:03:58PM +0200, Stefano Lattarini wrote:
> 
>> On 09/21/2012 10:49 PM, Jeff King wrote:
>>>
>>> Oh. It sounds like setting $SHELL to zsh is really the problem, then. If
>>> it is not Bourne-compatible when called as "zsh", then it really should
>>> be called in a way that turns on compatibility mode (bash will do this
>>> when called as "sh", but you can also do it with "bash --posix").
>>>
>> AFAIK, if Zsh is called as "sh", it too will run in Bourne compatibility
>> mode; not sure how to force this compatibility from the command line though
>> (albeit I'd guess there is some way to do so).
>> [...]
> 
> Thanks for digging. I think this case, though, is that we were simply
> using the wrong variable ($SHELL instead of $SHELL_PATH). Your
> workarounds would help if somebody put zsh into $SHELL_PATH, but
> fundamentally that is not a sane thing to be doing, so I think we can
> just consider doing so user error and not bother working around it.
> 
FWIW, I agree.

Best regards,
  Stefano

Re: How do I run tests under Valgrind?

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Sat, Sep 22, 2012 at 03:03:58PM +0200, Stefano Lattarini wrote:
>
>> On 09/21/2012 10:49 PM, Jeff King wrote:
>> >
>> > Oh. It sounds like setting $SHELL to zsh is really the problem, then. If
>> > it is not Bourne-compatible when called as "zsh", then it really should
>> > be called in a way that turns on compatibility mode (bash will do this
>> > when called as "sh", but you can also do it with "bash --posix").
>> >
>> AFAIK, if Zsh is called as "sh", it too will run in Bourne compatibility
>> mode; not sure how to force this compatibility from the command line though
>> (albeit I'd guess there is some way to do so).
>> [...]
>
> Thanks for digging. I think this case, though, is that we were simply
> using the wrong variable ($SHELL instead of $SHELL_PATH). Your
> workarounds would help if somebody put zsh into $SHELL_PATH, but
> fundamentally that is not a sane thing to be doing, so I think we can
> just consider doing so user error and not bother working around it.

Yeah, 100% agreed with your assessment, including the part thanking
Stefano.

Thanks.

[PATCH] t/test-lib.sh: do not trust $SHELL

[Ramkumar Ramachandra]

Replace $SHELL with an explicit `/bin/sh`, as some shells do not
support all the features used in the script.  For example, ZSH does
not respect IFS, which is used in line 478.

Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 t/test-lib.sh |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/t/test-lib.sh b/t/test-lib.sh
index f8e3733..5710488 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -24,7 +24,7 @@ done,*)
 *' --tee '*|*' --va'*)
 	mkdir -p test-results
 	BASE=test-results/$(basename "$0" .sh)
-	(GIT_TEST_TEE_STARTED=done ${SHELL-sh} "$0" "$@" 2>&1;
+	(GIT_TEST_TEE_STARTED=done /bin/sh "$0" "$@" 2>&1;
 	 echo $? > $BASE.exit) | tee $BASE.out
 	test "$(cat $BASE.exit)" = 0
 	exit
-- 
1.7.8.1.362.g5d6df.dirty

Re: [PATCH] t/test-lib.sh: do not trust $SHELL

[Jeff King]

On Sat, Sep 22, 2012 at 02:22:46AM +0530, Ramkumar Ramachandra wrote:

> Replace $SHELL with an explicit `/bin/sh`, as some shells do not
> support all the features used in the script.  For example, ZSH does
> not respect IFS, which is used in line 478.

I don't think that is the right thing to do. The point of SHELL is to
point at a bourne-compatible shell. On some systems, the main reason to
set it is that /bin/sh is _broken_, and we are trying to avoid it.

A bigger question is: why are you setting SHELL=zsh in the first place?

-Peff

Re: [PATCH] t/test-lib.sh: do not trust $SHELL

[Ramkumar Ramachandra]

Hi,

Jeff King wrote:
> On Sat, Sep 22, 2012 at 02:22:46AM +0530, Ramkumar Ramachandra wrote:
>
>> Replace $SHELL with an explicit `/bin/sh`, as some shells do not
>> support all the features used in the script.  For example, ZSH does
>> not respect IFS, which is used in line 478.
>
> I don't think that is the right thing to do. The point of SHELL is to
> point at a bourne-compatible shell. On some systems, the main reason to
> set it is that /bin/sh is _broken_, and we are trying to avoid it.

But you're only avoiding it in the --tee/ --va* codepath.  In the
normal codepath, you're stuck with /bin/sh anyway.

> A bigger question is: why are you setting SHELL=zsh in the first place?

I use ZSH as my primary shell, so SHELL is set to zsh when I run
tests.  How can we trust $SHELL to be a bourne-compatible shell?

Ram

Re: [PATCH] t/test-lib.sh: do not trust $SHELL

[Jeff King]

On Sat, Sep 22, 2012 at 02:37:38AM +0530, Ramkumar Ramachandra wrote:

> > I don't think that is the right thing to do. The point of SHELL is to
> > point at a bourne-compatible shell. On some systems, the main reason to
> > set it is that /bin/sh is _broken_, and we are trying to avoid it.
> 
> But you're only avoiding it in the --tee/ --va* codepath.  In the
> normal codepath, you're stuck with /bin/sh anyway.

No, the #!-header is only information. When you run "make test" we
actually invoke the shell ourselves using $SHELL_PATH.

> > A bigger question is: why are you setting SHELL=zsh in the first place?
> 
> I use ZSH as my primary shell, so SHELL is set to zsh when I run
> tests.  How can we trust $SHELL to be a bourne-compatible shell?

Ah, my fault. I was thinking we overrode $SHELL along with $SHELL_PATH,
but we do not.

The correct patch is to stop using $SHELL, but not to switch to a manual
/bin/sh. It should use $SHELL_PATH instead, which is how you tell git
your path to a sane bourne shell.

-Peff

Re: [PATCH] t/test-lib.sh: do not trust $SHELL

[Ramkumar Ramachandra]

Hi Peff,

Jeff King wrote:
> On Sat, Sep 22, 2012 at 02:37:38AM +0530, Ramkumar Ramachandra wrote:
>
>> > I don't think that is the right thing to do. The point of SHELL is to
>> > point at a bourne-compatible shell. On some systems, the main reason to
>> > set it is that /bin/sh is _broken_, and we are trying to avoid it.
>>
>> But you're only avoiding it in the --tee/ --va* codepath.  In the
>> normal codepath, you're stuck with /bin/sh anyway.
>
> No, the #!-header is only information. When you run "make test" we
> actually invoke the shell ourselves using $SHELL_PATH.

My SHELL_PATH is not set, and I can see SHELL_PATH ?= $(SHELL) in the
Makefile.  Which shell is it supposed to point to?  If you're
proposing to use a variable that's only set in the Makefile in the
test, you're not allowing users to run the test as a standalone-
that's not a good change, is it?

Ram

Re: [PATCH] t/test-lib.sh: do not trust $SHELL

[Andreas Schwab]

Ramkumar Ramachandra <artagnon@gmail.com> writes:

> My SHELL_PATH is not set, and I can see SHELL_PATH ?= $(SHELL) in the
> Makefile.  Which shell is it supposed to point to?

Inside a makefile the variable SHELL is special in that it is never
imported from the environment.  If not set it defaults to /bin/sh.

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: [PATCH] t/test-lib.sh: do not trust $SHELL

[Junio C Hamano]

Andreas Schwab <schwab@linux-m68k.org> writes:

> Ramkumar Ramachandra <artagnon@gmail.com> writes:
>
>> My SHELL_PATH is not set, and I can see SHELL_PATH ?= $(SHELL) in the
>> Makefile.  Which shell is it supposed to point to?
>
> Inside a makefile the variable SHELL is special in that it is never
> imported from the environment.  If not set it defaults to /bin/sh.

Ahh, I forgot about that.  Then the current construct is perfectly
fine.

The reference to ${SHELL-/bin/sh} in the test need to be updated to
SHELL_PATH as Peff suggested in the other subthread.

Thanks.

Re: [PATCH] t/test-lib.sh: do not trust $SHELL

[Ramkumar Ramachandra]

Hi Junio,

Junio C Hamano wrote:
> The reference to ${SHELL-/bin/sh} in the test need to be updated to
> SHELL_PATH as Peff suggested in the other subthread.

For that, the entire block needs to be moved down to come after `.
GIT_BUILD_DIR="$TEST_DIRECTORY"/..`.  Is this okay?

diff --git a/t/test-lib.sh b/t/test-lib.sh
index dfa86e4..2284b8b 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -15,22 +15,6 @@
 # You should have received a copy of the GNU General Public License
 # along with this program.  If not, see http://www.gnu.org/licenses/ .

-# if --tee was passed, write the output not only to the terminal, but
-# additionally to the file test-results/$BASENAME.out, too.
-case "$GIT_TEST_TEE_STARTED, $* " in
-done,*)
-	# do not redirect again
-	;;
-*' --tee '*|*' --va'*)
-	mkdir -p test-results
-	BASE=test-results/$(basename "$0" .sh)
-	(GIT_TEST_TEE_STARTED=done ${SHELL-sh} "$0" "$@" 2>&1;
-	 echo $? > $BASE.exit) | tee $BASE.out
-	test "$(cat $BASE.exit)" = 0
-	exit
-	;;
-esac
-
 # Keep the original TERM for say_color
 ORIGINAL_TERM=$TERM

@@ -54,6 +38,22 @@ GIT_BUILD_DIR="$TEST_DIRECTORY"/..
 . "$GIT_BUILD_DIR"/GIT-BUILD-OPTIONS
 export PERL_PATH SHELL_PATH

+# if --tee was passed, write the output not only to the terminal, but
+# additionally to the file test-results/$BASENAME.out, too.
+case "$GIT_TEST_TEE_STARTED, $* " in
+done,*)
+	# do not redirect again
+	;;
+*' --tee '*|*' --va'*)
+	mkdir -p test-results
+	BASE=test-results/$(basename "$0" .sh)
+	(GIT_TEST_TEE_STARTED=done ${SHELL_PATH} "$0" "$@" 2>&1;
+	 echo $? > $BASE.exit) | tee $BASE.out
+	test "$(cat $BASE.exit)" = 0
+	exit
+	;;
+esac
+
 # For repeatability, reset the environment to known value.
 LANG=C
 LC_ALL=C

Re: [PATCH] t/test-lib.sh: do not trust $SHELL

[Junio C Hamano]

Ramkumar Ramachandra <artagnon@gmail.com> writes:

> Junio C Hamano wrote:
>> The reference to ${SHELL-/bin/sh} in the test need to be updated to
>> SHELL_PATH as Peff suggested in the other subthread.
>
> For that, the entire block needs to be moved down to come after `.
> GIT_BUILD_DIR="$TEST_DIRECTORY"/..`.  Is this okay?

Have you tested it with --tee (or valgrind) and does that work?

Re: [PATCH] t/test-lib.sh: do not trust $SHELL

[Ramkumar Ramachandra]

Hi again,

On Sat, Sep 22, 2012 at 10:22 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Ramkumar Ramachandra <artagnon@gmail.com> writes:
>
>> Junio C Hamano wrote:
>>> The reference to ${SHELL-/bin/sh} in the test need to be updated to
>>> SHELL_PATH as Peff suggested in the other subthread.
>>
>> For that, the entire block needs to be moved down to come after `.
>> GIT_BUILD_DIR="$TEST_DIRECTORY"/..`.  Is this okay?
>
> Have you tested it with --tee (or valgrind) and does that work?

Yes, it works.

Ram

Re: [PATCH] t/test-lib.sh: do not trust $SHELL

[Ramkumar Ramachandra]

Here's a patch.

-- 8< --
From: Ramkumar Ramachandra <artagnon@gmail.com>
Date: Sat, 22 Sep 2012 10:25:10 +0530
Subject: [PATCH] test-lib: do not trust $SHELL

Do not trust $SHELL to be a bourne-compatible shell.  Instead, use the
Makefile variable $SHELL_PATH.  This fixes a bug: when a test was run
with --tee and $SHELL was set to ZSH, $PATH on line 479 was not
getting split due to ZSH not respecting $IFS.

Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 t/test-lib.sh | 32 ++++++++++++++++----------------
 1 file changed, 16 insertions(+), 16 deletions(-)

diff --git a/t/test-lib.sh b/t/test-lib.sh
index f8e3733..798bf93 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -15,22 +15,6 @@
 # You should have received a copy of the GNU General Public License
 # along with this program.  If not, see http://www.gnu.org/licenses/ .

-# if --tee was passed, write the output not only to the terminal, but
-# additionally to the file test-results/$BASENAME.out, too.
-case "$GIT_TEST_TEE_STARTED, $* " in
-done,*)
-	# do not redirect again
-	;;
-*' --tee '*|*' --va'*)
-	mkdir -p test-results
-	BASE=test-results/$(basename "$0" .sh)
-	(GIT_TEST_TEE_STARTED=done ${SHELL-sh} "$0" "$@" 2>&1;
-	 echo $? > $BASE.exit) | tee $BASE.out
-	test "$(cat $BASE.exit)" = 0
-	exit
-	;;
-esac
-
 # Keep the original TERM for say_color
 ORIGINAL_TERM=$TERM

@@ -54,6 +38,22 @@ GIT_BUILD_DIR="$TEST_DIRECTORY"/..
 . "$GIT_BUILD_DIR"/GIT-BUILD-OPTIONS
 export PERL_PATH SHELL_PATH

+# if --tee was passed, write the output not only to the terminal, but
+# additionally to the file test-results/$BASENAME.out, too.
+case "$GIT_TEST_TEE_STARTED, $* " in
+done,*)
+	# do not redirect again
+	;;
+*' --tee '*|*' --va'*)
+	mkdir -p test-results
+	BASE=test-results/$(basename "$0" .sh)
+	(GIT_TEST_TEE_STARTED=done ${SHELL_PATH} "$0" "$@" 2>&1;
+	 echo $? > $BASE.exit) | tee $BASE.out
+	test "$(cat $BASE.exit)" = 0
+	exit
+	;;
+esac
+
 # For repeatability, reset the environment to known value.
 LANG=C
 LC_ALL=C
-- 
1.7.12.GIT

Re: [PATCH] t/test-lib.sh: do not trust $SHELL

[Junio C Hamano]

Ramkumar Ramachandra <artagnon@gmail.com> writes:

> Here's a patch.
>
> -- 8< --
> From: Ramkumar Ramachandra <artagnon@gmail.com>
> Date: Sat, 22 Sep 2012 10:25:10 +0530
> Subject: [PATCH] test-lib: do not trust $SHELL
>
> Do not trust $SHELL to be a bourne-compatible shell.  Instead, use the
> Makefile variable $SHELL_PATH.  This fixes a bug: when a test was run
> with --tee and $SHELL was set to ZSH, $PATH on line 479 was not
> getting split due to ZSH not respecting $IFS.
>
> Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
> ---

The part that this starts letting run, which the original "Re-run
the command under tee as early as possible" wanted to avoid running,
does not affect anything that would affect how we run that tee magic
(e.g. "mkdir -p test-results" will still create it directly inside
the directory the test script was started in), so I think this patch
is safe _for now_.

However, it forces people who need to update earlier parts of this
script to be extra careful; it has been true before the patch, and
the patch makes it even more so.

I am not opposed to queuing this as an interim solution, but I
wonder if we can get rid of that double-launch altogether.

Instead of re-launching the script with its output piped to "tee",
can't we do the same by redirecting our standard output to the file
in the file, and spawn a "tail -f" that reads from the file and
outputs to our original output?  Something along the lines of:

        mkdir -p test-results
        tee_base=test-results/$(basename "$0" .sh)

        # empty the file and start "tail -f" on it ...
        : >"$tee_base.out"
        ( tail -f "$tee_base.out" ) &
        tee_pid=$!
	trap 'kill $tee_pid; exit' 0 1 2 3
	# ... and then redirect our output to it
        exec >"$tee_base.out"

and wrap it in a shell helper function that is called from where the
parsing of the command line arguments for "--tee" happens, and don't
forget to kill $tee_pid when we exit.

Hrm?

Re: [PATCH] t/test-lib.sh: do not trust $SHELL

[Ramkumar Ramachandra]

Hi Junio,

Junio C Hamano wrote:
> Ramkumar Ramachandra <artagnon@gmail.com> writes:
>
>> Here's a patch.
>>
>> -- 8< --
>> From: Ramkumar Ramachandra <artagnon@gmail.com>
>> Date: Sat, 22 Sep 2012 10:25:10 +0530
>> Subject: [PATCH] test-lib: do not trust $SHELL
>>
>> Do not trust $SHELL to be a bourne-compatible shell.  Instead, use the
>> Makefile variable $SHELL_PATH.  This fixes a bug: when a test was run
>> with --tee and $SHELL was set to ZSH, $PATH on line 479 was not
>> getting split due to ZSH not respecting $IFS.
>>
>> Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
>> ---
>
> The part that this starts letting run, which the original "Re-run
> the command under tee as early as possible" wanted to avoid running,
> does not affect anything that would affect how we run that tee magic
> (e.g. "mkdir -p test-results" will still create it directly inside
> the directory the test script was started in), so I think this patch
> is safe _for now_.
>
> However, it forces people who need to update earlier parts of this
> script to be extra careful; it has been true before the patch, and
> the patch makes it even more so.
>
> I am not opposed to queuing this as an interim solution, but I
> wonder if we can get rid of that double-launch altogether.

I see you've queued it in `pu` after rewriting the commit message.

> Instead of re-launching the script with its output piped to "tee",
> can't we do the same by redirecting our standard output to the file
> in the file, and spawn a "tail -f" that reads from the file and
> outputs to our original output?  Something along the lines of:
>
>         mkdir -p test-results
>         tee_base=test-results/$(basename "$0" .sh)
>
>         # empty the file and start "tail -f" on it ...
>         : >"$tee_base.out"
>         ( tail -f "$tee_base.out" ) &
>         tee_pid=$!
>         trap 'kill $tee_pid; exit' 0 1 2 3
>         # ... and then redirect our output to it
>         exec >"$tee_base.out"
>
> and wrap it in a shell helper function that is called from where the
> parsing of the command line arguments for "--tee" happens, and don't
> forget to kill $tee_pid when we exit.
>
> Hrm?

Good idea.  I'll write a patch to do this once the interim solution
graduates to `master`.

Ram

Re: [PATCH] t/test-lib.sh: do not trust $SHELL

[Junio C Hamano]

Ramkumar Ramachandra <artagnon@gmail.com> writes:

> Hi Peff,
>
> Jeff King wrote:
>> On Sat, Sep 22, 2012 at 02:37:38AM +0530, Ramkumar Ramachandra wrote:
>>
>>> > I don't think that is the right thing to do. The point of SHELL is to
>>> > point at a bourne-compatible shell. On some systems, the main reason to
>>> > set it is that /bin/sh is _broken_, and we are trying to avoid it.
>>>
>>> But you're only avoiding it in the --tee/ --va* codepath.  In the
>>> normal codepath, you're stuck with /bin/sh anyway.
>>
>> No, the #!-header is only information. When you run "make test" we
>> actually invoke the shell ourselves using $SHELL_PATH.
>
> My SHELL_PATH is not set, and I can see SHELL_PATH ?= $(SHELL) in the
> Makefile.  Which shell is it supposed to point to?

SHELL_PATH is always supposed to point to a Bourne that can be used
to run POSIXy shell scripts.  I think the fallback you pointed out
above assumes that the majority of people who type "make" use Bourne
compatibles as their $SHELL and the default is to help the majority.

It may not hurt to add a note to INSTALL for people who use $SHELL
that is not Bourne (csh and zsh users, but there may be others) that
they need to set SHELL_PATH, of course.

Re: [PATCH] t/test-lib.sh: do not trust $SHELL

[Jeff King]

On Sat, Sep 22, 2012 at 03:04:50AM +0530, Ramkumar Ramachandra wrote:

> > No, the #!-header is only information. When you run "make test" we
> > actually invoke the shell ourselves using $SHELL_PATH.
> 
> My SHELL_PATH is not set, and I can see SHELL_PATH ?= $(SHELL) in the
> Makefile.  Which shell is it supposed to point to? 

Something bourne compatible. If you are on a sane system, /bin/sh is
fine (and is the default).

> If you're proposing to use a variable that's only set in the Makefile
> in the test, you're not allowing users to run the test as a
> standalone- that's not a good change, is it?

It gets written to GIT-BUILD-OPTIONS, which should get pulled in by
test-lib.sh. There may be an ordering problem with the command line
parsing though.

-Peff

Re: [PATCH] t/test-lib.sh: do not trust $SHELL

[Andreas Schwab]

Jeff King <peff@peff.net> writes:

> A bigger question is: why are you setting SHELL=zsh in the first place?

SHELL is set to the login shell by default.

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: [PATCH] t/test-lib.sh: do not trust $SHELL

[Jeff King]

On Fri, Sep 21, 2012 at 11:08:34PM +0200, Andreas Schwab wrote:

> Jeff King <peff@peff.net> writes:
> 
> > A bigger question is: why are you setting SHELL=zsh in the first place?
> 
> SHELL is set to the login shell by default.

Yeah, sorry, I was thinking this was coming from our $SHELL_PATH
Makefile variable, and that he was setting that. The real solution is to
properly use $SHELL_PATH instead of $SHELL.

-Peff