Re: [PATCH 2/8] midx-write.c: reduce argument count for `get_sorted_entries()`

From: Patrick Steinhardt <ps@pks.im>
Date: May 29, 2024, 07:47 UTC
Message-ID: <ZlbdrczSqKgueK0g@tanuki>
In-Reply-To: <07dad5a5812794be6e355b1e0eb3722d452f292b.1716482279.git.me@ttaylorr.com>

On Thu, May 23, 2024 at 12:38:06PM -0400, Taylor Blau wrote:

Show 36 quoted lines

> diff --git a/midx-write.c b/midx-write.c
> index 03e95ae821..ad32e8953d 100644
> --- a/midx-write.c
> +++ b/midx-write.c
> @@ -299,21 +299,17 @@ static void midx_fanout_add_pack_fanout(struct midx_fanout *fanout,
>   * Copy only the de-duplicated entries (selected by most-recent modified time
>   * of a packfile containing the object).
>   */
> -static struct pack_midx_entry *get_sorted_entries(struct multi_pack_index *m,
> -						  struct pack_info *info,
> -						  uint32_t nr_packs,
> -						  size_t *nr_objects,
> -						  int preferred_pack)
> +static struct pack_midx_entry *get_sorted_entries(struct write_midx_context *ctx)
>  {
>  	uint32_t cur_fanout, cur_pack, cur_object;
>  	size_t alloc_objects, total_objects = 0;
>  	struct midx_fanout fanout = { 0 };
>  	struct pack_midx_entry *deduplicated_entries = NULL;
> -	uint32_t start_pack = m ? m->num_packs : 0;
> +	uint32_t start_pack = ctx->m ? ctx->m->num_packs : 0;
>  
> -	for (cur_pack = start_pack; cur_pack < nr_packs; cur_pack++)
> +	for (cur_pack = start_pack; cur_pack < ctx->nr; cur_pack++)
>  		total_objects = st_add(total_objects,
> -				       info[cur_pack].p->num_objects);
> +				       ctx->info[cur_pack].p->num_objects);
>  
>  	/*
>  	 * As we de-duplicate by fanout value, we expect the fanout
> @@ -324,25 +320,25 @@ static struct pack_midx_entry *get_sorted_entries(struct multi_pack_index *m,
>  
>  	ALLOC_ARRAY(fanout.entries, fanout.alloc);
>  	ALLOC_ARRAY(deduplicated_entries, alloc_objects);
> -	*nr_objects = 0;
> +	ctx->entries_nr = 0;

Nit: I think it's a bit surprising that a getter function would modify
the passed in structure. It's also a bit puzzling that we assign
`entries_nr` in here, but rely on the caller to set the corresponding
`entries` field. I think we should either have the caller assign both
fields, or we should rename the function and assign both of these fields
in the function.

Patrick