{"thread":{"id":"41598","subject":"\"./t0001-init.sh --valgrind\" is broken","startedAt":"2016-03-03T00:07:40Z","lastAt":"2016-03-03T18:17:34Z","messageCount":9,"participants":["Christian Couder","Duy Nguyen","Junio C Hamano","Johannes Sixt","Jeff King"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"280097","messageId":"CAP8UFD0-FxoPe5-35NAKSkegFBoRPy1+BOsqN-y+QJUoXAqR3g@mail.gmail.com","threadId":"41598","inReplyTo":null,"subject":"\"./t0001-init.sh --valgrind\" is broken","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2016-03-03T00:07:40Z","receivedAt":"2016-03-03T00:07:40Z","isPatch":false,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"Hi,\n\nIt looks like commit 57ea7123c86771f47f34e7d92d1822d8b429897a (git.c:\nmake sure we do not leak GIT_* to alias scripts, Dec 20 14:50:19 2015)\nbroke \"./t0001-init.sh --valgrind\".\n\nI get:\n\nexpecting success:\n        (\n                env | sed -ne \"/^GIT_/s/=.*//p\" &&\n                echo GIT_PREFIX &&        # setup.c\n                echo GIT_TEXTDOMAINDIR    # wrapper-for-bin.sh\n        ) | sort | uniq >expected &&\n        cat <<-\\EOF >script &&\n        #!/bin/sh\n        env | sed -ne \"/^GIT_/s/=.*//p\" | sort >actual\n        exit 0\n        EOF\n        chmod 755 script &&\n        git config alias.script \\!./script &&\n        ( mkdir sub && cd sub && git script ) &&\n        test_cmp expected actual\n\n--- expected    2016-03-03 00:05:17.113754381 +0000\n+++ actual      2016-03-03 00:05:19.041783583 +0000\n@@ -10,7 +10,6 @@\n GIT_PREFIX\n GIT_TEMPLATE_DIR\n GIT_TEST_TEE_STARTED\n-GIT_TEXTDOMAINDIR\n GIT_TRACE_BARE\n GIT_VALGRIND\n GIT_VALGRIND_ENABLED\nnot ok 6 - No extra GIT_* on alias scripts\n\nIt's late here so I don't have time to work on this tonight.\n\nThanks,\nChristian.\n"},{"id":"280098","messageId":"CACsJy8AL_RGEb2z44-yoL+3yF7n1+FMLqZkmWwUDFJnEPYS_nw@mail.gmail.com","threadId":"41598","inReplyTo":"CAP8UFD0-FxoPe5-35NAKSkegFBoRPy1+BOsqN-y+QJUoXAqR3g@mail.gmail.com","subject":"Re: \"./t0001-init.sh --valgrind\" is broken","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2016-03-03T01:04:54Z","receivedAt":"2016-03-03T01:04:54Z","isPatch":false,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Thu, Mar 3, 2016 at 7:07 AM, Christian Couder\n<christian.couder@gmail.com> wrote:\n> Hi,\n>\n> It looks like commit 57ea7123c86771f47f34e7d92d1822d8b429897a (git.c:\n> make sure we do not leak GIT_* to alias scripts, Dec 20 14:50:19 2015)\n> broke \"./t0001-init.sh --valgrind\".\n\nJust wanted to confirm the problem. I will look at it later today.\n-- \nDuy\n"},{"id":"280101","messageId":"xmqqsi082rqc.fsf@gitster.mtv.corp.google.com","threadId":"41598","inReplyTo":"CACsJy8AL_RGEb2z44-yoL+3yF7n1+FMLqZkmWwUDFJnEPYS_nw@mail.gmail.com","subject":"Re: \"./t0001-init.sh --valgrind\" is broken","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-03-03T02:57:31Z","receivedAt":"2016-03-03T02:57:31Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Duy Nguyen <pclouds@gmail.com> writes:\n\n> On Thu, Mar 3, 2016 at 7:07 AM, Christian Couder\n> <christian.couder@gmail.com> wrote:\n>> Hi,\n>>\n>> It looks like commit 57ea7123c86771f47f34e7d92d1822d8b429897a (git.c:\n>> make sure we do not leak GIT_* to alias scripts, Dec 20 14:50:19 2015)\n>> broke \"./t0001-init.sh --valgrind\".\n>\n> Just wanted to confirm the problem. I will look at it later today.\n\nThanks.\n"},{"id":"280105","messageId":"56D7DFD5.8010108@kdbg.org","threadId":"41598","inReplyTo":"CACsJy8AL_RGEb2z44-yoL+3yF7n1+FMLqZkmWwUDFJnEPYS_nw@mail.gmail.com","subject":"Re: \"./t0001-init.sh --valgrind\" is broken","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2016-03-03T06:55:17Z","receivedAt":"2016-03-03T06:55:17Z","isPatch":false,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 03.03.2016 um 02:04 schrieb Duy Nguyen:\n> On Thu, Mar 3, 2016 at 7:07 AM, Christian Couder\n> <christian.couder@gmail.com> wrote:\n>> Hi,\n>>\n>> It looks like commit 57ea7123c86771f47f34e7d92d1822d8b429897a (git.c:\n>> make sure we do not leak GIT_* to alias scripts, Dec 20 14:50:19 2015)\n>> broke \"./t0001-init.sh --valgrind\".\n> \n> Just wanted to confirm the problem. I will look at it later today.\n> \n\nHere's a patch.\n\n---- 8< ----\nSubject: [PATCH] t0001: fix GIT_* environment variable check under --valgrind\n\nWhen a test case is run without --valgrind, the wrap-for-bin.sh\nhelper script inserts the environment variable GIT_TEXTDOMAINDIR, but\nwhen run with --valgrind, the variable is missing. A recently\nintroduced test case expects the presence of the variable, though, and\nfails under --valgrind.\n\nRewrite the test case to strip conditially defined environment variables\nfrom both expected and actual output.\n\nSigned-off-by: Johannes Sixt <j6t@kdbg.org>\n---\n t/t0001-init.sh | 20 +++++++++-----------\n 1 file changed, 9 insertions(+), 11 deletions(-)\n\ndiff --git a/t/t0001-init.sh b/t/t0001-init.sh\nindex 295aa59..a5b9e7a 100755\n--- a/t/t0001-init.sh\n+++ b/t/t0001-init.sh\n@@ -88,19 +88,17 @@ test_expect_success 'plain nested in bare through aliased command' '\n '\n \n test_expect_success 'No extra GIT_* on alias scripts' '\n-\t(\n-\t\tenv | sed -ne \"/^GIT_/s/=.*//p\" &&\n-\t\techo GIT_PREFIX &&        # setup.c\n-\t\techo GIT_TEXTDOMAINDIR    # wrapper-for-bin.sh\n-\t) | sort | uniq >expected &&\n-\tcat <<-\\EOF >script &&\n-\t#!/bin/sh\n-\tenv | sed -ne \"/^GIT_/s/=.*//p\" | sort >actual\n-\texit 0\n+\twrite_script script <<-\\EOF &&\n+\tenv |\n+\t\tsed -n \\\n+\t\t\t-e \"/^GIT_PREFIX=/d\" \\\n+\t\t\t-e \"/^GIT_TEXTDOMAINDIR=/d\" \\\n+\t\t\t-e \"/^GIT_/s/=.*//p\" |\n+\t\tsort\n \tEOF\n-\tchmod 755 script &&\n+\t./script >expected &&\n \tgit config alias.script \\!./script &&\n-\t( mkdir sub && cd sub && git script ) &&\n+\t( mkdir sub && cd sub && git script >../actual ) &&\n \ttest_cmp expected actual\n '\n \n-- \n2.7.0.118.g90056ae\n"},{"id":"280109","messageId":"CACsJy8CVfJdCmwWo50wHsc6DP6ft71d5bfBaFSJpv9XcNRwdSQ@mail.gmail.com","threadId":"41598","inReplyTo":"56D7DFD5.8010108@kdbg.org","subject":"Re: \"./t0001-init.sh --valgrind\" is broken","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2016-03-03T12:09:12Z","receivedAt":"2016-03-03T12:09:12Z","isPatch":false,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"+the-other-Johannes who added valgrind support.\n\nOn Thu, Mar 3, 2016 at 1:55 PM, Johannes Sixt <j6t@kdbg.org> wrote:\n> ---- 8< ----\n> Subject: [PATCH] t0001: fix GIT_* environment variable check under --valgrind\n>\n> When a test case is run without --valgrind, the wrap-for-bin.sh\n> helper script inserts the environment variable GIT_TEXTDOMAINDIR, but\n> when run with --valgrind, the variable is missing. A recently\n> introduced test case expects the presence of the variable, though, and\n> fails under --valgrind.\n\nYep.\n\nIt's interesting though that valgrind sets up some variables without\ngoing through bin-wrappers. That's understandable because valgrind\nsupport is added (in 4e1be63) 10 months before bin-wrappers (in\nea92519).  But it's probably better that we inject valgrind command\nfrom inside bin-wrappers script, the same way we inject gdb, I think.\n\n> Rewrite the test case to strip conditially defined environment variables\n> from both expected and actual output.\n\nOr we could set GIT_TEXTDOMAINDIR in the \"if test -n $valgrind\" code\nin test-lib.sh, which makes the two more consistent. Also simpler\npatch.\n\n> Signed-off-by: Johannes Sixt <j6t@kdbg.org>\n> ---\n>  t/t0001-init.sh | 20 +++++++++-----------\n>  1 file changed, 9 insertions(+), 11 deletions(-)\n>\n> diff --git a/t/t0001-init.sh b/t/t0001-init.sh\n> index 295aa59..a5b9e7a 100755\n> --- a/t/t0001-init.sh\n> +++ b/t/t0001-init.sh\n> @@ -88,19 +88,17 @@ test_expect_success 'plain nested in bare through aliased command' '\n>  '\n>\n>  test_expect_success 'No extra GIT_* on alias scripts' '\n> -       (\n> -               env | sed -ne \"/^GIT_/s/=.*//p\" &&\n> -               echo GIT_PREFIX &&        # setup.c\n> -               echo GIT_TEXTDOMAINDIR    # wrapper-for-bin.sh\n> -       ) | sort | uniq >expected &&\n> -       cat <<-\\EOF >script &&\n> -       #!/bin/sh\n> -       env | sed -ne \"/^GIT_/s/=.*//p\" | sort >actual\n> -       exit 0\n> +       write_script script <<-\\EOF &&\n> +       env |\n> +               sed -n \\\n> +                       -e \"/^GIT_PREFIX=/d\" \\\n> +                       -e \"/^GIT_TEXTDOMAINDIR=/d\" \\\n> +                       -e \"/^GIT_/s/=.*//p\" |\n> +               sort\n>         EOF\n> -       chmod 755 script &&\n> +       ./script >expected &&\n>         git config alias.script \\!./script &&\n> -       ( mkdir sub && cd sub && git script ) &&\n> +       ( mkdir sub && cd sub && git script >../actual ) &&\n>         test_cmp expected actual\n>  '\n>\n> --\n> 2.7.0.118.g90056ae\n>\n\n\n\n-- \nDuy\n"},{"id":"280110","messageId":"CACsJy8AE9VovwuviwOLxRDTAbTXCivRVhk8ia4mdUnMN-0Y4OA@mail.gmail.com","threadId":"41598","inReplyTo":"CACsJy8CVfJdCmwWo50wHsc6DP6ft71d5bfBaFSJpv9XcNRwdSQ@mail.gmail.com","subject":"Re: \"./t0001-init.sh --valgrind\" is broken","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2016-03-03T12:16:55Z","receivedAt":"2016-03-03T12:16:55Z","isPatch":false,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Thu, Mar 3, 2016 at 7:09 PM, Duy Nguyen <pclouds@gmail.com> wrote:\n> But it's probably better that we inject valgrind command\n> from inside bin-wrappers script, the same way we inject gdb, I think.\n\nFor the best of both worlds, we should recreate bin-wrappers in\ntest-lib.sh (i.e. the valgrind way), not in Makefile. Somewhat\nunrelated, but because topdir is getting really crowded and\nbin-wrappers is used for the test suite only, it should be moved\ninside t/ (i'm going to move all test-* to t/ too, later).\n-- \nDuy\n"},{"id":"280121","messageId":"xmqqk2lj3692.fsf@gitster.mtv.corp.google.com","threadId":"41598","inReplyTo":"CACsJy8AE9VovwuviwOLxRDTAbTXCivRVhk8ia4mdUnMN-0Y4OA@mail.gmail.com","subject":"Re: \"./t0001-init.sh --valgrind\" is broken","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-03-03T15:56:09Z","receivedAt":"2016-03-03T15:56:09Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Duy Nguyen <pclouds@gmail.com> writes:\n\n> On Thu, Mar 3, 2016 at 7:09 PM, Duy Nguyen <pclouds@gmail.com> wrote:\n>> But it's probably better that we inject valgrind command\n>> from inside bin-wrappers script, the same way we inject gdb, I think.\n>\n> For the best of both worlds, we should recreate bin-wrappers in\n> test-lib.sh (i.e. the valgrind way), not in Makefile.\n>\n> Somewhat\n> unrelated, but because topdir is getting really crowded and\n> bin-wrappers is used for the test suite only, it should be moved\n> inside t/ (i'm going to move all test-* to t/ too, later).\n\nWeren't there people who pointed their bin-wrappers/ with $PATH to\ntest/use freshly baked Git before they convince themselves that they\nwant to install it?  Not building it from the top-level Makefile\nand moving it to elsewhere would be two breakages for them.\n\nI am not sure if that is a good idea.\n\nMoving test-* sources out of the top-level is a good idea, and\nplacing test-* binaries somewhere other than the top-level is also a\ngood idea.  Just like t/lib-*.sh are helpers for tests, these are\nalso test helpers that happen to be written in C and compiled, so I\ndon't have a strong objection to make t/ the new location for\nthem--a different location (e.g. a new \"test-helpers/\" directory) is\nalso something I can go with.\n\nThanks.\n\nIn any case, are these two messages objections to J6t's fix, or are\nyou fine with the fix for 2.8-rc1 and merely raising ideas to redo\nit in a different (i.e. your) way after 2.8 final, or are you\nplanning to do a fix in a different way for 2.8-rc1?\n"},{"id":"280134","messageId":"20160303180501.GA23116@sigill.intra.peff.net","threadId":"41598","inReplyTo":"CACsJy8CVfJdCmwWo50wHsc6DP6ft71d5bfBaFSJpv9XcNRwdSQ@mail.gmail.com","subject":"Re: \"./t0001-init.sh --valgrind\" is broken","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-03-03T18:05:01Z","receivedAt":"2016-03-03T18:05:01Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Mar 03, 2016 at 07:09:12PM +0700, Duy Nguyen wrote:\n\n> +the-other-Johannes who added valgrind support.\n> \n> On Thu, Mar 3, 2016 at 1:55 PM, Johannes Sixt <j6t@kdbg.org> wrote:\n> > ---- 8< ----\n> > Subject: [PATCH] t0001: fix GIT_* environment variable check under --valgrind\n> >\n> > When a test case is run without --valgrind, the wrap-for-bin.sh\n> > helper script inserts the environment variable GIT_TEXTDOMAINDIR, but\n> > when run with --valgrind, the variable is missing. A recently\n> > introduced test case expects the presence of the variable, though, and\n> > fails under --valgrind.\n> \n> Yep.\n> \n> It's interesting though that valgrind sets up some variables without\n> going through bin-wrappers. That's understandable because valgrind\n> support is added (in 4e1be63) 10 months before bin-wrappers (in\n> ea92519).  But it's probably better that we inject valgrind command\n> from inside bin-wrappers script, the same way we inject gdb, I think.\n\nI had the same thought and even started on a patch, but it doesn't quite\nwork. The bin-wrappers are all about intercepting what goes into the\nuser's $PATH, and pointing our libexec dir at the main build.\n\nSo we have \"git\" and \"git-upload-pack\" in bin-wrappers, but not\n\"git-log\". Whereas the valgrind code wants to intercept _all_ of the\ntest script's invocations of git, including ones spawned by scripts,\nother git commands, etc. So conceptually, it wants to intercept\n$GIT_EXEC_PATH.\n\n-Peff\n"},{"id":"280136","messageId":"56D87FBE.5050908@kdbg.org","threadId":"41598","inReplyTo":"CACsJy8CVfJdCmwWo50wHsc6DP6ft71d5bfBaFSJpv9XcNRwdSQ@mail.gmail.com","subject":"Re: \"./t0001-init.sh --valgrind\" is broken","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2016-03-03T18:17:34Z","receivedAt":"2016-03-03T18:17:34Z","isPatch":false,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 03.03.2016 um 13:09 schrieb Duy Nguyen:\n> +the-other-Johannes who added valgrind support.\n>\n> On Thu, Mar 3, 2016 at 1:55 PM, Johannes Sixt <j6t@kdbg.org> wrote:\n>> ---- 8< ----\n>> Subject: [PATCH] t0001: fix GIT_* environment variable check under --valgrind\n>>\n>> When a test case is run without --valgrind, the wrap-for-bin.sh\n>> helper script inserts the environment variable GIT_TEXTDOMAINDIR, but\n>> when run with --valgrind, the variable is missing. A recently\n>> introduced test case expects the presence of the variable, though, and\n>> fails under --valgrind.\n>\n> Yep.\n>\n> It's interesting though that valgrind sets up some variables without\n> going through bin-wrappers. That's understandable because valgrind\n> support is added (in 4e1be63) 10 months before bin-wrappers (in\n> ea92519).  But it's probably better that we inject valgrind command\n> from inside bin-wrappers script, the same way we inject gdb, I think.\n>\n>> Rewrite the test case to strip conditially defined environment variables\n>> from both expected and actual output.\n>\n> Or we could set GIT_TEXTDOMAINDIR in the \"if test -n $valgrind\" code\n> in test-lib.sh, which makes the two more consistent. Also simpler\n> patch.\n\nMy fix (or something along its lines) is needed nevertheless. Just \ns/--valgrind/--with-dashes/g in the commit message if you want to fix \nthe --valgrind case differently ;-)\n\nI run tests on Windows --with-dashes in the hopes that it saves a fork \nand exec or two on every git invocation.\n\n-- Hannes\n"}]}