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.
> 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
>
