Re: [PATCH v2 08/21] config: make dependency on repo in `read_early_config()` explicit

From: Patrick Steinhardt <ps@pks.im>
Date: Sep 4, 2024, 07:14 UTC
Message-ID: <ZtgIyjGnpwX3VvYP@pks.im>
In-Reply-To: <D3X4D0DUL3TS.1FAOC0KJ08D9L@jamesliu.io>

On Wed, Sep 04, 2024 at 11:46:47AM +1000, James Liu wrote:

Show 42 quoted lines

> On Fri Aug 30, 2024 at 7:09 PM AEST, Patrick Steinhardt wrote:
> > diff --git a/config.c b/config.c
> > index a8357ea9544..043e1c8a078 100644
> > --- a/config.c
> > +++ b/config.c
> > @@ -6,8 +6,6 @@
> >   *
> >   */
> >  
> > -#define USE_THE_REPOSITORY_VARIABLE
> > -
> >  #include "git-compat-util.h"
> >  #include "abspath.h"
> >  #include "advice.h"
> > @@ -2204,7 +2202,7 @@ static void configset_iter(struct config_set *set, config_fn_t fn, void *data)
> >  	}
> >  }
> >  
> > -void read_early_config(config_fn_t cb, void *data)
> > +void read_early_config(struct repository *repo, config_fn_t cb, void *data)
> >  {
> >  	struct config_options opts = {0};
> >  	struct strbuf commondir = STRBUF_INIT;
> > @@ -2212,9 +2210,9 @@ void read_early_config(config_fn_t cb, void *data)
> >  
> >  	opts.respect_includes = 1;
> >  
> > -	if (have_git_dir()) {
> > -		opts.commondir = repo_get_common_dir(the_repository);
> > -		opts.git_dir = repo_get_git_dir(the_repository);
> > +	if (repo && repo->gitdir) {
> > +		opts.commondir = repo_get_common_dir(repo);
> > +		opts.git_dir = repo_get_git_dir(repo);
> >  	/*
> >  	 * When setup_git_directory() was not yet asked to discover the
> >  	 * GIT_DIR, we ask discover_git_directory() to figure out whether there
> 
> It doesn't really matter either way since we perform the same checks, but should we do
> 
>         if (repo && repo_get_git_dir(repo))
> 
> instead of accessing the field directly?

We in fact can't. `read_early_config()` is typically invoked with the global `the_repository`. That variable is always set, but its `git_dir` may not be populated depending on when exactly we call this function and whether or not we are inside a repository. With `repo_get_git_dir()` we'd now die in scenarios where it's unset, which we do not want.

Patrick