Re: [PATCH v5 8/9] config: make `packed_git_(limit|window_size)` non-global variables
From: Jeff King <hidden>
Date: 2024-11-04 17:38:29
On Mon, Nov 04, 2024 at 12:41:46PM +0100, Karthik Nayak wrote:
quoted hunk ↗ jump to hunk
@@ -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.
quoted hunk ↗ jump to hunk
@@ -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