Re: [PATCH] test-lib: unset trace2 parent envvars

From: Junio C Hamano <gitster@pobox.com>
Date: Jan 14, 2022, 06:01 UTC
Message-ID: <xmqq35lqetzw.fsf@gitster.g>
In-Reply-To: <82e51a52e20fbe13a5a898a0a2f6dbe1188e3fa3.1642116539.git.steadmon@google.com>

Josh Steadmon <steadmon@google.com> writes:

> 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"`.

Well explained. The paragraph makes it clear how easy to trigger and get bitten by this problem.

Show 5 quoted lines

> 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. So fix this in general by unsetting
> GIT_TRACE2_PARENT_NAME and GIT_TRACE2_PARENT_SID in test-lib.sh.

This probably makes sense, but I wonder how it interacts with a user who runs "cd t && GIT_TRACE2=blah ./t0987-test-this.sh" to trace the entire test script, though.

Show 17 quoted lines

> Reported-by: Emily Shaffer <emilyshaffer@google.com>
> Helped-by: Jonathan Tan <jonathantanmy@google.com>
> Signed-off-by: Josh Steadmon <steadmon@google.com>
> ---
>  t/test-lib.sh | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/t/test-lib.sh b/t/test-lib.sh
> index 0f7a137c7d..e4716b0b86 100644
> --- a/t/test-lib.sh
> +++ b/t/test-lib.sh
> @@ -449,6 +449,8 @@ unset VISUAL EMAIL LANGUAGE $("$PERL_PATH" -e '
>  unset XDG_CACHE_HOME
>  unset XDG_CONFIG_HOME
>  unset GITPERLLIB
> +unset GIT_TRACE2_PARENT_NAME
> +unset GIT_TRACE2_PARENT_SID

Hmph. Have you noticed the more generic "We want to unset almost everything that begins with GIT_, other than those selected few that are designed to be used to affect the tests" above the part you touched?

I am wondering if we should tweak the list there, instead of special casing these two and these two only. There is a pattern that allows anything that match "^GIT_(other|TRACE|things)", and I suspect that the pattern is way too loose (i.e. it allows any garbage to follow, and by allowing "TRACE", it also catches "TRACE2" because the former is a prefix of the latter), which is a problem.