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

["Ævar Arnfjörð Bjarmason" <avarab@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;