From: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
To: Derrick Stolee via GitGitGadget <gitgitgadget@gmail.com>
Cc: git@vger.kernel.org, peff@peff.net, gitster@pobox.com,
me@ttaylorr.com, Derrick Stolee <derrickstolee@github.com>,
Derrick Stolee <dstolee@microsoft.com>
Subject: Re: [PATCH 1/2] test-lib.sh: set GIT_TRACE2_EVENT_NESTING
Date: Mon, 29 Nov 2021 15:12:20 +0100 [thread overview]
Message-ID: <211129.86fsrff36j.gmgdl@evledraar.gmail.com> (raw)
In-Reply-To: <3d1bbc91aa3a465ecec2de130456b9ccc08f3032.1638193666.git.gitgitgadget@gmail.com>
On Mon, Nov 29 2021, Derrick Stolee via GitGitGadget wrote:
> From: Derrick Stolee <dstolee@microsoft.com>
>
> The GIT_TRACE2_EVENT feed has a limited nesting depth to avoid
> overloading the feed when recursing into deep paths while adding more
> nested regions.
>
> Some tests use the GIT_TRACE2_EVENT feed to look for internal events,
> ensuring that intended behavior is happening.
>
> One such example is in t4216-log-bloom.sh which looks for a statistic
> given as a trace2_data_intmax() call. This test started failing under
> '-x' with 2ca245f8be5 (csum-file.h: increase hashfile buffer size,
> 2021-05-18) because the change in stderr triggered the progress API to
> create an extra trace2 region, ejecting the statistic.
>
> This change increases the value of GIT_TRACE2_EVENT_NESTING across the
> entire test suite to avoid errors like this. Future changes will remove
> custom assignments of GIT_TRACE2_EVENT_NESTING from some test scripts
> that were aware of this limitation.
>
> Reported-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> Helped-by: Jeff King <peff@peff.net>
> Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
> ---
> t/test-lib.sh | 7 +++++++
> 1 file changed, 7 insertions(+)
>
> diff --git a/t/test-lib.sh b/t/test-lib.sh
> index 2679a7596a6..2e815c41c8f 100644
> --- a/t/test-lib.sh
> +++ b/t/test-lib.sh
> @@ -476,6 +476,13 @@ export GIT_TEST_MERGE_ALGORITHM
> GIT_TRACE_BARE=1
> export GIT_TRACE_BARE
>
> +# Some tests scan the GIT_TRACE2_EVENT feed for events, but the
> +# default depth is 2, which frequently causes issues when the
> +# events are wrapped in new regions. Set it to a sufficiently
> +# large depth to avoid custom changes in the test suite.
> +GIT_TRACE2_EVENT_NESTING=100
> +export GIT_TRACE2_EVENT_NESTING
> +
Thanks for following up on this.
Rather than hardcoding 100 wouldn't it make sense to have something like
the below (which I barely checked for whether it compiled or not, just
to check how hard it was to change), so that we can just set this to a
"false" value to disable the nesting guard entirely?
Needing to dance around the value beint an integer or true/false is an
unfortunate side-effect of there not being two separate "enable nesting
guard?" and "nest level?" config knob, but there's not much to do about
that at this point, so I just mapped "false" to "-1" internally.
Maybe values of 0 and 1 should be an error in any case? I didn't check,
that would make this approach simpler.
diff --git a/Documentation/config/trace2.txt b/Documentation/config/trace2.txt
index fe1642f0d40..2be5474fdcd 100644
--- a/Documentation/config/trace2.txt
+++ b/Documentation/config/trace2.txt
@@ -35,10 +35,14 @@ trace2.eventBrief::
`GIT_TRACE2_EVENT_BRIEF` environment variable. Defaults to false.
trace2.eventNesting::
- Integer. Specifies desired depth of nested regions in the
+ Integer or "false" boolean. Specifies desired depth of nested regions in the
event output. Regions deeper than this value will be
omitted. May be overridden by the `GIT_TRACE2_EVENT_NESTING`
environment variable. Defaults to 2.
++
+Set this to a "false" value (see linkgit:git-config[1] for accepted
+values, i.e. "false", "off" etc.) to remove the limitation on nesting
+entirely.
trace2.configParams::
A comma-separated list of patterns of "important" config
diff --git a/trace2/tr2_tgt_event.c b/trace2/tr2_tgt_event.c
index 3a0014417cc..135d3fbd8c3 100644
--- a/trace2/tr2_tgt_event.c
+++ b/trace2/tr2_tgt_event.c
@@ -53,8 +53,22 @@ static int fn_init(void)
return want;
nesting = tr2_sysenv_get(TR2_SYSENV_EVENT_NESTING);
- if (nesting && *nesting && ((max_nesting = atoi(nesting)) > 0))
- tr2env_event_max_nesting_levels = max_nesting;
+ if (nesting) {
+ /*
+ * TODO: Maybe expose the "off" part of
+ * git_parse_maybe_bool_text() as
+ * git_parse_maybe_bool_text_false() and use it here?
+ * Note that we accept "GIT_TRACE2_EVENT_NESTING=" and
+ * "trace2.eventNesting=" as "false", as with the rest
+ * of the config mechanism and git_parse_maybe_bool()
+ * users.
+ */
+ if (!*nesting || !strcmp(nesting, "false") ||
+ !strcmp(nesting, "no") || !strcmp(nesting, "off"))
+ tr2env_event_max_nesting_levels = -1;
+ else if (*nesting && ((max_nesting = atoi(nesting)) > 0))
+ tr2env_event_max_nesting_levels = max_nesting;
+ }
brief = tr2_sysenv_get(TR2_SYSENV_EVENT_BRIEF);
if (brief && *brief &&
@@ -503,6 +517,15 @@ static void fn_repo_fl(const char *file, int line,
jw_release(&jw);
}
+static int nesting_ok(int nr_open_regions)
+{
+ if (tr2env_event_max_nesting_levels == -1)
+ return 1;
+ if (nr_open_regions <= tr2env_event_max_nesting_levels)
+ return 1;
+ return 0;
+}
+
static void fn_region_enter_printf_va_fl(const char *file, int line,
uint64_t us_elapsed_absolute,
const char *category,
@@ -512,7 +535,7 @@ static void fn_region_enter_printf_va_fl(const char *file, int line,
{
const char *event_name = "region_enter";
struct tr2tls_thread_ctx *ctx = tr2tls_get_self();
- if (ctx->nr_open_regions <= tr2env_event_max_nesting_levels) {
+ if (nesting_ok(ctx->nr_open_regions)) {
struct json_writer jw = JSON_WRITER_INIT;
jw_object_begin(&jw, 0);
@@ -537,7 +560,7 @@ static void fn_region_leave_printf_va_fl(
{
const char *event_name = "region_leave";
struct tr2tls_thread_ctx *ctx = tr2tls_get_self();
- if (ctx->nr_open_regions <= tr2env_event_max_nesting_levels) {
+ if (nesting_ok(ctx->nr_open_regions)) {
struct json_writer jw = JSON_WRITER_INIT;
double t_rel = (double)us_elapsed_region / 1000000.0;
@@ -564,7 +587,7 @@ static void fn_data_fl(const char *file, int line, uint64_t us_elapsed_absolute,
{
const char *event_name = "data";
struct tr2tls_thread_ctx *ctx = tr2tls_get_self();
- if (ctx->nr_open_regions <= tr2env_event_max_nesting_levels) {
+ if (nesting_ok(ctx->nr_open_regions)) {
struct json_writer jw = JSON_WRITER_INIT;
double t_abs = (double)us_elapsed_absolute / 1000000.0;
double t_rel = (double)us_elapsed_region / 1000000.0;
@@ -592,7 +615,7 @@ static void fn_data_json_fl(const char *file, int line,
{
const char *event_name = "data_json";
struct tr2tls_thread_ctx *ctx = tr2tls_get_self();
- if (ctx->nr_open_regions <= tr2env_event_max_nesting_levels) {
+ if (nesting_ok(ctx->nr_open_regions)) {
struct json_writer jw = JSON_WRITER_INIT;
double t_abs = (double)us_elapsed_absolute / 1000000.0;
double t_rel = (double)us_elapsed_region / 1000000.0;
next prev parent reply other threads:[~2021-11-29 16:36 UTC|newest]
Thread overview: 50+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-11-29 13:47 [PATCH 0/2] Set GIT_TRACE2_EVENT_NESTING in test-lib.sh Derrick Stolee via GitGitGadget
2021-11-29 13:47 ` [PATCH 1/2] test-lib.sh: set GIT_TRACE2_EVENT_NESTING Derrick Stolee via GitGitGadget
2021-11-29 14:12 ` Ævar Arnfjörð Bjarmason [this message]
2021-11-29 18:28 ` Junio C Hamano
2021-11-29 18:32 ` Junio C Hamano
2021-11-29 13:47 ` [PATCH 2/2] t/t*: remove custom GIT_TRACE2_EVENT_NESTING Derrick Stolee via GitGitGadget
2021-11-29 18:40 ` [PATCH 0/2] Set GIT_TRACE2_EVENT_NESTING in test-lib.sh Jeff King
2021-11-29 18:44 ` Jeff King
2021-11-30 0:04 ` Taylor Blau
2021-11-30 15:34 ` Derrick Stolee
2021-11-30 22:43 ` Jeff Hostetler
2021-12-01 19:46 ` Jeff King
2021-11-29 20:13 ` [PATCH] test-lib.sh: have all tests pass under "-x", remove BASH_XTRACEFD Ævar Arnfjörð Bjarmason
2021-11-29 23:23 ` Eric Sunshine
2021-11-30 21:08 ` Jeff King
2021-11-30 21:50 ` Ævar Arnfjörð Bjarmason
2021-11-30 22:44 ` SZEDER Gábor
2021-12-01 14:06 ` Ævar Arnfjörð Bjarmason
2021-12-01 19:38 ` Jeff King
2021-12-01 18:38 ` Junio C Hamano
2021-12-01 20:11 ` [PATCH v2 0/2] " Ævar Arnfjörð Bjarmason
2021-12-01 20:11 ` [PATCH v2 1/2] t1510: remove need for "test_untraceable", retain coverage Ævar Arnfjörð Bjarmason
2021-12-02 19:16 ` SZEDER Gábor
2021-12-02 19:28 ` SZEDER Gábor
2021-12-10 9:47 ` Jeff King
2021-12-10 10:08 ` Ævar Arnfjörð Bjarmason
2022-02-06 21:40 ` SZEDER Gábor
2021-12-01 20:11 ` [PATCH v2 2/2] test-lib.sh: remove the now-unused "test_untraceable" facility Ævar Arnfjörð Bjarmason
2021-12-10 10:07 ` [PATCH v3 0/2] test-lib.sh: have all tests pass under "-x", remove BASH_XTRACEFD Ævar Arnfjörð Bjarmason
2021-12-10 10:07 ` [PATCH v3 1/2] t1510: remove need for "test_untraceable", retain coverage Ævar Arnfjörð Bjarmason
2021-12-10 10:07 ` [PATCH v3 2/2] test-lib.sh: remove the now-unused "test_untraceable" facility Ævar Arnfjörð Bjarmason
2021-12-12 16:32 ` SZEDER Gábor
2021-12-12 17:06 ` Ævar Arnfjörð Bjarmason
2021-12-12 20:14 ` SZEDER Gábor
2021-12-13 18:51 ` Junio C Hamano
2021-12-14 16:43 ` Jeff King
2021-12-15 17:05 ` Junio C Hamano
2021-12-15 17:17 ` Jeff King
2021-12-15 17:32 ` Junio C Hamano
2021-12-16 13:04 ` Ævar Arnfjörð Bjarmason
2021-12-13 1:38 ` [PATCH v4 0/3] test-lib.sh: have all tests pass under "-x", remove BASH_XTRACEFD Ævar Arnfjörð Bjarmason
2021-12-13 1:38 ` [PATCH v4 1/3] t1510: remove need for "test_untraceable", retain coverage Ævar Arnfjörð Bjarmason
2021-12-13 1:38 ` [PATCH v4 2/3] test-lib.sh: remove the now-unused "test_untraceable" facility Ævar Arnfjörð Bjarmason
2021-12-13 1:38 ` [PATCH v4 3/3] test-lib.sh: remove "BASH_XTRACEFD" Ævar Arnfjörð Bjarmason
2022-02-21 23:11 ` SZEDER Gábor
2022-02-22 15:14 ` Ævar Arnfjörð Bjarmason
2021-12-13 5:43 ` [PATCH v4 0/3] test-lib.sh: have all tests pass under "-x", remove BASH_XTRACEFD SZEDER Gábor
2022-02-21 19:52 ` Ævar Arnfjörð Bjarmason
2022-02-21 21:03 ` SZEDER Gábor
2022-02-21 22:41 ` Ævar Arnfjörð Bjarmason
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: http://vger.kernel.org/majordomo-info.html
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=211129.86fsrff36j.gmgdl@evledraar.gmail.com \
--to=avarab@gmail.com \
--cc=derrickstolee@github.com \
--cc=dstolee@microsoft.com \
--cc=git@vger.kernel.org \
--cc=gitgitgadget@gmail.com \
--cc=gitster@pobox.com \
--cc=me@ttaylorr.com \
--cc=peff@peff.net \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).