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

On 8/6/26 05:47, Junio C Hamano wrote:
> Tian Yuchen <[email protected]> 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 <[email protected]>
>> Mentored-by: Ayush Chandekar <[email protected]>
>> Mentored-by: Olamide Caleb Bello <[email protected]>
>> Signed-off-by: Tian Yuchen <[email protected]>
>> ---
>>   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);