me@xxxxxxxxxxxx writes:
> 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...
>
Ah, well, as you know by now, it is to cleanup the usage of the global
config state in packfile.c. I think I brief over it in the cover letter
but like you mentioned in the next patch, I'll amend and add some
details here too.
>> @@ -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?
>
Yup, indeed, that is the point.
> The rest looks good.
>
> Thanks,
> Taylor
Attachment:
signature.asc
Description: PGP signature
