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

From: karthik nayak <karthik.188@gmail.com>
Date: Oct 29, 2024, 16:07 UTC
Message-ID: <CAOLa=ZRO9-ob6QrstLJJTrvnbJGpQAymYvLQh8Z5bFxGYJgiHw@mail.gmail.com>
In-Reply-To: <Zx+98PYOONmoA4A7@nand.local>

me@ttaylorr.com writes:

Show 14 quoted lines

> 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.

Show 49 quoted lines

>> @@ -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