From: Bello Olamide <belkid98@gmail.com>
Date: Mon, 09 Feb 2026 08:47:05 GMT
Subject: Re: [Outreachy PATCH v6 1/3] environment: stop storing `core.attributesFile` globally
Message-ID: <CAD=f0L9h-_pqak2tjjpASmt91mx2RPha2ZW7bsvmBNdhEv=VAw@mail.gmail.com>
In-Reply-To: <c95a7730-7b14-4be0-a4e4-861b2f5430ea@gmail.com>

On Wed, 4 Feb 2026 at 17:39, Phillip Wood <phillip.wood123@gmail.com> wrote:
>
> On 03/02/2026 15:42, Olamide Caleb Bello wrote:
> > The `core.attributeFile` config value is parsed in
> > git_default_core_config(), loaded eagerly and stored in the global
> > variable `git_attributes_file`. Storing this value in a global variable
> > can lead to it being overwritten by another repository when more than one
> > Git repository run in the same Git process.
> >
> > Create a new struct `repo_config_values` to hold this value and
> > other repository dependent values parsed by `git_default_config()`.
> > This will ensure the current behaviour remains the same while also
> > enabling the libification of Git.
> >
> > An accessor function 'repo_config_values()' is created and used to access
> > the new struct member of the repository struct.
> > This is to ensure that we detect if the struct repository has been
> > initialized and also prevent double initialization of the repository.
>
> Sounds sensible. This paragraph could be reflowed.

Okay

>
> > It is important to note that `git_default_config()` is a wrapper to other
> > `git_default_*_config()` functions such as `git_default_core_config()`.
> > Therefore to access and modify this global variable,
> > the change has to be made `git_default_core_config()`.
>
> I'm not sure what this paragraph is saying with regard to the changes in
> this patch.

Okay I will fix it.

>
> > --- a/environment.c
> > +++ b/environment.c
> > @@ -756,3 +757,8 @@ int git_default_config(const char *var, const char *value,
> >       /* Add other config variables here and to Documentation/config.adoc. */
> >       return 0;
> >   }
> > +
> > +void repo_config_values_init(struct repo_config_values *cfg)
> > +{
> > +     cfg->attributes_file = NULL;
> > +}
>
> Should we be free()ing cfg->attributes_file when the repository instance
> is free()d? At the moment we're using "the_repository" which points to a
> static instance so it does not make any practical difference but once we
> start storing the config per-repository instance we will need to free
> the config when the repository instance is free()d.

Okay I will keep this in mind.

>
> > diff --git a/repository.c b/repository.c
> > index c7e75215ac..a9b727540f 100644
> > --- a/repository.c
> > +++ b/repository.c
> > @@ -50,13 +50,25 @@ static void set_default_hash_algo(struct repository *repo)
> >       repo_set_hash_algo(repo, algo);
> >   }
> >
> > +struct repo_config_values *repo_config_values(struct repository *repo)
> > +{
> > +     if(!repo->initialized)
> > +             BUG("config values from uninitialized repository");
>
> This check and the one in initialize_repository() below assume that the
> repository instance is zeroed out when it is created, that's a
> reasonable requirement but we should probably document it as our other
> data structures tend not to require that they're zeroed out before they
> are initialized. For example
>
>         struct strbuf buf;
>         strbuf_init(&buf, 0);
>
> is perfectly fine as strbuf_init() does not assume the instance passed
> to it has been zeroed out.
>
> As we only support retrieving values from "the_repository" at the moment
> we should perhaps add
>
>         if (repo != the_repository)
>                 BUG("trying to read config from wrong repository instance");

Okay noted

>
> Everything else looks fine to me
>

Thanks