{"thread":{"id":"61047","subject":"[PATCH 0/4] trace2: move 'def_param' events into 'cmd_name' and 'cmd_alias'","startedAt":"2024-03-04T15:40:13Z","lastAt":"2024-03-07T15:22:35Z","messageCount":13,"participants":["Jeff Hostetler via GitGitGadget","Josh Steadmon","Junio C Hamano","Jeff Hostetler"],"isPatch":true,"patchVersion":1,"patchTotal":4},"messages":[{"id":"489893","messageId":"pull.1679.git.1709566808.gitgitgadget@gmail.com","threadId":"61047","inReplyTo":null,"subject":"[PATCH 0/4] trace2: move 'def_param' events into 'cmd_name' and 'cmd_alias'","fromName":"Jeff Hostetler via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-03-04T15:40:04Z","receivedAt":"2024-03-04T15:40:13Z","isPatch":true,"sender":{"key":"git@jeffhostetler.com","avatar":null},"body":"Some Git commands do not emit def_param events for interesting config and\nenvironment variable settings. Let's fix that.\n\nBuiltin commands compiled into git.c have the normal control flow and emit a\ncmd_name event and then def_param events for each interesting config and\nenvironment variable. However, some special \"query\" commands, like\n--exec-path, or some forms of alias expansion, emitted a cmd_name but did\nnot emit def_param events.\n\nAlso, special commands git-remote-https is built from remote-curl.c and\ngit-http-fetch is built from http-fetch.c and do not use the normal set up\nin git.c. These emitted a cmd_name but not def_param events.\n\nTo minimize the footprint of this commit, move the calls to\ntrace2_cmd_list_config() and trace2_cmd_list_env_vars() into\ntrace2_cmd_name() and trace2_cmd_alias() so that we always get a set\ndef_param events when a cmd_name or cmd_alias event is generated.\n\nUsers can define local config settings on a repo to classify/name a repo\n(e.g. \"project-foo\" vs \"personal\") and use the def_param feature to label\nTrace2 data so that (a third-party) telemetry service does not collect data\non personal repos or so that telemetry from one work repo is distinguishable\nfrom another work repo.\n\nJeff Hostetler (4):\n  t0211: demonstrate missing 'def_param' events for certain commands\n  trace2: avoid emitting 'def_param' set more than once\n  trace2: emit 'def_param' set with 'cmd_name' event\n  trace2: remove unneeded calls to generate 'def_param' set\n\n git.c                  |   6 --\n t/t0211-trace2-perf.sh | 231 +++++++++++++++++++++++++++++++++++++++++\n trace2.c               |  15 +++\n 3 files changed, 246 insertions(+), 6 deletions(-)\n\n\nbase-commit: 0f9d4d28b7e6021b7e6db192b7bf47bd3a0d0d1d\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1679%2Fjeffhostetler%2Falways-emit-def-param-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1679/jeffhostetler/always-emit-def-param-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/1679\n-- \ngitgitgadget\n"},{"id":"489894","messageId":"b378b93242a7870772fdb53d7bf2d58d3347ba62.1709566808.git.gitgitgadget@gmail.com","threadId":"61047","inReplyTo":"pull.1679.git.1709566808.gitgitgadget@gmail.com","subject":"[PATCH 1/4] t0211: demonstrate missing 'def_param' events for certain commands","fromName":"Jeff Hostetler via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-03-04T15:40:05Z","receivedAt":"2024-03-04T15:40:13Z","isPatch":true,"sender":{"key":"git@jeffhostetler.com","avatar":null},"body":"From: Jeff Hostetler <jeffhostetler@github.com>\n\nSome Git commands fail to emit 'def_param' events for interesting\nconfig and environment variable settings.\n\nAdd unit tests to demonstrate this.\n\nMost commands are considered \"builtin\" and are based upon git.c.\nThese typically do emit 'def_param' events.  Exceptions are some of\nthe \"query\" commands, the \"run-dashed\" mechanism, and alias handling.\n\nCommands built from remote-curl.c (instead of git.c), such as\n\"git-remote-https\", do not emit 'def_param' events.\n\nLikewise, \"git-http-fetch\" is built http-fetch.c and does not emit\nthem.\n\nSigned-off-by: Jeff Hostetler <jeffhostetler@github.com>\n---\n t/t0211-trace2-perf.sh | 231 +++++++++++++++++++++++++++++++++++++++++\n 1 file changed, 231 insertions(+)\n\ndiff --git a/t/t0211-trace2-perf.sh b/t/t0211-trace2-perf.sh\nindex 290b6eaaab1..588c5bad033 100755\n--- a/t/t0211-trace2-perf.sh\n+++ b/t/t0211-trace2-perf.sh\n@@ -287,4 +287,235 @@ test_expect_success 'unsafe URLs are redacted by default' '\n \tgrep \"d0|main|def_param|.*|remote.origin.url:https://user:pwd@example.com\" actual\n '\n \n+# Confirm that the requested command produces a \"cmd_name\" and a\n+# set of \"def_param\" events.\n+#\n+try_simple () {\n+\ttest_when_finished \"rm prop.perf actual\" &&\n+\n+\tcmd=$1 &&\n+\tcmd_name=$2 &&\n+\n+\ttest_config_global \"trace2.configParams\" \"cfg.prop.*\" &&\n+\ttest_config_global \"trace2.envvars\" \"ENV_PROP_FOO,ENV_PROP_BAR\" &&\n+\n+\ttest_config_global \"cfg.prop.foo\" \"red\" &&\n+\n+\tENV_PROP_FOO=blue \\\n+\t\tGIT_TRACE2_PERF=\"$(pwd)/prop.perf\" \\\n+\t\t\t$cmd &&\n+\tperl \"$TEST_DIRECTORY/t0211/scrub_perf.perl\" <prop.perf >actual &&\n+\tgrep \"d0|main|cmd_name|.*|$cmd_name\" actual &&\n+\tgrep \"d0|main|def_param|.*|cfg.prop.foo:red\" actual &&\n+\tgrep \"d0|main|def_param|.*|ENV_PROP_FOO:blue\" actual\n+}\n+\n+# Representative mainstream builtin Git command dispatched\n+# in run_builtin() in git.c\n+#\n+test_expect_success 'expect def_params for normal builtin command' '\n+\ttry_simple \"git version\" \"version\"\n+'\n+\n+# Representative query command dispatched in handle_options()\n+# in git.c\n+#\n+test_expect_failure 'expect def_params for query command' '\n+\ttry_simple \"git --man-path\" \"_query_\"\n+'\n+\n+# remote-curl.c does not use the builtin setup in git.c, so confirm\n+# that executables built from remote-curl.c emit def_params.\n+#\n+# Also tests the dashed-command handling where \"git foo\" silently\n+# spawns \"git-foo\".  Make sure that both commands should emit\n+# def_params.\n+#\n+# Pass bogus arguments to remote-https and allow the command to fail\n+# because we don't actually have a remote to fetch from.  We just want\n+# to see the run-dashed code run an executable built from\n+# remote-curl.c rather than git.c.  Confirm that we get def_param\n+# events from both layers.\n+#\n+test_expect_failure 'expect def_params for remote-curl and _run_dashed_' '\n+\ttest_when_finished \"rm prop.perf actual\" &&\n+\n+\ttest_config_global \"trace2.configParams\" \"cfg.prop.*\" &&\n+\ttest_config_global \"trace2.envvars\" \"ENV_PROP_FOO,ENV_PROP_BAR\" &&\n+\n+\ttest_config_global \"cfg.prop.foo\" \"red\" &&\n+\n+\ttest_might_fail env \\\n+\t\tENV_PROP_FOO=blue \\\n+\t\tGIT_TRACE2_PERF=\"$(pwd)/prop.perf\" \\\n+\t\tgit remote-http x y &&\n+\n+\tperl \"$TEST_DIRECTORY/t0211/scrub_perf.perl\" <prop.perf >actual &&\n+\n+\tgrep \"d0|main|cmd_name|.*|_run_dashed_\" actual &&\n+\tgrep \"d0|main|def_param|.*|cfg.prop.foo:red\" actual &&\n+\tgrep \"d0|main|def_param|.*|ENV_PROP_FOO:blue\" actual &&\n+\n+\tgrep \"d1|main|cmd_name|.*|remote-curl\" actual &&\n+\tgrep \"d1|main|def_param|.*|cfg.prop.foo:red\" actual &&\n+\tgrep \"d1|main|def_param|.*|ENV_PROP_FOO:blue\" actual\n+'\n+\n+# Similarly, `git-http-fetch` is not built from git.c so do a\n+# trivial fetch so that the main git.c run-dashed code spawns\n+# an executable built from http-fetch.c.  Confirm that we get\n+# def_param events from both layers.\n+#\n+test_expect_failure 'expect def_params for http-fetch and _run_dashed_' '\n+\ttest_when_finished \"rm prop.perf actual\" &&\n+\n+\ttest_config_global \"trace2.configParams\" \"cfg.prop.*\" &&\n+\ttest_config_global \"trace2.envvars\" \"ENV_PROP_FOO,ENV_PROP_BAR\" &&\n+\n+\ttest_config_global \"cfg.prop.foo\" \"red\" &&\n+\n+\ttest_might_fail env \\\n+\t\tENV_PROP_FOO=blue \\\n+\t\tGIT_TRACE2_PERF=\"$(pwd)/prop.perf\" \\\n+\t\tgit http-fetch --stdin file:/// <<-EOF &&\n+\tEOF\n+\n+\tperl \"$TEST_DIRECTORY/t0211/scrub_perf.perl\" <prop.perf >actual &&\n+\n+\tgrep \"d0|main|cmd_name|.*|_run_dashed_\" actual &&\n+\tgrep \"d0|main|def_param|.*|cfg.prop.foo:red\" actual &&\n+\tgrep \"d0|main|def_param|.*|ENV_PROP_FOO:blue\" actual &&\n+\n+\tgrep \"d1|main|cmd_name|.*|http-fetch\" actual &&\n+\tgrep \"d1|main|def_param|.*|cfg.prop.foo:red\" actual &&\n+\tgrep \"d1|main|def_param|.*|ENV_PROP_FOO:blue\" actual\n+'\n+\n+# Historically, alias expansion explicitly emitted the def_param\n+# events (independent of whether the command was a builtin, a Git\n+# command or arbitrary shell command) so that it wasn't dependent\n+# upon the unpeeling of the alias. Let's make sure that we preserve\n+# the net effect.\n+#\n+test_expect_success 'expect def_params during git alias expansion' '\n+\ttest_when_finished \"rm prop.perf actual\" &&\n+\n+\ttest_config_global \"trace2.configParams\" \"cfg.prop.*\" &&\n+\ttest_config_global \"trace2.envvars\" \"ENV_PROP_FOO,ENV_PROP_BAR\" &&\n+\n+\ttest_config_global \"cfg.prop.foo\" \"red\" &&\n+\n+\ttest_config_global \"alias.xxx\" \"version\" &&\n+\n+\tENV_PROP_FOO=blue \\\n+\t\tGIT_TRACE2_PERF=\"$(pwd)/prop.perf\" \\\n+\t\t\tgit xxx &&\n+\n+\tperl \"$TEST_DIRECTORY/t0211/scrub_perf.perl\" <prop.perf >actual &&\n+\n+\t# \"git xxx\" is first mapped to \"git-xxx\" and the child will fail.\n+\tgrep \"d0|main|cmd_name|.*|_run_dashed_ (_run_dashed_)\" actual &&\n+\n+\t# We unpeel that and substitute \"version\" into \"xxx\" (giving\n+\t# \"git version\") and update the cmd_name event.\n+\tgrep \"d0|main|cmd_name|.*|_run_git_alias_ (_run_dashed_/_run_git_alias_)\" actual &&\n+\n+\t# These def_param events could be associated with either of the\n+\t# above cmd_name events.  It does not matter.\n+\tgrep \"d0|main|def_param|.*|cfg.prop.foo:red\" actual &&\n+\tgrep \"d0|main|def_param|.*|ENV_PROP_FOO:blue\" actual &&\n+\n+\t# The \"git version\" child sees a different cmd_name hierarchy.\n+\t# Also test the def_param (only for completeness).\n+\tgrep \"d1|main|cmd_name|.*|version (_run_dashed_/_run_git_alias_/version)\" actual &&\n+\tgrep \"d1|main|def_param|.*|cfg.prop.foo:red\" actual &&\n+\tgrep \"d1|main|def_param|.*|ENV_PROP_FOO:blue\" actual\n+'\n+\n+test_expect_success 'expect def_params during shell alias expansion' '\n+\ttest_when_finished \"rm prop.perf actual\" &&\n+\n+\ttest_config_global \"trace2.configParams\" \"cfg.prop.*\" &&\n+\ttest_config_global \"trace2.envvars\" \"ENV_PROP_FOO,ENV_PROP_BAR\" &&\n+\n+\ttest_config_global \"cfg.prop.foo\" \"red\" &&\n+\n+\ttest_config_global \"alias.xxx\" \"!git version\" &&\n+\n+\tENV_PROP_FOO=blue \\\n+\t\tGIT_TRACE2_PERF=\"$(pwd)/prop.perf\" \\\n+\t\t\tgit xxx &&\n+\n+\tperl \"$TEST_DIRECTORY/t0211/scrub_perf.perl\" <prop.perf >actual &&\n+\n+\t# \"git xxx\" is first mapped to \"git-xxx\" and the child will fail.\n+\tgrep \"d0|main|cmd_name|.*|_run_dashed_ (_run_dashed_)\" actual &&\n+\n+\t# We unpeel that and substitute \"git version\" for \"git xxx\" (as a\n+\t# shell command.  Another cmd_name event is emitted as we unpeel.\n+\tgrep \"d0|main|cmd_name|.*|_run_shell_alias_ (_run_dashed_/_run_shell_alias_)\" actual &&\n+\n+\t# These def_param events could be associated with either of the\n+\t# above cmd_name events.  It does not matter.\n+\tgrep \"d0|main|def_param|.*|cfg.prop.foo:red\" actual &&\n+\tgrep \"d0|main|def_param|.*|ENV_PROP_FOO:blue\" actual &&\n+\n+\t# We get the following only because we used a git command for the\n+\t# shell command. In general, it could have been a shell script and\n+\t# we would see nothing.\n+\t#\n+\t# The child knows the cmd_name hierarchy so it includes it.\n+\tgrep \"d1|main|cmd_name|.*|version (_run_dashed_/_run_shell_alias_/version)\" actual &&\n+\tgrep \"d1|main|def_param|.*|cfg.prop.foo:red\" actual &&\n+\tgrep \"d1|main|def_param|.*|ENV_PROP_FOO:blue\" actual\n+'\n+\n+test_expect_failure 'expect def_params during nested git alias expansion' '\n+\ttest_when_finished \"rm prop.perf actual\" &&\n+\n+\ttest_config_global \"trace2.configParams\" \"cfg.prop.*\" &&\n+\ttest_config_global \"trace2.envvars\" \"ENV_PROP_FOO,ENV_PROP_BAR\" &&\n+\n+\ttest_config_global \"cfg.prop.foo\" \"red\" &&\n+\n+\ttest_config_global \"alias.xxx\" \"yyy\" &&\n+\ttest_config_global \"alias.yyy\" \"version\" &&\n+\n+\tENV_PROP_FOO=blue \\\n+\t\tGIT_TRACE2_PERF=\"$(pwd)/prop.perf\" \\\n+\t\t\tgit xxx &&\n+\n+\tperl \"$TEST_DIRECTORY/t0211/scrub_perf.perl\" <prop.perf >actual &&\n+\n+\t# \"git xxx\" is first mapped to \"git-xxx\" and try to spawn \"git-xxx\"\n+\t# and the child will fail.\n+\tgrep \"d0|main|cmd_name|.*|_run_dashed_ (_run_dashed_)\" actual &&\n+\tgrep \"d0|main|child_start|.*|.* class:dashed argv:\\[git-xxx\\]\" actual &&\n+\n+\t# We unpeel that and substitute \"yyy\" into \"xxx\" (giving \"git yyy\")\n+\t# and spawn \"git-yyy\" and the child will fail.\n+\tgrep \"d0|main|alias|.*|alias:xxx argv:\\[yyy\\]\" actual &&\n+\tgrep \"d0|main|cmd_name|.*|_run_dashed_ (_run_dashed_/_run_dashed_)\" actual &&\n+\tgrep \"d0|main|child_start|.*|.* class:dashed argv:\\[git-yyy\\]\" actual &&\n+\n+\t# We unpeel that and substitute \"version\" into \"xxx\" (giving\n+\t# \"git version\") and update the cmd_name event.\n+\tgrep \"d0|main|alias|.*|alias:yyy argv:\\[version\\]\" actual &&\n+\tgrep \"d0|main|cmd_name|.*|_run_git_alias_ (_run_dashed_/_run_dashed_/_run_git_alias_)\" actual &&\n+\n+\t# These def_param events could be associated with any of the\n+\t# above cmd_name events.  It does not matter.\n+\tgrep \"d0|main|def_param|.*|cfg.prop.foo:red\" actual >actual.matches &&\n+\tgrep \"d0|main|def_param|.*|ENV_PROP_FOO:blue\" actual &&\n+\n+\t# However, we do not want them repeated each time we unpeel.\n+\ttest_line_count = 1 actual.matches &&\n+\n+\t# The \"git version\" child sees a different cmd_name hierarchy.\n+\t# Also test the def_param (only for completeness).\n+\tgrep \"d1|main|cmd_name|.*|version (_run_dashed_/_run_dashed_/_run_git_alias_/version)\" actual &&\n+\tgrep \"d1|main|def_param|.*|cfg.prop.foo:red\" actual &&\n+\tgrep \"d1|main|def_param|.*|ENV_PROP_FOO:blue\" actual\n+'\n+\n test_done\n-- \ngitgitgadget\n\n"},{"id":"489895","messageId":"65068e97597241e297f5d7cdb60012be1784e9dc.1709566808.git.gitgitgadget@gmail.com","threadId":"61047","inReplyTo":"pull.1679.git.1709566808.gitgitgadget@gmail.com","subject":"[PATCH 2/4] trace2: avoid emitting 'def_param' set more than once","fromName":"Jeff Hostetler via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-03-04T15:40:06Z","receivedAt":"2024-03-04T15:40:14Z","isPatch":true,"sender":{"key":"git@jeffhostetler.com","avatar":null},"body":"From: Jeff Hostetler <jeffhostetler@github.com>\n\nDuring nested alias expansion it is possible for\n\"trace2_cmd_list_config()\" and \"trace2_cmd_list_env_vars()\"\nto be called more than once.  This causes a full set of\n'def_param' events to be emitted each time.  Let's avoid\nthat.\n\nAdd code to those two functions to only emit them once.\n\nSigned-off-by: Jeff Hostetler <jeffhostetler@github.com>\n---\n t/t0211-trace2-perf.sh |  2 +-\n trace2.c               | 12 ++++++++++++\n 2 files changed, 13 insertions(+), 1 deletion(-)\n\ndiff --git a/t/t0211-trace2-perf.sh b/t/t0211-trace2-perf.sh\nindex 588c5bad033..7b353195396 100755\n--- a/t/t0211-trace2-perf.sh\n+++ b/t/t0211-trace2-perf.sh\n@@ -470,7 +470,7 @@ test_expect_success 'expect def_params during shell alias expansion' '\n \tgrep \"d1|main|def_param|.*|ENV_PROP_FOO:blue\" actual\n '\n \n-test_expect_failure 'expect def_params during nested git alias expansion' '\n+test_expect_success 'expect def_params during nested git alias expansion' '\n \ttest_when_finished \"rm prop.perf actual\" &&\n \n \ttest_config_global \"trace2.configParams\" \"cfg.prop.*\" &&\ndiff --git a/trace2.c b/trace2.c\nindex f1e268bd159..facce641ef3 100644\n--- a/trace2.c\n+++ b/trace2.c\n@@ -464,17 +464,29 @@ void trace2_cmd_alias_fl(const char *file, int line, const char *alias,\n \n void trace2_cmd_list_config_fl(const char *file, int line)\n {\n+\tstatic int emitted = 0;\n+\n \tif (!trace2_enabled)\n \t\treturn;\n \n+\tif (emitted)\n+\t\treturn;\n+\temitted = 1;\n+\n \ttr2_cfg_list_config_fl(file, line);\n }\n \n void trace2_cmd_list_env_vars_fl(const char *file, int line)\n {\n+\tstatic int emitted = 0;\n+\n \tif (!trace2_enabled)\n \t\treturn;\n \n+\tif (emitted)\n+\t\treturn;\n+\temitted = 1;\n+\n \ttr2_list_env_vars_fl(file, line);\n }\n \n-- \ngitgitgadget\n\n"},{"id":"489896","messageId":"9507184b4f1147be529898023d8d504819596f71.1709566808.git.gitgitgadget@gmail.com","threadId":"61047","inReplyTo":"pull.1679.git.1709566808.gitgitgadget@gmail.com","subject":"[PATCH 3/4] trace2: emit 'def_param' set with 'cmd_name' event","fromName":"Jeff Hostetler via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-03-04T15:40:07Z","receivedAt":"2024-03-04T15:40:15Z","isPatch":true,"sender":{"key":"git@jeffhostetler.com","avatar":null},"body":"From: Jeff Hostetler <jeffhostetler@github.com>\n\nSome commands do not cause a set of 'def_param' events to be emitted.\nThis includes \"git-remote-https\", \"git-http-fetch\", and various\n\"query\" commands, like \"git --man-path\".\n\nSince all of these commands do emit a 'cmd_name' event, add code to\nthe \"trace2_cmd_name()\" function to generate the set of 'def_param'\nevents.\n\nWe can later remove explicit calls to \"trace2_cmd_list_config()\" and\n\"trace2_cmd_list_env_vars()\" in git.c.\n\nSigned-off-by: Jeff Hostetler <jeffhostetler@github.com>\n---\n t/t0211-trace2-perf.sh | 6 +++---\n trace2.c               | 3 +++\n 2 files changed, 6 insertions(+), 3 deletions(-)\n\ndiff --git a/t/t0211-trace2-perf.sh b/t/t0211-trace2-perf.sh\nindex 7b353195396..13ef69b92f8 100755\n--- a/t/t0211-trace2-perf.sh\n+++ b/t/t0211-trace2-perf.sh\n@@ -320,7 +320,7 @@ test_expect_success 'expect def_params for normal builtin command' '\n # Representative query command dispatched in handle_options()\n # in git.c\n #\n-test_expect_failure 'expect def_params for query command' '\n+test_expect_success 'expect def_params for query command' '\n \ttry_simple \"git --man-path\" \"_query_\"\n '\n \n@@ -337,7 +337,7 @@ test_expect_failure 'expect def_params for query command' '\n # remote-curl.c rather than git.c.  Confirm that we get def_param\n # events from both layers.\n #\n-test_expect_failure 'expect def_params for remote-curl and _run_dashed_' '\n+test_expect_success 'expect def_params for remote-curl and _run_dashed_' '\n \ttest_when_finished \"rm prop.perf actual\" &&\n \n \ttest_config_global \"trace2.configParams\" \"cfg.prop.*\" &&\n@@ -366,7 +366,7 @@ test_expect_failure 'expect def_params for remote-curl and _run_dashed_' '\n # an executable built from http-fetch.c.  Confirm that we get\n # def_param events from both layers.\n #\n-test_expect_failure 'expect def_params for http-fetch and _run_dashed_' '\n+test_expect_success 'expect def_params for http-fetch and _run_dashed_' '\n \ttest_when_finished \"rm prop.perf actual\" &&\n \n \ttest_config_global \"trace2.configParams\" \"cfg.prop.*\" &&\ndiff --git a/trace2.c b/trace2.c\nindex facce641ef3..f894532d053 100644\n--- a/trace2.c\n+++ b/trace2.c\n@@ -433,6 +433,9 @@ void trace2_cmd_name_fl(const char *file, int line, const char *name)\n \tfor_each_wanted_builtin (j, tgt_j)\n \t\tif (tgt_j->pfn_command_name_fl)\n \t\t\ttgt_j->pfn_command_name_fl(file, line, name, hierarchy);\n+\n+\ttrace2_cmd_list_config();\n+\ttrace2_cmd_list_env_vars();\n }\n \n void trace2_cmd_mode_fl(const char *file, int line, const char *mode)\n-- \ngitgitgadget\n\n"},{"id":"489897","messageId":"e8528715ebf97c12622c2e73f914ab4228a0927c.1709566808.git.gitgitgadget@gmail.com","threadId":"61047","inReplyTo":"pull.1679.git.1709566808.gitgitgadget@gmail.com","subject":"[PATCH 4/4] trace2: remove unneeded calls to generate 'def_param' set","fromName":"Jeff Hostetler via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-03-04T15:40:08Z","receivedAt":"2024-03-04T15:40:15Z","isPatch":true,"sender":{"key":"git@jeffhostetler.com","avatar":null},"body":"From: Jeff Hostetler <jeffhostetler@github.com>\n\nNow that \"trace2_cmd_name()\" implicitly calls \"trace2_cmd_list_config()\"\nand \"trace2_cmd_list_env_vars()\", we don't need to explicitly call them.\n\nSigned-off-by: Jeff Hostetler <jeffhostetler@github.com>\n---\n git.c | 6 ------\n 1 file changed, 6 deletions(-)\n\ndiff --git a/git.c b/git.c\nindex 7068a184b0a..a769d72ab8f 100644\n--- a/git.c\n+++ b/git.c\n@@ -373,8 +373,6 @@ static int handle_alias(int *argcp, const char ***argv)\n \t\t\tstrvec_pushv(&child.args, (*argv) + 1);\n \n \t\t\ttrace2_cmd_alias(alias_command, child.args.v);\n-\t\t\ttrace2_cmd_list_config();\n-\t\t\ttrace2_cmd_list_env_vars();\n \t\t\ttrace2_cmd_name(\"_run_shell_alias_\");\n \n \t\t\tret = run_command(&child);\n@@ -411,8 +409,6 @@ static int handle_alias(int *argcp, const char ***argv)\n \t\tCOPY_ARRAY(new_argv + count, *argv + 1, *argcp);\n \n \t\ttrace2_cmd_alias(alias_command, new_argv);\n-\t\ttrace2_cmd_list_config();\n-\t\ttrace2_cmd_list_env_vars();\n \n \t\t*argv = new_argv;\n \t\t*argcp += count - 1;\n@@ -462,8 +458,6 @@ static int run_builtin(struct cmd_struct *p, int argc, const char **argv)\n \n \ttrace_argv_printf(argv, \"trace: built-in: git\");\n \ttrace2_cmd_name(p->cmd);\n-\ttrace2_cmd_list_config();\n-\ttrace2_cmd_list_env_vars();\n \n \tvalidate_cache_entries(the_repository->index);\n \tstatus = p->fn(argc, argv, prefix);\n-- \ngitgitgadget\n"},{"id":"490116","messageId":"ZejkVOVQBZhLVfHW@google.com","threadId":"61047","inReplyTo":"e8528715ebf97c12622c2e73f914ab4228a0927c.1709566808.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 4/4] trace2: remove unneeded calls to generate 'def_param' set","fromName":"Josh Steadmon","fromEmail":"steadmon@google.com","sentAt":"2024-03-06T21:47:00Z","receivedAt":"2024-03-06T21:47:06Z","isPatch":true,"sender":{"key":"steadmon@google.com","avatar":"https://avatars.githubusercontent.com/u/2654920?v=4"},"body":"On 2024.03.04 15:40, Jeff Hostetler via GitGitGadget wrote:\n> From: Jeff Hostetler <jeffhostetler@github.com>\n> \n> Now that \"trace2_cmd_name()\" implicitly calls \"trace2_cmd_list_config()\"\n> and \"trace2_cmd_list_env_vars()\", we don't need to explicitly call them.\n> \n> Signed-off-by: Jeff Hostetler <jeffhostetler@github.com>\n> ---\n>  git.c | 6 ------\n>  1 file changed, 6 deletions(-)\n> \n> diff --git a/git.c b/git.c\n> index 7068a184b0a..a769d72ab8f 100644\n> --- a/git.c\n> +++ b/git.c\n> @@ -373,8 +373,6 @@ static int handle_alias(int *argcp, const char ***argv)\n>  \t\t\tstrvec_pushv(&child.args, (*argv) + 1);\n>  \n>  \t\t\ttrace2_cmd_alias(alias_command, child.args.v);\n> -\t\t\ttrace2_cmd_list_config();\n> -\t\t\ttrace2_cmd_list_env_vars();\n>  \t\t\ttrace2_cmd_name(\"_run_shell_alias_\");\n>  \n>  \t\t\tret = run_command(&child);\n> @@ -411,8 +409,6 @@ static int handle_alias(int *argcp, const char ***argv)\n>  \t\tCOPY_ARRAY(new_argv + count, *argv + 1, *argcp);\n>  \n>  \t\ttrace2_cmd_alias(alias_command, new_argv);\n> -\t\ttrace2_cmd_list_config();\n> -\t\ttrace2_cmd_list_env_vars();\n>  \n>  \t\t*argv = new_argv;\n>  \t\t*argcp += count - 1;\n> @@ -462,8 +458,6 @@ static int run_builtin(struct cmd_struct *p, int argc, const char **argv)\n>  \n>  \ttrace_argv_printf(argv, \"trace: built-in: git\");\n>  \ttrace2_cmd_name(p->cmd);\n> -\ttrace2_cmd_list_config();\n> -\ttrace2_cmd_list_env_vars();\n>  \n>  \tvalidate_cache_entries(the_repository->index);\n>  \tstatus = p->fn(argc, argv, prefix);\n> -- \n> gitgitgadget\n> \n\nI'd personally prefer to see this squashed into Patch 3, but I don't\nfeel too strongly about it. Either way, the series LGTM.\n\nReviewed-by: Josh Steadmon <steadmon@google.com>\n"},{"id":"490119","messageId":"xmqqwmqfowfo.fsf@gitster.g","threadId":"61047","inReplyTo":"ZejkVOVQBZhLVfHW@google.com","subject":"Re: [PATCH 4/4] trace2: remove unneeded calls to generate 'def_param' set","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-03-06T21:57:31Z","receivedAt":"2024-03-06T21:57:34Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Josh Steadmon <steadmon@google.com> writes:\n\n> On 2024.03.04 15:40, Jeff Hostetler via GitGitGadget wrote:\n>> From: Jeff Hostetler <jeffhostetler@github.com>\n>> \n>> Now that \"trace2_cmd_name()\" implicitly calls \"trace2_cmd_list_config()\"\n>> and \"trace2_cmd_list_env_vars()\", we don't need to explicitly call them.\n>> \n>> Signed-off-by: Jeff Hostetler <jeffhostetler@github.com>\n>> ---\n>>  git.c | 6 ------\n>>  1 file changed, 6 deletions(-)\n>> \n>> diff --git a/git.c b/git.c\n>> index 7068a184b0a..a769d72ab8f 100644\n>> --- a/git.c\n>> +++ b/git.c\n>> @@ -373,8 +373,6 @@ static int handle_alias(int *argcp, const char ***argv)\n>>  \t\t\tstrvec_pushv(&child.args, (*argv) + 1);\n>>  \n>>  \t\t\ttrace2_cmd_alias(alias_command, child.args.v);\n>> -\t\t\ttrace2_cmd_list_config();\n>> -\t\t\ttrace2_cmd_list_env_vars();\n>>  \t\t\ttrace2_cmd_name(\"_run_shell_alias_\");\n>>  \n>>  \t\t\tret = run_command(&child);\n>> @@ -411,8 +409,6 @@ static int handle_alias(int *argcp, const char ***argv)\n>>  \t\tCOPY_ARRAY(new_argv + count, *argv + 1, *argcp);\n>>  \n>>  \t\ttrace2_cmd_alias(alias_command, new_argv);\n>> -\t\ttrace2_cmd_list_config();\n>> -\t\ttrace2_cmd_list_env_vars();\n>>  \n>>  \t\t*argv = new_argv;\n>>  \t\t*argcp += count - 1;\n>> @@ -462,8 +458,6 @@ static int run_builtin(struct cmd_struct *p, int argc, const char **argv)\n>>  \n>>  \ttrace_argv_printf(argv, \"trace: built-in: git\");\n>>  \ttrace2_cmd_name(p->cmd);\n>> -\ttrace2_cmd_list_config();\n>> -\ttrace2_cmd_list_env_vars();\n>>  \n>>  \tvalidate_cache_entries(the_repository->index);\n>>  \tstatus = p->fn(argc, argv, prefix);\n>> -- \n>> gitgitgadget\n>> \n>\n> I'd personally prefer to see this squashed into Patch 3, but I don't\n> feel too strongly about it. Either way, the series LGTM.\n>\n> Reviewed-by: Josh Steadmon <steadmon@google.com>\n\nLet's see what JeffH says about this.  I agree with you that making\nsome stuff redundant in [Patch 3/4] and fixing the redundancy in\nthis step does feel somewhat roundabout way of doing this.\n\nThanks.\n"},{"id":"490121","messageId":"f1c1847f-49fa-5573-55f4-7cca401df401@jeffhostetler.com","threadId":"61047","inReplyTo":"xmqqwmqfowfo.fsf@gitster.g","subject":"Re: [PATCH 4/4] trace2: remove unneeded calls to generate 'def_param' set","fromName":"Jeff Hostetler","fromEmail":"git@jeffhostetler.com","sentAt":"2024-03-06T22:54:36Z","receivedAt":"2024-03-06T22:54:37Z","isPatch":true,"sender":{"key":"git@jeffhostetler.com","avatar":null},"body":"\n\nOn 3/6/24 4:57 PM, Junio C Hamano wrote:\n> Josh Steadmon <steadmon@google.com> writes:\n> \n>> On 2024.03.04 15:40, Jeff Hostetler via GitGitGadget wrote:\n>>> From: Jeff Hostetler <jeffhostetler@github.com>\n>>>\n>>> Now that \"trace2_cmd_name()\" implicitly calls \"trace2_cmd_list_config()\"\n>>> and \"trace2_cmd_list_env_vars()\", we don't need to explicitly call them.\n>>>\n>>> Signed-off-by: Jeff Hostetler <jeffhostetler@github.com>\n>>> ---\n>>>   git.c | 6 ------\n>>>   1 file changed, 6 deletions(-)\n>>>\n>>> diff --git a/git.c b/git.c\n>>> index 7068a184b0a..a769d72ab8f 100644\n>>> --- a/git.c\n>>> +++ b/git.c\n>>> @@ -373,8 +373,6 @@ static int handle_alias(int *argcp, const char ***argv)\n>>>   \t\t\tstrvec_pushv(&child.args, (*argv) + 1);\n>>>   \n>>>   \t\t\ttrace2_cmd_alias(alias_command, child.args.v);\n>>> -\t\t\ttrace2_cmd_list_config();\n>>> -\t\t\ttrace2_cmd_list_env_vars();\n>>>   \t\t\ttrace2_cmd_name(\"_run_shell_alias_\");\n>>>   \n>>>   \t\t\tret = run_command(&child);\n>>> @@ -411,8 +409,6 @@ static int handle_alias(int *argcp, const char ***argv)\n>>>   \t\tCOPY_ARRAY(new_argv + count, *argv + 1, *argcp);\n>>>   \n>>>   \t\ttrace2_cmd_alias(alias_command, new_argv);\n>>> -\t\ttrace2_cmd_list_config();\n>>> -\t\ttrace2_cmd_list_env_vars();\n>>>   \n>>>   \t\t*argv = new_argv;\n>>>   \t\t*argcp += count - 1;\n>>> @@ -462,8 +458,6 @@ static int run_builtin(struct cmd_struct *p, int argc, const char **argv)\n>>>   \n>>>   \ttrace_argv_printf(argv, \"trace: built-in: git\");\n>>>   \ttrace2_cmd_name(p->cmd);\n>>> -\ttrace2_cmd_list_config();\n>>> -\ttrace2_cmd_list_env_vars();\n>>>   \n>>>   \tvalidate_cache_entries(the_repository->index);\n>>>   \tstatus = p->fn(argc, argv, prefix);\n>>> -- \n>>> gitgitgadget\n>>>\n>>\n>> I'd personally prefer to see this squashed into Patch 3, but I don't\n>> feel too strongly about it. Either way, the series LGTM.\n>>\n>> Reviewed-by: Josh Steadmon <steadmon@google.com>\n> \n> Let's see what JeffH says about this.  I agree with you that making\n> some stuff redundant in [Patch 3/4] and fixing the redundancy in\n> this step does feel somewhat roundabout way of doing this.\n> \n> Thanks.\n> \n\nSure we can merge them.  That's fine.  I can send a V4 or if you want\nto just squash them together that's fine.\n\nJeff\n"},{"id":"490122","messageId":"xmqqsf13otii.fsf@gitster.g","threadId":"61047","inReplyTo":"f1c1847f-49fa-5573-55f4-7cca401df401@jeffhostetler.com","subject":"Re: [PATCH 4/4] trace2: remove unneeded calls to generate 'def_param' set","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-03-06T23:00:37Z","receivedAt":"2024-03-06T23:00:40Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff Hostetler <git@jeffhostetler.com> writes:\n\n>>> Reviewed-by: Josh Steadmon <steadmon@google.com>\n>> Let's see what JeffH says about this.  I agree with you that making\n>> some stuff redundant in [Patch 3/4] and fixing the redundancy in\n>> this step does feel somewhat roundabout way of doing this.\n>> Thanks.\n>> \n>\n> Sure we can merge them.  That's fine.  I can send a V4 or if you want\n> to just squash them together that's fine.\n\nLet's have a v4 describing the change for combined 3&4 in your\nwords, with Josh's Reviewed-by: already added to the trailers.\n\nThanks, both of you.\n"},{"id":"490187","messageId":"pull.1679.v2.git.1709824949.gitgitgadget@gmail.com","threadId":"61047","inReplyTo":"pull.1679.git.1709566808.gitgitgadget@gmail.com","subject":"[PATCH v2 0/3] trace2: move generation of 'def_param' events into code for 'cmd_name'","fromName":"Jeff Hostetler via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-03-07T15:22:26Z","receivedAt":"2024-03-07T15:22:32Z","isPatch":true,"sender":{"key":"git@jeffhostetler.com","avatar":null},"body":"Here is version 2 of this series. The only change from V1 is to combine the\nlast two commits as discussed.\n\nThanks Jeff\n\n----------------------------------------------------------------------------\n\nSome Git commands do not emit def_param events for interesting config and\nenvironment variable settings. Let's fix that.\n\nBuiltin commands compiled into git.c have the normal control flow and emit a\ncmd_name event and then def_param events for each interesting config and\nenvironment variable. However, some special \"query\" commands, like\n--exec-path, or some forms of alias expansion, emitted a cmd_name but did\nnot emit def_param events.\n\nAlso, special commands git-remote-https is built from remote-curl.c and\ngit-http-fetch is built from http-fetch.c and do not use the normal set up\nin git.c. These emitted a cmd_name but not def_param events.\n\nTo minimize the footprint of this commit, move the calls to\ntrace2_cmd_list_config() and trace2_cmd_list_env_vars() into\ntrace2_cmd_name() so that we always get a set of def_param events when a\ncmd_name event is generated.\n\nUsers can define local config settings on a repo to classify/name a repo\n(e.g. \"project-foo\" vs \"personal\") and use the def_param feature to label\nTrace2 data so that (a third-party) telemetry service does not collect data\non personal repos or so that telemetry from one work repo is distinguishable\nfrom another work repo in database queries.\n\nJeff Hostetler (3):\n  t0211: demonstrate missing 'def_param' events for certain commands\n  trace2: avoid emitting 'def_param' set more than once\n  trace2: emit 'def_param' set with 'cmd_name' event\n\n git.c                  |   6 --\n t/t0211-trace2-perf.sh | 231 +++++++++++++++++++++++++++++++++++++++++\n trace2.c               |  15 +++\n 3 files changed, 246 insertions(+), 6 deletions(-)\n\n\nbase-commit: 0f9d4d28b7e6021b7e6db192b7bf47bd3a0d0d1d\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1679%2Fjeffhostetler%2Falways-emit-def-param-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1679/jeffhostetler/always-emit-def-param-v2\nPull-Request: https://github.com/gitgitgadget/git/pull/1679\n\nRange-diff vs v1:\n\n 1:  b378b93242a = 1:  b378b93242a t0211: demonstrate missing 'def_param' events for certain commands\n 2:  65068e97597 = 2:  65068e97597 trace2: avoid emitting 'def_param' set more than once\n 3:  9507184b4f1 ! 3:  178721cd4f0 trace2: emit 'def_param' set with 'cmd_name' event\n     @@ Commit message\n          the \"trace2_cmd_name()\" function to generate the set of 'def_param'\n          events.\n      \n     -    We can later remove explicit calls to \"trace2_cmd_list_config()\" and\n     -    \"trace2_cmd_list_env_vars()\" in git.c.\n     +    Remove explicit calls to \"trace2_cmd_list_config()\" and\n     +    \"trace2_cmd_list_env_vars()\" in git.c since they are no longer needed.\n      \n     +    Reviewed-by: Josh Steadmon <steadmon@google.com>\n          Signed-off-by: Jeff Hostetler <jeffhostetler@github.com>\n      \n     + ## git.c ##\n     +@@ git.c: static int handle_alias(int *argcp, const char ***argv)\n     + \t\t\tstrvec_pushv(&child.args, (*argv) + 1);\n     + \n     + \t\t\ttrace2_cmd_alias(alias_command, child.args.v);\n     +-\t\t\ttrace2_cmd_list_config();\n     +-\t\t\ttrace2_cmd_list_env_vars();\n     + \t\t\ttrace2_cmd_name(\"_run_shell_alias_\");\n     + \n     + \t\t\tret = run_command(&child);\n     +@@ git.c: static int handle_alias(int *argcp, const char ***argv)\n     + \t\tCOPY_ARRAY(new_argv + count, *argv + 1, *argcp);\n     + \n     + \t\ttrace2_cmd_alias(alias_command, new_argv);\n     +-\t\ttrace2_cmd_list_config();\n     +-\t\ttrace2_cmd_list_env_vars();\n     + \n     + \t\t*argv = new_argv;\n     + \t\t*argcp += count - 1;\n     +@@ git.c: static int run_builtin(struct cmd_struct *p, int argc, const char **argv)\n     + \n     + \ttrace_argv_printf(argv, \"trace: built-in: git\");\n     + \ttrace2_cmd_name(p->cmd);\n     +-\ttrace2_cmd_list_config();\n     +-\ttrace2_cmd_list_env_vars();\n     + \n     + \tvalidate_cache_entries(the_repository->index);\n     + \tstatus = p->fn(argc, argv, prefix);\n     +\n       ## t/t0211-trace2-perf.sh ##\n      @@ t/t0211-trace2-perf.sh: test_expect_success 'expect def_params for normal builtin command' '\n       # Representative query command dispatched in handle_options()\n 4:  e8528715ebf < -:  ----------- trace2: remove unneeded calls to generate 'def_param' set\n\n-- \ngitgitgadget\n"},{"id":"490188","messageId":"65068e97597241e297f5d7cdb60012be1784e9dc.1709824949.git.gitgitgadget@gmail.com","threadId":"61047","inReplyTo":"pull.1679.v2.git.1709824949.gitgitgadget@gmail.com","subject":"[PATCH v2 2/3] trace2: avoid emitting 'def_param' set more than once","fromName":"Jeff Hostetler via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-03-07T15:22:28Z","receivedAt":"2024-03-07T15:22:33Z","isPatch":true,"sender":{"key":"git@jeffhostetler.com","avatar":null},"body":"From: Jeff Hostetler <jeffhostetler@github.com>\n\nDuring nested alias expansion it is possible for\n\"trace2_cmd_list_config()\" and \"trace2_cmd_list_env_vars()\"\nto be called more than once.  This causes a full set of\n'def_param' events to be emitted each time.  Let's avoid\nthat.\n\nAdd code to those two functions to only emit them once.\n\nSigned-off-by: Jeff Hostetler <jeffhostetler@github.com>\n---\n t/t0211-trace2-perf.sh |  2 +-\n trace2.c               | 12 ++++++++++++\n 2 files changed, 13 insertions(+), 1 deletion(-)\n\ndiff --git a/t/t0211-trace2-perf.sh b/t/t0211-trace2-perf.sh\nindex 588c5bad033..7b353195396 100755\n--- a/t/t0211-trace2-perf.sh\n+++ b/t/t0211-trace2-perf.sh\n@@ -470,7 +470,7 @@ test_expect_success 'expect def_params during shell alias expansion' '\n \tgrep \"d1|main|def_param|.*|ENV_PROP_FOO:blue\" actual\n '\n \n-test_expect_failure 'expect def_params during nested git alias expansion' '\n+test_expect_success 'expect def_params during nested git alias expansion' '\n \ttest_when_finished \"rm prop.perf actual\" &&\n \n \ttest_config_global \"trace2.configParams\" \"cfg.prop.*\" &&\ndiff --git a/trace2.c b/trace2.c\nindex f1e268bd159..facce641ef3 100644\n--- a/trace2.c\n+++ b/trace2.c\n@@ -464,17 +464,29 @@ void trace2_cmd_alias_fl(const char *file, int line, const char *alias,\n \n void trace2_cmd_list_config_fl(const char *file, int line)\n {\n+\tstatic int emitted = 0;\n+\n \tif (!trace2_enabled)\n \t\treturn;\n \n+\tif (emitted)\n+\t\treturn;\n+\temitted = 1;\n+\n \ttr2_cfg_list_config_fl(file, line);\n }\n \n void trace2_cmd_list_env_vars_fl(const char *file, int line)\n {\n+\tstatic int emitted = 0;\n+\n \tif (!trace2_enabled)\n \t\treturn;\n \n+\tif (emitted)\n+\t\treturn;\n+\temitted = 1;\n+\n \ttr2_list_env_vars_fl(file, line);\n }\n \n-- \ngitgitgadget\n\n"},{"id":"490189","messageId":"b378b93242a7870772fdb53d7bf2d58d3347ba62.1709824949.git.gitgitgadget@gmail.com","threadId":"61047","inReplyTo":"pull.1679.v2.git.1709824949.gitgitgadget@gmail.com","subject":"[PATCH v2 1/3] t0211: demonstrate missing 'def_param' events for certain commands","fromName":"Jeff Hostetler via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-03-07T15:22:27Z","receivedAt":"2024-03-07T15:22:33Z","isPatch":true,"sender":{"key":"git@jeffhostetler.com","avatar":null},"body":"From: Jeff Hostetler <jeffhostetler@github.com>\n\nSome Git commands fail to emit 'def_param' events for interesting\nconfig and environment variable settings.\n\nAdd unit tests to demonstrate this.\n\nMost commands are considered \"builtin\" and are based upon git.c.\nThese typically do emit 'def_param' events.  Exceptions are some of\nthe \"query\" commands, the \"run-dashed\" mechanism, and alias handling.\n\nCommands built from remote-curl.c (instead of git.c), such as\n\"git-remote-https\", do not emit 'def_param' events.\n\nLikewise, \"git-http-fetch\" is built http-fetch.c and does not emit\nthem.\n\nSigned-off-by: Jeff Hostetler <jeffhostetler@github.com>\n---\n t/t0211-trace2-perf.sh | 231 +++++++++++++++++++++++++++++++++++++++++\n 1 file changed, 231 insertions(+)\n\ndiff --git a/t/t0211-trace2-perf.sh b/t/t0211-trace2-perf.sh\nindex 290b6eaaab1..588c5bad033 100755\n--- a/t/t0211-trace2-perf.sh\n+++ b/t/t0211-trace2-perf.sh\n@@ -287,4 +287,235 @@ test_expect_success 'unsafe URLs are redacted by default' '\n \tgrep \"d0|main|def_param|.*|remote.origin.url:https://user:pwd@example.com\" actual\n '\n \n+# Confirm that the requested command produces a \"cmd_name\" and a\n+# set of \"def_param\" events.\n+#\n+try_simple () {\n+\ttest_when_finished \"rm prop.perf actual\" &&\n+\n+\tcmd=$1 &&\n+\tcmd_name=$2 &&\n+\n+\ttest_config_global \"trace2.configParams\" \"cfg.prop.*\" &&\n+\ttest_config_global \"trace2.envvars\" \"ENV_PROP_FOO,ENV_PROP_BAR\" &&\n+\n+\ttest_config_global \"cfg.prop.foo\" \"red\" &&\n+\n+\tENV_PROP_FOO=blue \\\n+\t\tGIT_TRACE2_PERF=\"$(pwd)/prop.perf\" \\\n+\t\t\t$cmd &&\n+\tperl \"$TEST_DIRECTORY/t0211/scrub_perf.perl\" <prop.perf >actual &&\n+\tgrep \"d0|main|cmd_name|.*|$cmd_name\" actual &&\n+\tgrep \"d0|main|def_param|.*|cfg.prop.foo:red\" actual &&\n+\tgrep \"d0|main|def_param|.*|ENV_PROP_FOO:blue\" actual\n+}\n+\n+# Representative mainstream builtin Git command dispatched\n+# in run_builtin() in git.c\n+#\n+test_expect_success 'expect def_params for normal builtin command' '\n+\ttry_simple \"git version\" \"version\"\n+'\n+\n+# Representative query command dispatched in handle_options()\n+# in git.c\n+#\n+test_expect_failure 'expect def_params for query command' '\n+\ttry_simple \"git --man-path\" \"_query_\"\n+'\n+\n+# remote-curl.c does not use the builtin setup in git.c, so confirm\n+# that executables built from remote-curl.c emit def_params.\n+#\n+# Also tests the dashed-command handling where \"git foo\" silently\n+# spawns \"git-foo\".  Make sure that both commands should emit\n+# def_params.\n+#\n+# Pass bogus arguments to remote-https and allow the command to fail\n+# because we don't actually have a remote to fetch from.  We just want\n+# to see the run-dashed code run an executable built from\n+# remote-curl.c rather than git.c.  Confirm that we get def_param\n+# events from both layers.\n+#\n+test_expect_failure 'expect def_params for remote-curl and _run_dashed_' '\n+\ttest_when_finished \"rm prop.perf actual\" &&\n+\n+\ttest_config_global \"trace2.configParams\" \"cfg.prop.*\" &&\n+\ttest_config_global \"trace2.envvars\" \"ENV_PROP_FOO,ENV_PROP_BAR\" &&\n+\n+\ttest_config_global \"cfg.prop.foo\" \"red\" &&\n+\n+\ttest_might_fail env \\\n+\t\tENV_PROP_FOO=blue \\\n+\t\tGIT_TRACE2_PERF=\"$(pwd)/prop.perf\" \\\n+\t\tgit remote-http x y &&\n+\n+\tperl \"$TEST_DIRECTORY/t0211/scrub_perf.perl\" <prop.perf >actual &&\n+\n+\tgrep \"d0|main|cmd_name|.*|_run_dashed_\" actual &&\n+\tgrep \"d0|main|def_param|.*|cfg.prop.foo:red\" actual &&\n+\tgrep \"d0|main|def_param|.*|ENV_PROP_FOO:blue\" actual &&\n+\n+\tgrep \"d1|main|cmd_name|.*|remote-curl\" actual &&\n+\tgrep \"d1|main|def_param|.*|cfg.prop.foo:red\" actual &&\n+\tgrep \"d1|main|def_param|.*|ENV_PROP_FOO:blue\" actual\n+'\n+\n+# Similarly, `git-http-fetch` is not built from git.c so do a\n+# trivial fetch so that the main git.c run-dashed code spawns\n+# an executable built from http-fetch.c.  Confirm that we get\n+# def_param events from both layers.\n+#\n+test_expect_failure 'expect def_params for http-fetch and _run_dashed_' '\n+\ttest_when_finished \"rm prop.perf actual\" &&\n+\n+\ttest_config_global \"trace2.configParams\" \"cfg.prop.*\" &&\n+\ttest_config_global \"trace2.envvars\" \"ENV_PROP_FOO,ENV_PROP_BAR\" &&\n+\n+\ttest_config_global \"cfg.prop.foo\" \"red\" &&\n+\n+\ttest_might_fail env \\\n+\t\tENV_PROP_FOO=blue \\\n+\t\tGIT_TRACE2_PERF=\"$(pwd)/prop.perf\" \\\n+\t\tgit http-fetch --stdin file:/// <<-EOF &&\n+\tEOF\n+\n+\tperl \"$TEST_DIRECTORY/t0211/scrub_perf.perl\" <prop.perf >actual &&\n+\n+\tgrep \"d0|main|cmd_name|.*|_run_dashed_\" actual &&\n+\tgrep \"d0|main|def_param|.*|cfg.prop.foo:red\" actual &&\n+\tgrep \"d0|main|def_param|.*|ENV_PROP_FOO:blue\" actual &&\n+\n+\tgrep \"d1|main|cmd_name|.*|http-fetch\" actual &&\n+\tgrep \"d1|main|def_param|.*|cfg.prop.foo:red\" actual &&\n+\tgrep \"d1|main|def_param|.*|ENV_PROP_FOO:blue\" actual\n+'\n+\n+# Historically, alias expansion explicitly emitted the def_param\n+# events (independent of whether the command was a builtin, a Git\n+# command or arbitrary shell command) so that it wasn't dependent\n+# upon the unpeeling of the alias. Let's make sure that we preserve\n+# the net effect.\n+#\n+test_expect_success 'expect def_params during git alias expansion' '\n+\ttest_when_finished \"rm prop.perf actual\" &&\n+\n+\ttest_config_global \"trace2.configParams\" \"cfg.prop.*\" &&\n+\ttest_config_global \"trace2.envvars\" \"ENV_PROP_FOO,ENV_PROP_BAR\" &&\n+\n+\ttest_config_global \"cfg.prop.foo\" \"red\" &&\n+\n+\ttest_config_global \"alias.xxx\" \"version\" &&\n+\n+\tENV_PROP_FOO=blue \\\n+\t\tGIT_TRACE2_PERF=\"$(pwd)/prop.perf\" \\\n+\t\t\tgit xxx &&\n+\n+\tperl \"$TEST_DIRECTORY/t0211/scrub_perf.perl\" <prop.perf >actual &&\n+\n+\t# \"git xxx\" is first mapped to \"git-xxx\" and the child will fail.\n+\tgrep \"d0|main|cmd_name|.*|_run_dashed_ (_run_dashed_)\" actual &&\n+\n+\t# We unpeel that and substitute \"version\" into \"xxx\" (giving\n+\t# \"git version\") and update the cmd_name event.\n+\tgrep \"d0|main|cmd_name|.*|_run_git_alias_ (_run_dashed_/_run_git_alias_)\" actual &&\n+\n+\t# These def_param events could be associated with either of the\n+\t# above cmd_name events.  It does not matter.\n+\tgrep \"d0|main|def_param|.*|cfg.prop.foo:red\" actual &&\n+\tgrep \"d0|main|def_param|.*|ENV_PROP_FOO:blue\" actual &&\n+\n+\t# The \"git version\" child sees a different cmd_name hierarchy.\n+\t# Also test the def_param (only for completeness).\n+\tgrep \"d1|main|cmd_name|.*|version (_run_dashed_/_run_git_alias_/version)\" actual &&\n+\tgrep \"d1|main|def_param|.*|cfg.prop.foo:red\" actual &&\n+\tgrep \"d1|main|def_param|.*|ENV_PROP_FOO:blue\" actual\n+'\n+\n+test_expect_success 'expect def_params during shell alias expansion' '\n+\ttest_when_finished \"rm prop.perf actual\" &&\n+\n+\ttest_config_global \"trace2.configParams\" \"cfg.prop.*\" &&\n+\ttest_config_global \"trace2.envvars\" \"ENV_PROP_FOO,ENV_PROP_BAR\" &&\n+\n+\ttest_config_global \"cfg.prop.foo\" \"red\" &&\n+\n+\ttest_config_global \"alias.xxx\" \"!git version\" &&\n+\n+\tENV_PROP_FOO=blue \\\n+\t\tGIT_TRACE2_PERF=\"$(pwd)/prop.perf\" \\\n+\t\t\tgit xxx &&\n+\n+\tperl \"$TEST_DIRECTORY/t0211/scrub_perf.perl\" <prop.perf >actual &&\n+\n+\t# \"git xxx\" is first mapped to \"git-xxx\" and the child will fail.\n+\tgrep \"d0|main|cmd_name|.*|_run_dashed_ (_run_dashed_)\" actual &&\n+\n+\t# We unpeel that and substitute \"git version\" for \"git xxx\" (as a\n+\t# shell command.  Another cmd_name event is emitted as we unpeel.\n+\tgrep \"d0|main|cmd_name|.*|_run_shell_alias_ (_run_dashed_/_run_shell_alias_)\" actual &&\n+\n+\t# These def_param events could be associated with either of the\n+\t# above cmd_name events.  It does not matter.\n+\tgrep \"d0|main|def_param|.*|cfg.prop.foo:red\" actual &&\n+\tgrep \"d0|main|def_param|.*|ENV_PROP_FOO:blue\" actual &&\n+\n+\t# We get the following only because we used a git command for the\n+\t# shell command. In general, it could have been a shell script and\n+\t# we would see nothing.\n+\t#\n+\t# The child knows the cmd_name hierarchy so it includes it.\n+\tgrep \"d1|main|cmd_name|.*|version (_run_dashed_/_run_shell_alias_/version)\" actual &&\n+\tgrep \"d1|main|def_param|.*|cfg.prop.foo:red\" actual &&\n+\tgrep \"d1|main|def_param|.*|ENV_PROP_FOO:blue\" actual\n+'\n+\n+test_expect_failure 'expect def_params during nested git alias expansion' '\n+\ttest_when_finished \"rm prop.perf actual\" &&\n+\n+\ttest_config_global \"trace2.configParams\" \"cfg.prop.*\" &&\n+\ttest_config_global \"trace2.envvars\" \"ENV_PROP_FOO,ENV_PROP_BAR\" &&\n+\n+\ttest_config_global \"cfg.prop.foo\" \"red\" &&\n+\n+\ttest_config_global \"alias.xxx\" \"yyy\" &&\n+\ttest_config_global \"alias.yyy\" \"version\" &&\n+\n+\tENV_PROP_FOO=blue \\\n+\t\tGIT_TRACE2_PERF=\"$(pwd)/prop.perf\" \\\n+\t\t\tgit xxx &&\n+\n+\tperl \"$TEST_DIRECTORY/t0211/scrub_perf.perl\" <prop.perf >actual &&\n+\n+\t# \"git xxx\" is first mapped to \"git-xxx\" and try to spawn \"git-xxx\"\n+\t# and the child will fail.\n+\tgrep \"d0|main|cmd_name|.*|_run_dashed_ (_run_dashed_)\" actual &&\n+\tgrep \"d0|main|child_start|.*|.* class:dashed argv:\\[git-xxx\\]\" actual &&\n+\n+\t# We unpeel that and substitute \"yyy\" into \"xxx\" (giving \"git yyy\")\n+\t# and spawn \"git-yyy\" and the child will fail.\n+\tgrep \"d0|main|alias|.*|alias:xxx argv:\\[yyy\\]\" actual &&\n+\tgrep \"d0|main|cmd_name|.*|_run_dashed_ (_run_dashed_/_run_dashed_)\" actual &&\n+\tgrep \"d0|main|child_start|.*|.* class:dashed argv:\\[git-yyy\\]\" actual &&\n+\n+\t# We unpeel that and substitute \"version\" into \"xxx\" (giving\n+\t# \"git version\") and update the cmd_name event.\n+\tgrep \"d0|main|alias|.*|alias:yyy argv:\\[version\\]\" actual &&\n+\tgrep \"d0|main|cmd_name|.*|_run_git_alias_ (_run_dashed_/_run_dashed_/_run_git_alias_)\" actual &&\n+\n+\t# These def_param events could be associated with any of the\n+\t# above cmd_name events.  It does not matter.\n+\tgrep \"d0|main|def_param|.*|cfg.prop.foo:red\" actual >actual.matches &&\n+\tgrep \"d0|main|def_param|.*|ENV_PROP_FOO:blue\" actual &&\n+\n+\t# However, we do not want them repeated each time we unpeel.\n+\ttest_line_count = 1 actual.matches &&\n+\n+\t# The \"git version\" child sees a different cmd_name hierarchy.\n+\t# Also test the def_param (only for completeness).\n+\tgrep \"d1|main|cmd_name|.*|version (_run_dashed_/_run_dashed_/_run_git_alias_/version)\" actual &&\n+\tgrep \"d1|main|def_param|.*|cfg.prop.foo:red\" actual &&\n+\tgrep \"d1|main|def_param|.*|ENV_PROP_FOO:blue\" actual\n+'\n+\n test_done\n-- \ngitgitgadget\n\n"},{"id":"490190","messageId":"178721cd4f044af44b9d7e625cabf63c5e19c75d.1709824949.git.gitgitgadget@gmail.com","threadId":"61047","inReplyTo":"pull.1679.v2.git.1709824949.gitgitgadget@gmail.com","subject":"[PATCH v2 3/3] trace2: emit 'def_param' set with 'cmd_name' event","fromName":"Jeff Hostetler via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-03-07T15:22:29Z","receivedAt":"2024-03-07T15:22:35Z","isPatch":true,"sender":{"key":"git@jeffhostetler.com","avatar":null},"body":"From: Jeff Hostetler <jeffhostetler@github.com>\n\nSome commands do not cause a set of 'def_param' events to be emitted.\nThis includes \"git-remote-https\", \"git-http-fetch\", and various\n\"query\" commands, like \"git --man-path\".\n\nSince all of these commands do emit a 'cmd_name' event, add code to\nthe \"trace2_cmd_name()\" function to generate the set of 'def_param'\nevents.\n\nRemove explicit calls to \"trace2_cmd_list_config()\" and\n\"trace2_cmd_list_env_vars()\" in git.c since they are no longer needed.\n\nReviewed-by: Josh Steadmon <steadmon@google.com>\nSigned-off-by: Jeff Hostetler <jeffhostetler@github.com>\n---\n git.c                  | 6 ------\n t/t0211-trace2-perf.sh | 6 +++---\n trace2.c               | 3 +++\n 3 files changed, 6 insertions(+), 9 deletions(-)\n\ndiff --git a/git.c b/git.c\nindex 7068a184b0a..a769d72ab8f 100644\n--- a/git.c\n+++ b/git.c\n@@ -373,8 +373,6 @@ static int handle_alias(int *argcp, const char ***argv)\n \t\t\tstrvec_pushv(&child.args, (*argv) + 1);\n \n \t\t\ttrace2_cmd_alias(alias_command, child.args.v);\n-\t\t\ttrace2_cmd_list_config();\n-\t\t\ttrace2_cmd_list_env_vars();\n \t\t\ttrace2_cmd_name(\"_run_shell_alias_\");\n \n \t\t\tret = run_command(&child);\n@@ -411,8 +409,6 @@ static int handle_alias(int *argcp, const char ***argv)\n \t\tCOPY_ARRAY(new_argv + count, *argv + 1, *argcp);\n \n \t\ttrace2_cmd_alias(alias_command, new_argv);\n-\t\ttrace2_cmd_list_config();\n-\t\ttrace2_cmd_list_env_vars();\n \n \t\t*argv = new_argv;\n \t\t*argcp += count - 1;\n@@ -462,8 +458,6 @@ static int run_builtin(struct cmd_struct *p, int argc, const char **argv)\n \n \ttrace_argv_printf(argv, \"trace: built-in: git\");\n \ttrace2_cmd_name(p->cmd);\n-\ttrace2_cmd_list_config();\n-\ttrace2_cmd_list_env_vars();\n \n \tvalidate_cache_entries(the_repository->index);\n \tstatus = p->fn(argc, argv, prefix);\ndiff --git a/t/t0211-trace2-perf.sh b/t/t0211-trace2-perf.sh\nindex 7b353195396..13ef69b92f8 100755\n--- a/t/t0211-trace2-perf.sh\n+++ b/t/t0211-trace2-perf.sh\n@@ -320,7 +320,7 @@ test_expect_success 'expect def_params for normal builtin command' '\n # Representative query command dispatched in handle_options()\n # in git.c\n #\n-test_expect_failure 'expect def_params for query command' '\n+test_expect_success 'expect def_params for query command' '\n \ttry_simple \"git --man-path\" \"_query_\"\n '\n \n@@ -337,7 +337,7 @@ test_expect_failure 'expect def_params for query command' '\n # remote-curl.c rather than git.c.  Confirm that we get def_param\n # events from both layers.\n #\n-test_expect_failure 'expect def_params for remote-curl and _run_dashed_' '\n+test_expect_success 'expect def_params for remote-curl and _run_dashed_' '\n \ttest_when_finished \"rm prop.perf actual\" &&\n \n \ttest_config_global \"trace2.configParams\" \"cfg.prop.*\" &&\n@@ -366,7 +366,7 @@ test_expect_failure 'expect def_params for remote-curl and _run_dashed_' '\n # an executable built from http-fetch.c.  Confirm that we get\n # def_param events from both layers.\n #\n-test_expect_failure 'expect def_params for http-fetch and _run_dashed_' '\n+test_expect_success 'expect def_params for http-fetch and _run_dashed_' '\n \ttest_when_finished \"rm prop.perf actual\" &&\n \n \ttest_config_global \"trace2.configParams\" \"cfg.prop.*\" &&\ndiff --git a/trace2.c b/trace2.c\nindex facce641ef3..f894532d053 100644\n--- a/trace2.c\n+++ b/trace2.c\n@@ -433,6 +433,9 @@ void trace2_cmd_name_fl(const char *file, int line, const char *name)\n \tfor_each_wanted_builtin (j, tgt_j)\n \t\tif (tgt_j->pfn_command_name_fl)\n \t\t\ttgt_j->pfn_command_name_fl(file, line, name, hierarchy);\n+\n+\ttrace2_cmd_list_config();\n+\ttrace2_cmd_list_env_vars();\n }\n \n void trace2_cmd_mode_fl(const char *file, int line, const char *mode)\n-- \ngitgitgadget\n"}]}