Re: [PATCH v2 7/8] config: make `delta_base_cache_limit` a non-global variable

From: me@ttaylorr.com <me@ttaylorr.com>
Date: Oct 28, 2024, 16:38 UTC
Message-ID: <Zx+98PYOONmoA4A7@nand.local>
In-Reply-To: <89ee3f48ce982236dd7841fa1a5cb15aa256b901.1730122499.git.karthik.188@gmail.com>

On Mon, Oct 28, 2024 at 02:43:45PM +0100, Karthik Nayak wrote:

Show 9 quoted lines

> The `delta_base_cache_limit` variable is a global config variable used
> by multiple subsystems. Let's make this non-global, by adding this
> variable to the stack of each of the subsystems where it is used.
>
> In `gc.c` we add it to the `gc_config` struct and also the constructor
> function. In `index-pack.c` we add it to the `pack_idx_option` struct
> and its constructor. Finally, in `packfile.c` we dynamically retrieve
> this value from the repository config, since the value is only used once
> in the entire subsystem.

OK. Perhaps I am not quite following why this change is necessary, at least in the context of the rest of this series. But let's read on...

> @@ -1604,6 +1604,10 @@ static int git_index_pack_config(const char *k, const char *v,
>  		else
>  			opts->flags &= ~WRITE_REV;

Not a huge deal, and not the fault of your patch here, but the if(!strcmp(k, "pack.writereverseindex")) block should terminate with a "return 0".

> +	if (!strcmp(k, "core.deltabasecachelimit")) {
> +		opts->delta_base_cache_limit = git_config_ulong(k, v, ctx->kvi);
> +		return 0;

But here you do 'return 0;' at the end of handling the 'core.deltabasecachelimit' configuration value. Good.

Show 13 quoted lines

> diff --git a/config.c b/config.c
> index a11bb85da3..728ef98e42 100644
> --- a/config.c
> +++ b/config.c
> @@ -1515,11 +1515,6 @@ static int git_default_core_config(const char *var, const char *value,
>  		return 0;
>  	}
>
> -	if (!strcmp(var, "core.deltabasecachelimit")) {
> -		delta_base_cache_limit = git_config_ulong(var, value, ctx->kvi);
> -		return 0;
> -	}
> -

This is safe to drop from git_default_core_config() because the static variable from environment.h is gone, so nobody is accidentally reading an zero'd value.

Show 11 quoted lines

> diff --git a/pack-objects.h b/pack-objects.h
> index b9898a4e64..3f6f504203 100644
> --- a/pack-objects.h
> +++ b/pack-objects.h
> @@ -7,7 +7,8 @@
>
>  struct repository;
>
> -#define DEFAULT_DELTA_CACHE_SIZE (256 * 1024 * 1024)
> +#define DEFAULT_DELTA_CACHE_SIZE       (256 * 1024 * 1024)
> +#define DEFAULT_DELTA_BASE_CACHE_LIMIT (96 * 1024 * 1024)

Adding DEFAULT_DELTA_BASE_CACHE_LIMIT makes sense, and I assume the diff on the line above is clang-format noise to keep the two declarations aligned or something?

The rest looks good.

Thanks, Taylor