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

From: Junio C Hamano <[email-masked]>
Date: Fri, 08 Nov 2024 12:42:05 +0900

Karthik Nayak <[email-masked]> writes:

> The variables `packed_git_window_size` and `packed_git_limit` are global
> config variables used in the `packfile.c` file. Since it is only used in
> this file, let's change it from being a global config variable to a
> local variable for the subsystem.
>
> We do this by introducing a new local `packfile_config` struct in
> `packfile.c` and also adding the required function to parse the said
> config. We then use this within `packfile.c` to obtain the variables.

This patch has no string "packfile_config" in it, other than the one
in the above string.  A stale description?

>  		if (!win) {
> -			size_t window_align = packed_git_window_size / 2;
> +			size_t window_align;
>  			off_t len;
> +			struct repo_settings *settings;
> +
> +			/* lazy load the settings incase it hasn't been setup */

"incase" -> "in case"?

> +			prepare_repo_settings(p->repo);
> +			settings = &p->repo->settings;

This change is curious.  How can p->repo be uninitialized?  p is a
packed-git list created in some repository, surely it should already
be initialized, no?

> +
> +			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 +667,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 */
>  			win->base = xmmap_gently(NULL, win->len,

Other than that, the changes to the above block that uses the local
variable "settings" looks good.

Thanks.