[PATCH 0/3] environment: clean up repository config handling

[PATCH 0/3] environment: clean up repository config handling

[Tian Yuchen]

Hi all,

This series contains several cleanup patches for repository configuration
handling.

No functional changes are intended. The patches make the related code
more consistent and easier to maintain by improving documentation,
formatting, and the organization of repo_config_values.

RFC:

If there are other small cleanups in this area that would be useful to
include, suggestions are welcome.

Regards, yuchen

Tian Yuchen (3):
  environment: simplify repository config getters
  environment: clarify repository config getter documentation
  environment: reorder variables in repo_config_values structure

 environment.c | 49 ++++++++++++++++++++++++++++++-------------------
 environment.h | 31 ++++++++++++++++---------------
 2 files changed, 46 insertions(+), 34 deletions(-)

-- 
2.43.0

[PATCH 1/3] environment: simplify repository config getters

[Tian Yuchen]

Drop unnecessary parentheses and NULL checks in repository config
getters.

These getters are only used with non-NULL repositories, so the
extra checks do not match their current callers.

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 | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/environment.c b/environment.c
index 76ee65e62b..f5628b6758 100644
--- a/environment.c
+++ b/environment.c
@@ -119,23 +119,23 @@ int is_bare_repository(struct repository *repo)
 
 int repo_protect_ntfs(struct repository *repo)
 {
-	return (repo && repo->initialized) ?
-		repo_config_values(repo)->protect_ntfs :
-		PROTECT_NTFS_DEFAULT;
+	return repo->initialized
+		? repo_config_values(repo)->protect_ntfs
+		: PROTECT_NTFS_DEFAULT;
 }
 
 int repo_protect_hfs(struct repository *repo)
 {
-	return (repo && repo->initialized) ?
-		repo_config_values(repo)->protect_hfs :
-		PROTECT_HFS_DEFAULT;
+	return repo->initialized
+		? repo_config_values(repo)->protect_hfs
+		: PROTECT_HFS_DEFAULT;
 }
 
 int repo_ignore_case(struct repository *repo)
 {
-	return (repo && repo->initialized) ?
-		repo_config_values(repo)->ignore_case :
-		0;
+	return repo->initialized
+		? repo_config_values(repo)->ignore_case
+		: 0;
 }
 
 int repo_trust_executable_bit(struct repository *repo)
-- 
2.43.0

Re: [PATCH 1/3] environment: simplify repository config getters

[Junio C Hamano]

Tian Yuchen <cat@malon.dev> writes:

> Drop unnecessary parentheses and NULL checks in repository config
> getters.
>
> These getters are only used with non-NULL repositories, so the
> extra checks do not match their current callers.

OK.  If repo MUST always be non-NULL, even when we haven't fully
initialized them, then not punting on repo==NULL case like the
original code is definitely an improvement.

Looking good.

>
> 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 | 18 +++++++++---------
>  1 file changed, 9 insertions(+), 9 deletions(-)
>
> diff --git a/environment.c b/environment.c
> index 76ee65e62b..f5628b6758 100644
> --- a/environment.c
> +++ b/environment.c
> @@ -119,23 +119,23 @@ int is_bare_repository(struct repository *repo)
>  
>  int repo_protect_ntfs(struct repository *repo)
>  {
> -	return (repo && repo->initialized) ?
> -		repo_config_values(repo)->protect_ntfs :
> -		PROTECT_NTFS_DEFAULT;
> +	return repo->initialized
> +		? repo_config_values(repo)->protect_ntfs
> +		: PROTECT_NTFS_DEFAULT;
>  }
>  
>  int repo_protect_hfs(struct repository *repo)
>  {
> -	return (repo && repo->initialized) ?
> -		repo_config_values(repo)->protect_hfs :
> -		PROTECT_HFS_DEFAULT;
> +	return repo->initialized
> +		? repo_config_values(repo)->protect_hfs
> +		: PROTECT_HFS_DEFAULT;
>  }
>  
>  int repo_ignore_case(struct repository *repo)
>  {
> -	return (repo && repo->initialized) ?
> -		repo_config_values(repo)->ignore_case :
> -		0;
> +	return repo->initialized
> +		? repo_config_values(repo)->ignore_case
> +		: 0;
>  }
>  
>  int repo_trust_executable_bit(struct repository *repo)

[PATCH 2/3] environment: clarify repository config getter documentation

[Tian Yuchen]

Update the comment above repository config getters to describe their
common behavior.

The getters handle repositories that are not fully initialized by
returning the corresponding default values.

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.h | 11 +++--------
 1 file changed, 3 insertions(+), 8 deletions(-)

diff --git a/environment.h b/environment.h
index e7ec5b0437..30678257b5 100644
--- a/environment.h
+++ b/environment.h
@@ -175,18 +175,13 @@ int git_default_core_config(const char *var, const char *value,
 			    const struct config_context *ctx, void *cb);
 
 /*
- * Getters for the `protect_hfs` and `protect_ntfs` fields of `struct repo_config_values`.
- * They check `repo->initialized` to prevent calling `repo_config_values()`
- * before the repository setup is fully complete or in non-git environments.
+ * Getters for configuration variables in `struct repo_config_values`.
+ * These functions handle uninitialized repositories or non-git
+ * environments by returning appropriate default values.
  */
 int repo_protect_hfs(struct repository *repo);
 int repo_protect_ntfs(struct repository *repo);
 
-/*
- * Getter for the `ignore_case` field of `struct repo_config_values`.
- * It checks `repo->initialized` to prevent calling repo_config_values()`
- * before the repository setup is fully complete or in non-git environments.
- */
 int repo_ignore_case(struct repository *repo);
 
 int repo_trust_executable_bit(struct repository *repo);
-- 
2.43.0

Re: [PATCH 2/3] environment: clarify repository config getter documentation

[Junio C Hamano]

Tian Yuchen <cat@malon.dev> writes:

> Update the comment above repository config getters to describe their
> common behavior.
>
> The getters handle repositories that are not fully initialized by
> returning the corresponding default values.
>
> 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.h | 11 +++--------
>  1 file changed, 3 insertions(+), 8 deletions(-)
>
> diff --git a/environment.h b/environment.h
> index e7ec5b0437..30678257b5 100644
> --- a/environment.h
> +++ b/environment.h
> @@ -175,18 +175,13 @@ int git_default_core_config(const char *var, const char *value,
>  			    const struct config_context *ctx, void *cb);
>  
>  /*
> - * Getters for the `protect_hfs` and `protect_ntfs` fields of `struct repo_config_values`.
> - * They check `repo->initialized` to prevent calling `repo_config_values()`
> - * before the repository setup is fully complete or in non-git environments.
> + * Getters for configuration variables in `struct repo_config_values`.
> + * These functions handle uninitialized repositories or non-git
> + * environments by returning appropriate default values.
>   */
>  int repo_protect_hfs(struct repository *repo);
>  int repo_protect_ntfs(struct repository *repo);
>  
> -/*

Two puzzlements.

 * Is the above comment block meant to apply to repo_ignore_case()
   in addition to repo_protect_ntfs() and repo_protect_hfs()?  If
   so, the blank line before repo_ignore_case() is a bit misleading.

 * The phrase "uninitialized repositories or non-Git environments"
   strongly hints that I can pass NULL to indicate that we are
   running in a non-Git environment.  However, the change in
   [PATCH 1/3] we just saw means I would get a segfault if I did so,
   does it not?

> - * Getter for the `ignore_case` field of `struct repo_config_values`.
> - * It checks `repo->initialized` to prevent calling repo_config_values()`
> - * before the repository setup is fully complete or in non-git environments.
> - */
>  int repo_ignore_case(struct repository *repo);
>  
>  int repo_trust_executable_bit(struct repository *repo);

Re: [PATCH 2/3] environment: clarify repository config getter documentation

[Tian Yuchen]

On 8/6/26 05:38, Junio C Hamano wrote:
> Tian Yuchen <cat@malon.dev> writes:
> 
>> Update the comment above repository config getters to describe their
>> common behavior.
>>
>> The getters handle repositories that are not fully initialized by
>> returning the corresponding default values.
>>
>> 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.h | 11 +++--------
>>   1 file changed, 3 insertions(+), 8 deletions(-)
>>
>> diff --git a/environment.h b/environment.h
>> index e7ec5b0437..30678257b5 100644
>> --- a/environment.h
>> +++ b/environment.h
>> @@ -175,18 +175,13 @@ int git_default_core_config(const char *var, const char *value,
>>   			    const struct config_context *ctx, void *cb);
>>   
>>   /*
>> - * Getters for the `protect_hfs` and `protect_ntfs` fields of `struct repo_config_values`.
>> - * They check `repo->initialized` to prevent calling `repo_config_values()`
>> - * before the repository setup is fully complete or in non-git environments.
>> + * Getters for configuration variables in `struct repo_config_values`.
>> + * These functions handle uninitialized repositories or non-git
>> + * environments by returning appropriate default values.
>>    */
>>   int repo_protect_hfs(struct repository *repo);
>>   int repo_protect_ntfs(struct repository *repo);
>>   
>> -/*
> 
> Two puzzlements.
> 
>   * Is the above comment block meant to apply to repo_ignore_case()
>     in addition to repo_protect_ntfs() and repo_protect_hfs()?  If
>     so, the blank line before repo_ignore_case() is a bit misleading.
> 

Not really, they are meant to apply to all getters below. I will remove 
the blank lines.

>   * The phrase "uninitialized repositories or non-Git environments"
>     strongly hints that I can pass NULL to indicate that we are
>     running in a non-Git environment.  However, the change in
>     [PATCH 1/3] we just saw means I would get a segfault if I did so,
>     does it not?
> 

This is a mistake. I meant "these getters can handle repositories, even 
when they are not fully initailzed" but not "these getters can handle 
whatever we pass in". So I will change it in the next reroll.

>> - * Getter for the `ignore_case` field of `struct repo_config_values`.
>> - * It checks `repo->initialized` to prevent calling repo_config_values()`
>> - * before the repository setup is fully complete or in non-git environments.
>> - */
>>   int repo_ignore_case(struct repository *repo);
>>   
>>   int repo_trust_executable_bit(struct repository *repo);

Thanks! yuchen

[PATCH 3/3] environment: reorder variables in repo_config_values structure

[Tian Yuchen]

Reorder the fields in struct repo_config_values and its initialization
function to follow the order of configuration sections.

Keeping the declaration and initialization order aligned makes the
structure easier to review and maintain.

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 | 20 +++++++++++++-------
 2 files changed, 34 insertions(+), 17 deletions(-)

diff --git a/environment.c b/environment.c
index f5628b6758..918d8b50b8 100644
--- a/environment.c
+++ b/environment.c
@@ -745,31 +745,42 @@ int git_default_config(const char *var, const char *value,
 
 void repo_config_values_init(struct repo_config_values *cfg)
 {
+	/* core */
 	cfg->attributes_file = NULL;
 	cfg->excludes_file = NULL;
 	cfg->editor_program = NULL;
 	cfg->pager_program = NULL;
 	cfg->askpass_program = NULL;
-	cfg->apply_default_whitespace = NULL;
-	cfg->apply_default_ignorewhitespace = NULL;
-	cfg->push_default = PUSH_DEFAULT_UNSPECIFIED;
-	cfg->autorebase = AUTOREBASE_NEVER;
 	cfg->object_creation_mode = OBJECT_CREATION_MODE;
 	cfg->apply_sparse_checkout = 0;
+	cfg->trust_ctime = 1;
+	cfg->check_stat = 1;
+	cfg->zlib_compression_level = Z_BEST_SPEED;
+	cfg->precomposed_unicode = -1;
+	cfg->core_sparse_checkout_cone = 0;
+	cfg->warn_on_object_refname_ambiguity = 1;
 	cfg->protect_hfs = PROTECT_HFS_DEFAULT;
 	cfg->protect_ntfs = PROTECT_NTFS_DEFAULT;
 	cfg->ignore_case = 0;
 	cfg->trust_executable_bit = 1;
 	cfg->has_symlinks = platform_has_symlinks();
+
+	/* apply */
+	cfg->apply_default_whitespace = NULL;
+	cfg->apply_default_ignorewhitespace = NULL;
+
+	/* branch */
+	cfg->autorebase = AUTOREBASE_NEVER;
 	cfg->branch_track = BRANCH_TRACK_REMOTE;
-	cfg->trust_ctime = 1;
-	cfg->check_stat = 1;
-	cfg->zlib_compression_level = Z_BEST_SPEED;
+
+	/* pack */
 	cfg->pack_compression_level = Z_DEFAULT_COMPRESSION;
-	cfg->precomposed_unicode = -1; /* see probe_utf8_pathname_composition() */
-	cfg->core_sparse_checkout_cone = 0;
+
+	/* push */
+	cfg->push_default = PUSH_DEFAULT_UNSPECIFIED;
+
+	/* sparse */
 	cfg->sparse_expect_files_outside_of_patterns = 0;
-	cfg->warn_on_object_refname_ambiguity = 1;
 }
 
 void repo_config_values_clear(struct repo_config_values *cfg)
diff --git a/environment.h b/environment.h
index 30678257b5..52ed13c0fc 100644
--- a/environment.h
+++ b/environment.h
@@ -121,16 +121,11 @@ struct repo_config_values {
 	char *editor_program;
 	char *pager_program;
 	char *askpass_program;
-	char *apply_default_whitespace;
-	char *apply_default_ignorewhitespace;
-	enum push_default_type push_default;
-	enum rebase_setup_type autorebase;
 	enum object_creation_mode object_creation_mode;
 	int apply_sparse_checkout;
 	int trust_ctime;
 	int check_stat;
 	int zlib_compression_level;
-	int pack_compression_level;
 	int precomposed_unicode;
 	int core_sparse_checkout_cone;
 	int warn_on_object_refname_ambiguity;
@@ -140,11 +135,22 @@ struct repo_config_values {
 	int trust_executable_bit;
 	int has_symlinks;
 
-	/* section "sparse" config values */
-	int sparse_expect_files_outside_of_patterns;
+	/* section "apply" config values */
+	char *apply_default_whitespace;
+	char *apply_default_ignorewhitespace;
 
 	/* section "branch" config values */
+	enum rebase_setup_type autorebase;
 	enum branch_track branch_track;
+
+	/* section "pack" config values */
+	int pack_compression_level;
+
+	/* section "push" config values */
+	enum push_default_type push_default;
+
+	/* section "sparse" config values */
+	int sparse_expect_files_outside_of_patterns;
 };
 
 struct repo_config_values *repo_config_values(struct repository *repo);
-- 
2.43.0

Re: [PATCH 3/3] environment: reorder variables in repo_config_values structure

[Junio C Hamano]

Tian Yuchen <cat@malon.dev> writes:

> Reorder the fields in struct repo_config_values and its initialization
> function to follow the order of configuration sections.
>
> Keeping the declaration and initialization order aligned makes the
> structure easier to review and maintain.

Really?

Do you have some automated tool to make sure these initialization
assignments in the environment.c file and declaration in the
environment.h file match the order in Documentation/config/*.adoc or
something else?  Have you designated some list as the authoritative
source of truth to check these against?  Without such a list to
check the code against and a mechanism to enforce the ordering, I
find it hard to agree with such a claim that this makes it easier to
maintain.

It is typical to list the structure members in the order of stricter
to looser alignment requirement of their types.  I do not know how
strictly it is followed for "struct repo_config_values", but by
spreading pointer valued members more widely with smaller enums in
between, the change certainly is making the overall structure size
larger by requiring more padding between the members with different
alignment requirements.  Not that we would have 100s of instances of
these structures.

> 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 | 20 +++++++++++++-------
>  2 files changed, 34 insertions(+), 17 deletions(-)
>
> diff --git a/environment.c b/environment.c
> index f5628b6758..918d8b50b8 100644
> --- a/environment.c
> +++ b/environment.c
> @@ -745,31 +745,42 @@ int git_default_config(const char *var, const char *value,
>  
>  void repo_config_values_init(struct repo_config_values *cfg)
>  {
> +	/* core */
>  	cfg->attributes_file = NULL;
>  	cfg->excludes_file = NULL;
>  	cfg->editor_program = NULL;
>  	cfg->pager_program = NULL;
>  	cfg->askpass_program = NULL;
> -	cfg->apply_default_whitespace = NULL;
> -	cfg->apply_default_ignorewhitespace = NULL;
> -	cfg->push_default = PUSH_DEFAULT_UNSPECIFIED;
> -	cfg->autorebase = AUTOREBASE_NEVER;
>  	cfg->object_creation_mode = OBJECT_CREATION_MODE;
>  	cfg->apply_sparse_checkout = 0;
> +	cfg->trust_ctime = 1;
> +	cfg->check_stat = 1;
> +	cfg->zlib_compression_level = Z_BEST_SPEED;
> +	cfg->precomposed_unicode = -1;
> +	cfg->core_sparse_checkout_cone = 0;
> +	cfg->warn_on_object_refname_ambiguity = 1;
>  	cfg->protect_hfs = PROTECT_HFS_DEFAULT;
>  	cfg->protect_ntfs = PROTECT_NTFS_DEFAULT;
>  	cfg->ignore_case = 0;
>  	cfg->trust_executable_bit = 1;
>  	cfg->has_symlinks = platform_has_symlinks();
> +
> +	/* apply */
> +	cfg->apply_default_whitespace = NULL;
> +	cfg->apply_default_ignorewhitespace = NULL;
> +
> +	/* branch */
> +	cfg->autorebase = AUTOREBASE_NEVER;
>  	cfg->branch_track = BRANCH_TRACK_REMOTE;
> -	cfg->trust_ctime = 1;
> -	cfg->check_stat = 1;
> -	cfg->zlib_compression_level = Z_BEST_SPEED;
> +
> +	/* pack */
>  	cfg->pack_compression_level = Z_DEFAULT_COMPRESSION;
> -	cfg->precomposed_unicode = -1; /* see probe_utf8_pathname_composition() */
> -	cfg->core_sparse_checkout_cone = 0;
> +
> +	/* push */
> +	cfg->push_default = PUSH_DEFAULT_UNSPECIFIED;
> +
> +	/* sparse */
>  	cfg->sparse_expect_files_outside_of_patterns = 0;
> -	cfg->warn_on_object_refname_ambiguity = 1;
>  }
>  
>  void repo_config_values_clear(struct repo_config_values *cfg)
> diff --git a/environment.h b/environment.h
> index 30678257b5..52ed13c0fc 100644
> --- a/environment.h
> +++ b/environment.h
> @@ -121,16 +121,11 @@ struct repo_config_values {
>  	char *editor_program;
>  	char *pager_program;
>  	char *askpass_program;
> -	char *apply_default_whitespace;
> -	char *apply_default_ignorewhitespace;
> -	enum push_default_type push_default;
> -	enum rebase_setup_type autorebase;
>  	enum object_creation_mode object_creation_mode;
>  	int apply_sparse_checkout;
>  	int trust_ctime;
>  	int check_stat;
>  	int zlib_compression_level;
> -	int pack_compression_level;
>  	int precomposed_unicode;
>  	int core_sparse_checkout_cone;
>  	int warn_on_object_refname_ambiguity;
> @@ -140,11 +135,22 @@ struct repo_config_values {
>  	int trust_executable_bit;
>  	int has_symlinks;
>  
> -	/* section "sparse" config values */
> -	int sparse_expect_files_outside_of_patterns;
> +	/* section "apply" config values */
> +	char *apply_default_whitespace;
> +	char *apply_default_ignorewhitespace;
>  
>  	/* section "branch" config values */
> +	enum rebase_setup_type autorebase;
>  	enum branch_track branch_track;
> +
> +	/* section "pack" config values */
> +	int pack_compression_level;
> +
> +	/* section "push" config values */
> +	enum push_default_type push_default;
> +
> +	/* section "sparse" config values */
> +	int sparse_expect_files_outside_of_patterns;
>  };
>  
>  struct repo_config_values *repo_config_values(struct repository *repo);

Re: [PATCH 3/3] environment: reorder variables in repo_config_values structure

[Tian Yuchen]

On 8/6/26 05:47, Junio C Hamano wrote:
> Tian Yuchen <cat@malon.dev> writes:
> 
>> Reorder the fields in struct repo_config_values and its initialization
>> function to follow the order of configuration sections.
>>
>> Keeping the declaration and initialization order aligned makes the
>> structure easier to review and maintain.
> 
> Really?
> 
> Do you have some automated tool to make sure these initialization
> assignments in the environment.c file and declaration in the
> environment.h file match the order in Documentation/config/*.adoc or
> something else?  Have you designated some list as the authoritative
> source of truth to check these against?  Without such a list to
> check the code against and a mechanism to enforce the ordering, I
> find it hard to agree with such a claim that this makes it easier to
> maintain.

I see.

> 
> It is typical to list the structure members in the order of stricter
> to looser alignment requirement of their types.  I do not know how
> strictly it is followed for "struct repo_config_values", but by
> spreading pointer valued members more widely with smaller enums in
> between, the change certainly is making the overall structure size
> larger by requiring more padding between the members with different
> alignment requirements.  Not that we would have 100s of instances of
> these structures.
> 

Oh, I overlooked the size issue. Thanks for pointing out.


I think I will drop this commit. However, the original comments:

struct repo_config_values {
	/* section "core" config values */
	char *attributes_file;
	char *excludes_file;
	char *editor_program;
	char *pager_program;
	char *askpass_program;
	char *apply_default_whitespace;
	char *apply_default_ignorewhitespace;
	enum push_default_type push_default;
	enum rebase_setup_type autorebase;
	enum object_creation_mode object_creation_mode;
	int apply_sparse_checkout;
	int trust_ctime;
	int check_stat;
	int zlib_compression_level;
	int pack_compression_level;
	int precomposed_unicode;
	int core_sparse_checkout_cone;
	int warn_on_object_refname_ambiguity;
	int protect_hfs;
	int protect_ntfs;
	int ignore_case;
	int trust_executable_bit;
	int has_symlinks;

	/* section "sparse" config values */
	int sparse_expect_files_outside_of_patterns;

	/* section "branch" config values */
	enum branch_track branch_track;
};

still do not accurately reflect the grouping of the members, right? Can 
we remove them directly instead?


Thanks, yuchen

>> 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 | 20 +++++++++++++-------
>>   2 files changed, 34 insertions(+), 17 deletions(-)
>>
>> diff --git a/environment.c b/environment.c
>> index f5628b6758..918d8b50b8 100644
>> --- a/environment.c
>> +++ b/environment.c
>> @@ -745,31 +745,42 @@ int git_default_config(const char *var, const char *value,
>>   
>>   void repo_config_values_init(struct repo_config_values *cfg)
>>   {
>> +	/* core */
>>   	cfg->attributes_file = NULL;
>>   	cfg->excludes_file = NULL;
>>   	cfg->editor_program = NULL;
>>   	cfg->pager_program = NULL;
>>   	cfg->askpass_program = NULL;
>> -	cfg->apply_default_whitespace = NULL;
>> -	cfg->apply_default_ignorewhitespace = NULL;
>> -	cfg->push_default = PUSH_DEFAULT_UNSPECIFIED;
>> -	cfg->autorebase = AUTOREBASE_NEVER;
>>   	cfg->object_creation_mode = OBJECT_CREATION_MODE;
>>   	cfg->apply_sparse_checkout = 0;
>> +	cfg->trust_ctime = 1;
>> +	cfg->check_stat = 1;
>> +	cfg->zlib_compression_level = Z_BEST_SPEED;
>> +	cfg->precomposed_unicode = -1;
>> +	cfg->core_sparse_checkout_cone = 0;
>> +	cfg->warn_on_object_refname_ambiguity = 1;
>>   	cfg->protect_hfs = PROTECT_HFS_DEFAULT;
>>   	cfg->protect_ntfs = PROTECT_NTFS_DEFAULT;
>>   	cfg->ignore_case = 0;
>>   	cfg->trust_executable_bit = 1;
>>   	cfg->has_symlinks = platform_has_symlinks();
>> +
>> +	/* apply */
>> +	cfg->apply_default_whitespace = NULL;
>> +	cfg->apply_default_ignorewhitespace = NULL;
>> +
>> +	/* branch */
>> +	cfg->autorebase = AUTOREBASE_NEVER;
>>   	cfg->branch_track = BRANCH_TRACK_REMOTE;
>> -	cfg->trust_ctime = 1;
>> -	cfg->check_stat = 1;
>> -	cfg->zlib_compression_level = Z_BEST_SPEED;
>> +
>> +	/* pack */
>>   	cfg->pack_compression_level = Z_DEFAULT_COMPRESSION;
>> -	cfg->precomposed_unicode = -1; /* see probe_utf8_pathname_composition() */
>> -	cfg->core_sparse_checkout_cone = 0;
>> +
>> +	/* push */
>> +	cfg->push_default = PUSH_DEFAULT_UNSPECIFIED;
>> +
>> +	/* sparse */
>>   	cfg->sparse_expect_files_outside_of_patterns = 0;
>> -	cfg->warn_on_object_refname_ambiguity = 1;
>>   }
>>   
>>   void repo_config_values_clear(struct repo_config_values *cfg)
>> diff --git a/environment.h b/environment.h
>> index 30678257b5..52ed13c0fc 100644
>> --- a/environment.h
>> +++ b/environment.h
>> @@ -121,16 +121,11 @@ struct repo_config_values {
>>   	char *editor_program;
>>   	char *pager_program;
>>   	char *askpass_program;
>> -	char *apply_default_whitespace;
>> -	char *apply_default_ignorewhitespace;
>> -	enum push_default_type push_default;
>> -	enum rebase_setup_type autorebase;
>>   	enum object_creation_mode object_creation_mode;
>>   	int apply_sparse_checkout;
>>   	int trust_ctime;
>>   	int check_stat;
>>   	int zlib_compression_level;
>> -	int pack_compression_level;
>>   	int precomposed_unicode;
>>   	int core_sparse_checkout_cone;
>>   	int warn_on_object_refname_ambiguity;
>> @@ -140,11 +135,22 @@ struct repo_config_values {
>>   	int trust_executable_bit;
>>   	int has_symlinks;
>>   
>> -	/* section "sparse" config values */
>> -	int sparse_expect_files_outside_of_patterns;
>> +	/* section "apply" config values */
>> +	char *apply_default_whitespace;
>> +	char *apply_default_ignorewhitespace;
>>   
>>   	/* section "branch" config values */
>> +	enum rebase_setup_type autorebase;
>>   	enum branch_track branch_track;
>> +
>> +	/* section "pack" config values */
>> +	int pack_compression_level;
>> +
>> +	/* section "push" config values */
>> +	enum push_default_type push_default;
>> +
>> +	/* section "sparse" config values */
>> +	int sparse_expect_files_outside_of_patterns;
>>   };
>>   
>>   struct repo_config_values *repo_config_values(struct repository *repo);

Re: [PATCH 3/3] environment: reorder variables in repo_config_values structure

[Junio C Hamano]

Tian Yuchen <cat@malon.dev> writes:

> On 8/6/26 05:47, Junio C Hamano wrote:
>> Tian Yuchen <cat@malon.dev> writes:
>> 
>>> Reorder the fields in struct repo_config_values and its initialization
>>> function to follow the order of configuration sections.
>>>
>>> Keeping the declaration and initialization order aligned makes the
>>> structure easier to review and maintain.
>> 
>> Really?
>> 
>> Do you have some automated tool to make sure these initialization
>> assignments in the environment.c file and declaration in the
>> environment.h file match the order in Documentation/config/*.adoc or
>> something else?  Have you designated some list as the authoritative
>> source of truth to check these against?  Without such a list to
>> check the code against and a mechanism to enforce the ordering, I
>> find it hard to agree with such a claim that this makes it easier to
>> maintain.
>
> I see.
>
>> 
>> It is typical to list the structure members in the order of stricter
>> to looser alignment requirement of their types.  I do not know how
>> strictly it is followed for "struct repo_config_values", but by
>> spreading pointer valued members more widely with smaller enums in
>> between, the change certainly is making the overall structure size
>> larger by requiring more padding between the members with different
>> alignment requirements.  Not that we would have 100s of instances of
>> these structures.
>> 
>
> Oh, I overlooked the size issue. Thanks for pointing out.

I didn't mean to "point out" any size issue.  As I said, it is not
like we have hundreds of these, so padding bloat here and there
would not matter and if we get a readability boost by reordering
into a sensible order, that by itself could be a win.

Re: [PATCH 3/3] environment: reorder variables in repo_config_values structure

[Tian Yuchen]

On 8/7/26 00:42, Junio C Hamano wrote:
> Tian Yuchen <cat@malon.dev> writes:
> 
>> On 8/6/26 05:47, Junio C Hamano wrote:
>>> Tian Yuchen <cat@malon.dev> writes:
>>>
>>>> Reorder the fields in struct repo_config_values and its initialization
>>>> function to follow the order of configuration sections.
>>>>
>>>> Keeping the declaration and initialization order aligned makes the
>>>> structure easier to review and maintain.
>>>
>>> Really?
>>>
>>> Do you have some automated tool to make sure these initialization
>>> assignments in the environment.c file and declaration in the
>>> environment.h file match the order in Documentation/config/*.adoc or
>>> something else?  Have you designated some list as the authoritative
>>> source of truth to check these against?  Without such a list to
>>> check the code against and a mechanism to enforce the ordering, I
>>> find it hard to agree with such a claim that this makes it easier to
>>> maintain.
>>
>> I see.
>>
>>>
>>> It is typical to list the structure members in the order of stricter
>>> to looser alignment requirement of their types.  I do not know how
>>> strictly it is followed for "struct repo_config_values", but by
>>> spreading pointer valued members more widely with smaller enums in
>>> between, the change certainly is making the overall structure size
>>> larger by requiring more padding between the members with different
>>> alignment requirements.  Not that we would have 100s of instances of
>>> these structures.
>>>
>>
>> Oh, I overlooked the size issue. Thanks for pointing out.
> 
> I didn't mean to "point out" any size issue.  As I said, it is not
> like we have hundreds of these, so padding bloat here and there
> would not matter and if we get a readability boost by reordering
> into a sensible order, that by itself could be a win.
> 

Okay.

As you said before, the boost on readability seems to be limited. 
Reordering by config section is not a strong maintenance rule without an 
authoritative source. So let's don't reorder them anyways.

Thanks, yuchen

[PATCH v2 0/3] environment: clean up repository config handling

[Tian Yuchen]

Hi all,

This series contains several cleanup patches for repository configuration
handling.

No functional changes are intended. The patches make the related code
more consistent and easier to maintain by improving documentation,
formatting, and the organization of repo_config_values.

RFC:
If there are other small cleanups in this area that would be useful to
include, suggestions are welcome.

Regards, yuchen

Changes since v1:

 - in commit 2/3, drop several blank lines to group the getters under the
 comment. Note that the comment does not apply to repo_excludes_file.

 - in commit 3/3, do not change the order of the members. Instead, drop
 the original comments that do not accurately categorize them.

Tian Yuchen (3):
  environment: simplify repository config getters
  environment: clarify repository config getter documentation
  environment: remove inaccurate repo_config_values comments

 environment.c | 18 +++++++++---------
 environment.h | 19 +++----------------
 2 files changed, 12 insertions(+), 25 deletions(-)

-- 
2.43.0

[PATCH v2 1/3] environment: simplify repository config getters

[Tian Yuchen]

Drop unnecessary parentheses and NULL checks in repository config
getters.

These getters are only used with non-NULL repositories, so the
extra checks do not match their current callers.

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 | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/environment.c b/environment.c
index 76ee65e62b..f5628b6758 100644
--- a/environment.c
+++ b/environment.c
@@ -119,23 +119,23 @@ int is_bare_repository(struct repository *repo)
 
 int repo_protect_ntfs(struct repository *repo)
 {
-	return (repo && repo->initialized) ?
-		repo_config_values(repo)->protect_ntfs :
-		PROTECT_NTFS_DEFAULT;
+	return repo->initialized
+		? repo_config_values(repo)->protect_ntfs
+		: PROTECT_NTFS_DEFAULT;
 }
 
 int repo_protect_hfs(struct repository *repo)
 {
-	return (repo && repo->initialized) ?
-		repo_config_values(repo)->protect_hfs :
-		PROTECT_HFS_DEFAULT;
+	return repo->initialized
+		? repo_config_values(repo)->protect_hfs
+		: PROTECT_HFS_DEFAULT;
 }
 
 int repo_ignore_case(struct repository *repo)
 {
-	return (repo && repo->initialized) ?
-		repo_config_values(repo)->ignore_case :
-		0;
+	return repo->initialized
+		? repo_config_values(repo)->ignore_case
+		: 0;
 }
 
 int repo_trust_executable_bit(struct repository *repo)
-- 
2.43.0

Re: [PATCH v2 1/3] environment: simplify repository config getters

[Junio C Hamano]

Tian Yuchen <cat@malon.dev> writes:

> Drop unnecessary parentheses and NULL checks in repository config
> getters.
>
> These getters are only used with non-NULL repositories, so the
> extra checks do not match their current callers.

You would need to explain why it is sensible to enforce on future
callers the same rule that current callers honor, or why it is
unlikely that we will gain any more callers in the future (which
would justify catering only to current callers).

> 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 | 18 +++++++++---------
>  1 file changed, 9 insertions(+), 9 deletions(-)
>
> diff --git a/environment.c b/environment.c
> index 76ee65e62b..f5628b6758 100644
> --- a/environment.c
> +++ b/environment.c
> @@ -119,23 +119,23 @@ int is_bare_repository(struct repository *repo)
>  
>  int repo_protect_ntfs(struct repository *repo)
>  {
> -	return (repo && repo->initialized) ?
> -		repo_config_values(repo)->protect_ntfs :
> -		PROTECT_NTFS_DEFAULT;
> +	return repo->initialized
> +		? repo_config_values(repo)->protect_ntfs
> +		: PROTECT_NTFS_DEFAULT;
>  }
>  
>  int repo_protect_hfs(struct repository *repo)
>  {
> -	return (repo && repo->initialized) ?
> -		repo_config_values(repo)->protect_hfs :
> -		PROTECT_HFS_DEFAULT;
> +	return repo->initialized
> +		? repo_config_values(repo)->protect_hfs
> +		: PROTECT_HFS_DEFAULT;
>  }
>  
>  int repo_ignore_case(struct repository *repo)
>  {
> -	return (repo && repo->initialized) ?
> -		repo_config_values(repo)->ignore_case :
> -		0;
> +	return repo->initialized
> +		? repo_config_values(repo)->ignore_case
> +		: 0;
>  }
>  
>  int repo_trust_executable_bit(struct repository *repo)

Re: [PATCH v2 1/3] environment: simplify repository config getters

[Tian Yuchen]

On 8/7/26 00:50, Junio C Hamano wrote:
> Tian Yuchen <cat@malon.dev> writes:
> 
>> Drop unnecessary parentheses and NULL checks in repository config
>> getters.
>>
>> These getters are only used with non-NULL repositories, so the
>> extra checks do not match their current callers.
> 
> You would need to explain why it is sensible to enforce on future
> callers the same rule that current callers honor, or why it is
> unlikely that we will gain any more callers in the future (which
> would justify catering only to current callers).
> 
>> 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 | 18 +++++++++---------
>>   1 file changed, 9 insertions(+), 9 deletions(-)
>>
>> diff --git a/environment.c b/environment.c
>> index 76ee65e62b..f5628b6758 100644
>> --- a/environment.c
>> +++ b/environment.c
>> @@ -119,23 +119,23 @@ int is_bare_repository(struct repository *repo)
>>   
>>   int repo_protect_ntfs(struct repository *repo)
>>   {
>> -	return (repo && repo->initialized) ?
>> -		repo_config_values(repo)->protect_ntfs :
>> -		PROTECT_NTFS_DEFAULT;
>> +	return repo->initialized
>> +		? repo_config_values(repo)->protect_ntfs
>> +		: PROTECT_NTFS_DEFAULT;
>>   }
>>   
>>   int repo_protect_hfs(struct repository *repo)
>>   {
>> -	return (repo && repo->initialized) ?
>> -		repo_config_values(repo)->protect_hfs :
>> -		PROTECT_HFS_DEFAULT;
>> +	return repo->initialized
>> +		? repo_config_values(repo)->protect_hfs
>> +		: PROTECT_HFS_DEFAULT;
>>   }
>>   
>>   int repo_ignore_case(struct repository *repo)
>>   {
>> -	return (repo && repo->initialized) ?
>> -		repo_config_values(repo)->ignore_case :
>> -		0;
>> +	return repo->initialized
>> +		? repo_config_values(repo)->ignore_case
>> +		: 0;
>>   }
>>   
>>   int repo_trust_executable_bit(struct repository *repo)

I see, will change the commit message then ;)

Thanks, yuchen

[PATCH v2 2/3] environment: clarify repository config getter documentation

[Tian Yuchen]

Update the comment above repository config getters to describe their
common behavior.

The getters handle repositories that are not fully initialized by
returning the corresponding default values.

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.h | 14 +++-----------
 1 file changed, 3 insertions(+), 11 deletions(-)

diff --git a/environment.h b/environment.h
index e7ec5b0437..1a58b553b5 100644
--- a/environment.h
+++ b/environment.h
@@ -175,22 +175,14 @@ int git_default_core_config(const char *var, const char *value,
 			    const struct config_context *ctx, void *cb);
 
 /*
- * Getters for the `protect_hfs` and `protect_ntfs` fields of `struct repo_config_values`.
- * They check `repo->initialized` to prevent calling `repo_config_values()`
- * before the repository setup is fully complete or in non-git environments.
+ * Getters for configuration variables in `struct repo_config_values`.
+ * These functions handle repositories that are not fully initialized
+ * by returning appropriate default values.
  */
 int repo_protect_hfs(struct repository *repo);
 int repo_protect_ntfs(struct repository *repo);
-
-/*
- * Getter for the `ignore_case` field of `struct repo_config_values`.
- * It checks `repo->initialized` to prevent calling repo_config_values()`
- * before the repository setup is fully complete or in non-git environments.
- */
 int repo_ignore_case(struct repository *repo);
-
 int repo_trust_executable_bit(struct repository *repo);
-
 int repo_has_symlinks(struct repository *repo);
 
 const char *repo_excludes_file(struct repository *repo);
-- 
2.43.0

Re: [PATCH v2 2/3] environment: clarify repository config getter documentation

[Junio C Hamano]

Tian Yuchen <cat@malon.dev> writes:

> Update the comment above repository config getters to describe their
> common behavior.
>
> The getters handle repositories that are not fully initialized by
> returning the corresponding default values.
>
> 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.h | 14 +++-----------
>  1 file changed, 3 insertions(+), 11 deletions(-)
>
> diff --git a/environment.h b/environment.h
> index e7ec5b0437..1a58b553b5 100644
> --- a/environment.h
> +++ b/environment.h
> @@ -175,22 +175,14 @@ int git_default_core_config(const char *var, const char *value,
>  			    const struct config_context *ctx, void *cb);
>  
>  /*
> - * Getters for the `protect_hfs` and `protect_ntfs` fields of `struct repo_config_values`.
> - * They check `repo->initialized` to prevent calling `repo_config_values()`
> - * before the repository setup is fully complete or in non-git environments.
> + * Getters for configuration variables in `struct repo_config_values`.
> + * These functions handle repositories that are not fully initialized
> + * by returning appropriate default values.
>   */

Do we also want to mention that calling them when the caller is
outside a repository is an error, or is it obvious enough?

>  int repo_protect_hfs(struct repository *repo);
>  int repo_protect_ntfs(struct repository *repo);
> -
> -/*
> - * Getter for the `ignore_case` field of `struct repo_config_values`.
> - * It checks `repo->initialized` to prevent calling repo_config_values()`
> - * before the repository setup is fully complete or in non-git environments.
> - */
>  int repo_ignore_case(struct repository *repo);
> -
>  int repo_trust_executable_bit(struct repository *repo);
> -
>  int repo_has_symlinks(struct repository *repo);
>  
>  const char *repo_excludes_file(struct repository *repo);

[PATCH v2 3/3] environment: remove inaccurate repo_config_values comments

[Tian Yuchen]

The section comments in struct repo_config_values do not accurately
describe all members grouped under them. Remove them rather than implying
a relationship that does not exist.

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.h | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/environment.h b/environment.h
index 1a58b553b5..ab52330159 100644
--- a/environment.h
+++ b/environment.h
@@ -115,7 +115,6 @@ enum object_creation_mode {
 };
 
 struct repo_config_values {
-	/* section "core" config values */
 	char *attributes_file;
 	char *excludes_file;
 	char *editor_program;
@@ -139,11 +138,7 @@ struct repo_config_values {
 	int ignore_case;
 	int trust_executable_bit;
 	int has_symlinks;
-
-	/* section "sparse" config values */
 	int sparse_expect_files_outside_of_patterns;
-
-	/* section "branch" config values */
 	enum branch_track branch_track;
 };
 
-- 
2.43.0

[PATCH v3 0/3] environment: clean up repository config handling

[Tian Yuchen]

Hi all,

This series contains several cleanup patches for repository configuration
handling.

No functional changes are intended. The patches make the related code
more consistent and easier to maintain by improving documentation,
formatting, and the organization of repo_config_values.

RFC:
If there are other small cleanups in this area that would be useful to
include, suggestions are welcome.

Regards, yuchen

Changes since v2:

 - in the commit message of patch 1/3, explain why NULL repository is
 not allowed.

 - in patch 2/3, mention in the comment that NULL repository shouldn't
 be passed in.

Tian Yuchen (3):
  environment: drop redundant NULL checks in config getters
  environment: clarify repository config getter documentation
  environment: remove inaccurate repo_config_values comments

 environment.c | 18 +++++++++---------
 environment.h | 20 ++++----------------
 2 files changed, 13 insertions(+), 25 deletions(-)

-- 
2.43.0

[PATCH v3 1/3] environment: drop redundant NULL checks in config getters

[Tian Yuchen]

These repository config getters require a valid repository pointer.
While an uninitialized repository is a valid state and is handled by
returning default values, passing NULL is a programming error.

Drop the NULL checks so that invalid callers are not silently accepted.

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 | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/environment.c b/environment.c
index 76ee65e62b..f5628b6758 100644
--- a/environment.c
+++ b/environment.c
@@ -119,23 +119,23 @@ int is_bare_repository(struct repository *repo)
 
 int repo_protect_ntfs(struct repository *repo)
 {
-	return (repo && repo->initialized) ?
-		repo_config_values(repo)->protect_ntfs :
-		PROTECT_NTFS_DEFAULT;
+	return repo->initialized
+		? repo_config_values(repo)->protect_ntfs
+		: PROTECT_NTFS_DEFAULT;
 }
 
 int repo_protect_hfs(struct repository *repo)
 {
-	return (repo && repo->initialized) ?
-		repo_config_values(repo)->protect_hfs :
-		PROTECT_HFS_DEFAULT;
+	return repo->initialized
+		? repo_config_values(repo)->protect_hfs
+		: PROTECT_HFS_DEFAULT;
 }
 
 int repo_ignore_case(struct repository *repo)
 {
-	return (repo && repo->initialized) ?
-		repo_config_values(repo)->ignore_case :
-		0;
+	return repo->initialized
+		? repo_config_values(repo)->ignore_case
+		: 0;
 }
 
 int repo_trust_executable_bit(struct repository *repo)
-- 
2.43.0

[PATCH v3 2/3] environment: clarify repository config getter documentation

[Tian Yuchen]

Update the comment above repository config getters to describe their
common behavior.

The getters handle repositories that are not fully initialized by
returning the corresponding default values.

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.h | 15 ++++-----------
 1 file changed, 4 insertions(+), 11 deletions(-)

diff --git a/environment.h b/environment.h
index e7ec5b0437..6f864c1635 100644
--- a/environment.h
+++ b/environment.h
@@ -175,22 +175,15 @@ int git_default_core_config(const char *var, const char *value,
 			    const struct config_context *ctx, void *cb);
 
 /*
- * Getters for the `protect_hfs` and `protect_ntfs` fields of `struct repo_config_values`.
- * They check `repo->initialized` to prevent calling `repo_config_values()`
- * before the repository setup is fully complete or in non-git environments.
+ * Getters for configuration variables in `struct repo_config_values`.
+ * These functions require a non-NULL repository pointer and handle
+ * repositories that are not fully initialized by returning appropriate
+ * default values.
  */
 int repo_protect_hfs(struct repository *repo);
 int repo_protect_ntfs(struct repository *repo);
-
-/*
- * Getter for the `ignore_case` field of `struct repo_config_values`.
- * It checks `repo->initialized` to prevent calling repo_config_values()`
- * before the repository setup is fully complete or in non-git environments.
- */
 int repo_ignore_case(struct repository *repo);
-
 int repo_trust_executable_bit(struct repository *repo);
-
 int repo_has_symlinks(struct repository *repo);
 
 const char *repo_excludes_file(struct repository *repo);
-- 
2.43.0

[PATCH v3 3/3] environment: remove inaccurate repo_config_values comments

[Tian Yuchen]

The section comments in struct repo_config_values do not accurately
describe all members grouped under them. Remove them rather than implying
a relationship that does not exist.

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.h | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/environment.h b/environment.h
index 6f864c1635..67fd387d35 100644
--- a/environment.h
+++ b/environment.h
@@ -115,7 +115,6 @@ enum object_creation_mode {
 };
 
 struct repo_config_values {
-	/* section "core" config values */
 	char *attributes_file;
 	char *excludes_file;
 	char *editor_program;
@@ -139,11 +138,7 @@ struct repo_config_values {
 	int ignore_case;
 	int trust_executable_bit;
 	int has_symlinks;
-
-	/* section "sparse" config values */
 	int sparse_expect_files_outside_of_patterns;
-
-	/* section "branch" config values */
 	enum branch_track branch_track;
 };
 
-- 
2.43.0

Re: [PATCH v3 0/3] environment: clean up repository config handling

[Patrick Steinhardt]

On Fri, Aug 07, 2026 at 04:59:29PM +0800, Tian Yuchen wrote:
> Hi all,
> 
> This series contains several cleanup patches for repository configuration
> handling.
> 
> No functional changes are intended. The patches make the related code
> more consistent and easier to maintain by improving documentation,
> formatting, and the organization of repo_config_values.
> 
> RFC:
> If there are other small cleanups in this area that would be useful to
> include, suggestions are welcome.

Somewhat unrelated to this patch series, but I was wondering whether you
plan to drop the limitation in `repo_config_values()` that requires that
the passed-in repository is `the_repository`. This limitation is
starting to create problems as more and more of our infrastructure is
migrating into `struct repo_config_values`, so using a different repo
than `the_repository` is starting to become harder and harder in our
codebase.

Thanks!

Patrick

Re: [PATCH v3 0/3] environment: clean up repository config handling

[Junio C Hamano]

Patrick Steinhardt <ps@pks.im> writes:

> On Fri, Aug 07, 2026 at 04:59:29PM +0800, Tian Yuchen wrote:
>> Hi all,
>> 
>> This series contains several cleanup patches for repository configuration
>> handling.
>> 
>> No functional changes are intended. The patches make the related code
>> more consistent and easier to maintain by improving documentation,
>> formatting, and the organization of repo_config_values.
>> 
>> RFC:
>> If there are other small cleanups in this area that would be useful to
>> include, suggestions are welcome.
>
> Somewhat unrelated to this patch series, but I was wondering whether you
> plan to drop the limitation in `repo_config_values()` that requires that
> the passed-in repository is `the_repository`. This limitation is
> starting to create problems as more and more of our infrastructure is
> migrating into `struct repo_config_values`, so using a different repo
> than `the_repository` is starting to become harder and harder in our
> codebase.
>
> Thanks!
>
> Patrick

Hmph, that is an interesting point.  What is our plan to really
enable the use of repository instances other than 'the_repository'
here?  They of course need to be initialized with repo_init(),
but is that enough to sensibly use the embedded 'repo_settings'
and 'repo_config_values' structures?  (By the way, it is not
entirely clear to me why we need both and how we sift variables
between them.)  Some code paths need to work outside a repository
and still need to know about per-user or per-system settings.
We were perfectly happy reading from global variables when we had
the majority of them there.  It is my understanding that they are
now found in 'repo_config_values' or 'repo_settings' associated
with 'the_repository', which I think is something we cannot
really avoid doing.  Unless we try to get rid of 'the_repository'
and instead have free-standing 'repo_settings' and
'repo_config_values' structures that are not tied to any
repository instance, we are back to depending on a set of global
variables. 😞

In any case, all of that has little to do with this series, I
suspect, unless we are redesigning these configurations and
settings in such a way that they are not necessarily tied to
any repository instance.  While I do not know the exact details,
I can imagine a hierarchical system where system- and
user-wide sets of setting values are known independently of any
repository, only to be overridden by per-repository settings
using a last-one-wins strategy at lookup time.

Re: [PATCH v3 0/3] environment: clean up repository config handling

[Patrick Steinhardt]

On Fri, Aug 07, 2026 at 02:11:08PM -0700, Junio C Hamano wrote:
> Patrick Steinhardt <ps@pks.im> writes:
> 
> > On Fri, Aug 07, 2026 at 04:59:29PM +0800, Tian Yuchen wrote:
> >> Hi all,
> >> 
> >> This series contains several cleanup patches for repository configuration
> >> handling.
> >> 
> >> No functional changes are intended. The patches make the related code
> >> more consistent and easier to maintain by improving documentation,
> >> formatting, and the organization of repo_config_values.
> >> 
> >> RFC:
> >> If there are other small cleanups in this area that would be useful to
> >> include, suggestions are welcome.
> >
> > Somewhat unrelated to this patch series, but I was wondering whether you
> > plan to drop the limitation in `repo_config_values()` that requires that
> > the passed-in repository is `the_repository`. This limitation is
> > starting to create problems as more and more of our infrastructure is
> > migrating into `struct repo_config_values`, so using a different repo
> > than `the_repository` is starting to become harder and harder in our
> > codebase.
> >
> > Thanks!
> >
> > Patrick
> 
> Hmph, that is an interesting point.  What is our plan to really
> enable the use of repository instances other than 'the_repository'
> here?  They of course need to be initialized with repo_init(),
> but is that enough to sensibly use the embedded 'repo_settings'
> and 'repo_config_values' structures?  (By the way, it is not
> entirely clear to me why we need both and how we sift variables
> between them.)

Yeah, this split is adding to the confusion indeed. I think that we
should make it a goal to unify those going forward.

[snip]
> In any case, all of that has little to do with this series, I
> suspect, unless we are redesigning these configurations and
> settings in such a way that they are not necessarily tied to
> any repository instance.  While I do not know the exact details,
> I can imagine a hierarchical system where system- and
> user-wide sets of setting values are known independently of any
> repository, only to be overridden by per-repository settings
> using a last-one-wins strategy at lookup time.

I've been wondering for a while whether we're operating at the wrong
level here. Both `repo_settings` and `repo_config_values` indicates that
we're operating in the context of a repository, but as you mention that
may not even be the case.

I don't think the approach is inherently flawed though. From my point
of view, the best way forward is to merge those two and then generalize
them into something like `git_config_values` or `git_settings`,
depending on which of both variants we want to retain. We would then
have two levels:

  - One on the repository level as we have it today.

  - One truly global variable, because that stuff in fact _is_ global.

We'd then adapt `repo_config_values()` so that it knows to populate
either of those variables depending on whether or not the user passes a
valid repository, and returns a constant pointer to the respective
structure. Callers MUST NOT modify that structure -- if they want to,
they'll have to make a copy and pass it down the calling stack.

The last part about not modifying that structure could be quite a bit
painful though, as it would mean that we might have to adapt call chains
to pass down a `struct git_config_values` instead of a `struct
repository`. But arguably, that's the right thing to do anyway for at
least some subsystems that are independent of repositories.

As you say though, none of this is really related to this patch series
at hand, and I don't think we need to resolve this discussion before we
can merge it. I just want to make sure that we have a plan for how to
get rid of `the_repository` instead of only shuffling stuff around.

Patrick