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

From: Junio C Hamano <gitster@pobox.com>
Date: Nov 8, 2024, 07:49 UTC
Message-ID: <xmqqo72qnp1o.fsf@gitster.g>
In-Reply-To: <cover.1730976185.git.karthik.188@gmail.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.

Show 11 quoted lines

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

Show 16 quoted lines

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

Show 14 quoted lines

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