[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.

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

[SZEDER Gábor]

On Fri, Jun 26, 2026 at 03:50:36PM +0800, Tian Yuchen wrote:
> 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;
> +}

This function has a 'repo' parameter, which is not used in the
function at all.  This causes build failure when trying to build this
commit using DEVELOPER=1:

  environment.c: In function ‘repo_excludes_file’:
  environment.c:137:51: error: unused parameter ‘repo’ [-Werror=unused-parameter]
    137 | const char *repo_excludes_file(struct repository *repo)
        |                                ~~~~~~~~~~~~~~~~~~~^~~~
  cc1: all warnings being treated as errors
  make: *** [Makefile:2922: environment.o] Error 1

Please make sure that all commits can be built with 'make
DEVELOPER=1'.

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

[Junio C Hamano]

SZEDER Gábor <szeder.dev@gmail.com> writes:

> On Fri, Jun 26, 2026 at 03:50:36PM +0800, Tian Yuchen wrote:
>> 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;
>> +}
>
> This function has a 'repo' parameter, which is not used in the
> function at all.  This causes build failure when trying to build this
> commit using DEVELOPER=1:
>
>   environment.c: In function ‘repo_excludes_file’:
>   environment.c:137:51: error: unused parameter ‘repo’ [-Werror=unused-parameter]
>     137 | const char *repo_excludes_file(struct repository *repo)
>         |                                ~~~~~~~~~~~~~~~~~~~^~~~
>   cc1: all warnings being treated as errors
>   make: *** [Makefile:2922: environment.o] Error 1
>
> Please make sure that all commits can be built with 'make
> DEVELOPER=1'.

Good point.  In this case, we can start with UNUSED in step 1/2 and
then drop the UNUSED in the second step.  I wonder how harder to read
it would become if these two patches are squashed together...

Thanks.

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

[Tian Yuchen]

On 6/26/26 23:42, Junio C Hamano wrote:

Sorry, there was a problem with my Thunderbird client... so the my 
emails didn't send without me noticing...

> 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.

Regarding this, I forgot to bring --in-reply-to. I'll remember next time.

Thanks, yuchen

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

[Tian Yuchen]

On 6/27/26 03:12, Junio C Hamano wrote:
> 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.

Yes, indeed. I initially thought these were two solutions leading to the 
same goal, but now that you mention it, I think your point is more valid.

First, 'repo_config_values_clear()' shouldn't know whether "only 
'the_repository' is currently supported" (even if this logic is 
necessary, this function is clearly not the optimal place), so we 
shouldn't use 'repo != the_repository' to determine this.

Second, what's wrong with '!cfg' is that what it's supposed to avoid 
deviates from what it tries to avoid. 'repo_config_values(repo)' 
shouldn't validly return NULL at all. I admit this code is nonsensical.

Using 'repo->initialized' to determine this is consistent with our 
previous approach. I'll try it. Thanks for the review!

Regards, yuchen

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

[Tian Yuchen]

On 6/27/26 05:45, Junio C Hamano wrote:
> SZEDER Gábor <szeder.dev@gmail.com> writes:
> 
>> On Fri, Jun 26, 2026 at 03:50:36PM +0800, Tian Yuchen wrote:
>>> 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;
>>> +}
>>
>> This function has a 'repo' parameter, which is not used in the
>> function at all.  This causes build failure when trying to build this
>> commit using DEVELOPER=1:
>>
>>    environment.c: In function ‘repo_excludes_file’:
>>    environment.c:137:51: error: unused parameter ‘repo’ [-Werror=unused-parameter]
>>      137 | const char *repo_excludes_file(struct repository *repo)
>>          |                                ~~~~~~~~~~~~~~~~~~~^~~~
>>    cc1: all warnings being treated as errors
>>    make: *** [Makefile:2922: environment.o] Error 1
>>
>> Please make sure that all commits can be built with 'make
>> DEVELOPER=1'.
> 
> Good point.  In this case, we can start with UNUSED in step 1/2 and
> then drop the UNUSED in the second step.  I wonder how harder to read
> it would become if these two patches are squashed together...

That makes sense. I think these two commits can be squadshed together... 
I hope so.

> 
> Thanks.

Regards, yuchen

[PATCH v4 0/1] environment: move excludes_file into repo_config_values

[Tian Yuchen]

This patch 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 patch mainly does three things:

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

 - Move the variables into the struct repo_config_values.

 - Introduce the memory destructor 'repo_config_values_clear()'.


Changes since V2:

 - Squash together the previous two commits into one.

 - The 'repo->initialized' check is used in both the getter and destructor.
 This eliminates redundant checks and follows the fail-fast principle. This
 is consistent with the previous global variable removal patches [1][2][3].

Note: Resending as v3 purely to fix a malformed In-Reply-To header in v2
that broke the email thread. No actual code changes since v2...

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/20260610093635.139719-1-cat@malon.dev/T/#m856253610936d052a798259bfc06d598561e53c4
[2] https://lore.kernel.org/git/20260606143412.15443-1-cat@malon.dev/
[3] https://lore.kernel.org/git/20260617154929.564498-2-cat@malon.dev/T/#m8843984a6175a1a4c7e00877085c77b0c72f5803

Tian Yuchen (1):
  environment: move excludes_file into repo_config_values

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

-- 
2.43.0

[PATCH v4 1/1] environment: move excludes_file into repo_config_values

[Tian Yuchen]

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

Continue the libification effort by encapsulating this lazy-loading
fallback logic into a proper getter and moving the variable into
'struct repo_config_values'.

Since 'excludes_file' is a dynamically allocated string, it requires
proper heap memory management. Introduce repo_config_values_clear()
and wire it up in repo_clear() to safely free this memory when a
repository instance is destroyed. Also clean up the heap-allocated
'attributes_file' in this new destructor while we are at it.

Note: 'if (!repo->initialized)' is added in both the getter and the
destructor. This ensures we safely return or bypass cleaning up
uninitialized repositories without hardcoding a dependency on
'the_repository'.

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 | 30 +++++++++++++++++++++++++++---
 environment.h | 13 ++++++++++++-
 repository.c  |  1 +
 4 files changed, 42 insertions(+), 6 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..2519c60918 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;
@@ -134,6 +133,17 @@ 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 (!repo || !repo->initialized)
+		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)
 {
 	return startup_info->have_repository
@@ -461,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")) {
@@ -715,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;
@@ -726,3 +737,16 @@ 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->initialized)
+		return;
+
+	cfg = repo_config_values(repo);
+
+	FREE_AND_NULL(cfg->attributes_file);
+	FREE_AND_NULL(cfg->excludes_file);
+}
diff --git a/environment.h b/environment.h
index 6f18286955..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,8 +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);
 
+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
@@ -208,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 v4 1/1] environment: move excludes_file into repo_config_values

[Tian Yuchen]

Hi all,

Apologies again for the duplicate...

On 6/28/26 00:08, Tian Yuchen wrote:

> +const char *repo_excludes_file(struct repository *repo)
> +{
> +	if (!repo || !repo->initialized)
> +		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;
> +}

One more thing:

I deliberately didn't write a comment for the getter because it will 
probably be merged with comments from the previous several patches in 
some form in the near future... I'm not sure if it would be more 
appropriate to write a separate patch to add the corresponding comments 
then.

Regards, yuchen

Re: [PATCH v4 1/1] environment: move excludes_file into repo_config_values

[Junio C Hamano]

Tian Yuchen <cat@malon.dev> writes:

> Hi all,
>
> Apologies again for the duplicate...
>
> On 6/28/26 00:08, Tian Yuchen wrote:
>
>> +const char *repo_excludes_file(struct repository *repo)
>> +{
>> +	if (!repo || !repo->initialized)
>> +		return NULL;

I might already have said this, but I am not sure why want to be as
loose as this code.  It is not limited to this line, but I think we
saw plenty of other "We know we must get an already initialized
thing here, and the subsequent operation we perform on that thing
will cause us to die() later, so let's return silently and early
to avoid hitting die()" attempts to sweep problems under the rug.

Wouldn't we rather want to try to be more strict and say

	if (!repo || !repo->initialized)
		BUG("repo must be an initialied repository");

here?  Aren't all the callers of this function supposed to be
dealing with an already initialized repository?


>> +	if (!repo_config_values(repo)->excludes_file)
>> +		repo_config_values(repo)->excludes_file = xdg_config_home("ignore");
>> +
>> +	return repo_config_values(repo)->excludes_file;
>> +}
>
> One more thing:
>
> I deliberately didn't write a comment for the getter because it will 
> probably be merged with comments from the previous several patches in 
> some form in the near future... I'm not sure if it would be more 
> appropriate to write a separate patch to add the corresponding comments 
> then.

That's very sensible.