Re: [PATCH v6 00/15] Trace2 tracing facility

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

On Thu, Mar 21 2019, Johannes Schindelin wrote:

> Hi Ævar,
>
> On Sun, 17 Mar 2019, Ævar Arnfjörð Bjarmason wrote:
>
>>
>> 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.
>
> 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.

>> 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.
>
> Yes, I agree, those are good goals to address.
>
> Ciao,
> Dscho
>
>> > 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
>>