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?
[...]
> --- 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
[...]
> +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.
[...]
> --- /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] = { ... },
[...]
> +/*
> + * 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
