Taylor Blau <me@xxxxxxxxxxxx> writes:
> 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 ...")?
>
Indeed, will add.
>> 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 ---
>
Thanks, will patch this in. I try and avoid such things to mostly make
it easier to review code block movements. But here I think it is indeed
nicer to change for the better.
>> +
>> +
>
> Extra newline here (after the definition of packfile_config())?
>
Oops!
> The rest all looks good.
>
> Thanks,
> Taylor
Thanks for the thorough review. Appreciate it!
Karthik
Attachment:
signature.asc
Description: PGP signature
