[PATCH v2 0/2] environment: move excludes_file into repo_config_values

[PATCH v2 0/2] environment: move excludes_file into repo_config_values

[Tian Yuchen]

This series continues the libification effort by migrating the global
string variable 'excludes_file' into 'struct repo_config_values'. Since
this is a dynamically allocated variable, the migration requires proper
heap memory management.

The series is structured in two commits:

 - Abstract the XDG fallback lazy-loading logic out of dir.c into a proper
getter.

 - Move the variables into the struct and introduce 'repo_config_values_clear()'.

Note on Submodules: A temporary shield 'if (repo != the_repository)' is
included in both the getter and the clear function. This prevents
uninitialized submodules from triggering the BUG() assertion.
(Inspiration: [1])

Changes since V1:

 - fix a typo in the second commit.

 - initialize excludes_file to NULL in repo_config_values_init().

 - call FREE_AND_NULL() to free attributes_file as well in
 repo_config_values_clear().

Thanks!

Mentored-by: Christian Couder <christian.couder@gmail.com>
Mentored-by: Ayush Chandekar <ayu.chandekar@gmail.com>
Mentored-by: Olamide Caleb Bello <belkid98@gmail.com>
Signed-off-by: Tian Yuchen <cat@malon.dev>

[1] https://lore.kernel.org/git/c95a7730-7b14-4be0-a4e4-861b2f5430ea@gmail.com/

Tian Yuchen (2):
  dir: encapsulate excludes_file lazy-load
  environment: move excludes_file into repo_config_values

 dir.c         |  4 ++--
 environment.c | 32 +++++++++++++++++++++++++++++---
 environment.h | 13 ++++++++++++-
 repository.c  |  1 +
 4 files changed, 44 insertions(+), 6 deletions(-)

-- 
2.43.0

[PATCH v2 1/2] dir: encapsulate excludes_file lazy-load

[Tian Yuchen]

The global variable 'excludes_file' is used to track the path to the
global ignore file, 'core.excludesfile'. If this variable is NULL,
setup_standard_excludes() in dir.c forcefully evaluates and assigns
the XDG default path to it.

Introduce repo_excludes_file() as a getter to encapsulate this
lazy-loading logic. This prepares the variable to be safely moved
into 'struct repo_config_values' in the subsequent commit.

Mentored-by: Christian Couder <christian.couder@gmail.com>
Mentored-by: Ayush Chandekar <ayu.chandekar@gmail.com>
Mentored-by: Olamide Caleb Bello <belkid98@gmail.com>
Signed-off-by: Tian Yuchen <cat@malon.dev>
---
 dir.c         | 4 ++--
 environment.c | 7 +++++++
 environment.h | 5 +++++
 3 files changed, 14 insertions(+), 2 deletions(-)

diff --git a/dir.c b/dir.c
index 7a73690fbc..4f87a52b3c 100644
--- a/dir.c
+++ b/dir.c
@@ -3481,11 +3481,11 @@ static GIT_PATH_FUNC(git_path_info_exclude, "info/exclude")
 
 void setup_standard_excludes(struct dir_struct *dir)
 {
+	const char *excludes_file = repo_excludes_file(the_repository);
+
 	dir->exclude_per_dir = ".gitignore";
 
 	/* core.excludesfile defaulting to $XDG_CONFIG_HOME/git/ignore */
-	if (!excludes_file)
-		excludes_file = xdg_config_home("ignore");
 	if (excludes_file && !access_or_warn(excludes_file, R_OK, 0))
 		add_patterns_from_file_1(dir, excludes_file,
 					 dir->untracked ? &dir->internal.ss_excludes_file : NULL);
diff --git a/environment.c b/environment.c
index ba2c60103f..8efcaeafa6 100644
--- a/environment.c
+++ b/environment.c
@@ -134,6 +134,13 @@ int is_bare_repository(void)
 	return is_bare_repository_cfg && !repo_get_work_tree(the_repository);
 }
 
+const char *repo_excludes_file(struct repository *repo)
+{
+	if (!excludes_file)
+		excludes_file = xdg_config_home("ignore");
+	return excludes_file;
+}
+
 int have_git_dir(void)
 {
 	return startup_info->have_repository
diff --git a/environment.h b/environment.h
index 6f18286955..52d531e4ea 100644
--- a/environment.h
+++ b/environment.h
@@ -133,6 +133,11 @@ int git_default_config(const char *, const char *,
 int git_default_core_config(const char *var, const char *value,
 			    const struct config_context *ctx, void *cb);
 
+/*
+ * TODO: This still relies on the global state.
+ */
+const char *repo_excludes_file(struct repository *repo);
+
 void repo_config_values_init(struct repo_config_values *cfg);
 
 /*
-- 
2.43.0

[PATCH v2 2/2] environment: move excludes_file into repo_config_values

[Tian Yuchen]

Continue the libification effort by moving the 'excludes_file' global
variable into 'struct repo_config_values'.

Since 'excludes_file' is a dynamically allocated string (char *), it
requires proper memory management. Introduce repo_config_values_clear()
to safely free the heap memory when repository instance is destroyed.

Note:

 - 'if (repo != the_repository)' fallback logic is temporarily added
in both the getter and the clear function. This prevents calling
repo_config_values() on uninitialized submodules, which triggers BUG().

 - 'attribute_file' is another string variable that was migrated
 earlier. Its FREE_AND_NULL() call is also added to
 repo_config_values_clear().

Mentored-by: Christian Couder <christian.couder@gmail.com>
Mentored-by: Ayush Chandekar <ayu.chandekar@gmail.com>
Mentored-by: Olamide Caleb Bello <belkid98@gmail.com>
Signed-off-by: Tian Yuchen <cat@malon.dev>
---
 environment.c | 31 +++++++++++++++++++++++++------
 environment.h | 14 ++++++++++----
 repository.c  |  1 +
 3 files changed, 36 insertions(+), 10 deletions(-)

diff --git a/environment.c b/environment.c
index 8efcaeafa6..b8dfa3e213 100644
--- a/environment.c
+++ b/environment.c
@@ -57,7 +57,6 @@ enum fsync_method fsync_method = FSYNC_METHOD_DEFAULT;
 enum fsync_component fsync_components = FSYNC_COMPONENTS_DEFAULT;
 char *editor_program;
 char *askpass_program;
-char *excludes_file;
 enum auto_crlf auto_crlf = AUTO_CRLF_FALSE;
 enum eol core_eol = EOL_UNSET;
 int global_conv_flags_eol = CONV_EOL_RNDTRP_WARN;
@@ -136,9 +135,13 @@ int is_bare_repository(void)
 
 const char *repo_excludes_file(struct repository *repo)
 {
-	if (!excludes_file)
-		excludes_file = xdg_config_home("ignore");
-	return excludes_file;
+	if (!repo || !repo->initialized || repo != the_repository)
+		return NULL;
+
+	if (!repo_config_values(repo)->excludes_file)
+		repo_config_values(repo)->excludes_file = xdg_config_home("ignore");
+
+	return repo_config_values(repo)->excludes_file;
 }
 
 int have_git_dir(void)
@@ -468,8 +471,8 @@ int git_default_core_config(const char *var, const char *value,
 	}
 
 	if (!strcmp(var, "core.excludesfile")) {
-		FREE_AND_NULL(excludes_file);
-		return git_config_pathname(&excludes_file, var, value);
+		FREE_AND_NULL(cfg->excludes_file);
+		return git_config_pathname(&cfg->excludes_file, var, value);
 	}
 
 	if (!strcmp(var, "core.whitespace")) {
@@ -722,6 +725,7 @@ int git_default_config(const char *var, const char *value,
 void repo_config_values_init(struct repo_config_values *cfg)
 {
 	cfg->attributes_file = NULL;
+	cfg->excludes_file = NULL;
 	cfg->apply_sparse_checkout = 0;
 	cfg->branch_track = BRANCH_TRACK_REMOTE;
 	cfg->trust_ctime = 1;
@@ -733,3 +737,18 @@ void repo_config_values_init(struct repo_config_values *cfg)
 	cfg->sparse_expect_files_outside_of_patterns = 0;
 	cfg->warn_on_object_refname_ambiguity = 1;
 }
+
+void repo_config_values_clear(struct repository *repo)
+{
+	struct repo_config_values *cfg;
+
+	if (repo != the_repository)
+		return;
+
+	cfg = repo_config_values(repo);
+	if (!cfg)
+		return;
+
+	FREE_AND_NULL(cfg->attributes_file);
+	FREE_AND_NULL(cfg->excludes_file);
+}
diff --git a/environment.h b/environment.h
index 52d531e4ea..2e8352de7f 100644
--- a/environment.h
+++ b/environment.h
@@ -90,6 +90,7 @@ struct repository;
 struct repo_config_values {
 	/* section "core" config values */
 	char *attributes_file;
+	char *excludes_file;
 	int apply_sparse_checkout;
 	int trust_ctime;
 	int check_stat;
@@ -133,13 +134,19 @@ int git_default_config(const char *, const char *,
 int git_default_core_config(const char *var, const char *value,
 			    const struct config_context *ctx, void *cb);
 
-/*
- * TODO: This still relies on the global state.
- */
 const char *repo_excludes_file(struct repository *repo);
 
 void repo_config_values_init(struct repo_config_values *cfg);
 
+/*
+ * Frees memory allocated for dynamically loaded configuration values
+ * inside `repo_config_values`.
+ *
+ * As dynamically allocated variables are migrated into this struct,
+ * their FREE_AND_NULL() calls should be appended here.
+ */
+void repo_config_values_clear(struct repository *repo);
+
 /*
  * TODO: All the below state either explicitly or implicitly relies on
  * `the_repository`. We should eventually get rid of these and make the
@@ -213,7 +220,6 @@ extern char *git_log_output_encoding;
 
 extern char *editor_program;
 extern char *askpass_program;
-extern char *excludes_file;
 
 /*
  * The character that begins a commented line in user-editable file
diff --git a/repository.c b/repository.c
index 187dd471c4..b31f1b7852 100644
--- a/repository.c
+++ b/repository.c
@@ -388,6 +388,7 @@ void repo_clear(struct repository *repo)
 	FREE_AND_NULL(repo->parsed_objects);
 
 	repo_settings_clear(repo);
+	repo_config_values_clear(repo);
 
 	if (repo->config) {
 		git_configset_clear(repo->config);
-- 
2.43.0

Re: [PATCH v2 0/2] environment: move excludes_file into repo_config_values

[Junio C Hamano]

Tian Yuchen <cat@malon.dev> writes:

> This series continues the libification effort by migrating the global
> string variable 'excludes_file' into 'struct repo_config_values'. Since
> this is a dynamically allocated variable, the migration requires proper
> heap memory management.

This appears here:

  https://lore.kernel.org/git/20260626075037.532164-1-cat@malon.dev/

and as you can see, there is no linkage back to the previous round.

The lack of In-Reply-To and References headers unfortunately delays my
automation in marking topics with newer iterations available to be
reviewed when I come back to the keyboard, which happens overnight.

Re: [PATCH v2 2/2] environment: move excludes_file into repo_config_values

[Junio C Hamano]

Tian Yuchen <cat@malon.dev> writes:

> Continue the libification effort by moving the 'excludes_file' global
> variable into 'struct repo_config_values'.
>
> Since 'excludes_file' is a dynamically allocated string (char *), it
> requires proper memory management. Introduce repo_config_values_clear()
> to safely free the heap memory when repository instance is destroyed.
>
> Note:
>
>  - 'if (repo != the_repository)' fallback logic is temporarily added
> in both the getter and the clear function. This prevents calling
> repo_config_values() on uninitialized submodules, which triggers BUG().
>
>  - 'attribute_file' is another string variable that was migrated
>  earlier. Its FREE_AND_NULL() call is also added to
>  repo_config_values_clear().

OK.  I think the placement of the new member in repo_config_values
in this round, moving to the spot next to existing attributes_file,
makes more sense than the previous round, too.

Looking good.  Shall we mark it as ready for 'next' now?

Re: [PATCH v2 2/2] environment: move excludes_file into repo_config_values

[Junio C Hamano]

Tian Yuchen <cat@malon.dev> writes:

> Continue the libification effort by moving the 'excludes_file' global
> variable into 'struct repo_config_values'.
>
> Since 'excludes_file' is a dynamically allocated string (char *), it
> requires proper memory management. Introduce repo_config_values_clear()
> to safely free the heap memory when repository instance is destroyed.
>
> Note:
>
>  - 'if (repo != the_repository)' fallback logic is temporarily added
> in both the getter and the clear function. This prevents calling
> repo_config_values() on uninitialized submodules, which triggers BUG().

Would it be possible for the function to be called on the_repository
before it gets initialized?

> +void repo_config_values_clear(struct repository *repo)
> +{
> +	struct repo_config_values *cfg;

What I am wondering is if this check

> +	if (repo != the_repository)
> +		return;

wants to be more like

	if (!repo->initialized)
		return;

or even

	if (!repo->initialized) {
		BUG("clearing uninitialised repo config");
		return;
	}

Or perhaps not doing anything special there.

For that matter,

> +
> +	cfg = repo_config_values(repo);
> +	if (!cfg)
> +		return;

Wouldn't it be a bug to see NULL returned from the above function?
Why is it healthy to pretend as if nothing bad happened?

> +	FREE_AND_NULL(cfg->attributes_file);
> +	FREE_AND_NULL(cfg->excludes_file);
> +}


What do we want to happen when somebody does want to access (not
_clear(), but repo_config_values() itself) repo_config_values() in
today's code?  Don't we want to catch such a code as buggy?  Isn't
it the reason why repository.c:repo_config_values() check these
conditions and calls BUG() in the first place?  And if that is the
case, I find that a caller that "works around" by pretending nothing
bad happened and not calling repo_config_values(), like the above
code does, highly questionable, as it smells like sweeping problems
that you designed other parts of the code to detect with BUG() under
the rug.

In the longer run, we would want to have separate settings, which
used to be global variables but now are stored in per repository
config, available and usable in the context of each repository that
they are configured within.  If a caller wants to clear per repo
config for a repository instance by calling this function, this
function is in no place to tweak the intention of the caller by
short-circuiting the request and pretending it did what it was asked
to do.  In other words, the rest of the code not quite prepared to
deal with these global variables that turned into per repository
configuration values is *not* a problem this function should
address.  Let it be noticed by repo_config_values() function to
catch offending callers for now, and once the codebase becomes ready
to use one repo_config_values per repository, this function does not
have to change.