Re: [PATCH 18/20] config: don't depend on `the_repository` with branch conditions

[Patrick Steinhardt <ps@pks.im>]

On Fri, Aug 09, 2024 at 03:47:50PM -0500, Justin Tobler wrote:
> On 24/08/07 08:58AM, Patrick Steinhardt wrote:
> > When computing branch "includeIf" conditions we use `the_repository` to
> > obtain the main ref store. We really shouldn't depend on this global
> > repository though, but should instead use the repository that is being
> > passed to us via `struct config_include_data`. Otherwise, when parsing
> > configuration of e.g. submodules, we may end up evaluating the condition
> > the via the wrong refdb.
> > 
> > Fix this.
> > 
> > Signed-off-by: Patrick Steinhardt <ps@pks.im>
> > ---
> >  config.c | 9 +++++----
> >  1 file changed, 5 insertions(+), 4 deletions(-)
> > 
> > diff --git a/config.c b/config.c
> > index 831c9eacb0..08437f75e5 100644
> > --- a/config.c
> > +++ b/config.c
> > @@ -300,13 +300,14 @@ static int include_by_gitdir(const struct key_value_info *kvi,
> >  	return ret;
> >  }
> >  
> > -static int include_by_branch(const char *cond, size_t cond_len)
> > +static int include_by_branch(struct config_include_data *data,
> > +			     const char *cond, size_t cond_len)
> >  {
> >  	int flags;
> >  	int ret;
> >  	struct strbuf pattern = STRBUF_INIT;
> > -	const char *refname = !the_repository->gitdir ?
> > -		NULL : refs_resolve_ref_unsafe(get_main_ref_store(the_repository),
> > +	const char *refname = (!data->repo || !data->repo->gitdir) ?
> > +		NULL : refs_resolve_ref_unsafe(get_main_ref_store(data->repo),
> >  					       "HEAD", 0, NULL, &flags);
> >  	const char *shortname;
> 
> This works the same so long as `config_include_data` always has its
> repository set. I wonder if for `!data->repo` we should instead signal a
> BUG? Otherwise we would silently return NULL in such cases. Maybe that
> would be the desired behavior though?

It is expected that the repository may not be set, namely when reading
configuration via `read_early_config()` and `read_very_early_config()`.
We wouldn't want to hit the refdb there, so that is fine.

We also have `read_protected_config()`, which requires a bit more
thought. It does respect includes, so you may think we want to read the
refdb there. But protected config is defined as having system, global or
command scope, which to me means that we shouldn't end up reading config
data from the current repository. So not hitting the refdb there also
seems like the right thing to do.

Patrick