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@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.
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
