[PATCH] perf: fix test_export with SHELL=zsh

[PATCH] perf: fix test_export with SHELL=zsh

[René Scharfe]

Unlike other shells, zsh doesn't do word-splitting on variables.  This
is documented in https://zsh.sourceforge.io/FAQ/zshfaq03.html#31.  That
breaks the perf function test_export because it uses a space-separated
variable as a poor man's array, and as a consequence p0000 fails with
"not ok 3 - test_export works".  Pass the value through an unquoted
command substitution to force word-splitting even in zsh.

Signed-off-by: René Scharfe <l.s.r@web.de>
---
 t/perf/perf-lib.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/perf/perf-lib.sh b/t/perf/perf-lib.sh
index f5ed092ee5..f74cfd35d6 100644
--- a/t/perf/perf-lib.sh
+++ b/t/perf/perf-lib.sh
@@ -165,7 +165,7 @@ test_export () {
 '"$1"'
 ret=$?
 needles=
-for v in $test_export_
+for v in $(echo "$test_export_")
 do
 	needles="$needles;s/^$v=/export $v=/p"
 done
--
2.33.0

Re: [PATCH] perf: fix test_export with SHELL=zsh

[Johannes Altmanninger]

On Sat, Oct 02, 2021 at 11:40:02AM +0200, René Scharfe wrote:
> Pass the value through an unquoted command substitution to force
> word-splitting even in zsh.

This seems like the wrong direction.  There are probably other places where
we rely on such POSIX features.  Also it doesn't fix the case where users
use other non-POSIX shells, say

	SHELL=/bin/fish t/perf/run p0000-perf-lib-sanity.sh

We already have solutions in place - our top-level Makefile has

	# Define SHELL_PATH to a POSIX shell if your /bin/sh is broken.
	...
	ifndef SHELL_PATH
		SHELL_PATH = /bin/sh
	endif
	...
	SHELL = $(SHELL_PATH)

Meanwhile t/Makefile uses

	SHELL_PATH ?= $(SHELL)
	TEST_SHELL_PATH ?= $(SHELL_PATH)

A related fix is to make those two consistent.  I would prefer the version
in the top-level Makefile, because t/Makefile gives the false illusion that
$SHELL is honored:

	SHELL=garbage make -C t t4211-line-log.sh # succeeds because make sets SHELL=/bin/sh
	make SHELL=garbage -C t t4211-line-log.sh # fails

---

Anyway, for this issue we should just have t/perf/perf-lib.sh follow our
convention of using ${SHELL_PATH:-/bin/sh} instead of $SHELL.

Looks like t/interop/Makefile has the same problem.

I'll prepare the patches later unless someone beats me to it :)

Re: [PATCH] perf: fix test_export with SHELL=zsh

[brian m. carlson]

[-- Attachment #1: Type: text/plain, Size: 1039 bytes --]

On 2021-10-02 at 09:40:02, René Scharfe wrote:
> Unlike other shells, zsh doesn't do word-splitting on variables.  This
> is documented in https://zsh.sourceforge.io/FAQ/zshfaq03.html#31.  That
> breaks the perf function test_export because it uses a space-separated
> variable as a poor man's array, and as a consequence p0000 fails with
> "not ok 3 - test_export works".  Pass the value through an unquoted
> command substitution to force word-splitting even in zsh.

There are a variety of places in our testsuite where zsh is broken in
zsh mode.  I recently sent a patch to make it do the right thing for
subshells in sh mode which has not yet been released, and as far as I'm
aware, with that patch, our testsuite is fully functional with zsh when
it's run in sh mode.

Note that in sh mode, zsh enables the SH_WORD_SPLIT option, and this
should work just fine.  The easiest way to do that is to create a
symlink to zsh called "sh" and invoke that.
-- 
brian m. carlson (he/him or they/them)
Toronto, Ontario, CA

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 262 bytes --]

[PATCH] t/perf: do not run tests in user's $SHELL

[Johannes Altmanninger]

The environment variable $SHELL is usually set to the user's
interactive shell. We never use that shell for build and test scripts
because it might not be a POSIX shell.

Perf tests are run inside $SHELL via a wrapper defined in
t/perf/perf-lib.sh. Use $TEST_SHELL_PATH like elsewhere.

Signed-off-by: Johannes Altmanninger <aclopte@gmail.com>
---

Regarding the inconsistency around $(SHELL) in Makefiles: we could do
something like

	-SHELL_PATH ?= $(SHELL)
	+SHELL_PATH ?= /bin/sh
	+SHELL = $(SHELL_PATH)

in some Makefiles. Though the upside (consistency & slightly easier to build
with broken /bin/sh) seems fairly low, so I'd leave it be.

 t/perf/perf-lib.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/perf/perf-lib.sh b/t/perf/perf-lib.sh
index f5ed092ee5..e6b59ecbf7 100644
--- a/t/perf/perf-lib.sh
+++ b/t/perf/perf-lib.sh
@@ -157,7 +157,7 @@ test_run_perf_ () {
 	test_cleanup=:
 	test_export_="test_cleanup"
 	export test_cleanup test_export_
-	"$GTIME" -f "%E %U %S" -o test_time.$i "$SHELL" -c '
+	"$GTIME" -f "%E %U %S" -o test_time.$i "$TEST_SHELL_PATH" -c '
 . '"$TEST_DIRECTORY"/test-lib-functions.sh'
 test_export () {
 	test_export_="$test_export_ $*"
-- 
2.33.0.rc2.dirty

Re: [PATCH] t/perf: do not run tests in user's $SHELL

[Jeff King]

On Thu, Oct 07, 2021 at 08:47:16PM +0200, Johannes Altmanninger wrote:

> The environment variable $SHELL is usually set to the user's
> interactive shell. We never use that shell for build and test scripts
> because it might not be a POSIX shell.
> 
> Perf tests are run inside $SHELL via a wrapper defined in
> t/perf/perf-lib.sh. Use $TEST_SHELL_PATH like elsewhere.

Yes, I think this is the right thing to do. We didn't have
$TEST_SHELL_PATH back when this code was added, but I think it should
have been $SHELL_PATH from the start.

I wondered if we would always have TEST_SHELL_PATH set, but we should:
it is put unconditionally into GIT-BUILD-OPTIONS, and we will always
load that via test-lib.sh, even if the test is run outside of "make".

> ---
> 
> Regarding the inconsistency around $(SHELL) in Makefiles: we could do
> something like
> 
> 	-SHELL_PATH ?= $(SHELL)
> 	+SHELL_PATH ?= /bin/sh
> 	+SHELL = $(SHELL_PATH)
> 
> in some Makefiles. Though the upside (consistency & slightly easier to build
> with broken /bin/sh) seems fairly low, so I'd leave it be.

In general assuming that $SHELL is a valid /bin/sh replacement is
questionable (e.g., if your login shell is zsh or god forbid tcsh). But
I think GNU make will set SHELL=/bin/sh, rather than pick it up from the
environment (probably for this exact reason).

-Peff

Re: [PATCH] t/perf: do not run tests in user's $SHELL

[Johannes Altmanninger]

On Thu, Oct 07, 2021 at 11:07:43PM -0400, Jeff King wrote:

> I wondered if we would always have TEST_SHELL_PATH set, but we should:
> it is put unconditionally into GIT-BUILD-OPTIONS, and we will always
> load that via test-lib.sh, even if the test is run outside of "make".

Yeah I was gonna add this to the commit message but didn't because it's fairly
easy to check. Probably still better to include it. I can resend if that helps.

Re: [PATCH] t/perf: do not run tests in user's $SHELL

[Jeff King]

On Fri, Oct 08, 2021 at 07:34:50AM +0200, Johannes Altmanninger wrote:

> On Thu, Oct 07, 2021 at 11:07:43PM -0400, Jeff King wrote:
> 
> > I wondered if we would always have TEST_SHELL_PATH set, but we should:
> > it is put unconditionally into GIT-BUILD-OPTIONS, and we will always
> > load that via test-lib.sh, even if the test is run outside of "make".
> 
> Yeah I was gonna add this to the commit message but didn't because it's fairly
> easy to check. Probably still better to include it. I can resend if that helps.

I think it's probably fine as-is. Mostly I was talking to myself out
loud to let people follow along my review process. :)

-Peff