[PATCH 0/2] Set GIT_TRACE2_EVENT_NESTING in test-lib.sh

[PATCH 0/2] Set GIT_TRACE2_EVENT_NESTING in test-lib.sh

[Derrick Stolee via GitGitGadget]

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).

Thanks, -Stolee

Derrick Stolee (2):
  test-lib.sh: set GIT_TRACE2_EVENT_NESTING
  t/t*: remove custom GIT_TRACE2_EVENT_NESTING

 t/t1092-sparse-checkout-compatibility.sh | 6 +++---
 t/t4216-log-bloom.sh                     | 2 --
 t/t5310-pack-bitmaps.sh                  | 2 +-
 t/t5705-session-id-in-capabilities.sh    | 2 --
 t/t7519-status-fsmonitor.sh              | 2 +-
 t/test-lib.sh                            | 7 +++++++
 6 files changed, 12 insertions(+), 9 deletions(-)


base-commit: 35151cf0720460a897cde9b8039af364743240e7
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1085%2Fderrickstolee%2Ftrace2-depth-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1085/derrickstolee/trace2-depth-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1085
-- 
gitgitgadget

[PATCH 1/2] test-lib.sh: set GIT_TRACE2_EVENT_NESTING

[Derrick Stolee via GitGitGadget]

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
+
 # Use specific version of the index file format
 if test -n "${GIT_TEST_INDEX_VERSION:+isset}"
 then
-- 
gitgitgadget

[PATCH 2/2] t/t*: remove custom GIT_TRACE2_EVENT_NESTING

[Derrick Stolee via GitGitGadget]

From: Derrick Stolee <dstolee@microsoft.com>

The previous change modified GIT_TRACE2_EVENT_NESTING by default within
test-lib.sh. These custom assignments throughout the test suite are no
longer necessary.

Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
---
 t/t1092-sparse-checkout-compatibility.sh | 6 +++---
 t/t4216-log-bloom.sh                     | 2 --
 t/t5310-pack-bitmaps.sh                  | 2 +-
 t/t5705-session-id-in-capabilities.sh    | 2 --
 t/t7519-status-fsmonitor.sh              | 2 +-
 5 files changed, 5 insertions(+), 9 deletions(-)

diff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh
index 16fbd2c6db9..2de957905b0 100755
--- a/t/t1092-sparse-checkout-compatibility.sh
+++ b/t/t1092-sparse-checkout-compatibility.sh
@@ -688,7 +688,7 @@ test_expect_success 'submodule handling' '
 test_expect_success 'sparse-index is expanded and converted back' '
 	init_repos &&
 
-	GIT_TRACE2_EVENT="$(pwd)/trace2.txt" GIT_TRACE2_EVENT_NESTING=10 \
+	GIT_TRACE2_EVENT="$(pwd)/trace2.txt" \
 		git -C sparse-index -c core.fsmonitor="" reset --hard &&
 	test_region index convert_to_sparse trace2.txt &&
 	test_region index ensure_full_index trace2.txt
@@ -702,10 +702,10 @@ ensure_not_expanded () {
 	then
 		shift &&
 		test_must_fail env \
-			GIT_TRACE2_EVENT="$(pwd)/trace2.txt" GIT_TRACE2_EVENT_NESTING=10 \
+			GIT_TRACE2_EVENT="$(pwd)/trace2.txt" \
 			git -C sparse-index "$@" || return 1
 	else
-		GIT_TRACE2_EVENT="$(pwd)/trace2.txt" GIT_TRACE2_EVENT_NESTING=10 \
+		GIT_TRACE2_EVENT="$(pwd)/trace2.txt" \
 			git -C sparse-index "$@" || return 1
 	fi &&
 	test_region ! index ensure_full_index trace2.txt
diff --git a/t/t4216-log-bloom.sh b/t/t4216-log-bloom.sh
index 50f206db550..8018c12a6a4 100755
--- a/t/t4216-log-bloom.sh
+++ b/t/t4216-log-bloom.sh
@@ -175,13 +175,11 @@ test_expect_success 'persist filter settings' '
 	test_when_finished rm -rf .git/objects/info/commit-graph* &&
 	rm -rf .git/objects/info/commit-graph* &&
 	GIT_TRACE2_EVENT="$(pwd)/trace2.txt" \
-		GIT_TRACE2_EVENT_NESTING=5 \
 		GIT_TEST_BLOOM_SETTINGS_NUM_HASHES=9 \
 		GIT_TEST_BLOOM_SETTINGS_BITS_PER_ENTRY=15 \
 		git commit-graph write --reachable --changed-paths &&
 	grep "{\"hash_version\":1,\"num_hashes\":9,\"bits_per_entry\":15,\"max_changed_paths\":512" trace2.txt &&
 	GIT_TRACE2_EVENT="$(pwd)/trace2-auto.txt" \
-		GIT_TRACE2_EVENT_NESTING=5 \
 		git commit-graph write --reachable --changed-paths &&
 	grep "{\"hash_version\":1,\"num_hashes\":9,\"bits_per_entry\":15,\"max_changed_paths\":512" trace2-auto.txt
 '
diff --git a/t/t5310-pack-bitmaps.sh b/t/t5310-pack-bitmaps.sh
index dcf03d324a2..9104e842621 100755
--- a/t/t5310-pack-bitmaps.sh
+++ b/t/t5310-pack-bitmaps.sh
@@ -35,7 +35,7 @@ test_expect_success 'setup writing bitmaps during repack' '
 '
 
 test_expect_success 'full repack creates bitmaps' '
-	GIT_TRACE2_EVENT_NESTING=4 GIT_TRACE2_EVENT="$(pwd)/trace" \
+	GIT_TRACE2_EVENT="$(pwd)/trace" \
 		git repack -ad &&
 	ls .git/objects/pack/ | grep bitmap >output &&
 	test_line_count = 1 output &&
diff --git a/t/t5705-session-id-in-capabilities.sh b/t/t5705-session-id-in-capabilities.sh
index eb8c79aafdd..ed38c76c290 100755
--- a/t/t5705-session-id-in-capabilities.sh
+++ b/t/t5705-session-id-in-capabilities.sh
@@ -32,7 +32,6 @@ do
 		test_when_finished "git -C local push --delete origin new-branch" &&
 		cp -r "$LOCAL_PRISTINE" local &&
 		git -C local pull --no-rebase origin &&
-		GIT_TRACE2_EVENT_NESTING=5 \
 		GIT_TRACE2_EVENT="$(pwd)/tr2-client-events" \
 		git -c protocol.version=$PROTO -C local push \
 			--receive-pack "GIT_TRACE2_EVENT=\"$(pwd)/tr2-server-events\" git-receive-pack" \
@@ -65,7 +64,6 @@ do
 		test_when_finished "git -C local push --delete origin new-branch" &&
 		cp -r "$LOCAL_PRISTINE" local &&
 		git -C local pull --no-rebase origin &&
-		GIT_TRACE2_EVENT_NESTING=5 \
 		GIT_TRACE2_EVENT="$(pwd)/tr2-client-events" \
 		git -c protocol.version=$PROTO -C local push \
 			--receive-pack "GIT_TRACE2_EVENT=\"$(pwd)/tr2-server-events\" git-receive-pack" \
diff --git a/t/t7519-status-fsmonitor.sh b/t/t7519-status-fsmonitor.sh
index f488d930dfd..a5e2233cb19 100755
--- a/t/t7519-status-fsmonitor.sh
+++ b/t/t7519-status-fsmonitor.sh
@@ -390,7 +390,7 @@ test_expect_success 'status succeeds after staging/unstaging' '
 # during a call to 'git status'. Otherwise, we verify that we _do_ call it.
 check_sparse_index_behavior () {
 	git -C full status --porcelain=v2 >expect &&
-	GIT_TRACE2_EVENT="$(pwd)/trace2.txt" GIT_TRACE2_EVENT_NESTING=10 \
+	GIT_TRACE2_EVENT="$(pwd)/trace2.txt" \
 		git -C sparse status --porcelain=v2 >actual &&
 	test_region $1 index ensure_full_index trace2.txt &&
 	test_region fsm_hook query trace2.txt &&
-- 
gitgitgadget

Re: [PATCH 1/2] test-lib.sh: set GIT_TRACE2_EVENT_NESTING

[Ævar Arnfjörð Bjarmason]

On Mon, Nov 29 2021, Derrick Stolee via GitGitGadget wrote:

> 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?

Needing to dance around the value beint an integer or true/false is an
unfortunate side-effect of there not being two separate "enable nesting
guard?" and "nest level?" config knob, but there's not much to do about
that at this point, so I just mapped "false" to "-1" internally.

Maybe values of 0 and 1 should be an error in any case? I didn't check,
that would make this approach simpler.

diff --git a/Documentation/config/trace2.txt b/Documentation/config/trace2.txt
index fe1642f0d40..2be5474fdcd 100644
--- a/Documentation/config/trace2.txt
+++ b/Documentation/config/trace2.txt
@@ -35,10 +35,14 @@ trace2.eventBrief::
 	`GIT_TRACE2_EVENT_BRIEF` environment variable.  Defaults to false.
 
 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.
 
 trace2.configParams::
 	A comma-separated list of patterns of "important" config
diff --git a/trace2/tr2_tgt_event.c b/trace2/tr2_tgt_event.c
index 3a0014417cc..135d3fbd8c3 100644
--- a/trace2/tr2_tgt_event.c
+++ b/trace2/tr2_tgt_event.c
@@ -53,8 +53,22 @@ static int fn_init(void)
 		return want;
 
 	nesting = tr2_sysenv_get(TR2_SYSENV_EVENT_NESTING);
-	if (nesting && *nesting && ((max_nesting = atoi(nesting)) > 0))
-		tr2env_event_max_nesting_levels = max_nesting;
+	if (nesting) {
+		/*
+		 * TODO: Maybe expose the "off" part of
+		 * git_parse_maybe_bool_text() as
+		 * git_parse_maybe_bool_text_false() and use it here?
+		 * Note that we accept "GIT_TRACE2_EVENT_NESTING=" and
+		 * "trace2.eventNesting=" as "false", as with the rest
+		 * of the config mechanism and git_parse_maybe_bool()
+		 * users.
+		 */
+		if (!*nesting || !strcmp(nesting, "false") ||
+		    !strcmp(nesting, "no") || !strcmp(nesting, "off"))
+			tr2env_event_max_nesting_levels = -1;
+		else if (*nesting && ((max_nesting = atoi(nesting)) > 0))
+			tr2env_event_max_nesting_levels = max_nesting;
+	}
 
 	brief = tr2_sysenv_get(TR2_SYSENV_EVENT_BRIEF);
 	if (brief && *brief &&
@@ -503,6 +517,15 @@ static void fn_repo_fl(const char *file, int line,
 	jw_release(&jw);
 }
 
+static int nesting_ok(int nr_open_regions)
+{
+	if (tr2env_event_max_nesting_levels == -1)
+		return 1;
+	if (nr_open_regions <= tr2env_event_max_nesting_levels)
+		return 1;
+	return 0;
+}
+
 static void fn_region_enter_printf_va_fl(const char *file, int line,
 					 uint64_t us_elapsed_absolute,
 					 const char *category,
@@ -512,7 +535,7 @@ static void fn_region_enter_printf_va_fl(const char *file, int line,
 {
 	const char *event_name = "region_enter";
 	struct tr2tls_thread_ctx *ctx = tr2tls_get_self();
-	if (ctx->nr_open_regions <= tr2env_event_max_nesting_levels) {
+	if (nesting_ok(ctx->nr_open_regions)) {
 		struct json_writer jw = JSON_WRITER_INIT;
 
 		jw_object_begin(&jw, 0);
@@ -537,7 +560,7 @@ static void fn_region_leave_printf_va_fl(
 {
 	const char *event_name = "region_leave";
 	struct tr2tls_thread_ctx *ctx = tr2tls_get_self();
-	if (ctx->nr_open_regions <= tr2env_event_max_nesting_levels) {
+	if (nesting_ok(ctx->nr_open_regions)) {
 		struct json_writer jw = JSON_WRITER_INIT;
 		double t_rel = (double)us_elapsed_region / 1000000.0;
 
@@ -564,7 +587,7 @@ static void fn_data_fl(const char *file, int line, uint64_t us_elapsed_absolute,
 {
 	const char *event_name = "data";
 	struct tr2tls_thread_ctx *ctx = tr2tls_get_self();
-	if (ctx->nr_open_regions <= tr2env_event_max_nesting_levels) {
+	if (nesting_ok(ctx->nr_open_regions)) {
 		struct json_writer jw = JSON_WRITER_INIT;
 		double t_abs = (double)us_elapsed_absolute / 1000000.0;
 		double t_rel = (double)us_elapsed_region / 1000000.0;
@@ -592,7 +615,7 @@ static void fn_data_json_fl(const char *file, int line,
 {
 	const char *event_name = "data_json";
 	struct tr2tls_thread_ctx *ctx = tr2tls_get_self();
-	if (ctx->nr_open_regions <= tr2env_event_max_nesting_levels) {
+	if (nesting_ok(ctx->nr_open_regions)) {
 		struct json_writer jw = JSON_WRITER_INIT;
 		double t_abs = (double)us_elapsed_absolute / 1000000.0;
 		double t_rel = (double)us_elapsed_region / 1000000.0;

Re: [PATCH 1/2] test-lib.sh: set GIT_TRACE2_EVENT_NESTING

[Junio C Hamano]

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

> 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?

I agree that could be a better endgame if it works.

That is where our agreement ends.  I strongly prefer to have a
small and focused fix for immediate problem at hand, and then a
fundamental "(could be) better" change separately, so that we can
cool the latter longer.

This difference between us is not limited to this topic.  I often
get irritated to see an attempt to jump to the endgame with too big
a change in a single step.  Please don't.

Thanks.

Re: [PATCH 1/2] test-lib.sh: set GIT_TRACE2_EVENT_NESTING

[Junio C Hamano]

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

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

If the value of 5 set to .eventNesting allows up to 5 levels of
nesting, I would imagine some readers expect that "false" or "off"
would forbid nesting of events altogether.  For "false" to work
well, the variable needs renaming to trace2.limitEventNesting, or
something like that.

Re: [PATCH 0/2] Set GIT_TRACE2_EVENT_NESTING in test-lib.sh

[Jeff King]

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?

-Peff

Re: [PATCH 0/2] Set GIT_TRACE2_EVENT_NESTING in test-lib.sh

[Jeff King]

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.

-Peff

[PATCH] test-lib.sh: have all tests pass under "-x", remove BASH_XTRACEFD

[Ævar Arnfjörð Bjarmason]

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>
---

I hacked this up the other day when looking at the --verbose-log
v.s. -v failure[1] that's now being addressed in[2].

I'd originally run into that because I was trying to debug
t1510-repo-setup.sh and found that -x inexplicably didn't work, only
to discover it was he only consumer of "test_untracable".

1. https://lore.kernel.org/git/211125.86pmqoifp8.gmgdl@evledraar.gmail.com/
2. https://lore.kernel.org/git/pull.1085.git.1638193666.gitgitgadget@gmail.com/

 t/README              |  3 --
 t/t1510-repo-setup.sh | 34 +++++++++++++---------
 t/test-lib.sh         | 66 +++++--------------------------------------
 3 files changed, 27 insertions(+), 76 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/t1510-repo-setup.sh b/t/t1510-repo-setup.sh
index 591505a39c0..7eb65a52c08 100755
--- a/t/t1510-repo-setup.sh
+++ b/t/t1510-repo-setup.sh
@@ -40,8 +40,14 @@ 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_must_be_empty_trace () {
+	if want_trace
+	then
+		say "$TEST_NAME does not check test_must_be_empty on \"$@\" under -x"
+		return 0
+	fi
+	test_must_be_empty "$@"
+}
 
 TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
@@ -235,14 +241,14 @@ 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
+	test_must_be_empty_trace message
 '
 
 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
+	test_must_be_empty_trace message
 '
 
 test_expect_success '#2: worktree defaults to cwd with explicit GIT_DIR' '
@@ -269,7 +275,7 @@ test_expect_success '#4: core.worktree without GIT_DIR set is accepted' '
 	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
+	test_must_be_empty_trace message
 '
 
 test_expect_success '#5: core.worktree + GIT_WORK_TREE is accepted' '
@@ -280,7 +286,7 @@ test_expect_success '#5: 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
+	test_must_be_empty_trace message
 '
 
 test_expect_success '#6: setting GIT_DIR brings core.worktree to life' '
@@ -377,7 +383,7 @@ test_expect_success '#9: GIT_WORK_TREE accepted with gitfile' '
 	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
+	test_must_be_empty_trace message
 '
 
 test_expect_success '#10: GIT_DIR can point to gitfile' '
@@ -403,7 +409,7 @@ 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
+	test_must_be_empty_trace message
 '
 
 test_expect_success '#13: core.worktree+GIT_WORK_TREE accepted (with gitfile)' '
@@ -411,7 +417,7 @@ test_expect_success '#13: core.worktree+GIT_WORK_TREE accepted (with gitfile)' '
 	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
+	test_must_be_empty_trace message
 '
 
 # case #14.
@@ -566,7 +572,7 @@ 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
+	test_must_be_empty_trace message
 '
 
 test_expect_success '#18: bare .git named by GIT_DIR has no worktree' '
@@ -595,7 +601,7 @@ test_expect_success '#20a: core.worktree without GIT_DIR accepted (inside .git)'
 		"$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
+	test_must_be_empty_trace message
 '
 
 test_expect_success '#20b/c: core.worktree and core.bare conflict' '
@@ -627,7 +633,7 @@ test_expect_success '#21: setup, core.worktree warns before overriding core.bare
 		export GIT_WORK_TREE &&
 		git status >/dev/null
 	) 2>message &&
-	test_must_be_empty message
+	test_must_be_empty_trace message
 
 '
 run_wt_tests 21
@@ -743,7 +749,7 @@ test_expect_success '#25: GIT_WORK_TREE accepted if GIT_DIR unset (bare gitfile
 	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
+	test_must_be_empty_trace message
 '
 
 test_expect_success '#26: bare repo has no worktree (GIT_DIR -> gitfile case)' '
@@ -781,7 +787,7 @@ test_expect_success '#29: setup' '
 		export GIT_WORK_TREE &&
 		git status
 	) 2>message &&
-	test_must_be_empty message
+	test_must_be_empty_trace message
 '
 run_wt_tests 29 gitfile
 
diff --git a/t/test-lib.sh b/t/test-lib.sh
index 2679a7596a6..38a0fc558c9 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.841.ge54f711571c

Re: [PATCH] test-lib.sh: have all tests pass under "-x", remove BASH_XTRACEFD

[Eric Sunshine]

On Mon, Nov 29, 2021 at 5:20 PM Ævar Arnfjörð Bjarmason
<avarab@gmail.com> wrote:
> 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".

s/instead instead/instead/

> 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>

Re: [PATCH 0/2] Set GIT_TRACE2_EVENT_NESTING in test-lib.sh

[Taylor Blau]

On Mon, Nov 29, 2021 at 01:44:59PM -0500, Jeff King wrote:
> 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.

Yeah, I agree. This test in particular must have bad luck, because I
noticed the same failure in GitHub's fork when merging with 2.33 (but
due to some custom trace2 regions causing us to blow past the limit even
without `-v`).

It might be nice to allow setting 'GIT_TRACE2_EVENT_NESTING=false' or
something similar, but let's deal with that elsewhere.

hanks,
Taylor

Re: [PATCH 0/2] Set GIT_TRACE2_EVENT_NESTING in test-lib.sh

[Derrick Stolee]

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

Re: [PATCH] test-lib.sh: have all tests pass under "-x", remove BASH_XTRACEFD

[Jeff King]

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.

-Peff

Re: [PATCH] test-lib.sh: have all tests pass under "-x", remove BASH_XTRACEFD

[Ævar Arnfjörð Bjarmason]

On Tue, Nov 30 2021, 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. 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.

I'd be fine with a hard dependency on bash, but that seems unlikely to
happen, and if that's not the case having these hybrid modes seems like
the worst of both worlds.

We already hard depend on -x working without BASH_XTRACEFD for all tests
except this one test don't we? I.e our CI runs with --verbose-log -x,
and t1510-repo-setup.sh was the only test that was turning it off, and
some of that runs with a /bin/sh of "dash".

Now when that test fails we'll get nice -x output like for every other
test, and AFAICT nothing else is changed by this patch, except the
obvious change of us not having to support these two modes anymore.

Re: [PATCH 0/2] Set GIT_TRACE2_EVENT_NESTING in test-lib.sh

[Jeff Hostetler]

On 11/30/21 10:34 AM, Derrick Stolee wrote:
> 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
> 


yeah, I set a fairly conservative nesting limit.  my fear was that
someone would add a region or data event to something that recurses
deeply -- like treewalking or directory scanning -- which in a monorepo
would be bad since we might get millions of rows of data.

most of the useful timing data comes from the higher level regions,
such as we spent x% scanning tracked files and y% scanning untracked
files.  once we get down inside a particular "concept", the directory-
by-directory numbers aren't usually that useful (and/or the overhead of
logging every directory we visit dominates).

WRT allowing data events to ignore the nesting limit, we have the
same monorepo risk if for example someone logs how many files were
in each directory visited, the name of every untracked file, or every
file that had some property of interest.  That might look fine on a
shallow WT, but might generate an unusable amount of data on a large
repo.  so i'd recommend we proceed carefully here.  (i'm not against
it, i'm just saying we should think about it.)


using trace2 in a test to confirm that a piece of code was visited
or not visited is cool, but something i don't think i anticipated.
so i agree that we should increase the limit in the test suite.


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.

(i'd also consider changing the "total_objects" data event to include
include the region title so that they are more easily identified in a
grep, but I digress.)


Thanks,
Jeff

Re: [PATCH] test-lib.sh: have all tests pass under "-x", remove BASH_XTRACEFD

[SZEDER Gábor]

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.

Re: [PATCH] test-lib.sh: have all tests pass under "-x", remove BASH_XTRACEFD

[Ævar Arnfjörð Bjarmason]

On Tue, Nov 30 2021, SZEDER Gábor wrote:

> 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?

Re: [PATCH] test-lib.sh: have all tests pass under "-x", remove BASH_XTRACEFD

[Junio C Hamano]

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

> 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,

Even more preferrable is not to skip the tests, no?  Is this because
we capture the standard error stream and do not care where, between
the command being tested and the shell running the command, the
output came from?

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

If the objective is to work around the "shell trace and program
output are mixed together" issue, BASH_XTRACEFD is doing much more
superb job to "solve" it than "some tests depend on running without
'-x', so punt and skip them" to "work around" it, I would have to
say.

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

It is much simpler to punt and skip all tests.  You can even have
"if run with -x stop and skip all" in early part of test-lib.sh and
that would "work around" without developers constantly having to
worry about the distinction between test_must_be_empty and the trace
variant.

It is a sad that you have to trade one good thing for another.
Somehow, this change feels like a regression, not an improvement.

Re: [PATCH] test-lib.sh: have all tests pass under "-x", remove BASH_XTRACEFD

[Jeff King]

On Wed, Dec 01, 2021 at 03:06:44PM +0100, Ævar Arnfjörð Bjarmason wrote:

> 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?

I don't know that I'd call it purely hypothetical. It was the state for
many years in the past. And there has to be per-test hackery to keep it
that way. So I wouldn't call XTRACEFD a workaround at all; in my mind it
was the solution.

Now if people want to do that per-test hackery _also_, because it keeps
"-x" more useful for folks who can't or won't use bash, I have no
problem with that. What I was hoping to avoid was making it a
hard requirement, because now it's one more subtle thing for test
writers to have to remember and deal with.

But maybe that ship is sailed. If we are running with "-x" and dash in
CI, then they're going to see confusing failures there regardless.

-Peff

Re: [PATCH 0/2] Set GIT_TRACE2_EVENT_NESTING in test-lib.sh

[Jeff King]

On Tue, Nov 30, 2021 at 05:43:28PM -0500, Jeff Hostetler wrote:

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

Yeah, that would at least make these things deterministic, and we'd
notice a nesting problem in the tests.

I do think bumping the default nesting level a few notches may be worth
it. Even if we didn't get rid of it entirely (and I am sympathetic to
the notion that recursive operations could produce gigantic logs), that
would give some headroom for reasonable levels of logging.

-Peff

[PATCH v2 0/2] test-lib.sh: have all tests pass under "-x", remove BASH_XTRACEFD

[Ævar Arnfjörð Bjarmason]

I think this v2 should nicely address the outstanding comments on
v1. I.e. v1 made the trade-off of not caring about -x in t1510 for
some of its assertions.

But as noted in the new 1/2 here we can get a much better and easily
debuggable test regardless of which shell we use if we don't use the
pattern that's being worked around by test_untraceable.

Which, is the updated 2/2 notes is mostly what SZEDER was doing in
2018 when "test_untraceable" was introduced, it's just that t1510
wasn't migrated over at the time, as some other tests in the his
series discussed in 2/2 were.

Ævar Arnfjörð Bjarmason (2):
  t1510: remove need for "test_untraceable", retain coverage
  test-lib.sh: remove the now-unused "test_untraceable" facility

 t/README              |  3 --
 t/t1510-repo-setup.sh | 83 +++++++++++++++++++++----------------------
 t/test-lib.sh         | 66 ++++------------------------------
 3 files changed, 47 insertions(+), 105 deletions(-)

Range-diff against v1:
-:  ----------- > 1:  91402624777 t1510: remove need for "test_untraceable", retain coverage
1:  9f735bd0d49 ! 2:  867d18d14bd test-lib.sh: have all tests pass under "-x", remove BASH_XTRACEFD
    @@ Metadata
     Author: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
     
      ## Commit message ##
    -    test-lib.sh: have all tests pass under "-x", remove BASH_XTRACEFD
    +    test-lib.sh: remove the now-unused "test_untraceable" facility
     
    -    Change the "t1510-repo-setup.sh" test to use a new
    -    "test_must_be_empty_trace" wrapper, instead of turning on
    -    "test_untraceable=UnfortunatelyYes".
    +    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).
     
    -    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".
    +    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.
     
    -    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.
    +    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).
     
    -    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.
    +    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.
     
    -    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.
    +    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: appropriately before running "make". Short options can be bundled, i.e
      -d::
      --debug::
     
    - ## t/t1510-repo-setup.sh ##
    -@@ t/t1510-repo-setup.sh: 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_must_be_empty_trace () {
    -+	if want_trace
    -+	then
    -+		say "$TEST_NAME does not check test_must_be_empty on \"$@\" under -x"
    -+		return 0
    -+	fi
    -+	test_must_be_empty "$@"
    -+}
    - 
    - TEST_PASSES_SANITIZE_LEAK=true
    - . ./test-lib.sh
    -@@ t/t1510-repo-setup.sh: 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
    -+	test_must_be_empty_trace message
    - '
    - 
    - 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
    -+	test_must_be_empty_trace message
    - '
    - 
    - test_expect_success '#2: worktree defaults to cwd with explicit GIT_DIR' '
    -@@ t/t1510-repo-setup.sh: test_expect_success '#4: core.worktree without GIT_DIR set is accepted' '
    - 	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
    -+	test_must_be_empty_trace message
    - '
    - 
    - test_expect_success '#5: core.worktree + GIT_WORK_TREE is accepted' '
    -@@ t/t1510-repo-setup.sh: test_expect_success '#5: 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
    -+	test_must_be_empty_trace message
    - '
    - 
    - test_expect_success '#6: setting GIT_DIR brings core.worktree to life' '
    -@@ t/t1510-repo-setup.sh: test_expect_success '#9: GIT_WORK_TREE accepted with gitfile' '
    - 	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
    -+	test_must_be_empty_trace message
    - '
    - 
    - test_expect_success '#10: GIT_DIR can point to gitfile' '
    -@@ t/t1510-repo-setup.sh: 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
    -+	test_must_be_empty_trace message
    - '
    - 
    - test_expect_success '#13: core.worktree+GIT_WORK_TREE accepted (with gitfile)' '
    -@@ t/t1510-repo-setup.sh: test_expect_success '#13: core.worktree+GIT_WORK_TREE accepted (with gitfile)' '
    - 	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
    -+	test_must_be_empty_trace message
    - '
    - 
    - # case #14.
    -@@ t/t1510-repo-setup.sh: 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
    -+	test_must_be_empty_trace message
    - '
    - 
    - test_expect_success '#18: bare .git named by GIT_DIR has no worktree' '
    -@@ t/t1510-repo-setup.sh: test_expect_success '#20a: core.worktree without GIT_DIR accepted (inside .git)'
    - 		"$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
    -+	test_must_be_empty_trace message
    - '
    - 
    - test_expect_success '#20b/c: core.worktree and core.bare conflict' '
    -@@ t/t1510-repo-setup.sh: test_expect_success '#21: setup, core.worktree warns before overriding core.bare
    - 		export GIT_WORK_TREE &&
    - 		git status >/dev/null
    - 	) 2>message &&
    --	test_must_be_empty message
    -+	test_must_be_empty_trace message
    - 
    - '
    - run_wt_tests 21
    -@@ t/t1510-repo-setup.sh: test_expect_success '#25: GIT_WORK_TREE accepted if GIT_DIR unset (bare gitfile
    - 	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
    -+	test_must_be_empty_trace message
    - '
    - 
    - test_expect_success '#26: bare repo has no worktree (GIT_DIR -> gitfile case)' '
    -@@ t/t1510-repo-setup.sh: test_expect_success '#29: setup' '
    - 		export GIT_WORK_TREE &&
    - 		git status
    - 	) 2>message &&
    --	test_must_be_empty message
    -+	test_must_be_empty_trace message
    - '
    - run_wt_tests 29 gitfile
    - 
    -
      ## t/test-lib.sh ##
     @@ t/test-lib.sh: then
      	exit
-- 
2.34.1.876.gdb91009a90c

[PATCH v2 1/2] t1510: remove need for "test_untraceable", retain coverage

[Ævar Arnfjörð Bjarmason]

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

[PATCH v2 2/2] test-lib.sh: remove the now-unused "test_untraceable" facility

[Ævar Arnfjörð Bjarmason]

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.876.gdb91009a90c

Re: [PATCH v2 1/2] t1510: remove need for "test_untraceable", retain coverage

[SZEDER Gábor]

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.

>  		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
> 

Re: [PATCH v2 1/2] t1510: remove need for "test_untraceable", retain coverage

[SZEDER Gábor]

On Thu, Dec 02, 2021 at 08:16:35PM +0100, SZEDER Gábor wrote:
> 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.

Ah, that's because this hunk is executed in a subshell that starts
with 'cd "$1"', creating an extra 'stderr' file in a subdirectory:

  $ ./t1510-repo-setup.sh -r 1 -d
  [...]
  $ find trash\ directory.t1510-repo-setup/ |grep stderr
  trash directory.t1510-repo-setup/0/stderr
  trash directory.t1510-repo-setup/0/sub/stderr

Changing that redirection to '2>>"$TRASH_DIRECTORY"/stderr' makes a
bunch of tests fail with:

  + test_must_be_empty stderr
  'stderr' is not empty, it contains:
  + pwd
  error: last command exited with $?=1
  + rm stderr
  not ok 1 - #0: nonbare repo, no explicit configuration

Re: [PATCH v2 1/2] t1510: remove need for "test_untraceable", retain coverage

[Jeff King]

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

[PATCH v3 0/2] test-lib.sh: have all tests pass under "-x", remove BASH_XTRACEFD

[Ævar Arnfjörð Bjarmason]

This re-roll fixes a stupid mistake of mine pointed out by SZEDER[1]
(thanks!): I was redirecting to "stderr" across function calls, but
some of those changed directories. Now we'll pass around an absolute
path from the top-level function.

That bug hid an issue where we'd include the trace output in that
stderr log still due to an interpolation of $(pwd), we can just change
that to $PWD, which won't have that issue.

1. https://lore.kernel.org/git/20211202192802.GC1991@szeder.dev/

Ævar Arnfjörð Bjarmason (2):
  t1510: remove need for "test_untraceable", retain coverage
  test-lib.sh: remove the now-unused "test_untraceable" facility

 t/README              |  3 --
 t/t1510-repo-setup.sh | 85 +++++++++++++++++++++----------------------
 t/test-lib.sh         | 66 ++++-----------------------------
 3 files changed, 49 insertions(+), 105 deletions(-)

Range-diff against v2:
1:  91402624777 ! 1:  7876202c5b0 t1510: remove need for "test_untraceable", retain coverage
    @@ Commit message
         use of "test_when_finished"), but the pattern enforces better test
         hygiene.
     
    +    The functions it calls might change directories, so we pass an
    +    absolute "$stderr_log_path" around. We also need to change a "$(pwd)"
    +    to "$PWD" in test_repo(), on e.g. "dash" that interpolation will be
    +    traced and part of the "2>>" redirection.
    +
    +    1. https://lore.kernel.org/git/YbMiK1wHzBfYvK2a@coredump.intra.peff.net/
    +
         Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
     
      ## t/t1510-repo-setup.sh ##
    @@ t/t1510-repo-setup.sh: test_repo () {
      		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 &&
    ++		GIT_TRACE_SETUP="$PWD/trace" git symbolic-ref HEAD >/dev/null 2>>"$stderr_log_path" &&
      		grep '^setup: ' trace >result &&
      		test_cmp expected result
      	)
    @@ t/t1510-repo-setup.sh: maybe_config () {
      	if test "$value" != unset
      	then
     -		git config --file="$file" "$var" "$value"
    -+		git config --file="$file" "$var" "$value" 2>>stderr
    ++		git config --file="$file" "$var" "$value" 2>>"$stderr_log_path"
      	fi
      }
      
    @@ t/t1510-repo-setup.sh: setup_repo () {
      	sane_unset GIT_DIR GIT_WORK_TREE &&
      
     -	git -c init.defaultBranch=initial init "$name" &&
    -+	git -c init.defaultBranch=initial init "$name" 2>>stderr &&
    ++	git -c init.defaultBranch=initial init "$name" 2>>"$stderr_log_path" &&
      	maybe_config "$name/.git/config" core.worktree "$worktreecfg" &&
      	maybe_config "$name/.git/config" core.bare "$barecfg" &&
      	mkdir -p "$name/sub/sub" &&
    @@ t/t1510-repo-setup.sh: run_wt_tests () {
      #	(git dir) (work tree) (cwd) (prefix)	<-- from subdir
      try_repo () {
      	name=$1 worktreeenv=$2 gitdirenv=$3 &&
    -+	test_when_finished "rm stderr" &&
    ++	stderr_log_path="$PWD/stderr" &&
    ++
    ++	test_when_finished "rm \"$stderr_log_path\"" &&
      	setup_repo "$name" "$4" "$5" "$6" &&
      	shift 6 &&
      	try_case "$name" "$worktreeenv" "$gitdirenv" \
      		"$1" "$2" "$3" "$4" &&
    -+	test_must_be_empty stderr &&
    ++	test_must_be_empty "$stderr_log_path" &&
      	shift 4 &&
      	case "$gitdirenv" in
      	/* | ?:/* | unset) ;;
    @@ t/t1510-repo-setup.sh: try_repo () {
      	try_case "$name/sub" "$worktreeenv" "$gitdirenv" \
     -		"$1" "$2" "$3" "$4"
     +		"$1" "$2" "$3" "$4" &&
    -+	test_must_be_empty stderr
    ++	test_must_be_empty "$stderr_log_path"
      }
      
      # Bit 0 = GIT_WORK_TREE
2:  867d18d14bd = 2:  a7fc794e20d test-lib.sh: remove the now-unused "test_untraceable" facility
-- 
2.34.1.932.g36842105b61

[PATCH v3 1/2] t1510: remove need for "test_untraceable", retain coverage

[Ævar Arnfjörð Bjarmason]

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.

The functions it calls might change directories, so we pass an
absolute "$stderr_log_path" around. We also need to change a "$(pwd)"
to "$PWD" in test_repo(), on e.g. "dash" that interpolation will be
traced and part of the "2>>" redirection.

1. https://lore.kernel.org/git/YbMiK1wHzBfYvK2a@coredump.intra.peff.net/

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/t1510-repo-setup.sh | 85 +++++++++++++++++++++----------------------
 1 file changed, 42 insertions(+), 43 deletions(-)

diff --git a/t/t1510-repo-setup.sh b/t/t1510-repo-setup.sh
index 591505a39c0..fc94d8b49be 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_log_path" &&
 		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_log_path"
 	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_log_path" &&
 	maybe_config "$name/.git/config" core.worktree "$worktreecfg" &&
 	maybe_config "$name/.git/config" core.bare "$barecfg" &&
 	mkdir -p "$name/sub/sub" &&
@@ -210,10 +207,14 @@ run_wt_tests () {
 #	(git dir) (work tree) (cwd) (prefix)	<-- from subdir
 try_repo () {
 	name=$1 worktreeenv=$2 gitdirenv=$3 &&
+	stderr_log_path="$PWD/stderr" &&
+
+	test_when_finished "rm \"$stderr_log_path\"" &&
 	setup_repo "$name" "$4" "$5" "$6" &&
 	shift 6 &&
 	try_case "$name" "$worktreeenv" "$gitdirenv" \
 		"$1" "$2" "$3" "$4" &&
+	test_must_be_empty "$stderr_log_path" &&
 	shift 4 &&
 	case "$gitdirenv" in
 	/* | ?:/* | unset) ;;
@@ -221,7 +222,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_log_path"
 }
 
 # Bit 0 = GIT_WORK_TREE
@@ -234,15 +236,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 +268,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 +377,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 +402,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 +547,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 +562,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 +589,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 +623,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 +739,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 +778,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.932.g36842105b61

[PATCH v3 2/2] test-lib.sh: remove the now-unused "test_untraceable" facility

[Ævar Arnfjörð Bjarmason]

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

Re: [PATCH v2 1/2] t1510: remove need for "test_untraceable", retain coverage

[Ævar Arnfjörð Bjarmason]

On Fri, Dec 10 2021, Jeff King wrote:

> 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

I just re-rolled a v3 with a fix for this and the general issue pointed
out by SZEDER, thanks both.

But as far as keeping test_untraceable goes, isn't this a good argument
for dropping it?

I.e. if bash was the odd shell out that included the $(...) command in
the trace output it would be a good reason to keep it, then we could
perhaps use it without worrying about our trace output being corrupted
when we did a "test_cmp" on "stderr" later.

But it's "dash" that's including it, so unless we're going to outright
forbid it to have -x output (which is inconvienent) we'll need to deal
with t either way (in my re-roll I just replaced $(pwd) with $PWD).

Re: [PATCH v3 2/2] test-lib.sh: remove the now-unused "test_untraceable" facility

[SZEDER Gábor]

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.

>  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
> 

Re: [PATCH v3 2/2] test-lib.sh: remove the now-unused "test_untraceable" facility

[Ævar Arnfjörð Bjarmason]

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/?

Re: [PATCH v3 2/2] test-lib.sh: remove the now-unused "test_untraceable" facility

[SZEDER Gábor]

On Sun, Dec 12, 2021 at 06:06:31PM +0100, Ævar Arnfjörð Bjarmason wrote:
> 
> 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

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.

[PATCH v4 0/3] test-lib.sh: have all tests pass under "-x", remove BASH_XTRACEFD

[Ævar Arnfjörð Bjarmason]

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.

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

[PATCH v4 1/3] t1510: remove need for "test_untraceable", retain coverage

[Ævar Arnfjörð Bjarmason]

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.

The functions it calls might change directories, so we pass an
absolute "$stderr_log_path" around. We also need to change a "$(pwd)"
to "$PWD" in test_repo(), on e.g. "dash" that interpolation will be
traced and part of the "2>>" redirection.

1. https://lore.kernel.org/git/YbMiK1wHzBfYvK2a@coredump.intra.peff.net/

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/t1510-repo-setup.sh | 85 +++++++++++++++++++++----------------------
 1 file changed, 42 insertions(+), 43 deletions(-)

diff --git a/t/t1510-repo-setup.sh b/t/t1510-repo-setup.sh
index 591505a39c0..fc94d8b49be 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_log_path" &&
 		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_log_path"
 	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_log_path" &&
 	maybe_config "$name/.git/config" core.worktree "$worktreecfg" &&
 	maybe_config "$name/.git/config" core.bare "$barecfg" &&
 	mkdir -p "$name/sub/sub" &&
@@ -210,10 +207,14 @@ run_wt_tests () {
 #	(git dir) (work tree) (cwd) (prefix)	<-- from subdir
 try_repo () {
 	name=$1 worktreeenv=$2 gitdirenv=$3 &&
+	stderr_log_path="$PWD/stderr" &&
+
+	test_when_finished "rm \"$stderr_log_path\"" &&
 	setup_repo "$name" "$4" "$5" "$6" &&
 	shift 6 &&
 	try_case "$name" "$worktreeenv" "$gitdirenv" \
 		"$1" "$2" "$3" "$4" &&
+	test_must_be_empty "$stderr_log_path" &&
 	shift 4 &&
 	case "$gitdirenv" in
 	/* | ?:/* | unset) ;;
@@ -221,7 +222,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_log_path"
 }
 
 # Bit 0 = GIT_WORK_TREE
@@ -234,15 +236,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 +268,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 +377,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 +402,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 +547,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 +562,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 +589,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 +623,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 +739,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 +778,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.1024.g573f2f4b767

[PATCH v4 2/3] test-lib.sh: remove the now-unused "test_untraceable" facility

[Ævar Arnfjörð Bjarmason]

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 | 23 -----------------------
 2 files changed, 26 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..ddfa0567e9e 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
-- 
2.34.1.1024.g573f2f4b767

[PATCH v4 3/3] test-lib.sh: remove "BASH_XTRACEFD"

[Ævar Arnfjörð Bjarmason]

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.

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"):

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

And finally that:

    "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 | 43 +++++++------------------------------------
 1 file changed, 7 insertions(+), 36 deletions(-)

diff --git a/t/test-lib.sh b/t/test-lib.sh
index ddfa0567e9e..b008716917b 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -627,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
@@ -926,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.1024.g573f2f4b767

Re: [PATCH v4 0/3] test-lib.sh: have all tests pass under "-x", remove BASH_XTRACEFD

[SZEDER Gábor]

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.

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.

> 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
> 

Re: [PATCH v3 2/2] test-lib.sh: remove the now-unused "test_untraceable" facility

[Junio C Hamano]

SZEDER Gábor <szeder.dev@gmail.com> writes:

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

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"?

Re: [PATCH v3 2/2] test-lib.sh: remove the now-unused "test_untraceable" facility

[Jeff King]

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, 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).

Maybe that's OK. We've had fewer incidences of the problem popping up
than I would have expected.

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.

> 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 fixes from 571e472dc4 (Merge branch 'sg/test-x', 2018-03-14)
show what had to be done to get where we are today.

-Peff

Re: [PATCH v3 2/2] test-lib.sh: remove the now-unused "test_untraceable" facility

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> 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

Yeah, that is a simple example that clearly shows how removal of
BASH_XTRACEFD makes developer's life horrible.

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

Yeah, that matches my understanding.  Unfortunately we cannot easily
remove "dash -x" from CI while keeping "dash" without "-x" X-<.

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?

Re: [PATCH v3 2/2] test-lib.sh: remove the now-unused "test_untraceable" facility

[Jeff King]

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.

-Peff

Re: [PATCH v3 2/2] test-lib.sh: remove the now-unused "test_untraceable" facility

[Junio C Hamano]

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 ;-)

Re: [PATCH v3 2/2] test-lib.sh: remove the now-unused "test_untraceable" facility

[Ævar Arnfjörð Bjarmason]

[Replying to the thread-at-large]

On Tue, Dec 14 2021, Jeff King wrote:

> 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[...]

Yes, I agree that's much better than what you need to do when not under
bash and when you can't benefit from BASH_XTRACEFD.

But unless we're talking about requiring bash and/or not supporting -x
at all (which seems to be overkill, seeing as only one test scipt wasn't
compatible with it) this discussion seems a bit like talking about how
some code is nicer in C17 than C99 or C89.

Sure, it is. But if you're also supporting the latter two it's usually
not worth it to maintain both with an ifdef.

Similarly, we can't really get any real benefits from BASH_XTRACEFD as
long as we're going to support "-x" with other shells.

Literally the only part of the test suite where we hard-depended on it
is the code adjusted in my 1/2 here. I do think if we could rely on the
pre-image it would be nicer, but not so nice that I don't want "-x"
working under "dash".

And in any case carrying a "BASH_XTRACEFD" seems to be just a dead end
for that reason.

On Wed, Dec 15 2021, Junio C Hamano wrote:

> 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 ;-)

Yes, I've run into those cases and I don't think BASH_XTRACEFD really
helps at all. Yes you'll get a test failure because trace output got
into something we're invoking "test_cmp" on, or whatever.

But that's never a subtle failure, and the fix for it brings us back to
the question of whether we're going to support non-bash, or take the
more drastic step of marking the whole test script as bash-only under
"-x".

I think the answers to those are always going to be "yes" and "no", and
thus we're not getting any benefit from "BASH_XTRACEFD".

Re: [PATCH v2 1/2] t1510: remove need for "test_untraceable", retain coverage

[SZEDER Gábor]

On Fri, Dec 10, 2021 at 04:47:23AM -0500, Jeff King wrote:
> 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:

Indeed, although POSIX seems to be quite clear about what should
happen in this case: the specs for simple commands [1] state that
redirections should be performed before variable assignments are
expanded for, among other things, command substitution.

>   $ 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

So in case of these commands the shell should first redirect stdout
and stderr, then expand the command substitution, thus it should write
the trace for the 'echo foo' within to stderr _while_ stderr is
redirected.  It seems that dash, for once, does conform to POSIX.

Although the standard-conform behavior is potentially more problematic
for us, because it would cause test failure with tracing enabled when
used like this:

  GIT_TRACE="$(pwd)/trace" git cmd --opts 2>actual.err &&
  test_cmp expected.err actual.err

because the "+ pwd" trace output from the command substitution would
go to 'err.actual'; 9b2ac68f27 (t5526: use $TRASH_DIRECTORY to specify
the path of GIT_TRACE log file, 2018-02-24) fixed a case like this.


[1] https://pubs.opengroup.org/onlinepubs/9699919799/utilities/V3_chap02.html#tag_18_09_01

Re: [PATCH v4 0/3] test-lib.sh: have all tests pass under "-x", remove BASH_XTRACEFD

[Ævar Arnfjörð Bjarmason]

[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

Re: [PATCH v4 0/3] test-lib.sh: have all tests pass under "-x", remove BASH_XTRACEFD

[SZEDER Gábor]

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

Re: [PATCH v4 0/3] test-lib.sh: have all tests pass under "-x", remove BASH_XTRACEFD

[Ævar Arnfjörð Bjarmason]

On Mon, Feb 21 2022, SZEDER Gábor wrote:

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

I understand, thanks.

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

Yes I agree that would be a bad argument, but it's not the one I'm
making.

The one I am making is in
https://lore.kernel.org/git/211216.864k78bsjs.gmgdl@evledraar.gmail.com

I.e. I agree that it's a useful feature, and I wish in the abstract that we
could make real use of it.

But that would mean a hard dependency on bash, which I don't think would
be acceptable, or something anyone's advocating for.

As long as we're not going down that road I don't see the point in using
it.

The only practical use of it in the test suite is to support a
special-case I'm making un-special in 1/3, because I wanted "-x" output
without needing to run bash. As it turned out it wasn't hard to do, and
is consistent with how other tests are written).

So what's your argument for it? I.e. how are we practically going to use
it in a way that makes a difference?

I only see us not really using it, because our behavior in practice will
1=1 mirror non-bash shells.

By keeping it around we're only exposing ourselves to edge cases and
hiding bugs that we'd like to fix sooner than later anyway (and which
are fixed as of this 1/3, and before that mostly were in your earlier
series).

So would test code that depended on bash and BASH_XTRACEFD be more
"reliable and robust"? Absolutely, you wouldn't need to worry about some
interpolated $(pwd) or whatever in a string getting into your -x output
when you didn't expect it.

But with how we're using it it's doing the opposite of that. It'll only
hide those bugs for non-bash users.

I may be entirely wrong, or perhaps I haven't considered some edge case
or trade-off you have in mind. But I'm not able to bridge that gap with
your rather terse replies :)

>> 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

Re: [PATCH v4 3/3] test-lib.sh: remove "BASH_XTRACEFD"

[SZEDER Gábor]

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.

Re: [PATCH v4 3/3] test-lib.sh: remove "BASH_XTRACEFD"

[Ævar Arnfjörð Bjarmason]

On Tue, Feb 22 2022, SZEDER Gábor wrote:

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

Yes we could definitely live with it. My aim here was to tighten up the
cycle of testing, submitting/pushing a patch, having CI fail because
that test was on bash only etc.

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

Sure, if you feel that strongly about it I'm fine with dropping this
version and submitting a re-roll without the test-lib.sh changes.

I just genuinely didn't grok why you wanted to keep it before.

But I get it now, thanks.

I think it's fair to say that it's for the exact same reason I'd like to
get rid of it, you just think of it as a feature, not a bug :)

I.e. the "landmine" of getting something working under bash, only to
find out later that it fails on other shells.

But everyone's workflow is different. It sounds as though you sometimes
start hacking tests to use bash-specific features that you later distill
down to POSIX-compatibility.

Which I don't think I've ever done, since doing so is going to require a
rewrite anyway I might as well struggle to get it POSIX-ly working in
the first place.

But I can see how that's useful, and I don't want to take that away from
you.

Do you feel equally strongly that this mode of operation must be the
default under bash, or that it would be OK to have it be opt-in under a
knob like GIT_TEST_BASHISMS=true, or something like that?

The reason I ran into this in the first place was because I saw such
unexplained behavior changes with bash v.s. non-bash, only to arrive at
this part of test-lib.sh (which I'm sure I'd read N times before, but
had forgotten about).

I don't want to make your local workflow harder, but I'd think it would
be fair to say that the average git.git developer won't be as keenly
aware of shell differences as you, and would be better served by having
this subtlety be opt-in.

I.e. it's surely more likely that someone is to write POSIX-compatible
test code for ML submission, than intentionally using a bash-only
feature for prototyping, which will start breaking once it's exposed to
dash et al in CI.

Or, if you feel strongly about that too I wouldn't mind much if
"GIT_TEST_BASHISMS=true" was the default, as long as I can set it to
"GIT_TEST_BASHISMS=false" and /other/ bash-specific behavior without
further opting it in to optional bash-only behavior that's off by
default.

Assuming you'd reply "no" to not wanting such a knob *and* having it by
off by default, would that be a "yes" to having the knob and needing to
turn it off?

Thanks!