Re: [PATCH v6 0/9] packfile: avoid using the 'the_repository' global variable

[Junio C Hamano <gitster@pobox.com>]

Karthik Nayak <karthik.188@gmail.com> 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.