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

> On Mon, Nov 04, 2024 at 12:41:46PM +0100, Karthik Nayak wrote:
>
>> @@ -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.
>

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