{"thread":{"id":"57121","subject":"[PATCH RESEND] t/perf: do not run tests in user's $SHELL","startedAt":"2021-12-20T11:05:35Z","lastAt":"2021-12-25T08:22:29Z","messageCount":6,"participants":["René Scharfe","Ævar Arnfjörð Bjarmason","Johannes Altmanninger","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"444449","messageId":"f22f978a-26eb-8fe9-cab4-3fd60df69635@web.de","threadId":"57121","inReplyTo":null,"subject":"[PATCH RESEND] t/perf: do not run tests in user's $SHELL","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2021-12-20T11:05:18Z","receivedAt":"2021-12-20T11:05:35Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"From: Johannes Altmanninger \n\nThe environment variable $SHELL is usually set to the user's\ninteractive shell. We never use that shell for build and test scripts\nbecause it might not be a POSIX shell.\n\nPerf tests are run inside $SHELL via a wrapper defined in\nt/perf/perf-lib.sh. Use $TEST_SHELL_PATH like elsewhere.\n\nSigned-off-by: Johannes Altmanninger \nAcked-by: Jeff King \n---\nOriginal submission:\nhttps://lore.kernel.org/git/20211007184716.1187677-1-aclopte@gmail.com/\n\n t/perf/perf-lib.sh | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/t/perf/perf-lib.sh b/t/perf/perf-lib.sh\nindex 780a7402d5..407252bac7 100644\n--- a/t/perf/perf-lib.sh\n+++ b/t/perf/perf-lib.sh\n@@ -161,7 +161,7 @@ test_run_perf_ () {\n \ttest_cleanup=:\n \ttest_export_=\"test_cleanup\"\n \texport test_cleanup test_export_\n-\t\"$GTIME\" -f \"%E %U %S\" -o test_time.$i \"$SHELL\" -c '\n+\t\"$GTIME\" -f \"%E %U %S\" -o test_time.$i \"$TEST_SHELL_PATH\" -c '\n . '\"$TEST_DIRECTORY\"/test-lib-functions.sh'\n test_export () {\n \ttest_export_=\"$test_export_ $*\"\n--\n2.34.0\n"},{"id":"444452","messageId":"211220.86bl1bwkp9.gmgdl@evledraar.gmail.com","threadId":"57121","inReplyTo":"f22f978a-26eb-8fe9-cab4-3fd60df69635@web.de","subject":"Re: [PATCH RESEND] t/perf: do not run tests in user's $SHELL","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-12-20T11:56:58Z","receivedAt":"2021-12-20T11:59:51Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Mon, Dec 20 2021, René Scharfe wrote:\n\n> From: Johannes Altmanninger \n>\n> The environment variable $SHELL is usually set to the user's\n> interactive shell. We never use that shell for build and test scripts\n> because it might not be a POSIX shell.\n>\n> Perf tests are run inside $SHELL via a wrapper defined in\n> t/perf/perf-lib.sh. Use $TEST_SHELL_PATH like elsewhere.\n>\n> Signed-off-by: Johannes Altmanninger \n> Acked-by: Jeff King \n> ---\n> Original submission:\n> https://lore.kernel.org/git/20211007184716.1187677-1-aclopte@gmail.com/\n\nThis LGTM & I think it could be picked up as-is.\n\nJust a nit in case af a re-roll. I think it would help to summarize the\nhistory a bit per\nhttps://lore.kernel.org/git/YV+1%2F0b5bN3o6qRG@coredump.intra.peff.net/. I.e. something\nlike:\n \n In 342e9ef2d9e (Introduce a performance testing framework, 2012-02-17)\n when t/perf was introduced the TEST_SHELL_PATH was not part of\n GIT-BUILD-OPTIONS. That was added later in 3f824e91c84 (t/Makefile:\n introduce TEST_SHELL_PATH, 2017-12-08). We will always have that\n available in perf-lib.sh since test-lib.sh will load it before this code\n is executed.\n\n> t/perf/perf-lib.sh | 2 +-\n> 1 file changed, 1 insertion(+), 1 deletion(-)\n>\n> diff --git a/t/perf/perf-lib.sh b/t/perf/perf-lib.sh\n> index 780a7402d5..407252bac7 100644\n> --- a/t/perf/perf-lib.sh\n> +++ b/t/perf/perf-lib.sh\n> @@ -161,7 +161,7 @@ test_run_perf_ () {\n> \ttest_cleanup=:\n> \ttest_export_=\"test_cleanup\"\n> \texport test_cleanup test_export_\n> -\t\"$GTIME\" -f \"%E %U %S\" -o test_time.$i \"$SHELL\" -c '\n> +\t\"$GTIME\" -f \"%E %U %S\" -o test_time.$i \"$TEST_SHELL_PATH\" -c '\n> . '\"$TEST_DIRECTORY\"/test-lib-functions.sh'\n> test_export () {\n> \ttest_export_=\"$test_export_ $*\"\n\n"},{"id":"444458","messageId":"20211220131121.mdxe7o6p3y25fzbw@gmail.com","threadId":"57121","inReplyTo":"211220.86bl1bwkp9.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH RESEND] t/perf: do not run tests in user's $SHELL","fromName":"Johannes Altmanninger","fromEmail":"aclopte@gmail.com","sentAt":"2021-12-20T13:11:21Z","receivedAt":"2021-12-20T13:11:29Z","isPatch":true,"sender":{"key":"aclopte@gmail.com","avatar":"https://avatars.githubusercontent.com/u/6853872?v=4"},"body":"On Mon, Dec 20, 2021 at 12:56:58PM +0100, Ævar Arnfjörð Bjarmason wrote:\n> \n> On Mon, Dec 20 2021, René Scharfe wrote:\n> \n> > From: Johannes Altmanninger \n> >\n> > The environment variable $SHELL is usually set to the user's\n> > interactive shell. We never use that shell for build and test scripts\n> > because it might not be a POSIX shell.\n> >\n> > Perf tests are run inside $SHELL via a wrapper defined in\n> > t/perf/perf-lib.sh. Use $TEST_SHELL_PATH like elsewhere.\n> >\n> > Signed-off-by: Johannes Altmanninger \n> > Acked-by: Jeff King \n> > ---\n> > Original submission:\n> > https://lore.kernel.org/git/20211007184716.1187677-1-aclopte@gmail.com/\n> \n> This LGTM & I think it could be picked up as-is.\n> \n> Just a nit in case af a re-roll. I think it would help to summarize the\n> history a bit per\n> https://lore.kernel.org/git/YV+1%2F0b5bN3o6qRG@coredump.intra.peff.net/. I.e. something\n> like:\n> \n> In 342e9ef2d9e (Introduce a performance testing framework, 2012-02-17)\n> when t/perf was introduced the TEST_SHELL_PATH was not part of\n> GIT-BUILD-OPTIONS.\n\n(but SHELL_PATH was, which is what we should have used back then)\n\n> That was added later in 3f824e91c84 (t/Makefile:\n> introduce TEST_SHELL_PATH, 2017-12-08). We will always have that\n> available in perf-lib.sh since test-lib.sh will load it before this code\n> is executed.\n\nyes that's a good thing to point out\n\n> \n> > t/perf/perf-lib.sh | 2 +-\n> > 1 file changed, 1 insertion(+), 1 deletion(-)\n> >\n> > diff --git a/t/perf/perf-lib.sh b/t/perf/perf-lib.sh\n> > index 780a7402d5..407252bac7 100644\n> > --- a/t/perf/perf-lib.sh\n> > +++ b/t/perf/perf-lib.sh\n> > @@ -161,7 +161,7 @@ test_run_perf_ () {\n> > \ttest_cleanup=:\n> > \ttest_export_=\"test_cleanup\"\n> > \texport test_cleanup test_export_\n> > -\t\"$GTIME\" -f \"%E %U %S\" -o test_time.$i \"$SHELL\" -c '\n> > +\t\"$GTIME\" -f \"%E %U %S\" -o test_time.$i \"$TEST_SHELL_PATH\" -c '\n> > . '\"$TEST_DIRECTORY\"/test-lib-functions.sh'\n> > test_export () {\n> > \ttest_export_=\"$test_export_ $*\"\n> \n"},{"id":"444541","messageId":"xmqqilvjugu0.fsf@gitster.g","threadId":"57121","inReplyTo":"20211220131121.mdxe7o6p3y25fzbw@gmail.com","subject":"Re: [PATCH RESEND] t/perf: do not run tests in user's $SHELL","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-12-20T21:06:15Z","receivedAt":"2021-12-20T21:06:27Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Altmanninger writes:\n\n> On Mon, Dec 20, 2021 at 12:56:58PM +0100, Ævar Arnfjörð Bjarmason wrote:\n>> \n>> On Mon, Dec 20 2021, René Scharfe wrote:\n>> \n>> > From: Johannes Altmanninger \n>> >\n>> > The environment variable $SHELL is usually set to the user's\n>> > interactive shell. We never use that shell for build and test scripts\n>> > because it might not be a POSIX shell.\n>> >\n>> > Perf tests are run inside $SHELL via a wrapper defined in\n>> > t/perf/perf-lib.sh. Use $TEST_SHELL_PATH like elsewhere.\n>> >\n>> > Signed-off-by: Johannes Altmanninger \n>> > Acked-by: Jeff King \n>> > ---\n>> > Original submission:\n>> > https://lore.kernel.org/git/20211007184716.1187677-1-aclopte@gmail.com/\n>> \n>> This LGTM & I think it could be picked up as-is.\n>> \n>> Just a nit in case af a re-roll. I think it would help to summarize the\n>> history a bit per\n>> https://lore.kernel.org/git/YV+1%2F0b5bN3o6qRG@coredump.intra.peff.net/. I.e. something\n>> like:\n>> \n>> In 342e9ef2d9e (Introduce a performance testing framework, 2012-02-17)\n>> when t/perf was introduced the TEST_SHELL_PATH was not part of\n>> GIT-BUILD-OPTIONS.\n>\n> (but SHELL_PATH was, which is what we should have used back then)\n>\n>> That was added later in 3f824e91c84 (t/Makefile:\n>> introduce TEST_SHELL_PATH, 2017-12-08). We will always have that\n>> available in perf-lib.sh since test-lib.sh will load it before this code\n>> is executed.\n>\n> yes that's a good thing to point out\n\nCare to redo the patch in a final form, then?\n\nThanks.\n\n>\n>> \n>> > t/perf/perf-lib.sh | 2 +-\n>> > 1 file changed, 1 insertion(+), 1 deletion(-)\n>> >\n>> > diff --git a/t/perf/perf-lib.sh b/t/perf/perf-lib.sh\n>> > index 780a7402d5..407252bac7 100644\n>> > --- a/t/perf/perf-lib.sh\n>> > +++ b/t/perf/perf-lib.sh\n>> > @@ -161,7 +161,7 @@ test_run_perf_ () {\n>> > \ttest_cleanup=:\n>> > \ttest_export_=\"test_cleanup\"\n>> > \texport test_cleanup test_export_\n>> > -\t\"$GTIME\" -f \"%E %U %S\" -o test_time.$i \"$SHELL\" -c '\n>> > +\t\"$GTIME\" -f \"%E %U %S\" -o test_time.$i \"$TEST_SHELL_PATH\" -c '\n>> > . '\"$TEST_DIRECTORY\"/test-lib-functions.sh'\n>> > test_export () {\n>> > \ttest_export_=\"$test_export_ $*\"\n>> \n"},{"id":"444941","messageId":"20211225074736.ozxhbb67pzpaud6g@gmail.com","threadId":"57121","inReplyTo":"xmqqilvjugu0.fsf@gitster.g","subject":"Re: [PATCH RESEND] t/perf: do not run tests in user's $SHELL","fromName":"Johannes Altmanninger","fromEmail":"aclopte@gmail.com","sentAt":"2021-12-25T07:47:36Z","receivedAt":"2021-12-25T07:47:43Z","isPatch":true,"sender":{"key":"aclopte@gmail.com","avatar":"https://avatars.githubusercontent.com/u/6853872?v=4"},"body":"On Mon, Dec 20, 2021 at 01:06:15PM -0800, Junio C Hamano wrote:\n> Johannes Altmanninger writes:\n> \n> > On Mon, Dec 20, 2021 at 12:56:58PM +0100, Ævar Arnfjörð Bjarmason wrote:\n> >> That was added later in 3f824e91c84 (t/Makefile:\n> >> introduce TEST_SHELL_PATH, 2017-12-08). We will always have that\n> >> available in perf-lib.sh since test-lib.sh will load it before this code\n> >> is executed.\n> >\n> > yes that's a good thing to point out\n> \n> Care to redo the patch in a final form, then?\n\nOf course. I should have acked that earlier, sorry. I waited until I could\nsend my updated version but didn't find a quiet moment until today. Anyway,\nsending the updated patch now.\n"},{"id":"444951","messageId":"20211225081656.1311583-1-aclopte@gmail.com","threadId":"57121","inReplyTo":"xmqqilvjugu0.fsf@gitster.g","subject":"[PATCH v2] t/perf: do not run tests in user's $SHELL","fromName":"Johannes Altmanninger","fromEmail":"aclopte@gmail.com","sentAt":"2021-12-25T08:16:58Z","receivedAt":"2021-12-25T08:22:29Z","isPatch":true,"sender":{"key":"aclopte@gmail.com","avatar":"https://avatars.githubusercontent.com/u/6853872?v=4"},"body":"The environment variable $SHELL is usually set to the user's\ninteractive shell. Our build and test scripts never use $SHELL because\nthere are no guarantees about its input language. Instead, we use\n/bin/sh which should be a POSIX shell.\n\nFor systems with a broken /bin/sh, we allow to override that path via\nSHELL_PATH. To run tests in yet another shell we allow to override\nSHELL_PATH with TEST_SHELL_PATH.\n\nPerf tests run in $SHELL via a wrapper defined in t/perf/perf-lib.sh,\nso they break with e.g. SHELL=python. Use TEST_SHELL_PATH like\nin other tests. TEST_SHELL_PATH is always defined because\nt/perf/perf-lib.sh includes t/test-lib.sh, which includes\nGIT-BUILD-OPTIONS.\n\nAcked-by: Jeff King \nSigned-off-by: Johannes Altmanninger \n---\n t/perf/perf-lib.sh | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\nI extended the commit message because in hindsight it was overly terse\n(judging from both re-reading it and from review comments).\n\nWe could add more Acked-bys but one seems enough here.\n\nrange-diff to the first version:\n\n @@ Commit message\n t/perf: do not run tests in user's $SHELL\n \n The environment variable $SHELL is usually set to the user's\n - interactive shell. We never use that shell for build and test scripts\n - because it might not be a POSIX shell.\n + interactive shell. Our build and test scripts never use $SHELL because\n + there are no guarantees about its input language. Instead, we use\n + /bin/sh which should be a POSIX shell.\n \n - Perf tests are run inside $SHELL via a wrapper defined in\n - t/perf/perf-lib.sh. Use $TEST_SHELL_PATH like elsewhere.\n + For systems with a broken /bin/sh, we allow to override that path via\n + SHELL_PATH. To run tests in yet another shell we allow to override\n + SHELL_PATH with TEST_SHELL_PATH.\n +\n + Perf tests run in $SHELL via a wrapper defined in t/perf/perf-lib.sh,\n + so they break with e.g. SHELL=python. Use TEST_SHELL_PATH like\n + in other tests. TEST_SHELL_PATH is always defined because\n + t/perf/perf-lib.sh includes t/test-lib.sh, which includes\n + GIT-BUILD-OPTIONS.\n +\n + Acked-by: Jeff King \n\ndiff --git a/t/perf/perf-lib.sh b/t/perf/perf-lib.sh\nindex 780a7402d5..407252bac7 100644\n--- a/t/perf/perf-lib.sh\n+++ b/t/perf/perf-lib.sh\n@@ -161,7 +161,7 @@ test_run_perf_ () {\n \ttest_cleanup=:\n \ttest_export_=\"test_cleanup\"\n \texport test_cleanup test_export_\n-\t\"$GTIME\" -f \"%E %U %S\" -o test_time.$i \"$SHELL\" -c '\n+\t\"$GTIME\" -f \"%E %U %S\" -o test_time.$i \"$TEST_SHELL_PATH\" -c '\n . '\"$TEST_DIRECTORY\"/test-lib-functions.sh'\n test_export () {\n \ttest_export_=\"$test_export_ $*\"\n-- \n2.34.1\n\n"}]}