On Sun, Sep 28, 2025 at 06:10:14PM -0400, Taylor Blau wrote:

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