* [PATCH 0/4] trace2: move 'def_param' events into 'cmd_name' and 'cmd_alias'
@ 2024-03-04 15:40 Jeff Hostetler via GitGitGadget
2024-03-04 15:40 ` [PATCH 1/4] t0211: demonstrate missing 'def_param' events for certain commands Jeff Hostetler via GitGitGadget
` (4 more replies)
0 siblings, 5 replies; 13+ messages in thread
From: Jeff Hostetler via GitGitGadget @ 2024-03-04 15:40 UTC (permalink / raw
To: git; +Cc: Jeff Hostetler
Some Git commands do not emit def_param events for interesting config and
environment variable settings. Let's fix that.
Builtin commands compiled into git.c have the normal control flow and emit a
cmd_name event and then def_param events for each interesting config and
environment variable. However, some special "query" commands, like
--exec-path, or some forms of alias expansion, emitted a cmd_name but did
not emit def_param events.
Also, special commands git-remote-https is built from remote-curl.c and
git-http-fetch is built from http-fetch.c and do not use the normal set up
in git.c. These emitted a cmd_name but not def_param events.
To minimize the footprint of this commit, move the calls to
trace2_cmd_list_config() and trace2_cmd_list_env_vars() into
trace2_cmd_name() and trace2_cmd_alias() so that we always get a set
def_param events when a cmd_name or cmd_alias event is generated.
Users can define local config settings on a repo to classify/name a repo
(e.g. "project-foo" vs "personal") and use the def_param feature to label
Trace2 data so that (a third-party) telemetry service does not collect data
on personal repos or so that telemetry from one work repo is distinguishable
from another work repo.
Jeff Hostetler (4):
t0211: demonstrate missing 'def_param' events for certain commands
trace2: avoid emitting 'def_param' set more than once
trace2: emit 'def_param' set with 'cmd_name' event
trace2: remove unneeded calls to generate 'def_param' set
git.c | 6 --
t/t0211-trace2-perf.sh | 231 +++++++++++++++++++++++++++++++++++++++++
trace2.c | 15 +++
3 files changed, 246 insertions(+), 6 deletions(-)
base-commit: 0f9d4d28b7e6021b7e6db192b7bf47bd3a0d0d1d
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1679%2Fjeffhostetler%2Falways-emit-def-param-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1679/jeffhostetler/always-emit-def-param-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1679
--
gitgitgadget
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 1/4] t0211: demonstrate missing 'def_param' events for certain commands
2024-03-04 15:40 [PATCH 0/4] trace2: move 'def_param' events into 'cmd_name' and 'cmd_alias' Jeff Hostetler via GitGitGadget
@ 2024-03-04 15:40 ` Jeff Hostetler via GitGitGadget
2024-03-04 15:40 ` [PATCH 2/4] trace2: avoid emitting 'def_param' set more than once Jeff Hostetler via GitGitGadget
` (3 subsequent siblings)
4 siblings, 0 replies; 13+ messages in thread
From: Jeff Hostetler via GitGitGadget @ 2024-03-04 15:40 UTC (permalink / raw
To: git; +Cc: Jeff Hostetler, Jeff Hostetler
From: Jeff Hostetler <jeffhostetler@github.com>
Some Git commands fail to emit 'def_param' events for interesting
config and environment variable settings.
Add unit tests to demonstrate this.
Most commands are considered "builtin" and are based upon git.c.
These typically do emit 'def_param' events. Exceptions are some of
the "query" commands, the "run-dashed" mechanism, and alias handling.
Commands built from remote-curl.c (instead of git.c), such as
"git-remote-https", do not emit 'def_param' events.
Likewise, "git-http-fetch" is built http-fetch.c and does not emit
them.
Signed-off-by: Jeff Hostetler <jeffhostetler@github.com>
---
t/t0211-trace2-perf.sh | 231 +++++++++++++++++++++++++++++++++++++++++
1 file changed, 231 insertions(+)
diff --git a/t/t0211-trace2-perf.sh b/t/t0211-trace2-perf.sh
index 290b6eaaab1..588c5bad033 100755
--- a/t/t0211-trace2-perf.sh
+++ b/t/t0211-trace2-perf.sh
@@ -287,4 +287,235 @@ test_expect_success 'unsafe URLs are redacted by default' '
grep "d0|main|def_param|.*|remote.origin.url:https://user:pwd@example.com" actual
'
+# Confirm that the requested command produces a "cmd_name" and a
+# set of "def_param" events.
+#
+try_simple () {
+ test_when_finished "rm prop.perf actual" &&
+
+ cmd=$1 &&
+ cmd_name=$2 &&
+
+ test_config_global "trace2.configParams" "cfg.prop.*" &&
+ test_config_global "trace2.envvars" "ENV_PROP_FOO,ENV_PROP_BAR" &&
+
+ test_config_global "cfg.prop.foo" "red" &&
+
+ ENV_PROP_FOO=blue \
+ GIT_TRACE2_PERF="$(pwd)/prop.perf" \
+ $cmd &&
+ perl "$TEST_DIRECTORY/t0211/scrub_perf.perl" <prop.perf >actual &&
+ grep "d0|main|cmd_name|.*|$cmd_name" actual &&
+ grep "d0|main|def_param|.*|cfg.prop.foo:red" actual &&
+ grep "d0|main|def_param|.*|ENV_PROP_FOO:blue" actual
+}
+
+# Representative mainstream builtin Git command dispatched
+# in run_builtin() in git.c
+#
+test_expect_success 'expect def_params for normal builtin command' '
+ try_simple "git version" "version"
+'
+
+# Representative query command dispatched in handle_options()
+# in git.c
+#
+test_expect_failure 'expect def_params for query command' '
+ try_simple "git --man-path" "_query_"
+'
+
+# remote-curl.c does not use the builtin setup in git.c, so confirm
+# that executables built from remote-curl.c emit def_params.
+#
+# Also tests the dashed-command handling where "git foo" silently
+# spawns "git-foo". Make sure that both commands should emit
+# def_params.
+#
+# Pass bogus arguments to remote-https and allow the command to fail
+# because we don't actually have a remote to fetch from. We just want
+# to see the run-dashed code run an executable built from
+# remote-curl.c rather than git.c. Confirm that we get def_param
+# events from both layers.
+#
+test_expect_failure 'expect def_params for remote-curl and _run_dashed_' '
+ test_when_finished "rm prop.perf actual" &&
+
+ test_config_global "trace2.configParams" "cfg.prop.*" &&
+ test_config_global "trace2.envvars" "ENV_PROP_FOO,ENV_PROP_BAR" &&
+
+ test_config_global "cfg.prop.foo" "red" &&
+
+ test_might_fail env \
+ ENV_PROP_FOO=blue \
+ GIT_TRACE2_PERF="$(pwd)/prop.perf" \
+ git remote-http x y &&
+
+ perl "$TEST_DIRECTORY/t0211/scrub_perf.perl" <prop.perf >actual &&
+
+ grep "d0|main|cmd_name|.*|_run_dashed_" actual &&
+ grep "d0|main|def_param|.*|cfg.prop.foo:red" actual &&
+ grep "d0|main|def_param|.*|ENV_PROP_FOO:blue" actual &&
+
+ grep "d1|main|cmd_name|.*|remote-curl" actual &&
+ grep "d1|main|def_param|.*|cfg.prop.foo:red" actual &&
+ grep "d1|main|def_param|.*|ENV_PROP_FOO:blue" actual
+'
+
+# Similarly, `git-http-fetch` is not built from git.c so do a
+# trivial fetch so that the main git.c run-dashed code spawns
+# an executable built from http-fetch.c. Confirm that we get
+# def_param events from both layers.
+#
+test_expect_failure 'expect def_params for http-fetch and _run_dashed_' '
+ test_when_finished "rm prop.perf actual" &&
+
+ test_config_global "trace2.configParams" "cfg.prop.*" &&
+ test_config_global "trace2.envvars" "ENV_PROP_FOO,ENV_PROP_BAR" &&
+
+ test_config_global "cfg.prop.foo" "red" &&
+
+ test_might_fail env \
+ ENV_PROP_FOO=blue \
+ GIT_TRACE2_PERF="$(pwd)/prop.perf" \
+ git http-fetch --stdin file:/// <<-EOF &&
+ EOF
+
+ perl "$TEST_DIRECTORY/t0211/scrub_perf.perl" <prop.perf >actual &&
+
+ grep "d0|main|cmd_name|.*|_run_dashed_" actual &&
+ grep "d0|main|def_param|.*|cfg.prop.foo:red" actual &&
+ grep "d0|main|def_param|.*|ENV_PROP_FOO:blue" actual &&
+
+ grep "d1|main|cmd_name|.*|http-fetch" actual &&
+ grep "d1|main|def_param|.*|cfg.prop.foo:red" actual &&
+ grep "d1|main|def_param|.*|ENV_PROP_FOO:blue" actual
+'
+
+# Historically, alias expansion explicitly emitted the def_param
+# events (independent of whether the command was a builtin, a Git
+# command or arbitrary shell command) so that it wasn't dependent
+# upon the unpeeling of the alias. Let's make sure that we preserve
+# the net effect.
+#
+test_expect_success 'expect def_params during git alias expansion' '
+ test_when_finished "rm prop.perf actual" &&
+
+ test_config_global "trace2.configParams" "cfg.prop.*" &&
+ test_config_global "trace2.envvars" "ENV_PROP_FOO,ENV_PROP_BAR" &&
+
+ test_config_global "cfg.prop.foo" "red" &&
+
+ test_config_global "alias.xxx" "version" &&
+
+ ENV_PROP_FOO=blue \
+ GIT_TRACE2_PERF="$(pwd)/prop.perf" \
+ git xxx &&
+
+ perl "$TEST_DIRECTORY/t0211/scrub_perf.perl" <prop.perf >actual &&
+
+ # "git xxx" is first mapped to "git-xxx" and the child will fail.
+ grep "d0|main|cmd_name|.*|_run_dashed_ (_run_dashed_)" actual &&
+
+ # We unpeel that and substitute "version" into "xxx" (giving
+ # "git version") and update the cmd_name event.
+ grep "d0|main|cmd_name|.*|_run_git_alias_ (_run_dashed_/_run_git_alias_)" actual &&
+
+ # These def_param events could be associated with either of the
+ # above cmd_name events. It does not matter.
+ grep "d0|main|def_param|.*|cfg.prop.foo:red" actual &&
+ grep "d0|main|def_param|.*|ENV_PROP_FOO:blue" actual &&
+
+ # The "git version" child sees a different cmd_name hierarchy.
+ # Also test the def_param (only for completeness).
+ grep "d1|main|cmd_name|.*|version (_run_dashed_/_run_git_alias_/version)" actual &&
+ grep "d1|main|def_param|.*|cfg.prop.foo:red" actual &&
+ grep "d1|main|def_param|.*|ENV_PROP_FOO:blue" actual
+'
+
+test_expect_success 'expect def_params during shell alias expansion' '
+ test_when_finished "rm prop.perf actual" &&
+
+ test_config_global "trace2.configParams" "cfg.prop.*" &&
+ test_config_global "trace2.envvars" "ENV_PROP_FOO,ENV_PROP_BAR" &&
+
+ test_config_global "cfg.prop.foo" "red" &&
+
+ test_config_global "alias.xxx" "!git version" &&
+
+ ENV_PROP_FOO=blue \
+ GIT_TRACE2_PERF="$(pwd)/prop.perf" \
+ git xxx &&
+
+ perl "$TEST_DIRECTORY/t0211/scrub_perf.perl" <prop.perf >actual &&
+
+ # "git xxx" is first mapped to "git-xxx" and the child will fail.
+ grep "d0|main|cmd_name|.*|_run_dashed_ (_run_dashed_)" actual &&
+
+ # We unpeel that and substitute "git version" for "git xxx" (as a
+ # shell command. Another cmd_name event is emitted as we unpeel.
+ grep "d0|main|cmd_name|.*|_run_shell_alias_ (_run_dashed_/_run_shell_alias_)" actual &&
+
+ # These def_param events could be associated with either of the
+ # above cmd_name events. It does not matter.
+ grep "d0|main|def_param|.*|cfg.prop.foo:red" actual &&
+ grep "d0|main|def_param|.*|ENV_PROP_FOO:blue" actual &&
+
+ # We get the following only because we used a git command for the
+ # shell command. In general, it could have been a shell script and
+ # we would see nothing.
+ #
+ # The child knows the cmd_name hierarchy so it includes it.
+ grep "d1|main|cmd_name|.*|version (_run_dashed_/_run_shell_alias_/version)" actual &&
+ grep "d1|main|def_param|.*|cfg.prop.foo:red" actual &&
+ grep "d1|main|def_param|.*|ENV_PROP_FOO:blue" actual
+'
+
+test_expect_failure 'expect def_params during nested git alias expansion' '
+ test_when_finished "rm prop.perf actual" &&
+
+ test_config_global "trace2.configParams" "cfg.prop.*" &&
+ test_config_global "trace2.envvars" "ENV_PROP_FOO,ENV_PROP_BAR" &&
+
+ test_config_global "cfg.prop.foo" "red" &&
+
+ test_config_global "alias.xxx" "yyy" &&
+ test_config_global "alias.yyy" "version" &&
+
+ ENV_PROP_FOO=blue \
+ GIT_TRACE2_PERF="$(pwd)/prop.perf" \
+ git xxx &&
+
+ perl "$TEST_DIRECTORY/t0211/scrub_perf.perl" <prop.perf >actual &&
+
+ # "git xxx" is first mapped to "git-xxx" and try to spawn "git-xxx"
+ # and the child will fail.
+ grep "d0|main|cmd_name|.*|_run_dashed_ (_run_dashed_)" actual &&
+ grep "d0|main|child_start|.*|.* class:dashed argv:\[git-xxx\]" actual &&
+
+ # We unpeel that and substitute "yyy" into "xxx" (giving "git yyy")
+ # and spawn "git-yyy" and the child will fail.
+ grep "d0|main|alias|.*|alias:xxx argv:\[yyy\]" actual &&
+ grep "d0|main|cmd_name|.*|_run_dashed_ (_run_dashed_/_run_dashed_)" actual &&
+ grep "d0|main|child_start|.*|.* class:dashed argv:\[git-yyy\]" actual &&
+
+ # We unpeel that and substitute "version" into "xxx" (giving
+ # "git version") and update the cmd_name event.
+ grep "d0|main|alias|.*|alias:yyy argv:\[version\]" actual &&
+ grep "d0|main|cmd_name|.*|_run_git_alias_ (_run_dashed_/_run_dashed_/_run_git_alias_)" actual &&
+
+ # These def_param events could be associated with any of the
+ # above cmd_name events. It does not matter.
+ grep "d0|main|def_param|.*|cfg.prop.foo:red" actual >actual.matches &&
+ grep "d0|main|def_param|.*|ENV_PROP_FOO:blue" actual &&
+
+ # However, we do not want them repeated each time we unpeel.
+ test_line_count = 1 actual.matches &&
+
+ # The "git version" child sees a different cmd_name hierarchy.
+ # Also test the def_param (only for completeness).
+ grep "d1|main|cmd_name|.*|version (_run_dashed_/_run_dashed_/_run_git_alias_/version)" actual &&
+ grep "d1|main|def_param|.*|cfg.prop.foo:red" actual &&
+ grep "d1|main|def_param|.*|ENV_PROP_FOO:blue" actual
+'
+
test_done
--
gitgitgadget
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH 2/4] trace2: avoid emitting 'def_param' set more than once
2024-03-04 15:40 [PATCH 0/4] trace2: move 'def_param' events into 'cmd_name' and 'cmd_alias' Jeff Hostetler via GitGitGadget
2024-03-04 15:40 ` [PATCH 1/4] t0211: demonstrate missing 'def_param' events for certain commands Jeff Hostetler via GitGitGadget
@ 2024-03-04 15:40 ` Jeff Hostetler via GitGitGadget
2024-03-04 15:40 ` [PATCH 3/4] trace2: emit 'def_param' set with 'cmd_name' event Jeff Hostetler via GitGitGadget
` (2 subsequent siblings)
4 siblings, 0 replies; 13+ messages in thread
From: Jeff Hostetler via GitGitGadget @ 2024-03-04 15:40 UTC (permalink / raw
To: git; +Cc: Jeff Hostetler, Jeff Hostetler
From: Jeff Hostetler <jeffhostetler@github.com>
During nested alias expansion it is possible for
"trace2_cmd_list_config()" and "trace2_cmd_list_env_vars()"
to be called more than once. This causes a full set of
'def_param' events to be emitted each time. Let's avoid
that.
Add code to those two functions to only emit them once.
Signed-off-by: Jeff Hostetler <jeffhostetler@github.com>
---
t/t0211-trace2-perf.sh | 2 +-
trace2.c | 12 ++++++++++++
2 files changed, 13 insertions(+), 1 deletion(-)
diff --git a/t/t0211-trace2-perf.sh b/t/t0211-trace2-perf.sh
index 588c5bad033..7b353195396 100755
--- a/t/t0211-trace2-perf.sh
+++ b/t/t0211-trace2-perf.sh
@@ -470,7 +470,7 @@ test_expect_success 'expect def_params during shell alias expansion' '
grep "d1|main|def_param|.*|ENV_PROP_FOO:blue" actual
'
-test_expect_failure 'expect def_params during nested git alias expansion' '
+test_expect_success 'expect def_params during nested git alias expansion' '
test_when_finished "rm prop.perf actual" &&
test_config_global "trace2.configParams" "cfg.prop.*" &&
diff --git a/trace2.c b/trace2.c
index f1e268bd159..facce641ef3 100644
--- a/trace2.c
+++ b/trace2.c
@@ -464,17 +464,29 @@ void trace2_cmd_alias_fl(const char *file, int line, const char *alias,
void trace2_cmd_list_config_fl(const char *file, int line)
{
+ static int emitted = 0;
+
if (!trace2_enabled)
return;
+ if (emitted)
+ return;
+ emitted = 1;
+
tr2_cfg_list_config_fl(file, line);
}
void trace2_cmd_list_env_vars_fl(const char *file, int line)
{
+ static int emitted = 0;
+
if (!trace2_enabled)
return;
+ if (emitted)
+ return;
+ emitted = 1;
+
tr2_list_env_vars_fl(file, line);
}
--
gitgitgadget
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH 3/4] trace2: emit 'def_param' set with 'cmd_name' event
2024-03-04 15:40 [PATCH 0/4] trace2: move 'def_param' events into 'cmd_name' and 'cmd_alias' Jeff Hostetler via GitGitGadget
2024-03-04 15:40 ` [PATCH 1/4] t0211: demonstrate missing 'def_param' events for certain commands Jeff Hostetler via GitGitGadget
2024-03-04 15:40 ` [PATCH 2/4] trace2: avoid emitting 'def_param' set more than once Jeff Hostetler via GitGitGadget
@ 2024-03-04 15:40 ` Jeff Hostetler via GitGitGadget
2024-03-04 15:40 ` [PATCH 4/4] trace2: remove unneeded calls to generate 'def_param' set Jeff Hostetler via GitGitGadget
2024-03-07 15:22 ` [PATCH v2 0/3] trace2: move generation of 'def_param' events into code for 'cmd_name' Jeff Hostetler via GitGitGadget
4 siblings, 0 replies; 13+ messages in thread
From: Jeff Hostetler via GitGitGadget @ 2024-03-04 15:40 UTC (permalink / raw
To: git; +Cc: Jeff Hostetler, Jeff Hostetler
From: Jeff Hostetler <jeffhostetler@github.com>
Some commands do not cause a set of 'def_param' events to be emitted.
This includes "git-remote-https", "git-http-fetch", and various
"query" commands, like "git --man-path".
Since all of these commands do emit a 'cmd_name' event, add code to
the "trace2_cmd_name()" function to generate the set of 'def_param'
events.
We can later remove explicit calls to "trace2_cmd_list_config()" and
"trace2_cmd_list_env_vars()" in git.c.
Signed-off-by: Jeff Hostetler <jeffhostetler@github.com>
---
t/t0211-trace2-perf.sh | 6 +++---
trace2.c | 3 +++
2 files changed, 6 insertions(+), 3 deletions(-)
diff --git a/t/t0211-trace2-perf.sh b/t/t0211-trace2-perf.sh
index 7b353195396..13ef69b92f8 100755
--- a/t/t0211-trace2-perf.sh
+++ b/t/t0211-trace2-perf.sh
@@ -320,7 +320,7 @@ test_expect_success 'expect def_params for normal builtin command' '
# Representative query command dispatched in handle_options()
# in git.c
#
-test_expect_failure 'expect def_params for query command' '
+test_expect_success 'expect def_params for query command' '
try_simple "git --man-path" "_query_"
'
@@ -337,7 +337,7 @@ test_expect_failure 'expect def_params for query command' '
# remote-curl.c rather than git.c. Confirm that we get def_param
# events from both layers.
#
-test_expect_failure 'expect def_params for remote-curl and _run_dashed_' '
+test_expect_success 'expect def_params for remote-curl and _run_dashed_' '
test_when_finished "rm prop.perf actual" &&
test_config_global "trace2.configParams" "cfg.prop.*" &&
@@ -366,7 +366,7 @@ test_expect_failure 'expect def_params for remote-curl and _run_dashed_' '
# an executable built from http-fetch.c. Confirm that we get
# def_param events from both layers.
#
-test_expect_failure 'expect def_params for http-fetch and _run_dashed_' '
+test_expect_success 'expect def_params for http-fetch and _run_dashed_' '
test_when_finished "rm prop.perf actual" &&
test_config_global "trace2.configParams" "cfg.prop.*" &&
diff --git a/trace2.c b/trace2.c
index facce641ef3..f894532d053 100644
--- a/trace2.c
+++ b/trace2.c
@@ -433,6 +433,9 @@ void trace2_cmd_name_fl(const char *file, int line, const char *name)
for_each_wanted_builtin (j, tgt_j)
if (tgt_j->pfn_command_name_fl)
tgt_j->pfn_command_name_fl(file, line, name, hierarchy);
+
+ trace2_cmd_list_config();
+ trace2_cmd_list_env_vars();
}
void trace2_cmd_mode_fl(const char *file, int line, const char *mode)
--
gitgitgadget
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH 4/4] trace2: remove unneeded calls to generate 'def_param' set
2024-03-04 15:40 [PATCH 0/4] trace2: move 'def_param' events into 'cmd_name' and 'cmd_alias' Jeff Hostetler via GitGitGadget
` (2 preceding siblings ...)
2024-03-04 15:40 ` [PATCH 3/4] trace2: emit 'def_param' set with 'cmd_name' event Jeff Hostetler via GitGitGadget
@ 2024-03-04 15:40 ` Jeff Hostetler via GitGitGadget
2024-03-06 21:47 ` Josh Steadmon
2024-03-07 15:22 ` [PATCH v2 0/3] trace2: move generation of 'def_param' events into code for 'cmd_name' Jeff Hostetler via GitGitGadget
4 siblings, 1 reply; 13+ messages in thread
From: Jeff Hostetler via GitGitGadget @ 2024-03-04 15:40 UTC (permalink / raw
To: git; +Cc: Jeff Hostetler, Jeff Hostetler
From: Jeff Hostetler <jeffhostetler@github.com>
Now that "trace2_cmd_name()" implicitly calls "trace2_cmd_list_config()"
and "trace2_cmd_list_env_vars()", we don't need to explicitly call them.
Signed-off-by: Jeff Hostetler <jeffhostetler@github.com>
---
git.c | 6 ------
1 file changed, 6 deletions(-)
diff --git a/git.c b/git.c
index 7068a184b0a..a769d72ab8f 100644
--- a/git.c
+++ b/git.c
@@ -373,8 +373,6 @@ static int handle_alias(int *argcp, const char ***argv)
strvec_pushv(&child.args, (*argv) + 1);
trace2_cmd_alias(alias_command, child.args.v);
- trace2_cmd_list_config();
- trace2_cmd_list_env_vars();
trace2_cmd_name("_run_shell_alias_");
ret = run_command(&child);
@@ -411,8 +409,6 @@ static int handle_alias(int *argcp, const char ***argv)
COPY_ARRAY(new_argv + count, *argv + 1, *argcp);
trace2_cmd_alias(alias_command, new_argv);
- trace2_cmd_list_config();
- trace2_cmd_list_env_vars();
*argv = new_argv;
*argcp += count - 1;
@@ -462,8 +458,6 @@ static int run_builtin(struct cmd_struct *p, int argc, const char **argv)
trace_argv_printf(argv, "trace: built-in: git");
trace2_cmd_name(p->cmd);
- trace2_cmd_list_config();
- trace2_cmd_list_env_vars();
validate_cache_entries(the_repository->index);
status = p->fn(argc, argv, prefix);
--
gitgitgadget
^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH 4/4] trace2: remove unneeded calls to generate 'def_param' set
2024-03-04 15:40 ` [PATCH 4/4] trace2: remove unneeded calls to generate 'def_param' set Jeff Hostetler via GitGitGadget
@ 2024-03-06 21:47 ` Josh Steadmon
2024-03-06 21:57 ` Junio C Hamano
0 siblings, 1 reply; 13+ messages in thread
From: Josh Steadmon @ 2024-03-06 21:47 UTC (permalink / raw
To: Jeff Hostetler via GitGitGadget; +Cc: git, Jeff Hostetler
On 2024.03.04 15:40, Jeff Hostetler via GitGitGadget wrote:
> From: Jeff Hostetler <jeffhostetler@github.com>
>
> Now that "trace2_cmd_name()" implicitly calls "trace2_cmd_list_config()"
> and "trace2_cmd_list_env_vars()", we don't need to explicitly call them.
>
> Signed-off-by: Jeff Hostetler <jeffhostetler@github.com>
> ---
> git.c | 6 ------
> 1 file changed, 6 deletions(-)
>
> diff --git a/git.c b/git.c
> index 7068a184b0a..a769d72ab8f 100644
> --- a/git.c
> +++ b/git.c
> @@ -373,8 +373,6 @@ static int handle_alias(int *argcp, const char ***argv)
> strvec_pushv(&child.args, (*argv) + 1);
>
> trace2_cmd_alias(alias_command, child.args.v);
> - trace2_cmd_list_config();
> - trace2_cmd_list_env_vars();
> trace2_cmd_name("_run_shell_alias_");
>
> ret = run_command(&child);
> @@ -411,8 +409,6 @@ static int handle_alias(int *argcp, const char ***argv)
> COPY_ARRAY(new_argv + count, *argv + 1, *argcp);
>
> trace2_cmd_alias(alias_command, new_argv);
> - trace2_cmd_list_config();
> - trace2_cmd_list_env_vars();
>
> *argv = new_argv;
> *argcp += count - 1;
> @@ -462,8 +458,6 @@ static int run_builtin(struct cmd_struct *p, int argc, const char **argv)
>
> trace_argv_printf(argv, "trace: built-in: git");
> trace2_cmd_name(p->cmd);
> - trace2_cmd_list_config();
> - trace2_cmd_list_env_vars();
>
> validate_cache_entries(the_repository->index);
> status = p->fn(argc, argv, prefix);
> --
> gitgitgadget
>
I'd personally prefer to see this squashed into Patch 3, but I don't
feel too strongly about it. Either way, the series LGTM.
Reviewed-by: Josh Steadmon <steadmon@google.com>
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 4/4] trace2: remove unneeded calls to generate 'def_param' set
2024-03-06 21:47 ` Josh Steadmon
@ 2024-03-06 21:57 ` Junio C Hamano
2024-03-06 22:54 ` Jeff Hostetler
0 siblings, 1 reply; 13+ messages in thread
From: Junio C Hamano @ 2024-03-06 21:57 UTC (permalink / raw
To: Josh Steadmon; +Cc: Jeff Hostetler via GitGitGadget, git, Jeff Hostetler
Josh Steadmon <steadmon@google.com> writes:
> On 2024.03.04 15:40, Jeff Hostetler via GitGitGadget wrote:
>> From: Jeff Hostetler <jeffhostetler@github.com>
>>
>> Now that "trace2_cmd_name()" implicitly calls "trace2_cmd_list_config()"
>> and "trace2_cmd_list_env_vars()", we don't need to explicitly call them.
>>
>> Signed-off-by: Jeff Hostetler <jeffhostetler@github.com>
>> ---
>> git.c | 6 ------
>> 1 file changed, 6 deletions(-)
>>
>> diff --git a/git.c b/git.c
>> index 7068a184b0a..a769d72ab8f 100644
>> --- a/git.c
>> +++ b/git.c
>> @@ -373,8 +373,6 @@ static int handle_alias(int *argcp, const char ***argv)
>> strvec_pushv(&child.args, (*argv) + 1);
>>
>> trace2_cmd_alias(alias_command, child.args.v);
>> - trace2_cmd_list_config();
>> - trace2_cmd_list_env_vars();
>> trace2_cmd_name("_run_shell_alias_");
>>
>> ret = run_command(&child);
>> @@ -411,8 +409,6 @@ static int handle_alias(int *argcp, const char ***argv)
>> COPY_ARRAY(new_argv + count, *argv + 1, *argcp);
>>
>> trace2_cmd_alias(alias_command, new_argv);
>> - trace2_cmd_list_config();
>> - trace2_cmd_list_env_vars();
>>
>> *argv = new_argv;
>> *argcp += count - 1;
>> @@ -462,8 +458,6 @@ static int run_builtin(struct cmd_struct *p, int argc, const char **argv)
>>
>> trace_argv_printf(argv, "trace: built-in: git");
>> trace2_cmd_name(p->cmd);
>> - trace2_cmd_list_config();
>> - trace2_cmd_list_env_vars();
>>
>> validate_cache_entries(the_repository->index);
>> status = p->fn(argc, argv, prefix);
>> --
>> gitgitgadget
>>
>
> I'd personally prefer to see this squashed into Patch 3, but I don't
> feel too strongly about it. Either way, the series LGTM.
>
> Reviewed-by: Josh Steadmon <steadmon@google.com>
Let's see what JeffH says about this. I agree with you that making
some stuff redundant in [Patch 3/4] and fixing the redundancy in
this step does feel somewhat roundabout way of doing this.
Thanks.
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 4/4] trace2: remove unneeded calls to generate 'def_param' set
2024-03-06 21:57 ` Junio C Hamano
@ 2024-03-06 22:54 ` Jeff Hostetler
2024-03-06 23:00 ` Junio C Hamano
0 siblings, 1 reply; 13+ messages in thread
From: Jeff Hostetler @ 2024-03-06 22:54 UTC (permalink / raw
To: Junio C Hamano, Josh Steadmon
Cc: Jeff Hostetler via GitGitGadget, git, Jeff Hostetler
On 3/6/24 4:57 PM, Junio C Hamano wrote:
> Josh Steadmon <steadmon@google.com> writes:
>
>> On 2024.03.04 15:40, Jeff Hostetler via GitGitGadget wrote:
>>> From: Jeff Hostetler <jeffhostetler@github.com>
>>>
>>> Now that "trace2_cmd_name()" implicitly calls "trace2_cmd_list_config()"
>>> and "trace2_cmd_list_env_vars()", we don't need to explicitly call them.
>>>
>>> Signed-off-by: Jeff Hostetler <jeffhostetler@github.com>
>>> ---
>>> git.c | 6 ------
>>> 1 file changed, 6 deletions(-)
>>>
>>> diff --git a/git.c b/git.c
>>> index 7068a184b0a..a769d72ab8f 100644
>>> --- a/git.c
>>> +++ b/git.c
>>> @@ -373,8 +373,6 @@ static int handle_alias(int *argcp, const char ***argv)
>>> strvec_pushv(&child.args, (*argv) + 1);
>>>
>>> trace2_cmd_alias(alias_command, child.args.v);
>>> - trace2_cmd_list_config();
>>> - trace2_cmd_list_env_vars();
>>> trace2_cmd_name("_run_shell_alias_");
>>>
>>> ret = run_command(&child);
>>> @@ -411,8 +409,6 @@ static int handle_alias(int *argcp, const char ***argv)
>>> COPY_ARRAY(new_argv + count, *argv + 1, *argcp);
>>>
>>> trace2_cmd_alias(alias_command, new_argv);
>>> - trace2_cmd_list_config();
>>> - trace2_cmd_list_env_vars();
>>>
>>> *argv = new_argv;
>>> *argcp += count - 1;
>>> @@ -462,8 +458,6 @@ static int run_builtin(struct cmd_struct *p, int argc, const char **argv)
>>>
>>> trace_argv_printf(argv, "trace: built-in: git");
>>> trace2_cmd_name(p->cmd);
>>> - trace2_cmd_list_config();
>>> - trace2_cmd_list_env_vars();
>>>
>>> validate_cache_entries(the_repository->index);
>>> status = p->fn(argc, argv, prefix);
>>> --
>>> gitgitgadget
>>>
>>
>> I'd personally prefer to see this squashed into Patch 3, but I don't
>> feel too strongly about it. Either way, the series LGTM.
>>
>> Reviewed-by: Josh Steadmon <steadmon@google.com>
>
> Let's see what JeffH says about this. I agree with you that making
> some stuff redundant in [Patch 3/4] and fixing the redundancy in
> this step does feel somewhat roundabout way of doing this.
>
> Thanks.
>
Sure we can merge them. That's fine. I can send a V4 or if you want
to just squash them together that's fine.
Jeff
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 4/4] trace2: remove unneeded calls to generate 'def_param' set
2024-03-06 22:54 ` Jeff Hostetler
@ 2024-03-06 23:00 ` Junio C Hamano
0 siblings, 0 replies; 13+ messages in thread
From: Junio C Hamano @ 2024-03-06 23:00 UTC (permalink / raw
To: Jeff Hostetler
Cc: Josh Steadmon, Jeff Hostetler via GitGitGadget, git,
Jeff Hostetler
Jeff Hostetler <git@jeffhostetler.com> writes:
>>> Reviewed-by: Josh Steadmon <steadmon@google.com>
>> Let's see what JeffH says about this. I agree with you that making
>> some stuff redundant in [Patch 3/4] and fixing the redundancy in
>> this step does feel somewhat roundabout way of doing this.
>> Thanks.
>>
>
> Sure we can merge them. That's fine. I can send a V4 or if you want
> to just squash them together that's fine.
Let's have a v4 describing the change for combined 3&4 in your
words, with Josh's Reviewed-by: already added to the trailers.
Thanks, both of you.
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH v2 0/3] trace2: move generation of 'def_param' events into code for 'cmd_name'
2024-03-04 15:40 [PATCH 0/4] trace2: move 'def_param' events into 'cmd_name' and 'cmd_alias' Jeff Hostetler via GitGitGadget
` (3 preceding siblings ...)
2024-03-04 15:40 ` [PATCH 4/4] trace2: remove unneeded calls to generate 'def_param' set Jeff Hostetler via GitGitGadget
@ 2024-03-07 15:22 ` Jeff Hostetler via GitGitGadget
2024-03-07 15:22 ` [PATCH v2 1/3] t0211: demonstrate missing 'def_param' events for certain commands Jeff Hostetler via GitGitGadget
` (2 more replies)
4 siblings, 3 replies; 13+ messages in thread
From: Jeff Hostetler via GitGitGadget @ 2024-03-07 15:22 UTC (permalink / raw
To: git; +Cc: Josh Steadmon, Jeff Hostetler, Jeff Hostetler
Here is version 2 of this series. The only change from V1 is to combine the
last two commits as discussed.
Thanks Jeff
----------------------------------------------------------------------------
Some Git commands do not emit def_param events for interesting config and
environment variable settings. Let's fix that.
Builtin commands compiled into git.c have the normal control flow and emit a
cmd_name event and then def_param events for each interesting config and
environment variable. However, some special "query" commands, like
--exec-path, or some forms of alias expansion, emitted a cmd_name but did
not emit def_param events.
Also, special commands git-remote-https is built from remote-curl.c and
git-http-fetch is built from http-fetch.c and do not use the normal set up
in git.c. These emitted a cmd_name but not def_param events.
To minimize the footprint of this commit, move the calls to
trace2_cmd_list_config() and trace2_cmd_list_env_vars() into
trace2_cmd_name() so that we always get a set of def_param events when a
cmd_name event is generated.
Users can define local config settings on a repo to classify/name a repo
(e.g. "project-foo" vs "personal") and use the def_param feature to label
Trace2 data so that (a third-party) telemetry service does not collect data
on personal repos or so that telemetry from one work repo is distinguishable
from another work repo in database queries.
Jeff Hostetler (3):
t0211: demonstrate missing 'def_param' events for certain commands
trace2: avoid emitting 'def_param' set more than once
trace2: emit 'def_param' set with 'cmd_name' event
git.c | 6 --
t/t0211-trace2-perf.sh | 231 +++++++++++++++++++++++++++++++++++++++++
trace2.c | 15 +++
3 files changed, 246 insertions(+), 6 deletions(-)
base-commit: 0f9d4d28b7e6021b7e6db192b7bf47bd3a0d0d1d
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1679%2Fjeffhostetler%2Falways-emit-def-param-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1679/jeffhostetler/always-emit-def-param-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/1679
Range-diff vs v1:
1: b378b93242a = 1: b378b93242a t0211: demonstrate missing 'def_param' events for certain commands
2: 65068e97597 = 2: 65068e97597 trace2: avoid emitting 'def_param' set more than once
3: 9507184b4f1 ! 3: 178721cd4f0 trace2: emit 'def_param' set with 'cmd_name' event
@@ Commit message
the "trace2_cmd_name()" function to generate the set of 'def_param'
events.
- We can later remove explicit calls to "trace2_cmd_list_config()" and
- "trace2_cmd_list_env_vars()" in git.c.
+ Remove explicit calls to "trace2_cmd_list_config()" and
+ "trace2_cmd_list_env_vars()" in git.c since they are no longer needed.
+ Reviewed-by: Josh Steadmon <steadmon@google.com>
Signed-off-by: Jeff Hostetler <jeffhostetler@github.com>
+ ## git.c ##
+@@ git.c: static int handle_alias(int *argcp, const char ***argv)
+ strvec_pushv(&child.args, (*argv) + 1);
+
+ trace2_cmd_alias(alias_command, child.args.v);
+- trace2_cmd_list_config();
+- trace2_cmd_list_env_vars();
+ trace2_cmd_name("_run_shell_alias_");
+
+ ret = run_command(&child);
+@@ git.c: static int handle_alias(int *argcp, const char ***argv)
+ COPY_ARRAY(new_argv + count, *argv + 1, *argcp);
+
+ trace2_cmd_alias(alias_command, new_argv);
+- trace2_cmd_list_config();
+- trace2_cmd_list_env_vars();
+
+ *argv = new_argv;
+ *argcp += count - 1;
+@@ git.c: static int run_builtin(struct cmd_struct *p, int argc, const char **argv)
+
+ trace_argv_printf(argv, "trace: built-in: git");
+ trace2_cmd_name(p->cmd);
+- trace2_cmd_list_config();
+- trace2_cmd_list_env_vars();
+
+ validate_cache_entries(the_repository->index);
+ status = p->fn(argc, argv, prefix);
+
## t/t0211-trace2-perf.sh ##
@@ t/t0211-trace2-perf.sh: test_expect_success 'expect def_params for normal builtin command' '
# Representative query command dispatched in handle_options()
4: e8528715ebf < -: ----------- trace2: remove unneeded calls to generate 'def_param' set
--
gitgitgadget
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH v2 1/3] t0211: demonstrate missing 'def_param' events for certain commands
2024-03-07 15:22 ` [PATCH v2 0/3] trace2: move generation of 'def_param' events into code for 'cmd_name' Jeff Hostetler via GitGitGadget
@ 2024-03-07 15:22 ` Jeff Hostetler via GitGitGadget
2024-03-07 15:22 ` [PATCH v2 2/3] trace2: avoid emitting 'def_param' set more than once Jeff Hostetler via GitGitGadget
2024-03-07 15:22 ` [PATCH v2 3/3] trace2: emit 'def_param' set with 'cmd_name' event Jeff Hostetler via GitGitGadget
2 siblings, 0 replies; 13+ messages in thread
From: Jeff Hostetler via GitGitGadget @ 2024-03-07 15:22 UTC (permalink / raw
To: git; +Cc: Josh Steadmon, Jeff Hostetler, Jeff Hostetler, Jeff Hostetler
From: Jeff Hostetler <jeffhostetler@github.com>
Some Git commands fail to emit 'def_param' events for interesting
config and environment variable settings.
Add unit tests to demonstrate this.
Most commands are considered "builtin" and are based upon git.c.
These typically do emit 'def_param' events. Exceptions are some of
the "query" commands, the "run-dashed" mechanism, and alias handling.
Commands built from remote-curl.c (instead of git.c), such as
"git-remote-https", do not emit 'def_param' events.
Likewise, "git-http-fetch" is built http-fetch.c and does not emit
them.
Signed-off-by: Jeff Hostetler <jeffhostetler@github.com>
---
t/t0211-trace2-perf.sh | 231 +++++++++++++++++++++++++++++++++++++++++
1 file changed, 231 insertions(+)
diff --git a/t/t0211-trace2-perf.sh b/t/t0211-trace2-perf.sh
index 290b6eaaab1..588c5bad033 100755
--- a/t/t0211-trace2-perf.sh
+++ b/t/t0211-trace2-perf.sh
@@ -287,4 +287,235 @@ test_expect_success 'unsafe URLs are redacted by default' '
grep "d0|main|def_param|.*|remote.origin.url:https://user:pwd@example.com" actual
'
+# Confirm that the requested command produces a "cmd_name" and a
+# set of "def_param" events.
+#
+try_simple () {
+ test_when_finished "rm prop.perf actual" &&
+
+ cmd=$1 &&
+ cmd_name=$2 &&
+
+ test_config_global "trace2.configParams" "cfg.prop.*" &&
+ test_config_global "trace2.envvars" "ENV_PROP_FOO,ENV_PROP_BAR" &&
+
+ test_config_global "cfg.prop.foo" "red" &&
+
+ ENV_PROP_FOO=blue \
+ GIT_TRACE2_PERF="$(pwd)/prop.perf" \
+ $cmd &&
+ perl "$TEST_DIRECTORY/t0211/scrub_perf.perl" <prop.perf >actual &&
+ grep "d0|main|cmd_name|.*|$cmd_name" actual &&
+ grep "d0|main|def_param|.*|cfg.prop.foo:red" actual &&
+ grep "d0|main|def_param|.*|ENV_PROP_FOO:blue" actual
+}
+
+# Representative mainstream builtin Git command dispatched
+# in run_builtin() in git.c
+#
+test_expect_success 'expect def_params for normal builtin command' '
+ try_simple "git version" "version"
+'
+
+# Representative query command dispatched in handle_options()
+# in git.c
+#
+test_expect_failure 'expect def_params for query command' '
+ try_simple "git --man-path" "_query_"
+'
+
+# remote-curl.c does not use the builtin setup in git.c, so confirm
+# that executables built from remote-curl.c emit def_params.
+#
+# Also tests the dashed-command handling where "git foo" silently
+# spawns "git-foo". Make sure that both commands should emit
+# def_params.
+#
+# Pass bogus arguments to remote-https and allow the command to fail
+# because we don't actually have a remote to fetch from. We just want
+# to see the run-dashed code run an executable built from
+# remote-curl.c rather than git.c. Confirm that we get def_param
+# events from both layers.
+#
+test_expect_failure 'expect def_params for remote-curl and _run_dashed_' '
+ test_when_finished "rm prop.perf actual" &&
+
+ test_config_global "trace2.configParams" "cfg.prop.*" &&
+ test_config_global "trace2.envvars" "ENV_PROP_FOO,ENV_PROP_BAR" &&
+
+ test_config_global "cfg.prop.foo" "red" &&
+
+ test_might_fail env \
+ ENV_PROP_FOO=blue \
+ GIT_TRACE2_PERF="$(pwd)/prop.perf" \
+ git remote-http x y &&
+
+ perl "$TEST_DIRECTORY/t0211/scrub_perf.perl" <prop.perf >actual &&
+
+ grep "d0|main|cmd_name|.*|_run_dashed_" actual &&
+ grep "d0|main|def_param|.*|cfg.prop.foo:red" actual &&
+ grep "d0|main|def_param|.*|ENV_PROP_FOO:blue" actual &&
+
+ grep "d1|main|cmd_name|.*|remote-curl" actual &&
+ grep "d1|main|def_param|.*|cfg.prop.foo:red" actual &&
+ grep "d1|main|def_param|.*|ENV_PROP_FOO:blue" actual
+'
+
+# Similarly, `git-http-fetch` is not built from git.c so do a
+# trivial fetch so that the main git.c run-dashed code spawns
+# an executable built from http-fetch.c. Confirm that we get
+# def_param events from both layers.
+#
+test_expect_failure 'expect def_params for http-fetch and _run_dashed_' '
+ test_when_finished "rm prop.perf actual" &&
+
+ test_config_global "trace2.configParams" "cfg.prop.*" &&
+ test_config_global "trace2.envvars" "ENV_PROP_FOO,ENV_PROP_BAR" &&
+
+ test_config_global "cfg.prop.foo" "red" &&
+
+ test_might_fail env \
+ ENV_PROP_FOO=blue \
+ GIT_TRACE2_PERF="$(pwd)/prop.perf" \
+ git http-fetch --stdin file:/// <<-EOF &&
+ EOF
+
+ perl "$TEST_DIRECTORY/t0211/scrub_perf.perl" <prop.perf >actual &&
+
+ grep "d0|main|cmd_name|.*|_run_dashed_" actual &&
+ grep "d0|main|def_param|.*|cfg.prop.foo:red" actual &&
+ grep "d0|main|def_param|.*|ENV_PROP_FOO:blue" actual &&
+
+ grep "d1|main|cmd_name|.*|http-fetch" actual &&
+ grep "d1|main|def_param|.*|cfg.prop.foo:red" actual &&
+ grep "d1|main|def_param|.*|ENV_PROP_FOO:blue" actual
+'
+
+# Historically, alias expansion explicitly emitted the def_param
+# events (independent of whether the command was a builtin, a Git
+# command or arbitrary shell command) so that it wasn't dependent
+# upon the unpeeling of the alias. Let's make sure that we preserve
+# the net effect.
+#
+test_expect_success 'expect def_params during git alias expansion' '
+ test_when_finished "rm prop.perf actual" &&
+
+ test_config_global "trace2.configParams" "cfg.prop.*" &&
+ test_config_global "trace2.envvars" "ENV_PROP_FOO,ENV_PROP_BAR" &&
+
+ test_config_global "cfg.prop.foo" "red" &&
+
+ test_config_global "alias.xxx" "version" &&
+
+ ENV_PROP_FOO=blue \
+ GIT_TRACE2_PERF="$(pwd)/prop.perf" \
+ git xxx &&
+
+ perl "$TEST_DIRECTORY/t0211/scrub_perf.perl" <prop.perf >actual &&
+
+ # "git xxx" is first mapped to "git-xxx" and the child will fail.
+ grep "d0|main|cmd_name|.*|_run_dashed_ (_run_dashed_)" actual &&
+
+ # We unpeel that and substitute "version" into "xxx" (giving
+ # "git version") and update the cmd_name event.
+ grep "d0|main|cmd_name|.*|_run_git_alias_ (_run_dashed_/_run_git_alias_)" actual &&
+
+ # These def_param events could be associated with either of the
+ # above cmd_name events. It does not matter.
+ grep "d0|main|def_param|.*|cfg.prop.foo:red" actual &&
+ grep "d0|main|def_param|.*|ENV_PROP_FOO:blue" actual &&
+
+ # The "git version" child sees a different cmd_name hierarchy.
+ # Also test the def_param (only for completeness).
+ grep "d1|main|cmd_name|.*|version (_run_dashed_/_run_git_alias_/version)" actual &&
+ grep "d1|main|def_param|.*|cfg.prop.foo:red" actual &&
+ grep "d1|main|def_param|.*|ENV_PROP_FOO:blue" actual
+'
+
+test_expect_success 'expect def_params during shell alias expansion' '
+ test_when_finished "rm prop.perf actual" &&
+
+ test_config_global "trace2.configParams" "cfg.prop.*" &&
+ test_config_global "trace2.envvars" "ENV_PROP_FOO,ENV_PROP_BAR" &&
+
+ test_config_global "cfg.prop.foo" "red" &&
+
+ test_config_global "alias.xxx" "!git version" &&
+
+ ENV_PROP_FOO=blue \
+ GIT_TRACE2_PERF="$(pwd)/prop.perf" \
+ git xxx &&
+
+ perl "$TEST_DIRECTORY/t0211/scrub_perf.perl" <prop.perf >actual &&
+
+ # "git xxx" is first mapped to "git-xxx" and the child will fail.
+ grep "d0|main|cmd_name|.*|_run_dashed_ (_run_dashed_)" actual &&
+
+ # We unpeel that and substitute "git version" for "git xxx" (as a
+ # shell command. Another cmd_name event is emitted as we unpeel.
+ grep "d0|main|cmd_name|.*|_run_shell_alias_ (_run_dashed_/_run_shell_alias_)" actual &&
+
+ # These def_param events could be associated with either of the
+ # above cmd_name events. It does not matter.
+ grep "d0|main|def_param|.*|cfg.prop.foo:red" actual &&
+ grep "d0|main|def_param|.*|ENV_PROP_FOO:blue" actual &&
+
+ # We get the following only because we used a git command for the
+ # shell command. In general, it could have been a shell script and
+ # we would see nothing.
+ #
+ # The child knows the cmd_name hierarchy so it includes it.
+ grep "d1|main|cmd_name|.*|version (_run_dashed_/_run_shell_alias_/version)" actual &&
+ grep "d1|main|def_param|.*|cfg.prop.foo:red" actual &&
+ grep "d1|main|def_param|.*|ENV_PROP_FOO:blue" actual
+'
+
+test_expect_failure 'expect def_params during nested git alias expansion' '
+ test_when_finished "rm prop.perf actual" &&
+
+ test_config_global "trace2.configParams" "cfg.prop.*" &&
+ test_config_global "trace2.envvars" "ENV_PROP_FOO,ENV_PROP_BAR" &&
+
+ test_config_global "cfg.prop.foo" "red" &&
+
+ test_config_global "alias.xxx" "yyy" &&
+ test_config_global "alias.yyy" "version" &&
+
+ ENV_PROP_FOO=blue \
+ GIT_TRACE2_PERF="$(pwd)/prop.perf" \
+ git xxx &&
+
+ perl "$TEST_DIRECTORY/t0211/scrub_perf.perl" <prop.perf >actual &&
+
+ # "git xxx" is first mapped to "git-xxx" and try to spawn "git-xxx"
+ # and the child will fail.
+ grep "d0|main|cmd_name|.*|_run_dashed_ (_run_dashed_)" actual &&
+ grep "d0|main|child_start|.*|.* class:dashed argv:\[git-xxx\]" actual &&
+
+ # We unpeel that and substitute "yyy" into "xxx" (giving "git yyy")
+ # and spawn "git-yyy" and the child will fail.
+ grep "d0|main|alias|.*|alias:xxx argv:\[yyy\]" actual &&
+ grep "d0|main|cmd_name|.*|_run_dashed_ (_run_dashed_/_run_dashed_)" actual &&
+ grep "d0|main|child_start|.*|.* class:dashed argv:\[git-yyy\]" actual &&
+
+ # We unpeel that and substitute "version" into "xxx" (giving
+ # "git version") and update the cmd_name event.
+ grep "d0|main|alias|.*|alias:yyy argv:\[version\]" actual &&
+ grep "d0|main|cmd_name|.*|_run_git_alias_ (_run_dashed_/_run_dashed_/_run_git_alias_)" actual &&
+
+ # These def_param events could be associated with any of the
+ # above cmd_name events. It does not matter.
+ grep "d0|main|def_param|.*|cfg.prop.foo:red" actual >actual.matches &&
+ grep "d0|main|def_param|.*|ENV_PROP_FOO:blue" actual &&
+
+ # However, we do not want them repeated each time we unpeel.
+ test_line_count = 1 actual.matches &&
+
+ # The "git version" child sees a different cmd_name hierarchy.
+ # Also test the def_param (only for completeness).
+ grep "d1|main|cmd_name|.*|version (_run_dashed_/_run_dashed_/_run_git_alias_/version)" actual &&
+ grep "d1|main|def_param|.*|cfg.prop.foo:red" actual &&
+ grep "d1|main|def_param|.*|ENV_PROP_FOO:blue" actual
+'
+
test_done
--
gitgitgadget
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH v2 2/3] trace2: avoid emitting 'def_param' set more than once
2024-03-07 15:22 ` [PATCH v2 0/3] trace2: move generation of 'def_param' events into code for 'cmd_name' Jeff Hostetler via GitGitGadget
2024-03-07 15:22 ` [PATCH v2 1/3] t0211: demonstrate missing 'def_param' events for certain commands Jeff Hostetler via GitGitGadget
@ 2024-03-07 15:22 ` Jeff Hostetler via GitGitGadget
2024-03-07 15:22 ` [PATCH v2 3/3] trace2: emit 'def_param' set with 'cmd_name' event Jeff Hostetler via GitGitGadget
2 siblings, 0 replies; 13+ messages in thread
From: Jeff Hostetler via GitGitGadget @ 2024-03-07 15:22 UTC (permalink / raw
To: git; +Cc: Josh Steadmon, Jeff Hostetler, Jeff Hostetler, Jeff Hostetler
From: Jeff Hostetler <jeffhostetler@github.com>
During nested alias expansion it is possible for
"trace2_cmd_list_config()" and "trace2_cmd_list_env_vars()"
to be called more than once. This causes a full set of
'def_param' events to be emitted each time. Let's avoid
that.
Add code to those two functions to only emit them once.
Signed-off-by: Jeff Hostetler <jeffhostetler@github.com>
---
t/t0211-trace2-perf.sh | 2 +-
trace2.c | 12 ++++++++++++
2 files changed, 13 insertions(+), 1 deletion(-)
diff --git a/t/t0211-trace2-perf.sh b/t/t0211-trace2-perf.sh
index 588c5bad033..7b353195396 100755
--- a/t/t0211-trace2-perf.sh
+++ b/t/t0211-trace2-perf.sh
@@ -470,7 +470,7 @@ test_expect_success 'expect def_params during shell alias expansion' '
grep "d1|main|def_param|.*|ENV_PROP_FOO:blue" actual
'
-test_expect_failure 'expect def_params during nested git alias expansion' '
+test_expect_success 'expect def_params during nested git alias expansion' '
test_when_finished "rm prop.perf actual" &&
test_config_global "trace2.configParams" "cfg.prop.*" &&
diff --git a/trace2.c b/trace2.c
index f1e268bd159..facce641ef3 100644
--- a/trace2.c
+++ b/trace2.c
@@ -464,17 +464,29 @@ void trace2_cmd_alias_fl(const char *file, int line, const char *alias,
void trace2_cmd_list_config_fl(const char *file, int line)
{
+ static int emitted = 0;
+
if (!trace2_enabled)
return;
+ if (emitted)
+ return;
+ emitted = 1;
+
tr2_cfg_list_config_fl(file, line);
}
void trace2_cmd_list_env_vars_fl(const char *file, int line)
{
+ static int emitted = 0;
+
if (!trace2_enabled)
return;
+ if (emitted)
+ return;
+ emitted = 1;
+
tr2_list_env_vars_fl(file, line);
}
--
gitgitgadget
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH v2 3/3] trace2: emit 'def_param' set with 'cmd_name' event
2024-03-07 15:22 ` [PATCH v2 0/3] trace2: move generation of 'def_param' events into code for 'cmd_name' Jeff Hostetler via GitGitGadget
2024-03-07 15:22 ` [PATCH v2 1/3] t0211: demonstrate missing 'def_param' events for certain commands Jeff Hostetler via GitGitGadget
2024-03-07 15:22 ` [PATCH v2 2/3] trace2: avoid emitting 'def_param' set more than once Jeff Hostetler via GitGitGadget
@ 2024-03-07 15:22 ` Jeff Hostetler via GitGitGadget
2 siblings, 0 replies; 13+ messages in thread
From: Jeff Hostetler via GitGitGadget @ 2024-03-07 15:22 UTC (permalink / raw
To: git; +Cc: Josh Steadmon, Jeff Hostetler, Jeff Hostetler, Jeff Hostetler
From: Jeff Hostetler <jeffhostetler@github.com>
Some commands do not cause a set of 'def_param' events to be emitted.
This includes "git-remote-https", "git-http-fetch", and various
"query" commands, like "git --man-path".
Since all of these commands do emit a 'cmd_name' event, add code to
the "trace2_cmd_name()" function to generate the set of 'def_param'
events.
Remove explicit calls to "trace2_cmd_list_config()" and
"trace2_cmd_list_env_vars()" in git.c since they are no longer needed.
Reviewed-by: Josh Steadmon <steadmon@google.com>
Signed-off-by: Jeff Hostetler <jeffhostetler@github.com>
---
git.c | 6 ------
t/t0211-trace2-perf.sh | 6 +++---
trace2.c | 3 +++
3 files changed, 6 insertions(+), 9 deletions(-)
diff --git a/git.c b/git.c
index 7068a184b0a..a769d72ab8f 100644
--- a/git.c
+++ b/git.c
@@ -373,8 +373,6 @@ static int handle_alias(int *argcp, const char ***argv)
strvec_pushv(&child.args, (*argv) + 1);
trace2_cmd_alias(alias_command, child.args.v);
- trace2_cmd_list_config();
- trace2_cmd_list_env_vars();
trace2_cmd_name("_run_shell_alias_");
ret = run_command(&child);
@@ -411,8 +409,6 @@ static int handle_alias(int *argcp, const char ***argv)
COPY_ARRAY(new_argv + count, *argv + 1, *argcp);
trace2_cmd_alias(alias_command, new_argv);
- trace2_cmd_list_config();
- trace2_cmd_list_env_vars();
*argv = new_argv;
*argcp += count - 1;
@@ -462,8 +458,6 @@ static int run_builtin(struct cmd_struct *p, int argc, const char **argv)
trace_argv_printf(argv, "trace: built-in: git");
trace2_cmd_name(p->cmd);
- trace2_cmd_list_config();
- trace2_cmd_list_env_vars();
validate_cache_entries(the_repository->index);
status = p->fn(argc, argv, prefix);
diff --git a/t/t0211-trace2-perf.sh b/t/t0211-trace2-perf.sh
index 7b353195396..13ef69b92f8 100755
--- a/t/t0211-trace2-perf.sh
+++ b/t/t0211-trace2-perf.sh
@@ -320,7 +320,7 @@ test_expect_success 'expect def_params for normal builtin command' '
# Representative query command dispatched in handle_options()
# in git.c
#
-test_expect_failure 'expect def_params for query command' '
+test_expect_success 'expect def_params for query command' '
try_simple "git --man-path" "_query_"
'
@@ -337,7 +337,7 @@ test_expect_failure 'expect def_params for query command' '
# remote-curl.c rather than git.c. Confirm that we get def_param
# events from both layers.
#
-test_expect_failure 'expect def_params for remote-curl and _run_dashed_' '
+test_expect_success 'expect def_params for remote-curl and _run_dashed_' '
test_when_finished "rm prop.perf actual" &&
test_config_global "trace2.configParams" "cfg.prop.*" &&
@@ -366,7 +366,7 @@ test_expect_failure 'expect def_params for remote-curl and _run_dashed_' '
# an executable built from http-fetch.c. Confirm that we get
# def_param events from both layers.
#
-test_expect_failure 'expect def_params for http-fetch and _run_dashed_' '
+test_expect_success 'expect def_params for http-fetch and _run_dashed_' '
test_when_finished "rm prop.perf actual" &&
test_config_global "trace2.configParams" "cfg.prop.*" &&
diff --git a/trace2.c b/trace2.c
index facce641ef3..f894532d053 100644
--- a/trace2.c
+++ b/trace2.c
@@ -433,6 +433,9 @@ void trace2_cmd_name_fl(const char *file, int line, const char *name)
for_each_wanted_builtin (j, tgt_j)
if (tgt_j->pfn_command_name_fl)
tgt_j->pfn_command_name_fl(file, line, name, hierarchy);
+
+ trace2_cmd_list_config();
+ trace2_cmd_list_env_vars();
}
void trace2_cmd_mode_fl(const char *file, int line, const char *mode)
--
gitgitgadget
^ permalink raw reply related [flat|nested] 13+ messages in thread
end of thread, other threads:[~2024-03-07 15:25 UTC | newest]
Thread overview: 13+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-03-04 15:40 [PATCH 0/4] trace2: move 'def_param' events into 'cmd_name' and 'cmd_alias' Jeff Hostetler via GitGitGadget
2024-03-04 15:40 ` [PATCH 1/4] t0211: demonstrate missing 'def_param' events for certain commands Jeff Hostetler via GitGitGadget
2024-03-04 15:40 ` [PATCH 2/4] trace2: avoid emitting 'def_param' set more than once Jeff Hostetler via GitGitGadget
2024-03-04 15:40 ` [PATCH 3/4] trace2: emit 'def_param' set with 'cmd_name' event Jeff Hostetler via GitGitGadget
2024-03-04 15:40 ` [PATCH 4/4] trace2: remove unneeded calls to generate 'def_param' set Jeff Hostetler via GitGitGadget
2024-03-06 21:47 ` Josh Steadmon
2024-03-06 21:57 ` Junio C Hamano
2024-03-06 22:54 ` Jeff Hostetler
2024-03-06 23:00 ` Junio C Hamano
2024-03-07 15:22 ` [PATCH v2 0/3] trace2: move generation of 'def_param' events into code for 'cmd_name' Jeff Hostetler via GitGitGadget
2024-03-07 15:22 ` [PATCH v2 1/3] t0211: demonstrate missing 'def_param' events for certain commands Jeff Hostetler via GitGitGadget
2024-03-07 15:22 ` [PATCH v2 2/3] trace2: avoid emitting 'def_param' set more than once Jeff Hostetler via GitGitGadget
2024-03-07 15:22 ` [PATCH v2 3/3] trace2: emit 'def_param' set with 'cmd_name' event Jeff Hostetler via GitGitGadget
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).