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

[Taylor Blau <me@ttaylorr.com>]

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())?

The rest all looks good.

Thanks,
Taylor