On Mon, Nov 29 2021, Derrick Stolee via GitGitGadget wrote:
> From: Derrick Stolee <dstolee@xxxxxxxxxxxxx>
>
> 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@xxxxxxxxx>
> Helped-by: Jeff King <peff@xxxxxxxx>
> Signed-off-by: Derrick Stolee <dstolee@xxxxxxxxxxxxx>
> ---
> 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;
