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 <bmwill@xxxxxxxxxx> 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