| Message ID | e99b1dad549bb1e87011d8722b8582172390aa04.1730122499.git.karthik.188@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | packfile: avoid using the 'the_repository' global variable | expand |
On Mon, Oct 28, 2024 at 02:43:46PM +0100, Karthik Nayak wrote: > 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. Ahh. Now the motivation of the previous patch is clearer. Have you considered hinting at the motivation here in the previous commit message (e.g., "this gets us part of the way towards ...")? > 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; Very satisfying :-). > +struct packfile_config { > + unsigned long packed_git_window_size; > + unsigned long packed_git_limit; > +}; > + > +#define PACKFILE_CONFIG_INIT \ > +{ \ > + .packed_git_window_size = DEFAULT_PACKED_GIT_WINDOW_SIZE, \ > + .packed_git_limit = DEFAULT_PACKED_GIT_LIMIT, \ s/, /, / > +static int packfile_config(const char *var, const char *value, > + const struct config_context *ctx, void *cb) > { > + struct packfile_config *config = cb; > + > + if (!strcmp(var, "core.packedgitwindowsize")) { > + int pgsz_x2 = getpagesize() * 2; > + config->packed_git_window_size = git_config_ulong(var, value, ctx->kvi); > + > + /* This value must be multiple of (pagesize * 2) */ > + config->packed_git_window_size /= pgsz_x2; > + if (config->packed_git_window_size < 1) > + config->packed_git_window_size = 1; > + config->packed_git_window_size *= pgsz_x2; > + return 0; > + } > + > + if (!strcmp(var, "core.packedgitlimit")) { > + config->packed_git_limit = git_config_ulong(var, value, ctx->kvi); > + return 0; > + } > + > + return git_default_config(var, value, ctx, cb); > +} I get that this was copy/pasted from elsewhere, but it would be nice to replace the "every if statement ends in 'return 0' to keep them mutually exclusive" with else if statements instead: --- 8< --- diff --git a/packfile.c b/packfile.c index cfbfcdc2b8..c8af29bf0a 100644 --- a/packfile.c +++ b/packfile.c @@ -72,15 +72,11 @@ static int packfile_config(const char *var, const char *value, if (config->packed_git_window_size < 1) config->packed_git_window_size = 1; config->packed_git_window_size *= pgsz_x2; - return 0; - } - - if (!strcmp(var, "core.packedgitlimit")) { + } else if (!strcmp(var, "core.packedgitlimit")) { config->packed_git_limit = git_config_ulong(var, value, ctx->kvi); - return 0; + } else { + return git_default_config(var, value, ctx, cb); } - - return git_default_config(var, value, ctx, cb); } --- >8 --- > + > + Extra newline here (after the definition of packfile_config())? The rest all looks good. Thanks, Taylor
Taylor Blau <me@ttaylorr.com> writes: > On Mon, Oct 28, 2024 at 02:43:46PM +0100, Karthik Nayak wrote: >> 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. > > Ahh. Now the motivation of the previous patch is clearer. Have you > considered hinting at the motivation here in the previous commit message > (e.g., "this gets us part of the way towards ...")? > Indeed, will add. >> 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; > > Very satisfying :-). > >> +struct packfile_config { >> + unsigned long packed_git_window_size; >> + unsigned long packed_git_limit; >> +}; >> + >> +#define PACKFILE_CONFIG_INIT \ >> +{ \ >> + .packed_git_window_size = DEFAULT_PACKED_GIT_WINDOW_SIZE, \ >> + .packed_git_limit = DEFAULT_PACKED_GIT_LIMIT, \ > > s/, /, / > >> +static int packfile_config(const char *var, const char *value, >> + const struct config_context *ctx, void *cb) >> { >> + struct packfile_config *config = cb; >> + >> + if (!strcmp(var, "core.packedgitwindowsize")) { >> + int pgsz_x2 = getpagesize() * 2; >> + config->packed_git_window_size = git_config_ulong(var, value, ctx->kvi); >> + >> + /* This value must be multiple of (pagesize * 2) */ >> + config->packed_git_window_size /= pgsz_x2; >> + if (config->packed_git_window_size < 1) >> + config->packed_git_window_size = 1; >> + config->packed_git_window_size *= pgsz_x2; >> + return 0; >> + } >> + >> + if (!strcmp(var, "core.packedgitlimit")) { >> + config->packed_git_limit = git_config_ulong(var, value, ctx->kvi); >> + return 0; >> + } >> + >> + return git_default_config(var, value, ctx, cb); >> +} > > I get that this was copy/pasted from elsewhere, but it would be nice to > replace the "every if statement ends in 'return 0' to keep them mutually > exclusive" with else if statements instead: > > --- 8< --- > diff --git a/packfile.c b/packfile.c > index cfbfcdc2b8..c8af29bf0a 100644 > --- a/packfile.c > +++ b/packfile.c > @@ -72,15 +72,11 @@ static int packfile_config(const char *var, const char *value, > if (config->packed_git_window_size < 1) > config->packed_git_window_size = 1; > config->packed_git_window_size *= pgsz_x2; > - return 0; > - } > - > - if (!strcmp(var, "core.packedgitlimit")) { > + } else if (!strcmp(var, "core.packedgitlimit")) { > config->packed_git_limit = git_config_ulong(var, value, ctx->kvi); > - return 0; > + } else { > + return git_default_config(var, value, ctx, cb); > } > - > - return git_default_config(var, value, ctx, cb); > } > --- >8 --- > Thanks, will patch this in. I try and avoid such things to mostly make it easier to review code block movements. But here I think it is indeed nicer to change for the better. >> + >> + > > Extra newline here (after the definition of packfile_config())? > Oops! > The rest all looks good. > > Thanks, > Taylor Thanks for the thorough review. Appreciate it! Karthik
On Tue, Oct 29, 2024 at 12:09:09PM -0400, karthik nayak wrote:
> Thanks for the thorough review. Appreciate it!
Thanks for working on it!
Thanks,
Taylor
diff --git a/builtin/fast-import.c b/builtin/fast-import.c index f4892d7f37..9056447bd0 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 93b0d6af31..cfbfcdc2b8 100644 --- a/packfile.c +++ b/packfile.c @@ -1,4 +1,3 @@ -#define USE_THE_REPOSITORY_VARIABLE #include "git-compat-util.h" #include "environment.h" @@ -27,6 +26,17 @@ #include "config.h" #include "pack-objects.h" +struct packfile_config { + unsigned long packed_git_window_size; + unsigned long packed_git_limit; +}; + +#define PACKFILE_CONFIG_INIT \ +{ \ + .packed_git_window_size = DEFAULT_PACKED_GIT_WINDOW_SIZE, \ + .packed_git_limit = DEFAULT_PACKED_GIT_LIMIT, \ +} + char *odb_pack_name(struct repository *repo, struct strbuf *buf, const unsigned char *hash, const char *ext) { @@ -48,15 +58,44 @@ static size_t pack_mapped; #define SZ_FMT PRIuMAX static inline uintmax_t sz_fmt(size_t s) { return s; } -void pack_report(void) +static int packfile_config(const char *var, const char *value, + const struct config_context *ctx, void *cb) { + struct packfile_config *config = cb; + + if (!strcmp(var, "core.packedgitwindowsize")) { + int pgsz_x2 = getpagesize() * 2; + config->packed_git_window_size = git_config_ulong(var, value, ctx->kvi); + + /* This value must be multiple of (pagesize * 2) */ + config->packed_git_window_size /= pgsz_x2; + if (config->packed_git_window_size < 1) + config->packed_git_window_size = 1; + config->packed_git_window_size *= pgsz_x2; + return 0; + } + + if (!strcmp(var, "core.packedgitlimit")) { + config->packed_git_limit = git_config_ulong(var, value, ctx->kvi); + return 0; + } + + return git_default_config(var, value, ctx, cb); +} + + +void pack_report(struct repository *repo) +{ + struct packfile_config config = PACKFILE_CONFIG_INIT; + repo_config(repo, packfile_config, &config); + 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(config.packed_git_window_size), + sz_fmt(config.packed_git_limit)); fprintf(stderr, "pack_report: pack_used_ctr = %10u\n" "pack_report: pack_mmap_calls = %10u\n" @@ -652,20 +691,25 @@ unsigned char *use_pack(struct packed_git *p, break; } if (!win) { - size_t window_align = packed_git_window_size / 2; + struct packfile_config config = PACKFILE_CONFIG_INIT; + size_t window_align; off_t len; + repo_config(p->repo, packfile_config, &config); + window_align = config.packed_git_window_size / 2; + if (p->pack_fd == -1 && open_packed_git(p)) die("packfile %s cannot be accessed", p->pack_name); 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 > config.packed_git_window_size) + len = config.packed_git_window_size; win->len = (size_t)len; pack_mapped += win->len; - while (packed_git_limit < pack_mapped + + while (config.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 c1883e60ef..3409aef35d 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
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. Signed-off-by: Karthik Nayak <karthik.188@gmail.com> --- builtin/fast-import.c | 4 +-- config.c | 17 ------------ environment.c | 2 -- packfile.c | 60 +++++++++++++++++++++++++++++++++++++------ packfile.h | 2 +- 5 files changed, 55 insertions(+), 30 deletions(-)