On Fri, Feb 15 2019, Jeff Hostetler wrote:
> On 2/14/2019 7:33 AM, Ævar Arnfjörð Bjarmason wrote:
>>
>> On Wed, Feb 06 2019, Jeff Hostetler via GitGitGadget wrote:
>>
>>> V6 addresses: [] The remaining hdr-check warning in trace2/tr2_tls.h
>>>
>>> There are no other outstanding comments that I'm aware of.
>>
>> Not a comment on this, just a follow-up question. I started looking into
>> whether this could be driven by config instead of getenv(). A lot easier
>> to set up in some cases than injecting env variables, especialy if the
>> log target supported a strftime() string, is any of that something
>> you've looked into already (so I don't do dupe work...).
>>
>> There's the chicken & egg problem with wanting to do traces way before
>> we get to reading config, so I expect that such a facility would need to
>> work by always trace record at the beginning until we get far enough to
>> write the config, and then either stop and throw away the buffer, or
>> write out the existing trace to the configured target, and continue.
>>
>
> Yes, I beat my head against the config settings for quite a while
> before settling on using an env var.
>
> I wanted to get the:
> () full process elapsed time,
> () the full original argv,
> () all of the command dispatch, run-dashed, and alias expansion,
> () and for my atexit() to run last.
> So I hooked into main() before the config is loaded.
>
> In most commands, the config is processed about the same time as
> parse_options() is called. And we have to insert code in
> git_default_config() to load my settings. This happens after all
> of the .git dir discovery, "-c" and "-C" processing, alias expansion,
> command dispatch and etc. And the argv received in the cmd_*()
> function has been modified. So we lose some information. (I tried
> this for a while and didn't like the results.)
>
> I also tried using read_early_config() various places, but there
> were problems here too. Too early and the "-c" settings weren't
> parsed yet. And there was an issue about when .git dir was discovered,
> so local config settings weren't ready yet.
>
> I also recall having a problem when doing an early iteration with
> side effects (or rather the early iteration caused something to be
> set that caused the real iteration (in cmd_*()) to short-cut), but
> I don't remember the details.
>
> So we have a custom installer that also sets the environment variable
> after git is installed and haven't had any problems.
>
>
> I hesitate to say we should always start allocating a bunch of data
> and spinning up the TLS data and etc. before we know if tracing is
> wanted. Just seems expensive for most users.
>
>
> 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.
It would ignore everything except trace.* or wherever namespace we'll
put this in, and "-c" etc. wouldn't work. We could just document that as
a limitation for now which could be fixed later.
It wouldn't make things worse, and would mean you could easily set
logging system-wide without needing to inject environment variables in
lots of custom (and sometimes hard-to-do) places, which I expect is the
most common use-case, and is the one I have.
> WRT the strftime() question, we could either add syntax to the
> env var value (or the tr2 config setting) to have some tokens
> for that. I just stuck with absolute pathnames since I started
> by copying what was done for GIT_TRACE.
>
> Jeff
