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
