Re: [PATCH 44/49] builtin/repack.c: pass `write_pack_opts` to `finish_pack_objects_cmd()`

Taylor Blau
2025-10-15T21:35:06Z

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