On Mon, Oct 28, 2024 at 02:43:45PM +0100, Karthik Nayak wrote:
> 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.
> 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.
> 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
