Re: [PATCH 1/2] get_main_ref_store: BUG() when outside a repository

[Stefan Beller <sbeller@google.com>]

On Fri, May 18, 2018 at 3:25 PM, Jeff King <peff@peff.net> wrote:
> If we don't have a repository, then we can't initialize the
> ref store.  Prior to 64a741619d (refs: store the main ref
> store inside the repository struct, 2018-04-11), we'd try to
> access get_git_dir(), and outside a repository that would
> trigger a BUG(). After that commit, though, we directly use
> the_repository->git_dir; if it's NULL we'll just segfault.
>
> Let's catch this case and restore the BUG() behavior.
> Obviously we don't ever want to hit this code, but a BUG()
> is a lot more helpful than a segfault if we do.

That is true and an immediate bandaid; an alternative would
be to do:

  if (!r->gitdir)
    /* not in a git directory ? */
    return NULL;
    /* We signal the caller the absence of a git repo by NULL ness
      of the ref store */

However that we would need to catch at all callers of
get_main_ref_store and error out accordingly.

Then the trade off would be, how many callers to the main ref
store do we have compared to options that rely on a ref store
present? (I assume a patch like the second patch would show
up in more cases now for all BUGs that we discover via this patch.
The tradeoff is just if we want to report all these early by checking
the config and system state, or wait until we get NULL ref store
and then bail)

I think checking early makes sense; I am not so sure about this
patch; for the time being it makes sense, though in the long run,
we rather want to catch this at a higher level:

r->gitdir is supposedly never NULL, so we shall not
produce this state. Maybe we want to set the_repository to NULL
if set_git_dir fails (via repo_set_gitdir in setup_git_env, which both
return void today).

Enough of my rambling, the patches look good!
Stefan