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

[Patrick Steinhardt <ps@pks.im>]

[-- Attachment #1: Type: text/plain, Size: 4502 bytes --]

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
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]