| Message ID | ccf1337305db60f1c8174e9b309e2a9e04ce1487.1701198172.git.me@ttaylorr.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | pack-objects: multi-pack verbatim reuse | expand |
On Tue, Nov 28, 2023 at 02:08:05PM -0500, Taylor Blau wrote: > When selecting which packfiles will be written while generating a MIDX, > the MIDX internals fill out a 'struct pack_info' with various pieces of > book-keeping. > > Instead of filling out each field of the `pack_info` structure > individually in each of the two spots that modify the array of such > structures (`ctx->info`), extract a common routine that does this for > us. > > This reduces the code duplication by a modest amount. But more > importantly, it zero-initializes the structure before assigning values > into it. This hardens us for a future change which will add additional > fields to this structure which (until this patch) was not > zero-initialized. > > As a result, any new fields added to the `pack_info` structure need only > be updated in a single location, instead of at each spot within midx.c. > > There are no functional changes in this patch. > > Signed-off-by: Taylor Blau <me@ttaylorr.com> > --- > midx.c | 35 +++++++++++++++++++---------------- > 1 file changed, 19 insertions(+), 16 deletions(-) > > diff --git a/midx.c b/midx.c > index 3b727dc633..591b3c636e 100644 > --- a/midx.c > +++ b/midx.c > @@ -464,6 +464,17 @@ struct pack_info { > unsigned expired : 1; > }; > > +static void fill_pack_info(struct pack_info *info, > + struct packed_git *p, char *pack_name, > + uint32_t orig_pack_int_id) > +{ > + memset(info, 0, sizeof(struct pack_info)); > + > + info->orig_pack_int_id = orig_pack_int_id; > + info->pack_name = pack_name; > + info->p = p; > +} Nit: all callers manually call `xstrdup(pack_name)` and pass that to `fill_pack_info()`. We could consider doing this in here instead so that ownership of the string becomes a tad clearer. > static int pack_info_compare(const void *_a, const void *_b) > { > struct pack_info *a = (struct pack_info *)_a; > @@ -504,6 +515,7 @@ static void add_pack_to_midx(const char *full_path, size_t full_path_len, > const char *file_name, void *data) > { > struct write_midx_context *ctx = data; > + struct packed_git *p; > > if (ends_with(file_name, ".idx")) { > display_progress(ctx->progress, ++ctx->pack_paths_checked); > @@ -530,17 +542,14 @@ static void add_pack_to_midx(const char *full_path, size_t full_path_len, > > ALLOC_GROW(ctx->info, ctx->nr + 1, ctx->alloc); > > - ctx->info[ctx->nr].p = add_packed_git(full_path, > - full_path_len, > - 0); > - > - if (!ctx->info[ctx->nr].p) { > + p = add_packed_git(full_path, full_path_len, 0); > + if (!p) { > warning(_("failed to add packfile '%s'"), > full_path); > return; > } > > - if (open_pack_index(ctx->info[ctx->nr].p)) { > + if (open_pack_index(p)) { > warning(_("failed to open pack-index '%s'"), > full_path); > close_pack(ctx->info[ctx->nr].p); Isn't `ctx->info[ctx->nr].p` still uninitialized at this point? > @@ -548,9 +557,8 @@ static void add_pack_to_midx(const char *full_path, size_t full_path_len, > return; > } > > - ctx->info[ctx->nr].pack_name = xstrdup(file_name); > - ctx->info[ctx->nr].orig_pack_int_id = ctx->nr; > - ctx->info[ctx->nr].expired = 0; > + fill_pack_info(&ctx->info[ctx->nr], p, xstrdup(file_name), > + ctx->nr); > ctx->nr++; > } > } > @@ -1310,11 +1318,6 @@ static int write_midx_internal(const char *object_dir, > for (i = 0; i < ctx.m->num_packs; i++) { > ALLOC_GROW(ctx.info, ctx.nr + 1, ctx.alloc); > > - ctx.info[ctx.nr].orig_pack_int_id = i; > - ctx.info[ctx.nr].pack_name = xstrdup(ctx.m->pack_names[i]); > - ctx.info[ctx.nr].p = ctx.m->packs[i]; > - ctx.info[ctx.nr].expired = 0; > - > if (flags & MIDX_WRITE_REV_INDEX) { > /* > * If generating a reverse index, need to have > @@ -1330,10 +1333,10 @@ static int write_midx_internal(const char *object_dir, > if (open_pack_index(ctx.m->packs[i])) > die(_("could not open index for %s"), > ctx.m->packs[i]->pack_name); > - ctx.info[ctx.nr].p = ctx.m->packs[i]; Just to make sure I'm not missing anything, but this assignment here was basically redundant before this patch already, right? Patrick > } > > - ctx.nr++; > + fill_pack_info(&ctx.info[ctx.nr++], ctx.m->packs[i], > + xstrdup(ctx.m->pack_names[i]), i); > } > } > > -- > 2.43.0.24.g980b318f98 >
On Thu, Nov 30, 2023 at 11:18:37AM +0100, Patrick Steinhardt wrote: > On Tue, Nov 28, 2023 at 02:08:05PM -0500, Taylor Blau wrote: > > When selecting which packfiles will be written while generating a MIDX, > > the MIDX internals fill out a 'struct pack_info' with various pieces of > > book-keeping. > > > > Instead of filling out each field of the `pack_info` structure > > individually in each of the two spots that modify the array of such > > structures (`ctx->info`), extract a common routine that does this for > > us. > > > > This reduces the code duplication by a modest amount. But more > > importantly, it zero-initializes the structure before assigning values > > into it. This hardens us for a future change which will add additional > > fields to this structure which (until this patch) was not > > zero-initialized. > > > > As a result, any new fields added to the `pack_info` structure need only > > be updated in a single location, instead of at each spot within midx.c. > > > > There are no functional changes in this patch. > > > > Signed-off-by: Taylor Blau <me@ttaylorr.com> > > --- > > midx.c | 35 +++++++++++++++++++---------------- > > 1 file changed, 19 insertions(+), 16 deletions(-) > > > > diff --git a/midx.c b/midx.c > > index 3b727dc633..591b3c636e 100644 > > --- a/midx.c > > +++ b/midx.c > > @@ -464,6 +464,17 @@ struct pack_info { > > unsigned expired : 1; > > }; > > > > +static void fill_pack_info(struct pack_info *info, > > + struct packed_git *p, char *pack_name, > > + uint32_t orig_pack_int_id) > > +{ > > + memset(info, 0, sizeof(struct pack_info)); > > + > > + info->orig_pack_int_id = orig_pack_int_id; > > + info->pack_name = pack_name; > > + info->p = p; > > +} > > Nit: all callers manually call `xstrdup(pack_name)` and pass that to > `fill_pack_info()`. We could consider doing this in here instead so that > ownership of the string becomes a tad clearer. That's a great idea. I think we'd also want to mark the pack_name argument as const, not just because xstrdup() requires it, but also because it communicates the ownership more clearly. I'll squash something like this in: --- >8 --- diff --git a/midx.c b/midx.c index b8b3f41024..6fb5e237b7 100644 --- a/midx.c +++ b/midx.c @@ -465,13 +465,13 @@ struct pack_info { }; static void fill_pack_info(struct pack_info *info, - struct packed_git *p, char *pack_name, + struct packed_git *p, const char *pack_name, uint32_t orig_pack_int_id) { memset(info, 0, sizeof(struct pack_info)); info->orig_pack_int_id = orig_pack_int_id; - info->pack_name = pack_name; + info->pack_name = xstrdup(pack_name); info->p = p; } @@ -557,8 +557,7 @@ static void add_pack_to_midx(const char *full_path, size_t full_path_len, return; } - fill_pack_info(&ctx->info[ctx->nr], p, xstrdup(file_name), - ctx->nr); + fill_pack_info(&ctx->info[ctx->nr], p, file_name, ctx->nr); ctx->nr++; } } @@ -1336,7 +1335,7 @@ static int write_midx_internal(const char *object_dir, } fill_pack_info(&ctx.info[ctx.nr++], ctx.m->packs[i], - xstrdup(ctx.m->pack_names[i]), i); + ctx.m->pack_names[i], i); } } --- 8< --- > > - if (open_pack_index(ctx->info[ctx->nr].p)) { > > + if (open_pack_index(p)) { > > warning(_("failed to open pack-index '%s'"), > > full_path); > > close_pack(ctx->info[ctx->nr].p); > > Isn't `ctx->info[ctx->nr].p` still uninitialized at this point? Great catch, thank you! > > @@ -1330,10 +1333,10 @@ static int write_midx_internal(const char *object_dir, > > if (open_pack_index(ctx.m->packs[i])) > > die(_("could not open index for %s"), > > ctx.m->packs[i]->pack_name); > > - ctx.info[ctx.nr].p = ctx.m->packs[i]; > > Just to make sure I'm not missing anything, but this assignment here was > basically redundant before this patch already, right? I think that's right, but in either case we're assigning the pack once at the end of each loop iteration via a single call to fill_pack_info(). Since we're using ctx.m->packs[i] in both places (after a call to prepare_midx_pack()), we should be OK here. Thanks, Taylor
diff --git a/midx.c b/midx.c index 3b727dc633..591b3c636e 100644 --- a/midx.c +++ b/midx.c @@ -464,6 +464,17 @@ struct pack_info { unsigned expired : 1; }; +static void fill_pack_info(struct pack_info *info, + struct packed_git *p, char *pack_name, + uint32_t orig_pack_int_id) +{ + memset(info, 0, sizeof(struct pack_info)); + + info->orig_pack_int_id = orig_pack_int_id; + info->pack_name = pack_name; + info->p = p; +} + static int pack_info_compare(const void *_a, const void *_b) { struct pack_info *a = (struct pack_info *)_a; @@ -504,6 +515,7 @@ static void add_pack_to_midx(const char *full_path, size_t full_path_len, const char *file_name, void *data) { struct write_midx_context *ctx = data; + struct packed_git *p; if (ends_with(file_name, ".idx")) { display_progress(ctx->progress, ++ctx->pack_paths_checked); @@ -530,17 +542,14 @@ static void add_pack_to_midx(const char *full_path, size_t full_path_len, ALLOC_GROW(ctx->info, ctx->nr + 1, ctx->alloc); - ctx->info[ctx->nr].p = add_packed_git(full_path, - full_path_len, - 0); - - if (!ctx->info[ctx->nr].p) { + p = add_packed_git(full_path, full_path_len, 0); + if (!p) { warning(_("failed to add packfile '%s'"), full_path); return; } - if (open_pack_index(ctx->info[ctx->nr].p)) { + if (open_pack_index(p)) { warning(_("failed to open pack-index '%s'"), full_path); close_pack(ctx->info[ctx->nr].p); @@ -548,9 +557,8 @@ static void add_pack_to_midx(const char *full_path, size_t full_path_len, return; } - ctx->info[ctx->nr].pack_name = xstrdup(file_name); - ctx->info[ctx->nr].orig_pack_int_id = ctx->nr; - ctx->info[ctx->nr].expired = 0; + fill_pack_info(&ctx->info[ctx->nr], p, xstrdup(file_name), + ctx->nr); ctx->nr++; } } @@ -1310,11 +1318,6 @@ static int write_midx_internal(const char *object_dir, for (i = 0; i < ctx.m->num_packs; i++) { ALLOC_GROW(ctx.info, ctx.nr + 1, ctx.alloc); - ctx.info[ctx.nr].orig_pack_int_id = i; - ctx.info[ctx.nr].pack_name = xstrdup(ctx.m->pack_names[i]); - ctx.info[ctx.nr].p = ctx.m->packs[i]; - ctx.info[ctx.nr].expired = 0; - if (flags & MIDX_WRITE_REV_INDEX) { /* * If generating a reverse index, need to have @@ -1330,10 +1333,10 @@ static int write_midx_internal(const char *object_dir, if (open_pack_index(ctx.m->packs[i])) die(_("could not open index for %s"), ctx.m->packs[i]->pack_name); - ctx.info[ctx.nr].p = ctx.m->packs[i]; } - ctx.nr++; + fill_pack_info(&ctx.info[ctx.nr++], ctx.m->packs[i], + xstrdup(ctx.m->pack_names[i]), i); } }
When selecting which packfiles will be written while generating a MIDX, the MIDX internals fill out a 'struct pack_info' with various pieces of book-keeping. Instead of filling out each field of the `pack_info` structure individually in each of the two spots that modify the array of such structures (`ctx->info`), extract a common routine that does this for us. This reduces the code duplication by a modest amount. But more importantly, it zero-initializes the structure before assigning values into it. This hardens us for a future change which will add additional fields to this structure which (until this patch) was not zero-initialized. As a result, any new fields added to the `pack_info` structure need only be updated in a single location, instead of at each spot within midx.c. There are no functional changes in this patch. Signed-off-by: Taylor Blau <me@ttaylorr.com> --- midx.c | 35 +++++++++++++++++++---------------- 1 file changed, 19 insertions(+), 16 deletions(-)