{"thread":{"id":"31569","subject":"How do I run tests under Valgrind?","startedAt":"2012-09-17T17:01:07Z","lastAt":"2012-09-26T03:49:36Z","messageCount":37,"participants":["Ramkumar Ramachandra","Jeff King","Johannes Sixt","Stefano Lattarini","Andreas Schwab","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"199250","messageId":"CALkWK0m_9OsAfG_pF3hUDW+EKCyZCn9NiDKKEW6AEOMmAw=yuA@mail.gmail.com","threadId":"31569","inReplyTo":null,"subject":"How do I run tests under Valgrind?","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2012-09-17T17:01:07Z","receivedAt":"2012-09-17T17:01:07Z","isPatch":false,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Hi,\n\nI tried running `make valgind` inside t/ and got:\n\n    bug in test framework: multiple prerequisite tags do not work reliably\n\nwhich means that even the basic tests don't pass.  Am I doing something wrong?\n\nRam\n"},{"id":"199256","messageId":"20120917172022.GA1179@sigill.intra.peff.net","threadId":"31569","inReplyTo":"CALkWK0m_9OsAfG_pF3hUDW+EKCyZCn9NiDKKEW6AEOMmAw=yuA@mail.gmail.com","subject":"Re: How do I run tests under Valgrind?","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-09-17T17:20:22Z","receivedAt":"2012-09-17T17:20:22Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Sep 17, 2012 at 10:31:07PM +0530, Ramkumar Ramachandra wrote:\n\n> I tried running `make valgind` inside t/ and got:\n> \n>     bug in test framework: multiple prerequisite tags do not work reliably\n> \n> which means that even the basic tests don't pass.  Am I doing something wrong?\n\nNo, that should work (and it does work here). I assume you can pass\nt0000 without --valgrind?\n\nIt seems very odd that valgrind would have an impact here, since those\ntests are not even running git.\n\n-Peff\n"},{"id":"199258","messageId":"CALkWK0m378ApSwa1xiYUqEjMny5m0wt3KacqdDRU1qt=cw6k8g@mail.gmail.com","threadId":"31569","inReplyTo":"20120917172022.GA1179@sigill.intra.peff.net","subject":"Re: How do I run tests under Valgrind?","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2012-09-17T17:23:18Z","receivedAt":"2012-09-17T17:23:18Z","isPatch":false,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Hi Peff,\n\nJeff King wrote:\n> No, that should work (and it does work here). I assume you can pass\n> t0000 without --valgrind?\n\nYes.  Here's the output from running it with --valgrind.  I'm digging\ndeeper to see what went wrong.\n\nmake_valgrind_symlink:6: permission denied:\n/home/artagnon/src/git/t/../git-instaweb\n./test-lib.sh:487: no matches found:\n/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-*\nInitialized empty Git repository in /home/artagnon/src/git/t/trash\ndirectory.test-lib/.git/\nexpecting success:\n        find .git/objects -type f -print >should-be-empty &&\n        test_line_count = 0 should-be-empty\n\nok 1 - .git/objects should be empty after git init in an empty repo\n\nexpecting success:\n        find .git/objects -type d -print >full-of-directories &&\n        test_line_count = 3 full-of-directories\n\nok 2 - .git/objects should have 3 subdirectories\n\nexpecting success:\n        :\n\nok 3 - success is reported like this\n\nchecking known breakage:\n        false\n\nnot ok 4 - pretend we have a known breakage # TODO known breakage\n\nexpecting success:\n        mkdir passing-todo &&\n        (cd passing-todo &&\n        cat >passing-todo.sh <<-EOF &&\n        #!/bin/sh\n\n        test_description='A passing TODO test\n\n        This is run in a sub test-lib so that we do not get incorrect\n        passing metrics\n        '\n\n        # Point to the t/test-lib.sh, which isn't in ../ as usual\n        TEST_DIRECTORY=\"/home/artagnon/src/git/t\"\n        . \"$TEST_DIRECTORY\"/test-lib.sh\n\n        test_expect_failure 'pretend we have fixed a known breakage' '\n                :\n        '\n\n        test_done\n        EOF\n        chmod +x passing-todo.sh &&\n        ./passing-todo.sh >out 2>err &&\n        ! test -s err &&\n        sed -e 's/^> //' >expect <<-\\EOF &&\n        > ok 1 - pretend we have fixed a known breakage # TODO known breakage\n        > # fixed 1 known breakage(s)\n        > # passed all 1 test(s)\n        > 1..1\n        EOF\n        test_cmp expect out)\n\ntest_cmp:1: command not found: diff -u\nnot ok - 5 pretend we have fixed a known breakage (run in sub test-lib)\n#\n#               mkdir passing-todo &&\n#               (cd passing-todo &&\n#               cat >passing-todo.sh <<-EOF &&\n#               #!/bin/sh\n#\n#               test_description='A passing TODO test\n#\n#               This is run in a sub test-lib so that we do not get incorrect\n#               passing metrics\n#               '\n#\n#               # Point to the t/test-lib.sh, which isn't in ../ as usual\n#               TEST_DIRECTORY=\"/home/artagnon/src/git/t\"\n#               . \"$TEST_DIRECTORY\"/test-lib.sh\n#\n#               test_expect_failure 'pretend we have fixed a known breakage' '\n#                       :\n#               '\n#\n#               test_done\n#               EOF\n#               chmod +x passing-todo.sh &&\n#               ./passing-todo.sh >out 2>err &&\n#               ! test -s err &&\n#               sed -e 's/^> //' >expect <<-\\EOF &&\n#               > ok 1 - pretend we have fixed a known breakage # TODO\nknown breakage\n#               > # fixed 1 known breakage(s)\n#               > # passed all 1 test(s)\n#               > 1..1\n#               EOF\n#               test_cmp expect out)\n#\n\nexpecting success:\n        test_have_prereq HAVEIT &&\n        haveit=yes\n\nok 6 - test runs if prerequisite is satisfied\n\nskipping test: unmet prerequisite causes test to be skipped\n        donthaveit=no\n\nok 7 # skip unmet prerequisite causes test to be skipped (missing DONTHAVEIT)\n\nskipping test: test runs if prerequisites are satisfied\n        test_have_prereq HAVEIT &&\n        test_have_prereq HAVETHIS &&\n        haveit=yes\n\nok 8 # skip test runs if prerequisites are satisfied (missing HAVETHIS,HAVEIT)\n\nskipping test: unmet prerequisites causes test to be skipped\n        donthaveit=no\n\nok 9 # skip unmet prerequisites causes test to be skipped (missing\nHAVEIT,DONTHAVEIT)\n\nskipping test: unmet prerequisites causes test to be skipped\n        donthaveiteither=no\n\nok 10 # skip unmet prerequisites causes test to be skipped (missing\nDONTHAVEIT,HAVEIT)\n\nbug in test framework: multiple prerequisite tags do not work reliably\nFATAL: Unexpected exit with code 1\n\n> It seems very odd that valgrind would have an impact here, since those\n> tests are not even running git.\n\nYeah, I know!\n\nRam\n"},{"id":"199259","messageId":"20120917173531.GB1179@sigill.intra.peff.net","threadId":"31569","inReplyTo":"CALkWK0m378ApSwa1xiYUqEjMny5m0wt3KacqdDRU1qt=cw6k8g@mail.gmail.com","subject":"Re: How do I run tests under Valgrind?","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-09-17T17:35:31Z","receivedAt":"2012-09-17T17:35:31Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Sep 17, 2012 at 10:53:18PM +0530, Ramkumar Ramachandra wrote:\n\n> Hi Peff,\n> \n> Jeff King wrote:\n> > No, that should work (and it does work here). I assume you can pass\n> > t0000 without --valgrind?\n> \n> Yes.  Here's the output from running it with --valgrind.  I'm digging\n> deeper to see what went wrong.\n> \n> make_valgrind_symlink:6: permission denied:\n> /home/artagnon/src/git/t/../git-instaweb\n> ./test-lib.sh:487: no matches found:\n> /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-*\n\nThat's certainly odd. It sounds like the valgrind setup is broken for\nyou. Can you run:\n\n  sh -x t0000-basic.sh --valgrind\n\nand see what's happening near those weird errors?\n\n> test_cmp:1: command not found: diff -u\n\nLack of diff is going to be a problem. What OS is this? Do you really\nnot have diff? Or is there something funny going on with your PATH?\n\n-Peff\n"},{"id":"199262","messageId":"CALkWK0kDPGY_3v5SLPtyf+azUwA7msvQOvA+MaCgueZ71i2yGw@mail.gmail.com","threadId":"31569","inReplyTo":"20120917173531.GB1179@sigill.intra.peff.net","subject":"Re: How do I run tests under Valgrind?","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2012-09-17T17:39:27Z","receivedAt":"2012-09-17T17:39:27Z","isPatch":false,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Hi again,\n\nJeff King wrote:\n> That's certainly odd. It sounds like the valgrind setup is broken for\n> you. Can you run:\n>\n>   sh -x t0000-basic.sh --valgrind\n>\n> and see what's happening near those weird errors?\n\nNot helpful:\n\n+ . ./test-lib.sh\n+ mkdir -p test-results\n+ basename t0000-basic.sh .sh\n+ BASE=test-results/t0000-basic\n+ GIT_TEST_TEE_STARTED=done /usr/bin/zsh t0000-basic.sh --valgrind\n+ tee test-results/t0000-basic.out\n\n>> test_cmp:1: command not found: diff -u\n>\n> Lack of diff is going to be a problem. What OS is this? Do you really\n> not have diff? Or is there something funny going on with your PATH?\n\nIt's plain Ubuntu.  Ofcourse I have `diff`- I don't know what's going on.\n\nRam\n"},{"id":"199263","messageId":"20120917174439.GD1179@sigill.intra.peff.net","threadId":"31569","inReplyTo":"CALkWK0kDPGY_3v5SLPtyf+azUwA7msvQOvA+MaCgueZ71i2yGw@mail.gmail.com","subject":"Re: How do I run tests under Valgrind?","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-09-17T17:44:39Z","receivedAt":"2012-09-17T17:44:39Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Sep 17, 2012 at 11:09:27PM +0530, Ramkumar Ramachandra wrote:\n\n> Hi again,\n> \n> Jeff King wrote:\n> > That's certainly odd. It sounds like the valgrind setup is broken for\n> > you. Can you run:\n> >\n> >   sh -x t0000-basic.sh --valgrind\n> >\n> > and see what's happening near those weird errors?\n> \n> Not helpful:\n> \n> + . ./test-lib.sh\n> + mkdir -p test-results\n> + basename t0000-basic.sh .sh\n> + BASE=test-results/t0000-basic\n> + GIT_TEST_TEE_STARTED=done /usr/bin/zsh t0000-basic.sh --valgrind\n> + tee test-results/t0000-basic.out\n\nOh, bleh. Stupid automatic --tee for valgrind. Try this:\n\n  SHELL=\"/usr/bin/zsh -x\" ./t0000-basic.sh --valgrind\n\nI am also doing my tests with \"dash\" as my shell. You might try setting\nyour SHELL to /bin/sh to see if it makes a difference.\n\n> >> test_cmp:1: command not found: diff -u\n> >\n> > Lack of diff is going to be a problem. What OS is this? Do you really\n> > not have diff? Or is there something funny going on with your PATH?\n> \n> It's plain Ubuntu.  Ofcourse I have `diff`- I don't know what's going on.\n\nMaybe the PATH-munging in test-lib.sh is screwing up your PATH somehow.\nBut it all looks pretty simple. The \"sh -x\" output can maybe tell us\nmore.\n\n-Peff\n"},{"id":"199264","messageId":"5057640C.80402@kdbg.org","threadId":"31569","inReplyTo":"20120917174439.GD1179@sigill.intra.peff.net","subject":"Re: How do I run tests under Valgrind?","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2012-09-17T17:55:24Z","receivedAt":"2012-09-17T17:55:24Z","isPatch":false,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 17.09.2012 19:44, schrieb Jeff King:\n> Oh, bleh. Stupid automatic --tee for valgrind. Try this:\n> \n>   SHELL=\"/usr/bin/zsh -x\" ./t0000-basic.sh --valgrind\n> \n> I am also doing my tests with \"dash\" as my shell. You might try setting\n> your SHELL to /bin/sh to see if it makes a difference.\n\nShouldn't -v be used as well? Or is --valgrind different?\n\n-- Hannes\n"},{"id":"199265","messageId":"20120917175726.GE1179@sigill.intra.peff.net","threadId":"31569","inReplyTo":"5057640C.80402@kdbg.org","subject":"Re: How do I run tests under Valgrind?","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-09-17T17:57:26Z","receivedAt":"2012-09-17T17:57:26Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Sep 17, 2012 at 07:55:24PM +0200, Johannes Sixt wrote:\n\n> Am 17.09.2012 19:44, schrieb Jeff King:\n> > Oh, bleh. Stupid automatic --tee for valgrind. Try this:\n> > \n> >   SHELL=\"/usr/bin/zsh -x\" ./t0000-basic.sh --valgrind\n> > \n> > I am also doing my tests with \"dash\" as my shell. You might try setting\n> > your SHELL to /bin/sh to see if it makes a difference.\n> \n> Shouldn't -v be used as well? Or is --valgrind different?\n\nIt turns \"-v\" on automatically.\n\n-Peff\n"},{"id":"199268","messageId":"CALkWK0kjar3q74EJMSY54S0_bWmurWiUuxkRxoYYjNUYm+6RWg@mail.gmail.com","threadId":"31569","inReplyTo":"20120917174439.GD1179@sigill.intra.peff.net","subject":"Re: How do I run tests under Valgrind?","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2012-09-17T18:28:27Z","receivedAt":"2012-09-17T18:28:27Z","isPatch":false,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Hi,\n\nJeff King wrote:\n> Oh, bleh. Stupid automatic --tee for valgrind. Try this:\n>\n>   SHELL=\"/usr/bin/zsh -x\" ./t0000-basic.sh --valgrind\n\nI got tons of output, but found it unhelpful.  Any other ideas?\n\nRam\n"},{"id":"199269","messageId":"20120917182957.GF1179@sigill.intra.peff.net","threadId":"31569","inReplyTo":"CALkWK0mkBbY7dUyaZAqqKE3ZMfE_xU6em_KCOKM9nsTjUP-9pA@mail.gmail.com","subject":"Re: How do I run tests under Valgrind?","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-09-17T18:29:57Z","receivedAt":"2012-09-17T18:29:57Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Sep 17, 2012 at 11:19:35PM +0530, Ramkumar Ramachandra wrote:\n\n> +./test-lib.sh:477> file=/home/artagnon/src/git/t/../git-instaweb\n> +./test-lib.sh:479> make_valgrind_symlink\n> /home/artagnon/src/git/t/../git-instaweb\n> +make_valgrind_symlink:5> test -x /home/artagnon/src/git/t/../git-instaweb\n> make_valgrind_symlink:6: permission denied:\n> /home/artagnon/src/git/t/../git-instaweb\n> +make_valgrind_symlink:6> test '#!' '=' ''\n> +make_valgrind_symlink:7> return\n\nWeird. The line in question is this:\n\n  test \"#!\" != \"$(head -c 2 < \"$symlink_target\")\"\n\nIs there some problem with running \"head\" on your system? If it were\nhead failing to open git-instaweb, it would look more like:\n\n  $ head -c 2 /etc/shadow\n  head: cannot open `/etc/shadow' for reading: Permission denied\n\n-Peff\n"},{"id":"199687","messageId":"CALkWK0=nMxNfmAaBTmXeADQubTayC_2_ut5Nq3QXba9yfQr9yA@mail.gmail.com","threadId":"31569","inReplyTo":"20120917182957.GF1179@sigill.intra.peff.net","subject":"Re: How do I run tests under Valgrind?","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2012-09-21T19:31:32Z","receivedAt":"2012-09-21T19:31:32Z","isPatch":false,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Hi Peff,\n\nI was able to reproduce the problem on all my machines, and I consider\nthis very disturbing.  However, I was successfully able to corner the\nissue. I have an overtly long $PATH that's not getting split properly\nby `IFS=:` in one corner case -- in other words, this shell script\nfails to execute properly when called with `--tee` (just set a really\nlong $PATH and try):\n\n\n    case \"$GIT_TEST_TEE_STARTED, $* \" in\n    done,*)\n    \t# do not redirect again\n    \t;;    #!/bin/sh\n\n    case \"$GIT_TEST_TEE_STARTED, $* \" in\n    done,*)\n    \t# do not redirect again\n    \t;;\n    *' --tee '*|*' --va'*)\n    \tmkdir -p test-results\n    \tBASE=test-results/$(basename \"$0\" .sh)\n    \t(GIT_TEST_TEE_STARTED=done ${SHELL-sh} \"$0\" \"$@\" 2>&1;\n    \t echo $? > $BASE.exit) | tee $BASE.out\n    \ttest \"$(cat $BASE.exit)\" = 0\n    \texit\n    \t;;\n    esac\n\n    OLDIFS=$IFS\n    IFS=:\n    for path in $PATH\n    do\n        ls \"$path\"/git-* 2> /dev/null |\n        while read file\n        do\n    \techo $file\n        done\n    done\n\nI'm still trying to figure out what exactly the problem is, and how to patch it.\n\nThanks.\n\nRam\n"},{"id":"199690","messageId":"CALkWK0kWvrirPjXXzBBBQxKDcwpd_+nQ2eeT6SE8gDFW5T7WFQ@mail.gmail.com","threadId":"31569","inReplyTo":"CALkWK0=nMxNfmAaBTmXeADQubTayC_2_ut5Nq3QXba9yfQr9yA@mail.gmail.com","subject":"Re: How do I run tests under Valgrind?","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2012-09-21T19:58:27Z","receivedAt":"2012-09-21T19:58:27Z","isPatch":false,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Hi again,\n\nRamkumar Ramachandra wrote:\n> I was able to reproduce the problem on all my machines, and I consider\n> this very disturbing.  However, I was successfully able to corner the\n> issue. I have an overtly long $PATH that's not getting split properly\n> by `IFS=:` in one corner case -- in other words, this shell script\n> fails to execute properly when called with `--tee` (just set a really\n> long $PATH and try):\n\nOops.  Looks like it has nothing to do with an overtly long $PATH.  It\nhas something to do with $SHELL being zsh though, because other shells\nwork.  Looking deeper into this.\n\nRam\n"},{"id":"199692","messageId":"505CCA55.6030609@gmail.com","threadId":"31569","inReplyTo":"CALkWK0kWvrirPjXXzBBBQxKDcwpd_+nQ2eeT6SE8gDFW5T7WFQ@mail.gmail.com","subject":"Re: How do I run tests under Valgrind?","fromName":"Stefano Lattarini","fromEmail":"stefano.lattarini@gmail.com","sentAt":"2012-09-21T20:13:09Z","receivedAt":"2012-09-21T20:13:09Z","isPatch":false,"sender":{"key":"stefano.lattarini@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1429199?v=4"},"body":"On 09/21/2012 09:58 PM, Ramkumar Ramachandra wrote:\n> Hi again,\n> \n> Ramkumar Ramachandra wrote:\n>> I was able to reproduce the problem on all my machines, and I consider\n>> this very disturbing.  However, I was successfully able to corner the\n>> issue. I have an overtly long $PATH that's not getting split properly\n>> by `IFS=:` in one corner case -- in other words, this shell script\n>> fails to execute properly when called with `--tee` (just set a really\n>> long $PATH and try):\n> \n> Oops.  Looks like it has nothing to do with an overtly long $PATH.  It\n> has something to do with $SHELL being zsh though, because other shells\n> work.  Looking deeper into this.\n> \nZsh doesn't do word-splitting by default on variable expansions:\n\n    $ zsh -c 'v=\"1 2 3\"; for x in $v; do echo \"$x\"; done'\n    1 2 3\n\nunless you set the SH_WORD_SPLIT option, or put Zsh in Bourne-compatibility\nmode somehow:\n\n    $ zsh -o SH_WORD_SPLIT -c 'v=\"1 2 3\"; for x in $v; do echo \"$x\"; done'\n    1\n    2\n    3\n\n    $ zsh -c 'emulate sh; v=\"1 2 3\"; for x in $v; do echo \"$x\"; done'\n    1\n    2\n    3\n\nMore info at: <http://zsh.sourceforge.net/FAQ/zshfaq02.html>\n\nHTH,\n  Stefano\n"},{"id":"199693","messageId":"CALkWK0mk6m44J8nuqVUvgxWE+RoRtg+uAewyYjwDRFS+fDkNRw@mail.gmail.com","threadId":"31569","inReplyTo":"505CCA55.6030609@gmail.com","subject":"Re: How do I run tests under Valgrind?","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2012-09-21T20:17:46Z","receivedAt":"2012-09-21T20:17:46Z","isPatch":false,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Hi Stefano,\n\nStefano Lattarini wrote:\n> Zsh doesn't do word-splitting by default on variable expansions:\n>\n>     $ zsh -c 'v=\"1 2 3\"; for x in $v; do echo \"$x\"; done'\n>     1 2 3\n>\n> unless you set the SH_WORD_SPLIT option, or put Zsh in Bourne-compatibility\n> mode somehow:\n\n... but didn't we set $IFS for this purpose?  The following segment of\ncode works:\n\n    IFS=:\n    for path in $PATH\n    do\n        ls \"$path\"/git-* 2> /dev/null |\n        while read file\n        do\n    \t    echo $file\n        done\n    done\n\nRam\n"},{"id":"199695","messageId":"CALkWK0=ZCCoNhtvf0VyLGbcssLOKF5FYKf83xYki4fr69b2LFg@mail.gmail.com","threadId":"31569","inReplyTo":"CALkWK0mk6m44J8nuqVUvgxWE+RoRtg+uAewyYjwDRFS+fDkNRw@mail.gmail.com","subject":"Re: How do I run tests under Valgrind?","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2012-09-21T20:46:07Z","receivedAt":"2012-09-21T20:46:07Z","isPatch":false,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Hi again,\n\nRamkumar Ramachandra wrote:\n> ... but didn't we set $IFS for this purpose?  The following segment of\n> code works:\n\nI'm sorry, it doesn't.  That is the problem.\n\nRam\n"},{"id":"199696","messageId":"20120921204907.GA22977@sigill.intra.peff.net","threadId":"31569","inReplyTo":"505CCA55.6030609@gmail.com","subject":"Re: How do I run tests under Valgrind?","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-09-21T20:49:07Z","receivedAt":"2012-09-21T20:49:07Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Sep 21, 2012 at 10:13:09PM +0200, Stefano Lattarini wrote:\n\n> On 09/21/2012 09:58 PM, Ramkumar Ramachandra wrote:\n> > Hi again,\n> > \n> > Ramkumar Ramachandra wrote:\n> >> I was able to reproduce the problem on all my machines, and I consider\n> >> this very disturbing.  However, I was successfully able to corner the\n> >> issue. I have an overtly long $PATH that's not getting split properly\n> >> by `IFS=:` in one corner case -- in other words, this shell script\n> >> fails to execute properly when called with `--tee` (just set a really\n> >> long $PATH and try):\n> > \n> > Oops.  Looks like it has nothing to do with an overtly long $PATH.  It\n> > has something to do with $SHELL being zsh though, because other shells\n> > work.  Looking deeper into this.\n> > \n> Zsh doesn't do word-splitting by default on variable expansions:\n> \n>     $ zsh -c 'v=\"1 2 3\"; for x in $v; do echo \"$x\"; done'\n>     1 2 3\n> \n> unless you set the SH_WORD_SPLIT option, or put Zsh in Bourne-compatibility\n> mode somehow:\n\nOh. It sounds like setting $SHELL to zsh is really the problem, then. If\nit is not Bourne-compatible when called as \"zsh\", then it really should\nbe called in a way that turns on compatibility mode (bash will do this\nwhen called as \"sh\", but you can also do it with \"bash --posix\").\n\n-Peff\n"},{"id":"199698","messageId":"1348260766-25287-1-git-send-email-artagnon@gmail.com","threadId":"31569","inReplyTo":"505CCA55.6030609@gmail.com","subject":"[PATCH] t/test-lib.sh: do not trust $SHELL","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2012-09-21T20:52:46Z","receivedAt":"2012-09-21T20:52:46Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Replace $SHELL with an explicit `/bin/sh`, as some shells do not\nsupport all the features used in the script.  For example, ZSH does\nnot respect IFS, which is used in line 478.\n\nSigned-off-by: Ramkumar Ramachandra <artagnon@gmail.com>\n---\n t/test-lib.sh |    2 +-\n 1 files changed, 1 insertions(+), 1 deletions(-)\n\ndiff --git a/t/test-lib.sh b/t/test-lib.sh\nindex f8e3733..5710488 100644\n--- a/t/test-lib.sh\n+++ b/t/test-lib.sh\n@@ -24,7 +24,7 @@ done,*)\n *' --tee '*|*' --va'*)\n \tmkdir -p test-results\n \tBASE=test-results/$(basename \"$0\" .sh)\n-\t(GIT_TEST_TEE_STARTED=done ${SHELL-sh} \"$0\" \"$@\" 2>&1;\n+\t(GIT_TEST_TEE_STARTED=done /bin/sh \"$0\" \"$@\" 2>&1;\n \t echo $? > $BASE.exit) | tee $BASE.out\n \ttest \"$(cat $BASE.exit)\" = 0\n \texit\n-- \n1.7.8.1.362.g5d6df.dirty\n"},{"id":"199699","messageId":"20120921205834.GC22977@sigill.intra.peff.net","threadId":"31569","inReplyTo":"1348260766-25287-1-git-send-email-artagnon@gmail.com","subject":"Re: [PATCH] t/test-lib.sh: do not trust $SHELL","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-09-21T20:58:34Z","receivedAt":"2012-09-21T20:58:34Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Sep 22, 2012 at 02:22:46AM +0530, Ramkumar Ramachandra wrote:\n\n> Replace $SHELL with an explicit `/bin/sh`, as some shells do not\n> support all the features used in the script.  For example, ZSH does\n> not respect IFS, which is used in line 478.\n\nI don't think that is the right thing to do. The point of SHELL is to\npoint at a bourne-compatible shell. On some systems, the main reason to\nset it is that /bin/sh is _broken_, and we are trying to avoid it.\n\nA bigger question is: why are you setting SHELL=zsh in the first place?\n\n-Peff\n"},{"id":"199701","messageId":"CALkWK0kRzN_yQZ1JqJogBs6Z1nLhofBijHzeWR5YfQYHOtpaBA@mail.gmail.com","threadId":"31569","inReplyTo":"20120921205834.GC22977@sigill.intra.peff.net","subject":"Re: [PATCH] t/test-lib.sh: do not trust $SHELL","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2012-09-21T21:07:38Z","receivedAt":"2012-09-21T21:07:38Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Hi,\n\nJeff King wrote:\n> On Sat, Sep 22, 2012 at 02:22:46AM +0530, Ramkumar Ramachandra wrote:\n>\n>> Replace $SHELL with an explicit `/bin/sh`, as some shells do not\n>> support all the features used in the script.  For example, ZSH does\n>> not respect IFS, which is used in line 478.\n>\n> I don't think that is the right thing to do. The point of SHELL is to\n> point at a bourne-compatible shell. On some systems, the main reason to\n> set it is that /bin/sh is _broken_, and we are trying to avoid it.\n\nBut you're only avoiding it in the --tee/ --va* codepath.  In the\nnormal codepath, you're stuck with /bin/sh anyway.\n\n> A bigger question is: why are you setting SHELL=zsh in the first place?\n\nI use ZSH as my primary shell, so SHELL is set to zsh when I run\ntests.  How can we trust $SHELL to be a bourne-compatible shell?\n\nRam\n"},{"id":"199702","messageId":"m2obkz9j8t.fsf@igel.home","threadId":"31569","inReplyTo":"20120921205834.GC22977@sigill.intra.peff.net","subject":"Re: [PATCH] t/test-lib.sh: do not trust $SHELL","fromName":"Andreas Schwab","fromEmail":"schwab@linux-m68k.org","sentAt":"2012-09-21T21:08:34Z","receivedAt":"2012-09-21T21:08:34Z","isPatch":true,"sender":{"key":"schwab@linux-m68k.org","avatar":"https://avatars.githubusercontent.com/u/2175493?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> A bigger question is: why are you setting SHELL=zsh in the first place?\n\nSHELL is set to the login shell by default.\n\nAndreas.\n\n-- \nAndreas Schwab, schwab@linux-m68k.org\nGPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5\n\"And now for something completely different.\"\n"},{"id":"199704","messageId":"20120921211217.GA24134@sigill.intra.peff.net","threadId":"31569","inReplyTo":"CALkWK0kRzN_yQZ1JqJogBs6Z1nLhofBijHzeWR5YfQYHOtpaBA@mail.gmail.com","subject":"Re: [PATCH] t/test-lib.sh: do not trust $SHELL","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-09-21T21:12:17Z","receivedAt":"2012-09-21T21:12:17Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Sep 22, 2012 at 02:37:38AM +0530, Ramkumar Ramachandra wrote:\n\n> > I don't think that is the right thing to do. The point of SHELL is to\n> > point at a bourne-compatible shell. On some systems, the main reason to\n> > set it is that /bin/sh is _broken_, and we are trying to avoid it.\n> \n> But you're only avoiding it in the --tee/ --va* codepath.  In the\n> normal codepath, you're stuck with /bin/sh anyway.\n\nNo, the #!-header is only information. When you run \"make test\" we\nactually invoke the shell ourselves using $SHELL_PATH.\n\n> > A bigger question is: why are you setting SHELL=zsh in the first place?\n> \n> I use ZSH as my primary shell, so SHELL is set to zsh when I run\n> tests.  How can we trust $SHELL to be a bourne-compatible shell?\n\nAh, my fault. I was thinking we overrode $SHELL along with $SHELL_PATH,\nbut we do not.\n\nThe correct patch is to stop using $SHELL, but not to switch to a manual\n/bin/sh. It should use $SHELL_PATH instead, which is how you tell git\nyour path to a sane bourne shell.\n\n-Peff\n"},{"id":"199705","messageId":"20120921211306.GB24134@sigill.intra.peff.net","threadId":"31569","inReplyTo":"m2obkz9j8t.fsf@igel.home","subject":"Re: [PATCH] t/test-lib.sh: do not trust $SHELL","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-09-21T21:13:06Z","receivedAt":"2012-09-21T21:13:06Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Sep 21, 2012 at 11:08:34PM +0200, Andreas Schwab wrote:\n\n> Jeff King <peff@peff.net> writes:\n> \n> > A bigger question is: why are you setting SHELL=zsh in the first place?\n> \n> SHELL is set to the login shell by default.\n\nYeah, sorry, I was thinking this was coming from our $SHELL_PATH\nMakefile variable, and that he was setting that. The real solution is to\nproperly use $SHELL_PATH instead of $SHELL.\n\n-Peff\n"},{"id":"199706","messageId":"CALkWK0nLLEF7wnUhF0JUAZVP6GG3KHmuYSDZLPS7uGCZPfhV3w@mail.gmail.com","threadId":"31569","inReplyTo":"20120921211217.GA24134@sigill.intra.peff.net","subject":"Re: [PATCH] t/test-lib.sh: do not trust $SHELL","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2012-09-21T21:34:50Z","receivedAt":"2012-09-21T21:34:50Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Hi Peff,\n\nJeff King wrote:\n> On Sat, Sep 22, 2012 at 02:37:38AM +0530, Ramkumar Ramachandra wrote:\n>\n>> > I don't think that is the right thing to do. The point of SHELL is to\n>> > point at a bourne-compatible shell. On some systems, the main reason to\n>> > set it is that /bin/sh is _broken_, and we are trying to avoid it.\n>>\n>> But you're only avoiding it in the --tee/ --va* codepath.  In the\n>> normal codepath, you're stuck with /bin/sh anyway.\n>\n> No, the #!-header is only information. When you run \"make test\" we\n> actually invoke the shell ourselves using $SHELL_PATH.\n\nMy SHELL_PATH is not set, and I can see SHELL_PATH ?= $(SHELL) in the\nMakefile.  Which shell is it supposed to point to?  If you're\nproposing to use a variable that's only set in the Makefile in the\ntest, you're not allowing users to run the test as a standalone-\nthat's not a good change, is it?\n\nRam\n"},{"id":"199708","messageId":"m2k3vn9gyu.fsf@igel.home","threadId":"31569","inReplyTo":"CALkWK0nLLEF7wnUhF0JUAZVP6GG3KHmuYSDZLPS7uGCZPfhV3w@mail.gmail.com","subject":"Re: [PATCH] t/test-lib.sh: do not trust $SHELL","fromName":"Andreas Schwab","fromEmail":"schwab@linux-m68k.org","sentAt":"2012-09-21T21:57:45Z","receivedAt":"2012-09-21T21:57:45Z","isPatch":true,"sender":{"key":"schwab@linux-m68k.org","avatar":"https://avatars.githubusercontent.com/u/2175493?v=4"},"body":"Ramkumar Ramachandra <artagnon@gmail.com> writes:\n\n> My SHELL_PATH is not set, and I can see SHELL_PATH ?= $(SHELL) in the\n> Makefile.  Which shell is it supposed to point to?\n\nInside a makefile the variable SHELL is special in that it is never\nimported from the environment.  If not set it defaults to /bin/sh.\n\nAndreas.\n\n-- \nAndreas Schwab, schwab@linux-m68k.org\nGPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5\n\"And now for something completely different.\"\n"},{"id":"199709","messageId":"7vlig33umv.fsf@alter.siamese.dyndns.org","threadId":"31569","inReplyTo":"CALkWK0nLLEF7wnUhF0JUAZVP6GG3KHmuYSDZLPS7uGCZPfhV3w@mail.gmail.com","subject":"Re: [PATCH] t/test-lib.sh: do not trust $SHELL","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-09-21T21:59:04Z","receivedAt":"2012-09-21T21:59:04Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ramkumar Ramachandra <artagnon@gmail.com> writes:\n\n> Hi Peff,\n>\n> Jeff King wrote:\n>> On Sat, Sep 22, 2012 at 02:37:38AM +0530, Ramkumar Ramachandra wrote:\n>>\n>>> > I don't think that is the right thing to do. The point of SHELL is to\n>>> > point at a bourne-compatible shell. On some systems, the main reason to\n>>> > set it is that /bin/sh is _broken_, and we are trying to avoid it.\n>>>\n>>> But you're only avoiding it in the --tee/ --va* codepath.  In the\n>>> normal codepath, you're stuck with /bin/sh anyway.\n>>\n>> No, the #!-header is only information. When you run \"make test\" we\n>> actually invoke the shell ourselves using $SHELL_PATH.\n>\n> My SHELL_PATH is not set, and I can see SHELL_PATH ?= $(SHELL) in the\n> Makefile.  Which shell is it supposed to point to?\n\nSHELL_PATH is always supposed to point to a Bourne that can be used\nto run POSIXy shell scripts.  I think the fallback you pointed out\nabove assumes that the majority of people who type \"make\" use Bourne\ncompatibles as their $SHELL and the default is to help the majority.\n\nIt may not hurt to add a note to INSTALL for people who use $SHELL\nthat is not Bourne (csh and zsh users, but there may be others) that\nthey need to set SHELL_PATH, of course.\n"},{"id":"199710","messageId":"20120921221543.GA29100@sigill.intra.peff.net","threadId":"31569","inReplyTo":"CALkWK0nLLEF7wnUhF0JUAZVP6GG3KHmuYSDZLPS7uGCZPfhV3w@mail.gmail.com","subject":"Re: [PATCH] t/test-lib.sh: do not trust $SHELL","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-09-21T22:15:43Z","receivedAt":"2012-09-21T22:15:43Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Sep 22, 2012 at 03:04:50AM +0530, Ramkumar Ramachandra wrote:\n\n> > No, the #!-header is only information. When you run \"make test\" we\n> > actually invoke the shell ourselves using $SHELL_PATH.\n> \n> My SHELL_PATH is not set, and I can see SHELL_PATH ?= $(SHELL) in the\n> Makefile.  Which shell is it supposed to point to? \n\nSomething bourne compatible. If you are on a sane system, /bin/sh is\nfine (and is the default).\n\n> If you're proposing to use a variable that's only set in the Makefile\n> in the test, you're not allowing users to run the test as a\n> standalone- that's not a good change, is it?\n\nIt gets written to GIT-BUILD-OPTIONS, which should get pulled in by\ntest-lib.sh. There may be an ordering problem with the command line\nparsing though.\n\n-Peff\n"},{"id":"199711","messageId":"7vhaqr3t6j.fsf@alter.siamese.dyndns.org","threadId":"31569","inReplyTo":"m2k3vn9gyu.fsf@igel.home","subject":"Re: [PATCH] t/test-lib.sh: do not trust $SHELL","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-09-21T22:30:28Z","receivedAt":"2012-09-21T22:30:28Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Andreas Schwab <schwab@linux-m68k.org> writes:\n\n> Ramkumar Ramachandra <artagnon@gmail.com> writes:\n>\n>> My SHELL_PATH is not set, and I can see SHELL_PATH ?= $(SHELL) in the\n>> Makefile.  Which shell is it supposed to point to?\n>\n> Inside a makefile the variable SHELL is special in that it is never\n> imported from the environment.  If not set it defaults to /bin/sh.\n\nAhh, I forgot about that.  Then the current construct is perfectly\nfine.\n\nThe reference to ${SHELL-/bin/sh} in the test need to be updated to\nSHELL_PATH as Peff suggested in the other subthread.\n\nThanks.\n"},{"id":"199722","messageId":"CALkWK0kxmD_HJtnV0ShSdpgSjoLQHtv0=b4s=utG52zFLb2Bjg@mail.gmail.com","threadId":"31569","inReplyTo":"7vhaqr3t6j.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] t/test-lib.sh: do not trust $SHELL","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2012-09-22T04:26:04Z","receivedAt":"2012-09-22T04:26:04Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Hi Junio,\n\nJunio C Hamano wrote:\n> The reference to ${SHELL-/bin/sh} in the test need to be updated to\n> SHELL_PATH as Peff suggested in the other subthread.\n\nFor that, the entire block needs to be moved down to come after `.\nGIT_BUILD_DIR=\"$TEST_DIRECTORY\"/..`.  Is this okay?\n\ndiff --git a/t/test-lib.sh b/t/test-lib.sh\nindex dfa86e4..2284b8b 100644\n--- a/t/test-lib.sh\n+++ b/t/test-lib.sh\n@@ -15,22 +15,6 @@\n # You should have received a copy of the GNU General Public License\n # along with this program.  If not, see http://www.gnu.org/licenses/ .\n\n-# if --tee was passed, write the output not only to the terminal, but\n-# additionally to the file test-results/$BASENAME.out, too.\n-case \"$GIT_TEST_TEE_STARTED, $* \" in\n-done,*)\n-\t# do not redirect again\n-\t;;\n-*' --tee '*|*' --va'*)\n-\tmkdir -p test-results\n-\tBASE=test-results/$(basename \"$0\" .sh)\n-\t(GIT_TEST_TEE_STARTED=done ${SHELL-sh} \"$0\" \"$@\" 2>&1;\n-\t echo $? > $BASE.exit) | tee $BASE.out\n-\ttest \"$(cat $BASE.exit)\" = 0\n-\texit\n-\t;;\n-esac\n-\n # Keep the original TERM for say_color\n ORIGINAL_TERM=$TERM\n\n@@ -54,6 +38,22 @@ GIT_BUILD_DIR=\"$TEST_DIRECTORY\"/..\n . \"$GIT_BUILD_DIR\"/GIT-BUILD-OPTIONS\n export PERL_PATH SHELL_PATH\n\n+# if --tee was passed, write the output not only to the terminal, but\n+# additionally to the file test-results/$BASENAME.out, too.\n+case \"$GIT_TEST_TEE_STARTED, $* \" in\n+done,*)\n+\t# do not redirect again\n+\t;;\n+*' --tee '*|*' --va'*)\n+\tmkdir -p test-results\n+\tBASE=test-results/$(basename \"$0\" .sh)\n+\t(GIT_TEST_TEE_STARTED=done ${SHELL_PATH} \"$0\" \"$@\" 2>&1;\n+\t echo $? > $BASE.exit) | tee $BASE.out\n+\ttest \"$(cat $BASE.exit)\" = 0\n+\texit\n+\t;;\n+esac\n+\n # For repeatability, reset the environment to known value.\n LANG=C\n LC_ALL=C\n"},{"id":"199725","messageId":"7vwqzm3bhf.fsf@alter.siamese.dyndns.org","threadId":"31569","inReplyTo":"CALkWK0kxmD_HJtnV0ShSdpgSjoLQHtv0=b4s=utG52zFLb2Bjg@mail.gmail.com","subject":"Re: [PATCH] t/test-lib.sh: do not trust $SHELL","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-09-22T04:52:44Z","receivedAt":"2012-09-22T04:52:44Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ramkumar Ramachandra <artagnon@gmail.com> writes:\n\n> Junio C Hamano wrote:\n>> The reference to ${SHELL-/bin/sh} in the test need to be updated to\n>> SHELL_PATH as Peff suggested in the other subthread.\n>\n> For that, the entire block needs to be moved down to come after `.\n> GIT_BUILD_DIR=\"$TEST_DIRECTORY\"/..`.  Is this okay?\n\nHave you tested it with --tee (or valgrind) and does that work?\n"},{"id":"199726","messageId":"CALkWK0nTWQ3g75fN5wQLwgeW986vAzGdXajSGCY5SdpK8W=0aA@mail.gmail.com","threadId":"31569","inReplyTo":"7vwqzm3bhf.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] t/test-lib.sh: do not trust $SHELL","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2012-09-22T04:54:15Z","receivedAt":"2012-09-22T04:54:15Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Hi again,\n\nOn Sat, Sep 22, 2012 at 10:22 AM, Junio C Hamano <gitster@pobox.com> wrote:\n> Ramkumar Ramachandra <artagnon@gmail.com> writes:\n>\n>> Junio C Hamano wrote:\n>>> The reference to ${SHELL-/bin/sh} in the test need to be updated to\n>>> SHELL_PATH as Peff suggested in the other subthread.\n>>\n>> For that, the entire block needs to be moved down to come after `.\n>> GIT_BUILD_DIR=\"$TEST_DIRECTORY\"/..`.  Is this okay?\n>\n> Have you tested it with --tee (or valgrind) and does that work?\n\nYes, it works.\n\nRam\n"},{"id":"199727","messageId":"CALkWK0kWJ3ndEMGzyGGMEa+ko3BTShC7_o-HVsXzFCGwRem5Jw@mail.gmail.com","threadId":"31569","inReplyTo":"CALkWK0nTWQ3g75fN5wQLwgeW986vAzGdXajSGCY5SdpK8W=0aA@mail.gmail.com","subject":"Re: [PATCH] t/test-lib.sh: do not trust $SHELL","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2012-09-22T04:57:01Z","receivedAt":"2012-09-22T04:57:01Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Here's a patch.\n\n-- 8< --\nFrom: Ramkumar Ramachandra <artagnon@gmail.com>\nDate: Sat, 22 Sep 2012 10:25:10 +0530\nSubject: [PATCH] test-lib: do not trust $SHELL\n\nDo not trust $SHELL to be a bourne-compatible shell.  Instead, use the\nMakefile variable $SHELL_PATH.  This fixes a bug: when a test was run\nwith --tee and $SHELL was set to ZSH, $PATH on line 479 was not\ngetting split due to ZSH not respecting $IFS.\n\nSigned-off-by: Ramkumar Ramachandra <artagnon@gmail.com>\n---\n t/test-lib.sh | 32 ++++++++++++++++----------------\n 1 file changed, 16 insertions(+), 16 deletions(-)\n\ndiff --git a/t/test-lib.sh b/t/test-lib.sh\nindex f8e3733..798bf93 100644\n--- a/t/test-lib.sh\n+++ b/t/test-lib.sh\n@@ -15,22 +15,6 @@\n # You should have received a copy of the GNU General Public License\n # along with this program.  If not, see http://www.gnu.org/licenses/ .\n\n-# if --tee was passed, write the output not only to the terminal, but\n-# additionally to the file test-results/$BASENAME.out, too.\n-case \"$GIT_TEST_TEE_STARTED, $* \" in\n-done,*)\n-\t# do not redirect again\n-\t;;\n-*' --tee '*|*' --va'*)\n-\tmkdir -p test-results\n-\tBASE=test-results/$(basename \"$0\" .sh)\n-\t(GIT_TEST_TEE_STARTED=done ${SHELL-sh} \"$0\" \"$@\" 2>&1;\n-\t echo $? > $BASE.exit) | tee $BASE.out\n-\ttest \"$(cat $BASE.exit)\" = 0\n-\texit\n-\t;;\n-esac\n-\n # Keep the original TERM for say_color\n ORIGINAL_TERM=$TERM\n\n@@ -54,6 +38,22 @@ GIT_BUILD_DIR=\"$TEST_DIRECTORY\"/..\n . \"$GIT_BUILD_DIR\"/GIT-BUILD-OPTIONS\n export PERL_PATH SHELL_PATH\n\n+# if --tee was passed, write the output not only to the terminal, but\n+# additionally to the file test-results/$BASENAME.out, too.\n+case \"$GIT_TEST_TEE_STARTED, $* \" in\n+done,*)\n+\t# do not redirect again\n+\t;;\n+*' --tee '*|*' --va'*)\n+\tmkdir -p test-results\n+\tBASE=test-results/$(basename \"$0\" .sh)\n+\t(GIT_TEST_TEE_STARTED=done ${SHELL_PATH} \"$0\" \"$@\" 2>&1;\n+\t echo $? > $BASE.exit) | tee $BASE.out\n+\ttest \"$(cat $BASE.exit)\" = 0\n+\texit\n+\t;;\n+esac\n+\n # For repeatability, reset the environment to known value.\n LANG=C\n LC_ALL=C\n-- \n1.7.12.GIT\n"},{"id":"199738","messageId":"505DB73E.2020108@gmail.com","threadId":"31569","inReplyTo":"20120921204907.GA22977@sigill.intra.peff.net","subject":"Re: How do I run tests under Valgrind?","fromName":"Stefano Lattarini","fromEmail":"stefano.lattarini@gmail.com","sentAt":"2012-09-22T13:03:58Z","receivedAt":"2012-09-22T13:03:58Z","isPatch":false,"sender":{"key":"stefano.lattarini@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1429199?v=4"},"body":"On 09/21/2012 10:49 PM, Jeff King wrote:\n>\n> Oh. It sounds like setting $SHELL to zsh is really the problem, then. If\n> it is not Bourne-compatible when called as \"zsh\", then it really should\n> be called in a way that turns on compatibility mode (bash will do this\n> when called as \"sh\", but you can also do it with \"bash --posix\").\n>\nAFAIK, if Zsh is called as \"sh\", it too will run in Bourne compatibility\nmode; not sure how to force this compatibility from the command line though\n(albeit I'd guess there is some way to do so).\n\nAs further reference, here is the trick autoconf uses to (try to) put Zsh\nin Bourne compatibility mode after it has been invoked:\n\n    if test -n \"${ZSH_VERSION+set}\" && (emulate sh) >/dev/null 2>&1; then\n       emulate sh\n       NULLCMD=:\n       # Pre-4.2 versions of Zsh do word splitting on ${1+\"$@\"}, which\n       # is contrary to our usage.  Disable this feature.\n       alias -g '${1+\"$@\"}'='\"$@\"'\n       setopt NO_GLOB_SUBST\n    fi\n\nYou might think to use something like that in 't/test-lib.sh' (and other \"shell\nlibraries\" of the testsuite), but sadly that would not be enough; this excerpt\nfrom the Automake suite shows why:\n\n   # If Zsh is not started directly in POSIX-compatibility mode, it has some\n   # incompatibilities in the handling of $0 that conflict with our usage;\n   # i.e., $0 inside a file sourced with the '.' builtin is temporarily set\n   # to the name of the sourced file.  Work around that.  The apparently\n   # useless 'eval' here is needed by at least dash 0.5.2, to prevent it\n   # from bailing out with an error like \"Syntax error: Bad substitution\".\n   # Note that a bug in some versions of Zsh prevents us from resetting $0\n   # in a sourced script, so the use of $argv0.  For more info see:\n   #  <http://www.zsh.org/mla/workers/2009/msg01140.html>\n   eval 'argv0=${functrace[-1]%:*}' && test -f \"$argv0\" || {\n     echo \"Cannot determine the path of running test script.\" >&2\n     echo \"Your Zsh (version $ZSH_VERSION) is probably too old.\" >&2\n     exit 99\n   }\n\nSince I see that '$0' is used in (at least) 't/perf/perf-lib.sh' and\n't/test-lib.sh', you'd need to copy the snippet above (or write an\nequivalent one) in those files, and change them to use '$argv0' instead\nof '$0' in few (but not all) places.  Not sure whether it's worth it\nthough -- given that you seems to have solved the issue already with a\nsimpler change, I'd say it's not.\n\nRegards,\n  Stefano\n"},{"id":"199741","messageId":"20120922174741.GA6722@sigill.intra.peff.net","threadId":"31569","inReplyTo":"505DB73E.2020108@gmail.com","subject":"Re: How do I run tests under Valgrind?","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-09-22T17:47:42Z","receivedAt":"2012-09-22T17:47:42Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Sep 22, 2012 at 03:03:58PM +0200, Stefano Lattarini wrote:\n\n> On 09/21/2012 10:49 PM, Jeff King wrote:\n> >\n> > Oh. It sounds like setting $SHELL to zsh is really the problem, then. If\n> > it is not Bourne-compatible when called as \"zsh\", then it really should\n> > be called in a way that turns on compatibility mode (bash will do this\n> > when called as \"sh\", but you can also do it with \"bash --posix\").\n> >\n> AFAIK, if Zsh is called as \"sh\", it too will run in Bourne compatibility\n> mode; not sure how to force this compatibility from the command line though\n> (albeit I'd guess there is some way to do so).\n> [...]\n\nThanks for digging. I think this case, though, is that we were simply\nusing the wrong variable ($SHELL instead of $SHELL_PATH). Your\nworkarounds would help if somebody put zsh into $SHELL_PATH, but\nfundamentally that is not a sane thing to be doing, so I think we can\njust consider doing so user error and not bother working around it.\n\n-Peff\n"},{"id":"199743","messageId":"505E0179.1070106@gmail.com","threadId":"31569","inReplyTo":"20120922174741.GA6722@sigill.intra.peff.net","subject":"Re: How do I run tests under Valgrind?","fromName":"Stefano Lattarini","fromEmail":"stefano.lattarini@gmail.com","sentAt":"2012-09-22T18:20:41Z","receivedAt":"2012-09-22T18:20:41Z","isPatch":false,"sender":{"key":"stefano.lattarini@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1429199?v=4"},"body":"On 09/22/2012 07:47 PM, Jeff King wrote:\n> On Sat, Sep 22, 2012 at 03:03:58PM +0200, Stefano Lattarini wrote:\n> \n>> On 09/21/2012 10:49 PM, Jeff King wrote:\n>>>\n>>> Oh. It sounds like setting $SHELL to zsh is really the problem, then. If\n>>> it is not Bourne-compatible when called as \"zsh\", then it really should\n>>> be called in a way that turns on compatibility mode (bash will do this\n>>> when called as \"sh\", but you can also do it with \"bash --posix\").\n>>>\n>> AFAIK, if Zsh is called as \"sh\", it too will run in Bourne compatibility\n>> mode; not sure how to force this compatibility from the command line though\n>> (albeit I'd guess there is some way to do so).\n>> [...]\n> \n> Thanks for digging. I think this case, though, is that we were simply\n> using the wrong variable ($SHELL instead of $SHELL_PATH). Your\n> workarounds would help if somebody put zsh into $SHELL_PATH, but\n> fundamentally that is not a sane thing to be doing, so I think we can\n> just consider doing so user error and not bother working around it.\n> \nFWIW, I agree.\n\nBest regards,\n  Stefano\n"},{"id":"199744","messageId":"7vobkx3j9i.fsf@alter.siamese.dyndns.org","threadId":"31569","inReplyTo":"CALkWK0kWJ3ndEMGzyGGMEa+ko3BTShC7_o-HVsXzFCGwRem5Jw@mail.gmail.com","subject":"Re: [PATCH] t/test-lib.sh: do not trust $SHELL","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-09-22T20:16:57Z","receivedAt":"2012-09-22T20:16:57Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ramkumar Ramachandra <artagnon@gmail.com> writes:\n\n> Here's a patch.\n>\n> -- 8< --\n> From: Ramkumar Ramachandra <artagnon@gmail.com>\n> Date: Sat, 22 Sep 2012 10:25:10 +0530\n> Subject: [PATCH] test-lib: do not trust $SHELL\n>\n> Do not trust $SHELL to be a bourne-compatible shell.  Instead, use the\n> Makefile variable $SHELL_PATH.  This fixes a bug: when a test was run\n> with --tee and $SHELL was set to ZSH, $PATH on line 479 was not\n> getting split due to ZSH not respecting $IFS.\n>\n> Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>\n> ---\n\nThe part that this starts letting run, which the original \"Re-run\nthe command under tee as early as possible\" wanted to avoid running,\ndoes not affect anything that would affect how we run that tee magic\n(e.g. \"mkdir -p test-results\" will still create it directly inside\nthe directory the test script was started in), so I think this patch\nis safe _for now_.\n\nHowever, it forces people who need to update earlier parts of this\nscript to be extra careful; it has been true before the patch, and\nthe patch makes it even more so.\n\nI am not opposed to queuing this as an interim solution, but I\nwonder if we can get rid of that double-launch altogether.\n\nInstead of re-launching the script with its output piped to \"tee\",\ncan't we do the same by redirecting our standard output to the file\nin the file, and spawn a \"tail -f\" that reads from the file and\noutputs to our original output?  Something along the lines of:\n\n        mkdir -p test-results\n        tee_base=test-results/$(basename \"$0\" .sh)\n\n        # empty the file and start \"tail -f\" on it ...\n        : >\"$tee_base.out\"\n        ( tail -f \"$tee_base.out\" ) &\n        tee_pid=$!\n\ttrap 'kill $tee_pid; exit' 0 1 2 3\n\t# ... and then redirect our output to it\n        exec >\"$tee_base.out\"\n\nand wrap it in a shell helper function that is called from where the\nparsing of the command line arguments for \"--tee\" happens, and don't\nforget to kill $tee_pid when we exit.\n\nHrm?\n"},{"id":"199746","messageId":"7vd31d3iwl.fsf@alter.siamese.dyndns.org","threadId":"31569","inReplyTo":"20120922174741.GA6722@sigill.intra.peff.net","subject":"Re: How do I run tests under Valgrind?","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-09-22T20:24:42Z","receivedAt":"2012-09-22T20:24:42Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> On Sat, Sep 22, 2012 at 03:03:58PM +0200, Stefano Lattarini wrote:\n>\n>> On 09/21/2012 10:49 PM, Jeff King wrote:\n>> >\n>> > Oh. It sounds like setting $SHELL to zsh is really the problem, then. If\n>> > it is not Bourne-compatible when called as \"zsh\", then it really should\n>> > be called in a way that turns on compatibility mode (bash will do this\n>> > when called as \"sh\", but you can also do it with \"bash --posix\").\n>> >\n>> AFAIK, if Zsh is called as \"sh\", it too will run in Bourne compatibility\n>> mode; not sure how to force this compatibility from the command line though\n>> (albeit I'd guess there is some way to do so).\n>> [...]\n>\n> Thanks for digging. I think this case, though, is that we were simply\n> using the wrong variable ($SHELL instead of $SHELL_PATH). Your\n> workarounds would help if somebody put zsh into $SHELL_PATH, but\n> fundamentally that is not a sane thing to be doing, so I think we can\n> just consider doing so user error and not bother working around it.\n\nYeah, 100% agreed with your assessment, including the part thanking\nStefano.\n\nThanks.\n"},{"id":"199923","messageId":"CALkWK0=1Bep0gBJAQQefDiZ7LMkXqXt91jPkgYkac0QkMBgOGw@mail.gmail.com","threadId":"31569","inReplyTo":"7vobkx3j9i.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] t/test-lib.sh: do not trust $SHELL","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2012-09-26T03:49:36Z","receivedAt":"2012-09-26T03:49:36Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Hi Junio,\n\nJunio C Hamano wrote:\n> Ramkumar Ramachandra <artagnon@gmail.com> writes:\n>\n>> Here's a patch.\n>>\n>> -- 8< --\n>> From: Ramkumar Ramachandra <artagnon@gmail.com>\n>> Date: Sat, 22 Sep 2012 10:25:10 +0530\n>> Subject: [PATCH] test-lib: do not trust $SHELL\n>>\n>> Do not trust $SHELL to be a bourne-compatible shell.  Instead, use the\n>> Makefile variable $SHELL_PATH.  This fixes a bug: when a test was run\n>> with --tee and $SHELL was set to ZSH, $PATH on line 479 was not\n>> getting split due to ZSH not respecting $IFS.\n>>\n>> Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>\n>> ---\n>\n> The part that this starts letting run, which the original \"Re-run\n> the command under tee as early as possible\" wanted to avoid running,\n> does not affect anything that would affect how we run that tee magic\n> (e.g. \"mkdir -p test-results\" will still create it directly inside\n> the directory the test script was started in), so I think this patch\n> is safe _for now_.\n>\n> However, it forces people who need to update earlier parts of this\n> script to be extra careful; it has been true before the patch, and\n> the patch makes it even more so.\n>\n> I am not opposed to queuing this as an interim solution, but I\n> wonder if we can get rid of that double-launch altogether.\n\nI see you've queued it in `pu` after rewriting the commit message.\n\n> Instead of re-launching the script with its output piped to \"tee\",\n> can't we do the same by redirecting our standard output to the file\n> in the file, and spawn a \"tail -f\" that reads from the file and\n> outputs to our original output?  Something along the lines of:\n>\n>         mkdir -p test-results\n>         tee_base=test-results/$(basename \"$0\" .sh)\n>\n>         # empty the file and start \"tail -f\" on it ...\n>         : >\"$tee_base.out\"\n>         ( tail -f \"$tee_base.out\" ) &\n>         tee_pid=$!\n>         trap 'kill $tee_pid; exit' 0 1 2 3\n>         # ... and then redirect our output to it\n>         exec >\"$tee_base.out\"\n>\n> and wrap it in a shell helper function that is called from where the\n> parsing of the command line arguments for \"--tee\" happens, and don't\n> forget to kill $tee_pid when we exit.\n>\n> Hrm?\n\nGood idea.  I'll write a patch to do this once the interim solution\ngraduates to `master`.\n\nRam\n"}]}