Re: [PATCH v5 8/9] config: make `packed_git_(limit|window_size)` non-global variables

From: Jeff King <peff@peff.net>
Date: Nov 4, 2024, 17:38 UTC
Message-ID: <20241104173828.GC2989472@coredump.intra.peff.net>
In-Reply-To: <2730aacd8e5add0918d806131d0f31a0b2474915.1730714298.git.karthik.188@gmail.com>

On Mon, Nov 04, 2024 at 12:41:46PM +0100, Karthik Nayak wrote:

Show 28 quoted lines

> @@ -652,8 +651,11 @@ unsigned char *use_pack(struct packed_git *p,
>  				break;
>  		}
>  		if (!win) {
> -			size_t window_align = packed_git_window_size / 2;
> +			size_t window_align;
>  			off_t len;
> +			struct repo_settings *settings = &p->repo->settings;
> +
> +			window_align = settings->packed_git_window_size / 2;
>  
>  			if (p->pack_fd == -1 && open_packed_git(p))
>  				die("packfile %s cannot be accessed", p->pack_name);
> @@ -661,11 +663,12 @@ unsigned char *use_pack(struct packed_git *p,
>  			CALLOC_ARRAY(win, 1);
>  			win->offset = (offset / window_align) * window_align;
>  			len = p->pack_size - win->offset;
> -			if (len > packed_git_window_size)
> -				len = packed_git_window_size;
> +			if (len > settings->packed_git_window_size)
> +				len = settings->packed_git_window_size;
>  			win->len = (size_t)len;
>  			pack_mapped += win->len;
> -			while (packed_git_limit < pack_mapped
> +
> +			while (settings->packed_git_limit < pack_mapped
>  				&& unuse_one_window(p))
>  				; /* nothing */

Much nicer than the earlier version of the patch.

Do we need to call prepare_repo_settings() here? It looks like the intent is that it would be lazy-loaded, and I don't think there's any guarantee that somebody else would have done so.

Show 17 quoted lines

> @@ -123,6 +124,19 @@ void prepare_repo_settings(struct repository *r)
>  	 * removed.
>  	 */
>  	r->settings.command_requires_full_index = 1;
> +
> +	if (!repo_config_get_ulong(r, "core.packedgitwindowsize", &longval)) {
> +		int pgsz_x2 = getpagesize() * 2;
> +
> +		/* This value must be multiple of (pagesize * 2) */
> +		longval /= pgsz_x2;
> +		if (longval < 1)
> +			longval = 1;
> +		r->settings.packed_git_window_size = longval * pgsz_x2;
> +	}
> +
> +	if (!repo_config_get_ulong(r, "core.packedgitlimit", &longval))
> +		r->settings.packed_git_limit = longval;

And this looks like a faithful conversion of the existing parsing. Since we're switching from git_config_ulong() to repo_config_get_ulong(), we could take the opportunity to swap out for the size_t parser, but:

  1. I'm just as happy for that to happen separately, and leave this as
     a patch which should not have any behavior change.

  2. It looks like we do not yet have a size_t variant for the configset
     accessors. :)

-Peff