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

Jeff King
2025-10-15T11:01:57Z

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.

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.

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)

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.

-Peff