From: D. Ben Knoble <ben.knoble@gmail.com>
Date: Tue, 01 Sep 2026 12:36:22 GMT
Subject: Re: [PATCH v6 3/3] core: convert build-time USE_NSEC into runtime core.useNanosec
Message-ID: <CALnO6CAZYvnv3fMWkU0pqY+XN3ncBqVav49ZEvzV0LMtmkYO0Q@mail.gmail.com>
In-Reply-To: <20260901045403.GA1075462@coredump.intra.peff.net>

On Tue, Sep 1, 2026 at 12:54 AM Jeff King <peff@peff.net> wrote:
>
> On Mon, Aug 31, 2026 at 04:01:37PM -0400, D. Ben Knoble wrote:
>
> > diff --git a/environment.c b/environment.c
> > index 6676e6f5ae..c83cf44839 100644
> > --- a/environment.c
> > +++ b/environment.c
> > @@ -571,6 +571,13 @@ int git_default_core_config(const char *var, const char *value,
> >               return 0;
> >       }
> >
> > +#ifndef NO_NSEC
> > +     if (!strcmp(var, "core.usenanosec")) {
> > +             cfg->use_nanosec = git_config_bool(var, value);
> > +             return 0;
> > +     }
> > +#endif
>
> This hunk made me wonder if we even need to do any build-time magic here
> at all. If your platform doesn't support nanosecond stat entries, then
> you're probably not going to ask for core.usenanosec in the first place.
> But if you do, I think the code still works; we fake the entries as "0",
> so they'd always yield a racy tie, just as if core.usenanosec was
> disabled.

At first I thought you meant we fake the cfg->use_nanosec as 0; it
took me a moment to realize you mean that we fake the index entries as
0ns. (That is what you mean, right?)

In that case, yes, I suppose it would work. Might be confusing in a
debugger to see use_nanosec set and checked, though?

> I guess you might be able to get into a funny state, though, if you
> build two versions of Git, one with NO_NSEC and one without, on a system
> that actually does support nanosecond timestamps. Because IIRC even if
> we aren't _using_ the values, we still store them in the index. So an
> index generated with the regular build would store the actual nanosec
> stamps, which would then get a false comparison using the NO_NSEC
> version.
>
> That seems quite unlikely to happen in practice, and there is a certain
> amount of "if it hurts, don't do that".

Hm, yeah. I haven't thought too hard either about the interactions
where you toggle core.usenanosec on and off, but giving it an initial
think they seem fine. Unlike this hypothetical case, when it's off we
don't look at the ns fields, so I don't think we end up with any false
negatives.

And in this hypothetical, by restricting the option parsing we avoid
reading the ns values on unsupported platforms, I think?

The build-time conditional _does_ mean that if your distro (e.g.)
provides a NO_NSEC build, you can't access the core.usenanosec feature
without compiling yourself, even if your platform supports it. But I
haven't thought too hard either about what it looks like to get rid of
NO_NSEC entirely, and I'm not totally sure if that's a good idea.

> But it's not like by dropping
> this #ifndef we could get rid of NO_NSEC. So it would not simplify the
> code overall, nor the number of build knobs that we expose to the user.
> So it probably is reasonable to keep it.
>
> I haven't been following the topic closely, but from my cursory read
> everything else looked as I'd expect it to.
>
> -Peff

Sounds good, thanks!

-- 
D. Ben Knoble