From: Taylor Blau <me@ttaylorr.com>
Date: Wed, 15 Oct 2025 21:35:03 GMT
Subject: Re: [PATCH 44/49] builtin/repack.c: pass `write_pack_opts` to `finish_pack_objects_cmd()`
Message-ID: <aPAThyMlw4oNEBnJ@nand.local>
In-Reply-To: <20251015110155.GF2250228@coredump.intra.peff.net>

On Wed, Oct 15, 2025 at 07:01:55AM -0400, Jeff King wrote:
> On Sun, Sep 28, 2025 at 06:10:14PM -0400, Taylor Blau wrote:
>
> >  int cmd_repack(int argc,
> > @@ -294,6 +294,7 @@ int cmd_repack(int argc,
> >  	struct existing_packs existing = EXISTING_PACKS_INIT;
> >  	struct pack_geometry geometry = { 0 };
> >  	struct tempfile *refs_snapshot = NULL;
> > +	struct write_pack_opts opts = { 0 };
> >  	int i, ret;
> >  	int show_progress;
> >
> > @@ -560,7 +561,10 @@ int cmd_repack(int argc,
> >  		fclose(in);
> >  	}
> >
> > -	ret = finish_pack_objects_cmd(repo->hash_algo, &cmd, &names, 1);
> > +	opts.packdir = packdir;
> > +	opts.destination = packdir;
> > +	opts.packtmp = packtmp;
> > +	ret = finish_pack_objects_cmd(repo->hash_algo, &opts, &cmd, &names);
> >  	if (ret)
> >  		goto cleanup;
>
> This looks a bit funny: we are creating a write_pack_opts struct but
> only filling in some of the fields. Notably po_args is NULL. It's
> zero'd, which is good, but as a matter of API design, how can a caller
> know which fields are important for a given function and which are not?
>
> I guess I could have expected cmd_repack() to make a fully fleshed-out
> write_pack_opts up front, and then pass it to all functions which need
> it. Rather than making a custom one for this one function.
>
> But I guess that gets a little weird, because it really has at least
> _two_ such opts: one for the regular pack-objects invocation and one for
> the cruft pack. And really maybe one more, for the filtered pack? And
> the conditional blocks for cruft and filter_options packs shadow the
> "opts" variable. Yuck.

Yeah, it is unfortunate that not all instances where we generate a pack
use the same pack destination, or even the same set of options to
pack-objects.

> I'm not sure if there's a good solution. My inclination is to say that
> po_args should not be in "opts" at all, and that each function should
> take a separate po_args as appropriate. And then we could just have one
> write_pack_opts in cmd_repack(). But there is another snag: we sometimes
> override the "destination" field with "filter_to" or "expire_to".
>
> So the struct really isn't "here are the opts for repacking", but rather
> "let's marshal some arguments into a struct for one function call".
> Which of course is a bit verbose in C, hence these extra shadowed "opts"
> variables.

I think that only solves part of the problem, though. Among the other
spots that call functions which take a `struct write_pack_opts`, not all
of them use the same destination, so we'd still have to create multiple
`struct write_pack_opts` instances in `cmd_repack()`, or pass the
destination separately, which feels equally odd to me.

> So I dunno that I am really asking for any change. This might be the
> least bad way of writing it. I guess if we could avoid the shadowing
> like this:
>
> diff --git a/builtin/repack.c b/builtin/repack.c
> index ad60c4290d..4e4519b180 100644
> --- a/builtin/repack.c
> +++ b/builtin/repack.c
> @@ -108,7 +108,6 @@ int cmd_repack(int argc,
>  	struct existing_packs existing = EXISTING_PACKS_INIT;
>  	struct pack_geometry geometry = { 0 };
>  	struct tempfile *refs_snapshot = NULL;
> -	struct write_pack_opts opts = { 0 };
>  	int i, ret;
>  	int show_progress;
>
> @@ -372,12 +371,15 @@ int cmd_repack(int argc,
>  		fclose(in);
>  	}
>
> -	opts.packdir = packdir;
> -	opts.destination = packdir;
> -	opts.packtmp = packtmp;
> -	ret = finish_pack_objects_cmd(repo->hash_algo, &opts, &cmd, &names);
> -	if (ret)
> -		goto cleanup;
> +	{
> +		struct write_pack_opts opts = { 0 };
> +		opts.packdir = packdir;
> +		opts.destination = packdir;
> +		opts.packtmp = packtmp;
> +		ret = finish_pack_objects_cmd(repo->hash_algo, &opts, &cmd, &names);
> +		if (ret)
> +			goto cleanup;
> +	}
>
>  	if (!names.nr) {
>  		if (!po_args.quiet)

Yeah, I think that is the most reasonable outcome, at least within the
scope of this patch/series.

> That might be nice, but introducing the extra block is kind of ugly. Of
> course you can get really fancy with a compound literal, like:
>
> @@ -372,10 +371,13 @@ int cmd_repack(int argc,
>  		fclose(in);
>  	}
>
> -	opts.packdir = packdir;
> -	opts.destination = packdir;
> -	opts.packtmp = packtmp;
> -	ret = finish_pack_objects_cmd(repo->hash_algo, &opts, &cmd, &names);
> +	ret = finish_pack_objects_cmd(repo->hash_algo,
> +				      &(struct write_pack_opts) {
> +					.packdir = packdir,
> +					.destination = packdir,
> +					.packtmp = packtmp,
> +				      },
> +				      &cmd, &names);
>  	if (ret)
>  		goto cleanup;
>
>
> But that may be a bridge too far.

As much as I'd like to use a compound literal here, 9b2527caa4
(CodingGuidelines: document test balloons in flight, 2025-07-23)
prevents us from adding new instances of them in our codebase for the
time being.

Thanks,
Taylor