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

From: Jeff King <peff@peff.net>
Date: Oct 15, 2025, 11:01 UTC
Message-ID: <20251015110155.GF2250228@coredump.intra.peff.net>
In-Reply-To: <11f78993f1d2d56b204398788780b30318d83afa.1759097191.git.me@ttaylorr.com>

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

Show 20 quoted lines

>  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