{"thread":{"id":"56626","subject":"[PATCH] perf: fix test_export with SHELL=zsh","startedAt":"2021-10-02T09:40:11Z","lastAt":"2021-10-08T05:41:37Z","messageCount":7,"participants":["René Scharfe","Johannes Altmanninger","brian m. carlson","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"437761","messageId":"8b70d04f-0ad1-6e68-f5a2-2d8ec3bb98ea@web.de","threadId":"56626","inReplyTo":null,"subject":"[PATCH] perf: fix test_export with SHELL=zsh","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2021-10-02T09:40:02Z","receivedAt":"2021-10-02T09:40:11Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Unlike other shells, zsh doesn't do word-splitting on variables.  This\nis documented in https://zsh.sourceforge.io/FAQ/zshfaq03.html#31.  That\nbreaks the perf function test_export because it uses a space-separated\nvariable as a poor man's array, and as a consequence p0000 fails with\n\"not ok 3 - test_export works\".  Pass the value through an unquoted\ncommand substitution to force word-splitting even in zsh.\n\nSigned-off-by: René Scharfe <l.s.r@web.de>\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 f5ed092ee5..f74cfd35d6 100644\n--- a/t/perf/perf-lib.sh\n+++ b/t/perf/perf-lib.sh\n@@ -165,7 +165,7 @@ test_export () {\n '\"$1\"'\n ret=$?\n needles=\n-for v in $test_export_\n+for v in $(echo \"$test_export_\")\n do\n \tneedles=\"$needles;s/^$v=/export $v=/p\"\n done\n--\n2.33.0\n"},{"id":"437762","messageId":"20211002112640.hrn2ojndhoa2dd4c@gmail.com","threadId":"56626","inReplyTo":"8b70d04f-0ad1-6e68-f5a2-2d8ec3bb98ea@web.de","subject":"Re: [PATCH] perf: fix test_export with SHELL=zsh","fromName":"Johannes Altmanninger","fromEmail":"aclopte@gmail.com","sentAt":"2021-10-02T11:26:40Z","receivedAt":"2021-10-02T11:26:47Z","isPatch":true,"sender":{"key":"aclopte@gmail.com","avatar":"https://avatars.githubusercontent.com/u/6853872?v=4"},"body":"On Sat, Oct 02, 2021 at 11:40:02AM +0200, René Scharfe wrote:\n> Pass the value through an unquoted command substitution to force\n> word-splitting even in zsh.\n\nThis seems like the wrong direction.  There are probably other places where\nwe rely on such POSIX features.  Also it doesn't fix the case where users\nuse other non-POSIX shells, say\n\n\tSHELL=/bin/fish t/perf/run p0000-perf-lib-sanity.sh\n\nWe already have solutions in place - our top-level Makefile has\n\n\t# Define SHELL_PATH to a POSIX shell if your /bin/sh is broken.\n\t...\n\tifndef SHELL_PATH\n\t\tSHELL_PATH = /bin/sh\n\tendif\n\t...\n\tSHELL = $(SHELL_PATH)\n\nMeanwhile t/Makefile uses\n\n\tSHELL_PATH ?= $(SHELL)\n\tTEST_SHELL_PATH ?= $(SHELL_PATH)\n\nA related fix is to make those two consistent.  I would prefer the version\nin the top-level Makefile, because t/Makefile gives the false illusion that\n$SHELL is honored:\n\n\tSHELL=garbage make -C t t4211-line-log.sh # succeeds because make sets SHELL=/bin/sh\n\tmake SHELL=garbage -C t t4211-line-log.sh # fails\n\n---\n\nAnyway, for this issue we should just have t/perf/perf-lib.sh follow our\nconvention of using ${SHELL_PATH:-/bin/sh} instead of $SHELL.\n\nLooks like t/interop/Makefile has the same problem.\n\nI'll prepare the patches later unless someone beats me to it :)\n"},{"id":"437790","messageId":"YVjI3AgD7SK4zxy+@camp.crustytoothpaste.net","threadId":"56626","inReplyTo":"8b70d04f-0ad1-6e68-f5a2-2d8ec3bb98ea@web.de","subject":"Re: [PATCH] perf: fix test_export with SHELL=zsh","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2021-10-02T21:02:20Z","receivedAt":"2021-10-02T21:03:30Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On 2021-10-02 at 09:40:02, René Scharfe wrote:\n> Unlike other shells, zsh doesn't do word-splitting on variables.  This\n> is documented in https://zsh.sourceforge.io/FAQ/zshfaq03.html#31.  That\n> breaks the perf function test_export because it uses a space-separated\n> variable as a poor man's array, and as a consequence p0000 fails with\n> \"not ok 3 - test_export works\".  Pass the value through an unquoted\n> command substitution to force word-splitting even in zsh.\n\nThere are a variety of places in our testsuite where zsh is broken in\nzsh mode.  I recently sent a patch to make it do the right thing for\nsubshells in sh mode which has not yet been released, and as far as I'm\naware, with that patch, our testsuite is fully functional with zsh when\nit's run in sh mode.\n\nNote that in sh mode, zsh enables the SH_WORD_SPLIT option, and this\nshould work just fine.  The easiest way to do that is to create a\nsymlink to zsh called \"sh\" and invoke that.\n-- \nbrian m. carlson (he/him or they/them)\nToronto, Ontario, CA\n"},{"id":"438193","messageId":"20211007184716.1187677-1-aclopte@gmail.com","threadId":"56626","inReplyTo":"20211002112640.hrn2ojndhoa2dd4c@gmail.com","subject":"[PATCH] t/perf: do not run tests in user's $SHELL","fromName":"Johannes Altmanninger","fromEmail":"aclopte@gmail.com","sentAt":"2021-10-07T18:47:16Z","receivedAt":"2021-10-07T18:47:32Z","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. 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 <aclopte@gmail.com>\n---\n\nRegarding the inconsistency around $(SHELL) in Makefiles: we could do\nsomething like\n\n\t-SHELL_PATH ?= $(SHELL)\n\t+SHELL_PATH ?= /bin/sh\n\t+SHELL = $(SHELL_PATH)\n\nin some Makefiles. Though the upside (consistency & slightly easier to build\nwith broken /bin/sh) seems fairly low, so I'd leave it be.\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 f5ed092ee5..e6b59ecbf7 100644\n--- a/t/perf/perf-lib.sh\n+++ b/t/perf/perf-lib.sh\n@@ -157,7 +157,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.33.0.rc2.dirty\n\n"},{"id":"438256","messageId":"YV+1/0b5bN3o6qRG@coredump.intra.peff.net","threadId":"56626","inReplyTo":"20211007184716.1187677-1-aclopte@gmail.com","subject":"Re: [PATCH] t/perf: do not run tests in user's $SHELL","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-10-08T03:07:43Z","receivedAt":"2021-10-08T03:07:45Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Oct 07, 2021 at 08:47:16PM +0200, Johannes Altmanninger wrote:\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\nYes, I think this is the right thing to do. We didn't have\n$TEST_SHELL_PATH back when this code was added, but I think it should\nhave been $SHELL_PATH from the start.\n\nI wondered if we would always have TEST_SHELL_PATH set, but we should:\nit is put unconditionally into GIT-BUILD-OPTIONS, and we will always\nload that via test-lib.sh, even if the test is run outside of \"make\".\n\n> ---\n> \n> Regarding the inconsistency around $(SHELL) in Makefiles: we could do\n> something like\n> \n> \t-SHELL_PATH ?= $(SHELL)\n> \t+SHELL_PATH ?= /bin/sh\n> \t+SHELL = $(SHELL_PATH)\n> \n> in some Makefiles. Though the upside (consistency & slightly easier to build\n> with broken /bin/sh) seems fairly low, so I'd leave it be.\n\nIn general assuming that $SHELL is a valid /bin/sh replacement is\nquestionable (e.g., if your login shell is zsh or god forbid tcsh). But\nI think GNU make will set SHELL=/bin/sh, rather than pick it up from the\nenvironment (probably for this exact reason).\n\n-Peff\n"},{"id":"438272","messageId":"20211008053450.wjttkhsbbsnuwwpt@gmail.com","threadId":"56626","inReplyTo":"YV+1/0b5bN3o6qRG@coredump.intra.peff.net","subject":"Re: [PATCH] t/perf: do not run tests in user's $SHELL","fromName":"Johannes Altmanninger","fromEmail":"aclopte@gmail.com","sentAt":"2021-10-08T05:34:50Z","receivedAt":"2021-10-08T05:34:57Z","isPatch":true,"sender":{"key":"aclopte@gmail.com","avatar":"https://avatars.githubusercontent.com/u/6853872?v=4"},"body":"On Thu, Oct 07, 2021 at 11:07:43PM -0400, Jeff King wrote:\n\n> I wondered if we would always have TEST_SHELL_PATH set, but we should:\n> it is put unconditionally into GIT-BUILD-OPTIONS, and we will always\n> load that via test-lib.sh, even if the test is run outside of \"make\".\n\nYeah I was gonna add this to the commit message but didn't because it's fairly\neasy to check. Probably still better to include it. I can resend if that helps.\n"},{"id":"438273","messageId":"YV/aD6NKdG+w9ghr@coredump.intra.peff.net","threadId":"56626","inReplyTo":"20211008053450.wjttkhsbbsnuwwpt@gmail.com","subject":"Re: [PATCH] t/perf: do not run tests in user's $SHELL","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-10-08T05:41:35Z","receivedAt":"2021-10-08T05:41:37Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Oct 08, 2021 at 07:34:50AM +0200, Johannes Altmanninger wrote:\n\n> On Thu, Oct 07, 2021 at 11:07:43PM -0400, Jeff King wrote:\n> \n> > I wondered if we would always have TEST_SHELL_PATH set, but we should:\n> > it is put unconditionally into GIT-BUILD-OPTIONS, and we will always\n> > load that via test-lib.sh, even if the test is run outside of \"make\".\n> \n> Yeah I was gonna add this to the commit message but didn't because it's fairly\n> easy to check. Probably still better to include it. I can resend if that helps.\n\nI think it's probably fine as-is. Mostly I was talking to myself out\nloud to let people follow along my review process. :)\n\n-Peff\n"}]}