Re: [PATCH v2] Fix false positives in t3404 due to SHELL=/bin/false

Jonathan Nieder [off-list ref] writes:

Matthieu Moy wrote:

quoted

"Vallon, Justin" [off-list ref] writes:

quoted

quoted

--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh

@@ -71,8 +71,9 @@ test_expect_success 'setup' '
 # "exec" commands are ran with the user shell by default, but this may
 # be non-POSIX. For example, if SHELL=zsh then ">file" doesn't work
 # to create a file. Unseting SHELL avoids such non-portable behavior
-# in tests.
+# in tests. It must be exported for it to take effect where needed.
 SHELL=
+export SHELL

(my bad, I wrote this SHELL= without exporting it. Since bash
re-exports already exported variables when they are assigned, and my
/bin/sh points to bash, I didn't notice)

Isn't that how export works in all Bourne-style shells?  For example:

	$ env var=outside dash -c '
		var=inside;
		dash -c "echo \$var"
	  '
	inside
	$

Maybe in the failing case SHELL was not exported but just set to
/bin/false in .bashrc or similar?

Thanks, you saved me some time responding ;-)

Matthieu's diagnosis is only half correct in that bash is why he didn't
notice the problem, but if in this sequence

	var=foo
        export var
        var=bar
        some-command

some-command does not see "bar" as the value of environment variable
"var", your shell is not POSIX (there is no such thing as "re-exporting").

Either a variable is marked with the export attribute, in which case the
processes spawned from the shell sees the value of the then-current shell
variable in their environments, or they don't for shell variables that are
not marked with the export attribute.

The real reason the problem went unnoticed was because bash automatially
marks SHELL with the export attribute.

Because POSIX shells are required to mark variables they inherit from the
environment with the export attribute, your tests will run with SHELL
exported to the environment if your usual shell is bash (i.e. SHELL is
already exported to processes it spawns), even if you use another POSIX
shell to run your git and tests.  That makes the issue doubly harder to
notice.

-----Original Message-----
From: Junio C Hamano [mailto:gitster@pobox.com]
Sent: Tuesday, January 04, 2011 6:39 PM
To: Jonathan Nieder
Cc: Matthieu Moy; Vallon, Justin; Robin H. Johnson; git@vger.kernel.org
Subject: Re: [PATCH v2] Fix false positives in t3404 due to
SHELL=/bin/false

Jonathan Nieder [off-list ref] writes:

quoted

Matthieu Moy wrote:

quoted

(my bad, I wrote this SHELL= without exporting it. Since bash
re-exports already exported variables when they are assigned, and my
/bin/sh points to bash, I didn't notice)

Isn't that how export works in all Bourne-style shells?  For example:

	$ env var=outside dash -c '
		var=inside;
		dash -c "echo \$var"
	  '
	inside
	$

Maybe in the failing case SHELL was not exported but just set to
/bin/false in .bashrc or similar?

Thanks, you saved me some time responding ;-)

Matthieu's diagnosis is only half correct in that bash is why he didn't
notice the problem, but if in this sequence

var=foo
       export var
       var=bar
       some-command

some-command does not see "bar" as the value of environment variable
"var", your shell is not POSIX (there is no such thing as "re-exporting").

But, when you say "your shell", you are really referring to /bin/sh, because this behavior is being observed in t/t3404-rebase-interactive.sh.  Which leads to...

Robin: have you observed the problem with Gentoo's /bin/sh?

X=1 ; export X ; /bin/sh -c 'X= ; env | grep ^X='

If so, I would qualify the export with a comment about the mis-behavior:

# Reexport in case sh is non-POSIX
export SHELL

(or, just unset SHELL)

Else, I don't think the re-export is needed (something else is causing your trouble).

Because POSIX shells are required to mark variables they inherit from the
environment with the export attribute, your tests will run with SHELL
exported to the environment if your usual shell is bash (i.e. SHELL is
already exported to processes it spawns), even if you use another POSIX
shell to run your git and tests.  That makes the issue doubly harder to
notice.

I don't really follow this.  The #! line is /bin/sh.  The user's $SHELL does not come into play.  Either SHELL is in /bin/sh's environment and it should be cleared in the child, or it isn't and it won't matter.

-- 
-Justin