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

From: Jeff King <peff@peff.net>
Date: Oct 10, 2025, 06:04 UTC
Message-ID: <20251010060401.GF1965904@coredump.intra.peff.net>
In-Reply-To: <df75c3130bced8d5489dc9184701a0a544617049.1759097191.git.me@ttaylorr.com>

On Sun, Sep 28, 2025 at 06:08:16PM -0400, Taylor Blau wrote:

Show 8 quoted lines

> 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.

OK. I don't think there's anything wrong here, but it feels like we ought to be able to pull this from the existing "struct repository" context that's in the existing_packs struct.

I had to dig a little to find it, but I think that always comes from:

  packdir = mkpathdup("%s/pack", repo_get_object_directory(repo));

in cmd_repack(). So we'd have to recreate that string ourselves. It feels like object_database (or I guess odb_source) should have an easy way to get the pack directory, but it doesn't seem to.

So I dunno. I think it is mostly academic, as you are just moving around code here. But as an API, the separate "packdir" argument makes me wonder if I can pass anything I like to these functions. But really, it should match the packdir used to create the lists in the existing_packs struct. So it's kind of a brittle API. I'm not sure how much we should care for this series, as it is mostly about moving code around, and not providing some kind of flexible repack API.

-Peff