Re: [PATCH 17/49] builtin/repack.c: pass "packdir" when removing packs

From: Taylor Blau <me@ttaylorr.com>
Date: Oct 7, 2025, 20:23 UTC
Message-ID: <aOV2xNDIWGHG2sy/@nand.local>
In-Reply-To: <aNsUfrE-2Wc8_1Fm@pks.im>

On Tue, Sep 30, 2025 at 01:21:34AM +0200, Patrick Steinhardt wrote:

Show 14 quoted lines

> On Sun, Sep 28, 2025 at 06:08:16PM -0400, Taylor Blau wrote:
> > builtin/repack.c defines a static "packdir" to instruct pack-objects on
> > where to write any new packfiles. This is also the directory scanned
> > when removing any packfiles which were made redundant by the latest
> > repack.
> >
> > Prepare to move the "existing_packs_remove_redundant" function to its
> > own compilation unit by passing in this information as a parameter to
> > that function.
>
> I would have expected that we also get rid of that static variable
> either in this commit or a follow-up. Is there any reason why you
> skipped over this? If so, it would be great to highlight in the commit
> message.

We can't get rid of "packdir" quite yet, because it's used in other functions (which get a similar treatment as this one does in later patches).

We could (and probably should) drop this, packtmp_name, and packtmp as static variables within builtin/repack.c. We could do it as a fiftieth patch in this series like so:

--- 8< ---

diff --git a/builtin/repack.c b/builtin/repack.c
index ad60c4290d..565538bb24 100644
--- a/builtin/repack.c
+++ b/builtin/repack.c
@@ -27,7 +27,6 @@ static int pack_everything;
 static int write_bitmaps = -1;
 static int use_delta_islands;
 static int run_update_server_info = 1;
-static char *packdir, *packtmp_name, *packtmp;
 static int midx_must_contain_cruft = 1;

 static const char *const git_repack_usage[] = {
@@ -111,6 +110,7 @@ int cmd_repack(int argc,
 	struct write_pack_opts opts = { 0 };
 	int i, ret;
 	int show_progress;
+	char *packdir, *packtmp_name, *packtmp;

 	/* variables to be filled by option parsing */
 	struct repack_config_ctx config_ctx;
--- >8 ---

But I'd like to err on the side of post-merge followups here to avoid
dumping 49+ more patches on the list. Of course, if there are
showstoppers or other objections, I can easily reroll the series, but
I'd rather avoid doing so on a series this long.

Thanks,
Taylor