Re: [PATCH v3 5/5] bitmap: add trace2 outputs during open "bitmap" file

From: Jeff Hostetler <git@jeffhostetler.com>
Date: Jun 23, 2022, 15:14 UTC
Message-ID: <a4dda985-a319-2c24-2957-5116831142b8@jeffhostetler.com>
In-Reply-To: <20220623093836.113288-1-dyroneteng@gmail.com>

On 6/23/22 5:38 AM, Teng Long wrote:

> On Wed, 22 Jun 2022 08:51:18 -0400, Jeff Hostetler wrote:
>

[...]

Show 30 quoted lines

> I remove the changes of "repo_config_bool" from my v4 patch
> then I try to know about "GIT_TRACE2_CONFIG_PARAMS" and
> "trace2.configparams". Thereby, when I:
> 
> Execute "GIT_TRACE2_PERF=1 GIT_TRACE2_CONFIG_PARAMS=core.multipackIndex git rev-list  --test-bitmap HEAD"
> 
> 15:21:38.812782 git.c:461                    | d0 | main                     | def_param    |     |           |           |              | core.multipackindex:false
> 15:21:38.812797 git.c:461                    | d0 | main                     | def_param    |     |           |           |              | core.multipackindex:false
> 
> I checked my configs, I found if there exists multiple level configs.
> it'll print multiple times. Like If I config all the global, system
> and local on "core.multipackIndex=false" , the output will be:
> 
> 15:41:50.614108 git.c:462                    | d0 | main                     | def_param    |     |           |           |              | core.multipackindex:false
> 15:41:50.614123 git.c:462                    | d0 | main                     | def_param    |     |           |           |              | core.multipackindex:false
> 15:41:50.614136 git.c:462                    | d0 | main                     | def_param    |     |           |           |              | core.multipackindex:false
> 
> And if I modified the local scope of core.multipackIndex to "true",
> the output will be:
> 
> 15:45:39.200172 git.c:462                    | d0 | main                     | def_param    |     |           |           |              | core.multipackindex:false
> 15:45:39.200186 git.c:462                    | d0 | main                     | def_param    |     |           |           |              | core.multipackindex:false
> 15:45:39.200200 git.c:462                    | d0 | main                     | def_param    |     |           |           |              | core.multipackindex:true
> 
> I'm not sure it's an intentional design or here should be only
> print the final value that takes effect or should print all the
> values if config multiple scopes on the same config.
> 
> Hence, I made a temporary patch below to try to add some
> identifying information to know why we output these lines, like:

I had intended it to only print the final "effective" value that the command would actually use. I don't remember if I ever experimented with config values that are set at multiple levels, but testing it now shows that you're right it does print a "def_param" message for each level.

I would suggest we fix it to only print the final value since we really just want to know whether the command will or will not use a feature. The inheritance is somewhat irrelevant.

But the change to do that may be quite a mess, so printing the scope name as you have here is fine. And may help with support when debugging users having confusing problems, so I'm fine with it.

Thanks, Jeff