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@xxxxxxxxxxxx>
> ---
> 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
>
Attachment:
signature.asc
Description: PGP signature
