Re: [PATCH v6 00/15] Trace2 tracing facility
- From
- Jeff Hostetler <git@jeffhostetler.com>
- Date
- Mar 22, 2019, 14:01 UTC
- Message-ID
- <37cffd65-58bd-3fae-a4f1-44038ed52f60@jeffhostetler.com>
- In-Reply-To
- <nycvar.QRO.7.76.6.1903221416230.41@tvgsbejvaqbjf.bet>
On 3/22/2019 9:17 AM, Johannes Schindelin wrote:
Show 40 quoted lines
> Hi Ævar,
>
> On Thu, 21 Mar 2019, Ævar Arnfjörð Bjarmason wrote:
>
>> On Thu, Mar 21 2019, Johannes Schindelin wrote:
>>
>>> On Sun, 17 Mar 2019, Ævar Arnfjörð Bjarmason wrote:
>>>
>>>>
>>>> On Fri, Feb 15 2019, Jeff Hostetler wrote:
>>>>
>>>>> I could see having a "~/.git_tr2_config" or something similar in
>>>>> some place like "/etc" that only contained the Trace2 settings. It
>>>>> would be safe to read very early inside main() and we would not
>>>>> consider it to be part of the real config. For example, "git
>>>>> config" would not know about it. Then you could enforce a
>>>>> system-wide setting without any of the env var issues.
>>>>
>>>> I haven't written a patch for this, but it seems to me that we could
>>>> just start reading /etc/gitconfig via some custom config callback
>>>> code early as e.g. 58b284a2e91 ("worktree: add per-worktree config
>>>> files", 2018-10-21) does for the worktree config.
>>>
>>> Oy. Oy, oy, oy.
>>>
>>> Maybe use `read_early_config()` instead? That would be *a lot*
>>> cleaner. You could maybe use a9bcf6586d1a (alias: use the early config
>>> machinery to expand aliases, 2017-06-14) as an inspiration.
>>
>> Thanks. I was thinking *only* to do /etc/gitconfig and not the whole
>> .git/config -> ~/.gitconfig etc. sequence just in terms of saving
>> critical time (this is the performance trace path, after all...).
>>
>> But on a second reading I see that read_early_config() can do that if
>> you set config_source->file, opts->respect_includes etc. I.e. it just
>> (depending on options) resolves to git_config_from_file() which
>> 58b284a2e91 used directly.
>
> Sure, it can exclude the repo and user config, but would that not be
> rather confusing?This was hidden in my earlier message.
There's a lot a config machinery here with lots of chicken-n-egg problems. I want to know at the top of main() as quickly as possible whether trace2 should be enabled.
I don't want to slow down git by spinning up a bunch of trace2 state and wait until the git-dir is discovered, the "-c" args are processed, and we dispatch into the builtin layer and the config is enumerated to know if it should really be on or not.
I also didn't want to introduce another full iteration of the full config system for startup performance reasons.
I played with read_early_config() at one point and it always seemed to introduce side-effects. Perhaps I was calling it earlier than it was expecting and that triggered some of the git-dir discovery or something. I don't remember all the details, I just remember that it changed some behaviors in subtle ways.
Perhaps I could call something like git_config_from_file() with the right set of magic bits to get it to parse exactly 1 system config file that would contain my trace2 settings. Hopefully, this will not have any side-effects.
But if we lump them in with the main /etc/gitconfig settings, we would have to explain that these trace2 config settings are system-only and ARE NOT overridden by "-c", global, local, ... config settings. This would get confusing if the user tried to set local values and did: git config --list --show-origin and it showed system and local values but yet "stubbornly" refused to use the local values over the system values. (I think this was Johannes' point.)
That's why I was suggesting a system trace2 config file that is a peer of /etc/gitconfig (maybe /etc/gittrace2) that would have these values and not be expected to interact with the main config system. That is, we just use the git_config_ routines to parse the file format, rather than inventing another file format, but not change the expectation of the established config value inheritance system.
If there's no objections, I'll take a look at doing this.
Thanks Jeff