patch, 2 partsSet GIT_TRACE2_EVENT_NESTING in test-lib.sh

> From: Derrick Stolee <dstolee@microsoft.com>
>
> The GIT_TRACE2_EVENT feed has a limited nesting depth to avoid
> overloading the feed when recursing into deep paths while adding more
> nested regions.
>
> Some tests use the GIT_TRACE2_EVENT feed to look for internal events,
> ensuring that intended behavior is happening.
>
> One such example is in t4216-log-bloom.sh which looks for a statistic
> given as a trace2_data_intmax() call. This test started failing under
> '-x' with 2ca245f8be5 (csum-file.h: increase hashfile buffer size,
> 2021-05-18) because the change in stderr triggered the progress API to
> create an extra trace2 region, ejecting the statistic.
>
> This change increases the value of GIT_TRACE2_EVENT_NESTING across the
> entire test suite to avoid errors like this. Future changes will remove
> custom assignments of GIT_TRACE2_EVENT_NESTING from some test scripts
> that were aware of this limitation.
>
> Reported-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> Helped-by: Jeff King <peff@peff.net>
> Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
> ---
>  t/test-lib.sh | 7 +++++++
>  1 file changed, 7 insertions(+)
>
> diff --git a/t/test-lib.sh b/t/test-lib.sh
> index 2679a7596a6..2e815c41c8f 100644
> --- a/t/test-lib.sh
> +++ b/t/test-lib.sh
> @@ -476,6 +476,13 @@ export GIT_TEST_MERGE_ALGORITHM
>  GIT_TRACE_BARE=1
>  export GIT_TRACE_BARE
>  
> +# Some tests scan the GIT_TRACE2_EVENT feed for events, but the
> +# default depth is 2, which frequently causes issues when the
> +# events are wrapped in new regions. Set it to a sufficiently
> +# large depth to avoid custom changes in the test suite.
> +GIT_TRACE2_EVENT_NESTING=100
> +export GIT_TRACE2_EVENT_NESTING
> +

> Thanks for following up on this.
>
> Rather than hardcoding 100 wouldn't it make sense to have something like
> the below (which I barely checked for whether it compiled or not, just
> to check how hard it was to change), so that we can just set this to a
> "false" value to disable the nesting guard entirely?

>  trace2.eventNesting::
> -	Integer.  Specifies desired depth of nested regions in the
> +	Integer or "false" boolean.  Specifies desired depth of nested regions in the
>  	event output.  Regions deeper than this value will be
>  	omitted.  May be overridden by the `GIT_TRACE2_EVENT_NESTING`
>  	environment variable.  Defaults to 2.
> ++
> +Set this to a "false" value (see linkgit:git-config[1] for accepted
> +values, i.e. "false", "off" etc.) to remove the limitation on nesting
> +entirely.

> As reported by Ævar [1] and diagnosed by Peff in a reply, the default
> GIT_TRACE2_EVENT_NESTING is set so low that tests that look for trace2
> events can start failing due to an added trace2 region. This can even be
> subtle, including the fact that the progress API adds trace2 regions when in
> use (which can depend on the verbose output of a test script).

> On Mon, Nov 29, 2021 at 01:47:44PM +0000, Derrick Stolee via GitGitGadget wrote:
> 
> > As reported by Ævar [1] and diagnosed by Peff in a reply, the default
> > GIT_TRACE2_EVENT_NESTING is set so low that tests that look for trace2
> > events can start failing due to an added trace2 region. This can even be
> > subtle, including the fact that the progress API adds trace2 regions when in
> > use (which can depend on the verbose output of a test script).
> 
> I think this is a good change for fixing the immediate problem of the
> test suite failing.
> 
> But I have to wonder if this is masking a problem that real users will
> see. Aren't we silently discarding trace2 output that could be useful to
> somebody debugging or trying to get performance metrics?
> 
> I.e., should we be bumping the internal nesting max of 2 to something
> higher? If that would that cause problems with existing metrics, should
> we be designing new metrics to avoid nesting?

> On Mon, Nov 29, 2021 at 01:40:00PM -0500, Jeff King wrote:
>
> > On Mon, Nov 29, 2021 at 01:47:44PM +0000, Derrick Stolee via GitGitGadget wrote:
> >
> > > As reported by Ævar [1] and diagnosed by Peff in a reply, the default
> > > GIT_TRACE2_EVENT_NESTING is set so low that tests that look for trace2
> > > events can start failing due to an added trace2 region. This can even be
> > > subtle, including the fact that the progress API adds trace2 regions when in
> > > use (which can depend on the verbose output of a test script).
> >
> > I think this is a good change for fixing the immediate problem of the
> > test suite failing.
> >
> > But I have to wonder if this is masking a problem that real users will
> > see. Aren't we silently discarding trace2 output that could be useful to
> > somebody debugging or trying to get performance metrics?
> >
> > I.e., should we be bumping the internal nesting max of 2 to something
> > higher? If that would that cause problems with existing metrics, should
> > we be designing new metrics to avoid nesting?
>
> One thing I should have added to the first paragraph: the patches
> themselves look fine and I'm OK with this as a band-aid in the short
> term.

> On Mon, Nov 29, 2021 at 01:47:44PM +0000, Derrick Stolee via GitGitGadget wrote:
> 
>> As reported by Ævar [1] and diagnosed by Peff in a reply, the default
>> GIT_TRACE2_EVENT_NESTING is set so low that tests that look for trace2
>> events can start failing due to an added trace2 region. This can even be
>> subtle, including the fact that the progress API adds trace2 regions when in
>> use (which can depend on the verbose output of a test script).
> 
> I think this is a good change for fixing the immediate problem of the
> test suite failing.
> 
> But I have to wonder if this is masking a problem that real users will
> see. Aren't we silently discarding trace2 output that could be useful to
> somebody debugging or trying to get performance metrics?
> 
> I.e., should we be bumping the internal nesting max of 2 to something
> higher? If that would that cause problems with existing metrics, should
> we be designing new metrics to avoid nesting?

> On 11/29/2021 1:40 PM, Jeff King wrote:
>> On Mon, Nov 29, 2021 at 01:47:44PM +0000, Derrick Stolee via GitGitGadget wrote:
>>
>>> As reported by Ævar [1] and diagnosed by Peff in a reply, the default
>>> GIT_TRACE2_EVENT_NESTING is set so low that tests that look for trace2
>>> events can start failing due to an added trace2 region. This can even be
>>> subtle, including the fact that the progress API adds trace2 regions when in
>>> use (which can depend on the verbose output of a test script).
>>
>> I think this is a good change for fixing the immediate problem of the
>> test suite failing.
>>
>> But I have to wonder if this is masking a problem that real users will
>> see. Aren't we silently discarding trace2 output that could be useful to
>> somebody debugging or trying to get performance metrics?
>>
>> I.e., should we be bumping the internal nesting max of 2 to something
>> higher? If that would that cause problems with existing metrics, should
>> we be designing new metrics to avoid nesting?
> 
> One thing this makes me think about is that we might want to
> have items such as trace2_data_intmax() and trace2_data_string()
> not be subject to the nesting limit. Only have the nesting apply
> to the regions (which nest).
> 
> CC'ing Jeff Hostetler for his thoughts on that idea. We will have
> a discussion offline to see if the progress regions have started
> to cause inconsistencies in our existing telemetry stream of
> internal users.
> 
> Thanks,
> -Stolee
> 

> and yes, the addition of trace2 calls inside the progress code
> complicates the nesting.  I'm wondering if the progress code should
> always emit the trace2 events, regardless of whether the progress msg
> itself appears on the user's console.  That way we have consistent
> logging -- the whole point of adding the region in the progress
> code was to bracket the time spent doing the thing that needed
> progress messaging after all.  the fact that you don't have a console
> doesn't change the fact that there's an expensive computation there.

> Change the "t1510-repo-setup.sh" test to use a new
> "test_must_be_empty_trace" wrapper, instead of turning on
> "test_untraceable=UnfortunatelyYes".
>
> The only reason the test was incompatible with "-x" was because of
> these "test_must_be_empty" checks, which we can instead instead skip
> if we're running under "set -x".

> Skipping the tests is preferable to not having the "-x" output at all,
> as it's much easier to debug the test. The result loss of test
> coverage is minimal. If someone is adjusting a "test_must_be_empty"
> test a failure might go away under "-x", but the new "say" we emit
> here should highlight that appropriately.
>
> Since the only user of "test_untraceable" is gone, we can remove that,
> not only isn't it used now, but I think the rationale for using it in
> the future no longer applies.
>
> We'll be better off by using a simple wrapper like the new
> "test_must_be_empty_trace". See 58275069288 (t1510-repo-setup: mark as
> untraceable with '-x', 2018-02-24) and 5fc98e79fc0 (t: add means to
> disable '-x' tracing for individual test scripts, 2018-02-24) for the
> addition of "test_untraceable".
>
> Once that's been removed we can dig deeper and see that we only have
> "BASH_XTRACEFD" due to an earlier attempt to work around the same
> issue. See d88785e424a (test-lib: set BASH_XTRACEFD automatically,
> 2016-05-11) and the 90c8a1db9d6 (test-lib: silence "-x" cleanup under
> bash, 2017-12-08) follow-up.
>
> I.e. we had redirection in "test_eval_" to get more relevant trace
> output under bash, which in turn was only needed because
> BASH_XTRACEFD=1 was set, which in turn was trying to work around test
> failures under "set -x".
>
> It's better if our test suite works the same way on all shells, rather
> than getting a passing run under "bash", only to have it fail with
> "-x" under say "dash". As the deleted code shows this is much simpler
> to implement across our supported POSIX shells.
>
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>

> Once that's been removed we can dig deeper and see that we only have
> "BASH_XTRACEFD" due to an earlier attempt to work around the same
> issue. See d88785e424a (test-lib: set BASH_XTRACEFD automatically,
> 2016-05-11) and the 90c8a1db9d6 (test-lib: silence "-x" cleanup under
> bash, 2017-12-08) follow-up.
>
> I.e. we had redirection in "test_eval_" to get more relevant trace
> output under bash, which in turn was only needed because
> BASH_XTRACEFD=1 was set, which in turn was trying to work around test
> failures under "set -x".
> 
> It's better if our test suite works the same way on all shells, rather
> than getting a passing run under "bash", only to have it fail with
> "-x" under say "dash". As the deleted code shows this is much simpler
> to implement across our supported POSIX shells.

> On Mon, Nov 29, 2021 at 09:13:23PM +0100, Ævar Arnfjörð Bjarmason wrote:
>
>> Once that's been removed we can dig deeper and see that we only have
>> "BASH_XTRACEFD" due to an earlier attempt to work around the same
>> issue. See d88785e424a (test-lib: set BASH_XTRACEFD automatically,
>> 2016-05-11) and the 90c8a1db9d6 (test-lib: silence "-x" cleanup under
>> bash, 2017-12-08) follow-up.
>>
>> I.e. we had redirection in "test_eval_" to get more relevant trace
>> output under bash, which in turn was only needed because
>> BASH_XTRACEFD=1 was set, which in turn was trying to work around test
>> failures under "set -x".
>> 
>> It's better if our test suite works the same way on all shells, rather
>> than getting a passing run under "bash", only to have it fail with
>> "-x" under say "dash". As the deleted code shows this is much simpler
>> to implement across our supported POSIX shells.
>
> I'm mildly negative on dropping BASH_XTRACEFD. IMHO it is not worth the
> maintenance headache to have to remain vigilant against any shell
> function's stderr being examined, when there is single-line solution
> that fixes everything. Yes, the cost of using bash is high on some
> platforms, but "-x" is an optional feature (though I am sympathetic to
> people who are _surprised_ when "-x" breaks things, because it really is
> a subtle thing, and knowing "you should try using bash" is not
> immediately obvious).
>
> Some folks (like Gábor) disagree and have done the work so far to make
> sure that we pass even with "-x". But it feels like this is committing
> the whole project to that maintenance. I dunno.

> On Mon, Nov 29, 2021 at 09:13:23PM +0100, Ævar Arnfjörð Bjarmason wrote:
> 
> > Once that's been removed we can dig deeper and see that we only have
> > "BASH_XTRACEFD" due to an earlier attempt to work around the same
> > issue. See d88785e424a (test-lib: set BASH_XTRACEFD automatically,
> > 2016-05-11) and the 90c8a1db9d6 (test-lib: silence "-x" cleanup under
> > bash, 2017-12-08) follow-up.
> >
> > I.e. we had redirection in "test_eval_" to get more relevant trace
> > output under bash, which in turn was only needed because
> > BASH_XTRACEFD=1 was set, which in turn was trying to work around test
> > failures under "set -x".
> > 
> > It's better if our test suite works the same way on all shells, rather
> > than getting a passing run under "bash", only to have it fail with
> > "-x" under say "dash". As the deleted code shows this is much simpler
> > to implement across our supported POSIX shells.
> 
> I'm mildly negative on dropping BASH_XTRACEFD.

> IMHO it is not worth the
> maintenance headache to have to remain vigilant against any shell
> function's stderr being examined, when there is single-line solution
> that fixes everything. Yes, the cost of using bash is high on some
> platforms, but "-x" is an optional feature (though I am sympathetic to
> people who are _surprised_ when "-x" breaks things, because it really is
> a subtle thing, and knowing "you should try using bash" is not
> immediately obvious).
> 
> Some folks (like Gábor) disagree and have done the work so far to make
> sure that we pass even with "-x". But it feels like this is committing
> the whole project to that maintenance. I dunno.

> On Tue, Nov 30, 2021 at 04:08:48PM -0500, Jeff King wrote:
>> On Mon, Nov 29, 2021 at 09:13:23PM +0100, Ævar Arnfjörð Bjarmason wrote:
>> 
>> > Once that's been removed we can dig deeper and see that we only have
>> > "BASH_XTRACEFD" due to an earlier attempt to work around the same
>> > issue. See d88785e424a (test-lib: set BASH_XTRACEFD automatically,
>> > 2016-05-11) and the 90c8a1db9d6 (test-lib: silence "-x" cleanup under
>> > bash, 2017-12-08) follow-up.
>> >
>> > I.e. we had redirection in "test_eval_" to get more relevant trace
>> > output under bash, which in turn was only needed because
>> > BASH_XTRACEFD=1 was set, which in turn was trying to work around test
>> > failures under "set -x".
>> > 
>> > It's better if our test suite works the same way on all shells, rather
>> > than getting a passing run under "bash", only to have it fail with
>> > "-x" under say "dash". As the deleted code shows this is much simpler
>> > to implement across our supported POSIX shells.
>> 
>> I'm mildly negative on dropping BASH_XTRACEFD.
>
> I agree, using BASH_XTRACEFD is the most robust (and least hacky) way
> to get reliable trace from our test scripts, and we should definitely
> keep it.
>
>> IMHO it is not worth the
>> maintenance headache to have to remain vigilant against any shell
>> function's stderr being examined, when there is single-line solution
>> that fixes everything. Yes, the cost of using bash is high on some
>> platforms, but "-x" is an optional feature (though I am sympathetic to
>> people who are _surprised_ when "-x" breaks things, because it really is
>> a subtle thing, and knowing "you should try using bash" is not
>> immediately obvious).
>> 
>> Some folks (like Gábor) disagree and have done the work so far to make
>> sure that we pass even with "-x". But it feels like this is committing
>> the whole project to that maintenance. I dunno.
>
> It's not that I disagree but rather it's in our best interest to keep
> '-x' working with non-Bash shells as well, because:
>
>   - We run our tests with '-x' in CI, because we want to get as much
>     information out of test failures as possible.
>
>   - We run our tests with dash in the Ubuntu jobs not only because
>     that's the default /bin/sh, but more importantly because we want
>     to avoid bashisms in our test suite.

> I don't really mind keeping BASH_XTRACEFD if it's doing something
> useful, but I feel like I'm missing something here. Is it really doing
> something useful?
> 
> AFAICT the ony case where it mattered was t1510-repo-setup.sh, which
> with my upthread
> <patch-1.1-9f735bd0d49-20211129T200950Z-avarab@gmail.com> now works with
> -x, at the trivial cost of skipping a small bi of the test with -x.
> 
> I suppose we could move this BASH_XTRACEFD to tht file in particular if
> anyone feel strongly about the trivial loss of tracing that entails. I
> figured just skipping it under "-x" and adding a "say" to that effect
> was a better trade-off.
> 
> For the rest of the test suite BASH_XTRACEFD effectively didn't matter,
> since all our tests had to work under --verbose-log -x anyway under
> dash.
> 
> Am I just wrong about this line of thinking, or is it purely that you
> two would like to keep BASH_XTRACEFD in case some hypothetical future
> caller wants to make use of "test_untraceable=UnfortunatelyYes" again?

> Change the "t1510-repo-setup.sh" test to use a new
> "test_must_be_empty_trace" wrapper, instead of turning on
> "test_untraceable=UnfortunatelyYes".
>
> The only reason the test was incompatible with "-x" was because of
> these "test_must_be_empty" checks, which we can instead instead skip
> if we're running under "set -x".
>
> Skipping the tests is preferable to not having the "-x" output at all,

> Once that's been removed we can dig deeper and see that we only have
> "BASH_XTRACEFD" due to an earlier attempt to work around the same
> issue. See d88785e424a (test-lib: set BASH_XTRACEFD automatically,
> 2016-05-11) and the 90c8a1db9d6 (test-lib: silence "-x" cleanup under
> bash, 2017-12-08) follow-up.

> Amend the tests checking whether stderr is empty added in
> 4868b2ea17b (Subject: setup: officially support --work-tree without
> --git-dir, 2011-01-19) work portably on all POSIX shells, instead
> suppressing the trace output with "test_untraceable" on shells that
> aren't bash.
> 
> The tests that used the "try_repo" helper wanted to check whether git
> commands invoked therein would emit anything on stderr. To do this
> they invoked the function and redirected the stderr to a "message"
> file.
> 
> In 58275069288 (t1510-repo-setup: mark as untraceable with '-x',
> 2018-02-24) these were made to use "test_untraceable" introduced in
> 5fc98e79fc0 (t: add means to disable '-x' tracing for individual test
> scripts, 2018-02-24).
> 
> It is better to have the "try_repo" function itself start with a
> "test_when_finished 'rm stderr'", and then redirect the stderr output
> from git commands it invokes via its helpers to a "stderr" file.
> 
> This means that if we have a failure it'll be closer to the source of
> the problem, and most importantly isn't incompatible with "-x" on
> shells that aren't "bash".
> 
> We also need to split those tests that had two "try_repo" invocations
> into different tests, which'll further help to narrow down any
> potential failures. This wasn't strictly necessary (an artifact of the
> use of "test_when_finished"), but the pattern enforces better test
> hygiene.
> 
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
>  t/t1510-repo-setup.sh | 83 +++++++++++++++++++++----------------------
>  1 file changed, 40 insertions(+), 43 deletions(-)
> 
> diff --git a/t/t1510-repo-setup.sh b/t/t1510-repo-setup.sh
> index 591505a39c0..f1748ac4a19 100755
> --- a/t/t1510-repo-setup.sh
> +++ b/t/t1510-repo-setup.sh
> @@ -40,9 +40,6 @@ A few rules for repo setup:
>      prefix is NULL.
>  "
>  
> -# This test heavily relies on the standard error of nested function calls.
> -test_untraceable=UnfortunatelyYes
> -
>  TEST_PASSES_SANITIZE_LEAK=true
>  . ./test-lib.sh
>  
> @@ -62,7 +59,7 @@ test_repo () {
>  			export GIT_WORK_TREE
>  		fi &&
>  		rm -f trace &&
> -		GIT_TRACE_SETUP="$(pwd)/trace" git symbolic-ref HEAD >/dev/null &&
> +		GIT_TRACE_SETUP="$(pwd)/trace" git symbolic-ref HEAD >/dev/null 2>>stderr &&

>  		grep '^setup: ' trace >result &&
>  		test_cmp expected result
>  	)
> @@ -72,7 +69,7 @@ maybe_config () {
>  	file=$1 var=$2 value=$3 &&
>  	if test "$value" != unset
>  	then
> -		git config --file="$file" "$var" "$value"
> +		git config --file="$file" "$var" "$value" 2>>stderr
>  	fi
>  }
>  
> @@ -80,7 +77,7 @@ setup_repo () {
>  	name=$1 worktreecfg=$2 gitfile=$3 barecfg=$4 &&
>  	sane_unset GIT_DIR GIT_WORK_TREE &&
>  
> -	git -c init.defaultBranch=initial init "$name" &&
> +	git -c init.defaultBranch=initial init "$name" 2>>stderr &&
>  	maybe_config "$name/.git/config" core.worktree "$worktreecfg" &&
>  	maybe_config "$name/.git/config" core.bare "$barecfg" &&
>  	mkdir -p "$name/sub/sub" &&
> @@ -210,10 +207,12 @@ run_wt_tests () {
>  #	(git dir) (work tree) (cwd) (prefix)	<-- from subdir
>  try_repo () {
>  	name=$1 worktreeenv=$2 gitdirenv=$3 &&
> +	test_when_finished "rm stderr" &&
>  	setup_repo "$name" "$4" "$5" "$6" &&
>  	shift 6 &&
>  	try_case "$name" "$worktreeenv" "$gitdirenv" \
>  		"$1" "$2" "$3" "$4" &&
> +	test_must_be_empty stderr &&
>  	shift 4 &&
>  	case "$gitdirenv" in
>  	/* | ?:/* | unset) ;;
> @@ -221,7 +220,8 @@ try_repo () {
>  		gitdirenv=../$gitdirenv ;;
>  	esac &&
>  	try_case "$name/sub" "$worktreeenv" "$gitdirenv" \
> -		"$1" "$2" "$3" "$4"
> +		"$1" "$2" "$3" "$4" &&
> +	test_must_be_empty stderr
>  }
>  
>  # Bit 0 = GIT_WORK_TREE
> @@ -234,15 +234,13 @@ try_repo () {
>  test_expect_success '#0: nonbare repo, no explicit configuration' '
>  	try_repo 0 unset unset unset "" unset \
>  		.git "$here/0" "$here/0" "(null)" \
> -		.git "$here/0" "$here/0" sub/ 2>message &&
> -	test_must_be_empty message
> +		.git "$here/0" "$here/0" sub/
>  '
>  
>  test_expect_success '#1: GIT_WORK_TREE without explicit GIT_DIR is accepted' '
>  	try_repo 1 "$here" unset unset "" unset \
>  		"$here/1/.git" "$here" "$here" 1/ \
> -		"$here/1/.git" "$here" "$here" 1/sub/ 2>message &&
> -	test_must_be_empty message
> +		"$here/1/.git" "$here" "$here" 1/sub/
>  '
>  
>  test_expect_success '#2: worktree defaults to cwd with explicit GIT_DIR' '
> @@ -268,19 +266,20 @@ test_expect_success '#4: core.worktree without GIT_DIR set is accepted' '
>  	mkdir -p 4/sub sub &&
>  	try_case 4 unset unset \
>  		.git "$here/4/sub" "$here/4" "(null)" \
> -		"$here/4/.git" "$here/4/sub" "$here/4/sub" "(null)" 2>message &&
> -	test_must_be_empty message
> +		"$here/4/.git" "$here/4/sub" "$here/4/sub" "(null)"
>  '
>  
> -test_expect_success '#5: core.worktree + GIT_WORK_TREE is accepted' '
> +test_expect_success '#5.1: core.worktree + GIT_WORK_TREE is accepted' '
>  	# or: you cannot intimidate away the lack of GIT_DIR setting
>  	try_repo 5 "$here" unset "$here/5" "" unset \
>  		"$here/5/.git" "$here" "$here" 5/ \
> -		"$here/5/.git" "$here" "$here" 5/sub/ 2>message &&
> +		"$here/5/.git" "$here" "$here" 5/sub/
> +'
> +
> +test_expect_success '#5.2: core.worktree + GIT_WORK_TREE is accepted' '
>  	try_repo 5a .. unset "$here/5a" "" unset \
>  		"$here/5a/.git" "$here" "$here" 5a/ \
> -		"$here/5a/.git" "$here/5a" "$here/5a" sub/ &&
> -	test_must_be_empty message
> +		"$here/5a/.git" "$here/5a" "$here/5a" sub/
>  '
>  
>  test_expect_success '#6: setting GIT_DIR brings core.worktree to life' '
> @@ -376,8 +375,7 @@ test_expect_success '#9: GIT_WORK_TREE accepted with gitfile' '
>  	mkdir -p 9/wt &&
>  	try_repo 9 wt unset unset gitfile unset \
>  		"$here/9.git" "$here/9/wt" "$here/9" "(null)" \
> -		"$here/9.git" "$here/9/sub/wt" "$here/9/sub" "(null)" 2>message &&
> -	test_must_be_empty message
> +		"$here/9.git" "$here/9/sub/wt" "$here/9/sub" "(null)"
>  '
>  
>  test_expect_success '#10: GIT_DIR can point to gitfile' '
> @@ -402,16 +400,14 @@ run_wt_tests 11 gitfile
>  test_expect_success '#12: core.worktree with gitfile is accepted' '
>  	try_repo 12 unset unset "$here/12" gitfile unset \
>  		"$here/12.git" "$here/12" "$here/12" "(null)" \
> -		"$here/12.git" "$here/12" "$here/12" sub/ 2>message &&
> -	test_must_be_empty message
> +		"$here/12.git" "$here/12" "$here/12" sub/
>  '
>  
>  test_expect_success '#13: core.worktree+GIT_WORK_TREE accepted (with gitfile)' '
>  	# or: you cannot intimidate away the lack of GIT_DIR setting
>  	try_repo 13 non-existent-too unset non-existent gitfile unset \
>  		"$here/13.git" "$here/13/non-existent-too" "$here/13" "(null)" \
> -		"$here/13.git" "$here/13/sub/non-existent-too" "$here/13/sub" "(null)" 2>message &&
> -	test_must_be_empty message
> +		"$here/13.git" "$here/13/sub/non-existent-too" "$here/13/sub" "(null)"
>  '
>  
>  # case #14.
> @@ -549,8 +545,7 @@ test_expect_success '#17: GIT_WORK_TREE without explicit GIT_DIR is accepted (ba
>  	mkdir -p 17b/.git/wt/sub &&
>  
>  	try_case 17a/.git "$here/17a" unset \
> -		"$here/17a/.git" "$here/17a" "$here/17a" .git/ \
> -		2>message &&
> +		"$here/17a/.git" "$here/17a" "$here/17a" .git/ &&
>  	try_case 17a/.git/wt "$here/17a" unset \
>  		"$here/17a/.git" "$here/17a" "$here/17a" .git/wt/ &&
>  	try_case 17a/.git/wt/sub "$here/17a" unset \
> @@ -565,14 +560,16 @@ test_expect_success '#17: GIT_WORK_TREE without explicit GIT_DIR is accepted (ba
>  
>  	try_repo 17c "$here/17c" unset unset "" true \
>  		.git "$here/17c" "$here/17c" "(null)" \
> -		"$here/17c/.git" "$here/17c" "$here/17c" sub/ 2>message &&
> -	test_must_be_empty message
> +		"$here/17c/.git" "$here/17c" "$here/17c" sub/
>  '
>  
> -test_expect_success '#18: bare .git named by GIT_DIR has no worktree' '
> +test_expect_success '#18.1: bare .git named by GIT_DIR has no worktree' '
>  	try_repo 18 unset .git unset "" true \
>  		.git "(null)" "$here/18" "(null)" \
> -		../.git "(null)" "$here/18/sub" "(null)" &&
> +		../.git "(null)" "$here/18/sub" "(null)"
> +'
> +
> +test_expect_success '#18.2: bare .git named by GIT_DIR has no worktree' '
>  	try_repo 18b unset "$here/18b/.git" unset "" true \
>  		"$here/18b/.git" "(null)" "$here/18b" "(null)" \
>  		"$here/18b/.git" "(null)" "$here/18b/sub" "(null)"
> @@ -590,12 +587,11 @@ test_expect_success '#20a: core.worktree without GIT_DIR accepted (inside .git)'
>  	setup_repo 20a "$here/20a" "" unset &&
>  	mkdir -p 20a/.git/wt/sub &&
>  	try_case 20a/.git unset unset \
> -		"$here/20a/.git" "$here/20a" "$here/20a" .git/ 2>message &&
> +		"$here/20a/.git" "$here/20a" "$here/20a" .git/ &&
>  	try_case 20a/.git/wt unset unset \
>  		"$here/20a/.git" "$here/20a" "$here/20a" .git/wt/ &&
>  	try_case 20a/.git/wt/sub unset unset \
> -		"$here/20a/.git" "$here/20a" "$here/20a" .git/wt/sub/ &&
> -	test_must_be_empty message
> +		"$here/20a/.git" "$here/20a" "$here/20a" .git/wt/sub/
>  '
>  
>  test_expect_success '#20b/c: core.worktree and core.bare conflict' '
> @@ -625,10 +621,9 @@ test_expect_success '#21: setup, core.worktree warns before overriding core.bare
>  		cd 21/.git &&
>  		GIT_WORK_TREE="$here/21" &&
>  		export GIT_WORK_TREE &&
> -		git status >/dev/null
> -	) 2>message &&
> -	test_must_be_empty message
> -
> +		git status 2>message &&
> +		test_must_be_empty message
> +	)
>  '
>  run_wt_tests 21
>  
> @@ -742,14 +737,16 @@ test_expect_success '#24: bare repo has no worktree (gitfile case)' '
>  test_expect_success '#25: GIT_WORK_TREE accepted if GIT_DIR unset (bare gitfile case)' '
>  	try_repo 25 "$here/25" unset unset gitfile true \
>  		"$here/25.git" "$here/25" "$here/25" "(null)"  \
> -		"$here/25.git" "$here/25" "$here/25" "sub/" 2>message &&
> -	test_must_be_empty message
> +		"$here/25.git" "$here/25" "$here/25" "sub/"
>  '
>  
> -test_expect_success '#26: bare repo has no worktree (GIT_DIR -> gitfile case)' '
> +test_expect_success '#26.1: bare repo has no worktree (GIT_DIR -> gitfile case)' '
>  	try_repo 26 unset "$here/26/.git" unset gitfile true \
>  		"$here/26.git" "(null)" "$here/26" "(null)" \
> -		"$here/26.git" "(null)" "$here/26/sub" "(null)" &&
> +		"$here/26.git" "(null)" "$here/26/sub" "(null)"
> +'
> +
> +test_expect_success '#26.2: bare repo has no worktree (GIT_DIR -> gitfile case)' '
>  	try_repo 26b unset .git unset gitfile true \
>  		"$here/26b.git" "(null)" "$here/26b" "(null)" \
>  		"$here/26b.git" "(null)" "$here/26b/sub" "(null)"
> @@ -779,9 +776,9 @@ test_expect_success '#29: setup' '
>  		cd 29 &&
>  		GIT_WORK_TREE="$here/29" &&
>  		export GIT_WORK_TREE &&
> -		git status
> -	) 2>message &&
> -	test_must_be_empty message
> +		git status 2>message &&
> +		test_must_be_empty message
> +	)
>  '
>  run_wt_tests 29 gitfile
>  
> -- 
> 2.34.1.876.gdb91009a90c
> 

> On Wed, Dec 01, 2021 at 09:11:41PM +0100, Ævar Arnfjörð Bjarmason wrote:
> > Amend the tests checking whether stderr is empty added in
> > 4868b2ea17b (Subject: setup: officially support --work-tree without
> > --git-dir, 2011-01-19) work portably on all POSIX shells, instead
> > suppressing the trace output with "test_untraceable" on shells that
> > aren't bash.
> > 
> > The tests that used the "try_repo" helper wanted to check whether git
> > commands invoked therein would emit anything on stderr. To do this
> > they invoked the function and redirected the stderr to a "message"
> > file.
> > 
> > In 58275069288 (t1510-repo-setup: mark as untraceable with '-x',
> > 2018-02-24) these were made to use "test_untraceable" introduced in
> > 5fc98e79fc0 (t: add means to disable '-x' tracing for individual test
> > scripts, 2018-02-24).
> > 
> > It is better to have the "try_repo" function itself start with a
> > "test_when_finished 'rm stderr'", and then redirect the stderr output
> > from git commands it invokes via its helpers to a "stderr" file.
> > 
> > This means that if we have a failure it'll be closer to the source of
> > the problem, and most importantly isn't incompatible with "-x" on
> > shells that aren't "bash".
> > 
> > We also need to split those tests that had two "try_repo" invocations
> > into different tests, which'll further help to narrow down any
> > potential failures. This wasn't strictly necessary (an artifact of the
> > use of "test_when_finished"), but the pattern enforces better test
> > hygiene.
> > 
> > Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> > ---
> >  t/t1510-repo-setup.sh | 83 +++++++++++++++++++++----------------------
> >  1 file changed, 40 insertions(+), 43 deletions(-)
> > 
> > diff --git a/t/t1510-repo-setup.sh b/t/t1510-repo-setup.sh
> > index 591505a39c0..f1748ac4a19 100755
> > --- a/t/t1510-repo-setup.sh
> > +++ b/t/t1510-repo-setup.sh
> > @@ -40,9 +40,6 @@ A few rules for repo setup:
> >      prefix is NULL.
> >  "
> >  
> > -# This test heavily relies on the standard error of nested function calls.
> > -test_untraceable=UnfortunatelyYes
> > -
> >  TEST_PASSES_SANITIZE_LEAK=true
> >  . ./test-lib.sh
> >  
> > @@ -62,7 +59,7 @@ test_repo () {
> >  			export GIT_WORK_TREE
> >  		fi &&
> >  		rm -f trace &&
> > -		GIT_TRACE_SETUP="$(pwd)/trace" git symbolic-ref HEAD >/dev/null &&
> > +		GIT_TRACE_SETUP="$(pwd)/trace" git symbolic-ref HEAD >/dev/null 2>>stderr &&
> 
> I suspect that it's lines like this that make Peff argue for
> BASH_XTRACEFD :)
> 
> While this is not a compound command, it does contain a command
> substitution, and the trace generated when executing the command in
> that command substitution goes to the command's stderr, and then,
> because of the redirection, to the 'stderr' file.
> 
> I find it suspicious that this doesn't trigger a failure in a
> 'test_must_be_empty stderr' later on.

> > @@ -62,7 +59,7 @@ test_repo () {
> >  			export GIT_WORK_TREE
> >  		fi &&
> >  		rm -f trace &&
> > -		GIT_TRACE_SETUP="$(pwd)/trace" git symbolic-ref HEAD >/dev/null &&
> > +		GIT_TRACE_SETUP="$(pwd)/trace" git symbolic-ref HEAD >/dev/null 2>>stderr &&
> 
> I suspect that it's lines like this that make Peff argue for
> BASH_XTRACEFD :)
> 
> While this is not a compound command, it does contain a command
> substitution, and the trace generated when executing the command in
> that command substitution goes to the command's stderr, and then,
> because of the redirection, to the 'stderr' file.

> On Thu, Dec 02, 2021 at 08:16:35PM +0100, SZEDER Gábor wrote:
>
>> > @@ -62,7 +59,7 @@ test_repo () {
>> >  			export GIT_WORK_TREE
>> >  		fi &&
>> >  		rm -f trace &&
>> > -		GIT_TRACE_SETUP="$(pwd)/trace" git symbolic-ref HEAD >/dev/null &&
>> > +		GIT_TRACE_SETUP="$(pwd)/trace" git symbolic-ref HEAD >/dev/null 2>>stderr &&
>> 
>> I suspect that it's lines like this that make Peff argue for
>> BASH_XTRACEFD :)
>> 
>> While this is not a compound command, it does contain a command
>> substitution, and the trace generated when executing the command in
>> that command substitution goes to the command's stderr, and then,
>> because of the redirection, to the 'stderr' file.
>
> Better still, the behavior varies between shells:
>
>   $ bash -c 'set -x; FOO=$(echo foo) echo main >stdout 2>stderr; set +x; grep . stdout stderr'
>   ++ echo foo
>   + FOO=foo
>   + echo main
>   + set +x
>   stdout:main
>
>   $ dash -c 'set -x; FOO=$(echo foo) echo main >stdout 2>stderr; set +x; grep . stdout stderr'
>   + FOO=foo echo main
>   + set +x
>   stdout:main
>   stderr:+ echo foo
>
> -Peff

> On Thu, Dec 02, 2021 at 08:16:35PM +0100, SZEDER Gábor wrote:
> 
> > > @@ -62,7 +59,7 @@ test_repo () {
> > >  			export GIT_WORK_TREE
> > >  		fi &&
> > >  		rm -f trace &&
> > > -		GIT_TRACE_SETUP="$(pwd)/trace" git symbolic-ref HEAD >/dev/null &&
> > > +		GIT_TRACE_SETUP="$(pwd)/trace" git symbolic-ref HEAD >/dev/null 2>>stderr &&
> > 
> > I suspect that it's lines like this that make Peff argue for
> > BASH_XTRACEFD :)
> > 
> > While this is not a compound command, it does contain a command
> > substitution, and the trace generated when executing the command in
> > that command substitution goes to the command's stderr, and then,
> > because of the redirection, to the 'stderr' file.
> 
> Better still, the behavior varies between shells:

>   $ bash -c 'set -x; FOO=$(echo foo) echo main >stdout 2>stderr; set +x; grep . stdout stderr'
>   ++ echo foo
>   + FOO=foo
>   + echo main
>   + set +x
>   stdout:main
> 
>   $ dash -c 'set -x; FOO=$(echo foo) echo main >stdout 2>stderr; set +x; grep . stdout stderr'
>   + FOO=foo echo main
>   + set +x
>   stdout:main
>   stderr:+ echo foo

> In the preceding commit the use of "test_untraceable=UnfortunatelyYes"
> was removed from "t1510-repo-setup.sh" in favor of more narrow
> redirections of the output of specific commands (and not entire
> sub-shells or functions).
> 
> This is in line with the fixes in the series that introduced the
> "test_untraceable" facility. See 571e472dc43 (Merge branch
> 'sg/test-x', 2018-03-14) for the series as a whole, and
> e.g. 91538d0cde9 (t5570-git-daemon: don't check the stderr of a
> subshell, 2018-02-24) for a commit that's in line with the changes in
> the preceding commit.
> 
> We've thus solved the TODO item noted when "test_untraceable" was
> added to "t1510-repo-setup.sh" in 58275069288 (t1510-repo-setup: mark
> as untraceable with '-x', 2018-02-24).
> 
> So let's remove the feature entirely. Not only is it currently unused,
> but it actively encourages an anti-pattern in our tests. We should be
> testing the output of specific commands, not entire subshells or
> functions.
> 
> That the "-x" output had to be disabled as a result is only one
> symptom, but even under bash those tests will be harder to debug as
> the subsequent check of the redirected file will be far removed from
> the command that emitted the output.
> 
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
>  t/README      |  3 ---
>  t/test-lib.sh | 66 ++++++---------------------------------------------
>  2 files changed, 7 insertions(+), 62 deletions(-)
> 
> diff --git a/t/README b/t/README
> index 29f72354bf1..3d30bbff34a 100644
> --- a/t/README
> +++ b/t/README
> @@ -86,9 +86,6 @@ appropriately before running "make". Short options can be bundled, i.e.
>  -x::
>  	Turn on shell tracing (i.e., `set -x`) during the tests
>  	themselves. Implies `--verbose`.
> -	Ignored in test scripts that set the variable 'test_untraceable'
> -	to a non-empty value, unless it's run with a Bash version
> -	supporting BASH_XTRACEFD, i.e. v4.1 or later.
>  
>  -d::
>  --debug::
> diff --git a/t/test-lib.sh b/t/test-lib.sh
> index 57efcc5e97a..b008716917b 100644
> --- a/t/test-lib.sh
> +++ b/t/test-lib.sh
> @@ -381,29 +381,6 @@ then
>  	exit
>  fi
>  
> -if test -n "$trace" && test -n "$test_untraceable"
> -then
> -	# '-x' tracing requested, but this test script can't be reliably
> -	# traced, unless it is run with a Bash version supporting
> -	# BASH_XTRACEFD (introduced in Bash v4.1).
> -	#
> -	# Perform this version check _after_ the test script was
> -	# potentially re-executed with $TEST_SHELL_PATH for '--tee' or
> -	# '--verbose-log', so the right shell is checked and the
> -	# warning is issued only once.
> -	if test -n "$BASH_VERSION" && eval '
> -	     test ${BASH_VERSINFO[0]} -gt 4 || {
> -	       test ${BASH_VERSINFO[0]} -eq 4 &&
> -	       test ${BASH_VERSINFO[1]} -ge 1
> -	     }
> -	   '
> -	then
> -		: Executed by a Bash version supporting BASH_XTRACEFD.  Good.
> -	else
> -		echo >&2 "warning: ignoring -x; '$0' is untraceable without BASH_XTRACEFD"
> -		trace=
> -	fi
> -fi
>  if test -n "$trace" && test -z "$verbose_log"
>  then
>  	verbose=t
> @@ -650,19 +627,6 @@ else
>  	exec 4>/dev/null 3>/dev/null
>  fi
>  
> -# Send any "-x" output directly to stderr to avoid polluting tests
> -# which capture stderr. We can do this unconditionally since it
> -# has no effect if tracing isn't turned on.
> -#
> -# Note that this sets up the trace fd as soon as we assign the variable, so it
> -# must come after the creation of descriptor 4 above. Likewise, we must never
> -# unset this, as it has the side effect of closing descriptor 4, which we
> -# use to show verbose tests to the user.
> -#
> -# Note also that we don't need or want to export it. The tracing is local to
> -# this shell, and we would not want to influence any shells we exec.
> -BASH_XTRACEFD=4

>  test_failure=0
>  test_count=0
>  test_fixed=0
> @@ -949,36 +913,20 @@ test_eval_ () {
>  	# the shell from printing the "set +x" to turn it off (nor the saving
>  	# of $? before that). But we can make sure that the output goes to
>  	# /dev/null.
> -	#
> -	# There are a few subtleties here:
> -	#
> -	#   - we have to redirect descriptor 4 in addition to 2, to cover
> -	#     BASH_XTRACEFD
> -	#
> -	#   - the actual eval has to come before the redirection block (since
> -	#     it needs to see descriptor 4 to set up its stderr)
> -	#
> -	#   - likewise, any error message we print must be outside the block to
> -	#     access descriptor 4
> -	#
> -	#   - checking $? has to come immediately after the eval, but it must
> -	#     be _inside_ the block to avoid polluting the "set -x" output
> -	#
> -
> -	test_eval_inner_ "$@" </dev/null >&3 2>&4
>  	{
> +		test_eval_inner_ "$@" </dev/null >&3 2>&4
>  		test_eval_ret_=$?
>  		if want_trace
>  		then
>  			test 1 = $trace_level_ && set +x
>  			trace_level_=$(($trace_level_-1))
> -		fi
> -	} 2>/dev/null 4>&2
>  
> -	if test "$test_eval_ret_" != 0 && want_trace
> -	then
> -		say_color error >&4 "error: last command exited with \$?=$test_eval_ret_"
> -	fi
> +			if test "$test_eval_ret_" != 0
> +			then
> +				say_color error >&4 "error: last command exited with \$?=$test_eval_ret_"
> +			fi
> +		fi
> +	} 2>/dev/null
>  	return $test_eval_ret_
>  }
>  
> -- 
> 2.34.1.932.g36842105b61
> 

> On Fri, Dec 10, 2021 at 11:07:55AM +0100, Ævar Arnfjörð Bjarmason wrote:
>> In the preceding commit the use of "test_untraceable=UnfortunatelyYes"
>> was removed from "t1510-repo-setup.sh" in favor of more narrow
>> redirections of the output of specific commands (and not entire
>> sub-shells or functions).
>> 
>> This is in line with the fixes in the series that introduced the
>> "test_untraceable" facility. See 571e472dc43 (Merge branch
>> 'sg/test-x', 2018-03-14) for the series as a whole, and
>> e.g. 91538d0cde9 (t5570-git-daemon: don't check the stderr of a
>> subshell, 2018-02-24) for a commit that's in line with the changes in
>> the preceding commit.
>> 
>> We've thus solved the TODO item noted when "test_untraceable" was
>> added to "t1510-repo-setup.sh" in 58275069288 (t1510-repo-setup: mark
>> as untraceable with '-x', 2018-02-24).
>> 
>> So let's remove the feature entirely. Not only is it currently unused,
>> but it actively encourages an anti-pattern in our tests. We should be
>> testing the output of specific commands, not entire subshells or
>> functions.
>> 
>> That the "-x" output had to be disabled as a result is only one
>> symptom, but even under bash those tests will be harder to debug as
>> the subsequent check of the redirected file will be far removed from
>> the command that emitted the output.
>> 
>> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
>> ---
>>  t/README      |  3 ---
>>  t/test-lib.sh | 66 ++++++---------------------------------------------
>>  2 files changed, 7 insertions(+), 62 deletions(-)
>> 
>> diff --git a/t/README b/t/README
>> index 29f72354bf1..3d30bbff34a 100644
>> --- a/t/README
>> +++ b/t/README
>> @@ -86,9 +86,6 @@ appropriately before running "make". Short options can be bundled, i.e.
>>  -x::
>>  	Turn on shell tracing (i.e., `set -x`) during the tests
>>  	themselves. Implies `--verbose`.
>> -	Ignored in test scripts that set the variable 'test_untraceable'
>> -	to a non-empty value, unless it's run with a Bash version
>> -	supporting BASH_XTRACEFD, i.e. v4.1 or later.
>>  
>>  -d::
>>  --debug::
>> diff --git a/t/test-lib.sh b/t/test-lib.sh
>> index 57efcc5e97a..b008716917b 100644
>> --- a/t/test-lib.sh
>> +++ b/t/test-lib.sh
>> @@ -381,29 +381,6 @@ then
>>  	exit
>>  fi
>>  
>> -if test -n "$trace" && test -n "$test_untraceable"
>> -then
>> -	# '-x' tracing requested, but this test script can't be reliably
>> -	# traced, unless it is run with a Bash version supporting
>> -	# BASH_XTRACEFD (introduced in Bash v4.1).
>> -	#
>> -	# Perform this version check _after_ the test script was
>> -	# potentially re-executed with $TEST_SHELL_PATH for '--tee' or
>> -	# '--verbose-log', so the right shell is checked and the
>> -	# warning is issued only once.
>> -	if test -n "$BASH_VERSION" && eval '
>> -	     test ${BASH_VERSINFO[0]} -gt 4 || {
>> -	       test ${BASH_VERSINFO[0]} -eq 4 &&
>> -	       test ${BASH_VERSINFO[1]} -ge 1
>> -	     }
>> -	   '
>> -	then
>> -		: Executed by a Bash version supporting BASH_XTRACEFD.  Good.
>> -	else
>> -		echo >&2 "warning: ignoring -x; '$0' is untraceable without BASH_XTRACEFD"
>> -		trace=
>> -	fi
>> -fi
>>  if test -n "$trace" && test -z "$verbose_log"
>>  then
>>  	verbose=t
>> @@ -650,19 +627,6 @@ else
>>  	exec 4>/dev/null 3>/dev/null
>>  fi
>>  
>> -# Send any "-x" output directly to stderr to avoid polluting tests
>> -# which capture stderr. We can do this unconditionally since it
>> -# has no effect if tracing isn't turned on.
>> -#
>> -# Note that this sets up the trace fd as soon as we assign the variable, so it
>> -# must come after the creation of descriptor 4 above. Likewise, we must never
>> -# unset this, as it has the side effect of closing descriptor 4, which we
>> -# use to show verbose tests to the user.
>> -#
>> -# Note also that we don't need or want to export it. The tracing is local to
>> -# this shell, and we would not want to influence any shells we exec.
>> -BASH_XTRACEFD=4
>
> Please do not remove BASH_XTRACEFD.  And especially not like this,
> without even mentioning it in the commit message.

> 
> On Sun, Dec 12 2021, SZEDER Gábor wrote:
> 
> > On Fri, Dec 10, 2021 at 11:07:55AM +0100, Ævar Arnfjörð Bjarmason wrote:
> >> In the preceding commit the use of "test_untraceable=UnfortunatelyYes"
> >> was removed from "t1510-repo-setup.sh" in favor of more narrow
> >> redirections of the output of specific commands (and not entire
> >> sub-shells or functions).
> >> 
> >> This is in line with the fixes in the series that introduced the
> >> "test_untraceable" facility. See 571e472dc43 (Merge branch
> >> 'sg/test-x', 2018-03-14) for the series as a whole, and
> >> e.g. 91538d0cde9 (t5570-git-daemon: don't check the stderr of a
> >> subshell, 2018-02-24) for a commit that's in line with the changes in
> >> the preceding commit.
> >> 
> >> We've thus solved the TODO item noted when "test_untraceable" was
> >> added to "t1510-repo-setup.sh" in 58275069288 (t1510-repo-setup: mark
> >> as untraceable with '-x', 2018-02-24).
> >> 
> >> So let's remove the feature entirely. Not only is it currently unused,
> >> but it actively encourages an anti-pattern in our tests. We should be
> >> testing the output of specific commands, not entire subshells or
> >> functions.
> >> 
> >> That the "-x" output had to be disabled as a result is only one
> >> symptom, but even under bash those tests will be harder to debug as
> >> the subsequent check of the redirected file will be far removed from
> >> the command that emitted the output.
> >> 
> >> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> >> ---
> >>  t/README      |  3 ---
> >>  t/test-lib.sh | 66 ++++++---------------------------------------------
> >>  2 files changed, 7 insertions(+), 62 deletions(-)
> >> 
> >> diff --git a/t/README b/t/README
> >> index 29f72354bf1..3d30bbff34a 100644
> >> --- a/t/README
> >> +++ b/t/README
> >> @@ -86,9 +86,6 @@ appropriately before running "make". Short options can be bundled, i.e.
> >>  -x::
> >>  	Turn on shell tracing (i.e., `set -x`) during the tests
> >>  	themselves. Implies `--verbose`.
> >> -	Ignored in test scripts that set the variable 'test_untraceable'
> >> -	to a non-empty value, unless it's run with a Bash version
> >> -	supporting BASH_XTRACEFD, i.e. v4.1 or later.
> >>  
> >>  -d::
> >>  --debug::
> >> diff --git a/t/test-lib.sh b/t/test-lib.sh
> >> index 57efcc5e97a..b008716917b 100644
> >> --- a/t/test-lib.sh
> >> +++ b/t/test-lib.sh
> >> @@ -381,29 +381,6 @@ then
> >>  	exit
> >>  fi
> >>  
> >> -if test -n "$trace" && test -n "$test_untraceable"
> >> -then
> >> -	# '-x' tracing requested, but this test script can't be reliably
> >> -	# traced, unless it is run with a Bash version supporting
> >> -	# BASH_XTRACEFD (introduced in Bash v4.1).
> >> -	#
> >> -	# Perform this version check _after_ the test script was
> >> -	# potentially re-executed with $TEST_SHELL_PATH for '--tee' or
> >> -	# '--verbose-log', so the right shell is checked and the
> >> -	# warning is issued only once.
> >> -	if test -n "$BASH_VERSION" && eval '
> >> -	     test ${BASH_VERSINFO[0]} -gt 4 || {
> >> -	       test ${BASH_VERSINFO[0]} -eq 4 &&
> >> -	       test ${BASH_VERSINFO[1]} -ge 1
> >> -	     }
> >> -	   '
> >> -	then
> >> -		: Executed by a Bash version supporting BASH_XTRACEFD.  Good.
> >> -	else
> >> -		echo >&2 "warning: ignoring -x; '$0' is untraceable without BASH_XTRACEFD"
> >> -		trace=
> >> -	fi
> >> -fi
> >>  if test -n "$trace" && test -z "$verbose_log"
> >>  then
> >>  	verbose=t
> >> @@ -650,19 +627,6 @@ else
> >>  	exec 4>/dev/null 3>/dev/null
> >>  fi
> >>  
> >> -# Send any "-x" output directly to stderr to avoid polluting tests
> >> -# which capture stderr. We can do this unconditionally since it
> >> -# has no effect if tracing isn't turned on.
> >> -#
> >> -# Note that this sets up the trace fd as soon as we assign the variable, so it
> >> -# must come after the creation of descriptor 4 above. Likewise, we must never
> >> -# unset this, as it has the side effect of closing descriptor 4, which we
> >> -# use to show verbose tests to the user.
> >> -#
> >> -# Note also that we don't need or want to export it. The tracing is local to
> >> -# this shell, and we would not want to influence any shells we exec.
> >> -BASH_XTRACEFD=4
> >
> > Please do not remove BASH_XTRACEFD.  And especially not like this,
> > without even mentioning it in the commit message.
> 
> I can re-roll with an amended commit message that explicitly mentions
> it

>, but that doesn't address your "please do not remove",
> 
> Do you see reason to keep it at the end-state fo this series? E.g. a
> counter-argument to
> https://lore.kernel.org/git/211210.86pmq4daxm.gmgdl@evledraar.gmail.com/?

>> > Please do not remove BASH_XTRACEFD.  And especially not like this,
>> > without even mentioning it in the commit message.
>> 
>> I can re-roll with an amended commit message that explicitly mentions
>> it
>
> It should rather be a separate patch...
>
>>, but that doesn't address your "please do not remove",
>> 
>> Do you see reason to keep it at the end-state fo this series? E.g. a
>> counter-argument to
>> https://lore.kernel.org/git/211210.86pmq4daxm.gmgdl@evledraar.gmail.com/?
>
> I don't see any argument pertinent to BASH_XTRACEFD in general in that
> email, of in favor of its removal in particular, and there are no
> valid arguments for its removal in earlier emails in this thread
> either.

> > I don't see any argument pertinent to BASH_XTRACEFD in general in that
> > email, of in favor of its removal in particular, and there are no
> > valid arguments for its removal in earlier emails in this thread
> > either.
> 
> If I am reading Ævar right, the argument is "dash would not be fixed
> with BASH_XTRACEFD, so there needs another way that would work there,
> and if the approach happens to work also for bash, then there is no
> reason to use BASH_XTRACEFD", I think.
> 
> Now, if the way Ævar came up with to help shells with "-x" not to
> contaminate their standard error stream that our test scripts want
> to inspect is worse to write, understand, and maintain, compared to
> the way we have been writing our tests that inspect their standard
> errors, without having to worry about "-x" output thanks to the use
> of BASH_XTRACEFD, it may make a regression to developer
> productivity, but I am not sure if that is the case.

> I think [1/2] of this same series can serve an example of how tests
> must be tweaked to live under the world order without BASH_XTRACEFD?
> Having to set and use a temporary file to capture the standard error
> output and append to it upfront looks uglier than each individual
> test locally capturing the standard error output from a single
> invocation of a helper function, but it does not look _too_ bad to
> me.  Can we find another example to argue for BASH_XTRACEFD, how
> much it makes it easier to write tests that work even under "-x"?

> I think the method for handling this in the test scripts _is_ worse to
> write, understand, and maintain. The problem to me is less that it's
> ugly to workaround (which as you note in this case is not great, but not
> _too_ bad), but that it's a subtle friction point that may jump up and
> bite any test-writer who does something like:
>
>   (foo && bar) 2>stderr

> So my view had always been that BASH_XTRACEFD is the good solution, and
> if people want to make "-x" work reliably under other shells, then I
> won't stop them. But somewhere along the way Gábor did a bunch of fixes
> to get things mostly running, and the use of dash with "-x" got added to
> CI, so now it's a de facto requirement (if you care about CI
> complaining, which we increasingly do).
> ...
> My vision was that we'd leave BASH_XTRACEFD so people using it could
> remain oblivious if they chose, but if the ship has sailed via CI, then
> that might have less value.

> On Wed, Dec 15, 2021 at 09:05:15AM -0800, Junio C Hamano wrote:
>
>> Still.  I wonder if keeping BASH_XTRACEFD helps developers, when
>> they need to diagnose a new breakage?  If their new test fails only
>> in the "dash -x" run but not "bash -x" at the CI, for example?
>
> I have done that, but usually only after realizing that "./t1234-foo.sh"
> passes and "./t1234-foo.sh -x" does not. So I think just tweaking use of
> "-x" would be the main tool.

> On Mon, Dec 13, 2021 at 10:51:14AM -0800, Junio C Hamano wrote:
>
>> > I don't see any argument pertinent to BASH_XTRACEFD in general in that
>> > email, of in favor of its removal in particular, and there are no
>> > valid arguments for its removal in earlier emails in this thread
>> > either.
>> 
>> If I am reading Ævar right, the argument is "dash would not be fixed
>> with BASH_XTRACEFD, so there needs another way that would work there,
>> and if the approach happens to work also for bash, then there is no
>> reason to use BASH_XTRACEFD", I think.
>> 
>> Now, if the way Ævar came up with to help shells with "-x" not to
>> contaminate their standard error stream that our test scripts want
>> to inspect is worse to write, understand, and maintain, compared to
>> the way we have been writing our tests that inspect their standard
>> errors, without having to worry about "-x" output thanks to the use
>> of BASH_XTRACEFD, it may make a regression to developer
>> productivity, but I am not sure if that is the case.
>
> I think the method for handling this in the test scripts _is_ worse to
> write, understand, and maintain. The problem to me is less that it's
> ugly to workaround (which as you note in this case is not great, but not
> _too_ bad), but that it's a subtle friction point that may jump up and
> bite any test-writer who does something like:
>
>   (foo && bar) 2>stderr
>
> So my view had always been that BASH_XTRACEFD is the good
> solution[...]

> Jeff King <peff@peff.net> writes:
>
>> On Wed, Dec 15, 2021 at 09:05:15AM -0800, Junio C Hamano wrote:
>>
>>> Still.  I wonder if keeping BASH_XTRACEFD helps developers, when
>>> they need to diagnose a new breakage?  If their new test fails only
>>> in the "dash -x" run but not "bash -x" at the CI, for example?
>>
>> I have done that, but usually only after realizing that "./t1234-foo.sh"
>> passes and "./t1234-foo.sh -x" does not. So I think just tweaking use of
>> "-x" would be the main tool.
>
> Ah, that's true.  You only need to compare "sh -x test.sh" with "sh
> test.sh" with any value of "sh", especially after BASH_XTRACEFD is
> removed, demoting "bash" to the same state as "dash" wrt "-x".
>
> OK, I am more OK with the removal of BASH_XTRACEFD support than
> before ;-)

> Stop setting "BASH_XTRACEFD=4" to direct "-x" output to file
> descriptor 4 under bash.
> 
> When it was added in d88785e424a (test-lib: set BASH_XTRACEFD
> automatically, 2016-05-11) it was needed as a workaround for various
> tests that didn't pass cleanly under "-x".
> 
> Most of those were later fixed in 71e472dc43 (Merge branch
> 'sg/test-x', 2018-03-14), and in the preceding commits we've fixed the
> final remaining and removed the "test_untraceable" facility.

> We could retain "BASH_XTRACEFD=4" anyway, but doing so is bad because:
> 
>  1) Since we're caring about "-x" passing in CI under "dash" on Ubuntu
>     using "BASH_XTRACEFD=4" will amount to hiding an error we'll run
>     into eventually. Tests will pass locally with "bash", but fail in
>     CI with "dash" (or under other non-"bash" shells).

> On Mon, Dec 13, 2021 at 02:38:36AM +0100, Ævar Arnfjörð Bjarmason wrote:
>> Stop setting "BASH_XTRACEFD=4" to direct "-x" output to file
>> descriptor 4 under bash.
>> 
>> When it was added in d88785e424a (test-lib: set BASH_XTRACEFD
>> automatically, 2016-05-11) it was needed as a workaround for various
>> tests that didn't pass cleanly under "-x".
>> 
>> Most of those were later fixed in 71e472dc43 (Merge branch
>> 'sg/test-x', 2018-03-14), and in the preceding commits we've fixed the
>> final remaining and removed the "test_untraceable" facility.
>
> Those commits made the test suite pass with '-x' without BASH_XTRACEFD
> only when all went well, but during development that's often not the
> case.  So let's not forget about c5c39f4e34 (test-lib: fix interrupt
> handling with 'dash' and '--verbose-log -x', 2019-03-13), before which
> dash was not really suitable to run tests involving daemon processes
> with '-x' during development.  If dash were to announce redirections
> in its '-x' trace, like many not as quite as popular shells do, then
> the workaround in that commit wouldn't work at all.
>
> In general, between POSIX leaving a lot of things explicitly
> unspecified, or, worse, silently unspecified, shells not conforming to
> POSIX, being buggy, and/or implementing their own extensions, I am
> actually quite surprised that it works as well as it does with so many
> shell.  At least as far as we know it, and I wouldn't at all be
> surprised if there were unknown issues lurking in some corner cases
> and/or with some more exotic shells.
>
>> We could retain "BASH_XTRACEFD=4" anyway, but doing so is bad because:
>> 
>>  1) Since we're caring about "-x" passing in CI under "dash" on Ubuntu
>>     using "BASH_XTRACEFD=4" will amount to hiding an error we'll run
>>     into eventually. Tests will pass locally with "bash", but fail in
>>     CI with "dash" (or under other non-"bash" shells).
>
> This is not "bad", this is exactly what we use CI for.  This is the
> smae case as when the test suite passes on a developer's Linux box,
> but breaks on OSX or Windows in CI.

> Furthermore, while I fully agree that keeping the whole test suite
> passing with '-x' without BASH_XTRACEFD is desirable, I do think it's
> a bad idea to forbid developers from using it while hacking away to
> scratch their itches.  I for one sometimes deliberately use various
> bashisms in my tests, including 'test_cmp'-ing the stderr of loops and
> functions, because they make writing tests then and there easier, when
> at that point I'd rather focus my attention on getting the C changes
> right, and clean them up eventually when I deem the changes worthy for
> submission.
>
>
> Overall I consider this patch as a cleanup solely for cleanup's sake,
> without any benefits at all.
>
> I'm kind of low on time myself as well, at least to argue about this
> any further.  Therefore, as the one who did the vast majority of work
> to make '-x' work even without BASH_XTRACEFD, I leave here my firm:
>
>   Not-Acked-By: SZEDER Gábor <szeder.dev@gmail.com>
>
> to any patch that attempts to remove support for BASH_XTRACEFD.

> 1. https://lore.kernel.org/git/cover-v3-0.2-00000000000-20211210T100512Z-avarab@gmail.com/
> 2. https://lore.kernel.org/git/20211212201441.GB3400@szeder.dev/
> 
> Ævar Arnfjörð Bjarmason (3):
>   t1510: remove need for "test_untraceable", retain coverage
>   test-lib.sh: remove the now-unused "test_untraceable" facility
>   test-lib.sh: remove "BASH_XTRACEFD"
> 
>  t/README              |  3 --
>  t/t1510-repo-setup.sh | 85 +++++++++++++++++++++----------------------
>  t/test-lib.sh         | 66 ++++-----------------------------
>  3 files changed, 49 insertions(+), 105 deletions(-)
> 
> Range-diff against v3:
> 1:  7876202c5b0 = 1:  9e7b089dc50 t1510: remove need for "test_untraceable", retain coverage
> -:  ----------- > 2:  60883fd95cb test-lib.sh: remove the now-unused "test_untraceable" facility
> 2:  a7fc794e20d ! 3:  8b5ae33376e test-lib.sh: remove the now-unused "test_untraceable" facility
>     @@ Metadata
>      Author: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
>      
>       ## Commit message ##
>     -    test-lib.sh: remove the now-unused "test_untraceable" facility
>     +    test-lib.sh: remove "BASH_XTRACEFD"
>      
>     -    In the preceding commit the use of "test_untraceable=UnfortunatelyYes"
>     -    was removed from "t1510-repo-setup.sh" in favor of more narrow
>     -    redirections of the output of specific commands (and not entire
>     -    sub-shells or functions).
>     +    Stop setting "BASH_XTRACEFD=4" to direct "-x" output to file
>     +    descriptor 4 under bash.
>      
>     -    This is in line with the fixes in the series that introduced the
>     -    "test_untraceable" facility. See 571e472dc43 (Merge branch
>     -    'sg/test-x', 2018-03-14) for the series as a whole, and
>     -    e.g. 91538d0cde9 (t5570-git-daemon: don't check the stderr of a
>     -    subshell, 2018-02-24) for a commit that's in line with the changes in
>     -    the preceding commit.
>     +    When it was added in d88785e424a (test-lib: set BASH_XTRACEFD
>     +    automatically, 2016-05-11) it was needed as a workaround for various
>     +    tests that didn't pass cleanly under "-x".
>      
>     -    We've thus solved the TODO item noted when "test_untraceable" was
>     -    added to "t1510-repo-setup.sh" in 58275069288 (t1510-repo-setup: mark
>     -    as untraceable with '-x', 2018-02-24).
>     +    Most of those were later fixed in 71e472dc43 (Merge branch
>     +    'sg/test-x', 2018-03-14), and in the preceding commits we've fixed the
>     +    final remaining and removed the "test_untraceable" facility.
>      
>     -    So let's remove the feature entirely. Not only is it currently unused,
>     -    but it actively encourages an anti-pattern in our tests. We should be
>     -    testing the output of specific commands, not entire subshells or
>     -    functions.
>     +    The reason we don't need this anymore is becomes clear from reading
>     +    the rationale in d88785e424a and applying those arguments to the
>     +    current state of the codebase. In particular it said (with "this" and
>     +    "it" referring to the problem of tests failing under "-x"):
>      
>     -    That the "-x" output had to be disabled as a result is only one
>     -    symptom, but even under bash those tests will be harder to debug as
>     -    the subsequent check of the redirected file will be far removed from
>     -    the command that emitted the output.
>     +        "there here isn't a portable or scalable solution to this [...] we
>     +        can work around it by pointing the "set -x" output to our
>     +        descriptor 4"
>      
>     -    Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
>     +    And finally that:
>      
>     - ## t/README ##
>     -@@ t/README: appropriately before running "make". Short options can be bundled, i.e.
>     - -x::
>     - 	Turn on shell tracing (i.e., `set -x`) during the tests
>     - 	themselves. Implies `--verbose`.
>     --	Ignored in test scripts that set the variable 'test_untraceable'
>     --	to a non-empty value, unless it's run with a Bash version
>     --	supporting BASH_XTRACEFD, i.e. v4.1 or later.
>     - 
>     - -d::
>     - --debug::
>     +        "Automatic tests for our "-x" option may be a bit too meta"
>     +
>     +    Those tests are exactly what we've had since aedffe95250 (travis-ci:
>     +    run tests with '-x' tracing, 2018-02-24), so punting on fixing issues
>     +    with "-x" by using "BASH_XTRACEFD=4" isn't needed anymore, we're now
>     +    committing to maintaining the test suite in a way that won't break
>     +    under "-x".
>     +
>     +    We could retain "BASH_XTRACEFD=4" anyway, but doing so is bad because:
>     +
>     +     1) Since we're caring about "-x" passing in CI under "dash" on Ubuntu
>     +        using "BASH_XTRACEFD=4" will amount to hiding an error we'll run
>     +        into eventually. Tests will pass locally with "bash", but fail in
>     +        CI with "dash" (or under other non-"bash" shells).
>     +
>     +     2) As the amended code in "test_eval_" shows (an amended revert to
>     +        the pre-image of d88785e424a) it's simpler to not have to take
>     +        this "bash" special-case into account.
>     +
>     +    Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
>      
>       ## t/test-lib.sh ##
>     -@@ t/test-lib.sh: then
>     - 	exit
>     - fi
>     - 
>     --if test -n "$trace" && test -n "$test_untraceable"
>     --then
>     --	# '-x' tracing requested, but this test script can't be reliably
>     --	# traced, unless it is run with a Bash version supporting
>     --	# BASH_XTRACEFD (introduced in Bash v4.1).
>     --	#
>     --	# Perform this version check _after_ the test script was
>     --	# potentially re-executed with $TEST_SHELL_PATH for '--tee' or
>     --	# '--verbose-log', so the right shell is checked and the
>     --	# warning is issued only once.
>     --	if test -n "$BASH_VERSION" && eval '
>     --	     test ${BASH_VERSINFO[0]} -gt 4 || {
>     --	       test ${BASH_VERSINFO[0]} -eq 4 &&
>     --	       test ${BASH_VERSINFO[1]} -ge 1
>     --	     }
>     --	   '
>     --	then
>     --		: Executed by a Bash version supporting BASH_XTRACEFD.  Good.
>     --	else
>     --		echo >&2 "warning: ignoring -x; '$0' is untraceable without BASH_XTRACEFD"
>     --		trace=
>     --	fi
>     --fi
>     - if test -n "$trace" && test -z "$verbose_log"
>     - then
>     - 	verbose=t
>      @@ t/test-lib.sh: else
>       	exec 4>/dev/null 3>/dev/null
>       fi
> -- 
> 2.34.1.1024.g573f2f4b767
> 

> On Mon, Dec 13, 2021 at 02:38:33AM +0100, Ævar Arnfjörð Bjarmason wrote:
>> A re-arrangement-only change to v3[1]. The previous 2/2 is now split
>> into two commits, as requested by SZEDER[2] in the removal of
>> BASH_XTRACEFD is now its own commit & the rationale for doing so is
>> outlined in detail.
>
> I'm afraid I wasn't clear.  What I meant was that if we were to remove
> BASH_XTRACEFD, then it should be done its own commit.

>> 1. https://lore.kernel.org/git/cover-v3-0.2-00000000000-20211210T100512Z-avarab@gmail.com/
>> 2. https://lore.kernel.org/git/20211212201441.GB3400@szeder.dev/
>> 
>> Ævar Arnfjörð Bjarmason (3):
>>   t1510: remove need for "test_untraceable", retain coverage
>>   test-lib.sh: remove the now-unused "test_untraceable" facility
>>   test-lib.sh: remove "BASH_XTRACEFD"
>> 
>>  t/README              |  3 --
>>  t/t1510-repo-setup.sh | 85 +++++++++++++++++++++----------------------
>>  t/test-lib.sh         | 66 ++++-----------------------------
>>  3 files changed, 49 insertions(+), 105 deletions(-)
>> 
>> Range-diff against v3:
>> 1:  7876202c5b0 = 1:  9e7b089dc50 t1510: remove need for "test_untraceable", retain coverage
>> -:  ----------- > 2:  60883fd95cb test-lib.sh: remove the now-unused "test_untraceable" facility
>> 2:  a7fc794e20d ! 3:  8b5ae33376e test-lib.sh: remove the now-unused "test_untraceable" facility
>>     @@ Metadata
>>      Author: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
>>      
>>       ## Commit message ##
>>     -    test-lib.sh: remove the now-unused "test_untraceable" facility
>>     +    test-lib.sh: remove "BASH_XTRACEFD"
>>      
>>     -    In the preceding commit the use of "test_untraceable=UnfortunatelyYes"
>>     -    was removed from "t1510-repo-setup.sh" in favor of more narrow
>>     -    redirections of the output of specific commands (and not entire
>>     -    sub-shells or functions).
>>     +    Stop setting "BASH_XTRACEFD=4" to direct "-x" output to file
>>     +    descriptor 4 under bash.
>>      
>>     -    This is in line with the fixes in the series that introduced the
>>     -    "test_untraceable" facility. See 571e472dc43 (Merge branch
>>     -    'sg/test-x', 2018-03-14) for the series as a whole, and
>>     -    e.g. 91538d0cde9 (t5570-git-daemon: don't check the stderr of a
>>     -    subshell, 2018-02-24) for a commit that's in line with the changes in
>>     -    the preceding commit.
>>     +    When it was added in d88785e424a (test-lib: set BASH_XTRACEFD
>>     +    automatically, 2016-05-11) it was needed as a workaround for various
>>     +    tests that didn't pass cleanly under "-x".
>>      
>>     -    We've thus solved the TODO item noted when "test_untraceable" was
>>     -    added to "t1510-repo-setup.sh" in 58275069288 (t1510-repo-setup: mark
>>     -    as untraceable with '-x', 2018-02-24).
>>     +    Most of those were later fixed in 71e472dc43 (Merge branch
>>     +    'sg/test-x', 2018-03-14), and in the preceding commits we've fixed the
>>     +    final remaining and removed the "test_untraceable" facility.
>>      
>>     -    So let's remove the feature entirely. Not only is it currently unused,
>>     -    but it actively encourages an anti-pattern in our tests. We should be
>>     -    testing the output of specific commands, not entire subshells or
>>     -    functions.
>>     +    The reason we don't need this anymore is becomes clear from reading
>>     +    the rationale in d88785e424a and applying those arguments to the
>>     +    current state of the codebase. In particular it said (with "this" and
>>     +    "it" referring to the problem of tests failing under "-x"):
>>      
>>     -    That the "-x" output had to be disabled as a result is only one
>>     -    symptom, but even under bash those tests will be harder to debug as
>>     -    the subsequent check of the redirected file will be far removed from
>>     -    the command that emitted the output.
>>     +        "there here isn't a portable or scalable solution to this [...] we
>>     +        can work around it by pointing the "set -x" output to our
>>     +        descriptor 4"
>>      
>>     -    Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
>>     +    And finally that:
>>      
>>     - ## t/README ##
>>     -@@ t/README: appropriately before running "make". Short options can be bundled, i.e.
>>     - -x::
>>     - 	Turn on shell tracing (i.e., `set -x`) during the tests
>>     - 	themselves. Implies `--verbose`.
>>     --	Ignored in test scripts that set the variable 'test_untraceable'
>>     --	to a non-empty value, unless it's run with a Bash version
>>     --	supporting BASH_XTRACEFD, i.e. v4.1 or later.
>>     - 
>>     - -d::
>>     - --debug::
>>     +        "Automatic tests for our "-x" option may be a bit too meta"
>>     +
>>     +    Those tests are exactly what we've had since aedffe95250 (travis-ci:
>>     +    run tests with '-x' tracing, 2018-02-24), so punting on fixing issues
>>     +    with "-x" by using "BASH_XTRACEFD=4" isn't needed anymore, we're now
>>     +    committing to maintaining the test suite in a way that won't break
>>     +    under "-x".
>>     +
>>     +    We could retain "BASH_XTRACEFD=4" anyway, but doing so is bad because:
>>     +
>>     +     1) Since we're caring about "-x" passing in CI under "dash" on Ubuntu
>>     +        using "BASH_XTRACEFD=4" will amount to hiding an error we'll run
>>     +        into eventually. Tests will pass locally with "bash", but fail in
>>     +        CI with "dash" (or under other non-"bash" shells).
>>     +
>>     +     2) As the amended code in "test_eval_" shows (an amended revert to
>>     +        the pre-image of d88785e424a) it's simpler to not have to take
>>     +        this "bash" special-case into account.
>>     +
>>     +    Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
>>      
>>       ## t/test-lib.sh ##
>>     -@@ t/test-lib.sh: then
>>     - 	exit
>>     - fi
>>     - 
>>     --if test -n "$trace" && test -n "$test_untraceable"
>>     --then
>>     --	# '-x' tracing requested, but this test script can't be reliably
>>     --	# traced, unless it is run with a Bash version supporting
>>     --	# BASH_XTRACEFD (introduced in Bash v4.1).
>>     --	#
>>     --	# Perform this version check _after_ the test script was
>>     --	# potentially re-executed with $TEST_SHELL_PATH for '--tee' or
>>     --	# '--verbose-log', so the right shell is checked and the
>>     --	# warning is issued only once.
>>     --	if test -n "$BASH_VERSION" && eval '
>>     --	     test ${BASH_VERSINFO[0]} -gt 4 || {
>>     --	       test ${BASH_VERSINFO[0]} -eq 4 &&
>>     --	       test ${BASH_VERSINFO[1]} -ge 1
>>     --	     }
>>     --	   '
>>     --	then
>>     --		: Executed by a Bash version supporting BASH_XTRACEFD.  Good.
>>     --	else
>>     --		echo >&2 "warning: ignoring -x; '$0' is untraceable without BASH_XTRACEFD"
>>     --		trace=
>>     --	fi
>>     --fi
>>     - if test -n "$trace" && test -z "$verbose_log"
>>     - then
>>     - 	verbose=t
>>      @@ t/test-lib.sh: else
>>       	exec 4>/dev/null 3>/dev/null
>>       fi
>> -- 
>> 2.34.1.1024.g573f2f4b767
>> 

> 
> [Junio: If you'd like to pick up this series it still cleanly applies,
> and merges cleanly with "seen"]
> 
> On Mon, Dec 13 2021, SZEDER Gábor wrote:
> 
> Sorry about the late reply, things getting lost around the holidays
> etc.
> 
> > On Mon, Dec 13, 2021 at 02:38:33AM +0100, Ævar Arnfjörð Bjarmason wrote:
> >> A re-arrangement-only change to v3[1]. The previous 2/2 is now split
> >> into two commits, as requested by SZEDER[2] in the removal of
> >> BASH_XTRACEFD is now its own commit & the rationale for doing so is
> >> outlined in detail.
> >
> > I'm afraid I wasn't clear.  What I meant was that if we were to remove
> > BASH_XTRACEFD, then it should be done its own commit.
> 
> Aside from whether you think removing it is a good idea, isn't that what
> this v4 does?

> In 1/3 I fix -x under non-BASH_XTRACEFD, 2/3 removes test_untraceable,
> and 3/3 the use of BASH_XTRACEFD.
> 
> > But again: BASH_XTRACEFD is the only simple yet reliable and robust
> > way to get -x trace from our tests, so do not remove it.
> 
> Just to tie off this loose end, I re-read the thread ending in [1] (sent
> after this reply of yours) and I think my [1] addresses this.

> Maybe you still disagree, but I don't see how that squares with
> "reliable and robust" here.
> 
> I.e. unless we're talking about carrying bash-specific code in the test
> suite we can't make any real use of the feature, as our tests will need
> to be compatible with other POSIX shells.
> 
> I mean, the code changed in 1/3 *was* that bash-specific code, but as
> that change shows it was rather easily made non-bash-specific. And
> unless we think we'll add other bash-specific tests (I don't see why,
> the cross-shell -x support is rather easy to do) ....
> 
> 1. https://lore.kernel.org/git/211216.864k78bsjs.gmgdl@evledraar.gmail.com/
> 
> >> 1. https://lore.kernel.org/git/cover-v3-0.2-00000000000-20211210T100512Z-avarab@gmail.com/
> >> 2. https://lore.kernel.org/git/20211212201441.GB3400@szeder.dev/
> >> 
> >> Ævar Arnfjörð Bjarmason (3):
> >>   t1510: remove need for "test_untraceable", retain coverage
> >>   test-lib.sh: remove the now-unused "test_untraceable" facility
> >>   test-lib.sh: remove "BASH_XTRACEFD"
> >> 
> >>  t/README              |  3 --
> >>  t/t1510-repo-setup.sh | 85 +++++++++++++++++++++----------------------
> >>  t/test-lib.sh         | 66 ++++-----------------------------
> >>  3 files changed, 49 insertions(+), 105 deletions(-)
> >> 
> >> Range-diff against v3:
> >> 1:  7876202c5b0 = 1:  9e7b089dc50 t1510: remove need for "test_untraceable", retain coverage
> >> -:  ----------- > 2:  60883fd95cb test-lib.sh: remove the now-unused "test_untraceable" facility
> >> 2:  a7fc794e20d ! 3:  8b5ae33376e test-lib.sh: remove the now-unused "test_untraceable" facility
> >>     @@ Metadata
> >>      Author: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> >>      
> >>       ## Commit message ##
> >>     -    test-lib.sh: remove the now-unused "test_untraceable" facility
> >>     +    test-lib.sh: remove "BASH_XTRACEFD"
> >>      
> >>     -    In the preceding commit the use of "test_untraceable=UnfortunatelyYes"
> >>     -    was removed from "t1510-repo-setup.sh" in favor of more narrow
> >>     -    redirections of the output of specific commands (and not entire
> >>     -    sub-shells or functions).
> >>     +    Stop setting "BASH_XTRACEFD=4" to direct "-x" output to file
> >>     +    descriptor 4 under bash.
> >>      
> >>     -    This is in line with the fixes in the series that introduced the
> >>     -    "test_untraceable" facility. See 571e472dc43 (Merge branch
> >>     -    'sg/test-x', 2018-03-14) for the series as a whole, and
> >>     -    e.g. 91538d0cde9 (t5570-git-daemon: don't check the stderr of a
> >>     -    subshell, 2018-02-24) for a commit that's in line with the changes in
> >>     -    the preceding commit.
> >>     +    When it was added in d88785e424a (test-lib: set BASH_XTRACEFD
> >>     +    automatically, 2016-05-11) it was needed as a workaround for various
> >>     +    tests that didn't pass cleanly under "-x".
> >>      
> >>     -    We've thus solved the TODO item noted when "test_untraceable" was
> >>     -    added to "t1510-repo-setup.sh" in 58275069288 (t1510-repo-setup: mark
> >>     -    as untraceable with '-x', 2018-02-24).
> >>     +    Most of those were later fixed in 71e472dc43 (Merge branch
> >>     +    'sg/test-x', 2018-03-14), and in the preceding commits we've fixed the
> >>     +    final remaining and removed the "test_untraceable" facility.
> >>      
> >>     -    So let's remove the feature entirely. Not only is it currently unused,
> >>     -    but it actively encourages an anti-pattern in our tests. We should be
> >>     -    testing the output of specific commands, not entire subshells or
> >>     -    functions.
> >>     +    The reason we don't need this anymore is becomes clear from reading
> >>     +    the rationale in d88785e424a and applying those arguments to the
> >>     +    current state of the codebase. In particular it said (with "this" and
> >>     +    "it" referring to the problem of tests failing under "-x"):
> >>      
> >>     -    That the "-x" output had to be disabled as a result is only one
> >>     -    symptom, but even under bash those tests will be harder to debug as
> >>     -    the subsequent check of the redirected file will be far removed from
> >>     -    the command that emitted the output.
> >>     +        "there here isn't a portable or scalable solution to this [...] we
> >>     +        can work around it by pointing the "set -x" output to our
> >>     +        descriptor 4"
> >>      
> >>     -    Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> >>     +    And finally that:
> >>      
> >>     - ## t/README ##
> >>     -@@ t/README: appropriately before running "make". Short options can be bundled, i.e.
> >>     - -x::
> >>     - 	Turn on shell tracing (i.e., `set -x`) during the tests
> >>     - 	themselves. Implies `--verbose`.
> >>     --	Ignored in test scripts that set the variable 'test_untraceable'
> >>     --	to a non-empty value, unless it's run with a Bash version
> >>     --	supporting BASH_XTRACEFD, i.e. v4.1 or later.
> >>     - 
> >>     - -d::
> >>     - --debug::
> >>     +        "Automatic tests for our "-x" option may be a bit too meta"
> >>     +
> >>     +    Those tests are exactly what we've had since aedffe95250 (travis-ci:
> >>     +    run tests with '-x' tracing, 2018-02-24), so punting on fixing issues
> >>     +    with "-x" by using "BASH_XTRACEFD=4" isn't needed anymore, we're now
> >>     +    committing to maintaining the test suite in a way that won't break
> >>     +    under "-x".
> >>     +
> >>     +    We could retain "BASH_XTRACEFD=4" anyway, but doing so is bad because:
> >>     +
> >>     +     1) Since we're caring about "-x" passing in CI under "dash" on Ubuntu
> >>     +        using "BASH_XTRACEFD=4" will amount to hiding an error we'll run
> >>     +        into eventually. Tests will pass locally with "bash", but fail in
> >>     +        CI with "dash" (or under other non-"bash" shells).
> >>     +
> >>     +     2) As the amended code in "test_eval_" shows (an amended revert to
> >>     +        the pre-image of d88785e424a) it's simpler to not have to take
> >>     +        this "bash" special-case into account.
> >>     +
> >>     +    Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> >>      
> >>       ## t/test-lib.sh ##
> >>     -@@ t/test-lib.sh: then
> >>     - 	exit
> >>     - fi
> >>     - 
> >>     --if test -n "$trace" && test -n "$test_untraceable"
> >>     --then
> >>     --	# '-x' tracing requested, but this test script can't be reliably
> >>     --	# traced, unless it is run with a Bash version supporting
> >>     --	# BASH_XTRACEFD (introduced in Bash v4.1).
> >>     --	#
> >>     --	# Perform this version check _after_ the test script was
> >>     --	# potentially re-executed with $TEST_SHELL_PATH for '--tee' or
> >>     --	# '--verbose-log', so the right shell is checked and the
> >>     --	# warning is issued only once.
> >>     --	if test -n "$BASH_VERSION" && eval '
> >>     --	     test ${BASH_VERSINFO[0]} -gt 4 || {
> >>     --	       test ${BASH_VERSINFO[0]} -eq 4 &&
> >>     --	       test ${BASH_VERSINFO[1]} -ge 1
> >>     --	     }
> >>     --	   '
> >>     --	then
> >>     --		: Executed by a Bash version supporting BASH_XTRACEFD.  Good.
> >>     --	else
> >>     --		echo >&2 "warning: ignoring -x; '$0' is untraceable without BASH_XTRACEFD"
> >>     --		trace=
> >>     --	fi
> >>     --fi
> >>     - if test -n "$trace" && test -z "$verbose_log"
> >>     - then
> >>     - 	verbose=t
> >>      @@ t/test-lib.sh: else
> >>       	exec 4>/dev/null 3>/dev/null
> >>       fi
> >> -- 
> >> 2.34.1.1024.g573f2f4b767
> >> 
> 
> o

> On Mon, Feb 21, 2022 at 08:52:18PM +0100, Ævar Arnfjörð Bjarmason wrote:
>> 
>> [Junio: If you'd like to pick up this series it still cleanly applies,
>> and merges cleanly with "seen"]
>> 
>> On Mon, Dec 13 2021, SZEDER Gábor wrote:
>> 
>> Sorry about the late reply, things getting lost around the holidays
>> etc.
>> 
>> > On Mon, Dec 13, 2021 at 02:38:33AM +0100, Ævar Arnfjörð Bjarmason wrote:
>> >> A re-arrangement-only change to v3[1]. The previous 2/2 is now split
>> >> into two commits, as requested by SZEDER[2] in the removal of
>> >> BASH_XTRACEFD is now its own commit & the rationale for doing so is
>> >> outlined in detail.
>> >
>> > I'm afraid I wasn't clear.  What I meant was that if we were to remove
>> > BASH_XTRACEFD, then it should be done its own commit.
>> 
>> Aside from whether you think removing it is a good idea, isn't that what
>> this v4 does?
>
> Well, yes, but I now realize that I wasn't sufficiently clear the
> second time around, either, and emphasis doesn't travel well over
> email.  What I meant was that: _if_ at all we were to remove it, then
> it should be a separate patch, but since we should most definitely
> keep it, those hunks should be simply dropped.

>> In 1/3 I fix -x under non-BASH_XTRACEFD, 2/3 removes test_untraceable,
>> and 3/3 the use of BASH_XTRACEFD.
>> 
>> > But again: BASH_XTRACEFD is the only simple yet reliable and robust
>> > way to get -x trace from our tests, so do not remove it.
>> 
>> Just to tie off this loose end, I re-read the thread ending in [1] (sent
>> after this reply of yours) and I think my [1] addresses this.
>
> It doesn't at all; "if CI passes without it, then we can remove it" is
> not a convincing argument.

>> Maybe you still disagree, but I don't see how that squares with
>> "reliable and robust" here.
>> 
>> I.e. unless we're talking about carrying bash-specific code in the test
>> suite we can't make any real use of the feature, as our tests will need
>> to be compatible with other POSIX shells.
>> 
>> I mean, the code changed in 1/3 *was* that bash-specific code, but as
>> that change shows it was rather easily made non-bash-specific. And
>> unless we think we'll add other bash-specific tests (I don't see why,
>> the cross-shell -x support is rather easy to do) ....
>> 
>> 1. https://lore.kernel.org/git/211216.864k78bsjs.gmgdl@evledraar.gmail.com/
>> 
>> >> 1. https://lore.kernel.org/git/cover-v3-0.2-00000000000-20211210T100512Z-avarab@gmail.com/
>> >> 2. https://lore.kernel.org/git/20211212201441.GB3400@szeder.dev/
>> >> 
>> >> Ævar Arnfjörð Bjarmason (3):
>> >>   t1510: remove need for "test_untraceable", retain coverage
>> >>   test-lib.sh: remove the now-unused "test_untraceable" facility
>> >>   test-lib.sh: remove "BASH_XTRACEFD"
>> >> 
>> >>  t/README              |  3 --
>> >>  t/t1510-repo-setup.sh | 85 +++++++++++++++++++++----------------------
>> >>  t/test-lib.sh         | 66 ++++-----------------------------
>> >>  3 files changed, 49 insertions(+), 105 deletions(-)
>> >> 
>> >> Range-diff against v3:
>> >> 1:  7876202c5b0 = 1:  9e7b089dc50 t1510: remove need for "test_untraceable", retain coverage
>> >> -:  ----------- > 2:  60883fd95cb test-lib.sh: remove the now-unused "test_untraceable" facility
>> >> 2:  a7fc794e20d ! 3:  8b5ae33376e test-lib.sh: remove the now-unused "test_untraceable" facility
>> >>     @@ Metadata
>> >>      Author: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
>> >>      
>> >>       ## Commit message ##
>> >>     -    test-lib.sh: remove the now-unused "test_untraceable" facility
>> >>     +    test-lib.sh: remove "BASH_XTRACEFD"
>> >>      
>> >>     -    In the preceding commit the use of "test_untraceable=UnfortunatelyYes"
>> >>     -    was removed from "t1510-repo-setup.sh" in favor of more narrow
>> >>     -    redirections of the output of specific commands (and not entire
>> >>     -    sub-shells or functions).
>> >>     +    Stop setting "BASH_XTRACEFD=4" to direct "-x" output to file
>> >>     +    descriptor 4 under bash.
>> >>      
>> >>     -    This is in line with the fixes in the series that introduced the
>> >>     -    "test_untraceable" facility. See 571e472dc43 (Merge branch
>> >>     -    'sg/test-x', 2018-03-14) for the series as a whole, and
>> >>     -    e.g. 91538d0cde9 (t5570-git-daemon: don't check the stderr of a
>> >>     -    subshell, 2018-02-24) for a commit that's in line with the changes in
>> >>     -    the preceding commit.
>> >>     +    When it was added in d88785e424a (test-lib: set BASH_XTRACEFD
>> >>     +    automatically, 2016-05-11) it was needed as a workaround for various
>> >>     +    tests that didn't pass cleanly under "-x".
>> >>      
>> >>     -    We've thus solved the TODO item noted when "test_untraceable" was
>> >>     -    added to "t1510-repo-setup.sh" in 58275069288 (t1510-repo-setup: mark
>> >>     -    as untraceable with '-x', 2018-02-24).
>> >>     +    Most of those were later fixed in 71e472dc43 (Merge branch
>> >>     +    'sg/test-x', 2018-03-14), and in the preceding commits we've fixed the
>> >>     +    final remaining and removed the "test_untraceable" facility.
>> >>      
>> >>     -    So let's remove the feature entirely. Not only is it currently unused,
>> >>     -    but it actively encourages an anti-pattern in our tests. We should be
>> >>     -    testing the output of specific commands, not entire subshells or
>> >>     -    functions.
>> >>     +    The reason we don't need this anymore is becomes clear from reading
>> >>     +    the rationale in d88785e424a and applying those arguments to the
>> >>     +    current state of the codebase. In particular it said (with "this" and
>> >>     +    "it" referring to the problem of tests failing under "-x"):
>> >>      
>> >>     -    That the "-x" output had to be disabled as a result is only one
>> >>     -    symptom, but even under bash those tests will be harder to debug as
>> >>     -    the subsequent check of the redirected file will be far removed from
>> >>     -    the command that emitted the output.
>> >>     +        "there here isn't a portable or scalable solution to this [...] we
>> >>     +        can work around it by pointing the "set -x" output to our
>> >>     +        descriptor 4"
>> >>      
>> >>     -    Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
>> >>     +    And finally that:
>> >>      
>> >>     - ## t/README ##
>> >>     -@@ t/README: appropriately before running "make". Short options can be bundled, i.e.
>> >>     - -x::
>> >>     - 	Turn on shell tracing (i.e., `set -x`) during the tests
>> >>     - 	themselves. Implies `--verbose`.
>> >>     --	Ignored in test scripts that set the variable 'test_untraceable'
>> >>     --	to a non-empty value, unless it's run with a Bash version
>> >>     --	supporting BASH_XTRACEFD, i.e. v4.1 or later.
>> >>     - 
>> >>     - -d::
>> >>     - --debug::
>> >>     +        "Automatic tests for our "-x" option may be a bit too meta"
>> >>     +
>> >>     +    Those tests are exactly what we've had since aedffe95250 (travis-ci:
>> >>     +    run tests with '-x' tracing, 2018-02-24), so punting on fixing issues
>> >>     +    with "-x" by using "BASH_XTRACEFD=4" isn't needed anymore, we're now
>> >>     +    committing to maintaining the test suite in a way that won't break
>> >>     +    under "-x".
>> >>     +
>> >>     +    We could retain "BASH_XTRACEFD=4" anyway, but doing so is bad because:
>> >>     +
>> >>     +     1) Since we're caring about "-x" passing in CI under "dash" on Ubuntu
>> >>     +        using "BASH_XTRACEFD=4" will amount to hiding an error we'll run
>> >>     +        into eventually. Tests will pass locally with "bash", but fail in
>> >>     +        CI with "dash" (or under other non-"bash" shells).
>> >>     +
>> >>     +     2) As the amended code in "test_eval_" shows (an amended revert to
>> >>     +        the pre-image of d88785e424a) it's simpler to not have to take
>> >>     +        this "bash" special-case into account.
>> >>     +
>> >>     +    Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
>> >>      
>> >>       ## t/test-lib.sh ##
>> >>     -@@ t/test-lib.sh: then
>> >>     - 	exit
>> >>     - fi
>> >>     - 
>> >>     --if test -n "$trace" && test -n "$test_untraceable"
>> >>     --then
>> >>     --	# '-x' tracing requested, but this test script can't be reliably
>> >>     --	# traced, unless it is run with a Bash version supporting
>> >>     --	# BASH_XTRACEFD (introduced in Bash v4.1).
>> >>     --	#
>> >>     --	# Perform this version check _after_ the test script was
>> >>     --	# potentially re-executed with $TEST_SHELL_PATH for '--tee' or
>> >>     --	# '--verbose-log', so the right shell is checked and the
>> >>     --	# warning is issued only once.
>> >>     --	if test -n "$BASH_VERSION" && eval '
>> >>     --	     test ${BASH_VERSINFO[0]} -gt 4 || {
>> >>     --	       test ${BASH_VERSINFO[0]} -eq 4 &&
>> >>     --	       test ${BASH_VERSINFO[1]} -ge 1
>> >>     --	     }
>> >>     --	   '
>> >>     --	then
>> >>     --		: Executed by a Bash version supporting BASH_XTRACEFD.  Good.
>> >>     --	else
>> >>     --		echo >&2 "warning: ignoring -x; '$0' is untraceable without BASH_XTRACEFD"
>> >>     --		trace=
>> >>     --	fi
>> >>     --fi
>> >>     - if test -n "$trace" && test -z "$verbose_log"
>> >>     - then
>> >>     - 	verbose=t
>> >>      @@ t/test-lib.sh: else
>> >>       	exec 4>/dev/null 3>/dev/null
>> >>       fi
>> >> -- 
>> >> 2.34.1.1024.g573f2f4b767
>> >> 
>> 
>> o