{"thread":{"id":"56994","subject":"[PATCH 0/2] Set GIT_TRACE2_EVENT_NESTING in test-lib.sh","startedAt":"2021-11-29T15:40:40Z","lastAt":"2022-02-22T15:30:25Z","messageCount":50,"participants":["Derrick Stolee via GitGitGadget","Ævar Arnfjörð Bjarmason","Junio C Hamano","Jeff King","Eric Sunshine","Taylor Blau","Derrick Stolee","Jeff Hostetler","SZEDER Gábor"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"442441","messageId":"pull.1085.git.1638193666.gitgitgadget@gmail.com","threadId":"56994","inReplyTo":null,"subject":"[PATCH 0/2] Set GIT_TRACE2_EVENT_NESTING in test-lib.sh","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-11-29T13:47:44Z","receivedAt":"2021-11-29T15:40:40Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"As reported by Ævar [1] and diagnosed by Peff in a reply, the default\nGIT_TRACE2_EVENT_NESTING is set so low that tests that look for trace2\nevents can start failing due to an added trace2 region. This can even be\nsubtle, including the fact that the progress API adds trace2 regions when in\nuse (which can depend on the verbose output of a test script).\n\nThanks, -Stolee\n\nDerrick Stolee (2):\n test-lib.sh: set GIT_TRACE2_EVENT_NESTING\n t/t*: remove custom GIT_TRACE2_EVENT_NESTING\n\n t/t1092-sparse-checkout-compatibility.sh | 6 +++---\n t/t4216-log-bloom.sh | 2 --\n t/t5310-pack-bitmaps.sh | 2 +-\n t/t5705-session-id-in-capabilities.sh | 2 --\n t/t7519-status-fsmonitor.sh | 2 +-\n t/test-lib.sh | 7 +++++++\n 6 files changed, 12 insertions(+), 9 deletions(-)\n\n\nbase-commit: 35151cf0720460a897cde9b8039af364743240e7\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1085%2Fderrickstolee%2Ftrace2-depth-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1085/derrickstolee/trace2-depth-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/1085\n-- \ngitgitgadget\n"},{"id":"442442","messageId":"3d1bbc91aa3a465ecec2de130456b9ccc08f3032.1638193666.git.gitgitgadget@gmail.com","threadId":"56994","inReplyTo":"pull.1085.git.1638193666.gitgitgadget@gmail.com","subject":"[PATCH 1/2] test-lib.sh: set GIT_TRACE2_EVENT_NESTING","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-11-29T13:47:45Z","receivedAt":"2021-11-29T15:41:09Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee \n\nThe GIT_TRACE2_EVENT feed has a limited nesting depth to avoid\noverloading the feed when recursing into deep paths while adding more\nnested regions.\n\nSome tests use the GIT_TRACE2_EVENT feed to look for internal events,\nensuring that intended behavior is happening.\n\nOne such example is in t4216-log-bloom.sh which looks for a statistic\ngiven as a trace2_data_intmax() call. This test started failing under\n'-x' with 2ca245f8be5 (csum-file.h: increase hashfile buffer size,\n2021-05-18) because the change in stderr triggered the progress API to\ncreate an extra trace2 region, ejecting the statistic.\n\nThis change increases the value of GIT_TRACE2_EVENT_NESTING across the\nentire test suite to avoid errors like this. Future changes will remove\ncustom assignments of GIT_TRACE2_EVENT_NESTING from some test scripts\nthat were aware of this limitation.\n\nReported-by: Ævar Arnfjörð Bjarmason \nHelped-by: Jeff King \nSigned-off-by: Derrick Stolee \n---\n t/test-lib.sh | 7 +++++++\n 1 file changed, 7 insertions(+)\n\ndiff --git a/t/test-lib.sh b/t/test-lib.sh\nindex 2679a7596a6..2e815c41c8f 100644\n--- a/t/test-lib.sh\n+++ b/t/test-lib.sh\n@@ -476,6 +476,13 @@ export GIT_TEST_MERGE_ALGORITHM\n GIT_TRACE_BARE=1\n export GIT_TRACE_BARE\n \n+# Some tests scan the GIT_TRACE2_EVENT feed for events, but the\n+# default depth is 2, which frequently causes issues when the\n+# events are wrapped in new regions. Set it to a sufficiently\n+# large depth to avoid custom changes in the test suite.\n+GIT_TRACE2_EVENT_NESTING=100\n+export GIT_TRACE2_EVENT_NESTING\n+\n # Use specific version of the index file format\n if test -n \"${GIT_TEST_INDEX_VERSION:+isset}\"\n then\n-- \ngitgitgadget\n\n"},{"id":"442443","messageId":"d5ecfca9d2ccf820ac21823bec490e901ff1fa08.1638193666.git.gitgitgadget@gmail.com","threadId":"56994","inReplyTo":"pull.1085.git.1638193666.gitgitgadget@gmail.com","subject":"[PATCH 2/2] t/t*: remove custom GIT_TRACE2_EVENT_NESTING","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-11-29T13:47:46Z","receivedAt":"2021-11-29T15:41:10Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee \n\nThe previous change modified GIT_TRACE2_EVENT_NESTING by default within\ntest-lib.sh. These custom assignments throughout the test suite are no\nlonger necessary.\n\nSigned-off-by: Derrick Stolee \n---\n t/t1092-sparse-checkout-compatibility.sh | 6 +++---\n t/t4216-log-bloom.sh | 2 --\n t/t5310-pack-bitmaps.sh | 2 +-\n t/t5705-session-id-in-capabilities.sh | 2 --\n t/t7519-status-fsmonitor.sh | 2 +-\n 5 files changed, 5 insertions(+), 9 deletions(-)\n\ndiff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh\nindex 16fbd2c6db9..2de957905b0 100755\n--- a/t/t1092-sparse-checkout-compatibility.sh\n+++ b/t/t1092-sparse-checkout-compatibility.sh\n@@ -688,7 +688,7 @@ test_expect_success 'submodule handling' '\n test_expect_success 'sparse-index is expanded and converted back' '\n \tinit_repos &&\n \n-\tGIT_TRACE2_EVENT=\"$(pwd)/trace2.txt\" GIT_TRACE2_EVENT_NESTING=10 \\\n+\tGIT_TRACE2_EVENT=\"$(pwd)/trace2.txt\" \\\n \t\tgit -C sparse-index -c core.fsmonitor=\"\" reset --hard &&\n \ttest_region index convert_to_sparse trace2.txt &&\n \ttest_region index ensure_full_index trace2.txt\n@@ -702,10 +702,10 @@ ensure_not_expanded () {\n \tthen\n \t\tshift &&\n \t\ttest_must_fail env \\\n-\t\t\tGIT_TRACE2_EVENT=\"$(pwd)/trace2.txt\" GIT_TRACE2_EVENT_NESTING=10 \\\n+\t\t\tGIT_TRACE2_EVENT=\"$(pwd)/trace2.txt\" \\\n \t\t\tgit -C sparse-index \"$@\" || return 1\n \telse\n-\t\tGIT_TRACE2_EVENT=\"$(pwd)/trace2.txt\" GIT_TRACE2_EVENT_NESTING=10 \\\n+\t\tGIT_TRACE2_EVENT=\"$(pwd)/trace2.txt\" \\\n \t\t\tgit -C sparse-index \"$@\" || return 1\n \tfi &&\n \ttest_region ! index ensure_full_index trace2.txt\ndiff --git a/t/t4216-log-bloom.sh b/t/t4216-log-bloom.sh\nindex 50f206db550..8018c12a6a4 100755\n--- a/t/t4216-log-bloom.sh\n+++ b/t/t4216-log-bloom.sh\n@@ -175,13 +175,11 @@ test_expect_success 'persist filter settings' '\n \ttest_when_finished rm -rf .git/objects/info/commit-graph* &&\n \trm -rf .git/objects/info/commit-graph* &&\n \tGIT_TRACE2_EVENT=\"$(pwd)/trace2.txt\" \\\n-\t\tGIT_TRACE2_EVENT_NESTING=5 \\\n \t\tGIT_TEST_BLOOM_SETTINGS_NUM_HASHES=9 \\\n \t\tGIT_TEST_BLOOM_SETTINGS_BITS_PER_ENTRY=15 \\\n \t\tgit commit-graph write --reachable --changed-paths &&\n \tgrep \"{\\\"hash_version\\\":1,\\\"num_hashes\\\":9,\\\"bits_per_entry\\\":15,\\\"max_changed_paths\\\":512\" trace2.txt &&\n \tGIT_TRACE2_EVENT=\"$(pwd)/trace2-auto.txt\" \\\n-\t\tGIT_TRACE2_EVENT_NESTING=5 \\\n \t\tgit commit-graph write --reachable --changed-paths &&\n \tgrep \"{\\\"hash_version\\\":1,\\\"num_hashes\\\":9,\\\"bits_per_entry\\\":15,\\\"max_changed_paths\\\":512\" trace2-auto.txt\n '\ndiff --git a/t/t5310-pack-bitmaps.sh b/t/t5310-pack-bitmaps.sh\nindex dcf03d324a2..9104e842621 100755\n--- a/t/t5310-pack-bitmaps.sh\n+++ b/t/t5310-pack-bitmaps.sh\n@@ -35,7 +35,7 @@ test_expect_success 'setup writing bitmaps during repack' '\n '\n \n test_expect_success 'full repack creates bitmaps' '\n-\tGIT_TRACE2_EVENT_NESTING=4 GIT_TRACE2_EVENT=\"$(pwd)/trace\" \\\n+\tGIT_TRACE2_EVENT=\"$(pwd)/trace\" \\\n \t\tgit repack -ad &&\n \tls .git/objects/pack/ | grep bitmap >output &&\n \ttest_line_count = 1 output &&\ndiff --git a/t/t5705-session-id-in-capabilities.sh b/t/t5705-session-id-in-capabilities.sh\nindex eb8c79aafdd..ed38c76c290 100755\n--- a/t/t5705-session-id-in-capabilities.sh\n+++ b/t/t5705-session-id-in-capabilities.sh\n@@ -32,7 +32,6 @@ do\n \t\ttest_when_finished \"git -C local push --delete origin new-branch\" &&\n \t\tcp -r \"$LOCAL_PRISTINE\" local &&\n \t\tgit -C local pull --no-rebase origin &&\n-\t\tGIT_TRACE2_EVENT_NESTING=5 \\\n \t\tGIT_TRACE2_EVENT=\"$(pwd)/tr2-client-events\" \\\n \t\tgit -c protocol.version=$PROTO -C local push \\\n \t\t\t--receive-pack \"GIT_TRACE2_EVENT=\\\"$(pwd)/tr2-server-events\\\" git-receive-pack\" \\\n@@ -65,7 +64,6 @@ do\n \t\ttest_when_finished \"git -C local push --delete origin new-branch\" &&\n \t\tcp -r \"$LOCAL_PRISTINE\" local &&\n \t\tgit -C local pull --no-rebase origin &&\n-\t\tGIT_TRACE2_EVENT_NESTING=5 \\\n \t\tGIT_TRACE2_EVENT=\"$(pwd)/tr2-client-events\" \\\n \t\tgit -c protocol.version=$PROTO -C local push \\\n \t\t\t--receive-pack \"GIT_TRACE2_EVENT=\\\"$(pwd)/tr2-server-events\\\" git-receive-pack\" \\\ndiff --git a/t/t7519-status-fsmonitor.sh b/t/t7519-status-fsmonitor.sh\nindex f488d930dfd..a5e2233cb19 100755\n--- a/t/t7519-status-fsmonitor.sh\n+++ b/t/t7519-status-fsmonitor.sh\n@@ -390,7 +390,7 @@ test_expect_success 'status succeeds after staging/unstaging' '\n # during a call to 'git status'. Otherwise, we verify that we _do_ call it.\n check_sparse_index_behavior () {\n \tgit -C full status --porcelain=v2 >expect &&\n-\tGIT_TRACE2_EVENT=\"$(pwd)/trace2.txt\" GIT_TRACE2_EVENT_NESTING=10 \\\n+\tGIT_TRACE2_EVENT=\"$(pwd)/trace2.txt\" \\\n \t\tgit -C sparse status --porcelain=v2 >actual &&\n \ttest_region $1 index ensure_full_index trace2.txt &&\n \ttest_region fsm_hook query trace2.txt &&\n-- \ngitgitgadget\n"},{"id":"442448","messageId":"211129.86fsrff36j.gmgdl@evledraar.gmail.com","threadId":"56994","inReplyTo":"3d1bbc91aa3a465ecec2de130456b9ccc08f3032.1638193666.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 1/2] test-lib.sh: set GIT_TRACE2_EVENT_NESTING","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-11-29T14:12:20Z","receivedAt":"2021-11-29T16:36:42Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Mon, Nov 29 2021, Derrick Stolee via GitGitGadget wrote:\n\n> From: Derrick Stolee \n>\n> The GIT_TRACE2_EVENT feed has a limited nesting depth to avoid\n> overloading the feed when recursing into deep paths while adding more\n> nested regions.\n>\n> Some tests use the GIT_TRACE2_EVENT feed to look for internal events,\n> ensuring that intended behavior is happening.\n>\n> One such example is in t4216-log-bloom.sh which looks for a statistic\n> given as a trace2_data_intmax() call. This test started failing under\n> '-x' with 2ca245f8be5 (csum-file.h: increase hashfile buffer size,\n> 2021-05-18) because the change in stderr triggered the progress API to\n> create an extra trace2 region, ejecting the statistic.\n>\n> This change increases the value of GIT_TRACE2_EVENT_NESTING across the\n> entire test suite to avoid errors like this. Future changes will remove\n> custom assignments of GIT_TRACE2_EVENT_NESTING from some test scripts\n> that were aware of this limitation.\n>\n> Reported-by: Ævar Arnfjörð Bjarmason \n> Helped-by: Jeff King \n> Signed-off-by: Derrick Stolee \n> ---\n> t/test-lib.sh | 7 +++++++\n> 1 file changed, 7 insertions(+)\n>\n> diff --git a/t/test-lib.sh b/t/test-lib.sh\n> index 2679a7596a6..2e815c41c8f 100644\n> --- a/t/test-lib.sh\n> +++ b/t/test-lib.sh\n> @@ -476,6 +476,13 @@ export GIT_TEST_MERGE_ALGORITHM\n> GIT_TRACE_BARE=1\n> export GIT_TRACE_BARE\n> \n> +# Some tests scan the GIT_TRACE2_EVENT feed for events, but the\n> +# default depth is 2, which frequently causes issues when the\n> +# events are wrapped in new regions. Set it to a sufficiently\n> +# large depth to avoid custom changes in the test suite.\n> +GIT_TRACE2_EVENT_NESTING=100\n> +export GIT_TRACE2_EVENT_NESTING\n> +\n\nThanks for following up on this.\n\nRather than hardcoding 100 wouldn't it make sense to have something like\nthe below (which I barely checked for whether it compiled or not, just\nto check how hard it was to change), so that we can just set this to a\n\"false\" value to disable the nesting guard entirely?\n\nNeeding to dance around the value beint an integer or true/false is an\nunfortunate side-effect of there not being two separate \"enable nesting\nguard?\" and \"nest level?\" config knob, but there's not much to do about\nthat at this point, so I just mapped \"false\" to \"-1\" internally.\n\nMaybe values of 0 and 1 should be an error in any case? I didn't check,\nthat would make this approach simpler.\n\ndiff --git a/Documentation/config/trace2.txt b/Documentation/config/trace2.txt\nindex fe1642f0d40..2be5474fdcd 100644\n--- a/Documentation/config/trace2.txt\n+++ b/Documentation/config/trace2.txt\n@@ -35,10 +35,14 @@ trace2.eventBrief::\n \t`GIT_TRACE2_EVENT_BRIEF` environment variable. Defaults to false.\n \n trace2.eventNesting::\n-\tInteger. Specifies desired depth of nested regions in the\n+\tInteger or \"false\" boolean. Specifies desired depth of nested regions in the\n \tevent output. Regions deeper than this value will be\n \tomitted. May be overridden by the `GIT_TRACE2_EVENT_NESTING`\n \tenvironment variable. Defaults to 2.\n++\n+Set this to a \"false\" value (see linkgit:git-config[1] for accepted\n+values, i.e. \"false\", \"off\" etc.) to remove the limitation on nesting\n+entirely.\n \n trace2.configParams::\n \tA comma-separated list of patterns of \"important\" config\ndiff --git a/trace2/tr2_tgt_event.c b/trace2/tr2_tgt_event.c\nindex 3a0014417cc..135d3fbd8c3 100644\n--- a/trace2/tr2_tgt_event.c\n+++ b/trace2/tr2_tgt_event.c\n@@ -53,8 +53,22 @@ static int fn_init(void)\n \t\treturn want;\n \n \tnesting = tr2_sysenv_get(TR2_SYSENV_EVENT_NESTING);\n-\tif (nesting && *nesting && ((max_nesting = atoi(nesting)) > 0))\n-\t\ttr2env_event_max_nesting_levels = max_nesting;\n+\tif (nesting) {\n+\t\t/*\n+\t\t * TODO: Maybe expose the \"off\" part of\n+\t\t * git_parse_maybe_bool_text() as\n+\t\t * git_parse_maybe_bool_text_false() and use it here?\n+\t\t * Note that we accept \"GIT_TRACE2_EVENT_NESTING=\" and\n+\t\t * \"trace2.eventNesting=\" as \"false\", as with the rest\n+\t\t * of the config mechanism and git_parse_maybe_bool()\n+\t\t * users.\n+\t\t */\n+\t\tif (!*nesting || !strcmp(nesting, \"false\") ||\n+\t\t !strcmp(nesting, \"no\") || !strcmp(nesting, \"off\"))\n+\t\t\ttr2env_event_max_nesting_levels = -1;\n+\t\telse if (*nesting && ((max_nesting = atoi(nesting)) > 0))\n+\t\t\ttr2env_event_max_nesting_levels = max_nesting;\n+\t}\n \n \tbrief = tr2_sysenv_get(TR2_SYSENV_EVENT_BRIEF);\n \tif (brief && *brief &&\n@@ -503,6 +517,15 @@ static void fn_repo_fl(const char *file, int line,\n \tjw_release(&jw);\n }\n \n+static int nesting_ok(int nr_open_regions)\n+{\n+\tif (tr2env_event_max_nesting_levels == -1)\n+\t\treturn 1;\n+\tif (nr_open_regions <= tr2env_event_max_nesting_levels)\n+\t\treturn 1;\n+\treturn 0;\n+}\n+\n static void fn_region_enter_printf_va_fl(const char *file, int line,\n \t\t\t\t\t uint64_t us_elapsed_absolute,\n \t\t\t\t\t const char *category,\n@@ -512,7 +535,7 @@ static void fn_region_enter_printf_va_fl(const char *file, int line,\n {\n \tconst char *event_name = \"region_enter\";\n \tstruct tr2tls_thread_ctx *ctx = tr2tls_get_self();\n-\tif (ctx->nr_open_regions <= tr2env_event_max_nesting_levels) {\n+\tif (nesting_ok(ctx->nr_open_regions)) {\n \t\tstruct json_writer jw = JSON_WRITER_INIT;\n \n \t\tjw_object_begin(&jw, 0);\n@@ -537,7 +560,7 @@ static void fn_region_leave_printf_va_fl(\n {\n \tconst char *event_name = \"region_leave\";\n \tstruct tr2tls_thread_ctx *ctx = tr2tls_get_self();\n-\tif (ctx->nr_open_regions <= tr2env_event_max_nesting_levels) {\n+\tif (nesting_ok(ctx->nr_open_regions)) {\n \t\tstruct json_writer jw = JSON_WRITER_INIT;\n \t\tdouble t_rel = (double)us_elapsed_region / 1000000.0;\n \n@@ -564,7 +587,7 @@ static void fn_data_fl(const char *file, int line, uint64_t us_elapsed_absolute,\n {\n \tconst char *event_name = \"data\";\n \tstruct tr2tls_thread_ctx *ctx = tr2tls_get_self();\n-\tif (ctx->nr_open_regions <= tr2env_event_max_nesting_levels) {\n+\tif (nesting_ok(ctx->nr_open_regions)) {\n \t\tstruct json_writer jw = JSON_WRITER_INIT;\n \t\tdouble t_abs = (double)us_elapsed_absolute / 1000000.0;\n \t\tdouble t_rel = (double)us_elapsed_region / 1000000.0;\n@@ -592,7 +615,7 @@ static void fn_data_json_fl(const char *file, int line,\n {\n \tconst char *event_name = \"data_json\";\n \tstruct tr2tls_thread_ctx *ctx = tr2tls_get_self();\n-\tif (ctx->nr_open_regions <= tr2env_event_max_nesting_levels) {\n+\tif (nesting_ok(ctx->nr_open_regions)) {\n \t\tstruct json_writer jw = JSON_WRITER_INIT;\n \t\tdouble t_abs = (double)us_elapsed_absolute / 1000000.0;\n \t\tdouble t_rel = (double)us_elapsed_region / 1000000.0;\n"},{"id":"442465","messageId":"xmqqk0gq3jfs.fsf@gitster.g","threadId":"56994","inReplyTo":"211129.86fsrff36j.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH 1/2] test-lib.sh: set GIT_TRACE2_EVENT_NESTING","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-11-29T18:28:39Z","receivedAt":"2021-11-29T18:30:49Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n> Thanks for following up on this.\n>\n> Rather than hardcoding 100 wouldn't it make sense to have something like\n> the below (which I barely checked for whether it compiled or not, just\n> to check how hard it was to change), so that we can just set this to a\n> \"false\" value to disable the nesting guard entirely?\n\nI agree that could be a better endgame if it works.\n\nThat is where our agreement ends. I strongly prefer to have a\nsmall and focused fix for immediate problem at hand, and then a\nfundamental \"(could be) better\" change separately, so that we can\ncool the latter longer.\n\nThis difference between us is not limited to this topic. I often\nget irritated to see an attempt to jump to the endgame with too big\na change in a single step. Please don't.\n\nThanks.\n"},{"id":"442466","messageId":"xmqqfsre3j9j.fsf@gitster.g","threadId":"56994","inReplyTo":"211129.86fsrff36j.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH 1/2] test-lib.sh: set GIT_TRACE2_EVENT_NESTING","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-11-29T18:32:24Z","receivedAt":"2021-11-29T18:34:34Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n> trace2.eventNesting::\n> -\tInteger. Specifies desired depth of nested regions in the\n> +\tInteger or \"false\" boolean. Specifies desired depth of nested regions in the\n> \tevent output. Regions deeper than this value will be\n> \tomitted. May be overridden by the `GIT_TRACE2_EVENT_NESTING`\n> \tenvironment variable. Defaults to 2.\n> ++\n> +Set this to a \"false\" value (see linkgit:git-config[1] for accepted\n> +values, i.e. \"false\", \"off\" etc.) to remove the limitation on nesting\n> +entirely.\n\nIf the value of 5 set to .eventNesting allows up to 5 levels of\nnesting, I would imagine some readers expect that \"false\" or \"off\"\nwould forbid nesting of events altogether. For \"false\" to work\nwell, the variable needs renaming to trace2.limitEventNesting, or\nsomething like that.\n\n\n"},{"id":"442467","messageId":"YaUegEGxfAf72O9Z@coredump.intra.peff.net","threadId":"56994","inReplyTo":"pull.1085.git.1638193666.gitgitgadget@gmail.com","subject":"Re: [PATCH 0/2] Set GIT_TRACE2_EVENT_NESTING in test-lib.sh","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-11-29T18:40:00Z","receivedAt":"2021-11-29T18:42:19Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Nov 29, 2021 at 01:47:44PM +0000, Derrick Stolee via GitGitGadget wrote:\n\n> As reported by Ævar [1] and diagnosed by Peff in a reply, the default\n> GIT_TRACE2_EVENT_NESTING is set so low that tests that look for trace2\n> events can start failing due to an added trace2 region. This can even be\n> subtle, including the fact that the progress API adds trace2 regions when in\n> use (which can depend on the verbose output of a test script).\n\nI think this is a good change for fixing the immediate problem of the\ntest suite failing.\n\nBut I have to wonder if this is masking a problem that real users will\nsee. Aren't we silently discarding trace2 output that could be useful to\nsomebody debugging or trying to get performance metrics?\n\nI.e., should we be bumping the internal nesting max of 2 to something\nhigher? If that would that cause problems with existing metrics, should\nwe be designing new metrics to avoid nesting?\n\n-Peff\n"},{"id":"442468","messageId":"YaUfq88UwWVcb7KG@coredump.intra.peff.net","threadId":"56994","inReplyTo":"YaUegEGxfAf72O9Z@coredump.intra.peff.net","subject":"Re: [PATCH 0/2] Set GIT_TRACE2_EVENT_NESTING in test-lib.sh","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-11-29T18:44:59Z","receivedAt":"2021-11-29T18:47:03Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Nov 29, 2021 at 01:40:00PM -0500, Jeff King wrote:\n\n> On Mon, Nov 29, 2021 at 01:47:44PM +0000, Derrick Stolee via GitGitGadget wrote:\n> \n> > As reported by Ævar [1] and diagnosed by Peff in a reply, the default\n> > GIT_TRACE2_EVENT_NESTING is set so low that tests that look for trace2\n> > events can start failing due to an added trace2 region. This can even be\n> > subtle, including the fact that the progress API adds trace2 regions when in\n> > use (which can depend on the verbose output of a test script).\n> \n> I think this is a good change for fixing the immediate problem of the\n> test suite failing.\n> \n> But I have to wonder if this is masking a problem that real users will\n> see. Aren't we silently discarding trace2 output that could be useful to\n> somebody debugging or trying to get performance metrics?\n> \n> I.e., should we be bumping the internal nesting max of 2 to something\n> higher? If that would that cause problems with existing metrics, should\n> we be designing new metrics to avoid nesting?\n\nOne thing I should have added to the first paragraph: the patches\nthemselves look fine and I'm OK with this as a band-aid in the short\nterm.\n\n-Peff\n"},{"id":"442513","messageId":"patch-1.1-9f735bd0d49-20211129T200950Z-avarab@gmail.com","threadId":"56994","inReplyTo":"pull.1085.git.1638193666.gitgitgadget@gmail.com","subject":"[PATCH] test-lib.sh: have all tests pass under \"-x\", remove BASH_XTRACEFD","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-11-29T20:13:23Z","receivedAt":"2021-11-29T22:20:31Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Change the \"t1510-repo-setup.sh\" test to use a new\n\"test_must_be_empty_trace\" wrapper, instead of turning on\n\"test_untraceable=UnfortunatelyYes\".\n\nThe only reason the test was incompatible with \"-x\" was because of\nthese \"test_must_be_empty\" checks, which we can instead instead skip\nif we're running under \"set -x\".\n\nSkipping the tests is preferable to not having the \"-x\" output at all,\nas it's much easier to debug the test. The result loss of test\ncoverage is minimal. If someone is adjusting a \"test_must_be_empty\"\ntest a failure might go away under \"-x\", but the new \"say\" we emit\nhere should highlight that appropriately.\n\nSince the only user of \"test_untraceable\" is gone, we can remove that,\nnot only isn't it used now, but I think the rationale for using it in\nthe future no longer applies.\n\nWe'll be better off by using a simple wrapper like the new\n\"test_must_be_empty_trace\". See 58275069288 (t1510-repo-setup: mark as\nuntraceable with '-x', 2018-02-24) and 5fc98e79fc0 (t: add means to\ndisable '-x' tracing for individual test scripts, 2018-02-24) for the\naddition of \"test_untraceable\".\n\nOnce that's been removed we can dig deeper and see that we only have\n\"BASH_XTRACEFD\" due to an earlier attempt to work around the same\nissue. See d88785e424a (test-lib: set BASH_XTRACEFD automatically,\n2016-05-11) and the 90c8a1db9d6 (test-lib: silence \"-x\" cleanup under\nbash, 2017-12-08) follow-up.\n\nI.e. we had redirection in \"test_eval_\" to get more relevant trace\noutput under bash, which in turn was only needed because\nBASH_XTRACEFD=1 was set, which in turn was trying to work around test\nfailures under \"set -x\".\n\nIt's better if our test suite works the same way on all shells, rather\nthan getting a passing run under \"bash\", only to have it fail with\n\"-x\" under say \"dash\". As the deleted code shows this is much simpler\nto implement across our supported POSIX shells.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n\nI hacked this up the other day when looking at the --verbose-log\nv.s. -v failure[1] that's now being addressed in[2].\n\nI'd originally run into that because I was trying to debug\nt1510-repo-setup.sh and found that -x inexplicably didn't work, only\nto discover it was he only consumer of \"test_untracable\".\n\n1. https://lore.kernel.org/git/211125.86pmqoifp8.gmgdl@evledraar.gmail.com/\n2. https://lore.kernel.org/git/pull.1085.git.1638193666.gitgitgadget@gmail.com/\n\n t/README | 3 --\n t/t1510-repo-setup.sh | 34 +++++++++++++---------\n t/test-lib.sh | 66 +++++--------------------------------------\n 3 files changed, 27 insertions(+), 76 deletions(-)\n\ndiff --git a/t/README b/t/README\nindex 29f72354bf1..3d30bbff34a 100644\n--- a/t/README\n+++ b/t/README\n@@ -86,9 +86,6 @@ appropriately before running \"make\". Short options can be bundled, i.e.\n -x::\n \tTurn on shell tracing (i.e., `set -x`) during the tests\n \tthemselves. Implies `--verbose`.\n-\tIgnored in test scripts that set the variable 'test_untraceable'\n-\tto a non-empty value, unless it's run with a Bash version\n-\tsupporting BASH_XTRACEFD, i.e. v4.1 or later.\n \n -d::\n --debug::\ndiff --git a/t/t1510-repo-setup.sh b/t/t1510-repo-setup.sh\nindex 591505a39c0..7eb65a52c08 100755\n--- a/t/t1510-repo-setup.sh\n+++ b/t/t1510-repo-setup.sh\n@@ -40,8 +40,14 @@ A few rules for repo setup:\n prefix is NULL.\n \"\n \n-# This test heavily relies on the standard error of nested function calls.\n-test_untraceable=UnfortunatelyYes\n+test_must_be_empty_trace () {\n+\tif want_trace\n+\tthen\n+\t\tsay \"$TEST_NAME does not check test_must_be_empty on \\\"$@\\\" under -x\"\n+\t\treturn 0\n+\tfi\n+\ttest_must_be_empty \"$@\"\n+}\n \n TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n@@ -235,14 +241,14 @@ test_expect_success '#0: nonbare repo, no explicit configuration' '\n \ttry_repo 0 unset unset unset \"\" unset \\\n \t\t.git \"$here/0\" \"$here/0\" \"(null)\" \\\n \t\t.git \"$here/0\" \"$here/0\" sub/ 2>message &&\n-\ttest_must_be_empty message\n+\ttest_must_be_empty_trace message\n '\n \n test_expect_success '#1: GIT_WORK_TREE without explicit GIT_DIR is accepted' '\n \ttry_repo 1 \"$here\" unset unset \"\" unset \\\n \t\t\"$here/1/.git\" \"$here\" \"$here\" 1/ \\\n \t\t\"$here/1/.git\" \"$here\" \"$here\" 1/sub/ 2>message &&\n-\ttest_must_be_empty message\n+\ttest_must_be_empty_trace message\n '\n \n test_expect_success '#2: worktree defaults to cwd with explicit GIT_DIR' '\n@@ -269,7 +275,7 @@ test_expect_success '#4: core.worktree without GIT_DIR set is accepted' '\n \ttry_case 4 unset unset \\\n \t\t.git \"$here/4/sub\" \"$here/4\" \"(null)\" \\\n \t\t\"$here/4/.git\" \"$here/4/sub\" \"$here/4/sub\" \"(null)\" 2>message &&\n-\ttest_must_be_empty message\n+\ttest_must_be_empty_trace message\n '\n \n test_expect_success '#5: core.worktree + GIT_WORK_TREE is accepted' '\n@@ -280,7 +286,7 @@ test_expect_success '#5: core.worktree + GIT_WORK_TREE is accepted' '\n \ttry_repo 5a .. unset \"$here/5a\" \"\" unset \\\n \t\t\"$here/5a/.git\" \"$here\" \"$here\" 5a/ \\\n \t\t\"$here/5a/.git\" \"$here/5a\" \"$here/5a\" sub/ &&\n-\ttest_must_be_empty message\n+\ttest_must_be_empty_trace message\n '\n \n test_expect_success '#6: setting GIT_DIR brings core.worktree to life' '\n@@ -377,7 +383,7 @@ test_expect_success '#9: GIT_WORK_TREE accepted with gitfile' '\n \ttry_repo 9 wt unset unset gitfile unset \\\n \t\t\"$here/9.git\" \"$here/9/wt\" \"$here/9\" \"(null)\" \\\n \t\t\"$here/9.git\" \"$here/9/sub/wt\" \"$here/9/sub\" \"(null)\" 2>message &&\n-\ttest_must_be_empty message\n+\ttest_must_be_empty_trace message\n '\n \n test_expect_success '#10: GIT_DIR can point to gitfile' '\n@@ -403,7 +409,7 @@ test_expect_success '#12: core.worktree with gitfile is accepted' '\n \ttry_repo 12 unset unset \"$here/12\" gitfile unset \\\n \t\t\"$here/12.git\" \"$here/12\" \"$here/12\" \"(null)\" \\\n \t\t\"$here/12.git\" \"$here/12\" \"$here/12\" sub/ 2>message &&\n-\ttest_must_be_empty message\n+\ttest_must_be_empty_trace message\n '\n \n test_expect_success '#13: core.worktree+GIT_WORK_TREE accepted (with gitfile)' '\n@@ -411,7 +417,7 @@ test_expect_success '#13: core.worktree+GIT_WORK_TREE accepted (with gitfile)' '\n \ttry_repo 13 non-existent-too unset non-existent gitfile unset \\\n \t\t\"$here/13.git\" \"$here/13/non-existent-too\" \"$here/13\" \"(null)\" \\\n \t\t\"$here/13.git\" \"$here/13/sub/non-existent-too\" \"$here/13/sub\" \"(null)\" 2>message &&\n-\ttest_must_be_empty message\n+\ttest_must_be_empty_trace message\n '\n \n # case #14.\n@@ -566,7 +572,7 @@ test_expect_success '#17: GIT_WORK_TREE without explicit GIT_DIR is accepted (ba\n \ttry_repo 17c \"$here/17c\" unset unset \"\" true \\\n \t\t.git \"$here/17c\" \"$here/17c\" \"(null)\" \\\n \t\t\"$here/17c/.git\" \"$here/17c\" \"$here/17c\" sub/ 2>message &&\n-\ttest_must_be_empty message\n+\ttest_must_be_empty_trace message\n '\n \n test_expect_success '#18: bare .git named by GIT_DIR has no worktree' '\n@@ -595,7 +601,7 @@ test_expect_success '#20a: core.worktree without GIT_DIR accepted (inside .git)'\n \t\t\"$here/20a/.git\" \"$here/20a\" \"$here/20a\" .git/wt/ &&\n \ttry_case 20a/.git/wt/sub unset unset \\\n \t\t\"$here/20a/.git\" \"$here/20a\" \"$here/20a\" .git/wt/sub/ &&\n-\ttest_must_be_empty message\n+\ttest_must_be_empty_trace message\n '\n \n test_expect_success '#20b/c: core.worktree and core.bare conflict' '\n@@ -627,7 +633,7 @@ test_expect_success '#21: setup, core.worktree warns before overriding core.bare\n \t\texport GIT_WORK_TREE &&\n \t\tgit status >/dev/null\n \t) 2>message &&\n-\ttest_must_be_empty message\n+\ttest_must_be_empty_trace message\n \n '\n run_wt_tests 21\n@@ -743,7 +749,7 @@ test_expect_success '#25: GIT_WORK_TREE accepted if GIT_DIR unset (bare gitfile\n \ttry_repo 25 \"$here/25\" unset unset gitfile true \\\n \t\t\"$here/25.git\" \"$here/25\" \"$here/25\" \"(null)\" \\\n \t\t\"$here/25.git\" \"$here/25\" \"$here/25\" \"sub/\" 2>message &&\n-\ttest_must_be_empty message\n+\ttest_must_be_empty_trace message\n '\n \n test_expect_success '#26: bare repo has no worktree (GIT_DIR -> gitfile case)' '\n@@ -781,7 +787,7 @@ test_expect_success '#29: setup' '\n \t\texport GIT_WORK_TREE &&\n \t\tgit status\n \t) 2>message &&\n-\ttest_must_be_empty message\n+\ttest_must_be_empty_trace message\n '\n run_wt_tests 29 gitfile\n \ndiff --git a/t/test-lib.sh b/t/test-lib.sh\nindex 2679a7596a6..38a0fc558c9 100644\n--- a/t/test-lib.sh\n+++ b/t/test-lib.sh\n@@ -381,29 +381,6 @@ then\n \texit\n fi\n \n-if test -n \"$trace\" && test -n \"$test_untraceable\"\n-then\n-\t# '-x' tracing requested, but this test script can't be reliably\n-\t# traced, unless it is run with a Bash version supporting\n-\t# BASH_XTRACEFD (introduced in Bash v4.1).\n-\t#\n-\t# Perform this version check _after_ the test script was\n-\t# potentially re-executed with $TEST_SHELL_PATH for '--tee' or\n-\t# '--verbose-log', so the right shell is checked and the\n-\t# warning is issued only once.\n-\tif test -n \"$BASH_VERSION\" && eval '\n-\t test ${BASH_VERSINFO[0]} -gt 4 || {\n-\t test ${BASH_VERSINFO[0]} -eq 4 &&\n-\t test ${BASH_VERSINFO[1]} -ge 1\n-\t }\n-\t '\n-\tthen\n-\t\t: Executed by a Bash version supporting BASH_XTRACEFD. Good.\n-\telse\n-\t\techo >&2 \"warning: ignoring -x; '$0' is untraceable without BASH_XTRACEFD\"\n-\t\ttrace=\n-\tfi\n-fi\n if test -n \"$trace\" && test -z \"$verbose_log\"\n then\n \tverbose=t\n@@ -650,19 +627,6 @@ else\n \texec 4>/dev/null 3>/dev/null\n fi\n \n-# Send any \"-x\" output directly to stderr to avoid polluting tests\n-# which capture stderr. We can do this unconditionally since it\n-# has no effect if tracing isn't turned on.\n-#\n-# Note that this sets up the trace fd as soon as we assign the variable, so it\n-# must come after the creation of descriptor 4 above. Likewise, we must never\n-# unset this, as it has the side effect of closing descriptor 4, which we\n-# use to show verbose tests to the user.\n-#\n-# Note also that we don't need or want to export it. The tracing is local to\n-# this shell, and we would not want to influence any shells we exec.\n-BASH_XTRACEFD=4\n-\n test_failure=0\n test_count=0\n test_fixed=0\n@@ -949,36 +913,20 @@ test_eval_ () {\n \t# the shell from printing the \"set +x\" to turn it off (nor the saving\n \t# of $? before that). But we can make sure that the output goes to\n \t# /dev/null.\n-\t#\n-\t# There are a few subtleties here:\n-\t#\n-\t# - we have to redirect descriptor 4 in addition to 2, to cover\n-\t# BASH_XTRACEFD\n-\t#\n-\t# - the actual eval has to come before the redirection block (since\n-\t# it needs to see descriptor 4 to set up its stderr)\n-\t#\n-\t# - likewise, any error message we print must be outside the block to\n-\t# access descriptor 4\n-\t#\n-\t# - checking $? has to come immediately after the eval, but it must\n-\t# be _inside_ the block to avoid polluting the \"set -x\" output\n-\t#\n-\n-\ttest_eval_inner_ \"$@\" &3 2>&4\n \t{\n+\t\ttest_eval_inner_ \"$@\" &3 2>&4\n \t\ttest_eval_ret_=$?\n \t\tif want_trace\n \t\tthen\n \t\t\ttest 1 = $trace_level_ && set +x\n \t\t\ttrace_level_=$(($trace_level_-1))\n-\t\tfi\n-\t} 2>/dev/null 4>&2\n \n-\tif test \"$test_eval_ret_\" != 0 && want_trace\n-\tthen\n-\t\tsay_color error >&4 \"error: last command exited with \\$?=$test_eval_ret_\"\n-\tfi\n+\t\t\tif test \"$test_eval_ret_\" != 0\n+\t\t\tthen\n+\t\t\t\tsay_color error >&4 \"error: last command exited with \\$?=$test_eval_ret_\"\n+\t\t\tfi\n+\t\tfi\n+\t} 2>/dev/null\n \treturn $test_eval_ret_\n }\n \n-- \n2.34.1.841.ge54f711571c\n\n"},{"id":"442578","messageId":"CAPig+cQ-WsuLLfUiseEithhGXWRgS=nB95BNe+k8iSPCCxJ2xw@mail.gmail.com","threadId":"56994","inReplyTo":"patch-1.1-9f735bd0d49-20211129T200950Z-avarab@gmail.com","subject":"Re: [PATCH] test-lib.sh: have all tests pass under \"-x\", remove BASH_XTRACEFD","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2021-11-29T23:23:43Z","receivedAt":"2021-11-29T23:23:58Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Mon, Nov 29, 2021 at 5:20 PM Ævar Arnfjörð Bjarmason\n wrote:\n> Change the \"t1510-repo-setup.sh\" test to use a new\n> \"test_must_be_empty_trace\" wrapper, instead of turning on\n> \"test_untraceable=UnfortunatelyYes\".\n>\n> The only reason the test was incompatible with \"-x\" was because of\n> these \"test_must_be_empty\" checks, which we can instead instead skip\n> if we're running under \"set -x\".\n\ns/instead instead/instead/\n\n> Skipping the tests is preferable to not having the \"-x\" output at all,\n> as it's much easier to debug the test. The result loss of test\n> coverage is minimal. If someone is adjusting a \"test_must_be_empty\"\n> test a failure might go away under \"-x\", but the new \"say\" we emit\n> here should highlight that appropriately.\n>\n> Since the only user of \"test_untraceable\" is gone, we can remove that,\n> not only isn't it used now, but I think the rationale for using it in\n> the future no longer applies.\n>\n> We'll be better off by using a simple wrapper like the new\n> \"test_must_be_empty_trace\". See 58275069288 (t1510-repo-setup: mark as\n> untraceable with '-x', 2018-02-24) and 5fc98e79fc0 (t: add means to\n> disable '-x' tracing for individual test scripts, 2018-02-24) for the\n> addition of \"test_untraceable\".\n>\n> Once that's been removed we can dig deeper and see that we only have\n> \"BASH_XTRACEFD\" due to an earlier attempt to work around the same\n> issue. See d88785e424a (test-lib: set BASH_XTRACEFD automatically,\n> 2016-05-11) and the 90c8a1db9d6 (test-lib: silence \"-x\" cleanup under\n> bash, 2017-12-08) follow-up.\n>\n> I.e. we had redirection in \"test_eval_\" to get more relevant trace\n> output under bash, which in turn was only needed because\n> BASH_XTRACEFD=1 was set, which in turn was trying to work around test\n> failures under \"set -x\".\n>\n> It's better if our test suite works the same way on all shells, rather\n> than getting a passing run under \"bash\", only to have it fail with\n> \"-x\" under say \"dash\". As the deleted code shows this is much simpler\n> to implement across our supported POSIX shells.\n>\n> Signed-off-by: Ævar Arnfjörð Bjarmason \n"},{"id":"442587","messageId":"YaVqo476kVKtT1Rg@nand.local","threadId":"56994","inReplyTo":"YaUfq88UwWVcb7KG@coredump.intra.peff.net","subject":"Re: [PATCH 0/2] Set GIT_TRACE2_EVENT_NESTING in test-lib.sh","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2021-11-30T00:04:51Z","receivedAt":"2021-11-30T00:04:57Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Mon, Nov 29, 2021 at 01:44:59PM -0500, Jeff King wrote:\n> On Mon, Nov 29, 2021 at 01:40:00PM -0500, Jeff King wrote:\n>\n> > On Mon, Nov 29, 2021 at 01:47:44PM +0000, Derrick Stolee via GitGitGadget wrote:\n> >\n> > > As reported by Ævar [1] and diagnosed by Peff in a reply, the default\n> > > GIT_TRACE2_EVENT_NESTING is set so low that tests that look for trace2\n> > > events can start failing due to an added trace2 region. This can even be\n> > > subtle, including the fact that the progress API adds trace2 regions when in\n> > > use (which can depend on the verbose output of a test script).\n> >\n> > I think this is a good change for fixing the immediate problem of the\n> > test suite failing.\n> >\n> > But I have to wonder if this is masking a problem that real users will\n> > see. Aren't we silently discarding trace2 output that could be useful to\n> > somebody debugging or trying to get performance metrics?\n> >\n> > I.e., should we be bumping the internal nesting max of 2 to something\n> > higher? If that would that cause problems with existing metrics, should\n> > we be designing new metrics to avoid nesting?\n>\n> One thing I should have added to the first paragraph: the patches\n> themselves look fine and I'm OK with this as a band-aid in the short\n> term.\n\nYeah, I agree. This test in particular must have bad luck, because I\nnoticed the same failure in GitHub's fork when merging with 2.33 (but\ndue to some custom trace2 regions causing us to blow past the limit even\nwithout `-v`).\n\nIt might be nice to allow setting 'GIT_TRACE2_EVENT_NESTING=false' or\nsomething similar, but let's deal with that elsewhere.\n\nhanks,\nTaylor\n"},{"id":"442672","messageId":"86555f24-e7c4-64cb-a55c-7e2580e895f2@gmail.com","threadId":"56994","inReplyTo":"YaUegEGxfAf72O9Z@coredump.intra.peff.net","subject":"Re: [PATCH 0/2] Set GIT_TRACE2_EVENT_NESTING in test-lib.sh","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2021-11-30T15:34:03Z","receivedAt":"2021-11-30T15:34:08Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 11/29/2021 1:40 PM, Jeff King wrote:\n> On Mon, Nov 29, 2021 at 01:47:44PM +0000, Derrick Stolee via GitGitGadget wrote:\n> \n>> As reported by Ævar [1] and diagnosed by Peff in a reply, the default\n>> GIT_TRACE2_EVENT_NESTING is set so low that tests that look for trace2\n>> events can start failing due to an added trace2 region. This can even be\n>> subtle, including the fact that the progress API adds trace2 regions when in\n>> use (which can depend on the verbose output of a test script).\n> \n> I think this is a good change for fixing the immediate problem of the\n> test suite failing.\n> \n> But I have to wonder if this is masking a problem that real users will\n> see. Aren't we silently discarding trace2 output that could be useful to\n> somebody debugging or trying to get performance metrics?\n> \n> I.e., should we be bumping the internal nesting max of 2 to something\n> higher? If that would that cause problems with existing metrics, should\n> we be designing new metrics to avoid nesting?\n\nOne thing this makes me think about is that we might want to\nhave items such as trace2_data_intmax() and trace2_data_string()\nnot be subject to the nesting limit. Only have the nesting apply\nto the regions (which nest).\n\nCC'ing Jeff Hostetler for his thoughts on that idea. We will have\na discussion offline to see if the progress regions have started\nto cause inconsistencies in our existing telemetry stream of\ninternal users.\n\nThanks,\n-Stolee\n"},{"id":"442685","messageId":"YaaS4Idhdyo2wZ9q@coredump.intra.peff.net","threadId":"56994","inReplyTo":"patch-1.1-9f735bd0d49-20211129T200950Z-avarab@gmail.com","subject":"Re: [PATCH] test-lib.sh: have all tests pass under \"-x\", remove BASH_XTRACEFD","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-11-30T21:08:48Z","receivedAt":"2021-11-30T21:08:54Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Nov 29, 2021 at 09:13:23PM +0100, Ævar Arnfjörð Bjarmason wrote:\n\n> Once that's been removed we can dig deeper and see that we only have\n> \"BASH_XTRACEFD\" due to an earlier attempt to work around the same\n> issue. See d88785e424a (test-lib: set BASH_XTRACEFD automatically,\n> 2016-05-11) and the 90c8a1db9d6 (test-lib: silence \"-x\" cleanup under\n> bash, 2017-12-08) follow-up.\n>\n> I.e. we had redirection in \"test_eval_\" to get more relevant trace\n> output under bash, which in turn was only needed because\n> BASH_XTRACEFD=1 was set, which in turn was trying to work around test\n> failures under \"set -x\".\n> \n> It's better if our test suite works the same way on all shells, rather\n> than getting a passing run under \"bash\", only to have it fail with\n> \"-x\" under say \"dash\". As the deleted code shows this is much simpler\n> to implement across our supported POSIX shells.\n\nI'm mildly negative on dropping BASH_XTRACEFD. IMHO it is not worth the\nmaintenance headache to have to remain vigilant against any shell\nfunction's stderr being examined, when there is single-line solution\nthat fixes everything. Yes, the cost of using bash is high on some\nplatforms, but \"-x\" is an optional feature (though I am sympathetic to\npeople who are _surprised_ when \"-x\" breaks things, because it really is\na subtle thing, and knowing \"you should try using bash\" is not\nimmediately obvious).\n\nSome folks (like Gábor) disagree and have done the work so far to make\nsure that we pass even with \"-x\". But it feels like this is committing\nthe whole project to that maintenance. I dunno.\n\n-Peff\n"},{"id":"442709","messageId":"211130.86y255b995.gmgdl@evledraar.gmail.com","threadId":"56994","inReplyTo":"YaaS4Idhdyo2wZ9q@coredump.intra.peff.net","subject":"Re: [PATCH] test-lib.sh: have all tests pass under \"-x\", remove BASH_XTRACEFD","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-11-30T21:50:04Z","receivedAt":"2021-11-30T21:53:49Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Tue, Nov 30 2021, Jeff King wrote:\n\n> On Mon, Nov 29, 2021 at 09:13:23PM +0100, Ævar Arnfjörð Bjarmason wrote:\n>\n>> Once that's been removed we can dig deeper and see that we only have\n>> \"BASH_XTRACEFD\" due to an earlier attempt to work around the same\n>> issue. See d88785e424a (test-lib: set BASH_XTRACEFD automatically,\n>> 2016-05-11) and the 90c8a1db9d6 (test-lib: silence \"-x\" cleanup under\n>> bash, 2017-12-08) follow-up.\n>>\n>> I.e. we had redirection in \"test_eval_\" to get more relevant trace\n>> output under bash, which in turn was only needed because\n>> BASH_XTRACEFD=1 was set, which in turn was trying to work around test\n>> failures under \"set -x\".\n>> \n>> It's better if our test suite works the same way on all shells, rather\n>> than getting a passing run under \"bash\", only to have it fail with\n>> \"-x\" under say \"dash\". As the deleted code shows this is much simpler\n>> to implement across our supported POSIX shells.\n>\n> I'm mildly negative on dropping BASH_XTRACEFD. IMHO it is not worth the\n> maintenance headache to have to remain vigilant against any shell\n> function's stderr being examined, when there is single-line solution\n> that fixes everything. Yes, the cost of using bash is high on some\n> platforms, but \"-x\" is an optional feature (though I am sympathetic to\n> people who are _surprised_ when \"-x\" breaks things, because it really is\n> a subtle thing, and knowing \"you should try using bash\" is not\n> immediately obvious).\n>\n> Some folks (like Gábor) disagree and have done the work so far to make\n> sure that we pass even with \"-x\". But it feels like this is committing\n> the whole project to that maintenance. I dunno.\n\nI'd be fine with a hard dependency on bash, but that seems unlikely to\nhappen, and if that's not the case having these hybrid modes seems like\nthe worst of both worlds.\n\nWe already hard depend on -x working without BASH_XTRACEFD for all tests\nexcept this one test don't we? I.e our CI runs with --verbose-log -x,\nand t1510-repo-setup.sh was the only test that was turning it off, and\nsome of that runs with a /bin/sh of \"dash\".\n\nNow when that test fails we'll get nice -x output like for every other\ntest, and AFAICT nothing else is changed by this patch, except the\nobvious change of us not having to support these two modes anymore.\n"},{"id":"442715","messageId":"8c80be3d-7151-272c-dc42-377380bf9754@jeffhostetler.com","threadId":"56994","inReplyTo":"86555f24-e7c4-64cb-a55c-7e2580e895f2@gmail.com","subject":"Re: [PATCH 0/2] Set GIT_TRACE2_EVENT_NESTING in test-lib.sh","fromName":"Jeff Hostetler","fromEmail":"git@jeffhostetler.com","sentAt":"2021-11-30T22:43:28Z","receivedAt":"2021-11-30T22:44:07Z","isPatch":true,"sender":{"key":"git@jeffhostetler.com","avatar":null},"body":"\n\nOn 11/30/21 10:34 AM, Derrick Stolee wrote:\n> On 11/29/2021 1:40 PM, Jeff King wrote:\n>> On Mon, Nov 29, 2021 at 01:47:44PM +0000, Derrick Stolee via GitGitGadget wrote:\n>>\n>>> As reported by Ævar [1] and diagnosed by Peff in a reply, the default\n>>> GIT_TRACE2_EVENT_NESTING is set so low that tests that look for trace2\n>>> events can start failing due to an added trace2 region. This can even be\n>>> subtle, including the fact that the progress API adds trace2 regions when in\n>>> use (which can depend on the verbose output of a test script).\n>>\n>> I think this is a good change for fixing the immediate problem of the\n>> test suite failing.\n>>\n>> But I have to wonder if this is masking a problem that real users will\n>> see. Aren't we silently discarding trace2 output that could be useful to\n>> somebody debugging or trying to get performance metrics?\n>>\n>> I.e., should we be bumping the internal nesting max of 2 to something\n>> higher? If that would that cause problems with existing metrics, should\n>> we be designing new metrics to avoid nesting?\n> \n> One thing this makes me think about is that we might want to\n> have items such as trace2_data_intmax() and trace2_data_string()\n> not be subject to the nesting limit. Only have the nesting apply\n> to the regions (which nest).\n> \n> CC'ing Jeff Hostetler for his thoughts on that idea. We will have\n> a discussion offline to see if the progress regions have started\n> to cause inconsistencies in our existing telemetry stream of\n> internal users.\n> \n> Thanks,\n> -Stolee\n> \n\n\nyeah, I set a fairly conservative nesting limit. my fear was that\nsomeone would add a region or data event to something that recurses\ndeeply -- like treewalking or directory scanning -- which in a monorepo\nwould be bad since we might get millions of rows of data.\n\nmost of the useful timing data comes from the higher level regions,\nsuch as we spent x% scanning tracked files and y% scanning untracked\nfiles. once we get down inside a particular \"concept\", the directory-\nby-directory numbers aren't usually that useful (and/or the overhead of\nlogging every directory we visit dominates).\n\nWRT allowing data events to ignore the nesting limit, we have the\nsame monorepo risk if for example someone logs how many files were\nin each directory visited, the name of every untracked file, or every\nfile that had some property of interest. That might look fine on a\nshallow WT, but might generate an unusable amount of data on a large\nrepo. so i'd recommend we proceed carefully here. (i'm not against\nit, i'm just saying we should think about it.)\n\n\nusing trace2 in a test to confirm that a piece of code was visited\nor not visited is cool, but something i don't think i anticipated.\nso i agree that we should increase the limit in the test suite.\n\n\nand yes, the addition of trace2 calls inside the progress code\ncomplicates the nesting. I'm wondering if the progress code should\nalways emit the trace2 events, regardless of whether the progress msg\nitself appears on the user's console. That way we have consistent\nlogging -- the whole point of adding the region in the progress\ncode was to bracket the time spent doing the thing that needed\nprogress messaging after all. the fact that you don't have a console\ndoesn't change the fact that there's an expensive computation there.\n\n(i'd also consider changing the \"total_objects\" data event to include\ninclude the region title so that they are more easily identified in a\ngrep, but I digress.)\n\n\nThanks,\nJeff\n\n"},{"id":"442716","messageId":"20211130224435.GA1991@szeder.dev","threadId":"56994","inReplyTo":"YaaS4Idhdyo2wZ9q@coredump.intra.peff.net","subject":"Re: [PATCH] test-lib.sh: have all tests pass under \"-x\", remove BASH_XTRACEFD","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2021-11-30T22:44:35Z","receivedAt":"2021-11-30T22:44:57Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Tue, Nov 30, 2021 at 04:08:48PM -0500, Jeff King wrote:\n> On Mon, Nov 29, 2021 at 09:13:23PM +0100, Ævar Arnfjörð Bjarmason wrote:\n> \n> > Once that's been removed we can dig deeper and see that we only have\n> > \"BASH_XTRACEFD\" due to an earlier attempt to work around the same\n> > issue. See d88785e424a (test-lib: set BASH_XTRACEFD automatically,\n> > 2016-05-11) and the 90c8a1db9d6 (test-lib: silence \"-x\" cleanup under\n> > bash, 2017-12-08) follow-up.\n> >\n> > I.e. we had redirection in \"test_eval_\" to get more relevant trace\n> > output under bash, which in turn was only needed because\n> > BASH_XTRACEFD=1 was set, which in turn was trying to work around test\n> > failures under \"set -x\".\n> > \n> > It's better if our test suite works the same way on all shells, rather\n> > than getting a passing run under \"bash\", only to have it fail with\n> > \"-x\" under say \"dash\". As the deleted code shows this is much simpler\n> > to implement across our supported POSIX shells.\n> \n> I'm mildly negative on dropping BASH_XTRACEFD.\n\nI agree, using BASH_XTRACEFD is the most robust (and least hacky) way\nto get reliable trace from our test scripts, and we should definitely\nkeep it.\n\n> IMHO it is not worth the\n> maintenance headache to have to remain vigilant against any shell\n> function's stderr being examined, when there is single-line solution\n> that fixes everything. Yes, the cost of using bash is high on some\n> platforms, but \"-x\" is an optional feature (though I am sympathetic to\n> people who are _surprised_ when \"-x\" breaks things, because it really is\n> a subtle thing, and knowing \"you should try using bash\" is not\n> immediately obvious).\n> \n> Some folks (like Gábor) disagree and have done the work so far to make\n> sure that we pass even with \"-x\". But it feels like this is committing\n> the whole project to that maintenance. I dunno.\n\nIt's not that I disagree but rather it's in our best interest to keep\n'-x' working with non-Bash shells as well, because:\n\n - We run our tests with '-x' in CI, because we want to get as much\n information out of test failures as possible.\n\n - We run our tests with dash in the Ubuntu jobs not only because\n that's the default /bin/sh, but more importantly because we want\n to avoid bashisms in our test suite.\n\n"},{"id":"442776","messageId":"211201.868rx4beer.gmgdl@evledraar.gmail.com","threadId":"56994","inReplyTo":"20211130224435.GA1991@szeder.dev","subject":"Re: [PATCH] test-lib.sh: have all tests pass under \"-x\", remove BASH_XTRACEFD","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-12-01T14:06:44Z","receivedAt":"2021-12-01T14:14:52Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Tue, Nov 30 2021, SZEDER Gábor wrote:\n\n> On Tue, Nov 30, 2021 at 04:08:48PM -0500, Jeff King wrote:\n>> On Mon, Nov 29, 2021 at 09:13:23PM +0100, Ævar Arnfjörð Bjarmason wrote:\n>> \n>> > Once that's been removed we can dig deeper and see that we only have\n>> > \"BASH_XTRACEFD\" due to an earlier attempt to work around the same\n>> > issue. See d88785e424a (test-lib: set BASH_XTRACEFD automatically,\n>> > 2016-05-11) and the 90c8a1db9d6 (test-lib: silence \"-x\" cleanup under\n>> > bash, 2017-12-08) follow-up.\n>> >\n>> > I.e. we had redirection in \"test_eval_\" to get more relevant trace\n>> > output under bash, which in turn was only needed because\n>> > BASH_XTRACEFD=1 was set, which in turn was trying to work around test\n>> > failures under \"set -x\".\n>> > \n>> > It's better if our test suite works the same way on all shells, rather\n>> > than getting a passing run under \"bash\", only to have it fail with\n>> > \"-x\" under say \"dash\". As the deleted code shows this is much simpler\n>> > to implement across our supported POSIX shells.\n>> \n>> I'm mildly negative on dropping BASH_XTRACEFD.\n>\n> I agree, using BASH_XTRACEFD is the most robust (and least hacky) way\n> to get reliable trace from our test scripts, and we should definitely\n> keep it.\n>\n>> IMHO it is not worth the\n>> maintenance headache to have to remain vigilant against any shell\n>> function's stderr being examined, when there is single-line solution\n>> that fixes everything. Yes, the cost of using bash is high on some\n>> platforms, but \"-x\" is an optional feature (though I am sympathetic to\n>> people who are _surprised_ when \"-x\" breaks things, because it really is\n>> a subtle thing, and knowing \"you should try using bash\" is not\n>> immediately obvious).\n>> \n>> Some folks (like Gábor) disagree and have done the work so far to make\n>> sure that we pass even with \"-x\". But it feels like this is committing\n>> the whole project to that maintenance. I dunno.\n>\n> It's not that I disagree but rather it's in our best interest to keep\n> '-x' working with non-Bash shells as well, because:\n>\n> - We run our tests with '-x' in CI, because we want to get as much\n> information out of test failures as possible.\n>\n> - We run our tests with dash in the Ubuntu jobs not only because\n> that's the default /bin/sh, but more importantly because we want\n> to avoid bashisms in our test suite.\n\nI don't really mind keeping BASH_XTRACEFD if it's doing something\nuseful, but I feel like I'm missing something here. Is it really doing\nsomething useful?\n\nAFAICT the ony case where it mattered was t1510-repo-setup.sh, which\nwith my upthread\n now works with\n-x, at the trivial cost of skipping a small bi of the test with -x.\n\nI suppose we could move this BASH_XTRACEFD to tht file in particular if\nanyone feel strongly about the trivial loss of tracing that entails. I\nfigured just skipping it under \"-x\" and adding a \"say\" to that effect\nwas a better trade-off.\n\nFor the rest of the test suite BASH_XTRACEFD effectively didn't matter,\nsince all our tests had to work under --verbose-log -x anyway under\ndash.\n\nAm I just wrong about this line of thinking, or is it purely that you\ntwo would like to keep BASH_XTRACEFD in case some hypothetical future\ncaller wants to make use of \"test_untraceable=UnfortunatelyYes\" again?\n\n"},{"id":"442784","messageId":"xmqqpmqg1873.fsf@gitster.g","threadId":"56994","inReplyTo":"patch-1.1-9f735bd0d49-20211129T200950Z-avarab@gmail.com","subject":"Re: [PATCH] test-lib.sh: have all tests pass under \"-x\", remove BASH_XTRACEFD","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-12-01T18:38:56Z","receivedAt":"2021-12-01T18:39:01Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n> Change the \"t1510-repo-setup.sh\" test to use a new\n> \"test_must_be_empty_trace\" wrapper, instead of turning on\n> \"test_untraceable=UnfortunatelyYes\".\n>\n> The only reason the test was incompatible with \"-x\" was because of\n> these \"test_must_be_empty\" checks, which we can instead instead skip\n> if we're running under \"set -x\".\n>\n> Skipping the tests is preferable to not having the \"-x\" output at all,\n\nEven more preferrable is not to skip the tests, no? Is this because\nwe capture the standard error stream and do not care where, between\nthe command being tested and the shell running the command, the\noutput came from?\n\n> Once that's been removed we can dig deeper and see that we only have\n> \"BASH_XTRACEFD\" due to an earlier attempt to work around the same\n> issue. See d88785e424a (test-lib: set BASH_XTRACEFD automatically,\n> 2016-05-11) and the 90c8a1db9d6 (test-lib: silence \"-x\" cleanup under\n> bash, 2017-12-08) follow-up.\n\nIf the objective is to work around the \"shell trace and program\noutput are mixed together\" issue, BASH_XTRACEFD is doing much more\nsuperb job to \"solve\" it than \"some tests depend on running without\n'-x', so punt and skip them\" to \"work around\" it, I would have to\nsay.\n\n> It's better if our test suite works the same way on all shells, rather\n> than getting a passing run under \"bash\", only to have it fail with\n> \"-x\" under say \"dash\". As the deleted code shows this is much simpler\n> to implement across our supported POSIX shells.\n\nIt is much simpler to punt and skip all tests. You can even have\n\"if run with -x stop and skip all\" in early part of test-lib.sh and\nthat would \"work around\" without developers constantly having to\nworry about the distinction between test_must_be_empty and the trace\nvariant.\n\nIt is a sad that you have to trade one good thing for another.\nSomehow, this change feels like a regression, not an improvement.\n\n\n"},{"id":"442792","messageId":"YafPIOD2bmeYrPR2@coredump.intra.peff.net","threadId":"56994","inReplyTo":"211201.868rx4beer.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH] test-lib.sh: have all tests pass under \"-x\", remove BASH_XTRACEFD","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-12-01T19:38:08Z","receivedAt":"2021-12-01T19:38:33Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Dec 01, 2021 at 03:06:44PM +0100, Ævar Arnfjörð Bjarmason wrote:\n\n> I don't really mind keeping BASH_XTRACEFD if it's doing something\n> useful, but I feel like I'm missing something here. Is it really doing\n> something useful?\n> \n> AFAICT the ony case where it mattered was t1510-repo-setup.sh, which\n> with my upthread\n> now works with\n> -x, at the trivial cost of skipping a small bi of the test with -x.\n> \n> I suppose we could move this BASH_XTRACEFD to tht file in particular if\n> anyone feel strongly about the trivial loss of tracing that entails. I\n> figured just skipping it under \"-x\" and adding a \"say\" to that effect\n> was a better trade-off.\n> \n> For the rest of the test suite BASH_XTRACEFD effectively didn't matter,\n> since all our tests had to work under --verbose-log -x anyway under\n> dash.\n> \n> Am I just wrong about this line of thinking, or is it purely that you\n> two would like to keep BASH_XTRACEFD in case some hypothetical future\n> caller wants to make use of \"test_untraceable=UnfortunatelyYes\" again?\n\nI don't know that I'd call it purely hypothetical. It was the state for\nmany years in the past. And there has to be per-test hackery to keep it\nthat way. So I wouldn't call XTRACEFD a workaround at all; in my mind it\nwas the solution.\n\nNow if people want to do that per-test hackery _also_, because it keeps\n\"-x\" more useful for folks who can't or won't use bash, I have no\nproblem with that. What I was hoping to avoid was making it a\nhard requirement, because now it's one more subtle thing for test\nwriters to have to remember and deal with.\n\nBut maybe that ship is sailed. If we are running with \"-x\" and dash in\nCI, then they're going to see confusing failures there regardless.\n\n-Peff\n"},{"id":"442794","messageId":"YafRACXUuVLs5kX4@coredump.intra.peff.net","threadId":"56994","inReplyTo":"8c80be3d-7151-272c-dc42-377380bf9754@jeffhostetler.com","subject":"Re: [PATCH 0/2] Set GIT_TRACE2_EVENT_NESTING in test-lib.sh","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-12-01T19:46:08Z","receivedAt":"2021-12-01T19:46:13Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Nov 30, 2021 at 05:43:28PM -0500, Jeff Hostetler wrote:\n\n> and yes, the addition of trace2 calls inside the progress code\n> complicates the nesting. I'm wondering if the progress code should\n> always emit the trace2 events, regardless of whether the progress msg\n> itself appears on the user's console. That way we have consistent\n> logging -- the whole point of adding the region in the progress\n> code was to bracket the time spent doing the thing that needed\n> progress messaging after all. the fact that you don't have a console\n> doesn't change the fact that there's an expensive computation there.\n\nYeah, that would at least make these things deterministic, and we'd\nnotice a nesting problem in the tests.\n\nI do think bumping the default nesting level a few notches may be worth\nit. Even if we didn't get rid of it entirely (and I am sympathetic to\nthe notion that recursive operations could produce gigantic logs), that\nwould give some headroom for reasonable levels of logging.\n\n-Peff\n"},{"id":"442796","messageId":"cover-v2-0.2-00000000000-20211201T200801Z-avarab@gmail.com","threadId":"56994","inReplyTo":"patch-1.1-9f735bd0d49-20211129T200950Z-avarab@gmail.com","subject":"[PATCH v2 0/2] test-lib.sh: have all tests pass under \"-x\", remove BASH_XTRACEFD","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-12-01T20:11:40Z","receivedAt":"2021-12-01T20:12:11Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"I think this v2 should nicely address the outstanding comments on\nv1. I.e. v1 made the trade-off of not caring about -x in t1510 for\nsome of its assertions.\n\nBut as noted in the new 1/2 here we can get a much better and easily\ndebuggable test regardless of which shell we use if we don't use the\npattern that's being worked around by test_untraceable.\n\nWhich, is the updated 2/2 notes is mostly what SZEDER was doing in\n2018 when \"test_untraceable\" was introduced, it's just that t1510\nwasn't migrated over at the time, as some other tests in the his\nseries discussed in 2/2 were.\n\nÆvar Arnfjörð Bjarmason (2):\n t1510: remove need for \"test_untraceable\", retain coverage\n test-lib.sh: remove the now-unused \"test_untraceable\" facility\n\n t/README | 3 --\n t/t1510-repo-setup.sh | 83 +++++++++++++++++++++----------------------\n t/test-lib.sh | 66 ++++------------------------------\n 3 files changed, 47 insertions(+), 105 deletions(-)\n\nRange-diff against v1:\n-: ----------- > 1: 91402624777 t1510: remove need for \"test_untraceable\", retain coverage\n1: 9f735bd0d49 ! 2: 867d18d14bd test-lib.sh: have all tests pass under \"-x\", remove BASH_XTRACEFD\n @@ Metadata\n Author: Ævar Arnfjörð Bjarmason \n \n ## Commit message ##\n - test-lib.sh: have all tests pass under \"-x\", remove BASH_XTRACEFD\n + test-lib.sh: remove the now-unused \"test_untraceable\" facility\n \n - Change the \"t1510-repo-setup.sh\" test to use a new\n - \"test_must_be_empty_trace\" wrapper, instead of turning on\n - \"test_untraceable=UnfortunatelyYes\".\n + In the preceding commit the use of \"test_untraceable=UnfortunatelyYes\"\n + was removed from \"t1510-repo-setup.sh\" in favor of more narrow\n + redirections of the output of specific commands (and not entire\n + sub-shells or functions).\n \n - The only reason the test was incompatible with \"-x\" was because of\n - these \"test_must_be_empty\" checks, which we can instead instead skip\n - if we're running under \"set -x\".\n + This is in line with the fixes in the series that introduced the\n + \"test_untraceable\" facility. See 571e472dc43 (Merge branch\n + 'sg/test-x', 2018-03-14) for the series as a whole, and\n + e.g. 91538d0cde9 (t5570-git-daemon: don't check the stderr of a\n + subshell, 2018-02-24) for a commit that's in line with the changes in\n + the preceding commit.\n \n - Skipping the tests is preferable to not having the \"-x\" output at all,\n - as it's much easier to debug the test. The result loss of test\n - coverage is minimal. If someone is adjusting a \"test_must_be_empty\"\n - test a failure might go away under \"-x\", but the new \"say\" we emit\n - here should highlight that appropriately.\n + We've thus solved the TODO item noted when \"test_untraceable\" was\n + added to \"t1510-repo-setup.sh\" in 58275069288 (t1510-repo-setup: mark\n + as untraceable with '-x', 2018-02-24).\n \n - Since the only user of \"test_untraceable\" is gone, we can remove that,\n - not only isn't it used now, but I think the rationale for using it in\n - the future no longer applies.\n + So let's remove the feature entirely. Not only is it currently unused,\n + but it actively encourages an anti-pattern in our tests. We should be\n + testing the output of specific commands, not entire subshells or\n + functions.\n \n - We'll be better off by using a simple wrapper like the new\n - \"test_must_be_empty_trace\". See 58275069288 (t1510-repo-setup: mark as\n - untraceable with '-x', 2018-02-24) and 5fc98e79fc0 (t: add means to\n - disable '-x' tracing for individual test scripts, 2018-02-24) for the\n - addition of \"test_untraceable\".\n -\n - Once that's been removed we can dig deeper and see that we only have\n - \"BASH_XTRACEFD\" due to an earlier attempt to work around the same\n - issue. See d88785e424a (test-lib: set BASH_XTRACEFD automatically,\n - 2016-05-11) and the 90c8a1db9d6 (test-lib: silence \"-x\" cleanup under\n - bash, 2017-12-08) follow-up.\n -\n - I.e. we had redirection in \"test_eval_\" to get more relevant trace\n - output under bash, which in turn was only needed because\n - BASH_XTRACEFD=1 was set, which in turn was trying to work around test\n - failures under \"set -x\".\n -\n - It's better if our test suite works the same way on all shells, rather\n - than getting a passing run under \"bash\", only to have it fail with\n - \"-x\" under say \"dash\". As the deleted code shows this is much simpler\n - to implement across our supported POSIX shells.\n + That the \"-x\" output had to be disabled as a result is only one\n + symptom, but even under bash those tests will be harder to debug as\n + the subsequent check of the redirected file will be far removed from\n + the command that emitted the output.\n \n Signed-off-by: Ævar Arnfjörð Bjarmason \n \n @@ t/README: appropriately before running \"make\". Short options can be bundled, i.e\n -d::\n --debug::\n \n - ## t/t1510-repo-setup.sh ##\n -@@ t/t1510-repo-setup.sh: A few rules for repo setup:\n - prefix is NULL.\n - \"\n - \n --# This test heavily relies on the standard error of nested function calls.\n --test_untraceable=UnfortunatelyYes\n -+test_must_be_empty_trace () {\n -+\tif want_trace\n -+\tthen\n -+\t\tsay \"$TEST_NAME does not check test_must_be_empty on \\\"$@\\\" under -x\"\n -+\t\treturn 0\n -+\tfi\n -+\ttest_must_be_empty \"$@\"\n -+}\n - \n - TEST_PASSES_SANITIZE_LEAK=true\n - . ./test-lib.sh\n -@@ t/t1510-repo-setup.sh: test_expect_success '#0: nonbare repo, no explicit configuration' '\n - \ttry_repo 0 unset unset unset \"\" unset \\\n - \t\t.git \"$here/0\" \"$here/0\" \"(null)\" \\\n - \t\t.git \"$here/0\" \"$here/0\" sub/ 2>message &&\n --\ttest_must_be_empty message\n -+\ttest_must_be_empty_trace message\n - '\n - \n - test_expect_success '#1: GIT_WORK_TREE without explicit GIT_DIR is accepted' '\n - \ttry_repo 1 \"$here\" unset unset \"\" unset \\\n - \t\t\"$here/1/.git\" \"$here\" \"$here\" 1/ \\\n - \t\t\"$here/1/.git\" \"$here\" \"$here\" 1/sub/ 2>message &&\n --\ttest_must_be_empty message\n -+\ttest_must_be_empty_trace message\n - '\n - \n - test_expect_success '#2: worktree defaults to cwd with explicit GIT_DIR' '\n -@@ t/t1510-repo-setup.sh: test_expect_success '#4: core.worktree without GIT_DIR set is accepted' '\n - \ttry_case 4 unset unset \\\n - \t\t.git \"$here/4/sub\" \"$here/4\" \"(null)\" \\\n - \t\t\"$here/4/.git\" \"$here/4/sub\" \"$here/4/sub\" \"(null)\" 2>message &&\n --\ttest_must_be_empty message\n -+\ttest_must_be_empty_trace message\n - '\n - \n - test_expect_success '#5: core.worktree + GIT_WORK_TREE is accepted' '\n -@@ t/t1510-repo-setup.sh: test_expect_success '#5: core.worktree + GIT_WORK_TREE is accepted' '\n - \ttry_repo 5a .. unset \"$here/5a\" \"\" unset \\\n - \t\t\"$here/5a/.git\" \"$here\" \"$here\" 5a/ \\\n - \t\t\"$here/5a/.git\" \"$here/5a\" \"$here/5a\" sub/ &&\n --\ttest_must_be_empty message\n -+\ttest_must_be_empty_trace message\n - '\n - \n - test_expect_success '#6: setting GIT_DIR brings core.worktree to life' '\n -@@ t/t1510-repo-setup.sh: test_expect_success '#9: GIT_WORK_TREE accepted with gitfile' '\n - \ttry_repo 9 wt unset unset gitfile unset \\\n - \t\t\"$here/9.git\" \"$here/9/wt\" \"$here/9\" \"(null)\" \\\n - \t\t\"$here/9.git\" \"$here/9/sub/wt\" \"$here/9/sub\" \"(null)\" 2>message &&\n --\ttest_must_be_empty message\n -+\ttest_must_be_empty_trace message\n - '\n - \n - test_expect_success '#10: GIT_DIR can point to gitfile' '\n -@@ t/t1510-repo-setup.sh: test_expect_success '#12: core.worktree with gitfile is accepted' '\n - \ttry_repo 12 unset unset \"$here/12\" gitfile unset \\\n - \t\t\"$here/12.git\" \"$here/12\" \"$here/12\" \"(null)\" \\\n - \t\t\"$here/12.git\" \"$here/12\" \"$here/12\" sub/ 2>message &&\n --\ttest_must_be_empty message\n -+\ttest_must_be_empty_trace message\n - '\n - \n - test_expect_success '#13: core.worktree+GIT_WORK_TREE accepted (with gitfile)' '\n -@@ t/t1510-repo-setup.sh: test_expect_success '#13: core.worktree+GIT_WORK_TREE accepted (with gitfile)' '\n - \ttry_repo 13 non-existent-too unset non-existent gitfile unset \\\n - \t\t\"$here/13.git\" \"$here/13/non-existent-too\" \"$here/13\" \"(null)\" \\\n - \t\t\"$here/13.git\" \"$here/13/sub/non-existent-too\" \"$here/13/sub\" \"(null)\" 2>message &&\n --\ttest_must_be_empty message\n -+\ttest_must_be_empty_trace message\n - '\n - \n - # case #14.\n -@@ t/t1510-repo-setup.sh: test_expect_success '#17: GIT_WORK_TREE without explicit GIT_DIR is accepted (ba\n - \ttry_repo 17c \"$here/17c\" unset unset \"\" true \\\n - \t\t.git \"$here/17c\" \"$here/17c\" \"(null)\" \\\n - \t\t\"$here/17c/.git\" \"$here/17c\" \"$here/17c\" sub/ 2>message &&\n --\ttest_must_be_empty message\n -+\ttest_must_be_empty_trace message\n - '\n - \n - test_expect_success '#18: bare .git named by GIT_DIR has no worktree' '\n -@@ t/t1510-repo-setup.sh: test_expect_success '#20a: core.worktree without GIT_DIR accepted (inside .git)'\n - \t\t\"$here/20a/.git\" \"$here/20a\" \"$here/20a\" .git/wt/ &&\n - \ttry_case 20a/.git/wt/sub unset unset \\\n - \t\t\"$here/20a/.git\" \"$here/20a\" \"$here/20a\" .git/wt/sub/ &&\n --\ttest_must_be_empty message\n -+\ttest_must_be_empty_trace message\n - '\n - \n - test_expect_success '#20b/c: core.worktree and core.bare conflict' '\n -@@ t/t1510-repo-setup.sh: test_expect_success '#21: setup, core.worktree warns before overriding core.bare\n - \t\texport GIT_WORK_TREE &&\n - \t\tgit status >/dev/null\n - \t) 2>message &&\n --\ttest_must_be_empty message\n -+\ttest_must_be_empty_trace message\n - \n - '\n - run_wt_tests 21\n -@@ t/t1510-repo-setup.sh: test_expect_success '#25: GIT_WORK_TREE accepted if GIT_DIR unset (bare gitfile\n - \ttry_repo 25 \"$here/25\" unset unset gitfile true \\\n - \t\t\"$here/25.git\" \"$here/25\" \"$here/25\" \"(null)\" \\\n - \t\t\"$here/25.git\" \"$here/25\" \"$here/25\" \"sub/\" 2>message &&\n --\ttest_must_be_empty message\n -+\ttest_must_be_empty_trace message\n - '\n - \n - test_expect_success '#26: bare repo has no worktree (GIT_DIR -> gitfile case)' '\n -@@ t/t1510-repo-setup.sh: test_expect_success '#29: setup' '\n - \t\texport GIT_WORK_TREE &&\n - \t\tgit status\n - \t) 2>message &&\n --\ttest_must_be_empty message\n -+\ttest_must_be_empty_trace message\n - '\n - run_wt_tests 29 gitfile\n - \n -\n ## t/test-lib.sh ##\n @@ t/test-lib.sh: then\n \texit\n-- \n2.34.1.876.gdb91009a90c\n\n"},{"id":"442797","messageId":"patch-v2-1.2-91402624777-20211201T200801Z-avarab@gmail.com","threadId":"56994","inReplyTo":"cover-v2-0.2-00000000000-20211201T200801Z-avarab@gmail.com","subject":"[PATCH v2 1/2] t1510: remove need for \"test_untraceable\", retain coverage","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-12-01T20:11:41Z","receivedAt":"2021-12-01T20:12:35Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Amend the tests checking whether stderr is empty added in\n4868b2ea17b (Subject: setup: officially support --work-tree without\n--git-dir, 2011-01-19) work portably on all POSIX shells, instead\nsuppressing the trace output with \"test_untraceable\" on shells that\naren't bash.\n\nThe tests that used the \"try_repo\" helper wanted to check whether git\ncommands invoked therein would emit anything on stderr. To do this\nthey invoked the function and redirected the stderr to a \"message\"\nfile.\n\nIn 58275069288 (t1510-repo-setup: mark as untraceable with '-x',\n2018-02-24) these were made to use \"test_untraceable\" introduced in\n5fc98e79fc0 (t: add means to disable '-x' tracing for individual test\nscripts, 2018-02-24).\n\nIt is better to have the \"try_repo\" function itself start with a\n\"test_when_finished 'rm stderr'\", and then redirect the stderr output\nfrom git commands it invokes via its helpers to a \"stderr\" file.\n\nThis means that if we have a failure it'll be closer to the source of\nthe problem, and most importantly isn't incompatible with \"-x\" on\nshells that aren't \"bash\".\n\nWe also need to split those tests that had two \"try_repo\" invocations\ninto different tests, which'll further help to narrow down any\npotential failures. This wasn't strictly necessary (an artifact of the\nuse of \"test_when_finished\"), but the pattern enforces better test\nhygiene.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n t/t1510-repo-setup.sh | 83 +++++++++++++++++++++----------------------\n 1 file changed, 40 insertions(+), 43 deletions(-)\n\ndiff --git a/t/t1510-repo-setup.sh b/t/t1510-repo-setup.sh\nindex 591505a39c0..f1748ac4a19 100755\n--- a/t/t1510-repo-setup.sh\n+++ b/t/t1510-repo-setup.sh\n@@ -40,9 +40,6 @@ A few rules for repo setup:\n prefix is NULL.\n \"\n \n-# This test heavily relies on the standard error of nested function calls.\n-test_untraceable=UnfortunatelyYes\n-\n TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n \n@@ -62,7 +59,7 @@ test_repo () {\n \t\t\texport GIT_WORK_TREE\n \t\tfi &&\n \t\trm -f trace &&\n-\t\tGIT_TRACE_SETUP=\"$(pwd)/trace\" git symbolic-ref HEAD >/dev/null &&\n+\t\tGIT_TRACE_SETUP=\"$(pwd)/trace\" git symbolic-ref HEAD >/dev/null 2>>stderr &&\n \t\tgrep '^setup: ' trace >result &&\n \t\ttest_cmp expected result\n \t)\n@@ -72,7 +69,7 @@ maybe_config () {\n \tfile=$1 var=$2 value=$3 &&\n \tif test \"$value\" != unset\n \tthen\n-\t\tgit config --file=\"$file\" \"$var\" \"$value\"\n+\t\tgit config --file=\"$file\" \"$var\" \"$value\" 2>>stderr\n \tfi\n }\n \n@@ -80,7 +77,7 @@ setup_repo () {\n \tname=$1 worktreecfg=$2 gitfile=$3 barecfg=$4 &&\n \tsane_unset GIT_DIR GIT_WORK_TREE &&\n \n-\tgit -c init.defaultBranch=initial init \"$name\" &&\n+\tgit -c init.defaultBranch=initial init \"$name\" 2>>stderr &&\n \tmaybe_config \"$name/.git/config\" core.worktree \"$worktreecfg\" &&\n \tmaybe_config \"$name/.git/config\" core.bare \"$barecfg\" &&\n \tmkdir -p \"$name/sub/sub\" &&\n@@ -210,10 +207,12 @@ run_wt_tests () {\n #\t(git dir) (work tree) (cwd) (prefix)\t<-- from subdir\n try_repo () {\n \tname=$1 worktreeenv=$2 gitdirenv=$3 &&\n+\ttest_when_finished \"rm stderr\" &&\n \tsetup_repo \"$name\" \"$4\" \"$5\" \"$6\" &&\n \tshift 6 &&\n \ttry_case \"$name\" \"$worktreeenv\" \"$gitdirenv\" \\\n \t\t\"$1\" \"$2\" \"$3\" \"$4\" &&\n+\ttest_must_be_empty stderr &&\n \tshift 4 &&\n \tcase \"$gitdirenv\" in\n \t/* | ?:/* | unset) ;;\n@@ -221,7 +220,8 @@ try_repo () {\n \t\tgitdirenv=../$gitdirenv ;;\n \tesac &&\n \ttry_case \"$name/sub\" \"$worktreeenv\" \"$gitdirenv\" \\\n-\t\t\"$1\" \"$2\" \"$3\" \"$4\"\n+\t\t\"$1\" \"$2\" \"$3\" \"$4\" &&\n+\ttest_must_be_empty stderr\n }\n \n # Bit 0 = GIT_WORK_TREE\n@@ -234,15 +234,13 @@ try_repo () {\n test_expect_success '#0: nonbare repo, no explicit configuration' '\n \ttry_repo 0 unset unset unset \"\" unset \\\n \t\t.git \"$here/0\" \"$here/0\" \"(null)\" \\\n-\t\t.git \"$here/0\" \"$here/0\" sub/ 2>message &&\n-\ttest_must_be_empty message\n+\t\t.git \"$here/0\" \"$here/0\" sub/\n '\n \n test_expect_success '#1: GIT_WORK_TREE without explicit GIT_DIR is accepted' '\n \ttry_repo 1 \"$here\" unset unset \"\" unset \\\n \t\t\"$here/1/.git\" \"$here\" \"$here\" 1/ \\\n-\t\t\"$here/1/.git\" \"$here\" \"$here\" 1/sub/ 2>message &&\n-\ttest_must_be_empty message\n+\t\t\"$here/1/.git\" \"$here\" \"$here\" 1/sub/\n '\n \n test_expect_success '#2: worktree defaults to cwd with explicit GIT_DIR' '\n@@ -268,19 +266,20 @@ test_expect_success '#4: core.worktree without GIT_DIR set is accepted' '\n \tmkdir -p 4/sub sub &&\n \ttry_case 4 unset unset \\\n \t\t.git \"$here/4/sub\" \"$here/4\" \"(null)\" \\\n-\t\t\"$here/4/.git\" \"$here/4/sub\" \"$here/4/sub\" \"(null)\" 2>message &&\n-\ttest_must_be_empty message\n+\t\t\"$here/4/.git\" \"$here/4/sub\" \"$here/4/sub\" \"(null)\"\n '\n \n-test_expect_success '#5: core.worktree + GIT_WORK_TREE is accepted' '\n+test_expect_success '#5.1: core.worktree + GIT_WORK_TREE is accepted' '\n \t# or: you cannot intimidate away the lack of GIT_DIR setting\n \ttry_repo 5 \"$here\" unset \"$here/5\" \"\" unset \\\n \t\t\"$here/5/.git\" \"$here\" \"$here\" 5/ \\\n-\t\t\"$here/5/.git\" \"$here\" \"$here\" 5/sub/ 2>message &&\n+\t\t\"$here/5/.git\" \"$here\" \"$here\" 5/sub/\n+'\n+\n+test_expect_success '#5.2: core.worktree + GIT_WORK_TREE is accepted' '\n \ttry_repo 5a .. unset \"$here/5a\" \"\" unset \\\n \t\t\"$here/5a/.git\" \"$here\" \"$here\" 5a/ \\\n-\t\t\"$here/5a/.git\" \"$here/5a\" \"$here/5a\" sub/ &&\n-\ttest_must_be_empty message\n+\t\t\"$here/5a/.git\" \"$here/5a\" \"$here/5a\" sub/\n '\n \n test_expect_success '#6: setting GIT_DIR brings core.worktree to life' '\n@@ -376,8 +375,7 @@ test_expect_success '#9: GIT_WORK_TREE accepted with gitfile' '\n \tmkdir -p 9/wt &&\n \ttry_repo 9 wt unset unset gitfile unset \\\n \t\t\"$here/9.git\" \"$here/9/wt\" \"$here/9\" \"(null)\" \\\n-\t\t\"$here/9.git\" \"$here/9/sub/wt\" \"$here/9/sub\" \"(null)\" 2>message &&\n-\ttest_must_be_empty message\n+\t\t\"$here/9.git\" \"$here/9/sub/wt\" \"$here/9/sub\" \"(null)\"\n '\n \n test_expect_success '#10: GIT_DIR can point to gitfile' '\n@@ -402,16 +400,14 @@ run_wt_tests 11 gitfile\n test_expect_success '#12: core.worktree with gitfile is accepted' '\n \ttry_repo 12 unset unset \"$here/12\" gitfile unset \\\n \t\t\"$here/12.git\" \"$here/12\" \"$here/12\" \"(null)\" \\\n-\t\t\"$here/12.git\" \"$here/12\" \"$here/12\" sub/ 2>message &&\n-\ttest_must_be_empty message\n+\t\t\"$here/12.git\" \"$here/12\" \"$here/12\" sub/\n '\n \n test_expect_success '#13: core.worktree+GIT_WORK_TREE accepted (with gitfile)' '\n \t# or: you cannot intimidate away the lack of GIT_DIR setting\n \ttry_repo 13 non-existent-too unset non-existent gitfile unset \\\n \t\t\"$here/13.git\" \"$here/13/non-existent-too\" \"$here/13\" \"(null)\" \\\n-\t\t\"$here/13.git\" \"$here/13/sub/non-existent-too\" \"$here/13/sub\" \"(null)\" 2>message &&\n-\ttest_must_be_empty message\n+\t\t\"$here/13.git\" \"$here/13/sub/non-existent-too\" \"$here/13/sub\" \"(null)\"\n '\n \n # case #14.\n@@ -549,8 +545,7 @@ test_expect_success '#17: GIT_WORK_TREE without explicit GIT_DIR is accepted (ba\n \tmkdir -p 17b/.git/wt/sub &&\n \n \ttry_case 17a/.git \"$here/17a\" unset \\\n-\t\t\"$here/17a/.git\" \"$here/17a\" \"$here/17a\" .git/ \\\n-\t\t2>message &&\n+\t\t\"$here/17a/.git\" \"$here/17a\" \"$here/17a\" .git/ &&\n \ttry_case 17a/.git/wt \"$here/17a\" unset \\\n \t\t\"$here/17a/.git\" \"$here/17a\" \"$here/17a\" .git/wt/ &&\n \ttry_case 17a/.git/wt/sub \"$here/17a\" unset \\\n@@ -565,14 +560,16 @@ test_expect_success '#17: GIT_WORK_TREE without explicit GIT_DIR is accepted (ba\n \n \ttry_repo 17c \"$here/17c\" unset unset \"\" true \\\n \t\t.git \"$here/17c\" \"$here/17c\" \"(null)\" \\\n-\t\t\"$here/17c/.git\" \"$here/17c\" \"$here/17c\" sub/ 2>message &&\n-\ttest_must_be_empty message\n+\t\t\"$here/17c/.git\" \"$here/17c\" \"$here/17c\" sub/\n '\n \n-test_expect_success '#18: bare .git named by GIT_DIR has no worktree' '\n+test_expect_success '#18.1: bare .git named by GIT_DIR has no worktree' '\n \ttry_repo 18 unset .git unset \"\" true \\\n \t\t.git \"(null)\" \"$here/18\" \"(null)\" \\\n-\t\t../.git \"(null)\" \"$here/18/sub\" \"(null)\" &&\n+\t\t../.git \"(null)\" \"$here/18/sub\" \"(null)\"\n+'\n+\n+test_expect_success '#18.2: bare .git named by GIT_DIR has no worktree' '\n \ttry_repo 18b unset \"$here/18b/.git\" unset \"\" true \\\n \t\t\"$here/18b/.git\" \"(null)\" \"$here/18b\" \"(null)\" \\\n \t\t\"$here/18b/.git\" \"(null)\" \"$here/18b/sub\" \"(null)\"\n@@ -590,12 +587,11 @@ test_expect_success '#20a: core.worktree without GIT_DIR accepted (inside .git)'\n \tsetup_repo 20a \"$here/20a\" \"\" unset &&\n \tmkdir -p 20a/.git/wt/sub &&\n \ttry_case 20a/.git unset unset \\\n-\t\t\"$here/20a/.git\" \"$here/20a\" \"$here/20a\" .git/ 2>message &&\n+\t\t\"$here/20a/.git\" \"$here/20a\" \"$here/20a\" .git/ &&\n \ttry_case 20a/.git/wt unset unset \\\n \t\t\"$here/20a/.git\" \"$here/20a\" \"$here/20a\" .git/wt/ &&\n \ttry_case 20a/.git/wt/sub unset unset \\\n-\t\t\"$here/20a/.git\" \"$here/20a\" \"$here/20a\" .git/wt/sub/ &&\n-\ttest_must_be_empty message\n+\t\t\"$here/20a/.git\" \"$here/20a\" \"$here/20a\" .git/wt/sub/\n '\n \n test_expect_success '#20b/c: core.worktree and core.bare conflict' '\n@@ -625,10 +621,9 @@ test_expect_success '#21: setup, core.worktree warns before overriding core.bare\n \t\tcd 21/.git &&\n \t\tGIT_WORK_TREE=\"$here/21\" &&\n \t\texport GIT_WORK_TREE &&\n-\t\tgit status >/dev/null\n-\t) 2>message &&\n-\ttest_must_be_empty message\n-\n+\t\tgit status 2>message &&\n+\t\ttest_must_be_empty message\n+\t)\n '\n run_wt_tests 21\n \n@@ -742,14 +737,16 @@ test_expect_success '#24: bare repo has no worktree (gitfile case)' '\n test_expect_success '#25: GIT_WORK_TREE accepted if GIT_DIR unset (bare gitfile case)' '\n \ttry_repo 25 \"$here/25\" unset unset gitfile true \\\n \t\t\"$here/25.git\" \"$here/25\" \"$here/25\" \"(null)\" \\\n-\t\t\"$here/25.git\" \"$here/25\" \"$here/25\" \"sub/\" 2>message &&\n-\ttest_must_be_empty message\n+\t\t\"$here/25.git\" \"$here/25\" \"$here/25\" \"sub/\"\n '\n \n-test_expect_success '#26: bare repo has no worktree (GIT_DIR -> gitfile case)' '\n+test_expect_success '#26.1: bare repo has no worktree (GIT_DIR -> gitfile case)' '\n \ttry_repo 26 unset \"$here/26/.git\" unset gitfile true \\\n \t\t\"$here/26.git\" \"(null)\" \"$here/26\" \"(null)\" \\\n-\t\t\"$here/26.git\" \"(null)\" \"$here/26/sub\" \"(null)\" &&\n+\t\t\"$here/26.git\" \"(null)\" \"$here/26/sub\" \"(null)\"\n+'\n+\n+test_expect_success '#26.2: bare repo has no worktree (GIT_DIR -> gitfile case)' '\n \ttry_repo 26b unset .git unset gitfile true \\\n \t\t\"$here/26b.git\" \"(null)\" \"$here/26b\" \"(null)\" \\\n \t\t\"$here/26b.git\" \"(null)\" \"$here/26b/sub\" \"(null)\"\n@@ -779,9 +776,9 @@ test_expect_success '#29: setup' '\n \t\tcd 29 &&\n \t\tGIT_WORK_TREE=\"$here/29\" &&\n \t\texport GIT_WORK_TREE &&\n-\t\tgit status\n-\t) 2>message &&\n-\ttest_must_be_empty message\n+\t\tgit status 2>message &&\n+\t\ttest_must_be_empty message\n+\t)\n '\n run_wt_tests 29 gitfile\n \n-- \n2.34.1.876.gdb91009a90c\n\n"},{"id":"442798","messageId":"patch-v2-2.2-867d18d14bd-20211201T200801Z-avarab@gmail.com","threadId":"56994","inReplyTo":"cover-v2-0.2-00000000000-20211201T200801Z-avarab@gmail.com","subject":"[PATCH v2 2/2] test-lib.sh: remove the now-unused \"test_untraceable\" facility","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-12-01T20:11:42Z","receivedAt":"2021-12-01T20:12:47Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"In the preceding commit the use of \"test_untraceable=UnfortunatelyYes\"\nwas removed from \"t1510-repo-setup.sh\" in favor of more narrow\nredirections of the output of specific commands (and not entire\nsub-shells or functions).\n\nThis is in line with the fixes in the series that introduced the\n\"test_untraceable\" facility. See 571e472dc43 (Merge branch\n'sg/test-x', 2018-03-14) for the series as a whole, and\ne.g. 91538d0cde9 (t5570-git-daemon: don't check the stderr of a\nsubshell, 2018-02-24) for a commit that's in line with the changes in\nthe preceding commit.\n\nWe've thus solved the TODO item noted when \"test_untraceable\" was\nadded to \"t1510-repo-setup.sh\" in 58275069288 (t1510-repo-setup: mark\nas untraceable with '-x', 2018-02-24).\n\nSo let's remove the feature entirely. Not only is it currently unused,\nbut it actively encourages an anti-pattern in our tests. We should be\ntesting the output of specific commands, not entire subshells or\nfunctions.\n\nThat the \"-x\" output had to be disabled as a result is only one\nsymptom, but even under bash those tests will be harder to debug as\nthe subsequent check of the redirected file will be far removed from\nthe command that emitted the output.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n t/README | 3 ---\n t/test-lib.sh | 66 ++++++---------------------------------------------\n 2 files changed, 7 insertions(+), 62 deletions(-)\n\ndiff --git a/t/README b/t/README\nindex 29f72354bf1..3d30bbff34a 100644\n--- a/t/README\n+++ b/t/README\n@@ -86,9 +86,6 @@ appropriately before running \"make\". Short options can be bundled, i.e.\n -x::\n \tTurn on shell tracing (i.e., `set -x`) during the tests\n \tthemselves. Implies `--verbose`.\n-\tIgnored in test scripts that set the variable 'test_untraceable'\n-\tto a non-empty value, unless it's run with a Bash version\n-\tsupporting BASH_XTRACEFD, i.e. v4.1 or later.\n \n -d::\n --debug::\ndiff --git a/t/test-lib.sh b/t/test-lib.sh\nindex 57efcc5e97a..b008716917b 100644\n--- a/t/test-lib.sh\n+++ b/t/test-lib.sh\n@@ -381,29 +381,6 @@ then\n \texit\n fi\n \n-if test -n \"$trace\" && test -n \"$test_untraceable\"\n-then\n-\t# '-x' tracing requested, but this test script can't be reliably\n-\t# traced, unless it is run with a Bash version supporting\n-\t# BASH_XTRACEFD (introduced in Bash v4.1).\n-\t#\n-\t# Perform this version check _after_ the test script was\n-\t# potentially re-executed with $TEST_SHELL_PATH for '--tee' or\n-\t# '--verbose-log', so the right shell is checked and the\n-\t# warning is issued only once.\n-\tif test -n \"$BASH_VERSION\" && eval '\n-\t test ${BASH_VERSINFO[0]} -gt 4 || {\n-\t test ${BASH_VERSINFO[0]} -eq 4 &&\n-\t test ${BASH_VERSINFO[1]} -ge 1\n-\t }\n-\t '\n-\tthen\n-\t\t: Executed by a Bash version supporting BASH_XTRACEFD. Good.\n-\telse\n-\t\techo >&2 \"warning: ignoring -x; '$0' is untraceable without BASH_XTRACEFD\"\n-\t\ttrace=\n-\tfi\n-fi\n if test -n \"$trace\" && test -z \"$verbose_log\"\n then\n \tverbose=t\n@@ -650,19 +627,6 @@ else\n \texec 4>/dev/null 3>/dev/null\n fi\n \n-# Send any \"-x\" output directly to stderr to avoid polluting tests\n-# which capture stderr. We can do this unconditionally since it\n-# has no effect if tracing isn't turned on.\n-#\n-# Note that this sets up the trace fd as soon as we assign the variable, so it\n-# must come after the creation of descriptor 4 above. Likewise, we must never\n-# unset this, as it has the side effect of closing descriptor 4, which we\n-# use to show verbose tests to the user.\n-#\n-# Note also that we don't need or want to export it. The tracing is local to\n-# this shell, and we would not want to influence any shells we exec.\n-BASH_XTRACEFD=4\n-\n test_failure=0\n test_count=0\n test_fixed=0\n@@ -949,36 +913,20 @@ test_eval_ () {\n \t# the shell from printing the \"set +x\" to turn it off (nor the saving\n \t# of $? before that). But we can make sure that the output goes to\n \t# /dev/null.\n-\t#\n-\t# There are a few subtleties here:\n-\t#\n-\t# - we have to redirect descriptor 4 in addition to 2, to cover\n-\t# BASH_XTRACEFD\n-\t#\n-\t# - the actual eval has to come before the redirection block (since\n-\t# it needs to see descriptor 4 to set up its stderr)\n-\t#\n-\t# - likewise, any error message we print must be outside the block to\n-\t# access descriptor 4\n-\t#\n-\t# - checking $? has to come immediately after the eval, but it must\n-\t# be _inside_ the block to avoid polluting the \"set -x\" output\n-\t#\n-\n-\ttest_eval_inner_ \"$@\" &3 2>&4\n \t{\n+\t\ttest_eval_inner_ \"$@\" &3 2>&4\n \t\ttest_eval_ret_=$?\n \t\tif want_trace\n \t\tthen\n \t\t\ttest 1 = $trace_level_ && set +x\n \t\t\ttrace_level_=$(($trace_level_-1))\n-\t\tfi\n-\t} 2>/dev/null 4>&2\n \n-\tif test \"$test_eval_ret_\" != 0 && want_trace\n-\tthen\n-\t\tsay_color error >&4 \"error: last command exited with \\$?=$test_eval_ret_\"\n-\tfi\n+\t\t\tif test \"$test_eval_ret_\" != 0\n+\t\t\tthen\n+\t\t\t\tsay_color error >&4 \"error: last command exited with \\$?=$test_eval_ret_\"\n+\t\t\tfi\n+\t\tfi\n+\t} 2>/dev/null\n \treturn $test_eval_ret_\n }\n \n-- \n2.34.1.876.gdb91009a90c\n\n"},{"id":"442912","messageId":"20211202191635.GB1991@szeder.dev","threadId":"56994","inReplyTo":"patch-v2-1.2-91402624777-20211201T200801Z-avarab@gmail.com","subject":"Re: [PATCH v2 1/2] t1510: remove need for \"test_untraceable\", retain coverage","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2021-12-02T19:16:35Z","receivedAt":"2021-12-02T19:16:49Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Wed, Dec 01, 2021 at 09:11:41PM +0100, Ævar Arnfjörð Bjarmason wrote:\n> Amend the tests checking whether stderr is empty added in\n> 4868b2ea17b (Subject: setup: officially support --work-tree without\n> --git-dir, 2011-01-19) work portably on all POSIX shells, instead\n> suppressing the trace output with \"test_untraceable\" on shells that\n> aren't bash.\n> \n> The tests that used the \"try_repo\" helper wanted to check whether git\n> commands invoked therein would emit anything on stderr. To do this\n> they invoked the function and redirected the stderr to a \"message\"\n> file.\n> \n> In 58275069288 (t1510-repo-setup: mark as untraceable with '-x',\n> 2018-02-24) these were made to use \"test_untraceable\" introduced in\n> 5fc98e79fc0 (t: add means to disable '-x' tracing for individual test\n> scripts, 2018-02-24).\n> \n> It is better to have the \"try_repo\" function itself start with a\n> \"test_when_finished 'rm stderr'\", and then redirect the stderr output\n> from git commands it invokes via its helpers to a \"stderr\" file.\n> \n> This means that if we have a failure it'll be closer to the source of\n> the problem, and most importantly isn't incompatible with \"-x\" on\n> shells that aren't \"bash\".\n> \n> We also need to split those tests that had two \"try_repo\" invocations\n> into different tests, which'll further help to narrow down any\n> potential failures. This wasn't strictly necessary (an artifact of the\n> use of \"test_when_finished\"), but the pattern enforces better test\n> hygiene.\n> \n> Signed-off-by: Ævar Arnfjörð Bjarmason \n> ---\n> t/t1510-repo-setup.sh | 83 +++++++++++++++++++++----------------------\n> 1 file changed, 40 insertions(+), 43 deletions(-)\n> \n> diff --git a/t/t1510-repo-setup.sh b/t/t1510-repo-setup.sh\n> index 591505a39c0..f1748ac4a19 100755\n> --- a/t/t1510-repo-setup.sh\n> +++ b/t/t1510-repo-setup.sh\n> @@ -40,9 +40,6 @@ A few rules for repo setup:\n> prefix is NULL.\n> \"\n> \n> -# This test heavily relies on the standard error of nested function calls.\n> -test_untraceable=UnfortunatelyYes\n> -\n> TEST_PASSES_SANITIZE_LEAK=true\n> . ./test-lib.sh\n> \n> @@ -62,7 +59,7 @@ test_repo () {\n> \t\t\texport GIT_WORK_TREE\n> \t\tfi &&\n> \t\trm -f trace &&\n> -\t\tGIT_TRACE_SETUP=\"$(pwd)/trace\" git symbolic-ref HEAD >/dev/null &&\n> +\t\tGIT_TRACE_SETUP=\"$(pwd)/trace\" git symbolic-ref HEAD >/dev/null 2>>stderr &&\n\nI suspect that it's lines like this that make Peff argue for\nBASH_XTRACEFD :)\n\nWhile this is not a compound command, it does contain a command\nsubstitution, and the trace generated when executing the command in\nthat command substitution goes to the command's stderr, and then,\nbecause of the redirection, to the 'stderr' file.\n\nI find it suspicious that this doesn't trigger a failure in a\n'test_must_be_empty stderr' later on.\n\n> \t\tgrep '^setup: ' trace >result &&\n> \t\ttest_cmp expected result\n> \t)\n> @@ -72,7 +69,7 @@ maybe_config () {\n> \tfile=$1 var=$2 value=$3 &&\n> \tif test \"$value\" != unset\n> \tthen\n> -\t\tgit config --file=\"$file\" \"$var\" \"$value\"\n> +\t\tgit config --file=\"$file\" \"$var\" \"$value\" 2>>stderr\n> \tfi\n> }\n> \n> @@ -80,7 +77,7 @@ setup_repo () {\n> \tname=$1 worktreecfg=$2 gitfile=$3 barecfg=$4 &&\n> \tsane_unset GIT_DIR GIT_WORK_TREE &&\n> \n> -\tgit -c init.defaultBranch=initial init \"$name\" &&\n> +\tgit -c init.defaultBranch=initial init \"$name\" 2>>stderr &&\n> \tmaybe_config \"$name/.git/config\" core.worktree \"$worktreecfg\" &&\n> \tmaybe_config \"$name/.git/config\" core.bare \"$barecfg\" &&\n> \tmkdir -p \"$name/sub/sub\" &&\n> @@ -210,10 +207,12 @@ run_wt_tests () {\n> #\t(git dir) (work tree) (cwd) (prefix)\t<-- from subdir\n> try_repo () {\n> \tname=$1 worktreeenv=$2 gitdirenv=$3 &&\n> +\ttest_when_finished \"rm stderr\" &&\n> \tsetup_repo \"$name\" \"$4\" \"$5\" \"$6\" &&\n> \tshift 6 &&\n> \ttry_case \"$name\" \"$worktreeenv\" \"$gitdirenv\" \\\n> \t\t\"$1\" \"$2\" \"$3\" \"$4\" &&\n> +\ttest_must_be_empty stderr &&\n> \tshift 4 &&\n> \tcase \"$gitdirenv\" in\n> \t/* | ?:/* | unset) ;;\n> @@ -221,7 +220,8 @@ try_repo () {\n> \t\tgitdirenv=../$gitdirenv ;;\n> \tesac &&\n> \ttry_case \"$name/sub\" \"$worktreeenv\" \"$gitdirenv\" \\\n> -\t\t\"$1\" \"$2\" \"$3\" \"$4\"\n> +\t\t\"$1\" \"$2\" \"$3\" \"$4\" &&\n> +\ttest_must_be_empty stderr\n> }\n> \n> # Bit 0 = GIT_WORK_TREE\n> @@ -234,15 +234,13 @@ try_repo () {\n> test_expect_success '#0: nonbare repo, no explicit configuration' '\n> \ttry_repo 0 unset unset unset \"\" unset \\\n> \t\t.git \"$here/0\" \"$here/0\" \"(null)\" \\\n> -\t\t.git \"$here/0\" \"$here/0\" sub/ 2>message &&\n> -\ttest_must_be_empty message\n> +\t\t.git \"$here/0\" \"$here/0\" sub/\n> '\n> \n> test_expect_success '#1: GIT_WORK_TREE without explicit GIT_DIR is accepted' '\n> \ttry_repo 1 \"$here\" unset unset \"\" unset \\\n> \t\t\"$here/1/.git\" \"$here\" \"$here\" 1/ \\\n> -\t\t\"$here/1/.git\" \"$here\" \"$here\" 1/sub/ 2>message &&\n> -\ttest_must_be_empty message\n> +\t\t\"$here/1/.git\" \"$here\" \"$here\" 1/sub/\n> '\n> \n> test_expect_success '#2: worktree defaults to cwd with explicit GIT_DIR' '\n> @@ -268,19 +266,20 @@ test_expect_success '#4: core.worktree without GIT_DIR set is accepted' '\n> \tmkdir -p 4/sub sub &&\n> \ttry_case 4 unset unset \\\n> \t\t.git \"$here/4/sub\" \"$here/4\" \"(null)\" \\\n> -\t\t\"$here/4/.git\" \"$here/4/sub\" \"$here/4/sub\" \"(null)\" 2>message &&\n> -\ttest_must_be_empty message\n> +\t\t\"$here/4/.git\" \"$here/4/sub\" \"$here/4/sub\" \"(null)\"\n> '\n> \n> -test_expect_success '#5: core.worktree + GIT_WORK_TREE is accepted' '\n> +test_expect_success '#5.1: core.worktree + GIT_WORK_TREE is accepted' '\n> \t# or: you cannot intimidate away the lack of GIT_DIR setting\n> \ttry_repo 5 \"$here\" unset \"$here/5\" \"\" unset \\\n> \t\t\"$here/5/.git\" \"$here\" \"$here\" 5/ \\\n> -\t\t\"$here/5/.git\" \"$here\" \"$here\" 5/sub/ 2>message &&\n> +\t\t\"$here/5/.git\" \"$here\" \"$here\" 5/sub/\n> +'\n> +\n> +test_expect_success '#5.2: core.worktree + GIT_WORK_TREE is accepted' '\n> \ttry_repo 5a .. unset \"$here/5a\" \"\" unset \\\n> \t\t\"$here/5a/.git\" \"$here\" \"$here\" 5a/ \\\n> -\t\t\"$here/5a/.git\" \"$here/5a\" \"$here/5a\" sub/ &&\n> -\ttest_must_be_empty message\n> +\t\t\"$here/5a/.git\" \"$here/5a\" \"$here/5a\" sub/\n> '\n> \n> test_expect_success '#6: setting GIT_DIR brings core.worktree to life' '\n> @@ -376,8 +375,7 @@ test_expect_success '#9: GIT_WORK_TREE accepted with gitfile' '\n> \tmkdir -p 9/wt &&\n> \ttry_repo 9 wt unset unset gitfile unset \\\n> \t\t\"$here/9.git\" \"$here/9/wt\" \"$here/9\" \"(null)\" \\\n> -\t\t\"$here/9.git\" \"$here/9/sub/wt\" \"$here/9/sub\" \"(null)\" 2>message &&\n> -\ttest_must_be_empty message\n> +\t\t\"$here/9.git\" \"$here/9/sub/wt\" \"$here/9/sub\" \"(null)\"\n> '\n> \n> test_expect_success '#10: GIT_DIR can point to gitfile' '\n> @@ -402,16 +400,14 @@ run_wt_tests 11 gitfile\n> test_expect_success '#12: core.worktree with gitfile is accepted' '\n> \ttry_repo 12 unset unset \"$here/12\" gitfile unset \\\n> \t\t\"$here/12.git\" \"$here/12\" \"$here/12\" \"(null)\" \\\n> -\t\t\"$here/12.git\" \"$here/12\" \"$here/12\" sub/ 2>message &&\n> -\ttest_must_be_empty message\n> +\t\t\"$here/12.git\" \"$here/12\" \"$here/12\" sub/\n> '\n> \n> test_expect_success '#13: core.worktree+GIT_WORK_TREE accepted (with gitfile)' '\n> \t# or: you cannot intimidate away the lack of GIT_DIR setting\n> \ttry_repo 13 non-existent-too unset non-existent gitfile unset \\\n> \t\t\"$here/13.git\" \"$here/13/non-existent-too\" \"$here/13\" \"(null)\" \\\n> -\t\t\"$here/13.git\" \"$here/13/sub/non-existent-too\" \"$here/13/sub\" \"(null)\" 2>message &&\n> -\ttest_must_be_empty message\n> +\t\t\"$here/13.git\" \"$here/13/sub/non-existent-too\" \"$here/13/sub\" \"(null)\"\n> '\n> \n> # case #14.\n> @@ -549,8 +545,7 @@ test_expect_success '#17: GIT_WORK_TREE without explicit GIT_DIR is accepted (ba\n> \tmkdir -p 17b/.git/wt/sub &&\n> \n> \ttry_case 17a/.git \"$here/17a\" unset \\\n> -\t\t\"$here/17a/.git\" \"$here/17a\" \"$here/17a\" .git/ \\\n> -\t\t2>message &&\n> +\t\t\"$here/17a/.git\" \"$here/17a\" \"$here/17a\" .git/ &&\n> \ttry_case 17a/.git/wt \"$here/17a\" unset \\\n> \t\t\"$here/17a/.git\" \"$here/17a\" \"$here/17a\" .git/wt/ &&\n> \ttry_case 17a/.git/wt/sub \"$here/17a\" unset \\\n> @@ -565,14 +560,16 @@ test_expect_success '#17: GIT_WORK_TREE without explicit GIT_DIR is accepted (ba\n> \n> \ttry_repo 17c \"$here/17c\" unset unset \"\" true \\\n> \t\t.git \"$here/17c\" \"$here/17c\" \"(null)\" \\\n> -\t\t\"$here/17c/.git\" \"$here/17c\" \"$here/17c\" sub/ 2>message &&\n> -\ttest_must_be_empty message\n> +\t\t\"$here/17c/.git\" \"$here/17c\" \"$here/17c\" sub/\n> '\n> \n> -test_expect_success '#18: bare .git named by GIT_DIR has no worktree' '\n> +test_expect_success '#18.1: bare .git named by GIT_DIR has no worktree' '\n> \ttry_repo 18 unset .git unset \"\" true \\\n> \t\t.git \"(null)\" \"$here/18\" \"(null)\" \\\n> -\t\t../.git \"(null)\" \"$here/18/sub\" \"(null)\" &&\n> +\t\t../.git \"(null)\" \"$here/18/sub\" \"(null)\"\n> +'\n> +\n> +test_expect_success '#18.2: bare .git named by GIT_DIR has no worktree' '\n> \ttry_repo 18b unset \"$here/18b/.git\" unset \"\" true \\\n> \t\t\"$here/18b/.git\" \"(null)\" \"$here/18b\" \"(null)\" \\\n> \t\t\"$here/18b/.git\" \"(null)\" \"$here/18b/sub\" \"(null)\"\n> @@ -590,12 +587,11 @@ test_expect_success '#20a: core.worktree without GIT_DIR accepted (inside .git)'\n> \tsetup_repo 20a \"$here/20a\" \"\" unset &&\n> \tmkdir -p 20a/.git/wt/sub &&\n> \ttry_case 20a/.git unset unset \\\n> -\t\t\"$here/20a/.git\" \"$here/20a\" \"$here/20a\" .git/ 2>message &&\n> +\t\t\"$here/20a/.git\" \"$here/20a\" \"$here/20a\" .git/ &&\n> \ttry_case 20a/.git/wt unset unset \\\n> \t\t\"$here/20a/.git\" \"$here/20a\" \"$here/20a\" .git/wt/ &&\n> \ttry_case 20a/.git/wt/sub unset unset \\\n> -\t\t\"$here/20a/.git\" \"$here/20a\" \"$here/20a\" .git/wt/sub/ &&\n> -\ttest_must_be_empty message\n> +\t\t\"$here/20a/.git\" \"$here/20a\" \"$here/20a\" .git/wt/sub/\n> '\n> \n> test_expect_success '#20b/c: core.worktree and core.bare conflict' '\n> @@ -625,10 +621,9 @@ test_expect_success '#21: setup, core.worktree warns before overriding core.bare\n> \t\tcd 21/.git &&\n> \t\tGIT_WORK_TREE=\"$here/21\" &&\n> \t\texport GIT_WORK_TREE &&\n> -\t\tgit status >/dev/null\n> -\t) 2>message &&\n> -\ttest_must_be_empty message\n> -\n> +\t\tgit status 2>message &&\n> +\t\ttest_must_be_empty message\n> +\t)\n> '\n> run_wt_tests 21\n> \n> @@ -742,14 +737,16 @@ test_expect_success '#24: bare repo has no worktree (gitfile case)' '\n> test_expect_success '#25: GIT_WORK_TREE accepted if GIT_DIR unset (bare gitfile case)' '\n> \ttry_repo 25 \"$here/25\" unset unset gitfile true \\\n> \t\t\"$here/25.git\" \"$here/25\" \"$here/25\" \"(null)\" \\\n> -\t\t\"$here/25.git\" \"$here/25\" \"$here/25\" \"sub/\" 2>message &&\n> -\ttest_must_be_empty message\n> +\t\t\"$here/25.git\" \"$here/25\" \"$here/25\" \"sub/\"\n> '\n> \n> -test_expect_success '#26: bare repo has no worktree (GIT_DIR -> gitfile case)' '\n> +test_expect_success '#26.1: bare repo has no worktree (GIT_DIR -> gitfile case)' '\n> \ttry_repo 26 unset \"$here/26/.git\" unset gitfile true \\\n> \t\t\"$here/26.git\" \"(null)\" \"$here/26\" \"(null)\" \\\n> -\t\t\"$here/26.git\" \"(null)\" \"$here/26/sub\" \"(null)\" &&\n> +\t\t\"$here/26.git\" \"(null)\" \"$here/26/sub\" \"(null)\"\n> +'\n> +\n> +test_expect_success '#26.2: bare repo has no worktree (GIT_DIR -> gitfile case)' '\n> \ttry_repo 26b unset .git unset gitfile true \\\n> \t\t\"$here/26b.git\" \"(null)\" \"$here/26b\" \"(null)\" \\\n> \t\t\"$here/26b.git\" \"(null)\" \"$here/26b/sub\" \"(null)\"\n> @@ -779,9 +776,9 @@ test_expect_success '#29: setup' '\n> \t\tcd 29 &&\n> \t\tGIT_WORK_TREE=\"$here/29\" &&\n> \t\texport GIT_WORK_TREE &&\n> -\t\tgit status\n> -\t) 2>message &&\n> -\ttest_must_be_empty message\n> +\t\tgit status 2>message &&\n> +\t\ttest_must_be_empty message\n> +\t)\n> '\n> run_wt_tests 29 gitfile\n> \n> -- \n> 2.34.1.876.gdb91009a90c\n> \n"},{"id":"442913","messageId":"20211202192802.GC1991@szeder.dev","threadId":"56994","inReplyTo":"20211202191635.GB1991@szeder.dev","subject":"Re: [PATCH v2 1/2] t1510: remove need for \"test_untraceable\", retain coverage","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2021-12-02T19:28:02Z","receivedAt":"2021-12-02T19:28:07Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Thu, Dec 02, 2021 at 08:16:35PM +0100, SZEDER Gábor wrote:\n> On Wed, Dec 01, 2021 at 09:11:41PM +0100, Ævar Arnfjörð Bjarmason wrote:\n> > Amend the tests checking whether stderr is empty added in\n> > 4868b2ea17b (Subject: setup: officially support --work-tree without\n> > --git-dir, 2011-01-19) work portably on all POSIX shells, instead\n> > suppressing the trace output with \"test_untraceable\" on shells that\n> > aren't bash.\n> > \n> > The tests that used the \"try_repo\" helper wanted to check whether git\n> > commands invoked therein would emit anything on stderr. To do this\n> > they invoked the function and redirected the stderr to a \"message\"\n> > file.\n> > \n> > In 58275069288 (t1510-repo-setup: mark as untraceable with '-x',\n> > 2018-02-24) these were made to use \"test_untraceable\" introduced in\n> > 5fc98e79fc0 (t: add means to disable '-x' tracing for individual test\n> > scripts, 2018-02-24).\n> > \n> > It is better to have the \"try_repo\" function itself start with a\n> > \"test_when_finished 'rm stderr'\", and then redirect the stderr output\n> > from git commands it invokes via its helpers to a \"stderr\" file.\n> > \n> > This means that if we have a failure it'll be closer to the source of\n> > the problem, and most importantly isn't incompatible with \"-x\" on\n> > shells that aren't \"bash\".\n> > \n> > We also need to split those tests that had two \"try_repo\" invocations\n> > into different tests, which'll further help to narrow down any\n> > potential failures. This wasn't strictly necessary (an artifact of the\n> > use of \"test_when_finished\"), but the pattern enforces better test\n> > hygiene.\n> > \n> > Signed-off-by: Ævar Arnfjörð Bjarmason \n> > ---\n> > t/t1510-repo-setup.sh | 83 +++++++++++++++++++++----------------------\n> > 1 file changed, 40 insertions(+), 43 deletions(-)\n> > \n> > diff --git a/t/t1510-repo-setup.sh b/t/t1510-repo-setup.sh\n> > index 591505a39c0..f1748ac4a19 100755\n> > --- a/t/t1510-repo-setup.sh\n> > +++ b/t/t1510-repo-setup.sh\n> > @@ -40,9 +40,6 @@ A few rules for repo setup:\n> > prefix is NULL.\n> > \"\n> > \n> > -# This test heavily relies on the standard error of nested function calls.\n> > -test_untraceable=UnfortunatelyYes\n> > -\n> > TEST_PASSES_SANITIZE_LEAK=true\n> > . ./test-lib.sh\n> > \n> > @@ -62,7 +59,7 @@ test_repo () {\n> > \t\t\texport GIT_WORK_TREE\n> > \t\tfi &&\n> > \t\trm -f trace &&\n> > -\t\tGIT_TRACE_SETUP=\"$(pwd)/trace\" git symbolic-ref HEAD >/dev/null &&\n> > +\t\tGIT_TRACE_SETUP=\"$(pwd)/trace\" git symbolic-ref HEAD >/dev/null 2>>stderr &&\n> \n> I suspect that it's lines like this that make Peff argue for\n> BASH_XTRACEFD :)\n> \n> While this is not a compound command, it does contain a command\n> substitution, and the trace generated when executing the command in\n> that command substitution goes to the command's stderr, and then,\n> because of the redirection, to the 'stderr' file.\n> \n> I find it suspicious that this doesn't trigger a failure in a\n> 'test_must_be_empty stderr' later on.\n\nAh, that's because this hunk is executed in a subshell that starts\nwith 'cd \"$1\"', creating an extra 'stderr' file in a subdirectory:\n\n $ ./t1510-repo-setup.sh -r 1 -d\n [...]\n $ find trash\\ directory.t1510-repo-setup/ |grep stderr\n trash directory.t1510-repo-setup/0/stderr\n trash directory.t1510-repo-setup/0/sub/stderr\n\nChanging that redirection to '2>>\"$TRASH_DIRECTORY\"/stderr' makes a\nbunch of tests fail with:\n\n + test_must_be_empty stderr\n 'stderr' is not empty, it contains:\n + pwd\n error: last command exited with $?=1\n + rm stderr\n not ok 1 - #0: nonbare repo, no explicit configuration\n\n\n"},{"id":"443768","messageId":"YbMiK1wHzBfYvK2a@coredump.intra.peff.net","threadId":"56994","inReplyTo":"20211202191635.GB1991@szeder.dev","subject":"Re: [PATCH v2 1/2] t1510: remove need for \"test_untraceable\", retain coverage","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-12-10T09:47:23Z","receivedAt":"2021-12-10T09:47:31Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Dec 02, 2021 at 08:16:35PM +0100, SZEDER Gábor wrote:\n\n> > @@ -62,7 +59,7 @@ test_repo () {\n> > \t\t\texport GIT_WORK_TREE\n> > \t\tfi &&\n> > \t\trm -f trace &&\n> > -\t\tGIT_TRACE_SETUP=\"$(pwd)/trace\" git symbolic-ref HEAD >/dev/null &&\n> > +\t\tGIT_TRACE_SETUP=\"$(pwd)/trace\" git symbolic-ref HEAD >/dev/null 2>>stderr &&\n> \n> I suspect that it's lines like this that make Peff argue for\n> BASH_XTRACEFD :)\n> \n> While this is not a compound command, it does contain a command\n> substitution, and the trace generated when executing the command in\n> that command substitution goes to the command's stderr, and then,\n> because of the redirection, to the 'stderr' file.\n\nBetter still, the behavior varies between shells:\n\n $ bash -c 'set -x; FOO=$(echo foo) echo main >stdout 2>stderr; set +x; grep . stdout stderr'\n ++ echo foo\n + FOO=foo\n + echo main\n + set +x\n stdout:main\n\n $ dash -c 'set -x; FOO=$(echo foo) echo main >stdout 2>stderr; set +x; grep . stdout stderr'\n + FOO=foo echo main\n + set +x\n stdout:main\n stderr:+ echo foo\n\n-Peff\n"},{"id":"443772","messageId":"cover-v3-0.2-00000000000-20211210T100512Z-avarab@gmail.com","threadId":"56994","inReplyTo":"cover-v2-0.2-00000000000-20211201T200801Z-avarab@gmail.com","subject":"[PATCH v3 0/2] test-lib.sh: have all tests pass under \"-x\", remove BASH_XTRACEFD","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-12-10T10:07:53Z","receivedAt":"2021-12-10T10:08:01Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"This re-roll fixes a stupid mistake of mine pointed out by SZEDER[1]\n(thanks!): I was redirecting to \"stderr\" across function calls, but\nsome of those changed directories. Now we'll pass around an absolute\npath from the top-level function.\n\nThat bug hid an issue where we'd include the trace output in that\nstderr log still due to an interpolation of $(pwd), we can just change\nthat to $PWD, which won't have that issue.\n\n1. https://lore.kernel.org/git/20211202192802.GC1991@szeder.dev/\n\nÆvar Arnfjörð Bjarmason (2):\n t1510: remove need for \"test_untraceable\", retain coverage\n test-lib.sh: remove the now-unused \"test_untraceable\" facility\n\n t/README | 3 --\n t/t1510-repo-setup.sh | 85 +++++++++++++++++++++----------------------\n t/test-lib.sh | 66 ++++-----------------------------\n 3 files changed, 49 insertions(+), 105 deletions(-)\n\nRange-diff against v2:\n1: 91402624777 ! 1: 7876202c5b0 t1510: remove need for \"test_untraceable\", retain coverage\n @@ Commit message\n use of \"test_when_finished\"), but the pattern enforces better test\n hygiene.\n \n + The functions it calls might change directories, so we pass an\n + absolute \"$stderr_log_path\" around. We also need to change a \"$(pwd)\"\n + to \"$PWD\" in test_repo(), on e.g. \"dash\" that interpolation will be\n + traced and part of the \"2>>\" redirection.\n +\n + 1. https://lore.kernel.org/git/YbMiK1wHzBfYvK2a@coredump.intra.peff.net/\n +\n Signed-off-by: Ævar Arnfjörð Bjarmason \n \n ## t/t1510-repo-setup.sh ##\n @@ t/t1510-repo-setup.sh: test_repo () {\n \t\tfi &&\n \t\trm -f trace &&\n -\t\tGIT_TRACE_SETUP=\"$(pwd)/trace\" git symbolic-ref HEAD >/dev/null &&\n -+\t\tGIT_TRACE_SETUP=\"$(pwd)/trace\" git symbolic-ref HEAD >/dev/null 2>>stderr &&\n ++\t\tGIT_TRACE_SETUP=\"$PWD/trace\" git symbolic-ref HEAD >/dev/null 2>>\"$stderr_log_path\" &&\n \t\tgrep '^setup: ' trace >result &&\n \t\ttest_cmp expected result\n \t)\n @@ t/t1510-repo-setup.sh: maybe_config () {\n \tif test \"$value\" != unset\n \tthen\n -\t\tgit config --file=\"$file\" \"$var\" \"$value\"\n -+\t\tgit config --file=\"$file\" \"$var\" \"$value\" 2>>stderr\n ++\t\tgit config --file=\"$file\" \"$var\" \"$value\" 2>>\"$stderr_log_path\"\n \tfi\n }\n \n @@ t/t1510-repo-setup.sh: setup_repo () {\n \tsane_unset GIT_DIR GIT_WORK_TREE &&\n \n -\tgit -c init.defaultBranch=initial init \"$name\" &&\n -+\tgit -c init.defaultBranch=initial init \"$name\" 2>>stderr &&\n ++\tgit -c init.defaultBranch=initial init \"$name\" 2>>\"$stderr_log_path\" &&\n \tmaybe_config \"$name/.git/config\" core.worktree \"$worktreecfg\" &&\n \tmaybe_config \"$name/.git/config\" core.bare \"$barecfg\" &&\n \tmkdir -p \"$name/sub/sub\" &&\n @@ t/t1510-repo-setup.sh: run_wt_tests () {\n #\t(git dir) (work tree) (cwd) (prefix)\t<-- from subdir\n try_repo () {\n \tname=$1 worktreeenv=$2 gitdirenv=$3 &&\n -+\ttest_when_finished \"rm stderr\" &&\n ++\tstderr_log_path=\"$PWD/stderr\" &&\n ++\n ++\ttest_when_finished \"rm \\\"$stderr_log_path\\\"\" &&\n \tsetup_repo \"$name\" \"$4\" \"$5\" \"$6\" &&\n \tshift 6 &&\n \ttry_case \"$name\" \"$worktreeenv\" \"$gitdirenv\" \\\n \t\t\"$1\" \"$2\" \"$3\" \"$4\" &&\n -+\ttest_must_be_empty stderr &&\n ++\ttest_must_be_empty \"$stderr_log_path\" &&\n \tshift 4 &&\n \tcase \"$gitdirenv\" in\n \t/* | ?:/* | unset) ;;\n @@ t/t1510-repo-setup.sh: try_repo () {\n \ttry_case \"$name/sub\" \"$worktreeenv\" \"$gitdirenv\" \\\n -\t\t\"$1\" \"$2\" \"$3\" \"$4\"\n +\t\t\"$1\" \"$2\" \"$3\" \"$4\" &&\n -+\ttest_must_be_empty stderr\n ++\ttest_must_be_empty \"$stderr_log_path\"\n }\n \n # Bit 0 = GIT_WORK_TREE\n2: 867d18d14bd = 2: a7fc794e20d test-lib.sh: remove the now-unused \"test_untraceable\" facility\n-- \n2.34.1.932.g36842105b61\n\n"},{"id":"443773","messageId":"patch-v3-1.2-7876202c5b0-20211210T100512Z-avarab@gmail.com","threadId":"56994","inReplyTo":"cover-v3-0.2-00000000000-20211210T100512Z-avarab@gmail.com","subject":"[PATCH v3 1/2] t1510: remove need for \"test_untraceable\", retain coverage","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-12-10T10:07:54Z","receivedAt":"2021-12-10T10:08:04Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Amend the tests checking whether stderr is empty added in\n4868b2ea17b (Subject: setup: officially support --work-tree without\n--git-dir, 2011-01-19) work portably on all POSIX shells, instead\nsuppressing the trace output with \"test_untraceable\" on shells that\naren't bash.\n\nThe tests that used the \"try_repo\" helper wanted to check whether git\ncommands invoked therein would emit anything on stderr. To do this\nthey invoked the function and redirected the stderr to a \"message\"\nfile.\n\nIn 58275069288 (t1510-repo-setup: mark as untraceable with '-x',\n2018-02-24) these were made to use \"test_untraceable\" introduced in\n5fc98e79fc0 (t: add means to disable '-x' tracing for individual test\nscripts, 2018-02-24).\n\nIt is better to have the \"try_repo\" function itself start with a\n\"test_when_finished 'rm stderr'\", and then redirect the stderr output\nfrom git commands it invokes via its helpers to a \"stderr\" file.\n\nThis means that if we have a failure it'll be closer to the source of\nthe problem, and most importantly isn't incompatible with \"-x\" on\nshells that aren't \"bash\".\n\nWe also need to split those tests that had two \"try_repo\" invocations\ninto different tests, which'll further help to narrow down any\npotential failures. This wasn't strictly necessary (an artifact of the\nuse of \"test_when_finished\"), but the pattern enforces better test\nhygiene.\n\nThe functions it calls might change directories, so we pass an\nabsolute \"$stderr_log_path\" around. We also need to change a \"$(pwd)\"\nto \"$PWD\" in test_repo(), on e.g. \"dash\" that interpolation will be\ntraced and part of the \"2>>\" redirection.\n\n1. https://lore.kernel.org/git/YbMiK1wHzBfYvK2a@coredump.intra.peff.net/\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n t/t1510-repo-setup.sh | 85 +++++++++++++++++++++----------------------\n 1 file changed, 42 insertions(+), 43 deletions(-)\n\ndiff --git a/t/t1510-repo-setup.sh b/t/t1510-repo-setup.sh\nindex 591505a39c0..fc94d8b49be 100755\n--- a/t/t1510-repo-setup.sh\n+++ b/t/t1510-repo-setup.sh\n@@ -40,9 +40,6 @@ A few rules for repo setup:\n prefix is NULL.\n \"\n \n-# This test heavily relies on the standard error of nested function calls.\n-test_untraceable=UnfortunatelyYes\n-\n TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n \n@@ -62,7 +59,7 @@ test_repo () {\n \t\t\texport GIT_WORK_TREE\n \t\tfi &&\n \t\trm -f trace &&\n-\t\tGIT_TRACE_SETUP=\"$(pwd)/trace\" git symbolic-ref HEAD >/dev/null &&\n+\t\tGIT_TRACE_SETUP=\"$PWD/trace\" git symbolic-ref HEAD >/dev/null 2>>\"$stderr_log_path\" &&\n \t\tgrep '^setup: ' trace >result &&\n \t\ttest_cmp expected result\n \t)\n@@ -72,7 +69,7 @@ maybe_config () {\n \tfile=$1 var=$2 value=$3 &&\n \tif test \"$value\" != unset\n \tthen\n-\t\tgit config --file=\"$file\" \"$var\" \"$value\"\n+\t\tgit config --file=\"$file\" \"$var\" \"$value\" 2>>\"$stderr_log_path\"\n \tfi\n }\n \n@@ -80,7 +77,7 @@ setup_repo () {\n \tname=$1 worktreecfg=$2 gitfile=$3 barecfg=$4 &&\n \tsane_unset GIT_DIR GIT_WORK_TREE &&\n \n-\tgit -c init.defaultBranch=initial init \"$name\" &&\n+\tgit -c init.defaultBranch=initial init \"$name\" 2>>\"$stderr_log_path\" &&\n \tmaybe_config \"$name/.git/config\" core.worktree \"$worktreecfg\" &&\n \tmaybe_config \"$name/.git/config\" core.bare \"$barecfg\" &&\n \tmkdir -p \"$name/sub/sub\" &&\n@@ -210,10 +207,14 @@ run_wt_tests () {\n #\t(git dir) (work tree) (cwd) (prefix)\t<-- from subdir\n try_repo () {\n \tname=$1 worktreeenv=$2 gitdirenv=$3 &&\n+\tstderr_log_path=\"$PWD/stderr\" &&\n+\n+\ttest_when_finished \"rm \\\"$stderr_log_path\\\"\" &&\n \tsetup_repo \"$name\" \"$4\" \"$5\" \"$6\" &&\n \tshift 6 &&\n \ttry_case \"$name\" \"$worktreeenv\" \"$gitdirenv\" \\\n \t\t\"$1\" \"$2\" \"$3\" \"$4\" &&\n+\ttest_must_be_empty \"$stderr_log_path\" &&\n \tshift 4 &&\n \tcase \"$gitdirenv\" in\n \t/* | ?:/* | unset) ;;\n@@ -221,7 +222,8 @@ try_repo () {\n \t\tgitdirenv=../$gitdirenv ;;\n \tesac &&\n \ttry_case \"$name/sub\" \"$worktreeenv\" \"$gitdirenv\" \\\n-\t\t\"$1\" \"$2\" \"$3\" \"$4\"\n+\t\t\"$1\" \"$2\" \"$3\" \"$4\" &&\n+\ttest_must_be_empty \"$stderr_log_path\"\n }\n \n # Bit 0 = GIT_WORK_TREE\n@@ -234,15 +236,13 @@ try_repo () {\n test_expect_success '#0: nonbare repo, no explicit configuration' '\n \ttry_repo 0 unset unset unset \"\" unset \\\n \t\t.git \"$here/0\" \"$here/0\" \"(null)\" \\\n-\t\t.git \"$here/0\" \"$here/0\" sub/ 2>message &&\n-\ttest_must_be_empty message\n+\t\t.git \"$here/0\" \"$here/0\" sub/\n '\n \n test_expect_success '#1: GIT_WORK_TREE without explicit GIT_DIR is accepted' '\n \ttry_repo 1 \"$here\" unset unset \"\" unset \\\n \t\t\"$here/1/.git\" \"$here\" \"$here\" 1/ \\\n-\t\t\"$here/1/.git\" \"$here\" \"$here\" 1/sub/ 2>message &&\n-\ttest_must_be_empty message\n+\t\t\"$here/1/.git\" \"$here\" \"$here\" 1/sub/\n '\n \n test_expect_success '#2: worktree defaults to cwd with explicit GIT_DIR' '\n@@ -268,19 +268,20 @@ test_expect_success '#4: core.worktree without GIT_DIR set is accepted' '\n \tmkdir -p 4/sub sub &&\n \ttry_case 4 unset unset \\\n \t\t.git \"$here/4/sub\" \"$here/4\" \"(null)\" \\\n-\t\t\"$here/4/.git\" \"$here/4/sub\" \"$here/4/sub\" \"(null)\" 2>message &&\n-\ttest_must_be_empty message\n+\t\t\"$here/4/.git\" \"$here/4/sub\" \"$here/4/sub\" \"(null)\"\n '\n \n-test_expect_success '#5: core.worktree + GIT_WORK_TREE is accepted' '\n+test_expect_success '#5.1: core.worktree + GIT_WORK_TREE is accepted' '\n \t# or: you cannot intimidate away the lack of GIT_DIR setting\n \ttry_repo 5 \"$here\" unset \"$here/5\" \"\" unset \\\n \t\t\"$here/5/.git\" \"$here\" \"$here\" 5/ \\\n-\t\t\"$here/5/.git\" \"$here\" \"$here\" 5/sub/ 2>message &&\n+\t\t\"$here/5/.git\" \"$here\" \"$here\" 5/sub/\n+'\n+\n+test_expect_success '#5.2: core.worktree + GIT_WORK_TREE is accepted' '\n \ttry_repo 5a .. unset \"$here/5a\" \"\" unset \\\n \t\t\"$here/5a/.git\" \"$here\" \"$here\" 5a/ \\\n-\t\t\"$here/5a/.git\" \"$here/5a\" \"$here/5a\" sub/ &&\n-\ttest_must_be_empty message\n+\t\t\"$here/5a/.git\" \"$here/5a\" \"$here/5a\" sub/\n '\n \n test_expect_success '#6: setting GIT_DIR brings core.worktree to life' '\n@@ -376,8 +377,7 @@ test_expect_success '#9: GIT_WORK_TREE accepted with gitfile' '\n \tmkdir -p 9/wt &&\n \ttry_repo 9 wt unset unset gitfile unset \\\n \t\t\"$here/9.git\" \"$here/9/wt\" \"$here/9\" \"(null)\" \\\n-\t\t\"$here/9.git\" \"$here/9/sub/wt\" \"$here/9/sub\" \"(null)\" 2>message &&\n-\ttest_must_be_empty message\n+\t\t\"$here/9.git\" \"$here/9/sub/wt\" \"$here/9/sub\" \"(null)\"\n '\n \n test_expect_success '#10: GIT_DIR can point to gitfile' '\n@@ -402,16 +402,14 @@ run_wt_tests 11 gitfile\n test_expect_success '#12: core.worktree with gitfile is accepted' '\n \ttry_repo 12 unset unset \"$here/12\" gitfile unset \\\n \t\t\"$here/12.git\" \"$here/12\" \"$here/12\" \"(null)\" \\\n-\t\t\"$here/12.git\" \"$here/12\" \"$here/12\" sub/ 2>message &&\n-\ttest_must_be_empty message\n+\t\t\"$here/12.git\" \"$here/12\" \"$here/12\" sub/\n '\n \n test_expect_success '#13: core.worktree+GIT_WORK_TREE accepted (with gitfile)' '\n \t# or: you cannot intimidate away the lack of GIT_DIR setting\n \ttry_repo 13 non-existent-too unset non-existent gitfile unset \\\n \t\t\"$here/13.git\" \"$here/13/non-existent-too\" \"$here/13\" \"(null)\" \\\n-\t\t\"$here/13.git\" \"$here/13/sub/non-existent-too\" \"$here/13/sub\" \"(null)\" 2>message &&\n-\ttest_must_be_empty message\n+\t\t\"$here/13.git\" \"$here/13/sub/non-existent-too\" \"$here/13/sub\" \"(null)\"\n '\n \n # case #14.\n@@ -549,8 +547,7 @@ test_expect_success '#17: GIT_WORK_TREE without explicit GIT_DIR is accepted (ba\n \tmkdir -p 17b/.git/wt/sub &&\n \n \ttry_case 17a/.git \"$here/17a\" unset \\\n-\t\t\"$here/17a/.git\" \"$here/17a\" \"$here/17a\" .git/ \\\n-\t\t2>message &&\n+\t\t\"$here/17a/.git\" \"$here/17a\" \"$here/17a\" .git/ &&\n \ttry_case 17a/.git/wt \"$here/17a\" unset \\\n \t\t\"$here/17a/.git\" \"$here/17a\" \"$here/17a\" .git/wt/ &&\n \ttry_case 17a/.git/wt/sub \"$here/17a\" unset \\\n@@ -565,14 +562,16 @@ test_expect_success '#17: GIT_WORK_TREE without explicit GIT_DIR is accepted (ba\n \n \ttry_repo 17c \"$here/17c\" unset unset \"\" true \\\n \t\t.git \"$here/17c\" \"$here/17c\" \"(null)\" \\\n-\t\t\"$here/17c/.git\" \"$here/17c\" \"$here/17c\" sub/ 2>message &&\n-\ttest_must_be_empty message\n+\t\t\"$here/17c/.git\" \"$here/17c\" \"$here/17c\" sub/\n '\n \n-test_expect_success '#18: bare .git named by GIT_DIR has no worktree' '\n+test_expect_success '#18.1: bare .git named by GIT_DIR has no worktree' '\n \ttry_repo 18 unset .git unset \"\" true \\\n \t\t.git \"(null)\" \"$here/18\" \"(null)\" \\\n-\t\t../.git \"(null)\" \"$here/18/sub\" \"(null)\" &&\n+\t\t../.git \"(null)\" \"$here/18/sub\" \"(null)\"\n+'\n+\n+test_expect_success '#18.2: bare .git named by GIT_DIR has no worktree' '\n \ttry_repo 18b unset \"$here/18b/.git\" unset \"\" true \\\n \t\t\"$here/18b/.git\" \"(null)\" \"$here/18b\" \"(null)\" \\\n \t\t\"$here/18b/.git\" \"(null)\" \"$here/18b/sub\" \"(null)\"\n@@ -590,12 +589,11 @@ test_expect_success '#20a: core.worktree without GIT_DIR accepted (inside .git)'\n \tsetup_repo 20a \"$here/20a\" \"\" unset &&\n \tmkdir -p 20a/.git/wt/sub &&\n \ttry_case 20a/.git unset unset \\\n-\t\t\"$here/20a/.git\" \"$here/20a\" \"$here/20a\" .git/ 2>message &&\n+\t\t\"$here/20a/.git\" \"$here/20a\" \"$here/20a\" .git/ &&\n \ttry_case 20a/.git/wt unset unset \\\n \t\t\"$here/20a/.git\" \"$here/20a\" \"$here/20a\" .git/wt/ &&\n \ttry_case 20a/.git/wt/sub unset unset \\\n-\t\t\"$here/20a/.git\" \"$here/20a\" \"$here/20a\" .git/wt/sub/ &&\n-\ttest_must_be_empty message\n+\t\t\"$here/20a/.git\" \"$here/20a\" \"$here/20a\" .git/wt/sub/\n '\n \n test_expect_success '#20b/c: core.worktree and core.bare conflict' '\n@@ -625,10 +623,9 @@ test_expect_success '#21: setup, core.worktree warns before overriding core.bare\n \t\tcd 21/.git &&\n \t\tGIT_WORK_TREE=\"$here/21\" &&\n \t\texport GIT_WORK_TREE &&\n-\t\tgit status >/dev/null\n-\t) 2>message &&\n-\ttest_must_be_empty message\n-\n+\t\tgit status 2>message &&\n+\t\ttest_must_be_empty message\n+\t)\n '\n run_wt_tests 21\n \n@@ -742,14 +739,16 @@ test_expect_success '#24: bare repo has no worktree (gitfile case)' '\n test_expect_success '#25: GIT_WORK_TREE accepted if GIT_DIR unset (bare gitfile case)' '\n \ttry_repo 25 \"$here/25\" unset unset gitfile true \\\n \t\t\"$here/25.git\" \"$here/25\" \"$here/25\" \"(null)\" \\\n-\t\t\"$here/25.git\" \"$here/25\" \"$here/25\" \"sub/\" 2>message &&\n-\ttest_must_be_empty message\n+\t\t\"$here/25.git\" \"$here/25\" \"$here/25\" \"sub/\"\n '\n \n-test_expect_success '#26: bare repo has no worktree (GIT_DIR -> gitfile case)' '\n+test_expect_success '#26.1: bare repo has no worktree (GIT_DIR -> gitfile case)' '\n \ttry_repo 26 unset \"$here/26/.git\" unset gitfile true \\\n \t\t\"$here/26.git\" \"(null)\" \"$here/26\" \"(null)\" \\\n-\t\t\"$here/26.git\" \"(null)\" \"$here/26/sub\" \"(null)\" &&\n+\t\t\"$here/26.git\" \"(null)\" \"$here/26/sub\" \"(null)\"\n+'\n+\n+test_expect_success '#26.2: bare repo has no worktree (GIT_DIR -> gitfile case)' '\n \ttry_repo 26b unset .git unset gitfile true \\\n \t\t\"$here/26b.git\" \"(null)\" \"$here/26b\" \"(null)\" \\\n \t\t\"$here/26b.git\" \"(null)\" \"$here/26b/sub\" \"(null)\"\n@@ -779,9 +778,9 @@ test_expect_success '#29: setup' '\n \t\tcd 29 &&\n \t\tGIT_WORK_TREE=\"$here/29\" &&\n \t\texport GIT_WORK_TREE &&\n-\t\tgit status\n-\t) 2>message &&\n-\ttest_must_be_empty message\n+\t\tgit status 2>message &&\n+\t\ttest_must_be_empty message\n+\t)\n '\n run_wt_tests 29 gitfile\n \n-- \n2.34.1.932.g36842105b61\n\n"},{"id":"443774","messageId":"patch-v3-2.2-a7fc794e20d-20211210T100512Z-avarab@gmail.com","threadId":"56994","inReplyTo":"cover-v3-0.2-00000000000-20211210T100512Z-avarab@gmail.com","subject":"[PATCH v3 2/2] test-lib.sh: remove the now-unused \"test_untraceable\" facility","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-12-10T10:07:55Z","receivedAt":"2021-12-10T10:08:07Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"In the preceding commit the use of \"test_untraceable=UnfortunatelyYes\"\nwas removed from \"t1510-repo-setup.sh\" in favor of more narrow\nredirections of the output of specific commands (and not entire\nsub-shells or functions).\n\nThis is in line with the fixes in the series that introduced the\n\"test_untraceable\" facility. See 571e472dc43 (Merge branch\n'sg/test-x', 2018-03-14) for the series as a whole, and\ne.g. 91538d0cde9 (t5570-git-daemon: don't check the stderr of a\nsubshell, 2018-02-24) for a commit that's in line with the changes in\nthe preceding commit.\n\nWe've thus solved the TODO item noted when \"test_untraceable\" was\nadded to \"t1510-repo-setup.sh\" in 58275069288 (t1510-repo-setup: mark\nas untraceable with '-x', 2018-02-24).\n\nSo let's remove the feature entirely. Not only is it currently unused,\nbut it actively encourages an anti-pattern in our tests. We should be\ntesting the output of specific commands, not entire subshells or\nfunctions.\n\nThat the \"-x\" output had to be disabled as a result is only one\nsymptom, but even under bash those tests will be harder to debug as\nthe subsequent check of the redirected file will be far removed from\nthe command that emitted the output.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n t/README | 3 ---\n t/test-lib.sh | 66 ++++++---------------------------------------------\n 2 files changed, 7 insertions(+), 62 deletions(-)\n\ndiff --git a/t/README b/t/README\nindex 29f72354bf1..3d30bbff34a 100644\n--- a/t/README\n+++ b/t/README\n@@ -86,9 +86,6 @@ appropriately before running \"make\". Short options can be bundled, i.e.\n -x::\n \tTurn on shell tracing (i.e., `set -x`) during the tests\n \tthemselves. Implies `--verbose`.\n-\tIgnored in test scripts that set the variable 'test_untraceable'\n-\tto a non-empty value, unless it's run with a Bash version\n-\tsupporting BASH_XTRACEFD, i.e. v4.1 or later.\n \n -d::\n --debug::\ndiff --git a/t/test-lib.sh b/t/test-lib.sh\nindex 57efcc5e97a..b008716917b 100644\n--- a/t/test-lib.sh\n+++ b/t/test-lib.sh\n@@ -381,29 +381,6 @@ then\n \texit\n fi\n \n-if test -n \"$trace\" && test -n \"$test_untraceable\"\n-then\n-\t# '-x' tracing requested, but this test script can't be reliably\n-\t# traced, unless it is run with a Bash version supporting\n-\t# BASH_XTRACEFD (introduced in Bash v4.1).\n-\t#\n-\t# Perform this version check _after_ the test script was\n-\t# potentially re-executed with $TEST_SHELL_PATH for '--tee' or\n-\t# '--verbose-log', so the right shell is checked and the\n-\t# warning is issued only once.\n-\tif test -n \"$BASH_VERSION\" && eval '\n-\t test ${BASH_VERSINFO[0]} -gt 4 || {\n-\t test ${BASH_VERSINFO[0]} -eq 4 &&\n-\t test ${BASH_VERSINFO[1]} -ge 1\n-\t }\n-\t '\n-\tthen\n-\t\t: Executed by a Bash version supporting BASH_XTRACEFD. Good.\n-\telse\n-\t\techo >&2 \"warning: ignoring -x; '$0' is untraceable without BASH_XTRACEFD\"\n-\t\ttrace=\n-\tfi\n-fi\n if test -n \"$trace\" && test -z \"$verbose_log\"\n then\n \tverbose=t\n@@ -650,19 +627,6 @@ else\n \texec 4>/dev/null 3>/dev/null\n fi\n \n-# Send any \"-x\" output directly to stderr to avoid polluting tests\n-# which capture stderr. We can do this unconditionally since it\n-# has no effect if tracing isn't turned on.\n-#\n-# Note that this sets up the trace fd as soon as we assign the variable, so it\n-# must come after the creation of descriptor 4 above. Likewise, we must never\n-# unset this, as it has the side effect of closing descriptor 4, which we\n-# use to show verbose tests to the user.\n-#\n-# Note also that we don't need or want to export it. The tracing is local to\n-# this shell, and we would not want to influence any shells we exec.\n-BASH_XTRACEFD=4\n-\n test_failure=0\n test_count=0\n test_fixed=0\n@@ -949,36 +913,20 @@ test_eval_ () {\n \t# the shell from printing the \"set +x\" to turn it off (nor the saving\n \t# of $? before that). But we can make sure that the output goes to\n \t# /dev/null.\n-\t#\n-\t# There are a few subtleties here:\n-\t#\n-\t# - we have to redirect descriptor 4 in addition to 2, to cover\n-\t# BASH_XTRACEFD\n-\t#\n-\t# - the actual eval has to come before the redirection block (since\n-\t# it needs to see descriptor 4 to set up its stderr)\n-\t#\n-\t# - likewise, any error message we print must be outside the block to\n-\t# access descriptor 4\n-\t#\n-\t# - checking $? has to come immediately after the eval, but it must\n-\t# be _inside_ the block to avoid polluting the \"set -x\" output\n-\t#\n-\n-\ttest_eval_inner_ \"$@\" &3 2>&4\n \t{\n+\t\ttest_eval_inner_ \"$@\" &3 2>&4\n \t\ttest_eval_ret_=$?\n \t\tif want_trace\n \t\tthen\n \t\t\ttest 1 = $trace_level_ && set +x\n \t\t\ttrace_level_=$(($trace_level_-1))\n-\t\tfi\n-\t} 2>/dev/null 4>&2\n \n-\tif test \"$test_eval_ret_\" != 0 && want_trace\n-\tthen\n-\t\tsay_color error >&4 \"error: last command exited with \\$?=$test_eval_ret_\"\n-\tfi\n+\t\t\tif test \"$test_eval_ret_\" != 0\n+\t\t\tthen\n+\t\t\t\tsay_color error >&4 \"error: last command exited with \\$?=$test_eval_ret_\"\n+\t\t\tfi\n+\t\tfi\n+\t} 2>/dev/null\n \treturn $test_eval_ret_\n }\n \n-- \n2.34.1.932.g36842105b61\n\n"},{"id":"443775","messageId":"211210.86pmq4daxm.gmgdl@evledraar.gmail.com","threadId":"56994","inReplyTo":"YbMiK1wHzBfYvK2a@coredump.intra.peff.net","subject":"Re: [PATCH v2 1/2] t1510: remove need for \"test_untraceable\", retain coverage","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-12-10T10:08:06Z","receivedAt":"2021-12-10T10:14:01Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Fri, Dec 10 2021, Jeff King wrote:\n\n> On Thu, Dec 02, 2021 at 08:16:35PM +0100, SZEDER Gábor wrote:\n>\n>> > @@ -62,7 +59,7 @@ test_repo () {\n>> > \t\t\texport GIT_WORK_TREE\n>> > \t\tfi &&\n>> > \t\trm -f trace &&\n>> > -\t\tGIT_TRACE_SETUP=\"$(pwd)/trace\" git symbolic-ref HEAD >/dev/null &&\n>> > +\t\tGIT_TRACE_SETUP=\"$(pwd)/trace\" git symbolic-ref HEAD >/dev/null 2>>stderr &&\n>> \n>> I suspect that it's lines like this that make Peff argue for\n>> BASH_XTRACEFD :)\n>> \n>> While this is not a compound command, it does contain a command\n>> substitution, and the trace generated when executing the command in\n>> that command substitution goes to the command's stderr, and then,\n>> because of the redirection, to the 'stderr' file.\n>\n> Better still, the behavior varies between shells:\n>\n> $ bash -c 'set -x; FOO=$(echo foo) echo main >stdout 2>stderr; set +x; grep . stdout stderr'\n> ++ echo foo\n> + FOO=foo\n> + echo main\n> + set +x\n> stdout:main\n>\n> $ dash -c 'set -x; FOO=$(echo foo) echo main >stdout 2>stderr; set +x; grep . stdout stderr'\n> + FOO=foo echo main\n> + set +x\n> stdout:main\n> stderr:+ echo foo\n>\n> -Peff\n\nI just re-rolled a v3 with a fix for this and the general issue pointed\nout by SZEDER, thanks both.\n\nBut as far as keeping test_untraceable goes, isn't this a good argument\nfor dropping it?\n\nI.e. if bash was the odd shell out that included the $(...) command in\nthe trace output it would be a good reason to keep it, then we could\nperhaps use it without worrying about our trace output being corrupted\nwhen we did a \"test_cmp\" on \"stderr\" later.\n\nBut it's \"dash\" that's including it, so unless we're going to outright\nforbid it to have -x output (which is inconvienent) we'll need to deal\nwith t either way (in my re-roll I just replaced $(pwd) with $PWD).\n"},{"id":"443898","messageId":"20211212163207.GA3400@szeder.dev","threadId":"56994","inReplyTo":"patch-v3-2.2-a7fc794e20d-20211210T100512Z-avarab@gmail.com","subject":"Re: [PATCH v3 2/2] test-lib.sh: remove the now-unused \"test_untraceable\" facility","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2021-12-12T16:32:07Z","receivedAt":"2021-12-12T16:32:15Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Fri, Dec 10, 2021 at 11:07:55AM +0100, Ævar Arnfjörð Bjarmason wrote:\n> In the preceding commit the use of \"test_untraceable=UnfortunatelyYes\"\n> was removed from \"t1510-repo-setup.sh\" in favor of more narrow\n> redirections of the output of specific commands (and not entire\n> sub-shells or functions).\n> \n> This is in line with the fixes in the series that introduced the\n> \"test_untraceable\" facility. See 571e472dc43 (Merge branch\n> 'sg/test-x', 2018-03-14) for the series as a whole, and\n> e.g. 91538d0cde9 (t5570-git-daemon: don't check the stderr of a\n> subshell, 2018-02-24) for a commit that's in line with the changes in\n> the preceding commit.\n> \n> We've thus solved the TODO item noted when \"test_untraceable\" was\n> added to \"t1510-repo-setup.sh\" in 58275069288 (t1510-repo-setup: mark\n> as untraceable with '-x', 2018-02-24).\n> \n> So let's remove the feature entirely. Not only is it currently unused,\n> but it actively encourages an anti-pattern in our tests. We should be\n> testing the output of specific commands, not entire subshells or\n> functions.\n> \n> That the \"-x\" output had to be disabled as a result is only one\n> symptom, but even under bash those tests will be harder to debug as\n> the subsequent check of the redirected file will be far removed from\n> the command that emitted the output.\n> \n> Signed-off-by: Ævar Arnfjörð Bjarmason \n> ---\n> t/README | 3 ---\n> t/test-lib.sh | 66 ++++++---------------------------------------------\n> 2 files changed, 7 insertions(+), 62 deletions(-)\n> \n> diff --git a/t/README b/t/README\n> index 29f72354bf1..3d30bbff34a 100644\n> --- a/t/README\n> +++ b/t/README\n> @@ -86,9 +86,6 @@ appropriately before running \"make\". Short options can be bundled, i.e.\n> -x::\n> \tTurn on shell tracing (i.e., `set -x`) during the tests\n> \tthemselves. Implies `--verbose`.\n> -\tIgnored in test scripts that set the variable 'test_untraceable'\n> -\tto a non-empty value, unless it's run with a Bash version\n> -\tsupporting BASH_XTRACEFD, i.e. v4.1 or later.\n> \n> -d::\n> --debug::\n> diff --git a/t/test-lib.sh b/t/test-lib.sh\n> index 57efcc5e97a..b008716917b 100644\n> --- a/t/test-lib.sh\n> +++ b/t/test-lib.sh\n> @@ -381,29 +381,6 @@ then\n> \texit\n> fi\n> \n> -if test -n \"$trace\" && test -n \"$test_untraceable\"\n> -then\n> -\t# '-x' tracing requested, but this test script can't be reliably\n> -\t# traced, unless it is run with a Bash version supporting\n> -\t# BASH_XTRACEFD (introduced in Bash v4.1).\n> -\t#\n> -\t# Perform this version check _after_ the test script was\n> -\t# potentially re-executed with $TEST_SHELL_PATH for '--tee' or\n> -\t# '--verbose-log', so the right shell is checked and the\n> -\t# warning is issued only once.\n> -\tif test -n \"$BASH_VERSION\" && eval '\n> -\t test ${BASH_VERSINFO[0]} -gt 4 || {\n> -\t test ${BASH_VERSINFO[0]} -eq 4 &&\n> -\t test ${BASH_VERSINFO[1]} -ge 1\n> -\t }\n> -\t '\n> -\tthen\n> -\t\t: Executed by a Bash version supporting BASH_XTRACEFD. Good.\n> -\telse\n> -\t\techo >&2 \"warning: ignoring -x; '$0' is untraceable without BASH_XTRACEFD\"\n> -\t\ttrace=\n> -\tfi\n> -fi\n> if test -n \"$trace\" && test -z \"$verbose_log\"\n> then\n> \tverbose=t\n> @@ -650,19 +627,6 @@ else\n> \texec 4>/dev/null 3>/dev/null\n> fi\n> \n> -# Send any \"-x\" output directly to stderr to avoid polluting tests\n> -# which capture stderr. We can do this unconditionally since it\n> -# has no effect if tracing isn't turned on.\n> -#\n> -# Note that this sets up the trace fd as soon as we assign the variable, so it\n> -# must come after the creation of descriptor 4 above. Likewise, we must never\n> -# unset this, as it has the side effect of closing descriptor 4, which we\n> -# use to show verbose tests to the user.\n> -#\n> -# Note also that we don't need or want to export it. The tracing is local to\n> -# this shell, and we would not want to influence any shells we exec.\n> -BASH_XTRACEFD=4\n\nPlease do not remove BASH_XTRACEFD. And especially not like this,\nwithout even mentioning it in the commit message.\n\n> test_failure=0\n> test_count=0\n> test_fixed=0\n> @@ -949,36 +913,20 @@ test_eval_ () {\n> \t# the shell from printing the \"set +x\" to turn it off (nor the saving\n> \t# of $? before that). But we can make sure that the output goes to\n> \t# /dev/null.\n> -\t#\n> -\t# There are a few subtleties here:\n> -\t#\n> -\t# - we have to redirect descriptor 4 in addition to 2, to cover\n> -\t# BASH_XTRACEFD\n> -\t#\n> -\t# - the actual eval has to come before the redirection block (since\n> -\t# it needs to see descriptor 4 to set up its stderr)\n> -\t#\n> -\t# - likewise, any error message we print must be outside the block to\n> -\t# access descriptor 4\n> -\t#\n> -\t# - checking $? has to come immediately after the eval, but it must\n> -\t# be _inside_ the block to avoid polluting the \"set -x\" output\n> -\t#\n> -\n> -\ttest_eval_inner_ \"$@\" &3 2>&4\n> \t{\n> +\t\ttest_eval_inner_ \"$@\" &3 2>&4\n> \t\ttest_eval_ret_=$?\n> \t\tif want_trace\n> \t\tthen\n> \t\t\ttest 1 = $trace_level_ && set +x\n> \t\t\ttrace_level_=$(($trace_level_-1))\n> -\t\tfi\n> -\t} 2>/dev/null 4>&2\n> \n> -\tif test \"$test_eval_ret_\" != 0 && want_trace\n> -\tthen\n> -\t\tsay_color error >&4 \"error: last command exited with \\$?=$test_eval_ret_\"\n> -\tfi\n> +\t\t\tif test \"$test_eval_ret_\" != 0\n> +\t\t\tthen\n> +\t\t\t\tsay_color error >&4 \"error: last command exited with \\$?=$test_eval_ret_\"\n> +\t\t\tfi\n> +\t\tfi\n> +\t} 2>/dev/null\n> \treturn $test_eval_ret_\n> }\n> \n> -- \n> 2.34.1.932.g36842105b61\n> \n"},{"id":"443899","messageId":"211212.865yrtbvl1.gmgdl@evledraar.gmail.com","threadId":"56994","inReplyTo":"20211212163207.GA3400@szeder.dev","subject":"Re: [PATCH v3 2/2] test-lib.sh: remove the now-unused \"test_untraceable\" facility","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-12-12T17:06:31Z","receivedAt":"2021-12-12T17:07:42Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Sun, Dec 12 2021, SZEDER Gábor wrote:\n\n> On Fri, Dec 10, 2021 at 11:07:55AM +0100, Ævar Arnfjörð Bjarmason wrote:\n>> In the preceding commit the use of \"test_untraceable=UnfortunatelyYes\"\n>> was removed from \"t1510-repo-setup.sh\" in favor of more narrow\n>> redirections of the output of specific commands (and not entire\n>> sub-shells or functions).\n>> \n>> This is in line with the fixes in the series that introduced the\n>> \"test_untraceable\" facility. See 571e472dc43 (Merge branch\n>> 'sg/test-x', 2018-03-14) for the series as a whole, and\n>> e.g. 91538d0cde9 (t5570-git-daemon: don't check the stderr of a\n>> subshell, 2018-02-24) for a commit that's in line with the changes in\n>> the preceding commit.\n>> \n>> We've thus solved the TODO item noted when \"test_untraceable\" was\n>> added to \"t1510-repo-setup.sh\" in 58275069288 (t1510-repo-setup: mark\n>> as untraceable with '-x', 2018-02-24).\n>> \n>> So let's remove the feature entirely. Not only is it currently unused,\n>> but it actively encourages an anti-pattern in our tests. We should be\n>> testing the output of specific commands, not entire subshells or\n>> functions.\n>> \n>> That the \"-x\" output had to be disabled as a result is only one\n>> symptom, but even under bash those tests will be harder to debug as\n>> the subsequent check of the redirected file will be far removed from\n>> the command that emitted the output.\n>> \n>> Signed-off-by: Ævar Arnfjörð Bjarmason \n>> ---\n>> t/README | 3 ---\n>> t/test-lib.sh | 66 ++++++---------------------------------------------\n>> 2 files changed, 7 insertions(+), 62 deletions(-)\n>> \n>> diff --git a/t/README b/t/README\n>> index 29f72354bf1..3d30bbff34a 100644\n>> --- a/t/README\n>> +++ b/t/README\n>> @@ -86,9 +86,6 @@ appropriately before running \"make\". Short options can be bundled, i.e.\n>> -x::\n>> \tTurn on shell tracing (i.e., `set -x`) during the tests\n>> \tthemselves. Implies `--verbose`.\n>> -\tIgnored in test scripts that set the variable 'test_untraceable'\n>> -\tto a non-empty value, unless it's run with a Bash version\n>> -\tsupporting BASH_XTRACEFD, i.e. v4.1 or later.\n>> \n>> -d::\n>> --debug::\n>> diff --git a/t/test-lib.sh b/t/test-lib.sh\n>> index 57efcc5e97a..b008716917b 100644\n>> --- a/t/test-lib.sh\n>> +++ b/t/test-lib.sh\n>> @@ -381,29 +381,6 @@ then\n>> \texit\n>> fi\n>> \n>> -if test -n \"$trace\" && test -n \"$test_untraceable\"\n>> -then\n>> -\t# '-x' tracing requested, but this test script can't be reliably\n>> -\t# traced, unless it is run with a Bash version supporting\n>> -\t# BASH_XTRACEFD (introduced in Bash v4.1).\n>> -\t#\n>> -\t# Perform this version check _after_ the test script was\n>> -\t# potentially re-executed with $TEST_SHELL_PATH for '--tee' or\n>> -\t# '--verbose-log', so the right shell is checked and the\n>> -\t# warning is issued only once.\n>> -\tif test -n \"$BASH_VERSION\" && eval '\n>> -\t test ${BASH_VERSINFO[0]} -gt 4 || {\n>> -\t test ${BASH_VERSINFO[0]} -eq 4 &&\n>> -\t test ${BASH_VERSINFO[1]} -ge 1\n>> -\t }\n>> -\t '\n>> -\tthen\n>> -\t\t: Executed by a Bash version supporting BASH_XTRACEFD. Good.\n>> -\telse\n>> -\t\techo >&2 \"warning: ignoring -x; '$0' is untraceable without BASH_XTRACEFD\"\n>> -\t\ttrace=\n>> -\tfi\n>> -fi\n>> if test -n \"$trace\" && test -z \"$verbose_log\"\n>> then\n>> \tverbose=t\n>> @@ -650,19 +627,6 @@ else\n>> \texec 4>/dev/null 3>/dev/null\n>> fi\n>> \n>> -# Send any \"-x\" output directly to stderr to avoid polluting tests\n>> -# which capture stderr. We can do this unconditionally since it\n>> -# has no effect if tracing isn't turned on.\n>> -#\n>> -# Note that this sets up the trace fd as soon as we assign the variable, so it\n>> -# must come after the creation of descriptor 4 above. Likewise, we must never\n>> -# unset this, as it has the side effect of closing descriptor 4, which we\n>> -# use to show verbose tests to the user.\n>> -#\n>> -# Note also that we don't need or want to export it. The tracing is local to\n>> -# this shell, and we would not want to influence any shells we exec.\n>> -BASH_XTRACEFD=4\n>\n> Please do not remove BASH_XTRACEFD. And especially not like this,\n> without even mentioning it in the commit message.\n\nI can re-roll with an amended commit message that explicitly mentions\nit, but that doesn't address your \"please do not remove\",\n\nDo you see reason to keep it at the end-state fo this series? E.g. a\ncounter-argument to\nhttps://lore.kernel.org/git/211210.86pmq4daxm.gmgdl@evledraar.gmail.com/?\n"},{"id":"443932","messageId":"20211212201441.GB3400@szeder.dev","threadId":"56994","inReplyTo":"211212.865yrtbvl1.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH v3 2/2] test-lib.sh: remove the now-unused \"test_untraceable\" facility","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2021-12-12T20:14:41Z","receivedAt":"2021-12-12T20:15:51Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Sun, Dec 12, 2021 at 06:06:31PM +0100, Ævar Arnfjörð Bjarmason wrote:\n> \n> On Sun, Dec 12 2021, SZEDER Gábor wrote:\n> \n> > On Fri, Dec 10, 2021 at 11:07:55AM +0100, Ævar Arnfjörð Bjarmason wrote:\n> >> In the preceding commit the use of \"test_untraceable=UnfortunatelyYes\"\n> >> was removed from \"t1510-repo-setup.sh\" in favor of more narrow\n> >> redirections of the output of specific commands (and not entire\n> >> sub-shells or functions).\n> >> \n> >> This is in line with the fixes in the series that introduced the\n> >> \"test_untraceable\" facility. See 571e472dc43 (Merge branch\n> >> 'sg/test-x', 2018-03-14) for the series as a whole, and\n> >> e.g. 91538d0cde9 (t5570-git-daemon: don't check the stderr of a\n> >> subshell, 2018-02-24) for a commit that's in line with the changes in\n> >> the preceding commit.\n> >> \n> >> We've thus solved the TODO item noted when \"test_untraceable\" was\n> >> added to \"t1510-repo-setup.sh\" in 58275069288 (t1510-repo-setup: mark\n> >> as untraceable with '-x', 2018-02-24).\n> >> \n> >> So let's remove the feature entirely. Not only is it currently unused,\n> >> but it actively encourages an anti-pattern in our tests. We should be\n> >> testing the output of specific commands, not entire subshells or\n> >> functions.\n> >> \n> >> That the \"-x\" output had to be disabled as a result is only one\n> >> symptom, but even under bash those tests will be harder to debug as\n> >> the subsequent check of the redirected file will be far removed from\n> >> the command that emitted the output.\n> >> \n> >> Signed-off-by: Ævar Arnfjörð Bjarmason \n> >> ---\n> >> t/README | 3 ---\n> >> t/test-lib.sh | 66 ++++++---------------------------------------------\n> >> 2 files changed, 7 insertions(+), 62 deletions(-)\n> >> \n> >> diff --git a/t/README b/t/README\n> >> index 29f72354bf1..3d30bbff34a 100644\n> >> --- a/t/README\n> >> +++ b/t/README\n> >> @@ -86,9 +86,6 @@ appropriately before running \"make\". Short options can be bundled, i.e.\n> >> -x::\n> >> \tTurn on shell tracing (i.e., `set -x`) during the tests\n> >> \tthemselves. Implies `--verbose`.\n> >> -\tIgnored in test scripts that set the variable 'test_untraceable'\n> >> -\tto a non-empty value, unless it's run with a Bash version\n> >> -\tsupporting BASH_XTRACEFD, i.e. v4.1 or later.\n> >> \n> >> -d::\n> >> --debug::\n> >> diff --git a/t/test-lib.sh b/t/test-lib.sh\n> >> index 57efcc5e97a..b008716917b 100644\n> >> --- a/t/test-lib.sh\n> >> +++ b/t/test-lib.sh\n> >> @@ -381,29 +381,6 @@ then\n> >> \texit\n> >> fi\n> >> \n> >> -if test -n \"$trace\" && test -n \"$test_untraceable\"\n> >> -then\n> >> -\t# '-x' tracing requested, but this test script can't be reliably\n> >> -\t# traced, unless it is run with a Bash version supporting\n> >> -\t# BASH_XTRACEFD (introduced in Bash v4.1).\n> >> -\t#\n> >> -\t# Perform this version check _after_ the test script was\n> >> -\t# potentially re-executed with $TEST_SHELL_PATH for '--tee' or\n> >> -\t# '--verbose-log', so the right shell is checked and the\n> >> -\t# warning is issued only once.\n> >> -\tif test -n \"$BASH_VERSION\" && eval '\n> >> -\t test ${BASH_VERSINFO[0]} -gt 4 || {\n> >> -\t test ${BASH_VERSINFO[0]} -eq 4 &&\n> >> -\t test ${BASH_VERSINFO[1]} -ge 1\n> >> -\t }\n> >> -\t '\n> >> -\tthen\n> >> -\t\t: Executed by a Bash version supporting BASH_XTRACEFD. Good.\n> >> -\telse\n> >> -\t\techo >&2 \"warning: ignoring -x; '$0' is untraceable without BASH_XTRACEFD\"\n> >> -\t\ttrace=\n> >> -\tfi\n> >> -fi\n> >> if test -n \"$trace\" && test -z \"$verbose_log\"\n> >> then\n> >> \tverbose=t\n> >> @@ -650,19 +627,6 @@ else\n> >> \texec 4>/dev/null 3>/dev/null\n> >> fi\n> >> \n> >> -# Send any \"-x\" output directly to stderr to avoid polluting tests\n> >> -# which capture stderr. We can do this unconditionally since it\n> >> -# has no effect if tracing isn't turned on.\n> >> -#\n> >> -# Note that this sets up the trace fd as soon as we assign the variable, so it\n> >> -# must come after the creation of descriptor 4 above. Likewise, we must never\n> >> -# unset this, as it has the side effect of closing descriptor 4, which we\n> >> -# use to show verbose tests to the user.\n> >> -#\n> >> -# Note also that we don't need or want to export it. The tracing is local to\n> >> -# this shell, and we would not want to influence any shells we exec.\n> >> -BASH_XTRACEFD=4\n> >\n> > Please do not remove BASH_XTRACEFD. And especially not like this,\n> > without even mentioning it in the commit message.\n> \n> I can re-roll with an amended commit message that explicitly mentions\n> it\n\nIt should rather be a separate patch...\n\n>, but that doesn't address your \"please do not remove\",\n> \n> Do you see reason to keep it at the end-state fo this series? E.g. a\n> counter-argument to\n> https://lore.kernel.org/git/211210.86pmq4daxm.gmgdl@evledraar.gmail.com/?\n\nI don't see any argument pertinent to BASH_XTRACEFD in general in that\nemail, of in favor of its removal in particular, and there are no\nvalid arguments for its removal in earlier emails in this thread\neither.\n\n"},{"id":"443940","messageId":"cover-v4-0.3-00000000000-20211213T013559Z-avarab@gmail.com","threadId":"56994","inReplyTo":"cover-v3-0.2-00000000000-20211210T100512Z-avarab@gmail.com","subject":"[PATCH v4 0/3] test-lib.sh: have all tests pass under \"-x\", remove BASH_XTRACEFD","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-12-13T01:38:33Z","receivedAt":"2021-12-13T01:38:48Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"A re-arrangement-only change to v3[1]. The previous 2/2 is now split\ninto two commits, as requested by SZEDER[2] in the removal of\nBASH_XTRACEFD is now its own commit & the rationale for doing so is\noutlined in detail.\n\n1. https://lore.kernel.org/git/cover-v3-0.2-00000000000-20211210T100512Z-avarab@gmail.com/\n2. https://lore.kernel.org/git/20211212201441.GB3400@szeder.dev/\n\nÆvar Arnfjörð Bjarmason (3):\n t1510: remove need for \"test_untraceable\", retain coverage\n test-lib.sh: remove the now-unused \"test_untraceable\" facility\n test-lib.sh: remove \"BASH_XTRACEFD\"\n\n t/README | 3 --\n t/t1510-repo-setup.sh | 85 +++++++++++++++++++++----------------------\n t/test-lib.sh | 66 ++++-----------------------------\n 3 files changed, 49 insertions(+), 105 deletions(-)\n\nRange-diff against v3:\n1: 7876202c5b0 = 1: 9e7b089dc50 t1510: remove need for \"test_untraceable\", retain coverage\n-: ----------- > 2: 60883fd95cb test-lib.sh: remove the now-unused \"test_untraceable\" facility\n2: a7fc794e20d ! 3: 8b5ae33376e test-lib.sh: remove the now-unused \"test_untraceable\" facility\n @@ Metadata\n Author: Ævar Arnfjörð Bjarmason \n \n ## Commit message ##\n - test-lib.sh: remove the now-unused \"test_untraceable\" facility\n + test-lib.sh: remove \"BASH_XTRACEFD\"\n \n - In the preceding commit the use of \"test_untraceable=UnfortunatelyYes\"\n - was removed from \"t1510-repo-setup.sh\" in favor of more narrow\n - redirections of the output of specific commands (and not entire\n - sub-shells or functions).\n + Stop setting \"BASH_XTRACEFD=4\" to direct \"-x\" output to file\n + descriptor 4 under bash.\n \n - This is in line with the fixes in the series that introduced the\n - \"test_untraceable\" facility. See 571e472dc43 (Merge branch\n - 'sg/test-x', 2018-03-14) for the series as a whole, and\n - e.g. 91538d0cde9 (t5570-git-daemon: don't check the stderr of a\n - subshell, 2018-02-24) for a commit that's in line with the changes in\n - the preceding commit.\n + When it was added in d88785e424a (test-lib: set BASH_XTRACEFD\n + automatically, 2016-05-11) it was needed as a workaround for various\n + tests that didn't pass cleanly under \"-x\".\n \n - We've thus solved the TODO item noted when \"test_untraceable\" was\n - added to \"t1510-repo-setup.sh\" in 58275069288 (t1510-repo-setup: mark\n - as untraceable with '-x', 2018-02-24).\n + Most of those were later fixed in 71e472dc43 (Merge branch\n + 'sg/test-x', 2018-03-14), and in the preceding commits we've fixed the\n + final remaining and removed the \"test_untraceable\" facility.\n \n - So let's remove the feature entirely. Not only is it currently unused,\n - but it actively encourages an anti-pattern in our tests. We should be\n - testing the output of specific commands, not entire subshells or\n - functions.\n + The reason we don't need this anymore is becomes clear from reading\n + the rationale in d88785e424a and applying those arguments to the\n + current state of the codebase. In particular it said (with \"this\" and\n + \"it\" referring to the problem of tests failing under \"-x\"):\n \n - That the \"-x\" output had to be disabled as a result is only one\n - symptom, but even under bash those tests will be harder to debug as\n - the subsequent check of the redirected file will be far removed from\n - the command that emitted the output.\n + \"there here isn't a portable or scalable solution to this [...] we\n + can work around it by pointing the \"set -x\" output to our\n + descriptor 4\"\n \n - Signed-off-by: Ævar Arnfjörð Bjarmason \n + And finally that:\n \n - ## t/README ##\n -@@ t/README: appropriately before running \"make\". Short options can be bundled, i.e.\n - -x::\n - \tTurn on shell tracing (i.e., `set -x`) during the tests\n - \tthemselves. Implies `--verbose`.\n --\tIgnored in test scripts that set the variable 'test_untraceable'\n --\tto a non-empty value, unless it's run with a Bash version\n --\tsupporting BASH_XTRACEFD, i.e. v4.1 or later.\n - \n - -d::\n - --debug::\n + \"Automatic tests for our \"-x\" option may be a bit too meta\"\n +\n + Those tests are exactly what we've had since aedffe95250 (travis-ci:\n + run tests with '-x' tracing, 2018-02-24), so punting on fixing issues\n + with \"-x\" by using \"BASH_XTRACEFD=4\" isn't needed anymore, we're now\n + committing to maintaining the test suite in a way that won't break\n + under \"-x\".\n +\n + We could retain \"BASH_XTRACEFD=4\" anyway, but doing so is bad because:\n +\n + 1) Since we're caring about \"-x\" passing in CI under \"dash\" on Ubuntu\n + using \"BASH_XTRACEFD=4\" will amount to hiding an error we'll run\n + into eventually. Tests will pass locally with \"bash\", but fail in\n + CI with \"dash\" (or under other non-\"bash\" shells).\n +\n + 2) As the amended code in \"test_eval_\" shows (an amended revert to\n + the pre-image of d88785e424a) it's simpler to not have to take\n + this \"bash\" special-case into account.\n +\n + Signed-off-by: Ævar Arnfjörð Bjarmason \n \n ## t/test-lib.sh ##\n -@@ t/test-lib.sh: then\n - \texit\n - fi\n - \n --if test -n \"$trace\" && test -n \"$test_untraceable\"\n --then\n --\t# '-x' tracing requested, but this test script can't be reliably\n --\t# traced, unless it is run with a Bash version supporting\n --\t# BASH_XTRACEFD (introduced in Bash v4.1).\n --\t#\n --\t# Perform this version check _after_ the test script was\n --\t# potentially re-executed with $TEST_SHELL_PATH for '--tee' or\n --\t# '--verbose-log', so the right shell is checked and the\n --\t# warning is issued only once.\n --\tif test -n \"$BASH_VERSION\" && eval '\n --\t test ${BASH_VERSINFO[0]} -gt 4 || {\n --\t test ${BASH_VERSINFO[0]} -eq 4 &&\n --\t test ${BASH_VERSINFO[1]} -ge 1\n --\t }\n --\t '\n --\tthen\n --\t\t: Executed by a Bash version supporting BASH_XTRACEFD. Good.\n --\telse\n --\t\techo >&2 \"warning: ignoring -x; '$0' is untraceable without BASH_XTRACEFD\"\n --\t\ttrace=\n --\tfi\n --fi\n - if test -n \"$trace\" && test -z \"$verbose_log\"\n - then\n - \tverbose=t\n @@ t/test-lib.sh: else\n \texec 4>/dev/null 3>/dev/null\n fi\n-- \n2.34.1.1024.g573f2f4b767\n\n"},{"id":"443942","messageId":"patch-v4-1.3-9e7b089dc50-20211213T013559Z-avarab@gmail.com","threadId":"56994","inReplyTo":"cover-v4-0.3-00000000000-20211213T013559Z-avarab@gmail.com","subject":"[PATCH v4 1/3] t1510: remove need for \"test_untraceable\", retain coverage","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-12-13T01:38:34Z","receivedAt":"2021-12-13T01:38:48Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Amend the tests checking whether stderr is empty added in\n4868b2ea17b (Subject: setup: officially support --work-tree without\n--git-dir, 2011-01-19) work portably on all POSIX shells, instead\nsuppressing the trace output with \"test_untraceable\" on shells that\naren't bash.\n\nThe tests that used the \"try_repo\" helper wanted to check whether git\ncommands invoked therein would emit anything on stderr. To do this\nthey invoked the function and redirected the stderr to a \"message\"\nfile.\n\nIn 58275069288 (t1510-repo-setup: mark as untraceable with '-x',\n2018-02-24) these were made to use \"test_untraceable\" introduced in\n5fc98e79fc0 (t: add means to disable '-x' tracing for individual test\nscripts, 2018-02-24).\n\nIt is better to have the \"try_repo\" function itself start with a\n\"test_when_finished 'rm stderr'\", and then redirect the stderr output\nfrom git commands it invokes via its helpers to a \"stderr\" file.\n\nThis means that if we have a failure it'll be closer to the source of\nthe problem, and most importantly isn't incompatible with \"-x\" on\nshells that aren't \"bash\".\n\nWe also need to split those tests that had two \"try_repo\" invocations\ninto different tests, which'll further help to narrow down any\npotential failures. This wasn't strictly necessary (an artifact of the\nuse of \"test_when_finished\"), but the pattern enforces better test\nhygiene.\n\nThe functions it calls might change directories, so we pass an\nabsolute \"$stderr_log_path\" around. We also need to change a \"$(pwd)\"\nto \"$PWD\" in test_repo(), on e.g. \"dash\" that interpolation will be\ntraced and part of the \"2>>\" redirection.\n\n1. https://lore.kernel.org/git/YbMiK1wHzBfYvK2a@coredump.intra.peff.net/\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n t/t1510-repo-setup.sh | 85 +++++++++++++++++++++----------------------\n 1 file changed, 42 insertions(+), 43 deletions(-)\n\ndiff --git a/t/t1510-repo-setup.sh b/t/t1510-repo-setup.sh\nindex 591505a39c0..fc94d8b49be 100755\n--- a/t/t1510-repo-setup.sh\n+++ b/t/t1510-repo-setup.sh\n@@ -40,9 +40,6 @@ A few rules for repo setup:\n prefix is NULL.\n \"\n \n-# This test heavily relies on the standard error of nested function calls.\n-test_untraceable=UnfortunatelyYes\n-\n TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n \n@@ -62,7 +59,7 @@ test_repo () {\n \t\t\texport GIT_WORK_TREE\n \t\tfi &&\n \t\trm -f trace &&\n-\t\tGIT_TRACE_SETUP=\"$(pwd)/trace\" git symbolic-ref HEAD >/dev/null &&\n+\t\tGIT_TRACE_SETUP=\"$PWD/trace\" git symbolic-ref HEAD >/dev/null 2>>\"$stderr_log_path\" &&\n \t\tgrep '^setup: ' trace >result &&\n \t\ttest_cmp expected result\n \t)\n@@ -72,7 +69,7 @@ maybe_config () {\n \tfile=$1 var=$2 value=$3 &&\n \tif test \"$value\" != unset\n \tthen\n-\t\tgit config --file=\"$file\" \"$var\" \"$value\"\n+\t\tgit config --file=\"$file\" \"$var\" \"$value\" 2>>\"$stderr_log_path\"\n \tfi\n }\n \n@@ -80,7 +77,7 @@ setup_repo () {\n \tname=$1 worktreecfg=$2 gitfile=$3 barecfg=$4 &&\n \tsane_unset GIT_DIR GIT_WORK_TREE &&\n \n-\tgit -c init.defaultBranch=initial init \"$name\" &&\n+\tgit -c init.defaultBranch=initial init \"$name\" 2>>\"$stderr_log_path\" &&\n \tmaybe_config \"$name/.git/config\" core.worktree \"$worktreecfg\" &&\n \tmaybe_config \"$name/.git/config\" core.bare \"$barecfg\" &&\n \tmkdir -p \"$name/sub/sub\" &&\n@@ -210,10 +207,14 @@ run_wt_tests () {\n #\t(git dir) (work tree) (cwd) (prefix)\t<-- from subdir\n try_repo () {\n \tname=$1 worktreeenv=$2 gitdirenv=$3 &&\n+\tstderr_log_path=\"$PWD/stderr\" &&\n+\n+\ttest_when_finished \"rm \\\"$stderr_log_path\\\"\" &&\n \tsetup_repo \"$name\" \"$4\" \"$5\" \"$6\" &&\n \tshift 6 &&\n \ttry_case \"$name\" \"$worktreeenv\" \"$gitdirenv\" \\\n \t\t\"$1\" \"$2\" \"$3\" \"$4\" &&\n+\ttest_must_be_empty \"$stderr_log_path\" &&\n \tshift 4 &&\n \tcase \"$gitdirenv\" in\n \t/* | ?:/* | unset) ;;\n@@ -221,7 +222,8 @@ try_repo () {\n \t\tgitdirenv=../$gitdirenv ;;\n \tesac &&\n \ttry_case \"$name/sub\" \"$worktreeenv\" \"$gitdirenv\" \\\n-\t\t\"$1\" \"$2\" \"$3\" \"$4\"\n+\t\t\"$1\" \"$2\" \"$3\" \"$4\" &&\n+\ttest_must_be_empty \"$stderr_log_path\"\n }\n \n # Bit 0 = GIT_WORK_TREE\n@@ -234,15 +236,13 @@ try_repo () {\n test_expect_success '#0: nonbare repo, no explicit configuration' '\n \ttry_repo 0 unset unset unset \"\" unset \\\n \t\t.git \"$here/0\" \"$here/0\" \"(null)\" \\\n-\t\t.git \"$here/0\" \"$here/0\" sub/ 2>message &&\n-\ttest_must_be_empty message\n+\t\t.git \"$here/0\" \"$here/0\" sub/\n '\n \n test_expect_success '#1: GIT_WORK_TREE without explicit GIT_DIR is accepted' '\n \ttry_repo 1 \"$here\" unset unset \"\" unset \\\n \t\t\"$here/1/.git\" \"$here\" \"$here\" 1/ \\\n-\t\t\"$here/1/.git\" \"$here\" \"$here\" 1/sub/ 2>message &&\n-\ttest_must_be_empty message\n+\t\t\"$here/1/.git\" \"$here\" \"$here\" 1/sub/\n '\n \n test_expect_success '#2: worktree defaults to cwd with explicit GIT_DIR' '\n@@ -268,19 +268,20 @@ test_expect_success '#4: core.worktree without GIT_DIR set is accepted' '\n \tmkdir -p 4/sub sub &&\n \ttry_case 4 unset unset \\\n \t\t.git \"$here/4/sub\" \"$here/4\" \"(null)\" \\\n-\t\t\"$here/4/.git\" \"$here/4/sub\" \"$here/4/sub\" \"(null)\" 2>message &&\n-\ttest_must_be_empty message\n+\t\t\"$here/4/.git\" \"$here/4/sub\" \"$here/4/sub\" \"(null)\"\n '\n \n-test_expect_success '#5: core.worktree + GIT_WORK_TREE is accepted' '\n+test_expect_success '#5.1: core.worktree + GIT_WORK_TREE is accepted' '\n \t# or: you cannot intimidate away the lack of GIT_DIR setting\n \ttry_repo 5 \"$here\" unset \"$here/5\" \"\" unset \\\n \t\t\"$here/5/.git\" \"$here\" \"$here\" 5/ \\\n-\t\t\"$here/5/.git\" \"$here\" \"$here\" 5/sub/ 2>message &&\n+\t\t\"$here/5/.git\" \"$here\" \"$here\" 5/sub/\n+'\n+\n+test_expect_success '#5.2: core.worktree + GIT_WORK_TREE is accepted' '\n \ttry_repo 5a .. unset \"$here/5a\" \"\" unset \\\n \t\t\"$here/5a/.git\" \"$here\" \"$here\" 5a/ \\\n-\t\t\"$here/5a/.git\" \"$here/5a\" \"$here/5a\" sub/ &&\n-\ttest_must_be_empty message\n+\t\t\"$here/5a/.git\" \"$here/5a\" \"$here/5a\" sub/\n '\n \n test_expect_success '#6: setting GIT_DIR brings core.worktree to life' '\n@@ -376,8 +377,7 @@ test_expect_success '#9: GIT_WORK_TREE accepted with gitfile' '\n \tmkdir -p 9/wt &&\n \ttry_repo 9 wt unset unset gitfile unset \\\n \t\t\"$here/9.git\" \"$here/9/wt\" \"$here/9\" \"(null)\" \\\n-\t\t\"$here/9.git\" \"$here/9/sub/wt\" \"$here/9/sub\" \"(null)\" 2>message &&\n-\ttest_must_be_empty message\n+\t\t\"$here/9.git\" \"$here/9/sub/wt\" \"$here/9/sub\" \"(null)\"\n '\n \n test_expect_success '#10: GIT_DIR can point to gitfile' '\n@@ -402,16 +402,14 @@ run_wt_tests 11 gitfile\n test_expect_success '#12: core.worktree with gitfile is accepted' '\n \ttry_repo 12 unset unset \"$here/12\" gitfile unset \\\n \t\t\"$here/12.git\" \"$here/12\" \"$here/12\" \"(null)\" \\\n-\t\t\"$here/12.git\" \"$here/12\" \"$here/12\" sub/ 2>message &&\n-\ttest_must_be_empty message\n+\t\t\"$here/12.git\" \"$here/12\" \"$here/12\" sub/\n '\n \n test_expect_success '#13: core.worktree+GIT_WORK_TREE accepted (with gitfile)' '\n \t# or: you cannot intimidate away the lack of GIT_DIR setting\n \ttry_repo 13 non-existent-too unset non-existent gitfile unset \\\n \t\t\"$here/13.git\" \"$here/13/non-existent-too\" \"$here/13\" \"(null)\" \\\n-\t\t\"$here/13.git\" \"$here/13/sub/non-existent-too\" \"$here/13/sub\" \"(null)\" 2>message &&\n-\ttest_must_be_empty message\n+\t\t\"$here/13.git\" \"$here/13/sub/non-existent-too\" \"$here/13/sub\" \"(null)\"\n '\n \n # case #14.\n@@ -549,8 +547,7 @@ test_expect_success '#17: GIT_WORK_TREE without explicit GIT_DIR is accepted (ba\n \tmkdir -p 17b/.git/wt/sub &&\n \n \ttry_case 17a/.git \"$here/17a\" unset \\\n-\t\t\"$here/17a/.git\" \"$here/17a\" \"$here/17a\" .git/ \\\n-\t\t2>message &&\n+\t\t\"$here/17a/.git\" \"$here/17a\" \"$here/17a\" .git/ &&\n \ttry_case 17a/.git/wt \"$here/17a\" unset \\\n \t\t\"$here/17a/.git\" \"$here/17a\" \"$here/17a\" .git/wt/ &&\n \ttry_case 17a/.git/wt/sub \"$here/17a\" unset \\\n@@ -565,14 +562,16 @@ test_expect_success '#17: GIT_WORK_TREE without explicit GIT_DIR is accepted (ba\n \n \ttry_repo 17c \"$here/17c\" unset unset \"\" true \\\n \t\t.git \"$here/17c\" \"$here/17c\" \"(null)\" \\\n-\t\t\"$here/17c/.git\" \"$here/17c\" \"$here/17c\" sub/ 2>message &&\n-\ttest_must_be_empty message\n+\t\t\"$here/17c/.git\" \"$here/17c\" \"$here/17c\" sub/\n '\n \n-test_expect_success '#18: bare .git named by GIT_DIR has no worktree' '\n+test_expect_success '#18.1: bare .git named by GIT_DIR has no worktree' '\n \ttry_repo 18 unset .git unset \"\" true \\\n \t\t.git \"(null)\" \"$here/18\" \"(null)\" \\\n-\t\t../.git \"(null)\" \"$here/18/sub\" \"(null)\" &&\n+\t\t../.git \"(null)\" \"$here/18/sub\" \"(null)\"\n+'\n+\n+test_expect_success '#18.2: bare .git named by GIT_DIR has no worktree' '\n \ttry_repo 18b unset \"$here/18b/.git\" unset \"\" true \\\n \t\t\"$here/18b/.git\" \"(null)\" \"$here/18b\" \"(null)\" \\\n \t\t\"$here/18b/.git\" \"(null)\" \"$here/18b/sub\" \"(null)\"\n@@ -590,12 +589,11 @@ test_expect_success '#20a: core.worktree without GIT_DIR accepted (inside .git)'\n \tsetup_repo 20a \"$here/20a\" \"\" unset &&\n \tmkdir -p 20a/.git/wt/sub &&\n \ttry_case 20a/.git unset unset \\\n-\t\t\"$here/20a/.git\" \"$here/20a\" \"$here/20a\" .git/ 2>message &&\n+\t\t\"$here/20a/.git\" \"$here/20a\" \"$here/20a\" .git/ &&\n \ttry_case 20a/.git/wt unset unset \\\n \t\t\"$here/20a/.git\" \"$here/20a\" \"$here/20a\" .git/wt/ &&\n \ttry_case 20a/.git/wt/sub unset unset \\\n-\t\t\"$here/20a/.git\" \"$here/20a\" \"$here/20a\" .git/wt/sub/ &&\n-\ttest_must_be_empty message\n+\t\t\"$here/20a/.git\" \"$here/20a\" \"$here/20a\" .git/wt/sub/\n '\n \n test_expect_success '#20b/c: core.worktree and core.bare conflict' '\n@@ -625,10 +623,9 @@ test_expect_success '#21: setup, core.worktree warns before overriding core.bare\n \t\tcd 21/.git &&\n \t\tGIT_WORK_TREE=\"$here/21\" &&\n \t\texport GIT_WORK_TREE &&\n-\t\tgit status >/dev/null\n-\t) 2>message &&\n-\ttest_must_be_empty message\n-\n+\t\tgit status 2>message &&\n+\t\ttest_must_be_empty message\n+\t)\n '\n run_wt_tests 21\n \n@@ -742,14 +739,16 @@ test_expect_success '#24: bare repo has no worktree (gitfile case)' '\n test_expect_success '#25: GIT_WORK_TREE accepted if GIT_DIR unset (bare gitfile case)' '\n \ttry_repo 25 \"$here/25\" unset unset gitfile true \\\n \t\t\"$here/25.git\" \"$here/25\" \"$here/25\" \"(null)\" \\\n-\t\t\"$here/25.git\" \"$here/25\" \"$here/25\" \"sub/\" 2>message &&\n-\ttest_must_be_empty message\n+\t\t\"$here/25.git\" \"$here/25\" \"$here/25\" \"sub/\"\n '\n \n-test_expect_success '#26: bare repo has no worktree (GIT_DIR -> gitfile case)' '\n+test_expect_success '#26.1: bare repo has no worktree (GIT_DIR -> gitfile case)' '\n \ttry_repo 26 unset \"$here/26/.git\" unset gitfile true \\\n \t\t\"$here/26.git\" \"(null)\" \"$here/26\" \"(null)\" \\\n-\t\t\"$here/26.git\" \"(null)\" \"$here/26/sub\" \"(null)\" &&\n+\t\t\"$here/26.git\" \"(null)\" \"$here/26/sub\" \"(null)\"\n+'\n+\n+test_expect_success '#26.2: bare repo has no worktree (GIT_DIR -> gitfile case)' '\n \ttry_repo 26b unset .git unset gitfile true \\\n \t\t\"$here/26b.git\" \"(null)\" \"$here/26b\" \"(null)\" \\\n \t\t\"$here/26b.git\" \"(null)\" \"$here/26b/sub\" \"(null)\"\n@@ -779,9 +778,9 @@ test_expect_success '#29: setup' '\n \t\tcd 29 &&\n \t\tGIT_WORK_TREE=\"$here/29\" &&\n \t\texport GIT_WORK_TREE &&\n-\t\tgit status\n-\t) 2>message &&\n-\ttest_must_be_empty message\n+\t\tgit status 2>message &&\n+\t\ttest_must_be_empty message\n+\t)\n '\n run_wt_tests 29 gitfile\n \n-- \n2.34.1.1024.g573f2f4b767\n\n"},{"id":"443941","messageId":"patch-v4-2.3-60883fd95cb-20211213T013559Z-avarab@gmail.com","threadId":"56994","inReplyTo":"cover-v4-0.3-00000000000-20211213T013559Z-avarab@gmail.com","subject":"[PATCH v4 2/3] test-lib.sh: remove the now-unused \"test_untraceable\" facility","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-12-13T01:38:35Z","receivedAt":"2021-12-13T01:38:49Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"In the preceding commit the use of \"test_untraceable=UnfortunatelyYes\"\nwas removed from \"t1510-repo-setup.sh\" in favor of more narrow\nredirections of the output of specific commands (and not entire\nsub-shells or functions).\n\nThis is in line with the fixes in the series that introduced the\n\"test_untraceable\" facility. See 571e472dc43 (Merge branch\n'sg/test-x', 2018-03-14) for the series as a whole, and\ne.g. 91538d0cde9 (t5570-git-daemon: don't check the stderr of a\nsubshell, 2018-02-24) for a commit that's in line with the changes in\nthe preceding commit.\n\nWe've thus solved the TODO item noted when \"test_untraceable\" was\nadded to \"t1510-repo-setup.sh\" in 58275069288 (t1510-repo-setup: mark\nas untraceable with '-x', 2018-02-24).\n\nSo let's remove the feature entirely. Not only is it currently unused,\nbut it actively encourages an anti-pattern in our tests. We should be\ntesting the output of specific commands, not entire subshells or\nfunctions.\n\nThat the \"-x\" output had to be disabled as a result is only one\nsymptom, but even under bash those tests will be harder to debug as\nthe subsequent check of the redirected file will be far removed from\nthe command that emitted the output.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n t/README | 3 ---\n t/test-lib.sh | 23 -----------------------\n 2 files changed, 26 deletions(-)\n\ndiff --git a/t/README b/t/README\nindex 29f72354bf1..3d30bbff34a 100644\n--- a/t/README\n+++ b/t/README\n@@ -86,9 +86,6 @@ appropriately before running \"make\". Short options can be bundled, i.e.\n -x::\n \tTurn on shell tracing (i.e., `set -x`) during the tests\n \tthemselves. Implies `--verbose`.\n-\tIgnored in test scripts that set the variable 'test_untraceable'\n-\tto a non-empty value, unless it's run with a Bash version\n-\tsupporting BASH_XTRACEFD, i.e. v4.1 or later.\n \n -d::\n --debug::\ndiff --git a/t/test-lib.sh b/t/test-lib.sh\nindex 57efcc5e97a..ddfa0567e9e 100644\n--- a/t/test-lib.sh\n+++ b/t/test-lib.sh\n@@ -381,29 +381,6 @@ then\n \texit\n fi\n \n-if test -n \"$trace\" && test -n \"$test_untraceable\"\n-then\n-\t# '-x' tracing requested, but this test script can't be reliably\n-\t# traced, unless it is run with a Bash version supporting\n-\t# BASH_XTRACEFD (introduced in Bash v4.1).\n-\t#\n-\t# Perform this version check _after_ the test script was\n-\t# potentially re-executed with $TEST_SHELL_PATH for '--tee' or\n-\t# '--verbose-log', so the right shell is checked and the\n-\t# warning is issued only once.\n-\tif test -n \"$BASH_VERSION\" && eval '\n-\t test ${BASH_VERSINFO[0]} -gt 4 || {\n-\t test ${BASH_VERSINFO[0]} -eq 4 &&\n-\t test ${BASH_VERSINFO[1]} -ge 1\n-\t }\n-\t '\n-\tthen\n-\t\t: Executed by a Bash version supporting BASH_XTRACEFD. Good.\n-\telse\n-\t\techo >&2 \"warning: ignoring -x; '$0' is untraceable without BASH_XTRACEFD\"\n-\t\ttrace=\n-\tfi\n-fi\n if test -n \"$trace\" && test -z \"$verbose_log\"\n then\n \tverbose=t\n-- \n2.34.1.1024.g573f2f4b767\n\n"},{"id":"443943","messageId":"patch-v4-3.3-8b5ae33376e-20211213T013559Z-avarab@gmail.com","threadId":"56994","inReplyTo":"cover-v4-0.3-00000000000-20211213T013559Z-avarab@gmail.com","subject":"[PATCH v4 3/3] test-lib.sh: remove \"BASH_XTRACEFD\"","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-12-13T01:38:36Z","receivedAt":"2021-12-13T01:38:50Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Stop setting \"BASH_XTRACEFD=4\" to direct \"-x\" output to file\ndescriptor 4 under bash.\n\nWhen it was added in d88785e424a (test-lib: set BASH_XTRACEFD\nautomatically, 2016-05-11) it was needed as a workaround for various\ntests that didn't pass cleanly under \"-x\".\n\nMost of those were later fixed in 71e472dc43 (Merge branch\n'sg/test-x', 2018-03-14), and in the preceding commits we've fixed the\nfinal remaining and removed the \"test_untraceable\" facility.\n\nThe reason we don't need this anymore is becomes clear from reading\nthe rationale in d88785e424a and applying those arguments to the\ncurrent state of the codebase. In particular it said (with \"this\" and\n\"it\" referring to the problem of tests failing under \"-x\"):\n\n \"there here isn't a portable or scalable solution to this [...] we\n can work around it by pointing the \"set -x\" output to our\n descriptor 4\"\n\nAnd finally that:\n\n \"Automatic tests for our \"-x\" option may be a bit too meta\"\n\nThose tests are exactly what we've had since aedffe95250 (travis-ci:\nrun tests with '-x' tracing, 2018-02-24), so punting on fixing issues\nwith \"-x\" by using \"BASH_XTRACEFD=4\" isn't needed anymore, we're now\ncommitting to maintaining the test suite in a way that won't break\nunder \"-x\".\n\nWe could retain \"BASH_XTRACEFD=4\" anyway, but doing so is bad because:\n\n 1) Since we're caring about \"-x\" passing in CI under \"dash\" on Ubuntu\n using \"BASH_XTRACEFD=4\" will amount to hiding an error we'll run\n into eventually. Tests will pass locally with \"bash\", but fail in\n CI with \"dash\" (or under other non-\"bash\" shells).\n\n 2) As the amended code in \"test_eval_\" shows (an amended revert to\n the pre-image of d88785e424a) it's simpler to not have to take\n this \"bash\" special-case into account.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n t/test-lib.sh | 43 +++++++------------------------------------\n 1 file changed, 7 insertions(+), 36 deletions(-)\n\ndiff --git a/t/test-lib.sh b/t/test-lib.sh\nindex ddfa0567e9e..b008716917b 100644\n--- a/t/test-lib.sh\n+++ b/t/test-lib.sh\n@@ -627,19 +627,6 @@ else\n \texec 4>/dev/null 3>/dev/null\n fi\n \n-# Send any \"-x\" output directly to stderr to avoid polluting tests\n-# which capture stderr. We can do this unconditionally since it\n-# has no effect if tracing isn't turned on.\n-#\n-# Note that this sets up the trace fd as soon as we assign the variable, so it\n-# must come after the creation of descriptor 4 above. Likewise, we must never\n-# unset this, as it has the side effect of closing descriptor 4, which we\n-# use to show verbose tests to the user.\n-#\n-# Note also that we don't need or want to export it. The tracing is local to\n-# this shell, and we would not want to influence any shells we exec.\n-BASH_XTRACEFD=4\n-\n test_failure=0\n test_count=0\n test_fixed=0\n@@ -926,36 +913,20 @@ test_eval_ () {\n \t# the shell from printing the \"set +x\" to turn it off (nor the saving\n \t# of $? before that). But we can make sure that the output goes to\n \t# /dev/null.\n-\t#\n-\t# There are a few subtleties here:\n-\t#\n-\t# - we have to redirect descriptor 4 in addition to 2, to cover\n-\t# BASH_XTRACEFD\n-\t#\n-\t# - the actual eval has to come before the redirection block (since\n-\t# it needs to see descriptor 4 to set up its stderr)\n-\t#\n-\t# - likewise, any error message we print must be outside the block to\n-\t# access descriptor 4\n-\t#\n-\t# - checking $? has to come immediately after the eval, but it must\n-\t# be _inside_ the block to avoid polluting the \"set -x\" output\n-\t#\n-\n-\ttest_eval_inner_ \"$@\" &3 2>&4\n \t{\n+\t\ttest_eval_inner_ \"$@\" &3 2>&4\n \t\ttest_eval_ret_=$?\n \t\tif want_trace\n \t\tthen\n \t\t\ttest 1 = $trace_level_ && set +x\n \t\t\ttrace_level_=$(($trace_level_-1))\n-\t\tfi\n-\t} 2>/dev/null 4>&2\n \n-\tif test \"$test_eval_ret_\" != 0 && want_trace\n-\tthen\n-\t\tsay_color error >&4 \"error: last command exited with \\$?=$test_eval_ret_\"\n-\tfi\n+\t\t\tif test \"$test_eval_ret_\" != 0\n+\t\t\tthen\n+\t\t\t\tsay_color error >&4 \"error: last command exited with \\$?=$test_eval_ret_\"\n+\t\t\tfi\n+\t\tfi\n+\t} 2>/dev/null\n \treturn $test_eval_ret_\n }\n \n-- \n2.34.1.1024.g573f2f4b767\n\n"},{"id":"443945","messageId":"20211213054353.GC3400@szeder.dev","threadId":"56994","inReplyTo":"cover-v4-0.3-00000000000-20211213T013559Z-avarab@gmail.com","subject":"Re: [PATCH v4 0/3] test-lib.sh: have all tests pass under \"-x\", remove BASH_XTRACEFD","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2021-12-13T05:43:53Z","receivedAt":"2021-12-13T05:43:58Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Mon, Dec 13, 2021 at 02:38:33AM +0100, Ævar Arnfjörð Bjarmason wrote:\n> A re-arrangement-only change to v3[1]. The previous 2/2 is now split\n> into two commits, as requested by SZEDER[2] in the removal of\n> BASH_XTRACEFD is now its own commit & the rationale for doing so is\n> outlined in detail.\n\nI'm afraid I wasn't clear. What I meant was that if we were to remove\nBASH_XTRACEFD, then it should be done its own commit.\n\nBut again: BASH_XTRACEFD is the only simple yet reliable and robust\nway to get -x trace from our tests, so do not remove it.\n\n> 1. https://lore.kernel.org/git/cover-v3-0.2-00000000000-20211210T100512Z-avarab@gmail.com/\n> 2. https://lore.kernel.org/git/20211212201441.GB3400@szeder.dev/\n> \n> Ævar Arnfjörð Bjarmason (3):\n> t1510: remove need for \"test_untraceable\", retain coverage\n> test-lib.sh: remove the now-unused \"test_untraceable\" facility\n> test-lib.sh: remove \"BASH_XTRACEFD\"\n> \n> t/README | 3 --\n> t/t1510-repo-setup.sh | 85 +++++++++++++++++++++----------------------\n> t/test-lib.sh | 66 ++++-----------------------------\n> 3 files changed, 49 insertions(+), 105 deletions(-)\n> \n> Range-diff against v3:\n> 1: 7876202c5b0 = 1: 9e7b089dc50 t1510: remove need for \"test_untraceable\", retain coverage\n> -: ----------- > 2: 60883fd95cb test-lib.sh: remove the now-unused \"test_untraceable\" facility\n> 2: a7fc794e20d ! 3: 8b5ae33376e test-lib.sh: remove the now-unused \"test_untraceable\" facility\n> @@ Metadata\n> Author: Ævar Arnfjörð Bjarmason \n> \n> ## Commit message ##\n> - test-lib.sh: remove the now-unused \"test_untraceable\" facility\n> + test-lib.sh: remove \"BASH_XTRACEFD\"\n> \n> - In the preceding commit the use of \"test_untraceable=UnfortunatelyYes\"\n> - was removed from \"t1510-repo-setup.sh\" in favor of more narrow\n> - redirections of the output of specific commands (and not entire\n> - sub-shells or functions).\n> + Stop setting \"BASH_XTRACEFD=4\" to direct \"-x\" output to file\n> + descriptor 4 under bash.\n> \n> - This is in line with the fixes in the series that introduced the\n> - \"test_untraceable\" facility. See 571e472dc43 (Merge branch\n> - 'sg/test-x', 2018-03-14) for the series as a whole, and\n> - e.g. 91538d0cde9 (t5570-git-daemon: don't check the stderr of a\n> - subshell, 2018-02-24) for a commit that's in line with the changes in\n> - the preceding commit.\n> + When it was added in d88785e424a (test-lib: set BASH_XTRACEFD\n> + automatically, 2016-05-11) it was needed as a workaround for various\n> + tests that didn't pass cleanly under \"-x\".\n> \n> - We've thus solved the TODO item noted when \"test_untraceable\" was\n> - added to \"t1510-repo-setup.sh\" in 58275069288 (t1510-repo-setup: mark\n> - as untraceable with '-x', 2018-02-24).\n> + Most of those were later fixed in 71e472dc43 (Merge branch\n> + 'sg/test-x', 2018-03-14), and in the preceding commits we've fixed the\n> + final remaining and removed the \"test_untraceable\" facility.\n> \n> - So let's remove the feature entirely. Not only is it currently unused,\n> - but it actively encourages an anti-pattern in our tests. We should be\n> - testing the output of specific commands, not entire subshells or\n> - functions.\n> + The reason we don't need this anymore is becomes clear from reading\n> + the rationale in d88785e424a and applying those arguments to the\n> + current state of the codebase. In particular it said (with \"this\" and\n> + \"it\" referring to the problem of tests failing under \"-x\"):\n> \n> - That the \"-x\" output had to be disabled as a result is only one\n> - symptom, but even under bash those tests will be harder to debug as\n> - the subsequent check of the redirected file will be far removed from\n> - the command that emitted the output.\n> + \"there here isn't a portable or scalable solution to this [...] we\n> + can work around it by pointing the \"set -x\" output to our\n> + descriptor 4\"\n> \n> - Signed-off-by: Ævar Arnfjörð Bjarmason \n> + And finally that:\n> \n> - ## t/README ##\n> -@@ t/README: appropriately before running \"make\". Short options can be bundled, i.e.\n> - -x::\n> - \tTurn on shell tracing (i.e., `set -x`) during the tests\n> - \tthemselves. Implies `--verbose`.\n> --\tIgnored in test scripts that set the variable 'test_untraceable'\n> --\tto a non-empty value, unless it's run with a Bash version\n> --\tsupporting BASH_XTRACEFD, i.e. v4.1 or later.\n> - \n> - -d::\n> - --debug::\n> + \"Automatic tests for our \"-x\" option may be a bit too meta\"\n> +\n> + Those tests are exactly what we've had since aedffe95250 (travis-ci:\n> + run tests with '-x' tracing, 2018-02-24), so punting on fixing issues\n> + with \"-x\" by using \"BASH_XTRACEFD=4\" isn't needed anymore, we're now\n> + committing to maintaining the test suite in a way that won't break\n> + under \"-x\".\n> +\n> + We could retain \"BASH_XTRACEFD=4\" anyway, but doing so is bad because:\n> +\n> + 1) Since we're caring about \"-x\" passing in CI under \"dash\" on Ubuntu\n> + using \"BASH_XTRACEFD=4\" will amount to hiding an error we'll run\n> + into eventually. Tests will pass locally with \"bash\", but fail in\n> + CI with \"dash\" (or under other non-\"bash\" shells).\n> +\n> + 2) As the amended code in \"test_eval_\" shows (an amended revert to\n> + the pre-image of d88785e424a) it's simpler to not have to take\n> + this \"bash\" special-case into account.\n> +\n> + Signed-off-by: Ævar Arnfjörð Bjarmason \n> \n> ## t/test-lib.sh ##\n> -@@ t/test-lib.sh: then\n> - \texit\n> - fi\n> - \n> --if test -n \"$trace\" && test -n \"$test_untraceable\"\n> --then\n> --\t# '-x' tracing requested, but this test script can't be reliably\n> --\t# traced, unless it is run with a Bash version supporting\n> --\t# BASH_XTRACEFD (introduced in Bash v4.1).\n> --\t#\n> --\t# Perform this version check _after_ the test script was\n> --\t# potentially re-executed with $TEST_SHELL_PATH for '--tee' or\n> --\t# '--verbose-log', so the right shell is checked and the\n> --\t# warning is issued only once.\n> --\tif test -n \"$BASH_VERSION\" && eval '\n> --\t test ${BASH_VERSINFO[0]} -gt 4 || {\n> --\t test ${BASH_VERSINFO[0]} -eq 4 &&\n> --\t test ${BASH_VERSINFO[1]} -ge 1\n> --\t }\n> --\t '\n> --\tthen\n> --\t\t: Executed by a Bash version supporting BASH_XTRACEFD. Good.\n> --\telse\n> --\t\techo >&2 \"warning: ignoring -x; '$0' is untraceable without BASH_XTRACEFD\"\n> --\t\ttrace=\n> --\tfi\n> --fi\n> - if test -n \"$trace\" && test -z \"$verbose_log\"\n> - then\n> - \tverbose=t\n> @@ t/test-lib.sh: else\n> \texec 4>/dev/null 3>/dev/null\n> fi\n> -- \n> 2.34.1.1024.g573f2f4b767\n> \n"},{"id":"444017","messageId":"xmqqo85kcp99.fsf@gitster.g","threadId":"56994","inReplyTo":"20211212201441.GB3400@szeder.dev","subject":"Re: [PATCH v3 2/2] test-lib.sh: remove the now-unused \"test_untraceable\" facility","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-12-13T18:51:14Z","receivedAt":"2021-12-13T18:51:18Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"SZEDER Gábor writes:\n\n>> > Please do not remove BASH_XTRACEFD. And especially not like this,\n>> > without even mentioning it in the commit message.\n>> \n>> I can re-roll with an amended commit message that explicitly mentions\n>> it\n>\n> It should rather be a separate patch...\n>\n>>, but that doesn't address your \"please do not remove\",\n>> \n>> Do you see reason to keep it at the end-state fo this series? E.g. a\n>> counter-argument to\n>> https://lore.kernel.org/git/211210.86pmq4daxm.gmgdl@evledraar.gmail.com/?\n>\n> I don't see any argument pertinent to BASH_XTRACEFD in general in that\n> email, of in favor of its removal in particular, and there are no\n> valid arguments for its removal in earlier emails in this thread\n> either.\n\nIf I am reading Ævar right, the argument is \"dash would not be fixed\nwith BASH_XTRACEFD, so there needs another way that would work there,\nand if the approach happens to work also for bash, then there is no\nreason to use BASH_XTRACEFD\", I think.\n\nNow, if the way Ævar came up with to help shells with \"-x\" not to\ncontaminate their standard error stream that our test scripts want\nto inspect is worse to write, understand, and maintain, compared to\nthe way we have been writing our tests that inspect their standard\nerrors, without having to worry about \"-x\" output thanks to the use\nof BASH_XTRACEFD, it may make a regression to developer\nproductivity, but I am not sure if that is the case.\n\nI think [1/2] of this same series can serve an example of how tests\nmust be tweaked to live under the world order without BASH_XTRACEFD?\nHaving to set and use a temporary file to capture the standard error\noutput and append to it upfront looks uglier than each individual\ntest locally capturing the standard error output from a single\ninvocation of a helper function, but it does not look _too_ bad to\nme. Can we find another example to argue for BASH_XTRACEFD, how\nmuch it makes it easier to write tests that work even under \"-x\"?\n\n\n\n"},{"id":"444115","messageId":"YbjJuh4dVijL7jw4@coredump.intra.peff.net","threadId":"56994","inReplyTo":"xmqqo85kcp99.fsf@gitster.g","subject":"Re: [PATCH v3 2/2] test-lib.sh: remove the now-unused \"test_untraceable\" facility","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-12-14T16:43:38Z","receivedAt":"2021-12-14T16:43:41Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Dec 13, 2021 at 10:51:14AM -0800, Junio C Hamano wrote:\n\n> > I don't see any argument pertinent to BASH_XTRACEFD in general in that\n> > email, of in favor of its removal in particular, and there are no\n> > valid arguments for its removal in earlier emails in this thread\n> > either.\n> \n> If I am reading Ævar right, the argument is \"dash would not be fixed\n> with BASH_XTRACEFD, so there needs another way that would work there,\n> and if the approach happens to work also for bash, then there is no\n> reason to use BASH_XTRACEFD\", I think.\n> \n> Now, if the way Ævar came up with to help shells with \"-x\" not to\n> contaminate their standard error stream that our test scripts want\n> to inspect is worse to write, understand, and maintain, compared to\n> the way we have been writing our tests that inspect their standard\n> errors, without having to worry about \"-x\" output thanks to the use\n> of BASH_XTRACEFD, it may make a regression to developer\n> productivity, but I am not sure if that is the case.\n\nI think the method for handling this in the test scripts _is_ worse to\nwrite, understand, and maintain. The problem to me is less that it's\nugly to workaround (which as you note in this case is not great, but not\n_too_ bad), but that it's a subtle friction point that may jump up and\nbite any test-writer who does something like:\n\n (foo && bar) 2>stderr\n\nSo my view had always been that BASH_XTRACEFD is the good solution, and\nif people want to make \"-x\" work reliably under other shells, then I\nwon't stop them. But somewhere along the way Gábor did a bunch of fixes\nto get things mostly running, and the use of dash with \"-x\" got added to\nCI, so now it's a de facto requirement (if you care about CI\ncomplaining, which we increasingly do).\n\nMaybe that's OK. We've had fewer incidences of the problem popping up\nthan I would have expected.\n\nMy vision was that we'd leave BASH_XTRACEFD so people using it could\nremain oblivious if they chose, but if the ship has sailed via CI, then\nthat might have less value.\n\n> I think [1/2] of this same series can serve an example of how tests\n> must be tweaked to live under the world order without BASH_XTRACEFD?\n> Having to set and use a temporary file to capture the standard error\n> output and append to it upfront looks uglier than each individual\n> test locally capturing the standard error output from a single\n> invocation of a helper function, but it does not look _too_ bad to\n> me. Can we find another example to argue for BASH_XTRACEFD, how\n> much it makes it easier to write tests that work even under \"-x\"?\n\nI think the fixes from 571e472dc4 (Merge branch 'sg/test-x', 2018-03-14)\nshow what had to be done to get where we are today.\n\n-Peff\n"},{"id":"444152","messageId":"xmqqh7b994tw.fsf@gitster.g","threadId":"56994","inReplyTo":"YbjJuh4dVijL7jw4@coredump.intra.peff.net","subject":"Re: [PATCH v3 2/2] test-lib.sh: remove the now-unused \"test_untraceable\" facility","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-12-15T17:05:15Z","receivedAt":"2021-12-15T17:05:25Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> I think the method for handling this in the test scripts _is_ worse to\n> write, understand, and maintain. The problem to me is less that it's\n> ugly to workaround (which as you note in this case is not great, but not\n> _too_ bad), but that it's a subtle friction point that may jump up and\n> bite any test-writer who does something like:\n>\n> (foo && bar) 2>stderr\n\nYeah, that is a simple example that clearly shows how removal of\nBASH_XTRACEFD makes developer's life horrible.\n\n> So my view had always been that BASH_XTRACEFD is the good solution, and\n> if people want to make \"-x\" work reliably under other shells, then I\n> won't stop them. But somewhere along the way Gábor did a bunch of fixes\n> to get things mostly running, and the use of dash with \"-x\" got added to\n> CI, so now it's a de facto requirement (if you care about CI\n> complaining, which we increasingly do).\n> ...\n> My vision was that we'd leave BASH_XTRACEFD so people using it could\n> remain oblivious if they chose, but if the ship has sailed via CI, then\n> that might have less value.\n\nYeah, that matches my understanding. Unfortunately we cannot easily\nremove \"dash -x\" from CI while keeping \"dash\" without \"-x\" X-<.\n\nStill. I wonder if keeping BASH_XTRACEFD helps developers, when\nthey need to diagnose a new breakage? If their new test fails only\nin the \"dash -x\" run but not \"bash -x\" at the CI, for example?\n"},{"id":"444154","messageId":"YbojKafkC/JcX4d1@coredump.intra.peff.net","threadId":"56994","inReplyTo":"xmqqh7b994tw.fsf@gitster.g","subject":"Re: [PATCH v3 2/2] test-lib.sh: remove the now-unused \"test_untraceable\" facility","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-12-15T17:17:29Z","receivedAt":"2021-12-15T17:17:31Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Dec 15, 2021 at 09:05:15AM -0800, Junio C Hamano wrote:\n\n> Still. I wonder if keeping BASH_XTRACEFD helps developers, when\n> they need to diagnose a new breakage? If their new test fails only\n> in the \"dash -x\" run but not \"bash -x\" at the CI, for example?\n\nI have done that, but usually only after realizing that \"./t1234-foo.sh\"\npasses and \"./t1234-foo.sh -x\" does not. So I think just tweaking use of\n\"-x\" would be the main tool.\n\n-Peff\n"},{"id":"444155","messageId":"xmqqczlx93kg.fsf@gitster.g","threadId":"56994","inReplyTo":"YbojKafkC/JcX4d1@coredump.intra.peff.net","subject":"Re: [PATCH v3 2/2] test-lib.sh: remove the now-unused \"test_untraceable\" facility","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-12-15T17:32:31Z","receivedAt":"2021-12-15T17:32:38Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Wed, Dec 15, 2021 at 09:05:15AM -0800, Junio C Hamano wrote:\n>\n>> Still. I wonder if keeping BASH_XTRACEFD helps developers, when\n>> they need to diagnose a new breakage? If their new test fails only\n>> in the \"dash -x\" run but not \"bash -x\" at the CI, for example?\n>\n> I have done that, but usually only after realizing that \"./t1234-foo.sh\"\n> passes and \"./t1234-foo.sh -x\" does not. So I think just tweaking use of\n> \"-x\" would be the main tool.\n\nAh, that's true. You only need to compare \"sh -x test.sh\" with \"sh\ntest.sh\" with any value of \"sh\", especially after BASH_XTRACEFD is\nremoved, demoting \"bash\" to the same state as \"dash\" wrt \"-x\".\n\nOK, I am more OK with the removal of BASH_XTRACEFD support than\nbefore ;-)\n\n\n\n"},{"id":"444206","messageId":"211216.864k78bsjs.gmgdl@evledraar.gmail.com","threadId":"56994","inReplyTo":"xmqqczlx93kg.fsf@gitster.g","subject":"Re: [PATCH v3 2/2] test-lib.sh: remove the now-unused \"test_untraceable\" facility","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-12-16T13:04:58Z","receivedAt":"2021-12-16T13:14:36Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\n[Replying to the thread-at-large]\n\nOn Tue, Dec 14 2021, Jeff King wrote:\n\n> On Mon, Dec 13, 2021 at 10:51:14AM -0800, Junio C Hamano wrote:\n>\n>> > I don't see any argument pertinent to BASH_XTRACEFD in general in that\n>> > email, of in favor of its removal in particular, and there are no\n>> > valid arguments for its removal in earlier emails in this thread\n>> > either.\n>> \n>> If I am reading Ævar right, the argument is \"dash would not be fixed\n>> with BASH_XTRACEFD, so there needs another way that would work there,\n>> and if the approach happens to work also for bash, then there is no\n>> reason to use BASH_XTRACEFD\", I think.\n>> \n>> Now, if the way Ævar came up with to help shells with \"-x\" not to\n>> contaminate their standard error stream that our test scripts want\n>> to inspect is worse to write, understand, and maintain, compared to\n>> the way we have been writing our tests that inspect their standard\n>> errors, without having to worry about \"-x\" output thanks to the use\n>> of BASH_XTRACEFD, it may make a regression to developer\n>> productivity, but I am not sure if that is the case.\n>\n> I think the method for handling this in the test scripts _is_ worse to\n> write, understand, and maintain. The problem to me is less that it's\n> ugly to workaround (which as you note in this case is not great, but not\n> _too_ bad), but that it's a subtle friction point that may jump up and\n> bite any test-writer who does something like:\n>\n> (foo && bar) 2>stderr\n>\n> So my view had always been that BASH_XTRACEFD is the good\n> solution[...]\n\nYes, I agree that's much better than what you need to do when not under\nbash and when you can't benefit from BASH_XTRACEFD.\n\nBut unless we're talking about requiring bash and/or not supporting -x\nat all (which seems to be overkill, seeing as only one test scipt wasn't\ncompatible with it) this discussion seems a bit like talking about how\nsome code is nicer in C17 than C99 or C89.\n\nSure, it is. But if you're also supporting the latter two it's usually\nnot worth it to maintain both with an ifdef.\n\nSimilarly, we can't really get any real benefits from BASH_XTRACEFD as\nlong as we're going to support \"-x\" with other shells.\n\nLiterally the only part of the test suite where we hard-depended on it\nis the code adjusted in my 1/2 here. I do think if we could rely on the\npre-image it would be nicer, but not so nice that I don't want \"-x\"\nworking under \"dash\".\n\nAnd in any case carrying a \"BASH_XTRACEFD\" seems to be just a dead end\nfor that reason.\n\nOn Wed, Dec 15 2021, Junio C Hamano wrote:\n\n> Jeff King writes:\n>\n>> On Wed, Dec 15, 2021 at 09:05:15AM -0800, Junio C Hamano wrote:\n>>\n>>> Still. I wonder if keeping BASH_XTRACEFD helps developers, when\n>>> they need to diagnose a new breakage? If their new test fails only\n>>> in the \"dash -x\" run but not \"bash -x\" at the CI, for example?\n>>\n>> I have done that, but usually only after realizing that \"./t1234-foo.sh\"\n>> passes and \"./t1234-foo.sh -x\" does not. So I think just tweaking use of\n>> \"-x\" would be the main tool.\n>\n> Ah, that's true. You only need to compare \"sh -x test.sh\" with \"sh\n> test.sh\" with any value of \"sh\", especially after BASH_XTRACEFD is\n> removed, demoting \"bash\" to the same state as \"dash\" wrt \"-x\".\n>\n> OK, I am more OK with the removal of BASH_XTRACEFD support than\n> before ;-)\n\nYes, I've run into those cases and I don't think BASH_XTRACEFD really\nhelps at all. Yes you'll get a test failure because trace output got\ninto something we're invoking \"test_cmp\" on, or whatever.\n\nBut that's never a subtle failure, and the fix for it brings us back to\nthe question of whether we're going to support non-bash, or take the\nmore drastic step of marking the whole test script as bash-only under\n\"-x\".\n\nI think the answers to those are always going to be \"yes\" and \"no\", and\nthus we're not getting any benefit from \"BASH_XTRACEFD\".\n"},{"id":"447852","messageId":"20220206214024.GC1936@szeder.dev","threadId":"56994","inReplyTo":"YbMiK1wHzBfYvK2a@coredump.intra.peff.net","subject":"Re: [PATCH v2 1/2] t1510: remove need for \"test_untraceable\", retain coverage","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2022-02-06T21:40:24Z","receivedAt":"2022-02-06T21:40:31Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Fri, Dec 10, 2021 at 04:47:23AM -0500, Jeff King wrote:\n> On Thu, Dec 02, 2021 at 08:16:35PM +0100, SZEDER Gábor wrote:\n> \n> > > @@ -62,7 +59,7 @@ test_repo () {\n> > > \t\t\texport GIT_WORK_TREE\n> > > \t\tfi &&\n> > > \t\trm -f trace &&\n> > > -\t\tGIT_TRACE_SETUP=\"$(pwd)/trace\" git symbolic-ref HEAD >/dev/null &&\n> > > +\t\tGIT_TRACE_SETUP=\"$(pwd)/trace\" git symbolic-ref HEAD >/dev/null 2>>stderr &&\n> > \n> > I suspect that it's lines like this that make Peff argue for\n> > BASH_XTRACEFD :)\n> > \n> > While this is not a compound command, it does contain a command\n> > substitution, and the trace generated when executing the command in\n> > that command substitution goes to the command's stderr, and then,\n> > because of the redirection, to the 'stderr' file.\n> \n> Better still, the behavior varies between shells:\n\nIndeed, although POSIX seems to be quite clear about what should\nhappen in this case: the specs for simple commands [1] state that\nredirections should be performed before variable assignments are\nexpanded for, among other things, command substitution.\n\n> $ bash -c 'set -x; FOO=$(echo foo) echo main >stdout 2>stderr; set +x; grep . stdout stderr'\n> ++ echo foo\n> + FOO=foo\n> + echo main\n> + set +x\n> stdout:main\n> \n> $ dash -c 'set -x; FOO=$(echo foo) echo main >stdout 2>stderr; set +x; grep . stdout stderr'\n> + FOO=foo echo main\n> + set +x\n> stdout:main\n> stderr:+ echo foo\n\nSo in case of these commands the shell should first redirect stdout\nand stderr, then expand the command substitution, thus it should write\nthe trace for the 'echo foo' within to stderr _while_ stderr is\nredirected. It seems that dash, for once, does conform to POSIX.\n\nAlthough the standard-conform behavior is potentially more problematic\nfor us, because it would cause test failure with tracing enabled when\nused like this:\n\n GIT_TRACE=\"$(pwd)/trace\" git cmd --opts 2>actual.err &&\n test_cmp expected.err actual.err\n\nbecause the \"+ pwd\" trace output from the command substitution would\ngo to 'err.actual'; 9b2ac68f27 (t5526: use $TRASH_DIRECTORY to specify\nthe path of GIT_TRACE log file, 2018-02-24) fixed a case like this.\n\n\n[1] https://pubs.opengroup.org/onlinepubs/9699919799/utilities/V3_chap02.html#tag_18_09_01\n\n"},{"id":"449054","messageId":"220221.86v8x89ebo.gmgdl@evledraar.gmail.com","threadId":"56994","inReplyTo":"20211213054353.GC3400@szeder.dev","subject":"Re: [PATCH v4 0/3] test-lib.sh: have all tests pass under \"-x\", remove BASH_XTRACEFD","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-02-21T19:52:18Z","receivedAt":"2022-02-21T20:04:19Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\n[Junio: If you'd like to pick up this series it still cleanly applies,\nand merges cleanly with \"seen\"]\n\nOn Mon, Dec 13 2021, SZEDER Gábor wrote:\n\nSorry about the late reply, things getting lost around the holidays\netc.\n\n> On Mon, Dec 13, 2021 at 02:38:33AM +0100, Ævar Arnfjörð Bjarmason wrote:\n>> A re-arrangement-only change to v3[1]. The previous 2/2 is now split\n>> into two commits, as requested by SZEDER[2] in the removal of\n>> BASH_XTRACEFD is now its own commit & the rationale for doing so is\n>> outlined in detail.\n>\n> I'm afraid I wasn't clear. What I meant was that if we were to remove\n> BASH_XTRACEFD, then it should be done its own commit.\n\nAside from whether you think removing it is a good idea, isn't that what\nthis v4 does?\n\nIn 1/3 I fix -x under non-BASH_XTRACEFD, 2/3 removes test_untraceable,\nand 3/3 the use of BASH_XTRACEFD.\n\n> But again: BASH_XTRACEFD is the only simple yet reliable and robust\n> way to get -x trace from our tests, so do not remove it.\n\nJust to tie off this loose end, I re-read the thread ending in [1] (sent\nafter this reply of yours) and I think my [1] addresses this.\n\nMaybe you still disagree, but I don't see how that squares with\n\"reliable and robust\" here.\n\nI.e. unless we're talking about carrying bash-specific code in the test\nsuite we can't make any real use of the feature, as our tests will need\nto be compatible with other POSIX shells.\n\nI mean, the code changed in 1/3 *was* that bash-specific code, but as\nthat change shows it was rather easily made non-bash-specific. And\nunless we think we'll add other bash-specific tests (I don't see why,\nthe cross-shell -x support is rather easy to do) ....\n\n1. https://lore.kernel.org/git/211216.864k78bsjs.gmgdl@evledraar.gmail.com/\n\n>> 1. https://lore.kernel.org/git/cover-v3-0.2-00000000000-20211210T100512Z-avarab@gmail.com/\n>> 2. https://lore.kernel.org/git/20211212201441.GB3400@szeder.dev/\n>> \n>> Ævar Arnfjörð Bjarmason (3):\n>> t1510: remove need for \"test_untraceable\", retain coverage\n>> test-lib.sh: remove the now-unused \"test_untraceable\" facility\n>> test-lib.sh: remove \"BASH_XTRACEFD\"\n>> \n>> t/README | 3 --\n>> t/t1510-repo-setup.sh | 85 +++++++++++++++++++++----------------------\n>> t/test-lib.sh | 66 ++++-----------------------------\n>> 3 files changed, 49 insertions(+), 105 deletions(-)\n>> \n>> Range-diff against v3:\n>> 1: 7876202c5b0 = 1: 9e7b089dc50 t1510: remove need for \"test_untraceable\", retain coverage\n>> -: ----------- > 2: 60883fd95cb test-lib.sh: remove the now-unused \"test_untraceable\" facility\n>> 2: a7fc794e20d ! 3: 8b5ae33376e test-lib.sh: remove the now-unused \"test_untraceable\" facility\n>> @@ Metadata\n>> Author: Ævar Arnfjörð Bjarmason \n>> \n>> ## Commit message ##\n>> - test-lib.sh: remove the now-unused \"test_untraceable\" facility\n>> + test-lib.sh: remove \"BASH_XTRACEFD\"\n>> \n>> - In the preceding commit the use of \"test_untraceable=UnfortunatelyYes\"\n>> - was removed from \"t1510-repo-setup.sh\" in favor of more narrow\n>> - redirections of the output of specific commands (and not entire\n>> - sub-shells or functions).\n>> + Stop setting \"BASH_XTRACEFD=4\" to direct \"-x\" output to file\n>> + descriptor 4 under bash.\n>> \n>> - This is in line with the fixes in the series that introduced the\n>> - \"test_untraceable\" facility. See 571e472dc43 (Merge branch\n>> - 'sg/test-x', 2018-03-14) for the series as a whole, and\n>> - e.g. 91538d0cde9 (t5570-git-daemon: don't check the stderr of a\n>> - subshell, 2018-02-24) for a commit that's in line with the changes in\n>> - the preceding commit.\n>> + When it was added in d88785e424a (test-lib: set BASH_XTRACEFD\n>> + automatically, 2016-05-11) it was needed as a workaround for various\n>> + tests that didn't pass cleanly under \"-x\".\n>> \n>> - We've thus solved the TODO item noted when \"test_untraceable\" was\n>> - added to \"t1510-repo-setup.sh\" in 58275069288 (t1510-repo-setup: mark\n>> - as untraceable with '-x', 2018-02-24).\n>> + Most of those were later fixed in 71e472dc43 (Merge branch\n>> + 'sg/test-x', 2018-03-14), and in the preceding commits we've fixed the\n>> + final remaining and removed the \"test_untraceable\" facility.\n>> \n>> - So let's remove the feature entirely. Not only is it currently unused,\n>> - but it actively encourages an anti-pattern in our tests. We should be\n>> - testing the output of specific commands, not entire subshells or\n>> - functions.\n>> + The reason we don't need this anymore is becomes clear from reading\n>> + the rationale in d88785e424a and applying those arguments to the\n>> + current state of the codebase. In particular it said (with \"this\" and\n>> + \"it\" referring to the problem of tests failing under \"-x\"):\n>> \n>> - That the \"-x\" output had to be disabled as a result is only one\n>> - symptom, but even under bash those tests will be harder to debug as\n>> - the subsequent check of the redirected file will be far removed from\n>> - the command that emitted the output.\n>> + \"there here isn't a portable or scalable solution to this [...] we\n>> + can work around it by pointing the \"set -x\" output to our\n>> + descriptor 4\"\n>> \n>> - Signed-off-by: Ævar Arnfjörð Bjarmason \n>> + And finally that:\n>> \n>> - ## t/README ##\n>> -@@ t/README: appropriately before running \"make\". Short options can be bundled, i.e.\n>> - -x::\n>> - \tTurn on shell tracing (i.e., `set -x`) during the tests\n>> - \tthemselves. Implies `--verbose`.\n>> --\tIgnored in test scripts that set the variable 'test_untraceable'\n>> --\tto a non-empty value, unless it's run with a Bash version\n>> --\tsupporting BASH_XTRACEFD, i.e. v4.1 or later.\n>> - \n>> - -d::\n>> - --debug::\n>> + \"Automatic tests for our \"-x\" option may be a bit too meta\"\n>> +\n>> + Those tests are exactly what we've had since aedffe95250 (travis-ci:\n>> + run tests with '-x' tracing, 2018-02-24), so punting on fixing issues\n>> + with \"-x\" by using \"BASH_XTRACEFD=4\" isn't needed anymore, we're now\n>> + committing to maintaining the test suite in a way that won't break\n>> + under \"-x\".\n>> +\n>> + We could retain \"BASH_XTRACEFD=4\" anyway, but doing so is bad because:\n>> +\n>> + 1) Since we're caring about \"-x\" passing in CI under \"dash\" on Ubuntu\n>> + using \"BASH_XTRACEFD=4\" will amount to hiding an error we'll run\n>> + into eventually. Tests will pass locally with \"bash\", but fail in\n>> + CI with \"dash\" (or under other non-\"bash\" shells).\n>> +\n>> + 2) As the amended code in \"test_eval_\" shows (an amended revert to\n>> + the pre-image of d88785e424a) it's simpler to not have to take\n>> + this \"bash\" special-case into account.\n>> +\n>> + Signed-off-by: Ævar Arnfjörð Bjarmason \n>> \n>> ## t/test-lib.sh ##\n>> -@@ t/test-lib.sh: then\n>> - \texit\n>> - fi\n>> - \n>> --if test -n \"$trace\" && test -n \"$test_untraceable\"\n>> --then\n>> --\t# '-x' tracing requested, but this test script can't be reliably\n>> --\t# traced, unless it is run with a Bash version supporting\n>> --\t# BASH_XTRACEFD (introduced in Bash v4.1).\n>> --\t#\n>> --\t# Perform this version check _after_ the test script was\n>> --\t# potentially re-executed with $TEST_SHELL_PATH for '--tee' or\n>> --\t# '--verbose-log', so the right shell is checked and the\n>> --\t# warning is issued only once.\n>> --\tif test -n \"$BASH_VERSION\" && eval '\n>> --\t test ${BASH_VERSINFO[0]} -gt 4 || {\n>> --\t test ${BASH_VERSINFO[0]} -eq 4 &&\n>> --\t test ${BASH_VERSINFO[1]} -ge 1\n>> --\t }\n>> --\t '\n>> --\tthen\n>> --\t\t: Executed by a Bash version supporting BASH_XTRACEFD. Good.\n>> --\telse\n>> --\t\techo >&2 \"warning: ignoring -x; '$0' is untraceable without BASH_XTRACEFD\"\n>> --\t\ttrace=\n>> --\tfi\n>> --fi\n>> - if test -n \"$trace\" && test -z \"$verbose_log\"\n>> - then\n>> - \tverbose=t\n>> @@ t/test-lib.sh: else\n>> \texec 4>/dev/null 3>/dev/null\n>> fi\n>> -- \n>> 2.34.1.1024.g573f2f4b767\n>> \n\no\n"},{"id":"449058","messageId":"20220221210319.GA1658@szeder.dev","threadId":"56994","inReplyTo":"220221.86v8x89ebo.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH v4 0/3] test-lib.sh: have all tests pass under \"-x\", remove BASH_XTRACEFD","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2022-02-21T21:03:19Z","receivedAt":"2022-02-21T21:03:30Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Mon, Feb 21, 2022 at 08:52:18PM +0100, Ævar Arnfjörð Bjarmason wrote:\n> \n> [Junio: If you'd like to pick up this series it still cleanly applies,\n> and merges cleanly with \"seen\"]\n> \n> On Mon, Dec 13 2021, SZEDER Gábor wrote:\n> \n> Sorry about the late reply, things getting lost around the holidays\n> etc.\n> \n> > On Mon, Dec 13, 2021 at 02:38:33AM +0100, Ævar Arnfjörð Bjarmason wrote:\n> >> A re-arrangement-only change to v3[1]. The previous 2/2 is now split\n> >> into two commits, as requested by SZEDER[2] in the removal of\n> >> BASH_XTRACEFD is now its own commit & the rationale for doing so is\n> >> outlined in detail.\n> >\n> > I'm afraid I wasn't clear. What I meant was that if we were to remove\n> > BASH_XTRACEFD, then it should be done its own commit.\n> \n> Aside from whether you think removing it is a good idea, isn't that what\n> this v4 does?\n\nWell, yes, but I now realize that I wasn't sufficiently clear the\nsecond time around, either, and emphasis doesn't travel well over\nemail. What I meant was that: _if_ at all we were to remove it, then\nit should be a separate patch, but since we should most definitely\nkeep it, those hunks should be simply dropped.\n\n> In 1/3 I fix -x under non-BASH_XTRACEFD, 2/3 removes test_untraceable,\n> and 3/3 the use of BASH_XTRACEFD.\n> \n> > But again: BASH_XTRACEFD is the only simple yet reliable and robust\n> > way to get -x trace from our tests, so do not remove it.\n> \n> Just to tie off this loose end, I re-read the thread ending in [1] (sent\n> after this reply of yours) and I think my [1] addresses this.\n\nIt doesn't at all; \"if CI passes without it, then we can remove it\" is\nnot a convincing argument.\n\n> Maybe you still disagree, but I don't see how that squares with\n> \"reliable and robust\" here.\n> \n> I.e. unless we're talking about carrying bash-specific code in the test\n> suite we can't make any real use of the feature, as our tests will need\n> to be compatible with other POSIX shells.\n> \n> I mean, the code changed in 1/3 *was* that bash-specific code, but as\n> that change shows it was rather easily made non-bash-specific. And\n> unless we think we'll add other bash-specific tests (I don't see why,\n> the cross-shell -x support is rather easy to do) ....\n> \n> 1. https://lore.kernel.org/git/211216.864k78bsjs.gmgdl@evledraar.gmail.com/\n> \n> >> 1. https://lore.kernel.org/git/cover-v3-0.2-00000000000-20211210T100512Z-avarab@gmail.com/\n> >> 2. https://lore.kernel.org/git/20211212201441.GB3400@szeder.dev/\n> >> \n> >> Ævar Arnfjörð Bjarmason (3):\n> >> t1510: remove need for \"test_untraceable\", retain coverage\n> >> test-lib.sh: remove the now-unused \"test_untraceable\" facility\n> >> test-lib.sh: remove \"BASH_XTRACEFD\"\n> >> \n> >> t/README | 3 --\n> >> t/t1510-repo-setup.sh | 85 +++++++++++++++++++++----------------------\n> >> t/test-lib.sh | 66 ++++-----------------------------\n> >> 3 files changed, 49 insertions(+), 105 deletions(-)\n> >> \n> >> Range-diff against v3:\n> >> 1: 7876202c5b0 = 1: 9e7b089dc50 t1510: remove need for \"test_untraceable\", retain coverage\n> >> -: ----------- > 2: 60883fd95cb test-lib.sh: remove the now-unused \"test_untraceable\" facility\n> >> 2: a7fc794e20d ! 3: 8b5ae33376e test-lib.sh: remove the now-unused \"test_untraceable\" facility\n> >> @@ Metadata\n> >> Author: Ævar Arnfjörð Bjarmason \n> >> \n> >> ## Commit message ##\n> >> - test-lib.sh: remove the now-unused \"test_untraceable\" facility\n> >> + test-lib.sh: remove \"BASH_XTRACEFD\"\n> >> \n> >> - In the preceding commit the use of \"test_untraceable=UnfortunatelyYes\"\n> >> - was removed from \"t1510-repo-setup.sh\" in favor of more narrow\n> >> - redirections of the output of specific commands (and not entire\n> >> - sub-shells or functions).\n> >> + Stop setting \"BASH_XTRACEFD=4\" to direct \"-x\" output to file\n> >> + descriptor 4 under bash.\n> >> \n> >> - This is in line with the fixes in the series that introduced the\n> >> - \"test_untraceable\" facility. See 571e472dc43 (Merge branch\n> >> - 'sg/test-x', 2018-03-14) for the series as a whole, and\n> >> - e.g. 91538d0cde9 (t5570-git-daemon: don't check the stderr of a\n> >> - subshell, 2018-02-24) for a commit that's in line with the changes in\n> >> - the preceding commit.\n> >> + When it was added in d88785e424a (test-lib: set BASH_XTRACEFD\n> >> + automatically, 2016-05-11) it was needed as a workaround for various\n> >> + tests that didn't pass cleanly under \"-x\".\n> >> \n> >> - We've thus solved the TODO item noted when \"test_untraceable\" was\n> >> - added to \"t1510-repo-setup.sh\" in 58275069288 (t1510-repo-setup: mark\n> >> - as untraceable with '-x', 2018-02-24).\n> >> + Most of those were later fixed in 71e472dc43 (Merge branch\n> >> + 'sg/test-x', 2018-03-14), and in the preceding commits we've fixed the\n> >> + final remaining and removed the \"test_untraceable\" facility.\n> >> \n> >> - So let's remove the feature entirely. Not only is it currently unused,\n> >> - but it actively encourages an anti-pattern in our tests. We should be\n> >> - testing the output of specific commands, not entire subshells or\n> >> - functions.\n> >> + The reason we don't need this anymore is becomes clear from reading\n> >> + the rationale in d88785e424a and applying those arguments to the\n> >> + current state of the codebase. In particular it said (with \"this\" and\n> >> + \"it\" referring to the problem of tests failing under \"-x\"):\n> >> \n> >> - That the \"-x\" output had to be disabled as a result is only one\n> >> - symptom, but even under bash those tests will be harder to debug as\n> >> - the subsequent check of the redirected file will be far removed from\n> >> - the command that emitted the output.\n> >> + \"there here isn't a portable or scalable solution to this [...] we\n> >> + can work around it by pointing the \"set -x\" output to our\n> >> + descriptor 4\"\n> >> \n> >> - Signed-off-by: Ævar Arnfjörð Bjarmason \n> >> + And finally that:\n> >> \n> >> - ## t/README ##\n> >> -@@ t/README: appropriately before running \"make\". Short options can be bundled, i.e.\n> >> - -x::\n> >> - \tTurn on shell tracing (i.e., `set -x`) during the tests\n> >> - \tthemselves. Implies `--verbose`.\n> >> --\tIgnored in test scripts that set the variable 'test_untraceable'\n> >> --\tto a non-empty value, unless it's run with a Bash version\n> >> --\tsupporting BASH_XTRACEFD, i.e. v4.1 or later.\n> >> - \n> >> - -d::\n> >> - --debug::\n> >> + \"Automatic tests for our \"-x\" option may be a bit too meta\"\n> >> +\n> >> + Those tests are exactly what we've had since aedffe95250 (travis-ci:\n> >> + run tests with '-x' tracing, 2018-02-24), so punting on fixing issues\n> >> + with \"-x\" by using \"BASH_XTRACEFD=4\" isn't needed anymore, we're now\n> >> + committing to maintaining the test suite in a way that won't break\n> >> + under \"-x\".\n> >> +\n> >> + We could retain \"BASH_XTRACEFD=4\" anyway, but doing so is bad because:\n> >> +\n> >> + 1) Since we're caring about \"-x\" passing in CI under \"dash\" on Ubuntu\n> >> + using \"BASH_XTRACEFD=4\" will amount to hiding an error we'll run\n> >> + into eventually. Tests will pass locally with \"bash\", but fail in\n> >> + CI with \"dash\" (or under other non-\"bash\" shells).\n> >> +\n> >> + 2) As the amended code in \"test_eval_\" shows (an amended revert to\n> >> + the pre-image of d88785e424a) it's simpler to not have to take\n> >> + this \"bash\" special-case into account.\n> >> +\n> >> + Signed-off-by: Ævar Arnfjörð Bjarmason \n> >> \n> >> ## t/test-lib.sh ##\n> >> -@@ t/test-lib.sh: then\n> >> - \texit\n> >> - fi\n> >> - \n> >> --if test -n \"$trace\" && test -n \"$test_untraceable\"\n> >> --then\n> >> --\t# '-x' tracing requested, but this test script can't be reliably\n> >> --\t# traced, unless it is run with a Bash version supporting\n> >> --\t# BASH_XTRACEFD (introduced in Bash v4.1).\n> >> --\t#\n> >> --\t# Perform this version check _after_ the test script was\n> >> --\t# potentially re-executed with $TEST_SHELL_PATH for '--tee' or\n> >> --\t# '--verbose-log', so the right shell is checked and the\n> >> --\t# warning is issued only once.\n> >> --\tif test -n \"$BASH_VERSION\" && eval '\n> >> --\t test ${BASH_VERSINFO[0]} -gt 4 || {\n> >> --\t test ${BASH_VERSINFO[0]} -eq 4 &&\n> >> --\t test ${BASH_VERSINFO[1]} -ge 1\n> >> --\t }\n> >> --\t '\n> >> --\tthen\n> >> --\t\t: Executed by a Bash version supporting BASH_XTRACEFD. Good.\n> >> --\telse\n> >> --\t\techo >&2 \"warning: ignoring -x; '$0' is untraceable without BASH_XTRACEFD\"\n> >> --\t\ttrace=\n> >> --\tfi\n> >> --fi\n> >> - if test -n \"$trace\" && test -z \"$verbose_log\"\n> >> - then\n> >> - \tverbose=t\n> >> @@ t/test-lib.sh: else\n> >> \texec 4>/dev/null 3>/dev/null\n> >> fi\n> >> -- \n> >> 2.34.1.1024.g573f2f4b767\n> >> \n> \n> o\n"},{"id":"449062","messageId":"220221.86ee3val4u.gmgdl@evledraar.gmail.com","threadId":"56994","inReplyTo":"20220221210319.GA1658@szeder.dev","subject":"Re: [PATCH v4 0/3] test-lib.sh: have all tests pass under \"-x\", remove BASH_XTRACEFD","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-02-21T22:41:01Z","receivedAt":"2022-02-21T22:51:54Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Mon, Feb 21 2022, SZEDER Gábor wrote:\n\n> On Mon, Feb 21, 2022 at 08:52:18PM +0100, Ævar Arnfjörð Bjarmason wrote:\n>> \n>> [Junio: If you'd like to pick up this series it still cleanly applies,\n>> and merges cleanly with \"seen\"]\n>> \n>> On Mon, Dec 13 2021, SZEDER Gábor wrote:\n>> \n>> Sorry about the late reply, things getting lost around the holidays\n>> etc.\n>> \n>> > On Mon, Dec 13, 2021 at 02:38:33AM +0100, Ævar Arnfjörð Bjarmason wrote:\n>> >> A re-arrangement-only change to v3[1]. The previous 2/2 is now split\n>> >> into two commits, as requested by SZEDER[2] in the removal of\n>> >> BASH_XTRACEFD is now its own commit & the rationale for doing so is\n>> >> outlined in detail.\n>> >\n>> > I'm afraid I wasn't clear. What I meant was that if we were to remove\n>> > BASH_XTRACEFD, then it should be done its own commit.\n>> \n>> Aside from whether you think removing it is a good idea, isn't that what\n>> this v4 does?\n>\n> Well, yes, but I now realize that I wasn't sufficiently clear the\n> second time around, either, and emphasis doesn't travel well over\n> email. What I meant was that: _if_ at all we were to remove it, then\n> it should be a separate patch, but since we should most definitely\n> keep it, those hunks should be simply dropped.\n\nI understand, thanks.\n\n>> In 1/3 I fix -x under non-BASH_XTRACEFD, 2/3 removes test_untraceable,\n>> and 3/3 the use of BASH_XTRACEFD.\n>> \n>> > But again: BASH_XTRACEFD is the only simple yet reliable and robust\n>> > way to get -x trace from our tests, so do not remove it.\n>> \n>> Just to tie off this loose end, I re-read the thread ending in [1] (sent\n>> after this reply of yours) and I think my [1] addresses this.\n>\n> It doesn't at all; \"if CI passes without it, then we can remove it\" is\n> not a convincing argument.\n\nYes I agree that would be a bad argument, but it's not the one I'm\nmaking.\n\nThe one I am making is in\nhttps://lore.kernel.org/git/211216.864k78bsjs.gmgdl@evledraar.gmail.com\n\nI.e. I agree that it's a useful feature, and I wish in the abstract that we\ncould make real use of it.\n\nBut that would mean a hard dependency on bash, which I don't think would\nbe acceptable, or something anyone's advocating for.\n\nAs long as we're not going down that road I don't see the point in using\nit.\n\nThe only practical use of it in the test suite is to support a\nspecial-case I'm making un-special in 1/3, because I wanted \"-x\" output\nwithout needing to run bash. As it turned out it wasn't hard to do, and\nis consistent with how other tests are written).\n\nSo what's your argument for it? I.e. how are we practically going to use\nit in a way that makes a difference?\n\nI only see us not really using it, because our behavior in practice will\n1=1 mirror non-bash shells.\n\nBy keeping it around we're only exposing ourselves to edge cases and\nhiding bugs that we'd like to fix sooner than later anyway (and which\nare fixed as of this 1/3, and before that mostly were in your earlier\nseries).\n\nSo would test code that depended on bash and BASH_XTRACEFD be more\n\"reliable and robust\"? Absolutely, you wouldn't need to worry about some\ninterpolated $(pwd) or whatever in a string getting into your -x output\nwhen you didn't expect it.\n\nBut with how we're using it it's doing the opposite of that. It'll only\nhide those bugs for non-bash users.\n\nI may be entirely wrong, or perhaps I haven't considered some edge case\nor trade-off you have in mind. But I'm not able to bridge that gap with\nyour rather terse replies :)\n\n>> Maybe you still disagree, but I don't see how that squares with\n>> \"reliable and robust\" here.\n>> \n>> I.e. unless we're talking about carrying bash-specific code in the test\n>> suite we can't make any real use of the feature, as our tests will need\n>> to be compatible with other POSIX shells.\n>> \n>> I mean, the code changed in 1/3 *was* that bash-specific code, but as\n>> that change shows it was rather easily made non-bash-specific. And\n>> unless we think we'll add other bash-specific tests (I don't see why,\n>> the cross-shell -x support is rather easy to do) ....\n>> \n>> 1. https://lore.kernel.org/git/211216.864k78bsjs.gmgdl@evledraar.gmail.com/\n>> \n>> >> 1. https://lore.kernel.org/git/cover-v3-0.2-00000000000-20211210T100512Z-avarab@gmail.com/\n>> >> 2. https://lore.kernel.org/git/20211212201441.GB3400@szeder.dev/\n>> >> \n>> >> Ævar Arnfjörð Bjarmason (3):\n>> >> t1510: remove need for \"test_untraceable\", retain coverage\n>> >> test-lib.sh: remove the now-unused \"test_untraceable\" facility\n>> >> test-lib.sh: remove \"BASH_XTRACEFD\"\n>> >> \n>> >> t/README | 3 --\n>> >> t/t1510-repo-setup.sh | 85 +++++++++++++++++++++----------------------\n>> >> t/test-lib.sh | 66 ++++-----------------------------\n>> >> 3 files changed, 49 insertions(+), 105 deletions(-)\n>> >> \n>> >> Range-diff against v3:\n>> >> 1: 7876202c5b0 = 1: 9e7b089dc50 t1510: remove need for \"test_untraceable\", retain coverage\n>> >> -: ----------- > 2: 60883fd95cb test-lib.sh: remove the now-unused \"test_untraceable\" facility\n>> >> 2: a7fc794e20d ! 3: 8b5ae33376e test-lib.sh: remove the now-unused \"test_untraceable\" facility\n>> >> @@ Metadata\n>> >> Author: Ævar Arnfjörð Bjarmason \n>> >> \n>> >> ## Commit message ##\n>> >> - test-lib.sh: remove the now-unused \"test_untraceable\" facility\n>> >> + test-lib.sh: remove \"BASH_XTRACEFD\"\n>> >> \n>> >> - In the preceding commit the use of \"test_untraceable=UnfortunatelyYes\"\n>> >> - was removed from \"t1510-repo-setup.sh\" in favor of more narrow\n>> >> - redirections of the output of specific commands (and not entire\n>> >> - sub-shells or functions).\n>> >> + Stop setting \"BASH_XTRACEFD=4\" to direct \"-x\" output to file\n>> >> + descriptor 4 under bash.\n>> >> \n>> >> - This is in line with the fixes in the series that introduced the\n>> >> - \"test_untraceable\" facility. See 571e472dc43 (Merge branch\n>> >> - 'sg/test-x', 2018-03-14) for the series as a whole, and\n>> >> - e.g. 91538d0cde9 (t5570-git-daemon: don't check the stderr of a\n>> >> - subshell, 2018-02-24) for a commit that's in line with the changes in\n>> >> - the preceding commit.\n>> >> + When it was added in d88785e424a (test-lib: set BASH_XTRACEFD\n>> >> + automatically, 2016-05-11) it was needed as a workaround for various\n>> >> + tests that didn't pass cleanly under \"-x\".\n>> >> \n>> >> - We've thus solved the TODO item noted when \"test_untraceable\" was\n>> >> - added to \"t1510-repo-setup.sh\" in 58275069288 (t1510-repo-setup: mark\n>> >> - as untraceable with '-x', 2018-02-24).\n>> >> + Most of those were later fixed in 71e472dc43 (Merge branch\n>> >> + 'sg/test-x', 2018-03-14), and in the preceding commits we've fixed the\n>> >> + final remaining and removed the \"test_untraceable\" facility.\n>> >> \n>> >> - So let's remove the feature entirely. Not only is it currently unused,\n>> >> - but it actively encourages an anti-pattern in our tests. We should be\n>> >> - testing the output of specific commands, not entire subshells or\n>> >> - functions.\n>> >> + The reason we don't need this anymore is becomes clear from reading\n>> >> + the rationale in d88785e424a and applying those arguments to the\n>> >> + current state of the codebase. In particular it said (with \"this\" and\n>> >> + \"it\" referring to the problem of tests failing under \"-x\"):\n>> >> \n>> >> - That the \"-x\" output had to be disabled as a result is only one\n>> >> - symptom, but even under bash those tests will be harder to debug as\n>> >> - the subsequent check of the redirected file will be far removed from\n>> >> - the command that emitted the output.\n>> >> + \"there here isn't a portable or scalable solution to this [...] we\n>> >> + can work around it by pointing the \"set -x\" output to our\n>> >> + descriptor 4\"\n>> >> \n>> >> - Signed-off-by: Ævar Arnfjörð Bjarmason \n>> >> + And finally that:\n>> >> \n>> >> - ## t/README ##\n>> >> -@@ t/README: appropriately before running \"make\". Short options can be bundled, i.e.\n>> >> - -x::\n>> >> - \tTurn on shell tracing (i.e., `set -x`) during the tests\n>> >> - \tthemselves. Implies `--verbose`.\n>> >> --\tIgnored in test scripts that set the variable 'test_untraceable'\n>> >> --\tto a non-empty value, unless it's run with a Bash version\n>> >> --\tsupporting BASH_XTRACEFD, i.e. v4.1 or later.\n>> >> - \n>> >> - -d::\n>> >> - --debug::\n>> >> + \"Automatic tests for our \"-x\" option may be a bit too meta\"\n>> >> +\n>> >> + Those tests are exactly what we've had since aedffe95250 (travis-ci:\n>> >> + run tests with '-x' tracing, 2018-02-24), so punting on fixing issues\n>> >> + with \"-x\" by using \"BASH_XTRACEFD=4\" isn't needed anymore, we're now\n>> >> + committing to maintaining the test suite in a way that won't break\n>> >> + under \"-x\".\n>> >> +\n>> >> + We could retain \"BASH_XTRACEFD=4\" anyway, but doing so is bad because:\n>> >> +\n>> >> + 1) Since we're caring about \"-x\" passing in CI under \"dash\" on Ubuntu\n>> >> + using \"BASH_XTRACEFD=4\" will amount to hiding an error we'll run\n>> >> + into eventually. Tests will pass locally with \"bash\", but fail in\n>> >> + CI with \"dash\" (or under other non-\"bash\" shells).\n>> >> +\n>> >> + 2) As the amended code in \"test_eval_\" shows (an amended revert to\n>> >> + the pre-image of d88785e424a) it's simpler to not have to take\n>> >> + this \"bash\" special-case into account.\n>> >> +\n>> >> + Signed-off-by: Ævar Arnfjörð Bjarmason \n>> >> \n>> >> ## t/test-lib.sh ##\n>> >> -@@ t/test-lib.sh: then\n>> >> - \texit\n>> >> - fi\n>> >> - \n>> >> --if test -n \"$trace\" && test -n \"$test_untraceable\"\n>> >> --then\n>> >> --\t# '-x' tracing requested, but this test script can't be reliably\n>> >> --\t# traced, unless it is run with a Bash version supporting\n>> >> --\t# BASH_XTRACEFD (introduced in Bash v4.1).\n>> >> --\t#\n>> >> --\t# Perform this version check _after_ the test script was\n>> >> --\t# potentially re-executed with $TEST_SHELL_PATH for '--tee' or\n>> >> --\t# '--verbose-log', so the right shell is checked and the\n>> >> --\t# warning is issued only once.\n>> >> --\tif test -n \"$BASH_VERSION\" && eval '\n>> >> --\t test ${BASH_VERSINFO[0]} -gt 4 || {\n>> >> --\t test ${BASH_VERSINFO[0]} -eq 4 &&\n>> >> --\t test ${BASH_VERSINFO[1]} -ge 1\n>> >> --\t }\n>> >> --\t '\n>> >> --\tthen\n>> >> --\t\t: Executed by a Bash version supporting BASH_XTRACEFD. Good.\n>> >> --\telse\n>> >> --\t\techo >&2 \"warning: ignoring -x; '$0' is untraceable without BASH_XTRACEFD\"\n>> >> --\t\ttrace=\n>> >> --\tfi\n>> >> --fi\n>> >> - if test -n \"$trace\" && test -z \"$verbose_log\"\n>> >> - then\n>> >> - \tverbose=t\n>> >> @@ t/test-lib.sh: else\n>> >> \texec 4>/dev/null 3>/dev/null\n>> >> fi\n>> >> -- \n>> >> 2.34.1.1024.g573f2f4b767\n>> >> \n>> \n>> o\n\n"},{"id":"449064","messageId":"20220221231121.GB1658@szeder.dev","threadId":"56994","inReplyTo":"patch-v4-3.3-8b5ae33376e-20211213T013559Z-avarab@gmail.com","subject":"Re: [PATCH v4 3/3] test-lib.sh: remove \"BASH_XTRACEFD\"","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2022-02-21T23:11:21Z","receivedAt":"2022-02-21T23:11:28Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Mon, Dec 13, 2021 at 02:38:36AM +0100, Ævar Arnfjörð Bjarmason wrote:\n> Stop setting \"BASH_XTRACEFD=4\" to direct \"-x\" output to file\n> descriptor 4 under bash.\n> \n> When it was added in d88785e424a (test-lib: set BASH_XTRACEFD\n> automatically, 2016-05-11) it was needed as a workaround for various\n> tests that didn't pass cleanly under \"-x\".\n> \n> Most of those were later fixed in 71e472dc43 (Merge branch\n> 'sg/test-x', 2018-03-14), and in the preceding commits we've fixed the\n> final remaining and removed the \"test_untraceable\" facility.\n\nThose commits made the test suite pass with '-x' without BASH_XTRACEFD\nonly when all went well, but during development that's often not the\ncase. So let's not forget about c5c39f4e34 (test-lib: fix interrupt\nhandling with 'dash' and '--verbose-log -x', 2019-03-13), before which\ndash was not really suitable to run tests involving daemon processes\nwith '-x' during development. If dash were to announce redirections\nin its '-x' trace, like many not as quite as popular shells do, then\nthe workaround in that commit wouldn't work at all.\n\nIn general, between POSIX leaving a lot of things explicitly\nunspecified, or, worse, silently unspecified, shells not conforming to\nPOSIX, being buggy, and/or implementing their own extensions, I am\nactually quite surprised that it works as well as it does with so many\nshell. At least as far as we know it, and I wouldn't at all be\nsurprised if there were unknown issues lurking in some corner cases\nand/or with some more exotic shells.\n\n> We could retain \"BASH_XTRACEFD=4\" anyway, but doing so is bad because:\n> \n> 1) Since we're caring about \"-x\" passing in CI under \"dash\" on Ubuntu\n> using \"BASH_XTRACEFD=4\" will amount to hiding an error we'll run\n> into eventually. Tests will pass locally with \"bash\", but fail in\n> CI with \"dash\" (or under other non-\"bash\" shells).\n\nThis is not \"bad\", this is exactly what we use CI for. This is the\nsmae case as when the test suite passes on a developer's Linux box,\nbut breaks on OSX or Windows in CI.\n\nFurthermore, while I fully agree that keeping the whole test suite\npassing with '-x' without BASH_XTRACEFD is desirable, I do think it's\na bad idea to forbid developers from using it while hacking away to\nscratch their itches. I for one sometimes deliberately use various\nbashisms in my tests, including 'test_cmp'-ing the stderr of loops and\nfunctions, because they make writing tests then and there easier, when\nat that point I'd rather focus my attention on getting the C changes\nright, and clean them up eventually when I deem the changes worthy for\nsubmission.\n\n\nOverall I consider this patch as a cleanup solely for cleanup's sake,\nwithout any benefits at all.\n\n\nI'm kind of low on time myself as well, at least to argue about this\nany further. Therefore, as the one who did the vast majority of work\nto make '-x' work even without BASH_XTRACEFD, I leave here my firm:\n\n Not-Acked-By: SZEDER Gábor \n\nto any patch that attempts to remove support for BASH_XTRACEFD.\n\n"},{"id":"449139","messageId":"220222.86h78q7wc4.gmgdl@evledraar.gmail.com","threadId":"56994","inReplyTo":"20220221231121.GB1658@szeder.dev","subject":"Re: [PATCH v4 3/3] test-lib.sh: remove \"BASH_XTRACEFD\"","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-02-22T15:14:19Z","receivedAt":"2022-02-22T15:30:25Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Tue, Feb 22 2022, SZEDER Gábor wrote:\n\n> On Mon, Dec 13, 2021 at 02:38:36AM +0100, Ævar Arnfjörð Bjarmason wrote:\n>> Stop setting \"BASH_XTRACEFD=4\" to direct \"-x\" output to file\n>> descriptor 4 under bash.\n>> \n>> When it was added in d88785e424a (test-lib: set BASH_XTRACEFD\n>> automatically, 2016-05-11) it was needed as a workaround for various\n>> tests that didn't pass cleanly under \"-x\".\n>> \n>> Most of those were later fixed in 71e472dc43 (Merge branch\n>> 'sg/test-x', 2018-03-14), and in the preceding commits we've fixed the\n>> final remaining and removed the \"test_untraceable\" facility.\n>\n> Those commits made the test suite pass with '-x' without BASH_XTRACEFD\n> only when all went well, but during development that's often not the\n> case. So let's not forget about c5c39f4e34 (test-lib: fix interrupt\n> handling with 'dash' and '--verbose-log -x', 2019-03-13), before which\n> dash was not really suitable to run tests involving daemon processes\n> with '-x' during development. If dash were to announce redirections\n> in its '-x' trace, like many not as quite as popular shells do, then\n> the workaround in that commit wouldn't work at all.\n>\n> In general, between POSIX leaving a lot of things explicitly\n> unspecified, or, worse, silently unspecified, shells not conforming to\n> POSIX, being buggy, and/or implementing their own extensions, I am\n> actually quite surprised that it works as well as it does with so many\n> shell. At least as far as we know it, and I wouldn't at all be\n> surprised if there were unknown issues lurking in some corner cases\n> and/or with some more exotic shells.\n>\n>> We could retain \"BASH_XTRACEFD=4\" anyway, but doing so is bad because:\n>> \n>> 1) Since we're caring about \"-x\" passing in CI under \"dash\" on Ubuntu\n>> using \"BASH_XTRACEFD=4\" will amount to hiding an error we'll run\n>> into eventually. Tests will pass locally with \"bash\", but fail in\n>> CI with \"dash\" (or under other non-\"bash\" shells).\n>\n> This is not \"bad\", this is exactly what we use CI for. This is the\n> smae case as when the test suite passes on a developer's Linux box,\n> but breaks on OSX or Windows in CI.\n\nYes we could definitely live with it. My aim here was to tighten up the\ncycle of testing, submitting/pushing a patch, having CI fail because\nthat test was on bash only etc.\n\n> Furthermore, while I fully agree that keeping the whole test suite\n> passing with '-x' without BASH_XTRACEFD is desirable, I do think it's\n> a bad idea to forbid developers from using it while hacking away to\n> scratch their itches. I for one sometimes deliberately use various\n> bashisms in my tests, including 'test_cmp'-ing the stderr of loops and\n> functions, because they make writing tests then and there easier, when\n> at that point I'd rather focus my attention on getting the C changes\n> right, and clean them up eventually when I deem the changes worthy for\n> submission.\n>\n>\n> Overall I consider this patch as a cleanup solely for cleanup's sake,\n> without any benefits at all.\n>\n> I'm kind of low on time myself as well, at least to argue about this\n> any further. Therefore, as the one who did the vast majority of work\n> to make '-x' work even without BASH_XTRACEFD, I leave here my firm:\n>\n> Not-Acked-By: SZEDER Gábor \n>\n> to any patch that attempts to remove support for BASH_XTRACEFD.\n\nSure, if you feel that strongly about it I'm fine with dropping this\nversion and submitting a re-roll without the test-lib.sh changes.\n\nI just genuinely didn't grok why you wanted to keep it before.\n\nBut I get it now, thanks.\n\nI think it's fair to say that it's for the exact same reason I'd like to\nget rid of it, you just think of it as a feature, not a bug :)\n\nI.e. the \"landmine\" of getting something working under bash, only to\nfind out later that it fails on other shells.\n\nBut everyone's workflow is different. It sounds as though you sometimes\nstart hacking tests to use bash-specific features that you later distill\ndown to POSIX-compatibility.\n\nWhich I don't think I've ever done, since doing so is going to require a\nrewrite anyway I might as well struggle to get it POSIX-ly working in\nthe first place.\n\nBut I can see how that's useful, and I don't want to take that away from\nyou.\n\nDo you feel equally strongly that this mode of operation must be the\ndefault under bash, or that it would be OK to have it be opt-in under a\nknob like GIT_TEST_BASHISMS=true, or something like that?\n\nThe reason I ran into this in the first place was because I saw such\nunexplained behavior changes with bash v.s. non-bash, only to arrive at\nthis part of test-lib.sh (which I'm sure I'd read N times before, but\nhad forgotten about).\n\nI don't want to make your local workflow harder, but I'd think it would\nbe fair to say that the average git.git developer won't be as keenly\naware of shell differences as you, and would be better served by having\nthis subtlety be opt-in.\n\nI.e. it's surely more likely that someone is to write POSIX-compatible\ntest code for ML submission, than intentionally using a bash-only\nfeature for prototyping, which will start breaking once it's exposed to\ndash et al in CI.\n\nOr, if you feel strongly about that too I wouldn't mind much if\n\"GIT_TEST_BASHISMS=true\" was the default, as long as I can set it to\n\"GIT_TEST_BASHISMS=false\" and /other/ bash-specific behavior without\nfurther opting it in to optional bash-only behavior that's off by\ndefault.\n\nAssuming you'd reply \"no\" to not wanting such a knob *and* having it by\noff by default, would that be a \"yes\" to having the knob and needing to\nturn it off?\n\nThanks!\n"}]}