diff mbox series

[v2] test-lib: unset trace2 parent envvars

Message ID 2b5e573c22f226cbdb07d931d470a37bca7ffe2b.1643234866.git.steadmon@google.com (mailing list archive)
State New, archived
Headers show
Series [v2] test-lib: unset trace2 parent envvars | expand

Commit Message

Josh Steadmon Jan. 26, 2022, 10:10 p.m. UTC 
The trace2 subsystem can inherit certain information from parent
processes via environment variables; e.g., the parent command name and
session ID. This allows trace2 to note when a command is the child
process of another Git process, and to adjust various pieces of output
accordingly.

This behavior breaks certain tests that examine trace2 output when the
tests run as a child of another git process, such as in `git rebase -x
"make test"`.

While we could fix this by unsetting the relevant variables in the
affected tests (currently t0210, t0211, t0212, and t6421), this would
leave other tests vulnerable to similar breakage if new test cases are
added which inspect trace2 output.

In t/test-lib.sh, we keep a pattern of permitted GIT_* environment
variables. Variables matching /^GIT_TRACE.*/ are currently allowed via
this pattern. We want to preserve this behavior, because it can be
useful to collect trace output over the entire test suite. Instead of
modifying the allow-pattern, we instead fix this issue by unsetting only
the GIT_TRACE2_PARENT_NAME and GIT_TRACE2_PARENT_SID in t/test-lib.sh.

Reported-by: Emily Shaffer <emilyshaffer@google.com>
Signed-off-by: Josh Steadmon <steadmon@google.com>
---
Updated commit message and added code comments to explain why we keep
"TRACE" in the allow pattern.

 t/test-lib.sh | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)


base-commit: dcc0cd074f0c639a0df20461a301af6d45bd582e

Comments

Ævar Arnfjörð Bjarmason Jan. 27, 2022, 3:23 a.m. UTC | #1 
On Wed, Jan 26 2022, Josh Steadmon wrote:

> The trace2 subsystem can inherit certain information from parent
> processes via environment variables; e.g., the parent command name and
> session ID. This allows trace2 to note when a command is the child
> process of another Git process, and to adjust various pieces of output
> accordingly.
>
> This behavior breaks certain tests that examine trace2 output when the
> tests run as a child of another git process, such as in `git rebase -x
> "make test"`.
>
> While we could fix this by unsetting the relevant variables in the
> affected tests (currently t0210, t0211, t0212, and t6421), this would
> leave other tests vulnerable to similar breakage if new test cases are
> added which inspect trace2 output.
>
> In t/test-lib.sh, we keep a pattern of permitted GIT_* environment
> variables. Variables matching /^GIT_TRACE.*/ are currently allowed via
> this pattern. We want to preserve this behavior, because it can be
> useful to collect trace output over the entire test suite. Instead of
> modifying the allow-pattern, we instead fix this issue by unsetting only
> the GIT_TRACE2_PARENT_NAME and GIT_TRACE2_PARENT_SID in t/test-lib.sh.
>
> Reported-by: Emily Shaffer <emilyshaffer@google.com>
> Signed-off-by: Josh Steadmon <steadmon@google.com>
> ---
> Updated commit message and added code comments to explain why we keep
> "TRACE" in the allow pattern.
>
>  t/test-lib.sh | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/t/test-lib.sh b/t/test-lib.sh
> index 0f7a137c7d..faf25ba1b2 100644
> --- a/t/test-lib.sh
> +++ b/t/test-lib.sh
> @@ -432,7 +432,7 @@ EDITOR=:
>  unset VISUAL EMAIL LANGUAGE $("$PERL_PATH" -e '
>  	my @env = keys %ENV;
>  	my $ok = join("|", qw(
> -		TRACE
> +		TRACE	# Allow tracing in general, but see unsets below.

I think it would be good to turn that -e into a -we, which would tell
you why this doesn't work the way you think:

    Possible attempt to put comments in qw() list at -e line 14.
    Possible attempt to separate words with commas at -e line 14.

I.e. you added one string "#" to the array, and a string "Allow" etc.

I think replacing it with this would be clearer, and means that...

diff --git a/t/test-lib.sh b/t/test-lib.sh
index 0f7a137c7d8..9a7611f412f 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -432,7 +432,7 @@ EDITOR=:
 unset VISUAL EMAIL LANGUAGE $("$PERL_PATH" -e '
 	my @env = keys %ENV;
 	my $ok = join("|", qw(
-		TRACE
+		TRACE(?!2_(?:PARENT_NAME|PARENT_SID)$)
 		DEBUG
 		TEST
 		.*_TEST


>  		DEBUG
>  		TEST
>  		.*_TEST
> @@ -449,6 +449,10 @@ unset VISUAL EMAIL LANGUAGE $("$PERL_PATH" -e '
>  unset XDG_CACHE_HOME
>  unset XDG_CONFIG_HOME
>  unset GITPERLLIB
> +# Unset trace environment variables that can interfere with trace output used in
> +# certain tests.
> +unset GIT_TRACE2_PARENT_NAME
> +unset GIT_TRACE2_PARENT_SID
>  TEST_AUTHOR_LOCALNAME=author
>  TEST_AUTHOR_DOMAIN=example.com
>  GIT_AUTHOR_EMAIL=${TEST_AUTHOR_LOCALNAME}@${TEST_AUTHOR_DOMAIN}
>
> base-commit: dcc0cd074f0c639a0df20461a301af6d45bd582e

...you won't need this hunk, since it'll be obvious that we're
specifically excluding those variables.

But more generally I think it's best to replace this with the below,
i.e. to also fold the nearby "unset" into this command which'll find
everything we need to unset for us, since it was doing most of the work
anyway.

diff --git a/t/test-lib.sh b/t/test-lib.sh
index 0f7a137c7d8..ff4d4a553be 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -426,13 +426,12 @@ export LANG LC_ALL PAGER TZ COLUMNS
 EDITOR=:
 
 # A call to "unset" with no arguments causes at least Solaris 10
-# /usr/xpg4/bin/sh and /bin/ksh to bail out.  So keep the unsets
-# deriving from the command substitution clustered with the other
-# ones.
-unset VISUAL EMAIL LANGUAGE $("$PERL_PATH" -e '
+# /usr/xpg4/bin/sh and /bin/ksh to bail out.  So we'll need at least
+# one unconditional variable here, which might as well be VISUAL.
+unset VISUAL $("$PERL_PATH" -we '
 	my @env = keys %ENV;
 	my $ok = join("|", qw(
-		TRACE
+		TRACE(?!2_(?:PARENT_NAME|PARENT_SID)$)
 		DEBUG
 		TEST
 		.*_TEST
@@ -444,11 +443,20 @@ unset VISUAL EMAIL LANGUAGE $("$PERL_PATH" -e '
 		TRACE_CURL
 	));
 	my @vars = grep(/^GIT_/ && !/^GIT_($ok)/o, @env);
+	my @nongit = qw(
+		CDPATH
+		EMAIL
+		GITPERLLIB
+		GREP_OPTIONS
+		LANGUAGE
+		UNZIP
+		XDG_CACHE_HOME
+		XDG_CONFIG_HOME
+	);
+	push @vars => grep { exists $ENV{$_} } @nongit;
 	print join("\n", @vars);
 ')
-unset XDG_CACHE_HOME
-unset XDG_CONFIG_HOME
-unset GITPERLLIB
+
 TEST_AUTHOR_LOCALNAME=author
 TEST_AUTHOR_DOMAIN=example.com
 GIT_AUTHOR_EMAIL=${TEST_AUTHOR_LOCALNAME}@${TEST_AUTHOR_DOMAIN}
@@ -524,13 +532,6 @@ else
 	}
 fi
 
-# Protect ourselves from common misconfiguration to export
-# CDPATH into the environment
-unset CDPATH
-
-unset GREP_OPTIONS
-unset UNZIP
-
 case $(echo $GIT_TRACE |tr "[A-Z]" "[a-z]") in
 1|2|true)
 	GIT_TRACE=4
Junio C Hamano Jan. 27, 2022, 6:29 p.m. UTC | #2 
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

>> diff --git a/t/test-lib.sh b/t/test-lib.sh
>> index 0f7a137c7d..faf25ba1b2 100644
>> --- a/t/test-lib.sh
>> +++ b/t/test-lib.sh
>> @@ -432,7 +432,7 @@ EDITOR=:
>>  unset VISUAL EMAIL LANGUAGE $("$PERL_PATH" -e '
>>  	my @env = keys %ENV;
>>  	my $ok = join("|", qw(
>> -		TRACE
>> +		TRACE	# Allow tracing in general, but see unsets below.
>
> I think it would be good to turn that -e into a -we, which would tell
> you why this doesn't work the way you think:
>
>     Possible attempt to put comments in qw() list at -e line 14.
>     Possible attempt to separate words with commas at -e line 14.
>
> I.e. you added one string "#" to the array, and a string "Allow" etc.

Having an expert who can spot these things at first glance is handy
;-).  Very much appreciated.

In any case, the previous iteration has been in 'next' long enough
without causing trouble for those who use a variant based on 'next'
at $DAYJOB, I presume, so my recommendation is to just let it
graduate first, and pursue your suggestion to do more things in this
scriptlet (below, omitted) as a "further improvement" on top.

Thanks.
Josh Steadmon Feb. 1, 2022, 9:22 p.m. UTC | #3 
On 2022.01.27 04:23, Ævar Arnfjörð Bjarmason wrote:
> 
> On Wed, Jan 26 2022, Josh Steadmon wrote:
> 
> > The trace2 subsystem can inherit certain information from parent
> > processes via environment variables; e.g., the parent command name and
> > session ID. This allows trace2 to note when a command is the child
> > process of another Git process, and to adjust various pieces of output
> > accordingly.
> >
> > This behavior breaks certain tests that examine trace2 output when the
> > tests run as a child of another git process, such as in `git rebase -x
> > "make test"`.
> >
> > While we could fix this by unsetting the relevant variables in the
> > affected tests (currently t0210, t0211, t0212, and t6421), this would
> > leave other tests vulnerable to similar breakage if new test cases are
> > added which inspect trace2 output.
> >
> > In t/test-lib.sh, we keep a pattern of permitted GIT_* environment
> > variables. Variables matching /^GIT_TRACE.*/ are currently allowed via
> > this pattern. We want to preserve this behavior, because it can be
> > useful to collect trace output over the entire test suite. Instead of
> > modifying the allow-pattern, we instead fix this issue by unsetting only
> > the GIT_TRACE2_PARENT_NAME and GIT_TRACE2_PARENT_SID in t/test-lib.sh.
> >
> > Reported-by: Emily Shaffer <emilyshaffer@google.com>
> > Signed-off-by: Josh Steadmon <steadmon@google.com>
> > ---
> > Updated commit message and added code comments to explain why we keep
> > "TRACE" in the allow pattern.
> >
> >  t/test-lib.sh | 6 +++++-
> >  1 file changed, 5 insertions(+), 1 deletion(-)
> >
> > diff --git a/t/test-lib.sh b/t/test-lib.sh
> > index 0f7a137c7d..faf25ba1b2 100644
> > --- a/t/test-lib.sh
> > +++ b/t/test-lib.sh
> > @@ -432,7 +432,7 @@ EDITOR=:
> >  unset VISUAL EMAIL LANGUAGE $("$PERL_PATH" -e '
> >  	my @env = keys %ENV;
> >  	my $ok = join("|", qw(
> > -		TRACE
> > +		TRACE	# Allow tracing in general, but see unsets below.
> 
> I think it would be good to turn that -e into a -we, which would tell
> you why this doesn't work the way you think:
> 
>     Possible attempt to put comments in qw() list at -e line 14.
>     Possible attempt to separate words with commas at -e line 14.
> 
> I.e. you added one string "#" to the array, and a string "Allow" etc.

Whoops, yeah, thanks for the catch. I've forgotten most of the Perl I
ever knew but should have seen that.
diff mbox series

Patch

diff --git a/t/test-lib.sh b/t/test-lib.sh
index 0f7a137c7d..faf25ba1b2 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -432,7 +432,7 @@  EDITOR=:
 unset VISUAL EMAIL LANGUAGE $("$PERL_PATH" -e '
 	my @env = keys %ENV;
 	my $ok = join("|", qw(
-		TRACE
+		TRACE	# Allow tracing in general, but see unsets below.
 		DEBUG
 		TEST
 		.*_TEST
@@ -449,6 +449,10 @@  unset VISUAL EMAIL LANGUAGE $("$PERL_PATH" -e '
 unset XDG_CACHE_HOME
 unset XDG_CONFIG_HOME
 unset GITPERLLIB
+# Unset trace environment variables that can interfere with trace output used in
+# certain tests.
+unset GIT_TRACE2_PARENT_NAME
+unset GIT_TRACE2_PARENT_SID
 TEST_AUTHOR_LOCALNAME=author
 TEST_AUTHOR_DOMAIN=example.com
 GIT_AUTHOR_EMAIL=${TEST_AUTHOR_LOCALNAME}@${TEST_AUTHOR_DOMAIN}