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

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.

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 <[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);