| Message ID | 3d1bbc91aa3a465ecec2de130456b9ccc08f3032.1638193666.git.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | b8de3d6e0221ca1d6c65bbce3cacc6a2206f89f5 |
| Headers | show |
| Series | Set GIT_TRACE2_EVENT_NESTING in test-lib.sh | expand |
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;
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > Thanks for following up on this. > > Rather than hardcoding 100 wouldn't it make sense to have something like > the below (which I barely checked for whether it compiled or not, just > to check how hard it was to change), so that we can just set this to a > "false" value to disable the nesting guard entirely? I agree that could be a better endgame if it works. That is where our agreement ends. I strongly prefer to have a small and focused fix for immediate problem at hand, and then a fundamental "(could be) better" change separately, so that we can cool the latter longer. This difference between us is not limited to this topic. I often get irritated to see an attempt to jump to the endgame with too big a change in a single step. Please don't. Thanks.
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > trace2.eventNesting:: > - Integer. Specifies desired depth of nested regions in the > + Integer or "false" boolean. Specifies desired depth of nested regions in the > event output. Regions deeper than this value will be > omitted. May be overridden by the `GIT_TRACE2_EVENT_NESTING` > environment variable. Defaults to 2. > ++ > +Set this to a "false" value (see linkgit:git-config[1] for accepted > +values, i.e. "false", "off" etc.) to remove the limitation on nesting > +entirely. If the value of 5 set to .eventNesting allows up to 5 levels of nesting, I would imagine some readers expect that "false" or "off" would forbid nesting of events altogether. For "false" to work well, the variable needs renaming to trace2.limitEventNesting, or something like that.
diff --git a/t/test-lib.sh b/t/test-lib.sh index 2679a7596a6..2e815c41c8f 100644 --- a/t/test-lib.sh +++ b/t/test-lib.sh @@ -476,6 +476,13 @@ export GIT_TEST_MERGE_ALGORITHM GIT_TRACE_BARE=1 export GIT_TRACE_BARE +# Some tests scan the GIT_TRACE2_EVENT feed for events, but the +# default depth is 2, which frequently causes issues when the +# events are wrapped in new regions. Set it to a sufficiently +# large depth to avoid custom changes in the test suite. +GIT_TRACE2_EVENT_NESTING=100 +export GIT_TRACE2_EVENT_NESTING + # Use specific version of the index file format if test -n "${GIT_TEST_INDEX_VERSION:+isset}" then