Re: [PATCH v2 4/7] trace2: use system config for default trace2 settings

From: Jonathan Nieder <jrnieder@gmail.com>
Date: Apr 3, 2019, 00:00 UTC
Message-ID: <20190403000032.GA190454@google.com>
In-Reply-To: <d048f3ffb801adc7f1b4e48248ca31ebade1b37d.1553879063.git.gitgitgadget@gmail.com>

Hi,

Jeff Hostetler via GitGitGadget wrote:

> Teach git to read the system config (usually "/etc/gitconfig") for
> default Trace2 settings.  This allows system-wide Trace2 settings to
> be installed and inherited to make it easier to manage a collection of
> systems.

Yay!  Thanks for writing this, and sorry for the slow review.

[...]

> Only the system config file is used.  Trace2 config values are ignored
> in local, global, and other config files.  Likewise, the "-c" command
> line arguments are ignored for Trace2 values.  These limits are for
> performance reasons.

Can you say a bit more about this? If I'm willing to pay the performance cost, is there a way for me to turn on respecting other config files? What is that performance cost?

[...]

Show 14 quoted lines

> --- a/Documentation/technical/api-trace2.txt
> +++ b/Documentation/technical/api-trace2.txt
> @@ -117,6 +117,37 @@ values are recognized.
>  Socket type can be either `stream` or `dgram`.  If the socket type is
>  omitted, Git will try both.
>  
> +== Trace2 Settings in System Config
> +
> +Trace2 also reads configuration information from the system config.
> +This is intended to help adminstrators to gather system-wide Git
> +performance data.
> +
> +Trace2 only reads the system configuration, it does not read global,
> +local, worktree, or `-c` config settings.

An additional limitation is that this doesn't appear to support include.* directives. Intended?

[...]

> --- a/t/t0210-trace2-normal.sh
> +++ b/t/t0210-trace2-normal.sh

[...]

Show 10 quoted lines

> +MOCK=./mock_system_config
> +
> +test_expect_success 'setup mocked /etc/gitconfig' '
> +	git config --file $MOCK trace2.normalTarget "$(pwd)/trace.normal" &&
> +	git config --file $MOCK trace2.normalBrief 1
> +'
> +
> +test_expect_success 'using mock, normal stream, return code 0' '
> +	test_when_finished "rm trace.normal actual expect" &&
> +	GIT_TEST_TR2_SYSTEM_CONFIG="$MOCK" test-tool trace2 001return 0 &&

Tests run with GIT_CONFIG_NOSYSTEM=1 to protect themselves from any system config the user has.

So this would be easier to test if we can use user-level config for it.

[...]

Show 22 quoted lines

> --- /dev/null
> +++ b/trace2/tr2_sysenv.c
> @@ -0,0 +1,128 @@
> +#include "cache.h"
> +#include "config.h"
> +#include "dir.h"
> +#include "tr2_sysenv.h"
> +
> +/*
> + * Each entry represents a trace2 setting.
> + * See Documentation/technical/api-trace2.txt
> + */
> +struct tr2_sysenv_entry {
> +	const char *env_var_name;
> +	const char *git_config_name;
> +
> +	char *value;
> +	unsigned int getenv_called : 1;
> +};
> +
> +/*
> + * This table must match "enum tr2_sysenv_variable" in tr2_sysenv.h.

Can we deduplicate to avoid the need to match?

Perhaps using C99 array initializers would help:

	[TR2_SYSENV_CFG_PARAM] = { ... },

[...]

Show 24 quoted lines

> +/*
> + * Load Trace2 settings from the system config (usually "/etc/gitconfig"
> + * unless we were built with a runtime-prefix).  These are intended to
> + * define the default values for Trace2 as requested by the administrator.
> + */
> +void tr2_sysenv_load(void)
> +{
> +	const char *system_config_pathname;
> +	const char *test_pathname;
> +
> +	system_config_pathname = git_etc_gitconfig();
> +
> +	test_pathname = getenv("GIT_TEST_TR2_SYSTEM_CONFIG");
> +	if (test_pathname) {
> +		if (!*test_pathname || !strcmp(test_pathname, "0"))
> +			return; /* disable use of system config */
> +
> +		/* mock it with given test file */
> +		system_config_pathname = test_pathname;
> +	}
> +
> +	if (file_exists(system_config_pathname))
> +		git_config_from_file(tr2_sysenv_cb, system_config_pathname,
> +				     NULL);

This duplicates functionality from config.c and misses some features along the way (e.g. support for include.*).

Would read_early_config work? If we want a variant that doesn't discover_git_directory, we can change that function to handle it.

For our needs at Google, it would be very helpful to have support for include.* and reading settings from at least $HOME/.gitconfig in addition to /etc/gitconfig (even better if it supports per-repo config as well). I believe it would simplify the code and tests, too. If there's anything I can to help, please don't hesitate to ask.

Thanks, Jonathan