Re: [Outreachy PATCH v3 1/3] environment: stop storing `core.attributesFile` globally

>
> Olamide Caleb Bello <belkid98@gmail.com> writes:
>
> > The config value is parsed in git_default_core_config(), loaded
>
> I assume you mean 'core.attributesFile' because it's in the title. But
> personnally I don't mind seeing the name repeated in the body to make it
> more clear.

>
> > eagerly and stored in the global variable `git_attributes_file`.
> > Storing this value in a global variable can lead to unexpected
> > behaviours 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()` and
> > can be accessed per repository via `git_default_config()`.
>
> I'd suggest to split off the part after the second 'and' into a new
> sentence.

>
> > This will ensure the current behaviour remains the same while also
> > enabling the libification of Git.
>
> How is this true? Was that value already accessible through
> `git_default_config()`?
>
> > It is important to note that `git_default_config()` is a wrapper to other
> > `git_default_*_config()` such as `git_default_core_config()`.
>
> I'd suggest to insert 'functions' before 'such'.

>
> > Therefore to access and modify this global variable,
> > the change has to be made in the function which parses and
> > stores the value in the global variable.
>
> This doesn't clarify much for me. Do you mean 'git_attr_global_file()'
> and 'git_default_core_config()'?

>
> >
> > Suggested-by: Phillip Wood <phillip.wood123@gmail.com>
> > Mentored-by: Christian Couder <christian.couder@gmail.com>
> > Mentored-by: Usman Akinyemi <usmanakinyemi202@gmail.com>
> > Signed-off-by: Olamide Caleb Bello <belkid98@gmail.com>
> > ---
> >  attr.c        |  7 ++++---
> >  environment.c | 12 +++++++++---
> >  environment.h |  9 ++++++++-
> >  repository.c  |  1 +
> >  repository.h  |  4 ++++
> >  5 files changed, 26 insertions(+), 7 deletions(-)
> >
> > diff --git a/attr.c b/attr.c
> > index 4999b7e09d..fbb9eaffaf 100644
> > --- a/attr.c
> > +++ b/attr.c
> > @@ -881,10 +881,11 @@ const char *git_attr_system_file(void)
> >
> >  const char *git_attr_global_file(void)
> >  {
> > -     if (!git_attributes_file)
> > -             git_attributes_file = xdg_config_home("attributes");
> > +     struct repo_config_values *cfg = &the_repository->config_values;
>
> Is 'cfg' guaranteed to be != NULL?
>
> > +     if (!cfg->attributes_file_path)
> > +             cfg->attributes_file_path = xdg_config_home("attributes");
> >
> > -     return git_attributes_file;
> > +     return cfg->attributes_file_path;
> >  }
> >
> >  int git_attr_system_is_enabled(void)
> > diff --git a/environment.c b/environment.c
> > index a770b5921d..283db0a1a0 100644
> > --- a/environment.c
> > +++ b/environment.c
> > @@ -53,7 +53,6 @@ char *git_commit_encoding;
> >  char *git_log_output_encoding;
> >  char *apply_default_whitespace;
> >  char *apply_default_ignorewhitespace;
> > -char *git_attributes_file;
> >  int zlib_compression_level = Z_BEST_SPEED;
> >  int pack_compression_level = Z_DEFAULT_COMPRESSION;
> >  int fsync_object_files = -1;
> > @@ -327,6 +326,8 @@ static enum fsync_component parse_fsync_components(const char *var, const char *
> >  static int git_default_core_config(const char *var, const char *value,
> >                                  const struct config_context *ctx, void *cb)
> >  {
> > +     struct repo_config_values *cfg = &the_repository->config_values;
> > +
> >       /* This needs a better name */
> >       if (!strcmp(var, "core.filemode")) {
> >               trust_executable_bit = git_config_bool(var, value);
> > @@ -364,8 +365,8 @@ static int git_default_core_config(const char *var, const char *value,
> >       }
> >
> >       if (!strcmp(var, "core.attributesfile")) {
> > -             FREE_AND_NULL(git_attributes_file);
> > -             return git_config_pathname(&git_attributes_file, var, value);
> > +             FREE_AND_NULL(cfg->attributes_file_path);
> > +             return git_config_pathname(&cfg->attributes_file_path, var, value);
> >       }
> >
> >       if (!strcmp(var, "core.bare")) {
> > @@ -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_path = NULL;
> > +}
>
> I assume the reason for adding this function becomes clear in a later
> commit?

>
> > diff --git a/environment.h b/environment.h
> > index 51898c99cd..aea73ff25b 100644
> > --- a/environment.h
> > +++ b/environment.h
> > @@ -84,6 +84,12 @@ extern const char * const local_repo_env[];
> >
> >  struct strvec;
> >
> > +/* Config values parsed by git_default_config() */
>
> Mentioning here they get filled from git_default_config() doesn't feel
> really correct? Although I'm sure what comment would fit better, maybe
> just drop the comment above the struct. I see you have a similar comment
> in 'struct repository', where it *does* make sense.

>
> > +struct repo_config_values {
> > +     /* core config values */
>
> I prefer emphasizing it's the "section 'core'" or something like that.

>
> > +     char *attributes_file_path;
>
> Would it be overkill to append: /* `core.attributesFile` */? This can
> help when grepping through the codebase to find where some settings are
> being parsed into. What do you think?

>
> > +};
> > +
> >  /*
> >   * Wrapper of getenv() that returns a strdup value. This value is kept
> >   * in argv to be freed later.
> > @@ -107,6 +113,8 @@ const char *strip_namespace(const char *namespaced_ref);
> >  int git_default_config(const char *, const char *,
> >                      const struct config_context *, void *);
> >
> > +void repo_config_values_init(struct repo_config_values *cfg);
> > +
> >  /*
> >   * TODO: All the below state either explicitly or implicitly relies on
> >   * `the_repository`. We should eventually get rid of these and make the
> > @@ -152,7 +160,6 @@ extern int assume_unchanged;
> >  extern int warn_on_object_refname_ambiguity;
> >  extern char *apply_default_whitespace;
> >  extern char *apply_default_ignorewhitespace;
> > -extern char *git_attributes_file;
> >  extern int zlib_compression_level;
> >  extern int pack_compression_level;
> >  extern unsigned long pack_size_limit_cfg;
> > diff --git a/repository.c b/repository.c
> > index c7e75215ac..d308cd78bf 100644
> > --- a/repository.c
> > +++ b/repository.c
> > @@ -57,6 +57,7 @@ void initialize_repository(struct repository *repo)
> >       ALLOC_ARRAY(repo->index, 1);
> >       index_state_init(repo->index, repo);
> >       repo->check_deprecated_config = true;
> > +     repo_config_values_init(&repo->config_values);
> >
> >       /*
> >        * When a command runs inside a repository, it learns what
> > diff --git a/repository.h b/repository.h
> > index 6063c4b846..638a142577 100644
> > --- a/repository.h
> > +++ b/repository.h
> > @@ -3,6 +3,7 @@
> >
> >  #include "strmap.h"
> >  #include "repo-settings.h"
> > +#include "environment.h"
> >
> >  struct config_set;
> >  struct git_hash_algo;
> > @@ -148,6 +149,9 @@ struct repository {
> >       /* Repository's compatibility hash algorithm. */
> >       const struct git_hash_algo *compat_hash_algo;
> >
> > +     /* Repository's config values parsed by git_default_config() */
> > +     struct repo_config_values config_values;
> > +
> >       /* Repository's reference storage format, as serialized on disk. */
> >       enum ref_storage_format ref_storage_format;
> >
> > --
> > 2.34.1
> >
> >
>
> --
> Cheers,
> Toon