Feedback-ID: i197146af:Fastmail
Date: Fri, 14 Aug 2026 13:06:58 +0200
From: Patrick Steinhardt <ps@pks.im>
To: "D. Ben Knoble" <ben.knoble@gmail.com>
Cc: Junio C Hamano <gitster@pobox.com>, git@vger.kernel.org,
	Yuchen Tian <cat@malon.dev>, Todd Zullinger <tmz@pobox.com>,
	Olamide Caleb Bello <belkid98@gmail.com>
Subject: Re: [PATCH 3/3] core: convert build-time USE_NSEC into runtime
 core.useNanosec
Message-ID: <an720tZnot07HYiK@pks.im>
References: <annHlFwu4NKwmcLr@pks.im>
 <59E4039A-C9BA-4EFD-8022-77C73EB51ED0@gmail.com>
 <CALnO6CA5LdL74SqC9V_wJWi=Pf7+cHBDkuUFAJ7jCOVWZjBOzA@mail.gmail.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <CALnO6CA5LdL74SqC9V_wJWi=Pf7+cHBDkuUFAJ7jCOVWZjBOzA@mail.gmail.com>

On Thu, Aug 13, 2026 at 05:40:31PM -0400, D. Ben Knoble wrote:
> On Tue, Aug 11, 2026 at 12:26 PM Ben Knoble <ben.knoble@gmail.com> wrote:
> > > Le 10 août 2026 à 08:44, Patrick Steinhardt <ps@pks.im> a écrit :
> > > ﻿On Mon, Aug 10, 2026 at 08:27:51AM -0400, D. Ben Knoble wrote:
> > > [snip]
> > >> Back down to being on-par with original code. So that's good. The next
> > >> version will include some variant that reads a struct member instead
> > >> of going through repo_config_get_bool().
> > >>
> > >> But which? Reading the private_ member is obviously wrong; I suppose
> > >> I'm supposed to use repo_config_values() there. Or, rework the series
> > >> to put this member in repo_settings. I think I originally assumed that
> > >> struct is for things that are settings that aren't configured by
> > >> git-config, but… now I'm not sure. Looking at prepare_repo_settings()
> > >> shows lots of repo_cfg_*() calls. So I think I see how to adapt to
> > >> using repo_settings,
> > >>
> > >> Patrick, Junio, and Tian had a brief discussion in
> > >> <anlmwaEtwcCPse1N@pks.im> about the split creating confusion. I don't
> > >> really want to wait for it to settle to land this change, but we might
> > >> want to work together on identifying the best path forward for
> > >> core.useNanosec :)
> > >>
> > >> I don't suppose it really matters to me which struct I put the member
> > >> in. As I said, v2 will definitely fix the hot path lookup here. Just a
> > >> matter of input on which struct we want to use this time, I guess.
> > >
> > > I think `repo_config_values()` is the modern variant that we're slowly
> > > migrating stuff into. But that struct only works with `the_repository`,
> > > so the question is whether we ever use "core.useNsec" for a different
> > > repository. My hunch would be yes, for example when recusing into
> > > submodules, but I'm not sure.
> > >
> > > Patrick
> >
> > Thanks. I’m working on control-flow analysis to see what kinds of repo values end up there. Of course I’ll also run the test suite and so on with the repo_config_values change. But the analysis will take some time.
> 
> Ok, CI run: https://github.com/benknoble/git/actions/runs/31701945211.
> This demonstrates that nothing our test suite does across the many CI
> configurations ends up where with a non-the_repository-repository
> (ahem).
> 
> I have been working on control-flow analysis by hand in my Git time
> this week. It's of the form "Z calls Y calls X …" until we can see
> what the repository that's (eventually) fed to repo_config_values()
> here in is_racy_stat() is. My notes are one node per line, which
> indentation showing callee relationships. Some lines are pointers to
> other nodes to avoid duplicating work.
> 
> With that in mind, filtering out the pointer nodes, I've analyzed 214
> nodes in the graph. If I'm lucky, I'm approaching the halfway mark,
> but I somewhat doubt it.
> 
> But since CI shows things work… I'd rather not continue the analysis
> if we're satisfied for now. (Esp. since that will give me more Git
> time back for reviewing ;) It being outside-of-work time, I only have
> so much of it.)
> 
> A few other related things:
> - Some of the edges of the graph appear to be public libgit.a
> interfaces. That means we can't guarantee that only the_repository is
> used.
> - On a related note, I don't know how large the current "must only use
> the_repository" (e.g., via repo_config_values()) surface area is right
> now. Based on the partial analysis I mentioned above, this feels like
> it's introducing (or at least contributing to) a rather large surface
> area. So, this change might make it more critical to resolve the
> limitation mentioned in the other thread. OTOH, I don't think this
> change is likely to represent the only pervasive the_repository-only
> limitation, and I'm afraid it will never land if it must be
> the_repository clean (unless repo_settings is the_repository clean and
> we decide that's an acceptable place for this member).
> 
> So, idk. If we're happy with the CI run + use of repo_config_values()
> overall, I can send a v2 shortly (in next 24h), I think.
> 
> Thoughts? Strong opinions?

No strong opinions from my side, other than that we should stop
converting everything to `repo_config_values()` until we have a plan for
how to make it work with repositories other than `the_repository`.

I don't feel like holding this series in hostage though, so if your
analysis and the test suite both say that this is probably fine then we
may want to pursue it. Or we just use a global variable for it for the
time being and then wait until the `repo_config_values()` dust has
settled.

Patrick