Re: [PATCH v7 02/15] trace2: create new combined trace facility

From: SZEDER Gábor <szeder.dev@gmail.com>
Date: May 10, 2019, 17:28 UTC
Message-ID: <20190510172824.GR14763@szeder.dev>
In-Reply-To: <90e4e6af9300751fd2e7e98601ed0e60ca048e05.1550874298.git.gitgitgadget@gmail.com>

On Fri, Feb 22, 2019 at 02:25:01PM -0800, Jeff Hostetler via GitGitGadget wrote:

Show 25 quoted lines

> From: Jeff Hostetler <jeffhost@microsoft.com>
> 
> Create a new unified tracing facility for git.  The eventual intent is to
> replace the current trace_printf* and trace_performance* routines with a
> unified set of git_trace2* routines.
> 
> In addition to the usual printf-style API, trace2 provides higer-level
> event verbs with fixed-fields allowing structured data to be written.
> This makes post-processing and analysis easier for external tools.
> 
> Trace2 defines 3 output targets.  These are set using the environment
> variables "GIT_TR2", "GIT_TR2_PERF", and "GIT_TR2_EVENT".  These may be
> set to "1" or to an absolute pathname (just like the current GIT_TRACE).
> 
> * GIT_TR2 is intended to be a replacement for GIT_TRACE and logs command
>   summary data.
> 
> * GIT_TR2_PERF is intended as a replacement for GIT_TRACE_PERFORMANCE.
>   It extends the output with columns for the command process, thread,
>   repo, absolute and relative elapsed times.  It reports events for
>   child process start/stop, thread start/stop, and per-thread function
>   nesting.
> 
> * GIT_TR2_EVENT is a new structured format. It writes event data as a
>   series of JSON records.

Please document these new environment variables in 'Documentation/git.txt', where the other environment variables, including GIT_TRACE_*, are already documented.

While doing so, please note that the description about the possible values of these variables and of GIT_TRACE above is incomplete, because it's not just "1" or an absolute pathname. Quoting the description of GIT_TRACE:

  If this variable is set to "1", "2" or "true" (comparison is case
  insensitive), trace messages will be printed to stderr.
  
  If the variable is set to an integer value greater than 2 and lower
  than 10 (strictly) then Git will interpret this value as an open
  file descriptor and will try to write the trace messages into this
  file descriptor.

The way I see it in tr2_dst_get_trace_fd() below, this applies to GIT_TRACE2* as well, and it even offers the possibility to specify a Unix Domain Socket, too.

And sorry for barging in with a big bucket of paint this late, but, really... why GIT_TR2 instead of GIT_TRACE2?

Show 41 quoted lines

> +int tr2_dst_get_trace_fd(struct tr2_dst *dst)
> +{
> +	const char *tgt_value;
> +
> +	/* don't open twice */
> +	if (dst->initialized)
> +		return dst->fd;
> +
> +	dst->initialized = 1;
> +
> +	tgt_value = getenv(dst->env_var_name);
> +
> +	if (!tgt_value || !strcmp(tgt_value, "") || !strcmp(tgt_value, "0") ||
> +	    !strcasecmp(tgt_value, "false")) {
> +		dst->fd = 0;
> +		return dst->fd;
> +	}
> +
> +	if (!strcmp(tgt_value, "1") || !strcasecmp(tgt_value, "true")) {
> +		dst->fd = STDERR_FILENO;
> +		return dst->fd;
> +	}
> +
> +	if (strlen(tgt_value) == 1 && isdigit(*tgt_value)) {
> +		dst->fd = atoi(tgt_value);
> +		return dst->fd;
> +	}
> +
> +	if (is_absolute_path(tgt_value))
> +		return tr2_dst_try_path(dst, tgt_value);
> +
> +#ifndef NO_UNIX_SOCKETS
> +	if (starts_with(tgt_value, PREFIX_AF_UNIX))
> +		return tr2_dst_try_unix_domain_socket(dst, tgt_value);
> +#endif
> +
> +	/* Always warn about malformed values. */
> +	tr2_dst_malformed_warning(dst, tgt_value);
> +	tr2_dst_trace_disable(dst);
> +	return 0;
> +}