git/list[1] front-page[2] threads[3] people[4] search[5] about
 

Re: [PATCH v7 0/7] trace2: dump scope when print "interesting" config

From
Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date
Jul 19, 2022, 11:42 UTC
Message-ID
<220719.86tu7duxl3.gmgdl@evledraar.gmail.com>
In-Reply-To
<20220719112612.46679-1-tenglong.tl@tenglongtldeMacBook-Pro.local>
On Tue, Jul 19 2022, tenglong.tl wrote:
Show 5 quoted lines
>> Yeah likewise, it even applies directly on master. But I can live with
>> it :)
>
> Sorry, may I ask what's the recommanded way to deal this, alway use
> tag or master on update?

We generally submit patches on a single "topic", what a "topic" is is often fuzzy, and sometimes a topic that's mostly trying to do X will fix or change some unrelated Y "while at it".

But patch or patches at the end of a series don't depend on anything that comes before them, and could be "cherry-pick"'d directly on top of "master" that's generally a sign that you should be submitting two sets of patches, not one.

Per https://lore.kernel.org/git/2016ef2e342c2ec6517afa8ec3e57035021fb965.1650547400.git.dyroneteng@gmail.com/ the "let's log config" is just something you happened to run into on this topic, but it might have just as well been some other command.

So I think it's better to split it up into its own topic.
Show 44 quoted lines
>> One minor nit is that something like this (which needs to be fleshened
>> up) should be fixed up into 7/7 (and maybe we want to keep the "..."?):
>>
>> diff --git a/Documentation/technical/api-trace2.txt b/Documentation/technical/api-trace2.txt
>> index 49bb1ca1924..ce544982a37 100644
>> --- a/Documentation/technical/api-trace2.txt
>> +++ b/Documentation/technical/api-trace2.txt
>> @@ -716,7 +716,7 @@ The "exec_id" field is a command-unique id and is only useful if the
>>  ------------
>>  {
>>  	"event":"def_param",
>> -	...
>> +	"scope": ...
>>  	"param":"core.abbrev",
>>  	"value":"7"
>>  }
>
> I do a little check, the json format in this scenario is like :
>
> {
>   "event": "def_param",
>   "sid": "20220719T075535.279369Z-H1b0a19dc-P000093db",
>   "thread": "main",
>   "time": "2022-07-19T07:55:35.280720Z",
>   "file": "git.c",
>   "line": 461,
>   "scope": "global",
>   "param": "color.ui",
>   "value": "always"
> }
>
> So, I think this is ok maybe:
>
> diff --git a/Documentation/technical/api-trace2.txt b/Documentation/technical/api-trace2.txt
> index bb13ca3db8..d66da52686 100644
> --- a/Documentation/technical/api-trace2.txt
> +++ b/Documentation/technical/api-trace2.txt
> @@ -717,6 +717,7 @@ The "exec_id" field is a command-unique id and is only useful if the
>  {
>         "event":"def_param",
>         ...
> +       "scope":...
>         "param":"core.abbrev",
>         "value":"7"
Yes, that sound good, although I'd make that "scope" line be:
	"scope": "global" | "worktree" | <add more things to the list here>
Or just say:
	"scope": <a string that 'git config --show-scope' would return>,
Which covers all the possibilities, without hardcoding them there.
Show 9 quoted lines
>  }
>
>> And that the addition to api-trace2.txt seems to partially be something
>> that should just link to Documentation/config/trace2.txt, i.e. it's
>> generally documenting an existing facility.
>
> Do you mean the modification about `trace2.configParams` and you suggest
> to make a link to Documentation/config/trace2.txt(actually as git-config[1])
> in /Documentation/technical/api-trace2.txt?

I mean that part of what you're adding is about this new "scope" feature, but another part just seems to be explaining how the trace2.configParams works in general.

For the "works in general" let's either link to git-config(1), or if that explanation is lacking improve it & link to it.

Show 71 quoted lines
>> I think it would be great in any case to have that 7/7 split into what
>> we do now & docs for that, and then the minor addition of "scope".
>
> Let me try to understand this, it's better to split [7/7] into two commits.
>
> First commit is to add docs in /Documentation/technical/api-trace2.txt to let
> reader to find the print-config ability, by the way link the doc about the
> GIT_TRACE2_CONFIG_PARAM and trace2.configparami in git-config[1].
>
> Like:
>
> commit 2db47572d4462e3788a92fd355b97df13b9bcc39
> Author: Teng Long <dyroneteng@gmail.com>
> Date:   Tue Jul 19 17:30:35 2022 +0800
>
>     api-trace2.txt: add docs to print config
>
>     It's supported to print "interesting" config value to tr2 log by
>     setting the "GIT_TRACE2_CONFIG_PARAMS" environment variable and
>     the "trace2.configparam" config, let's add the related docs.
>
>     Signed-off-by: Teng Long <dyroneteng@gmail.com>
>
> diff --git a/Documentation/technical/api-trace2.txt b/Documentation/technical/api-trace2.txt
> index bb13ca3db8..4e411f3306 100644
> --- a/Documentation/technical/api-trace2.txt
> +++ b/Documentation/technical/api-trace2.txt
> @@ -1207,6 +1207,45 @@ 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(see linkgit:git-config[1). 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.000260 |           |              | /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
> +d0 | main                     | exit         |     |  0.000470 |           |              | code:0
> +d0 | main                     | atexit       |     |  0.000477 |           |              | code:0
> +----------------
>  == Future Work

Yes, something like that, although it's a bit odd to discuss "scope" here and not have the trace show it yet, but that's fixed below.:

Show 138 quoted lines
> Second is actually what we do in [7/7], modify to support print scope names
> and update the docs which based on what we just add in the first commit
>
> Like:
>
>
> commit 615ab4864fce6b8042778aa78799ac2656785710 (HEAD -> tl/bitmap-append-trace2-outputs)
> Author: Teng Long <dyroneteng@gmail.com>
> Date:   Thu Jul 7 21:48:32 2022 +0800
>
>     tr2: dump names if config exist in multiple scopes
>
>     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>
>
> diff --git a/Documentation/technical/api-trace2.txt b/Documentation/technical/api-trace2.txt
> index 4e411f3306..50f1e0c259 100644
> --- a/Documentation/technical/api-trace2.txt
> +++ b/Documentation/technical/api-trace2.txt
> @@ -717,6 +717,7 @@ The "exec_id" field is a command-unique id and is only useful if the
>  {
>         "event":"def_param",
>         ...
> +       "scope":...
>         "param":"core.abbrev",
>         "value":"7"
>  }
> @@ -1240,9 +1241,9 @@ d0 | main                     | version      |     |           |           |
>  d0 | main                     | start        |     |  0.000260 |           |              | /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
> +d0 | main                     | def_param    |     |           |           | scope:system | color.ui:never
> +d0 | main                     | def_param    |     |           |           | scope:global | color.ui:always
> +d0 | main                     | def_param    |     |           |           | scope:local  | color.ui:auto
>  d0 | main                     | exit         |     |  0.000470 |           |              | code:0
>  d0 | main                     | atexit       |     |  0.000477 |           |              | code:0
>  ----------------
> diff --git a/trace2/tr2_tgt_event.c b/trace2/tr2_tgt_event.c
> index c5c8cfbbaa..37a3163be1 100644
> --- a/trace2/tr2_tgt_event.c
> +++ b/trace2/tr2_tgt_event.c
> @@ -479,9 +479,12 @@ static void fn_param_fl(const char *file, int line, const char *param,
>  {
>         const char *event_name = "def_param";
>         struct json_writer jw = JSON_WRITER_INIT;
> +       enum config_scope scope = current_config_scope();
> +       const char *scope_name = config_scope_name(scope);
>
>         jw_object_begin(&jw, 0);
>         event_fmt_prepare(event_name, file, line, NULL, &jw);
> +       jw_object_string(&jw, "scope", scope_name);
>         jw_object_string(&jw, "param", param);
>         jw_object_string(&jw, "value", value);
>         jw_end(&jw);
> diff --git a/trace2/tr2_tgt_normal.c b/trace2/tr2_tgt_normal.c
> index c42fbade7f..69f8033077 100644
> --- a/trace2/tr2_tgt_normal.c
> +++ b/trace2/tr2_tgt_normal.c
> @@ -298,8 +298,11 @@ static void fn_param_fl(const char *file, int line, const char *param,
>                         const char *value)
>  {
>         struct strbuf buf_payload = STRBUF_INIT;
> +       enum config_scope scope = current_config_scope();
> +       const char *scope_name = config_scope_name(scope);
>
> -       strbuf_addf(&buf_payload, "def_param %s=%s", param, value);
> +       strbuf_addf(&buf_payload, "def_param scope:%s %s=%s", scope_name, param,
> +                   value);
>         normal_io_write_fl(file, line, &buf_payload);
>         strbuf_release(&buf_payload);
>  }
> diff --git a/trace2/tr2_tgt_perf.c b/trace2/tr2_tgt_perf.c
> index a1eff8bea3..8cb792488c 100644
> --- a/trace2/tr2_tgt_perf.c
> +++ b/trace2/tr2_tgt_perf.c
> @@ -441,12 +441,17 @@ static void fn_param_fl(const char *file, int line, const char *param,
>  {
>         const char *event_name = "def_param";
>         struct strbuf buf_payload = STRBUF_INIT;
> +       struct strbuf scope_payload = STRBUF_INIT;
> +       enum config_scope scope = current_config_scope();
> +       const char *scope_name = config_scope_name(scope);
>
>         strbuf_addf(&buf_payload, "%s:%s", param, value);
> +       strbuf_addf(&scope_payload, "%s:%s", "scope", scope_name);
>
> -       perf_io_write_fl(file, line, event_name, NULL, NULL, NULL, NULL,
> -                        &buf_payload);
> +       perf_io_write_fl(file, line, event_name, NULL, NULL, NULL,
> +                        scope_payload.buf, &buf_payload);
>         strbuf_release(&buf_payload);
> +       strbuf_release(&scope_payload);
>  }
>
>  static void fn_repo_fl(const char *file, int line,
>
> Am I understand accurately?
> Thanks.

Yes, exactly! That makes it much clearer what the functional change was about, i.e. we can see what parts of the trace are now different (the scope is added to the trace).

Previous: tenglong.tlNext: tenglong.tl
Message 118 of 128 in “trace2 output for bitmap decision path”
  1. 0/3 trace2 output for bitmap decision pathTeng Long, Mar 24, 2022
  2. 1/3 pack-bitmap.c: use "ret" in "open_midx_bitmap()"Teng Long, Mar 24, 2022
  3. Taylor BlauMar 24, 2022
  4. Teng LongMar 28, 2022
  5. Taylor BlauMar 30, 2022
  6. 2/3 pack-bitmap.c: add "break" statement in "open_pack_bitmap()"Teng Long, Mar 24, 2022
  7. Junio C HamanoMar 24, 2022
  8. Taylor BlauMar 24, 2022
  9. Taylor BlauMar 24, 2022
  10. Teng LongMar 29, 2022
  11. Taylor BlauMar 30, 2022
  12. Teng LongTeng Long, Mar 30, 2022
  13. Ævar Arnfjörð BjarmasonMar 30, 2022
  14. 3/3 bitmap: add trace outputs during open "bitmap" fileTeng Long, Mar 24, 2022
  15. Junio C HamanoMar 24, 2022
  16. Ævar Arnfjörð BjarmasonMar 24, 2022
  17. Teng LongTeng Long, Mar 29, 2022
  18. Ævar Arnfjörð BjarmasonMar 29, 2022
  19. 0/5 trace2 output for bitmap decision pathTeng Long, Apr 21, 2022
  20. 1/5 pack-bitmap.c: continue looping when first MIDX bitmap is foundTeng Long, Apr 21, 2022
  21. Taylor BlauMay 11, 2022
  22. 2/5 pack-bitmap.c: rename "idx_name" to "bitmap_name"Teng Long, Apr 21, 2022
  23. Taylor BlauMay 11, 2022
  24. 3/5 pack-bitmap.c: make warnings more detailed when opening bitmapTeng Long, Apr 21, 2022
  25. Taylor BlauApr 21, 2022
  26. Teng LongMay 6, 2022
  27. 5/5 pack-bitmap.c: using error() instead of silently returning -1Teng Long, Apr 21, 2022
  28. Ævar Arnfjörð BjarmasonApr 21, 2022
  29. Teng LongMay 6, 2022
  30. 4/5 bitmap: add trace2 outputs during open "bitmap" fileTeng Long, Apr 21, 2022
  31. Ævar Arnfjörð BjarmasonApr 21, 2022
  32. Teng LongMay 6, 2022
  33. Teng LongMay 6, 2022
  34. Jeff HostetlerApr 21, 2022
  35. Teng LongMay 6, 2022
  36. Jeff HostetlerMay 10, 2022
  37. 0/5 trace2 output for bitmap decision pathTeng Long, Jun 12, 2022
  38. 1/5 pack-bitmap.c: continue looping when first MIDX bitmap is foundTeng Long, Jun 12, 2022
  39. 3/5 pack-bitmap.c: make warnings support i18N when opening bitmapTeng Long, Jun 12, 2022
  40. 2/5 pack-bitmap.c: rename "idx_name" to "bitmap_name"Teng Long, Jun 12, 2022
  41. 4/5 pack-bitmap.c: using error() instead of silently returning -1Teng Long, Jun 12, 2022
  42. Taylor BlauJun 14, 2022
  43. Teng LongJun 20, 2022
  44. 5/5 bitmap: add trace2 outputs during open "bitmap" fileTeng Long, Jun 12, 2022
  45. Junio C HamanoJun 13, 2022
  46. Teng LongJun 20, 2022
  47. Taylor BlauJun 14, 2022
  48. Teng LongJun 21, 2022
  49. Jeff HostetlerJun 22, 2022
  50. Teng LongJun 23, 2022
  51. Jeff HostetlerJun 23, 2022
  52. Teng LongJun 24, 2022
  53. 0/5 trace2 output for bitmap decision pathTeng Long, Jun 21, 2022
  54. 1/5 pack-bitmap.c: continue looping when first MIDX bitmap is foundTeng Long, Jun 21, 2022
  55. 3/5 pack-bitmap.c: make warnings support i18N when opening bitmapTeng Long, Jun 21, 2022
  56. 2/5 pack-bitmap.c: rename "idx_name" to "bitmap_name"Teng Long, Jun 21, 2022
  57. 5/5 bitmap: add trace2 outputs during open "bitmap" fileTeng Long, Jun 21, 2022
  58. Jeff HostetlerJun 22, 2022
  59. Junio C HamanoJun 22, 2022
  60. 4/5 pack-bitmap.c: using error() instead of silently returning -1Teng Long, Jun 21, 2022
  61. 0/5 tr2: avoid to print "interesting" config repeatedlyTeng Long, Jun 28, 2022
  62. 1/5 pack-bitmap.c: continue looping when first MIDX bitmap is foundTeng Long, Jun 28, 2022
  63. 2/5 pack-bitmap.c: rename "idx_name" to "bitmap_name"Teng Long, Jun 28, 2022
  64. 3/5 pack-bitmap.c: using error() instead of silently returning -1Teng Long, Jun 28, 2022
  65. Junio C HamanoJun 28, 2022
  66. Teng LongJul 5, 2022
  67. Junio C HamanoJul 5, 2022
  68. 4/5 pack-bitmap.c: retrieve missing i18n translationsTeng Long, Jun 28, 2022
  69. Ævar Arnfjörð BjarmasonJun 28, 2022
  70. Eric SunshineJun 28, 2022
  71. Teng LongJul 6, 2022
  72. Teng LongJul 6, 2022
  73. Junio C HamanoJun 28, 2022
  74. Teng LongJul 7, 2022
  75. Junio C HamanoJul 7, 2022
  76. Teng LongJul 11, 2022
  77. 5/5 tr2: avoid to print "interesting" config repeatedlyTeng Long, Jun 28, 2022
  78. Ævar Arnfjörð BjarmasonJun 28, 2022
  79. Junio C HamanoJun 28, 2022
  80. Jeff HostetlerJul 1, 2022
  81. Teng LongJul 11, 2022
  82. Teng LongJul 11, 2022
  83. 0/7 trace2: dump scope when print "interesting" configTeng Long, Jul 11, 2022
  84. 1/7 clean: fixed issues related to text output formatTeng Long, Jul 11, 2022
  85. Junio C HamanoJul 11, 2022
  86. Teng LongJul 13, 2022
  87. 2/7 pack-bitmap.c: mark more strings for translationsTeng Long, Jul 11, 2022
  88. 3/7 pack-bitmap.c: rename "idx_name" to "bitmap_name"Teng Long, Jul 11, 2022
  89. 4/7 pack-bitmap.c: don't ignore ENOENT silentlyTeng Long, Jul 11, 2022
  90. Ævar Arnfjörð BjarmasonJul 11, 2022
  91. Teng LongJul 13, 2022
  92. Junio C HamanoJul 11, 2022
  93. Teng LongJul 14, 2022
  94. 5/7 pack-bitmap.c: using error() instead of silently returning -1Teng Long, Jul 11, 2022
  95. Ævar Arnfjörð BjarmasonJul 11, 2022
  96. Teng LongJul 15, 2022
  97. 6/7 pack-bitmap.c: continue looping when first MIDX bitmap is foundTeng Long, Jul 11, 2022
  98. 7/7 tr2: dump names if config exist in multiple scopesTeng Long, Jul 11, 2022
  99. Ævar Arnfjörð BjarmasonJul 11, 2022
  100. Jeff HostetlerJul 11, 2022
  101. Ævar Arnfjörð BjarmasonJul 11, 2022
  102. Teng LongJul 18, 2022
  103. 0/7 trace2: dump scope when print "interesting" configTeng Long, Jul 18, 2022
  104. 1/7 pack-bitmap.c: fix formatting of error messagesTeng Long, Jul 18, 2022
  105. 2/7 pack-bitmap.c: mark more strings for translationsTeng Long, Jul 18, 2022
  106. 3/7 pack-bitmap.c: rename "idx_name" to "bitmap_name"Teng Long, Jul 18, 2022
  107. 4/7 pack-bitmap.c: do not ignore error when opening a bitmap fileTeng Long, Jul 18, 2022
  108. 5/7 pack-bitmap.c: using error() instead of silently returning -1Teng Long, Jul 18, 2022
  109. 6/7 pack-bitmap.c: continue looping when first MIDX bitmap is foundTeng Long, Jul 18, 2022
  110. 7/7 tr2: dump names if config exist in multiple scopesTeng Long, Jul 18, 2022
  111. Jeff HostetlerJul 18, 2022
  112. tenglong.tlJul 19, 2022
  113. Junio C HamanoJul 19, 2022
  114. tenglong.tlJul 20, 2022
  115. Junio C HamanoJul 18, 2022
  116. Ævar Arnfjörð BjarmasonJul 18, 2022
  117. tenglong.tlJul 19, 2022
  118. Ævar Arnfjörð BjarmasonJul 19, 2022
  119. tenglong.tlJul 19, 2022
  120. 0/6 pack-bitmap.c: optimize error messagestenglong.tl, Jul 21, 2022
  121. 1/6 pack-bitmap.c: fix formatting of error messagestenglong.tl, Jul 21, 2022
  122. 2/6 pack-bitmap.c: mark more strings for translationstenglong.tl, Jul 21, 2022
  123. 3/6 pack-bitmap.c: rename "idx_name" to "bitmap_name"tenglong.tl, Jul 21, 2022
  124. 4/6 pack-bitmap.c: do not ignore error when opening a bitmap filetenglong.tl, Jul 21, 2022
  125. 5/6 pack-bitmap.c: using error() instead of silently returning -1tenglong.tl, Jul 21, 2022
  126. 6/6 pack-bitmap.c: continue looping when first MIDX bitmap is foundtenglong.tl, Jul 21, 2022
  127. Junio C HamanoJul 21, 2022
  128. tenglong.tlJul 22, 2022

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.