Jeff King <peff@xxxxxxxx> writes:
> 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.
>
I think it would be safer to do that, than to rely on the tests like I
did. I'll change that.
>> @@ -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
Yes, indeed. I'll leave it out of this. I'll follow up if I can! Thanks
Karthik
Attachment:
signature.asc
Description: PGP signature
