Subject: Re: [PATCH 04/31] setup: don't perform lazy initialization of repository state

From: Brandon Williams <[email-masked]>
Date: Mon, 5 Jun 2017 10:43:52 -0700

On 06/02, Jeff King wrote:
> On Thu, Jun 01, 2017 at 12:23:25PM -0700, Stefan Beller wrote:
> 
> > On Wed, May 31, 2017 at 2:43 PM, Brandon Williams <[email-masked]> wrote:
> > > Under some circumstances (bogus GIT_DIR value or the discovered gitdir
> > > is '.git') 'setup_git_directory()' won't initialize key repository
> > > state.  This leads to inconsistent state after running the setup code.
> > > To account for this inconsistent state, lazy initialization is done once
> > > a caller asks for the repository's gitdir or some other piece of
> > > repository state.  This is confusing and can be error prone.
> > >
> > > Instead let's tighten the expected outcome of 'setup_git_directory()'
> > > and ensure that it initializes repository state in all cases that would
> > > have been handled by lazy initialization.
> > 
> > Lazy init is usually there for a reason. (As in: it took too long to perform
> > it at all times, so it has been optimized to only perform the init when needed).
> 
> In the case of setup_git_env(), I think it was less about doing work and
> more that we didn't want to have to do explicit setup in each program.
> But over the years we've moved away from that, and in fact if you hit
> the lazy initialization these days you'll generally BUG() anyway.
> 
> _But_ I suspect there are still some cases you'd need to handle. For
> instance, it's still OK to skip calling setup_git_directory() if you've
> got $GIT_DIR in the environment (which is why we have have_git_dir()
> instead of checking startup_info->have_repository).

Yes there are a couple places that rely on the lazy initialization but
that's not due to setup not being run.  Rather it has to do with GIT_DIR
being set to a bogus directory so when setup is run gently it does
nothing.  Then at a later point in time the command tries to access
files in the gitdir (which triggers lazy init of the git environment).

So I think that explicitly doing the 'lazy init' portion (which ensures
that the env gets setup even if GIT_DIR is bogus) at the end of setup
should be sufficient, least it seems to be so though perhaps we can't
rely on our tests to tell us that.

> 
> I think it would be nice to do away with that, too, but we're not quite
> there yet (and if I am reading this patch correctly, we'd probably hit
> these BUGs in such cases).
> 
> -Peff

-- 
Brandon Williams