Re: [PATCH 03/10] lazily load core.sharedrepository

From: Jeff King <peff@peff.net>
Date: Mar 3, 2016, 18:23 UTC
Message-ID: <20160303182343.GA24171@sigill.intra.peff.net>
In-Reply-To: <CACsJy8DM4sG5MztfQnZHpaj3NAveS9S4-qLtSCmLEWhFyfKwTw@mail.gmail.com>

On Thu, Mar 03, 2016 at 08:00:03PM +0700, Duy Nguyen wrote:

Show 16 quoted lines

> On Tue, Mar 1, 2016 at 9:39 PM, Jeff King <peff@peff.net> wrote:
> > +       if (need_shared_repository_from_config) {
> > +               const char *var = "core.sharedrepository";
> > +               const char *value;
> > +               if (!git_config_get_value(var, &value))
> > +                       the_shared_repository = git_config_perm(var, value);
> > +               need_shared_repository_from_config = 0;
> > +       }
> 
> If config cache is invalidated and reloaded by some crazy code (alias
> code, most likely, but I didn't check), we could be in trouble. Is
> there any clean way we could hook in git_config_clear() and reset
> need_shared..  back  to 1 (or something of similar effect)? Or perhaps
> maintain a "clear counter", increased at every clear, and we keep a
> copy here, so we know if any more clear() has been called between
> get_shared..() calls.

Yeah, this caches forever. But I'm not sure that's really different from the existing code, which sets it in check_repository_format_gently(), which is only called during setup_git_directory(), which cannot be called twice. So it was effectively static for the full program anyway (modulo people tweaking the integer variable manually, which is supported here).

I think something like the "clear counter" you describe is also not quite sufficient. We have two sources for the variable: the config, and manually setting it. Once it has been manually set, I think we don't want to auto-read it from the config anymore (and stomp over what somebody set). I'm not sure even git_config_clear() is enough of a single to say "...and also reset everything that might have been set from these config variables in the past". I think we'd want to do per-variable invalidation, depending on what the caller wants.

So I'd rather punt on it for now, unless this is breaking a specific case. I think it should behave the same as the status quo.

-Peff