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(!".
> 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.
