| Message ID | 2730aacd8e5add0918d806131d0f31a0b2474915.1730714298.git.karthik.188@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | packfile: avoid using the 'the_repository' global variable | expand |
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
Jeff King <peff@peff.net> 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
diff --git a/builtin/fast-import.c b/builtin/fast-import.c index 3ccc4c5722..0ece070260 100644 --- a/builtin/fast-import.c +++ b/builtin/fast-import.c @@ -3539,7 +3539,7 @@ static void parse_argv(void) int cmd_fast_import(int argc, const char **argv, const char *prefix, - struct repository *repo UNUSED) + struct repository *repo) { unsigned int i; @@ -3660,7 +3660,7 @@ int cmd_fast_import(int argc, fprintf(stderr, " pools: %10lu KiB\n", (unsigned long)((tree_entry_allocd + fi_mem_pool.pool_alloc) /1024)); fprintf(stderr, " objects: %10" PRIuMAX " KiB\n", (alloc_count*sizeof(struct object_entry))/1024); fprintf(stderr, "---------------------------------------------------------------------\n"); - pack_report(); + pack_report(repo); fprintf(stderr, "---------------------------------------------------------------------\n"); fprintf(stderr, "\n"); } diff --git a/config.c b/config.c index 728ef98e42..2c295f7430 100644 --- a/config.c +++ b/config.c @@ -1493,28 +1493,11 @@ static int git_default_core_config(const char *var, const char *value, return 0; } - if (!strcmp(var, "core.packedgitwindowsize")) { - int pgsz_x2 = getpagesize() * 2; - packed_git_window_size = git_config_ulong(var, value, ctx->kvi); - - /* This value must be multiple of (pagesize * 2) */ - packed_git_window_size /= pgsz_x2; - if (packed_git_window_size < 1) - packed_git_window_size = 1; - packed_git_window_size *= pgsz_x2; - return 0; - } - if (!strcmp(var, "core.bigfilethreshold")) { big_file_threshold = git_config_ulong(var, value, ctx->kvi); return 0; } - if (!strcmp(var, "core.packedgitlimit")) { - packed_git_limit = git_config_ulong(var, value, ctx->kvi); - return 0; - } - if (!strcmp(var, "core.autocrlf")) { if (value && !strcasecmp(value, "input")) { auto_crlf = AUTO_CRLF_INPUT; diff --git a/environment.c b/environment.c index 8e5022c282..8389a27270 100644 --- a/environment.c +++ b/environment.c @@ -49,8 +49,6 @@ int fsync_object_files = -1; int use_fsync = -1; enum fsync_method fsync_method = FSYNC_METHOD_DEFAULT; enum fsync_component fsync_components = FSYNC_COMPONENTS_DEFAULT; -size_t packed_git_window_size = DEFAULT_PACKED_GIT_WINDOW_SIZE; -size_t packed_git_limit = DEFAULT_PACKED_GIT_LIMIT; unsigned long big_file_threshold = 512 * 1024 * 1024; char *editor_program; char *askpass_program; diff --git a/packfile.c b/packfile.c index 2ae35dd03f..e1b04a2a6a 100644 --- a/packfile.c +++ b/packfile.c @@ -1,4 +1,3 @@ -#define USE_THE_REPOSITORY_VARIABLE #include "git-compat-util.h" #include "environment.h" @@ -48,15 +47,15 @@ static size_t pack_mapped; #define SZ_FMT PRIuMAX static inline uintmax_t sz_fmt(size_t s) { return s; } -void pack_report(void) +void pack_report(struct repository *repo) { fprintf(stderr, "pack_report: getpagesize() = %10" SZ_FMT "\n" "pack_report: core.packedGitWindowSize = %10" SZ_FMT "\n" "pack_report: core.packedGitLimit = %10" SZ_FMT "\n", sz_fmt(getpagesize()), - sz_fmt(packed_git_window_size), - sz_fmt(packed_git_limit)); + sz_fmt(repo->settings.packed_git_window_size), + sz_fmt(repo->settings.packed_git_limit)); fprintf(stderr, "pack_report: pack_used_ctr = %10u\n" "pack_report: pack_mmap_calls = %10u\n" @@ -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 */ win->base = xmmap_gently(NULL, win->len, diff --git a/packfile.h b/packfile.h index addb95b0c4..58104fa009 100644 --- a/packfile.h +++ b/packfile.h @@ -89,7 +89,7 @@ unsigned long repo_approximate_object_count(struct repository *r); struct packed_git *find_oid_pack(const struct object_id *oid, struct packed_git *packs); -void pack_report(void); +void pack_report(struct repository *repo); /* * mmap the index file for the specified packfile (if it is not diff --git a/repo-settings.c b/repo-settings.c index 4699b4b365..0d875fdd86 100644 --- a/repo-settings.c +++ b/repo-settings.c @@ -26,6 +26,7 @@ void prepare_repo_settings(struct repository *r) const char *strval; int manyfiles; int read_changed_paths; + unsigned long longval; if (!r->gitdir) BUG("Cannot add settings for uninitialized repository"); @@ -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; } enum log_refs_config repo_settings_get_log_all_ref_updates(struct repository *repo) diff --git a/repo-settings.h b/repo-settings.h index 51d6156a11..b22d6438e2 100644 --- a/repo-settings.h +++ b/repo-settings.h @@ -57,12 +57,17 @@ struct repo_settings { int core_multi_pack_index; int warn_ambiguous_refs; /* lazily loaded via accessor */ + + size_t packed_git_window_size; + size_t packed_git_limit; }; #define REPO_SETTINGS_INIT { \ .index_version = -1, \ .core_untracked_cache = UNTRACKED_CACHE_KEEP, \ .fetch_negotiation_algorithm = FETCH_NEGOTIATION_CONSECUTIVE, \ .warn_ambiguous_refs = -1, \ + .packed_git_window_size = DEFAULT_PACKED_GIT_WINDOW_SIZE, \ + .packed_git_limit = DEFAULT_PACKED_GIT_LIMIT, \ } void prepare_repo_settings(struct repository *r);
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. With this, we rid `packfile.c` from all global variable usage and this means we can also remove the `USE_THE_REPOSITORY_VARIABLE` guard from the file. Helped-by: Taylor Blau <me@ttaylorr.com> Signed-off-by: Karthik Nayak <karthik.188@gmail.com> --- builtin/fast-import.c | 4 ++-- config.c | 17 ----------------- environment.c | 2 -- packfile.c | 19 +++++++++++-------- packfile.h | 2 +- repo-settings.c | 14 ++++++++++++++ repo-settings.h | 5 +++++ 7 files changed, 33 insertions(+), 30 deletions(-)