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

From: karthik nayak <karthik.188@gmail.com>
Date: Nov 8, 2024, 09:27 UTC
Message-ID: <CAOLa=ZQfU56VnqOOAGNJEeGqdEoKg9Mg0ic9Kfpk5qFepbYXig@mail.gmail.com>
In-Reply-To: <xmqqbjyqpf1u.fsf@gitster.g>

Junio C Hamano <gitster@pobox.com> writes:

Show 14 quoted lines

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

Yup indeed.

Show 10 quoted lines

>>  		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"?
>

Will change.

Show 7 quoted lines

>> +			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?
>

Here `p->repo` itself is expected to be initialized. We're however trying to initialize `p->repo->settings`. Which might not have been. If it is, `prepare_repo_settings` will return early.

Show 26 quoted lines

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