Re: [PATCH v2 8/8] config: make `packed_git_(limit|window_size)` non-global variables

> On Mon, Oct 28, 2024 at 02:43:46PM +0100, Karthik Nayak wrote:
>> The variables `packed_git_window_size` and `packed_git_limit` are global
>> config variables used in the `packfile.c` file. Since it is only used in
>> this file, let's change it from being a global config variable to a
>> local variable for the subsystem.
>>
>> We do this by introducing a new local `packfile_config` struct in
>> `packfile.c` and also adding the required function to parse the said
>> config. We then use this within `packfile.c` to obtain the variables.
>>
>> With this, we rid `packfile.c` from all global variable usage and this
>> means we can also remove the `USE_THE_REPOSITORY_VARIABLE` guard from
>> the file.
>
> Ahh. Now the motivation of the previous patch is clearer. Have you
> considered hinting at the motivation here in the previous commit message
> (e.g., "this gets us part of the way towards ...")?
>

>> diff --git a/environment.c b/environment.c
>> index 8e5022c282..8389a27270 100644
>> --- a/environment.c
>> +++ b/environment.c
>> @@ -49,8 +49,6 @@ int fsync_object_files = -1;
>>  int use_fsync = -1;
>>  enum fsync_method fsync_method = FSYNC_METHOD_DEFAULT;
>>  enum fsync_component fsync_components = FSYNC_COMPONENTS_DEFAULT;
>> -size_t packed_git_window_size = DEFAULT_PACKED_GIT_WINDOW_SIZE;
>> -size_t packed_git_limit = DEFAULT_PACKED_GIT_LIMIT;
>
> Very satisfying :-).
>
>> +struct packfile_config {
>> +	unsigned long packed_git_window_size;
>> +	unsigned long packed_git_limit;
>> +};
>> +
>> +#define PACKFILE_CONFIG_INIT \
>> +{ \
>> +	.packed_git_window_size = DEFAULT_PACKED_GIT_WINDOW_SIZE, \
>> +	.packed_git_limit = DEFAULT_PACKED_GIT_LIMIT,  \
>
> s/,  /, /
>
>> +static int packfile_config(const char *var, const char *value,
>> +			   const struct config_context *ctx, void *cb)
>>  {
>> +	struct packfile_config *config = cb;
>> +
>> +	if (!strcmp(var, "core.packedgitwindowsize")) {
>> +		int pgsz_x2 = getpagesize() * 2;
>> +		config->packed_git_window_size = git_config_ulong(var, value, ctx->kvi);
>> +
>> +		/* This value must be multiple of (pagesize * 2) */
>> +		config->packed_git_window_size /= pgsz_x2;
>> +		if (config->packed_git_window_size < 1)
>> +			config->packed_git_window_size = 1;
>> +		config->packed_git_window_size *= pgsz_x2;
>> +		return 0;
>> +	}
>> +
>> +	if (!strcmp(var, "core.packedgitlimit")) {
>> +		config->packed_git_limit = git_config_ulong(var, value, ctx->kvi);
>> +		return 0;
>> +	}
>> +
>> +	return git_default_config(var, value, ctx, cb);
>> +}
>
> I get that this was copy/pasted from elsewhere, but it would be nice to
> replace the "every if statement ends in 'return 0' to keep them mutually
> exclusive" with else if statements instead:
>
> --- 8< ---
> diff --git a/packfile.c b/packfile.c
> index cfbfcdc2b8..c8af29bf0a 100644
> --- a/packfile.c
> +++ b/packfile.c
> @@ -72,15 +72,11 @@ static int packfile_config(const char *var, const char *value,
>  		if (config->packed_git_window_size < 1)
>  			config->packed_git_window_size = 1;
>  		config->packed_git_window_size *= pgsz_x2;
> -		return 0;
> -	}
> -
> -	if (!strcmp(var, "core.packedgitlimit")) {
> +	} else if (!strcmp(var, "core.packedgitlimit")) {
>  		config->packed_git_limit = git_config_ulong(var, value, ctx->kvi);
> -		return 0;
> +	} else {
> +		return git_default_config(var, value, ctx, cb);
>  	}
> -
> -	return git_default_config(var, value, ctx, cb);
>  }
> --- >8 ---
>

>> +
>> +
>
> Extra newline here (after the definition of packfile_config())?
>