Junio C Hamano <gitster@xxxxxxxxx> writes:
> Karthik Nayak <karthik.188@xxxxxxxxx> writes:
>
>> Changes in v6:
>> - Lazy load repository settings in packfile.c. This ensures that the settings are
>> available for sure and we do not rely on callees setting it.
>> - Use `size_t` for `delta_base_cache_limit`.
>
> I'll trust the reviews made while I was gone and will comment only
> on the differences between the last iteration.
>
>> diff --git c/builtin/gc.c w/builtin/gc.c
>> index 9a10eb58bc..ad80c3aed2 100644
>> --- c/builtin/gc.c
>> +++ w/builtin/gc.c
>> @@ -138,7 +138,7 @@ struct gc_config {
>> char *repack_filter_to;
>> unsigned long big_pack_threshold;
>> unsigned long max_delta_cache_size;
>> - unsigned long delta_base_cache_limit;
>> + size_t delta_base_cache_limit;
>> };
>
> Makes sense.
>
>> @@ -170,6 +170,7 @@ static void gc_config(struct gc_config *cfg)
>> {
>> const char *value;
>> char *owned = NULL;
>> + unsigned long longval;
>>
>> if (!git_config_get_value("gc.packrefs", &value)) {
>> if (value && !strcmp(value, "notbare"))
>> @@ -207,7 +208,9 @@ static void gc_config(struct gc_config *cfg)
>>
>> git_config_get_ulong("gc.bigpackthreshold", &cfg->big_pack_threshold);
>> git_config_get_ulong("pack.deltacachesize", &cfg->max_delta_cache_size);
>> - git_config_get_ulong("core.deltabasecachelimit", &cfg->delta_base_cache_limit);
>> +
>> + if(!git_config_get_ulong("core.deltabasecachelimit", &longval))
>> + cfg->delta_base_cache_limit = longval;
>
> And this is a sensible way to fill size_t member with the value read
> into a ulong. Should "longval" be named after "unsigned long" instead
> of "long", by the way?
>
> There is a required SP missing inside "if(!".
>
Agreed, will fix both and send in a new version.
>> diff --git c/packfile.c w/packfile.c
>> index e1b04a2a6a..46f5369173 100644
>> --- c/packfile.c
>> +++ w/packfile.c
>> @@ -653,7 +653,11 @@ unsigned char *use_pack(struct packed_git *p,
>> if (!win) {
>> size_t window_align;
>> off_t len;
>> - struct repo_settings *settings = &p->repo->settings;
>> + struct repo_settings *settings;
>> +
>> + /* lazy load the settings incase it hasn't been setup */
>> + prepare_repo_settings(p->repo);
>> + settings = &p->repo->settings;
>
> This is a bit curious. I'll read the individual patch that has this
> change before commenting on it.
Attachment:
signature.asc
Description: PGP signature
