Re: [PATCH 08/17] midx-write.c: introduce `struct write_midx_opts`

From: Patrick Steinhardt <ps@pks.im>
Date: Dec 8, 2025, 18:26 UTC
Message-ID: <aTcYXJr_-IxPmC65@pks.im>
In-Reply-To: <47aae3bf2a83a2724aecd3314f8cc5d47e8013f1.1765053054.git.me@ttaylorr.com>

On Sat, Dec 06, 2025 at 03:31:22PM -0500, Taylor Blau wrote:

Show 32 quoted lines

> diff --git a/midx-write.c b/midx-write.c
> index c30f6a70d37..b262631ae45 100644
> --- a/midx-write.c
> +++ b/midx-write.c
> @@ -1014,14 +1014,20 @@ static void clear_midx_files(struct odb_source *source,
>  	strbuf_release(&buf);
>  }
>  
> -static int write_midx_internal(struct odb_source *source,
> -			       struct string_list *packs_to_include,
> -			       struct string_list *packs_to_drop,
> -			       const char *preferred_pack_name,
> -			       const char *refs_snapshot,
> -			       unsigned flags)
> +struct write_midx_opts {
> +	struct odb_source *source;
> +
> +	struct string_list *packs_to_include;
> +	struct string_list *packs_to_drop;
> +
> +	const char *preferred_pack_name;
> +	const char *refs_snapshot;
> +	unsigned flags;
> +};
> +
> +static int write_midx_internal(struct write_midx_opts *opts)
>  {
> -	struct repository *r = source->odb->repo;
> +	struct repository *r = opts->source->odb->repo;
>  	struct strbuf midx_name = STRBUF_INIT;
>  	unsigned char midx_hash[GIT_MAX_RAWSZ];
>  	uint32_t start_pack;

One might argue that parameters which _must_ be passed could be moved out of the structure and into the function signature, and as far as I understand, that would only be the `struct odb_source`. After all, we are talking about options, and a mandatory field is not really an option in my book. It also makes the interface at least a tiny bit more self documenting.

Other than that this patch looks like a nice improvement to me.

Show 14 quoted lines

> @@ -1566,8 +1586,11 @@ int expire_midx_packs(struct odb_source *source, unsigned flags)
>  	free(count);
>  
>  	if (packs_to_drop.nr)
> -		result = write_midx_internal(source, NULL,
> -					     &packs_to_drop, NULL, NULL, flags);
> +		result = write_midx_internal(&(struct write_midx_opts) {
> +					     .source = source,
> +					     .packs_to_drop = &packs_to_drop,
> +					     .flags = flags & MIDX_PROGRESS,
> +					     });
>  
>  	string_list_clear(&packs_to_drop, 0);
>

I think this syntax is not allowed in our codebase except for a test balloon just yet. See aso 9b2527caa4 (CodingGuidelines: document test balloons in flight, 2025-07-23):

    since late 2024 with v2.48.0-rc0~20, we have test balloons for
    compound literal syntax, e.g., (struct foo){ .member = value };
    our hope is that no platforms we care about have trouble using
    them, and officially adopt its wider use in mid 2026.  Do not add
    more use of the syntax until that happens.

Show 10 quoted lines

> @@ -1774,8 +1797,10 @@ int midx_repack(struct odb_source *source, size_t batch_size, unsigned flags)
>  		goto cleanup;
>  	}
>  
> -	result = write_midx_internal(source, NULL, NULL, NULL, NULL,
> -				     flags);
> +	result = write_midx_internal(&(struct write_midx_opts) {
> +				     .source = source,
> +				     .flags = flags,
> +				     });

Same here.

Patrick