Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

trace2: load trace2 settings from system config #169

Conversation

Projects
None yet
1 participant
@jeffhostetler
Copy link

commented Mar 26, 2019

Version 4 fixes a few clang-format warnings and simplifies the PID field in the SID.

Cc: gitster@pobox.com, peff@peff.net, jrnieder@gmail.com, steadmon@google.com, avarab@gmail.com

@jeffhostetler jeffhostetler force-pushed the jeffhostetler:core-tr2-startup-and-sysenv branch from 2dc5452 to 1865b52 Mar 26, 2019

@jeffhostetler

This comment has been minimized.

Copy link
Author

commented Mar 26, 2019

close and re-open to force a build

@jeffhostetler

This comment has been minimized.

Copy link
Author

commented Mar 26, 2019

re-open to force a build

@jeffhostetler jeffhostetler reopened this Mar 26, 2019

@jeffhostetler jeffhostetler changed the title Core tr2 startup and sysenv trace2: load trace2 settings from system config Mar 26, 2019

@jeffhostetler

This comment has been minimized.

Copy link
Author

commented Mar 26, 2019

trying again.

@jeffhostetler

This comment has been minimized.

Copy link
Author

commented Mar 26, 2019

re-opening to force another build

@jeffhostetler jeffhostetler reopened this Mar 26, 2019

@jeffhostetler jeffhostetler force-pushed the jeffhostetler:core-tr2-startup-and-sysenv branch 2 times, most recently from ae5bfdd to f34ef96 Mar 26, 2019

@jeffhostetler jeffhostetler force-pushed the jeffhostetler:core-tr2-startup-and-sysenv branch from a18cf3d to 7e0d4e2 Mar 28, 2019

@jeffhostetler

This comment has been minimized.

Copy link
Author

commented Mar 28, 2019

/submit

@gitgitgadget

This comment has been minimized.

Copy link

commented Mar 28, 2019

@@ -60,7 +60,7 @@ git version 2.20.1.155.g426c96fcdb
------------

This comment has been minimized.

Copy link
@gitgitgadget

gitgitgadget bot Mar 28, 2019

On the Git mailing list, =?utf-8?B?w4Z2YXIgQXJuZmrDtnLDsA==?= Bjarmason wrote (reply to this):


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@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.
>
> 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@microsoft.com>
> ---
>  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;

This comment has been minimized.

Copy link
@gitgitgadget

gitgitgadget bot Mar 28, 2019

On the Git mailing list, Jeff Hostetler wrote (reply to this):



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 <jeffhost@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

@jeffhostetler jeffhostetler force-pushed the jeffhostetler:core-tr2-startup-and-sysenv branch from 7e0d4e2 to 6af3907 Mar 28, 2019

@@ -60,7 +60,7 @@ git version 2.20.1.155.g426c96fcdb
------------

This comment has been minimized.

Copy link
@gitgitgadget

gitgitgadget bot Mar 28, 2019

On the Git mailing list, Josh Steadmon wrote (reply to this):

One quick comment:

On 2019.03.28 06:31, Jeff Hostetler via GitGitGadget wrote:
[...]
> 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"
>   *

Looks like the example needs to be updated with the new var name as
well.

@jeffhostetler jeffhostetler force-pushed the jeffhostetler:core-tr2-startup-and-sysenv branch 2 times, most recently from dd6161f to 4352952 Mar 29, 2019

@jeffhostetler

This comment has been minimized.

Copy link
Author

commented Mar 29, 2019

/submit

@gitgitgadget

This comment has been minimized.

Copy link

commented Mar 29, 2019

@@ -60,7 +60,7 @@ git version 2.20.1.155.g426c96fcdb
------------

This comment has been minimized.

Copy link
@gitgitgadget

gitgitgadget bot Mar 29, 2019

On the Git mailing list, =?utf-8?B?w4Z2YXIgQXJuZmrDtnLDsA==?= Bjarmason wrote (reply to this):


On Fri, Mar 29 2019, Jeff Hostetler via GitGitGadget wrote:

> From: Jeff Hostetler <jeffhost@microsoft.com>
>
> Update SID component construction to use the current UTC datetime
> and a portion of the SHA1 of the hostname.
>
> Use an simplified date/time format to make it easier to use the
> SID component as a logfile filename.
> [...]
> +static void tr2_sid_append_my_sid_component(void)
> +{
> +	const struct git_hash_algo *algo = &hash_algos[GIT_HASH_SHA1];
> +	struct tr2_tbuf tb_now;
> +	git_hash_ctx ctx;
> +	unsigned char hash[GIT_MAX_RAWSZ + 1];
> +	char hex[GIT_MAX_HEXSZ + 1];
> +	char hostname[HOST_NAME_MAX + 1];
> +
> +	tr2_tbuf_utc_datetime_for_filename(&tb_now);
> +	strbuf_addstr(&tr2sid_buf, tb_now.buf);
> +	strbuf_addch(&tr2sid_buf, '-');
> +
> +	if (xgethostname(hostname, sizeof(hostname)))
> +		xsnprintf(hostname, sizeof(hostname), "localhost");
> +
> +	algo->init_fn(&ctx);
> +	algo->update_fn(&ctx, hostname, strlen(hostname));
> +	algo->final_fn(hash, &ctx);
> +	hash_to_hex_algop_r(hex, hash, algo);
> +	strbuf_add(&tr2sid_buf, hex, 8);
> +
> +	strbuf_addch(&tr2sid_buf, '-');
> +	strbuf_addf(&tr2sid_buf, "%06"PRIuMAX, (uintmax_t)getpid());
> +}
> +

Thanks for turning my shitty half-formed idea into a patch :)

I wrote this on top to bikeshed this a bit further, wonder what you
think:
https://github.com/gitgitgadget/git/compare/pr-169/jeffhostetler/core-tr2-startup-and-sysenv-v2...avar:pr-169/jeffhostetler/core-tr2-startup-and-sysenv-v2

So e.g.:

    Before: 20190329-220413-446441-c2f5b994-018702
    After:  20190329T220431.244562Z-Hc2f5b994-P19812F

I.e:

 * Using <date>T<time> as is ISO 8601 convention/easier to read

 * <dateime>.<microseconds>Z, so seperating with "." to indicate it's
   the same value + add "Z" for "it's UTC". I'm least sure about the
   ".". Is that going to cause issues on Windows these days (the rest
   being the "extension"...).

 * I changed the hostname discovery so if gethostbyname() fails we'll
   print "-H00000000-" for that part, instead of "-H<first 8 chars of
   the sha1 for 'localhost'>-". Also prefix with "H" for "Host".

 * Wrap pids to 0xffff, prefix with "P" (Pid)" and trail with either "F"
   = Full or "W" = Wrapped (not the real PID).

 * I didn't add "T<datetime>" like "H" and "P" for the rest, since it's
   obvious what sort of value it is.

Maybe this is going a bit overboard, but I think it's easier to read at
a glance for humans, and since it's meant to be opaque to machines
anyway and the length is simliar enough not to matter I figure it's
worth it.

This comment has been minimized.

Copy link
@gitgitgadget

gitgitgadget bot Apr 1, 2019

On the Git mailing list, Jeff Hostetler wrote (reply to this):



On 3/29/2019 6:12 PM, Ævar Arnfjörð Bjarmason wrote:
> 
> On Fri, Mar 29 2019, Jeff Hostetler via GitGitGadget wrote:
> 
>> From: Jeff Hostetler <jeffhost@microsoft.com>
>>
>> Update SID component construction to use the current UTC datetime
>> and a portion of the SHA1 of the hostname.
>>
>> Use an simplified date/time format to make it easier to use the
>> SID component as a logfile filename.
>> [...]
>> +static void tr2_sid_append_my_sid_component(void)
>> +{
>> +	const struct git_hash_algo *algo = &hash_algos[GIT_HASH_SHA1];
>> +	struct tr2_tbuf tb_now;
>> +	git_hash_ctx ctx;
>> +	unsigned char hash[GIT_MAX_RAWSZ + 1];
>> +	char hex[GIT_MAX_HEXSZ + 1];
>> +	char hostname[HOST_NAME_MAX + 1];
>> +
>> +	tr2_tbuf_utc_datetime_for_filename(&tb_now);
>> +	strbuf_addstr(&tr2sid_buf, tb_now.buf);
>> +	strbuf_addch(&tr2sid_buf, '-');
>> +
>> +	if (xgethostname(hostname, sizeof(hostname)))
>> +		xsnprintf(hostname, sizeof(hostname), "localhost");
>> +
>> +	algo->init_fn(&ctx);
>> +	algo->update_fn(&ctx, hostname, strlen(hostname));
>> +	algo->final_fn(hash, &ctx);
>> +	hash_to_hex_algop_r(hex, hash, algo);
>> +	strbuf_add(&tr2sid_buf, hex, 8);
>> +
>> +	strbuf_addch(&tr2sid_buf, '-');
>> +	strbuf_addf(&tr2sid_buf, "%06"PRIuMAX, (uintmax_t)getpid());
>> +}
>> +
> 
> Thanks for turning my shitty half-formed idea into a patch :)
> 
> I wrote this on top to bikeshed this a bit further, wonder what you
> think:
> https://github.com/gitgitgadget/git/compare/pr-169/jeffhostetler/core-tr2-startup-and-sysenv-v2...avar:pr-169/jeffhostetler/core-tr2-startup-and-sysenv-v2
> 
> So e.g.:
> 
>      Before: 20190329-220413-446441-c2f5b994-018702
>      After:  20190329T220431.244562Z-Hc2f5b994-P19812F
> 
> I.e:
> 
>   * Using <date>T<time> as is ISO 8601 convention/easier to read
> 
>   * <dateime>.<microseconds>Z, so seperating with "." to indicate it's
>     the same value + add "Z" for "it's UTC". I'm least sure about the
>     ".". Is that going to cause issues on Windows these days (the rest
>     being the "extension"...).

I had a version that did just that.  I checked the various ISO and RFCs
and it seems like the fractional seconds is usually allowed between the
whole seconds and the "Z".

I've not seen any problems with that format.

I think I got spooked by your original suggestion to put the fraction in
a term after the whole "<date>T<time>Z" term.

I'll convert it back to match your suggestion.

> 
>   * I changed the hostname discovery so if gethostbyname() fails we'll
>     print "-H00000000-" for that part, instead of "-H<first 8 chars of
>     the sha1 for 'localhost'>-". Also prefix with "H" for "Host".

I like that.

> 
>   * Wrap pids to 0xffff, prefix with "P" (Pid)" and trail with either "F"
>     = Full or "W" = Wrapped (not the real PID).

I could see the "P".  I'm not sure about the hex -- I sometimes want to
do a "ps" or watch the processes go by in TaskManager and friends and
they all print the pid in decimal.  But it's not that big a deal.

> 
>   * I didn't add "T<datetime>" like "H" and "P" for the rest, since it's
>     obvious what sort of value it is.
> 
> Maybe this is going a bit overboard, but I think it's easier to read at
> a glance for humans, and since it's meant to be opaque to machines
> anyway and the length is simliar enough not to matter I figure it's
> worth it.
> 

I'll re-roll.
Thanks
Jeff

@@ -27,6 +27,8 @@ int main(int argc, const char **argv)
{

This comment has been minimized.

Copy link
@gitgitgadget

gitgitgadget bot Mar 29, 2019

On the Git mailing list, =?utf-8?B?w4Z2YXIgQXJuZmrDtnLDsA==?= Bjarmason wrote (reply to this):


On Fri, Mar 29 2019, Jeff Hostetler via GitGitGadget wrote:

> From: Jeff Hostetler <jeffhost@microsoft.com>
>
> Teach Windows version of git to report peak memory usage
> during exit() processing.
>
> Signed-off-by: Jeff Hostetler <jeffhost@microsoft.com>
> ---
>  common-main.c                            |  2 +-
>  compat/win32/trace2_win32_process_info.c | 50 ++++++++++++++++++++++--
>  trace2.c                                 |  2 +
>  trace2.h                                 | 14 +++++--
>  4 files changed, 60 insertions(+), 8 deletions(-)
>
> diff --git a/common-main.c b/common-main.c
> index 299ca62a72..582a7b1886 100644
> --- a/common-main.c
> +++ b/common-main.c
> @@ -41,7 +41,7 @@ int main(int argc, const char **argv)
>
>  	trace2_initialize();
>  	trace2_cmd_start(argv);
> -	trace2_collect_process_info();
> +	trace2_collect_process_info(TRACE2_PROCESS_INFO_STARTUP);
>
>  	git_setup_gettext();
>
> diff --git a/compat/win32/trace2_win32_process_info.c b/compat/win32/trace2_win32_process_info.c
> index 52bd62034b..2a514caed9 100644
> --- a/compat/win32/trace2_win32_process_info.c
> +++ b/compat/win32/trace2_win32_process_info.c
> @@ -1,5 +1,6 @@
>  #include "../../cache.h"
>  #include "../../json-writer.h"
> +#include "lazyload.h"
>  #include <Psapi.h>
>  #include <tlHelp32.h>
>
> @@ -137,11 +138,54 @@ static void get_is_being_debugged(void)
>  				   "windows/debugger_present", 1);
>  }
>
> -void trace2_collect_process_info(void)
> +/*
> + * Emit JSON data with the peak memory usage of the current process.
> + */
> +static void get_peak_memory_info(void)
> +{
> +	DECLARE_PROC_ADDR(psapi.dll, BOOL, GetProcessMemoryInfo,
> +			  HANDLE, PPROCESS_MEMORY_COUNTERS, DWORD);
> +
> +	if (INIT_PROC_ADDR(GetProcessMemoryInfo)) {
> +		PROCESS_MEMORY_COUNTERS pmc;
> +
> +		if (GetProcessMemoryInfo(GetCurrentProcess(), &pmc,
> +					 sizeof(pmc))) {
> +			struct json_writer jw = JSON_WRITER_INIT;
> +
> +			jw_object_begin(&jw, 0);
> +
> +#define KV(kv) #kv, (intmax_t)pmc.kv
> +
> +			jw_object_intmax(&jw, KV(PageFaultCount));
> +			jw_object_intmax(&jw, KV(PeakWorkingSetSize));
> +			jw_object_intmax(&jw, KV(PeakPagefileUsage));
> +
> +			jw_end(&jw);
> +
> +			trace2_data_json("process", the_repository,
> +					 "windows/memory", &jw);
> +			jw_release(&jw);
> +		}
> +	}
> +}
> +
> +void trace2_collect_process_info(enum trace2_process_info_reason reason)
>  {
>  	if (!trace2_is_enabled())
>  		return;
>
> -	get_is_being_debugged();
> -	get_ancestry();
> +	switch (reason) {
> +	case TRACE2_PROCESS_INFO_STARTUP:
> +		get_is_being_debugged();
> +		get_ancestry();
> +		return;
> +
> +	case TRACE2_PROCESS_INFO_EXIT:
> +		get_peak_memory_info();
> +		return;
> +
> +	default:
> +		BUG("trace2_collect_process_info: unknown reason '%d'", reason);
> +	}
>  }
> diff --git a/trace2.c b/trace2.c
> index 490b3f071e..6baa65cdf9 100644
> --- a/trace2.c
> +++ b/trace2.c
> @@ -213,6 +213,8 @@ int trace2_cmd_exit_fl(const char *file, int line, int code)
>  	if (!trace2_enabled)
>  		return code;
>
> +	trace2_collect_process_info(TRACE2_PROCESS_INFO_EXIT);
> +
>  	tr2main_exit_code = code;
>
>  	us_now = getnanotime() / 1000;
> diff --git a/trace2.h b/trace2.h
> index 894bfca7e0..888531eb08 100644
> --- a/trace2.h
> +++ b/trace2.h
> @@ -391,13 +391,19 @@ void trace2_printf(const char *fmt, ...);
>   * Optional platform-specific code to dump information about the
>   * current and any parent process(es).  This is intended to allow
>   * post-processors to know who spawned this git instance and anything
> - * else the platform may be able to tell us about the current process.
> + * else that the platform may be able to tell us about the current process.
>   */
> +
> +enum trace2_process_info_reason {
> +	TRACE2_PROCESS_INFO_STARTUP,
> +	TRACE2_PROCESS_INFO_EXIT,
> +};
> +
>  #if defined(GIT_WINDOWS_NATIVE)
> -void trace2_collect_process_info(void);
> +void trace2_collect_process_info(enum trace2_process_info_reason reason);
>  #else
> -#define trace2_collect_process_info() \
> -	do {                          \
> +#define trace2_collect_process_info(reason) \
> +	do {                                \
>  	} while (0)
>  #endif

FWIW this is the "VmPeak" line in /proc/$$/status on Linux. I don't know
if we can/should parse that in practice (I've been bitten in the past by
things in /proc having high cost on some kernel versions in the past,
notably smaps).

This comment has been minimized.

Copy link
@gitgitgadget

gitgitgadget bot Apr 1, 2019

On the Git mailing list, Jeff Hostetler wrote (reply to this):



On 3/29/2019 6:16 PM, Ævar Arnfjörð Bjarmason wrote:
> 
> On Fri, Mar 29 2019, Jeff Hostetler via GitGitGadget wrote:
> 
>> From: Jeff Hostetler <jeffhost@microsoft.com>
>>
>> Teach Windows version of git to report peak memory usage
>> during exit() processing.
>>
>> Signed-off-by: Jeff Hostetler <jeffhost@microsoft.com>
>> ---
>>   common-main.c                            |  2 +-
>>   compat/win32/trace2_win32_process_info.c | 50 ++++++++++++++++++++++--
>>   trace2.c                                 |  2 +
>>   trace2.h                                 | 14 +++++--
>>   4 files changed, 60 insertions(+), 8 deletions(-)
>>
>> diff --git a/common-main.c b/common-main.c
>> index 299ca62a72..582a7b1886 100644
>> --- a/common-main.c
>> +++ b/common-main.c
>> @@ -41,7 +41,7 @@ int main(int argc, const char **argv)
>>
>>   	trace2_initialize();
>>   	trace2_cmd_start(argv);
>> -	trace2_collect_process_info();
>> +	trace2_collect_process_info(TRACE2_PROCESS_INFO_STARTUP);
>>
>>   	git_setup_gettext();
>>
>> diff --git a/compat/win32/trace2_win32_process_info.c b/compat/win32/trace2_win32_process_info.c
>> index 52bd62034b..2a514caed9 100644
>> --- a/compat/win32/trace2_win32_process_info.c
>> +++ b/compat/win32/trace2_win32_process_info.c
>> @@ -1,5 +1,6 @@
>>   #include "../../cache.h"
>>   #include "../../json-writer.h"
>> +#include "lazyload.h"
>>   #include <Psapi.h>
>>   #include <tlHelp32.h>
>>
>> @@ -137,11 +138,54 @@ static void get_is_being_debugged(void)
>>   				   "windows/debugger_present", 1);
>>   }
>>
>> -void trace2_collect_process_info(void)
>> +/*
>> + * Emit JSON data with the peak memory usage of the current process.
>> + */
>> +static void get_peak_memory_info(void)
>> +{
>> +	DECLARE_PROC_ADDR(psapi.dll, BOOL, GetProcessMemoryInfo,
>> +			  HANDLE, PPROCESS_MEMORY_COUNTERS, DWORD);
>> +
>> +	if (INIT_PROC_ADDR(GetProcessMemoryInfo)) {
>> +		PROCESS_MEMORY_COUNTERS pmc;
>> +
>> +		if (GetProcessMemoryInfo(GetCurrentProcess(), &pmc,
>> +					 sizeof(pmc))) {
>> +			struct json_writer jw = JSON_WRITER_INIT;
>> +
>> +			jw_object_begin(&jw, 0);
>> +
>> +#define KV(kv) #kv, (intmax_t)pmc.kv
>> +
>> +			jw_object_intmax(&jw, KV(PageFaultCount));
>> +			jw_object_intmax(&jw, KV(PeakWorkingSetSize));
>> +			jw_object_intmax(&jw, KV(PeakPagefileUsage));
>> +
>> +			jw_end(&jw);
>> +
>> +			trace2_data_json("process", the_repository,
>> +					 "windows/memory", &jw);
>> +			jw_release(&jw);
>> +		}
>> +	}
>> +}
>> +
>> +void trace2_collect_process_info(enum trace2_process_info_reason reason)
>>   {
>>   	if (!trace2_is_enabled())
>>   		return;
>>
>> -	get_is_being_debugged();
>> -	get_ancestry();
>> +	switch (reason) {
>> +	case TRACE2_PROCESS_INFO_STARTUP:
>> +		get_is_being_debugged();
>> +		get_ancestry();
>> +		return;
>> +
>> +	case TRACE2_PROCESS_INFO_EXIT:
>> +		get_peak_memory_info();
>> +		return;
>> +
>> +	default:
>> +		BUG("trace2_collect_process_info: unknown reason '%d'", reason);
>> +	}
>>   }
>> diff --git a/trace2.c b/trace2.c
>> index 490b3f071e..6baa65cdf9 100644
>> --- a/trace2.c
>> +++ b/trace2.c
>> @@ -213,6 +213,8 @@ int trace2_cmd_exit_fl(const char *file, int line, int code)
>>   	if (!trace2_enabled)
>>   		return code;
>>
>> +	trace2_collect_process_info(TRACE2_PROCESS_INFO_EXIT);
>> +
>>   	tr2main_exit_code = code;
>>
>>   	us_now = getnanotime() / 1000;
>> diff --git a/trace2.h b/trace2.h
>> index 894bfca7e0..888531eb08 100644
>> --- a/trace2.h
>> +++ b/trace2.h
>> @@ -391,13 +391,19 @@ void trace2_printf(const char *fmt, ...);
>>    * Optional platform-specific code to dump information about the
>>    * current and any parent process(es).  This is intended to allow
>>    * post-processors to know who spawned this git instance and anything
>> - * else the platform may be able to tell us about the current process.
>> + * else that the platform may be able to tell us about the current process.
>>    */
>> +
>> +enum trace2_process_info_reason {
>> +	TRACE2_PROCESS_INFO_STARTUP,
>> +	TRACE2_PROCESS_INFO_EXIT,
>> +};
>> +
>>   #if defined(GIT_WINDOWS_NATIVE)
>> -void trace2_collect_process_info(void);
>> +void trace2_collect_process_info(enum trace2_process_info_reason reason);
>>   #else
>> -#define trace2_collect_process_info() \
>> -	do {                          \
>> +#define trace2_collect_process_info(reason) \
>> +	do {                                \
>>   	} while (0)
>>   #endif
> 
> FWIW this is the "VmPeak" line in /proc/$$/status on Linux. I don't know
> if we can/should parse that in practice (I've been bitten in the past by
> things in /proc having high cost on some kernel versions in the past,
> notably smaps).
> 

Thanks,

Yeah, I've avoided adding the corresponding platform-specific changes
in for non-Windows versions because I didn't want to venture off into
the weeds and introduce a perf or functional problem (for something
that I'm not going to make use of in the short term).  Whereas, we're
seeing occasional perf problems with our Windows users on the OS repo
where some (yet to be determined) commands will just go out to lunch.

I suspect we'll have similar problems with out Mac users in the near
future, but I didn't want to jump in there just yet.

Jeff



@@ -60,7 +60,7 @@ git version 2.20.1.155.g426c96fcdb
------------

This comment has been minimized.

Copy link
@gitgitgadget

gitgitgadget bot Apr 1, 2019

On the Git mailing list, Josh Steadmon wrote (reply to this):

On 2019.03.29 10:04, Jeff Hostetler via GitGitGadget wrote:
[...]
> 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
> +---------------------------------------------------
> +....
> +
> +

A question for the list: should these new config vars also be documented
in the git-config manpage, or is it better to keep these separate since
they are only read from the system config?

This comment has been minimized.

Copy link
@gitgitgadget

gitgitgadget bot Apr 1, 2019

On the Git mailing list, Jeff Hostetler wrote (reply to this):



On 4/1/2019 5:00 PM, Josh Steadmon wrote:
> On 2019.03.29 10:04, Jeff Hostetler via GitGitGadget wrote:
> [...]
>> 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
>> +---------------------------------------------------
>> +....
>> +
>> +
> 
> A question for the list: should these new config vars also be documented
> in the git-config manpage, or is it better to keep these separate since
> they are only read from the system config?
> 

I wasn't sure either, so I just kept them in the trace2 api doc for now.
Jeff

This comment has been minimized.

Copy link
@gitgitgadget

gitgitgadget bot Apr 3, 2019

On the Git mailing list, Jonathan Nieder wrote (reply to this):

Josh Steadmon wrote:

> A question for the list: should these new config vars also be documented
> in the git-config manpage, or is it better to keep these separate since
> they are only read from the system config?

Yes, they should be in the git-config manpage.

*If* they are read only from the system config, the documentation
there should say so.

Thanks,
Jonathan
@gitgitgadget

This comment has been minimized.

Copy link

commented Apr 1, 2019

On the Git mailing list, Josh Steadmon wrote (reply to this):

On 2019.03.29 10:04, Jeff Hostetler via GitGitGadget wrote:
> Here is version 2. It addresses most the V1 comments WRT the system config
> changes.
> 
> It also addresses the format and uniqueness of the SID as discussed in [1].
> The SID now containes: the UTC date/time, part of SHA1 of the hostname, and
> the PID and is formatted to make it safe for filenames.
> 
> It also contains (a somewhat unrelated platform-specific) commit to report
> total process memory usage at exit. This is helpful when looking for
> problematic commands that might have scaling problems.
> 
> [1] 
> https://public-inbox.org/git/51e88650-8667-df1f-13ef-4537f2e70346@jeffhostetler.com/T/#m6b4e6f2b0374d5ba88de8d0350ce6bf51b28d7da
> 
> 
> ----------------------------------------------------------------------------
> 
> Teach git to load default Trace2 settings from the system config (usually
> "/etc/gitconfig"). The existing GIT_TR2_* environment variables can be used
> to override the new system defaults. It also includes a little startup
> refactoring.
> 
> Note: I found interactive testing of this feature to be awkward on some
> platforms because of the use of prefix- or runtime-prefix-relative locations
> for the system configuration. It was easy to accidentally use an officially
> installed version of git to set a system config variable in the official
> system config directory; and then when testing with the test version of git,
> that value would not be seen because it was looking for the system config
> file in a different directory.
> 
> Jeff Hostetler (7):
>   trace2: refactor setting process starting time
>   trace2: add absolute elapsed time to start event
>   trace2: find exec-dir before trace2 initialization
>   trace2: use system config for default trace2 settings
>   trace2: report peak memory usage of the process
>   trace2: clarify UTC datetime formatting
>   trace2: make SIDs more unique
> 
>  Documentation/technical/api-trace2.txt   |  66 +++++++++---
>  Makefile                                 |   1 +
>  common-main.c                            |   8 +-
>  compat/mingw.c                           |   2 +
>  compat/win32/trace2_win32_process_info.c |  50 ++++++++-
>  t/t0210-trace2-normal.sh                 |  41 +++++++-
>  t/t0211-trace2-perf.sh                   |  53 ++++++++--
>  t/t0212-trace2-event.sh                  |  52 ++++++++-
>  trace2.c                                 |  21 +++-
>  trace2.h                                 |  43 ++++++--
>  trace2/tr2_cfg.c                         |   7 +-
>  trace2/tr2_dst.c                         |  24 ++---
>  trace2/tr2_dst.h                         |   3 +-
>  trace2/tr2_sid.c                         |  39 ++++++-
>  trace2/tr2_sysenv.c                      | 128 +++++++++++++++++++++++
>  trace2/tr2_sysenv.h                      |  36 +++++++
>  trace2/tr2_tbuf.c                        |  20 +++-
>  trace2/tr2_tbuf.h                        |   5 +-
>  trace2/tr2_tgt.h                         |   1 +
>  trace2/tr2_tgt_event.c                   |  53 +++++-----
>  trace2/tr2_tgt_normal.c                  |  19 ++--
>  trace2/tr2_tgt_perf.c                    |  23 ++--
>  trace2/tr2_tls.c                         |  38 ++++---
>  trace2/tr2_tls.h                         |   8 +-
>  24 files changed, 604 insertions(+), 137 deletions(-)
>  create mode 100644 trace2/tr2_sysenv.c
>  create mode 100644 trace2/tr2_sysenv.h
> 
> 
> base-commit: 041f5ea1cf987a4068ef5f39ba0a09be85952064
> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-169%2Fjeffhostetler%2Fcore-tr2-startup-and-sysenv-v2
> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-169/jeffhostetler/core-tr2-startup-and-sysenv-v2
> Pull-Request: https://github.com/gitgitgadget/git/pull/169

This series looks good to me, apart from the open question about
documentation. Thanks!

Reviewed-by: Josh Steadmon <steadmon@google.com>
@@ -60,7 +60,7 @@ git version 2.20.1.155.g426c96fcdb
------------

This comment has been minimized.

Copy link
@gitgitgadget

gitgitgadget bot Apr 3, 2019

On the Git mailing list, Jonathan Nieder wrote (reply to this):

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

This comment has been minimized.

Copy link
@gitgitgadget

gitgitgadget bot Apr 9, 2019

On the Git mailing list, Jeff Hostetler wrote (reply to this):



On 4/2/2019 8:00 PM, Jonathan Nieder wrote:
> 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?

Several thoughts here.  Some are performance-related and some are
startup chicken-n-egg problems.  I tried to add trace2 to the code
base in the least disruptive way possible and hopefully with minimal
side-effects on existing behaviors.  And also minimum impact on overall
performance when not in use.

So, with that in mind:

[] Trace2 should be initialized as early as and lightly as possible
    so that timers are started and it can capture events from other
    startup activities.  And so that we can tell as early as possible
    if trace2 should NOT be enabled and short-cut those calls and not
    waste time collecting unnecessary data.

[] Trace2 is initialized by calls from common-main.c:main().  This
    happens before cmd_main() is called, so we are very early in the
    startup process and very few things have been initialized.  For
    example, I had to let git_resolve_executable_dir() run so that
    we could find the location of the system config, but that's about
    it.  We have not even initialized "the_repository" yet.

[] WRT "-c var=val", this is processed by git.c:handle_options() and
    called from git.c:cmd_main() and git.c:handle_alias().  So the "-c"
    args won't be respected until after that point.

    Adding trace2 initialization after that point is a bit of a mess.
    Teaching trace2 to scan the argv for "-c" is just duplicating effort.
    And moving the "-c" processing earlier in the startup, changes the
    behavior of the "git-*" commands (which don't currently recognize
    the "-c" option).

    So that's why I'm suggesting we not respect "-c" for this.

[] By initializing trace2 inside main() we guarantee that it will get
    started.  If we wait to initialize it until after handle_options()
    returns, we miss events for commands that it handles itself (such
    as "git --exec-path" where it just prints and exits or syntax errors
    where it directly calls usage() and exits).

[] WRT to per-repo config values:

    In common-main.c:main() we have not yet discovered the .git dir,
    so repo-local config files are questionable at a very early point
    in the process startup.  Again, it comes down to whether we wait
    for gitdir discovery (and whatever file system scanning, --gitdir,
    or -C processing is required) before deciding whether to start
    trace2 or not.

    Also, per-repo trace2 config settings aren't available at the time
    of the clone, so just relying on them will leave our telemetry
    incomplete.  It's better to have a system (or maybe a user) setting
    and not a per-repo setting.

    Longer term, with submodules and plans to support them in-proc rather
    than via sub-processes, I don't think there should be any expectation
    that trace2 settings would adapt during recursive operations.  So for
    example, a top-level command might see different trace2 settings than
    a command run inside a submodule.  If submodule operations become
    in-proc, then to maintain that expected behavior we'd need to have
    per-repo trace2 settings.  (Granted, not impossible, but by saying no
    to that now, we can eliminate a possible pain point in the future.)

[] i'll add more on this topic at the bottom of this note in response to
    your per-user and includes questions.


> [...]
>> --- 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?

I didn't specifically intend to support or not-support include files
here.  Frankly, the complexity of the config code makes my eyes bleed.
I'll investigate adding it.


> 
> [...]
>> --- 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.

Thanks for the pointer.  I'll update the tr2_sysenv_load() to check
git_system_config() before trying to use the system config file.

> 
> So this would be easier to test if we can use user-level config for
> it.

I suppose.  I think it just moves the testing problem to a trick using
$HOME, right?  But it does get rid of that GIT_TEST_TR2_SYSTEM_CONFIG
symbol, so yeah a net win.  I'll investigate.  Thanks.


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

Cool.  I hadn't seen that syntax before.

> 
> [...]
>> +/*
>> + * 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.

read_early_config() might work, but it is pretty heavy and doesn't
address my above comments.

Perhaps a compromise would be to have it read just the system and
user config files (w/ includes turned on).  It shouldn't take too
much time and it shouldn't have the startup-order dependencies, I
think.

Thoughts??

> 
> Thanks,
> Jonathan
> 

Thanks,
Jeff

@jeffhostetler jeffhostetler force-pushed the jeffhostetler:core-tr2-startup-and-sysenv branch 2 times, most recently from cf2494c to 1da5af8 Apr 8, 2019

@jeffhostetler jeffhostetler force-pushed the jeffhostetler:core-tr2-startup-and-sysenv branch from 626ace5 to 3414016 Apr 15, 2019

@jeffhostetler

This comment has been minimized.

Copy link
Author

commented Apr 15, 2019

/submit

@gitgitgadget

This comment has been minimized.

Copy link

commented Apr 15, 2019

@gitgitgadget

This comment has been minimized.

Copy link

commented Apr 16, 2019

This patch series was integrated into pu via git@710f9be.

@gitgitgadget

This comment has been minimized.

Copy link

commented Apr 16, 2019

This patch series was integrated into pu via git@e77e51a.

@gitgitgadget

This comment has been minimized.

Copy link

commented Apr 18, 2019

This patch series was integrated into pu via git@51275b9.

@gitgitgadget

This comment has been minimized.

Copy link

commented Apr 19, 2019

This patch series was integrated into pu via git@e4b5bf9.

@gitgitgadget

This comment has been minimized.

Copy link

commented Apr 22, 2019

This patch series was integrated into pu via git@89808f9.

@gitgitgadget

This comment has been minimized.

Copy link

commented Apr 22, 2019

This patch series was integrated into pu via git@d86ca9e.

@gitgitgadget

This comment has been minimized.

Copy link

commented Apr 25, 2019

This patch series was integrated into pu via git@32ff138.

@gitgitgadget

This comment has been minimized.

Copy link

commented Apr 25, 2019

This patch series was integrated into pu via git@920b769.

@gitgitgadget

This comment has been minimized.

Copy link

commented Apr 25, 2019

This patch series was integrated into next via git@a5c08f1.

@gitgitgadget gitgitgadget bot added the next label Apr 25, 2019

@@ -1005,6 +1005,7 @@ LIB_OBJS += trace2/tr2_cfg.o
LIB_OBJS += trace2/tr2_cmd_name.o

This comment has been minimized.

Copy link
@gitgitgadget

gitgitgadget bot Apr 27, 2019

On the Git mailing list, SZEDER Gábor wrote (reply to this):

On Mon, Apr 15, 2019 at 01:39:47PM -0700, Jeff Hostetler via GitGitGadget wrote:
> From: Jeff Hostetler <jeffhost@microsoft.com>
> 
> Teach git to read the system and global config files 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 and global config files are used.  Repo and worktree
> local config files are ignored.  Likewise, the "-c" command line
> arguments are also ignored.  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).

Reading the configuration that early causes unexpected and undesired
behavior change:

  $ sudo chmod a-rwx /usr/local/etc/gitconfig
  $ ./BUILDS/v2.21.0/bin/git
  usage: git [--version] [--help] [-C <path>] [-c <name>=<value>]
  <... snip rest of usage ...>
  $ strace ./BUILDS/v2.21.0/bin/git 2>&1 |grep config -c
  0
  $ ./git
  fatal: unable to access '/usr/local/etc/gitconfig': Permission denied
  $ ./git --version
  fatal: unable to access '/usr/local/etc/gitconfig': Permission denied

I think at least 'git', 'git --help', and 'git --version' should Just
Work, no matter what.


This breaks the 32 bit Linux build job on Travis CI, because:

  - In the 32 bit Docker image we change UID from root to regular user
    while preserving the environment, including $HOME.
    
  - Since $HOME is the default build prefix, Git will look for the
    system-wide configuration under '/root/etc/gitconfig', which fails
    as a regular user.

  - Our test harness checks early (i.e. earlier than setting
    GIT_CONFIG_NOSYSTEM=1) whether Git has been built successfully by
    attempting to run '$GIT_BUILD_DIR}/git', which fails because of
    the inaccessible system-wide config file, and in turn the harness
    assumes that Git hasn't been built and aborts.

  https://travis-ci.org/git/git/jobs/524403682#L1258

This comment has been minimized.

Copy link
@gitgitgadget

gitgitgadget bot Apr 29, 2019

On the Git mailing list, Jeff Hostetler wrote (reply to this):



On 4/27/2019 9:43 AM, SZEDER Gábor wrote:
> On Mon, Apr 15, 2019 at 01:39:47PM -0700, Jeff Hostetler via GitGitGadget wrote:
>> From: Jeff Hostetler <jeffhost@microsoft.com>
>>
>> Teach git to read the system and global config files 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 and global config files are used.  Repo and worktree
>> local config files are ignored.  Likewise, the "-c" command line
>> arguments are also ignored.  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).
> 
> Reading the configuration that early causes unexpected and undesired
> behavior change:
> 
>    $ sudo chmod a-rwx /usr/local/etc/gitconfig
>    $ ./BUILDS/v2.21.0/bin/git
>    usage: git [--version] [--help] [-C <path>] [-c <name>=<value>]
>    <... snip rest of usage ...>
>    $ strace ./BUILDS/v2.21.0/bin/git 2>&1 |grep config -c
>    0
>    $ ./git
>    fatal: unable to access '/usr/local/etc/gitconfig': Permission denied
>    $ ./git --version
>    fatal: unable to access '/usr/local/etc/gitconfig': Permission denied
> 
> I think at least 'git', 'git --help', and 'git --version' should Just
> Work, no matter what.
> 
> 
> This breaks the 32 bit Linux build job on Travis CI, because:
> 
>    - In the 32 bit Docker image we change UID from root to regular user
>      while preserving the environment, including $HOME.
>      
>    - Since $HOME is the default build prefix, Git will look for the
>      system-wide configuration under '/root/etc/gitconfig', which fails
>      as a regular user.
> 
>    - Our test harness checks early (i.e. earlier than setting
>      GIT_CONFIG_NOSYSTEM=1) whether Git has been built successfully by
>      attempting to run '$GIT_BUILD_DIR}/git', which fails because of
>      the inaccessible system-wide config file, and in turn the harness
>      assumes that Git hasn't been built and aborts.
> 
>    https://travis-ci.org/git/git/jobs/524403682#L1258
> 

It appears that config.c:do_git_config_sequence() passes flags = 0
to access_or_die() rather than ACCESS_EACCES_OK as it does for the
other scopes.  This causes the fatal error, rather than ignoring the
problem.

Reasons for this were discussed in:
     4698c8feb1 config: allow inaccessible configuration under $HOME

I'll push a fix shortly.

Thanks
Jeff
trace2: fixup access problem on /etc/gitconfig in read_very_early_config
Teach do_git_config_sequence() to optionally gently check for access to
the system config.  Use this option in read_very_early_config() when
initializing trace2.

In [1] SZEDER Gábor reported that my changes in [2] introduced a
regression when the user does not have permission to read the system
config.

This commit addresses that problem by optionally ignoring that error.

[1] https://public-inbox.org/git/285beb2b2d740ce20fdd8af1becf371ab39703db.1554995916.git.gitgitgadget@gmail.com/T/#m342e839289aec515523a98b5e34d7f42d3f1fd79
[2] https://public-inbox.org/git/285beb2b2d740ce20fdd8af1becf371ab39703db.1554995916.git.gitgitgadget@gmail.com/T/#m11b59c9228c698442f750ee8f9b10c629399ae48

Signed-off-by: Jeff Hostetler <jeffhost@microsoft.com>
@jeffhostetler

This comment has been minimized.

Copy link
Author

commented Apr 29, 2019

/submit

@gitgitgadget

This comment has been minimized.

Copy link

commented Apr 29, 2019

@gitgitgadget

This comment has been minimized.

Copy link

commented Apr 29, 2019

On the Git mailing list, Jeff Hostetler wrote (reply to this):


I forgot to update the cover letter for V5.
V5 adds a commit to fix the permission problem in /etc/gitconfig
reported by SZEDER.

This commit could/should be squashed into commit 5 if wanted.
I left it on top for review purposes.

Jeff


On 4/29/2019 4:14 PM, Jeff Hostetler via GitGitGadget wrote:
> Version 4 fixes a few clang-format warnings and simplifies the PID field in
> the SID.
> 
> Jeff Hostetler (11):
>    config: initialize opts structure in repo_read_config()
>    trace2: refactor setting process starting time
>    trace2: add absolute elapsed time to start event
>    trace2: find exec-dir before trace2 initialization
>    config: add read_very_early_config()
>    trace2: use system/global config for default trace2 settings
>    trace2: report peak memory usage of the process
>    trace2: clarify UTC datetime formatting
>    trace2: make SIDs more unique
>    trace2: update docs to describe system/global config settings
>    trace2: fixup access problem on /etc/gitconfig in
>      read_very_early_config
> 
>   Documentation/config.txt                 |   2 +
>   Documentation/config/trace2.txt          |  56 ++++++++
>   Documentation/technical/api-trace2.txt   | 176 +++++++++++++----------
>   Documentation/trace2-target-values.txt   |  10 ++
>   Makefile                                 |   1 +
>   common-main.c                            |   8 +-
>   compat/mingw.c                           |   2 +
>   compat/win32/trace2_win32_process_info.c |  50 ++++++-
>   config.c                                 |  30 +++-
>   config.h                                 |   5 +
>   t/t0210-trace2-normal.sh                 |  49 ++++++-
>   t/t0211-trace2-perf.sh                   |  43 ++++--
>   t/t0212-trace2-event.sh                  |  42 +++++-
>   trace2.c                                 |  21 ++-
>   trace2.h                                 |  43 ++++--
>   trace2/tr2_cfg.c                         |   7 +-
>   trace2/tr2_dst.c                         |  26 ++--
>   trace2/tr2_dst.h                         |   3 +-
>   trace2/tr2_sid.c                         |  53 ++++++-
>   trace2/tr2_sysenv.c                      | 127 ++++++++++++++++
>   trace2/tr2_sysenv.h                      |  36 +++++
>   trace2/tr2_tbuf.c                        |  19 ++-
>   trace2/tr2_tbuf.h                        |   5 +-
>   trace2/tr2_tgt.h                         |   1 +
>   trace2/tr2_tgt_event.c                   |  53 +++----
>   trace2/tr2_tgt_normal.c                  |  19 +--
>   trace2/tr2_tgt_perf.c                    |  23 +--
>   trace2/tr2_tls.c                         |  38 +++--
>   trace2/tr2_tls.h                         |   8 +-
>   29 files changed, 752 insertions(+), 204 deletions(-)
>   create mode 100644 Documentation/config/trace2.txt
>   create mode 100644 Documentation/trace2-target-values.txt
>   create mode 100644 trace2/tr2_sysenv.c
>   create mode 100644 trace2/tr2_sysenv.h
> 
> 
> base-commit: 041f5ea1cf987a4068ef5f39ba0a09be85952064
> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-169%2Fjeffhostetler%2Fcore-tr2-startup-and-sysenv-v5
> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-169/jeffhostetler/core-tr2-startup-and-sysenv-v5
> Pull-Request: https://github.com/gitgitgadget/git/pull/169
> 
> Range-diff vs v4:
> 
>    1:  f6653f1c59 =  1:  f6653f1c59 config: initialize opts structure in repo_read_config()
>    2:  48e34834b6 =  2:  48e34834b6 trace2: refactor setting process starting time
>    3:  175371fb54 =  3:  175371fb54 trace2: add absolute elapsed time to start event
>    4:  94729b284c =  4:  94729b284c trace2: find exec-dir before trace2 initialization
>    5:  b0fe1385f1 =  5:  b0fe1385f1 config: add read_very_early_config()
>    6:  550cad6189 =  6:  550cad6189 trace2: use system/global config for default trace2 settings
>    7:  56d8ce3fd6 =  7:  56d8ce3fd6 trace2: report peak memory usage of the process
>    8:  196a9d2c85 =  8:  196a9d2c85 trace2: clarify UTC datetime formatting
>    9:  9fdcb50140 =  9:  9fdcb50140 trace2: make SIDs more unique
>   10:  3414016d04 = 10:  3414016d04 trace2: update docs to describe system/global config settings
>    -:  ---------- > 11:  18ce795360 trace2: fixup access problem on /etc/gitconfig in read_very_early_config
> 
@gitgitgadget

This comment has been minimized.

Copy link

commented May 7, 2019

On the Git mailing list, Junio C Hamano wrote (reply to this):

Jeff Hostetler <git@jeffhostetler.com> writes:

> I forgot to update the cover letter for V5.
> V5 adds a commit to fix the permission problem in /etc/gitconfig
> reported by SZEDER.
>
> This commit could/should be squashed into commit 5 if wanted.
> I left it on top for review purposes.

I think v4 has been in 'next' for several days already, so an
incremental update like PATCH 11/11 is good.  The first 10 patches
are identical to the previous round, so all is good ;-)
@gitgitgadget

This comment has been minimized.

Copy link

commented May 7, 2019

This patch series was integrated into pu via git@592685a.

@gitgitgadget

This comment has been minimized.

Copy link

commented May 8, 2019

This patch series was integrated into pu via git@ce15b46.

@gitgitgadget

This comment has been minimized.

Copy link

commented May 8, 2019

This patch series was integrated into pu via git@11d5f2f.

@gitgitgadget

This comment has been minimized.

Copy link

commented May 10, 2019

This patch series was integrated into pu via git@cda9b19.

@gitgitgadget

This comment has been minimized.

Copy link

commented May 10, 2019

This patch series was integrated into next via git@71c3967.

@gitgitgadget

This comment has been minimized.

Copy link

commented May 13, 2019

This patch series was integrated into pu via git@5b2d1c0.

@gitgitgadget

This comment has been minimized.

Copy link

commented May 13, 2019

This patch series was integrated into next via git@5b2d1c0.

@gitgitgadget

This comment has been minimized.

Copy link

commented May 13, 2019

This patch series was integrated into master via git@5b2d1c0.

@gitgitgadget gitgitgadget bot added the master label May 13, 2019

@gitgitgadget gitgitgadget bot closed this May 13, 2019

@gitgitgadget

This comment has been minimized.

Copy link

commented May 13, 2019

Closed via 5b2d1c0.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.