On 3/28/2019 10:36 AM, Ævar Arnfjörð Bjarmason wrote:
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 <jeffh...@microsoft.com>
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.
[...]
diff --git a/trace2/tr2_sysenv.c b/trace2/tr2_sysenv.c
new file mode 100644
index 0000000000..656613e371
--- /dev/null
[...]
+/* 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.
Good idea. Thanks!
+ 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):
[...]
This is a good idea, but I think I'll save this for a future effort
rather than add it to the current patch series. It just seems outside
of my scope right now and adds to the footprint of this series.
[...]
- 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?
I converted the "brief" instances in the normal and perf targets to
use git_parse_maybe_bool() already, but I missed this one.
The nesting one above is actually an integer value rather than a bool.
I'll rename the variables in the re-roll to clarify that.
[...]
- 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;
[...]
- 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;
Thanks for the review.
I'll push up another version shortly.
Jeff
