{"thread":{"id":"53040","subject":"[PATCH] trace2: teach Git to log environment variables","startedAt":"2020-03-20T21:06:25Z","lastAt":"2020-03-23T20:15:43Z","messageCount":4,"participants":["Josh Steadmon","Junio C Hamano","Jeff Hostetler"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"393548","messageId":"0f5607a4242cc7b61ad36d0782c9d1250c4d4d7d.1584737973.git.steadmon@google.com","threadId":"53040","inReplyTo":null,"subject":"[PATCH] trace2: teach Git to log environment variables","fromName":"Josh Steadmon","fromEmail":"steadmon@google.com","sentAt":"2020-03-20T21:06:15Z","receivedAt":"2020-03-20T21:06:25Z","isPatch":true,"sender":{"key":"steadmon@google.com","avatar":"https://avatars.githubusercontent.com/u/2654920?v=4"},"body":"Via trace2, Git can already log interesting config parameters (see the\ntrace2_cmd_list_config() function). However, this can grant an\nincomplete picture because many config parameters also allow overrides\nvia environment variables.\n\nTo allow for more complete logs, we add a new trace2_cmd_list_env_vars()\nfunction and supporting implementation, modeled after the pre-existing\nconfig param logging implementation.\n\nSigned-off-by: Josh Steadmon <steadmon@google.com>\n---\nAs I mentioned in the commit message, I modeled this pretty closely on\nthe config parameter reporting code. It may make sense to split some of\nthis out into its own file, particularly the parts in trace2/tr2_cfg.c.\nAlternatively, we could also just make the env var reporting part of the\nconfig param reporting. Let me know if you have a preference either way.\n\n Documentation/config/trace2.txt        |  9 ++++\n Documentation/technical/api-trace2.txt |  3 +-\n git.c                                  |  3 ++\n t/helper/test-tool.c                   |  1 +\n t/t0212-trace2-event.sh                | 37 ++++++++++++++++\n trace2.c                               |  9 ++++\n trace2.h                               | 13 ++++++\n trace2/tr2_cfg.c                       | 58 ++++++++++++++++++++++++++\n trace2/tr2_cfg.h                       |  8 ++++\n trace2/tr2_sysenv.c                    |  2 +\n trace2/tr2_sysenv.h                    |  1 +\n 11 files changed, 143 insertions(+), 1 deletion(-)\n\ndiff --git a/Documentation/config/trace2.txt b/Documentation/config/trace2.txt\nindex 4ce0b9a6d1..01d3afd8a8 100644\n--- a/Documentation/config/trace2.txt\n+++ b/Documentation/config/trace2.txt\n@@ -48,6 +48,15 @@ trace2.configParams::\n \tMay be overridden by the `GIT_TRACE2_CONFIG_PARAMS` environment\n \tvariable.  Unset by default.\n \n+trace2.envVars::\n+\tA comma-separated list of \"important\" environment variables that should\n+\tbe recorded in the trace2 output.  For example,\n+\t`GIT_HTTP_USER_AGENT,GIT_CONFIG` would cause the trace2 output to\n+\tcontain events listing the overrides for HTTP user agent and the\n+\tlocation of the Git configuration file (assuming any are set).  May be\n+\toverriden by the `GIT_TRACE2_ENV_VARS` environment variable.  Unset by\n+\tdefault.\n+\n trace2.destinationDebug::\n \tBoolean.  When true Git will print error messages when a\n \ttrace target destination cannot be opened for writing.\ndiff --git a/Documentation/technical/api-trace2.txt b/Documentation/technical/api-trace2.txt\nindex 4f07ceadcb..6b6085585d 100644\n--- a/Documentation/technical/api-trace2.txt\n+++ b/Documentation/technical/api-trace2.txt\n@@ -656,7 +656,8 @@ The \"exec_id\" field is a command-unique id and is only useful if the\n ------------\n \n `\"def_param\"`::\n-\tThis event is generated to log a global parameter.\n+\tThis event is generated to log a global parameter, such as a config\n+\tsetting, command-line flag, or environment variable.\n +\n ------------\n {\ndiff --git a/git.c b/git.c\nindex 7be7ad34bd..b7dbe82b0d 100644\n--- a/git.c\n+++ b/git.c\n@@ -351,6 +351,7 @@ static int handle_alias(int *argcp, const char ***argv)\n \n \t\t\ttrace2_cmd_alias(alias_command, child.args.argv);\n \t\t\ttrace2_cmd_list_config();\n+\t\t\ttrace2_cmd_list_env_vars();\n \t\t\ttrace2_cmd_name(\"_run_shell_alias_\");\n \n \t\t\tret = run_command(&child);\n@@ -388,6 +389,7 @@ static int handle_alias(int *argcp, const char ***argv)\n \n \t\ttrace2_cmd_alias(alias_command, new_argv);\n \t\ttrace2_cmd_list_config();\n+\t\ttrace2_cmd_list_env_vars();\n \n \t\t*argv = new_argv;\n \t\t*argcp += count - 1;\n@@ -439,6 +441,7 @@ static int run_builtin(struct cmd_struct *p, int argc, const char **argv)\n \ttrace_argv_printf(argv, \"trace: built-in: git\");\n \ttrace2_cmd_name(p->cmd);\n \ttrace2_cmd_list_config();\n+\ttrace2_cmd_list_env_vars();\n \n \tvalidate_cache_entries(the_repository->index);\n \tstatus = p->fn(argc, argv, prefix);\ndiff --git a/t/helper/test-tool.c b/t/helper/test-tool.c\nindex c9a232d238..4cdab7eef2 100644\n--- a/t/helper/test-tool.c\n+++ b/t/helper/test-tool.c\n@@ -111,6 +111,7 @@ int cmd_main(int argc, const char **argv)\n \t\t\targc--;\n \t\t\ttrace2_cmd_name(cmds[i].name);\n \t\t\ttrace2_cmd_list_config();\n+\t\t\ttrace2_cmd_list_env_vars();\n \t\t\treturn cmds[i].fn(argc, argv);\n \t\t}\n \t}\ndiff --git a/t/t0212-trace2-event.sh b/t/t0212-trace2-event.sh\nindex 7065a1b937..1529155cf0 100755\n--- a/t/t0212-trace2-event.sh\n+++ b/t/t0212-trace2-event.sh\n@@ -199,6 +199,43 @@ test_expect_success JSON_PP 'event stream, list config' '\n \ttest_cmp expect actual\n '\n \n+# Test listing of all \"interesting\" environment variables.\n+\n+test_expect_success JSON_PP 'event stream, list env vars' '\n+\ttest_when_finished \"rm trace.event actual expect\" &&\n+\tGIT_TRACE2_EVENT=\"$(pwd)/trace.event\" \\\n+\t\tGIT_TRACE2_ENV_VARS=\"A_VAR,OTHER_VAR,MISSING\" \\\n+\t\tA_VAR=1 OTHER_VAR=\"hello world\" test-tool trace2 001return 0 &&\n+\tperl \"$TEST_DIRECTORY/t0212/parse_events.perl\" <trace.event >actual &&\n+\tsed -e \"s/^|//\" >expect <<-EOF &&\n+\t|VAR1 = {\n+\t|  \"_SID0_\":{\n+\t|    \"argv\":[\n+\t|      \"_EXE_\",\n+\t|      \"trace2\",\n+\t|      \"001return\",\n+\t|      \"0\"\n+\t|    ],\n+\t|    \"exit_code\":0,\n+\t|    \"hierarchy\":\"trace2\",\n+\t|    \"name\":\"trace2\",\n+\t|    \"params\":[\n+\t|      {\n+\t|        \"param\":\"A_VAR\",\n+\t|        \"value\":\"1\"\n+\t|      },\n+\t|      {\n+\t|        \"param\":\"OTHER_VAR\",\n+\t|        \"value\":\"hello world\"\n+\t|      }\n+\t|    ],\n+\t|    \"version\":\"$V\"\n+\t|  }\n+\t|};\n+\tEOF\n+\ttest_cmp expect actual\n+'\n+\n test_expect_success JSON_PP 'basic trace2_data' '\n \ttest_when_finished \"rm trace.event actual expect\" &&\n \tGIT_TRACE2_EVENT=\"$(pwd)/trace.event\" test-tool trace2 006data test_category k1 v1 test_category k2 v2 &&\ndiff --git a/trace2.c b/trace2.c\nindex c7b4f14d29..2c6b570077 100644\n--- a/trace2.c\n+++ b/trace2.c\n@@ -121,6 +121,7 @@ static void tr2main_atexit_handler(void)\n \ttr2_sid_release();\n \ttr2_cmd_name_release();\n \ttr2_cfg_free_patterns();\n+\ttr2_cfg_free_env_vars();\n \ttr2_sysenv_release();\n \n \ttrace2_enabled = 0;\n@@ -311,6 +312,14 @@ void trace2_cmd_list_config_fl(const char *file, int line)\n \ttr2_cfg_list_config_fl(file, line);\n }\n \n+void trace2_cmd_list_env_vars_fl(const char *file, int line)\n+{\n+\tif (!trace2_enabled)\n+\t\treturn;\n+\n+\ttr2_list_env_vars_fl(file, line);\n+}\n+\n void trace2_cmd_set_config_fl(const char *file, int line, const char *key,\n \t\t\t      const char *value)\n {\ndiff --git a/trace2.h b/trace2.h\nindex e5e81c0533..b18bc5529c 100644\n--- a/trace2.h\n+++ b/trace2.h\n@@ -182,6 +182,19 @@ void trace2_cmd_list_config_fl(const char *file, int line);\n \n #define trace2_cmd_list_config() trace2_cmd_list_config_fl(__FILE__, __LINE__)\n \n+/*\n+ * Emit one or more 'def_param' events for \"important\" environment variables.\n+ *\n+ * Use the TR2_SYSENV_ENV_VARS setting to register a comma-separated list of\n+ * environment variables considered important.  For example:\n+ *     git config --system trace2.envVars 'GIT_HTTP_USER_AGENT,GIT_CONFIG'\n+ * or:\n+ *     GIT_TRACE2_ENV_VARS=\"GIT_HTTP_USER_AGENT,GIT_CONFIG\"\n+ */\n+void trace2_cmd_list_env_vars_fl(const char *file, int line);\n+\n+#define trace2_cmd_list_env_vars() trace2_cmd_list_env_vars_fl(__FILE__, __LINE__)\n+\n /*\n  * Emit a \"def_param\" event for the given config key/value pair IF\n  * we consider the key to be \"important\".\ndiff --git a/trace2/tr2_cfg.c b/trace2/tr2_cfg.c\nindex caa7f06948..ec9ac1a6ef 100644\n--- a/trace2/tr2_cfg.c\n+++ b/trace2/tr2_cfg.c\n@@ -7,6 +7,10 @@ static struct strbuf **tr2_cfg_patterns;\n static int tr2_cfg_count_patterns;\n static int tr2_cfg_loaded;\n \n+static struct strbuf **tr2_cfg_env_vars;\n+static int tr2_cfg_env_vars_count;\n+static int tr2_cfg_env_vars_loaded;\n+\n /*\n  * Parse a string containing a comma-delimited list of config keys\n  * or wildcard patterns into a list of strbufs.\n@@ -46,6 +50,45 @@ void tr2_cfg_free_patterns(void)\n \ttr2_cfg_loaded = 0;\n }\n \n+/*\n+ * Parse a string containing a comma-delimited list of environment variable\n+ * names into a list of strbufs.\n+ */\n+static int tr2_load_env_vars(void)\n+{\n+\tstruct strbuf **s;\n+\tconst char *varlist;\n+\n+\tif (tr2_cfg_env_vars_loaded)\n+\t\treturn tr2_cfg_env_vars_count;\n+\ttr2_cfg_env_vars_loaded = 1;\n+\n+\tvarlist = tr2_sysenv_get(TR2_SYSENV_ENV_VARS);\n+\tif (!varlist || !*varlist)\n+\t\treturn tr2_cfg_env_vars_count;\n+\n+\ttr2_cfg_env_vars = strbuf_split_buf(varlist, strlen(varlist), ',', -1);\n+\tfor (s = tr2_cfg_env_vars; *s; s++) {\n+\t\tstruct strbuf *buf = *s;\n+\n+\t\tif (buf->len && buf->buf[buf->len - 1] == ',')\n+\t\t\tstrbuf_setlen(buf, buf->len - 1);\n+\t\tstrbuf_trim_trailing_newline(*s);\n+\t\tstrbuf_trim(*s);\n+\t}\n+\n+\ttr2_cfg_env_vars_count = s - tr2_cfg_env_vars;\n+\treturn tr2_cfg_env_vars_count;\n+}\n+\n+void tr2_cfg_free_env_vars(void)\n+{\n+\tif (tr2_cfg_env_vars)\n+\t\tstrbuf_list_free(tr2_cfg_env_vars);\n+\ttr2_cfg_env_vars_count = 0;\n+\ttr2_cfg_env_vars_loaded = 0;\n+}\n+\n struct tr2_cfg_data {\n \tconst char *file;\n \tint line;\n@@ -79,6 +122,21 @@ void tr2_cfg_list_config_fl(const char *file, int line)\n \t\tread_early_config(tr2_cfg_cb, &data);\n }\n \n+void tr2_list_env_vars_fl(const char *file, int line)\n+{\n+\tstruct strbuf **s;\n+\n+\tif (tr2_load_env_vars() <= 0)\n+\t\treturn;\n+\n+\tfor (s = tr2_cfg_env_vars; *s; s++) {\n+\t\tstruct strbuf *buf = *s;\n+\t\tconst char *val = getenv(buf->buf);\n+\t\tif (val && *val)\n+\t\t\ttrace2_def_param_fl(file, line, buf->buf, val);\n+\t}\n+}\n+\n void tr2_cfg_set_fl(const char *file, int line, const char *key,\n \t\t    const char *value)\n {\ndiff --git a/trace2/tr2_cfg.h b/trace2/tr2_cfg.h\nindex d9c98f64dd..a11d71feb5 100644\n--- a/trace2/tr2_cfg.h\n+++ b/trace2/tr2_cfg.h\n@@ -7,6 +7,12 @@\n  */\n void tr2_cfg_list_config_fl(const char *file, int line);\n \n+/*\n+ * Iterate over all \"interesting\" environment variables and emit 'def_param'\n+ * events for them to TRACE2.\n+ */\n+void tr2_list_env_vars_fl(const char *file, int line);\n+\n /*\n  * Emit a \"def_param\" event for the given key/value pair IF we consider\n  * the key to be \"interesting\".\n@@ -16,4 +22,6 @@ void tr2_cfg_set_fl(const char *file, int line, const char *key,\n \n void tr2_cfg_free_patterns(void);\n \n+void tr2_cfg_free_env_vars(void);\n+\n #endif /* TR2_CFG_H */\ndiff --git a/trace2/tr2_sysenv.c b/trace2/tr2_sysenv.c\nindex 3c3792eca2..a380dcf910 100644\n--- a/trace2/tr2_sysenv.c\n+++ b/trace2/tr2_sysenv.c\n@@ -29,6 +29,8 @@ struct tr2_sysenv_entry {\n static struct tr2_sysenv_entry tr2_sysenv_settings[] = {\n \t[TR2_SYSENV_CFG_PARAM]     = { \"GIT_TRACE2_CONFIG_PARAMS\",\n \t\t\t\t       \"trace2.configparams\" },\n+\t[TR2_SYSENV_ENV_VARS]      = { \"GIT_TRACE2_ENV_VARS\",\n+\t\t\t\t       \"trace2.envvars\" },\n \n \t[TR2_SYSENV_DST_DEBUG]     = { \"GIT_TRACE2_DST_DEBUG\",\n \t\t\t\t       \"trace2.destinationdebug\" },\ndiff --git a/trace2/tr2_sysenv.h b/trace2/tr2_sysenv.h\nindex d4364a7b85..3292ee15bc 100644\n--- a/trace2/tr2_sysenv.h\n+++ b/trace2/tr2_sysenv.h\n@@ -11,6 +11,7 @@\n  */\n enum tr2_sysenv_variable {\n \tTR2_SYSENV_CFG_PARAM = 0,\n+\tTR2_SYSENV_ENV_VARS,\n \n \tTR2_SYSENV_DST_DEBUG,\n \n-- \n2.25.1.696.g5e7596f4ac-goog\n\n"},{"id":"393550","messageId":"xmqqimiybu3r.fsf@gitster.c.googlers.com","threadId":"53040","inReplyTo":"0f5607a4242cc7b61ad36d0782c9d1250c4d4d7d.1584737973.git.steadmon@google.com","subject":"Re: [PATCH] trace2: teach Git to log environment variables","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-03-20T21:21:28Z","receivedAt":"2020-03-20T21:21:39Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Josh Steadmon <steadmon@google.com> writes:\n\n> +trace2.envVars::\n> +\tA comma-separated list of \"important\" environment variables that should\n> +\tbe recorded in the trace2 output.  For example,\n> +\t`GIT_HTTP_USER_AGENT,GIT_CONFIG` would cause the trace2 output to\n> +\tcontain events listing the overrides for HTTP user agent and the\n> +\tlocation of the Git configuration file (assuming any are set).  May be\n> +\toverriden by the `GIT_TRACE2_ENV_VARS` environment variable.  Unset by\n> +\tdefault.\n\nIn other words, by default nothing is logged?\n\n>  \t\ttrace2_cmd_alias(alias_command, new_argv);\n>  \t\ttrace2_cmd_list_config();\n> +\t\ttrace2_cmd_list_env_vars();\n\nOK, so we treat the settings of configuration variables and\nenvironment variables pretty much the same.  Both affect how git\nbehaves for the end-users, and even though they are physically\ndifferent mechanisms, philosophically the reason they are worth\nlogging are the same for these two categories.\n\n> @@ -439,6 +441,7 @@ static int run_builtin(struct cmd_struct *p, int argc, const char **argv)\n>  \ttrace_argv_printf(argv, \"trace: built-in: git\");\n>  \ttrace2_cmd_name(p->cmd);\n>  \ttrace2_cmd_list_config();\n> +\ttrace2_cmd_list_env_vars();\n\nLikewise.  That is why these two appear together.\n\n>  \t\t\ttrace2_cmd_name(cmds[i].name);\n>  \t\t\ttrace2_cmd_list_config();\n> +\t\t\ttrace2_cmd_list_env_vars();\n\nAnd here.\n\n> diff --git a/trace2/tr2_sysenv.c b/trace2/tr2_sysenv.c\n> index 3c3792eca2..a380dcf910 100644\n> --- a/trace2/tr2_sysenv.c\n> +++ b/trace2/tr2_sysenv.c\n> @@ -29,6 +29,8 @@ struct tr2_sysenv_entry {\n>  static struct tr2_sysenv_entry tr2_sysenv_settings[] = {\n>  \t[TR2_SYSENV_CFG_PARAM]     = { \"GIT_TRACE2_CONFIG_PARAMS\",\n>  \t\t\t\t       \"trace2.configparams\" },\n> +\t[TR2_SYSENV_ENV_VARS]      = { \"GIT_TRACE2_ENV_VARS\",\n> +\t\t\t\t       \"trace2.envvars\" },\n>  \n>  \t[TR2_SYSENV_DST_DEBUG]     = { \"GIT_TRACE2_DST_DEBUG\",\n>  \t\t\t\t       \"trace2.destinationdebug\" },\n\nIn this array, similar things are grouped together and groups are\nseparated by a blank line in between.  As the new ENV_VARS are\ntreated pretty much the same way as CFG_PARAM, it is thrown in the\nsame group, instead of becoming a group on its own.  Makes sense.\n\n\n> diff --git a/trace2/tr2_sysenv.h b/trace2/tr2_sysenv.h\n> index d4364a7b85..3292ee15bc 100644\n> --- a/trace2/tr2_sysenv.h\n> +++ b/trace2/tr2_sysenv.h\n> @@ -11,6 +11,7 @@\n>   */\n>  enum tr2_sysenv_variable {\n>  \tTR2_SYSENV_CFG_PARAM = 0,\n> +\tTR2_SYSENV_ENV_VARS,\n>  \n>  \tTR2_SYSENV_DST_DEBUG,\n\nLikewise.\n\nThanks.\n"},{"id":"393775","messageId":"aa4d22be-3710-1377-97aa-8ce8cfde0e20@jeffhostetler.com","threadId":"53040","inReplyTo":"0f5607a4242cc7b61ad36d0782c9d1250c4d4d7d.1584737973.git.steadmon@google.com","subject":"Re: [PATCH] trace2: teach Git to log environment variables","fromName":"Jeff Hostetler","fromEmail":"git@jeffhostetler.com","sentAt":"2020-03-23T15:28:16Z","receivedAt":"2020-03-23T15:28:21Z","isPatch":true,"sender":{"key":"git@jeffhostetler.com","avatar":null},"body":"\nOn 3/20/20 5:06 PM, Josh Steadmon wrote:\n> Via trace2, Git can already log interesting config parameters (see the\n> trace2_cmd_list_config() function). However, this can grant an\n> incomplete picture because many config parameters also allow overrides\n> via environment variables.\n>\n> To allow for more complete logs, we add a new trace2_cmd_list_env_vars()\n> function and supporting implementation, modeled after the pre-existing\n> config param logging implementation.\n>\n> Signed-off-by: Josh Steadmon <steadmon@google.com>\n> ---\n> As I mentioned in the commit message, I modeled this pretty closely on\n> the config parameter reporting code. It may make sense to split some of\n> this out into its own file, particularly the parts in trace2/tr2_cfg.c.\n> Alternatively, we could also just make the env var reporting part of the\n> config param reporting. Let me know if you have a preference either way.\n>\nLooks good to me.\n\nJeff\n\n\n"},{"id":"393809","messageId":"xmqqzhc64yl3.fsf@gitster.c.googlers.com","threadId":"53040","inReplyTo":"aa4d22be-3710-1377-97aa-8ce8cfde0e20@jeffhostetler.com","subject":"Re: [PATCH] trace2: teach Git to log environment variables","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-03-23T20:15:36Z","receivedAt":"2020-03-23T20:15:43Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff Hostetler <git@jeffhostetler.com> writes:\n\n>> As I mentioned in the commit message, I modeled this pretty closely on\n>> the config parameter reporting code. It may make sense to split some of\n>> this out into its own file, particularly the parts in trace2/tr2_cfg.c.\n>> Alternatively, we could also just make the env var reporting part of the\n>> config param reporting. Let me know if you have a preference either way.\n>>\n> Looks good to me.\n\nYup, thanks both.\n"}]}