From: Jeff Hostetler <git@jeffhostetler.com>
Date: Mon, 18 Jul 2022 20:13:23 GMT
Subject: Re: [PATCH v7 7/7] tr2: dump names if config exist in multiple scopes
Message-ID: <d66f6bc7-4cd0-f585-83e9-9f7e2ee94d68@jeffhostetler.com>
In-Reply-To: <a01ae8478d3a8545241c5b064b6d369a330ee59f.1658159746.git.dyroneteng@gmail.com>



On 7/18/22 12:46 PM, Teng Long wrote:
> When we specify GIT_TRACE2_CONFIG_PARAMS or trace2.configparams,
> trace2 will prints "interesting" config values to log. Sometimes,
> when a config set in multiple scope files, the following output
> looks like (the irrelevant fields are omitted here as "..."):
> 
> ...| def_param    |  ...  | core.multipackindex:false
> ...| def_param    |  ...  | core.multipackindex:false
> ...| def_param    |  ...  | core.multipackindex:false
> 
> As the log shows, even each config in different scope is dumped, but
> we don't know which scope it comes from. Therefore, it's better to
> add the scope names as well to make them be more recognizable. For
> example, when execute:
> 
>      $ GIT_TRACE2_PERF=1 \
>      > GIT_TRACE2_CONFIG_PARAMS=core.multipackIndex \
>      > git rev-list --test-bitmap HEAD"
> 
> The following is the ouput (the irrelevant fields are omitted here
> as "..."):
> 
> Format normal:
> ... git.c:461 ... def_param scope:system core.multipackindex=false
> ... git.c:461 ... def_param scope:global core.multipackindex=false
> ... git.c:461 ... def_param scope:local core.multipackindex=false
> 
> Format perf:
> 
> ... | def_param    | ... | scope:system | core.multipackindex:false
> ... | def_param    | ... | scope:global | core.multipackindex:false
> ... | def_param    | ... | scope:local  | core.multipackindex:false
> 
> Format event:
> 
> {"event":"def_param", ... ,"scope":"system","param":"core.multipackindex","value":"false"}
> {"event":"def_param", ... ,"scope":"global","param":"core.multipackindex","value":"false"}
> {"event":"def_param", ... ,"scope":"local","param":"core.multipackindex","value":"false"}
> 
> Signed-off-by: Teng Long <dyroneteng@gmail.com>
> ---
>   Documentation/technical/api-trace2.txt | 45 ++++++++++++++++++++++++++
>   trace2/tr2_tgt_event.c                 |  3 ++
>   trace2/tr2_tgt_normal.c                |  5 ++-
>   trace2/tr2_tgt_perf.c                  |  9 ++++--
>   4 files changed, 59 insertions(+), 3 deletions(-)
> 
> diff --git a/Documentation/technical/api-trace2.txt b/Documentation/technical/api-trace2.txt
> index bb13ca3db8..48205a5ac5 100644
> --- a/Documentation/technical/api-trace2.txt
> +++ b/Documentation/technical/api-trace2.txt
> @@ -1207,6 +1207,51 @@ at offset 508.
>   This example also shows that thread names are assigned in a racy manner
>   as each thread starts and allocates TLS storage.
>   
> +Print Configs::
> +
> +	  Dump "interesting" config values to trace2 log.
> ++
> +The environment variable `GIT_TRACE2_CONFIG_PARAMS` and configuration
> +`trace2.configparams` can be used to output config values which you care
> +about. For example, assume that we want to config different `color.ui`
> +values in multiple scopes, such as:
> ++
> +----------------
> +$ git config --system color.ui never
> +$ git config --global color.ui always
> +$ git config --local color.ui auto
> +$ git config --list --show-scope | grep 'color.ui'
> +system  color.ui=never
> +global  color.ui=always
> +local   color.ui=auto
> +----------------
> ++
> +Then, mark the config `color.ui` as "interesting" config with
> +`GIT_TRACE2_CONFIG_PARAMS`:
> ++
> +----------------
> +$ export GIT_TRACE2_PERF_BRIEF=1
> +$ export GIT_TRACE2_PERF=~/log.perf
> +$ export GIT_TRACE2_CONFIG_PARAMS=color.ui
> +$ git version
> +...
> +$ cat ~/log.perf
> +d0 | main                     | version      |     |           |           |              | ...
> +d0 | main                     | start        |     |  0.000284 |           |              | /opt/git/master/bin/git version
> +d0 | main                     | cmd_ancestry |     |           |           |              | ancestry:[bash sshd sshd sshd systemd]
> +d0 | main                     | cmd_name     |     |           |           |              | version (version)
> +d0 | main                     | exit         |     |  0.000419 |           |              | code:0
> +d0 | main                     | atexit       |     |  0.000426 |           |              | code:0
> +d0 | main                     | version      |     |           |           |              | ...
> +d0 | main                     | start        |     |  0.000275 |           |              | /opt/git/master/bin/git version
> +d0 | main                     | cmd_ancestry |     |           |           |              | ancestry:[bash sshd sshd sshd systemd]
> +d0 | main                     | cmd_name     |     |           |           |              | version (version)
> +d0 | main                     | def_param    |     |           |           |              | color.ui:never
> +d0 | main                     | def_param    |     |           |           |              | color.ui:always
> +d0 | main                     | def_param    |     |           |           |              | color.ui:auto

shouldn't this example in the docs have the scope value in the
next to last column as you noted in the cover letter ?


> +d0 | main                     | exit         |     |  0.000543 |           |              | code:0
> +d0 | main                     | atexit       |     |  0.000549 |           |              | code:0
> +----------------
>   == Future Work
...

Otherwise, this all LGTM.
Thanks
Jeff