On Thu, Mar 28 2019, Jeff Hostetler via GitGitGadget wrote:
Thanks for working on this!
Haven't given this any deep testing. Just some observations:
> From: Jeff Hostetler <jeffhost@xxxxxxxxxxxxx>
>
> 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.
>
> The original GIT_TR2* environment variables are loaded afterwards and
> can be used to override the system settings.
>
> 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.
>
> (1) For users not using Trace2, there should be minimal overhead to
> detect that Trace2 is not enabled. In particular, Trace2 should not
> allocate lots of otherwise unused data strucutres.
>
> (2) For accurate performance measurements, Trace2 should be initialized
> as early in the git process as possible, and before most of the normal
> git process initialization (which involves discovering the .git directory
> and reading a hierarchy of config files).
>
> Added the GIT_TEST_TR2_SYSTEM_CONFIG environment variable for testing
> purposes to specify the pathname of a fake "system" config or disable
> use of the system config.
>
> Signed-off-by: Jeff Hostetler <jeffhost@xxxxxxxxxxxxx>
> ---
> Documentation/technical/api-trace2.txt | 31 ++++++
> Makefile | 1 +
> t/t0210-trace2-normal.sh | 41 +++++++-
> t/t0211-trace2-perf.sh | 41 +++++++-
> t/t0212-trace2-event.sh | 52 +++++++++-
> trace2.c | 4 +
> trace2.h | 7 +-
> trace2/tr2_cfg.c | 7 +-
> trace2/tr2_dst.c | 24 ++---
> trace2/tr2_dst.h | 3 +-
> trace2/tr2_sysenv.c | 125 +++++++++++++++++++++++++
> trace2/tr2_sysenv.h | 36 +++++++
> trace2/tr2_tgt_event.c | 19 ++--
> trace2/tr2_tgt_normal.c | 10 +-
> trace2/tr2_tgt_perf.c | 10 +-
> 15 files changed, 356 insertions(+), 55 deletions(-)
> create mode 100644 trace2/tr2_sysenv.c
> create mode 100644 trace2/tr2_sysenv.h
>
> diff --git a/Documentation/technical/api-trace2.txt b/Documentation/technical/api-trace2.txt
> index baaa1153bb..13ca595c69 100644
> --- 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.
> +
> +Trace2 will try to load the following system configuration settings
> +and then read the corresponding environment variables at startup.
> +
> +....
> +---------------------------------------------------
> +trace2.normalTarget GIT_TR2
> +trace2.normalBrief GIT_TR2_BRIEF
> +
> +trace2.perfTarget GIT_TR2_PERF
> +trace2.perfBrief GIT_TR2_PERF_BRIEF
> +
> +trace2.eventTarget GIT_TR2_EVENT
> +trace2.eventBrief GIT_TR2_EVENT_BRIEF
> +trace2.eventNesting GIT_TR2_EVENT_NESTING
> +
> +trace2.configParams GIT_TR2_CONFIG_PARAMS
> +
> +trace2.destinationDebug GIT_TR2_DST_DEBUG
> +---------------------------------------------------
> +....
> +
> +
> == Trace2 API
>
> All public Trace2 functions and macros are defined in `trace2.h` and
> diff --git a/Makefile b/Makefile
> index 3e03290d8f..9ddfa3dfe7 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -1005,6 +1005,7 @@ LIB_OBJS += trace2/tr2_cfg.o
> LIB_OBJS += trace2/tr2_cmd_name.o
> LIB_OBJS += trace2/tr2_dst.o
> LIB_OBJS += trace2/tr2_sid.o
> +LIB_OBJS += trace2/tr2_sysenv.o
> LIB_OBJS += trace2/tr2_tbuf.o
> LIB_OBJS += trace2/tr2_tgt_event.o
> LIB_OBJS += trace2/tr2_tgt_normal.o
> diff --git a/t/t0210-trace2-normal.sh b/t/t0210-trace2-normal.sh
> index 03a0aedb1d..5d4c04ed30 100755
> --- a/t/t0210-trace2-normal.sh
> +++ b/t/t0210-trace2-normal.sh
> @@ -1,5 +1,14 @@
> #!/bin/sh
>
> +# Disable loading of Trace2 settings from the system config
> +# (usually "/etc/gitconfig") to eliminate system dependencies.
> +GIT_TEST_TR2_SYSTEM_CONFIG=0 && export GIT_TEST_TR2_SYSTEM_CONFIG
> +
> +# Turn off any inherited trace2 settings for this test.
> +unset GIT_TR2 GIT_TR2_PERF GIT_TR2_EVENT
> +unset GIT_TR2_BRIEF
> +unset GIT_TR2_CONFIG_PARAMS
> +
> test_description='test trace2 facility (normal target)'
> . ./test-lib.sh
>
> @@ -15,11 +24,6 @@ PATH="$TTDIR:$PATH" && export PATH
> # Warning: So you may see extra lines in artifact files when
> # Warning: interactively debugging.
>
> -# Turn off any inherited trace2 settings for this test.
> -unset GIT_TR2 GIT_TR2_PERF GIT_TR2_EVENT
> -unset GIT_TR2_BRIEF
> -unset GIT_TR2_CONFIG_PARAMS
> -
> V=$(git version | sed -e 's/^git version //') && export V
>
> # There are multiple trace2 targets: normal, perf, and event.
> @@ -132,4 +136,31 @@ test_expect_success 'normal stream, error event' '
> test_cmp expect actual
> '
>
> +# Now test using system config by using a mocked up config file
> +# rather than inheriting "/etc/gitconfig". Here we do not use
> +# GIT_TR2* environment variables.
> +
> +unset GIT_TR2_BRIEF
> +
> +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 &&
> + perl "$TEST_DIRECTORY/t0210/scrub_normal.perl" <trace.normal >actual &&
> + cat >expect <<-EOF &&
> + version $V
> + start _EXE_ trace2 001return 0
> + cmd_name trace2 (trace2)
> + exit elapsed:_TIME_ code:0
> + atexit elapsed:_TIME_ code:0
> + EOF
> + test_cmp expect actual
> +'
> +
> test_done
> diff --git a/t/t0211-trace2-perf.sh b/t/t0211-trace2-perf.sh
> index c9694b29f7..abe35b2186 100755
> --- a/t/t0211-trace2-perf.sh
> +++ b/t/t0211-trace2-perf.sh
> @@ -1,5 +1,14 @@
> #!/bin/sh
>
> +# Disable loading of Trace2 settings from the system config
> +# (usually "/etc/gitconfig") to eliminate system dependencies.
> +GIT_TEST_TR2_SYSTEM_CONFIG=0 && export GIT_TEST_TR2_SYSTEM_CONFIG
> +
> +# Turn off any inherited trace2 settings for this test.
> +unset GIT_TR2 GIT_TR2_PERF GIT_TR2_EVENT
> +unset GIT_TR2_PERF_BRIEF
> +unset GIT_TR2_CONFIG_PARAMS
> +
> test_description='test trace2 facility (perf target)'
> . ./test-lib.sh
>
> @@ -15,11 +24,6 @@ PATH="$TTDIR:$PATH" && export PATH
> # Warning: So you may see extra lines in artifact files when
> # Warning: interactively debugging.
>
> -# Turn off any inherited trace2 settings for this test.
> -unset GIT_TR2 GIT_TR2_PERF GIT_TR2_EVENT
> -unset GIT_TR2_PERF_BRIEF
> -unset GIT_TR2_CONFIG_PARAMS
> -
> V=$(git version | sed -e 's/^git version //') && export V
>
> # There are multiple trace2 targets: normal, perf, and event.
> @@ -150,4 +154,31 @@ test_expect_success 'perf stream, child processes' '
> test_cmp expect actual
> '
>
> +# Now test using system config by using a mocked up config file
> +# rather than inheriting "/etc/gitconfig". Here we do not use
> +# GIT_TR2* environment variables.
> +
> +unset GIT_TR2_PERF_BRIEF
> +
> +MOCK=./mock_system_config
> +
> +test_expect_success 'setup mocked /etc/gitconfig' '
> + git config --file $MOCK trace2.perfTarget "$(pwd)/trace.perf" &&
> + git config --file $MOCK trace2.perfBrief 1
> +'
> +
> +test_expect_success 'using mock, perf stream, return code 0' '
> + test_when_finished "rm trace.perf actual expect" &&
> + GIT_TEST_TR2_SYSTEM_CONFIG="$MOCK" test-tool trace2 001return 0 &&
> + perl "$TEST_DIRECTORY/t0211/scrub_perf.perl" <trace.perf >actual &&
> + cat >expect <<-EOF &&
> + d0|main|version|||||$V
> + d0|main|start||_T_ABS_|||_EXE_ trace2 001return 0
> + d0|main|cmd_name|||||trace2 (trace2)
> + d0|main|exit||_T_ABS_|||code:0
> + d0|main|atexit||_T_ABS_|||code:0
> + EOF
> + test_cmp expect actual
> +'
> +
> test_done
> diff --git a/t/t0212-trace2-event.sh b/t/t0212-trace2-event.sh
> index 028b6c5671..c535496261 100755
> --- a/t/t0212-trace2-event.sh
> +++ b/t/t0212-trace2-event.sh
> @@ -1,5 +1,14 @@
> #!/bin/sh
>
> +# Disable loading of Trace2 settings from the system config
> +# (usually "/etc/gitconfig") to eliminate system dependencies.
> +GIT_TEST_TR2_SYSTEM_CONFIG=0 && export GIT_TEST_TR2_SYSTEM_CONFIG
> +
> +# Turn off any inherited trace2 settings for this test.
> +unset GIT_TR2 GIT_TR2_PERF GIT_TR2_EVENT
> +unset GIT_TR2_BARE
> +unset GIT_TR2_CONFIG_PARAMS
> +
> test_description='test trace2 facility'
> . ./test-lib.sh
>
> @@ -17,11 +26,6 @@ PATH="$TTDIR:$PATH" && export PATH
> # Warning: So you may see extra lines in artifact files when
> # Warning: interactively debugging.
>
> -# Turn off any inherited trace2 settings for this test.
> -unset GIT_TR2 GIT_TR2_PERF GIT_TR2_EVENT
> -unset GIT_TR2_BARE
> -unset GIT_TR2_CONFIG_PARAMS
> -
> V=$(git version | sed -e 's/^git version //') && export V
>
> # There are multiple trace2 targets: normal, perf, and event.
> @@ -233,4 +237,42 @@ test_expect_success JSON_PP 'basic trace2_data' '
> test_cmp expect actual
> '
>
> +# Now test using system config by using a mocked up config file
> +# rather than inheriting "/etc/gitconfig". Here we do not use
> +# GIT_TR2* environment variables.
> +
> +MOCK=./mock_system_config
> +
> +test_expect_success 'setup mocked /etc/gitconfig' '
> + git config --file $MOCK trace2.eventTarget "$(pwd)/trace.event"
> +'
> +
> +test_expect_success JSON_PP 'using mock, event stream, error event' '
> + test_when_finished "rm trace.event actual expect" &&
> + GIT_TEST_TR2_SYSTEM_CONFIG="$MOCK" test-tool trace2 003error "hello world" "this is a test" &&
> + perl "$TEST_DIRECTORY/t0212/parse_events.perl" <trace.event >actual &&
> + sed -e "s/^|//" >expect <<-EOF &&
> + |VAR1 = {
> + | "_SID0_":{
> + | "argv":[
> + | "_EXE_",
> + | "trace2",
> + | "003error",
> + | "hello world",
> + | "this is a test"
> + | ],
> + | "errors":[
> + | "%s",
> + | "%s"
> + | ],
> + | "exit_code":0,
> + | "hierarchy":"trace2",
> + | "name":"trace2",
> + | "version":"$V"
> + | }
> + |};
> + EOF
> + test_cmp expect actual
> +'
> +
> test_done
> diff --git a/trace2.c b/trace2.c
> index 1c180062dd..490b3f071e 100644
> --- a/trace2.c
> +++ b/trace2.c
> @@ -10,6 +10,7 @@
> #include "trace2/tr2_cmd_name.h"
> #include "trace2/tr2_dst.h"
> #include "trace2/tr2_sid.h"
> +#include "trace2/tr2_sysenv.h"
> #include "trace2/tr2_tgt.h"
> #include "trace2/tr2_tls.h"
>
> @@ -120,6 +121,7 @@ static void tr2main_atexit_handler(void)
> tr2_sid_release();
> tr2_cmd_name_release();
> tr2_cfg_free_patterns();
> + tr2_sysenv_release();
>
> trace2_enabled = 0;
> }
> @@ -155,6 +157,8 @@ void trace2_initialize_fl(const char *file, int line)
> if (trace2_enabled)
> return;
>
> + tr2_sysenv_load();
> +
> if (!tr2_tgt_want_builtins())
> return;
> trace2_enabled = 1;
> diff --git a/trace2.h b/trace2.h
> index 8f89e70c44..cda8349058 100644
> --- a/trace2.h
> +++ b/trace2.h
> @@ -38,7 +38,8 @@ void trace2_initialize_clock(void);
>
> /*
> * Initialize TRACE2 tracing facility if any of the builtin TRACE2
> - * targets are enabled in the environment. Emits a 'version' event.
> + * targets are enabled in the system config or the environment.
> + * Emits a 'version' event.
> *
> * Cleanup/Termination is handled automatically by a registered
> * atexit() routine.
> @@ -125,8 +126,8 @@ void trace2_cmd_alias_fl(const char *file, int line, const char *alias,
> * Emit one or more 'def_param' events for "interesting" configuration
> * settings.
> *
> - * The environment variable "GIT_TR2_CONFIG_PARAMS" can be set to a
> - * list of patterns considered important. For example:
> + * Use the TR2_SYSENV_CFG_PARAM setting to register a list of patterns
> + * configured important. For example:
> *
> * GIT_TR2_CONFIG_PARAMS="core.*,remote.*.url"
> *
> diff --git a/trace2/tr2_cfg.c b/trace2/tr2_cfg.c
> index b329921ac5..caa7f06948 100644
> --- a/trace2/tr2_cfg.c
> +++ b/trace2/tr2_cfg.c
> @@ -1,8 +1,7 @@
> #include "cache.h"
> #include "config.h"
> -#include "tr2_cfg.h"
> -
> -#define TR2_ENVVAR_CFG_PARAM "GIT_TR2_CONFIG_PARAMS"
> +#include "trace2/tr2_cfg.h"
> +#include "trace2/tr2_sysenv.h"
>
> static struct strbuf **tr2_cfg_patterns;
> static int tr2_cfg_count_patterns;
> @@ -21,7 +20,7 @@ static int tr2_cfg_load_patterns(void)
> return tr2_cfg_count_patterns;
> tr2_cfg_loaded = 1;
>
> - envvar = getenv(TR2_ENVVAR_CFG_PARAM);
> + envvar = tr2_sysenv_get(TR2_SYSENV_CFG_PARAM);
> if (!envvar || !*envvar)
> return tr2_cfg_count_patterns;
>
> diff --git a/trace2/tr2_dst.c b/trace2/tr2_dst.c
> index fd490a43ad..575cd69aa9 100644
> --- a/trace2/tr2_dst.c
> +++ b/trace2/tr2_dst.c
> @@ -1,5 +1,6 @@
> #include "cache.h"
> #include "trace2/tr2_dst.h"
> +#include "trace2/tr2_sysenv.h"
>
> /*
> * If a Trace2 target cannot be opened for writing, we should issue a
> @@ -7,17 +8,13 @@
> * or socket and beyond the user's control -- especially since every
> * git command (and sub-command) will print the message. So we silently
> * eat these warnings and just discard the trace data.
> - *
> - * Enable the following environment variable to see these warnings.
> */
> -#define TR2_ENVVAR_DST_DEBUG "GIT_TR2_DST_DEBUG"
> -
> static int tr2_dst_want_warning(void)
> {
> static int tr2env_dst_debug = -1;
>
> if (tr2env_dst_debug == -1) {
> - const char *env_value = getenv(TR2_ENVVAR_DST_DEBUG);
> + const char *env_value = tr2_sysenv_get(TR2_SYSENV_DST_DEBUG);
> if (!env_value || !*env_value)
> tr2env_dst_debug = 0;
> else
> @@ -42,7 +39,9 @@ static int tr2_dst_try_path(struct tr2_dst *dst, const char *tgt_value)
> if (fd == -1) {
> if (tr2_dst_want_warning())
> warning("trace2: could not open '%s' for '%s' tracing: %s",
> - tgt_value, dst->env_var_name, strerror(errno));
> + tgt_value,
> + tr2_sysenv_display_name(dst->sysenv_var),
> + strerror(errno));
>
> tr2_dst_trace_disable(dst);
> return 0;
> @@ -116,7 +115,7 @@ static int tr2_dst_try_unix_domain_socket(struct tr2_dst *dst,
> if (!path || !*path) {
> if (tr2_dst_want_warning())
> warning("trace2: invalid AF_UNIX value '%s' for '%s' tracing",
> - tgt_value, dst->env_var_name);
> + tgt_value, tr2_sysenv_display_name(dst->sysenv_var));
>
> tr2_dst_trace_disable(dst);
> return 0;
> @@ -126,7 +125,7 @@ static int tr2_dst_try_unix_domain_socket(struct tr2_dst *dst,
> strlen(path) >= sizeof(((struct sockaddr_un *)0)->sun_path)) {
> if (tr2_dst_want_warning())
> warning("trace2: invalid AF_UNIX path '%s' for '%s' tracing",
> - path, dst->env_var_name);
> + path, tr2_sysenv_display_name(dst->sysenv_var));
>
> tr2_dst_trace_disable(dst);
> return 0;
> @@ -148,7 +147,7 @@ static int tr2_dst_try_unix_domain_socket(struct tr2_dst *dst,
> error:
> if (tr2_dst_want_warning())
> warning("trace2: could not connect to socket '%s' for '%s' tracing: %s",
> - path, dst->env_var_name, strerror(e));
> + path, tr2_sysenv_display_name(dst->sysenv_var), strerror(e));
>
> tr2_dst_trace_disable(dst);
> return 0;
> @@ -168,7 +167,7 @@ static void tr2_dst_malformed_warning(struct tr2_dst *dst,
> struct strbuf buf = STRBUF_INIT;
>
> strbuf_addf(&buf, "trace2: unknown value for '%s': '%s'",
> - dst->env_var_name, tgt_value);
> + tr2_sysenv_display_name(dst->sysenv_var), tgt_value);
> warning("%s", buf.buf);
>
> strbuf_release(&buf);
> @@ -184,7 +183,7 @@ int tr2_dst_get_trace_fd(struct tr2_dst *dst)
>
> dst->initialized = 1;
>
> - tgt_value = getenv(dst->env_var_name);
> + tgt_value = tr2_sysenv_get(dst->sysenv_var);
>
> if (!tgt_value || !strcmp(tgt_value, "") || !strcmp(tgt_value, "0") ||
> !strcasecmp(tgt_value, "false")) {
> @@ -246,7 +245,8 @@ void tr2_dst_write_line(struct tr2_dst *dst, struct strbuf *buf_line)
> return;
>
> if (tr2_dst_want_warning())
> - warning("unable to write trace to '%s': %s", dst->env_var_name,
> + warning("unable to write trace to '%s': %s",
> + tr2_sysenv_display_name(dst->sysenv_var),
> strerror(errno));
> tr2_dst_trace_disable(dst);
> }
> diff --git a/trace2/tr2_dst.h b/trace2/tr2_dst.h
> index 9a64f05b02..3adf3bac13 100644
> --- a/trace2/tr2_dst.h
> +++ b/trace2/tr2_dst.h
> @@ -2,9 +2,10 @@
> #define TR2_DST_H
>
> struct strbuf;
> +#include "trace2/tr2_sysenv.h"
>
> struct tr2_dst {
> - const char *const env_var_name;
> + enum tr2_sysenv_variable sysenv_var;
> int fd;
> unsigned int initialized : 1;
> unsigned int need_close : 1;
> diff --git a/trace2/tr2_sysenv.c b/trace2/tr2_sysenv.c
> new file mode 100644
> index 0000000000..656613e371
> --- /dev/null
> +++ b/trace2/tr2_sysenv.c
> @@ -0,0 +1,125 @@
> +#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.
> + * These strings are constant and must match the published names as
> + * described in the documentation.
> + *
> + * We do not define entries for the GIT_TR2_PARENT_* environment
> + * variables because they are transient and used to pass information
> + * from parent to child git processes, rather than settings.
> + */
> +/* clang-format off */
> +static struct tr2_sysenv_entry tr2_sysenv_settings[] = {
> + { "GIT_TR2_CONFIG_PARAMS", "trace2.configparams" },
> +
> + { "GIT_TR2_DST_DEBUG", "trace2.destinationdebug" },
> +
> + { "GIT_TR2", "trace2.normaltarget" },
> + { "GIT_TR2_BRIEF", "trace2.normalbrief" },
> +
> + { "GIT_TR2_EVENT", "trace2.eventtarget" },
> + { "GIT_TR2_EVENT_BRIEF", "trace2.eventbrief" },
> + { "GIT_TR2_EVENT_NESTING", "trace2.eventnesting" },
> +
> + { "GIT_TR2_PERF", "trace2.perftarget" },
> + { "GIT_TR2_PERF_BRIEF", "trace2.perfbrief" },
> +};
> +/* clang-format on */
> +
> +static int tr2_sysenv_cb(const char *key, const char *value, void *d)
> +{
> + int k;
> +
I added:
if (!starts_with(key, "trace2."))
return 0;
Here, and everything works as expected. I think that's a good
idea. Makes this O(n) over N config keys instead of O(n*x) where x = num
entries in tr2_sysenv_settings.
> + for (k = 0; k < ARRAY_SIZE(tr2_sysenv_settings); k++) {
> + if (!strcmp(key, tr2_sysenv_settings[k].git_config_name)) {
> + free(tr2_sysenv_settings[k].value);
> + tr2_sysenv_settings[k].value = xstrdup(value);
> + return 0;
> + }
> + }
> +
> + return 0;
> +}
> +
> +/*
> + * 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);
Maybe this isn't worth it, but this "file_exists" thing is something we
could abstract in the config machinery (or maybe passing via
"config_options" makes more sense):
diff --git a/config.c b/config.c
index 0f0cdd8c0f..ea625a508f 100644
--- a/config.c
+++ b/config.c
@@ -1549,12 +1549,15 @@ static int git_config_from_stdin(config_fn_t fn, void *data)
int git_config_from_file_with_options(config_fn_t fn, const char *filename,
void *data,
- const struct config_options *opts)
+ const struct config_options *opts,
+ int or_warn)
{
int ret = -1;
FILE *f;
- f = fopen_or_warn(filename, "r");
+ f = or_warn
+ ? fopen_or_warn(filename, "r")
+ : fopen(filename, "r");
if (f) {
ret = do_config_from_file(fn, CONFIG_ORIGIN_FILE, filename,
filename, f, data, opts);
@@ -1565,7 +1568,7 @@ int git_config_from_file_with_options(config_fn_t fn, const char *filename,
int git_config_from_file(config_fn_t fn, const char *filename, void *data)
{
- return git_config_from_file_with_options(fn, filename, data, NULL);
+ return git_config_from_file_with_options(fn, filename, data, NULL, 1);
}
int git_config_from_mem(config_fn_t fn,
@@ -2787,7 +2790,7 @@ int git_config_set_multivar_in_file_gently(const char *config_filename,
*/
if (git_config_from_file_with_options(store_aux,
config_filename,
- &store, &opts)) {
+ &store, &opts, 1)) {
error(_("invalid config file %s"), config_filename);
ret = CONFIG_INVALID_FILE;
goto out_free;
diff --git a/config.h b/config.h
index ee5d3fa7b4..52e93e6cdc 100644
--- a/config.h
+++ b/config.h
@@ -72,7 +72,8 @@ extern int git_default_config(const char *, const char *, void *);
extern int git_config_from_file(config_fn_t fn, const char *, void *);
extern int git_config_from_file_with_options(config_fn_t fn, const char *,
void *,
- const struct config_options *);
+ const struct config_options *,
+ int);
extern int git_config_from_mem(config_fn_t fn,
const enum config_origin_type,
const char *name,
diff --git a/trace2/tr2_sysenv.c b/trace2/tr2_sysenv.c
index 656613e371..25a9c19d20 100644
--- a/trace2/tr2_sysenv.c
+++ b/trace2/tr2_sysenv.c
@@ -78,9 +81,9 @@ void tr2_sysenv_load(void)
system_config_pathname = test_pathname;
}
- if (file_exists(system_config_pathname))
- git_config_from_file(tr2_sysenv_cb, system_config_pathname,
- NULL);
+ git_config_from_file_with_options(tr2_sysenv_cb,
+ system_config_pathname, NULL, NULL,
+ 0);
}
/*
> +}
> +
> +/*
> + * Return the value for the requested setting. Start with the /etc/gitconfig
> + * value and allow the corresponding environment variable to override it.
> + */
> +const char *tr2_sysenv_get(enum tr2_sysenv_variable var)
> +{
> + if (var >= TR2_SYSENV_MUST_BE_LAST)
> + BUG("tr2_sysenv_get invalid var '%d'", var);
> +
> + if (!tr2_sysenv_settings[var].getenv_called) {
> + const char *v = getenv(tr2_sysenv_settings[var].env_var_name);
> + if (v && *v) {
> + free(tr2_sysenv_settings[var].value);
> + tr2_sysenv_settings[var].value = xstrdup(v);
> + }
> + tr2_sysenv_settings[var].getenv_called = 1;
> + }
> +
> + return tr2_sysenv_settings[var].value;
> +}
> +
> +/*
> + * Return a friendly name for this setting that is suitable for printing
> + * in an error messages.
> + */
> +const char *tr2_sysenv_display_name(enum tr2_sysenv_variable var)
> +{
> + if (var >= TR2_SYSENV_MUST_BE_LAST)
> + BUG("tr2_sysenv_get invalid var '%d'", var);
> +
> + return tr2_sysenv_settings[var].env_var_name;
> +}
> +
> +void tr2_sysenv_release(void)
> +{
> + int k;
> +
> + for (k = 0; k < ARRAY_SIZE(tr2_sysenv_settings); k++)
> + free(tr2_sysenv_settings[k].value);
> +}
> diff --git a/trace2/tr2_sysenv.h b/trace2/tr2_sysenv.h
> new file mode 100644
> index 0000000000..369b20bd87
> --- /dev/null
> +++ b/trace2/tr2_sysenv.h
> @@ -0,0 +1,36 @@
> +#ifndef TR2_SYSENV_H
> +#define TR2_SYSENV_H
> +
> +/*
> + * The Trace2 settings that can be loaded from /etc/gitconfig
> + * and/or user environment variables.
> + *
> + * Note that this set does not contain any of the transient
> + * environment variables used to pass information from parent
> + * to child git processes, such "GIT_TR2_PARENT_SID".
> + */
> +enum tr2_sysenv_variable {
> + TR2_SYSENV_CFG_PARAM = 0,
> +
> + TR2_SYSENV_DST_DEBUG,
> +
> + TR2_SYSENV_NORMAL,
> + TR2_SYSENV_NORMAL_BRIEF,
> +
> + TR2_SYSENV_EVENT,
> + TR2_SYSENV_EVENT_BRIEF,
> + TR2_SYSENV_EVENT_NESTING,
> +
> + TR2_SYSENV_PERF,
> + TR2_SYSENV_PERF_BRIEF,
> +
> + TR2_SYSENV_MUST_BE_LAST
> +};
> +
> +void tr2_sysenv_load(void);
> +
> +const char *tr2_sysenv_get(enum tr2_sysenv_variable);
> +const char *tr2_sysenv_display_name(enum tr2_sysenv_variable var);
> +void tr2_sysenv_release(void);
> +
> +#endif /* TR2_SYSENV_H */
> diff --git a/trace2/tr2_tgt_event.c b/trace2/tr2_tgt_event.c
> index 89a4d3ae9a..bb6e323953 100644
> --- a/trace2/tr2_tgt_event.c
> +++ b/trace2/tr2_tgt_event.c
> @@ -6,10 +6,11 @@
> #include "trace2/tr2_dst.h"
> #include "trace2/tr2_tbuf.h"
> #include "trace2/tr2_sid.h"
> +#include "trace2/tr2_sysenv.h"
> #include "trace2/tr2_tgt.h"
> #include "trace2/tr2_tls.h"
>
> -static struct tr2_dst tr2dst_event = { "GIT_TR2_EVENT", 0, 0, 0 };
> +static struct tr2_dst tr2dst_event = { TR2_SYSENV_EVENT, 0, 0, 0 };
>
> /*
> * The version number of the JSON data generated by the EVENT target
> @@ -28,17 +29,15 @@ static struct tr2_dst tr2dst_event = { "GIT_TR2_EVENT", 0, 0, 0 };
> * are primarily intended for the performance target during debugging.
> *
> * Some of the outer-most messages, however, may be of interest to the
> - * event target. Set this environment variable to a larger integer for
> - * more detail in the event target.
> + * event target. Use the TR2_SYSENV_EVENT_NESTING setting to increase
> + * region details in the event target.
> */
> -#define TR2_ENVVAR_EVENT_NESTING "GIT_TR2_EVENT_NESTING"
> static int tr2env_event_nesting_wanted = 2;
>
> /*
> - * Set this environment variable to true to omit the <time>, <file>, and
> + * Use the TR2_SYSENV_EVENT_BRIEF to omit the <time>, <file>, and
> * <line> fields from most events.
> */
> -#define TR2_ENVVAR_EVENT_BRIEF "GIT_TR2_EVENT_BRIEF"
> static int tr2env_event_brief;
>
> static int fn_init(void)
> @@ -46,17 +45,17 @@ static int fn_init(void)
> int want = tr2_dst_trace_want(&tr2dst_event);
> int want_nesting;
> int want_brief;
> - char *nesting;
> - char *brief;
> + const char *nesting;
> + const char *brief;
>
> if (!want)
> return want;
>
> - nesting = getenv(TR2_ENVVAR_EVENT_NESTING);
> + nesting = tr2_sysenv_get(TR2_SYSENV_EVENT_NESTING);
> if (nesting && ((want_nesting = atoi(nesting)) > 0))
> tr2env_event_nesting_wanted = want_nesting;
>
> - brief = getenv(TR2_ENVVAR_EVENT_BRIEF);
> + brief = tr2_sysenv_get(TR2_SYSENV_EVENT_BRIEF);
> if (brief && ((want_brief = atoi(brief)) > 0))
> tr2env_event_brief = want_brief;
A lot of this pre-dates this patch, but I wonder if the whole of trace2
couldn't make more use of config.c's bool parsing for things like
these. Maybe by having a "cfg_type" enum & parsed_value void* in
tr2_sysenv_entry?
> diff --git a/trace2/tr2_tgt_normal.c b/trace2/tr2_tgt_normal.c
> index 57f3e18f5b..3364223805 100644
> --- a/trace2/tr2_tgt_normal.c
> +++ b/trace2/tr2_tgt_normal.c
> @@ -4,19 +4,19 @@
> #include "quote.h"
> #include "version.h"
> #include "trace2/tr2_dst.h"
> +#include "trace2/tr2_sysenv.h"
> #include "trace2/tr2_tbuf.h"
> #include "trace2/tr2_tgt.h"
> #include "trace2/tr2_tls.h"
>
> -static struct tr2_dst tr2dst_normal = { "GIT_TR2", 0, 0, 0 };
> +static struct tr2_dst tr2dst_normal = { TR2_SYSENV_NORMAL, 0, 0, 0 };
>
> /*
> - * Set this environment variable to true to omit the "<time> <file>:<line>"
> + * Use the TR2_SYSENV_NORMAL_BRIEF setting to omit the "<time> <file>:<line>"
> * fields from each line written to the builtin normal target.
> *
> * Unit tests may want to use this to help with testing.
> */
> -#define TR2_ENVVAR_NORMAL_BRIEF "GIT_TR2_BRIEF"
> static int tr2env_normal_brief;
>
> #define TR2FMT_NORMAL_FL_WIDTH (50)
> @@ -25,12 +25,12 @@ static int fn_init(void)
> {
> int want = tr2_dst_trace_want(&tr2dst_normal);
> int want_brief;
> - char *brief;
> + const char *brief;
>
> if (!want)
> return want;
>
> - brief = getenv(TR2_ENVVAR_NORMAL_BRIEF);
> + brief = tr2_sysenv_get(TR2_SYSENV_NORMAL_BRIEF);
> if (brief && *brief &&
> ((want_brief = git_parse_maybe_bool(brief)) != -1))
> tr2env_normal_brief = want_brief;
> diff --git a/trace2/tr2_tgt_perf.c b/trace2/tr2_tgt_perf.c
> index 9c3b4d8a0f..1ad781c32e 100644
> --- a/trace2/tr2_tgt_perf.c
> +++ b/trace2/tr2_tgt_perf.c
> @@ -6,19 +6,19 @@
> #include "json-writer.h"
> #include "trace2/tr2_dst.h"
> #include "trace2/tr2_sid.h"
> +#include "trace2/tr2_sysenv.h"
> #include "trace2/tr2_tbuf.h"
> #include "trace2/tr2_tgt.h"
> #include "trace2/tr2_tls.h"
>
> -static struct tr2_dst tr2dst_perf = { "GIT_TR2_PERF", 0, 0, 0 };
> +static struct tr2_dst tr2dst_perf = { TR2_SYSENV_PERF, 0, 0, 0 };
>
> /*
> - * Set this environment variable to true to omit the "<time> <file>:<line>"
> + * Use TR2_SYSENV_PERF_BRIEF to omit the "<time> <file>:<line>"
> * fields from each line written to the builtin performance target.
> *
> * Unit tests may want to use this to help with testing.
> */
> -#define TR2_ENVVAR_PERF_BRIEF "GIT_TR2_PERF_BRIEF"
> static int tr2env_perf_brief;
>
> #define TR2FMT_PERF_FL_WIDTH (50)
> @@ -36,14 +36,14 @@ static int fn_init(void)
> {
> int want = tr2_dst_trace_want(&tr2dst_perf);
> int want_brief;
> - char *brief;
> + const char *brief;
>
> if (!want)
> return want;
>
> strbuf_addchars(&dots, '.', TR2_DOTS_BUFFER_SIZE);
>
> - brief = getenv(TR2_ENVVAR_PERF_BRIEF);
> + brief = tr2_sysenv_get(TR2_SYSENV_PERF_BRIEF);
> if (brief && *brief &&
> ((want_brief = git_parse_maybe_bool(brief)) != -1))
> tr2env_perf_brief = want_brief;
