Re: [PATCH 04/24] midx: factor out `fill_pack_info()`

[Taylor Blau <me@ttaylorr.com>]

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