[PATCH] trace2: teach Git to log environment variables

[PATCH] trace2: teach Git to log environment variables

[Josh Steadmon]

Via trace2, Git can already log interesting config parameters (see the
trace2_cmd_list_config() function). However, this can grant an
incomplete picture because many config parameters also allow overrides
via environment variables.

To allow for more complete logs, we add a new trace2_cmd_list_env_vars()
function and supporting implementation, modeled after the pre-existing
config param logging implementation.

Signed-off-by: Josh Steadmon <steadmon@google.com>
---
As I mentioned in the commit message, I modeled this pretty closely on
the config parameter reporting code. It may make sense to split some of
this out into its own file, particularly the parts in trace2/tr2_cfg.c.
Alternatively, we could also just make the env var reporting part of the
config param reporting. Let me know if you have a preference either way.

 Documentation/config/trace2.txt        |  9 ++++
 Documentation/technical/api-trace2.txt |  3 +-
 git.c                                  |  3 ++
 t/helper/test-tool.c                   |  1 +
 t/t0212-trace2-event.sh                | 37 ++++++++++++++++
 trace2.c                               |  9 ++++
 trace2.h                               | 13 ++++++
 trace2/tr2_cfg.c                       | 58 ++++++++++++++++++++++++++
 trace2/tr2_cfg.h                       |  8 ++++
 trace2/tr2_sysenv.c                    |  2 +
 trace2/tr2_sysenv.h                    |  1 +
 11 files changed, 143 insertions(+), 1 deletion(-)

diff --git a/Documentation/config/trace2.txt b/Documentation/config/trace2.txt
index 4ce0b9a6d1..01d3afd8a8 100644
--- a/Documentation/config/trace2.txt
+++ b/Documentation/config/trace2.txt
@@ -48,6 +48,15 @@ trace2.configParams::
 	May be overridden by the `GIT_TRACE2_CONFIG_PARAMS` environment
 	variable.  Unset by default.
 
+trace2.envVars::
+	A comma-separated list of "important" environment variables that should
+	be recorded in the trace2 output.  For example,
+	`GIT_HTTP_USER_AGENT,GIT_CONFIG` would cause the trace2 output to
+	contain events listing the overrides for HTTP user agent and the
+	location of the Git configuration file (assuming any are set).  May be
+	overriden by the `GIT_TRACE2_ENV_VARS` environment variable.  Unset by
+	default.
+
 trace2.destinationDebug::
 	Boolean.  When true Git will print error messages when a
 	trace target destination cannot be opened for writing.
diff --git a/Documentation/technical/api-trace2.txt b/Documentation/technical/api-trace2.txt
index 4f07ceadcb..6b6085585d 100644
--- a/Documentation/technical/api-trace2.txt
+++ b/Documentation/technical/api-trace2.txt
@@ -656,7 +656,8 @@ The "exec_id" field is a command-unique id and is only useful if the
 ------------
 
 `"def_param"`::
-	This event is generated to log a global parameter.
+	This event is generated to log a global parameter, such as a config
+	setting, command-line flag, or environment variable.
 +
 ------------
 {
diff --git a/git.c b/git.c
index 7be7ad34bd..b7dbe82b0d 100644
--- a/git.c
+++ b/git.c
@@ -351,6 +351,7 @@ static int handle_alias(int *argcp, const char ***argv)
 
 			trace2_cmd_alias(alias_command, child.args.argv);
 			trace2_cmd_list_config();
+			trace2_cmd_list_env_vars();
 			trace2_cmd_name("_run_shell_alias_");
 
 			ret = run_command(&child);
@@ -388,6 +389,7 @@ static int handle_alias(int *argcp, const char ***argv)
 
 		trace2_cmd_alias(alias_command, new_argv);
 		trace2_cmd_list_config();
+		trace2_cmd_list_env_vars();
 
 		*argv = new_argv;
 		*argcp += count - 1;
@@ -439,6 +441,7 @@ static int run_builtin(struct cmd_struct *p, int argc, const char **argv)
 	trace_argv_printf(argv, "trace: built-in: git");
 	trace2_cmd_name(p->cmd);
 	trace2_cmd_list_config();
+	trace2_cmd_list_env_vars();
 
 	validate_cache_entries(the_repository->index);
 	status = p->fn(argc, argv, prefix);
diff --git a/t/helper/test-tool.c b/t/helper/test-tool.c
index c9a232d238..4cdab7eef2 100644
--- a/t/helper/test-tool.c
+++ b/t/helper/test-tool.c
@@ -111,6 +111,7 @@ int cmd_main(int argc, const char **argv)
 			argc--;
 			trace2_cmd_name(cmds[i].name);
 			trace2_cmd_list_config();
+			trace2_cmd_list_env_vars();
 			return cmds[i].fn(argc, argv);
 		}
 	}
diff --git a/t/t0212-trace2-event.sh b/t/t0212-trace2-event.sh
index 7065a1b937..1529155cf0 100755
--- a/t/t0212-trace2-event.sh
+++ b/t/t0212-trace2-event.sh
@@ -199,6 +199,43 @@ test_expect_success JSON_PP 'event stream, list config' '
 	test_cmp expect actual
 '
 
+# Test listing of all "interesting" environment variables.
+
+test_expect_success JSON_PP 'event stream, list env vars' '
+	test_when_finished "rm trace.event actual expect" &&
+	GIT_TRACE2_EVENT="$(pwd)/trace.event" \
+		GIT_TRACE2_ENV_VARS="A_VAR,OTHER_VAR,MISSING" \
+		A_VAR=1 OTHER_VAR="hello world" test-tool trace2 001return 0 &&
+	perl "$TEST_DIRECTORY/t0212/parse_events.perl" <trace.event >actual &&
+	sed -e "s/^|//" >expect <<-EOF &&
+	|VAR1 = {
+	|  "_SID0_":{
+	|    "argv":[
+	|      "_EXE_",
+	|      "trace2",
+	|      "001return",
+	|      "0"
+	|    ],
+	|    "exit_code":0,
+	|    "hierarchy":"trace2",
+	|    "name":"trace2",
+	|    "params":[
+	|      {
+	|        "param":"A_VAR",
+	|        "value":"1"
+	|      },
+	|      {
+	|        "param":"OTHER_VAR",
+	|        "value":"hello world"
+	|      }
+	|    ],
+	|    "version":"$V"
+	|  }
+	|};
+	EOF
+	test_cmp expect actual
+'
+
 test_expect_success JSON_PP 'basic trace2_data' '
 	test_when_finished "rm trace.event actual expect" &&
 	GIT_TRACE2_EVENT="$(pwd)/trace.event" test-tool trace2 006data test_category k1 v1 test_category k2 v2 &&
diff --git a/trace2.c b/trace2.c
index c7b4f14d29..2c6b570077 100644
--- a/trace2.c
+++ b/trace2.c
@@ -121,6 +121,7 @@ static void tr2main_atexit_handler(void)
 	tr2_sid_release();
 	tr2_cmd_name_release();
 	tr2_cfg_free_patterns();
+	tr2_cfg_free_env_vars();
 	tr2_sysenv_release();
 
 	trace2_enabled = 0;
@@ -311,6 +312,14 @@ void trace2_cmd_list_config_fl(const char *file, int line)
 	tr2_cfg_list_config_fl(file, line);
 }
 
+void trace2_cmd_list_env_vars_fl(const char *file, int line)
+{
+	if (!trace2_enabled)
+		return;
+
+	tr2_list_env_vars_fl(file, line);
+}
+
 void trace2_cmd_set_config_fl(const char *file, int line, const char *key,
 			      const char *value)
 {
diff --git a/trace2.h b/trace2.h
index e5e81c0533..b18bc5529c 100644
--- a/trace2.h
+++ b/trace2.h
@@ -182,6 +182,19 @@ void trace2_cmd_list_config_fl(const char *file, int line);
 
 #define trace2_cmd_list_config() trace2_cmd_list_config_fl(__FILE__, __LINE__)
 
+/*
+ * Emit one or more 'def_param' events for "important" environment variables.
+ *
+ * Use the TR2_SYSENV_ENV_VARS setting to register a comma-separated list of
+ * environment variables considered important.  For example:
+ *     git config --system trace2.envVars 'GIT_HTTP_USER_AGENT,GIT_CONFIG'
+ * or:
+ *     GIT_TRACE2_ENV_VARS="GIT_HTTP_USER_AGENT,GIT_CONFIG"
+ */
+void trace2_cmd_list_env_vars_fl(const char *file, int line);
+
+#define trace2_cmd_list_env_vars() trace2_cmd_list_env_vars_fl(__FILE__, __LINE__)
+
 /*
  * Emit a "def_param" event for the given config key/value pair IF
  * we consider the key to be "important".
diff --git a/trace2/tr2_cfg.c b/trace2/tr2_cfg.c
index caa7f06948..ec9ac1a6ef 100644
--- a/trace2/tr2_cfg.c
+++ b/trace2/tr2_cfg.c
@@ -7,6 +7,10 @@ static struct strbuf **tr2_cfg_patterns;
 static int tr2_cfg_count_patterns;
 static int tr2_cfg_loaded;
 
+static struct strbuf **tr2_cfg_env_vars;
+static int tr2_cfg_env_vars_count;
+static int tr2_cfg_env_vars_loaded;
+
 /*
  * Parse a string containing a comma-delimited list of config keys
  * or wildcard patterns into a list of strbufs.
@@ -46,6 +50,45 @@ void tr2_cfg_free_patterns(void)
 	tr2_cfg_loaded = 0;
 }
 
+/*
+ * Parse a string containing a comma-delimited list of environment variable
+ * names into a list of strbufs.
+ */
+static int tr2_load_env_vars(void)
+{
+	struct strbuf **s;
+	const char *varlist;
+
+	if (tr2_cfg_env_vars_loaded)
+		return tr2_cfg_env_vars_count;
+	tr2_cfg_env_vars_loaded = 1;
+
+	varlist = tr2_sysenv_get(TR2_SYSENV_ENV_VARS);
+	if (!varlist || !*varlist)
+		return tr2_cfg_env_vars_count;
+
+	tr2_cfg_env_vars = strbuf_split_buf(varlist, strlen(varlist), ',', -1);
+	for (s = tr2_cfg_env_vars; *s; s++) {
+		struct strbuf *buf = *s;
+
+		if (buf->len && buf->buf[buf->len - 1] == ',')
+			strbuf_setlen(buf, buf->len - 1);
+		strbuf_trim_trailing_newline(*s);
+		strbuf_trim(*s);
+	}
+
+	tr2_cfg_env_vars_count = s - tr2_cfg_env_vars;
+	return tr2_cfg_env_vars_count;
+}
+
+void tr2_cfg_free_env_vars(void)
+{
+	if (tr2_cfg_env_vars)
+		strbuf_list_free(tr2_cfg_env_vars);
+	tr2_cfg_env_vars_count = 0;
+	tr2_cfg_env_vars_loaded = 0;
+}
+
 struct tr2_cfg_data {
 	const char *file;
 	int line;
@@ -79,6 +122,21 @@ void tr2_cfg_list_config_fl(const char *file, int line)
 		read_early_config(tr2_cfg_cb, &data);
 }
 
+void tr2_list_env_vars_fl(const char *file, int line)
+{
+	struct strbuf **s;
+
+	if (tr2_load_env_vars() <= 0)
+		return;
+
+	for (s = tr2_cfg_env_vars; *s; s++) {
+		struct strbuf *buf = *s;
+		const char *val = getenv(buf->buf);
+		if (val && *val)
+			trace2_def_param_fl(file, line, buf->buf, val);
+	}
+}
+
 void tr2_cfg_set_fl(const char *file, int line, const char *key,
 		    const char *value)
 {
diff --git a/trace2/tr2_cfg.h b/trace2/tr2_cfg.h
index d9c98f64dd..a11d71feb5 100644
--- a/trace2/tr2_cfg.h
+++ b/trace2/tr2_cfg.h
@@ -7,6 +7,12 @@
  */
 void tr2_cfg_list_config_fl(const char *file, int line);
 
+/*
+ * Iterate over all "interesting" environment variables and emit 'def_param'
+ * events for them to TRACE2.
+ */
+void tr2_list_env_vars_fl(const char *file, int line);
+
 /*
  * Emit a "def_param" event for the given key/value pair IF we consider
  * the key to be "interesting".
@@ -16,4 +22,6 @@ void tr2_cfg_set_fl(const char *file, int line, const char *key,
 
 void tr2_cfg_free_patterns(void);
 
+void tr2_cfg_free_env_vars(void);
+
 #endif /* TR2_CFG_H */
diff --git a/trace2/tr2_sysenv.c b/trace2/tr2_sysenv.c
index 3c3792eca2..a380dcf910 100644
--- a/trace2/tr2_sysenv.c
+++ b/trace2/tr2_sysenv.c
@@ -29,6 +29,8 @@ struct tr2_sysenv_entry {
 static struct tr2_sysenv_entry tr2_sysenv_settings[] = {
 	[TR2_SYSENV_CFG_PARAM]     = { "GIT_TRACE2_CONFIG_PARAMS",
 				       "trace2.configparams" },
+	[TR2_SYSENV_ENV_VARS]      = { "GIT_TRACE2_ENV_VARS",
+				       "trace2.envvars" },
 
 	[TR2_SYSENV_DST_DEBUG]     = { "GIT_TRACE2_DST_DEBUG",
 				       "trace2.destinationdebug" },
diff --git a/trace2/tr2_sysenv.h b/trace2/tr2_sysenv.h
index d4364a7b85..3292ee15bc 100644
--- a/trace2/tr2_sysenv.h
+++ b/trace2/tr2_sysenv.h
@@ -11,6 +11,7 @@
  */
 enum tr2_sysenv_variable {
 	TR2_SYSENV_CFG_PARAM = 0,
+	TR2_SYSENV_ENV_VARS,
 
 	TR2_SYSENV_DST_DEBUG,
 
-- 
2.25.1.696.g5e7596f4ac-goog

Re: [PATCH] trace2: teach Git to log environment variables

[Junio C Hamano]

Josh Steadmon <steadmon@google.com> writes:

> +trace2.envVars::
> +	A comma-separated list of "important" environment variables that should
> +	be recorded in the trace2 output.  For example,
> +	`GIT_HTTP_USER_AGENT,GIT_CONFIG` would cause the trace2 output to
> +	contain events listing the overrides for HTTP user agent and the
> +	location of the Git configuration file (assuming any are set).  May be
> +	overriden by the `GIT_TRACE2_ENV_VARS` environment variable.  Unset by
> +	default.

In other words, by default nothing is logged?

>  		trace2_cmd_alias(alias_command, new_argv);
>  		trace2_cmd_list_config();
> +		trace2_cmd_list_env_vars();

OK, so we treat the settings of configuration variables and
environment variables pretty much the same.  Both affect how git
behaves for the end-users, and even though they are physically
different mechanisms, philosophically the reason they are worth
logging are the same for these two categories.

> @@ -439,6 +441,7 @@ static int run_builtin(struct cmd_struct *p, int argc, const char **argv)
>  	trace_argv_printf(argv, "trace: built-in: git");
>  	trace2_cmd_name(p->cmd);
>  	trace2_cmd_list_config();
> +	trace2_cmd_list_env_vars();

Likewise.  That is why these two appear together.

>  			trace2_cmd_name(cmds[i].name);
>  			trace2_cmd_list_config();
> +			trace2_cmd_list_env_vars();

And here.

> diff --git a/trace2/tr2_sysenv.c b/trace2/tr2_sysenv.c
> index 3c3792eca2..a380dcf910 100644
> --- a/trace2/tr2_sysenv.c
> +++ b/trace2/tr2_sysenv.c
> @@ -29,6 +29,8 @@ struct tr2_sysenv_entry {
>  static struct tr2_sysenv_entry tr2_sysenv_settings[] = {
>  	[TR2_SYSENV_CFG_PARAM]     = { "GIT_TRACE2_CONFIG_PARAMS",
>  				       "trace2.configparams" },
> +	[TR2_SYSENV_ENV_VARS]      = { "GIT_TRACE2_ENV_VARS",
> +				       "trace2.envvars" },
>  
>  	[TR2_SYSENV_DST_DEBUG]     = { "GIT_TRACE2_DST_DEBUG",
>  				       "trace2.destinationdebug" },

In this array, similar things are grouped together and groups are
separated by a blank line in between.  As the new ENV_VARS are
treated pretty much the same way as CFG_PARAM, it is thrown in the
same group, instead of becoming a group on its own.  Makes sense.


> diff --git a/trace2/tr2_sysenv.h b/trace2/tr2_sysenv.h
> index d4364a7b85..3292ee15bc 100644
> --- a/trace2/tr2_sysenv.h
> +++ b/trace2/tr2_sysenv.h
> @@ -11,6 +11,7 @@
>   */
>  enum tr2_sysenv_variable {
>  	TR2_SYSENV_CFG_PARAM = 0,
> +	TR2_SYSENV_ENV_VARS,
>  
>  	TR2_SYSENV_DST_DEBUG,

Likewise.

Thanks.

Re: [PATCH] trace2: teach Git to log environment variables

[Jeff Hostetler]

On 3/20/20 5:06 PM, Josh Steadmon wrote:
> Via trace2, Git can already log interesting config parameters (see the
> trace2_cmd_list_config() function). However, this can grant an
> incomplete picture because many config parameters also allow overrides
> via environment variables.
>
> To allow for more complete logs, we add a new trace2_cmd_list_env_vars()
> function and supporting implementation, modeled after the pre-existing
> config param logging implementation.
>
> Signed-off-by: Josh Steadmon <steadmon@google.com>
> ---
> As I mentioned in the commit message, I modeled this pretty closely on
> the config parameter reporting code. It may make sense to split some of
> this out into its own file, particularly the parts in trace2/tr2_cfg.c.
> Alternatively, we could also just make the env var reporting part of the
> config param reporting. Let me know if you have a preference either way.
>
Looks good to me.

Jeff

Re: [PATCH] trace2: teach Git to log environment variables

[Junio C Hamano]

Jeff Hostetler <git@jeffhostetler.com> writes:

>> As I mentioned in the commit message, I modeled this pretty closely on
>> the config parameter reporting code. It may make sense to split some of
>> this out into its own file, particularly the parts in trace2/tr2_cfg.c.
>> Alternatively, we could also just make the env var reporting part of the
>> config param reporting. Let me know if you have a preference either way.
>>
> Looks good to me.

Yup, thanks both.